| Message ID | 20201126072331.1737632-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Commit | 71637c620a826434ca6f888b0364a036faa27ffa |
| Headers | show |
| Series | [1/2] i2c: Warn when device removing fails | expand |
On Thu, Nov 26, 2020 at 08:23:30AM +0100, Uwe Kleine-König wrote: > The driver core ignores the return value of struct bus_type::remove. So > warn if there is an error that went unnoticed before and return 0 > unconditionally in i2c_device_remove(). I wondered about the "return 0" part... > > This prepares changing struct bus_type::remove to return void. ... until I read this. You are working on that? > if (driver->remove) { > + int status = 0; No need to initialize to 0, or? > + > dev_dbg(dev, "remove\n"); > + > status = driver->remove(client); > + if (status) > + dev_warn(dev, "remove failed (%pe), will be ignored\n", ERR_PTR(status)); The rest and patch 2 look good.
Hey Wolfram, On Thu, Dec 10, 2020 at 09:10:44PM +0100, Wolfram Sang wrote: > On Thu, Nov 26, 2020 at 08:23:30AM +0100, Uwe Kleine-König wrote: > > The driver core ignores the return value of struct bus_type::remove. So > > warn if there is an error that went unnoticed before and return 0 > > unconditionally in i2c_device_remove(). > > I wondered about the "return 0" part... > > > > > This prepares changing struct bus_type::remove to return void. > > ... until I read this. You are working on that? Yes, I'm not paid for it, but it serves as an idle cleanup task for me. Greg even assists, see 8142a46c50d2dd8160c42284e1044eed3bec0d18. :-) > > > if (driver->remove) { > > + int status = 0; > > No need to initialize to 0, or? Right, this comes straight from: - int status = 0; from the current version of i2c_device_remove, where it was still relevant. I don't feel strong here, and if you do I can resend or you can fixup while applying. > > + > > dev_dbg(dev, "remove\n"); > > + > > status = driver->remove(client); > > + if (status) > > + dev_warn(dev, "remove failed (%pe), will be ignored\n", ERR_PTR(status)); > > The rest and patch 2 look good. Great. Liebe Grüße aus Freiburg! Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe, > Yes, I'm not paid for it, but it serves as an idle cleanup task for me. You have idle time? ;) > > No need to initialize to 0, or? > > Right, this comes straight from: > - int status = 0; > > from the current version of i2c_device_remove, where it was still > relevant. I don't feel strong here, and if you do I can resend or you > can fixup while applying. I will fix it. Also, I will add a comment mentioning this as cleanup preparation. Thanks and have a nice weekend! Wolfram
On Thu, Nov 26, 2020 at 08:23:30AM +0100, Uwe Kleine-König wrote: > The driver core ignores the return value of struct bus_type::remove. So > warn if there is an error that went unnoticed before and return 0 > unconditionally in i2c_device_remove(). > > This prepares changing struct bus_type::remove to return void. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Applied to for-next, thanks!
On Thu, Nov 26, 2020 at 08:23:31AM +0100, Uwe Kleine-König wrote: > A driver remove callback is only called if the device was bound before. > So it's sure that both dev and dev->driver are valid and dev is an i2c > device. If the check fails something louder than "return 0" might be > appropriate because the problem is grave (something like memory > corruption), otherwise the check is useless. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Applied to for-next, thanks!
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 573b5da145d1..86e43016ff85 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -551,15 +551,19 @@ static int i2c_device_remove(struct device *dev) { struct i2c_client *client = i2c_verify_client(dev); struct i2c_driver *driver; - int status = 0; if (!client || !dev->driver) return 0; driver = to_i2c_driver(dev->driver); if (driver->remove) { + int status = 0; + dev_dbg(dev, "remove\n"); + status = driver->remove(client); + if (status) + dev_warn(dev, "remove failed (%pe), will be ignored\n", ERR_PTR(status)); } dev_pm_domain_detach(&client->dev, true); @@ -571,7 +575,7 @@ static int i2c_device_remove(struct device *dev) if (client->flags & I2C_CLIENT_HOST_NOTIFY) pm_runtime_put(&client->adapter->dev); - return status; + return 0; } static void i2c_device_shutdown(struct device *dev)
The driver core ignores the return value of struct bus_type::remove. So warn if there is an error that went unnoticed before and return 0 unconditionally in i2c_device_remove(). This prepares changing struct bus_type::remove to return void. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/i2c/i2c-core-base.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)