From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Date: Mon, 26 Nov 2018 13:42:23 +0100 Subject: [U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust In-Reply-To: <20181126133746.2b4d1fe2@bbrezillon> References: <20181119205955.32042-1-boris.brezillon@bootlin.com> <20181119205955.32042-12-boris.brezillon@bootlin.com> <20181122094056.21a75afe@bbrezillon> <20181126094238.3bf4a76a@bbrezillon> <20181126133746.2b4d1fe2@bbrezillon> Message-ID: <20181126134223.719c1767@bbrezillon> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Mon, 26 Nov 2018 13:37:46 +0100 Boris Brezillon wrote: > 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. 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.