Re: [PATCH 07/10] mci: add support for discarding write blocks

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 30.07.24 12:05, Sascha Hauer wrote:
> On Tue, Jul 30, 2024 at 09:19:26AM +0200, Ahmad Fatoum wrote:

>> +	__be32 *ssr;
>> +	int err;
>> +	unsigned int au, eo, et, es;
>> +
>> +	if (!IS_ENABLED(CONFIG_MCI_ERASE))
>> +		return -ENOSYS;
> 
> I think we settled on using -EOPNOTSUPP in this case.

I like -ENOSYS, because it indicates that supper is merely missing instead of not
being available in the first place.

>> +	if (mci->can_trim) {
>> +		arg = MMC_TRIM_ARG;
>> +	} else {
>> +		/* We don't use discard, as it doesn't guarantee a fixed value */
>> +		arg = MMC_ERASE_ARG;
>> +		blkcnt = mmc_align_erase_size(mci, &from, &to, blkcnt);
>> +	}
>> +
>> +	if (blkcnt == 0)
>> +		return 0;
>> +
>> +	if (to <= from)
>> +		return -EINVAL;
> 
> When mmc_align_erase_size() is not called then we cannot arrive here
> as we already returned in the if (blkcnt == 0) check above.
> When mmc_align_erase_size() is called and this test triggers then it
> only reveals a bug in mmc_align_erase_size().
> 
> I think this test should go away.



>> +	while (i < blkcnt) {
>> +		sector_t blk_r;
>> +
>> +		/* TODO: While it's possible to clear many erase groups at once
>> +		 * and it greatly improves throughput, drivers need adjustment:
>> +		 *
>> +		 * Many drivers hardcode a maximal wait time before aborting
>> +		 * the wait for R1b and returning -ETIMEDOUT. With long
>> +		 * erases/trims, we are bound to run into this timeout, so for now
>> +		 * we just split into suifficiently small erases that are unlikely
>> +		 * to trigger the time.
>> +		 *
>> +		 * What Linux does and what we should be doing in barebox is:
>> +		 *
>> +		 *  - add a struct mci_cmd::busy_timeout member that drivers should
>> +		 *    use instead of hardcoding their own timeout delay. The busy
>> +		 *    timeout length can be calculated by the MCI core after
>> +		 *    consulting the appropriate CSD/EXT_CSD/SSR registers.
>> +		 *
>> +		 *  - add a struct mci_host::max_busy_timeout member, where drivers
>> +		 *    can indicate the maximum timeout they are able to support.
>> +		 *    The MCI core will never set a busy_timeout that exceeds this
>> +		 *    value.
>> +		 *
>> +		 *  Example Samsung eMMC 8GTF4:
>> +		 *
>> +		 *    time erase /dev/mmc2.part_of_512m # 1024 trims
>> +		 *    time: 2849ms
>> +		 *
>> +		 *    time erase /dev/mmc2.part_of_512m # single trim
>> +		 *    time: 56ms
>> +		 */
>> +
>> +		if (IS_SD(mci) && mci->ssr.au) {
>> +			blk_r = ((blkcnt - i) > mci->ssr.au) ?
>> +				mci->ssr.au : (blkcnt - i);
>> +		} else {
>> +			blk_r = ((blkcnt - i) > mci->erase_grp_size) ?
>> +				mci->erase_grp_size : (blkcnt - i);
>> +		}
>> +
>> +		rc =  mci_block_erase(mci, from, to, arg);
> 
> You say you split up the whole erase into sufficiently small erases, but
> 'from' and 'to' are never changed in this loop and you seem to erase
> the whole area multiple times.


Ouch. Will revisit.

Thanks,
Ahmad

> 
>> +		if (rc)
>> +			break;
>> +
>> +		/* Waiting for the ready status */
>> +		rc = mci_poll_until_ready(mci, 1000 /* ms */);
>> +		if (rc)
>> +			break;
>> +
>> +		i += blk_r;
>> +	}
>> +
>> +	return i == blkcnt ? 0 : rc;
>> +}
> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |