[RESEND RFC PATCH] mpt3sas: support target smid for [abort|query] task

[RESEND RFC PATCH] mpt3sas: support target smid for [abort|query] task

[Minwoo Im]

We can request task management IOCTL command(MPI2_FUNCTION_SCSI_TASK_MGMT)
to /dev/mpt3ctl.  If the given task_type is either abort task or query
task, it may need a field named "Initiator Port Transfer Tag to Manage"
in the IU.

Current code does not support to check target IPTT tag from the
tm_request.  This patch introduces to check TaskMID given from the
userspace as a target tag.  We have a rule of relationship between
(struct request *req->tag) and smid in mpt3sas_base.c:

3318 u16
3319 mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
3320         struct scsi_cmnd *scmd)
3321 {
3322         struct scsiio_tracker *request = scsi_cmd_priv(scmd);
3323         unsigned int tag = scmd->request->tag;
3324         u16 smid;
3325
3326         smid = tag + 1;

So if we want to abort a request tagged #X, then we can pass (X + 1) to
this IOCTL handler.

Cc: Sathya Prakash <sathya.prakash@broadcom.com>
Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
Cc: James E.J. Bottomley <jejb@linux.ibm.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: MPT-FusionLinux.pdl@broadcom.com
Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index b2bb47c14d35..5c7539dae713 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -596,15 +596,17 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg,
 		if (priv_data->sas_target->handle != handle)
 			continue;
 		st = scsi_cmd_priv(scmd);
-		tm_request->TaskMID = cpu_to_le16(st->smid);
-		found = 1;
+		if (tm_request->TaskMID == st->smid) {
+			tm_request->TaskMID = cpu_to_le16(st->smid);
+			found = 1;
+		}
 	}
 
 	if (!found) {
 		dctlprintk(ioc,
-			   ioc_info(ioc, "%s: handle(0x%04x), lun(%d), no active mid!!\n",
+			   ioc_info(ioc, "%s: handle(0x%04x), lun(%d), no matched mid(%d)!!\n",
 				    desc, le16_to_cpu(tm_request->DevHandle),
-				    lun));
+				    lun, tm_request->TaskMID));
 		tm_reply = ioc->ctl_cmds.reply;
 		tm_reply->DevHandle = tm_request->DevHandle;
 		tm_reply->Function = MPI2_FUNCTION_SCSI_TASK_MGMT;
-- 
2.16.1

Re: [RESEND RFC PATCH] mpt3sas: support target smid for [abort|query] task

[Minwoo Im]

Gentle ping. :)

> -----Original Message-----
> From: Minwoo Im <minwoo.im@samsung.com>
> Sent: Friday, June 21, 2019 3:37 PM
> To: sathya.prakash@broadcom.com; suganath-prabu.subramani@broadcom.com;
> jejb@linux.ibm.com; martin.petersen@oracle.com
> Cc: Minwoo Im <minwoo.im@samsung.com>; MPT-FusionLinux.pdl@broadcom.com;
> linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; linux-
> block@vger.kernel.org; Euihyeok Kwon <eh81.kwon@samsung.com>; Sarah Cho
> <sohyeon.jo@samsung.com>; Sanggwan Lee <sanggwan.lee@samsung.com>;
> Gyeongmin Nam <gm.nam@samsung.com>
> Subject: [RESEND RFC PATCH] mpt3sas: support target smid for [abort|query]
> task
> 
> We can request task management IOCTL command(MPI2_FUNCTION_SCSI_TASK_MGMT)
> to /dev/mpt3ctl.  If the given task_type is either abort task or query
> task, it may need a field named "Initiator Port Transfer Tag to Manage"
> in the IU.
> 
> Current code does not support to check target IPTT tag from the
> tm_request.  This patch introduces to check TaskMID given from the
> userspace as a target tag.  We have a rule of relationship between
> (struct request *req->tag) and smid in mpt3sas_base.c:
> 
> 3318 u16
> 3319 mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
> 3320         struct scsi_cmnd *scmd)
> 3321 {
> 3322         struct scsiio_tracker *request = scsi_cmd_priv(scmd);
> 3323         unsigned int tag = scmd->request->tag;
> 3324         u16 smid;
> 3325
> 3326         smid = tag + 1;
> 
> So if we want to abort a request tagged #X, then we can pass (X + 1) to
> this IOCTL handler.
> 
> Cc: Sathya Prakash <sathya.prakash@broadcom.com>
> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
> Cc: James E.J. Bottomley <jejb@linux.ibm.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: MPT-FusionLinux.pdl@broadcom.com
> Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index b2bb47c14d35..5c7539dae713 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -596,15 +596,17 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc,
> struct mpt3_ioctl_command *karg,
>  		if (priv_data->sas_target->handle != handle)
>  			continue;
>  		st = scsi_cmd_priv(scmd);
> -		tm_request->TaskMID = cpu_to_le16(st->smid);
> -		found = 1;
> +		if (tm_request->TaskMID == st->smid) {
> +			tm_request->TaskMID = cpu_to_le16(st->smid);
> +			found = 1;
> +		}
>  	}
> 
>  	if (!found) {
>  		dctlprintk(ioc,
> -			   ioc_info(ioc, "%s: handle(0x%04x), lun(%d), no
> active mid!!\n",
> +			   ioc_info(ioc, "%s: handle(0x%04x), lun(%d), no
> matched mid(%d)!!\n",
>  				    desc, le16_to_cpu(tm_request->DevHandle),
> -				    lun));
> +				    lun, tm_request->TaskMID));
>  		tm_reply = ioc->ctl_cmds.reply;
>  		tm_reply->DevHandle = tm_request->DevHandle;
>  		tm_reply->Function = MPI2_FUNCTION_SCSI_TASK_MGMT;
> --
> 2.16.1

