[Pdbg] [PATCH 5/7] libpdbg: Rescan fsi bus only once

[Amitay Isaacs]

On Tue, 2020-06-09 at 07:28 +0000, Joel Stanley wrote:
> On Tue, 26 May 2020 at 05:13, Amitay Isaacs <amitay at ozlabs.org>
> wrote:
> > On rescan, kernel driver re-creates all the slave devices.  This
> > invalidates all the slave deviced opened before the scan.
> > 
> > Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> > ---
> >  libpdbg/kernel.c | 37 +++++++++++++++++++++----------------
> >  1 file changed, 21 insertions(+), 16 deletions(-)
> > 
> > diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
> > index 2cc81f2..912895a 100644
> > --- a/libpdbg/kernel.c
> > +++ b/libpdbg/kernel.c
> > @@ -116,6 +116,15 @@ static void kernel_fsi_scan_devices(void)
> >         const char *kernel_path = kernel_get_fsi_path();
> >         char *path;
> >         int rc, fd;
> > +       static bool scanned = false;
> > +
> > +       /*
> > +        * On fsi bus rescan, kernel re-creates all the slave
> > device entries.
> > +        * It means any currently open devices will be invalid and
> > need to be
> > +        * re-opened.  So avoid scanning multiple times.
> > +        */
> > +       if (scanned)
> > +               return;
> > 
> >         if (!kernel_path)
> >                 return;
> > @@ -139,12 +148,13 @@ static void kernel_fsi_scan_devices(void)
> > 
> >         free(path);
> >         close(fd);
> > +
> > +       scanned = true;
> >  }
> > 
> >  int kernel_fsi_probe(struct pdbg_target *target)
> >  {
> >         struct fsi *fsi = target_to_fsi(target);
> > -       int tries = 5;
> >         int rc;
> >         const char *kernel_path = kernel_get_fsi_path();
> >         const char *fsi_path;
> > @@ -162,24 +172,19 @@ int kernel_fsi_probe(struct pdbg_target
> > *target)
> >                 return rc;
> >         }
> > 
> > -       while (tries) {
> > -               /* Open first raw device */
> > -               fsi->fd = open(path, O_RDWR | O_SYNC);
> > -               if (fsi->fd >= 0) {
> > -                       free(path);
> > -                       return 0;
> > -               }
> > -               tries--;
> > -
> > -               /* Scan */
> > -               kernel_fsi_scan_devices();
> > -               sleep(1);
> > -       }
> > -       if (fsi->fd < 0) {
> > -               PR_ERROR("Unable to open %s\n", path);
> > +       /* Always scan the fsi bus once */
> > +       kernel_fsi_scan_devices();
> 
> As your comment says, this changes the behavior to always scan even
> if
> the sysfs hierarchy is present. Previously it would try to open the
> "raw" file on the first master and if it was present it wouldn't
> disturb the setup.

In the earlier case, we tried to open only one device, so rescanning
didn't matter.  But if device 1 is opened and if device 2 cannot be
opened, then we cannot scan, as it makes the already open fd invalid.
So the only way was to either scan once or no scan at all.

Doing a scan is probably harmful on a working system, as other software
might have open fd for the raw device.  But on a broken system, you
would want to do a scan.  I am also fine with completely dropping the
scanning of fsi bus from pdbg.  

> Is there any reason you need to force a scan?
> 
> > +       sleep(1);
> > +
> > +       /* Open first raw device */
> > +       fsi->fd = open(path, O_RDWR | O_SYNC);
> > +       if (fsi->fd >= 0) {
> >                 free(path);
> > +               return 0;
> >         }
> > 
> > +       PR_ERROR("Unable to open %s\n", path);
> > +       free(path);
> >         return -1;
> >  }
> > 
> > --
> > 2.26.2
> > 
> > --
> > Pdbg mailing list
> > Pdbg at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/pdbg

Amitay.
-- 

NOW is the only thing that's real.