Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560028 > unrolled thread

Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands

Started byJames Bottomley <James.Bottomley@HansenPartnership.com>
First post2017-01-16 21:10 +0100
Last post2017-01-17 15:50 +0100
Articles 7 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-16 21:10 +0100
    Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-01-16 22:10 +0100
      Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands Ingo Molnar <mingo@kernel.org> - 2017-01-17 10:30 +0100
    Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands Sreekanth Reddy <sreekanth.reddy@broadcom.com> - 2017-01-17 15:20 +0100
      Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands Christoph Hellwig <hch@lst.de> - 2017-01-17 15:30 +0100
        Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-01-17 22:00 +0100
      Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-17 15:50 +0100

#1560028 — Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands

FromJames Bottomley <James.Bottomley@HansenPartnership.com>
Date2017-01-16 21:10 +0100
SubjectRe: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands
Message-ID<t0lLk-5zl-15@gated-at.bofh.it>
From 91d249409546569444897a1ffde65c421e064899 Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Sun, 1 Jan 2017 09:39:24 -0800
Subject: [PATCH] scsi: mpt3sas: fix hang on ata passthrough commands

mpt3sas has a firmware failure where it can only handle one pass
through ATA command at a time.  If another comes in, contrary to the
SAT standard, it will hang until the first one completes (causing long
commands like secure erase to timeout).  The original fix was to block
the device when an ATA command came in, but this caused a regression
with

commit 669f044170d8933c3d66d231b69ea97cb8447338
Author: Bart Van Assche <bart.vanassche@sandisk.com>
Date:   Tue Nov 22 16:17:13 2016 -0800

    scsi: srp_transport: Move queuecommand() wait code to SCSI core

So fix the original fix of the secure erase timeout by properly
returning SAM_STAT_BUSY like the SAT recommends.  The original patch
also had a concurrency problem since scsih_qcmd is lockless at that
point (this is fixed by using atomic bitops to set and test the flag).

Fixes: 18f6084a989ba1b (mpt3sas: Fix secure erase premature termination)
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

v2 - use bitops for lockless atomicity
v3 - update description, change function name
---
 drivers/scsi/mpt3sas/mpt3sas_base.h  | 12 +++++++++++
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 40 +++++++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 394fe13..dcb33f4 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -393,6 +393,7 @@ struct MPT3SAS_TARGET {
  * @eedp_enable: eedp support enable bit
  * @eedp_type: 0(type_1), 1(type_2), 2(type_3)
  * @eedp_block_length: block size
+ * @ata_command_pending: SATL passthrough outstanding for device
  */
 struct MPT3SAS_DEVICE {
 	struct MPT3SAS_TARGET *sas_target;
@@ -404,6 +405,17 @@ struct MPT3SAS_DEVICE {
 	u8	ignore_delay_remove;
 	/* Iopriority Command Handling */
 	u8	ncq_prio_enable;
+	/*
+	 * Bug workaround for SATL handling: the mpt2/3sas firmware
+	 * doesn't return BUSY or TASK_SET_FULL for subsequent
+	 * commands while a SATL pass through is in operation as the
+	 * spec requires, it simply does nothing with them until the
+	 * pass through completes, causing them possibly to timeout if
+	 * the passthrough is a long executing command (like format or
+	 * secure erase).  This variable allows us to do the right
+	 * thing while a SATL command is pending.
+	 */
+	unsigned long ata_command_pending;
 
 };
 
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index b5c966e..830e2c1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -3899,9 +3899,18 @@ _scsih_temp_threshold_events(struct MPT3SAS_ADAPTER *ioc,
 	}
 }
 
-static inline bool ata_12_16_cmd(struct scsi_cmnd *scmd)
+static int _scsih_set_satl_pending(struct scsi_cmnd *scmd, bool pending)
 {
-	return (scmd->cmnd[0] == ATA_12 || scmd->cmnd[0] == ATA_16);
+	struct MPT3SAS_DEVICE *priv = scmd->device->hostdata;
+
+	if  (scmd->cmnd[0] != ATA_12 && scmd->cmnd[0] != ATA_16)
+		return 0;
+
+	if (pending)
+		return test_and_set_bit(0, &priv->ata_command_pending);
+
+	clear_bit(0, &priv->ata_command_pending);
+	return 0;
 }
 
 /**
@@ -3925,9 +3934,7 @@ _scsih_flush_running_cmds(struct MPT3SAS_ADAPTER *ioc)
 		if (!scmd)
 			continue;
 		count++;
-		if (ata_12_16_cmd(scmd))
-			scsi_internal_device_unblock(scmd->device,
-							SDEV_RUNNING);
+		_scsih_set_satl_pending(scmd, false);
 		mpt3sas_base_free_smid(ioc, smid);
 		scsi_dma_unmap(scmd);
 		if (ioc->pci_error_recovery)
@@ -4063,13 +4070,6 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
 	if (ioc->logging_level & MPT_DEBUG_SCSI)
 		scsi_print_command(scmd);
 
-	/*
-	 * Lock the device for any subsequent command until command is
-	 * done.
-	 */
-	if (ata_12_16_cmd(scmd))
-		scsi_internal_device_block(scmd->device);
-
 	sas_device_priv_data = scmd->device->hostdata;
 	if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
 		scmd->result = DID_NO_CONNECT << 16;
@@ -4083,6 +4083,19 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
 		return 0;
 	}
 
