[PATCH 0/5] scsi: libsas: some code cleanups and bug fixes

[PATCH 0/5] scsi: libsas: some code cleanups and bug fixes

[Jason Yan]

I split some code cleanups and bug fixes patches from my earlier series:
https://lkml.org/lkml/2018/5/28/2154

These patches are separate to the subject of the earlier series and are just
small fixes. Hope it is much easier to review and test.

Jason Yan (5):
  scsi: libsas: delete dead code in scsi_transport_sas.c
  scsi: libsas: check the lldd callback correctly
  scsi: libsas: always unregister the old device if going to discover
    new
  scsi: libsas: check the ata device status by ata_dev_enabled()
  scsi: libsas: fix a race condition when smp task timeout

 drivers/scsi/hisi_sas/hisi_sas_main.c |  9 ++-------
 drivers/scsi/libsas/sas_ata.c         |  2 +-
 drivers/scsi/libsas/sas_discover.c    |  2 +-
 drivers/scsi/libsas/sas_expander.c    | 22 +++++++++-------------
 drivers/scsi/scsi_transport_sas.c     |  2 --
 5 files changed, 13 insertions(+), 24 deletions(-)

-- 
2.14.4

[PATCH 1/5] scsi: libsas: delete dead code in scsi_transport_sas.c

[Jason Yan]

This code is dead and no clue implies that it will be back again.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_transport_sas.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 0cd16e80b019..0a165b2b3e81 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -612,7 +612,6 @@ sas_phy_protocol_attr(identify.target_port_protocols,
 sas_phy_simple_attr(identify.sas_address, sas_address, "0x%016llx\n",
 		unsigned long long);
 sas_phy_simple_attr(identify.phy_identifier, phy_identifier, "%d\n", u8);
-//sas_phy_simple_attr(port_identifier, port_identifier, "%d\n", int);
 sas_phy_linkspeed_attr(negotiated_linkrate);
 sas_phy_linkspeed_attr(minimum_linkrate_hw);
 sas_phy_linkspeed_rw_attr(minimum_linkrate);
@@ -1802,7 +1801,6 @@ sas_attach_transport(struct sas_function_template *ft)
 	SETUP_PHY_ATTRIBUTE(device_type);
 	SETUP_PHY_ATTRIBUTE(sas_address);
 	SETUP_PHY_ATTRIBUTE(phy_identifier);
-	//SETUP_PHY_ATTRIBUTE(port_identifier);
 	SETUP_PHY_ATTRIBUTE(negotiated_linkrate);
 	SETUP_PHY_ATTRIBUTE(minimum_linkrate_hw);
 	SETUP_PHY_ATTRIBUTE_RW(minimum_linkrate);
-- 
2.14.4

[PATCH 2/5] scsi: libsas: check the lldd callback correctly

[Jason Yan]

We are using lldd_port_deformed so we'd better check if lldd_port_deformed
is NULL.

After this, we can remove hisi_sas_port_deformed() because it is just a
stub to avoid a NULL dereference caused by the wrong check.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 9 ++-------
 drivers/scsi/libsas/sas_discover.c    | 2 +-
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index a4e2e6aa9a6b..1975c9266978 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1861,10 +1861,6 @@ static void hisi_sas_port_formed(struct asd_sas_phy *sas_phy)
 	hisi_sas_port_notify_formed(sas_phy);
 }
 
