[PATCH v2 0/3] Fix alarmtimer suspend failure

[PATCH v2 0/3] 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.

Changes from v1:
 * Dropped first patch that got picked up
 * Reworked second patch to autogenerate the device id and use IS_ERR()

Stephen Boyd (3):
  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 | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)


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

[PATCH v2 1/3] 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 | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 9dc7a0913190..e1a863b5c0bf 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;
+	struct platform_device *pdev;
 	int ret = 0;
 
 	if (rtcdev)
@@ -102,9 +103,11 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 		return -1;
 
 	__ws = wakeup_source_register(dev, "alarmtimer");
+	pdev = platform_device_register_data(dev, "alarmtimer",
+					     PLATFORM_DEVID_AUTO, NULL, 0);
 
 	spin_lock_irqsave(&rtcdev_lock, flags);
-	if (!rtcdev) {
+	if (__ws && !IS_ERR(pdev) && !rtcdev) {
 		if (!try_module_get(rtc->owner)) {
 			ret = -1;
 			goto unlock;
@@ -115,10 +118,14 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 		get_device(dev);
 		ws = __ws;
 		__ws = NULL;
+		pdev = NULL;
+	} else {
+		ret = - 1;
 	}
 unlock:
 	spin_unlock_irqrestore(&rtcdev_lock, flags);
 
+	platform_device_unregister(pdev);
 	wakeup_source_unregister(__ws);
 
 	return ret;
@@ -905,8 +912,7 @@ static void get_boottime_timespec(struct timespec64 *tp)
  */
 static int __init alarmtimer_init(void)
 {
-	struct platform_device *pdev;
-	int error = 0;
+	int error;
 	int i;
 
 	alarmtimer_rtc_timer_init();
@@ -931,15 +937,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

Re: [PATCH v2 1/3] alarmtimer: Make alarmtimer platform device child of RTC device

[Stephen Boyd]

Quoting Stephen Boyd (2020-01-21 11:48:09)
> @@ -115,10 +118,14 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>                 get_device(dev);
>                 ws = __ws;
>                 __ws = NULL;
> +               pdev = NULL;
> +       } else {
> +               ret = - 1;

                         ^
Ick. This has an extra space.

>         }
>  unlock:
>         spin_unlock_irqrestore(&rtcdev_lock, flags);
>  
> +       platform_device_unregister(pdev);
>         wakeup_source_unregister(__ws);
>  
>         return ret;

Re: [PATCH v2 1/3] alarmtimer: Make alarmtimer platform device child of RTC device

[Doug Anderson]

Hi,

On Tue, Jan 21, 2020 at 11:48 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 | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)

Other than the extra space that you pointed out yourself (which maybe
could be fixed when applying to avoid an extra spin), this looks nice
to me.

With the caveat that I'm not an expert in this part of the code:

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

[PATCH v2 2/3] 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 | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index e1a863b5c0bf..0c9e97054da8 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -58,8 +58,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;
@@ -90,7 +88,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;
 
@@ -102,12 +99,13 @@ 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",
 					     PLATFORM_DEVID_AUTO, NULL, 0);
+	if (!IS_ERR(pdev))
+		device_init_wakeup(&pdev->dev, true);
 
 	spin_lock_irqsave(&rtcdev_lock, flags);
-	if (__ws && !IS_ERR(pdev) && !rtcdev) {
+	if (!IS_ERR(pdev) && !rtcdev) {
 		if (!try_module_get(rtc->owner)) {
 			ret = -1;
 			goto unlock;
@@ -116,8 +114,6 @@ 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;
 	} else {
 		ret = - 1;
@@ -126,7 +122,6 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	spin_unlock_irqrestore(&rtcdev_lock, flags);
 
 	platform_device_unregister(pdev);
-	wakeup_source_unregister(__ws);
 
 	return ret;
 }
@@ -293,7 +288,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;
 	}
 
@@ -308,7 +303,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

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

[Doug Anderson]

Hi,

On Tue, Jan 21, 2020 at 11:48 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 | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

Seems like a good idea to me.  With the same caveat that I'm no expert
in this part of the code:

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

[PATCH v2 3/3] 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.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
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 0c9e97054da8..6ea08fa62c46 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -67,8 +67,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)
 {
@@ -81,7 +79,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)
@@ -149,11 +146,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 v2 3/3] alarmtimer: Always export alarmtimer_get_rtcdev() and update docs

[Thomas Gleixner]

Stephen Boyd <swboyd@chromium.org> writes:
> 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.

This does not make any sense. Why would we export a trivial stub which
just returns NULL?

The right thing to do is to make that an inline function in the relevant
header file.

> @@ -67,8 +67,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.

Unrelated comment change which is not explained in the changelog. Please
make that a separate patch as it has absolutely nothing to do with the
stub function issue.

Thanks,

        tglx

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

[Stephen Boyd]

Quoting Thomas Gleixner (2020-01-23 17:04:49)
> Stephen Boyd <swboyd@chromium.org> writes:
> > 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.
> 
> This does not make any sense. Why would we export a trivial stub which
> just returns NULL?
> 
> The right thing to do is to make that an inline function in the relevant
> header file.

Ok. Will do.

> 
> > @@ -67,8 +67,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.
> 
> Unrelated comment change which is not explained in the changelog. Please
> make that a separate patch as it has absolutely nothing to do with the
> stub function issue.
> 

Fair enough. I only mentioned it with one word in the subject, 'docs'.
Sorry about that.