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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2c146e342ac5417db29645f5980ad16c@hyperstone.com>
Date:   Sun, 27 Nov 2022 17:36:24 +0000
From:   Christian Löhle <CLoehle@...erstone.com>
To:     Avri Altman <Avri.Altman@....com>,
        "adrian.hunter@...el.com" <adrian.hunter@...el.com>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] mmc: block: remove non-data R1B ioctl workaround

-----Original Message-----
From: Avri Altman <Avri.Altman@....com> 
Sent: Samstag, 26. November 2022 21:18
To: Christian Löhle <CLoehle@...erstone.com>; adrian.hunter@...el.com; ulf.hansson@...aro.org; linux-mmc@...r.kernel.org; linux-kernel@...r.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@...erstone.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