[PATCH v1 0/5] rtc: rtc-cmos: Assorted ACPI-related cleanups and fixes

[PATCH v1 0/5] rtc: rtc-cmos: Assorted ACPI-related cleanups and fixes

[Rafael J. Wysocki]

Hi All,

This series of patches does some assorted ACPI-related cleanups to the CMOS RTC
driver:
- redundant static variable is dropped,
- code duplication is reduced,
- code is relocated so as to drop a few unnecessary forward declarations of
  functions,
- functions are renamed to avoid confusion,
and fixes up an issue in the driver removal path.

Thanks!

[PATCH v1 1/5] rtc: rtc-cmos: Call cmos_wake_setup() from cmos_do_probe()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notice that cmos_wake_setup() is the only user of acpi_rtc_info and it
can operate on the cmos_rtc variable directly, so it need not set the
platform_data pointer before cmos_do_probe() is called.  Instead, it
can be called by cmos_do_probe() in the case when the platform_data
pointer is not set to implement the default behavior (which is to use
the FADT information as long as ACPI support is enabled).

Modify the code accordingly.

While at it, drop a comment that doesn't really match the code it is
supposed to be describing.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/rtc/rtc-cmos.c |   47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

Index: linux-pm/drivers/rtc/rtc-cmos.c
===================================================================
--- linux-pm.orig/drivers/rtc/rtc-cmos.c
+++ linux-pm/drivers/rtc/rtc-cmos.c
@@ -744,6 +744,8 @@ static irqreturn_t cmos_interrupt(int ir
 		return IRQ_NONE;
 }
 
+static void cmos_wake_setup(struct device *dev);
+
 #ifdef	CONFIG_PNP
 #define	INITSECTION
 
@@ -827,19 +829,27 @@ cmos_do_probe(struct device *dev, struct
 		if (info->address_space)
 			address_space = info->address_space;
 
-		if (info->rtc_day_alarm && info->rtc_day_alarm < 128)
-			cmos_rtc.day_alrm = info->rtc_day_alarm;
-		if (info->rtc_mon_alarm && info->rtc_mon_alarm < 128)
-			cmos_rtc.mon_alrm = info->rtc_mon_alarm;
-		if (info->rtc_century && info->rtc_century < 128)
-			cmos_rtc.century = info->rtc_century;
+		cmos_rtc.day_alrm = info->rtc_day_alarm;
+		cmos_rtc.mon_alrm = info->rtc_mon_alarm;
+		cmos_rtc.century = info->rtc_century;
 
 		if (info->wake_on && info->wake_off) {
 			cmos_rtc.wake_on = info->wake_on;
 			cmos_rtc.wake_off = info->wake_off;
 		}
+	} else {
+		cmos_wake_setup(dev);
 	}
 
+	if (cmos_rtc.day_alrm >= 128)
+		cmos_rtc.day_alrm = 0;
+
+	if (cmos_rtc.mon_alrm >= 128)
+		cmos_rtc.mon_alrm = 0;
+
+	if (cmos_rtc.century >= 128)
+		cmos_rtc.century = 0;
+
 	cmos_rtc.dev = dev;
 	dev_set_drvdata(dev, &cmos_rtc);
 
@@ -1275,13 +1285,6 @@ static void use_acpi_alarm_quirks(void)
 static inline void use_acpi_alarm_quirks(void) { }
 #endif
 
