Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f875638a-ea61-04cc-7621-974d4bd2dd34@intel.com>
Date:   Fri, 30 Jun 2023 11:48:51 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v5 05/10] acpi/battery: Move handler installing logic to
 driver
Content-Language: en-US
To:     "Rafael J. Wysocki" <rafael@kernel.org>
CC:     <linux-acpi@vger.kernel.org>, <dan.j.williams@intel.com>,
        <vishal.l.verma@intel.com>, <lenb@kernel.org>,
        <dave.jiang@intel.com>, <ira.weiny@intel.com>,
        <rui.zhang@intel.com>, <linux-kernel@vger.kernel.org>,
        <nvdimm@lists.linux.dev>,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>
References: <20230616165034.3630141-1-michal.wilczynski@intel.com>
 <20230616165034.3630141-6-michal.wilczynski@intel.com>
 <CAJZ5v0i1-7p-V0nFHoNWHDXRZ-xvVwTXJKM387PTP1d4k7Wrdg@mail.gmail.com>
From:   "Wilczynski, Michal" <michal.wilczynski@intel.com>
In-Reply-To: <CAJZ5v0i1-7p-V0nFHoNWHDXRZ-xvVwTXJKM387PTP1d4k7Wrdg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TDcyV0FmTTZjSDBUR3NBTVljb3VtbnVud1FzMXh5WG9xVzNJeEtidGY1SlVr?=
 =?utf-8?B?QThBSDFBTlZKUTdidUFSSlJ5NERPdWc1ZU45WjFsSUVHeUYvcjExN09EWWdi?=
 =?utf-8?B?Sm93MHQwM0dUT0ZUVm90Tys2b3F3R1M4NTdmVVVTQjI1UVZBZjd5dHI4dEV0?=
 =?utf-8?B?T0xUNWdGazc0Y0tiRndlakpTMmV1Y0JzYkhvelcrL3dIQkxKSGJidXRmNmE1?=
 =?utf-8?B?TlRDM1NHWWFSSE1zWm94ZGVjZ3BPNjE1VmJjcldHU21lcWFQZlFIRGZwWVRD?=
 =?utf-8?B?ZzcxQUUyNmRlaUhSNWhGc0xUdDR6Y1UvWFVjcFh2b2prWHRmcDJRbkFGUXlp?=
 =?utf-8?B?aWJZT3lZYmhRTVRJUGp2cFNWU0VxdmZnVWNzTWVQY3dZTVdhTkdDV1RlQlBB?=
 =?utf-8?B?TWJBdGxPVGJvNlhXS0Q3c01qZFZ1WVVQNnFicGRUVGkyZEppUWRnMmhEbG5I?=
 =?utf-8?B?MUwrckRnb2dmMDkrWk5LcEZNMDI4YXVianh4QllDSGx4bC9KTlB5TlR4eHcz?=
 =?utf-8?B?YjJMVysrNTdLMXVYY0YwUm5maWdLRDA3QzA3d3JrdzhKNUcvZkJoQkduSmtQ?=
 =?utf-8?B?MmRQSXkzL2NUR3dGSTNpZlp6emluK2ZuenUwZEZSUzI2c1JkbHBQSENueEtV?=
 =?utf-8?B?QzI4bXo0Q29pTEpBci91cEZCa2t1UDN2MVo2VWttaWRRcHI1NWM1L2lpU3Bt?=
 =?utf-8?B?WVB3cW0xNHRtTmtPKzNFWHlPeUd5dVVEVjdDN1JWNWhqYWkzUGFndmJIblZ1?=
 =?utf-8?B?OGd3a1JDbzU3YldBQnJmQUJjWXo2TXN1OUlubDQyN0d5OXNibXo3VjNPaE40?=
 =?utf-8?B?TDZXNElGaDN4c0dXWUlQS2pCZXQ1SXZCcm9Uc2pUUytEVWZsT2xyaWFEbTQ5?=
 =?utf-8?B?OXc1TUxGcFVNamFMZEhkdktGNW5GRFhHOG5tNEY2WjMzRFZHNWNRbzdvcGxZ?=
 =?utf-8?B?MWVlZHhCbjF3TDcyaEp4YXBoWFNuc0hJQ1FtL2pudkpqeUYvK0h2SXo1UlZ1?=
 =?utf-8?B?NHVRQkM5MjFqVnZYUk5ZS2o0bkptSysvc2ZSSThRMXg4OUJPbTlnN09Pa0kz?=
 =?utf-8?B?OWMxYkJMemNNZm93YmxlTWd1UHBubHFyMkpuRmFMUUdhaGxITTErMnlFdmxW?=
 =?utf-8?B?Tk9oUjRoRE9pT0d5WFdXc3BpZ2ZadXhYaWdQRW9jUkd1MXNiRmpGanNySDJz?=
 =?utf-8?B?azlBNkRZYUt0dDhOK2xZM3AvVHJtazJCMTZaY3VXVU1DTi9tMlVzNlhQMXdN?=
 =?utf-8?B?S0JqNmFvRGx0UHBxUTJCZ1MvRGZjSGJlVm1KaEp6UWdPSi8rSEVpRldJV0hl?=
 =?utf-8?B?U3JHSm5VVGo5aEJPc0ZITllzMklkV0ZwVUh4Rm03eGJJQ0VlclBJNU1nZ05B?=
 =?utf-8?B?U0tqenVwQTBLb29WenE4bVJYaktUTGx4eHJKTlJaRzkyRnkxT3FUaEVlYWdQ?=
 =?utf-8?B?bjlUN01VZGJtajRYZkRmWmNiaC9neGR4emhxcStaWHg3cHgrTENONmFJYmRt?=
 =?utf-8?B?OFJTb3NIYURVTDNubG42U3VhbURnZWtKVGlSL0dvK0tFZi9NLzZXZjEwdHB5?=
 =?utf-8?B?aTlDUXlqZ2VuMzJnQ3AwN3ZhbU9KTFRva2NUV25iczZuemhFdk1KS2VUamtG?=
 =?utf-8?B?SEd2OVIycVVPbU9pYjUxa0gxbDlwSEc1TnYwRVg4NzBQQ0tVajg3endNYXVt?=
 =?utf-8?B?VzUxdUE0OXNta3RKSXV1Vm9TNVVLb1YrLzVMWjYwN2tRV1RGZWRlVnNvYytU?=
 =?utf-8?B?TnF4TFVTMW56NnFVZksvdisxenRIbHUrRmRRR0FseEc0L1hCd3c0dm9SV3ow?=
 =?utf-8?B?aUNNVktRaG9mQ01vR0x3YjU4ZDFVbXVmeDRaZUQ3TTNRb0tSbGp6dEI1WmJq?=
 =?utf-8?B?dmc1VVFLaDZ5UTdtRks4eWc1VUNKcHhUWTVlNDdHdXpKOHd3dTRSWkh1aE1s?=
 =?utf-8?B?bHRMMmd5ZksvVnlOVktVRWNlTVE1OFZES1VYRW1Hdll1M1V6YWpULysvdTAz?=
 =?utf-8?B?ZE9ZQmNLcDZ4T3RTSC9XT0JlZit2cGlqeXVGcU9MNUdZM0J5RWdlbW5ObnpP?=
 =?utf-8?B?Q0JDYXNiRkNDVkIzRm5UbUFNQ3NxcUY2TksvTk1hSHZaSi9LOFBUN2xBc2dh?=
 =?utf-8?B?OXAwQ05RQ0tvREorUG1vMkxuQ1VkK2VtbVQ1ejZoUmhHcnlZWDZwd0ZpODNO?=
 =?utf-8?B?L2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a2f37d3b-ea1a-4394-8d0d-08db794f3969
