[PATCH v2 0/5] scsi: libsas: Some coding style fixes and cleanups

[PATCH v2 0/5] scsi: libsas: Some coding style fixes and cleanups

[Jason Yan]

A few coding style fixes and cleanups. There should be no functional
changes in this series besides the debug log prints.

v1->v2:
  1. Drop patch #2 in v1.
  2. Other misc changes suggested by John.

Jason Yan (5):
  scsi: libsas: move sas_get_ata_command_set() up to save the
    declaration
  scsi: libsas: change the coding style of sas_discover_sata()
  scsi: libsas: remove useless dev_list delete in
    sas_ex_discover_end_dev()
  scsi: libsas: factor out sas_ata_add_dev()
  scsi: libsas: factor out sas_ex_add_dev()

 drivers/scsi/libsas/sas_ata.c      |  88 ++++++++++++++++----
 drivers/scsi/libsas/sas_discover.c |   6 --
 drivers/scsi/libsas/sas_expander.c | 125 ++++++++++-------------------
 include/scsi/libsas.h              |   1 -
 include/scsi/sas_ata.h             |  15 ++++
 5 files changed, 129 insertions(+), 106 deletions(-)

-- 
2.31.1

[PATCH v2 1/5] scsi: libsas: move sas_get_ata_command_set() up to save the declaration

[Jason Yan]

There is a sas_get_ata_command_set() declaration above sas_get_ata_info()
to make it compile ok. However this function is defined in the same file
below. So move it up to save the declaration.

Also remove the variable 'fis' which is not needed in this function.

Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index f7439bf9cdc6..de3439ae358d 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -239,7 +239,17 @@ static struct sas_internal *dev_to_sas_internal(struct domain_device *dev)
 	return to_sas_internal(dev->port->ha->core.shost->transportt);
 }
 
-static int sas_get_ata_command_set(struct domain_device *dev);
+static int sas_get_ata_command_set(struct domain_device *dev)
+{
+	struct ata_taskfile tf;
+
+	if (dev->dev_type == SAS_SATA_PENDING)
+		return ATA_DEV_UNKNOWN;
+
+	ata_tf_from_fis(dev->frame_rcvd, &tf);
+
+	return ata_dev_classify(&tf);
+}
 
 int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy)
 {
@@ -637,20 +647,6 @@ void sas_ata_task_abort(struct sas_task *task)
 	complete(waiting);
 }
 
-static int sas_get_ata_command_set(struct domain_device *dev)
-{
-	struct dev_to_host_fis *fis =
-		(struct dev_to_host_fis *) dev->frame_rcvd;
-	struct ata_taskfile tf;
-
-	if (dev->dev_type == SAS_SATA_PENDING)
-		return ATA_DEV_UNKNOWN;
-
-	ata_tf_from_fis((const u8 *)fis, &tf);
-
-	return ata_dev_classify(&tf);
-}
-
 void sas_probe_sata(struct asd_sas_port *port)
 {
 	struct domain_device *dev, *n;
-- 
2.31.1

Re: [PATCH v2 1/5] scsi: libsas: move sas_get_ata_command_set() up to save the declaration

[John Garry]

On 13/12/2022 15:09, Jason Yan wrote:
> There is a sas_get_ata_command_set() declaration above sas_get_ata_info()
> to make it compile ok. However this function is defined in the same file
> below. So move it up to save the declaration.

nit: forward declaration

> 
> Also remove the variable 'fis' which is not needed in this function.
> 
> Cc: John Garry<john.g.garry@oracle.com>
> Signed-off-by: Jason Yan<yanaijie@huawei.com>

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

[PATCH v2 2/5] scsi: libsas: change the coding style of sas_discover_sata()

[Jason Yan]

The coding style where calling this interface is inconsistent with other
interfaces for sata devices. The standard style for other sata interfaces
is like:

    #ifdefine CONFIG_SCSI_SAS_ATA
    void sas_ata_task_abort(struct sas_task *task);
    #else
    static inline void sas_ata_task_abort(struct sas_task *task)
    {
    }
    #endif

And the callers does not have to do things like "#ifdefine CONFIG_SCSI_SAS_ATA"
and may call the interface directly. So follow the standard style here.

Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_discover.c | 6 ------
 include/scsi/libsas.h              | 1 -
 include/scsi/sas_ata.h             | 6 ++++++
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index d5bc1314c341..72fdb2e5d047 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -455,14 +455,8 @@ static void sas_discover_domain(struct work_struct *work)
 		break;
 	case SAS_SATA_DEV:
 	case SAS_SATA_PM:
-#ifdef CONFIG_SCSI_SAS_ATA
 		error = sas_discover_sata(dev);
 		break;
-#else
-		pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot attach\n");
-		fallthrough;
-#endif
-		/* Fall through - only for the #else condition above. */
 	default:
 		error = -ENXIO;
 		pr_err("unhandled device %d\n", dev->dev_type);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 1aee3d0ebbb2..159823e0afbf 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -735,7 +735,6 @@ void sas_unregister_domain_devices(struct asd_sas_port *port, int gone);
 void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
 void sas_discover_event(struct asd_sas_port *, enum discover_event ev);
 
-int  sas_discover_sata(struct domain_device *);
 int  sas_discover_end_dev(struct domain_device *);
 
 void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *);
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 9c927d46f136..2fd15f194316 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -36,6 +36,7 @@ void sas_ata_device_link_abort(struct domain_device *dev, bool force_reset);
 int sas_execute_ata_cmd(struct domain_device *device, u8 *fis,
 			int force_phy_id);
 int smp_ata_check_ready_type(struct ata_link *link);
+int sas_discover_sata(struct domain_device *dev);
 #else
 
 
@@ -103,6 +104,11 @@ static inline int smp_ata_check_ready_type(struct ata_link *link)
 {
 	return 0;
 }
+static inline int sas_discover_sata(struct domain_device *dev)
+{
+	pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot attach\n");
+	return -ENXIO;
+}
 #endif
 
 #endif /* _SAS_ATA_H_ */
-- 
2.31.1

Re: [PATCH v2 2/5] scsi: libsas: change the coding style of sas_discover_sata()

[John Garry]

On 13/12/2022 15:09, Jason Yan wrote:
> The coding style where calling this interface is inconsistent with other
> interfaces for sata devices. The standard style for other sata interfaces

nit: capitalize acronyms, so /s/sata/SATA/

> is like:
> 
>      #ifdefine CONFIG_SCSI_SAS_ATA
>      void sas_ata_task_abort(struct sas_task *task);
>      #else
>      static inline void sas_ata_task_abort(struct sas_task *task)
>      {
>      }
>      #endif
> 
> And the callers does not have to do things like "#ifdefine CONFIG_SCSI_SAS_ATA"
> and may call the interface directly. So follow the standard style here.
> 
> Cc: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

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

> ---
>   drivers/scsi/libsas/sas_discover.c | 6 ------
>   include/scsi/libsas.h              | 1 -
>   include/scsi/sas_ata.h             | 6 ++++++
>   3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index d5bc1314c341..72fdb2e5d047 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -455,14 +455,8 @@ static void sas_discover_domain(struct work_struct *work)
>   		break;
>   	case SAS_SATA_DEV:
>   	case SAS_SATA_PM:
> -#ifdef CONFIG_SCSI_SAS_ATA
>   		error = sas_discover_sata(dev);
>   		break;
> -#else
> -		pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot attach\n");
> -		fallthrough;
> -#endif
> -		/* Fall through - only for the #else condition above. */
>   	default:
>   		error = -ENXIO;
>   		pr_err("unhandled device %d\n", dev->dev_type);
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 1aee3d0ebbb2..159823e0afbf 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -735,7 +735,6 @@ void sas_unregister_domain_devices(struct asd_sas_port *port, int gone);
>   void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
>   void sas_discover_event(struct asd_sas_port *, enum discover_event ev);
>   
> -int  sas_discover_sata(struct domain_device *);
>   int  sas_discover_end_dev(struct domain_device *);
>   
>   void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *);
> diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
> index 9c927d46f136..2fd15f194316 100644
> --- a/include/scsi/sas_ata.h
> +++ b/include/scsi/sas_ata.h
> @@ -36,6 +36,7 @@ void sas_ata_device_link_abort(struct domain_device *dev, bool force_reset);
>   int sas_execute_ata_cmd(struct domain_device *device, u8 *fis,
>   			int force_phy_id);
>   int smp_ata_check_ready_type(struct ata_link *link);
> +int sas_discover_sata(struct domain_device *dev);
>   #else
>   
>   
> @@ -103,6 +104,11 @@ static inline int smp_ata_check_ready_type(struct ata_link *link)
>   {
>   	return 0;
>   }

