[PATCH 0/3] Add quirk to support exynos ufshci

[PATCH 0/3] Add quirk to support exynos ufshci

[jongmin jeong]

In ExynosAuto(variant of the Exynos for automotive), the UFS Storage needs
to be accessed from multi-OS.
To increase IO performance and reduce SW complexity, we implemented UFS-IOV
to support storage IO virtualization feature on UFS.

IO virtualization increases IO performance and reduce SW complexity
with small area cost. And IO virtualization supports virtual machine isolation
for Security and Safety which are requested by Multi-OS system such as
automotive application.

Below figure is the conception of UFS-IOV architeture.

    +------+          +------+
    | OS#1 |          | OS#2 |
    +------+          +------+
       |                 |
 +------------+     +------------+
 |  Physical  |     |   Virtual  |
 |    Host    |     |    Host    |
 +------------+     +------------+
   |      |              | <-- UTP_CMD_SAP, UTP_TM_SAP
   |   +-------------------------+
   |   |    Function Arbitor     |
   |   +-------------------------+
 +-------------------------------+
 |           UTP Layer           |
 +-------------------------------+
 +-------------------------------+
 |           UIC Layer           |
 +-------------------------------+

There are two types of host controllers on the UFS host controller
that we designed.
The UFS device has a Function Arbitor that arranges commands of each host.
When each host transmits a command to the Arbitor, the Arbitor transmits it
to the UTP layer.
Physical Host(PH) support all UFSHCI functions(all SAPs) same as conventional
UFSHCI.
Virtual Host(VH) support only data transfer function(UTP_CMD_SAP and UTP_TM_SAP).

In an environment where multiple OSs are used, the OS that has the leadership of
the system is called System OS. This system OS uses PH and controls error handling.

Since VH can only use less functions than PH, it is necessary to send a request
to PH for Detected Error Handling in VH. To interface among PH and VHs,
UFSHCI HW supports mailbox. PH can broadcast mail to other VH at the same time
with arguments and VH can mail to PH with arguments.
PH and VH generate interrupts when mails from PH or VH.

In this structure, the virtual host can't support some feature and need to skip
the some part of ufshcd code by using quirk.
This patchs add quirks so that the UIC command is ignored and the ufshcd init
process can be skipped for VH. Also, according to our UFS-IOV policy,
we added a quirk to the abort handler or device reset handler to call
the host reset handler.

jongmin jeong (3):
  scsi: ufs: add quirk to handle broken UIC command
  scsi: ufs: add quirk to enable host controller without interface
    configuration
  scsi: ufs: add quirk to support host reset only

 drivers/scsi/ufs/ufshcd.c | 13 +++++++++++++
 drivers/scsi/ufs/ufshcd.h | 18 ++++++++++++++++++
 2 files changed, 31 insertions(+)

-- 
2.31.1

[PATCH 1/3] scsi: ufs: add quirk to handle broken UIC command

[jongmin jeong]

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 not support UIC command.
To support this, we add the quirk and return 0 when the UIC command
send function is called.

Change-Id: Ie528726b29bcb643149440bf1c90eaa5995c5ac1
Signed-off-by: jongmin jeong <jjmin.jeong@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 +++
 drivers/scsi/ufs/ufshcd.h | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 4ba3bbf14df0..ec663cb58233 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2318,6 +2318,9 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	int ret;
 	unsigned long flags;
 