Re: [RESEND RFC PATCH] mpt3sas: support target smid for [abort|query] task

[Sreekanth Reddy]

On Fri, Jun 21, 2019 at 12:07 PM Minwoo Im <minwoo.im@samsung.com> wrote:
>
> We can request task management IOCTL command(MPI2_FUNCTION_SCSI_TASK_MGMT)
> to /dev/mpt3ctl.  If the given task_type is either abort task or query
> task, it may need a field named "Initiator Port Transfer Tag to Manage"
> in the IU.
>
> Current code does not support to check target IPTT tag from the
> tm_request.  This patch introduces to check TaskMID given from the
> userspace as a target tag.  We have a rule of relationship between
> (struct request *req->tag) and smid in mpt3sas_base.c:
>
> 3318 u16
> 3319 mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
> 3320         struct scsi_cmnd *scmd)
> 3321 {
> 3322         struct scsiio_tracker *request = scsi_cmd_priv(scmd);
> 3323         unsigned int tag = scmd->request->tag;
> 3324         u16 smid;
> 3325
> 3326         smid = tag + 1;
>
> So if we want to abort a request tagged #X, then we can pass (X + 1) to
> this IOCTL handler.
>
> Cc: Sathya Prakash <sathya.prakash@broadcom.com>
> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
> Cc: James E.J. Bottomley <jejb@linux.ibm.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: MPT-FusionLinux.pdl@broadcom.com
> Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index b2bb47c14d35..5c7539dae713 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -596,15 +596,17 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg,
>                 if (priv_data->sas_target->handle != handle)
>                         continue;
>                 st = scsi_cmd_priv(scmd);
> -               tm_request->TaskMID = cpu_to_le16(st->smid);
> -               found = 1;
> +               if (tm_request->TaskMID == st->smid) {

I think it will difficult for the user to find the smid that he want
to abort. For this user has to enable the scsi logging level and get
the tag and pass the ioctl with tag +1 value in TaskMID field. And
hence currently driver will loop over all the smid's and if it fines
any outstanding smid then it will issue task abort or task query TM
for this outstanding smid to the HBA firmware.

May be we can do like below,
* First check whether user provided "TaskMID" is non zero or not. if
user provided TaskMID is non-zero and if this TaskMID is outstanding
then driver will issue TaskAbort/QueryTask TM with this TaskMID value
else driver will loop over all the smid's and if finds any smid is
outstanding then it will issue TaskAbort/QueryTask TM with TaskMID
value set to outstanding smid.

With the above logic still legacy application will be supported
without breaking anything where they provide TaskMID filed as zero.
And it also allows the user to abort the IO which he wants.

Thanks,
Sreekanth

> +                       tm_request->TaskMID = cpu_to_le16(st->smid);
> +                       found = 1;
> +               }
>         }
>
>         if (!found) {
>                 dctlprintk(ioc,
> -                          ioc_info(ioc, "%s: handle(0x%04x), lun(%d), no active mid!!\n",
> +                          ioc_info(ioc, "%s: handle(0x%04x), lun(%d), no matched mid(%d)!!\n",
>                                     desc, le16_to_cpu(tm_request->DevHandle),
> -                                   lun));
> +                                   lun, tm_request->TaskMID));
>                 tm_reply = ioc->ctl_cmds.reply;
>                 tm_reply->DevHandle = tm_request->DevHandle;
>                 tm_reply->Function = MPI2_FUNCTION_SCSI_TASK_MGMT;
> --
> 2.16.1

Re: [RESEND RFC PATCH] mpt3sas: support target smid for [abort|query] task

[Minwoo Im]

Hi Sreekanth,

> >  drivers/scsi/mpt3sas/mpt3sas_ctl.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> > index b2bb47c14d35..5c7539dae713 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> > @@ -596,15 +596,17 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc,
> struct mpt3_ioctl_command *karg,
> >                 if (priv_data->sas_target->handle != handle)
> >                         continue;
> >                 st = scsi_cmd_priv(scmd);
> > -               tm_request->TaskMID = cpu_to_le16(st->smid);
> > -               found = 1;
> > +               if (tm_request->TaskMID == st->smid) {
> 
> I think it will difficult for the user to find the smid that he want
> to abort. For this user has to enable the scsi logging level and get
> the tag and pass the ioctl with tag +1 value in TaskMID field. And
> hence currently driver will loop over all the smid's and if it fines
> any outstanding smid then it will issue task abort or task query TM
> for this outstanding smid to the HBA firmware.

Sreekanth,

You're exactly right because I have done this kind of abort based on
The scsi logs with logging level configured.

> 
> May be we can do like below,
> * First check whether user provided "TaskMID" is non zero or not. if
> user provided TaskMID is non-zero and if this TaskMID is outstanding
> then driver will issue TaskAbort/QueryTask TM with this TaskMID value

Okay.  If I get it right, you mean to check the given TaskMID(!=0) is
Outstanding or not is by an iteration first.

> else driver will loop over all the smid's and if finds any smid is
> outstanding then it will issue TaskAbort/QueryTask TM with TaskMID
> value set to outstanding smid.

Got your point here.  I'll make a conditional statement for the case
where the TaskMID is zero which is the legacy behaviour.

> With the above logic still legacy application will be supported
> without breaking anything where they provide TaskMID filed as zero.
> And it also allows the user to abort the IO which he wants.

Sure, I'll prepare V2 patch soon.

Thanks Sreekanth,

		Minwoo Im