Re: [PATCH v2 13/20] mtd: dataflash: Warn about failure to unregister mtd device - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <tudor.ambarus@microchip.com>,
	Richard Weinberger <richard@nod.at>,
	linux-spi@vger.kernel.org, Mark Brown <broonie@kernel.org>,
	linux-mtd@lists.infradead.org, kernel@pengutronix.de
Subject: Re: [PATCH v2 13/20] mtd: dataflash: Warn about failure to unregister mtd device
Date: Wed, 13 Oct 2021 16:08:35 +0200	[thread overview]
Message-ID: <20211013140835.olo2dxdno6zlom7n@pengutronix.de> (raw)
In-Reply-To: <20211013144429.65b294e5@xps13>

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

On Wed, Oct 13, 2021 at 02:44:29PM +0200, Miquel Raynal wrote:
> Hi Uwe,
> 
> u.kleine-koenig@pengutronix.de 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ɪ]?)

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.

> > 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@pengutronix.de>
> > ---
> >  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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]