From patchwork Sat Jun 23 15:19:12 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kai-Heng Feng X-Patchwork-Id: 933720 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=canonical.com Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 41CfJZ1s3Tz9s01; Sun, 24 Jun 2018 01:20:30 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1fWkKu-0000X1-BV; Sat, 23 Jun 2018 15:20:24 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.86_2) (envelope-from ) id 1fWkKs-0000Ws-3Y for kernel-team@lists.ubuntu.com; Sat, 23 Jun 2018 15:20:22 +0000 Received: from [220.133.187.190] (helo=localhost) by youngberry.canonical.com with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.76) (envelope-from ) id 1fWkKq-00070H-LR for kernel-team@lists.ubuntu.com; Sat, 23 Jun 2018 15:20:21 +0000 From: Kai-Heng Feng To: kernel-team@lists.ubuntu.com Subject: [SRU] [Binioc/Unstable] [PATCH 2/3] UBUNTU: SAUCE: i2c:amd move out pointer in union i2c_event_base Date: Sat, 23 Jun 2018 23:19:12 +0800 Message-Id: <20180623151914.30931-3-kai.heng.feng@canonical.com> X-Mailer: git-send-email 2.17.0 In-Reply-To: <20180623151914.30931-1-kai.heng.feng@canonical.com> References: <20180623151914.30931-1-kai.heng.feng@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" 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 --- 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(-) 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;