[PATCH] mmc: block: remove non-data R1B ioctl workaround

[PATCH] mmc: block: remove non-data R1B ioctl workaround

[Christian Löhle]

The workaround of pretending R1B non-data transfers are
data transfers in order for the busy timeout to be respected
by the host controller driver is removed. It wasn't useful
in a long time.

Initially the workaround ensured that R1B commands did not
time out by setting the data timeout to be the command timeout
in commit cb87ea28ed9e ("mmc: core: Add mmc CMD+ACMD passthrough ioctl").
This was moved inside of an if clause with idata->buf_bytes being set
in commit 4d6144de8ba2 ("mmc: core: check for zero length ioctl data").
This patch intends to fix the issuing of R1B data command CMD24.
Its data timeout was being overwritten with 0 because cmd_timeout
wasn't set at the point the workaround applied, but data_timeout was.
But since the workaround was now inside of the idata->buf_bytes clause
and intended to fix R1B non-data transfers that do not have buf_bytes
set we can also remove the workaround altogether.

Fixes: cb87ea28ed9e ("mmc: core: Add mmc CMD+ACMD passthrough ioctl")
Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 drivers/mmc/core/block.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index db6d8a099910..20da7ed43e6d 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -514,19 +514,6 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 		if (idata->ic.data_timeout_ns)
 			data.timeout_ns = idata->ic.data_timeout_ns;
 
-		if ((cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
-			/*
-			 * Pretend this is a data transfer and rely on the
-			 * host driver to compute timeout.  When all host
-			 * drivers support cmd.cmd_timeout for R1B, this
-			 * can be changed to:
-			 *
-			 *     mrq.data = NULL;
-			 *     cmd.cmd_timeout = idata->ic.cmd_timeout_ms;
-			 */
-			data.timeout_ns = idata->ic.cmd_timeout_ms * 1000000;
-		}
-
 		mrq.data = &data;
 	}
 
-- 
2.37.3

Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

RE: [PATCH] mmc: block: remove non-data R1B ioctl workaround

[Avri Altman]

> The workaround of pretending R1B non-data transfers are data transfers in
> order for the busy timeout to be respected by the host controller driver is
> removed. It wasn't useful in a long time.
> 
> Initially the workaround ensured that R1B commands did not time out by
> setting the data timeout to be the command timeout in commit
> cb87ea28ed9e ("mmc: core: Add mmc CMD+ACMD passthrough ioctl").
> This was moved inside of an if clause with idata->buf_bytes being set in
> commit 4d6144de8ba2 ("mmc: core: check for zero length ioctl data").
> This patch intends to fix the issuing of R1B data command CMD24.
CMD24 response is R1?

> Its data timeout was being overwritten with 0 because cmd_timeout wasn't
> set at the point the workaround applied, but data_timeout was.
> But since the workaround was now inside of the idata->buf_bytes clause and
> intended to fix R1B non-data transfers that do not have buf_bytes set we can
> also remove the workaround altogether.
I find the above explanation a bit confusing.
Maybe just explain in 1 or 2 sentences why this workaround is no longer needed?

Thanks,
Avri

> 
> Fixes: cb87ea28ed9e ("mmc: core: Add mmc CMD+ACMD passthrough ioctl")
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>  drivers/mmc/core/block.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> db6d8a099910..20da7ed43e6d 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -514,19 +514,6 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card
> *card, struct mmc_blk_data *md,
>                 if (idata->ic.data_timeout_ns)
>                         data.timeout_ns = idata->ic.data_timeout_ns;
> 
> -               if ((cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
> -                       /*
> -                        * Pretend this is a data transfer and rely on the
> -                        * host driver to compute timeout.  When all host
> -                        * drivers support cmd.cmd_timeout for R1B, this
> -                        * can be changed to:
> -                        *
> -                        *     mrq.data = NULL;
> -                        *     cmd.cmd_timeout = idata->ic.cmd_timeout_ms;
> -                        */
> -                       data.timeout_ns = idata->ic.cmd_timeout_ms * 1000000;
> -               }
> -
>                 mrq.data = &data;
>         }
> 
> --
> 2.37.3
> 
> Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz Managing Director:
> Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782

