[PATCH 0/3] hwspinlock: Convert to platform remove callback returning void

[PATCH 0/3] hwspinlock: Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

this patch series adapts the platform drivers below drivers/hwspinlock to use
the .remove_new() callback. Compared to the traditional .remove() callback
.remove_new() returns no value. This is a good thing because the driver core
doesn't (and cannot) cope for errors during remove. The only effect of a
non-zero return value in .remove() is that the driver core emits a warning. The
device is removed anyhow and an early return from .remove() usually yields a
resource leak.

By changing the remove callback to return void driver authors cannot reasonably
assume any more that there is some kind of cleanup later.

The omap driver could return -EBUSY. This is first changed to return
zero to drop the duplicated error message. I assume this error path is
dangerous. For sure the platform device binding goes away and so
devm_platform_ioremap_resource is undone. So probably the user of the
hwspinlock that prevented its removal will soon access an unmapped
virtual address resulting in an oops. This is true with and without my
patch. IMHO hwspin_lock_unregister() shouldn't return an error code but
care that all users go away and then return void.

After this change the two platform_drivers can be trivially converted to
.remove_new().

Best regards
Uwe

Uwe Kleine-König (3):
  hwspinlock: omap: Emit only one error message for errors in .remove()
  hwspinlock: omap: Convert to platform remove callback returning void
  hwspinlock: u8500: Convert to platform remove callback returning void

 drivers/hwspinlock/omap_hwspinlock.c | 8 +++-----
 drivers/hwspinlock/u8500_hsem.c      | 6 ++----
 2 files changed, 5 insertions(+), 9 deletions(-)

base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
-- 
2.39.2

[PATCH 1/3] hwspinlock: omap: Emit only one error message for errors in .remove()

[Uwe Kleine-König]

If a remove callback of a platform driver returns a non-zero value, the
driver core emits an error message, otherwise ignores the value and
completes unbinding the device.

As omap_hwspinlock_remove() already emits an error message, suppress the
core's error message by returning zero.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/hwspinlock/omap_hwspinlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
index 1fb3a2550e29..93d787c78f3c 100644
--- a/drivers/hwspinlock/omap_hwspinlock.c
+++ b/drivers/hwspinlock/omap_hwspinlock.c
@@ -153,7 +153,7 @@ static int omap_hwspinlock_remove(struct platform_device *pdev)
 	ret = hwspin_lock_unregister(bank);
 	if (ret) {
 		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
-		return ret;
+		return 0;
 	}
 
 	pm_runtime_disable(&pdev->dev);
-- 
2.39.2

