[PATCH 0/1] Fix race between stop command and start of next command in atmel-mci

[PATCH 0/1] Fix race between stop command and start of next command in atmel-mci

[Tobias Schramm]

Hey,

while using both slots on controller version 0x210 I noticed a race
condition in atmel-mci. When sending a stop command the command ready
interrupt is enabled before writing to the command register. This causes
the associated interrupt to fire immediately, making the state machine
believe that the stop command has completed already.
Usually this does not seem to cause a problem. I suspect the roudtrip of
mmc command completion through the kernel simply takes long enough that
the stop command has usually completed once a new command is queued.
However when using both slots a command might be queued for the other
slot already, causing immediate dispatch of that command to the
controller. This then interrupts the stop command still being sent and
also corrupts the new command.

I have only tested this patch with controller version 0x210, but from
documentation it seems like this should be a correct fix also for other
controller versions.

Cheers,
Tobias

Tobias Schramm (1):
  mmc: atmel-mci: fix race between stop command and start of next
    command

 drivers/mmc/host/atmel-mci.c | 3 ---
 1 file changed, 3 deletions(-)

-- 
2.30.2

[PATCH 1/1] mmc: atmel-mci: fix race between stop command and start of next command

[Tobias Schramm]

This commit fixes a race between completion of stop command and start of a
new command.
Previously the command ready interrupt was enabled before stop command
was written to the command register. This caused the command ready
interrupt to fire immediately since the CMDRDY flag is asserted constantly
while there is no command in progress.
Consequently the command state machine will immediately advance to the
next state when the tasklet function is executed again, no matter
actual completion state of the stop command.
Thus a new command can then be dispatched immediately, interrupting and
corrupting the stop command on the CMD line.
Fix that by dropping the command ready interrupt enable before calling
atmci_send_stop_cmd. atmci_send_stop_cmd does already enable the
command ready interrupt, no further writes to ATMCI_IER are necessary.