RE: [PATCH] mmc: block: remove non-data R1B ioctl workaround

[Christian Löhle]

-----Original Message-----
From: Avri Altman <Avri.Altman@wdc.com> 
Sent: Samstag, 26. November 2022 21:18
To: Christian Löhle <CLoehle@hyperstone.com>; adrian.hunter@intel.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: RE: [PATCH] mmc: block: remove non-data R1B ioctl workaround

>> The workaround of pretending R1B non-data transfers are data transfers 
>> in order for the busy timeout to be respected by the host controller 
>> driver is removed. It wasn't useful in a long time.
>> 
>> Initially the workaround ensured that R1B commands did not time out by 
>> setting the data timeout to be the command timeout in commit 
>> cb87ea28ed9e ("mmc: core: Add mmc CMD+ACMD passthrough ioctl").
>> This was moved inside of an if clause with idata->buf_bytes being set 
>> in commit 4d6144de8ba2 ("mmc: core: check for zero length ioctl data").
>> This patch intends to fix the issuing of R1B data command CMD24.
> CMD24 response is R1?

Yes my bad, that was a relic of my userspace program to poll for busy.
In that case see below.

>
>> Its data timeout was being overwritten with 0 because cmd_timeout 
>> wasn't set at the point the workaround applied, but data_timeout was.
>> But since the workaround was now inside of the idata->buf_bytes clause 
>> and intended to fix R1B non-data transfers that do not have buf_bytes 
>> set we can also remove the workaround altogether.
> I find the above explanation a bit confusing.
> Maybe just explain in 1 or 2 sentences why this workaround is no longer needed?

My explanation could have been clearer if I understood why the original author chose
this way. Maybe some host controller drivers did not call mmc_request_done until
busy is no longer asserted if MMC_RSP_BUSY was set?
Maybe also the workaround was never necessary at all (my guess).
Why I think it can be removed:
The workaround has been moved inside of the if (idata->buf_bytes) clause and is for R1B
type command responses.
There are no commands with R1B responses that invoke data transfer in either direction.
The workaround is just dead code since commit 4d6144de8ba2 ("mmc: core: check for zero length ioctl data").
I will fix the commit message up a bit.

>
> Thanks,
> Avri
>
>> 
>> Fixes: cb87ea28ed9e ("mmc: core: Add mmc CMD+ACMD passthrough ioctl")
>> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
>> ---
>>  drivers/mmc/core/block.c | 13 -------------
>>  1 file changed, 13 deletions(-)
>> 
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 
>> db6d8a099910..20da7ed43e6d 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -514,19 +514,6 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card 
>> *card, struct mmc_blk_data *md,
>>                 if (idata->ic.data_timeout_ns)
>>                         data.timeout_ns = idata->ic.data_timeout_ns;
>> 
>> -               if ((cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
>> -                       /*
>> -                        * Pretend this is a data transfer and rely on the
>> -                        * host driver to compute timeout.  When all host
>> -                        * drivers support cmd.cmd_timeout for R1B, this
>> -                        * can be changed to:
>> -                        *
>> -                        *     mrq.data = NULL;
>> -                        *     cmd.cmd_timeout = idata->ic.cmd_timeout_ms;
>> -                        */
>> -                       data.timeout_ns = idata->ic.cmd_timeout_ms * 1000000;
>> -               }
>> -
>>                 mrq.data = &data;
>>         }
>> 
>> --
>> 2.37.3
>> 
>> Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz Managing Director:
>> Dr. Jan Peter Berns.
>> Commercial register of local courts: Freiburg HRB381782
>
>
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782