+	/*
+	 * Bug work around for firmware SATL handling.  The loop
+	 * is based on atomic operations and ensures consistency
+	 * since we're lockless at this point
+	 */
+	do {
+		if (sas_device_priv_data->ata_command_pending) {
+			scmd->result = SAM_STAT_BUSY;
+			scmd->scsi_done(scmd);
+			return 0;
+		}
+	} while (_scsih_set_satl_pending(scmd, true));
+
 	sas_target_priv_data = sas_device_priv_data->sas_target;
 
 	/* invalid device handle */
@@ -4650,8 +4663,7 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
 	if (scmd == NULL)
 		return 1;
 
-	if (ata_12_16_cmd(scmd))
-		scsi_internal_device_unblock(scmd->device, SDEV_RUNNING);
+	_scsih_set_satl_pending(scmd, false);
 
 	mpi_request = mpt3sas_base_get_msg_frame(ioc, smid);
 
-- 
2.6.6

[toc] | [next] | [standalone]

#1560058

From"Martin K. Petersen" <martin.petersen@oracle.com>
Date2017-01-16 22:10 +0100
Message-ID<t0mHn-6bh-1@gated-at.bofh.it>
In reply to#1560028
>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:

James> Subject: [PATCH] scsi: mpt3sas: fix hang on ata passthrough
James> commands

James> mpt3sas has a firmware failure where it can only handle one pass
James> through ATA command at a time.  If another comes in, contrary to
James> the SAT standard, it will hang until the first one completes
James> (causing long commands like secure erase to timeout).  The
James> original fix was to block the device when an ATA command came in,
James> but this caused a regression with

Broadcom folks: Please test and ack as soon as possible so we can get
this fix queued up.

Ingo: Since you appear to have hardware, it would be great if you could
test James' v3 (https://patchwork.kernel.org/patch/9519383/). Sorry for
the inconvenience.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1560385

FromIngo Molnar <mingo@kernel.org>
Date2017-01-17 10:30 +0100
Message-ID<t0yfx-5dT-51@gated-at.bofh.it>
In reply to#1560058
* Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> 
> James> Subject: [PATCH] scsi: mpt3sas: fix hang on ata passthrough
> James> commands
> 
> James> mpt3sas has a firmware failure where it can only handle one pass
> James> through ATA command at a time.  If another comes in, contrary to
> James> the SAT standard, it will hang until the first one completes
> James> (causing long commands like secure erase to timeout).  The
> James> original fix was to block the device when an ATA command came in,
> James> but this caused a regression with
> 
> Broadcom folks: Please test and ack as soon as possible so we can get
> this fix queued up.
> 
> Ingo: Since you appear to have hardware, it would be great if you could
> test James' v3 (https://patchwork.kernel.org/patch/9519383/). Sorry for
> the inconvenience.

As per the interdiff below v2->v3 did not change the code in any way, only the 
name of the function and a comment, so you can add this to v3 as well:

  Reported-by: Ingo Molnar <mingo@kernel.org>
  Tested-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 6f9b4c051e4d..830e2c10ba02 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -3899,7 +3899,7 @@ _scsih_temp_threshold_events(struct MPT3SAS_ADAPTER *ioc,
 	}
 }
 
