From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758382AbcHaG6K (ORCPT ); Wed, 31 Aug 2016 02:58:10 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:49700 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752363AbcHaG6H (ORCPT ); Wed, 31 Aug 2016 02:58:07 -0400 X-AuditID: cbfee68e-f79cb6d000006cfe-95-57c67ffcd365 Subject: Re: [PATCH 2/2] mmc: dw_mmc: avoid race condition of cpu and IDMAC To: Shawn Lin References: <1471599615-6249-1-git-send-email-shawn.lin@rock-chips.com> <1471599615-6249-2-git-send-email-shawn.lin@rock-chips.com> Cc: Ulf Hansson , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, Doug Anderson , Brian Norris , Heiko Stuebner , linux-rockchip@lists.infradead.org From: Jaehoon Chung Message-id: <30134bdf-bd25-9a7f-8888-a02b83dd178e@samsung.com> Date: Wed, 31 Aug 2016 15:58:04 +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: <1471599615-6249-2-git-send-email-shawn.lin@rock-chips.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrNIsWRmVeSWpSXmKPExsWyRsSkQPdP/bFwgw3vTSw2fXzPanF22UE2 i/+PXrNaXN41h83iyP9+RotPD/4zW9x5sp7V4vjacAcOj9kNF1k87lzbw+axeUm9x99Z+1k8 tl+bx+zxeZNcAFsUl01Kak5mWWqRvl0CV8a7na9YC9ZIVPxf8ZKxgXGrUBcjJ4eEgIlE84y/ LBC2mMSFe+vZuhi5OIQEVjBKTJu3nR2m6MP5h4wQiaWMEt/n7WaFcB4wSqx7+AisSljAW6Ll DMQoEQENiRtnr0ONuskosXXCNHYQh1ngL6PE0Y8PwDrYBHQktn87ztTFyMHBK2AnMe2aHUiY RUBVYk7rRjYQW1QgTOLkuXNg5bwCghI/Jt8DW8Ap4CEx78JjsFZmAT2J+xe1QMLMAvISm9e8 ZQZZJSHwkl3iUGMvM8RMAYlvkw+xgNRLCMhKbDrADPGZpMTBFTdYJjCKzUKyYRbC1FlIpi5g ZF7FKJpakFxQnJReZKRXnJhbXJqXrpecn7uJERiHp/8969vBePOA9SFGAQ5GJR7eA7OOhgux JpYVV+YeYjQFOmIis5Rocj4w2vNK4g2NzYwsTE1MjY3MLc2UxHkTpH4GCwmkJ5akZqemFqQW xReV5qQWH2Jk4uCUamCUmxd1MzcnYvnl7Qwrpx3em/J4dS5PveVfjldnH3o+nlqYseLfovDq pbHLb04q3pw4pVTkYrpQgOzrq9FO7CeazXa+0sxqKk1n+Vi1NrFfbheTgYAac9NlU8sysw9B Dz9NCrpU1u/+f/bM5xttiryrRVtPHZkjapcey1/351xl1dTzr/uuJ/cqsRRnJBpqMRcVJwIA XJvZL74CAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrBIsWRmVeSWpSXmKPExsVy+t9jAd0/9cfCDS5+0LPY9PE9q8XZZQfZ LP4/es1qcXnXHDaLI//7GS0+PfjPbHHnyXpWi+Nrwx04PGY3XGTxuHNtD5vH5iX1Hn9n7Wfx 2H5tHrPH501yAWxRDYw2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6hpYW5koKeYm5qbZK Lj4Bum6ZOUD3KCmUJeaUAoUCEouLlfTtME0IDXHTtYBpjND1DQmC6zEyQAMJaxgz3u18xVqw RqLi/4qXjA2MW4W6GDk5JARMJD6cf8gIYYtJXLi3nq2LkYtDSGApo8T3ebtZIZwHjBLrHj5i B6kSFvCWaDnzlwXEFhHQkLhx9jpUx01Gia0TprGDOMwCfxkljn58ANbBJqAjsf3bcaYuRg4O XgE7iWnX7EDCLAKqEnNaN7KB2KICYRInz50DK+cVEJT4Mfke2AJOAQ+JeRceg7UyC+hJ3L+o BRJmFpCX2LzmLfMERoFZSDpmIVTNQlK1gJF5FaNEakFyQXFSeq5hXmq5XnFibnFpXrpecn7u JkZwrD+T2sF4cJf7IUYBDkYlHt4HjMfChVgTy4orcw8xSnAwK4nwTq4GCvGmJFZWpRblxxeV 5qQWH2I0BXpjIrOUaHI+MA3llcQbGpuYGVkamRtaGBmbK4nzPv6/LkxIID2xJDU7NbUgtQim j4mDU6qBMbFhV9rHhXzfjS47PE1Y62b58lbIjBw/7Vexcit458o86Kt/2u4ebD5n+247sYfC iyZJSiqs23G38jOb+Gep9Rx7nvYcubwscc/MQzPve9X2HdqYkXljigVX4M038ttNF2+qtDr+ 1ORh8w2pyTcORHpNFLbbfifhh8tqVpY1Xe8vGiy64xu8zlCJpTgj0VCLuag4EQDsItdRCwMA AA== 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 08/19/2016 06:40 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. > > Signed-off-by: Shawn Lin > --- > > drivers/mmc/host/dw_mmc.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 0a5a49f..e640f83 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -473,6 +473,7 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host, > { > unsigned int desc_len; > struct idmac_desc_64addr *desc_first, *desc_last, *desc; > + unsigned long timeout = jiffies + msecs_to_jiffies(100); > int i; > > desc_first = desc_last = desc = host->sg_cpu; > @@ -489,6 +490,20 @@ 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. > + */ > + while (readl(&desc->des0) & IDMAC_DES0_OWN) { > + if (time_after(jiffies, timeout)) { > + dev_err(host->dev, "DESC is still owned by IDMAC.\n"); > + break; Doesn't it need the error handling? Just display the message? Best Regards, Jaehoon Chung > + } > + udelay(10); > + } > + > + /* > * Set the OWN bit and disable interrupts > * for this descriptor > */ > @@ -525,6 +540,7 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host, > { > unsigned int desc_len; > struct idmac_desc *desc_first, *desc_last, *desc; > + unsigned long timeout = jiffies + msecs_to_jiffies(100); > int i; > > desc_first = desc_last = desc = host->sg_cpu; > @@ -541,6 +557,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. > + */ > + while (readl(&desc->des0) & IDMAC_DES0_OWN) { > + if (time_after(jiffies, timeout)) { > + dev_err(host->dev, "DESC is still owned by IDMAC.\n"); > + break; > + } > + udelay(10); > + } > + > + /* > * Set the OWN bit and disable interrupts > * for this descriptor > */ >