Currently logic for installing notifications from ACPI devices is
implemented using notify callback in struct acpi_driver. Preparations
are being made to replace acpi_driver with more generic struct
platform_driver, which doesn't contain notify callback. Furthermore
as of now handlers are being called indirectly through
acpi_notify_device(), which decreases performance.
Call acpi_dev_install_notify_handler() at the end of .add() callback.
Call acpi_dev_remove_notify_handler() at the beginning of .remove()
callback. Change arguments passed to the notify function to match with
what's required by acpi_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
drivers/acpi/ac.c | 33 ++++++++++++++++++++++++---------
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 1ace70b831cd..207ee3c85bad 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
static int acpi_ac_add(struct acpi_device *device);
static void acpi_ac_remove(struct acpi_device *device);
-static void acpi_ac_notify(struct acpi_device *device, u32 event);
+static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
static const struct acpi_device_id ac_device_ids[] = {
{"ACPI0003", 0},
@@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
.name = "ac",
.class = ACPI_AC_CLASS,
.ids = ac_device_ids,
- .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
.ops = {
.add = acpi_ac_add,
.remove = acpi_ac_remove,
- .notify = acpi_ac_notify,
},
.drv.pm = &acpi_ac_pm,
};
@@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
};
/* Driver Model */
-static void acpi_ac_notify(struct acpi_device *device, u32 event)
+static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
{
- struct acpi_ac *ac = acpi_driver_data(device);
+ struct acpi_device *device = data;
+ struct acpi_ac *ac;
+
+ ac = acpi_driver_data(device);
if (!ac)
return;
@@ -235,7 +236,7 @@ static int acpi_ac_add(struct acpi_device *device)
result = acpi_ac_get_state(ac);
if (result)
- goto end;
+ goto err_release_ac;
psy_cfg.drv_data = ac;
@@ -248,7 +249,7 @@ static int acpi_ac_add(struct acpi_device *device)
&ac->charger_desc, &psy_cfg);
if (IS_ERR(ac->charger)) {
result = PTR_ERR(ac->charger);
- goto end;
+ goto err_release_ac;
}
pr_info("%s [%s] (%s)\n", acpi_device_name(device),
@@ -256,9 +257,20 @@ static int acpi_ac_add(struct acpi_device *device)
ac->battery_nb.notifier_call = acpi_ac_battery_notify;
register_acpi_notifier(&ac->battery_nb);
-end:
+
+ result = acpi_dev_install_notify_handler(device,
+ ACPI_ALL_NOTIFY,
+ acpi_ac_notify);
if (result)
- kfree(ac);
+ goto err_unregister;
+
+ return 0;
+
+err_unregister:
+ power_supply_unregister(ac->charger);
+ unregister_acpi_notifier(&ac->battery_nb);
+err_release_ac:
+ kfree(ac);
return result;
}
@@ -297,6 +309,9 @@ static void acpi_ac_remove(struct acpi_device *device)
ac = acpi_driver_data(device);
+ acpi_dev_remove_notify_handler(device,
+ ACPI_ALL_NOTIFY,
+ acpi_ac_notify);
power_supply_unregister(ac->charger);
unregister_acpi_notifier(&ac->battery_nb);
--
2.41.0
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<[email protected]> wrote:
>
> Currently logic for installing notifications from ACPI devices is
> implemented using notify callback in struct acpi_driver. Preparations
> are being made to replace acpi_driver with more generic struct
> platform_driver, which doesn't contain notify callback. Furthermore
> as of now handlers are being called indirectly through
> acpi_notify_device(), which decreases performance.
>
> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify function to match with
> what's required by acpi_install_notify_handler(). Remove .notify
> callback initialization in acpi_driver.
>
> Suggested-by: Rafael J. Wysocki <[email protected]>
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> drivers/acpi/ac.c | 33 ++++++++++++++++++++++++---------
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> index 1ace70b831cd..207ee3c85bad 100644
> --- a/drivers/acpi/ac.c
> +++ b/drivers/acpi/ac.c
> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
>
> static int acpi_ac_add(struct acpi_device *device);
> static void acpi_ac_remove(struct acpi_device *device);
> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
>
> static const struct acpi_device_id ac_device_ids[] = {
> {"ACPI0003", 0},
> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
> .name = "ac",
> .class = ACPI_AC_CLASS,
> .ids = ac_device_ids,
> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> .ops = {
> .add = acpi_ac_add,
> .remove = acpi_ac_remove,
> - .notify = acpi_ac_notify,
> },
> .drv.pm = &acpi_ac_pm,
> };
> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
> };
>
> /* Driver Model */
> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_ac *ac = acpi_driver_data(device);
This line doesn't need to be changed. Just add the device variable
definition above it.
And the same pattern is present in the other patches in the series.
> + struct acpi_device *device = data;
> + struct acpi_ac *ac;
> +
> + ac = acpi_driver_data(device);
>
> if (!ac)
> return;
On 6/29/2023 5:55 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <[email protected]> wrote:
>> Currently logic for installing notifications from ACPI devices is
>> implemented using notify callback in struct acpi_driver. Preparations
>> are being made to replace acpi_driver with more generic struct
>> platform_driver, which doesn't contain notify callback. Furthermore
>> as of now handlers are being called indirectly through
>> acpi_notify_device(), which decreases performance.
>>
>> Call acpi_dev_install_notify_handler() at the end of .add() callback.
>> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
>> callback. Change arguments passed to the notify function to match with
>> what's required by acpi_install_notify_handler(). Remove .notify
>> callback initialization in acpi_driver.
>>
>> Suggested-by: Rafael J. Wysocki <[email protected]>
>> Signed-off-by: Michal Wilczynski <[email protected]>
>> ---
>> drivers/acpi/ac.c | 33 ++++++++++++++++++++++++---------
>> 1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
>> index 1ace70b831cd..207ee3c85bad 100644
>> --- a/drivers/acpi/ac.c
>> +++ b/drivers/acpi/ac.c
>> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
>>
>> static int acpi_ac_add(struct acpi_device *device);
>> static void acpi_ac_remove(struct acpi_device *device);
>> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
>> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
>>
>> static const struct acpi_device_id ac_device_ids[] = {
>> {"ACPI0003", 0},
>> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
>> .name = "ac",
>> .class = ACPI_AC_CLASS,
>> .ids = ac_device_ids,
>> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
>> .ops = {
>> .add = acpi_ac_add,
>> .remove = acpi_ac_remove,
>> - .notify = acpi_ac_notify,
>> },
>> .drv.pm = &acpi_ac_pm,
>> };
>> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
>> };
>>
>> /* Driver Model */
>> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
>> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
>> {
>> - struct acpi_ac *ac = acpi_driver_data(device);
> This line doesn't need to be changed. Just add the device variable
> definition above it.
>
> And the same pattern is present in the other patches in the series.
I like the Reverse Christmas Tree, but sure will change that
>
>> + struct acpi_device *device = data;
>> + struct acpi_ac *ac;
>> +
>> + ac = acpi_driver_data(device);
>>
>> if (!ac)
>> return;
On Fri, Jun 30, 2023 at 11:41 AM Rafael J. Wysocki <[email protected]> wrote:
>
> On Fri, Jun 30, 2023 at 11:39 AM Wilczynski, Michal
> <[email protected]> wrote:
> >
> >
> >
> > On 6/29/2023 5:55 PM, Rafael J. Wysocki wrote:
> > > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > > <[email protected]> wrote:
> > >> Currently logic for installing notifications from ACPI devices is
> > >> implemented using notify callback in struct acpi_driver. Preparations
> > >> are being made to replace acpi_driver with more generic struct
> > >> platform_driver, which doesn't contain notify callback. Furthermore
> > >> as of now handlers are being called indirectly through
> > >> acpi_notify_device(), which decreases performance.
> > >>
> > >> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> > >> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> > >> callback. Change arguments passed to the notify function to match with
> > >> what's required by acpi_install_notify_handler(). Remove .notify
> > >> callback initialization in acpi_driver.
> > >>
> > >> Suggested-by: Rafael J. Wysocki <[email protected]>
> > >> Signed-off-by: Michal Wilczynski <[email protected]>
> > >> ---
> > >> drivers/acpi/ac.c | 33 ++++++++++++++++++++++++---------
> > >> 1 file changed, 24 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> > >> index 1ace70b831cd..207ee3c85bad 100644
> > >> --- a/drivers/acpi/ac.c
> > >> +++ b/drivers/acpi/ac.c
> > >> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
> > >>
> > >> static int acpi_ac_add(struct acpi_device *device);
> > >> static void acpi_ac_remove(struct acpi_device *device);
> > >> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
> > >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
> > >>
> > >> static const struct acpi_device_id ac_device_ids[] = {
> > >> {"ACPI0003", 0},
> > >> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
> > >> .name = "ac",
> > >> .class = ACPI_AC_CLASS,
> > >> .ids = ac_device_ids,
> > >> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> > >> .ops = {
> > >> .add = acpi_ac_add,
> > >> .remove = acpi_ac_remove,
> > >> - .notify = acpi_ac_notify,
> > >> },
> > >> .drv.pm = &acpi_ac_pm,
> > >> };
> > >> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
> > >> };
> > >>
> > >> /* Driver Model */
> > >> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
> > >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
> > >> {
> > >> - struct acpi_ac *ac = acpi_driver_data(device);
> > > This line doesn't need to be changed. Just add the device variable
> > > definition above it.
> > >
> > > And the same pattern is present in the other patches in the series.
> >
> > I like the Reverse Christmas Tree, but sure will change that
>
> I do too, but in the cases when it costs 3 extra lines of code I'd
> rather have something simpler.
Besides, moving code around is not strictly related to the functional
changes made by the patch and it kind of hides those changes. It is
better to move code around in a separate patch if you really want to
do that.
On Fri, Jun 30, 2023 at 11:39 AM Wilczynski, Michal
<[email protected]> wrote:
>
>
>
> On 6/29/2023 5:55 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > <[email protected]> wrote:
> >> Currently logic for installing notifications from ACPI devices is
> >> implemented using notify callback in struct acpi_driver. Preparations
> >> are being made to replace acpi_driver with more generic struct
> >> platform_driver, which doesn't contain notify callback. Furthermore
> >> as of now handlers are being called indirectly through
> >> acpi_notify_device(), which decreases performance.
> >>
> >> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> >> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> >> callback. Change arguments passed to the notify function to match with
> >> what's required by acpi_install_notify_handler(). Remove .notify
> >> callback initialization in acpi_driver.
> >>
> >> Suggested-by: Rafael J. Wysocki <[email protected]>
> >> Signed-off-by: Michal Wilczynski <[email protected]>
> >> ---
> >> drivers/acpi/ac.c | 33 ++++++++++++++++++++++++---------
> >> 1 file changed, 24 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> >> index 1ace70b831cd..207ee3c85bad 100644
> >> --- a/drivers/acpi/ac.c
> >> +++ b/drivers/acpi/ac.c
> >> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
> >>
> >> static int acpi_ac_add(struct acpi_device *device);
> >> static void acpi_ac_remove(struct acpi_device *device);
> >> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
> >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
> >>
> >> static const struct acpi_device_id ac_device_ids[] = {
> >> {"ACPI0003", 0},
> >> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
> >> .name = "ac",
> >> .class = ACPI_AC_CLASS,
> >> .ids = ac_device_ids,
> >> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> >> .ops = {
> >> .add = acpi_ac_add,
> >> .remove = acpi_ac_remove,
> >> - .notify = acpi_ac_notify,
> >> },
> >> .drv.pm = &acpi_ac_pm,
> >> };
> >> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
> >> };
> >>
> >> /* Driver Model */
> >> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
> >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
> >> {
> >> - struct acpi_ac *ac = acpi_driver_data(device);
> > This line doesn't need to be changed. Just add the device variable
> > definition above it.
> >
> > And the same pattern is present in the other patches in the series.
>
> I like the Reverse Christmas Tree, but sure will change that
I do too, but in the cases when it costs 3 extra lines of code I'd
rather have something simpler.