Re: [PATCH v2 13/20] mtd: dataflash: Warn about failure to unregister mtd device

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Uwe,

u.kleine-koenig@xxxxxxxxxxxxxx wrote on Wed, 13 Oct 2021 16:08:35 +0200:

> On Wed, Oct 13, 2021 at 02:44:29PM +0200, Miquel Raynal wrote:
> > Hi Uwe,
> > 
> > u.kleine-koenig@xxxxxxxxxxxxxx wrote on Tue, 12 Oct 2021 17:39:38 +0200:
> >   
> > > When an spi driver's remove function returns a non-zero error code  
> > 
> > Should we s/an spi/a SPI/?  
> 
> My (German) knowledge about the English Grammar claims that independent
> of how you spell SPI, it must be "an" because when I say it, it's
> [ɛspi:aɪ] (unless you call it [spaɪ]?)

I (personally) pronounce it [spaɪ] with my French background and it
looks wrong to my eyes to use "an" before SPI because of that, but this
is biased and possibly wrong as well so please keep it your way, it's
fine.

> In my eyes "spi" is right, because SPI is the protocol and "spi" is
> the kernel framework. But I don't feel strong here and you're already
> the second who suggests something similar.

I get it. Indeed I always use uppercase letters when I use acronyms
(such as SPI or MTD) and it's the first time I hear that the lowercase
letter refers to the framework more than the protocol, but TBH the
explanation kind of seduces me :)

> > > nothing happens apart from emitting a generic error message. Make
> > > this error message more device specific and return zero instead.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/mtd/devices/mtd_dataflash.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mtd/devices/mtd_dataflash.c
> > > b/drivers/mtd/devices/mtd_dataflash.c index
> > > 9802e265fca8..2691b6b79df8 100644 ---
> > > a/drivers/mtd/devices/mtd_dataflash.c +++
> > > b/drivers/mtd/devices/mtd_dataflash.c @@ -919,7 +919,10 @@ static
> > > int dataflash_remove(struct spi_device *spi) status =
> > > mtd_device_unregister(&flash->mtd); if (status == 0)
> > >  		kfree(flash);
> > > -	return status;
> > > +	else
> > > +		dev_warn(&spi->dev, "Failed to unregister mtd
> > > device (%pe)\n",
> > > +			 ERR_PTR(status));
> > > +	return 0;  
> > 
> > As part of a recent NAND cleanup series we ended up adding
> > WARN_ON() [1] to make it very clear that if this happens, it's not
> > expected at all (it was Boris' advice).
> > 
> > I don't think there is only one good solution but perhaps its best
> > to keep it sync'ed with the other drivers in MTD?  
> 
> Well, if WARN_ON or dev_warn is the right thing is subjective. Your
> subjective counts more as you're an MTD maintainer. Can rework
> accordingly for v3.

Then let's all use WARN_ON() for now.

Thanks,
Miquèl