Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Geert Uytterhoeven]

Hi Simon,

On Fri, Jun 16, 2017 at 12:03 PM, Simon Horman  wrote:
> On Fri, Jun 16, 2017 at 09:33:31AM +0200, Simon Horman wrote:
>> On Fri, Jun 16, 2017 at 09:23:15AM +0200, Geert Uytterhoeven wrote:
>> > Hi Simon,
>> >
>> > On Fri, Jun 16, 2017 at 9:13 AM, Simon Horman  wrote:
>> > > Sorry for letting this bug creep in to renesas-drivers.
>> > > Do you want a patch against your tree? I will plan to post v2 of
>> > > this patchset soon in any case.
>> >
>> > You can post such a patch to please users of renesas-drivers, but I won't
>> > apply it now. I assume the issue will be fixed when next renesas-drivers
>> > release is created ;-)
>>
>> That is my assumption too :)
>
> Revised patches posted as "[PATCH mmc/next v2 0/4] mmc: renesas_sdhi: add
> support for R-Car Gen3 SDHI DMAC"

Thanks!

> Pushed to topic/sdhi-gen3-dma-2017-v2 in my renesas tree.
> Do you need a fresh pull request?

Not needed, I already updated my list of topics to merge.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Simon Horman]

On Fri, Jun 16, 2017 at 09:33:31AM +0200, Simon Horman wrote:
> On Fri, Jun 16, 2017 at 09:23:15AM +0200, Geert Uytterhoeven wrote:
> > Hi Simon,
> > 
> > On Fri, Jun 16, 2017 at 9:13 AM, Simon Horman  wrote:
> > > Sorry for letting this bug creep in to renesas-drivers.
> > > Do you want a patch against your tree? I will plan to post v2 of
> > > this patchset soon in any case.
> > 
> > You can post such a patch to please users of renesas-drivers, but I won't
> > apply it now. I assume the issue will be fixed when next renesas-drivers
> > release is created ;-)
> 
> That is my assumption too :)

Revised patches posted as "[PATCH mmc/next v2 0/4] mmc: renesas_sdhi: add
support for R-Car Gen3 SDHI DMAC"

Pushed to topic/sdhi-gen3-dma-2017-v2 in my renesas tree.
Do you need a fresh pull request?

> I'll see about providing a fix patch too.

Fix is:

From: Simon Horman 
Subject: [PATCH] mmc: renesas-sdhi: implement complete to DMA op

Implement complete DMA op to resolve regression introduced by
the introduction of that DMA op.

This resolves a panic when initialising SDHI on non Gen3 platforms
which use DMA with SDHI.

This is planned to be squashed into
"mmc: tmio, renesas-sdhi: add complete to DMA ops" for
submission to upstream.

Fixes: f6bcd9f5e392 ("mmc: tmio: add complete to DMA ops")
Signed-off-by: Simon Horman 
---
 drivers/mmc/host/renesas_sdhi_sys_dmac.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c 
b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
index 90f8d11c4868..2897ee0b444a 100644
--- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
@@ -106,6 +106,11 @@ static void renesas_sdhi_sys_dmac_abort_dma(struct 
tmio_mmc_host *host)
renesas_sdhi_sys_dmac_enable_dma(host, true);
 }
 
+static void renesas_sdhi_sys_dmac_complete_dma(struct tmio_mmc_host *host)
+{
+   complete(>dma_dataend);
+}
+
 static void renesas_sdhi_sys_dmac_dma_callback(void *arg)
 {
struct tmio_mmc_host *host = arg;
@@ -427,6 +432,7 @@ static const struct tmio_mmc_dma_ops 
renesas_sdhi_sys_dmac_dma_ops = {
.request = renesas_sdhi_sys_dmac_request_dma,
.release = renesas_sdhi_sys_dmac_release_dma,
.abort = renesas_sdhi_sys_dmac_abort_dma,
+   .complete = renesas_sdhi_sys_dmac_complete_dma,
 };
 
 static int renesas_sdhi_sys_dmac_probe(struct platform_device *pdev)
-- 
2.1.4

Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Simon Horman]

On Fri, Jun 16, 2017 at 09:23:15AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Fri, Jun 16, 2017 at 9:13 AM, Simon Horman  wrote:
> > Sorry for letting this bug creep in to renesas-drivers.
> > Do you want a patch against your tree? I will plan to post v2 of
> > this patchset soon in any case.
> 
> You can post such a patch to please users of renesas-drivers, but I won't
> apply it now. I assume the issue will be fixed when next renesas-drivers
> release is created ;-)

