Hi, John
On 2023/11/29 20:54, John Garry wrote:
On 28/11/2023 03:45, yangxingui wrote:
On 2023/11/28 3:28, John Garry wrote:
On 24/11/2023 02:27, yangxingui wrote:
We already do this in sas_ex_join_wide_port(), right?
No, If the addr of ex_phy matches dev->parent,
sas_ex_join_wide_port() will not be called, but
sas_add_parent_port() will be called as follows:
static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
{
struct expander_device *ex = &dev->ex_dev;
struct ex_phy *ex_phy = &ex->ex_phy[phy_id];
struct domain_device *child = NULL;
int res = 0;
<...>
/* Parent and domain coherency */
if (!dev->parent && sas_phy_match_port_addr(dev->port,
ex_phy)) {
sas_add_parent_port(dev, phy_id);
return 0;
}
if (dev->parent && sas_phy_match_dev_addr(dev->parent,
ex_phy)) {
sas_add_parent_port(dev, phy_id);
if (ex_phy->routing_attr == TABLE_ROUTING)
sas_configure_phy(dev, phy_id,
dev->port->sas_addr, 1);
return 0;
}
<...>
}
I am not saying that what we do now does not have a problem - I am
just trying to understand what currently happens
ok, because ex_phy->port is not set when calling
sas_add_parent_port(), when deleting phy from the parent wide port,
it is not removed from the phy_list of the parent wide port as follows:
static void sas_unregister_devs_sas_addr(struct domain_device *parent,
int phy_id, bool last)
{
<...>
// Since ex_phy->port is not set, this branch will not be enter
But then how does this ever work? It is because we follow path
sas_rediscover_dev() -> sas_discover_new() ->
sas_ex_discover_devices() -> sas_ex_discover_dev() ->
sas_add_parent_port(), and not sas_rediscover_dev() ->
sas_discover_new() -> sas_ex_join_wide_port()? If so, is that because
ephy->sas_attached_phy == 0 in sas_discover_new() ->
sas_ex_join_wide_port() and it fails?
BTW, about something mentioned earlier - adding the phy19 with SAS_ADDR
Yes,
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;
}
}
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;
+}
+
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);
}
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?
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.
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?
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.
Thanks,
Xingui