-static void hisi_sas_port_deformed(struct asd_sas_phy *sas_phy)
-{
-}
-
 static int hisi_sas_write_gpio(struct sas_ha_struct *sha, u8 reg_type,
 			u8 reg_index, u8 reg_count, u8 *write_data)
 {
@@ -1954,10 +1950,9 @@ static struct sas_domain_function_template hisi_sas_transport_ops = {
 	.lldd_I_T_nexus_reset	= hisi_sas_I_T_nexus_reset,
 	.lldd_lu_reset		= hisi_sas_lu_reset,
 	.lldd_query_task	= hisi_sas_query_task,
-	.lldd_clear_nexus_ha = hisi_sas_clear_nexus_ha,
+	.lldd_clear_nexus_ha	= hisi_sas_clear_nexus_ha,
 	.lldd_port_formed	= hisi_sas_port_formed,
-	.lldd_port_deformed = hisi_sas_port_deformed,
-	.lldd_write_gpio = hisi_sas_write_gpio,
+	.lldd_write_gpio	= hisi_sas_write_gpio,
 };
 
 void hisi_sas_init_mem(struct hisi_hba *hisi_hba)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 0148ae62a52a..dde433aa59c2 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -260,7 +260,7 @@ static void sas_suspend_devices(struct work_struct *work)
 	 * phy_list is not being mutated
 	 */
 	list_for_each_entry(phy, &port->phy_list, port_phy_el) {
-		if (si->dft->lldd_port_formed)
+		if (si->dft->lldd_port_deformed)
 			si->dft->lldd_port_deformed(phy);
 		phy->suspended = 1;
 		port->suspended = 1;
-- 
2.14.4

[PATCH 3/5] scsi: libsas: always unregister the old device if going to discover new

[Jason Yan]

If we went into sas_rediscover_dev() the attached_sas_addr was already
insured not to be zero. So it's unnecessary to check if the
attached_sas_addr is zero.

And although if the sas address is not changed, we always have to
unregister the old device when we are going to register a new one. We
cannot just leave the device there and bring up the new.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: chenxiang <chenxiang66@hisilicon.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/libsas/sas_expander.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index fadc99cb60df..52222940d398 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -2054,14 +2054,11 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
 		return res;
 	}
 
-	/* delete the old link */
-	if (SAS_ADDR(phy->attached_sas_addr) &&
-	    SAS_ADDR(sas_addr) != SAS_ADDR(phy->attached_sas_addr)) {
-		SAS_DPRINTK("ex %016llx phy 0x%x replace %016llx\n",
-			    SAS_ADDR(dev->sas_addr), phy_id,
-			    SAS_ADDR(phy->attached_sas_addr));
-		sas_unregister_devs_sas_addr(dev, phy_id, last);
-	}
+	/* we always have to delete the old device when we went here */
+	SAS_DPRINTK("ex %016llx phy 0x%x replace %016llx\n",
+		    SAS_ADDR(dev->sas_addr), phy_id,
+		    SAS_ADDR(phy->attached_sas_addr));
+	sas_unregister_devs_sas_addr(dev, phy_id, last);
 
 	return sas_discover_new(dev, phy_id);
 }
-- 
2.14.4

[PATCH 4/5] scsi: libsas: check the ata device status by ata_dev_enabled()

[Jason Yan]

When ata device IDENTIFY failed, the ata device status is
ATA_DEV_UNKNOWN. The libata reported like:

[113518.620433] ata5.00: qc timeout (cmd 0xec)
[113518.653646] ata5.00: failed to IDENTIFY (I/O error, err_mask=0x4)

But libsas verifies the device status by ata_dev_disabled(), which
skiped ATA_DEV_UNKNOWN. This will make libsas think the ata device
probing succeed the device cannot be actually brought up. And even the
new bcast of this device will be considered as flutter and will not
probe this device again.

Change ata_dev_disabled() to !ata_dev_enabled() so that libsas can
deal with this if the ata device probe failed. New bcasts can let us
try to probe the device again and bring it up if it is fine to
IDENTIFY.

Tested-by: Zhou Yupeng <zhouyupeng1@huawei.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libsas/sas_ata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 64a958a99f6a..4f6cdf53e913 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -654,7 +654,7 @@ void sas_probe_sata(struct asd_sas_port *port)
 		/* if libata could not bring the link up, don't surface
 		 * the device
 		 */
-		if (ata_dev_disabled(sas_to_ata_dev(dev)))
+		if (!ata_dev_enabled(sas_to_ata_dev(dev)))
 			sas_fail_probe(dev, __func__, -ENODEV);
 	}
 
-- 
2.14.4

