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: <a7d71bff-8577-09f5-5014-5987da1b7c6b@intel.com>
Date:   Fri, 30 Jun 2023 11:42:31 +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 04/10] acpi/video: 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-5-michal.wilczynski@intel.com>
 <CAJZ5v0hZ-2Ee0DCcgzHJrOikRTOJuCbEbQj24xui_-vmbd+47Q@mail.gmail.com>
From:   "Wilczynski, Michal" <michal.wilczynski@intel.com>
In-Reply-To: <CAJZ5v0hZ-2Ee0DCcgzHJrOikRTOJuCbEbQj24xui_-vmbd+47Q@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?dWMwNHBLeStJMEF5WWdBaitkY0pNS2k2Ri94M1NldktSTXpLUSs5dXhxRkJp?=
 =?utf-8?B?VURpbTdKMGduNGRVRmh6d2U3NXc4YXJnd2hPWGd2TGkxeDlYTWl1bm5pbEtE?=
 =?utf-8?B?SUwva3psaEZ3aDY5L3V6MGordUVvVnBqZGNsY2FibmdOazMvc2NTMktqYTZR?=
 =?utf-8?B?U2tZZ3NlN3hQdVVUbGZoT3BRQVcweHRlY3paTzdMMzNOMHQ0WVZ5RXY1NCtI?=
 =?utf-8?B?YllHSm9KcjNoU0U1aUdSdzNTcWJROGlmUGc3WFgxa21ma1RpQjBjWUdrMHFp?=
 =?utf-8?B?SS9XQW1aOVJsVUFsN1ZFK1Z1UkZyczN3bm5RczM3N1B2RndWdGU1VHJCeE5F?=
 =?utf-8?B?OHZSMDl2TXF0Y2dEaVBwSDhCVWcvc3JRL3pZUEtBUU9mWW1vWHRLWEZUUDhV?=
 =?utf-8?B?OERNbnVWbzB1LzNIcC9JTE5tY1lBYW55K2IzdldPOTVUY1BZZkRjOHFvcFN2?=
 =?utf-8?B?Z1lGVlhIWWtFM2I1azJUUG1aUWNwUjBHMHhoTkRvVXBNRnRjN3BTUTZzQSth?=
 =?utf-8?B?amZaY2dNSmxWRFpkb2Nqc3FhQU5oUDFSRHZKVC8vdnlPNmJWQmdpbENBVEhB?=
 =?utf-8?B?RElCRENidWlpZUJ2QlZjSHorSnc5MlQ2UnVjMzFoUC9MR2FjY0dES1QzbHd2?=
 =?utf-8?B?MktaYU1EV1dDQlp1QXpLazVXNTAwVEVnay8rT3J6cWhiWm9QR1FIVnRKbHVK?=
 =?utf-8?B?Rk9udkY0RTFlM2Z5VEFLQkNVRjZWd1BYM1VtZndjcWdaWUNBbjhrNnpFNlRi?=
 =?utf-8?B?V2E0OWtKdW1IMWg3NXdJbDNnckJnNTNzRFBvaDlpOXlRbXVOZ1BNNmFGaThO?=
 =?utf-8?B?YUlIMERZd0t1b2dKRUVqdDdHZnVyT2hZcVh2OTdRZDVOOUtCMk9RUjhNYVAw?=
 =?utf-8?B?ZHBSb0k3ZzI0Y0VkM1lWdENITm5XNUYzYThrM3hnb3l3bU5hSWgxVzYzemVC?=
 =?utf-8?B?ek55M2NNVmlKUWVHSU0zYUJEa3pieTJrR1ppNDQ2QmVhRjBZaHlVeUZIYThF?=
 =?utf-8?B?NUt0ak11ME9sYmpDSGZ0andKTXpCRzI2UVA5dmVDek44Mkd6WU8xWnNmV0pK?=
 =?utf-8?B?K0tNNnBxUWRJbkxTZGRaZFFtZkkxcTlJZXRWVDIxVy9taE05V1JzcEFPRzFy?=
 =?utf-8?B?N2NSM0NLNXMxUWRNYzVtZnh0YXFLOHZFTVF6MTJ4Sk9IeDg2VDlGQzJCeXdZ?=
 =?utf-8?B?a3ViMlFIYUVjd2VwbGNmVlNBaGhxQ2pETTdiRWE4Snh6WlhxYkh5eUttZVp0?=
 =?utf-8?B?Nzk1Y2hkSlBwRm55YklwUlBTZkEwYVhLZ0ZPYzE2UFBuTEJHQWs1Ri93aXFn?=
 =?utf-8?B?anRLWkhENkVWTjlIUEF3czF6V1dtaTg4OStXbkllU29va2tYVzV0ODhYVmcv?=
 =?utf-8?B?RUF3d3ZVTHRuRHZlcEpZdDNxR1lwNHdYT0lRaDlzSzNscXQ1QitNU2VPNjBh?=
 =?utf-8?B?TkVGWElJbkcyYlZqdytCamNnOHBHS2l3bzI2NFNMcEppeityRUV3bi9CUk11?=
 =?utf-8?B?WlhyU0xEeFhWUVBuWjl4WUpDRFlrOTlZWEZoaWc0UmhhWThKdFZYK3QxVG9p?=
 =?utf-8?B?YUVNc09Eb2RLcW5PQXNlbGlDaWs1K3VmS2xVN2c5YllzalpFamtINkd6NXB6?=
 =?utf-8?B?RUZGeEt1NUlJVmtERGZZSHBCQ3BmRjVEQ2lTalkvZ3NLNGl4ME1KQVhZM1V1?=
 =?utf-8?B?TmQvSWJja2tiRmh0eXQxSEF6MGZnWGlhV1FRVnFmYk5tWS9ocVBVcFI0a0Rt?=
 =?utf-8?B?ZmpyVkRQajlRRDhFRnhnOE5FckxXV3ZhSWl2M2RwdzFWUGVabysxUkpWd1JR?=
 =?utf-8?B?N25GOWZZdlp3NmJJNXhHSHA0YlhPa0FUQ0V1dFp1amwvbno1UWhrK2Y0VjZS?=
 =?utf-8?B?UGdxa0l5aHhScy9xTGtGNXNiNzdlc2VBOU42eTZ3SlZwQWtEbUJ0SUo1eXlN?=
 =?utf-8?B?QkZYdW5iQ3VUWE5FNjJHbVlNNmVadXUydnJ5MEd0eDJWUTF6RUJ5MnoyVmZr?=
 =?utf-8?B?ZHpjeFVJeFhGay90aE5XMU5uU1AwU1MvUExjQUM3bzNhc2szN3RpREhQbjdw?=
 =?utf-8?B?a0NHZnZVSWE3Qk5oYzBkWjczOVkxcmY5ZnJORUd5LzlPOCt6YzZjNktCYW5z?=
 =?utf-8?B?UVBhM2JYVS9RV29Nc0NtdHpkYlVscnFkRVNnSCtNZTBDSG0zYStUUW1IcVM1?=
 =?utf-8?B?eEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 22d58d20-c0d7-4182-5506-08db794e5711
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:42:38.0745
 (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: PHn7voEAp0UxrFEGpWPNrhTPhALu6/e4unTmoOqgHiyD4fCx7FPgpIZweh3UylleP7T5m70Ui0DRzLxleyG1+aAQLUu9HQqDwNDHexhSBd8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5486
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,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 5:58 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.
>>
>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> ---
>>  drivers/acpi/acpi_video.c | 26 ++++++++++++++++++++++----
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>> index 62f4364e4460..60b7013d0009 100644
>> --- a/drivers/acpi/acpi_video.c
>> +++ b/drivers/acpi/acpi_video.c
>> @@ -77,7 +77,7 @@ static DEFINE_MUTEX(video_list_lock);
>>  static LIST_HEAD(video_bus_head);
>>  static int acpi_video_bus_add(struct acpi_device *device);
>>  static void acpi_video_bus_remove(struct acpi_device *device);
>> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
>> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data);
>>
>>  /*
>>   * Indices in the _BCL method response: the first two items are special,
>> @@ -104,7 +104,6 @@ static struct acpi_driver acpi_video_bus = {
>>         .ops = {
>>                 .add = acpi_video_bus_add,
>>                 .remove = acpi_video_bus_remove,
>> -               .notify = acpi_video_bus_notify,
>>                 },
>>  };
>>
>> @@ -1527,12 +1526,15 @@ static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
>>                                   acpi_osi_is_win8() ? 0 : 1);
>>  }
>>
>> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
>> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
>>  {
>> -       struct acpi_video_bus *video = acpi_driver_data(device);
>> +       struct acpi_device *device = data;
>> +       struct acpi_video_bus *video;
>>         struct input_dev *input;
>>         int keycode = 0;
>>
>> +       video = acpi_driver_data(device);
>> +
>>         if (!video || !video->input)
>>                 return;
>>
>> @@ -2053,8 +2055,20 @@ static int acpi_video_bus_add(struct acpi_device *device)
>>
>>         acpi_video_bus_add_notify_handler(video);
>>
>> +       error = acpi_dev_install_notify_handler(device,
>> +                                               ACPI_DEVICE_NOTIFY,
>> +                                               acpi_video_bus_notify);
>> +       if (error)
>> +               goto err_remove_and_unregister_video;
> This label name is a bit too long and the second half of it doesn't
> really add any value IMV.  err_remove would be sufficient.

I've seen different patterns in the code, sometimes the label describe what failed,
sometimes it describe what needs to be cleaned up. I don't really have a strong
preference, just thought it might be useful to the reader. Will change as suggested.

>
>> +
>>         return 0;
>>
>> +err_remove_and_unregister_video:
>> +       mutex_lock(&video_list_lock);
>> +       list_del(&video->entry);
>> +       mutex_unlock(&video_list_lock);
>> +       acpi_video_bus_remove_notify_handler(video);
>> +       acpi_video_bus_unregister_backlight(video);
>>  err_put_video:
>>         acpi_video_bus_put_devices(video);
>>         kfree(video->attached_array);
>> @@ -2075,6 +2089,10 @@ static void acpi_video_bus_remove(struct acpi_device *device)
>>
>>         video = acpi_driver_data(device);
>>
>> +       acpi_dev_remove_notify_handler(device,
>> +                                      ACPI_DEVICE_NOTIFY,
>> +                                      acpi_video_bus_notify);
>> +
>>         mutex_lock(&video_list_lock);
>>         list_del(&video->entry);
>>         mutex_unlock(&video_list_lock);
>> --
>> 2.41.0
>>