[PATCH] mtd: remove stale comment in mtd_oob_ops structure

[PATCH] mtd: remove stale comment in mtd_oob_ops structure

[Miquel Raynal]

A comment in the kernel doc of the mtd_oob_ops structure tells that it
is not possible to write more than one page with OOB. This was
probably true at some time in the past but today it is entirely wrong.

As one can see for instance in the nand_do_write_ops() helper available
in the NAND core, this implementation called by mtd->_write_oob()
simply loops over the pages until everything has been written.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/mtd/mtd.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index a86c4fa93115..59578aab277a 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -66,10 +66,6 @@ struct mtd_erase_region_info {
  *		mode = MTD_OPS_PLACE_OOB or MTD_OPS_RAW)
  * @datbuf:	data buffer - if NULL only oob data are read/written
  * @oobbuf:	oob data buffer
- *
- * Note, it is allowed to read more than one OOB area at one go, but not write.
- * The interface assumes that the OOB write requests program only one page's
- * OOB area.
  */
 struct mtd_oob_ops {
 	unsigned int	mode;
-- 
2.14.1

Re: [PATCH] mtd: remove stale comment in mtd_oob_ops structure

[Boris Brezillon]

On Sat, 14 Jul 2018 14:32:34 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> A comment in the kernel doc of the mtd_oob_ops structure tells that it
> is not possible to write more than one page with OOB. This was
> probably true at some time in the past but today it is entirely wrong.
> 
> As one can see for instance in the nand_do_write_ops() helper available
> in the NAND core, this implementation called by mtd->_write_oob()
> simply loops over the pages until everything has been written.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/linux/mtd/mtd.h | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index a86c4fa93115..59578aab277a 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -66,10 +66,6 @@ struct mtd_erase_region_info {
>   *		mode = MTD_OPS_PLACE_OOB or MTD_OPS_RAW)
>   * @datbuf:	data buffer - if NULL only oob data are read/written
>   * @oobbuf:	oob data buffer
> - *
> - * Note, it is allowed to read more than one OOB area at one go, but not write.
> - * The interface assumes that the OOB write requests program only one page's
> - * OOB area.

Hm, I wouldn't be so prompt at nuking this comment since some
implementations (like onenand) really have this limitation. How about
adjusting the comment to reflect the fact that this limitation is
implementation dependent?

"
Note, some MTD drivers do not allow you to write more than one OOB area
at one go. If you try to do that on such an MTD device, -EINVAL will be
returned. If you want to make your implementation portable on all kind
of MTD devices you should split the write request into several
sub-requests when the request crosses a page boundary.
"

>   */
>  struct mtd_oob_ops {
>  	unsigned int	mode;

Re: [PATCH] mtd: remove stale comment in mtd_oob_ops structure

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 17 Jul 2018
14:25:24 +0200:

> On Sat, 14 Jul 2018 14:32:34 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > A comment in the kernel doc of the mtd_oob_ops structure tells that it
> > is not possible to write more than one page with OOB. This was
> > probably true at some time in the past but today it is entirely wrong.
> > 
> > As one can see for instance in the nand_do_write_ops() helper available
> > in the NAND core, this implementation called by mtd->_write_oob()
> > simply loops over the pages until everything has been written.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/linux/mtd/mtd.h | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > index a86c4fa93115..59578aab277a 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -66,10 +66,6 @@ struct mtd_erase_region_info {
> >   *		mode = MTD_OPS_PLACE_OOB or MTD_OPS_RAW)
> >   * @datbuf:	data buffer - if NULL only oob data are read/written
> >   * @oobbuf:	oob data buffer
> > - *
> > - * Note, it is allowed to read more than one OOB area at one go, but not write.
> > - * The interface assumes that the OOB write requests program only one page's
> > - * OOB area.  
> 
> Hm, I wouldn't be so prompt at nuking this comment since some
> implementations (like onenand) really have this limitation. How about

Didn't know/see.

> adjusting the comment to reflect the fact that this limitation is
> implementation dependent?
> 
> "
> Note, some MTD drivers do not allow you to write more than one OOB area
> at one go. If you try to do that on such an MTD device, -EINVAL will be
> returned. If you want to make your implementation portable on all kind
> of MTD devices you should split the write request into several
> sub-requests when the request crosses a page boundary.
> "

Fine by me.

However, for a future work, I would think that it's something that we
should handle at MTD level (or even in the onenand driver), not at user
level, and simplify the ABI.


Thanks,
Miquèl

Re: [PATCH] mtd: remove stale comment in mtd_oob_ops structure

[Boris Brezillon]

On Tue, 17 Jul 2018 14:34:05 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@bootlin.com> wrote on Tue, 17 Jul 2018
> 14:25:24 +0200:
> 
> > On Sat, 14 Jul 2018 14:32:34 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > A comment in the kernel doc of the mtd_oob_ops structure tells that it
> > > is not possible to write more than one page with OOB. This was
> > > probably true at some time in the past but today it is entirely wrong.
> > > 
> > > As one can see for instance in the nand_do_write_ops() helper available
> > > in the NAND core, this implementation called by mtd->_write_oob()
> > > simply loops over the pages until everything has been written.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  include/linux/mtd/mtd.h | 4 ----
> > >  1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > > index a86c4fa93115..59578aab277a 100644
> > > --- a/include/linux/mtd/mtd.h
> > > +++ b/include/linux/mtd/mtd.h
> > > @@ -66,10 +66,6 @@ struct mtd_erase_region_info {
> > >   *		mode = MTD_OPS_PLACE_OOB or MTD_OPS_RAW)
> > >   * @datbuf:	data buffer - if NULL only oob data are read/written
> > >   * @oobbuf:	oob data buffer
> > > - *
> > > - * Note, it is allowed to read more than one OOB area at one go, but not write.
> > > - * The interface assumes that the OOB write requests program only one page's
> > > - * OOB area.    
> > 
> > Hm, I wouldn't be so prompt at nuking this comment since some
> > implementations (like onenand) really have this limitation. How about  
> 
> Didn't know/see.
> 
> > adjusting the comment to reflect the fact that this limitation is
> > implementation dependent?
> > 
> > "
> > Note, some MTD drivers do not allow you to write more than one OOB area
> > at one go. If you try to do that on such an MTD device, -EINVAL will be
> > returned. If you want to make your implementation portable on all kind
> > of MTD devices you should split the write request into several
> > sub-requests when the request crosses a page boundary.
> > "  
> 
> Fine by me.
> 
> However, for a future work, I would think that it's something that we
> should handle at MTD level (or even in the onenand driver), not at user
> level, and simplify the ABI.

I definitely agree with that.