[PATCH v3 1/1] ata: sata_rescan must scan for block devices

[Heinrich Schuchardt]

On 14.08.24 14:40, Simon Glass wrote:
> On Wed, 14 Aug 2024 at 01:10, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> A system may have multiple SATA controller. Removing the controller with
>> the lowest sequence number before probing all SATA controllers makes no
>> sense.
>>
>> In sata_rescan we remove all block devices which are children of SATA
>> controllers. We also have to remove the bootdev devices as they will be
>> created when scanning for block devices.
>>
>> After probing all SATA controllers we must scan for block devices otherwise
>> we end up without any SATA block device.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>> v3:
>>          continue unbinding and scanning if one device fails
>>          write a message if scanning fails
>>          add device name to error message
>> v2:
>>          Scanning for device on an unprobed SATA controller leads
>>          to an illegal memory access.
>>          Use uclass_foreach_dev_probe() instead of uclass_foreach_dev().
>> ---
>>   drivers/ata/sata.c | 48 +++++++++++++++++++++++++---------------------
>>   1 file changed, 26 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/ata/sata.c b/drivers/ata/sata.c
>> index 84437d3d346..89cd516f3d6 100644
>> --- a/drivers/ata/sata.c
>> +++ b/drivers/ata/sata.c
>> @@ -9,9 +9,12 @@
>>    *             Dave Liu <daveliu at freescale.com>
>>    */
>>
>> +#define LOG_CATEGORY UCLASS_AHCI
>> +
>>   #include <ahci.h>
>>   #include <blk.h>
>>   #include <dm.h>
>> +#include <log.h>
>>   #include <part.h>
>>   #include <sata.h>
>>   #include <dm/device-internal.h>
>> @@ -49,38 +52,39 @@ int sata_scan(struct udevice *dev)
>>
>>   int sata_rescan(bool verbose)
>>   {
>> +       struct uclass *uc;
>> +       struct udevice *dev; /* SATA controller */
>>          int ret;
>> -       struct udevice *dev;
>>
>>          if (verbose)
>> -               printf("Removing devices on SATA bus...\n");
>> -
>> -       blk_unbind_all(UCLASS_AHCI);
>> -
>> -       ret = uclass_find_first_device(UCLASS_AHCI, &dev);
>> -       if (ret || !dev) {
>> -               printf("Cannot find SATA device (err=%d)\n", ret);
>> -               return -ENOENT;
>> -       }
>> -
>> -       ret = device_remove(dev, DM_REMOVE_NORMAL);
>> -       if (ret) {
>> -               printf("Cannot remove SATA device '%s' (err=%d)\n", dev->name, ret);
>> -               return -ENOSYS;
>> +               printf("scanning bus for devices...\n");
>> +
>> +       ret = uclass_get(UCLASS_AHCI, &uc);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Remove all children of SATA devices (blk and bootdev) */
>> +       uclass_foreach_dev(dev, uc) {
>> +               log_debug("unbind %s\n", dev->name);
>> +               ret = device_chld_remove(dev, NULL, DM_REMOVE_NORMAL);
>> +               if (!ret)
>> +                       ret = device_chld_unbind(dev, NULL);
>> +               if (ret && verbose) {
>> +                       log_err("Unbinding from %s failed (%dE)\n",
>> +                               dev->name, ret);
>> +               }
>>          }
> 
> This code is in usb_stop() to, so please put it in a shared function.
> How about uclass_unbind_children(enum uclass_id) ?

git grep -A5 -n uclass_foreach_dev | grep device_chld_remove

only finds

drivers/scsi/scsi.c-571-
                 ret = device_chld_remove(dev, NULL, DM_REMOVE_NORMAL);

Where did you see matching USB code?

scsi_scan() could also use a rewrite to avoid stopping at the first error.

I guess sata_rescan() and scsi_scan() could be the same function taking 
a uclass ID as an argument?

Best regards

Heinrich

> 
>>
>>          if (verbose)
>>                  printf("Rescanning SATA bus for devices...\n");
>>
>> -       ret = uclass_probe_all(UCLASS_AHCI);
>> -
>> -       if (ret == -ENODEV) {
>> -               if (verbose)
>> -                       printf("No SATA block device found\n");
>> -               return 0;
>> +       uclass_foreach_dev_probe(UCLASS_AHCI, dev) {
>> +               ret = sata_scan(dev);
>> +               if (ret && verbose)
>> +                       log_err("Scanning %s failed (%dE)\n", dev->name, ret);
>>          }
>>
>> -       return ret;
>> +       return 0;
>>   }
>>
>>   static unsigned long sata_bread(struct udevice *dev, lbaint_t start,
>> --
>> 2.45.2
>>
> 
> If you have time, please add a test for ahci (dm/test/ahci)
> 
> Regards,
> Simon