linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] alarmtimer: Fix rebind failure
@ 2023-09-22  8:12 Biju Das
  2023-10-09  7:00 ` Biju Das
  2023-10-09 15:10 ` Thomas Gleixner
  0 siblings, 2 replies; 12+ messages in thread
From: Biju Das @ 2023-09-22  8:12 UTC (permalink / raw)
  To: Thomas Gleixner, Alessandro Zummo, Alexandre Belloni
  Cc: Biju Das, John Stultz, Stephen Boyd, Douglas Anderson,
	Geert Uytterhoeven, Biju Das, linux-rtc, linux-kernel,
	linux-renesas-soc

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] alarmtimer: Fix rebind failure
  2023-09-22  8:12 [PATCH v2] alarmtimer: Fix rebind failure Biju Das
@ 2023-10-09  7:00 ` Biju Das
  2023-10-09 15:10 ` Thomas Gleixner
  1 sibling, 0 replies; 12+ messages in thread
From: Biju Das @ 2023-10-09  7:00 UTC (permalink / raw)
  To: Thomas Gleixner, Alessandro Zummo, Alexandre Belloni
  Cc: John Stultz, Stephen Boyd, Douglas Anderson, Geert Uytterhoeven,
	Biju Das, linux-rtc, linux-kernel, linux-renesas-soc

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] alarmtimer: Fix rebind failure
  2023-09-22  8:12 [PATCH v2] alarmtimer: Fix rebind failure Biju Das
  2023-10-09  7:00 ` Biju Das
@ 2023-10-09 15:10 ` Thomas Gleixner
  2023-10-09 15:30   ` Biju Das
  1 sibling, 1 reply; 12+ messages in thread
From: Thomas Gleixner @ 2023-10-09 15:10 UTC (permalink / raw)
  To: Biju Das, Alessandro Zummo, Alexandre Belloni
  Cc: Biju Das, John Stultz, Stephen Boyd, Douglas Anderson,
	Geert Uytterhoeven, Biju Das, linux-rtc, linux-kernel,
	linux-renesas-soc

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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] alarmtimer: Fix rebind failure
  2023-10-09 15:10 ` Thomas Gleixner
@ 2023-10-09 15:30   ` Biju Das
  2023-10-09 15:46     ` Thomas Gleixner
  0 siblings, 1 reply; 12+ messages in thread
From: Biju Das @ 2023-10-09 15:30 UTC (permalink / raw)
  To: Thomas Gleixner, Alessandro Zummo, Alexandre Belloni
  Cc: John Stultz, Stephen Boyd, Douglas Anderson, Geert Uytterhoeven,
	Biju Das, linux-rtc, linux-kernel, linux-renesas-soc

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] alarmtimer: Fix rebind failure
  2023-10-09 15:30   ` Biju Das
@ 2023-10-09 15:46     ` Thomas Gleixner
  2023-10-09 17:02       ` Biju Das
  0 siblings, 1 reply; 12+ messages in thread
From: Thomas Gleixner @ 2023-10-09 15:46 UTC (permalink / raw)
  To: Biju Das, Alessandro Zummo, Alexandre Belloni
  Cc: John Stultz, Stephen Boyd, Douglas Anderson, Geert Uytterhoeven,
	Biju Das, linux-rtc, linux-kernel, linux-renesas-soc

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] alarmtimer: Fix rebind failure
  2023-10-09 15:46     ` Thomas Gleixner
@ 2023-10-09 17:02       ` Biju Das
  2023-10-09 22:00         ` Thomas Gleixner
  0 siblings, 1 reply; 12+ messages in thread
From: Biju Das @ 2023-10-09 17:02 UTC (permalink / raw)
  To: Thomas Gleixner, Alessandro Zummo, Alexandre Belloni
  Cc: John Stultz, Stephen Boyd, Douglas Anderson, Geert Uytterhoeven,
	Biju Das, linux-rtc, linux-kernel, linux-renesas-soc

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] alarmtimer: Fix rebind failure
  2023-10-09 17:02       ` Biju Das
@ 2023-10-09 22:00         ` Thomas Gleixner
  2023-10-10  6:18           ` Biju Das
  0 siblings, 1 reply; 12+ messages in thread
From: Thomas Gleixner @ 2023-10-09 22:00 UTC (permalink / raw)
  To: Biju Das, Alessandro Zummo, Alexandre Belloni
  Cc: John Stultz, Stephen Boyd, Douglas Anderson, Geert Uytterhoeven,
	Biju Das, linux-rtc, linux-kernel, linux-renesas-soc

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












^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] alarmtimer: Fix rebind failure
  2023-10-09 22:00         ` Thomas Gleixner
