Messages in this thread | | | Date | Mon, 27 Nov 2023 19:28:43 +0000 | Subject | Re: [PATCH v4] scsi: libsas: Fix the failure of adding phy with zero-address to port | From | John Garry <> |
| |
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 = 0 ever to a sas_port seems wrong.
> if (phy->port) { > sas_port_delete_phy(phy->port, phy->phy); > sas_device_set_phy(found, phy->port); > if (phy->port->num_phys == 0) { > list_add_tail(&phy->port->del_list, > &parent->port->sas_port_del_list); > if (ex_dev->parent_port == phy->port) > ex_dev->parent_port = NULL; > } > phy->port = NULL; > } > }
Thanks, John
| |