| Message ID | 20210527030901.88403-4-jjmin.jeong@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add quirk to support exynos ufshci | expand |
Hi jongmin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.13-rc3 next-20210526]
[cannot apply to target/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/jongmin-jeong/scsi-ufs-add-quirk-to-handle-broken-UIC-command/20210527-120913
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: i386-randconfig-r035-20210526 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/1418a728f0fcd3a50e41d3314d8d9e1ec672a0c7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review jongmin-jeong/scsi-ufs-add-quirk-to-handle-broken-UIC-command/20210527-120913
git checkout 1418a728f0fcd3a50e41d3314d8d9e1ec672a0c7
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/scsi/ufs/ufshcd.c: In function 'ufshcd_eh_device_reset_handler':
>> drivers/scsi/ufs/ufshcd.c:6827:9: warning: 'hba' is used uninitialized in this function [-Wuninitialized]
6827 | if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER)
| ~~~^~~~~~~~
vim +/hba +6827 drivers/scsi/ufs/ufshcd.c
6810
6811 /**
6812 * ufshcd_eh_device_reset_handler - device reset handler registered to
6813 * scsi layer.
6814 * @cmd: SCSI command pointer
6815 *
6816 * Returns SUCCESS/FAILED
6817 */
6818 static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
6819 {
6820 struct Scsi_Host *host;
6821 struct ufs_hba *hba;
6822 u32 pos;
6823 int err;
6824 u8 resp = 0xF, lun;
6825 unsigned long flags;
6826
> 6827 if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER)
6828 return ufshcd_eh_host_reset_handler(cmd);
6829
6830 host = cmd->device->host;
6831 hba = shost_priv(host);
6832
6833 lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
6834 err = ufshcd_issue_tm_cmd(hba, lun, 0, UFS_LOGICAL_RESET, &resp);
6835 if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
6836 if (!err)
6837 err = resp;
6838 goto out;
6839 }
6840
6841 /* clear the commands that were pending for corresponding LUN */
6842 for_each_set_bit(pos, &hba->outstanding_reqs, hba->nutrs) {
6843 if (hba->lrb[pos].lun == lun) {
6844 err = ufshcd_clear_cmd(hba, pos);
6845 if (err)
6846 break;
6847 }
6848 }
6849 spin_lock_irqsave(host->host_lock, flags);
6850 ufshcd_transfer_req_compl(hba);
6851 spin_unlock_irqrestore(host->host_lock, flags);
6852
6853 out:
6854 hba->req_abort_count = 0;
6855 ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, (u32)err);
6856 if (!err) {
6857 err = SUCCESS;
6858 } else {
6859 dev_err(hba->dev, "%s: failed with err %d\n", __func__, err);
6860 err = FAILED;
6861 }
6862 return err;
6863 }
6864
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 2021-05-27 11:09, jongmin jeong wrote: > samsung ExynosAuto SoC has two types of host controller interface to > support the virtualization of UFS Device. > One is the physical host(PH) that the same as conventaional UFSHCI, > and the other is the virtual host(VH) that support data transfer > function only. > > In this structure, the virtual host does support host reset handler > only. > This patch calls the host reset handler when abort or device reset > handler > has occured in the virtual host. > > Change-Id: I3f07e772415a35fe1e7374e02b3c37ef0bf5660d > Signed-off-by: jongmin jeong <jjmin.jeong@samsung.com> > --- > drivers/scsi/ufs/ufshcd.c | 7 +++++++ > drivers/scsi/ufs/ufshcd.h | 6 ++++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 4787e40c6a2d..9d1912290f87 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -6826,6 +6826,9 @@ static int ufshcd_eh_device_reset_handler(struct > scsi_cmnd *cmd) > u8 resp = 0xF, lun; > unsigned long flags; > > + if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER) > + return ufshcd_eh_host_reset_handler(cmd); > + > host = cmd->device->host; > hba = shost_priv(host); > > @@ -6972,6 +6975,10 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) > host = cmd->device->host; > hba = shost_priv(host); > tag = cmd->request->tag; > + > + if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER) > + return ufshcd_eh_host_reset_handler(cmd); > + Unless you are not using runtime PM. Otherwise, when abort happens to SSU cmd (sent from pm ops), your change will lead to a live lock, because ufshcd_eh_host_reset_handler() invokes error handling and flushes it, error handling calls runtime_pm_get_sync(), which flushes pm ops, while pm ops is blocked by SSU cmd. To be on the safe side, you should move these codes after below check in ufshcd_abort(), right before sending task abort TMRs. /* * Task abort to the device W-LUN is illegal. When this command * will fail, due to spec violation, scsi err handling next step * will be to send LU reset which, again, is a spec violation. * To avoid these unnecessary/illegal steps, first we clean up * the lrb taken by this cmd and mark the lrb as in_use, then * queue the eh_work and bail. */ if (lrbp->lun == UFS_UPIU_UFS_DEVICE_WLUN) { ... goto out; } Thanks, Can Guo. > lrbp = &hba->lrb[tag]; > if (!ufshcd_valid_tag(hba, tag)) { > dev_err(hba->dev, > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 0ab4c296be32..82a9c6889978 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -581,6 +581,12 @@ enum ufshcd_quirks { > * support interface configuration. > */ > UFSHCD_QUIRK_SKIP_INTERFACE_CONFIGURATION = 1 << 16, > + > + /* > + * This quirk needs to be enabled if the host controller support > + * host reset handler only. > + */ > + UFSHCD_QUIRK_BROKEN_RESET_HANDLER = 1 << 17, > }; > > enum ufshcd_caps {
On 2021-05-27 11:09, jongmin jeong wrote: > samsung ExynosAuto SoC has two types of host controller interface to > support the virtualization of UFS Device. > One is the physical host(PH) that the same as conventaional UFSHCI, > and the other is the virtual host(VH) that support data transfer > function only. > > In this structure, the virtual host does support host reset handler > only. > This patch calls the host reset handler when abort or device reset > handler > has occured in the virtual host. One more question, as per the plot in the cover letter, the VH does support TMRs. Why are you trying to make ufshcd_abort() and ufshcd_eh_device_reset_handler() no-ops? Thanks, Can Guo. > > Change-Id: I3f07e772415a35fe1e7374e02b3c37ef0bf5660d > Signed-off-by: jongmin jeong <jjmin.jeong@samsung.com> > --- > drivers/scsi/ufs/ufshcd.c | 7 +++++++ > drivers/scsi/ufs/ufshcd.h | 6 ++++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 4787e40c6a2d..9d1912290f87 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -6826,6 +6826,9 @@ static int ufshcd_eh_device_reset_handler(struct > scsi_cmnd *cmd) > u8 resp = 0xF, lun; > unsigned long flags; > > + if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER) > + return ufshcd_eh_host_reset_handler(cmd); > + > host = cmd->device->host; > hba = shost_priv(host); > > @@ -6972,6 +6975,10 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) > host = cmd->device->host; > hba = shost_priv(host); > tag = cmd->request->tag; > + > + if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER) > + return ufshcd_eh_host_reset_handler(cmd); > + > lrbp = &hba->lrb[tag]; > if (!ufshcd_valid_tag(hba, tag)) { > dev_err(hba->dev, > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 0ab4c296be32..82a9c6889978 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -581,6 +581,12 @@ enum ufshcd_quirks { > * support interface configuration. > */ > UFSHCD_QUIRK_SKIP_INTERFACE_CONFIGURATION = 1 << 16, > + > + /* > + * This quirk needs to be enabled if the host controller support > + * host reset handler only. > + */ > + UFSHCD_QUIRK_BROKEN_RESET_HANDLER = 1 << 17, > }; > > enum ufshcd_caps {
Hi jongmin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.13-rc3 next-20210526]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/jongmin-jeong/scsi-ufs-add-quirk-to-handle-broken-UIC-command/20210527-120913
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: powerpc64-randconfig-r016-20210526 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 6505c630407c5feec818f0bb1c284f9eeebf2071)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/1418a728f0fcd3a50e41d3314d8d9e1ec672a0c7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review jongmin-jeong/scsi-ufs-add-quirk-to-handle-broken-UIC-command/20210527-120913
git checkout 1418a728f0fcd3a50e41d3314d8d9e1ec672a0c7
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/scsi/ufs/ufshcd.c:6827:6: warning: variable 'hba' is uninitialized when used here [-Wuninitialized]
if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER)
^~~
drivers/scsi/ufs/ufshcd.c:6821:21: note: initialize the variable 'hba' to silence this warning
struct ufs_hba *hba;
^
= NULL
1 warning generated.
vim +/hba +6827 drivers/scsi/ufs/ufshcd.c
6810
6811 /**
6812 * ufshcd_eh_device_reset_handler - device reset handler registered to
6813 * scsi layer.
6814 * @cmd: SCSI command pointer
6815 *
6816 * Returns SUCCESS/FAILED
6817 */
6818 static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
6819 {
6820 struct Scsi_Host *host;
6821 struct ufs_hba *hba;
6822 u32 pos;
6823 int err;
6824 u8 resp = 0xF, lun;
6825 unsigned long flags;
6826
> 6827 if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER)
6828 return ufshcd_eh_host_reset_handler(cmd);
6829
6830 host = cmd->device->host;
6831 hba = shost_priv(host);
6832
6833 lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
6834 err = ufshcd_issue_tm_cmd(hba, lun, 0, UFS_LOGICAL_RESET, &resp);
6835 if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
6836 if (!err)
6837 err = resp;
6838 goto out;
6839 }
6840
6841 /* clear the commands that were pending for corresponding LUN */
6842 for_each_set_bit(pos, &hba->outstanding_reqs, hba->nutrs) {
6843 if (hba->lrb[pos].lun == lun) {
6844 err = ufshcd_clear_cmd(hba, pos);
6845 if (err)
6846 break;
6847 }
6848 }
6849 spin_lock_irqsave(host->host_lock, flags);
6850 ufshcd_transfer_req_compl(hba);
6851 spin_unlock_irqrestore(host->host_lock, flags);
6852
6853 out:
6854 hba->req_abort_count = 0;
6855 ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, (u32)err);
6856 if (!err) {
6857 err = SUCCESS;
6858 } else {
6859 dev_err(hba->dev, "%s: failed with err %d\n", __func__, err);
6860 err = FAILED;
6861 }
6862 return err;
6863 }
6864
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> > samsung ExynosAuto SoC has two types of host controller interface to > > support the virtualization of UFS Device. > > One is the physical host(PH) that the same as conventaional UFSHCI, > > and the other is the virtual host(VH) that support data transfer > > function only. > > > > In this structure, the virtual host does support host reset handler > > only. > > This patch calls the host reset handler when abort or device reset > > handler has occured in the virtual host. > > One more question, as per the plot in the cover letter, the VH does > support TMRs. > Why are you trying to make ufshcd_abort() and > ufshcd_eh_device_reset_handler() > no-ops? > > Thanks, > > Can Guo. Can, Thanks for your review. Yes, you are right about ufshcd_abort. it would be better to call if it fails abort handling has failed. I will apply this modification to the next patch. device reset handler case is a little different. We do not want the virtual host to do device reset(LUN reset). According to our virtualization architecture, virtual OSs can share LUNs. Therefore, we are trying to prevent the reset for LUN in guest OS. > > > > Change-Id: I3f07e772415a35fe1e7374e02b3c37ef0bf5660d > > Signed-off-by: jongmin jeong <jjmin.jeong@samsung.com> > > --- > > drivers/scsi/ufs/ufshcd.c | 7 +++++++ drivers/scsi/ufs/ufshcd.h | 6 > > ++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index 4787e40c6a2d..9d1912290f87 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -6826,6 +6826,9 @@ static int ufshcd_eh_device_reset_handler(struct > > scsi_cmnd *cmd) > > u8 resp = 0xF, lun; > > unsigned long flags; > > > > + if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER) > > + return ufshcd_eh_host_reset_handler(cmd); > > + > > host = cmd->device->host; > > hba = shost_priv(host); > > > > @@ -6972,6 +6975,10 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) > > host = cmd->device->host; > > hba = shost_priv(host); > > tag = cmd->request->tag; > > + > > + if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER) > > + return ufshcd_eh_host_reset_handler(cmd); > > + > > lrbp = &hba->lrb[tag]; > > if (!ufshcd_valid_tag(hba, tag)) { > > dev_err(hba->dev, > > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > > index 0ab4c296be32..82a9c6889978 100644 > > --- a/drivers/scsi/ufs/ufshcd.h > > +++ b/drivers/scsi/ufs/ufshcd.h > > @@ -581,6 +581,12 @@ enum ufshcd_quirks { > > * support interface configuration. > > */ > > UFSHCD_QUIRK_SKIP_INTERFACE_CONFIGURATION = 1 << 16, > > + > > + /* > > + * This quirk needs to be enabled if the host controller support > > + * host reset handler only. > > + */ > > + UFSHCD_QUIRK_BROKEN_RESET_HANDLER = 1 << 17, > > }; > > > > enum ufshcd_caps { Best Regards, Jongmin jeong
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 4787e40c6a2d..9d1912290f87 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -6826,6 +6826,9 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd) u8 resp = 0xF, lun; unsigned long flags; + if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER) + return ufshcd_eh_host_reset_handler(cmd); + host = cmd->device->host; hba = shost_priv(host); @@ -6972,6 +6975,10 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) host = cmd->device->host; hba = shost_priv(host); tag = cmd->request->tag; + + if (hba->quirks & UFSHCD_QUIRK_BROKEN_RESET_HANDLER) + return ufshcd_eh_host_reset_handler(cmd); + lrbp = &hba->lrb[tag]; if (!ufshcd_valid_tag(hba, tag)) { dev_err(hba->dev, diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 0ab4c296be32..82a9c6889978 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -581,6 +581,12 @@ enum ufshcd_quirks { * support interface configuration. */ UFSHCD_QUIRK_SKIP_INTERFACE_CONFIGURATION = 1 << 16, + + /* + * This quirk needs to be enabled if the host controller support + * host reset handler only. + */ + UFSHCD_QUIRK_BROKEN_RESET_HANDLER = 1 << 17, }; enum ufshcd_caps {
samsung ExynosAuto SoC has two types of host controller interface to support the virtualization of UFS Device. One is the physical host(PH) that the same as conventaional UFSHCI, and the other is the virtual host(VH) that support data transfer function only. In this structure, the virtual host does support host reset handler only. This patch calls the host reset handler when abort or device reset handler has occured in the virtual host. Change-Id: I3f07e772415a35fe1e7374e02b3c37ef0bf5660d Signed-off-by: jongmin jeong <jjmin.jeong@samsung.com> --- drivers/scsi/ufs/ufshcd.c | 7 +++++++ drivers/scsi/ufs/ufshcd.h | 6 ++++++ 2 files changed, 13 insertions(+)