| Message ID | 20240814071037.34446-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v3,1/1] ata: sata_rescan must scan for block devices | expand |
Hi Heinrich, On Wed, Aug 14, 2024 at 2:10 PM Heinrich Schuchardt <heinrich.schuchardt@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@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(). Reviewed-by: Tony Dinh <mibodhi@gmail.com> All the best, Tony > --- > 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@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); > + } > } > > 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 >
On Wed, 14 Aug 2024 at 01:10, Heinrich Schuchardt <heinrich.schuchardt@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@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@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
On 14.08.24 14:40, Simon Glass wrote: > On Wed, 14 Aug 2024 at 01:10, Heinrich Schuchardt > <heinrich.schuchardt@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@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@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? 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
Hi Heinrich, On Wed, 14 Aug 2024 at 06:53, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 14.08.24 14:40, Simon Glass wrote: > > On Wed, 14 Aug 2024 at 01:10, Heinrich Schuchardt > > <heinrich.schuchardt@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@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@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 >
Hi Simon, On Fri, Aug 16, 2024 at 3:32 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Heinrich, > > On Wed, 14 Aug 2024 at 06:53, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: > > > > On 14.08.24 14:40, Simon Glass wrote: > > > On Wed, 14 Aug 2024 at 01:10, Heinrich Schuchardt > > > <heinrich.schuchardt@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@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@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. All the best, Tony > > 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 > >
Hi Tony, On Sun, 18 Aug 2024 at 06:01, Tony Dinh <mibodhi@gmail.com> wrote: > > Hi Simon, > > On Fri, Aug 16, 2024 at 3:32 AM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Heinrich, > > > > On Wed, 14 Aug 2024 at 06:53, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > > > > > > On 14.08.24 14:40, Simon Glass wrote: > > > > On Wed, 14 Aug 2024 at 01:10, Heinrich Schuchardt > > > > <heinrich.schuchardt@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@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@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@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
On Wed, 14 Aug 2024 09:10:37 +0200, Heinrich Schuchardt 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. > > [...] Applied to u-boot/next, thanks!
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@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); + } } 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,
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@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(-)