mmc: dw_mmc: fix falling from idmac to PIO mode when dw_mci_reset occurs

Message ID 20180314193051.16791-1-Evgeniy.Didin@synopsys.com
State New
Headers show
Series
  • mmc: dw_mmc: fix falling from idmac to PIO mode when dw_mci_reset occurs
Related show

Commit Message

Evgeniy Didin March 14, 2018, 7:30 p.m.
It was found that in IDMAC mode after soft-reset driver switches 
to PIO mode.

That's what happens in case of DTO timeout overflow calculation failure:
1. soft-reset is called
2. driver restarts dma
3. descriptors states are checked, one of descriptor is owned by the IDMAC.
4. driver can't use DMA and then switches to PIO mode.

Failure was already fixed in: 
https://www.spinics.net/lists/linux-mmc/msg48125.html.

Behaviour while soft-reset is not something we except or 
even want to happen. So we switch from dw_mci_idmac_reset
to dw_mci_idmac_init, so descriptors are cleaned before starting dma. 

And while at it explicitly zero des0 which otherwise might
contain garbage as being allocated by dmam_alloc_coherent().

Signed-off-by: Evgeniy Didin <Evgeniy.Didin@synopsys.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Cc: Shawn Lin <shawn.lin@rock-chips.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
Cc: linux-snps-arc@lists.infradead.org
---
 drivers/mmc/host/dw_mmc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Ulf Hansson March 15, 2018, 9:57 a.m. | #1
On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote:
> It was found that in IDMAC mode after soft-reset driver switches
> to PIO mode.
>
> That's what happens in case of DTO timeout overflow calculation failure:
> 1. soft-reset is called
> 2. driver restarts dma
> 3. descriptors states are checked, one of descriptor is owned by the IDMAC.
> 4. driver can't use DMA and then switches to PIO mode.
>
> Failure was already fixed in:
> https://www.spinics.net/lists/linux-mmc/msg48125.html.

Evgeniy,

It seems like I should squash this fix into the above commit? Makes sense?

Kind regards
Uffe

>
> Behaviour while soft-reset is not something we except or
> even want to happen. So we switch from dw_mci_idmac_reset
> to dw_mci_idmac_init, so descriptors are cleaned before starting dma.
>
> And while at it explicitly zero des0 which otherwise might
> contain garbage as being allocated by dmam_alloc_coherent().
>
> Signed-off-by: Evgeniy Didin <Evgeniy.Didin@synopsys.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> Cc: Shawn Lin <shawn.lin@rock-chips.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> Cc: linux-snps-arc@lists.infradead.org
> ---
>  drivers/mmc/host/dw_mmc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 0aa39975f33b..26f2ac107702 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -558,6 +558,7 @@ static int dw_mci_idmac_init(struct dw_mci *host)
>                                         (sizeof(struct idmac_desc_64addr) *
>                                                         (i + 1))) >> 32;
>                         /* Initialize reserved and buffer size fields to "0" */
> +                       p->des0 = 0;
>                         p->des1 = 0;
>                         p->des2 = 0;
>                         p->des3 = 0;
> @@ -580,6 +581,7 @@ static int dw_mci_idmac_init(struct dw_mci *host)
>                      i++, p++) {
>                         p->des3 = cpu_to_le32(host->sg_dma +
>                                         (sizeof(struct idmac_desc) * (i + 1)));
> +                       p->des0 = 0;
>                         p->des1 = 0;
>                 }
>
> @@ -1795,8 +1797,8 @@ static bool dw_mci_reset(struct dw_mci *host)
>         }
>
>         if (host->use_dma == TRANS_MODE_IDMAC)
> -               /* It is also recommended that we reset and reprogram idmac */
> -               dw_mci_idmac_reset(host);
> +               /* It is also required that we reinit idmac */
> +               dw_mci_idmac_init(host);
>
>         ret = true;
>
> --
> 2.11.0
>
Evgeniy Didin March 15, 2018, 1:18 p.m. | #2
Hello Ulf,

On Thu, 2018-03-15 at 10:57 +0100, Ulf Hansson wrote:
> On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote:

> > It was found that in IDMAC mode after soft-reset driver switches

> > to PIO mode.

> > 

> > That's what happens in case of DTO timeout overflow calculation failure:

> > 1. soft-reset is called

> > 2. driver restarts dma

> > 3. descriptors states are checked, one of descriptor is owned by the IDMAC.

> > 4. driver can't use DMA and then switches to PIO mode.

> > 

> > Failure was already fixed in:

> > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dmmc_msg48125.html&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=vQk-RIbjwN0zvlwiMSpq3LYUTNf7Gqc4ujhosYI

> > TtAw&m=6rPWpKUYQD3kY-2OEikJUWyEJvwKJljWHFC8rd2TCak&s=JjX0Dx8-eSyW2cATMsnG1eAzrKgoDkS1bcS5XYrVtlE&e=.

> 

> Evgeniy,

> 

