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

[Andrew Jeffery]

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 at 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 at mistburn:/tmp$ cat access.c
#include <unistd.h>

int main(void)
{
        return access(NULL, F_OK);
}
2 15:57:32 andrew at 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 at 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 at 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.