nit: is there a blank line missing?

> +static inline int sas_discover_sata(struct domain_device *dev)
> +{
> +	pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot attach\n");
> +	return -ENXIO;
> +}
>   #endif
>   
>   #endif /* _SAS_ATA_H_ */

[PATCH v2 3/5] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev()

[Jason Yan]

The domain device 'child' is allocated in sas_ex_discover_end_dev() and
never been added to dev_list. It used to be added to the dev_list in this
function. But after the following two fixes it is added to the disco_list
instead. So the list_del() and locking left is useless now.

Fixes: 87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery competing with ata error handling")
Fixes: 92625f9bff38 ("[SCSI] libsas: restore scan order")
Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index a04cad620e93..29e1b93b0964 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -875,9 +875,6 @@ static struct domain_device *sas_ex_discover_end_dev(
  out_list_del:
 	sas_rphy_free(child->rphy);
 	list_del(&child->disco_list_node);
-	spin_lock_irq(&parent->port->dev_list_lock);
-	list_del(&child->dev_list_node);
-	spin_unlock_irq(&parent->port->dev_list_lock);
  out_free:
 	sas_port_delete(phy->port);
  out_err:
-- 
2.31.1

Re: [PATCH v2 3/5] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev()

[John Garry]

On 13/12/2022 15:09, Jason Yan wrote:
> The domain device 'child' is allocated in sas_ex_discover_end_dev() and
> never been added to dev_list. It used to be added to the dev_list in this
> function. But after the following two fixes it is added to the disco_list
> instead. So the list_del() and locking left is useless now.
> 
> Fixes: 87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery competing with ata error handling")
> Fixes: 92625f9bff38 ("[SCSI] libsas: restore scan order")
> Cc: John Garry<john.g.garry@oracle.com>
> Signed-off-by: Jason Yan<yanaijie@huawei.com>

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

[PATCH v2 4/5] scsi: libsas: factor out sas_ata_add_dev()

[Jason Yan]

Factor out sas_ata_add_dev() and put it in sas_ata.c since it is a sata
related interface. Also follow the standard coding style to define an
inline empty function when CONFIG_SCSI_SAS_ATA is not enabled.

Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c      | 62 ++++++++++++++++++++++++++++++
 drivers/scsi/libsas/sas_expander.c | 54 +-------------------------
 include/scsi/sas_ata.h             |  9 +++++
 3 files changed, 73 insertions(+), 52 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index de3439ae358d..13fbb8629057 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -675,6 +675,68 @@ void sas_probe_sata(struct asd_sas_port *port)
 
 }
 
