[PATCH 1/2] i2c: Warn when device removing fails

[PATCH 1/2] i2c: Warn when device removing fails

[Uwe Kleine-König]

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(-)

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)
-- 
2.29.2

[PATCH 2/2] i2c: remove check that can never be true

[Uwe Kleine-König]

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>
---
 drivers/i2c/i2c-core-base.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 86e43016ff85..be995a95c4ac 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -549,12 +549,9 @@ static int i2c_device_probe(struct device *dev)
 
 static int i2c_device_remove(struct device *dev)
 {
-	struct i2c_client	*client = i2c_verify_client(dev);
+	struct i2c_client	*client = to_i2c_client(dev);
 	struct i2c_driver	*driver;
 
-	if (!client || !dev->driver)
-		return 0;
-
 	driver = to_i2c_driver(dev->driver);
 	if (driver->remove) {
 		int status = 0;
-- 
2.29.2

Re: [PATCH 1/2] i2c: Warn when device removing fails

[Wolfram Sang]

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

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.


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

Re: [PATCH 1/2] i2c: Warn when device removing fails

[Uwe Kleine-König]

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

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/ |

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

Re: [PATCH 1/2] i2c: Warn when device removing fails

[Wolfram Sang]

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

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


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

Re: [PATCH 1/2] i2c: Warn when device removing fails

[Wolfram Sang]

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

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!


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

Re: [PATCH 2/2] i2c: remove check that can never be true

[Wolfram Sang]

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

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!


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