On Wed, May 17, 2017 at 05:29:11PM +0200, Boris Brezillon wrote:
> Hi Chris,
>
> On Wed, 17 May 2017 17:39:07 +1200
> Chris Packham <[email protected]> wrote:
>
> > Setting the of_node for the mtd device allows the generic mtd code to
> > setup the partitions. Additionally we must specify a non-zero erasesize
> > for the partitions to be writeable.
> >
> > Signed-off-by: Chris Packham <[email protected]>
> > ---
> > drivers/mtd/devices/mchp23k256.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/mtd/devices/mchp23k256.c b/drivers/mtd/devices/mchp23k256.c
> > index 2542f5b8b63f..02c6b9dcbd3e 100644
> > --- a/drivers/mtd/devices/mchp23k256.c
> > +++ b/drivers/mtd/devices/mchp23k256.c
> > @@ -143,6 +143,7 @@ static int mchp23k256_probe(struct spi_device *spi)
> >
> > data = dev_get_platdata(&spi->dev);
> >
> > + mtd_set_of_node(&flash->mtd, spi->dev.of_node);
> > flash->mtd.dev.parent = &spi->dev;
> > flash->mtd.type = MTD_RAM;
> > flash->mtd.flags = MTD_CAP_RAM;
> > @@ -151,6 +152,10 @@ static int mchp23k256_probe(struct spi_device *spi)
> > flash->mtd._read = mchp23k256_read;
> > flash->mtd._write = mchp23k256_write;
> >
> > + flash->mtd.erasesize = PAGE_SIZE;
> > + while (flash->mtd.size & (flash->mtd.erasesize - 1))
> > + flash->mtd.erasesize >>= 1;
> > +
>
> Can we fix allocate_partition() to properly handle the
> master->erasesize == 0 case instead of doing that?
Is everything actually ready for the eraseblock size to be 0? That would
seem surprising to many applications, I would think. Can you, for
instance, even use UBI on such a device?
BTW, I feel like this check is a little more natural to do with
'mtd->flags & MTD_NO_ERASE', rather than checking the (apparently
meaningless) erasesize.
(I realize there's a later version of these patches, but I figured I'd
put my comments where the suggestion was brought up.)
Brian
Le Thu, 1 Jun 2017 11:43:40 -0700,
Brian Norris <[email protected]> a écrit :
> On Wed, May 17, 2017 at 05:29:11PM +0200, Boris Brezillon wrote:
> > Hi Chris,
> >
> > On Wed, 17 May 2017 17:39:07 +1200
> > Chris Packham <[email protected]> wrote:
> >
> > > Setting the of_node for the mtd device allows the generic mtd code to
> > > setup the partitions. Additionally we must specify a non-zero erasesize
> > > for the partitions to be writeable.
> > >
> > > Signed-off-by: Chris Packham <[email protected]>
> > > ---
> > > drivers/mtd/devices/mchp23k256.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/mtd/devices/mchp23k256.c b/drivers/mtd/devices/mchp23k256.c
> > > index 2542f5b8b63f..02c6b9dcbd3e 100644
> > > --- a/drivers/mtd/devices/mchp23k256.c
> > > +++ b/drivers/mtd/devices/mchp23k256.c
> > > @@ -143,6 +143,7 @@ static int mchp23k256_probe(struct spi_device *spi)
> > >
> > > data = dev_get_platdata(&spi->dev);
> > >
> > > + mtd_set_of_node(&flash->mtd, spi->dev.of_node);
> > > flash->mtd.dev.parent = &spi->dev;
> > > flash->mtd.type = MTD_RAM;
> > > flash->mtd.flags = MTD_CAP_RAM;
> > > @@ -151,6 +152,10 @@ static int mchp23k256_probe(struct spi_device *spi)
> > > flash->mtd._read = mchp23k256_read;
> > > flash->mtd._write = mchp23k256_write;
> > >
> > > + flash->mtd.erasesize = PAGE_SIZE;
> > > + while (flash->mtd.size & (flash->mtd.erasesize - 1))
> > > + flash->mtd.erasesize >>= 1;
> > > +
> >
> > Can we fix allocate_partition() to properly handle the
> > master->erasesize == 0 case instead of doing that?
>
> Is everything actually ready for the eraseblock size to be 0? That would
> seem surprising to many applications, I would think. Can you, for
> instance, even use UBI on such a device?
Well, I think it's already broken. AFAICT this driver does not
implement ->_erase(), and mtd_erase() does not check if MTD_NO_ERASE is
set before calling mtd->_erase(), neither UBI does before calling
mtd_erase().
Between a NULL pointer exception and a div-by-zero exception, I can't
decide what is better :-).
IMO, we'd better add a check in UBI to refuse to attach a device with
MTD_NO_ERASE or mtd->erasesize == 0, and fix other places that don't
check erasesize value instead of putting a fake erasesize and using a
dummy ->_erase() implementation for those devices that simply can't be
erased.
We should also probably complain with -ENOTSUPP when someone calls
mtd_erase() on a device with MTD_NO_ERASE and add more checks in the
add_mtd_device() to detect drivers that don't have MTD_NO_ERASE set
and do not implement ->_erase() or leave ->erasesize to 0.
>
> BTW, I feel like this check is a little more natural to do with
> 'mtd->flags & MTD_NO_ERASE', rather than checking the (apparently
> meaningless) erasesize.
Fair enough.
On Thu, Jun 01, 2017 at 10:47:12PM +0200, Boris Brezillon wrote:
> Le Thu, 1 Jun 2017 11:43:40 -0700,
> Brian Norris <[email protected]> a ?crit :
> > On Wed, May 17, 2017 at 05:29:11PM +0200, Boris Brezillon wrote:
> > > On Wed, 17 May 2017 17:39:07 +1200
> > > Chris Packham <[email protected]> wrote:
> > > > @@ -151,6 +152,10 @@ static int mchp23k256_probe(struct spi_device *spi)
> > > > flash->mtd._read = mchp23k256_read;
> > > > flash->mtd._write = mchp23k256_write;
> > > >
> > > > + flash->mtd.erasesize = PAGE_SIZE;
> > > > + while (flash->mtd.size & (flash->mtd.erasesize - 1))
> > > > + flash->mtd.erasesize >>= 1;
> > > > +
> > >
> > > Can we fix allocate_partition() to properly handle the
> > > master->erasesize == 0 case instead of doing that?
> >
> > Is everything actually ready for the eraseblock size to be 0? That would
> > seem surprising to many applications, I would think. Can you, for
> > instance, even use UBI on such a device?
>
> Well, I think it's already broken. AFAICT this driver does not
> implement ->_erase(), and mtd_erase() does not check if MTD_NO_ERASE is
> set before calling mtd->_erase(), neither UBI does before calling
> mtd_erase().
Sure.
> Between a NULL pointer exception and a div-by-zero exception, I can't
> decide what is better :-).
Well, there are other potential problems than that. What if someone was
iterating over the device size, by increments of erasesize? Infinite
loop! Or what about anything that might have assumed
'writesize < erasesize'?
I'm mostly thinking out loud, because I'm not sure there's a really good
way to handle this, other than stop making those assumptions.
(A *possible* solution would be to have MTD enforce a fake erasesize for
NO_ERASE flash, instead of making drivers do it, like Chris was trying.
But I'm not sure that's a good one.)
> IMO, we'd better add a check in UBI to refuse to attach a device with
> MTD_NO_ERASE or mtd->erasesize == 0, and fix other places that don't
> check erasesize value instead of putting a fake erasesize and using a
> dummy ->_erase() implementation for those devices that simply can't be
> erased.
That's probably a good idea.
> We should also probably complain with -ENOTSUPP when someone calls
> mtd_erase() on a device with MTD_NO_ERASE and add more checks in the
> add_mtd_device() to detect drivers that don't have MTD_NO_ERASE set
> and do not implement ->_erase() or leave ->erasesize to 0.
Yep.
> > BTW, I feel like this check is a little more natural to do with
> > 'mtd->flags & MTD_NO_ERASE', rather than checking the (apparently
> > meaningless) erasesize.
>
> Fair enough.
OK, well I'll take another look at v4, but that might be my only
criticism then.
Overall though, a "NO_ERASE" MTD makes me wonder why it's an MTD in the
first place. I guess we're kinda the wild west of things that don't fit
into the block subsystem...
Brian
On Thu, 1 Jun 2017 15:01:56 -0700
Brian Norris <[email protected]> wrote:
> On Thu, Jun 01, 2017 at 10:47:12PM +0200, Boris Brezillon wrote:
> > Le Thu, 1 Jun 2017 11:43:40 -0700,
> > Brian Norris <[email protected]> a écrit :
> > > On Wed, May 17, 2017 at 05:29:11PM +0200, Boris Brezillon wrote:
> > > > On Wed, 17 May 2017 17:39:07 +1200
> > > > Chris Packham <[email protected]> wrote:
> > > > > @@ -151,6 +152,10 @@ static int mchp23k256_probe(struct spi_device *spi)
> > > > > flash->mtd._read = mchp23k256_read;
> > > > > flash->mtd._write = mchp23k256_write;
> > > > >
> > > > > + flash->mtd.erasesize = PAGE_SIZE;
> > > > > + while (flash->mtd.size & (flash->mtd.erasesize - 1))
> > > > > + flash->mtd.erasesize >>= 1;
> > > > > +
> > > >
> > > > Can we fix allocate_partition() to properly handle the
> > > > master->erasesize == 0 case instead of doing that?
> > >
> > > Is everything actually ready for the eraseblock size to be 0? That would
> > > seem surprising to many applications, I would think. Can you, for
> > > instance, even use UBI on such a device?
> >
> > Well, I think it's already broken. AFAICT this driver does not
> > implement ->_erase(), and mtd_erase() does not check if MTD_NO_ERASE is
> > set before calling mtd->_erase(), neither UBI does before calling
> > mtd_erase().
>
> Sure.
>
> > Between a NULL pointer exception and a div-by-zero exception, I can't
> > decide what is better :-).
>
> Well, there are other potential problems than that. What if someone was
> iterating over the device size, by increments of erasesize? Infinite
> loop! Or what about anything that might have assumed
> 'writesize < erasesize'?
Absolutely, div-by-zero is not the only problem we can face.
>
> I'm mostly thinking out loud, because I'm not sure there's a really good
> way to handle this, other than stop making those assumptions.
>
> (A *possible* solution would be to have MTD enforce a fake erasesize for
> NO_ERASE flash, instead of making drivers do it, like Chris was trying.
> But I'm not sure that's a good one.)
Actually, by doing that we keep encouraging people to not do the right
thing :-).
>
> > IMO, we'd better add a check in UBI to refuse to attach a device with
> > MTD_NO_ERASE or mtd->erasesize == 0, and fix other places that don't
> > check erasesize value instead of putting a fake erasesize and using a
> > dummy ->_erase() implementation for those devices that simply can't be
> > erased.
>
> That's probably a good idea.
I'll try to post patches soon, but they definitely won't fix all
problems. For example, we just can't catch the infinite loop issue you
are mentioning above.
>
> > We should also probably complain with -ENOTSUPP when someone calls
> > mtd_erase() on a device with MTD_NO_ERASE and add more checks in the
> > add_mtd_device() to detect drivers that don't have MTD_NO_ERASE set
> > and do not implement ->_erase() or leave ->erasesize to 0.
>
> Yep.
>
> > > BTW, I feel like this check is a little more natural to do with
> > > 'mtd->flags & MTD_NO_ERASE', rather than checking the (apparently
> > > meaningless) erasesize.
> >
> > Fair enough.
>
> OK, well I'll take another look at v4, but that might be my only
> criticism then.
>
> Overall though, a "NO_ERASE" MTD makes me wonder why it's an MTD in the
> first place. I guess we're kinda the wild west of things that don't fit
> into the block subsystem...
Yes, that's exactly what the MTD subsystem is: a place where you can
put drivers for memory devices that are not block devices :-). Don't
know if it's a good thing or not, but it definitely makes MTD users
life harder.
BTW, MTD_NO_ERASE is not the only problem we have with UBI or JFFS2.
Are we guaranteed that an erase operation fills an eraseblock with
ones? Don't we have mem technologies that are filling them with zeros?
Note that mtdram is artificially setting the mem-region to 0xff in its
dummy erase operation, so maybe it's a implicit rule that ->_erase() is
supposed to fill eraseblocks with 0xff.
On Fri, Jun 02, 2017 at 11:04:06AM +0200, Boris Brezillon wrote:
> BTW, MTD_NO_ERASE is not the only problem we have with UBI or JFFS2.
> Are we guaranteed that an erase operation fills an eraseblock with
> ones? Don't we have mem technologies that are filling them with zeros?
> Note that mtdram is artificially setting the mem-region to 0xff in its
> dummy erase operation, so maybe it's a implicit rule that ->_erase() is
> supposed to fill eraseblocks with 0xff.
I've wondered about the general assumption. But mtdram isn't really a
good example, because it clearly calls itself a "test mtd device". So it
makes sense it would emulate common MTDs (i.e., flash memory).
Brian