[PATCH 0/2] mfd: twlx030: i2c remove callback cleanup

[PATCH 0/2] mfd: twlx030: i2c remove callback cleanup

[Uwe Kleine-König]

Hello,

the remove paths of the twl4030 chip can fail and then returns an error
code in twl_remove() early. This isn't a good thing, because the device
will still go away with some resources not freed.
For the twl6030 this cannot happen, and the first patch is just a small
cleanup. For the twl4030 the situation is improved a bit: When the
failure happens, the dummy slave devices are removed now.

Note that twl4030_exit_irq() is incomplete. The irq isn't freed and
maybe some more cleanup is missing which might boom if an irq triggers
after the device is removed. Not sure that twl6030_exit_irq() is better
in this regard.

I noticed this issue because I work on making i2c_driver::remove return
void as returning a value != 0 there is almost always an error attached
to wrong expectations.

Best regards
Uwe

Uwe Kleine-König (2):
  mfd: twl6030: Make twl6030_exit_irq() return void
  mfd: twl4030: Make twl4030_exit_irq() return void

 drivers/mfd/twl-core.c    | 8 ++------
 drivers/mfd/twl-core.h    | 4 ++--
 drivers/mfd/twl4030-irq.c | 7 ++-----
 drivers/mfd/twl6030-irq.c | 3 +--
 4 files changed, 7 insertions(+), 15 deletions(-)

base-commit: 455e73a07f6e288b0061dfcf4fcf54fa9fe06458
-- 
2.34.1

[PATCH 1/2] mfd: twl6030: Make twl6030_exit_irq() return void

[Uwe Kleine-König]

This function returns 0 unconditionally, so there is no benefit in
returning a value at all and make the caller do error checking.

Also the caller (twl_remove()) cannot do anything sensible with an error
code. Passing it up the call stack isn't a good option because the i2c core
ignores error codes (apart from emitting an error message).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mfd/twl-core.c    | 4 ++--
 drivers/mfd/twl-core.h    | 2 +-
 drivers/mfd/twl6030-irq.c | 3 +--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 289b556dede2..d4194faf1cc3 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -1036,12 +1036,12 @@ static void clocks_init(struct device *dev,
 static int twl_remove(struct i2c_client *client)
 {
 	unsigned i, num_slaves;
-	int status;
+	int status = 0;
 
 	if (twl_class_is_4030())
 		status = twl4030_exit_irq();
 	else
-		status = twl6030_exit_irq();
+		twl6030_exit_irq();
 
 	if (status < 0)
 		return status;
diff --git a/drivers/mfd/twl-core.h b/drivers/mfd/twl-core.h
index 6f96c2009a9f..1b916d2e8752 100644
--- a/drivers/mfd/twl-core.h
+++ b/drivers/mfd/twl-core.h
@@ -3,7 +3,7 @@
 #define __TWL_CORE_H__
 
 extern int twl6030_init_irq(struct device *dev, int irq_num);
-extern int twl6030_exit_irq(void);
+extern void twl6030_exit_irq(void);
 extern int twl4030_init_irq(struct device *dev, int irq_num);
 extern int twl4030_exit_irq(void);
 extern int twl4030_init_chip_irq(const char *chip);
diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index 97af6c2a6007..3c03681c124c 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -438,7 +438,7 @@ int twl6030_init_irq(struct device *dev, int irq_num)
 	return status;
 }
 
-int twl6030_exit_irq(void)
+void twl6030_exit_irq(void)
 {
 	if (twl6030_irq && twl6030_irq->twl_irq) {
 		unregister_pm_notifier(&twl6030_irq->pm_nb);
@@ -453,6 +453,5 @@ int twl6030_exit_irq(void)
 		 * in this module.
 		 */
 	}
-	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 1/2] mfd: twl6030: Make twl6030_exit_irq() return void

[Lee Jones]

On Thu, 13 Jan 2022, Uwe Kleine-König wrote:

> This function returns 0 unconditionally, so there is no benefit in
> returning a value at all and make the caller do error checking.
> 
> Also the caller (twl_remove()) cannot do anything sensible with an error
> code. Passing it up the call stack isn't a good option because the i2c core
> ignores error codes (apart from emitting an error message).
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/mfd/twl-core.c    | 4 ++--
>  drivers/mfd/twl-core.h    | 2 +-
>  drivers/mfd/twl6030-irq.c | 3 +--
>  3 files changed, 4 insertions(+), 5 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] mfd: twl6030: Make twl6030_exit_irq() return void