That is my assumption too :)

I'll see about providing a fix patch too.

Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Geert Uytterhoeven]

Hi Simon,

On Fri, Jun 16, 2017 at 9:13 AM, Simon Horman  wrote:
> Sorry for letting this bug creep in to renesas-drivers.
> Do you want a patch against your tree? I will plan to post v2 of
> this patchset soon in any case.

You can post such a patch to please users of renesas-drivers, but I won't
apply it now. I assume the issue will be fixed when next renesas-drivers
release is created ;-)

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Simon Horman]

On Fri, Jun 16, 2017 at 09:09:31AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Fri, Jun 16, 2017 at 9:04 AM, Simon Horman  wrote:
> > On Fri, Jun 16, 2017 at 08:25:12AM +0200, Geert Uytterhoeven wrote:
> >> On Fri, Jun 16, 2017 at 8:06 AM, Simon Horman  wrote:
> >> > On Fri, Jun 09, 2017 at 09:35:20AM +0200, Geert Uytterhoeven wrote:
> >> >> On Thu, Jun 8, 2017 at 3:09 PM, Simon Horman 
> >> >>  wrote:
> >> >> > Add complete to DMA ops to allow DMAC implementation dependent
> >> >> > handling of DMA completion.
> >> >> >
> >> >> > Signed-off-by: Simon Horman 
> >> >> > ---
> >> >> >  drivers/mmc/host/tmio_mmc.h  |  1 +
> >> >> >  drivers/mmc/host/tmio_mmc_core.c | 10 --
> >> >> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> >> >> > index 9c94b6eb9b49..82725f9de359 100644
> >> >> > --- a/drivers/mmc/host/tmio_mmc.h
> >> >> > +++ b/drivers/mmc/host/tmio_mmc.h
> >> >> > @@ -121,6 +121,7 @@ struct tmio_mmc_dma_ops {
> >> >> > struct tmio_mmc_data *pdata);
> >> >> > void (*release)(struct tmio_mmc_host *host);
> >> >> > void (*abort)(struct tmio_mmc_host *host);
> >> >> > +   void (*complete)(struct tmio_mmc_host *host);
> >> >> >  };
> >> >> >
> >> >> >  struct tmio_mmc_host {
> >> >> > diff --git a/drivers/mmc/host/tmio_mmc_core.c 
> >> >> > b/drivers/mmc/host/tmio_mmc_core.c
> >> >> > index 0e76e099dc7f..26fe9bb64849 100644
> >> >> > --- a/drivers/mmc/host/tmio_mmc_core.c
> >> >> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> >> >> > @@ -86,6 +86,12 @@ static inline void tmio_mmc_abort_dma(struct 
> >> >> > tmio_mmc_host *host)
> >> >> > host->dma_ops->abort(host);
> >> >> >  }
> >> >> >
> >> >> > +static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
> >> >> > +{
> >> >> > +   if (host->dma_ops)
> >> >> > +   host->dma_ops->complete(host);
> >> >>
> >> >> I believe this will cause a crash and thus break bisection, as no driver
> >> >> fills in the complete pointer yet.
> >> >
> >> > Indeed. And it seems to cause a panic on non-Gen3 users of the driver.
> >> >
> >> > I think the fix should be:
> >> >
> >> > diff --git a/drivers/mmc/host/tmio_mmc_core.c 
> >> > b/drivers/mmc/host/tmio_mmc_core.c
> >> > index 26fe9bb64849..9ec2e933b6f0 100644
> >> > --- a/drivers/mmc/host/tmio_mmc_core.c
> >> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> >> > @@ -88,7 +88,7 @@ static inline void tmio_mmc_abort_dma(struct 
> >> > tmio_mmc_host *host)
> >> >
> >> >  static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
> >> >  {
> >> > -   if (host->dma_ops)
> >> > +   if (host->dma_ops && host->dma_ops->complete)
> >> > host->dma_ops->complete(host);
> >> >  }
> >>
> >> Is it? Now nothing will be completed?
> >>
> >> The old code did:
> >>
> >>  complete(>dma_dataend);
> >>
> >> so perhaps
> >>
> >> else
> >> complete(>dma_dataend);
> >>
> >> with the "else" branch to be deleted when all drivers that need it provide
> >> their own complete callback?
> >
> > Thanks, I see that is still a problem.
> >
> > As there is only one other driver and its closerly related
> > perhaps it could just implement the callback in this patch?
> 
> Yes, that sounds good to me.
> Else you need 3 patches:
>   1. Introduced .complete field,
>   2. Update driver to provide .complete handledr,
>   3. Update generic code.

Yes, and 3 patch seems a bit excessive in this case.

Sorry for letting this bug creep in to renesas-drivers.
Do you want a patch against your tree? I will plan to post v2 of
this patchset soon in any case.

Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Geert Uytterhoeven]

