[Pdbg] [PATCH v2 1/7] libpdbg: Store fsi_fd state information in fsi structure

[Alistair Popple]

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.

>  		}
> +		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.

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,
>