> It seems like I should squash this fix into the above commit? Makes sense?

IMHO it is not a good idea, because this fix is orthogonal to the above commit.
Debugging drto overflow issue we found this strange behavior.
This might be general issue of soft-reset. So I think this should be separate patch.
Also if these patches are independent, it will be easier to backport them.

Best regards,
Evgeniy Didin

> Kind regards

> Uffe

> 

> > 

> > Behaviour while soft-reset is not something we except or

> > even want to happen. So we switch from dw_mci_idmac_reset

> > to dw_mci_idmac_init, so descriptors are cleaned before starting dma.

> > 

> > And while at it explicitly zero des0 which otherwise might

> > contain garbage as being allocated by dmam_alloc_coherent().

> > 

> > Signed-off-by: Evgeniy Didin <Evgeniy.Didin@synopsys.com>

> > Cc: Jaehoon Chung <jh80.chung@samsung.com>

> > Cc: Ulf Hansson <ulf.hansson@linaro.org>

> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>

> > Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com>

> > Cc: Shawn Lin <shawn.lin@rock-chips.com>

> > Cc: Alexey Brodkin <abrodkin@synopsys.com>

> > Cc: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>

> > Cc: linux-snps-arc@lists.infradead.org

> > ---

> >  drivers/mmc/host/dw_mmc.c | 6 ++++--

> >  1 file changed, 4 insertions(+), 2 deletions(-)

> > 

> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c

> > index 0aa39975f33b..26f2ac107702 100644

> > --- a/drivers/mmc/host/dw_mmc.c

> > +++ b/drivers/mmc/host/dw_mmc.c

> > @@ -558,6 +558,7 @@ static int dw_mci_idmac_init(struct dw_mci *host)

> >                                         (sizeof(struct idmac_desc_64addr) *

> >                                                         (i + 1))) >> 32;

> >                         /* Initialize reserved and buffer size fields to "0" */

> > +                       p->des0 = 0;

> >                         p->des1 = 0;

> >                         p->des2 = 0;

> >                         p->des3 = 0;

> > @@ -580,6 +581,7 @@ static int dw_mci_idmac_init(struct dw_mci *host)

> >                      i++, p++) {

> >                         p->des3 = cpu_to_le32(host->sg_dma +

> >                                         (sizeof(struct idmac_desc) * (i + 1)));

> > +                       p->des0 = 0;

> >                         p->des1 = 0;

> >                 }

> > 

> > @@ -1795,8 +1797,8 @@ static bool dw_mci_reset(struct dw_mci *host)

> >         }

> > 

> >         if (host->use_dma == TRANS_MODE_IDMAC)

> > -               /* It is also recommended that we reset and reprogram idmac */

> > -               dw_mci_idmac_reset(host);

> > +               /* It is also required that we reinit idmac */

> > +               dw_mci_idmac_init(host);

> > 

> >         ret = true;

> > 

> > --

> > 2.11.0

> >
Ulf Hansson March 15, 2018, 1:39 p.m. | #3
On 15 March 2018 at 14:18, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote:
> Hello Ulf,
>
> On Thu, 2018-03-15 at 10:57 +0100, Ulf Hansson wrote:
>> On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote:
>> > It was found that in IDMAC mode after soft-reset driver switches
>> > to PIO mode.
>> >
>> > That's what happens in case of DTO timeout overflow calculation failure:
>> > 1. soft-reset is called
>> > 2. driver restarts dma
>> > 3. descriptors states are checked, one of descriptor is owned by the IDMAC.
>> > 4. driver can't use DMA and then switches to PIO mode.
>> >
>> > Failure was already fixed in:
>> > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dmmc_msg48125.html&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=vQk-RIbjwN0zvlwiMSpq3LYUTNf7Gqc4ujhosYI
>> > TtAw&m=6rPWpKUYQD3kY-2OEikJUWyEJvwKJljWHFC8rd2TCak&s=JjX0Dx8-eSyW2cATMsnG1eAzrKgoDkS1bcS5XYrVtlE&e=.
>>
>> Evgeniy,
>>
>> It seems like I should squash this fix into the above commit? Makes sense?
> IMHO it is not a good idea, because this fix is orthogonal to the above commit.
> Debugging drto overflow issue we found this strange behavior.

Okay! Thanks for clarifying.

> This might be general issue of soft-reset. So I think this should be separate patch.
> Also if these patches are independent, it will be easier to backport them.

Should I add a stable tag then?

[...]

