| Message ID | 20231218225229.2542156-3-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix the error path in ufshcd_async_scan() | expand |
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>
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)
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.
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.
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)
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(-)