+int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *phy,
+		    struct domain_device *child, int phy_id)
+{
+	struct sas_rphy *rphy;
+	int ret;
+
+	if (child->linkrate > parent->min_linkrate) {
+		struct sas_phy *cphy = child->phy;
+		enum sas_linkrate min_prate = cphy->minimum_linkrate,
+			parent_min_lrate = parent->min_linkrate,
+			min_linkrate = (min_prate > parent_min_lrate) ?
+					parent_min_lrate : 0;
+		struct sas_phy_linkrates rates = {
+			.maximum_linkrate = parent->min_linkrate,
+			.minimum_linkrate = min_linkrate,
+		};
+
+		pr_notice("ex %016llx phy%02d SATA device linkrate > min pathway connection rate, attempting to lower device linkrate\n",
+			  SAS_ADDR(child->sas_addr), phy_id);
+		ret = sas_smp_phy_control(parent, phy_id,
+					  PHY_FUNC_LINK_RESET, &rates);
+		if (ret) {
+			pr_err("ex %016llx phy%02d SATA device could not set linkrate (%d)\n",
+			       SAS_ADDR(child->sas_addr), phy_id, ret);
+			return ret;
+		}
+		pr_notice("ex %016llx phy%02d SATA device set linkrate successfully\n",
+			  SAS_ADDR(child->sas_addr), phy_id);
+		child->linkrate = child->min_linkrate;
+	}
+	ret = sas_get_ata_info(child, phy);
+	if (ret)
+		return ret;
+
+	sas_init_dev(child);
+	ret = sas_ata_init(child);
+	if (ret)
+		return ret;
+
+	rphy = sas_end_device_alloc(phy->port);
+	if (!rphy)
+		return ret;
+
+	rphy->identify.phy_identifier = phy_id;
+	child->rphy = rphy;
+	get_device(&rphy->dev);
+
+	list_add_tail(&child->disco_list_node, &parent->port->disco_list);
+
+	ret = sas_discover_sata(child);
+	if (ret) {
+		pr_notice("sas_discover_sata() for device %16llx at %016llx:%02d returned 0x%x\n",
+			  SAS_ADDR(child->sas_addr),
+			  SAS_ADDR(parent->sas_addr), phy_id, ret);
+		sas_rphy_free(child->rphy);
+		list_del(&child->disco_list_node);
+		return ret;
+	}
+
+	return 0;
+}
+
 static void sas_ata_flush_pm_eh(struct asd_sas_port *port, const char *func)
 {
 	struct domain_device *dev, *n;
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 29e1b93b0964..0e4e09a0286a 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -785,61 +785,11 @@ static struct domain_device *sas_ex_discover_end_dev(
 	sas_ex_get_linkrate(parent, child, phy);
 	sas_device_set_phy(child, phy->port);
 
-#ifdef CONFIG_SCSI_SAS_ATA
 	if ((phy->attached_tproto & SAS_PROTOCOL_STP) || phy->attached_sata_dev) {
-		if (child->linkrate > parent->min_linkrate) {
-			struct sas_phy *cphy = child->phy;
-			enum sas_linkrate min_prate = cphy->minimum_linkrate,
-				parent_min_lrate = parent->min_linkrate,
-				min_linkrate = (min_prate > parent_min_lrate) ?
-					       parent_min_lrate : 0;
-			struct sas_phy_linkrates rates = {
-				.maximum_linkrate = parent->min_linkrate,
-				.minimum_linkrate = min_linkrate,
-			};
-			int ret;
-
-			pr_notice("ex %016llx phy%02d SATA device linkrate > min pathway connection rate, attempting to lower device linkrate\n",
-				   SAS_ADDR(child->sas_addr), phy_id);
-			ret = sas_smp_phy_control(parent, phy_id,
-						  PHY_FUNC_LINK_RESET, &rates);
-			if (ret) {
-				pr_err("ex %016llx phy%02d SATA device could not set linkrate (%d)\n",
-				       SAS_ADDR(child->sas_addr), phy_id, ret);
-				goto out_free;
-			}
-			pr_notice("ex %016llx phy%02d SATA device set linkrate successfully\n",
-				  SAS_ADDR(child->sas_addr), phy_id);
-			child->linkrate = child->min_linkrate;
-		}
-		res = sas_get_ata_info(child, phy);
-		if (res)
-			goto out_free;
-
-		sas_init_dev(child);
-		res = sas_ata_init(child);
+		res = sas_ata_add_dev(parent, phy, child, phy_id);
 		if (res)
 			goto out_free;
-		rphy = sas_end_device_alloc(phy->port);
-		if (!rphy)
-			goto out_free;
-		rphy->identify.phy_identifier = phy_id;
-
-		child->rphy = rphy;
-		get_device(&rphy->dev);
-
-		list_add_tail(&child->disco_list_node, &parent->port->disco_list);
-
-		res = sas_discover_sata(child);
-		if (res) {
-			pr_notice("sas_discover_sata() for device %16llx at %016llx:%02d returned 0x%x\n",
-				  SAS_ADDR(child->sas_addr),
-				  SAS_ADDR(parent->sas_addr), phy_id, res);
-			goto out_list_del;
-		}
-	} else
-#endif
-	  if (phy->attached_tproto & SAS_PROTOCOL_SSP) {
+	} else if (phy->attached_tproto & SAS_PROTOCOL_SSP) {
 		child->dev_type = SAS_END_DEVICE;
 		rphy = sas_end_device_alloc(phy->port);
 		/* FIXME: error handling */
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 2fd15f194316..dbc0f4830d05 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -37,6 +37,8 @@ int sas_execute_ata_cmd(struct domain_device *device, u8 *fis,
 			int force_phy_id);
 int smp_ata_check_ready_type(struct ata_link *link);
 int sas_discover_sata(struct domain_device *dev);
+int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *phy,
+		    struct domain_device *child, int phy_id);
 #else
 
 
@@ -109,6 +111,13 @@ static inline int sas_discover_sata(struct domain_device *dev)
 	pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot attach\n");
 	return -ENXIO;
 }
+static inline int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *phy,
+				  struct domain_device *child, int phy_id)
+{
+	pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot add device, target proto 0x%x at %016llx:0x%x\n",
+		  phy->attached_tproto, SAS_ADDR(parent->sas_addr), phy_id);
+	return -ENODEV;
+}
 #endif
 
 #endif /* _SAS_ATA_H_ */