-static int set_satl_pending(struct scsi_cmnd *scmd, bool pending)
+static int _scsih_set_satl_pending(struct scsi_cmnd *scmd, bool pending)
 {
 	struct MPT3SAS_DEVICE *priv = scmd->device->hostdata;
 
@@ -3934,7 +3934,7 @@ _scsih_flush_running_cmds(struct MPT3SAS_ADAPTER *ioc)
 		if (!scmd)
 			continue;
 		count++;
-		set_satl_pending(scmd, false);
+		_scsih_set_satl_pending(scmd, false);
 		mpt3sas_base_free_smid(ioc, smid);
 		scsi_dma_unmap(scmd);
 		if (ioc->pci_error_recovery)
@@ -4084,7 +4084,9 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
 	}
 
 	/*
-	 * Bug work around for firmware SATL handling
+	 * Bug work around for firmware SATL handling.  The loop
+	 * is based on atomic operations and ensures consistency
+	 * since we're lockless at this point
 	 */
 	do {
 		if (sas_device_priv_data->ata_command_pending) {
@@ -4092,7 +4094,7 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
 			scmd->scsi_done(scmd);
 			return 0;
 		}
-	} while (set_satl_pending(scmd, true));
+	} while (_scsih_set_satl_pending(scmd, true));
 
 	sas_target_priv_data = sas_device_priv_data->sas_target;
 
@@ -4661,7 +4663,7 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
 	if (scmd == NULL)
 		return 1;
 
-	set_satl_pending(scmd, false);
+	_scsih_set_satl_pending(scmd, false);
 
 	mpi_request = mpt3sas_base_get_msg_frame(ioc, smid);

[toc] | [prev] | [next] | [standalone]

#1560624

