Hi Tony,
On Sun, 18 Aug 2024 at 06:01, Tony Dinh <mibo...@gmail.com> wrote:
>
> Hi Simon,
>
> On Fri, Aug 16, 2024 at 3:32 AM Simon Glass <s...@chromium.org> wrote:
> >
> > 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.
>
> 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 <s...@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
