[Pdbg] [PATCH v2 1/7] libpdbg: Store fsi_fd state information in fsi structure
Alistair Popple
alistair at popple.id.au
Mon Jun 15 17:52:03 AEST 2020
On Monday, 15 June 2020 5:37:44 PM AEST Amitay Isaacs wrote:
> On Mon, 2020-06-15 at 17:22 +1000, Alistair Popple wrote:
> > On Wednesday, 10 June 2020 3:24:20 PM AEST Amitay Isaacs wrote:
> > > This allows to have different fd for different fsi targets. Also,
> > > we
> > > can close the fd cleanly in release procedure.
> > >
> > > Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> > > ---
> > > libpdbg/hwunit.h | 1 +
> > > libpdbg/kernel.c | 87 +++++++++++++++++++++++---------------------
> > > ----
> > > 2 files changed, 43 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/libpdbg/hwunit.h b/libpdbg/hwunit.h
> > > index 61aea0f..e3a8426 100644
> > > --- a/libpdbg/hwunit.h
> > > +++ b/libpdbg/hwunit.h
> > > @@ -112,6 +112,7 @@ struct fsi {
> > > int (*read)(struct fsi *, uint32_t, uint32_t *);
> > > int (*write)(struct fsi *, uint32_t, uint32_t);
> > > enum chip_type chip_type;
> > > + int fd;
> > > };
> > > #define target_to_fsi(x) container_of(x, struct fsi, target)
> > >
> > > diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
> > > index c4637a7..7914e50 100644
> > > --- a/libpdbg/kernel.c
> > > +++ b/libpdbg/kernel.c
> > > @@ -33,7 +33,6 @@
> > > #define OPENFSI_LEGACY_PATH "/sys/bus/platform/devices/gpio-fsi/"
> > > #define OPENFSI_PATH "/sys/class/fsi-master/"
> > >
> > > -int fsi_fd;
> > > const char *fsi_base;
> > >
> > > const char *kernel_get_fsi_path(void)
> > > @@ -66,14 +65,14 @@ static int kernel_fsi_getcfam(struct fsi *fsi,
> > > uint32_t
> > addr64, uint32_t *value)
> > > int rc;
> > > uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) <<
> > > 2);
> > >
> > > - rc = lseek(fsi_fd, addr, SEEK_SET);
> > > + rc = lseek(fsi->fd, addr, SEEK_SET);
> > > if (rc < 0) {
> > > rc = errno;
> > > PR_WARNING("seek failed: %s\n", strerror(errno));
> > > return rc;
> > > }
> > >
> > > - rc = read(fsi_fd, &tmp, 4);
> > > + rc = read(fsi->fd, &tmp, 4);
> > > if (rc < 0) {
> > > rc = errno;
> > > if ((addr64 & 0xfff) != 0xc09)
> > > @@ -93,7 +92,7 @@ static int kernel_fsi_putcfam(struct fsi *fsi,
> > > uint32_t
> > addr64, uint32_t data)
> > > int rc;
> > > uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) <<
> > > 2);
> > >
> > > - rc = lseek(fsi_fd, addr, SEEK_SET);
> > > + rc = lseek(fsi->fd, addr, SEEK_SET);
> > > if (rc < 0) {
> > > rc = errno;
> > > PR_WARNING("seek failed: %s\n", strerror(errno));
> > > @@ -101,7 +100,7 @@ static int kernel_fsi_putcfam(struct fsi *fsi,
> > > uint32_t
> > addr64, uint32_t data)
> > > }
> > >
> > > tmp = htobe32(data);
> > > - rc = write(fsi_fd, &tmp, 4);
> > > + rc = write(fsi->fd, &tmp, 4);
> > > if (rc < 0) {
> > > rc = errno;
> > > PR_ERROR("Failed to write to 0x%08" PRIx32 " (%016"
> > > PRIx32 ")\n",
> > addr, addr64);
> > > @@ -111,16 +110,6 @@ static int kernel_fsi_putcfam(struct fsi *fsi,
> > > uint32_t
> > addr64, uint32_t data)
> > > return 0;
> > > }
> > >
> > > -#if 0
> > > -/* TODO: At present we don't have a generic destroy method as
> > > there aren't
> > many
> > > - * use cases for it. So for the moment we can just let the OS
> > > close the file
> > > - * descriptor on exit. */
> > > -static void kernel_fsi_destroy(struct pdbg_target *target)
> > > -{
> > > - close(fsi_fd);
> > > -}
> > > -#endif
> > > -
> > > static void kernel_fsi_scan_devices(void)
> > > {
> > > const char one = '1';
> > > @@ -154,51 +143,59 @@ static void kernel_fsi_scan_devices(void)
> > >
> > > int kernel_fsi_probe(struct pdbg_target *target)
> > > {
> > > - if (!fsi_fd) {
> > > - int tries = 5;
> > > - int rc;
> > > - const char *kernel_path = kernel_get_fsi_path();
> > > - char *path;
> > > -
> > > - if (!kernel_path)
> > > - return -1;
> > > -
> > > - rc = asprintf(&path, "%s/fsi0/slave at 00:00/raw",
> > kernel_get_fsi_path());
> > > - if (rc < 0) {
> > > - PR_ERROR("Unable create fsi path\n");
> > > - return rc;
> > > - }
> > > + struct fsi *fsi = target_to_fsi(target);
> > > + int tries = 5;
> > > + int rc;
> > > + const char *kernel_path = kernel_get_fsi_path();
> > > + char *path;
> > >
> > > - 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);
> > > + if (!kernel_path)
> > > + return -1;
> > > +
> > > + rc = asprintf(&path, "%s/fsi0/slave at 00:00/raw",
> > > kernel_get_fsi_path());
> > > + if (rc < 0) {
> > > + PR_ERROR("Unable create fsi path\n");
> > > + return rc;
> > > + }
> > > +
> > > + while (tries) {
> > > + /* Open first raw device */
> > > + fsi->fd = open(path, O_RDWR | O_SYNC);
> > > + if (fsi->fd >= 0) {
> > > free(path);
> > > - return -1;
> > > + return 0;
> >
> > Minor nit that doesn't need fixing but IMHO it would probably have
> > been clearer
> > to make this a for loop and break out rather than return.
>
> I can still do that, if that'll make things clearer.
>
> >
> > > }
> > > + tries--;
> > >
> > > + /* Scan */
> > > + kernel_fsi_scan_devices();
> > > + sleep(1);
> > > + }
> > > + if (fsi->fd < 0) {
> > > + PR_ERROR("Unable to open %s\n", path);
> > > + free(path);
> > > }
> > >
> > > return -1;
> > > }
> > >
> > > +static void kernel_fsi_release(struct pdbg_target *target)
> > > +{
> > > + struct fsi *fsi = target_to_fsi(target);
> > > +
> > > + if (fsi->fd != -1) {
> >
> > In theory I think generic pdbg logic should prevent this from ever
> > being false
> > as release should only get called on a successful probe.
>
> Yes, we can definitely drop the check. I was going nuts trying to
> figure out what will happen with releasing virtual targets. I am
> convinced now that each target will be released only once.
Maybe print a warning, but it doesn't really matter because I merged them
anyway :-)
- Alistair
> >
> > Reviewed-by: Alistair Popple <alistair at popple.id.au>
> >
> > > + close(fsi->fd);
> > > + fsi->fd = -1;
> > > + }
> > > +}
> > > +
> > > static struct fsi kernel_fsi = {
> > > .target = {
> > > .name = "Kernel based FSI master",
> > > .compatible = "ibm,kernel-fsi",
> > > .class = "fsi",
> > > .probe = kernel_fsi_probe,
> > > + .release = kernel_fsi_release,
> > > },
> > > .read = kernel_fsi_getcfam,
> > > .write = kernel_fsi_putcfam,
> > >
> >
> >
> >
>
> Amitay.
>
More information about the Pdbg
mailing list