X-MS-Exchange-CrossTenant-AuthSource: CO6PR11MB5603.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jun 2023 09:48:57.7964
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: XzVFF5jO3DuX8yfV4iC/AA6FcUDXGdTAYKMdvVcUL5xhkbhcLJmfYmiWS7VI9iCaKG0d/kGKoyJqtJC4qVmhJ+C4p2bYjBlzrihVo1jM6Tw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV3PR11MB8602
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/29/2023 6:05 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <michal.wilczynski@intel.com> 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.
>>
>> While at it, fix lack of whitespaces in .remove() callback.
>>
>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> ---
>>  drivers/acpi/battery.c | 30 ++++++++++++++++++++++++------
>>  1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index 9c67ed02d797..6337e7b1f6e1 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -1034,11 +1034,14 @@ static void acpi_battery_refresh(struct acpi_battery *battery)
>>  }
>>
>>  /* Driver Interface */
>> -static void acpi_battery_notify(struct acpi_device *device, u32 event)
>> +static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
>>  {
>> -       struct acpi_battery *battery = acpi_driver_data(device);
>> +       struct acpi_device *device = data;
>> +       struct acpi_battery *battery;
>>         struct power_supply *old;
>>
>> +       battery = acpi_driver_data(device);
>> +
>>         if (!battery)
>>                 return;
>>         old = battery->bat;
>> @@ -1212,13 +1215,23 @@ static int acpi_battery_add(struct acpi_device *device)
>>
>>         device_init_wakeup(&device->dev, 1);
>>
>> -       return result;
>> +       result = acpi_dev_install_notify_handler(device,
>> +                                                ACPI_ALL_NOTIFY,
>> +                                                acpi_battery_notify);
>> +       if (result)
>> +               goto fail_deinit_wakup_and_unregister;
> You could call the label "fail_pm", for example, which would be more
> concise and so slightly easier to follow, without any loss of clarity
> AFAICS.

Sure

>
>> +
>> +       return 0;
>>
>> +fail_deinit_wakup_and_unregister:
>> +       device_init_wakeup(&device->dev, 0);
>> +       unregister_pm_notifier(&battery->pm_nb);
>>  fail:
>>         sysfs_remove_battery(battery);
>>         mutex_destroy(&battery->lock);
>>         mutex_destroy(&battery->sysfs_lock);
>>         kfree(battery);
>> +
>>         return result;
>>  }
>>
>> @@ -1228,10 +1241,17 @@ static void acpi_battery_remove(struct acpi_device *device)
>>
>>         if (!device || !acpi_driver_data(device))
>>                 return;
>> -       device_init_wakeup(&device->dev, 0);
>> +
>>         battery = acpi_driver_data(device);
>> +
>> +       acpi_dev_remove_notify_handler(device,
>> +                                      ACPI_ALL_NOTIFY,
>> +                                      acpi_battery_notify);
>> +
>> +       device_init_wakeup(&device->dev, 0);
>>         unregister_pm_notifier(&battery->pm_nb);
>>         sysfs_remove_battery(battery);
>> +
>>         mutex_destroy(&battery->lock);
>>         mutex_destroy(&battery->sysfs_lock);
>>         kfree(battery);
>> @@ -1264,11 +1284,9 @@ static struct acpi_driver acpi_battery_driver = {
>>         .name = "battery",
>>         .class = ACPI_BATTERY_CLASS,
>>         .ids = battery_device_ids,
>> -       .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
>>         .ops = {
>>                 .add = acpi_battery_add,
>>                 .remove = acpi_battery_remove,
>> -               .notify = acpi_battery_notify,
>>                 },
>>         .drv.pm = &acpi_battery_pm,
>>  };
>> --
>> 2.41.0
>>