[PATCH 5/5] scsi: libsas: fix a race condition when smp task timeout

[Jason Yan]

When the lldd is processing the complete sas task in interrupt and set
the task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to
be triggered at the same time. And smp_task_timedout() will complete the
task wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may
freed before lldd end the interrupt process. Thus a use-after-free will
happen.

Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
set. And remove the check of the return value of the del_timer().

Reported-by: chenxiang <chenxiang66@hisilicon.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libsas/sas_expander.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 52222940d398..0d1f72752ca2 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -48,17 +48,16 @@ static void smp_task_timedout(struct timer_list *t)
 	unsigned long flags;
 
 	spin_lock_irqsave(&task->task_state_lock, flags);
-	if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
+	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
 		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
+		complete(&task->slow_task->completion);
+	}
 	spin_unlock_irqrestore(&task->task_state_lock, flags);
-
-	complete(&task->slow_task->completion);
 }
 
 static void smp_task_done(struct sas_task *task)
 {
-	if (!del_timer(&task->slow_task->timer))
-		return;
+	del_timer(&task->slow_task->timer);
 	complete(&task->slow_task->completion);
 }
 
-- 
2.14.4

Re: [PATCH 2/5] scsi: libsas: check the lldd callback correctly

[John Garry]

On 12/09/2018 09:29, Jason Yan wrote:
> We are using lldd_port_deformed so we'd better check if lldd_port_deformed
> is NULL.
>
> After this, we can remove hisi_sas_port_deformed() because it is just a
> stub to avoid a NULL dereference caused by the wrong check.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Acked-by: John Garry <john.garry@huawei.com>

> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---

Re: [PATCH 1/5] scsi: libsas: delete dead code in scsi_transport_sas.c

[John Garry]

On 12/09/2018 09:29, Jason Yan wrote:
> This code is dead and no clue implies that it will be back again.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Reviewed-by: John Garry <john.garry@huawei.com>

> ---
>  drivers/scsi/scsi_transport_sas.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index 0cd16e80b019..0a165b2b3e81 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -612,7 +612,6 @@ sas_phy_protocol_attr(identify.target_port_protocols,
>  sas_phy_simple_attr(identify.sas_address, sas_address, "0x%016llx\n",
>  		unsigned long long);
>  sas_phy_simple_attr(identify.phy_identifier, phy_identifier, "%d\n", u8);
> -//sas_phy_simple_attr(port_identifier, port_identifier, "%d\n", int);
>  sas_phy_linkspeed_attr(negotiated_linkrate);
>  sas_phy_linkspeed_attr(minimum_linkrate_hw);
>  sas_phy_linkspeed_rw_attr(minimum_linkrate);
> @@ -1802,7 +1801,6 @@ sas_attach_transport(struct sas_function_template *ft)
>  	SETUP_PHY_ATTRIBUTE(device_type);
>  	SETUP_PHY_ATTRIBUTE(sas_address);
>  	SETUP_PHY_ATTRIBUTE(phy_identifier);
> -	//SETUP_PHY_ATTRIBUTE(port_identifier);
>  	SETUP_PHY_ATTRIBUTE(negotiated_linkrate);
>  	SETUP_PHY_ATTRIBUTE(minimum_linkrate_hw);
>  	SETUP_PHY_ATTRIBUTE_RW(minimum_linkrate);
>

Re: [PATCH 5/5] scsi: libsas: fix a race condition when smp task timeout

[John Garry]

On 12/09/2018 09:29, Jason Yan wrote:
> When the lldd is processing the complete sas task in interrupt and set
> the task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to
> be triggered at the same time. And smp_task_timedout() will complete the
> task wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may
> freed before lldd end the interrupt process. Thus a use-after-free will
> happen.
>
> Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
> set. And remove the check of the return value of the del_timer().
>

Hi Jason,

Please mention that once the LLDD sets DONE, it must call task->done(), 
which will call smp_task_done()->complete()

> Reported-by: chenxiang <chenxiang66@hisilicon.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 52222940d398..0d1f72752ca2 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -48,17 +48,16 @@ static void smp_task_timedout(struct timer_list *t)
>  	unsigned long flags;
>
>  	spin_lock_irqsave(&task->task_state_lock, flags);
> -	if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
> +	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>  		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
> +		complete(&task->slow_task->completion);

Nit: for consistency with any other time we use this lock, can we call 
complete() outside the lock? Maybe just use a flag variable for this.

> +	}
>  	spin_unlock_irqrestore(&task->task_state_lock, flags);
> -
> -	complete(&task->slow_task->completion);
>  }
>
>  static void smp_task_done(struct sas_task *task)
>  {
> -	if (!del_timer(&task->slow_task->timer))
> -		return;
> +	del_timer(&task->slow_task->timer);
>  	complete(&task->slow_task->completion);
>  }