[Uwe Kleine-König]

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

Hello Lee,

On Thu, Apr 28, 2022 at 05:24:21PM +0100, Lee Jones wrote:
> On Thu, 13 Jan 2022, Uwe Kleine-König wrote:
> 
> > This function returns 0 unconditionally, so there is no benefit in
> > returning a value at all and make the caller do error checking.
> > 
> > Also the caller (twl_remove()) cannot do anything sensible with an error
> > code. Passing it up the call stack isn't a good option because the i2c core
> > ignores error codes (apart from emitting an error message).
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/mfd/twl-core.c    | 4 ++--
> >  drivers/mfd/twl-core.h    | 2 +-
> >  drivers/mfd/twl6030-irq.c | 3 +--
> >  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> Applied, thanks.

I would have expected these to appear in next since you wrote to have
applied this series. But they have not though your claim to have applied
them is over three weeks old now?! :-\

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

Re: [PATCH 1/2] mfd: twl6030: Make twl6030_exit_irq() return void

[Lee Jones]

On Mon, 23 May 2022, Uwe Kleine-König wrote:

> Hello Lee,
> 
> On Thu, Apr 28, 2022 at 05:24:21PM +0100, Lee Jones wrote:
> > On Thu, 13 Jan 2022, Uwe Kleine-König wrote:
> > 
> > > This function returns 0 unconditionally, so there is no benefit in
> > > returning a value at all and make the caller do error checking.
> > > 
> > > Also the caller (twl_remove()) cannot do anything sensible with an error
> > > code. Passing it up the call stack isn't a good option because the i2c core
> > > ignores error codes (apart from emitting an error message).
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/mfd/twl-core.c    | 4 ++--
> > >  drivers/mfd/twl-core.h    | 2 +-
> > >  drivers/mfd/twl6030-irq.c | 3 +--
> > >  3 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > Applied, thanks.
> 
> I would have expected these to appear in next since you wrote to have
> applied this series. But they have not though your claim to have applied
> them is over three weeks old now?! :-\

Don't worry.  They're both applied and will be in v5.19.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/2] mfd: twl4030: Make twl4030_exit_irq() return void

[Uwe Kleine-König]

If twl4030_exit_irq() returns an error, the effect is that the caller
(twl_remove()) forwards the error to the i2c core without unregistering
its dummy slave devices. This only makes the i2c core emit another
error message and then it still removes the device.

