UBUNTU: SAUCE: i2c:amd move out pointer in union i2c_event_base

BugLink: https://bugs.launchpad.net/bugs/1773940
"Unable to handle kernel paging request" occurs at this line of code:
         privdata->eventval.buf[i] = readdata;

The pointer "buf" in union "i2c_event_base" will change its value as
soon as any union member gets modified. We can observe this behaviour in
amd_mp2_irq_isr(). This makes buf points to the wrong place, and causes
the trouble we saw when it gets accessed.

Put pointer inside union is not a good idea, so refactor the structure
to make sure the pointer and the union do not overlap.

Also remove the NULL check for eventval.buf, as its value comes from
read_cfg.buf.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Acked-by: AceLan Kao <acelan.kao@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Acked-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.c b/drivers/i2c/busses/i2c-amd-pci-mp2.c
index 2266bf1568536a2ee6741af7039b25a30859c90a..8a3042b447d15d1b7e6619f68af03c1086979def 100644 (file)
--- a/drivers/i2c/busses/i2c-amd-pci-mp2.c
+++ b/drivers/i2c/busses/i2c-amd-pci-mp2.c
@@ -111,11 +111,6 @@ int amd_mp2_read(struct pci_dev *dev, struct i2c_read_config read_cfg)
 
        if (read_cfg.length <= 32) {
                i2c_cmd_base.s.mem_type = use_c2pmsg;
-               if (!privdata->eventval.buf) {
-                       dev_err(ndev_dev(privdata), "%s no mem for buf received\n",
-                               __func__);
-                       return -ENOMEM;
-               }
                privdata->eventval.buf = (u32 *)read_cfg.buf;
                dev_dbg(ndev_dev(privdata), "%s buf: %llx\n", __func__,
                        (u64)privdata->eventval.buf);
@@ -164,7 +159,8 @@ int amd_mp2_write(struct pci_dev *dev, struct i2c_write_config write_cfg)
 {
        struct amd_mp2_dev *privdata = pci_get_drvdata(dev);
        union i2c_cmd_base i2c_cmd_base;
-       int i = 0;
+       int i;
+       int buf_len;
 
        privdata->requested = true;
        dev_dbg(ndev_dev(privdata), "%s addr: %x id: %d\n", __func__,
@@ -198,7 +194,8 @@ int amd_mp2_write(struct pci_dev *dev, struct i2c_write_config write_cfg)
 
        if (write_cfg.length <= 32) {
                i2c_cmd_base.s.mem_type = use_c2pmsg;
-               for (i = 0; i < ((write_cfg.length + 3) / 4); i++) {
+               buf_len = (write_cfg.length + 3) / 4;
+               for (i = 0; i < buf_len; i++) {
                        writel(write_cfg.buf[i],
                               privdata->mmio + (AMD_C2P_MSG2 + i * 4));
                }
@@ -241,15 +238,17 @@ static void amd_mp2_pci_work(struct work_struct *work)
 {
        struct amd_mp2_dev *privdata = mp2_dev(work);
        u32 readdata = 0;
-       int i = 0;
-       int sts = privdata->eventval.r.status;
-       int res = privdata->eventval.r.response;
-       int len = privdata->eventval.r.length;
+       int i;
+       int buf_len;
+       int sts = privdata->eventval.base.r.status;
+       int res = privdata->eventval.base.r.response;
+       int len = privdata->eventval.base.r.length;
 
        if (res == command_success && sts == i2c_readcomplete_event) {
                if (privdata->ops->read_complete) {
                        if (len <= 32) {
-                               for (i = 0; i < ((len + 3) / 4); i++) {
+                               buf_len = (len + 3) / 4;
+                               for (i = 0; i < buf_len; i++) {
                                        readdata = readl(privdata->mmio +
                                                        (AMD_C2P_MSG2 + i * 4));
                                        privdata->eventval.buf[i] = readdata;
@@ -284,12 +283,12 @@ static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
        val = readl(privdata->mmio + AMD_P2C_MSG1);
        if (val != 0) {
                writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
-               privdata->eventval.ul = val;
+               privdata->eventval.base.ul = val;
        } else {
                val = readl(privdata->mmio + AMD_P2C_MSG2);
                if (val != 0) {
                        writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
-                       privdata->eventval.ul = val;
+                       privdata->eventval.base.ul = val;
                }
        }
 

diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.h b/drivers/i2c/busses/i2c-amd-pci-mp2.h
index a84389122885432c78cb03c7e1ae764acbaf9038..da77b9fe0ecb92ae90e9b1bc36f4d7a2ee9292cd 100644 (file)
--- a/drivers/i2c/busses/i2c-amd-pci-mp2.h
+++ b/drivers/i2c/busses/i2c-amd-pci-mp2.h
@@ -163,16 +163,18 @@ enum status_type {
        i2C_bus_notinitialized
 };
 
-union i2c_event_base {
-       u32 ul;
-       struct {
-               enum response_type response : 2; /*!< bit: 0..1 I2C res type */
-               enum status_type status : 5; /*!< bit: 2..6 status_type */
-               enum mem_type mem_type : 1; /*!< bit: 7 0-DRAM;1- C2PMsg o/p */
-               enum i2c_bus_index bus_id : 4; /*!< bit: 8..11 I2C Bus ID */
-               u32 length : 12; /*!< bit:16..29 length */
-               u32 slave_addr : 8; /*!< bit: 15 debug msg include in p2c msg */
-       } r; /*!< Structure used for bit access */
+struct i2c_event {
+       union {
+               u32 ul;
+               struct {
+                       enum response_type response : 2; /*!< bit: 0..1 I2C res type */
+                       enum status_type status : 5; /*!< bit: 2..6 status_type */
+                       enum mem_type mem_type : 1; /*!< bit: 7 0-DRAM;1- C2PMsg o/p */
+                       enum i2c_bus_index bus_id : 4; /*!< bit: 8..11 I2C Bus ID */
+                       u32 length : 12; /*!< bit:16..29 length */
+                       u32 slave_addr : 8; /*!< bit: 15 debug msg include in p2c msg */
+               } r; /*!< Structure used for bit access */
+       } base;
        u32 *buf;
 };
 
@@ -204,9 +206,9 @@ struct i2c_read_config {
 
 // struct to send/receive data b/w pci and i2c drivers
 struct amd_i2c_pci_ops {
-       int (*read_complete)(union i2c_event_base event, void *dev_ctx);
-       int (*write_complete)(union i2c_event_base event, void *dev_ctx);
-       int (*connect_complete)(union i2c_event_base event, void *dev_ctx);
+       int (*read_complete)(struct i2c_event event, void *dev_ctx);
+       int (*write_complete)(struct i2c_event event, void *dev_ctx);
+       int (*connect_complete)(struct i2c_event event, void *dev_ctx);
 };
 
 struct amd_i2c_common {
@@ -222,7 +224,7 @@ struct amd_mp2_dev {
        struct dentry *debugfs_dir;
        struct dentry *debugfs_info;
        void __iomem *mmio;
-       union i2c_event_base eventval;
+       struct i2c_event eventval;
        enum i2c_cmd reqcmd;
        struct i2c_connect_config connect_cfg;
        struct i2c_read_config read_cfg;

diff --git a/drivers/i2c/busses/i2c-amd-platdrv.c b/drivers/i2c/busses/i2c-amd-platdrv.c
index 5f195c98ca9e922ad5abfb1af49e0b9ce6e680e9..8366d159798088777877bf1161d5b2ac2346c738 100644 (file)
--- a/drivers/i2c/busses/i2c-amd-platdrv.c
+++ b/drivers/i2c/busses/i2c-amd-platdrv.c
@@ -71,34 +71,37 @@ struct amd_i2c_dev {
 
 };
 
-static int i2c_amd_read_completion(union i2c_event_base event, void *dev_ctx)
+static int i2c_amd_read_completion(struct i2c_event event, void *dev_ctx)
 {
        struct amd_i2c_dev *i2c_dev = (struct amd_i2c_dev *)dev_ctx;
        struct amd_i2c_common *commond = &i2c_dev->i2c_common;
-       int i = 0;
+       int i;
+       int buf_len;
 
-       if (event.r.status == i2c_readcomplete_event) {
-               if (event.r.length <= 32) {
+       if (event.base.r.status == i2c_readcomplete_event) {
+               if (event.base.r.length <= 32) {
                        pr_devel(" in %s i2c_dev->msg_buf :%p\n",
                                 __func__, i2c_dev->msg_buf);
 
                        memcpy(i2c_dev->msg_buf->buf,
-                              (unsigned char *)event.buf, event.r.length);
+                              (unsigned char *)event.buf, event.base.r.length);
 
-                       for (i = 0; i < ((event.r.length + 3) / 4); i++)
+                       buf_len = (event.base.r.length + 3) / 4;
+                       for (i = 0; i < buf_len; i++)
                                pr_devel("%s:%s readdata:%x\n",
                                         DRIVER_NAME, __func__, event.buf[i]);
 
                } else {
                        memcpy(i2c_dev->msg_buf->buf,
                               (unsigned char *)commond->read_cfg.buf,
-                               event.r.length);
+                               event.base.r.length);
                        pr_devel("%s:%s virt:%llx phy_addr:%llx\n",
                                 DRIVER_NAME, __func__,
                                (u64)commond->read_cfg.buf,
                                (u64)commond->read_cfg.phy_addr);
 
-                       for (i = 0; i < ((event.r.length + 3) / 4); i++)
+                       buf_len = (event.base.r.length + 3) / 4;
+                       for (i = 0; i < buf_len; i++)
                                pr_devel("%s:%s readdata:%x\n",
                                         DRIVER_NAME, __func__, ((unsigned int *)
                                commond->read_cfg.buf)[i]);
@@ -110,21 +113,21 @@ static int i2c_amd_read_completion(union i2c_event_base event, void *dev_ctx)
        return 0;
 }
 
-static int i2c_amd_write_completion(union i2c_event_base event, void *dev_ctx)
+static int i2c_amd_write_completion(struct i2c_event event, void *dev_ctx)
 {
        struct amd_i2c_dev *i2c_dev = (struct amd_i2c_dev *)dev_ctx;
 
-       if (event.r.status == i2c_writecomplete_event)
+       if (event.base.r.status == i2c_writecomplete_event)
                complete(&i2c_dev->msg_complete);
 
        return 0;
 }
 
-static int i2c_amd_connect_completion(union i2c_event_base event, void *dev_ctx)
+static int i2c_amd_connect_completion(struct i2c_event event, void *dev_ctx)
 {
        struct amd_i2c_dev *i2c_dev = (struct amd_i2c_dev *)dev_ctx;
 
-       if (event.r.status == i2c_busenable_complete)
+       if (event.base.r.status == i2c_busenable_complete)
                complete(&i2c_dev->msg_complete);
 
        return 0;
@@ -168,7 +171,7 @@ static int i2c_amd_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
        struct amd_i2c_dev *dev = i2c_get_adapdata(adap);
        struct amd_i2c_common *i2c_common = &dev->i2c_common;
 
-       int i = 0;
+       int i;
        unsigned long timeout;
        struct i2c_msg *pmsg;
        unsigned char *dma_buf = NULL;