Do we also need this change or similar:
static int smp_execute_task_sg(struct domain_device *dev,
	if ((task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
		SAS_DPRINTK("smp task timed out or aborted\n");
		i->dft->lldd_abort_task(task);
-		if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
-			SAS_DPRINTK("SMP task aborted and not done\n");
-			break;
-		}
+		break;

To me, the ABORTED and DONE states are mutually exclusive.

>
>

Thanks,
John

Re: [PATCH 1/5] scsi: libsas: delete dead code in scsi_transport_sas.c

[Hannes Reinecke]

On 09/12/2018 10:29 AM, Jason Yan wrote:
> This code is dead and no clue implies that it will be back again.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/scsi_transport_sas.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index 0cd16e80b019..0a165b2b3e81 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -612,7 +612,6 @@ sas_phy_protocol_attr(identify.target_port_protocols,
>  sas_phy_simple_attr(identify.sas_address, sas_address, "0x%016llx\n",
>  		unsigned long long);
>  sas_phy_simple_attr(identify.phy_identifier, phy_identifier, "%d\n", u8);
> -//sas_phy_simple_attr(port_identifier, port_identifier, "%d\n", int);
>  sas_phy_linkspeed_attr(negotiated_linkrate);
>  sas_phy_linkspeed_attr(minimum_linkrate_hw);
>  sas_phy_linkspeed_rw_attr(minimum_linkrate);
> @@ -1802,7 +1801,6 @@ sas_attach_transport(struct sas_function_template *ft)
>  	SETUP_PHY_ATTRIBUTE(device_type);
>  	SETUP_PHY_ATTRIBUTE(sas_address);
>  	SETUP_PHY_ATTRIBUTE(phy_identifier);
> -	//SETUP_PHY_ATTRIBUTE(port_identifier);
>  	SETUP_PHY_ATTRIBUTE(negotiated_linkrate);
>  	SETUP_PHY_ATTRIBUTE(minimum_linkrate_hw);
>  	SETUP_PHY_ATTRIBUTE_RW(minimum_linkrate);
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH 2/5] scsi: libsas: check the lldd callback correctly

[Hannes Reinecke]

On 09/12/2018 10:29 AM, Jason Yan wrote:
> We are using lldd_port_deformed so we'd better check if lldd_port_deformed
> is NULL.
> 
> After this, we can remove hisi_sas_port_deformed() because it is just a
> stub to avoid a NULL dereference caused by the wrong check.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/hisi_sas/hisi_sas_main.c | 9 ++-------
>  drivers/scsi/libsas/sas_discover.c    | 2 +-
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index a4e2e6aa9a6b..1975c9266978 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -1861,10 +1861,6 @@ static void hisi_sas_port_formed(struct asd_sas_phy *sas_phy)
>  	hisi_sas_port_notify_formed(sas_phy);
>  }
>  
> -static void hisi_sas_port_deformed(struct asd_sas_phy *sas_phy)
> -{
> -}
> -
>  static int hisi_sas_write_gpio(struct sas_ha_struct *sha, u8 reg_type,
>  			u8 reg_index, u8 reg_count, u8 *write_data)
>  {
> @@ -1954,10 +1950,9 @@ static struct sas_domain_function_template hisi_sas_transport_ops = {
>  	.lldd_I_T_nexus_reset	= hisi_sas_I_T_nexus_reset,
>  	.lldd_lu_reset		= hisi_sas_lu_reset,
>  	.lldd_query_task	= hisi_sas_query_task,
> -	.lldd_clear_nexus_ha = hisi_sas_clear_nexus_ha,
> +	.lldd_clear_nexus_ha	= hisi_sas_clear_nexus_ha,
>  	.lldd_port_formed	= hisi_sas_port_formed,
> -	.lldd_port_deformed = hisi_sas_port_deformed,
> -	.lldd_write_gpio = hisi_sas_write_gpio,
> +	.lldd_write_gpio	= hisi_sas_write_gpio,
>  };
>  
>  void hisi_sas_init_mem(struct hisi_hba *hisi_hba)
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index 0148ae62a52a..dde433aa59c2 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -260,7 +260,7 @@ static void sas_suspend_devices(struct work_struct *work)
>  	 * phy_list is not being mutated
>  	 */
>  	list_for_each_entry(phy, &port->phy_list, port_phy_el) {
> -		if (si->dft->lldd_port_formed)
> +		if (si->dft->lldd_port_deformed)
>  			si->dft->lldd_port_deformed(phy);
>  		phy->suspended = 1;
>  		port->suspended = 1;
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH 3/5] scsi: libsas: always unregister the old device if going to discover new

[Hannes Reinecke]

On 09/12/2018 10:29 AM, Jason Yan wrote:
> If we went into sas_rediscover_dev() the attached_sas_addr was already
> insured not to be zero. So it's unnecessary to check if the
> attached_sas_addr is zero.
> 
> And although if the sas address is not changed, we always have to
> unregister the old device when we are going to register a new one. We
> cannot just leave the device there and bring up the new.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: chenxiang <chenxiang66@hisilicon.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/libsas/sas_expander.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index fadc99cb60df..52222940d398 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -2054,14 +2054,11 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
>  		return res;
>  	}
>  
> -	/* delete the old link */
> -	if (SAS_ADDR(phy->attached_sas_addr) &&
> -	    SAS_ADDR(sas_addr) != SAS_ADDR(phy->attached_sas_addr)) {
> -		SAS_DPRINTK("ex %016llx phy 0x%x replace %016llx\n",
> -			    SAS_ADDR(dev->sas_addr), phy_id,
> -			    SAS_ADDR(phy->attached_sas_addr));
> -		sas_unregister_devs_sas_addr(dev, phy_id, last);
> -	}
> +	/* we always have to delete the old device when we went here */
> +	SAS_DPRINTK("ex %016llx phy 0x%x replace %016llx\n",
> +		    SAS_ADDR(dev->sas_addr), phy_id,
> +		    SAS_ADDR(phy->attached_sas_addr));
> +	sas_unregister_devs_sas_addr(dev, phy_id, last);
>  
>  	return sas_discover_new(dev, phy_id);
>  }
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH 4/5] scsi: libsas: check the ata device status by ata_dev_enabled()

