| Message ID | 20190926070133.26053-1-joel@jms.id.au |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] kernel: Use FSI master 'class' path | expand |
On Thu, 26 Sep 2019, at 16:31, 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> > --- > v3: > - fix probe bug > - add newlines to all of the print statements > - no need to call access() at the in dtb.c, just check if we have a > valid path > v2: > - Test for new path first so that code is being tested/used > - Print a debug message when no devices found > --- > libpdbg/dtb.c | 15 ++++++++-- > libpdbg/kernel.c | 71 +++++++++++++++++++++++++++++++++++++++-------- > libpdbg/libpdbg.h | 2 ++ > 3 files changed, 73 insertions(+), 15 deletions(-) > > diff --git a/libpdbg/dtb.c b/libpdbg/dtb.c > index 6c4beeda4fe6..53e9b7351e1b 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,8 +69,7 @@ static enum pdbg_backend default_backend(void) > if (rc == 0) /* AMI BMC */ > return PDBG_BACKEND_I2C; > > - rc = access(OPENFSI_BMC, F_OK); > - if (rc == 0) /* Kernel interface. OpenBMC */ > + if (kernel_get_fsi_path() != NULL) /* Kernel interface. OpenBMC */ > return PDBG_BACKEND_KERNEL; > > return PDBG_BACKEND_FAKE; > @@ -134,7 +134,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()); It looks like we could still get NULL from kernel_get_fsi_path() here if the user has specified the kernel backend on the commandline. We're fine if the backend is autodetected as the selected backend is predicated on kernel_get_fsi_path() not returning NULL in the previous hunk. Maybe the user gets what they deserve in this case, but it probably isn't the mechanism we should use for erroring out. Andrew
On Friday, 27 September 2019 5:02:40 PM AEST Andrew Jeffery wrote: > > On Thu, 26 Sep 2019, at 16:31, 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> > > --- > > v3: > > - fix probe bug > > - add newlines to all of the print statements > > - no need to call access() at the in dtb.c, just check if we have a > > valid path > > v2: > > - Test for new path first so that code is being tested/used > > - Print a debug message when no devices found > > --- > > libpdbg/dtb.c | 15 ++++++++-- > > libpdbg/kernel.c | 71 +++++++++++++++++++++++++++++++++++++++-------- > > libpdbg/libpdbg.h | 2 ++ > > 3 files changed, 73 insertions(+), 15 deletions(-) > > > > diff --git a/libpdbg/dtb.c b/libpdbg/dtb.c > > index 6c4beeda4fe6..53e9b7351e1b 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,8 +69,7 @@ static enum pdbg_backend default_backend(void) > > if (rc == 0) /* AMI BMC */ > > return PDBG_BACKEND_I2C; > > > > - rc = access(OPENFSI_BMC, F_OK); > > - if (rc == 0) /* Kernel interface. OpenBMC */ > > + if (kernel_get_fsi_path() != NULL) /* Kernel interface. OpenBMC */ > > return PDBG_BACKEND_KERNEL; > > > > return PDBG_BACKEND_FAKE; > > @@ -134,7 +134,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()); > > It looks like we could still get NULL from kernel_get_fsi_path() here if the user has > specified the kernel backend on the commandline. We're fine if the backend is > autodetected as the selected backend is predicated on kernel_get_fsi_path() not > returning NULL in the previous hunk. > > Maybe the user gets what they deserve in this case, but it probably isn't the > mechanism we should use for erroring out. Right. A segfault is always a sub-optimal error. Thanks for the review, I can do a simple fix when I merge it. - Alistair > Andrew >
diff --git a/libpdbg/dtb.c b/libpdbg/dtb.c index 6c4beeda4fe6..53e9b7351e1b 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,8 +69,7 @@ static enum pdbg_backend default_backend(void) if (rc == 0) /* AMI BMC */ return PDBG_BACKEND_I2C; - rc = access(OPENFSI_BMC, F_OK); - if (rc == 0) /* Kernel interface. OpenBMC */ + if (kernel_get_fsi_path() != NULL) /* Kernel interface. OpenBMC */ return PDBG_BACKEND_KERNEL; return PDBG_BACKEND_FAKE; @@ -134,7 +134,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..476531e6297d 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\n", strerror(errno)); return rc; } @@ -53,7 +80,7 @@ static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64, uint32_t *value) /* We expect reads of 0xc09 to occasionally * fail as the probing code uses it to see * if anything is present on the link. */ - PR_ERROR("Failed to read from 0x%08" PRIx32 " (%016" PRIx32 ")", (uint32_t) addr, addr64); + PR_ERROR("Failed to read from 0x%08" PRIx32 " (%016" PRIx32 ")\n", (uint32_t) addr, addr64); return rc; } *value = be32toh(tmp); @@ -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\n", strerror(errno)); return rc; } @@ -77,7 +104,7 @@ static int kernel_fsi_putcfam(struct fsi *fsi, uint32_t addr64, uint32_t data) rc = write(fsi_fd, &tmp, 4); if (rc < 0) { rc = errno; - PR_ERROR("Failed to write to 0x%08" PRIx32 " (%016" PRIx32 ")", addr, addr64); + PR_ERROR("Failed to write to 0x%08" PRIx32 " (%016" PRIx32 ")\n", addr, addr64); 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\n"); + 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\n", 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\n", 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/slave@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(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\n", 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);
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> --- v3: - fix probe bug - add newlines to all of the print statements - no need to call access() at the in dtb.c, just check if we have a valid path v2: - Test for new path first so that code is being tested/used - Print a debug message when no devices found --- libpdbg/dtb.c | 15 ++++++++-- libpdbg/kernel.c | 71 +++++++++++++++++++++++++++++++++++++++-------- libpdbg/libpdbg.h | 2 ++ 3 files changed, 73 insertions(+), 15 deletions(-)