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

[Simon Glass]

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) ?

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