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

[Tony Dinh]

Hi Heinrich,

Thanks for upgrading this driver to handle multiple SATA controllers.
Please see my comments below.

On Fri, Aug 9, 2024 at 1:56 AM 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>
> ---
> 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 | 46 ++++++++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/ata/sata.c b/drivers/ata/sata.c
> index 84437d3d346..2139ae9903e 100644
> --- a/drivers/ata/sata.c
> +++ b/drivers/ata/sata.c
> @@ -49,38 +49,40 @@ 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) {
> +                       if (verbose)
> +                               printf("unable to unbind devices (%dE)\n", ret);
> +                       return log_msg_ret("unb", ret);

We should not bail out too early here. If the current controller
scanning has any problem, fail loudly and continue with the next
controller. IOW, remove the above "return log_msg_ret("unb", ret)".

> +               }
>         }
>
>         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)
> +                       return ret;

Same comment, are we bailing out too early here? If the current
controller probing has any problem, fail loudly and probe the next
controller. IOW, replace the above "return ret" with printf();

All the best,
Tony

>         }
>
> -       return ret;
> +       return 0;
>  }
>
>  static unsigned long sata_bread(struct udevice *dev, lbaint_t start,
> --
> 2.45.2
>