[PATCH 0/6] libsas: Some minor improvements and tidy-up

[PATCH 0/6] libsas: Some minor improvements and tidy-up

[John Garry]

This patchset introduces some minor improvements and tidy-up, including:
- fix PHY info in sysfs for PHY events
- min pathway rate programming improvement
- some other tidy-up and neatening

John Garry (6):
  scsi: libsas: Stop hardcoding SAS address length
  scsi: libsas: Try to retain programmed min linkrate for SATA min
    pathway unmatch fixing
  scsi: libsas: Improve vague log in SAS rediscovery
  scsi: libsas: Inject revalidate event for root port event
  scsi: libsas: Do discovery on empty PHY to update PHY info
  scsi: libsas: Print expander PHY indexes in decimal

 drivers/scsi/libsas/sas_ata.c      |  2 +-
 drivers/scsi/libsas/sas_expander.c | 83 ++++++++++++++++++------------
 drivers/scsi/libsas/sas_init.c     | 42 ++++++++-------
 drivers/scsi/libsas/sas_phy.c      |  7 ++-
 drivers/scsi/libsas/sas_port.c     | 24 +++++++--
 include/scsi/libsas.h              | 13 +++--
 6 files changed, 108 insertions(+), 63 deletions(-)

-- 
2.17.1

[PATCH 1/6] scsi: libsas: Stop hardcoding SAS address length

[John Garry]

Many times we use 8 for SAS address length, while we already have a macro
for this - SAS_ADDR_SIZE.

Replace instances of this with the macro. However, don't touch the
SAS address array sizes sas.h, as these are defined according to the
SAS spec.

Some missing whitespaces are also added, and whitespace indentation
in sas_hash_addr() is also fixed (see sas_hash_addr()).

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 15 +++++------
 drivers/scsi/libsas/sas_init.c     | 40 ++++++++++++++++--------------
 include/scsi/libsas.h              |  6 ++---
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 17b45a0c7bc3..93f297199d4c 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1151,7 +1151,7 @@ static int sas_find_sub_addr(struct domain_device *dev, u8 *sub_addr)
 		     phy->attached_dev_type == SAS_FANOUT_EXPANDER_DEVICE) &&
 		    phy->routing_attr == SUBTRACTIVE_ROUTING) {
 
-			memcpy(sub_addr, phy->attached_sas_addr,SAS_ADDR_SIZE);
+			memcpy(sub_addr, phy->attached_sas_addr, SAS_ADDR_SIZE);
 
 			return 1;
 		}
