| Message ID | 1594971107-37463-1-git-send-email-sh425.lee@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC,v2] scsi: ufs: set STATE_ERROR when ufshcd_probe_hba() failed | expand |
> > set STATE_ERR like below to prevent a lockup(IO stuck) > when ufshcd_probe_hba() returns error. > > Change-Id: I6c85ff290503cc9414d7f5fdd934295497b854ff No need for the gerrit change id > Signed-off-by: Lee Sang Hyun <sh425.lee@samsung.com> Reviewed-by: Avri Altman <avri.altman@wdc.com>
Hi Sang Hyun, On 2020-07-17 15:31, Lee Sang Hyun wrote: > set STATE_ERR like below to prevent a lockup(IO stuck) > when ufshcd_probe_hba() returns error. > > Change-Id: I6c85ff290503cc9414d7f5fdd934295497b854ff > Signed-off-by: Lee Sang Hyun <sh425.lee@samsung.com> > --- > drivers/scsi/ufs/ufshcd.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index ad4fc82..37e4105 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -7368,6 +7368,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, > bool async) > { > int ret; > ktime_t start = ktime_get(); > + unsigned long flags; > > ret = ufshcd_link_startup(hba); > if (ret) > @@ -7439,6 +7440,11 @@ static int ufshcd_probe_hba(struct ufs_hba > *hba, bool async) > ufshcd_auto_hibern8_enable(hba); > > out: > + if (ret) { > + spin_lock_irqsave(hba->host->host_lock, flags); > + hba->ufshcd_state = UFSHCD_STATE_ERROR; > + spin_unlock_irqrestore(hba->host->host_lock, flags); > + } > > trace_ufshcd_init(dev_name(hba->dev), ret, > ktime_to_us(ktime_sub(ktime_get(), start)), This change is included in my change "scsi: ufs: Fix up and simplify error recovery mechanism". Besides, this change seems not complete because #1 You are only protecting your changes with spin lock in ufshcd_probe_hba, what about the other line "hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;"? #2 As you are giving "hba->ufshcd_state = UFSHCD_STATE_ERROR;" in ufshcd_probe_hba, why keep the same lines in ufshcd_error_handler and ufshcd_eh_host_reset_handler? Thanks, Can Guo.
> Hi Sang Hyun, > > On 2020-07-17 15:31, Lee Sang Hyun wrote: > > set STATE_ERR like below to prevent a lockup(IO stuck) when > > ufshcd_probe_hba() returns error. > > > > Change-Id: I6c85ff290503cc9414d7f5fdd934295497b854ff > > Signed-off-by: Lee Sang Hyun <sh425.lee@samsung.com> > > --- > > drivers/scsi/ufs/ufshcd.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index ad4fc82..37e4105 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -7368,6 +7368,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, > > bool async) { > > int ret; > > ktime_t start = ktime_get(); > > + unsigned long flags; > > > > ret = ufshcd_link_startup(hba); > > if (ret) > > @@ -7439,6 +7440,11 @@ static int ufshcd_probe_hba(struct ufs_hba > > *hba, bool async) > > ufshcd_auto_hibern8_enable(hba); > > > > out: > > + if (ret) { > > + spin_lock_irqsave(hba->host->host_lock, flags); > > + hba->ufshcd_state = UFSHCD_STATE_ERROR; > > + spin_unlock_irqrestore(hba->host->host_lock, flags); > > + } > > > > trace_ufshcd_init(dev_name(hba->dev), ret, > > ktime_to_us(ktime_sub(ktime_get(), start)), > > This change is included in my change > "scsi: ufs: Fix up and simplify error recovery mechanism". "scsi: ufs: Fix up and simplify error recovery mechanism". I checked the patch, and confirmed that the contents of my patch were also included. > > Besides, this change seems not complete because > > #1 You are only protecting your changes with spin lock in > ufshcd_probe_hba, what about the other line "hba->ufshcd_state = > UFSHCD_STATE_OPERATIONAL;"? > > #2 As you are giving "hba->ufshcd_state = UFSHCD_STATE_ERROR;" > in ufshcd_probe_hba, why keep the same lines in ufshcd_error_handler and > ufshcd_eh_host_reset_handler? > > Thanks, > > Can Guo.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index ad4fc82..37e4105 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -7368,6 +7368,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool async) { int ret; ktime_t start = ktime_get(); + unsigned long flags; ret = ufshcd_link_startup(hba); if (ret) @@ -7439,6 +7440,11 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool async) ufshcd_auto_hibern8_enable(hba); out: + if (ret) { + spin_lock_irqsave(hba->host->host_lock, flags); + hba->ufshcd_state = UFSHCD_STATE_ERROR; + spin_unlock_irqrestore(hba->host->host_lock, flags); + } trace_ufshcd_init(dev_name(hba->dev), ret, ktime_to_us(ktime_sub(ktime_get(), start)),
set STATE_ERR like below to prevent a lockup(IO stuck) when ufshcd_probe_hba() returns error. Change-Id: I6c85ff290503cc9414d7f5fdd934295497b854ff Signed-off-by: Lee Sang Hyun <sh425.lee@samsung.com> --- drivers/scsi/ufs/ufshcd.c | 6 ++++++ 1 file changed, 6 insertions(+)