From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753901AbcIVE01 (ORCPT ); Thu, 22 Sep 2016 00:26:27 -0400 Received: from mailout1.samsung.com ([203.254.224.24]:53739 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753399AbcIVE0Z (ORCPT ); Thu, 22 Sep 2016 00:26:25 -0400 X-AuditID: cbfee68e-f79fa6d0000063e2-31-57e35d6b9165 Subject: Re: [PATCH v2 2/4] mmc: dw_mmc: avoid race condition of cpu and IDMAC To: Shawn Lin References: <1472789679-15121-1-git-send-email-shawn.lin@rock-chips.com> <1472789679-15121-2-git-send-email-shawn.lin@rock-chips.com> Cc: Ulf Hansson , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, Doug Anderson , Heiko Stuebner , linux-rockchip@lists.infradead.org From: Jaehoon Chung Message-id: <7b36b86e-18d0-7f54-4679-eccb66632638@samsung.com> Date: Thu, 22 Sep 2016 13:26:19 +0900 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-version: 1.0 In-reply-to: <1472789679-15121-2-git-send-email-shawn.lin@rock-chips.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrOIsWRmVeSWpSXmKPExsWyRsSkUDc79nG4wbZ91hZnlx1ks/j/6DWr xeVdc9gsjvzvZ7T49OA/s8WdJ+tZLY6vDXdg95jdcJHF4861PWwem5fUe/ydtZ/FY/u1ecwe nzfJBbBFcdmkpOZklqUW6dslcGWs3f+BteCff8Wx9s0sDYwPbboYOTkkBEwkrq/9zwJhi0lc uLeerYuRi0NIYAWjxMOnk5hgij79uMkKkZjFKLGy+QULhPOAUaJx8kuwdmEBP4lN/7vAbBEB DYkbZ69DjbrNKLH09iawdmaBS4wSj//dB6tiE9CR2P7tONgOXgE7iUMrIQ5hEVCVmP/rClhc VCBM4uS5c+wQNYISPybfA6vhFPCU2NP7AqiGA2ionsT9i1ogYWYBeYnNa94yg+ySELjGLrGs +QUrxEwBiW+TD7GA1EsIyEpsOsAM8ZqkxMEVN1gmMIrNQrJhFsLUWUimLmBkXsUomlqQXFCc lF5kpFecmFtcmpeul5yfu4kRGH+n/z3r28F484D1IUYBDkYlHt4f4o/DhVgTy4orcw8xmgId MZFZSjQ5HxjleSXxhsZmRhamJqbGRuaWZkrivAlSP4OFBNITS1KzU1MLUovii0pzUosPMTJx cEo1MDaUvDj4y41h5iRri/4F7jd+P7dcxLHbbnmZbMYsvX27vcIjNIu2rXA/ojV7cmHs/46Z vkJ3jhv03u9f8nkDe7vNmTdZh9dudniTkyr4QurH+egppysis9a37CySu5rgvEtr0V3Pkqft YsWzW29nW3w1Y2s+LyPS/uhqqrC20+dF+QkTLlcdMFNiKc5INNRiLipOBAAXFykyugIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrCIsWRmVeSWpSXmKPExsVy+t9jQd3s2MfhBqu/mlicXXaQzeL/o9es Fpd3zWGzOPK/n9Hi04P/zBZ3nqxntTi+NtyB3WN2w0UWjzvX9rB5bF5S7/F31n4Wj+3X5jF7 fN4kF8AW1cBok5GamJJapJCal5yfkpmXbqvkHRzvHG9qZmCoa2hpYa6kkJeYm2qr5OIToOuW mQN0ipJCWWJOKVAoILG4WEnfDtOE0BA3XQuYxghd35AguB4jAzSQsIYxY+3+D6wF//wrjrVv ZmlgfGjTxcjJISFgIvHpx01WCFtM4sK99WxdjFwcQgKzGCVWNr9ggXAeMEo0Tn7JAlIlLOAn sel/F5gtIqAhcePsdaiO24wSS29vYgVxmAUuMUo8/ncfrIpNQEdi+7fjTCA2r4CdxKGV/8Hi LAKqEvN/XQGLiwqESZw8d44dokZQ4sfke2A1nAKeEnt6XwDVcAAN1ZO4f1ELJMwsIC+xec1b 5gmMQHcidMxCqJqFpGoBI/MqRonUguSC4qT0XMO81HK94sTc4tK8dL3k/NxNjOAYfya1g/Hg LvdDjAIcjEo8vBrfH4ULsSaWFVfmHmKU4GBWEuGdHP04XIg3JbGyKrUoP76oNCe1+BCjKdAb E5mlRJPzgeknryTe0NjEzMjSyNzQwsjYXEmc9/H/dWFCAumJJanZqakFqUUwfUwcnFINjOuz vu5qjr6XZR1/OPxb3NSkY1rzDmd1lEbxye2IV3D+z6z6KoH7gqtq3ap1QnfXiJoEv5y41NDk o/2j9AUhVtPFLl3TmrKP94rcgt+7Suc5PWj5lyXs5fd63c7QJ9y6Em4+zNv3iSi2GM7e5aDc 5eyQsUx3mch9nSXtURZsoXW65dHxK2LuKrEUZyQaajEXFScCAHZ1InsHAwAA DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/02/2016 01:14 PM, Shawn Lin wrote: > We could see an obvious race condition by test that > the former write operation by IDMAC aiming to clear > OWN bit reach right after the later configuration of > the same desc, which makes the IDMAC be in SUSPEND > state as the OWN bit was cleared by the asynchronous > write operation of IDMAC. The bug can be very easy > reproduced on RK3288 or similar when we reduce the > running rate of system buses and keep the CPU running > faster. So as two separate masters, IDMAC and cpu > write the same descriptor stored on the same address, > and this should be protected by adding check of OWN > bit before preparing new descriptors. Applied on my tree. Thanks! Best Regards, Jaehoon Chung > > Signed-off-by: Shawn Lin > > --- > > Changes in v2: > - fallback to PIO mode if failing to wait for OWN bit > - cleanup polluted desc chain as the own bit error could > occur on any place of the chain, so we need to clr the desc > configured before that one. Use the exist function to reinit > the desc chain. As this issue is really rare, so after applied, > the fio/iozone stress test didn't show up performance regression. > > drivers/mmc/host/dw_mmc.c | 208 ++++++++++++++++++++++++++++------------------ > 1 file changed, 129 insertions(+), 79 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 782b303..daa1c52 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -467,12 +467,87 @@ static void dw_mci_dmac_complete_dma(void *arg) > } > } > > -static inline void dw_mci_prepare_desc64(struct dw_mci *host, > +static int dw_mci_idmac_init(struct dw_mci *host) > +{ > + int i; > + > + if (host->dma_64bit_address == 1) { > + struct idmac_desc_64addr *p; > + /* Number of descriptors in the ring buffer */ > + host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc_64addr); > + > + /* Forward link the descriptor list */ > + for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; > + i++, p++) { > + p->des6 = (host->sg_dma + > + (sizeof(struct idmac_desc_64addr) * > + (i + 1))) & 0xffffffff; > + > + p->des7 = (u64)(host->sg_dma + > + (sizeof(struct idmac_desc_64addr) * > + (i + 1))) >> 32; > + /* Initialize reserved and buffer size fields to "0" */ > + p->des1 = 0; > + p->des2 = 0; > + p->des3 = 0; > + } > + > + /* Set the last descriptor as the end-of-ring descriptor */ > + p->des6 = host->sg_dma & 0xffffffff; > + p->des7 = (u64)host->sg_dma >> 32; > + p->des0 = IDMAC_DES0_ER; > + > + } else { > + struct idmac_desc *p; > + /* Number of descriptors in the ring buffer */ > + host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc); > + > + /* Forward link the descriptor list */ > + for (i = 0, p = host->sg_cpu; > + i < host->ring_size - 1; > + i++, p++) { > + p->des3 = cpu_to_le32(host->sg_dma + > + (sizeof(struct idmac_desc) * (i + 1))); > + p->des1 = 0; > + } > + > + /* Set the last descriptor as the end-of-ring descriptor */ > + p->des3 = cpu_to_le32(host->sg_dma); > + p->des0 = cpu_to_le32(IDMAC_DES0_ER); > + } > + > + dw_mci_idmac_reset(host); > + > + if (host->dma_64bit_address == 1) { > + /* Mask out interrupts - get Tx & Rx complete only */ > + mci_writel(host, IDSTS64, IDMAC_INT_CLR); > + mci_writel(host, IDINTEN64, SDMMC_IDMAC_INT_NI | > + SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI); > + > + /* Set the descriptor base address */ > + mci_writel(host, DBADDRL, host->sg_dma & 0xffffffff); > + mci_writel(host, DBADDRU, (u64)host->sg_dma >> 32); > + > + } else { > + /* Mask out interrupts - get Tx & Rx complete only */ > + mci_writel(host, IDSTS, IDMAC_INT_CLR); > + mci_writel(host, IDINTEN, SDMMC_IDMAC_INT_NI | > + SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI); > + > + /* Set the descriptor base address */ > + mci_writel(host, DBADDR, host->sg_dma); > + } > + > + return 0; > +} > + > +static inline int dw_mci_prepare_desc64(struct dw_mci *host, > struct mmc_data *data, > unsigned int sg_len) > { > unsigned int desc_len; > struct idmac_desc_64addr *desc_first, *desc_last, *desc; > + unsigned long timeout; > int i; > > desc_first = desc_last = desc = host->sg_cpu; > @@ -489,6 +564,19 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host, > length -= desc_len; > > /* > + * Wait for the former clear OWN bit operation > + * of IDMAC to make sure that this descriptor > + * isn't still owned by IDMAC as IDMAC's write > + * ops and CPU's read ops are asynchronous. > + */ > + timeout = jiffies + msecs_to_jiffies(100); > + while (readl(&desc->des0) & IDMAC_DES0_OWN) { > + if (time_after(jiffies, timeout)) > + goto err_own_bit; > + udelay(10); > + } > + > + /* > * Set the OWN bit and disable interrupts > * for this descriptor > */ > @@ -516,15 +604,24 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host, > /* Set last descriptor */ > desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); > desc_last->des0 |= IDMAC_DES0_LD; > + > + return 0; > +err_own_bit: > + /* restore the descriptor chain as it's polluted */ > + dev_dbg(host->dev, "desciptor is still owned by IDMAC.\n"); > + memset(host->sg_cpu, 0, PAGE_SIZE); > + dw_mci_idmac_init(host); > + return -EINVAL; > } > > > -static inline void dw_mci_prepare_desc32(struct dw_mci *host, > +static inline int dw_mci_prepare_desc32(struct dw_mci *host, > struct mmc_data *data, > unsigned int sg_len) > { > unsigned int desc_len; > struct idmac_desc *desc_first, *desc_last, *desc; > + unsigned long timeout; > int i; > > desc_first = desc_last = desc = host->sg_cpu; > @@ -541,6 +638,20 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host, > length -= desc_len; > > /* > + * Wait for the former clear OWN bit operation > + * of IDMAC to make sure that this descriptor > + * isn't still owned by IDMAC as IDMAC's write > + * ops and CPU's read ops are asynchronous. > + */ > + timeout = jiffies + msecs_to_jiffies(100); > + while (readl(&desc->des0) & > + cpu_to_le32(IDMAC_DES0_OWN)) { > + if (time_after(jiffies, timeout)) > + goto err_own_bit; > + udelay(10); > + } > + > + /* > * Set the OWN bit and disable interrupts > * for this descriptor > */ > @@ -569,16 +680,28 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host, > desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | > IDMAC_DES0_DIC)); > desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD); > + > + return 0; > +err_own_bit: > + /* restore the descriptor chain as it's polluted */ > + dev_dbg(host->dev, "desciptor is still owned by IDMAC.\n"); > + memset(host->sg_cpu, 0, PAGE_SIZE); > + dw_mci_idmac_init(host); > + return -EINVAL; > } > > static int dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len) > { > u32 temp; > + int ret; > > if (host->dma_64bit_address == 1) > - dw_mci_prepare_desc64(host, host->data, sg_len); > + ret = dw_mci_prepare_desc64(host, host->data, sg_len); > else > - dw_mci_prepare_desc32(host, host->data, sg_len); > + ret = dw_mci_prepare_desc32(host, host->data, sg_len); > + > + if (ret) > + goto out; > > /* drain writebuffer */ > wmb(); > @@ -603,81 +726,8 @@ static int dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len) > /* Start it running */ > mci_writel(host, PLDMND, 1); > > - return 0; > -} > - > -static int dw_mci_idmac_init(struct dw_mci *host) > -{ > - int i; > - > - if (host->dma_64bit_address == 1) { > - struct idmac_desc_64addr *p; > - /* Number of descriptors in the ring buffer */ > - host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc_64addr); > - > - /* Forward link the descriptor list */ > - for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; > - i++, p++) { > - p->des6 = (host->sg_dma + > - (sizeof(struct idmac_desc_64addr) * > - (i + 1))) & 0xffffffff; > - > - p->des7 = (u64)(host->sg_dma + > - (sizeof(struct idmac_desc_64addr) * > - (i + 1))) >> 32; > - /* Initialize reserved and buffer size fields to "0" */ > - p->des1 = 0; > - p->des2 = 0; > - p->des3 = 0; > - } > - > - /* Set the last descriptor as the end-of-ring descriptor */ > - p->des6 = host->sg_dma & 0xffffffff; > - p->des7 = (u64)host->sg_dma >> 32; > - p->des0 = IDMAC_DES0_ER; > - > - } else { > - struct idmac_desc *p; > - /* Number of descriptors in the ring buffer */ > - host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc); > - > - /* Forward link the descriptor list */ > - for (i = 0, p = host->sg_cpu; > - i < host->ring_size - 1; > - i++, p++) { > - p->des3 = cpu_to_le32(host->sg_dma + > - (sizeof(struct idmac_desc) * (i + 1))); > - p->des1 = 0; > - } > - > - /* Set the last descriptor as the end-of-ring descriptor */ > - p->des3 = cpu_to_le32(host->sg_dma); > - p->des0 = cpu_to_le32(IDMAC_DES0_ER); > - } > - > - dw_mci_idmac_reset(host); > - > - if (host->dma_64bit_address == 1) { > - /* Mask out interrupts - get Tx & Rx complete only */ > - mci_writel(host, IDSTS64, IDMAC_INT_CLR); > - mci_writel(host, IDINTEN64, SDMMC_IDMAC_INT_NI | > - SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI); > - > - /* Set the descriptor base address */ > - mci_writel(host, DBADDRL, host->sg_dma & 0xffffffff); > - mci_writel(host, DBADDRU, (u64)host->sg_dma >> 32); > - > - } else { > - /* Mask out interrupts - get Tx & Rx complete only */ > - mci_writel(host, IDSTS, IDMAC_INT_CLR); > - mci_writel(host, IDINTEN, SDMMC_IDMAC_INT_NI | > - SDMMC_IDMAC_INT_RI | SDMMC_IDMAC_INT_TI); > - > - /* Set the descriptor base address */ > - mci_writel(host, DBADDR, host->sg_dma); > - } > - > - return 0; > +out: > + return ret; > } > > static const struct dw_mci_dma_ops dw_mci_idmac_ops = { >