Kind regards
Uffe
Evgeniy Didin March 15, 2018, 5:24 p.m. | #4
On Thu, 2018-03-15 at 14:39 +0100, Ulf Hansson wrote:
> On 15 March 2018 at 14:18, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote:
> > Hello Ulf,
> > 
> > On Thu, 2018-03-15 at 10:57 +0100, Ulf Hansson wrote:
> > > On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote:
> > > > It was found that in IDMAC mode after soft-reset driver switches
> > > > to PIO mode.
> > > > 
> > > > That's what happens in case of DTO timeout overflow calculation failure:
> > > > 1. soft-reset is called
> > > > 2. driver restarts dma
> > > > 3. descriptors states are checked, one of descriptor is owned by the IDMAC.
> > > > 4. driver can't use DMA and then switches to PIO mode.
> > > > 
> > > > Failure was already fixed in:
> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dmmc_msg48125.html&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=vQk-RIbjwN0zvlwiMSpq3LYUTNf7Gqc4ujh
> > > > osYI
> > > > TtAw&m=6rPWpKUYQD3kY-2OEikJUWyEJvwKJljWHFC8rd2TCak&s=JjX0Dx8-eSyW2cATMsnG1eAzrKgoDkS1bcS5XYrVtlE&e=.
> > > 
> > > Evgeniy,
> > > 
> > > It seems like I should squash this fix into the above commit? Makes sense?
> > 
> > IMHO it is not a good idea, because this fix is orthogonal to the above commit.
> > Debugging drto overflow issue we found this strange behavior.
> 
> Okay! Thanks for clarifying.
> 
> > This might be general issue of soft-reset. So I think this should be separate patch.
> > Also if these patches are independent, it will be easier to backport them.
> 
> Should I add a stable tag then?

Sure. The issue can happen since v3.17, because starting with this version function dw_mci_reset was implemented.
Commit 3a33a94ce270 ("mmc: dw_mmc: change to use recommended reset procedure").

Best regards,
Evgeniy Didin
Ulf Hansson March 16, 2018, 7:41 a.m. | #5
On 15 March 2018 at 18:24, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote:
> On Thu, 2018-03-15 at 14:39 +0100, Ulf Hansson wrote:
>> On 15 March 2018 at 14:18, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote:
>> > Hello Ulf,
>> >
>> > On Thu, 2018-03-15 at 10:57 +0100, Ulf Hansson wrote:
>> > > On 14 March 2018 at 20:30, Evgeniy Didin <Evgeniy.Didin@synopsys.com> wrote:
>> > > > It was found that in IDMAC mode after soft-reset driver switches
>> > > > to PIO mode.
>> > > >
>> > > > That's what happens in case of DTO timeout overflow calculation failure:
>> > > > 1. soft-reset is called
>> > > > 2. driver restarts dma
>> > > > 3. descriptors states are checked, one of descriptor is owned by the IDMAC.
>> > > > 4. driver can't use DMA and then switches to PIO mode.
>> > > >
>> > > > Failure was already fixed in:
>> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dmmc_msg48125.html&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=vQk-RIbjwN0zvlwiMSpq3LYUTNf7Gqc4ujh
>> > > > osYI
>> > > > TtAw&m=6rPWpKUYQD3kY-2OEikJUWyEJvwKJljWHFC8rd2TCak&s=JjX0Dx8-eSyW2cATMsnG1eAzrKgoDkS1bcS5XYrVtlE&e=.
>> > >
>> > > Evgeniy,
>> > >
>> > > It seems like I should squash this fix into the above commit? Makes sense?
>> >
>> > IMHO it is not a good idea, because this fix is orthogonal to the above commit.
>> > Debugging drto overflow issue we found this strange behavior.
>>
>> Okay! Thanks for clarifying.
>>
>> > This might be general issue of soft-reset. So I think this should be separate patch.
>> > Also if these patches are independent, it will be easier to backport them.
>>
>> Should I add a stable tag then?
>
> Sure. The issue can happen since v3.17, because starting with this version function dw_mci_reset was implemented.
> Commit 3a33a94ce270 ("mmc: dw_mmc: change to use recommended reset procedure").

Alright. I tried applying it, and the first kernel version that seems
to work is 4.4. Thus I am adding that tag, if you or other people want
it for an earlier version, please to send a backported version to
@stable.

Kind regards
Uffe

Patch

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 0aa39975f33b..26f2ac107702 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -558,6 +558,7 @@  static int dw_mci_idmac_init(struct dw_mci *host)
 					(sizeof(struct idmac_desc_64addr) *
 							(i + 1))) >> 32;
 			/* Initialize reserved and buffer size fields to "0" */
+			p->des0 = 0;
 			p->des1 = 0;
 			p->des2 = 0;
 			p->des3 = 0;
@@ -580,6 +581,7 @@  static int dw_mci_idmac_init(struct dw_mci *host)
 		     i++, p++) {
 			p->des3 = cpu_to_le32(host->sg_dma +
 					(sizeof(struct idmac_desc) * (i + 1)));
+			p->des0 = 0;
 			p->des1 = 0;
 		}
 
@@ -1795,8 +1797,8 @@  static bool dw_mci_reset(struct dw_mci *host)
 	}
 
 	if (host->use_dma == TRANS_MODE_IDMAC)
-		/* It is also recommended that we reset and reprogram idmac */
-		dw_mci_idmac_reset(host);
+		/* It is also required that we reinit idmac */
+		dw_mci_idmac_init(host);
 
 	ret = true;