| Message ID | 20191014183849.14864-1-faiz_abbas@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] mmc: cqhci: commit descriptors before setting the doorbell | expand |
Hi, On 15/10/19 12:08 AM, Faiz Abbas wrote: > Add a write memory barrier to make sure that descriptors are actually > written to memory before ringing the doorbell. > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > --- > > This patch fixes a very infrequent ADMA error (1 out of 100 times) that > I have been seeing after enabling command queuing for J721e. > Also looking at memory-barriers.txt and this commit[1], > it looks like we should be doing this before any descriptor write > followed by a doorbell ring operation. It'll be nice if someone with more > expertise in memory barriers can comment. > > [1] ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the > doorbell") So I see that cqhci_readl/writel() use readl/writel_relaxed() which seems to be causing this issue. Should I just fix this by converting those to readl/writel with memory barriers instead? Thanks, Faiz
On 15/10/19 10:55 AM, Faiz Abbas wrote: > Hi, > > On 15/10/19 12:08 AM, Faiz Abbas wrote: >> Add a write memory barrier to make sure that descriptors are actually >> written to memory before ringing the doorbell. >> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >> --- >> >> This patch fixes a very infrequent ADMA error (1 out of 100 times) that >> I have been seeing after enabling command queuing for J721e. >> Also looking at memory-barriers.txt and this commit[1], >> it looks like we should be doing this before any descriptor write >> followed by a doorbell ring operation. It'll be nice if someone with more >> expertise in memory barriers can comment. >> >> [1] ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the >> doorbell") > > So I see that cqhci_readl/writel() use readl/writel_relaxed() which > seems to be causing this issue. Should I just fix this by converting > those to readl/writel with memory barriers instead? Perhaps we could do both changes i.e. add wmb() and convert to non-relaxed readl/writel
Adrian, On 15/10/19 7:15 PM, Adrian Hunter wrote: > On 15/10/19 10:55 AM, Faiz Abbas wrote: >> Hi, >> >> On 15/10/19 12:08 AM, Faiz Abbas wrote: >>> Add a write memory barrier to make sure that descriptors are actually >>> written to memory before ringing the doorbell. >>> >>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>> --- >>> >>> This patch fixes a very infrequent ADMA error (1 out of 100 times) that >>> I have been seeing after enabling command queuing for J721e. >>> Also looking at memory-barriers.txt and this commit[1], >>> it looks like we should be doing this before any descriptor write >>> followed by a doorbell ring operation. It'll be nice if someone with more >>> expertise in memory barriers can comment. >>> >>> [1] ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the >>> doorbell") >> >> So I see that cqhci_readl/writel() use readl/writel_relaxed() which >> seems to be causing this issue. Should I just fix this by converting >> those to readl/writel with memory barriers instead? > > Perhaps we could do both changes i.e. add wmb() and convert to non-relaxed > readl/writel > readl is implemented as readl_relaxed(); __rmb(); and writel is implemented as wmb(); writel_relaxed(); I think another wmb() before writel will be redundant. Maybe this patch is good enough in itself. Thanks, Faiz
Adrian, On 16/10/19 5:46 PM, Faiz Abbas wrote: > Adrian, > > On 15/10/19 7:15 PM, Adrian Hunter wrote: >> On 15/10/19 10:55 AM, Faiz Abbas wrote: >>> Hi, >>> >>> On 15/10/19 12:08 AM, Faiz Abbas wrote: >>>> Add a write memory barrier to make sure that descriptors are actually >>>> written to memory before ringing the doorbell. >>>> >>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>> --- >>>> >>>> This patch fixes a very infrequent ADMA error (1 out of 100 times) that >>>> I have been seeing after enabling command queuing for J721e. >>>> Also looking at memory-barriers.txt and this commit[1], >>>> it looks like we should be doing this before any descriptor write >>>> followed by a doorbell ring operation. It'll be nice if someone with more >>>> expertise in memory barriers can comment. >>>> >>>> [1] ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the >>>> doorbell") >>> >>> So I see that cqhci_readl/writel() use readl/writel_relaxed() which >>> seems to be causing this issue. Should I just fix this by converting >>> those to readl/writel with memory barriers instead? >> >> Perhaps we could do both changes i.e. add wmb() and convert to non-relaxed >> readl/writel >> > > readl is implemented as readl_relaxed(); __rmb(); > and > writel is implemented as wmb(); writel_relaxed(); > > I think another wmb() before writel will be redundant. > > Maybe this patch is good enough in itself. > Do you agree? Thanks, Faiz
On 18/10/19 10:22 AM, Faiz Abbas wrote: > Adrian, > > On 16/10/19 5:46 PM, Faiz Abbas wrote: >> Adrian, >> >> On 15/10/19 7:15 PM, Adrian Hunter wrote: >>> On 15/10/19 10:55 AM, Faiz Abbas wrote: >>>> Hi, >>>> >>>> On 15/10/19 12:08 AM, Faiz Abbas wrote: >>>>> Add a write memory barrier to make sure that descriptors are actually >>>>> written to memory before ringing the doorbell. >>>>> >>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>>> --- >>>>> >>>>> This patch fixes a very infrequent ADMA error (1 out of 100 times) that >>>>> I have been seeing after enabling command queuing for J721e. >>>>> Also looking at memory-barriers.txt and this commit[1], >>>>> it looks like we should be doing this before any descriptor write >>>>> followed by a doorbell ring operation. It'll be nice if someone with more >>>>> expertise in memory barriers can comment. >>>>> >>>>> [1] ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the >>>>> doorbell") >>>> >>>> So I see that cqhci_readl/writel() use readl/writel_relaxed() which >>>> seems to be causing this issue. Should I just fix this by converting >>>> those to readl/writel with memory barriers instead? >>> >>> Perhaps we could do both changes i.e. add wmb() and convert to non-relaxed >>> readl/writel >>> >> >> readl is implemented as readl_relaxed(); __rmb(); >> and >> writel is implemented as wmb(); writel_relaxed(); >> >> I think another wmb() before writel will be redundant. >> >> Maybe this patch is good enough in itself. >> > > Do you agree? Sure. Acked-by: Adrian Hunter <adrian.hunter@intel.com>
On Mon, 14 Oct 2019 at 20:37, Faiz Abbas <faiz_abbas@ti.com> wrote: > > Add a write memory barrier to make sure that descriptors are actually > written to memory before ringing the doorbell. > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> Applied for fixes and by adding a stable tag, thanks! BTW, do you have a valid commit that it fixes? Kind regards Uffe > --- > > This patch fixes a very infrequent ADMA error (1 out of 100 times) that > I have been seeing after enabling command queuing for J721e. > Also looking at memory-barriers.txt and this commit[1], > it looks like we should be doing this before any descriptor write > followed by a doorbell ring operation. It'll be nice if someone with more > expertise in memory barriers can comment. > > [1] ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the > doorbell") > > drivers/mmc/host/cqhci.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c > index f7bdae5354c3..5047f7343ffc 100644 > --- a/drivers/mmc/host/cqhci.c > +++ b/drivers/mmc/host/cqhci.c > @@ -611,7 +611,8 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq) > cq_host->slot[tag].flags = 0; > > cq_host->qcnt += 1; > - > + /* Make sure descriptors are ready before ringing the doorbell */ > + wmb(); > cqhci_writel(cq_host, 1 << tag, CQHCI_TDBR); > if (!(cqhci_readl(cq_host, CQHCI_TDBR) & (1 << tag))) > pr_debug("%s: cqhci: doorbell not set for tag %d\n", > -- > 2.19.2 >
Hi Uffe, On 18/10/19 4:28 PM, Ulf Hansson wrote: > On Mon, 14 Oct 2019 at 20:37, Faiz Abbas <faiz_abbas@ti.com> wrote: >> >> Add a write memory barrier to make sure that descriptors are actually >> written to memory before ringing the doorbell. >> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > > Applied for fixes and by adding a stable tag, thanks! > > BTW, do you have a valid commit that it fixes? > You can add: Fixes: a4080225f51d ("mmc: cqhci: support for command queue enabled host") Thanks, Faiz
diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c index f7bdae5354c3..5047f7343ffc 100644 --- a/drivers/mmc/host/cqhci.c +++ b/drivers/mmc/host/cqhci.c @@ -611,7 +611,8 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq) cq_host->slot[tag].flags = 0; cq_host->qcnt += 1; - + /* Make sure descriptors are ready before ringing the doorbell */ + wmb(); cqhci_writel(cq_host, 1 << tag, CQHCI_TDBR); if (!(cqhci_readl(cq_host, CQHCI_TDBR) & (1 << tag))) pr_debug("%s: cqhci: doorbell not set for tag %d\n",
Add a write memory barrier to make sure that descriptors are actually written to memory before ringing the doorbell. Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> --- This patch fixes a very infrequent ADMA error (1 out of 100 times) that I have been seeing after enabling command queuing for J721e. Also looking at memory-barriers.txt and this commit[1], it looks like we should be doing this before any descriptor write followed by a doorbell ring operation. It'll be nice if someone with more expertise in memory barriers can comment. [1] ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the doorbell") drivers/mmc/host/cqhci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)