* [PATCH] mmc: dw_mmc: Avoid hung state if GEN_CMD transfer fails @ 2021-11-03 18:27 Mårten Lindahl 2021-11-11 15:40 ` Vincent Whitchurch 0 siblings, 1 reply; 4+ messages in thread From: Mårten Lindahl @ 2021-11-03 18:27 UTC (permalink / raw) To: Jaehoon Chung, Ulf Hansson Cc: Doug Anderson, kernel, linux-mmc, Mårten Lindahl If we get a data error during a block transfer command, a stop command (CMD12) is normally initiated. But this does not work for the general command (CMD56), but instead the action is ignored and an uninitialized command struct is used for the stop action, with unexpected result. Fix this by adding a check for GEN_CMD when preparing stop transmission. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- drivers/mmc/host/dw_mmc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 6578cc64ae9e..988c32e93e03 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -335,7 +335,8 @@ static u32 dw_mci_prep_stop_abort(struct dw_mci *host, struct mmc_command *cmd) cmdr == MMC_WRITE_BLOCK || cmdr == MMC_WRITE_MULTIPLE_BLOCK || cmdr == MMC_SEND_TUNING_BLOCK || - cmdr == MMC_SEND_TUNING_BLOCK_HS200) { + cmdr == MMC_SEND_TUNING_BLOCK_HS200 || + cmdr == MMC_GEN_CMD) { stop->opcode = MMC_STOP_TRANSMISSION; stop->arg = 0; stop->flags = MMC_RSP_R1B | MMC_CMD_AC; -- 2.20.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] mmc: dw_mmc: Avoid hung state if GEN_CMD transfer fails 2021-11-03 18:27 [PATCH] mmc: dw_mmc: Avoid hung state if GEN_CMD transfer fails Mårten Lindahl @ 2021-11-11 15:40 ` Vincent Whitchurch 2021-11-12 12:46 ` Marten Lindahl 0 siblings, 1 reply; 4+ messages in thread From: Vincent Whitchurch @ 2021-11-11 15:40 UTC (permalink / raw) To: Mårten Lindahl Cc: Jaehoon Chung, Ulf Hansson, linux-mmc@vger.kernel.org, kernel, Doug Anderson On Wed, Nov 03, 2021 at 07:27:16PM +0100, Mårten Lindahl wrote: > If we get a data error during a block transfer command, a stop command > (CMD12) is normally initiated. But this does not work for the general > command (CMD56), but instead the action is ignored and an uninitialized > command struct is used for the stop action, with unexpected result. > > Fix this by adding a check for GEN_CMD when preparing stop transmission. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > --- > drivers/mmc/host/dw_mmc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 6578cc64ae9e..988c32e93e03 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -335,7 +335,8 @@ static u32 dw_mci_prep_stop_abort(struct dw_mci *host, struct mmc_command *cmd) > cmdr == MMC_WRITE_BLOCK || > cmdr == MMC_WRITE_MULTIPLE_BLOCK || > cmdr == MMC_SEND_TUNING_BLOCK || > - cmdr == MMC_SEND_TUNING_BLOCK_HS200) { > + cmdr == MMC_SEND_TUNING_BLOCK_HS200 || > + cmdr == MMC_GEN_CMD) { > stop->opcode = MMC_STOP_TRANSMISSION; > stop->arg = 0; > stop->flags = MMC_RSP_R1B | MMC_CMD_AC; While this fix looks correct for CMD56, the "Data transfer mode" sections of the eMMC and SD specifications list several more data commands, all of which can be aborted by CMD12, but which aren't handled in the if above. If I'm not mistaken, those will also result in an uninitialized stop command being sent in the case of an error, since the driver calls send_stop_abort() on any data error. Is there a reason why those other commands should not be in the list above, or should we fix this list so that CMD12 is initialized for all data commands except SD_IO_RW_EXTENDED? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mmc: dw_mmc: Avoid hung state if GEN_CMD transfer fails 2021-11-11 15:40 ` Vincent Whitchurch @ 2021-11-12 12:46 ` Marten Lindahl 2021-11-15 14:54 ` Ulf Hansson 0 siblings, 1 reply; 4+ messages in thread From: Marten Lindahl @ 2021-11-12 12:46 UTC (permalink / raw) To: Vincent Whitchurch Cc: Mårten Lindahl, Jaehoon Chung, Ulf Hansson, linux-mmc@vger.kernel.org, kernel, Doug Anderson On Thu, Nov 11, 2021 at 04:40:20PM +0100, Vincent Whitchurch wrote: > On Wed, Nov 03, 2021 at 07:27:16PM +0100, Mårten Lindahl wrote: > > If we get a data error during a block transfer command, a stop command > > (CMD12) is normally initiated. But this does not work for the general > > command (CMD56), but instead the action is ignored and an uninitialized > > command struct is used for the stop action, with unexpected result. > > > > Fix this by adding a check for GEN_CMD when preparing stop transmission. > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > --- > > drivers/mmc/host/dw_mmc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > index 6578cc64ae9e..988c32e93e03 100644 > > --- a/drivers/mmc/host/dw_mmc.c > > +++ b/drivers/mmc/host/dw_mmc.c > > @@ -335,7 +335,8 @@ static u32 dw_mci_prep_stop_abort(struct dw_mci *host, struct mmc_command *cmd) > > cmdr == MMC_WRITE_BLOCK || > > cmdr == MMC_WRITE_MULTIPLE_BLOCK || > > cmdr == MMC_SEND_TUNING_BLOCK || > > - cmdr == MMC_SEND_TUNING_BLOCK_HS200) { > > + cmdr == MMC_SEND_TUNING_BLOCK_HS200 || > > + cmdr == MMC_GEN_CMD) { > > stop->opcode = MMC_STOP_TRANSMISSION; > > stop->arg = 0; > > stop->flags = MMC_RSP_R1B | MMC_CMD_AC; Hi! > > While this fix looks correct for CMD56, the "Data transfer mode" > sections of the eMMC and SD specifications list several more data > commands, all of which can be aborted by CMD12, but which aren't handled > in the if above. > > If I'm not mistaken, those will also result in an uninitialized stop > command being sent in the case of an error, since the driver calls > send_stop_abort() on any data error. > > Is there a reason why those other commands should not be in the list > above, or should we fix this list so that CMD12 is initialized for all > data commands except SD_IO_RW_EXTENDED? I agree that there are more commands that according to the specification should be included in the list. The most obvious ones that I identify are: CMD26, CMD27, CMD30, CMD46, and CMD47. But the reason why I added just CMD56 is that I have an easy way to test this specific command, and which was how I saw the need for this fix. My suggestion is to separate adding the other commands to one (or more) separate patch(es), to when there are good test cases for those. Kind regards Mårten ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mmc: dw_mmc: Avoid hung state if GEN_CMD transfer fails 2021-11-12 12:46 ` Marten Lindahl @ 2021-11-15 14:54 ` Ulf Hansson 0 siblings, 0 replies; 4+ messages in thread From: Ulf Hansson @ 2021-11-15 14:54 UTC (permalink / raw) To: Marten Lindahl Cc: Vincent Whitchurch, Mårten Lindahl, Jaehoon Chung, linux-mmc@vger.kernel.org, kernel, Doug Anderson On Fri, 12 Nov 2021 at 13:46, Marten Lindahl <martenli@axis.com> wrote: > > On Thu, Nov 11, 2021 at 04:40:20PM +0100, Vincent Whitchurch wrote: > > On Wed, Nov 03, 2021 at 07:27:16PM +0100, Mårten Lindahl wrote: > > > If we get a data error during a block transfer command, a stop command > > > (CMD12) is normally initiated. But this does not work for the general > > > command (CMD56), but instead the action is ignored and an uninitialized > > > command struct is used for the stop action, with unexpected result. > > > > > > Fix this by adding a check for GEN_CMD when preparing stop transmission. > > > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > > --- > > > drivers/mmc/host/dw_mmc.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > > index 6578cc64ae9e..988c32e93e03 100644 > > > --- a/drivers/mmc/host/dw_mmc.c > > > +++ b/drivers/mmc/host/dw_mmc.c > > > @@ -335,7 +335,8 @@ static u32 dw_mci_prep_stop_abort(struct dw_mci *host, struct mmc_command *cmd) > > > cmdr == MMC_WRITE_BLOCK || > > > cmdr == MMC_WRITE_MULTIPLE_BLOCK || > > > cmdr == MMC_SEND_TUNING_BLOCK || > > > - cmdr == MMC_SEND_TUNING_BLOCK_HS200) { > > > + cmdr == MMC_SEND_TUNING_BLOCK_HS200 || > > > + cmdr == MMC_GEN_CMD) { > > > stop->opcode = MMC_STOP_TRANSMISSION; > > > stop->arg = 0; > > > stop->flags = MMC_RSP_R1B | MMC_CMD_AC; > > Hi! > > > > While this fix looks correct for CMD56, the "Data transfer mode" > > sections of the eMMC and SD specifications list several more data > > commands, all of which can be aborted by CMD12, but which aren't handled > > in the if above. > > > > If I'm not mistaken, those will also result in an uninitialized stop > > command being sent in the case of an error, since the driver calls > > send_stop_abort() on any data error. > > > > Is there a reason why those other commands should not be in the list > > above, or should we fix this list so that CMD12 is initialized for all > > data commands except SD_IO_RW_EXTENDED? > > I agree that there are more commands that according to the specification > should be included in the list. The most obvious ones that I identify are: > CMD26, CMD27, CMD30, CMD46, and CMD47. > > But the reason why I added just CMD56 is that I have an easy way to test > this specific command, and which was how I saw the need for this fix. > > My suggestion is to separate adding the other commands to one (or more) > separate patch(es), to when there are good test cases for those. I agree with both of you, it would be nice to make a change that takes care of all the relevant commands at once, rather than having to come back to this later again. On the other hand, as it's not always straightforward to test the other commands, I am fine with this as well. So, applied for next, thanks! Kind regards Uffe ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-15 14:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-03 18:27 [PATCH] mmc: dw_mmc: Avoid hung state if GEN_CMD transfer fails Mårten Lindahl 2021-11-11 15:40 ` Vincent Whitchurch 2021-11-12 12:46 ` Marten Lindahl 2021-11-15 14:54 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox