Re: [PATCH 01/20] mtd: nand: ecc: Add an I/O request tweaking mechanism

[Miquel Raynal <miquel.raynal@bootlin.com>]

Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Wed, 30 Sep
2020 09:53:08 +0200:

> Hello,
> 
> On Wed, 30 Sep 2020 01:01:05 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > +	/* Ensure the request covers the entire page */
> > +	if (true) {//todo (orig->datalen < nanddev_page_size(nand)) {  
> 
> This TODO and "if (true)" looks odd.

Oops, this is a debugging leftover; I don't remember getting a
checkpatch error for that. Surely it did though.

> 
> > +		ctx->bounce_data = true;
> > +		tweak->dataoffs = 0;
> > +		tweak->datalen = nanddev_page_size(nand);
> > +		tweak->databuf.in = ctx->spare_databuf;
> > +		memset(tweak->databuf.in, 0xFF, ctx->page_buffer_size);
> > +	}
> > +
> > +	if (true) {//todo (orig->ooblen < nanddev_per_page_oobsize(nand)) {  
> 
> Ditto.
> 
> Also, I find the wording "tweak" a bit vague, "tweak" really means
> nothing specific. What about prepare/unprepare instead of tweak/restore ?

I understand that "tweaking" might be a bit too vague, but
we already have a ->prepare_io_req() hook, from which we call
nand_ecc_tweark_req(). So this really is a helper that is being called
to hide the real user request (maybe a few bytes), in favor of a
'bigger' one (full page).

What about nand_ecc_extend/restore_req()? Other ideas welcome!

> 
> Also, shouldn't nand_ecc_init_req_tweaking() also allocate the struct
> nand_ecc_req_tweak_ctx so that this structure remains internal ?

This mechanism is part of the ECC engine logic, so if the structure is
allocated by nand_ecc_init_req_tweaking(), it means that the function
must return a pointer to the allocated structure, which should be saved
by the ECC engine driver. Instead, I decided to declare the "request
tweaking context" within the ECC engine configuration (which makes
sense IMHO). This is more or less the same, it just prevents an
extra (useless?) allocation. I guess both would work pretty much the
same.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/