[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Jagan Teki]

On Tue, Nov 27, 2018 at 6:06 PM Boris Brezillon
<boris.brezillon at bootlin.com> wrote:
>
> On Mon, 26 Nov 2018 14:25:44 +0100
> Miquel Raynal <miquel.raynal at bootlin.com> wrote:
>
> > Hi Jagan,
> >
> > Jagan Teki <jagan at openedev.com> wrote on Mon, 26 Nov 2018 18:35:01
> > +0530:
> >
> > > On 26/11/18 6:12 PM, Boris Brezillon wrote:
> > > > On Mon, 26 Nov 2018 13:37:46 +0100
> > > > Boris Brezillon <boris.brezillon at bootlin.com> wrote:
> > > >
> > > >> On Mon, 26 Nov 2018 16:42:48 +0530
> > > >> Jagan Teki <jagan at openedev.com> wrote:
> > > >>
> > > >>> On 26/11/18 2:12 PM, Boris Brezillon wrote:
> > > >>>> Hi Jagan,
> > > >>>>
> > > >>>> On Thu, 22 Nov 2018 09:40:56 +0100
> > > >>>> Boris Brezillon <boris.brezillon at bootlin.com> wrote:
> > > >>>>      >>>>>>> +       /*
> > > >>>>>>> +        * Setting mtd->priv to NULL is the best we can do. Thanks to that,
> > > >>>>>>> +        * the MTD layer can still call mtd hooks without risking a
> > > >>>>>>> +        * use-after-free bug. Still, things should be fixed to prevent the
> > > >>>>>>> +        * spi_flash object from being destroyed when del_mtd_device() fails.
> > > >>>>>>> +        */
> > > >>>>>>> +       sf_mtd_info.priv = NULL;
> > > >>>>>>> +       printf("Failed to unregister MTD %s and the spi_flash object is going away: you're in deep trouble!",
> > > >>>>>>> +              sf_mtd_info.name);
> > > >>>>>>
> > > >>>>>> Why do we need this print?
> > > >>>>>
> > > >>>>> Yes we do, just to keep the user informed that something bad happened
> > > >>>>> and its spi-flash is no longer usable (at least through the MTD layer).
> > > >>>>>     >>>>>> can't we do the same thing in MTD core
> > > >>>>>> itself, so-that it can be generic for all flash objects.
> > > >>>>>
> > > >>>>> del_mtd_device() can fail, so it's the caller responsibility to decide
> > > >>>>> what to do when that happens. Some users will propagate the error to
> > > >>>>> the upper layer and maybe cancel the device removal (AFAICT,
> > > >>>>> driver->remove() can return an error, not sure what happens in this
> > > >>>>> case though). For others, like spi-flash, the device will go away, and
> > > >>>>> all subsequent accesses will fail.
> > > >>>>
> > > >>>> I'm about to send a new version fixing the problem I mentioned in patch
> > > >>>> 3, but before doing that, I'd like to know if my answer convinced you or
> > > >>>> if you'd still prefer this message to go away (or be placed in
> > > >>>> mtdcore/mtdpart.c).
> > > >>>
> > > >>>
> > > >>> I'm thinking of having the message still in MTD by showing which
> > > >>> interface it would belongs, along with the details.
> > > >>
> > > >> Then we'd need something less
> > > >
> > > > Sorry, I inadvertently hit the send button :-/.
> > > >
> > > > So, I was about to say that we need something less worrisome than the
> > > > message I added in the SF layer if we move it to the MTD layer because
> > > > some drivers might actually do the right thing when del_mtd_device()
> > > > returns an error. I keep thinking that putting an error message in
> > > > mtdcore.c is not the right thing to do, but if you insist, I'll add
> > > > one (maybe a debug()). In any case I'd like to keep the one we have
> > > > here, because in this specific case, there's simply nothing we can do
> > > > when MTD dev removal fails.
> > > >
> > >
> > > Look like other flashes were deleting only via mtd_partitions. how would they know and does it not need for them to print the same information?
> >
> > Do you see that sf does not use MTD in a consistent manner? The whole
> > point of this commit is to handle sf's lightnesses in a "more robust"
> > manner. This warning belongs to sf and not to the mtdcore, because
> > as stated in the comment above, we should "prevent the spi_flash
> > object from being destroyed when del_mtd_device() fails". If you don't
> > want such warning in sf, I think the best thing to do is to fix the
> > root issue.
>
> Jagan, are you okay with this suggestion (add a debug() message in the
> del_mtd_device() path and keep the one we already have in sf_mtd.c)?

Please proceed.