[PATCH 0/4] Fix alarmtimer suspend failure

[PATCH 0/4] Fix alarmtimer suspend failure

[Stephen Boyd]

We recently ran into a suspend problem where the alarmtimer platform
driver's suspend function fails because the RTC that it's trying
to program is already suspended. This patch series fixes that problem
by making the platform device a child of the RTC.

The last two patches are non-critical changes to how we do the wakeup
and some code cleanup. The first patch fixes a memory leak that I
saw by inspection.

Stephen Boyd (4):
  alarmtimer: Unregister wakeup source when module get fails
  alarmtimer: Make alarmtimer platform device child of RTC device
  alarmtimer: Use wakeup source from alarmtimer platform device
  alarmtimer: Always export alarmtimer_get_rtcdev() and update docs

 kernel/time/alarmtimer.c | 41 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)


base-commit: 4becfd1b26ef4014a20fb38dd17554077adbed5d
-- 
Sent by a computer, using git, on the internet

[PATCH 1/4] alarmtimer: Unregister wakeup source when module get fails

[Stephen Boyd]

The alarmtimer_rtc_add_device() function creates a wakeup source and
then tries to grab a module reference. If that fails we return early
with a -1, but forget to remove the wakeup source. Cleanup this exit
path so we don't leave a dangling wakeup source allocated and named
'alarmtimer' that will conflict with another RTC device that may be
registered.

Fixes: 51218298a25e ("alarmtimer: Ensure RTC module is not unloaded")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 kernel/time/alarmtimer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 451f9d05ccfe..4b11f0309eee 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -88,6 +88,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	unsigned long flags;
 	struct rtc_device *rtc = to_rtc_device(dev);
 	struct wakeup_source *__ws;
+	int ret = 0;
 
 	if (rtcdev)
 		return -EBUSY;
@@ -102,8 +103,8 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	spin_lock_irqsave(&rtcdev_lock, flags);
 	if (!rtcdev) {
 		if (!try_module_get(rtc->owner)) {
-			spin_unlock_irqrestore(&rtcdev_lock, flags);
-			return -1;
+			ret = -1;
+			goto unlock;
 		}
 
 		rtcdev = rtc;
@@ -112,11 +113,12 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 		ws = __ws;
 		__ws = NULL;
 	}
+unlock:
 	spin_unlock_irqrestore(&rtcdev_lock, flags);
 
 	wakeup_source_unregister(__ws);
 
-	return 0;
+	return ret;
 }
 
 static inline void alarmtimer_rtc_timer_init(void)
-- 
Sent by a computer, using git, on the internet

[PATCH 2/4] alarmtimer: Make alarmtimer platform device child of RTC device

[Stephen Boyd]

The alarmtimer_suspend() function will fail if an RTC device is on a bus
such as SPI or i2c and that RTC device registers and probes after
alarmtimer_init() registers and probes the 'alarmtimer' platform device.
This is because system wide suspend suspends devices in the reverse
order of their probe. When alarmtimer_suspend() attempts to program the
RTC for a wakeup it will try to program an RTC device on a bus that has
already been suspended.

Let's move the alarmtimer device registration to be when the RTC we use
for wakeup is registered. Register the 'alarmtimer' platform device as a
child of the RTC device too, so that we can be guaranteed that the RTC
device won't be suspended when alarmtimer_suspend() is called.

Reported-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 kernel/time/alarmtimer.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 4b11f0309eee..ccb6aea4f1d4 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -88,6 +88,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	unsigned long flags;
 	struct rtc_device *rtc = to_rtc_device(dev);
 	struct wakeup_source *__ws;
+	struct platform_device *pdev;
 	int ret = 0;
 
 	if (rtcdev)
@@ -99,6 +100,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 		return -1;
 
 	__ws = wakeup_source_register(dev, "alarmtimer");
+	pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
 
 	spin_lock_irqsave(&rtcdev_lock, flags);
 	if (!rtcdev) {
@@ -112,10 +114,12 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 		get_device(dev);
 		ws = __ws;
 		__ws = NULL;
+		pdev = NULL;
 	}
 unlock:
 	spin_unlock_irqrestore(&rtcdev_lock, flags);
 
