[PATCH 0/2] Fix the error path in ufshcd_async_scan()

[PATCH 0/2] Fix the error path in ufshcd_async_scan()

[Bart Van Assche]

Hi Martin,

This patch series fixes a kernel crash triggered by the error path of
ufshcd_async_scan(). Please consider this patch series for the next merge
window.

Thanks,

Bart.

Bart Van Assche (2):
  scsi: ufs: Simplify power management during async scan
  scsi: ufs: Remove the ufshcd_hba_exit() call from ufshcd_async_scan()

 drivers/ufs/core/ufshcd.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

[PATCH 1/2] scsi: ufs: Simplify power management during async scan

[Bart Van Assche]

ufshcd_init() calls pm_runtime_get_sync() before it calls
async_schedule(). ufshcd_async_scan() calls pm_runtime_put_sync()
directly or indirectly from ufshcd_add_lus(). Simplify
ufshcd_async_scan() by always calling pm_runtime_put_sync() from
ufshcd_async_scan().

Cc: stable@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d6ae5d17892c..0ad8bde39cd1 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8711,7 +8711,6 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 
 	ufs_bsg_probe(hba);
 	scsi_scan_host(hba->host);
-	pm_runtime_put_sync(hba->dev);
 
 out:
 	return ret;
@@ -8980,15 +8979,15 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 
 	/* Probe and add UFS logical units  */
 	ret = ufshcd_add_lus(hba);
+
 out:
+	pm_runtime_put_sync(hba->dev);
 	/*
 	 * If we failed to initialize the device or the device is not
 	 * present, turn off the power/clocks etc.
 	 */
-	if (ret) {
-		pm_runtime_put_sync(hba->dev);
+	if (ret)
 		ufshcd_hba_exit(hba);
-	}
 }
 
 static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)

[PATCH 2/2] scsi: ufs: Remove the ufshcd_hba_exit() call from ufshcd_async_scan()

[Bart Van Assche]

Calling ufshcd_hba_exit() from a function that is called asynchronously
from ufshcd_init() is wrong because this triggers multiple race
conditions. Instead of calling ufshcd_hba_exit(), log an error message.

Reported-by: Daniel Mentz <danielmentz@google.com>
Fixes: 1d337ec2f35e ("ufs: improve init sequence")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0ad8bde39cd1..7c59d7a02243 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8982,12 +8982,9 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 
 out:
 	pm_runtime_put_sync(hba->dev);
-	/*
-	 * If we failed to initialize the device or the device is not
-	 * present, turn off the power/clocks etc.
-	 */
+
 	if (ret)
-		ufshcd_hba_exit(hba);
+		dev_err(hba->dev, "%s failed: %d\n", __func__, ret);
 }
 
 static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)

Re: [PATCH 1/2] scsi: ufs: Simplify power management during async scan

[Can Guo]

On 12/19/2023 6:52 AM, Bart Van Assche wrote:
> ufshcd_init() calls pm_runtime_get_sync() before it calls
> async_schedule(). ufshcd_async_scan() calls pm_runtime_put_sync()
> directly or indirectly from ufshcd_add_lus(). Simplify
> ufshcd_async_scan() by always calling pm_runtime_put_sync() from
> ufshcd_async_scan().
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufshcd.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d6ae5d17892c..0ad8bde39cd1 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8711,7 +8711,6 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>   
>   	ufs_bsg_probe(hba);
>   	scsi_scan_host(hba->host);
> -	pm_runtime_put_sync(hba->dev);
>   
>   out:
>   	return ret;
> @@ -8980,15 +8979,15 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>   
>   	/* Probe and add UFS logical units  */
>   	ret = ufshcd_add_lus(hba);
> +
>   out:
> +	pm_runtime_put_sync(hba->dev);
>   	/*
>   	 * If we failed to initialize the device or the device is not
>   	 * present, turn off the power/clocks etc.
>   	 */
> -	if (ret) {
> -		pm_runtime_put_sync(hba->dev);
> +	if (ret)
>   		ufshcd_hba_exit(hba);
> -	}
>   }
>   
>   static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)

Reviewed-by: Can Guo <quic_cang@quicinc.com>

Re: [PATCH 2/2] scsi: ufs: Remove the ufshcd_hba_exit() call from ufshcd_async_scan()

[Can Guo]

