From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jagan Teki Date: Mon, 26 Nov 2018 18:35:01 +0530 Subject: [U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust In-Reply-To: <20181126134223.719c1767@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> <20181126134223.719c1767@bbrezillon> Message-ID: <953f25d7-1e42-5d73-ad4d-b7c3f18f167a@openedev.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 26/11/18 6:12 PM, Boris Brezillon wrote: > 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. > 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?