FromSreekanth Reddy <sreekanth.reddy@broadcom.com>
Date2017-01-17 15:20 +0100
Message-ID<t0CM9-88g-5@gated-at.bofh.it>
In reply to#1560028
On Tue, Jan 17, 2017 at 1:31 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> From 91d249409546569444897a1ffde65c421e064899 Mon Sep 17 00:00:00 2001
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date: Sun, 1 Jan 2017 09:39:24 -0800
> Subject: [PATCH] scsi: mpt3sas: fix hang on ata passthrough commands
>
> mpt3sas has a firmware failure where it can only handle one pass
> through ATA command at a time.  If another comes in, contrary to the
> SAT standard, it will hang until the first one completes (causing long
> commands like secure erase to timeout).  The original fix was to block
> the device when an ATA command came in, but this caused a regression
> with
>
> commit 669f044170d8933c3d66d231b69ea97cb8447338
> Author: Bart Van Assche <bart.vanassche@sandisk.com>
> Date:   Tue Nov 22 16:17:13 2016 -0800
>
>     scsi: srp_transport: Move queuecommand() wait code to SCSI core
>
> So fix the original fix of the secure erase timeout by properly
> returning SAM_STAT_BUSY like the SAT recommends.  The original patch
> also had a concurrency problem since scsih_qcmd is lockless at that
> point (this is fixed by using atomic bitops to set and test the flag).
>
> Fixes: 18f6084a989ba1b (mpt3sas: Fix secure erase premature termination)
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>
> ---
>
> v2 - use bitops for lockless atomicity
> v3 - update description, change function name
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.h  | 12 +++++++++++
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 40 +++++++++++++++++++++++-------------
>  2 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
> index 394fe13..dcb33f4 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> @@ -393,6 +393,7 @@ struct MPT3SAS_TARGET {
>   * @eedp_enable: eedp support enable bit
>   * @eedp_type: 0(type_1), 1(type_2), 2(type_3)
>   * @eedp_block_length: block size
> + * @ata_command_pending: SATL passthrough outstanding for device
>   */
>  struct MPT3SAS_DEVICE {
>         struct MPT3SAS_TARGET *sas_target;
> @@ -404,6 +405,17 @@ struct MPT3SAS_DEVICE {
>         u8      ignore_delay_remove;
>         /* Iopriority Command Handling */
>         u8      ncq_prio_enable;
> +       /*
> +        * Bug workaround for SATL handling: the mpt2/3sas firmware
> +        * doesn't return BUSY or TASK_SET_FULL for subsequent
> +        * commands while a SATL pass through is in operation as the
> +        * spec requires, it simply does nothing with them until the
> +        * pass through completes, causing them possibly to timeout if
> +        * the passthrough is a long executing command (like format or
> +        * secure erase).  This variable allows us to do the right
> +        * thing while a SATL command is pending.
> +        */
> +       unsigned long ata_command_pending;
>
>  };
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index b5c966e..830e2c1 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -3899,9 +3899,18 @@ _scsih_temp_threshold_events(struct MPT3SAS_ADAPTER *ioc,
>         }
>  }
>
> -static inline bool ata_12_16_cmd(struct scsi_cmnd *scmd)
> +static int _scsih_set_satl_pending(struct scsi_cmnd *scmd, bool pending)
>  {
> -       return (scmd->cmnd[0] == ATA_12 || scmd->cmnd[0] == ATA_16);
> +       struct MPT3SAS_DEVICE *priv = scmd->device->hostdata;
> +
> +       if  (scmd->cmnd[0] != ATA_12 && scmd->cmnd[0] != ATA_16)
> +               return 0;
> +
> +       if (pending)
> +               return test_and_set_bit(0, &priv->ata_command_pending);
> +
> +       clear_bit(0, &priv->ata_command_pending);
> +       return 0;
>  }
>
>  /**
> @@ -3925,9 +3934,7 @@ _scsih_flush_running_cmds(struct MPT3SAS_ADAPTER *ioc)
>                 if (!scmd)
>                         continue;
>                 count++;
> -               if (ata_12_16_cmd(scmd))
> -                       scsi_internal_device_unblock(scmd->device,
> -                                                       SDEV_RUNNING);
> +               _scsih_set_satl_pending(scmd, false);
>                 mpt3sas_base_free_smid(ioc, smid);
>                 scsi_dma_unmap(scmd);
>                 if (ioc->pci_error_recovery)
> @@ -4063,13 +4070,6 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
>         if (ioc->logging_level & MPT_DEBUG_SCSI)
>                 scsi_print_command(scmd);
>
> -       /*
> -        * Lock the device for any subsequent command until command is
> -        * done.
> -        */
> -       if (ata_12_16_cmd(scmd))
> -               scsi_internal_device_block(scmd->device);
> -
>         sas_device_priv_data = scmd->device->hostdata;
>         if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
>                 scmd->result = DID_NO_CONNECT << 16;
> @@ -4083,6 +4083,19 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
>                 return 0;
>         }
>
> +       /*
> +        * Bug work around for firmware SATL handling.  The loop
> +        * is based on atomic operations and ensures consistency
> +        * since we're lockless at this point
> +        */
> +       do {
> +               if (sas_device_priv_data->ata_command_pending) {
> +                       scmd->result = SAM_STAT_BUSY;
> +                       scmd->scsi_done(scmd);
> +                       return 0;
> +               }
> +       } while (_scsih_set_satl_pending(scmd, true));
> +

[Sreekanth] Just for readability purpose, can use use "if (test_bit(0,
&sas_device_priv_data->ata_command_pending)"
 instead of "if (sas_device_priv_data->ata_command_pending)".
Since while setting & clearing the bit we are using atomic bit
operations. I don't see any issue functionality wise.

>         sas_target_priv_data = sas_device_priv_data->sas_target;
>
>         /* invalid device handle */
> @@ -4650,8 +4663,7 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
>         if (scmd == NULL)
>                 return 1;
>
> -       if (ata_12_16_cmd(scmd))
> -               scsi_internal_device_unblock(scmd->device, SDEV_RUNNING);
> +       _scsih_set_satl_pending(scmd, false);
>
>         mpi_request = mpt3sas_base_get_msg_frame(ioc, smid);
>
> --
> 2.6.6
>

Tested this patch. It is working fine. Please consider this patch as

Acked-by: Sreekanth Reddy <Sreekanth.Reddy@broadcom.com>

Thanks,
Sreekanth

[toc] | [prev] | [next] | [standalone]

#1560636

FromChristoph Hellwig <hch@lst.de>
Date2017-01-17 15:30 +0100
Message-ID<t0CVQ-8bo-15@gated-at.bofh.it>
In reply to#1560624
On Tue, Jan 17, 2017 at 07:43:51PM +0530, Sreekanth Reddy wrote:
> [Sreekanth] Just for readability purpose, can use use "if (test_bit(0,
> &sas_device_priv_data->ata_command_pending)"
>  instead of "if (sas_device_priv_data->ata_command_pending)".
> Since while setting & clearing the bit we are using atomic bit
> operations. I don't see any issue functionality wise.

