From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755203AbcHSIkA (ORCPT ); Fri, 19 Aug 2016 04:40:00 -0400 Received: from lucky1.263xmail.com ([211.157.147.132]:39781 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751599AbcHSIjH (ORCPT ); Fri, 19 Aug 2016 04:39:07 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: shawn.lin@rock-chips.com X-FST-TO: linux-rockchip@lists.infradead.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: shawn.lin@rock-chips.com X-UNIQUE-TAG: <2f5aca5de1278af082486992f3295682> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [RFC PATCH] mmc: dw_mmc: avoid race condition of cpu and IDMAC To: Jaehoon Chung , Shawn Lin References: <1471317827-6049-1-git-send-email-shawn.lin@rock-chips.com> <2562aa02-f83c-9d04-9cd9-c2de2a34d629@samsung.com> Cc: shawn.lin@rock-chips.com, Ulf Hansson , Heiko Stuebner , Brian Norris , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, Doug Anderson , linux-rockchip@lists.infradead.org From: Shawn Lin Message-ID: <5ffddf5f-b12e-8140-24a5-aa7138f5c288@rock-chips.com> Date: Fri, 19 Aug 2016 15:58:45 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <2562aa02-f83c-9d04-9cd9-c2de2a34d629@samsung.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 2016/8/17 17:52, Jaehoon Chung 写道: > Hi Shawn, > > On 08/16/2016 12:23 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 lowering the >> bandwidth of bus and aggravating the Qos to make the >> large numbers of IP fight for the priority. One possible >> replaceable solution may be alloc dual buff for the >> desc to avoid it but could still race each other >> theoretically. > > It makes sense. > But Sorry..i didn't understand what "when lowering the bandwidth of bus.." means.. I will amend it. What I actually mean is to reduce the running rate of system bus without touching the rate of CPU. This makes the CPU run more fater than other masters like IDMAC. > > According to TRM, we need to check whether OWN bit is set or not. > But it seems that it's related with controlling PLDMND register. > I need to check and read TRM in more detail. Per the TRM, "The IDMAC engine fetches the descriptor and checks the OWN bit. If the OWN bit is not set, it means that the host owns the descriptor. In this case the DMA enters suspend state and asserts the Descriptor Unable interrupt in the IDSTS register (IDSTS64 register, in case of 64-bit address configuration). In such a case, the host needs to release the IDMAC by writing any value to the poll demand register. " So in this case, it should generate a INT for IDSTS(64) and release the IDMAC by writing to PLDMND, then we retry it until IDMAC owns the descriptor. It's quite a verbose behaviour and we should do more a lot for the tasklet to overcome this pain... It's sane to just verify the own bit from memory when preparing desc, and it doesn't cost performance from the test as it is really few to meet this case. > >> >> Signed-off-by: Shawn Lin >> >> --- >> >> drivers/mmc/host/dw_mmc.c | 34 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 32380d5..7b01fab 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -490,6 +490,23 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, >> length -= desc_len; >> >> /* >> + * OWN bit should be clear by IDMAC after >> + * finishing transfer. Let's wait for the >> + * asynchronous operation of IDMAC and cpu >> + * to make sure that we do not rely on the >> + * order of Qos of bus and architecture. >> + * Otherwise we could see a race condition >> + * here that the former write operation of >> + * IDMAC(to clear the OWN bit) reach right >> + * after the later new configuration of desc >> + * which makes value of desc been covered >> + * leading to DMA_SUSPEND state as IDMAC fecth >> + * the wrong desc then. >> + */ >> + while ((readl(&desc->des0) & IDMAC_DES0_OWN)) >> + ; > > Well, I don't like this style..because it has the potential risk for infinite loop. > And Could you make the comment more simpler than now? yes, I will remove the RFC prefix and fix these. > >> + >> + /* >> * Set the OWN bit and disable interrupts >> * for this descriptor >> */ >> @@ -535,6 +552,23 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, >> length -= desc_len; >> >> /* >> + * OWN bit should be clear by IDMAC after >> + * finishing transfer. Let's wait for the >> + * asynchronous operation of IDMAC and cpu >> + * to make sure that we do not rely on the >> + * order of Qos of bus and architecture. >> + * Otherwise we could see a race condition >> + * here that the former write operation of >> + * IDMAC(to clear the OWN bit) reach right >> + * after the later new configuration of desc >> + * which makes value of desc been covered >> + * leading to DMA_SUSPEND state as IDMAC fecth >> + * the wrong desc then. >> + */ >> + while ((readl(&desc->des0) & IDMAC_DES0_OWN)) >> + ; > > Ditto. > >> + >> + /* >> * Set the OWN bit and disable interrupts >> * for this descriptor >> */ >> > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > -- Best Regards Shawn Lin From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shawn Lin Subject: Re: [RFC PATCH] mmc: dw_mmc: avoid race condition of cpu and IDMAC Date: Fri, 19 Aug 2016 15:58:45 +0800 Message-ID: <5ffddf5f-b12e-8140-24a5-aa7138f5c288@rock-chips.com> References: <1471317827-6049-1-git-send-email-shawn.lin@rock-chips.com> <2562aa02-f83c-9d04-9cd9-c2de2a34d629@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <2562aa02-f83c-9d04-9cd9-c2de2a34d629-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Jaehoon Chung Cc: Ulf Hansson , Heiko Stuebner , shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org, Brian Norris , linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Doug Anderson , linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: linux-mmc@vger.kernel.org 5ZyoIDIwMTYvOC8xNyAxNzo1MiwgSmFlaG9vbiBDaHVuZyDlhpnpgZM6Cj4gSGkgU2hhd24sCj4K PiBPbiAwOC8xNi8yMDE2IDEyOjIzIFBNLCBTaGF3biBMaW4gd3JvdGU6Cj4+IFdlIGNvdWxkIHNl ZSBhbiBvYnZpb3VzIHJhY2UgY29uZGl0aW9uIGJ5IHRlc3QgdGhhdAo+PiB0aGUgZm9ybWVyIHdy aXRlIG9wZXJhdGlvbiBieSBJRE1BQyBhaW1pbmcgdG8gY2xlYXIKPj4gT1dOIGJpdCByZWFjaCBy aWdodCBhZnRlciB0aGUgbGF0ZXIgY29uZmlndXJhdGlvbiBvZgo+PiB0aGUgc2FtZSBkZXNjLCB3 aGljaCBtYWtlcyB0aGUgSURNQUMgYmUgaW4gU1VTUEVORAo+PiBzdGF0ZSBhcyB0aGUgT1dOIGJp dCB3YXMgY2xlYXJlZCBieSB0aGUgYXN5bmNocm9ub3VzCj4+IHdyaXRlIG9wZXJhdGlvbiBvZiBJ RE1BQy4gVGhlIGJ1ZyBjYW4gYmUgdmVyeSBlYXN5Cj4+IHJlcHJvZHVjZWQgb24gUkszMjg4IG9y IHNpbWlsYXIgd2hlbiBsb3dlcmluZyB0aGUKPj4gYmFuZHdpZHRoIG9mIGJ1cyBhbmQgYWdncmF2 YXRpbmcgdGhlIFFvcyB0byBtYWtlIHRoZQo+PiBsYXJnZSBudW1iZXJzIG9mIElQIGZpZ2h0IGZv ciB0aGUgcHJpb3JpdHkuIE9uZSBwb3NzaWJsZQo+PiByZXBsYWNlYWJsZSBzb2x1dGlvbiBtYXkg YmUgYWxsb2MgZHVhbCBidWZmIGZvciB0aGUKPj4gZGVzYyB0byBhdm9pZCBpdCBidXQgY291bGQg c3RpbGwgcmFjZSBlYWNoIG90aGVyCj4+IHRoZW9yZXRpY2FsbHkuCj4KPiBJdCBtYWtlcyBzZW5z ZS4KPiBCdXQgU29ycnkuLmkgZGlkbid0IHVuZGVyc3RhbmQgd2hhdCAid2hlbiBsb3dlcmluZyB0 aGUgYmFuZHdpZHRoIG9mIGJ1cy4uIiBtZWFucy4uCgpJIHdpbGwgYW1lbmQgaXQuIFdoYXQgSSBh Y3R1YWxseSBtZWFuIGlzIHRvIHJlZHVjZSB0aGUgcnVubmluZyByYXRlIG9mCnN5c3RlbSBidXMg d2l0aG91dCB0b3VjaGluZyB0aGUgcmF0ZSBvZiBDUFUuIFRoaXMgbWFrZXMgdGhlIENQVSBydW4K bW9yZSBmYXRlciB0aGFuIG90aGVyIG1hc3RlcnMgbGlrZSBJRE1BQy4KCj4KPiBBY2NvcmRpbmcg dG8gVFJNLCB3ZSBuZWVkIHRvIGNoZWNrIHdoZXRoZXIgT1dOIGJpdCBpcyBzZXQgb3Igbm90Lgo+ IEJ1dCBpdCBzZWVtcyB0aGF0IGl0J3MgcmVsYXRlZCB3aXRoIGNvbnRyb2xsaW5nIFBMRE1ORCBy ZWdpc3Rlci4KPiBJIG5lZWQgdG8gY2hlY2sgYW5kIHJlYWQgVFJNIGluIG1vcmUgZGV0YWlsLgoK UGVyIHRoZSBUUk0sCgoiVGhlIElETUFDIGVuZ2luZSBmZXRjaGVzIHRoZSBkZXNjcmlwdG9yIGFu ZCBjaGVja3MgdGhlIE9XTiBiaXQuIElmIHRoZQpPV04gYml0IGlzIG5vdCBzZXQsIGl0IG1lYW5z IHRoYXQgdGhlIGhvc3Qgb3ducyB0aGUgZGVzY3JpcHRvci4gSW4gdGhpcwpjYXNlIHRoZSBETUEg ZW50ZXJzIHN1c3BlbmQgc3RhdGUgYW5kIGFzc2VydHMgdGhlIERlc2NyaXB0b3IgVW5hYmxlCmlu dGVycnVwdCBpbiB0aGUgSURTVFMgcmVnaXN0ZXIgKElEU1RTNjQgcmVnaXN0ZXIsIGluIGNhc2Ug b2YgNjQtYml0CmFkZHJlc3MgY29uZmlndXJhdGlvbikuIEluIHN1Y2ggYSBjYXNlLCB0aGUgaG9z dCBuZWVkcyB0byByZWxlYXNlIHRoZQpJRE1BQyBieSB3cml0aW5nIGFueSB2YWx1ZSB0byB0aGUg cG9sbCBkZW1hbmQgcmVnaXN0ZXIuICIKClNvIGluIHRoaXMgY2FzZSwgaXQgc2hvdWxkIGdlbmVy YXRlIGEgSU5UIGZvciBJRFNUUyg2NCkgYW5kIHJlbGVhc2UgdGhlCklETUFDIGJ5IHdyaXRpbmcg dG8gUExETU5ELCB0aGVuIHdlIHJldHJ5IGl0IHVudGlsIElETUFDIG93bnMgdGhlCmRlc2NyaXB0 b3IuIEl0J3MgcXVpdGUgYSB2ZXJib3NlIGJlaGF2aW91ciBhbmQgd2Ugc2hvdWxkIGRvIG1vcmUg YSBsb3QKZm9yIHRoZSB0YXNrbGV0IHRvIG92ZXJjb21lIHRoaXMgcGFpbi4uLgoKSXQncyBzYW5l IHRvIGp1c3QgdmVyaWZ5IHRoZSBvd24gYml0IGZyb20gbWVtb3J5IHdoZW4gcHJlcGFyaW5nIGRl c2MsCmFuZCBpdCBkb2Vzbid0IGNvc3QgcGVyZm9ybWFuY2UgZnJvbSB0aGUgdGVzdCBhcyBpdCBp cyByZWFsbHkgZmV3IHRvCm1lZXQgdGhpcyBjYXNlLgoKPgo+Pgo+PiBTaWduZWQtb2ZmLWJ5OiBT aGF3biBMaW4gPHNoYXduLmxpbkByb2NrLWNoaXBzLmNvbT4KPj4KPj4gLS0tCj4+Cj4+ICBkcml2 ZXJzL21tYy9ob3N0L2R3X21tYy5jIHwgMzQgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr KysrKwo+PiAgMSBmaWxlIGNoYW5nZWQsIDM0IGluc2VydGlvbnMoKykKPj4KPj4gZGlmZiAtLWdp dCBhL2RyaXZlcnMvbW1jL2hvc3QvZHdfbW1jLmMgYi9kcml2ZXJzL21tYy9ob3N0L2R3X21tYy5j Cj4+IGluZGV4IDMyMzgwZDUuLjdiMDFmYWIgMTAwNjQ0Cj4+IC0tLSBhL2RyaXZlcnMvbW1jL2hv c3QvZHdfbW1jLmMKPj4gKysrIGIvZHJpdmVycy9tbWMvaG9zdC9kd19tbWMuYwo+PiBAQCAtNDkw LDYgKzQ5MCwyMyBAQCBzdGF0aWMgdm9pZCBkd19tY2lfdHJhbnNsYXRlX3NnbGlzdChzdHJ1Y3Qg ZHdfbWNpICpob3N0LCBzdHJ1Y3QgbW1jX2RhdGEgKmRhdGEsCj4+ICAJCQkJbGVuZ3RoIC09IGRl c2NfbGVuOwo+Pgo+PiAgCQkJCS8qCj4+ICsJCQkJICogT1dOIGJpdCBzaG91bGQgYmUgY2xlYXIg YnkgSURNQUMgYWZ0ZXIKPj4gKwkJCQkgKiBmaW5pc2hpbmcgdHJhbnNmZXIuIExldCdzIHdhaXQg Zm9yIHRoZQo+PiArCQkJCSAqIGFzeW5jaHJvbm91cyBvcGVyYXRpb24gb2YgSURNQUMgYW5kIGNw dQo+PiArCQkJCSAqIHRvIG1ha2Ugc3VyZSB0aGF0IHdlIGRvIG5vdCByZWx5IG9uIHRoZQo+PiAr CQkJCSAqIG9yZGVyIG9mIFFvcyBvZiBidXMgYW5kIGFyY2hpdGVjdHVyZS4KPj4gKwkJCQkgKiBP dGhlcndpc2Ugd2UgY291bGQgc2VlIGEgcmFjZSBjb25kaXRpb24KPj4gKwkJCQkgKiBoZXJlIHRo YXQgdGhlIGZvcm1lciB3cml0ZSBvcGVyYXRpb24gb2YKPj4gKwkJCQkgKiBJRE1BQyh0byBjbGVh ciB0aGUgT1dOIGJpdCkgcmVhY2ggcmlnaHQKPj4gKwkJCQkgKiBhZnRlciB0aGUgbGF0ZXIgbmV3 IGNvbmZpZ3VyYXRpb24gb2YgZGVzYwo+PiArCQkJCSAqIHdoaWNoIG1ha2VzIHZhbHVlIG9mIGRl c2MgYmVlbiBjb3ZlcmVkCj4+ICsJCQkJICogbGVhZGluZyB0byBETUFfU1VTUEVORCBzdGF0ZSBh cyBJRE1BQyBmZWN0aAo+PiArCQkJCSAqIHRoZSB3cm9uZyBkZXNjIHRoZW4uCj4+ICsJCQkJICov Cj4+ICsJCQkJd2hpbGUgKChyZWFkbCgmZGVzYy0+ZGVzMCkgJiBJRE1BQ19ERVMwX09XTikpCj4+ ICsJCQkJCTsKPgo+IFdlbGwsIEkgZG9uJ3QgbGlrZSB0aGlzIHN0eWxlLi5iZWNhdXNlIGl0IGhh cyB0aGUgcG90ZW50aWFsIHJpc2sgZm9yIGluZmluaXRlIGxvb3AuCj4gQW5kIENvdWxkIHlvdSBt YWtlIHRoZSBjb21tZW50IG1vcmUgc2ltcGxlciB0aGFuIG5vdz8KCnllcywgSSB3aWxsIHJlbW92 ZSB0aGUgUkZDIHByZWZpeCBhbmQgZml4IHRoZXNlLgoKCj4KPj4gKwo+PiArCQkJCS8qCj4+ICAJ CQkJICogU2V0IHRoZSBPV04gYml0IGFuZCBkaXNhYmxlIGludGVycnVwdHMKPj4gIAkJCQkgKiBm b3IgdGhpcyBkZXNjcmlwdG9yCj4+ICAJCQkJICovCj4+IEBAIC01MzUsNiArNTUyLDIzIEBAIHN0 YXRpYyB2b2lkIGR3X21jaV90cmFuc2xhdGVfc2dsaXN0KHN0cnVjdCBkd19tY2kgKmhvc3QsIHN0 cnVjdCBtbWNfZGF0YSAqZGF0YSwKPj4gIAkJCQlsZW5ndGggLT0gZGVzY19sZW47Cj4+Cj4+ICAJ CQkJLyoKPj4gKwkJCQkgKiBPV04gYml0IHNob3VsZCBiZSBjbGVhciBieSBJRE1BQyBhZnRlcgo+ PiArCQkJCSAqIGZpbmlzaGluZyB0cmFuc2Zlci4gTGV0J3Mgd2FpdCBmb3IgdGhlCj4+ICsJCQkJ ICogYXN5bmNocm9ub3VzIG9wZXJhdGlvbiBvZiBJRE1BQyBhbmQgY3B1Cj4+ICsJCQkJICogdG8g bWFrZSBzdXJlIHRoYXQgd2UgZG8gbm90IHJlbHkgb24gdGhlCj4+ICsJCQkJICogb3JkZXIgb2Yg UW9zIG9mIGJ1cyBhbmQgYXJjaGl0ZWN0dXJlLgo+PiArCQkJCSAqIE90aGVyd2lzZSB3ZSBjb3Vs ZCBzZWUgYSByYWNlIGNvbmRpdGlvbgo+PiArCQkJCSAqIGhlcmUgdGhhdCB0aGUgZm9ybWVyIHdy aXRlIG9wZXJhdGlvbiBvZgo+PiArCQkJCSAqIElETUFDKHRvIGNsZWFyIHRoZSBPV04gYml0KSBy ZWFjaCByaWdodAo+PiArCQkJCSAqIGFmdGVyIHRoZSBsYXRlciBuZXcgY29uZmlndXJhdGlvbiBv ZiBkZXNjCj4+ICsJCQkJICogd2hpY2ggbWFrZXMgdmFsdWUgb2YgZGVzYyBiZWVuIGNvdmVyZWQK Pj4gKwkJCQkgKiBsZWFkaW5nIHRvIERNQV9TVVNQRU5EIHN0YXRlIGFzIElETUFDIGZlY3RoCj4+ ICsJCQkJICogdGhlIHdyb25nIGRlc2MgdGhlbi4KPj4gKwkJCQkgKi8KPj4gKwkJCQl3aGlsZSAo KHJlYWRsKCZkZXNjLT5kZXMwKSAmIElETUFDX0RFUzBfT1dOKSkKPj4gKwkJCQkJOwo+Cj4gRGl0 dG8uCj4KPj4gKwo+PiArCQkJCS8qCj4+ICAJCQkJICogU2V0IHRoZSBPV04gYml0IGFuZCBkaXNh YmxlIGludGVycnVwdHMKPj4gIAkJCQkgKiBmb3IgdGhpcyBkZXNjcmlwdG9yCj4+ICAJCQkJICov Cj4+Cj4KPgo+IF9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f Cj4gTGludXgtcm9ja2NoaXAgbWFpbGluZyBsaXN0Cj4gTGludXgtcm9ja2NoaXBAbGlzdHMuaW5m cmFkZWFkLm9yZwo+IGh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8v bGludXgtcm9ja2NoaXAKPgoKCi0tIApCZXN0IFJlZ2FyZHMKU2hhd24gTGluCgoKX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KTGludXgtcm9ja2NoaXAgbWFp bGluZyBsaXN0CkxpbnV4LXJvY2tjaGlwQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3Rz LmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1yb2NrY2hpcAo=