Re: [PATCH v4] scsi: libsas: Fix the failure of adding phy with zero-address to port

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, John

On 2023/12/1 17:22, John Garry wrote:
On 30/11/2023 03:53, yangxingui wrote:

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.

                 }
         }
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
Okay, then I will update the version and move it to sas_expander.c .


+
  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.

creating a port for SAS address == 0 and adding phys seems incorrect, right?
Yes. There are three possible ways to solve the problem of creating a port with a zero address: 1. Use the sas address obtained by querying the expander instead of the zero address.
2. Forbid the phy with an address of 0 to create a port.
3. When the rate is less than 1.5G, do not let it enter sas_ex_discover_end_dev().

Because when the device type is not empty, its SAS address is legal, and we are currently using the first one.



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.

ok, let me check that again later today.
OK.

Thanks
Xingui





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux