[PATCH v2] ufs: core: add missing post notify for power mode change

[PATCH v2] ufs: core: add missing post notify for power mode change

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

When the power mode change is successful but the power mode
hasn't actually changed, the post notification was missed.
Similar to the approach with hibernate/clock scale/hce enable,
having pre/post notifications in the same function will
make it easier to maintain.

Additionally, supplement the description of power parameters
for the pwr_change_notify callback.

Fixes: 7eb584db73be ("ufs: refactor configuring power mode")
Cc: stable@vger.kernel.org #6.11.x
Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c |  7 ++++---
 include/ufs/ufshcd.h      | 10 ++++++----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index abbe7135a977..814402e93a1e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4651,9 +4651,6 @@ static int ufshcd_change_power_mode(struct ufs_hba *hba,
 		dev_err(hba->dev,
 			"%s: power mode change failed %d\n", __func__, ret);
 	} else {
-		ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, NULL,
-								pwr_mode);
-
 		memcpy(&hba->pwr_info, pwr_mode,
 			sizeof(struct ufs_pa_layer_attr));
 	}
@@ -4682,6 +4679,10 @@ int ufshcd_config_pwr_mode(struct ufs_hba *hba,
 
 	ret = ufshcd_change_power_mode(hba, &final_params);
 
+	if (!ret)
+		ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, NULL,
+					&final_params);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(ufshcd_config_pwr_mode);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 3f68ae3e4330..1db754b4a4d6 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -308,7 +308,9 @@ struct ufs_pwr_mode_info {
  *                       to allow variant specific Uni-Pro initialization.
  * @pwr_change_notify: called before and after a power mode change
  *			is carried out to allow vendor spesific capabilities
- *			to be set.
+ *			to be set. PRE_CHANGE can modify final_params based
+ *			on desired_pwr_mode, but POST_CHANGE must not alter
+ *			the final_params parameter
  * @setup_xfer_req: called before any transfer request is issued
  *                  to set some things
  * @setup_task_mgmt: called before any task management request is issued
@@ -350,9 +352,9 @@ struct ufs_hba_variant_ops {
 	int	(*link_startup_notify)(struct ufs_hba *,
 				       enum ufs_notify_change_status);
 	int	(*pwr_change_notify)(struct ufs_hba *,
-					enum ufs_notify_change_status status,
-					struct ufs_pa_layer_attr *,
-					struct ufs_pa_layer_attr *);
+				enum ufs_notify_change_status status,
+				struct ufs_pa_layer_attr *desired_pwr_mode,
+				struct ufs_pa_layer_attr *final_params);
 	void	(*setup_xfer_req)(struct ufs_hba *hba, int tag,
 				  bool is_scsi_cmd);
 	void	(*setup_task_mgmt)(struct ufs_hba *, int, u8);
-- 
2.18.0

Re: [PATCH v2] ufs: core: add missing post notify for power mode change

[Bart Van Assche]

On 11/21/24 6:49 PM, peter.wang@mediatek.com wrote:
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 3f68ae3e4330..1db754b4a4d6 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -308,7 +308,9 @@ struct ufs_pwr_mode_info {
>    *                       to allow variant specific Uni-Pro initialization.
>    * @pwr_change_notify: called before and after a power mode change
>    *			is carried out to allow vendor spesific capabilities
> - *			to be set.
> + *			to be set. PRE_CHANGE can modify final_params based
> + *			on desired_pwr_mode, but POST_CHANGE must not alter
> + *			the final_params parameter
>    * @setup_xfer_req: called before any transfer request is issued
>    *                  to set some things
>    * @setup_task_mgmt: called before any task management request is issued
> @@ -350,9 +352,9 @@ struct ufs_hba_variant_ops {
>   	int	(*link_startup_notify)(struct ufs_hba *,
>   				       enum ufs_notify_change_status);
>   	int	(*pwr_change_notify)(struct ufs_hba *,
> -					enum ufs_notify_change_status status,
> -					struct ufs_pa_layer_attr *,
> -					struct ufs_pa_layer_attr *);
> +				enum ufs_notify_change_status status,
> +				struct ufs_pa_layer_attr *desired_pwr_mode,
> +				struct ufs_pa_layer_attr *final_params);
>   	void	(*setup_xfer_req)(struct ufs_hba *hba, int tag,
>   				  bool is_scsi_cmd);
>   	void	(*setup_task_mgmt)(struct ufs_hba *, int, u8);

'pwr_change_notify' probably should be split into two callback pointers
(one for PRE_CHANGE, one for POST_CHANGE) since the third and fourth
arguments have different roles for pre and post changes. Additionally,
this would allow to constify the fourth argument for the post callback.

Anyway, since this patch looks good to me:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2] ufs: core: add missing post notify for power mode change

[Martin K. Petersen]

On Fri, 22 Nov 2024 10:49:43 +0800, peter.wang@mediatek.com wrote:

> When the power mode change is successful but the power mode
> hasn't actually changed, the post notification was missed.
> Similar to the approach with hibernate/clock scale/hce enable,
> having pre/post notifications in the same function will
> make it easier to maintain.
> 
> Additionally, supplement the description of power parameters
> for the pwr_change_notify callback.
> 
> [...]

Applied to 6.13/scsi-fixes, thanks!

[1/1] ufs: core: add missing post notify for power mode change
      https://git.kernel.org/mkp/scsi/c/7f45ed5f0cd5

-- 
Martin K. Petersen	Oracle Linux Engineering