On 12/19/2023 6:52 AM, Bart Van Assche wrote:
> Calling ufshcd_hba_exit() from a function that is called asynchronously
> from ufshcd_init() is wrong because this triggers multiple race
> conditions. Instead of calling ufshcd_hba_exit(), log an error message.
> 
> Reported-by: Daniel Mentz <danielmentz@google.com>
> Fixes: 1d337ec2f35e ("ufs: improve init sequence")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufshcd.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0ad8bde39cd1..7c59d7a02243 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8982,12 +8982,9 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>   
>   out:
>   	pm_runtime_put_sync(hba->dev);
> -	/*
> -	 * If we failed to initialize the device or the device is not
> -	 * present, turn off the power/clocks etc.
> -	 */
> +
>   	if (ret)
> -		ufshcd_hba_exit(hba);
> +		dev_err(hba->dev, "%s failed: %d\n", __func__, ret);
>   }
>   
>   static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)

Reviewed-by: Can Guo <quic_cang@quicinc.com>

Re: [PATCH 1/2] scsi: ufs: Simplify power management during async scan

[Manivannan Sadhasivam]

On Mon, Dec 18, 2023 at 02:52:14PM -0800, Bart Van Assche wrote:
> ufshcd_init() calls pm_runtime_get_sync() before it calls
> async_schedule(). ufshcd_async_scan() calls pm_runtime_put_sync()
> directly or indirectly from ufshcd_add_lus(). Simplify
> ufshcd_async_scan() by always calling pm_runtime_put_sync() from
> ufshcd_async_scan().
> 
> Cc: stable@vger.kernel.org

No fixes tag?

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

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d6ae5d17892c..0ad8bde39cd1 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8711,7 +8711,6 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>  
>  	ufs_bsg_probe(hba);
>  	scsi_scan_host(hba->host);
> -	pm_runtime_put_sync(hba->dev);
>  
>  out:
>  	return ret;
> @@ -8980,15 +8979,15 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  
>  	/* Probe and add UFS logical units  */
>  	ret = ufshcd_add_lus(hba);
> +
>  out:
> +	pm_runtime_put_sync(hba->dev);
>  	/*
>  	 * If we failed to initialize the device or the device is not
>  	 * present, turn off the power/clocks etc.
>  	 */
> -	if (ret) {
> -		pm_runtime_put_sync(hba->dev);
> +	if (ret)
>  		ufshcd_hba_exit(hba);
> -	}
>  }
>  
>  static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/2] scsi: ufs: Remove the ufshcd_hba_exit() call from ufshcd_async_scan()

[Manivannan Sadhasivam]

On Mon, Dec 18, 2023 at 02:52:15PM -0800, Bart Van Assche wrote:
> Calling ufshcd_hba_exit() from a function that is called asynchronously
> from ufshcd_init() is wrong because this triggers multiple race
> conditions. Instead of calling ufshcd_hba_exit(), log an error message.
> 

This also means that during failure, resources will not be powered OFF. IMO, a
justification is needed why it is OK to left them powered ON.

> Reported-by: Daniel Mentz <danielmentz@google.com>
> Fixes: 1d337ec2f35e ("ufs: improve init sequence")

No need to backport this patch?

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

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0ad8bde39cd1..7c59d7a02243 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8982,12 +8982,9 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  
>  out:
>  	pm_runtime_put_sync(hba->dev);
> -	/*
> -	 * If we failed to initialize the device or the device is not
> -	 * present, turn off the power/clocks etc.
> -	 */
> +
>  	if (ret)
> -		ufshcd_hba_exit(hba);
> +		dev_err(hba->dev, "%s failed: %d\n", __func__, ret);
>  }
>  
>  static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] scsi: ufs: Simplify power management during async scan

[Bart Van Assche]

On 12/20/23 06:42, Manivannan Sadhasivam wrote:
> On Mon, Dec 18, 2023 at 02:52:14PM -0800, Bart Van Assche wrote:
>> ufshcd_init() calls pm_runtime_get_sync() before it calls
>> async_schedule(). ufshcd_async_scan() calls pm_runtime_put_sync()
>> directly or indirectly from ufshcd_add_lus(). Simplify
>> ufshcd_async_scan() by always calling pm_runtime_put_sync() from
>> ufshcd_async_scan().
>>
>> Cc: stable@vger.kernel.org
> 
> No fixes tag?

There is no Fixes: tag because this patch does not change the behavior of
the UFS driver. The Cc: stable tag is present because the next patch in this
series has a Fixes: tag and depends on this patch.

Thanks,

Bart.

Re: [PATCH 2/2] scsi: ufs: Remove the ufshcd_hba_exit() call from ufshcd_async_scan()