Hi Simon,

On Fri, Jun 16, 2017 at 9:04 AM, Simon Horman  wrote:
> On Fri, Jun 16, 2017 at 08:25:12AM +0200, Geert Uytterhoeven wrote:
>> On Fri, Jun 16, 2017 at 8:06 AM, Simon Horman  wrote:
>> > On Fri, Jun 09, 2017 at 09:35:20AM +0200, Geert Uytterhoeven wrote:
>> >> On Thu, Jun 8, 2017 at 3:09 PM, Simon Horman  
>> >> wrote:
>> >> > Add complete to DMA ops to allow DMAC implementation dependent
>> >> > handling of DMA completion.
>> >> >
>> >> > Signed-off-by: Simon Horman 
>> >> > ---
>> >> >  drivers/mmc/host/tmio_mmc.h  |  1 +
>> >> >  drivers/mmc/host/tmio_mmc_core.c | 10 --
>> >> >  2 files changed, 9 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
>> >> > index 9c94b6eb9b49..82725f9de359 100644
>> >> > --- a/drivers/mmc/host/tmio_mmc.h
>> >> > +++ b/drivers/mmc/host/tmio_mmc.h
>> >> > @@ -121,6 +121,7 @@ struct tmio_mmc_dma_ops {
>> >> > struct tmio_mmc_data *pdata);
>> >> > void (*release)(struct tmio_mmc_host *host);
>> >> > void (*abort)(struct tmio_mmc_host *host);
>> >> > +   void (*complete)(struct tmio_mmc_host *host);
>> >> >  };
>> >> >
>> >> >  struct tmio_mmc_host {
>> >> > diff --git a/drivers/mmc/host/tmio_mmc_core.c 
>> >> > b/drivers/mmc/host/tmio_mmc_core.c
>> >> > index 0e76e099dc7f..26fe9bb64849 100644
>> >> > --- a/drivers/mmc/host/tmio_mmc_core.c
>> >> > +++ b/drivers/mmc/host/tmio_mmc_core.c
>> >> > @@ -86,6 +86,12 @@ static inline void tmio_mmc_abort_dma(struct 
>> >> > tmio_mmc_host *host)
>> >> > host->dma_ops->abort(host);
>> >> >  }
>> >> >
>> >> > +static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
>> >> > +{
>> >> > +   if (host->dma_ops)
>> >> > +   host->dma_ops->complete(host);
>> >>
>> >> I believe this will cause a crash and thus break bisection, as no driver
>> >> fills in the complete pointer yet.
>> >
>> > Indeed. And it seems to cause a panic on non-Gen3 users of the driver.
>> >
>> > I think the fix should be:
>> >
>> > diff --git a/drivers/mmc/host/tmio_mmc_core.c 
>> > b/drivers/mmc/host/tmio_mmc_core.c
>> > index 26fe9bb64849..9ec2e933b6f0 100644
>> > --- a/drivers/mmc/host/tmio_mmc_core.c
>> > +++ b/drivers/mmc/host/tmio_mmc_core.c
>> > @@ -88,7 +88,7 @@ static inline void tmio_mmc_abort_dma(struct 
>> > tmio_mmc_host *host)
>> >
>> >  static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
>> >  {
>> > -   if (host->dma_ops)
>> > +   if (host->dma_ops && host->dma_ops->complete)
>> > host->dma_ops->complete(host);
>> >  }
>>
>> Is it? Now nothing will be completed?
>>
>> The old code did:
>>
>>  complete(>dma_dataend);
>>
>> so perhaps
>>
>> else
>> complete(>dma_dataend);
>>
>> with the "else" branch to be deleted when all drivers that need it provide
>> their own complete callback?
>
> Thanks, I see that is still a problem.
>
> As there is only one other driver and its closerly related
> perhaps it could just implement the callback in this patch?

Yes, that sounds good to me.
Else you need 3 patches:
  1. Introduced .complete field,
  2. Update driver to provide .complete handledr,
  3. Update generic code.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Simon Horman]