+	if (hba->quirks & UFSHCD_QUIRK_BROKEN_UIC_CMD)
+		return 0;
+
 	ufshcd_hold(hba, false);
 	mutex_lock(&hba->uic_cmd_mutex);
 	ufshcd_add_delay_before_dme_cmd(hba);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 0f0e62bfdafd..4a929bf7cf8e 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -569,6 +569,12 @@ enum ufshcd_quirks {
 	 * This quirk allows only sg entries aligned with page size.
 	 */
 	UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE		= 1 << 14,
+
+	/*
+	 * This quirk needs to be enabled if the host controller does not
+	 * support UIC command
+	 */
+	UFSHCD_QUIRK_BROKEN_UIC_CMD			= 1 << 15,
 };
 
 enum ufshcd_caps {
-- 
2.31.1

Re: [PATCH 1/3] scsi: ufs: add quirk to handle broken UIC command

[Christoph Hellwig]

On Thu, May 27, 2021 at 12:08:59PM +0900, 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.

You forgot to include the hunk that actually sets the quirk.

Also please work on the commit log formatting.

> Change-Id: Ie528726b29bcb643149440bf1c90eaa5995c5ac1

This kind of crap has no business in a commit log.

RE: [PATCH 1/3] scsi: ufs: add quirk to handle broken UIC command

[정종민]

> > 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.
> 
> You forgot to include the hunk that actually sets the quirk.

thanks for your review.
We will set up the quirk in exynos ufs driver.
I will upload the patch together in v2 patchs.

> 
> Also please work on the commit log formatting.

I'll erase the change-id log next patch too. Thank you.

> 
> > Change-Id: Ie528726b29bcb643149440bf1c90eaa5995c5ac1
> 
> This kind of crap has no business in a commit log.

Best Regards,
Jongmin jeong

[PATCH 2/3] scsi: ufs: add quirk to enable host controller without interface configuration

[jongmin jeong]

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 not support like device management.
This patch skips the interface configuration part that cannot be performed
in the virtual host.

Change-Id: I65b56f898da9d57c627b5752535dd563e4fd3e8d
Signed-off-by: jongmin jeong <jjmin.jeong@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 +++
 drivers/scsi/ufs/ufshcd.h | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ec663cb58233..4787e40c6a2d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7975,6 +7975,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
 	if (ret)
 		goto out;
 
+	if (hba->quirks & UFSHCD_QUIRK_SKIP_INTERFACE_CONFIGURATION)
+		goto out;
+
 	/* Debug counters initialization */
 	ufshcd_clear_dbg_ufs_stats(hba);
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 4a929bf7cf8e..0ab4c296be32 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -575,6 +575,12 @@ enum ufshcd_quirks {
 	 * support UIC command
 	 */
 	UFSHCD_QUIRK_BROKEN_UIC_CMD			= 1 << 15,
+
+	/*
+	 * This quirk needs to be enabled if the host controller cannot
+	 * support interface configuration.
+	 */
+	UFSHCD_QUIRK_SKIP_INTERFACE_CONFIGURATION	= 1 << 16,
 };
 
 enum ufshcd_caps {
-- 
2.31.1

[PATCH 3/3] scsi: ufs: add quirk to support host reset only

[jongmin jeong]

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);
+
 	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 {
-- 
2.31.1

Re: [PATCH 3/3] scsi: ufs: add quirk to support host reset only

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3460 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39113 bytes --]

Re: [PATCH 3/3] scsi: ufs: add quirk to support host reset only

[Can Guo]

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 {

Re: [PATCH 3/3] scsi: ufs: add quirk to support host reset only

[Can Guo]

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 {

RE: [PATCH 3/3] scsi: ufs: add quirk to support host reset only

[정종민]

> > 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

Re: [PATCH 3/3] scsi: ufs: add quirk to support host reset only

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3957 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34746 bytes --]

RE: [PATCH 0/3] Add quirk to support exynos ufshci

[Alim Akhtar]

Hi Jongmin

> -----Original Message-----
> From: jongmin jeong <jjmin.jeong@samsung.com>
> Sent: 27 May 2021 08:39
> To: jejb@linux.ibm.com; martin.petersen@oracle.com
> Cc: alim.akhtar@samsung.com; avri.altman@wdc.com;
> cang@codeaurora.org; beanhuo@micron.com; adrian.hunter@intel.com;
> linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org;
> jjmin.jeong@samsung.com
> Subject: [PATCH 0/3] Add quirk to support exynos ufshci
> 
> In ExynosAuto(variant of the Exynos for automotive), the UFS Storage needs
> to be accessed from multi-OS.
> To increase IO performance and reduce SW complexity, we implemented
> UFS-IOV to support storage IO virtualization feature on UFS.
> 
> IO virtualization increases IO performance and reduce SW complexity with
> small area cost. And IO virtualization supports virtual machine isolation for
> Security and Safety which are requested by Multi-OS system such as
> automotive application.
> 
> Below figure is the conception of UFS-IOV architeture.
> 
>     +------+          +------+
>     | OS#1 |          | OS#2 |
>     +------+          +------+
>        |                 |
>  +------------+     +------------+
>  |  Physical  |     |   Virtual  |
>  |    Host    |     |    Host    |
>  +------------+     +------------+
>    |      |              | <-- UTP_CMD_SAP, UTP_TM_SAP
>    |   +-------------------------+
>    |   |    Function Arbitor     |
>    |   +-------------------------+
>  +-------------------------------+
>  |           UTP Layer           |
>  +-------------------------------+
>  +-------------------------------+
>  |           UIC Layer           |
>  +-------------------------------+
> 
> There are two types of host controllers on the UFS host controller that we
> designed.
> The UFS device has a Function Arbitor that arranges commands of each host.
> When each host transmits a command to the Arbitor, the Arbitor transmits it
> to the UTP layer.
> Physical Host(PH) support all UFSHCI functions(all SAPs) same as
> conventional UFSHCI.
> Virtual Host(VH) support only data transfer function(UTP_CMD_SAP and
> UTP_TM_SAP).
> 
> In an environment where multiple OSs are used, the OS that has the
> leadership of the system is called System OS. This system OS uses PH and
> controls error handling.
> 
> Since VH can only use less functions than PH, it is necessary to send a request
> to PH for Detected Error Handling in VH. To interface among PH and VHs,
> UFSHCI HW supports mailbox. PH can broadcast mail to other VH at the same
> time with arguments and VH can mail to PH with arguments.
> PH and VH generate interrupts when mails from PH or VH.
> 
> In this structure, the virtual host can't support some feature and need to skip
> the some part of ufshcd code by using quirk.
> This patchs add quirks so that the UIC command is ignored and the ufshcd init
> process can be skipped for VH. Also, according to our UFS-IOV policy, we
> added a quirk to the abort handler or device reset handler to call the host
> reset handler.
> 
> jongmin jeong (3):
>   scsi: ufs: add quirk to handle broken UIC command
>   scsi: ufs: add quirk to enable host controller without interface
>     configuration
>   scsi: ufs: add quirk to support host reset only
> 
>  drivers/scsi/ufs/ufshcd.c | 13 +++++++++++++  drivers/scsi/ufs/ufshcd.h |
> 18 ++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
We do not encourage adding QUIRKs without having actual code that sets these quirks, so that we understand how these are actually getting used.
So better you post the all the changes, not just quirks.
> --
> 2.31.1

Re: [PATCH 0/3] Add quirk to support exynos ufshci

[Bart Van Assche]

On 5/26/21 8:08 PM, jongmin jeong wrote:
> In ExynosAuto(variant of the Exynos for automotive), the UFS Storage needs
> to be accessed from multi-OS.
> To increase IO performance and reduce SW complexity, we implemented UFS-IOV
> to support storage IO virtualization feature on UFS.
> 
> IO virtualization increases IO performance and reduce SW complexity
> with small area cost. And IO virtualization supports virtual machine isolation
> for Security and Safety which are requested by Multi-OS system such as
> automotive application.
> 
> Below figure is the conception of UFS-IOV architeture.
> 
>     +------+          +------+
>     | OS#1 |          | OS#2 |
>     +------+          +------+
>        |                 |
>  +------------+     +------------+
>  |  Physical  |     |   Virtual  |
>  |    Host    |     |    Host    |
>  +------------+     +------------+
>    |      |              | <-- UTP_CMD_SAP, UTP_TM_SAP
>    |   +-------------------------+
>    |   |    Function Arbitor     |
>    |   +-------------------------+
>  +-------------------------------+
>  |           UTP Layer           |
>  +-------------------------------+
>  +-------------------------------+
>  |           UIC Layer           |
>  +-------------------------------+
> 
> There are two types of host controllers on the UFS host controller
> that we designed.
> The UFS device has a Function Arbitor that arranges commands of each host.
> When each host transmits a command to the Arbitor, the Arbitor transmits it
> to the UTP layer.
> Physical Host(PH) support all UFSHCI functions(all SAPs) same as conventional
> UFSHCI.
> Virtual Host(VH) support only data transfer function(UTP_CMD_SAP and UTP_TM_SAP).
> 
> In an environment where multiple OSs are used, the OS that has the leadership of
> the system is called System OS. This system OS uses PH and controls error handling.
> 
> Since VH can only use less functions than PH, it is necessary to send a request
> to PH for Detected Error Handling in VH. To interface among PH and VHs,
> UFSHCI HW supports mailbox. PH can broadcast mail to other VH at the same time
> with arguments and VH can mail to PH with arguments.
> PH and VH generate interrupts when mails from PH or VH.
> 
> In this structure, the virtual host can't support some feature and need to skip
> the some part of ufshcd code by using quirk.
> This patchs add quirks so that the UIC command is ignored and the ufshcd init
> process can be skipped for VH. Also, according to our UFS-IOV policy,
> we added a quirk to the abort handler or device reset handler to call
> the host reset handler.

More information is needed about how multiple operating systems share a
single UFS device. Are LUNs shared across operating systems, does each
OS access separate LUNs or does each OS access different partitions on
the same LUN? Do you agree that for the latter two it is not necessary
to provide the virtual host access to the UTP layer?

Bart.