On 30/11/2023 03:53, yangxingui wrote:Okay, then I will update the version.
For phy19, when the phy is attached and added to the parent wide port, the path is:
sas_rediscover()
->sas_discover_new()
->sas_ex_discover_devices()
->sas_ex_discover_dev()
-> sas_add_parent_port().
ok, so then the change to set ex_phy->port = ex->parent_port looks ok. Maybe we can put this in a helper with the sas_port_add_phy() call, as it is duplicated in sas_ex_join_wide_port()
Do we also need to set ex_phy->phy_state (like sas_ex_join_wide_port())?
Well, okay, as follows?
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -856,9 +856,7 @@ static bool sas_ex_join_wide_port(struct domain_device *parent, int phy_id)
if (!memcmp(phy->attached_sas_addr, ephy->attached_sas_addr,
SAS_ADDR_SIZE) && ephy->port) {
- sas_port_add_phy(ephy->port, phy->phy);
- phy->port = ephy->port;
- phy->phy_state = PHY_DEVICE_DISCOVERED;
+ sas_port_add_ex_phy(ephy->port, phy);
return true;
this looks ok. How about adding this helper and using it in a separate change?
Okay, then I will update the version and move it to sas_expander.c .
}
}
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index e860d5b19880..39ffa60a9a01 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -189,6 +189,13 @@ static inline void sas_phy_set_target(struct asd_sas_phy *p, struct domain_devic
}
}
+static inline void sas_port_add_ex_phy(struct sas_port *port, struct ex_phy *ex_phy)
+{
+ sas_port_add_phy(port, ex_phy->phy);
+ ex_phy->port = port;
+ ex_phy->phy_state = PHY_DEVICE_DISCOVERED;
+}
I'd prefer sas_expander.c, but sas_add_parent_port() is here... having said that, sas_add_parent_port() is only used in sas_expander.c
Yes. There are three possible ways to solve the problem of creating a port with a zero address:
+
static inline void sas_add_parent_port(struct domain_device *dev, int phy_id)
{
struct expander_device *ex = &dev->ex_dev;
@@ -201,8 +208,7 @@ static inline void sas_add_parent_port(struct domain_device *dev, int phy_id)
BUG_ON(sas_port_add(ex->parent_port));
sas_port_mark_backlink(ex->parent_port);
}
- sas_port_add_phy(ex->parent_port, ex_phy->phy);
+ sas_port_add_ex_phy(ex->parent_port, ex_phy);
}
Yes, but in fact it has not reached that stage yet. After setting the address to 0, it will continue to create a new port and try to add other phys with the same address as it to this new port.
And the path called when it is removed from parent wide port is:
sas_rediscover()
->sas_unregister_devs_sas_addr() // The sas address of phy19 becomes 0. Since ex_phy->port is NULL, phy19 is not removed from the parent wide port's phy_list.
For phy0, it is connected to a new sata device.
sas_rediscover()
->sas_discover_new()->sas_ex_phy_discover()
->sas_ex_phy_discover_helper()
->sas_set_ex_phy() // The device type is stp. Since the linkrate is 5 and less than 1.5G, sas_address is set to 0.
Then when we get the proper linkrate later, will we then rediscover and set the proper SAS address? I am just wondering if this change is really required?
creating a port for SAS address == 0 and adding phys seems incorrect, right?
OK.
Yes, in order to avoid this situation, in the current patch, we will not force the SAS address to be set to 0 when the device type is not NULL, but will still use the address obtained after requesting the expander.
BTW, Even with the change to set ex_phy->port = ex->parent_port, are we still joining the host-attached expander phy (19) to a port with SAS address == 0?
ok, let me check that again later today.