On Fri, Jun 16, 2017 at 08:25:12AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Fri, Jun 16, 2017 at 8:06 AM, Simon Horman  wrote:
> > On Fri, Jun 09, 2017 at 09:35:20AM +0200, Geert Uytterhoeven wrote:
> >> On Thu, Jun 8, 2017 at 3:09 PM, Simon Horman  
> >> wrote:
> >> > Add complete to DMA ops to allow DMAC implementation dependent
> >> > handling of DMA completion.
> >> >
> >> > Signed-off-by: Simon Horman 
> >> > ---
> >> >  drivers/mmc/host/tmio_mmc.h  |  1 +
> >> >  drivers/mmc/host/tmio_mmc_core.c | 10 --
> >> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> >> > index 9c94b6eb9b49..82725f9de359 100644
> >> > --- a/drivers/mmc/host/tmio_mmc.h
> >> > +++ b/drivers/mmc/host/tmio_mmc.h
> >> > @@ -121,6 +121,7 @@ struct tmio_mmc_dma_ops {
> >> > struct tmio_mmc_data *pdata);
> >> > void (*release)(struct tmio_mmc_host *host);
> >> > void (*abort)(struct tmio_mmc_host *host);
> >> > +   void (*complete)(struct tmio_mmc_host *host);
> >> >  };
> >> >
> >> >  struct tmio_mmc_host {
> >> > diff --git a/drivers/mmc/host/tmio_mmc_core.c 
> >> > b/drivers/mmc/host/tmio_mmc_core.c
> >> > index 0e76e099dc7f..26fe9bb64849 100644
> >> > --- a/drivers/mmc/host/tmio_mmc_core.c
> >> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> >> > @@ -86,6 +86,12 @@ static inline void tmio_mmc_abort_dma(struct 
> >> > tmio_mmc_host *host)
> >> > host->dma_ops->abort(host);
> >> >  }
> >> >
> >> > +static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
> >> > +{
> >> > +   if (host->dma_ops)
> >> > +   host->dma_ops->complete(host);
> >>
> >> I believe this will cause a crash and thus break bisection, as no driver
> >> fills in the complete pointer yet.
> >
> > Indeed. And it seems to cause a panic on non-Gen3 users of the driver.
> >
> > I think the fix should be:
> >
> > diff --git a/drivers/mmc/host/tmio_mmc_core.c 
> > b/drivers/mmc/host/tmio_mmc_core.c
> > index 26fe9bb64849..9ec2e933b6f0 100644
> > --- a/drivers/mmc/host/tmio_mmc_core.c
> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> > @@ -88,7 +88,7 @@ static inline void tmio_mmc_abort_dma(struct 
> > tmio_mmc_host *host)
> >
> >  static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
> >  {
> > -   if (host->dma_ops)
> > +   if (host->dma_ops && host->dma_ops->complete)
> > host->dma_ops->complete(host);
> >  }
> 
> Is it? Now nothing will be completed?
> 
> The old code did:
> 
>  complete(>dma_dataend);
> 
> so perhaps
> 
> else
> complete(>dma_dataend);
> 
> with the "else" branch to be deleted when all drivers that need it provide
> their own complete callback?

Thanks, I see that is still a problem.

As there is only one other driver and its closerly related
perhaps it could just implement the callback in this patch?

> >> > +}
> >> > +
> >> >  void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
> >> >  {
> >> > host->sdcard_irq_mask &= ~(i & TMIO_MASK_IRQ);
> >> > @@ -604,11 +610,11 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host 
> >> > *host, unsigned int stat)
> >> >
> >> > if (done) {
> >> > tmio_mmc_disable_mmc_irqs(host, 
> >> > TMIO_STAT_DATAEND);
> >> > -   complete(>dma_dataend);
> >> > +   tmio_mmc_complete_dma(host);
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Geert Uytterhoeven]

Hi Simon,