+	platform_device_unregister(pdev);
 	wakeup_source_unregister(__ws);
 
 	return ret;
@@ -876,8 +880,7 @@ static struct platform_driver alarmtimer_driver = {
  */
 static int __init alarmtimer_init(void)
 {
-	struct platform_device *pdev;
-	int error = 0;
+	int error;
 	int i;
 
 	alarmtimer_rtc_timer_init();
@@ -900,15 +903,7 @@ static int __init alarmtimer_init(void)
 	if (error)
 		goto out_if;
 
-	pdev = platform_device_register_simple("alarmtimer", -1, NULL, 0);
-	if (IS_ERR(pdev)) {
-		error = PTR_ERR(pdev);
-		goto out_drv;
-	}
 	return 0;
-
-out_drv:
-	platform_driver_unregister(&alarmtimer_driver);
 out_if:
 	alarmtimer_rtc_interface_remove();
 	return error;
-- 
Sent by a computer, using git, on the internet

[PATCH 3/4] alarmtimer: Use wakeup source from alarmtimer platform device

[Stephen Boyd]

Use the wakeup source that can be associated with the 'alarmtimer'
platform device instead of registering another one by hand.

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 kernel/time/alarmtimer.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index ccb6aea4f1d4..be057638e89d 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -55,8 +55,6 @@ static DEFINE_SPINLOCK(freezer_delta_lock);
 #endif
 
 #ifdef CONFIG_RTC_CLASS
-static struct wakeup_source *ws;
-
 /* rtc timer and device for setting alarm wakeups at suspend */
 static struct rtc_timer		rtctimer;
 static struct rtc_device	*rtcdev;
@@ -87,7 +85,6 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 {
 	unsigned long flags;
 	struct rtc_device *rtc = to_rtc_device(dev);
-	struct wakeup_source *__ws;
 	struct platform_device *pdev;
 	int ret = 0;
 
@@ -99,8 +96,9 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	if (!device_may_wakeup(rtc->dev.parent))
 		return -1;
 
-	__ws = wakeup_source_register(dev, "alarmtimer");
 	pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
+	if (pdev)
+		device_init_wakeup(&pdev->dev, true);
 
 	spin_lock_irqsave(&rtcdev_lock, flags);
 	if (!rtcdev) {
@@ -112,15 +110,12 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 		rtcdev = rtc;
 		/* hold a reference so it doesn't go away */
 		get_device(dev);
-		ws = __ws;
-		__ws = NULL;
 		pdev = NULL;
 	}
 unlock:
 	spin_unlock_irqrestore(&rtcdev_lock, flags);
 
 	platform_device_unregister(pdev);
-	wakeup_source_unregister(__ws);
 
 	return ret;
 }
@@ -287,7 +282,7 @@ static int alarmtimer_suspend(struct device *dev)
 		return 0;
 
 	if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
-		__pm_wakeup_event(ws, 2 * MSEC_PER_SEC);
+		pm_wakeup_event(dev, 2 * MSEC_PER_SEC);
 		return -EBUSY;
 	}
 
@@ -302,7 +297,7 @@ static int alarmtimer_suspend(struct device *dev)
 	/* Set alarm, if in the past reject suspend briefly to handle */
 	ret = rtc_timer_start(rtc, &rtctimer, now, 0);
 	if (ret < 0)
-		__pm_wakeup_event(ws, MSEC_PER_SEC);
+		pm_wakeup_event(dev, MSEC_PER_SEC);
 	return ret;
 }
 
-- 
Sent by a computer, using git, on the internet

[PATCH 4/4] alarmtimer: Always export alarmtimer_get_rtcdev() and update docs

[Stephen Boyd]

The export isn't there for the stubbed version of
alarmtimer_get_rtcdev(), so move the export outside of the ifdef. And
rtcdev isn't used outside of this ifdef so we don't need to redefine it
as NULL.

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 kernel/time/alarmtimer.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index be057638e89d..dc4d3edc2a7d 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -64,8 +64,6 @@ static DEFINE_SPINLOCK(rtcdev_lock);
  * alarmtimer_get_rtcdev - Return selected rtcdevice
  *
  * This function returns the rtc device to use for wakealarms.
