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;
Date:   Thu, 29 Jun 2023 13:54:27 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Michal Wilczynski <michal.wilczynski@intel.com>,
        <linux-acpi@vger.kernel.org>
CC:     <rafael@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>,
        Michal Wilczynski <michal.wilczynski@intel.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>
Subject: RE: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to
 driver
Message-ID: <649def832ce1f_11e68529491@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230616165034.3630141-1-michal.wilczynski@intel.com>
 <20230616165034.3630141-10-michal.wilczynski@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230616165034.3630141-10-michal.wilczynski@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?9JafkKq/wrQgisCfwNMN6AOfqYoS1sTCFckxgdosKC1I/NZqQAZOPP+Qd6z9?=
 =?us-ascii?Q?UEi6Fgsk/EXGCeVjeWOVSnnV302ybvuA90nSmmWXF5AA7eygsPzccOVgM0ma?=
 =?us-ascii?Q?ghMcVMMTxVOuIE0gNN300LteVywMSiP0kIMbxYzKetBjA1hKJs8kJFGYPcVJ?=
 =?us-ascii?Q?DZrqVs4NOwrmLvbABrXLs5aFc+U0NBJxl2EomNX1qW1tGvyLAEZckM5ot9DF?=
 =?us-ascii?Q?8frre4vU1mKZICvxbgXJ9xvAq5y36LzQcJUO55paAT1dTpm6LVE7sNqsD6oe?=
 =?us-ascii?Q?FPpqF/Db4aMdGOv6lBbtyNzgd8ei7aM4AVF2EM2CNQ3mla09oTQVlcHaF+z9?=
 =?us-ascii?Q?DYC3RS/OFYsTtD852ZGWoV0ILrg8jjhVbxoztklLQlfsbbncA6QSbZAf3EMa?=
 =?us-ascii?Q?iNmrp0jXUAN8HGOtUSSKtPvg4UJnbmDI3cAfuXXlH/yNPgBG565DoaExvdtz?=
 =?us-ascii?Q?ioZD4DXxWQdEzELxEOL4Xw+m3IFGj09jkcc2dGjaLtSATT0UmoUI8YHUmzXX?=
 =?us-ascii?Q?8ZQOCA6HqdNm2DHBnFotpdu1RGAmz8R+DzsUUmqn3MM528b2+oHBRog4UMJn?=
 =?us-ascii?Q?UwLnEJ7aJFk/dF0X738mJn3J8jnHv9YSoKfL1CT9hze5jrqwsPRTJyK2IdJP?=
 =?us-ascii?Q?EeI2IazerYQQuH7TdQjdM5OQvSScqSqW4DINuXX0P7iEsB7uKe+sgOETmno2?=
 =?us-ascii?Q?aPo1XxYnSgJJyEULU7+VSQeori92Ww9YgMJuutytY3acCZLaTXlrkNN+KP2N?=
 =?us-ascii?Q?m4LppIcz/SFt1SxnHJUK5ff7SmuOA1F/pRtFvTLnnqrfle0bdAQsmVP4gpvH?=
 =?us-ascii?Q?GWp/LIiIScUPGo0LDfz2TC+poByk/0wYojUBpVIYzZK1JYF5EtSuEnmURA1x?=
 =?us-ascii?Q?Tquy3/6WjOeDFZsx630Jp9mvVvceq2NFVAAycnXf8Tsi/U5BRf+w7igqjahF?=
 =?us-ascii?Q?yxALom5KNKghBcC1O7xHPRSGenxeoInjI4B9yGnM4htsS/5ga1bJq4cI9zh6?=
 =?us-ascii?Q?2VFTiBq1SWNaE6zQVNYn989xF7/GBu2CmKfNlhR79JKBUilXDs+V7+Zwj34B?=
 =?us-ascii?Q?8oDyMCLiZG6+aBIvQVSbauKzed6W83m0rQO3eAi+LOYTBG8tS1PTpitK5PFI?=
 =?us-ascii?Q?+eJYXXJFy3hpsI5k0nQcV9j1Roy9u3pLto8Gb38emtKYSUkOsq50ObJu0Het?=
 =?us-ascii?Q?CNHO4kcnxtKi0kelBLcO0YhY5hdACrCaAzhcIjc6+PNgvpKmsNd+o/6Mg7nL?=
 =?us-ascii?Q?2PJHOaJkwZE2Vh7QLmWvBjQeNo7lcNSMuOHPifDEW8oVJqs+jnqib4CAwVmf?=
 =?us-ascii?Q?y4S0sMpwlq3qVLf/FVtzu+EITN0/YdXPwCRvtYZLYQ5SPMEJTEk6WgvXf45a?=
 =?us-ascii?Q?X2JYAT2ypeiZBHmEJ/sgfVpRsonlbK2Z4KTReN/P63WWf5BIOHt9GSoa7lob?=
 =?us-ascii?Q?5XwjmZKZ3xtYsEtPqxrTIovqTzhXLScd1mBhHjCFenDl4mB7mQt3WAyaxwoe?=
 =?us-ascii?Q?4FIulKXKWLGgmoJBPtgUFq1cHiMtrisluH2klKvBTieme62qw342/U96hHoW?=
 =?us-ascii?Q?S7NEESMtkmUACYioz/hzAKn5o1VyIZnA2OkStiuU+1cWu4YPbz9FK06heLub?=
 =?us-ascii?Q?Xw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6ad76c26-2564-4419-37b6-08db78e3088d
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jun 2023 20:54:30.2598
 (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: Ge2JaPNfUEHQgfi7yQiBkn3gRCZFZhNCVyhgao8t/l38Q49NpUXBX75MqtvtFxMcR8v5yTYUzY/Ft1DxhGQNbahXSbednNjCrrlWIglamTI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB6842
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE 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

Michal Wilczynski 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/nfit/core.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 95930e9d776c..a281bdfee8a0 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3312,11 +3312,13 @@ void acpi_nfit_shutdown(void *data)
>  }
>  EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>  
> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
>  {
> -	device_lock(&adev->dev);
> -	__acpi_nfit_notify(&adev->dev, adev->handle, event);
> -	device_unlock(&adev->dev);
> +	struct acpi_device *device = data;
> +
> +	device_lock(&device->dev);
> +	__acpi_nfit_notify(&device->dev, handle, event);
> +	device_unlock(&device->dev);
>  }
>  
>  static int acpi_nfit_add(struct acpi_device *adev)
> @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
>  
>  	if (rc)
>  		return rc;
> -	return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> +
> +	rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> +	if (rc)
> +		return rc;
> +
> +	return acpi_dev_install_notify_handler(adev,
> +					       ACPI_DEVICE_NOTIFY,
> +					       acpi_nfit_notify);
>  }
>  
>  static void acpi_nfit_remove(struct acpi_device *adev)
>  {
>  	/* see acpi_nfit_unregister */
> +
> +	acpi_dev_remove_notify_handler(adev,
> +				       ACPI_DEVICE_NOTIFY,
> +				       acpi_nfit_notify);

Please use devm to trigger this release rather than making
acpi_nfit_remove() contain any logic.

An additional cleanup opportunity with the ->add() path fully devm
instrumented would be to just delete acpi_nfit_remove() since it is
optional and serves no purpose.