* [PATCH 0/6] scsi: libsas: Some coding style fixes and cleanups
@ 2022-12-04 8:16 Jason Yan
2022-12-04 8:16 ` [PATCH 1/6] scsi: libsas: move sas_get_ata_command_set() up to save the declaration Jason Yan
` (5 more replies)
0 siblings, 6 replies; 19+ messages in thread
From: Jason Yan @ 2022-12-04 8:16 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, Jason Yan
A few coding style fixes and cleanups. There should be no functional
changes in this series besides the debug log prints.
Jason Yan (6):
scsi: libsas: move sas_get_ata_command_set() up to save the
declaration
scsi: libsas: delete wrapper function sas_discover_end_dev()
scsi: libsas: rename sas_discover_sata() and related refactors
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 | 94 ++++++++++++++++++----
drivers/scsi/libsas/sas_discover.c | 21 +----
drivers/scsi/libsas/sas_expander.c | 125 ++++++++++-------------------
include/scsi/libsas.h | 3 -
include/scsi/sas_ata.h | 15 ++++
5 files changed, 135 insertions(+), 123 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/6] scsi: libsas: move sas_get_ata_command_set() up to save the declaration
2022-12-04 8:16 [PATCH 0/6] scsi: libsas: Some coding style fixes and cleanups Jason Yan
@ 2022-12-04 8:16 ` Jason Yan
2022-12-05 8:45 ` John Garry
2022-12-04 8:16 ` [PATCH 2/6] scsi: libsas: delete wrapper function sas_discover_end_dev() Jason Yan
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jason Yan @ 2022-12-04 8:16 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, 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.
Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
drivers/scsi/libsas/sas_ata.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index f7439bf9cdc6..34009c330eb2 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -239,7 +239,19 @@ 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 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);
+}
int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy)
{
@@ -637,20 +649,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/6] scsi: libsas: delete wrapper function sas_discover_end_dev()
2022-12-04 8:16 [PATCH 0/6] scsi: libsas: Some coding style fixes and cleanups Jason Yan
2022-12-04 8:16 ` [PATCH 1/6] scsi: libsas: move sas_get_ata_command_set() up to save the declaration Jason Yan
@ 2022-12-04 8:16 ` Jason Yan
2022-12-05 8:57 ` John Garry
2022-12-04 8:16 ` [PATCH 3/6] scsi: libsas: rename sas_discover_sata() and related refactors Jason Yan
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jason Yan @ 2022-12-04 8:16 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, Jason Yan
After commit 0558f33c06bb ("scsi: libsas: direct call probe and destruct")
this function is only a wrapper of sas_notify_lldd_dev_found(). And the
function name does not reflect the real purpose of this function now.
Remove it and call sas_notify_lldd_dev_found() directly. The log is also
changed accordingly.
Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
drivers/scsi/libsas/sas_discover.c | 13 +------------
drivers/scsi/libsas/sas_expander.c | 4 ++--
include/scsi/libsas.h | 1 -
3 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index d5bc1314c341..efc6bf95bb67 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -269,17 +269,6 @@ static void sas_resume_devices(struct work_struct *work)
sas_resume_sata(port);
}
-/**
- * sas_discover_end_dev - discover an end device (SSP, etc)
- * @dev: pointer to domain device of interest
- *
- * See comment in sas_discover_sata().
- */
-int sas_discover_end_dev(struct domain_device *dev)
-{
- return sas_notify_lldd_dev_found(dev);
-}
-
/* ---------- Device registration and unregistration ---------- */
void sas_free_device(struct kref *kref)
@@ -447,7 +436,7 @@ static void sas_discover_domain(struct work_struct *work)
switch (dev->dev_type) {
case SAS_END_DEVICE:
- error = sas_discover_end_dev(dev);
+ error = sas_notify_lldd_dev_found(dev);
break;
case SAS_EDGE_EXPANDER_DEVICE:
case SAS_FANOUT_EXPANDER_DEVICE:
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index a04cad620e93..aa8ea3b1f2e4 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -855,9 +855,9 @@ static struct domain_device *sas_ex_discover_end_dev(
list_add_tail(&child->disco_list_node, &parent->port->disco_list);
- res = sas_discover_end_dev(child);
+ res = sas_notify_lldd_dev_found(child);
if (res) {
- pr_notice("sas_discover_end_dev() for device %016llx at %016llx:%02d returned 0x%x\n",
+ 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);
goto out_list_del;
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 1aee3d0ebbb2..87682390fb76 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -736,7 +736,6 @@ 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 *);
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/6] scsi: libsas: rename sas_discover_sata() and related refactors
2022-12-04 8:16 [PATCH 0/6] scsi: libsas: Some coding style fixes and cleanups Jason Yan
2022-12-04 8:16 ` [PATCH 1/6] scsi: libsas: move sas_get_ata_command_set() up to save the declaration Jason Yan
2022-12-04 8:16 ` [PATCH 2/6] scsi: libsas: delete wrapper function sas_discover_end_dev() Jason Yan
@ 2022-12-04 8:16 ` Jason Yan
2022-12-04 8:16 ` [PATCH 4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev() Jason Yan
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Jason Yan @ 2022-12-04 8:16 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, Jason Yan
After commit 0558f33c06bb ("scsi: libsas: direct call probe and destruct")
this name does not reflect the real purpose of this function. So rename
it to sas_ata_notify_lldd_dev_found().
On the other hand, 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_ata.c | 4 ++--
drivers/scsi/libsas/sas_discover.c | 8 +-------
drivers/scsi/libsas/sas_expander.c | 4 ++--
include/scsi/libsas.h | 2 --
include/scsi/sas_ata.h | 6 ++++++
5 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 34009c330eb2..01b6175a8960 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -738,14 +738,14 @@ void sas_resume_sata(struct asd_sas_port *port)
}
/**
- * sas_discover_sata - discover an STP/SATA domain device
+ * sas_ata_notify_lldd_dev_found - notify an STP/SATA domain device found
* @dev: pointer to struct domain_device of interest
*
* Devices directly attached to a HA port, have no parents. All other
* devices do, and should have their "parent" pointer set appropriately
* before calling this function.
*/
-int sas_discover_sata(struct domain_device *dev)
+int sas_ata_notify_lldd_dev_found(struct domain_device *dev)
{
if (dev->dev_type == SAS_SATA_PM)
return -ENODEV;
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index efc6bf95bb67..e7e3e230e8d1 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -444,14 +444,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);
+ error = sas_ata_notify_lldd_dev_found(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/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index aa8ea3b1f2e4..a8af723fab3c 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -830,9 +830,9 @@ static struct domain_device *sas_ex_discover_end_dev(
list_add_tail(&child->disco_list_node, &parent->port->disco_list);
- res = sas_discover_sata(child);
+ res = sas_ata_notify_lldd_dev_found(child);
if (res) {
- pr_notice("sas_discover_sata() for device %16llx at %016llx:%02d returned 0x%x\n",
+ pr_notice("notify lldd 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;
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 87682390fb76..b8b05f653a96 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -735,8 +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 *);
-
void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *);
void sas_init_dev(struct domain_device *);
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index 9c927d46f136..a3857595ba55 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_ata_notify_lldd_dev_found(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_ata_notify_lldd_dev_found(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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev()
2022-12-04 8:16 [PATCH 0/6] scsi: libsas: Some coding style fixes and cleanups Jason Yan
` (2 preceding siblings ...)
2022-12-04 8:16 ` [PATCH 3/6] scsi: libsas: rename sas_discover_sata() and related refactors Jason Yan
@ 2022-12-04 8:16 ` Jason Yan
2022-12-05 9:14 ` John Garry
2022-12-04 8:16 ` [PATCH 5/6] scsi: libsas: factor out sas_ata_add_dev() Jason Yan
2022-12-04 8:16 ` [PATCH 6/6] scsi: libsas: factor out sas_ex_add_dev() Jason Yan
5 siblings, 1 reply; 19+ messages in thread
From: Jason Yan @ 2022-12-04 8:16 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, Jason Yan
The domain device 'child' is allocated in sas_ex_discover_end_dev() and
never been added to dev_list. So remove the useless list_del() and
related locks.
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 a8af723fab3c..82ea7560a888 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/6] scsi: libsas: factor out sas_ata_add_dev()
2022-12-04 8:16 [PATCH 0/6] scsi: libsas: Some coding style fixes and cleanups Jason Yan
` (3 preceding siblings ...)
2022-12-04 8:16 ` [PATCH 4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev() Jason Yan
@ 2022-12-04 8:16 ` Jason Yan
2022-12-05 9:24 ` John Garry
2022-12-04 8:16 ` [PATCH 6/6] scsi: libsas: factor out sas_ex_add_dev() Jason Yan
5 siblings, 1 reply; 19+ messages in thread
From: Jason Yan @ 2022-12-04 8:16 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, 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 01b6175a8960..f440203eb280 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -677,6 +677,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_ata_notify_lldd_dev_found(child);
+ if (ret) {
+ pr_notice("notify lldd 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 82ea7560a888..747f4fc795f4 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_ata_notify_lldd_dev_found(child);
- if (res) {
- pr_notice("notify lldd 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 a3857595ba55..23a0853a5a0a 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_ata_notify_lldd_dev_found(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_ata_notify_lldd_dev_found(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 is not enabled, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 6/6] scsi: libsas: factor out sas_ex_add_dev()
2022-12-04 8:16 [PATCH 0/6] scsi: libsas: Some coding style fixes and cleanups Jason Yan
` (4 preceding siblings ...)
2022-12-04 8:16 ` [PATCH 5/6] scsi: libsas: factor out sas_ata_add_dev() Jason Yan
@ 2022-12-04 8:16 ` Jason Yan
2022-12-05 9:31 ` John Garry
5 siblings, 1 reply; 19+ messages in thread
From: Jason Yan @ 2022-12-04 8:16 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, 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 747f4fc795f4..3c72b167d43a 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 (unlikely(!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_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);
- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/6] scsi: libsas: move sas_get_ata_command_set() up to save the declaration
2022-12-04 8:16 ` [PATCH 1/6] scsi: libsas: move sas_get_ata_command_set() up to save the declaration Jason Yan
@ 2022-12-05 8:45 ` John Garry
2022-12-08 6:36 ` Jason Yan
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2022-12-05 8:45 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 04/12/2022 08:16, 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.
>
> Cc: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
Apart from comments, below:
Reviewed-by: John Garry <john.g.garry@oracle.com>
> ---
> drivers/scsi/libsas/sas_ata.c | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index f7439bf9cdc6..34009c330eb2 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -239,7 +239,19 @@ 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 dev_to_host_fis *fis =
> + (struct dev_to_host_fis *) dev->frame_rcvd;
nit: I did not think that we add a whitespace before casting. And I
think that it would be neater if fis was assigned separately, avoiding
overflowing the line in this way.
Having said that, it does seem odd to cast from u8 [] to struct
dev_to_host_fis * and then back to (u8 *) in the ata_tf_from_fis() call,
below.
> + 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);
> +}
>
> int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy)
> {
> @@ -637,20 +649,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;
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/6] scsi: libsas: delete wrapper function sas_discover_end_dev()
2022-12-04 8:16 ` [PATCH 2/6] scsi: libsas: delete wrapper function sas_discover_end_dev() Jason Yan
@ 2022-12-05 8:57 ` John Garry
2022-12-08 6:56 ` Jason Yan
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2022-12-05 8:57 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 04/12/2022 08:16, Jason Yan wrote:
> After commit 0558f33c06bb ("scsi: libsas: direct call probe and destruct")
> this function is only a wrapper of sas_notify_lldd_dev_found(). And the
> function name does not reflect the real purpose of this function now.
Why is this? Maybe add "dev_found" to the name could help.
> Remove it and call sas_notify_lldd_dev_found() directly. The log is also
> changed accordingly.
>
> Cc: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
> drivers/scsi/libsas/sas_discover.c | 13 +------------
> drivers/scsi/libsas/sas_expander.c | 4 ++--
> include/scsi/libsas.h | 1 -
> 3 files changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index d5bc1314c341..efc6bf95bb67 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -269,17 +269,6 @@ static void sas_resume_devices(struct work_struct *work)
> sas_resume_sata(port);
> }
>
> -/**
> - * sas_discover_end_dev - discover an end device (SSP, etc)
> - * @dev: pointer to domain device of interest
> - *
> - * See comment in sas_discover_sata().
> - */
> -int sas_discover_end_dev(struct domain_device *dev)
> -{
> - return sas_notify_lldd_dev_found(dev);
> -}
> -
> /* ---------- Device registration and unregistration ---------- */
>
> void sas_free_device(struct kref *kref)
> @@ -447,7 +436,7 @@ static void sas_discover_domain(struct work_struct *work)
>
> switch (dev->dev_type) {
> case SAS_END_DEVICE:
> - error = sas_discover_end_dev(dev);
> + error = sas_notify_lldd_dev_found(dev);
For me, personally, I prefer consistent API name, like
sas_discover_end_dev() and sas_discover_sata(), even if
sas_discover_end_dev() is just a wrapper.
> break;
> case SAS_EDGE_EXPANDER_DEVICE:
> case SAS_FANOUT_EXPANDER_DEVICE:
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index a04cad620e93..aa8ea3b1f2e4 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -855,9 +855,9 @@ static struct domain_device *sas_ex_discover_end_dev(
>
> list_add_tail(&child->disco_list_node, &parent->port->disco_list);
>
> - res = sas_discover_end_dev(child);
> + res = sas_notify_lldd_dev_found(child);
> if (res) {
> - pr_notice("sas_discover_end_dev() for device %016llx at %016llx:%02d returned 0x%x\n",
> + 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);
> goto out_list_del;
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 1aee3d0ebbb2..87682390fb76 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -736,7 +736,6 @@ 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 *);
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev()
2022-12-04 8:16 ` [PATCH 4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev() Jason Yan
@ 2022-12-05 9:14 ` John Garry
2022-12-08 7:21 ` Jason Yan
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2022-12-05 9:14 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 04/12/2022 08:16, Jason Yan wrote:
> The domain device 'child' is allocated in sas_ex_discover_end_dev() and
> never been added to dev_list. So remove the useless list_del() and
> related locks.
>
> 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 a8af723fab3c..82ea7560a888 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);
Since we have the spin lock'ing, this seems to be have been
intentionally added (and not some simple typo or similar) - any idea of
the origin?
Thanks,
John
> out_free:
> sas_port_delete(phy->port);
> out_err:
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/6] scsi: libsas: factor out sas_ata_add_dev()
2022-12-04 8:16 ` [PATCH 5/6] scsi: libsas: factor out sas_ata_add_dev() Jason Yan
@ 2022-12-05 9:24 ` John Garry
2022-12-08 7:26 ` Jason Yan
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2022-12-05 9:24 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 04/12/2022 08:16, Jason Yan wrote:
> +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 is not enabled, target proto 0x%x at %016llx:0x%x\n",
nit: how about add "SCSI_SAS_ATA is not enabled ..." or use similar to
the log in sas_discover_domain()?
> + phy->attached_tproto, SAS_ADDR(parent->sas_addr), phy_id);
> + return -ENODEV;
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/6] scsi: libsas: factor out sas_ex_add_dev()
2022-12-04 8:16 ` [PATCH 6/6] scsi: libsas: factor out sas_ex_add_dev() Jason Yan
@ 2022-12-05 9:31 ` John Garry
2022-12-08 8:07 ` Jason Yan
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2022-12-05 9:31 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 04/12/2022 08:16, 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>
> ---
> 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 747f4fc795f4..3c72b167d43a 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 (unlikely(!rphy))
nit: this is not fastpath so unlikely can be avoided
> + 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);
nit: these lines could be aligned with (, as it was before
> + 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 */
so has the error handling been fixed now?
> - 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_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);
> - 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:
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/6] scsi: libsas: move sas_get_ata_command_set() up to save the declaration
2022-12-05 8:45 ` John Garry
@ 2022-12-08 6:36 ` Jason Yan
0 siblings, 0 replies; 19+ messages in thread
From: Jason Yan @ 2022-12-08 6:36 UTC (permalink / raw)
To: John Garry, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 2022/12/5 16:45, John Garry wrote:
> On 04/12/2022 08:16, 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.
>>
>> Cc: John Garry <john.g.garry@oracle.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>
> Apart from comments, below:
> Reviewed-by: John Garry <john.g.garry@oracle.com>
>
Thank you John.
>> ---
>> drivers/scsi/libsas/sas_ata.c | 28 +++++++++++++---------------
>> 1 file changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_ata.c
>> b/drivers/scsi/libsas/sas_ata.c
>> index f7439bf9cdc6..34009c330eb2 100644
>> --- a/drivers/scsi/libsas/sas_ata.c
>> +++ b/drivers/scsi/libsas/sas_ata.c
>> @@ -239,7 +239,19 @@ 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 dev_to_host_fis *fis =
>> + (struct dev_to_host_fis *) dev->frame_rcvd;
>
> nit: I did not think that we add a whitespace before casting. And I
> think that it would be neater if fis was assigned separately, avoiding
> overflowing the line in this way.
>
> Having said that, it does seem odd to cast from u8 [] to struct
> dev_to_host_fis * and then back to (u8 *) in the ata_tf_from_fis() call,
> below.
Yeah, odd. I'd prefer remove 'fis' and then do:
ata_tf_from_fis(dev->frame_rcvd, &tf).
Thanks,
Jason
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/6] scsi: libsas: delete wrapper function sas_discover_end_dev()
2022-12-05 8:57 ` John Garry
@ 2022-12-08 6:56 ` Jason Yan
0 siblings, 0 replies; 19+ messages in thread
From: Jason Yan @ 2022-12-08 6:56 UTC (permalink / raw)
To: John Garry, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 2022/12/5 16:57, John Garry wrote:
> On 04/12/2022 08:16, Jason Yan wrote:
>> After commit 0558f33c06bb ("scsi: libsas: direct call probe and
>> destruct")
>> this function is only a wrapper of sas_notify_lldd_dev_found(). And the
>> function name does not reflect the real purpose of this function now.
>
> Why is this? Maybe add "dev_found" to the name could help.
>
>> Remove it and call sas_notify_lldd_dev_found() directly. The log is also
>> changed accordingly.
>>
>> Cc: John Garry <john.g.garry@oracle.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> ---
>> drivers/scsi/libsas/sas_discover.c | 13 +------------
>> drivers/scsi/libsas/sas_expander.c | 4 ++--
>> include/scsi/libsas.h | 1 -
>> 3 files changed, 3 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_discover.c
>> b/drivers/scsi/libsas/sas_discover.c
>> index d5bc1314c341..efc6bf95bb67 100644
>> --- a/drivers/scsi/libsas/sas_discover.c
>> +++ b/drivers/scsi/libsas/sas_discover.c
>> @@ -269,17 +269,6 @@ static void sas_resume_devices(struct work_struct
>> *work)
>> sas_resume_sata(port);
>> }
>> -/**
>> - * sas_discover_end_dev - discover an end device (SSP, etc)
>> - * @dev: pointer to domain device of interest
>> - *
>> - * See comment in sas_discover_sata().
>> - */
>> -int sas_discover_end_dev(struct domain_device *dev)
>> -{
>> - return sas_notify_lldd_dev_found(dev);
>> -}
>> -
>> /* ---------- Device registration and unregistration ---------- */
>> void sas_free_device(struct kref *kref)
>> @@ -447,7 +436,7 @@ static void sas_discover_domain(struct work_struct
>> *work)
>> switch (dev->dev_type) {
>> case SAS_END_DEVICE:
>> - error = sas_discover_end_dev(dev);
>> + error = sas_notify_lldd_dev_found(dev);
>
> For me, personally, I prefer consistent API name, like
> sas_discover_end_dev() and sas_discover_sata(), even if
> sas_discover_end_dev() is just a wrapper.
Fair enough. I was just thinking that this API name is not proper now
because it is only notifying the lldd.
I will drop this patch if you insist.
Thanks,
Jason
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev()
2022-12-05 9:14 ` John Garry
@ 2022-12-08 7:21 ` Jason Yan
2022-12-08 10:40 ` John Garry
0 siblings, 1 reply; 19+ messages in thread
From: Jason Yan @ 2022-12-08 7:21 UTC (permalink / raw)
To: John Garry, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 2022/12/5 17:14, John Garry wrote:
> On 04/12/2022 08:16, Jason Yan wrote:
>> The domain device 'child' is allocated in sas_ex_discover_end_dev() and
>> never been added to dev_list. So remove the useless list_del() and
>> related locks.
>>
>> 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 a8af723fab3c..82ea7560a888 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);
>
> Since we have the spin lock'ing, this seems to be have been
> intentionally added (and not some simple typo or similar) - any idea of
> the origin?
The new device used to be added to the dev_list in this function. But
after 92625f9bff38 ("[SCSI] libsas: restore scan order") and
87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery competing with
ata error handling") it is added to the disco_list instead. But the
list_del() and locking is forgot to be removed.
Thanks,
Jason
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/6] scsi: libsas: factor out sas_ata_add_dev()
2022-12-05 9:24 ` John Garry
@ 2022-12-08 7:26 ` Jason Yan
0 siblings, 0 replies; 19+ messages in thread
From: Jason Yan @ 2022-12-08 7:26 UTC (permalink / raw)
To: John Garry, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 2022/12/5 17:24, John Garry wrote:
> On 04/12/2022 08:16, Jason Yan wrote:
>> +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 is not enabled, target proto 0x%x at %016llx:0x%x\n",
>
> nit: how about add "SCSI_SAS_ATA is not enabled ..." or use similar to
> the log in sas_discover_domain()?
Yes, make sense. Will update.
Thanks,
Jason
>
>> + phy->attached_tproto, SAS_ADDR(parent->sas_addr), phy_id);
>> + return -ENODEV;
>
> .
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/6] scsi: libsas: factor out sas_ex_add_dev()
2022-12-05 9:31 ` John Garry
@ 2022-12-08 8:07 ` Jason Yan
0 siblings, 0 replies; 19+ messages in thread
From: Jason Yan @ 2022-12-08 8:07 UTC (permalink / raw)
To: John Garry, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 2022/12/5 17:31, John Garry wrote:
> On 04/12/2022 08:16, 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>
>> ---
>> 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 747f4fc795f4..3c72b167d43a 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 (unlikely(!rphy))
>
> nit: this is not fastpath so unlikely can be avoided
>
ok. It is only a hint for the compiler so not a big deal. I can delete it.
>> + 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);
>
> nit: these lines could be aligned with (, as it was before
Nice catch. Will fix.
>
>> + 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 */
>
> so has the error handling been fixed now?
IIUC, the error handling is ok so this comment can be deleted.
Thanks,
Jason
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev()
2022-12-08 7:21 ` Jason Yan
@ 2022-12-08 10:40 ` John Garry
2022-12-08 11:11 ` Jason Yan
0 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2022-12-08 10:40 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 08/12/2022 07:21, Jason Yan wrote:
>>> t_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);
>>
>> Since we have the spin lock'ing, this seems to be have been
>> intentionally added (and not some simple typo or similar) - any idea
>> of the origin?
>
> The new device used to be added to the dev_list in this function. But
> after 92625f9bff38 ("[SCSI] libsas: restore scan order") and
> 87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery competing with
> ata error handling") it is added to the disco_list instead. But the
> list_del() and locking is forgot to be removed.
OK, so can we have a fixes tag then? That even helps review, as I can
then quickly see where we went wrong.
Thanks,
John
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev()
2022-12-08 10:40 ` John Garry
@ 2022-12-08 11:11 ` Jason Yan
0 siblings, 0 replies; 19+ messages in thread
From: Jason Yan @ 2022-12-08 11:11 UTC (permalink / raw)
To: John Garry, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal
On 2022/12/8 18:40, John Garry wrote:
> On 08/12/2022 07:21, Jason Yan wrote:
>>>> t_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);
>>>
>>> Since we have the spin lock'ing, this seems to be have been
>>> intentionally added (and not some simple typo or similar) - any idea
>>> of the origin?
>>
>> The new device used to be added to the dev_list in this function. But
>> after 92625f9bff38 ("[SCSI] libsas: restore scan order") and
>> 87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery competing
>> with ata error handling") it is added to the disco_list instead. But
>> the list_del() and locking is forgot to be removed.
>
> OK, so can we have a fixes tag then? That even helps review, as I can
> then quickly see where we went wrong.
>
Sure.
Thanks,
Jason
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-12-08 11:12 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-04 8:16 [PATCH 0/6] scsi: libsas: Some coding style fixes and cleanups Jason Yan
2022-12-04 8:16 ` [PATCH 1/6] scsi: libsas: move sas_get_ata_command_set() up to save the declaration Jason Yan
2022-12-05 8:45 ` John Garry
2022-12-08 6:36 ` Jason Yan
2022-12-04 8:16 ` [PATCH 2/6] scsi: libsas: delete wrapper function sas_discover_end_dev() Jason Yan
2022-12-05 8:57 ` John Garry
2022-12-08 6:56 ` Jason Yan
2022-12-04 8:16 ` [PATCH 3/6] scsi: libsas: rename sas_discover_sata() and related refactors Jason Yan
2022-12-04 8:16 ` [PATCH 4/6] scsi: libsas: remove useless dev_list delete in sas_ex_discover_end_dev() Jason Yan
2022-12-05 9:14 ` John Garry
2022-12-08 7:21 ` Jason Yan
2022-12-08 10:40 ` John Garry
2022-12-08 11:11 ` Jason Yan
2022-12-04 8:16 ` [PATCH 5/6] scsi: libsas: factor out sas_ata_add_dev() Jason Yan
2022-12-05 9:24 ` John Garry
2022-12-08 7:26 ` Jason Yan
2022-12-04 8:16 ` [PATCH 6/6] scsi: libsas: factor out sas_ex_add_dev() Jason Yan
2022-12-05 9:31 ` John Garry
2022-12-08 8:07 ` Jason Yan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.