From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Date: Mon, 26 Nov 2018 14:25:44 +0100 Subject: [U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust In-Reply-To: <953f25d7-1e42-5d73-ad4d-b7c3f18f167a@openedev.com> References: <20181119205955.32042-1-boris.brezillon@bootlin.com> <20181119205955.32042-12-boris.brezillon@bootlin.com> <20181122094056.21a75afe@bbrezillon> <20181126094238.3bf4a76a@bbrezillon> <20181126133746.2b4d1fe2@bbrezillon> <20181126134223.719c1767@bbrezillon> <953f25d7-1e42-5d73-ad4d-b7c3f18f167a@openedev.com> Message-ID: <20181126142544.1272cd65@xps13> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: u-boot@lists.denx.de Hi Jagan, Jagan Teki 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 wrote: > >=20 > >> On Mon, 26 Nov 2018 16:42:48 +0530 > >> Jagan Teki wrote: > >> > >>> On 26/11/18 2:12 PM, Boris Brezillon wrote: > >>>> Hi Jagan, > >>>> > >>>> On Thu, 22 Nov 2018 09:40:56 +0100 > >>>> Boris Brezillon wrote: > >>>> >>>>>>> + /* > >>>>>>> + * Setting mtd->priv to NULL is the best we can do. Thank= s to that, > >>>>>>> + * the MTD layer can still call mtd hooks without risking= a > >>>>>>> + * use-after-free bug. Still, things should be fixed to p= revent the > >>>>>>> + * spi_flash object from being destroyed when del_mtd_dev= ice() fails. > >>>>>>> + */ > >>>>>>> + sf_mtd_info.priv =3D NULL; > >>>>>>> + printf("Failed to unregister MTD %s and the spi_flash obj= ect 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 happen= ed > >>>>> and its spi-flash is no longer usable (at least through the MTD lay= er). > >>>>> >>>>>> 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 dec= ide > >>>>> 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 pa= tch > >>>> 3, but before doing that, I'd like to know if my answer convinced yo= u 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 > >=20 > > Sorry, I inadvertently hit the send button :-/. > >=20 > > 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. > >=20 >=20 > 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. Thanks, Miqu=C3=A8l