[Hannes Reinecke]

On 09/12/2018 10:29 AM, Jason Yan wrote:
> When ata device IDENTIFY failed, the ata device status is
> ATA_DEV_UNKNOWN. The libata reported like:
> 
> [113518.620433] ata5.00: qc timeout (cmd 0xec)
> [113518.653646] ata5.00: failed to IDENTIFY (I/O error, err_mask=0x4)
> 
> But libsas verifies the device status by ata_dev_disabled(), which
> skiped ATA_DEV_UNKNOWN. This will make libsas think the ata device
> probing succeed the device cannot be actually brought up. And even the
> new bcast of this device will be considered as flutter and will not
> probe this device again.
> 
> Change ata_dev_disabled() to !ata_dev_enabled() so that libsas can
> deal with this if the ata device probe failed. New bcasts can let us
> try to probe the device again and bring it up if it is fine to
> IDENTIFY.
> 
> Tested-by: Zhou Yupeng <zhouyupeng1@huawei.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_ata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 64a958a99f6a..4f6cdf53e913 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -654,7 +654,7 @@ void sas_probe_sata(struct asd_sas_port *port)
>  		/* if libata could not bring the link up, don't surface
>  		 * the device
>  		 */
> -		if (ata_dev_disabled(sas_to_ata_dev(dev)))
> +		if (!ata_dev_enabled(sas_to_ata_dev(dev)))
>  			sas_fail_probe(dev, __func__, -ENODEV);
>  	}
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH 5/5] scsi: libsas: fix a race condition when smp task timeout