I agree.  Also while we're into nitpicking - it would be good to
give bit 0 of the bitmap a name instead of hardcoding the 0.

Except for these patch looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1561015

From"Martin K. Petersen" <martin.petersen@oracle.com>
Date2017-01-17 22:00 +0100
Message-ID<t0J1g-3nu-15@gated-at.bofh.it>
In reply to#1560636
>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> On Tue, Jan 17, 2017 at 07:43:51PM +0530, Sreekanth Reddy wrote:
>> [Sreekanth] Just for readability purpose, can use use "if
>> (test_bit(0, &sas_device_priv_data->ata_command_pending)" instead of
>> "if (sas_device_priv_data->ata_command_pending)".  Since while
>> setting & clearing the bit we are using atomic bit operations. I
>> don't see any issue functionality wise.

Christoph> I agree.  Also while we're into nitpicking - it would be good
Christoph> to give bit 0 of the bitmap a name instead of hardcoding the
Christoph> 0.

I tweaked the test case. We can name the bit later if more flags are
needed (and in that case the ata_command_pending would need to get
renamed too).

In any case. This issue has taken waaay too long to get resolved so the
patch is now queued up in 4.10/scsi-fixes.

Thanks everyone!

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1560663

FromJames Bottomley <James.Bottomley@HansenPartnership.com>
Date2017-01-17 15:50 +0100
Message-ID<t0Dfb-8i5-5@gated-at.bofh.it>
In reply to#1560624
On Tue, 2017-01-17 at 19:43 +0530, Sreekanth Reddy wrote:
> On Tue, Jan 17, 2017 at 1:31 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > From 91d249409546569444897a1ffde65c421e064899 Mon Sep 17 00:00:00
> > 2001
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Date: Sun, 1 Jan 2017 09:39:24 -0800
> > Subject: [PATCH] scsi: mpt3sas: fix hang on ata passthrough
> > commands
> > 
> > mpt3sas has a firmware failure where it can only handle one pass
> > through ATA command at a time.  If another comes in, contrary to
> > the
> > SAT standard, it will hang until the first one completes (causing
> > long
> > commands like secure erase to timeout).  The original fix was to
> > block
> > the device when an ATA command came in, but this caused a
> > regression
> > with
> > 
> > commit 669f044170d8933c3d66d231b69ea97cb8447338
> > Author: Bart Van Assche <bart.vanassche@sandisk.com>
> > Date:   Tue Nov 22 16:17:13 2016 -0800
> > 
> >     scsi: srp_transport: Move queuecommand() wait code to SCSI core
> > 
> > So fix the original fix of the secure erase timeout by properly
> > returning SAM_STAT_BUSY like the SAT recommends.  The original
> > patch
> > also had a concurrency problem since scsih_qcmd is lockless at that
> > point (this is fixed by using atomic bitops to set and test the
> > flag).
> > 
> > Fixes: 18f6084a989ba1b (mpt3sas: Fix secure erase premature
> > termination)
> > Signed-off-by: James Bottomley <
> > James.Bottomley@HansenPartnership.com>
> > 
> > ---
> > 
> > v2 - use bitops for lockless atomicity
> > v3 - update description, change function name
> > ---
> >  drivers/scsi/mpt3sas/mpt3sas_base.h  | 12 +++++++++++
> >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 40 +++++++++++++++++++++++-
> > ------------
> >  2 files changed, 38 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
> > b/drivers/scsi/mpt3sas/mpt3sas_base.h
> > index 394fe13..dcb33f4 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> > @@ -393,6 +393,7 @@ struct MPT3SAS_TARGET {
> >   * @eedp_enable: eedp support enable bit
> >   * @eedp_type: 0(type_1), 1(type_2), 2(type_3)
> >   * @eedp_block_length: block size
> > + * @ata_command_pending: SATL passthrough outstanding for device
> >   */
> >  struct MPT3SAS_DEVICE {
> >         struct MPT3SAS_TARGET *sas_target;
> > @@ -404,6 +405,17 @@ struct MPT3SAS_DEVICE {
> >         u8      ignore_delay_remove;
> >         /* Iopriority Command Handling */
> >         u8      ncq_prio_enable;
> > +       /*
> > +        * Bug workaround for SATL handling: the mpt2/3sas firmware
> > +        * doesn't return BUSY or TASK_SET_FULL for subsequent
> > +        * commands while a SATL pass through is in operation as
> > the
> > +        * spec requires, it simply does nothing with them until
> > the
> > +        * pass through completes, causing them possibly to timeout
> > if
> > +        * the passthrough is a long executing command (like format
> > or
> > +        * secure erase).  This variable allows us to do the right
> > +        * thing while a SATL command is pending.
> > +        */
> > +       unsigned long ata_command_pending;
> > 
> >  };
> > 
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > index b5c966e..830e2c1 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > @@ -3899,9 +3899,18 @@ _scsih_temp_threshold_events(struct
> > MPT3SAS_ADAPTER *ioc,
> >         }
> >  }
> > 
> > -static inline bool ata_12_16_cmd(struct scsi_cmnd *scmd)
> > +static int _scsih_set_satl_pending(struct scsi_cmnd *scmd, bool
> > pending)
> >  {
> > -       return (scmd->cmnd[0] == ATA_12 || scmd->cmnd[0] ==
> > ATA_16);
> > +       struct MPT3SAS_DEVICE *priv = scmd->device->hostdata;
> > +
> > +       if  (scmd->cmnd[0] != ATA_12 && scmd->cmnd[0] != ATA_16)
> > +               return 0;
> > +
> > +       if (pending)
> > +               return test_and_set_bit(0, &priv
> > ->ata_command_pending);
> > +
> > +       clear_bit(0, &priv->ata_command_pending);
> > +       return 0;
> >  }
> > 
> >  /**
> > @@ -3925,9 +3934,7 @@ _scsih_flush_running_cmds(struct
> > MPT3SAS_ADAPTER *ioc)
> >                 if (!scmd)
> >                         continue;
> >                 count++;
> > -               if (ata_12_16_cmd(scmd))
> > -                       scsi_internal_device_unblock(scmd->device,
> > -                                                      
> >  SDEV_RUNNING);
> > +               _scsih_set_satl_pending(scmd, false);
> >                 mpt3sas_base_free_smid(ioc, smid);
> >                 scsi_dma_unmap(scmd);
> >                 if (ioc->pci_error_recovery)
> > @@ -4063,13 +4070,6 @@ scsih_qcmd(struct Scsi_Host *shost, struct
> > scsi_cmnd *scmd)
> >         if (ioc->logging_level & MPT_DEBUG_SCSI)
> >                 scsi_print_command(scmd);
> > 
> > -       /*
> > -        * Lock the device for any subsequent command until command
> > is
> > -        * done.
> > -        */
> > -       if (ata_12_16_cmd(scmd))
> > -               scsi_internal_device_block(scmd->device);
> > -
> >         sas_device_priv_data = scmd->device->hostdata;
> >         if (!sas_device_priv_data || !sas_device_priv_data
> > ->sas_target) {
> >                 scmd->result = DID_NO_CONNECT << 16;
> > @@ -4083,6 +4083,19 @@ scsih_qcmd(struct Scsi_Host *shost, struct
> > scsi_cmnd *scmd)
> >                 return 0;
> >         }
> > 
> > +       /*
> > +        * Bug work around for firmware SATL handling.  The loop
> > +        * is based on atomic operations and ensures consistency
> > +        * since we're lockless at this point
> > +        */
> > +       do {
> > +               if (sas_device_priv_data->ata_command_pending) {
> > +                       scmd->result = SAM_STAT_BUSY;
> > +                       scmd->scsi_done(scmd);
> > +                       return 0;
> > +               }
> > +       } while (_scsih_set_satl_pending(scmd, true));
> > +
> 
> [Sreekanth] Just for readability purpose, can use use "if
> (test_bit(0,
> &sas_device_priv_data->ata_command_pending)"
>  instead of "if (sas_device_priv_data->ata_command_pending)".
> Since while setting & clearing the bit we are using atomic bit
> operations. I don't see any issue functionality wise.

It's taste I suppose.  I like the idea of exposing a true or false
value that can be read but which uses bitops under the cover to ensure
atomicity.

The clincher for why not is probably that if you want another go
around, I'm just about to get on a 'plane, so it won't get to it for a
while and Linus will likely revert the other patch if we don't get a
fix in before -rc4

James

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web