-- 
2.31.1

Re: [PATCH v2 4/5] scsi: libsas: factor out sas_ata_add_dev()

[John Garry]

On 13/12/2022 15:09, Jason Yan wrote:
> Factor out sas_ata_add_dev() and put it in sas_ata.c since it is a sata
> related interface. Also follow the standard coding style to define an
> inline empty function when CONFIG_SCSI_SAS_ATA is not enabled.
> 
> Cc: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Apart from comment, below:

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

> ---
>   drivers/scsi/libsas/sas_ata.c      | 62 ++++++++++++++++++++++++++++++
>   drivers/scsi/libsas/sas_expander.c | 54 +-------------------------
>   include/scsi/sas_ata.h             |  9 +++++
>   3 files changed, 73 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c

>   
>   
> @@ -109,6 +111,13 @@ static inline int sas_discover_sata(struct domain_device *dev)
>   	pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot attach\n");
>   	return -ENXIO;
>   }
> +static inline int sas_ata_add_dev(struct domain_device *parent, struct ex_phy *phy,
> +				  struct domain_device *child, int phy_id)
> +{
> +	pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot add device, target proto 0x%x at %016llx:0x%x\n",
> +		  phy->attached_tproto, SAS_ADDR(parent->sas_addr), phy_id);

Do you really think that we need to add all this info, like 
parent->sas_addr?

Indeed, I think that we could make all these prints for 
CONFIG_SCSI_SAS_ATA=N into a single global pr_notice_once(). That's just 
my thoughts.

> +	return -ENODEV;
> +}
>   #endif
>   
>   #endif /* _SAS_ATA_H_ */

Re: [PATCH v2 4/5] scsi: libsas: factor out sas_ata_add_dev()

[Jason Yan]

