[Pdbg] [PATCH v2 1/7] libpdbg: Store fsi_fd state information in fsi structure
Amitay Isaacs
amitay at ozlabs.org
Mon Jun 15 17:37:44 AEST 2020
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.
>
> 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.
--
Treasure the love you receive above all. It will survive long after your gold
and good health have vanished. - Og Mandino
More information about the Pdbg
mailing list