Hi Heinrich,
On Wed, 14 Aug 2024 at 06:53, Heinrich Schuchardt
<heinrich.schucha...@canonical.com> wrote:
>
> On 14.08.24 14:40, Simon Glass wrote:
> > On Wed, 14 Aug 2024 at 01:10, Heinrich Schuchardt
> > <heinrich.schucha...@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.schucha...@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 <dave...@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
>
