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

[Simon Glass]

Hi Tony,

On Sun, 18 Aug 2024 at 06:01, Tony Dinh <mibodhi at gmail.com> wrote:
>
> Hi Simon,
>
> On Fri, Aug 16, 2024 at 3:32 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > 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.
>
> IMHO, in the current implementation, the abstraction level is just
> right for the SATA, SCSI, and USB uclasses. There is no need to
> commonize further. I would keep the (re)scanning logic separate for
> each type of these devices.

Yes I agree the scanning logic does its own thing...the question is
what to do about the unbinding logic. Anyway, if Heinrich doesn't see
any common code, I think it is fine. I'll add my review tag just in
case.

Reviewed-by: Simon Glass <sjg at chromium.org>

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

Regards,
Simon