| Message ID | 20230314180020.2865734-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | hwspinlock: Convert to platform remove callback returning void | expand |
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
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
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?
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
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,