From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932708AbcITJt2 (ORCPT ); Tue, 20 Sep 2016 05:49:28 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:36317 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932616AbcITJtZ (ORCPT ); Tue, 20 Sep 2016 05:49:25 -0400 X-AuditID: cbfee68e-f79cb6d000006cfe-39-57e106238162 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> <35afa945-1b94-f870-d52e-6ab7b1da4a67@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: Date: Tue, 20 Sep 2016 18:49:22 +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: <35afa945-1b94-f870-d52e-6ab7b1da4a67@rock-chips.com> Content-type: text/plain; charset=gbk Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrBIsWRmVeSWpSXmKPExsWyRsSkUFeZ7WG4wZUTfBZnlx1ks/j/6DWr xeVdc9gsjvzvZ7T49OA/s8WdJ+tZLY6vDXdg95jdcJHF4861PWwem5fUe/ydtZ/FY/u1ecwe nzfJBbBFcdmkpOZklqUW6dslcGXseLWMtWB6XMXFxbUNjJ/tuxg5OSQETCSONfxlhLDFJC7c W8/WxcjFISSwglHi4uOVbDBFK+avYoVIzGKU+H7pAjuE84BR4uHr/6wgVcICfhKb/nexgNgi AhoSN85ehxo1mUni679msA5mgUuMEo//3QerYhPQkdj+7TgTiM0rYCex4u0qMJtFQFVi+4q1 7CC2qECYxMlz59ghagQlfky+B9bLKeAkcff4cjCbGah+29c7ULa8xOY1b5lBlkkIXGIH+u4o I8RQAYlvkw8BFXEAJWQlNh1ghvhNUuLgihssExjFZiFZMQvJ2FlIxi5gZF7FKJpakFxQnJRe ZKRXnJhbXJqXrpecn7uJERiBp/8969vBePOA9SFGAQ5GJR5ei6QH4UKsiWXFlbmHGE2BrpjI LCWanA+M87ySeENjMyMLUxNTYyNzSzMlcd4EqZ/BQgLpiSWp2ampBalF8UWlOanFhxiZODil GhjX3UtsUcx61qu4XXjlBWUdtZOCWoZyUgXTd4r+794htFhNXrfAc/7UOU+OC85n/qt17bS0 kk6Lns2nuqBPB64aHEvuennmiHC3EW/Z13cmlxcvW6dfoO//QEpbst7hicvXSK6SSI5peWs+ FZlnl0x/cOX6ppy2JZMuPrqoEH16KvPrCYf8E7qUWIozEg21mIuKEwEwsoCeuwIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrKIsWRmVeSWpSXmKPExsVy+t9jQV1ltofhBjsOsFucXXaQzeL/o9es Fpd3zWGzOPK/n9Hi04P/zBZ3nqxntTi+NtyB3WN2w0UWjzvX9rB5bF5S7/F31n4Wj+3X5jF7 fN4kF8AW1cBok5GamJJapJCal5yfkpmXbqvkHRzvHG9qZmCoa2hpYa6kkJeYm2qr5OIToOuW mQN0ipJCWWJOKVAoILG4WEnfDtOE0BA3XQuYxghd35AguB4jAzSQsIYxY8erZawF0+MqLi6u bWD8bN/FyMkhIWAisWL+KlYIW0ziwr31bF2MXBxCArMYJb5fusAO4TxglHj4+j9YlbCAn8Sm /10sILaIgIbEjbPXoTomM0l8/dcM1sEscIlR4vG/+2BVbAI6Etu/HWcCsXkF7CRWvF0FZrMI qEpsX7GWHcQWFQiTOHnuHDtEjaDEj8n3wHo5BZwk7h5fDmYzA9Vv+3oHypaX2LzmLfMERqBD EVpmISmbhaRsASPzKkaJ1ILkguKk9FyjvNRyveLE3OLSvHS95PzcTYzgKH8mvYPx8C73Q4wC HIxKPLwBD++HC7EmlhVX5h5ilOBgVhLhXfz/QbgQb0piZVVqUX58UWlOavEhRlOgRyYyS4km 5wMTUF5JvKGxiZmRpZG5oYWRsbmSOO/j/+vChATSE0tSs1NTC1KLYPqYODilGhgX3LgkFamW WxPOxpke8Vfo0bO3KhErf3Ko8CR3/vpo/23/jYZQ5UN77gcI6P2KUrk+wX2d3scn/NO+OprZ V8tIZNUXyh2sT+o+GDH7SaWM6Zla3RaHtd7JM54GmRsVi8T+4Nt8eo2Ek0MU7/pTM3hnKegu Xif/4Eie1sYVkmtfuuxR2J2nsU+JpTgj0VCLuag4EQASvUMHCAMAAA== 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 Hi Shawn, On 09/20/2016 06:47 PM, Shawn Lin wrote: > Hi Jaehoon, > > Friendly ping... :) Thanks for reminding! :) I forgot your patch-set..sorry! Best Regards, Jaehoon Chung > > On 2016/9/2 12:14, 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. >> >> 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 = { >> > >