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 |