On Fri, Jun 16, 2017 at 8:06 AM, Simon Horman  wrote:
> On Fri, Jun 09, 2017 at 09:35:20AM +0200, Geert Uytterhoeven wrote:
>> On Thu, Jun 8, 2017 at 3:09 PM, Simon Horman  
>> wrote:
>> > Add complete to DMA ops to allow DMAC implementation dependent
>> > handling of DMA completion.
>> >
>> > Signed-off-by: Simon Horman 
>> > ---
>> >  drivers/mmc/host/tmio_mmc.h  |  1 +
>> >  drivers/mmc/host/tmio_mmc_core.c | 10 --
>> >  2 files changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
>> > index 9c94b6eb9b49..82725f9de359 100644
>> > --- a/drivers/mmc/host/tmio_mmc.h
>> > +++ b/drivers/mmc/host/tmio_mmc.h
>> > @@ -121,6 +121,7 @@ struct tmio_mmc_dma_ops {
>> > struct tmio_mmc_data *pdata);
>> > void (*release)(struct tmio_mmc_host *host);
>> > void (*abort)(struct tmio_mmc_host *host);
>> > +   void (*complete)(struct tmio_mmc_host *host);
>> >  };
>> >
>> >  struct tmio_mmc_host {
>> > diff --git a/drivers/mmc/host/tmio_mmc_core.c 
>> > b/drivers/mmc/host/tmio_mmc_core.c
>> > index 0e76e099dc7f..26fe9bb64849 100644
>> > --- a/drivers/mmc/host/tmio_mmc_core.c
>> > +++ b/drivers/mmc/host/tmio_mmc_core.c
>> > @@ -86,6 +86,12 @@ static inline void tmio_mmc_abort_dma(struct 
>> > tmio_mmc_host *host)
>> > host->dma_ops->abort(host);
>> >  }
>> >
>> > +static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
>> > +{
>> > +   if (host->dma_ops)
>> > +   host->dma_ops->complete(host);
>>
>> I believe this will cause a crash and thus break bisection, as no driver
>> fills in the complete pointer yet.
>
> Indeed. And it seems to cause a panic on non-Gen3 users of the driver.
>
> I think the fix should be:
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c 
> b/drivers/mmc/host/tmio_mmc_core.c
> index 26fe9bb64849..9ec2e933b6f0 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -88,7 +88,7 @@ static inline void tmio_mmc_abort_dma(struct tmio_mmc_host 
> *host)
>
>  static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
>  {
> -   if (host->dma_ops)
> +   if (host->dma_ops && host->dma_ops->complete)
> host->dma_ops->complete(host);
>  }

Is it? Now nothing will be completed?

The old code did:

 complete(>dma_dataend);

so perhaps

else
complete(>dma_dataend);

with the "else" branch to be deleted when all drivers that need it provide
their own complete callback?

>> > +}
>> > +
>> >  void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
>> >  {
>> > host->sdcard_irq_mask &= ~(i & TMIO_MASK_IRQ);
>> > @@ -604,11 +610,11 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host 
>> > *host, unsigned int stat)
>> >
>> > if (done) {
>> > tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND);
>> > -   complete(>dma_dataend);
>> > +   tmio_mmc_complete_dma(host);

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Simon Horman]

On Fri, Jun 09, 2017 at 09:35:20AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Thu, Jun 8, 2017 at 3:09 PM, Simon Horman  
> wrote:
> > Add complete to DMA ops to allow DMAC implementation dependent
> > handling of DMA completion.
> >
> > Signed-off-by: Simon Horman 
> > ---
> >  drivers/mmc/host/tmio_mmc.h  |  1 +
> >  drivers/mmc/host/tmio_mmc_core.c | 10 --
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> > index 9c94b6eb9b49..82725f9de359 100644
> > --- a/drivers/mmc/host/tmio_mmc.h
> > +++ b/drivers/mmc/host/tmio_mmc.h
> > @@ -121,6 +121,7 @@ struct tmio_mmc_dma_ops {
> > struct tmio_mmc_data *pdata);
> > void (*release)(struct tmio_mmc_host *host);
> > void (*abort)(struct tmio_mmc_host *host);
> > +   void (*complete)(struct tmio_mmc_host *host);
> >  };
> >
> >  struct tmio_mmc_host {
> > diff --git a/drivers/mmc/host/tmio_mmc_core.c 
> > b/drivers/mmc/host/tmio_mmc_core.c
> > index 0e76e099dc7f..26fe9bb64849 100644
> > --- a/drivers/mmc/host/tmio_mmc_core.c
> > +++ b/drivers/mmc/host/tmio_mmc_core.c
> > @@ -86,6 +86,12 @@ static inline void tmio_mmc_abort_dma(struct 
> > tmio_mmc_host *host)
> > host->dma_ops->abort(host);
> >  }
> >
> > +static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
> > +{
> > +   if (host->dma_ops)
> > +   host->dma_ops->complete(host);
> 
> I believe this will cause a crash and thus break bisection, as no driver
> fills in the complete pointer yet.

Indeed. And it seems to cause a panic on non-Gen3 users of the driver.

I think the fix should be:

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 26fe9bb64849..9ec2e933b6f0 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -88,7 +88,7 @@ static inline void tmio_mmc_abort_dma(struct tmio_mmc_host 
*host)
 
 static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
 {
-   if (host->dma_ops)
+   if (host->dma_ops && host->dma_ops->complete)
host->dma_ops->complete(host);
 }
 

> 
> > +}
> > +
> >  void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
> >  {
> > host->sdcard_irq_mask &= ~(i & TMIO_MASK_IRQ);
> > @@ -604,11 +610,11 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host 
> > *host, unsigned int stat)
> >
> > if (done) {
> > tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND);
> > -   complete(>dma_dataend);
> > +   tmio_mmc_complete_dma(host);
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

Re: [PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Geert Uytterhoeven]

Hi Simon,

On Thu, Jun 8, 2017 at 3:09 PM, Simon Horman  wrote:
> Add complete to DMA ops to allow DMAC implementation dependent
> handling of DMA completion.
>
> Signed-off-by: Simon Horman 
> ---
>  drivers/mmc/host/tmio_mmc.h  |  1 +
>  drivers/mmc/host/tmio_mmc_core.c | 10 --
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 9c94b6eb9b49..82725f9de359 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -121,6 +121,7 @@ struct tmio_mmc_dma_ops {
> struct tmio_mmc_data *pdata);
> void (*release)(struct tmio_mmc_host *host);
> void (*abort)(struct tmio_mmc_host *host);
> +   void (*complete)(struct tmio_mmc_host *host);
>  };
>
>  struct tmio_mmc_host {
> diff --git a/drivers/mmc/host/tmio_mmc_core.c 
> b/drivers/mmc/host/tmio_mmc_core.c
> index 0e76e099dc7f..26fe9bb64849 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -86,6 +86,12 @@ static inline void tmio_mmc_abort_dma(struct tmio_mmc_host 
> *host)
> host->dma_ops->abort(host);
>  }
>
> +static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
> +{
> +   if (host->dma_ops)
> +   host->dma_ops->complete(host);

I believe this will cause a crash and thus break bisection, as no driver
fills in the complete pointer yet.

> +}
> +
>  void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
>  {
> host->sdcard_irq_mask &= ~(i & TMIO_MASK_IRQ);
> @@ -604,11 +610,11 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host 
> *host, unsigned int stat)
>
> if (done) {
> tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND);
> -   complete(>dma_dataend);
> +   tmio_mmc_complete_dma(host);

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[PATCH/RFC 2/5] mmc: tmio: add complete to DMA ops

[Simon Horman]

Add complete to DMA ops to allow DMAC implementation dependent
handling of DMA completion.

Signed-off-by: Simon Horman 
---
 drivers/mmc/host/tmio_mmc.h  |  1 +
 drivers/mmc/host/tmio_mmc_core.c | 10 --
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 9c94b6eb9b49..82725f9de359 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -121,6 +121,7 @@ struct tmio_mmc_dma_ops {
struct tmio_mmc_data *pdata);
void (*release)(struct tmio_mmc_host *host);
void (*abort)(struct tmio_mmc_host *host);
+   void (*complete)(struct tmio_mmc_host *host);
 };
 
 struct tmio_mmc_host {
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 0e76e099dc7f..26fe9bb64849 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -86,6 +86,12 @@ static inline void tmio_mmc_abort_dma(struct tmio_mmc_host 
*host)
host->dma_ops->abort(host);
 }
 
+static inline void tmio_mmc_complete_dma(struct tmio_mmc_host *host)
+{
+   if (host->dma_ops)
+   host->dma_ops->complete(host);
+}
+
 void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
 {
host->sdcard_irq_mask &= ~(i & TMIO_MASK_IRQ);
@@ -604,11 +610,11 @@ static void tmio_mmc_data_irq(struct tmio_mmc_host *host, 
unsigned int stat)
 
if (done) {
tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND);
-   complete(>dma_dataend);
+   tmio_mmc_complete_dma(host);
}
} else if (host->chan_rx && (data->flags & MMC_DATA_READ) && 
!host->force_pio) {
tmio_mmc_disable_mmc_irqs(host, TMIO_STAT_DATAEND);
-   complete(>dma_dataend);
+   tmio_mmc_complete_dma(host);
} else {
tmio_mmc_do_data_irq(host);
tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_READOP | 
TMIO_MASK_WRITEOP);
-- 
2.1.4