@ 2023-10-10  6:18           ` Biju Das
  2023-10-10 15:16             ` Thomas Gleixner
  0 siblings, 1 reply; 12+ messages in thread
From: Biju Das @ 2023-10-10  6:18 UTC (permalink / raw)
  To: Thomas Gleixner, Alessandro Zummo, Alexandre Belloni
  Cc: John Stultz, Stephen Boyd, Douglas Anderson, Geert Uytterhoeven,
	Biju Das, linux-rtc, linux-kernel, linux-renesas-soc

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] alarmtimer: Fix rebind failure
  2023-10-10  6:18           ` Biju Das
@ 2023-10-10 15:16             ` Thomas Gleixner
  2023-10-10 15:45               ` Geert Uytterhoeven
  0 siblings, 1 reply; 12+ messages in thread
From: Thomas Gleixner @ 2023-10-10 15:16 UTC (permalink / raw)
  To: Biju Das, Alessandro Zummo, Alexandre Belloni
  Cc: John Stultz, Stephen Boyd, Douglas Anderson, Geert Uytterhoeven,
	Biju Das, linux-rtc, linux-kernel, linux-renesas-soc,
	Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] alarmtimer: Fix rebind failure
  2023-10-10 15:16             ` Thomas Gleixner
@ 2023-10-10 15:45               ` Geert Uytterhoeven
  2023-10-11  6:58                 ` Biju Das
  0 siblings, 1 reply; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-10-10 15:45 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Biju Das, Alessandro Zummo, Alexandre Belloni, John Stultz,
	Stephen Boyd, Douglas Anderson, Geert Uytterhoeven, Biju Das,
	linux-rtc, linux-kernel, linux-renesas-soc, Greg Kroah-Hartman

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] alarmtimer: Fix rebind failure
  2023-10-10 15:45               ` Geert Uytterhoeven
@ 2023-10-11  6:58                 ` Biju Das
  2023-10-11  9:12                   ` Thomas Gleixner
  0 siblings, 1 reply; 12+ messages in thread
From: Biju Das @ 2023-10-11  6:58 UTC (permalink / raw)
  To: Geert Uytterhoeven, Thomas Gleixner
  Cc: Alessandro Zummo, Alexandre Belloni, John Stultz, Stephen Boyd,
	Douglas Anderson, Geert Uytterhoeven, Biju Das, linux-rtc,
	linux-kernel, linux-renesas-soc, Greg Kroah-Hartman

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 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] alarmtimer: Fix rebind failure
  2023-10-11  6:58                 ` Biju Das
@ 2023-10-11  9:12                   ` Thomas Gleixner
  0 siblings, 0 replies; 12+ messages in thread
From: Thomas Gleixner @ 2023-10-11  9:12 UTC (permalink / raw)
  To: Biju Das, Geert Uytterhoeven
  Cc: Alessandro Zummo, Alexandre Belloni, John Stultz, Stephen Boyd,
	Douglas Anderson, Geert Uytterhoeven, Biju Das, linux-rtc,
	linux-kernel, linux-renesas-soc, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-10-11  9:12 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-22  8:12 [PATCH v2] alarmtimer: Fix rebind failure Biju Das
2023-10-09  7:00 ` Biju Das
2023-10-09 15:10 ` Thomas Gleixner
2023-10-09 15:30   ` Biju Das
2023-10-09 15:46     ` Thomas Gleixner
2023-10-09 17:02       ` Biju Das
2023-10-09 22:00         ` Thomas Gleixner
2023-10-10  6:18           ` Biju Das
2023-10-10 15:16             ` Thomas Gleixner
2023-10-10 15:45               ` Geert Uytterhoeven
2023-10-11  6:58                 ` Biju Das
2023-10-11  9:12                   ` Thomas Gleixner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).