@@ -1163,7 +1163,7 @@ static int sas_check_level_subtractive_boundary(struct domain_device *dev)
 {
 	struct expander_device *ex = &dev->ex_dev;
 	struct domain_device *child;
-	u8 sub_addr[8] = {0, };
+	u8 sub_addr[SAS_ADDR_SIZE] = {0, };
 
 	list_for_each_entry(child, &ex->children, siblings) {
 		if (child->dev_type != SAS_EDGE_EXPANDER_DEVICE &&
@@ -1173,7 +1173,7 @@ static int sas_check_level_subtractive_boundary(struct domain_device *dev)
 			sas_find_sub_addr(child, sub_addr);
 			continue;
 		} else {
-			u8 s2[8];
+			u8 s2[SAS_ADDR_SIZE];
 
 			if (sas_find_sub_addr(child, s2) &&
 			    (SAS_ADDR(sub_addr) != SAS_ADDR(s2))) {
@@ -1760,10 +1760,11 @@ static int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
 
 	res = sas_get_phy_discover(dev, phy_id, disc_resp);
 	if (res == 0) {
-		memcpy(sas_addr, disc_resp->disc.attached_sas_addr, 8);
+		memcpy(sas_addr, disc_resp->disc.attached_sas_addr,
+		       SAS_ADDR_SIZE);
 		*type = to_dev_type(dr);
 		if (*type == 0)
-			memset(sas_addr, 0, 8);
+			memset(sas_addr, 0, SAS_ADDR_SIZE);
 	}
 	kfree(disc_resp);
 	return res;
@@ -2027,10 +2028,10 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
 	struct expander_device *ex = &dev->ex_dev;
 	struct ex_phy *phy = &ex->ex_phy[phy_id];
 	enum sas_device_type type = SAS_PHY_UNUSED;
-	u8 sas_addr[8];
+	u8 sas_addr[SAS_ADDR_SIZE];
 	int res;
 
-	memset(sas_addr, 0, 8);
+	memset(sas_addr, 0, SAS_ADDR_SIZE);
 	res = sas_get_phy_attached_dev(dev, phy_id, sas_addr, &type);
 	switch (res) {
 	case SMP_RESP_NO_PHY:
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 221340ee8651..213c85557bf9 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -87,25 +87,27 @@ EXPORT_SYMBOL_GPL(sas_free_task);
 /*------------ SAS addr hash -----------*/
 void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
 {
-        const u32 poly = 0x00DB2777;
-        u32     r = 0;
-        int     i;
-
-        for (i = 0; i < 8; i++) {
-                int b;
-                for (b = 7; b >= 0; b--) {
-                        r <<= 1;
-                        if ((1 << b) & sas_addr[i]) {
-                                if (!(r & 0x01000000))
-                                        r ^= poly;
-                        } else if (r & 0x01000000)
-                                r ^= poly;
-                }
-        }
-
-        hashed[0] = (r >> 16) & 0xFF;
-        hashed[1] = (r >> 8) & 0xFF ;
-        hashed[2] = r & 0xFF;
+	const u32 poly = 0x00DB2777;
+	u32 r = 0;
+	int i;
+
+	for (i = 0; i < SAS_ADDR_SIZE; i++) {
+		int b;
+
+		for (b = (SAS_ADDR_SIZE - 1); b >= 0; b--) {
+			r <<= 1;
+			if ((1 << b) & sas_addr[i]) {
+				if (!(r & 0x01000000))
+					r ^= poly;
+			} else if (r & 0x01000000) {
+				r ^= poly;
+			}
+		}
+	}
+
+	hashed[0] = (r >> 16) & 0xFF;
+	hashed[1] = (r >> 8) & 0xFF;
+	hashed[2] = r & 0xFF;
 }
 
 int sas_register_ha(struct sas_ha_struct *sas_ha)
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 56b2dba7d911..cfaaf1254211 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -245,9 +245,9 @@ static inline struct sas_discovery_event *to_sas_discovery_event(struct work_str
 struct sas_discovery {
 	struct sas_discovery_event disc_work[DISC_NUM_EVENTS];
 	unsigned long    pending;
-	u8     fanout_sas_addr[8];
-	u8     eeds_a[8];
-	u8     eeds_b[8];
+	u8     fanout_sas_addr[SAS_ADDR_SIZE];
+	u8     eeds_a[SAS_ADDR_SIZE];
+	u8     eeds_b[SAS_ADDR_SIZE];
 	int    max_level;
 };
 
-- 
2.17.1

[PATCH 2/6] scsi: libsas: Try to retain programmed min linkrate for SATA min pathway unmatch fixing

[John Garry]

Currently for fixing the linkrate matching during discovery such that
the linkrate of a SATA PHY does not exceed min pathway to initiator,
we set the SATA PHY programmed min linkrate to the same value as the
programmed max linkrate.

This is unnecessary, and we should be able to keep the same programmed
min linkrate if it is already lower than this new max programmed linkrate.

This patch makes that change.

In effect, this will not make much difference since we generally will
negotiate a linkrate at the programmed max linkrate, and the programmed
min linkrate will have no impact.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 93f297199d4c..dfdf3c94d326 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -826,9 +826,14 @@ static struct domain_device *sas_ex_discover_end_dev(
 #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 = parent->min_linkrate,
+				.minimum_linkrate = min_linkrate,
 			};
 			int ret;
 
-- 
2.17.1

[PATCH 3/6] scsi: libsas: Improve vague log in SAS rediscovery

[John Garry]

When an expander PHY which was part of a wideport disconnects, we would
see a log like this from sas_rediscover():
[   39.695554] sas: phy20 part of wide port with phy16

Here, phy20 is the PHY that disconnected, and phy16 is the lowest indexed
member PHY of the wideport.

The log implies the phy20 is still part of the wideport with phy16, so
is misleading or, at least, vague.

Improve the logs in SAS rediscovery by removing this log and adding
a log in sas_rediscover_dev() to tell what's really going on.

While we're at it, also make the logs in sas_find_bcast_dev() more
informative (and more consistent with the reset of the expander logs).

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index dfdf3c94d326..6f569a65d791 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1876,10 +1876,12 @@ static int sas_find_bcast_dev(struct domain_device *dev,
 		if (phy_id != -1) {
 			*src_dev = dev;
 			ex->ex_change_count = ex_change_count;
-			pr_info("Expander phy change count has changed\n");
+			pr_info("ex %016llx phy%d change count has changed\n",
+				SAS_ADDR(dev->sas_addr), phy_id);
 			return res;
 		} else
-			pr_info("Expander phys DID NOT change\n");
+			pr_info("ex %016llx phys DID NOT change\n",
+				SAS_ADDR(dev->sas_addr));
 	}
 	list_for_each_entry(ch, &ex->children, siblings) {
 		if (ch->dev_type == SAS_EDGE_EXPANDER_DEVICE || ch->dev_type == SAS_FANOUT_EXPANDER_DEVICE) {
@@ -2028,14 +2030,22 @@ static bool dev_type_flutter(enum sas_device_type new, enum sas_device_type old)
 	return false;
 }
 
-static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
+static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
+			      bool last, int sibling)
 {
 	struct expander_device *ex = &dev->ex_dev;
 	struct ex_phy *phy = &ex->ex_phy[phy_id];
 	enum sas_device_type type = SAS_PHY_UNUSED;
 	u8 sas_addr[SAS_ADDR_SIZE];
+	char msg[80] = "";
 	int res;
 
+	if (!last)
+		sprintf(msg, ", part of a wide port with phy%d", sibling);
+
+	pr_debug("ex %016llx rediscovering phy%d%s\n", SAS_ADDR(dev->sas_addr),
+		 phy_id, msg);
+
 	memset(sas_addr, 0, SAS_ADDR_SIZE);
 	res = sas_get_phy_attached_dev(dev, phy_id, sas_addr, &type);
 	switch (res) {
@@ -2115,13 +2125,11 @@ static int sas_rediscover(struct domain_device *dev, const int phy_id)
 				continue;
 			if (SAS_ADDR(phy->attached_sas_addr) ==
 			    SAS_ADDR(changed_phy->attached_sas_addr)) {
-				pr_debug("phy%d part of wide port with phy%d\n",
-					 phy_id, i);
 				last = false;
 				break;
 			}
 		}
-		res = sas_rediscover_dev(dev, phy_id, last);
+		res = sas_rediscover_dev(dev, phy_id, last, i);
 	} else
 		res = sas_discover_new(dev, phy_id);
 	return res;
-- 
2.17.1

[PATCH 4/6] scsi: libsas: Inject revalidate event for root port event

[John Garry]

According to the SAS spec, an expander device shall transmit BROADCAST
(CHANGE) from at least one phy in each expander port other than the
expander port that is the cause for transmitting BROADCAST (CHANGE).

As such, for when the link is lost for a root PHY attached to an
expander PHY, we get no broadcast event.

This causes an issue for libsas, in that we will not revalidate the
domain for these events.

As a solution, for when a root PHY is formed or deformed from a root
port, insert a broadcast event to trigger a domain revalidation.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_port.c | 24 +++++++++++++++++++++---
 include/scsi/libsas.h          |  7 +++++++
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index 03fe479359b6..38a10478605c 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -95,6 +95,7 @@ static void sas_form_port(struct asd_sas_phy *phy)
 	int i;
 	struct sas_ha_struct *sas_ha = phy->ha;
 	struct asd_sas_port *port = phy->port;
+	struct domain_device *port_dev;
 	struct sas_internal *si =
 		to_sas_internal(sas_ha->core.shost->transportt);
 	unsigned long flags;
@@ -153,8 +154,9 @@ static void sas_form_port(struct asd_sas_phy *phy)
 	}
 
 	/* add the phy to the port */
+	port_dev = port->port_dev;
 	list_add_tail(&phy->port_phy_el, &port->phy_list);
-	sas_phy_set_target(phy, port->port_dev);
+	sas_phy_set_target(phy, port_dev);
 	phy->port = port;
 	port->num_phys++;
 	port->phy_mask |= (1U << phy->id);
@@ -184,14 +186,21 @@ static void sas_form_port(struct asd_sas_phy *phy)
 		 port->phy_mask,
 		 SAS_ADDR(port->attached_sas_addr));
 
-	if (port->port_dev)
-		port->port_dev->pathways = port->num_phys;
+	if (port_dev)
+		port_dev->pathways = port->num_phys;
 
 	/* Tell the LLDD about this port formation. */
 	if (si->dft->lldd_port_formed)
 		si->dft->lldd_port_formed(phy);
 
 	sas_discover_event(phy->port, DISCE_DISCOVER_DOMAIN);
+	/* Only insert a revalidate event after initial discovery */
+	if (port_dev && sas_dev_type_is_expander(port_dev->dev_type)) {
+		struct expander_device *ex_dev = &port_dev->ex_dev;
+
+		ex_dev->ex_change_count = -1;
+		sas_discover_event(port, DISCE_REVALIDATE_DOMAIN);
+	}
 	flush_workqueue(sas_ha->disco_q);
 }
 
@@ -254,6 +263,15 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone)
 	spin_unlock(&port->phy_list_lock);
 	spin_unlock_irqrestore(&sas_ha->phy_port_lock, flags);
 
+	/* Only insert revalidate event if the port still has members */
+	if (port->port && dev && sas_dev_type_is_expander(dev->dev_type)) {
+		struct expander_device *ex_dev = &dev->ex_dev;
+
+		ex_dev->ex_change_count = -1;
+		sas_discover_event(port, DISCE_REVALIDATE_DOMAIN);
+	}
+	flush_workqueue(sas_ha->disco_q);
+
 	return;
 }
 
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index cfaaf1254211..b08febec7895 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -224,6 +224,13 @@ struct sas_work {
 	struct work_struct work;
 };
 
+/* Lots of code duplicates this in the SCSI tree, which can be factored out */
+static inline bool sas_dev_type_is_expander(enum sas_device_type type)
+{
+	return type == SAS_EDGE_EXPANDER_DEVICE ||
+	       type == SAS_FANOUT_EXPANDER_DEVICE;
+}
+
 static inline void INIT_SAS_WORK(struct sas_work *sw, void (*fn)(struct work_struct *))
 {
 	INIT_WORK(&sw->work, fn);
-- 
2.17.1

[PATCH 5/6] scsi: libsas: Do discovery on empty PHY to update PHY info

[John Garry]

When we discover the PHY is empty in sas_rediscover_dev(), the PHY
information (like negotiated linkrate) is not updated.

As such, for a user examining sysfs for that PHY, they would see
incorrect values:
root@(none)$ cd /sys/class/sas_phy/phy-0:0:20
root@(none)$ more negotiated_linkrate
3.0 Gbit
root@(none)$ echo 0 > enable
root@(none)$ more negotiated_linkrate
3.0 Gbit

So fix this, simply discover the PHY again, even though we know it's
empty; in the above example, this gives us:
root@(none)$ more negotiated_linkrate
Phy disabled

We must do this after unregistering the device associated with the PHY
(in sas_unregister_devs_sas_addr()).

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 6f569a65d791..ad96bc843acc 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -2068,6 +2068,11 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
 	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
 		phy->phy_state = PHY_EMPTY;
 		sas_unregister_devs_sas_addr(dev, phy_id, last);
+		/*
+		 * Even though the PHY is empty, for convenience we discover
+		 * the PHY to update the PHY info, like negotiated linkrate.
+		 */
+		sas_ex_phy_discover(dev, phy_id);
 		return res;
 	} else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
 		   dev_type_flutter(type, phy->attached_dev_type)) {
-- 
2.17.1

[PATCH 6/6] scsi: libsas: Print expander PHY indexes in decimal

[John Garry]

Currently we print expander PHY indexes in a mix of decimal and hex.

It is more consistent and also more convenient to read decimal, so
make this change.

We use width of 2 for expander and 1 for root PHYs prints.

Some lines which were needlessly spilling multiple lines are unified.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c      |  2 +-
 drivers/scsi/libsas/sas_expander.c | 44 +++++++++++++++---------------
 drivers/scsi/libsas/sas_init.c     |  2 +-
 drivers/scsi/libsas/sas_phy.c      |  7 ++---
 4 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 6f93fee2b21b..1ecca71df8b5 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -281,7 +281,7 @@ int sas_get_ata_info(struct domain_device *dev, struct ex_phy *phy)
 		res = sas_get_report_phy_sata(dev->parent, phy->phy_id,
 					      &dev->sata_dev.rps_resp);
 		if (res) {
-			pr_debug("report phy sata to %016llx:0x%x returned 0x%x\n",
+			pr_debug("report phy sata to %016llx:%02d returned 0x%x\n",
 				 SAS_ADDR(dev->parent->sas_addr),
 				 phy->phy_id, res);
 			return res;
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index ad96bc843acc..83f2fd70ce76 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -870,7 +870,7 @@ static struct domain_device *sas_ex_discover_end_dev(
 
 		res = sas_discover_sata(child);
 		if (res) {
-			pr_notice("sas_discover_sata() for device %16llx at %016llx:0x%x returned 0x%x\n",
+			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;
@@ -895,7 +895,7 @@ static struct domain_device *sas_ex_discover_end_dev(
 
 		res = sas_discover_end_dev(child);
 		if (res) {
-			pr_notice("sas_discover_end_dev() for device %16llx at %016llx:0x%x returned 0x%x\n",
+			pr_notice("sas_discover_end_dev() 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;
@@ -960,7 +960,7 @@ static struct domain_device *sas_ex_discover_expander(
 	int res;
 
 	if (phy->routing_attr == DIRECT_ROUTING) {
-		pr_warn("ex %016llx:0x%x:D <--> ex %016llx:0x%x is not allowed\n",
+		pr_warn("ex %016llx:%02d:D <--> ex %016llx:0x%x is not allowed\n",
 			SAS_ADDR(parent->sas_addr), phy_id,
 			SAS_ADDR(phy->attached_sas_addr),
 			phy->attached_phy_id);
@@ -1070,7 +1070,7 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
 	    ex_phy->attached_dev_type != SAS_FANOUT_EXPANDER_DEVICE &&
 	    ex_phy->attached_dev_type != SAS_EDGE_EXPANDER_DEVICE &&
 	    ex_phy->attached_dev_type != SAS_SATA_PENDING) {
-		pr_warn("unknown device type(0x%x) attached to ex %016llx phy 0x%x\n",
+		pr_warn("unknown device type(0x%x) attached to ex %016llx phy%02d\n",
 			ex_phy->attached_dev_type,
 			SAS_ADDR(dev->sas_addr),
 			phy_id);
@@ -1086,7 +1086,7 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
 	}
 
 	if (sas_ex_join_wide_port(dev, phy_id)) {
-		pr_debug("Attaching ex phy%d to wide port %016llx\n",
+		pr_debug("Attaching ex phy%02d to wide port %016llx\n",
 			 phy_id, SAS_ADDR(ex_phy->attached_sas_addr));
 		return res;
 	}
@@ -1098,7 +1098,7 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
 		break;
 	case SAS_FANOUT_EXPANDER_DEVICE:
 		if (SAS_ADDR(dev->port->disc.fanout_sas_addr)) {
-			pr_debug("second fanout expander %016llx phy 0x%x attached to ex %016llx phy 0x%x\n",
+			pr_debug("second fanout expander %016llx phy%02d attached to ex %016llx phy%02d\n",
 				 SAS_ADDR(ex_phy->attached_sas_addr),
 				 ex_phy->attached_phy_id,
 				 SAS_ADDR(dev->sas_addr),
@@ -1131,7 +1131,7 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
 			    SAS_ADDR(child->sas_addr)) {
 				ex->ex_phy[i].phy_state= PHY_DEVICE_DISCOVERED;
 				if (sas_ex_join_wide_port(dev, i))
-					pr_debug("Attaching ex phy%d to wide port %016llx\n",
+					pr_debug("Attaching ex phy%02d to wide port %016llx\n",
 						 i, SAS_ADDR(ex->ex_phy[i].attached_sas_addr));
 			}
 		}
@@ -1266,7 +1266,7 @@ static int sas_check_ex_subtractive_boundary(struct domain_device *dev)
 			else if (SAS_ADDR(sub_sas_addr) !=
 				 SAS_ADDR(phy->attached_sas_addr)) {
 
-				pr_notice("ex %016llx phy 0x%x diverges(%016llx) on subtractive boundary(%016llx). Disabled\n",
+				pr_notice("ex %016llx phy%02d diverges(%016llx) on subtractive boundary(%016llx). Disabled\n",
 					  SAS_ADDR(dev->sas_addr), i,
 					  SAS_ADDR(phy->attached_sas_addr),
 					  SAS_ADDR(sub_sas_addr));
@@ -1287,7 +1287,7 @@ static void sas_print_parent_topology_bug(struct domain_device *child,
 	};
 	struct domain_device *parent = child->parent;
 
-	pr_notice("%s ex %016llx phy 0x%x <--> %s ex %016llx phy 0x%x has %c:%c routing link!\n",
+	pr_notice("%s ex %016llx phy%02d <--> %s ex %016llx phy%02d has %c:%c routing link!\n",
 		  ex_type[parent->dev_type],
 		  SAS_ADDR(parent->sas_addr),
 		  parent_phy->phy_id,
@@ -1309,7 +1309,7 @@ static int sas_check_eeds(struct domain_device *child,
 
 	if (SAS_ADDR(parent->port->disc.fanout_sas_addr) != 0) {
 		res = -ENODEV;
-		pr_warn("edge ex %016llx phy S:0x%x <--> edge ex %016llx phy S:0x%x, while there is a fanout ex %016llx\n",
+		pr_warn("edge ex %016llx phy S:%02d <--> edge ex %016llx phy S:%02d, while there is a fanout ex %016llx\n",
 			SAS_ADDR(parent->sas_addr),
 			parent_phy->phy_id,
 			SAS_ADDR(child->sas_addr),
@@ -1332,7 +1332,7 @@ static int sas_check_eeds(struct domain_device *child,
 		;
 	else {
 		res = -ENODEV;
-		pr_warn("edge ex %016llx phy 0x%x <--> edge ex %016llx phy 0x%x link forms a third EEDS!\n",
+		pr_warn("edge ex %016llx phy%02d <--> edge ex %016llx phy%02d link forms a third EEDS!\n",
 			SAS_ADDR(parent->sas_addr),
 			parent_phy->phy_id,
 			SAS_ADDR(child->sas_addr),
@@ -1450,11 +1450,11 @@ static int sas_configure_present(struct domain_device *dev, int phy_id,
 			goto out;
 		res = rri_resp[2];
 		if (res == SMP_RESP_NO_INDEX) {
-			pr_warn("overflow of indexes: dev %016llx phy 0x%x index 0x%x\n",
+			pr_warn("overflow of indexes: dev %016llx phy%02d index 0x%x\n",
 				SAS_ADDR(dev->sas_addr), phy_id, i);
 			goto out;
 		} else if (res != SMP_RESP_FUNC_ACC) {
-			pr_notice("%s: dev %016llx phy 0x%x index 0x%x result 0x%x\n",
+			pr_notice("%s: dev %016llx phy%02d index 0x%x result 0x%x\n",
 				  __func__, SAS_ADDR(dev->sas_addr), phy_id,
 				  i, res);
 			goto out;
@@ -1520,7 +1520,7 @@ static int sas_configure_set(struct domain_device *dev, int phy_id,
 		goto out;
 	res = cri_resp[2];
 	if (res == SMP_RESP_NO_INDEX) {
-		pr_warn("overflow of indexes: dev %016llx phy 0x%x index 0x%x\n",
+		pr_warn("overflow of indexes: dev %016llx phy%02d index 0x%x\n",
 			SAS_ADDR(dev->sas_addr), phy_id, index);
 	}
 out:
@@ -1876,7 +1876,7 @@ static int sas_find_bcast_dev(struct domain_device *dev,
 		if (phy_id != -1) {
 			*src_dev = dev;
 			ex->ex_change_count = ex_change_count;
-			pr_info("ex %016llx phy%d change count has changed\n",
+			pr_info("ex %016llx phy%02d change count has changed\n",
 				SAS_ADDR(dev->sas_addr), phy_id);
 			return res;
 		} else
@@ -1991,7 +1991,7 @@ static int sas_discover_new(struct domain_device *dev, int phy_id)
 	struct domain_device *child;
 	int res;
 
-	pr_debug("ex %016llx phy%d new device attached\n",
+	pr_debug("ex %016llx phy%02d new device attached\n",
 		 SAS_ADDR(dev->sas_addr), phy_id);
 	res = sas_ex_phy_discover(dev, phy_id);
 	if (res)
@@ -2041,10 +2041,10 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
 	int res;
 
 	if (!last)
-		sprintf(msg, ", part of a wide port with phy%d", sibling);
+		sprintf(msg, ", part of a wide port with phy%02d", sibling);
 
-	pr_debug("ex %016llx rediscovering phy%d%s\n", SAS_ADDR(dev->sas_addr),
-		 phy_id, msg);
+	pr_debug("ex %016llx rediscovering phy%02d%s\n",
+		 SAS_ADDR(dev->sas_addr), phy_id, msg);
 
 	memset(sas_addr, 0, SAS_ADDR_SIZE);
 	res = sas_get_phy_attached_dev(dev, phy_id, sas_addr, &type);
@@ -2083,13 +2083,13 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
 
 		if (ata_dev && phy->attached_dev_type == SAS_SATA_PENDING)
 			action = ", needs recovery";
-		pr_debug("ex %016llx phy 0x%x broadcast flutter%s\n",
+		pr_debug("ex %016llx phy%02d broadcast flutter%s\n",
 			 SAS_ADDR(dev->sas_addr), phy_id, action);
 		return res;
 	}
 
 	/* we always have to delete the old device when we went here */
-	pr_info("ex %016llx phy 0x%x replace %016llx\n",
+	pr_info("ex %016llx phy%02d 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);
@@ -2119,7 +2119,7 @@ static int sas_rediscover(struct domain_device *dev, const int phy_id)
 	int i;
 	bool last = true;	/* is this the last phy of the port */
 
-	pr_debug("ex %016llx phy%d originated BROADCAST(CHANGE)\n",
+	pr_debug("ex %016llx phy%02d originated BROADCAST(CHANGE)\n",
 		 SAS_ADDR(dev->sas_addr), phy_id);
 
 	if (SAS_ADDR(changed_phy->attached_sas_addr) != 0) {
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 213c85557bf9..28a460c36c0b 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -625,7 +625,7 @@ struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy)
 	if (atomic_read(&phy->event_nr) > phy->ha->event_thres) {
 		if (i->dft->lldd_control_phy) {
 			if (cmpxchg(&phy->in_shutdown, 0, 1) == 0) {
-				pr_notice("The phy%02d bursting events, shut it down.\n",
+				pr_notice("The phy%d bursting events, shut it down.\n",
 					  phy->id);
 				sas_notify_phy_event(phy, PHYE_SHUTDOWN);
 			}
diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
index 0374243c85d0..e030e1452136 100644
--- a/drivers/scsi/libsas/sas_phy.c
+++ b/drivers/scsi/libsas/sas_phy.c
@@ -122,11 +122,10 @@ static void sas_phye_shutdown(struct work_struct *work)
 		phy->enabled = 0;
 		ret = i->dft->lldd_control_phy(phy, PHY_FUNC_DISABLE, NULL);
 		if (ret)
-			pr_notice("lldd disable phy%02d returned %d\n",
-				  phy->id, ret);
+			pr_notice("lldd disable phy%d returned %d\n", phy->id,
+				  ret);
 	} else
-		pr_notice("phy%02d is not enabled, cannot shutdown\n",
-			  phy->id);
+		pr_notice("phy%d is not enabled, cannot shutdown\n", phy->id);
 }
 
 /* ---------- Phy class registration ---------- */
-- 
2.17.1

Re: [PATCH 0/6] libsas: Some minor improvements and tidy-up

[Martin K. Petersen]

John,

> This patchset introduces some minor improvements and tidy-up, including:
> - fix PHY info in sysfs for PHY events
> - min pathway rate programming improvement
> - some other tidy-up and neatening

Applied to 5.2/scsi-queue, thanks!

One thing that always bugged me is the ambiguous "ex" prefix for many
libsas log messages. And in a way, your patch #3 makes things worse by
replacing the more descriptive "Expander" with "ex". It does make things
more consistent, though.

I'd really like to see all the libsas messages be cleaned up with a
sensible prefix and a consistent format where possible.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/6] libsas: Some minor improvements and tidy-up

[John Garry]

On 16/04/2019 00:33, Martin K. Petersen wrote:
>
> John,
>
>> This patchset introduces some minor improvements and tidy-up, including:
>> - fix PHY info in sysfs for PHY events
>> - min pathway rate programming improvement
>> - some other tidy-up and neatening
>
> Applied to 5.2/scsi-queue, thanks!
>

Hi Martin,

Sorry for the slow response.

> One thing that always bugged me is the ambiguous "ex" prefix for many
> libsas log messages.And in a way, your patch #3 makes things worse by
> replacing the more descriptive "Expander" with "ex". It does make things
> more consistent, though.

OK, understood. Personally I prefer the brevity of "ex". However, apart 
from this, adding the SAS address to the log does add value.

>
> I'd really like to see all the libsas messages be cleaned up with a
> sensible prefix and a consistent format where possible.

So all libsas messages do have the "sas: " prefix, but they still can be 
vague/inconsistent at times.

Anyway, I'll check what can be done here.

Thanks,


>