[Hannes Reinecke]

On 09/12/2018 10:29 AM, Jason Yan wrote:
> When the lldd is processing the complete sas task in interrupt and set
> the task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to
> be triggered at the same time. And smp_task_timedout() will complete the
> task wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may
> freed before lldd end the interrupt process. Thus a use-after-free will
> happen.
> 
> Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
> set. And remove the check of the return value of the del_timer().
> 
> Reported-by: chenxiang <chenxiang66@hisilicon.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 52222940d398..0d1f72752ca2 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -48,17 +48,16 @@ static void smp_task_timedout(struct timer_list *t)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&task->task_state_lock, flags);
> -	if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
> +	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>  		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
> +		complete(&task->slow_task->completion);
> +	}
>  	spin_unlock_irqrestore(&task->task_state_lock, flags);
> -
> -	complete(&task->slow_task->completion);
>  }
>  
>  static void smp_task_done(struct sas_task *task)
>  {
> -	if (!del_timer(&task->slow_task->timer))
> -		return;
> +	del_timer(&task->slow_task->timer);
>  	complete(&task->slow_task->completion);
>  }
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH 4/5] scsi: libsas: check the ata device status by ata_dev_enabled()

[John Garry]

+

On 12/09/2018 09:29, Jason Yan wrote:
> When ata device IDENTIFY failed, the ata device status is
> ATA_DEV_UNKNOWN. The libata reported like:
>
> [113518.620433] ata5.00: qc timeout (cmd 0xec)
> [113518.653646] ata5.00: failed to IDENTIFY (I/O error, err_mask=0x4)
>
> But libsas verifies the device status by ata_dev_disabled(), which
> skiped ATA_DEV_UNKNOWN. This will make libsas think the ata device

/s/skiped/skipped/

> probing succeed the device cannot be actually brought up. And even the
> new bcast of this device will be considered as flutter and will not
> probe this device again.
>
> Change ata_dev_disabled() to !ata_dev_enabled() so that libsas can
> deal with this if the ata device probe failed. New bcasts can let us
> try to probe the device again and bring it up if it is fine to
> IDENTIFY.
>
> Tested-by: Zhou Yupeng <zhouyupeng1@huawei.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Reviewed-by: John Garry <john.garry@huawei.com>

> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_ata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 64a958a99f6a..4f6cdf53e913 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -654,7 +654,7 @@ void sas_probe_sata(struct asd_sas_port *port)
>  		/* if libata could not bring the link up, don't surface
>  		 * the device
>  		 */
> -		if (ata_dev_disabled(sas_to_ata_dev(dev)))
> +		if (!ata_dev_enabled(sas_to_ata_dev(dev)))

I do wonder if ata_dev_disabled() needs to be updated to cover 
ATA_DEV_UNKNOWN also or even instead of this change?

>  			sas_fail_probe(dev, __func__, -ENODEV);
>  	}
>
>

Re: [PATCH 5/5] scsi: libsas: fix a race condition when smp task timeout

[Jason Yan]

On 2018/9/17 17:47, John Garry wrote:
> On 12/09/2018 09:29, Jason Yan wrote:
>> When the lldd is processing the complete sas task in interrupt and set
>> the task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to
>> be triggered at the same time. And smp_task_timedout() will complete the
>> task wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may
>> freed before lldd end the interrupt process. Thus a use-after-free will
>> happen.
>>
>> Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
>> set. And remove the check of the return value of the del_timer().
>>
>
> Hi Jason,
>
> Please mention that once the LLDD sets DONE, it must call task->done(),
> which will call smp_task_done()->complete()
>

