mbox series

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

Message ID 2276401.ElGaqSPkdT@kreacher
Headers show
Series rtc: rtc-cmos: Assorted ACPI-related cleanups and fixes | expand

Message

Rafael J. Wysocki Nov. 7, 2022, 7:58 p.m. UTC 
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!

Comments

Andy Shevchenko Nov. 7, 2022, 9:20 p.m. UTC | #1 
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.
Rafael J. Wysocki Nov. 8, 2022, 2:54 p.m. UTC | #2 
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.
Rafael J. Wysocki Nov. 8, 2022, 2:54 p.m. UTC | #3 
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.