In this situation it doesn't make sense to abort the remove cleanup and not
unregister the slave devices. So do that. Then return value is actually
unused and twl4030_exit_irq() can better be changed to return no value at
all.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mfd/twl-core.c    | 6 +-----
 drivers/mfd/twl-core.h    | 2 +-
 drivers/mfd/twl4030-irq.c | 7 ++-----
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index d4194faf1cc3..bd6659cf3bc0 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -1036,16 +1036,12 @@ static void clocks_init(struct device *dev,
 static int twl_remove(struct i2c_client *client)
 {
 	unsigned i, num_slaves;
-	int status = 0;
 
 	if (twl_class_is_4030())
-		status = twl4030_exit_irq();
+		twl4030_exit_irq();
 	else
 		twl6030_exit_irq();
 
-	if (status < 0)
-		return status;
-
 	num_slaves = twl_get_num_slaves();
 	for (i = 0; i < num_slaves; i++) {
 		struct twl_client	*twl = &twl_priv->twl_modules[i];
diff --git a/drivers/mfd/twl-core.h b/drivers/mfd/twl-core.h
index 1b916d2e8752..b4bf6a233bd0 100644
--- a/drivers/mfd/twl-core.h
+++ b/drivers/mfd/twl-core.h
@@ -5,7 +5,7 @@
 extern int twl6030_init_irq(struct device *dev, int irq_num);
 extern void twl6030_exit_irq(void);
 extern int twl4030_init_irq(struct device *dev, int irq_num);
-extern int twl4030_exit_irq(void);
+extern void twl4030_exit_irq(void);
 extern int twl4030_init_chip_irq(const char *chip);
 
 #endif /*  __TWL_CORE_H__ */
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index ab417438d1fa..4f576f0160a9 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -753,14 +753,11 @@ int twl4030_init_irq(struct device *dev, int irq_num)
 	return status;
 }
 
-int twl4030_exit_irq(void)
+void twl4030_exit_irq(void)
 {
 	/* FIXME undo twl_init_irq() */
-	if (twl4030_irq_base) {
+	if (twl4030_irq_base)
 		pr_err("twl4030: can't yet clean up IRQs?\n");
-		return -ENOSYS;
-	}
-	return 0;
 }
 
 int twl4030_init_chip_irq(const char *chip)
-- 
2.34.1

Re: [PATCH 2/2] mfd: twl4030: Make twl4030_exit_irq() return void

[Lee Jones]

On Thu, 13 Jan 2022, Uwe Kleine-König wrote:

> If twl4030_exit_irq() returns an error, the effect is that the caller
> (twl_remove()) forwards the error to the i2c core without unregistering
> its dummy slave devices. This only makes the i2c core emit another
> error message and then it still removes the device.
> 
> In this situation it doesn't make sense to abort the remove cleanup and not
> unregister the slave devices. So do that. Then return value is actually
> unused and twl4030_exit_irq() can better be changed to return no value at
> all.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/mfd/twl-core.c    | 6 +-----
>  drivers/mfd/twl-core.h    | 2 +-
>  drivers/mfd/twl4030-irq.c | 7 ++-----
>  3 files changed, 4 insertions(+), 11 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 0/2] mfd: twlx030: i2c remove callback cleanup

[Uwe Kleine-König]

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

On Thu, Jan 13, 2022 at 11:14:28AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> the remove paths of the twl4030 chip can fail and then returns an error
> code in twl_remove() early. This isn't a good thing, because the device
> will still go away with some resources not freed.
> For the twl6030 this cannot happen, and the first patch is just a small
> cleanup. For the twl4030 the situation is improved a bit: When the
> failure happens, the dummy slave devices are removed now.
> 
> Note that twl4030_exit_irq() is incomplete. The irq isn't freed and
> maybe some more cleanup is missing which might boom if an irq triggers
> after the device is removed. Not sure that twl6030_exit_irq() is better
> in this regard.
> 
> I noticed this issue because I work on making i2c_driver::remove return
> void as returning a value != 0 there is almost always an error attached
> to wrong expectations.

It's one merge window ago now that I sent these two patches and didn't
get any feedback. Did this series fell through the cracks?

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

Re: [PATCH 0/2] mfd: twlx030: i2c remove callback cleanup

[Lee Jones]

On Thu, 31 Mar 2022, Uwe Kleine-König wrote:

> On Thu, Jan 13, 2022 at 11:14:28AM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > the remove paths of the twl4030 chip can fail and then returns an error
> > code in twl_remove() early. This isn't a good thing, because the device
> > will still go away with some resources not freed.
> > For the twl6030 this cannot happen, and the first patch is just a small
> > cleanup. For the twl4030 the situation is improved a bit: When the
> > failure happens, the dummy slave devices are removed now.
> > 
> > Note that twl4030_exit_irq() is incomplete. The irq isn't freed and
> > maybe some more cleanup is missing which might boom if an irq triggers
> > after the device is removed. Not sure that twl6030_exit_irq() is better
> > in this regard.
> > 
> > I noticed this issue because I work on making i2c_driver::remove return
> > void as returning a value != 0 there is almost always an error attached
> > to wrong expectations.
> 
> It's one merge window ago now that I sent these two patches and didn't
> get any feedback. Did this series fell through the cracks?

Yes they did.

Feel free to submit [RESEND]s any time after 2 weeks with no reply.

They are now on my TODO list.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog