[v2] kernel: Use FSI master 'class' path

The paths to sysfs entries depend on the master driver used. This means
pdbg needs to use a different path when running against the hardware FSI
master present in the ast2600.

Instead of adding to an ever growing list of paths to check, the kernel
has created a 'fsi-master' class that any driver can add itself to. On a
kernel runnign this code, userspace only needs to check under
/sys/class/fsi-master for any platform.

This patch adds support for the class based path from a common accessor
in kernel.c.

It retains support for the existing gpio-fsi master. At some point in
the distant future the gpio-fsi code could be deleted.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2:
 - Test for new path first so that code is being tested/used
 - Print a debug message when no devices found
---
 libpdbg/dtb.c     | 14 ++++++++--
 libpdbg/kernel.c  | 67 ++++++++++++++++++++++++++++++++++++++++-------
 libpdbg/libpdbg.h |  2 ++
 3 files changed, 71 insertions(+), 12 deletions(-)

On Thu, 2019-09-26 at 15:41 +0930, Joel Stanley wrote:
> The paths to sysfs entries depend on the master driver used. This
> means
> pdbg needs to use a different path when running against the hardware
> FSI
> master present in the ast2600.
> 
> Instead of adding to an ever growing list of paths to check, the
> kernel
> has created a 'fsi-master' class that any driver can add itself to.
> On a
> kernel runnign this code, userspace only needs to check under
> /sys/class/fsi-master for any platform.
> 
> This patch adds support for the class based path from a common
> accessor
> in kernel.c.
> 
> It retains support for the existing gpio-fsi master. At some point in
> the distant future the gpio-fsi code could be deleted.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2:
>  - Test for new path first so that code is being tested/used
>  - Print a debug message when no devices found
> ---
>  libpdbg/dtb.c     | 14 ++++++++--
>  libpdbg/kernel.c  | 67 ++++++++++++++++++++++++++++++++++++++++-----
> --
>  libpdbg/libpdbg.h |  2 ++
>  3 files changed, 71 insertions(+), 12 deletions(-)
> 
> diff --git a/libpdbg/dtb.c b/libpdbg/dtb.c
> index 6c4beeda4fe6..2aea625feaf1 100644
> --- a/libpdbg/dtb.c
> +++ b/libpdbg/dtb.c
> @@ -13,6 +13,7 @@
>   * See the License for the specific language governing permissions
> and
>   * limitations under the License.
>   */
> +#define _GNU_SOURCE
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <stdbool.h>
> @@ -68,7 +69,7 @@ static enum pdbg_backend default_backend(void)
>  	if (rc == 0) /* AMI BMC */
>  		return PDBG_BACKEND_I2C;
>  
> -	rc = access(OPENFSI_BMC, F_OK);
> +	rc = access(kernel_get_fsi_path(), F_OK);
>  	if (rc == 0) /* Kernel interface. OpenBMC */
>  		return PDBG_BACKEND_KERNEL;
>  
> @@ -134,7 +135,16 @@ static void *bmc_target(void)
>  
>  	if (!pdbg_backend_option) {
>  		/* Try and determine the correct device type */
> -		cfam_id_file = fopen(FSI_CFAM_ID, "r");
> +		char *path;
> +
> +		rc = asprintf(&path, "%s/fsi0/slave@00:00/cfam_id",
> kernel_get_fsi_path());
> +		if (rc < 0) {
> +			pdbg_log(PDBG_ERROR, "Unable create fsi path");
> +			return NULL;
> +		}
> +
> +		cfam_id_file = fopen(path, "r");
> +		free(path);
>  		if (!cfam_id_file) {
>  			pdbg_log(PDBG_ERROR, "Unabled to open CFAM ID
> file\n");
>  			return NULL;
> diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
> index 4cc0334d7eb3..6d88298abfa7 100644
> --- a/libpdbg/kernel.c
> +++ b/libpdbg/kernel.c
> @@ -13,6 +13,7 @@
>   * See the License for the specific language governing permissions
> and
>   * limitations under the License.
>   */
> +#define _GNU_SOURCE
>  #include <stdint.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -29,10 +30,36 @@
>  #include "operations.h"
>  #include "hwunit.h"
>  
> -#define FSI_SCAN_PATH "/sys/bus/platform/devices/gpio-
> fsi/fsi0/rescan"
> -#define FSI_CFAM_PATH "/sys/devices/platform/gpio-fsi/fsi0/slave@00:
> 00/raw"
> +#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)
> +{
> +	int rc;
> +
> +	if (fsi_base)
> +		return fsi_base;
> +
> +	rc = access(OPENFSI_PATH, F_OK);
> +	if (rc == 0) {
> +		fsi_base = OPENFSI_PATH;
> +		return fsi_base;
> +	}
> +
> +	rc = access(OPENFSI_LEGACY_PATH, F_OK);
> +	if (rc == 0) {
> +		fsi_base = OPENFSI_LEGACY_PATH;
> +		return fsi_base;
> +	}
> +
> +	/* This is an error, but callers use this function when probing
> */
> +	PR_DEBUG("Failed to find kernel FSI path\n");
> +
> +	return NULL;
> +}
>  
>  static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64,
> uint32_t *value)
>  {
> @@ -42,7 +69,7 @@ static int kernel_fsi_getcfam(struct fsi *fsi,
> uint32_t addr64, uint32_t *value)
>  	rc = lseek(fsi_fd, addr, SEEK_SET);
>  	if (rc < 0) {
>  		rc = errno;
> -		PR_WARNING("Failed to seek %s", FSI_CFAM_PATH);
> +		PR_WARNING("seek failed: %s", strerror(errno));

I have a patch that fixes all the log messages which have missing
newlines.  Since you are touching most of the log messages in this
file, do you want to add the missing newlines?


>  		return rc;
>  	}
>  
> @@ -69,7 +96,7 @@ static int kernel_fsi_putcfam(struct fsi *fsi,
> uint32_t addr64, uint32_t data)
>  	rc = lseek(fsi_fd, addr, SEEK_SET);
>  	if (rc < 0) {
>  		rc = errno;
> -		PR_WARNING("Failed to seek %s", FSI_CFAM_PATH);
> +		PR_WARNING("seek failed: %s", strerror(errno));
>  		return rc;
>  	}
>  
> @@ -97,18 +124,27 @@ static void kernel_fsi_destroy(struct
> pdbg_target *target)
>  static void kernel_fsi_scan_devices(void)
>  {
>  	const char one = '1';
> +	char *path;
>  	int rc, fd;
>  
> -	fd = open(FSI_SCAN_PATH, O_WRONLY | O_SYNC);
> +	rc = asprintf(&path, "%s/fsi0/rescan", kernel_get_fsi_path());
> +	if (rc < 0) {
> +		PR_ERROR("Unable create fsi path");
> +		return;
> +	}
> +
> +	fd = open(path, O_WRONLY | O_SYNC);
>  	if (fd < 0) {
> -		PR_ERROR("Unable to open %s", FSI_SCAN_PATH);
> +		PR_ERROR("Unable to open %s", path);
> +		free(path);
>  		return;
>  	}
>  
>  	rc = write(fd, &one, sizeof(one));
>  	if (rc < 0)
> -		PR_ERROR("Unable to write to %s", FSI_SCAN_PATH);
> +		PR_ERROR("Unable to write to %s", path);
>  
> +	free(path);
>  	close(fd);
>  }
>  
> @@ -116,12 +152,22 @@ int kernel_fsi_probe(struct pdbg_target
> *target)
>  {
>  	if (!fsi_fd) {
>  		int tries = 5;
> +		int rc;
> +		char *path;
> +
> +		rc = asprintf(&path, "%s/fsi0/rescan",
> kernel_get_fsi_path());
> +		if (rc < 0) {
> +			PR_ERROR("Unable create fsi path");
> +			return rc;
> +		}
>  
>  		while (tries) {
>  			/* Open first raw device */
> -			fsi_fd = open(FSI_CFAM_PATH, O_RDWR | O_SYNC);
> -			if (fsi_fd >= 0)
> +			fsi_fd = open(path, O_RDWR | O_SYNC);
> +			if (fsi_fd >= 0) {
> +				free(path);
>  				return 0;
> +			}
>  			tries--;
>  
>  			/* Scan */
> @@ -129,7 +175,8 @@ int kernel_fsi_probe(struct pdbg_target *target)
>  			sleep(1);
>  		}
>  		if (fsi_fd < 0) {
> -			PR_ERROR("Unable to open %s", FSI_CFAM_PATH);
> +			PR_ERROR("Unable to open %s", path);
> +			free(path);
>  			return -1;
>  		}
>  
> diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> index 235ff85abdc6..468553bbf4b5 100644
> --- a/libpdbg/libpdbg.h
> +++ b/libpdbg/libpdbg.h
> @@ -121,6 +121,8 @@ void pdbg_target_priv_set(struct pdbg_target
> *target, void *priv);
>  struct pdbg_target *pdbg_target_root(void);
>  bool pdbg_target_compatible(struct pdbg_target *target, const char
> *compatible);
>  
> +const char *kernel_get_fsi_path(void);
> +
>  /* Translate an address offset for a target to absolute address in
> address
>   * space of a "base" target.  */
>  struct pdbg_target *pdbg_address_absolute(struct pdbg_target
> *target, uint64_t *addr);
> -- 
> 2.23.0
> 

Amitay.

On Thu, 26 Sep 2019, at 15:41, Joel Stanley wrote:
> The paths to sysfs entries depend on the master driver used. This means
> pdbg needs to use a different path when running against the hardware FSI
> master present in the ast2600.
> 
> Instead of adding to an ever growing list of paths to check, the kernel
> has created a 'fsi-master' class that any driver can add itself to. On a
> kernel runnign this code, userspace only needs to check under
> /sys/class/fsi-master for any platform.
> 
> This patch adds support for the class based path from a common accessor
> in kernel.c.
> 
> It retains support for the existing gpio-fsi master. At some point in
> the distant future the gpio-fsi code could be deleted.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2:
>  - Test for new path first so that code is being tested/used
>  - Print a debug message when no devices found
> ---
>  libpdbg/dtb.c     | 14 ++++++++--
>  libpdbg/kernel.c  | 67 ++++++++++++++++++++++++++++++++++++++++-------
>  libpdbg/libpdbg.h |  2 ++
>  3 files changed, 71 insertions(+), 12 deletions(-)
> 
> diff --git a/libpdbg/dtb.c b/libpdbg/dtb.c
> index 6c4beeda4fe6..2aea625feaf1 100644
> --- a/libpdbg/dtb.c
> +++ b/libpdbg/dtb.c
> @@ -13,6 +13,7 @@
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
>   */
> +#define _GNU_SOURCE
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <stdbool.h>
> @@ -68,7 +69,7 @@ static enum pdbg_backend default_backend(void)
>  	if (rc == 0) /* AMI BMC */
>  		return PDBG_BACKEND_I2C;
>  
> -	rc = access(OPENFSI_BMC, F_OK);
> +	rc = access(kernel_get_fsi_path(), F_OK);

kernel_get_fsi_path() can return NULL, however:

2 15:55:35 andrew@mistburn:/tmp$ cat access.c
#include <unistd.h>

int main(void)
{
        return access(NULL, F_OK);
}
2 15:57:32 andrew@mistburn:/tmp$ make CFLAGS="-Werror" access
cc -Werror    access.c   -o access
access.c: In function ‘main’:
access.c:5:9: error: null argument where non-null required (argument 1) [-Werror=nonnull]
  return access(NULL, F_OK);
         ^~~~~~
cc1: all warnings being treated as errors
make: *** [<builtin>: access] Error 1

Maybe just check if the result of  kernel_get_fsi_path() is NULL rather
than calling access() again?

>  	if (rc == 0) /* Kernel interface. OpenBMC */
>  		return PDBG_BACKEND_KERNEL;
>  
> @@ -134,7 +135,16 @@ static void *bmc_target(void)
>  
>  	if (!pdbg_backend_option) {
>  		/* Try and determine the correct device type */
> -		cfam_id_file = fopen(FSI_CFAM_ID, "r");
> +		char *path;
> +
> +		rc = asprintf(&path, "%s/fsi0/slave@00:00/cfam_id", kernel_get_fsi_path());
> +		if (rc < 0) {
> +			pdbg_log(PDBG_ERROR, "Unable create fsi path");
> +			return NULL;
> +		}
> +
> +		cfam_id_file = fopen(path, "r");
> +		free(path);
>  		if (!cfam_id_file) {
>  			pdbg_log(PDBG_ERROR, "Unabled to open CFAM ID file\n");
>  			return NULL;
> diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
> index 4cc0334d7eb3..6d88298abfa7 100644
> --- a/libpdbg/kernel.c
> +++ b/libpdbg/kernel.c
> @@ -13,6 +13,7 @@
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
>   */
> +#define _GNU_SOURCE
>  #include <stdint.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -29,10 +30,36 @@
>  #include "operations.h"
>  #include "hwunit.h"
>  
> -#define FSI_SCAN_PATH "/sys/bus/platform/devices/gpio-fsi/fsi0/rescan"
> -#define FSI_CFAM_PATH "/sys/devices/platform/gpio-fsi/fsi0/slave@00:00/raw"
> +#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)
> +{
> +	int rc;
> +
> +	if (fsi_base)
> +		return fsi_base;
> +
> +	rc = access(OPENFSI_PATH, F_OK);
> +	if (rc == 0) {
> +		fsi_base = OPENFSI_PATH;
> +		return fsi_base;
> +	}
> +
> +	rc = access(OPENFSI_LEGACY_PATH, F_OK);
> +	if (rc == 0) {
> +		fsi_base = OPENFSI_LEGACY_PATH;
> +		return fsi_base;
> +	}
> +
> +	/* This is an error, but callers use this function when probing */
> +	PR_DEBUG("Failed to find kernel FSI path\n");
> +
> +	return NULL;

Here's the problematic return.