On 2022/12/14 0:22, John Garry wrote:
> On 13/12/2022 15:09, Jason Yan wrote:
>> Factor out sas_ata_add_dev() and put it in sas_ata.c since it is a sata
>> related interface. Also follow the standard coding style to define an
>> inline empty function when CONFIG_SCSI_SAS_ATA is not enabled.
>>
>> Cc: John Garry <john.g.garry@oracle.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> 
> Apart from comment, below:
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 
>> ---
>>   drivers/scsi/libsas/sas_ata.c      | 62 ++++++++++++++++++++++++++++++
>>   drivers/scsi/libsas/sas_expander.c | 54 +-------------------------
>>   include/scsi/sas_ata.h             |  9 +++++
>>   3 files changed, 73 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_ata.c 
>> b/drivers/scsi/libsas/sas_ata.c
> 
>> @@ -109,6 +111,13 @@ static inline int sas_discover_sata(struct 
>> domain_device *dev)
>>       pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot 
>> attach\n");
>>       return -ENXIO;
>>   }
>> +static inline int sas_ata_add_dev(struct domain_device *parent, 
>> struct ex_phy *phy,
>> +                  struct domain_device *child, int phy_id)
>> +{
>> +    pr_notice("ATA device seen but CONFIG_SCSI_SAS_ATA=N so cannot 
>> add device, target proto 0x%x at %016llx:0x%x\n",
>> +          phy->attached_tproto, SAS_ADDR(parent->sas_addr), phy_id);
> 
> Do you really think that we need to add all this info, like 
> parent->sas_addr?
> 

I did not want to change the functionality at the first time so I kept 
these info printing.

> Indeed, I think that we could make all these prints for 
> CONFIG_SCSI_SAS_ATA=N into a single global pr_notice_once(). That's just 
> my thoughts.
> 

Yeah, makes sense. Let me have a try.

Thanks

>> +    return -ENODEV;
>> +}
>>   #endif
>>   #endif /* _SAS_ATA_H_ */
> 
> .

[PATCH v2 5/5] scsi: libsas: factor out sas_ex_add_dev()

[Jason Yan]

Factor out sas_ex_add_dev() to be consistent with sas_ata_add_dev() and
unify the error handling.

Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 68 +++++++++++++++++-------------
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 0e4e09a0286a..dc670304f181 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -751,13 +751,46 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
 	child->pathways = min(child->pathways, parent->pathways);
 }
 