[Bart Van Assche]

On 12/20/23 06:48, Manivannan Sadhasivam wrote:
> On Mon, Dec 18, 2023 at 02:52:15PM -0800, Bart Van Assche wrote:
>> Calling ufshcd_hba_exit() from a function that is called asynchronously
>> from ufshcd_init() is wrong because this triggers multiple race
>> conditions. Instead of calling ufshcd_hba_exit(), log an error message.
> 
> This also means that during failure, resources will not be powered OFF. IMO, a
> justification is needed why it is OK to left them powered ON.

I have never seen ufshcd_async_scan() fail other than during hardware bringup.
Has anyone else ever observed a ufshcd_async_scan() failure?

>> Reported-by: Daniel Mentz <danielmentz@google.com>
>> Fixes: 1d337ec2f35e ("ufs: improve init sequence")
> 
> No need to backport this patch?

Isn't the "Fixes:" tag sufficient? I don't think that it it necessary to add a
"Cc: stable" tag if a "Fixes:" tag is present.

Thanks,

Bart.

Re: [PATCH 2/2] scsi: ufs: Remove the ufshcd_hba_exit() call from ufshcd_async_scan()

[Manivannan Sadhasivam]

On Wed, Dec 20, 2023 at 08:44:21AM -0800, Bart Van Assche wrote:
> On 12/20/23 06:48, Manivannan Sadhasivam wrote:
> > On Mon, Dec 18, 2023 at 02:52:15PM -0800, Bart Van Assche wrote:
> > > Calling ufshcd_hba_exit() from a function that is called asynchronously
> > > from ufshcd_init() is wrong because this triggers multiple race
> > > conditions. Instead of calling ufshcd_hba_exit(), log an error message.
> > 
> > This also means that during failure, resources will not be powered OFF. IMO, a
> > justification is needed why it is OK to left them powered ON.
> 
> I have never seen ufshcd_async_scan() fail other than during hardware bringup.
> Has anyone else ever observed a ufshcd_async_scan() failure?
> 
> > > Reported-by: Daniel Mentz <danielmentz@google.com>
> > > Fixes: 1d337ec2f35e ("ufs: improve init sequence")
> > 
> > No need to backport this patch?
> 
> Isn't the "Fixes:" tag sufficient? I don't think that it it necessary to add a
> "Cc: stable" tag if a "Fixes:" tag is present.
> 

No. You need to explicitly CC stable list, if you want the commit to be
backported to stable releases. Even though the stable maintainers backport the
commits with Fixes tag, it is always strongly advised to explictly CC stable
list.

Here is an excerpt from Documentation/process/stable-kernel-rules.rst:

"There are three options to submit a change to -stable trees:

 1. Add a 'stable tag' to the description of a patch you then submit for
    mainline inclusion.
 2. Ask the stable team to pick up a patch already mainlined.
 3. Submit a patch to the stable team that is equivalent to a change already
    mainlined.

The sections below describe each of the options in more detail.

:ref:`option_1` is **strongly** preferred, it is the easiest and most common."

- Mani
> Thanks,
> 
> Bart.

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] scsi: ufs: Simplify power management during async scan

[Manivannan Sadhasivam]

On Wed, Dec 20, 2023 at 08:36:28AM -0800, Bart Van Assche wrote:
> On 12/20/23 06:42, Manivannan Sadhasivam wrote:
> > On Mon, Dec 18, 2023 at 02:52:14PM -0800, Bart Van Assche wrote:
> > > ufshcd_init() calls pm_runtime_get_sync() before it calls
> > > async_schedule(). ufshcd_async_scan() calls pm_runtime_put_sync()
> > > directly or indirectly from ufshcd_add_lus(). Simplify
> > > ufshcd_async_scan() by always calling pm_runtime_put_sync() from
> > > ufshcd_async_scan().
> > > 
> > > Cc: stable@vger.kernel.org
> > 
> > No fixes tag?
> 
> There is no Fixes: tag because this patch does not change the behavior of
> the UFS driver. The Cc: stable tag is present because the next patch in this
> series has a Fixes: tag and depends on this patch.
> 

Ok.

- Mani

> Thanks,
> 
> Bart.

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/2] Fix the error path in ufshcd_async_scan()

[Martin K. Petersen]

Bart,

> This patch series fixes a kernel crash triggered by the error path of
> ufshcd_async_scan(). Please consider this patch series for the next
> merge window.

Applied to 6.8/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering