linux-kernel - Re: [PATCH] scsi: libsas: Fix disk not being scanned in after being removed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0e336146-0f8b-4ce2-bc28-ab582e2c5f22@oracle.com>
Date: Wed, 6 Mar 2024 17:57:44 +0000
From: John Garry <john.g.garry@...cle.com>
To: yangxingui <yangxingui@...wei.com>, Jason Yan <yanaijie@...wei.com>,
        jejb@...ux.ibm.com, martin.petersen@...cle.com,
        damien.lemoal@...nsource.wdc.com
Cc: linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linuxarm@...wei.com, prime.zeng@...ilicon.com,
        chenxiang66@...ilicon.com, kangfenglong@...wei.com
Subject: Re: [PATCH] scsi: libsas: Fix disk not being scanned in after being
 removed

On 05/03/2024 11:25, yangxingui wrote:
>>
>> It's like the change in this patch.
> This doesn't work properly. the previous sas address will be compared 
> with the currently queried sas address and the previous phy information 
> will also be used when calling sas_unregister_devs_sas_addr() after the 
> sas_rediscover_dev() function calls sas_get_phy_attached_dev(). 
> Therefore, it is more appropriate to update the phy information after 
> the device is unregistered.

ok, fine

> as follows:
> static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>                                bool last, int sibling)
> {
>      ...
>         res = sas_get_phy_attached_dev(dev, phy_id, sas_addr, &type);
>          switch (res) {
>          case SMP_RESP_NO_PHY:
>                  phy->phy_state = PHY_NOT_PRESENT;
>                  sas_unregister_devs_sas_addr(dev, phy_id, last);
>                  return res;
>          case SMP_RESP_PHY_VACANT:
>                  phy->phy_state = PHY_VACANT;
>                  sas_unregister_devs_sas_addr(dev, phy_id, last);
>                  return res;
>          case SMP_RESP_FUNC_ACC:
>                  break;
>          case -ECOMM:
>                  break;
>          default:
>                  return res;
>          }
> 
>          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) && // <=== Compare the previous sas 
> address with the current sas address
>                     dev_type_flutter(type, phy->attached_dev_type)) {
>                  struct domain_device *ata_dev = sas_ex_to_ata(dev, 
> phy_id);
>                  char *action = "";
> 
>                  sas_ex_phy_discover(dev, phy_id);
> 
>                  if (ata_dev && phy->attached_dev_type == SAS_SATA_PENDING)
>                          action = ", needs recovery";
>                  pr_debug("ex %016llx phy%02d broadcast flutter%s\n",
>                           SAS_ADDR(dev->sas_addr), phy_id, action);
>                  return res;
>          }
> 
>>
>>
>>>
>>> OK, so let me have a closer look at v2.
>>
>> I have to say that v2 is quite complicated...
> Yes, but it works.

I'll check it again.

Thanks,
John