OK

>> Reported-by: chenxiang <chenxiang66@hisilicon.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> CC: John Garry <john.garry@huawei.com>
>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>> CC: Ewan Milne <emilne@redhat.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> CC: Tomas Henzl <thenzl@redhat.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c
>> b/drivers/scsi/libsas/sas_expander.c
>> index 52222940d398..0d1f72752ca2 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -48,17 +48,16 @@ static void smp_task_timedout(struct timer_list *t)
>>      unsigned long flags;
>>
>>      spin_lock_irqsave(&task->task_state_lock, flags);
>> -    if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
>> +    if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>>          task->task_state_flags |= SAS_TASK_STATE_ABORTED;
>> +        complete(&task->slow_task->completion);
>
> Nit: for consistency with any other time we use this lock, can we call
> complete() outside the lock? Maybe just use a flag variable for this.
>

Is it necessary to add a variable just for consistency with other places?

>> +    }
>>      spin_unlock_irqrestore(&task->task_state_lock, flags);
>> -
>> -    complete(&task->slow_task->completion);
>>  }
>>
>>  static void smp_task_done(struct sas_task *task)
>>  {
>> -    if (!del_timer(&task->slow_task->timer))
>> -        return;
>> +    del_timer(&task->slow_task->timer);
>>      complete(&task->slow_task->completion);
>>  }
>
> Do we also need this change or similar:
> static int smp_execute_task_sg(struct domain_device *dev,
>      if ((task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
>          SAS_DPRINTK("smp task timed out or aborted\n");
>          i->dft->lldd_abort_task(task);
> -        if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
> -            SAS_DPRINTK("SMP task aborted and not done\n");
> -            break;
> -        }
> +        break;
>
> To me, the ABORTED and DONE states are mutually exclusive.
>

This changes the logic a bit. To be safe, maybe we shall do this with 
another patch after some tests.

>>
>>
>
> Thanks,
> John
>
>
>
> .
>

Re: [PATCH 4/5] scsi: libsas: check the ata device status by ata_dev_enabled()

[Jason Yan]

On 2018/9/18 21:54, John Garry wrote:
> +
>
> On 12/09/2018 09:29, Jason Yan wrote:
>> When ata device IDENTIFY failed, the ata device status is
>> ATA_DEV_UNKNOWN. The libata reported like:
>>
>> [113518.620433] ata5.00: qc timeout (cmd 0xec)
>> [113518.653646] ata5.00: failed to IDENTIFY (I/O error, err_mask=0x4)
>>
>> But libsas verifies the device status by ata_dev_disabled(), which
>> skiped ATA_DEV_UNKNOWN. This will make libsas think the ata device
>
> /s/skiped/skipped/
>

OK, thanks.

>> probing succeed the device cannot be actually brought up. And even the
>> new bcast of this device will be considered as flutter and will not
>> probe this device again.
>>
>> Change ata_dev_disabled() to !ata_dev_enabled() so that libsas can
>> deal with this if the ata device probe failed. New bcasts can let us
>> try to probe the device again and bring it up if it is fine to
>> IDENTIFY.
>>
>> Tested-by: Zhou Yupeng <zhouyupeng1@huawei.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>
> Reviewed-by: John Garry <john.garry@huawei.com>
>
>> CC: John Garry <john.garry@huawei.com>
>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>> CC: Ewan Milne <emilne@redhat.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> CC: Tomas Henzl <thenzl@redhat.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/libsas/sas_ata.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_ata.c
>> b/drivers/scsi/libsas/sas_ata.c
>> index 64a958a99f6a..4f6cdf53e913 100644
>> --- a/drivers/scsi/libsas/sas_ata.c
>> +++ b/drivers/scsi/libsas/sas_ata.c
>> @@ -654,7 +654,7 @@ void sas_probe_sata(struct asd_sas_port *port)
>>          /* if libata could not bring the link up, don't surface
>>           * the device
>>           */
>> -        if (ata_dev_disabled(sas_to_ata_dev(dev)))
>> +        if (!ata_dev_enabled(sas_to_ata_dev(dev)))
>
> I do wonder if ata_dev_disabled() needs to be updated to cover
> ATA_DEV_UNKNOWN also or even instead of this change?
>

We cannot do this now because this will make the ata eh process wrong.

>>              sas_fail_probe(dev, __func__, -ENODEV);
>>      }
>>
>>
>
>
>
> .
>

Re: [PATCH 5/5] scsi: libsas: fix a race condition when smp task timeout

[John Garry]

On 19/09/2018 03:49, Jason Yan wrote:
>
>
> On 2018/9/17 17:47, John Garry wrote:
>> On 12/09/2018 09:29, Jason Yan wrote:
>>> When the lldd is processing the complete sas task in interrupt and set
>>> the task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to
>>> be triggered at the same time. And smp_task_timedout() will complete the
>>> task wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may
>>> freed before lldd end the interrupt process. Thus a use-after-free will
>>> happen.
>>>
>>> Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
>>> set. And remove the check of the return value of the del_timer().
>>>
>>
>> Hi Jason,
>>
>> Please mention that once the LLDD sets DONE, it must call task->done(),
>> which will call smp_task_done()->complete()
>>
>
> OK
>
>>> Reported-by: chenxiang <chenxiang66@hisilicon.com>
>>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>>> CC: John Garry <john.garry@huawei.com>
>>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>>> CC: Ewan Milne <emilne@redhat.com>
>>> CC: Christoph Hellwig <hch@lst.de>
>>> CC: Tomas Henzl <thenzl@redhat.com>
>>> CC: Dan Williams <dan.j.williams@intel.com>
>>> CC: Hannes Reinecke <hare@suse.com>
>>> ---
>>>  drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_expander.c
>>> b/drivers/scsi/libsas/sas_expander.c
>>> index 52222940d398..0d1f72752ca2 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -48,17 +48,16 @@ static void smp_task_timedout(struct timer_list *t)
>>>      unsigned long flags;
>>>
>>>      spin_lock_irqsave(&task->task_state_lock, flags);
>>> -    if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
>>> +    if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>>>          task->task_state_flags |= SAS_TASK_STATE_ABORTED;
>>> +        complete(&task->slow_task->completion);
>>
>> Nit: for consistency with any other time we use this lock, can we call
>> complete() outside the lock? Maybe just use a flag variable for this.
>>
>
> Is it necessary to add a variable just for consistency with other places?

Actually other places don't only check/set bits in the flag, so it's ok

>
>>> +    }
>>>      spin_unlock_irqrestore(&task->task_state_lock, flags);
>>> -
>>> -    complete(&task->slow_task->completion);
>>>  }
>>>
>>>  static void smp_task_done(struct sas_task *task)
>>>  {
>>> -    if (!del_timer(&task->slow_task->timer))
>>> -        return;
>>> +    del_timer(&task->slow_task->timer);
>>>      complete(&task->slow_task->completion);
>>>  }
>>
>> Do we also need this change or similar:
>> static int smp_execute_task_sg(struct domain_device *dev,
>>      if ((task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
>>          SAS_DPRINTK("smp task timed out or aborted\n");
>>          i->dft->lldd_abort_task(task);
>> -        if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>> -            SAS_DPRINTK("SMP task aborted and not done\n");
>> -            break;
>> -        }
>> +        break;
>>
>> To me, the ABORTED and DONE states are mutually exclusive.
>>
>
> This changes the logic a bit.

Does it really? According to above change, if state ABORTED set then 
should not have DONE set also.

Anyway, according to my analysis, this suggestion in affect only removes 
the print, which holds true.

Thanks,
John

To be safe, maybe we shall do this with
> another patch after some tests.



>
>>>
>>>
>>
>> Thanks,
>> John
>>
>>
>>
>> .
>>
>
>
> .
>