| Message ID | 20230922081208.26334-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] alarmtimer: Fix rebind failure | expand |
Hi all, Gentle ping. Are we happy with this patch as it fixes re-bind failure on RTC subsystem? Cheers, Biju > From: Biju Das <biju.das.jz@bp.renesas.com> > Sent: Friday, September 22, 2023 9:12 AM > Subject: [PATCH v2] alarmtimer: Fix rebind failure > > The resources allocated in alarmtimer_rtc_add_device() are not freed > leading to re-bind failure for the endpoint driver. Fix this issue by > adding alarmtimer_rtc_remove_device(). > > Fixes: c79108bd19a8 ("alarmtimer: Make alarmtimer platform device child of > RTC device") > Fixes: 7c94caca877b ("alarmtimer: Use wakeup source from alarmtimer > platform device") > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v1->v2: > * Add fixes tag. > * Replaced the variable rtc_pdev->alarmtimer_pdev > * Added the check rtcdev == rtc before unregistering the real alarmtimer. > Note: > This issue is found while adding irq support for built in RTC found on > Renesas PMIC RAA215300 device. This issue should present on all RTC > drivers which calls device_init_wakeup() in probe(). > --- > kernel/time/alarmtimer.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index > 8d9f13d847f0..04d67de8b1fe 100644 > --- a/kernel/time/alarmtimer.c > +++ b/kernel/time/alarmtimer.c > @@ -61,6 +61,7 @@ static DEFINE_SPINLOCK(freezer_delta_lock); > /* rtc timer and device for setting alarm wakeups at suspend */ > static struct rtc_timer rtctimer; > static struct rtc_device *rtcdev; > +static struct platform_device *alarmtimer_pdev; > static DEFINE_SPINLOCK(rtcdev_lock); > > /** > @@ -109,6 +110,7 @@ static int alarmtimer_rtc_add_device(struct device > *dev) > } > > rtcdev = rtc; > + alarmtimer_pdev = pdev; > /* hold a reference so it doesn't go away */ > get_device(dev); > pdev = NULL; > @@ -123,6 +125,22 @@ static int alarmtimer_rtc_add_device(struct device > *dev) > return ret; > } > > +static void alarmtimer_rtc_remove_device(struct device *dev) { > + struct rtc_device *rtc = to_rtc_device(dev); > + > + if (rtcdev == rtc) { > + module_put(rtc->owner); > + if (device_may_wakeup(rtc->dev.parent)) > + device_init_wakeup(&alarmtimer_pdev->dev, false); > + > + platform_device_unregister(alarmtimer_pdev); > + put_device(dev); > + alarmtimer_pdev = NULL; > + rtcdev = NULL; > + } > +} > + > static inline void alarmtimer_rtc_timer_init(void) { > rtc_timer_init(&rtctimer, NULL, NULL); @@ -130,6 +148,7 @@ static > inline void alarmtimer_rtc_timer_init(void) > > static struct class_interface alarmtimer_rtc_interface = { > .add_dev = &alarmtimer_rtc_add_device, > + .remove_dev = &alarmtimer_rtc_remove_device, > }; > > static int alarmtimer_rtc_interface_setup(void) > -- > 2.25.1
On Fri, Sep 22 2023 at 09:12, Biju Das wrote: > +static void alarmtimer_rtc_remove_device(struct device *dev) > +{ > + struct rtc_device *rtc = to_rtc_device(dev); > + > + if (rtcdev == rtc) { > + module_put(rtc->owner); > + if (device_may_wakeup(rtc->dev.parent)) > + device_init_wakeup(&alarmtimer_pdev->dev, false); > + > + platform_device_unregister(alarmtimer_pdev); > + put_device(dev); > + alarmtimer_pdev = NULL; > + rtcdev = NULL; > + } That's broken versus alarmtimer_get_rtcdev() and any user of it. Thanks, tglx
Hi Thomas Gleixner, > Subject: Re: [PATCH v2] alarmtimer: Fix rebind failure > > On Fri, Sep 22 2023 at 09:12, Biju Das wrote: > > +static void alarmtimer_rtc_remove_device(struct device *dev) { > > + struct rtc_device *rtc = to_rtc_device(dev); > > + > > + if (rtcdev == rtc) { > > + module_put(rtc->owner); > > + if (device_may_wakeup(rtc->dev.parent)) > > + device_init_wakeup(&alarmtimer_pdev->dev, false); > > + > > + platform_device_unregister(alarmtimer_pdev); > > + put_device(dev); > > + alarmtimer_pdev = NULL; > > + rtcdev = NULL; > > + } > > That's broken versus alarmtimer_get_rtcdev() and any user of it. You mean we should update[1] (charger-manager driver)as it is the one using alarmtimer_get_rtcdev()?? [1] https://elixir.bootlin.com/linux/v6.6-rc5/source/drivers/power/supply/charger-manager.c#L1447 Cheers, Biju
On Mon, Oct 09 2023 at 15:30, Biju Das wrote: >> Subject: Re: [PATCH v2] alarmtimer: Fix rebind failure >> >> On Fri, Sep 22 2023 at 09:12, Biju Das wrote: >> > +static void alarmtimer_rtc_remove_device(struct device *dev) { >> > + struct rtc_device *rtc = to_rtc_device(dev); >> > + >> > + if (rtcdev == rtc) { >> > + module_put(rtc->owner); >> > + if (device_may_wakeup(rtc->dev.parent)) >> > + device_init_wakeup(&alarmtimer_pdev->dev, false); >> > + >> > + platform_device_unregister(alarmtimer_pdev); >> > + put_device(dev); >> > + alarmtimer_pdev = NULL; >> > + rtcdev = NULL; >> > + } >> >> That's broken versus alarmtimer_get_rtcdev() and any user of it. > > You mean we should update[1] (charger-manager driver)as it is the one > using alarmtimer_get_rtcdev()?? # git grep -c alarmtimer_get_rtcdev drivers/power/supply/charger-manager.c:1 include/linux/alarmtimer.h:2 kernel/time/alarmtimer.c:10 Thanks, tglx
Hi Thomas Gleixner, > Subject: RE: [PATCH v2] alarmtimer: Fix rebind failure > > On Mon, Oct 09 2023 at 15:30, Biju Das wrote: > >> Subject: Re: [PATCH v2] alarmtimer: Fix rebind failure > >> > >> On Fri, Sep 22 2023 at 09:12, Biju Das wrote: > >> > +static void alarmtimer_rtc_remove_device(struct device *dev) { > >> > + struct rtc_device *rtc = to_rtc_device(dev); > >> > + > >> > + if (rtcdev == rtc) { > >> > + module_put(rtc->owner); > >> > + if (device_may_wakeup(rtc->dev.parent)) > >> > + device_init_wakeup(&alarmtimer_pdev->dev, false); > >> > + > >> > + platform_device_unregister(alarmtimer_pdev); > >> > + put_device(dev); > >> > + alarmtimer_pdev = NULL; > >> > + rtcdev = NULL; > >> > + } > >> > >> That's broken versus alarmtimer_get_rtcdev() and any user of it. > > > > You mean we should update[1] (charger-manager driver)as it is the one > > using alarmtimer_get_rtcdev()?? > > # git grep -c alarmtimer_get_rtcdev > drivers/power/supply/charger-manager.c:1 > include/linux/alarmtimer.h:2 > kernel/time/alarmtimer.c:10 kernel/time/alarmtimer.c has alarmtimer_get_rtcdev()check everywhere, that is missing in charger-manager.c. I will add the same, is it ok? Cheers, Biju
On Mon, Oct 09 2023 at 17:02, Biju Das wrote: >> On Mon, Oct 09 2023 at 15:30, Biju Das wrote: >> > You mean we should update[1] (charger-manager driver)as it is the one >> > using alarmtimer_get_rtcdev()?? >> >> # git grep -c alarmtimer_get_rtcdev >> drivers/power/supply/charger-manager.c:1 >> include/linux/alarmtimer.h:2 >> kernel/time/alarmtimer.c:10 > > kernel/time/alarmtimer.c has alarmtimer_get_rtcdev()check everywhere, > that is missing in charger-manager.c. I will add the same, is it ok? The code does in the init function: if (alarmtimer_get_rtcdev()) { .... } IOW, charger-manager.c expects that alarm is working when alarmtimer_get_rtcdev() returns non NULL at init. So ripping the RTC device out under it is going to result in a disfunctional driver. I'm not convinced that you can fix this by sprinkling a ton of checks around the code. But that's not the worst of it. The alarmtimer infrastructure is generally not designed for device/module removal. Why? The posix timer interface is fundamentally expecting that an armed alarm timer is actually functional. The fact that the class interface does not have a remove_dev callback is not an oversight and holding a reference on the module and a reference on the device is intended to ensure that the device cannot vanish. The changelog lacks any form of explanation why this is required and how removal of the registered RTC device is actually possible. Neither does it provide any analysis why this cannot result in malfunction. Thanks, tglx
Hi Thomas Gleixner, > Subject: RE: [PATCH v2] alarmtimer: Fix rebind failure > > On Mon, Oct 09 2023 at 17:02, Biju Das wrote: > >> On Mon, Oct 09 2023 at 15:30, Biju Das wrote: > >> > You mean we should update[1] (charger-manager driver)as it is the > >> > one using alarmtimer_get_rtcdev()?? > >> > >> # git grep -c alarmtimer_get_rtcdev > >> drivers/power/supply/charger-manager.c:1 > >> include/linux/alarmtimer.h:2 > >> kernel/time/alarmtimer.c:10 > > > > kernel/time/alarmtimer.c has alarmtimer_get_rtcdev()check everywhere, > > that is missing in charger-manager.c. I will add the same, is it ok? > > The code does in the init function: > > if (alarmtimer_get_rtcdev()) { > .... > } > > IOW, charger-manager.c expects that alarm is working when > alarmtimer_get_rtcdev() returns non NULL at init. So ripping the RTC device > out under it is going to result in a disfunctional driver. I'm not > convinced that you can fix this by sprinkling a ton of checks around the > code. > > But that's not the worst of it. The alarmtimer infrastructure is generally > not designed for device/module removal. Why? > > The posix timer interface is fundamentally expecting that an armed alarm > timer is actually functional. The fact that the class interface does not > have a remove_dev callback is not an oversight and holding a reference on > the module and a reference on the device is intended to ensure that the > device cannot vanish. Thanks for the explanation, I am not aware we should not remove RTC device. > > The changelog lacks any form of explanation why this is required and how > removal of the registered RTC device is actually possible. Neither does it > provide any analysis why this cannot result in malfunction. RTC driver is defined as a module, so I was testing remove/unbind followed by install/bind on RTC driver to check any resource leakage and found that device is not working properly. As you mentioned above, we should not remove RTC driver. So I would like to drop this patch. Is there any place we can document this to avoid another person doing same mistake? Cheers, Biju
Biju! On Tue, Oct 10 2023 at 06:18, Biju Das wrote: > RTC driver is defined as a module, so I was testing > remove/unbind followed by install/bind on RTC driver to check > any resource leakage and found that device is not working properly. > > As you mentioned above, we should not remove RTC driver. So I would > like to drop this patch. > > Is there any place we can document this to avoid another person doing > same mistake? The point is that the removal should not happen in the first place. Though it seems that even a held refcount on the module is not preventing that, which is bad to begin with. That aside I'm not saying that supporting removal is completely impossible. The charger driver can probably be fixed, but as this is a user space visible change this needs a lot of thoughts and proper analysis why such a change would be correct under all circumstances. Thanks, tglx
Hi Thomas, On Tue, Oct 10, 2023 at 5:16 PM Thomas Gleixner <tglx@linutronix.de> wrote: > On Tue, Oct 10 2023 at 06:18, Biju Das wrote: > > RTC driver is defined as a module, so I was testing > > remove/unbind followed by install/bind on RTC driver to check > > any resource leakage and found that device is not working properly. > > > > As you mentioned above, we should not remove RTC driver. So I would > > like to drop this patch. > > > > Is there any place we can document this to avoid another person doing > > same mistake? > > The point is that the removal should not happen in the first place. > > Though it seems that even a held refcount on the module is not > preventing that, which is bad to begin with. Indeed. I had expected to find at least one RTC driver for a device on a hot-pluggable bus like USB, but apparently we have none of these (yet). So currently RTC device removal can only be triggered by a manual sysfs unbind or delete_device. > That aside I'm not saying that supporting removal is completely > impossible. The charger driver can probably be fixed, but as this is a > user space visible change this needs a lot of thoughts and proper > analysis why such a change would be correct under all circumstances. The charger manager seems to be considered a legacy driver. Devices are only instantiated from the drivers/mfd/88pm860x.c (as used on Marvell PXA910 DKB boards), or directly from DT (no upstream users). The DT bindings say: Binding for the legacy charger manager driver. Please do not use for new products. The "if (alarmtimer_get_rtcdev()) { ... }" you pointed out in the probe function seems to be rather fragile, as it depends on probe order. And both CHARGER_MANAGER and RTC_DRV_88PM860X can be modular. Gr{oetje,eeting}s, Geert
Hi Geert and Thomas, > Subject: Re: [PATCH v2] alarmtimer: Fix rebind failure > > Hi Thomas, > > On Tue, Oct 10, 2023 at 5:16 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Tue, Oct 10 2023 at 06:18, Biju Das wrote: > > > RTC driver is defined as a module, so I was testing remove/unbind > > > followed by install/bind on RTC driver to check any resource leakage > > > and found that device is not working properly. > > > > > > As you mentioned above, we should not remove RTC driver. So I would > > > like to drop this patch. > > > > > > Is there any place we can document this to avoid another person > > > doing same mistake? > > > > The point is that the removal should not happen in the first place. > > > > Though it seems that even a held refcount on the module is not > > preventing that, which is bad to begin with. > > Indeed. I had expected to find at least one RTC driver for a device on a > hot-pluggable bus like USB, but apparently we have none of these (yet). So > currently RTC device removal can only be triggered by a manual sysfs unbind > or delete_device. > > > That aside I'm not saying that supporting removal is completely > > impossible. The charger driver can probably be fixed, but as this is a > > user space visible change this needs a lot of thoughts and proper > > analysis why such a change would be correct under all circumstances. > > The charger manager seems to be considered a legacy driver. > Devices are only instantiated from the drivers/mfd/88pm860x.c (as used on > Marvell PXA910 DKB boards), or directly from DT (no upstream users). The DT > bindings say: > > Binding for the legacy charger manager driver. > Please do not use for new products. > > The "if (alarmtimer_get_rtcdev()) { ... }" you pointed out in the probe > function seems to be rather fragile, as it depends on probe order. And > both CHARGER_MANAGER and RTC_DRV_88PM860X can be modular. Does it mean that current patch is fine? On normal scenario, no one will remove RTC device, so nothing to worry about battery charger. On exceptional cases if anyone wants to remove RTC driver, this patch will help(for eg: checking resource leak remove/unbind followed by modprobe/bind). Cheers, Biju
On Wed, Oct 11 2023 at 06:58, Biju Das wrote: >> On Tue, Oct 10, 2023 at 5:16 PM Thomas Gleixner <tglx@linutronix.de> wrote: >> >> The "if (alarmtimer_get_rtcdev()) { ... }" you pointed out in the probe >> function seems to be rather fragile, as it depends on probe order. And >> both CHARGER_MANAGER and RTC_DRV_88PM860X can be modular. > > Does it mean that current patch is fine? On normal scenario, no one > will remove RTC device, so nothing to worry about battery charger. On > exceptional cases if anyone wants to remove RTC driver, this patch > will help(for eg: checking resource leak remove/unbind followed by > modprobe/bind). Did you actually read what I wrote? Allowing removal of a registered RTC alarm device is a user space visible change as it violates the assumption that an armed alarm timer is actually functional. So unless you provide a proper analysis why this does not matter, this is going nowhere. Thanks, tglx
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index 8d9f13d847f0..04d67de8b1fe 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -61,6 +61,7 @@ static DEFINE_SPINLOCK(freezer_delta_lock); /* rtc timer and device for setting alarm wakeups at suspend */ static struct rtc_timer rtctimer; static struct rtc_device *rtcdev; +static struct platform_device *alarmtimer_pdev; static DEFINE_SPINLOCK(rtcdev_lock); /** @@ -109,6 +110,7 @@ static int alarmtimer_rtc_add_device(struct device *dev) } rtcdev = rtc; + alarmtimer_pdev = pdev; /* hold a reference so it doesn't go away */ get_device(dev); pdev = NULL; @@ -123,6 +125,22 @@ static int alarmtimer_rtc_add_device(struct device *dev) return ret; } +static void alarmtimer_rtc_remove_device(struct device *dev) +{ + struct rtc_device *rtc = to_rtc_device(dev); + + if (rtcdev == rtc) { + module_put(rtc->owner); + if (device_may_wakeup(rtc->dev.parent)) + device_init_wakeup(&alarmtimer_pdev->dev, false); + + platform_device_unregister(alarmtimer_pdev); + put_device(dev); + alarmtimer_pdev = NULL; + rtcdev = NULL; + } +} + static inline void alarmtimer_rtc_timer_init(void) { rtc_timer_init(&rtctimer, NULL, NULL); @@ -130,6 +148,7 @@ static inline void alarmtimer_rtc_timer_init(void) static struct class_interface alarmtimer_rtc_interface = { .add_dev = &alarmtimer_rtc_add_device, + .remove_dev = &alarmtimer_rtc_remove_device, }; static int alarmtimer_rtc_interface_setup(void)
The resources allocated in alarmtimer_rtc_add_device() are not freed leading to re-bind failure for the endpoint driver. Fix this issue by adding alarmtimer_rtc_remove_device(). Fixes: c79108bd19a8 ("alarmtimer: Make alarmtimer platform device child of RTC device") Fixes: 7c94caca877b ("alarmtimer: Use wakeup source from alarmtimer platform device") Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v1->v2: * Add fixes tag. * Replaced the variable rtc_pdev->alarmtimer_pdev * Added the check rtcdev == rtc before unregistering the real alarmtimer. Note: This issue is found while adding irq support for built in RTC found on Renesas PMIC RAA215300 device. This issue should present on all RTC drivers which calls device_init_wakeup() in probe(). --- kernel/time/alarmtimer.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)