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

[Simon Glass]

Hi Heinrich,

On Wed, 14 Aug 2024 at 06:53, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> 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?

It is in usb_stop() as I said above. See drivers/usb/host/usb-uclass.c

There might be other similar code, but what SATA is doing here is
similar to what others do, or want to do.

Regards,
Simon


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