[SRU,Binioc/Unstable,2/3] UBUNTU: SAUCE: i2c:amd move out pointer in union i2c_event_base

Message ID 20180623151914.30931-3-kai.heng.feng@canonical.com
State New
Headers show
Series
  • Enable AMD PCIe MP2 for AMDI0011
Related show

Commit Message

Kai-Heng Feng June 23, 2018, 3:19 p.m.
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>
---
 drivers/i2c/busses/i2c-amd-pci-mp2.c | 27 ++++++++++++-------------
 drivers/i2c/busses/i2c-amd-pci-mp2.h | 30 +++++++++++++++-------------
 drivers/i2c/busses/i2c-amd-platdrv.c | 29 +++++++++++++++------------
 3 files changed, 45 insertions(+), 41 deletions(-)

Patch

diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.c b/drivers/i2c/busses/i2c-amd-pci-mp2.c
index 2266bf156853..8a3042b447d1 100644
--- 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 a84389122885..da77b9fe0ecb 100644
--- 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 5f195c98ca9e..8366d1597980 100644
--- 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;