+static int sas_ex_add_dev(struct domain_device *parent, struct ex_phy *phy,
+			  struct domain_device *child, int phy_id)
+{
+	struct sas_rphy *rphy;
+	int res;
+
+	child->dev_type = SAS_END_DEVICE;
+	rphy = sas_end_device_alloc(phy->port);
+	if (!rphy)
+		return -ENOMEM;
+
+	child->tproto = phy->attached_tproto;
+	sas_init_dev(child);
+
+	child->rphy = rphy;
+	get_device(&rphy->dev);
+	rphy->identify.phy_identifier = phy_id;
+	sas_fill_in_rphy(child, rphy);
+
+	list_add_tail(&child->disco_list_node, &parent->port->disco_list);
+
+	res = sas_notify_lldd_dev_found(child);
+	if (res) {
+		pr_notice("notify lldd for device %016llx at %016llx:%02d returned 0x%x\n",
+			  SAS_ADDR(child->sas_addr),
+			  SAS_ADDR(parent->sas_addr), phy_id, res);
+		sas_rphy_free(child->rphy);
+		list_del(&child->disco_list_node);
+		return res;
+	}
+
+	return 0;
+}
+
 static struct domain_device *sas_ex_discover_end_dev(
 	struct domain_device *parent, int phy_id)
 {
 	struct expander_device *parent_ex = &parent->ex_dev;
 	struct ex_phy *phy = &parent_ex->ex_phy[phy_id];
 	struct domain_device *child = NULL;
-	struct sas_rphy *rphy;
 	int res;
 
 	if (phy->attached_sata_host || phy->attached_sata_ps)
@@ -787,44 +820,21 @@ static struct domain_device *sas_ex_discover_end_dev(
 
 	if ((phy->attached_tproto & SAS_PROTOCOL_STP) || phy->attached_sata_dev) {
 		res = sas_ata_add_dev(parent, phy, child, phy_id);
-		if (res)
-			goto out_free;
 	} else if (phy->attached_tproto & SAS_PROTOCOL_SSP) {
-		child->dev_type = SAS_END_DEVICE;
-		rphy = sas_end_device_alloc(phy->port);
-		/* FIXME: error handling */
-		if (unlikely(!rphy))
-			goto out_free;
-		child->tproto = phy->attached_tproto;
-		sas_init_dev(child);
-
-		child->rphy = rphy;
-		get_device(&rphy->dev);
-		rphy->identify.phy_identifier = phy_id;
-		sas_fill_in_rphy(child, rphy);
-
-		list_add_tail(&child->disco_list_node, &parent->port->disco_list);
-
-		res = sas_discover_end_dev(child);
-		if (res) {
-			pr_notice("sas_discover_end_dev() for device %016llx at %016llx:%02d returned 0x%x\n",
-				  SAS_ADDR(child->sas_addr),
-				  SAS_ADDR(parent->sas_addr), phy_id, res);
-			goto out_list_del;
-		}
+		res = sas_ex_add_dev(parent, phy, child, phy_id);
 	} else {
 		pr_notice("target proto 0x%x at %016llx:0x%x not handled\n",
 			  phy->attached_tproto, SAS_ADDR(parent->sas_addr),
 			  phy_id);
-		goto out_free;
+		res = -ENODEV;
 	}
 
+	if (res)
+		goto out_free;
+
 	list_add_tail(&child->siblings, &parent_ex->children);
 	return child;
 
- out_list_del:
-	sas_rphy_free(child->rphy);
-	list_del(&child->disco_list_node);
  out_free:
 	sas_port_delete(phy->port);
  out_err:
-- 
2.31.1

Re: [PATCH v2 5/5] scsi: libsas: factor out sas_ex_add_dev()

[John Garry]

On 13/12/2022 15:09, Jason Yan wrote:
> Factor out sas_ex_add_dev() to be consistent with sas_ata_add_dev() and
> unify the error handling.
> 
> Cc: John Garry<john.g.garry@oracle.com>
> Signed-off-by: Jason Yan<yanaijie@huawei.com>

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

Re: [PATCH v2 0/5] scsi: libsas: Some coding style fixes and cleanups

[John Garry]

On 13/12/2022 15:09, Jason Yan wrote:
> A few coding style fixes and cleanups. There should be no functional
> changes in this series besides the debug log prints.
> 
> v1->v2:
>    1. Drop patch #2 in v1.
>    2. Other misc changes suggested by John.

Note: it would be better to concisely mention the actual changes, so 
that we know what to look for in the new version. Just writing something 
like "incorporate changes suggested by <insert name>" is unfortunately 
not much use.

Thanks,
John

> 
> Jason Yan (5):
>    scsi: libsas: move sas_get_ata_command_set() up to save the
>      declaration
>    scsi: libsas: change the coding style of sas_discover_sata()
>    scsi: libsas: remove useless dev_list delete in
>      sas_ex_discover_end_dev()
>    scsi: libsas: factor out sas_ata_add_dev()
>    scsi: libsas: factor out sas_ex_add_dev()
> 
>   drivers/scsi/libsas/sas_ata.c      |  88 ++++++++++++++++----
>   drivers/scsi/libsas/sas_discover.c |   6 --
>   drivers/scsi/libsas/sas_expander.c | 125 ++++++++++-------------------
>   include/scsi/libsas.h              |   1 -
>   include/scsi/sas_ata.h             |  15 ++++
>   5 files changed, 129 insertions(+), 106 deletions(-)
> 

Re: [PATCH v2 0/5] scsi: libsas: Some coding style fixes and cleanups

[Jason Yan]

On 2022/12/14 0:12, John Garry wrote:
> On 13/12/2022 15:09, Jason Yan wrote:
>> A few coding style fixes and cleanups. There should be no functional
>> changes in this series besides the debug log prints.
>>
>> v1->v2:
>>    1. Drop patch #2 in v1.
>>    2. Other misc changes suggested by John.
> 
> Note: it would be better to concisely mention the actual changes, so 
> that we know what to look for in the new version. Just writing something 
> like "incorporate changes suggested by <insert name>" is unfortunately 
> not much use.
> 

Yeah, my bad.

Thank you for your patience, John.