[PATCH 2/3] hwspinlock: omap: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is (mostly) ignored
and this typically results in resource leaks. To improve here there is a
quest to make the remove callback return void. In the first step of this
quest all drivers are converted to .remove_new() which already returns
void.

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/hwspinlock/omap_hwspinlock.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
index 93d787c78f3c..5082f496acdc 100644
--- a/drivers/hwspinlock/omap_hwspinlock.c
+++ b/drivers/hwspinlock/omap_hwspinlock.c
@@ -145,7 +145,7 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int omap_hwspinlock_remove(struct platform_device *pdev)
+static void omap_hwspinlock_remove(struct platform_device *pdev)
 {
 	struct hwspinlock_device *bank = platform_get_drvdata(pdev);
 	int ret;
@@ -153,12 +153,10 @@ static int omap_hwspinlock_remove(struct platform_device *pdev)
 	ret = hwspin_lock_unregister(bank);
 	if (ret) {
 		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
-		return 0;
+		return;
 	}
 
 	pm_runtime_disable(&pdev->dev);
-
-	return 0;
 }
 
 static const struct of_device_id omap_hwspinlock_of_match[] = {
@@ -171,7 +169,7 @@ MODULE_DEVICE_TABLE(of, omap_hwspinlock_of_match);
 
 static struct platform_driver omap_hwspinlock_driver = {
 	.probe		= omap_hwspinlock_probe,
-	.remove		= omap_hwspinlock_remove,
+	.remove_new	= omap_hwspinlock_remove,
 	.driver		= {
 		.name	= "omap_hwspinlock",
 		.of_match_table = of_match_ptr(omap_hwspinlock_of_match),
-- 
2.39.2

Re: [PATCH 0/3] hwspinlock: Convert to platform remove callback returning void

[Uwe Kleine-König]

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

On Tue, Mar 14, 2023 at 07:00:20PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> this patch series adapts the platform drivers below drivers/hwspinlock to use
> the .remove_new() callback. Compared to the traditional .remove() callback
> .remove_new() returns no value. This is a good thing because the driver core
> doesn't (and cannot) cope for errors during remove. The only effect of a
> non-zero return value in .remove() is that the driver core emits a warning. The
> device is removed anyhow and an early return from .remove() usually yields a
> resource leak.
> 
> By changing the remove callback to return void driver authors cannot reasonably
> assume any more that there is some kind of cleanup later.
> 
> The omap driver could return -EBUSY. This is first changed to return
> zero to drop the duplicated error message. I assume this error path is
> dangerous. For sure the platform device binding goes away and so
> devm_platform_ioremap_resource is undone. So probably the user of the
> hwspinlock that prevented its removal will soon access an unmapped
> virtual address resulting in an oops. This is true with and without my
> patch. IMHO hwspin_lock_unregister() shouldn't return an error code but
> care that all users go away and then return void.
> 
> After this change the two platform_drivers can be trivially converted to
> .remove_new().

Gentle ping!

Who is supposed to apply this series (or point out a good reason to not
do that)?

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/3] hwspinlock: Convert to platform remove callback returning void

[Uwe Kleine-König]

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

Hello,

On Wed, Apr 12, 2023 at 07:16:10PM +0200, Uwe Kleine-König wrote:
> On Tue, Mar 14, 2023 at 07:00:20PM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > this patch series adapts the platform drivers below drivers/hwspinlock to use
> > the .remove_new() callback. Compared to the traditional .remove() callback
> > .remove_new() returns no value. This is a good thing because the driver core
> > doesn't (and cannot) cope for errors during remove. The only effect of a
> > non-zero return value in .remove() is that the driver core emits a warning. The
> > device is removed anyhow and an early return from .remove() usually yields a
> > resource leak.
> > 
> > By changing the remove callback to return void driver authors cannot reasonably
> > assume any more that there is some kind of cleanup later.
> > 
> > The omap driver could return -EBUSY. This is first changed to return
> > zero to drop the duplicated error message. I assume this error path is
> > dangerous. For sure the platform device binding goes away and so
> > devm_platform_ioremap_resource is undone. So probably the user of the
> > hwspinlock that prevented its removal will soon access an unmapped
> > virtual address resulting in an oops. This is true with and without my
> > patch. IMHO hwspin_lock_unregister() shouldn't return an error code but
> > care that all users go away and then return void.
> > 
> > After this change the two platform_drivers can be trivially converted to
> > .remove_new().
> 
> Gentle ping!
> 
> Who is supposed to apply this series (or point out a good reason to not
> do that)?

Still no maintainer feedback on my series :-\ Would a resend help?

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/3] hwspinlock: Convert to platform remove callback returning void

[Baolin Wang]

On 5/30/2023 9:56 PM, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Apr 12, 2023 at 07:16:10PM +0200, Uwe Kleine-König wrote:
>> On Tue, Mar 14, 2023 at 07:00:20PM +0100, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> this patch series adapts the platform drivers below drivers/hwspinlock to use
>>> the .remove_new() callback. Compared to the traditional .remove() callback
>>> .remove_new() returns no value. This is a good thing because the driver core
>>> doesn't (and cannot) cope for errors during remove. The only effect of a
>>> non-zero return value in .remove() is that the driver core emits a warning. The
>>> device is removed anyhow and an early return from .remove() usually yields a
>>> resource leak.
>>>
>>> By changing the remove callback to return void driver authors cannot reasonably
>>> assume any more that there is some kind of cleanup later.
>>>
>>> The omap driver could return -EBUSY. This is first changed to return
>>> zero to drop the duplicated error message. I assume this error path is
>>> dangerous. For sure the platform device binding goes away and so
>>> devm_platform_ioremap_resource is undone. So probably the user of the
>>> hwspinlock that prevented its removal will soon access an unmapped
>>> virtual address resulting in an oops. This is true with and without my
>>> patch. IMHO hwspin_lock_unregister() shouldn't return an error code but
>>> care that all users go away and then return void.
>>>
>>> After this change the two platform_drivers can be trivially converted to
>>> .remove_new().
>>
>> Gentle ping!
>>
>> Who is supposed to apply this series (or point out a good reason to not
>> do that)?
> 
> Still no maintainer feedback on my series :-\ Would a resend help?

For the whole patchset,
Acked-by: Baolin Wang <baolin.wang@linux.alibaba.com>

I think Bjorn can help to apply this patchset, Bjorn?

Re: [PATCH 0/3] hwspinlock: Convert to platform remove callback returning void

[Uwe Kleine-König]

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

Hello,

On Wed, May 31, 2023 at 09:12:40AM +0800, Baolin Wang wrote:
> On 5/30/2023 9:56 PM, Uwe Kleine-König wrote:
> > On Wed, Apr 12, 2023 at 07:16:10PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Mar 14, 2023 at 07:00:20PM +0100, Uwe Kleine-König wrote:
> > > > Hello,
> > > > 
> > > > this patch series adapts the platform drivers below drivers/hwspinlock to use
> > > > the .remove_new() callback. Compared to the traditional .remove() callback
> > > > .remove_new() returns no value. This is a good thing because the driver core
> > > > doesn't (and cannot) cope for errors during remove. The only effect of a
> > > > non-zero return value in .remove() is that the driver core emits a warning. The
> > > > device is removed anyhow and an early return from .remove() usually yields a
> > > > resource leak.
> > > > 
> > > > By changing the remove callback to return void driver authors cannot reasonably
> > > > assume any more that there is some kind of cleanup later.
> > > > 
> > > > The omap driver could return -EBUSY. This is first changed to return
> > > > zero to drop the duplicated error message. I assume this error path is
> > > > dangerous. For sure the platform device binding goes away and so
> > > > devm_platform_ioremap_resource is undone. So probably the user of the
> > > > hwspinlock that prevented its removal will soon access an unmapped
> > > > virtual address resulting in an oops. This is true with and without my
> > > > patch. IMHO hwspin_lock_unregister() shouldn't return an error code but
> > > > care that all users go away and then return void.
> > > > 
> > > > After this change the two platform_drivers can be trivially converted to
> > > > .remove_new().
> > > 
> > > Gentle ping!
> > > 
> > > Who is supposed to apply this series (or point out a good reason to not
> > > do that)?
> > 
> > Still no maintainer feedback on my series :-\ Would a resend help?
> 
> For the whole patchset,
> Acked-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> 
> I think Bjorn can help to apply this patchset, Bjorn?

up to now he didn't. I guess it's to late now for v6.5-rc1, but can at
least someone pick it up for the next cycle?

It still applies fine to current Linus's master. With am -3 it also
still applies to current next. If a resend would help, please tell me
what I should choose as base.

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/3] hwspinlock: Convert to platform remove callback returning void

[Bjorn Andersson]

On Tue, 14 Mar 2023 19:00:20 +0100, Uwe Kleine-König wrote:
> this patch series adapts the platform drivers below drivers/hwspinlock to use
> the .remove_new() callback. Compared to the traditional .remove() callback
> .remove_new() returns no value. This is a good thing because the driver core
> doesn't (and cannot) cope for errors during remove. The only effect of a
> non-zero return value in .remove() is that the driver core emits a warning. The
> device is removed anyhow and an early return from .remove() usually yields a
> resource leak.
> 
> [...]

Applied, thanks!

[1/3] hwspinlock: omap: Emit only one error message for errors in .remove()
      commit: 72a3a509f992b6bd182b3380913fe7b4f801075f
[2/3] hwspinlock: omap: Convert to platform remove callback returning void
      commit: 4cf16b6b743e0bbe3128cf97a193ee37110d597b
[3/3] hwspinlock: u8500: Convert to platform remove callback returning void
      commit: 9519793bb6a731a3dd2453ad8515e8866e84c48e

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>