Signed-off-by: Tobias Schramm <t.schramm@manjaro.org>
---
 drivers/mmc/host/atmel-mci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index bb9bbf1c927b..dd18440a90c5 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -1817,7 +1817,6 @@ static void atmci_tasklet_func(struct tasklet_struct *t)
 				atmci_writel(host, ATMCI_IER, ATMCI_NOTBUSY);
 				state = STATE_WAITING_NOTBUSY;
 			} else if (host->mrq->stop) {
-				atmci_writel(host, ATMCI_IER, ATMCI_CMDRDY);
 				atmci_send_stop_cmd(host, data);
 				state = STATE_SENDING_STOP;
 			} else {
@@ -1850,8 +1849,6 @@ static void atmci_tasklet_func(struct tasklet_struct *t)
 				 * command to send.
 				 */
 				if (host->mrq->stop) {
-					atmci_writel(host, ATMCI_IER,
-					             ATMCI_CMDRDY);
 					atmci_send_stop_cmd(host, data);
 					state = STATE_SENDING_STOP;
 				} else {
-- 
2.30.2

Re: [PATCH 1/1] mmc: atmel-mci: fix race between stop command and start of next command

[Ulf Hansson]

On Fri, 30 Dec 2022 at 20:43, Tobias Schramm <t.schramm@manjaro.org> wrote:
>
> This commit fixes a race between completion of stop command and start of a
> new command.
> Previously the command ready interrupt was enabled before stop command
> was written to the command register. This caused the command ready
> interrupt to fire immediately since the CMDRDY flag is asserted constantly
> while there is no command in progress.
> Consequently the command state machine will immediately advance to the
> next state when the tasklet function is executed again, no matter
> actual completion state of the stop command.
> Thus a new command can then be dispatched immediately, interrupting and
> corrupting the stop command on the CMD line.
> Fix that by dropping the command ready interrupt enable before calling
> atmci_send_stop_cmd. atmci_send_stop_cmd does already enable the
> command ready interrupt, no further writes to ATMCI_IER are necessary.
>
> Signed-off-by: Tobias Schramm <t.schramm@manjaro.org>

This looks reasonable to me. I assume we should tag this for stable kernels too?

Moreover, I would like to get an ack from Ludovic before applying.

Kind regards
Uffe

> ---
>  drivers/mmc/host/atmel-mci.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
> index bb9bbf1c927b..dd18440a90c5 100644
> --- a/drivers/mmc/host/atmel-mci.c
> +++ b/drivers/mmc/host/atmel-mci.c
> @@ -1817,7 +1817,6 @@ static void atmci_tasklet_func(struct tasklet_struct *t)
>                                 atmci_writel(host, ATMCI_IER, ATMCI_NOTBUSY);
>                                 state = STATE_WAITING_NOTBUSY;
>                         } else if (host->mrq->stop) {
> -                               atmci_writel(host, ATMCI_IER, ATMCI_CMDRDY);
>                                 atmci_send_stop_cmd(host, data);
>                                 state = STATE_SENDING_STOP;
>                         } else {
> @@ -1850,8 +1849,6 @@ static void atmci_tasklet_func(struct tasklet_struct *t)
>                                  * command to send.
>                                  */
>                                 if (host->mrq->stop) {
> -                                       atmci_writel(host, ATMCI_IER,
> -                                                    ATMCI_CMDRDY);
>                                         atmci_send_stop_cmd(host, data);
>                                         state = STATE_SENDING_STOP;
>                                 } else {
> --
> 2.30.2
>

Re: [PATCH 1/1] mmc: atmel-mci: fix race between stop command and start of next command

[Ludovic.Desroches]

On 30/12/2022 20:43, Tobias Schramm wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> This commit fixes a race between completion of stop command and start of a
> new command.
> Previously the command ready interrupt was enabled before stop command
> was written to the command register. This caused the command ready
> interrupt to fire immediately since the CMDRDY flag is asserted constantly
> while there is no command in progress.
> Consequently the command state machine will immediately advance to the
> next state when the tasklet function is executed again, no matter
> actual completion state of the stop command.
> Thus a new command can then be dispatched immediately, interrupting and
> corrupting the stop command on the CMD line.
> Fix that by dropping the command ready interrupt enable before calling
> atmci_send_stop_cmd. atmci_send_stop_cmd does already enable the
> command ready interrupt, no further writes to ATMCI_IER are necessary.
> 
> Signed-off-by: Tobias Schramm <t.schramm@manjaro.org>

Hi,

In theory this changes make sense. I'm always afraid when something is 
changed in this driver which handles many version of the IP...

As we never encountered this issue until now, I can't really test this 
fix. I checked on an old board at91sam9m10g45-ek that mmc is still 
working and it's okay.

So
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

Regards,
Ludovic

> ---
>   drivers/mmc/host/atmel-mci.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
> index bb9bbf1c927b..dd18440a90c5 100644
> --- a/drivers/mmc/host/atmel-mci.c
> +++ b/drivers/mmc/host/atmel-mci.c
> @@ -1817,7 +1817,6 @@ static void atmci_tasklet_func(struct tasklet_struct *t)
>                                  atmci_writel(host, ATMCI_IER, ATMCI_NOTBUSY);
>                                  state = STATE_WAITING_NOTBUSY;
>                          } else if (host->mrq->stop) {
> -                               atmci_writel(host, ATMCI_IER, ATMCI_CMDRDY);
>                                  atmci_send_stop_cmd(host, data);
>                                  state = STATE_SENDING_STOP;
>                          } else {
> @@ -1850,8 +1849,6 @@ static void atmci_tasklet_func(struct tasklet_struct *t)
>                                   * command to send.
>                                   */
>                                  if (host->mrq->stop) {
> -                                       atmci_writel(host, ATMCI_IER,
> -                                                    ATMCI_CMDRDY);
>                                          atmci_send_stop_cmd(host, data);
>                                          state = STATE_SENDING_STOP;
>                                  } else {
> --
> 2.30.2
> 

Re: [PATCH 1/1] mmc: atmel-mci: fix race between stop command and start of next command

[Ulf Hansson]

On Thu, 26 Jan 2023 at 15:44, <Ludovic.Desroches@microchip.com> wrote:
>
> On 30/12/2022 20:43, Tobias Schramm wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > This commit fixes a race between completion of stop command and start of a
> > new command.
> > Previously the command ready interrupt was enabled before stop command
> > was written to the command register. This caused the command ready
> > interrupt to fire immediately since the CMDRDY flag is asserted constantly
> > while there is no command in progress.
> > Consequently the command state machine will immediately advance to the
> > next state when the tasklet function is executed again, no matter
> > actual completion state of the stop command.
> > Thus a new command can then be dispatched immediately, interrupting and
> > corrupting the stop command on the CMD line.
> > Fix that by dropping the command ready interrupt enable before calling
> > atmci_send_stop_cmd. atmci_send_stop_cmd does already enable the
> > command ready interrupt, no further writes to ATMCI_IER are necessary.
> >
> > Signed-off-by: Tobias Schramm <t.schramm@manjaro.org>
>
> Hi,
>
> In theory this changes make sense. I'm always afraid when something is
> changed in this driver which handles many version of the IP...
>
> As we never encountered this issue until now, I can't really test this
> fix. I checked on an old board at91sam9m10g45-ek that mmc is still
> working and it's okay.
>
> So
> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
>
> Regards,
> Ludovic

Thanks for your ack and thoughts!

It's not clear to me whether the problem is hypothetical or in fact a
real problem. Tobias can you help to fill in here?

Nevertheless I have applied this for next, to allow more testing to be
done. In the meantime, we can discuss whether we should add a stable
tag or leave that to later as manual backports.

[...]

Kind regards
Uffe