- * If one has not already been chosen, it checks to see if a
- * functional rtc device is available.
  */
 struct rtc_device *alarmtimer_get_rtcdev(void)
 {
@@ -78,7 +76,6 @@ struct rtc_device *alarmtimer_get_rtcdev(void)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(alarmtimer_get_rtcdev);
 
 static int alarmtimer_rtc_add_device(struct device *dev,
 				struct class_interface *class_intf)
@@ -143,11 +140,11 @@ struct rtc_device *alarmtimer_get_rtcdev(void)
 {
 	return NULL;
 }
-#define rtcdev (NULL)
 static inline int alarmtimer_rtc_interface_setup(void) { return 0; }
 static inline void alarmtimer_rtc_interface_remove(void) { }
 static inline void alarmtimer_rtc_timer_init(void) { }
 #endif
+EXPORT_SYMBOL_GPL(alarmtimer_get_rtcdev);
 
 /**
  * alarmtimer_enqueue - Adds an alarm timer to an alarm_base timerqueue
-- 
Sent by a computer, using git, on the internet

Re: [PATCH 2/4] alarmtimer: Make alarmtimer platform device child of RTC device

[Doug Anderson]

Hi,

On Thu, Jan 9, 2020 at 7:59 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> The alarmtimer_suspend() function will fail if an RTC device is on a bus
> such as SPI or i2c and that RTC device registers and probes after
> alarmtimer_init() registers and probes the 'alarmtimer' platform device.
> This is because system wide suspend suspends devices in the reverse
> order of their probe. When alarmtimer_suspend() attempts to program the
> RTC for a wakeup it will try to program an RTC device on a bus that has
> already been suspended.
>
> Let's move the alarmtimer device registration to be when the RTC we use
> for wakeup is registered. Register the 'alarmtimer' platform device as a
> child of the RTC device too, so that we can be guaranteed that the RTC
> device won't be suspended when alarmtimer_suspend() is called.
>
> Reported-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  kernel/time/alarmtimer.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index 4b11f0309eee..ccb6aea4f1d4 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -88,6 +88,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>         unsigned long flags;
>         struct rtc_device *rtc = to_rtc_device(dev);
>         struct wakeup_source *__ws;
> +       struct platform_device *pdev;
>         int ret = 0;
>
>         if (rtcdev)
> @@ -99,6 +100,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>                 return -1;
>
>         __ws = wakeup_source_register(dev, "alarmtimer");
> +       pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);

Don't you need to check for an error here?  If pdev is an error you'll
continue on your merry way.  Before your patch if you got an error
registering the device it would have caused probe to fail.

I guess you'd only want it to be an error if "rtcdev" is NULL?

Otherwise (though I'm no expert on this code), it looks sane to me.


-Doug

Re: [PATCH 1/4] alarmtimer: Unregister wakeup source when module get fails

[Doug Anderson]

Hi,

On Thu, Jan 9, 2020 at 7:59 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> The alarmtimer_rtc_add_device() function creates a wakeup source and
> then tries to grab a module reference. If that fails we return early
> with a -1, but forget to remove the wakeup source. Cleanup this exit
> path so we don't leave a dangling wakeup source allocated and named
> 'alarmtimer' that will conflict with another RTC device that may be
> registered.
>
> Fixes: 51218298a25e ("alarmtimer: Ensure RTC module is not unloaded")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  kernel/time/alarmtimer.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/4] alarmtimer: Make alarmtimer platform device child of RTC device

[Thomas Gleixner]

Doug Anderson <dianders@chromium.org> writes:
> On Thu, Jan 9, 2020 at 7:59 AM Stephen Boyd <swboyd@chromium.org> wrote:
>> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
>> index 4b11f0309eee..ccb6aea4f1d4 100644
>> --- a/kernel/time/alarmtimer.c
>> +++ b/kernel/time/alarmtimer.c
>> @@ -88,6 +88,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>>         unsigned long flags;
>>         struct rtc_device *rtc = to_rtc_device(dev);
>>         struct wakeup_source *__ws;
>> +       struct platform_device *pdev;
>>         int ret = 0;
>>
>>         if (rtcdev)
>> @@ -99,6 +100,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>>                 return -1;
>>
>>         __ws = wakeup_source_register(dev, "alarmtimer");
>> +       pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
>
> Don't you need to check for an error here?  If pdev is an error you'll
> continue on your merry way.  Before your patch if you got an error
> registering the device it would have caused probe to fail.

Yes, that return value should be checked

> I guess you'd only want it to be an error if "rtcdev" is NULL?

If rtcdev is not NULL then this code is not reached. See the begin of
this function :)

Thanks,

        tglx

[tip: timers/core] alarmtimer: Unregister wakeup source when module get fails

[tip-bot2 for Stephen Boyd]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     6b6d188aae79a630957aefd88ff5c42af6553ee3
Gitweb:        https://git.kernel.org/tip/6b6d188aae79a630957aefd88ff5c42af6553ee3
Author:        Stephen Boyd <swboyd@chromium.org>
AuthorDate:    Thu, 09 Jan 2020 07:59:07 -08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 15 Jan 2020 11:16:54 +01:00

alarmtimer: Unregister wakeup source when module get fails

The alarmtimer_rtc_add_device() function creates a wakeup source and then
tries to grab a module reference. If that fails the function returns early
with an error code, but fails to remove the wakeup source.

Cleanup this exit path so there is no dangling wakeup source, which is
named 'alarmtime' left allocated which will conflict with another RTC
device that may be registered later.

Fixes: 51218298a25e ("alarmtimer: Ensure RTC module is not unloaded")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20200109155910.907-2-swboyd@chromium.org
---
 kernel/time/alarmtimer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index b51b36e..9dc7a09 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -91,6 +91,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	unsigned long flags;
 	struct rtc_device *rtc = to_rtc_device(dev);
 	struct wakeup_source *__ws;
+	int ret = 0;
 
 	if (rtcdev)
 		return -EBUSY;
@@ -105,8 +106,8 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	spin_lock_irqsave(&rtcdev_lock, flags);
 	if (!rtcdev) {
 		if (!try_module_get(rtc->owner)) {
-			spin_unlock_irqrestore(&rtcdev_lock, flags);
-			return -1;
+			ret = -1;
+			goto unlock;
 		}
 
 		rtcdev = rtc;
@@ -115,11 +116,12 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 		ws = __ws;
 		__ws = NULL;
 	}
+unlock:
 	spin_unlock_irqrestore(&rtcdev_lock, flags);
 
 	wakeup_source_unregister(__ws);
 
-	return 0;
+	return ret;
 }
 
 static inline void alarmtimer_rtc_timer_init(void)

Re: [PATCH 2/4] alarmtimer: Make alarmtimer platform device child of RTC device

[Stephen Boyd]

Quoting Thomas Gleixner (2020-01-15 02:07:09)
> Doug Anderson <dianders@chromium.org> writes:
> > On Thu, Jan 9, 2020 at 7:59 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> >> index 4b11f0309eee..ccb6aea4f1d4 100644
> >> --- a/kernel/time/alarmtimer.c
> >> +++ b/kernel/time/alarmtimer.c
> >> @@ -88,6 +88,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
> >>         unsigned long flags;
> >>         struct rtc_device *rtc = to_rtc_device(dev);
> >>         struct wakeup_source *__ws;
> >> +       struct platform_device *pdev;
> >>         int ret = 0;
> >>
> >>         if (rtcdev)
> >> @@ -99,6 +100,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
> >>                 return -1;
> >>
> >>         __ws = wakeup_source_register(dev, "alarmtimer");
> >> +       pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
> >
> > Don't you need to check for an error here?  If pdev is an error you'll
> > continue on your merry way.  Before your patch if you got an error
> > registering the device it would have caused probe to fail.
> 
> Yes, that return value should be checked
> 

Ok. Should __ws also be checked for error? I'm currently thinking of this patch
and then simplifying it in patch 3 of this series by removing __ws. Or
the series can swap patch 2 and 3.

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index ccb6aea4f1d4..3e1f4056e384 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -103,7 +103,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
 
 	spin_lock_irqsave(&rtcdev_lock, flags);
-	if (!rtcdev) {
+	if (__ws && pdev && !rtcdev) {
 		if (!try_module_get(rtc->owner)) {
 			ret = -1;
 			goto unlock;
@@ -115,6 +115,8 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 		ws = __ws;
 		__ws = NULL;
 		pdev = NULL;
+	} else {
+		ret = - 1;
 	}
 unlock:
 	spin_unlock_irqrestore(&rtcdev_lock, flags);

Re: [PATCH 2/4] alarmtimer: Make alarmtimer platform device child of RTC device

[Doug Anderson]

Hi,

On Wed, Jan 15, 2020 at 2:07 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Doug Anderson <dianders@chromium.org> writes:
> > On Thu, Jan 9, 2020 at 7:59 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> >> index 4b11f0309eee..ccb6aea4f1d4 100644
> >> --- a/kernel/time/alarmtimer.c
> >> +++ b/kernel/time/alarmtimer.c
> >> @@ -88,6 +88,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
> >>         unsigned long flags;
> >>         struct rtc_device *rtc = to_rtc_device(dev);
> >>         struct wakeup_source *__ws;
> >> +       struct platform_device *pdev;
> >>         int ret = 0;
> >>
> >>         if (rtcdev)
> >> @@ -99,6 +100,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
> >>                 return -1;
> >>
> >>         __ws = wakeup_source_register(dev, "alarmtimer");
> >> +       pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
> >
> > Don't you need to check for an error here?  If pdev is an error you'll
> > continue on your merry way.  Before your patch if you got an error
> > registering the device it would have caused probe to fail.
>
> Yes, that return value should be checked
>
> > I guess you'd only want it to be an error if "rtcdev" is NULL?
>
> If rtcdev is not NULL then this code is not reached. See the begin of
> this function :)

Wow, not sure how I missed that.  I guess the one at the top of the
function is an optimization, though?  It's being accessed without the
spinlock which means that it's not necessarily reliable, right?  I
guess once the rtcdev has been set then it is never unset, but it does
seem like if two threads could call alarmtimer_rtc_add_device() at the
same time then it's possible that we could end up calling
wakeup_source_register() for both of them.  Did I understand that
correctly?  If I did then maybe it deserves a comment?

-Doug

Re: [PATCH 2/4] alarmtimer: Make alarmtimer platform device child of RTC device

[Doug Anderson]

Hi,

On Wed, Jan 15, 2020 at 8:47 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Thomas Gleixner (2020-01-15 02:07:09)
> > Doug Anderson <dianders@chromium.org> writes:
> > > On Thu, Jan 9, 2020 at 7:59 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> > >> index 4b11f0309eee..ccb6aea4f1d4 100644
> > >> --- a/kernel/time/alarmtimer.c
> > >> +++ b/kernel/time/alarmtimer.c
> > >> @@ -88,6 +88,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
> > >>         unsigned long flags;
> > >>         struct rtc_device *rtc = to_rtc_device(dev);
> > >>         struct wakeup_source *__ws;
> > >> +       struct platform_device *pdev;
> > >>         int ret = 0;
> > >>
> > >>         if (rtcdev)
> > >> @@ -99,6 +100,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
> > >>                 return -1;
> > >>
> > >>         __ws = wakeup_source_register(dev, "alarmtimer");
> > >> +       pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
> > >
> > > Don't you need to check for an error here?  If pdev is an error you'll
> > > continue on your merry way.  Before your patch if you got an error
> > > registering the device it would have caused probe to fail.
> >
> > Yes, that return value should be checked
> >
>
> Ok. Should __ws also be checked for error? I'm currently thinking of this patch
> and then simplifying it in patch 3 of this series by removing __ws. Or
> the series can swap patch 2 and 3.
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index ccb6aea4f1d4..3e1f4056e384 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -103,7 +103,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>         pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
>
>         spin_lock_irqsave(&rtcdev_lock, flags);
> -       if (!rtcdev) {
> +       if (__ws && pdev && !rtcdev) {

I believe instead of pdev you want !IS_ERR(pdev)

...otherwise this seems sane.  I ran out of time last night to get to
patch #3 and #4 but I'll look at them shortly.  I don't have tons of
opinions for the ordering questions, so whatever seems cleanest?

-Doug

Re: [PATCH 3/4] alarmtimer: Use wakeup source from alarmtimer platform device

[Doug Anderson]

Hi,

On Thu, Jan 9, 2020 at 7:59 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Use the wakeup source that can be associated with the 'alarmtimer'
> platform device instead of registering another one by hand.
>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  kernel/time/alarmtimer.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index ccb6aea4f1d4..be057638e89d 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -55,8 +55,6 @@ static DEFINE_SPINLOCK(freezer_delta_lock);
>  #endif
>
>  #ifdef CONFIG_RTC_CLASS
> -static struct wakeup_source *ws;
> -
>  /* rtc timer and device for setting alarm wakeups at suspend */
>  static struct rtc_timer                rtctimer;
>  static struct rtc_device       *rtcdev;
> @@ -87,7 +85,6 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>  {
>         unsigned long flags;
>         struct rtc_device *rtc = to_rtc_device(dev);
> -       struct wakeup_source *__ws;
>         struct platform_device *pdev;
>         int ret = 0;
>
> @@ -99,8 +96,9 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>         if (!device_may_wakeup(rtc->dev.parent))
>                 return -1;
>
> -       __ws = wakeup_source_register(dev, "alarmtimer");
>         pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
> +       if (pdev)

It appears that the error case for platform_device_register_data()
needs to be checked with by IS_ERR().

Other than that, this seems like a sane idea to me and a nice cleanup.
With my usual caveat that I'm reviewing code that I'm by no means an
expert in, feel free to add my Reviewed-by with that change.

-Doug

Re: [PATCH 4/4] alarmtimer: Always export alarmtimer_get_rtcdev() and update docs

[Doug Anderson]

Hi,

On Thu, Jan 9, 2020 at 7:59 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> The export isn't there for the stubbed version of
> alarmtimer_get_rtcdev(), so move the export outside of the ifdef. And
> rtcdev isn't used outside of this ifdef so we don't need to redefine it
> as NULL.
>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  kernel/time/alarmtimer.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/4] alarmtimer: Make alarmtimer platform device child of RTC device

[Stephen Boyd]

Quoting Doug Anderson (2020-01-15 11:22:26)
> Hi,
> 
> On Wed, Jan 15, 2020 at 2:07 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Doug Anderson <dianders@chromium.org> writes:
> > > On Thu, Jan 9, 2020 at 7:59 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> > >> index 4b11f0309eee..ccb6aea4f1d4 100644
> > >> --- a/kernel/time/alarmtimer.c
> > >> +++ b/kernel/time/alarmtimer.c
> > >> @@ -88,6 +88,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
> > >>         unsigned long flags;
> > >>         struct rtc_device *rtc = to_rtc_device(dev);
> > >>         struct wakeup_source *__ws;
> > >> +       struct platform_device *pdev;
> > >>         int ret = 0;
> > >>
> > >>         if (rtcdev)
> > >> @@ -99,6 +100,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
> > >>                 return -1;
> > >>
> > >>         __ws = wakeup_source_register(dev, "alarmtimer");
> > >> +       pdev = platform_device_register_data(dev, "alarmtimer", -1, NULL, 0);
> > >
> > > Don't you need to check for an error here?  If pdev is an error you'll
> > > continue on your merry way.  Before your patch if you got an error
> > > registering the device it would have caused probe to fail.
> >
> > Yes, that return value should be checked
> >
> > > I guess you'd only want it to be an error if "rtcdev" is NULL?
> >
> > If rtcdev is not NULL then this code is not reached. See the begin of
> > this function :)
> 
> Wow, not sure how I missed that.  I guess the one at the top of the
> function is an optimization, though?  It's being accessed without the
> spinlock which means that it's not necessarily reliable, right?  I
> guess once the rtcdev has been set then it is never unset, but it does
> seem like if two threads could call alarmtimer_rtc_add_device() at the
> same time then it's possible that we could end up calling
> wakeup_source_register() for both of them.  Did I understand that
> correctly?  If I did then maybe it deserves a comment?

It also looks like we call wakeup_source_register() and get lucky that
they're both called alarmtimer but they don't conflict with each other
in sysfs. Once we try to add a device named "alarmtimer" to the platform
bus it is the only one that can be added. I'll have to make this
autogenerate some number for the device instead of using -1 as the id so
that we don't see device name conflicts on the same bus.