[PATCH 4/9] mtd: spi-nand: Add continuous read support

[Pratyush Yadav]

On Fri, Aug 23 2024, Miquel Raynal wrote:

> Hi Pratyush,
>
>> > @@ -684,7 +830,11 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
>> >  
>> >  	old_stats = mtd->ecc_stats;
>> >  
>> > -	ret = spinand_mtd_regular_page_read(mtd, from, ops, &max_bitflips);
>> > +	if (static_branch_unlikely(&cont_read_supported) &&
>> > +	    spinand_use_cont_read(mtd, from, ops))  
>> 
>> This looks a bit odd. If your system has two NAND devices, one with
>> continuous read support and one without, you will enable this static
>> branch and then attempt to use continuous read for both, right? I think
>> spinand_use_cont_read() should have a check for spinand->set_cont_read,
>> otherwise you end up calling a NULL function pointer for the flash
>> without continuous read.
>
> Mmmh that's right, I wanted a slightly more optimal path but the static
> branch doesn't play well here if it's a global parameter.
>
>> This would reduce the cost of checking set_cont_read in the hot path if
>> there are no flashes with continuous read. When you do have at least
>> one, you would have to pay it every time. I suppose you can do some sort
>> of double branch where you take the respective static branch if _all_ or
>> _none_ of the flashes have continuous read, and then the heterogeneous
>> setups do the check at runtime. But is spinand_mtd_read() even hot
>> enough to warrant such optimizations?
>
> I believe using static branches here would have been relevant with a
> single chip use-case, but I don't think it is worth using if we have
> more than one flash. It is an almost tepid path, at most, so I'll just
> check ->set_cont_read presence.

Sounds good.

>
>> > +		ret = spinand_mtd_continuous_page_read(mtd, from, ops, &max_bitflips);
>> > +	else
>> > +		ret = spinand_mtd_regular_page_read(mtd, from, ops, &max_bitflips);
>> >  
>> >  	if (ops->stats) {
>> >  		ops->stats->uncorrectable_errors +=
>> > @@ -874,6 +1024,9 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
>> >  	};
>> >  	struct spi_mem_dirmap_desc *desc;
>> >  
>> > +	if (static_branch_unlikely(&cont_read_supported))
>> > +		info.length = nanddev_eraseblock_size(nand);  
>> 
>> Same here. With a heterogeneous setup, you will set length to eraseblock
>> size even for the non-continuous-read flash. Though from a quick look
>> info.length doesn't seem to be used anywhere meaningful.
>
> It is a parameter for the spi controller driver. It may lead to an
> error if the size is too big for what the controller is capable of,
> which is not the case for any of the existing controllers which are
> all capable of mapping a block at least (source: me reading spi code).
>
> the info parameter being per-nand it's fine to tune it like that, but I
> agree the if() conditional is currently wrong in the case of dual
> devices.
>
>> In general, a static branch looks misused here. This isn't even a hot
>> path where you'd care about performance.
>
> TBH the read path might be considered hot, but not hot enough to
> justify too complex optimizations schemes, definitely.

To clarify, I meant this just for the usage in spinand_create_dirmap().
This function is called exactly once for each device at probe time. So
optimizations like static branches don't make much sense here.

For spinand_mtd_read() a case can plausibly be made for doing such
optimizations, though as you mentioned before the path likely isn't that
hot. If you fancy it, perhaps run some benchmarks to see what read
performance you get with static branches and without them to get some
real data.

-- 
Regards,
Pratyush Yadav