-/* Every ACPI platform has a mc146818 compatible "cmos rtc".  Here we find
- * its device node and pass extra config data.  This helps its driver use
- * capabilities that the now-obsolete mc146818 didn't have, and informs it
- * that this board's RTC is wakeup-capable (per ACPI spec).
- */
-static struct cmos_rtc_board_info acpi_rtc_info;
-
 static void cmos_wake_setup(struct device *dev)
 {
 	if (acpi_disabled)
@@ -1289,26 +1292,23 @@ static void cmos_wake_setup(struct devic
 
 	use_acpi_alarm_quirks();
 
-	acpi_rtc_info.wake_on = rtc_wake_on;
-	acpi_rtc_info.wake_off = rtc_wake_off;
+	cmos_rtc.wake_on = rtc_wake_on;
+	cmos_rtc.wake_off = rtc_wake_off;
 
-	/* workaround bug in some ACPI tables */
+	/* ACPI tables bug workaround. */
 	if (acpi_gbl_FADT.month_alarm && !acpi_gbl_FADT.day_alarm) {
 		dev_dbg(dev, "bogus FADT month_alarm (%d)\n",
 			acpi_gbl_FADT.month_alarm);
 		acpi_gbl_FADT.month_alarm = 0;
 	}
 
-	acpi_rtc_info.rtc_day_alarm = acpi_gbl_FADT.day_alarm;
-	acpi_rtc_info.rtc_mon_alarm = acpi_gbl_FADT.month_alarm;
-	acpi_rtc_info.rtc_century = acpi_gbl_FADT.century;
+	cmos_rtc.day_alrm = acpi_gbl_FADT.day_alarm;
+	cmos_rtc.mon_alrm = acpi_gbl_FADT.month_alarm;
+	cmos_rtc.century = acpi_gbl_FADT.century;
 
-	/* NOTE:  S4_RTC_WAKE is NOT currently useful to Linux */
 	if (acpi_gbl_FADT.flags & ACPI_FADT_S4_RTC_WAKE)
 		dev_info(dev, "RTC can wake from S4\n");
 
-	dev->platform_data = &acpi_rtc_info;
-
 	/* RTC always wakes from S1/S2/S3, and often S4/STD */
 	device_init_wakeup(dev, 1);
 }
@@ -1359,8 +1359,6 @@ static int cmos_pnp_probe(struct pnp_dev
 {
 	int irq, ret;
 
-	cmos_wake_setup(&pnp->dev);
-
 	if (pnp_port_start(pnp, 0) == 0x70 && !pnp_irq_valid(pnp, 0)) {
 		irq = 0;
 #ifdef CONFIG_X86
@@ -1468,7 +1466,6 @@ static int __init cmos_platform_probe(st
 	int irq, ret;
 
 	cmos_of_init(pdev);
-	cmos_wake_setup(&pdev->dev);
 
 	if (RTC_IOMAPPED)
 		resource = platform_get_resource(pdev, IORESOURCE_IO, 0);

[PATCH v1 2/5] rtc: rtc-cmos: Call rtc_wake_setup() from cmos_do_probe()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

To reduce code duplication, move the invocation of rtc_wake_setup()
into cmos_do_probe() and simplify the callers of the latter.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/rtc/rtc-cmos.c |   28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

Index: linux-pm/drivers/rtc/rtc-cmos.c
===================================================================
--- linux-pm.orig/drivers/rtc/rtc-cmos.c
+++ linux-pm/drivers/rtc/rtc-cmos.c
@@ -744,6 +744,7 @@ static irqreturn_t cmos_interrupt(int ir
 		return IRQ_NONE;
 }
 
+static inline void rtc_wake_setup(struct device *dev);
 static void cmos_wake_setup(struct device *dev);
 
 #ifdef	CONFIG_PNP
@@ -938,6 +939,13 @@ cmos_do_probe(struct device *dev, struct
 	nvmem_cfg.size = address_space - NVRAM_OFFSET;
 	devm_rtc_nvmem_register(cmos_rtc.rtc, &nvmem_cfg);
 
+	/*
+	 * Everything has gone well so far, so by default register a handler for
+	 * the ACPI RTC fixed event.
+	 */
+	if (!info)
+		rtc_wake_setup(dev);
+
 	dev_info(dev, "%s%s, %d bytes nvram%s\n",
 		 !is_valid_irq(rtc_irq) ? "no alarms" :
 		 cmos_rtc.mon_alrm ? "alarms up to one year" :
@@ -1357,7 +1365,7 @@ static void rtc_wake_setup(struct device
 
 static int cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 {
-	int irq, ret;
+	int irq;
 
 	if (pnp_port_start(pnp, 0) == 0x70 && !pnp_irq_valid(pnp, 0)) {
 		irq = 0;
@@ -1373,13 +1381,7 @@ static int cmos_pnp_probe(struct pnp_dev
 		irq = pnp_irq(pnp, 0);
 	}
 
-	ret = cmos_do_probe(&pnp->dev, pnp_get_resource(pnp, IORESOURCE_IO, 0), irq);
-	if (ret)
-		return ret;
-
-	rtc_wake_setup(&pnp->dev);
-
-	return 0;
+	return cmos_do_probe(&pnp->dev, pnp_get_resource(pnp, IORESOURCE_IO, 0), irq);
 }
 
 static void cmos_pnp_remove(struct pnp_dev *pnp)
@@ -1463,7 +1465,7 @@ static inline void cmos_of_init(struct p
 static int __init cmos_platform_probe(struct platform_device *pdev)
 {
 	struct resource *resource;
-	int irq, ret;
+	int irq;
 
 	cmos_of_init(pdev);
 
@@ -1475,13 +1477,7 @@ static int __init cmos_platform_probe(st
 	if (irq < 0)
 		irq = -1;
 
-	ret = cmos_do_probe(&pdev->dev, resource, irq);
-	if (ret)
-		return ret;
-
-	rtc_wake_setup(&pdev->dev);
-
-	return 0;
+	return cmos_do_probe(&pdev->dev, resource, irq);
 }
 
 static int cmos_platform_remove(struct platform_device *pdev)

[PATCH v1 3/5] rtc: rtc-cmos: Eliminate forward declarations of some functions

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reorder the ACPI-related code before cmos_do_probe() so as to eliminate
excessive forward declarations of some functions.

While at it, for consistency, add the inline modifier to the
definitions of empty stub static funtions and remove it from the
corresponding definitions of functions with non-empty bodies.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/rtc/rtc-cmos.c |  304 ++++++++++++++++++++++++-------------------------
 1 file changed, 149 insertions(+), 155 deletions(-)

Index: linux-pm/drivers/rtc/rtc-cmos.c
===================================================================
--- linux-pm.orig/drivers/rtc/rtc-cmos.c
+++ linux-pm/drivers/rtc/rtc-cmos.c
@@ -744,8 +744,155 @@ static irqreturn_t cmos_interrupt(int ir
 		return IRQ_NONE;
 }
 
-static inline void rtc_wake_setup(struct device *dev);
-static void cmos_wake_setup(struct device *dev);
+#ifdef	CONFIG_ACPI
+
+#include <linux/acpi.h>
+
+static u32 rtc_handler(void *context)
+{
+	struct device *dev = context;
+	struct cmos_rtc *cmos = dev_get_drvdata(dev);
+	unsigned char rtc_control = 0;
+	unsigned char rtc_intr;
+	unsigned long flags;
+
+
+	/*
+	 * Always update rtc irq when ACPI is used as RTC Alarm.
+	 * Or else, ACPI SCI is enabled during suspend/resume only,
+	 * update rtc irq in that case.
+	 */
+	if (cmos_use_acpi_alarm())
+		cmos_interrupt(0, (void *)cmos->rtc);
+	else {
+		/* Fix me: can we use cmos_interrupt() here as well? */
+		spin_lock_irqsave(&rtc_lock, flags);
+		if (cmos_rtc.suspend_ctrl)
+			rtc_control = CMOS_READ(RTC_CONTROL);
+		if (rtc_control & RTC_AIE) {
+			cmos_rtc.suspend_ctrl &= ~RTC_AIE;
+			CMOS_WRITE(rtc_control, RTC_CONTROL);
+			rtc_intr = CMOS_READ(RTC_INTR_FLAGS);
+			rtc_update_irq(cmos->rtc, 1, rtc_intr);
+		}
+		spin_unlock_irqrestore(&rtc_lock, flags);
+	}
+
+	pm_wakeup_hard_event(dev);
+	acpi_clear_event(ACPI_EVENT_RTC);
+	acpi_disable_event(ACPI_EVENT_RTC, 0);
+	return ACPI_INTERRUPT_HANDLED;
+}
+
+static void rtc_wake_setup(struct device *dev)
+{
+	if (acpi_disabled)
+		return;
+
+	acpi_install_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler, dev);
+	/*
+	 * After the RTC handler is installed, the Fixed_RTC event should
+	 * be disabled. Only when the RTC alarm is set will it be enabled.
+	 */
+	acpi_clear_event(ACPI_EVENT_RTC);
+	acpi_disable_event(ACPI_EVENT_RTC, 0);
+}
+
+static void rtc_wake_on(struct device *dev)
+{
+	acpi_clear_event(ACPI_EVENT_RTC);
+	acpi_enable_event(ACPI_EVENT_RTC, 0);
+}
+
+static void rtc_wake_off(struct device *dev)
+{
+	acpi_disable_event(ACPI_EVENT_RTC, 0);
+}
+
+#ifdef CONFIG_X86
+/* Enable use_acpi_alarm mode for Intel platforms no earlier than 2015 */
+static void use_acpi_alarm_quirks(void)
+{
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
+		return;
+
+	if (!is_hpet_enabled())
+		return;
+
+	if (dmi_get_bios_year() < 2015)
+		return;
+
+	use_acpi_alarm = true;
+}
+#else
+static inline void use_acpi_alarm_quirks(void) { }
+#endif
+
+static void cmos_wake_setup(struct device *dev)
+{
+	if (acpi_disabled)
+		return;
+
+	use_acpi_alarm_quirks();
+
+	cmos_rtc.wake_on = rtc_wake_on;
+	cmos_rtc.wake_off = rtc_wake_off;
+
+	/* ACPI tables bug workaround. */
+	if (acpi_gbl_FADT.month_alarm && !acpi_gbl_FADT.day_alarm) {
+		dev_dbg(dev, "bogus FADT month_alarm (%d)\n",
+			acpi_gbl_FADT.month_alarm);
+		acpi_gbl_FADT.month_alarm = 0;
+	}
+
+	cmos_rtc.day_alrm = acpi_gbl_FADT.day_alarm;
+	cmos_rtc.mon_alrm = acpi_gbl_FADT.month_alarm;
+	cmos_rtc.century = acpi_gbl_FADT.century;
+
+	if (acpi_gbl_FADT.flags & ACPI_FADT_S4_RTC_WAKE)
+		dev_info(dev, "RTC can wake from S4\n");
+
+	/* RTC always wakes from S1/S2/S3, and often S4/STD */
+	device_init_wakeup(dev, 1);
+}
+
+static void cmos_check_acpi_rtc_status(struct device *dev,
+					      unsigned char *rtc_control)
+{
+	struct cmos_rtc *cmos = dev_get_drvdata(dev);
+	acpi_event_status rtc_status;
+	acpi_status status;
+
+	if (acpi_gbl_FADT.flags & ACPI_FADT_FIXED_RTC)
+		return;
+
+	status = acpi_get_event_status(ACPI_EVENT_RTC, &rtc_status);
+	if (ACPI_FAILURE(status)) {
+		dev_err(dev, "Could not get RTC status\n");
+	} else if (rtc_status & ACPI_EVENT_FLAG_SET) {
+		unsigned char mask;
+		*rtc_control &= ~RTC_AIE;
+		CMOS_WRITE(*rtc_control, RTC_CONTROL);
+		mask = CMOS_READ(RTC_INTR_FLAGS);
+		rtc_update_irq(cmos->rtc, 1, mask);
+	}
+}
+
+#else /* !CONFIG_ACPI */
+
+static inline void rtc_wake_setup(struct device *dev)
+{
+}
+
+static inline void cmos_wake_setup(struct device *dev)
+{
+}
+
+static inline void cmos_check_acpi_rtc_status(struct device *dev,
+					      unsigned char *rtc_control)
+{
+}
+#endif /* CONFIG_ACPI */
 
 #ifdef	CONFIG_PNP
 #define	INITSECTION
@@ -1140,9 +1287,6 @@ static void cmos_check_wkalrm(struct dev
 	}
 }
 
-static void cmos_check_acpi_rtc_status(struct device *dev,
-				       unsigned char *rtc_control);
-
 static int __maybe_unused cmos_resume(struct device *dev)
 {
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
@@ -1209,156 +1353,6 @@ static SIMPLE_DEV_PM_OPS(cmos_pm_ops, cm
  * predate even PNPBIOS should set up platform_bus devices.
  */
 
-#ifdef	CONFIG_ACPI
-
-#include <linux/acpi.h>
-
-static u32 rtc_handler(void *context)
-{
-	struct device *dev = context;
-	struct cmos_rtc *cmos = dev_get_drvdata(dev);
-	unsigned char rtc_control = 0;
-	unsigned char rtc_intr;
-	unsigned long flags;
-
-
-	/*
-	 * Always update rtc irq when ACPI is used as RTC Alarm.
-	 * Or else, ACPI SCI is enabled during suspend/resume only,
-	 * update rtc irq in that case.
-	 */
-	if (cmos_use_acpi_alarm())
-		cmos_interrupt(0, (void *)cmos->rtc);
-	else {
-		/* Fix me: can we use cmos_interrupt() here as well? */
-		spin_lock_irqsave(&rtc_lock, flags);
-		if (cmos_rtc.suspend_ctrl)
-			rtc_control = CMOS_READ(RTC_CONTROL);
-		if (rtc_control & RTC_AIE) {
-			cmos_rtc.suspend_ctrl &= ~RTC_AIE;
-			CMOS_WRITE(rtc_control, RTC_CONTROL);
-			rtc_intr = CMOS_READ(RTC_INTR_FLAGS);
-			rtc_update_irq(cmos->rtc, 1, rtc_intr);
-		}
-		spin_unlock_irqrestore(&rtc_lock, flags);
-	}
-
-	pm_wakeup_hard_event(dev);
-	acpi_clear_event(ACPI_EVENT_RTC);
-	acpi_disable_event(ACPI_EVENT_RTC, 0);
-	return ACPI_INTERRUPT_HANDLED;
-}
-
-static inline void rtc_wake_setup(struct device *dev)
-{
-	if (acpi_disabled)
-		return;
-
-	acpi_install_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler, dev);
-	/*
-	 * After the RTC handler is installed, the Fixed_RTC event should
-	 * be disabled. Only when the RTC alarm is set will it be enabled.
-	 */
-	acpi_clear_event(ACPI_EVENT_RTC);
-	acpi_disable_event(ACPI_EVENT_RTC, 0);
-}
-
-static void rtc_wake_on(struct device *dev)
-{
-	acpi_clear_event(ACPI_EVENT_RTC);
-	acpi_enable_event(ACPI_EVENT_RTC, 0);
-}
-
-static void rtc_wake_off(struct device *dev)
-{
-	acpi_disable_event(ACPI_EVENT_RTC, 0);
-}
-
-#ifdef CONFIG_X86
-/* Enable use_acpi_alarm mode for Intel platforms no earlier than 2015 */
-static void use_acpi_alarm_quirks(void)
-{
-	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
-		return;
-
-	if (!is_hpet_enabled())
-		return;
-
-	if (dmi_get_bios_year() < 2015)
-		return;
-
-	use_acpi_alarm = true;
-}
-#else
-static inline void use_acpi_alarm_quirks(void) { }
-#endif
-
-static void cmos_wake_setup(struct device *dev)
-{
-	if (acpi_disabled)
-		return;
-
-	use_acpi_alarm_quirks();
-
-	cmos_rtc.wake_on = rtc_wake_on;
-	cmos_rtc.wake_off = rtc_wake_off;
-
-	/* ACPI tables bug workaround. */
-	if (acpi_gbl_FADT.month_alarm && !acpi_gbl_FADT.day_alarm) {
-		dev_dbg(dev, "bogus FADT month_alarm (%d)\n",
-			acpi_gbl_FADT.month_alarm);
-		acpi_gbl_FADT.month_alarm = 0;
-	}
-
-	cmos_rtc.day_alrm = acpi_gbl_FADT.day_alarm;
-	cmos_rtc.mon_alrm = acpi_gbl_FADT.month_alarm;
-	cmos_rtc.century = acpi_gbl_FADT.century;
-
-	if (acpi_gbl_FADT.flags & ACPI_FADT_S4_RTC_WAKE)
-		dev_info(dev, "RTC can wake from S4\n");
-
-	/* RTC always wakes from S1/S2/S3, and often S4/STD */
-	device_init_wakeup(dev, 1);
-}
-
-static void cmos_check_acpi_rtc_status(struct device *dev,
-				       unsigned char *rtc_control)
-{
-	struct cmos_rtc *cmos = dev_get_drvdata(dev);
-	acpi_event_status rtc_status;
-	acpi_status status;
-
-	if (acpi_gbl_FADT.flags & ACPI_FADT_FIXED_RTC)
-		return;
-
-	status = acpi_get_event_status(ACPI_EVENT_RTC, &rtc_status);
-	if (ACPI_FAILURE(status)) {
-		dev_err(dev, "Could not get RTC status\n");
-	} else if (rtc_status & ACPI_EVENT_FLAG_SET) {
-		unsigned char mask;
-		*rtc_control &= ~RTC_AIE;
-		CMOS_WRITE(*rtc_control, RTC_CONTROL);
-		mask = CMOS_READ(RTC_INTR_FLAGS);
-		rtc_update_irq(cmos->rtc, 1, mask);
-	}
-}
-
-#else
-
-static void cmos_wake_setup(struct device *dev)
-{
-}
-
-static void cmos_check_acpi_rtc_status(struct device *dev,
-				       unsigned char *rtc_control)
-{
-}
-
-static void rtc_wake_setup(struct device *dev)
-{
-}
-#endif
-
 #ifdef	CONFIG_PNP
 
 #include <linux/pnp.h>

[PATCH v1 4/5] rtc: rtc-cmos: Rename ACPI-related functions

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The names of rtc_wake_setup() and cmos_wake_setup() don't indicate
that these functions are ACPI-related, which is the case, and the
former doesn't really reflect the role of the function.

Rename them to acpi_rtc_event_setup() and cmos_acpi_wake_setup(),
respectively, to address this shortcoming.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/rtc/rtc-cmos.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/rtc/rtc-cmos.c
===================================================================
--- linux-pm.orig/drivers/rtc/rtc-cmos.c
+++ linux-pm/drivers/rtc/rtc-cmos.c
@@ -784,7 +784,7 @@ static u32 rtc_handler(void *context)
 	return ACPI_INTERRUPT_HANDLED;
 }
 
-static void rtc_wake_setup(struct device *dev)
+static void acpi_rtc_event_setup(struct device *dev)
 {
 	if (acpi_disabled)
 		return;
@@ -828,7 +828,7 @@ static void use_acpi_alarm_quirks(void)
 static inline void use_acpi_alarm_quirks(void) { }
 #endif
 
-static void cmos_wake_setup(struct device *dev)
+static void cmos_acpi_wake_setup(struct device *dev)
 {
 	if (acpi_disabled)
 		return;
@@ -880,11 +880,11 @@ static void cmos_check_acpi_rtc_status(s
 
 #else /* !CONFIG_ACPI */
 
-static inline void rtc_wake_setup(struct device *dev)
+static inline void acpi_rtc_event_setup(struct device *dev)
 {
 }
 
-static inline void cmos_wake_setup(struct device *dev)
+static inline void cmos_acpi_wake_setup(struct device *dev)
 {
 }
 
@@ -986,7 +986,7 @@ cmos_do_probe(struct device *dev, struct
 			cmos_rtc.wake_off = info->wake_off;
 		}
 	} else {
-		cmos_wake_setup(dev);
+		cmos_acpi_wake_setup(dev);
 	}
 
 	if (cmos_rtc.day_alrm >= 128)
@@ -1091,7 +1091,7 @@ cmos_do_probe(struct device *dev, struct
 	 * the ACPI RTC fixed event.
 	 */
 	if (!info)
-		rtc_wake_setup(dev);
+		acpi_rtc_event_setup(dev);
 
 	dev_info(dev, "%s%s, %d bytes nvram%s\n",
 		 !is_valid_irq(rtc_irq) ? "no alarms" :

[PATCH v1 5/5] rtc: rtc-cmos: Disable ACPI RTC event on removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make cmos_do_remove() drop the ACPI RTC fixed event handler so as to
prevent it from operating on stale data in case the event triggers
after driver removal.

While at it, make cmos_do_remove() also clear the driver data pointer
of the device and make cmos_acpi_wake_setup() do that in the error path
too.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/rtc/rtc-cmos.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

Index: linux-pm/drivers/rtc/rtc-cmos.c
===================================================================
--- linux-pm.orig/drivers/rtc/rtc-cmos.c
+++ linux-pm/drivers/rtc/rtc-cmos.c
@@ -798,6 +798,12 @@ static inline void acpi_rtc_event_setup(
 	acpi_disable_event(ACPI_EVENT_RTC, 0);
 }
 
+static inline void acpi_rtc_event_cleanup(void)
+{
+	if (!acpi_disabled)
+		acpi_remove_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler);
+}
+
 static void rtc_wake_on(struct device *dev)
 {
 	acpi_clear_event(ACPI_EVENT_RTC);
@@ -884,6 +890,10 @@ static inline void acpi_rtc_event_setup(
 {
 }
 
+static inline void acpi_rtc_event_cleanup(void)
+{
+}
+
 static inline void cmos_acpi_wake_setup(struct device *dev)
 {
 }
@@ -1109,6 +1119,7 @@ cleanup2:
 		free_irq(rtc_irq, cmos_rtc.rtc);
 cleanup1:
 	cmos_rtc.dev = NULL;
+	dev_set_drvdata(dev, NULL);
 cleanup0:
 	if (RTC_IOMAPPED)
 		release_region(ports->start, resource_size(ports));
@@ -1138,6 +1149,9 @@ static void cmos_do_remove(struct device
 			hpet_unregister_irq_handler(cmos_interrupt);
 	}
 
+	if (!dev_get_platdata(dev))
+		acpi_rtc_event_cleanup();
+
 	cmos->rtc = NULL;
 
 	ports = cmos->iomem;
@@ -1148,6 +1162,7 @@ static void cmos_do_remove(struct device
 	cmos->iomem = NULL;
 
 	cmos->dev = NULL;
+	dev_set_drvdata(dev, NULL);
 }
 
 static int cmos_aie_poweroff(struct device *dev)

Re: [PATCH v1 5/5] rtc: rtc-cmos: Disable ACPI RTC event on removal

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make cmos_do_remove() drop the ACPI RTC fixed event handler so as to
> prevent it from operating on stale data in case the event triggers
> after driver removal.
> 
> While at it, make cmos_do_remove() also clear the driver data pointer
> of the device and make cmos_acpi_wake_setup() do that in the error path
> too.

...

> +	dev_set_drvdata(dev, NULL);

> +	dev_set_drvdata(dev, NULL);

Maybe I'm missing something, but the cmos_do_remove() is called by ->remove()
callback of the real drivers (pnp and platform) and device core is already
doing this. So, don't know why you need these calls to be explicit.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/5] rtc: rtc-cmos: Rename ACPI-related functions

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 09:01:50PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The names of rtc_wake_setup() and cmos_wake_setup() don't indicate
> that these functions are ACPI-related, which is the case, and the
> former doesn't really reflect the role of the function.
> 
> Rename them to acpi_rtc_event_setup() and cmos_acpi_wake_setup(),
> respectively, to address this shortcoming.

Hmm... I'm not sure I understand why in one case acpi is a prefix and
in the other is kinda mid-suffix?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] rtc: rtc-cmos: Disable ACPI RTC event on removal

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:

...

> +static inline void acpi_rtc_event_cleanup(void)
> +{
> +	if (!acpi_disabled)

Btw, other functions look like using

	if (acpi_disabled)
		return;

pattern. Maybe here the same for the sake of consistency?

> +		acpi_remove_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] rtc: rtc-cmos: Call cmos_wake_setup() from cmos_do_probe()

[Zhang Rui]

On Mon, 2022-11-07 at 20:59 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notice that cmos_wake_setup() is the only user of acpi_rtc_info and
> it
> can operate on the cmos_rtc variable directly, so it need not set the
> platform_data pointer before cmos_do_probe() is called.  Instead, it
> can be called by cmos_do_probe() in the case when the platform_data
> pointer is not set to implement the default behavior (which is to use
> the FADT information as long as ACPI support is enabled).
> 

...

>  
> @@ -827,19 +829,27 @@ cmos_do_probe(struct device *dev, struct
>  		if (info->address_space)
>  			address_space = info->address_space;
>  
> -		if (info->rtc_day_alarm && info->rtc_day_alarm < 128)
> -			cmos_rtc.day_alrm = info->rtc_day_alarm;
> -		if (info->rtc_mon_alarm && info->rtc_mon_alarm < 128)
> -			cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> -		if (info->rtc_century && info->rtc_century < 128)
> -			cmos_rtc.century = info->rtc_century;
> +		cmos_rtc.day_alrm = info->rtc_day_alarm;
> +		cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> +		cmos_rtc.century = info->rtc_century;
>  
>  		if (info->wake_on && info->wake_off) {
>  			cmos_rtc.wake_on = info->wake_on;
>  			cmos_rtc.wake_off = info->wake_off;
>  		}
> +	} else {
> +		cmos_wake_setup(dev);
>  	}
>  
> 

Previously, before commit a474aaedac99 ("rtc-cmos: move wake setup from
ACPI glue into RTC driver"), dev->platform_data is set in
drivers/acpi/glue.c, and the above commit moves it to cmos_wake_setup()
in this file.

Now, with this patch, my understanding is that dev->platform_data is
never set, thus we can remove the 'info' variable and the
	if (info)
check above.

thanks,
rui

Re: [PATCH v1 1/5] rtc: rtc-cmos: Call cmos_wake_setup() from cmos_do_probe()

[Rafael J. Wysocki]

On Tue, Nov 8, 2022 at 3:31 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Mon, 2022-11-07 at 20:59 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Notice that cmos_wake_setup() is the only user of acpi_rtc_info and
> > it
> > can operate on the cmos_rtc variable directly, so it need not set the
> > platform_data pointer before cmos_do_probe() is called.  Instead, it
> > can be called by cmos_do_probe() in the case when the platform_data
> > pointer is not set to implement the default behavior (which is to use
> > the FADT information as long as ACPI support is enabled).
> >
>
> ...
>
> >
> > @@ -827,19 +829,27 @@ cmos_do_probe(struct device *dev, struct
> >               if (info->address_space)
> >                       address_space = info->address_space;
> >
> > -             if (info->rtc_day_alarm && info->rtc_day_alarm < 128)
> > -                     cmos_rtc.day_alrm = info->rtc_day_alarm;
> > -             if (info->rtc_mon_alarm && info->rtc_mon_alarm < 128)
> > -                     cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> > -             if (info->rtc_century && info->rtc_century < 128)
> > -                     cmos_rtc.century = info->rtc_century;
> > +             cmos_rtc.day_alrm = info->rtc_day_alarm;
> > +             cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> > +             cmos_rtc.century = info->rtc_century;
> >
> >               if (info->wake_on && info->wake_off) {
> >                       cmos_rtc.wake_on = info->wake_on;
> >                       cmos_rtc.wake_off = info->wake_off;
> >               }
> > +     } else {
> > +             cmos_wake_setup(dev);
> >       }
> >
> >
>
> Previously, before commit a474aaedac99 ("rtc-cmos: move wake setup from
> ACPI glue into RTC driver"), dev->platform_data is set in
> drivers/acpi/glue.c, and the above commit moves it to cmos_wake_setup()
> in this file.
>
> Now, with this patch, my understanding is that dev->platform_data is
> never set, thus we can remove the 'info' variable and the
>         if (info)
> check above.

There are other users of this driver which can be found by grepping
for cmos_rtc_board_info.

They create platform device objects with platform_data set which are
then bound to by this driver.

Re: [PATCH v1 4/5] rtc: rtc-cmos: Rename ACPI-related functions

[Rafael J. Wysocki]

On Mon, Nov 7, 2022 at 10:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Nov 07, 2022 at 09:01:50PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The names of rtc_wake_setup() and cmos_wake_setup() don't indicate
> > that these functions are ACPI-related, which is the case, and the
> > former doesn't really reflect the role of the function.
> >
> > Rename them to acpi_rtc_event_setup() and cmos_acpi_wake_setup(),
> > respectively, to address this shortcoming.
>
> Hmm... I'm not sure I understand why in one case acpi is a prefix and
> in the other is kinda mid-suffix?

Because the former installs an ACPI RTC fixed event handler and the
latter populates the cmos_rtc data structure in the ACPI case.

Maybe it would be better to call the latter cmos_wake_setup_acpi().

Re: [PATCH v1 5/5] rtc: rtc-cmos: Disable ACPI RTC event on removal

[Rafael J. Wysocki]

On Mon, Nov 7, 2022 at 10:21 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Make cmos_do_remove() drop the ACPI RTC fixed event handler so as to
> > prevent it from operating on stale data in case the event triggers
> > after driver removal.
> >
> > While at it, make cmos_do_remove() also clear the driver data pointer
> > of the device and make cmos_acpi_wake_setup() do that in the error path
> > too.
>
> ...
>
> > +     dev_set_drvdata(dev, NULL);
>
> > +     dev_set_drvdata(dev, NULL);
>
> Maybe I'm missing something, but the cmos_do_remove() is called by ->remove()
> callback of the real drivers (pnp and platform) and device core is already
> doing this. So, don't know why you need these calls to be explicit.

Good point, but then I guess I should move this patch to the front,
because the issue fixed by it may trigger a use-after-free in
rtc_handler() already.

Re: [PATCH v1 5/5] rtc: rtc-cmos: Disable ACPI RTC event on removal

[Rafael J. Wysocki]

On Mon, Nov 7, 2022 at 10:28 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Nov 07, 2022 at 09:03:06PM +0100, Rafael J. Wysocki wrote:
>
> ...
>
> > +static inline void acpi_rtc_event_cleanup(void)
> > +{
> > +     if (!acpi_disabled)
>
> Btw, other functions look like using
>
>         if (acpi_disabled)
>                 return;
>
> pattern. Maybe here the same for the sake of consistency?
>
> > +             acpi_remove_fixed_event_handler(ACPI_EVENT_RTC, rtc_handler);
> > +}
>

Well, it is more lines of code, but whatever.

Re: [PATCH v1 1/5] rtc: rtc-cmos: Call cmos_wake_setup() from cmos_do_probe()

[Zhang Rui]

On Tue, 2022-11-08 at 14:09 +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 8, 2022 at 3:31 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > On Mon, 2022-11-07 at 20:59 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Notice that cmos_wake_setup() is the only user of acpi_rtc_info
> > > and
> > > it
> > > can operate on the cmos_rtc variable directly, so it need not set
> > > the
> > > platform_data pointer before cmos_do_probe() is called.  Instead,
> > > it
> > > can be called by cmos_do_probe() in the case when the
> > > platform_data
> > > pointer is not set to implement the default behavior (which is to
> > > use
> > > the FADT information as long as ACPI support is enabled).
> > > 
> > 
> > ...
> > 
> > > @@ -827,19 +829,27 @@ cmos_do_probe(struct device *dev, struct
> > >               if (info->address_space)
> > >                       address_space = info->address_space;
> > > 
> > > -             if (info->rtc_day_alarm && info->rtc_day_alarm <
> > > 128)
> > > -                     cmos_rtc.day_alrm = info->rtc_day_alarm;
> > > -             if (info->rtc_mon_alarm && info->rtc_mon_alarm <
> > > 128)
> > > -                     cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> > > -             if (info->rtc_century && info->rtc_century < 128)
> > > -                     cmos_rtc.century = info->rtc_century;
> > > +             cmos_rtc.day_alrm = info->rtc_day_alarm;
> > > +             cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> > > +             cmos_rtc.century = info->rtc_century;
> > > 
> > >               if (info->wake_on && info->wake_off) {
> > >                       cmos_rtc.wake_on = info->wake_on;
> > >                       cmos_rtc.wake_off = info->wake_off;
> > >               }
> > > +     } else {
> > > +             cmos_wake_setup(dev);
> > >       }
> > > 
> > > 
> > 
> > Previously, before commit a474aaedac99 ("rtc-cmos: move wake setup
> > from
> > ACPI glue into RTC driver"), dev->platform_data is set in
> > drivers/acpi/glue.c, and the above commit moves it to
> > cmos_wake_setup()
> > in this file.
> > 
> > Now, with this patch, my understanding is that dev->platform_data
> > is
> > never set, thus we can remove the 'info' variable and the
> >         if (info)
> > check above.
> 
> There are other users of this driver which can be found by grepping
> for cmos_rtc_board_info.
> 
> They create platform device objects with platform_data set which are
> then bound to by this driver.

yeah, I overlooked this.

thanks,
rui