[Pdbg] [PATCH v3 2/2] pdbg: Remove default_backend() selection
Alistair Popple
alistair at popple.id.au
Wed Jul 3 16:03:56 AEST 2019
On Wednesday, 3 July 2019 2:14:50 PM AEST Amitay Isaacs wrote:
> Hi Alistair,
>
> Instead of keeping duplicate enum for backend and keeping them in sync
> with libpdbg enum for backend, should we just use libpdbg backend enum?
>
> pdbg_set_backend() requires enum pdbg_backend anyway..
Yeah, I meant to do that but forgot. Thanks.
- Alistair
> On Wed, 2019-07-03 at 13:20 +1000, Alistair Popple wrote:
> > Now that libpdbg can automatically select the correct device-tree the
> > pdbg application doesn't need code to do the same thing. By default
> > pdbg can just rely on the device-tree selected by libpdbg.
> >
> > Unfortunately to maintain backwards compatibility for users
> > explicitly
> > selecting a device-tree and device-tree node we need to retain most
> > of
> > the code and continue linking the device-trees into the
> > application. It would be good to remove these one day so print a
> > deprecation warning when a user does explicitly select a backend.
> >
> > Signed-off-by: Alistair Popple <alistair at popple.id.au>
> > ---
> >
> > Makefile.am | 14 +----
> > src/main.c | 121 ++++++++------------------------------------
> > src/main.h | 2 +-
> > src/options.h | 27 ----------
> > src/options_arm.c | 149 --------------------------------------------
> >
> > ----------
> >
> > src/options_def.c | 39 --------------
> > src/options_ppc.c | 72 --------------------------
> > 7 files changed, 23 insertions(+), 401 deletions(-)
> > delete mode 100644 src/options.h
> > delete mode 100644 src/options_arm.c
> > delete mode 100644 src/options_def.c
> > delete mode 100644 src/options_ppc.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index b317b96..e155583 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -70,7 +70,6 @@ endif
> >
> > DT = fake.dts $(DT_ARM) $(DT_PPC)
> >
> > -DT_objects = $(DT:.dts=.dtb.lo)
> >
> > DT_sources = $(DT:.dts=.dtb.S)
> > DT_headers = $(DT:.dts=.dt.h)
> >
> > @@ -106,16 +105,6 @@ pdbg_SOURCES = \
> >
> > src/util.c \
> > src/util.h
> >
> > -if TARGET_ARM
> > -pdbg_SOURCES += src/options_arm.c
> > -else
> > -if TARGET_PPC
> > -pdbg_SOURCES += src/options_ppc.c
> > -else
> > -pdbg_SOURCES += src/options_def.c
> > -endif
> > -endif
> > -
> >
> > pdbg_CFLAGS = -I$(top_srcdir)/libpdbg -Wall -Werror
> >
> > -DGIT_SHA1=\"${GIT_SHA1}\" \
> >
> > $(ARCH_FLAGS)
> >
> > @@ -129,12 +118,11 @@ else
> >
> > pdbg_CFLAGS += -DDISABLE_GDBSERVER
> > endif
> >
> > -src/main.c: $(DT_headers)
> >
> > libpdbg/dtb.c: $(DT_headers)
> >
> > src/pdbg-gdb_parser.$(OBJEXT): CFLAGS+=-Wno-unused-const-variable
> >
> > -pdbg_LDADD = $(DT_objects) libpdbg.la libccan.a \
> > +pdbg_LDADD = libpdbg.la libccan.a \
> >
> > -L.libs -lrt
> >
> > lib_LTLIBRARIES = libpdbg.la
> >
> > diff --git a/src/main.c b/src/main.c
> > index ad40d19..5ce4fdb 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -35,7 +35,6 @@
> >
> > #include "main.h"
> > #include "htm.h"
> >
> > -#include "options.h"
> >
> > #include "optcmd.h"
> > #include "progress.h"
> > #include "pdbgproxy.h"
> >
> > @@ -64,7 +63,7 @@
> >
> > #define THREADS_PER_CORE 8
> >
> > -static enum backend backend = KERNEL;
> > +static enum backend backend = DEFAULT_BACKEND;
> >
> > static char const *device_node;
> > static int i2c_addr = 0x50;
> >
> > @@ -407,7 +406,6 @@ static bool parse_options(int argc, char *argv[])
> >
> > backend = HOST;
> >
> > } else {
> >
> > fprintf(stderr, "Invalid backend
> >
> > '%s'\n", optarg);
> > - print_backends(stderr);
> >
> > opt_error = true;
> >
> > }
> > break;
> >
> > @@ -531,96 +529,6 @@ static bool parse_options(int argc, char
> > *argv[])
> >
> > return true;
> >
> > }
> >
> > -static bool target_selection(void)
> > -{
> > - switch (backend) {
> > -#ifdef TARGET_ARM
> > - case I2C:
> > - pdbg_targets_init(&_binary_p8_i2c_dtb_o_start);
> > - /* Set device for I2C backend */
> > - if (device_node) {
> > - struct pdbg_target *pib;
> > - const size_t len = strlen(device_node) + 1 /*
> > include last null */;
> > - pdbg_for_each_class_target("pib", pib) {
> > - pdbg_target_set_property(pib, "bus",
> > device_node, len);
> > - }
> > - }
> > - break;
> > -
> > - case FSI:
> > - if (device_node == NULL) {
> > - PR_ERROR("FSI backend requires a device
> > type\n");
> > - return false;
> > - }
> > - if (!strcmp(device_node, "p8"))
> > - pdbg_targets_init(&_binary_p8_fsi_dtb_o_start);
> > - else if (!strcmp(device_node, "p9w"))
> > - pdbg_targets_init(&_binary_p9w_fsi_dtb_o_start)
> > ;
> > - else if (!strcmp(device_node, "p9r"))
> > - pdbg_targets_init(&_binary_p9r_fsi_dtb_o_start)
> > ;
> > - else if (!strcmp(device_node, "p9z"))
> > - pdbg_targets_init(&_binary_p9z_fsi_dtb_o_start)
> > ;
> > - else {
> > - PR_ERROR("Invalid device type specified\n");
> > - return false;
> > - }
> > - break;
> > -
> > - case KERNEL:
> > - if (device_node == NULL) {
> > - PR_ERROR("kernel backend requires a device
> > type\n");
> > - return false;
> > - }
> > - if (!strcmp(device_node, "p8"))
> > - pdbg_targets_init(&_binary_p8_kernel_dtb_o_star
> > t);
> > - else
> > - pdbg_targets_init(&_binary_p9_kernel_dtb_o_star
> > t);
> > - break;
> > -
> > -#endif
> > -
> > -#ifdef TARGET_PPC
> > - case HOST:
> > - if (device_node == NULL) {
> > - PR_ERROR("Host backend requires a device
> > type\n");
> > - return false;
> > - }
> > - if (!strcmp(device_node, "p8"))
> > - pdbg_targets_init(&_binary_p8_host_dtb_o_start)
> > ;
> > - else if (!strcmp(device_node, "p9"))
> > - pdbg_targets_init(&_binary_p9_host_dtb_o_start)
> > ;
> > - else {
> > - PR_ERROR("Unsupported device type for host
> > backend\n");
> > - return false;
> > - }
> > - break;
> > -#endif
> > -
> > - case FAKE:
> > - pdbg_targets_init(&_binary_fake_dtb_o_start);
> > - break;
> > -
> > - default:
> > - /* parse_options deals with parsing user input, so it
> > should be
> > - * impossible to get here */
> > - assert(0);
> > - return false;
> > - }
> > -
> > - if (pathsel_count) {
> > - if (!path_target_parse(pathsel, pathsel_count))
> > - return false;
> > - }
> > -
> > - if (!path_target_present()) {
> > - printf("No valid targets found or specified. Try adding
> > -p/-c/-t options to specify a target.\n");
> > - printf("Alternatively run 'pdbg -a probe' to get a list
> > of all valid targets\n");
> > - return false;
> > - }
> > -
> > - return true;
> > -}
> > -
> >
> > static void print_target(struct pdbg_target *target, int level)
> > {
> >
> > int i;
> >
> > @@ -677,11 +585,6 @@ int main(int argc, char *argv[])
> >
> > optcmd_cmd_t *cmd;
> > struct pdbg_target *target;
> >
> > - backend = default_backend();
> > -
> > - if (!device_node)
> > - device_node = default_target(backend);
> > -
> >
> > if (!parse_options(argc, argv))
> >
> > return 1;
> >
> > @@ -690,9 +593,27 @@ int main(int argc, char *argv[])
> >
> > return 1;
> >
> > }
> >
> > - /* Disable unselected targets */
> > - if (!target_selection())
> > + if (backend) {
> > + fprintf(stderr, "WARNING: Explicit backend selection
> > should no longer be required\n");
> > + fprintf(stderr, " and will be deprecated in a
> > future release.\n");
> > + fprintf(stderr, " Removing -b/-d command line
> > options will remove this warning.\n");
> > + fprintf(stderr, " An explicit device-tree can
> > still be selected using the\n");
> > + fprintf(stderr, " LPDBG_DTB environment
> > variable instead.\n");
> > + pdbg_set_backend(backend, device_node);
> > + }
> > +
> > + pdbg_targets_init(NULL);
> > +
> > + if (pathsel_count) {
> > + if (!path_target_parse(pathsel, pathsel_count))
> > + return 1;
> > + }
> > +
> > + if (!path_target_present()) {
> > + printf("No valid targets found or specified. Try adding
> > -p/-c/-t options to specify a target.\n");
> > + printf("Alternatively run 'pdbg -a probe' to get a list
> > of all valid targets\n");
> >
> > return 1;
> >
> > + }
> >
> > /* Probe all selected targets */
> > for_each_path_target(target) {
> >
> > diff --git a/src/main.h b/src/main.h
> > index 78b4d92..1d5913e 100644
> > --- a/src/main.h
> > +++ b/src/main.h
> > @@ -19,7 +19,7 @@
> >
> > #include <libpdbg.h>
> >
> > -enum backend { FSI, I2C, KERNEL, FAKE, HOST };
> > +enum backend { DEFAULT_BACKEND = 0, FSI, I2C, KERNEL, FAKE, HOST };
> >
> > static inline bool target_is_disabled(struct pdbg_target *target)
> > {
> >
> > diff --git a/src/options.h b/src/options.h
> > deleted file mode 100644
> > index 67f15a8..0000000
> > --- a/src/options.h
> > +++ /dev/null
> > @@ -1,27 +0,0 @@
> > -/* Copyright 2017 IBM Corp.
> > - *
> > - * Licensed under the Apache License, Version 2.0 (the "License");
> > - * you may not use this file except in compliance with the License.
> > - * You may obtain a copy of the License at
> > - *
> > - * http://www.apache.org/licenses/LICENSE-2.0
> > - *
> > - * Unless required by applicable law or agreed to in writing,
> > software
> > - * distributed under the License is distributed on an "AS IS" BASIS,
> > - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > - * implied.
> > - * See the License for the specific language governing permissions
> > and
> > - * limitations under the License.
> > - */
> > -
> > -/* Default backend on this platform */
> > -enum backend default_backend(void);
> > -
> > -/* Print all possible backends on this platform */
> > -void print_backends(FILE *stream);
> > -
> > -/* The default (perhaps only) target for this backend */
> > -const char *default_target(enum backend backend);
> > -
> > -/* Print all possible targets on this platform */
> > -void print_targets(FILE *stream);
> > diff --git a/src/options_arm.c b/src/options_arm.c
> > deleted file mode 100644
> > index 0dbc731..0000000
> > --- a/src/options_arm.c
> > +++ /dev/null
> > @@ -1,149 +0,0 @@
> > -/* Copyright 2017 IBM Corp.
> > - *
> > - * Licensed under the Apache License, Version 2.0 (the "License");
> > - * you may not use this file except in compliance with the License.
> > - * You may obtain a copy of the License at
> > - *
> > - * http://www.apache.org/licenses/LICENSE-2.0
> > - *
> > - * Unless required by applicable law or agreed to in writing,
> > software
> > - * distributed under the License is distributed on an "AS IS" BASIS,
> > - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > - * implied.
> > - * See the License for the specific language governing permissions
> > and
> > - * limitations under the License.
> > - */
> > -#include <stdio.h>
> > -#include <string.h>
> > -#include <unistd.h>
> > -#include <errno.h>
> > -
> > -#include "main.h"
> > -
> > -#define AMI_BMC "/proc/ractrends/Helper/FwInfo"
> > -#define OPENFSI_BMC "/sys/bus/platform/devices/gpio-fsi/fsi0/"
> > -#define FSI_CFAM_ID "/sys/devices/platform/gpio-fsi/fsi0/slave at 00:00
> > /cfam_id"
> > -
> > -#define CHIP_ID_P8 0xea
> > -#define CHIP_ID_P9 0xd1
> > -#define CHIP_ID_P8P 0xd3
> > -
> > -enum backend default_backend(void)
> > -{
> > - int rc;
> > - rc = access(AMI_BMC, F_OK);
> > - if (rc == 0) /* AMI BMC */
> > - return I2C;
> > -
> > - rc = access(OPENFSI_BMC, F_OK);
> > - if (rc == 0) /* Kernel interface. OpenBMC */
> > - return KERNEL;
> > -
> > - return FAKE;
> > -}
> > -
> > -void print_backends(FILE *stream)
> > -{
> > - fprintf(stream, "Valid backends: i2c kernel fsi fake\n");
> > -}
> > -
> > -void print_targets(FILE *stream)
> > -{
> > - fprintf(stream, "kernel: No target is necessary\n");
> > - fprintf(stream, "i2c: No target is necessary\n");
> > - fprintf(stream, "fsi: p8 p9w p9r p9z\n");
> > -}
> > -
> > -static const char *default_kernel_target(void)
> > -{
> > - FILE *cfam_id_file;
> > -
> > - /* Try and determine the correct device type */
> > - cfam_id_file = fopen(FSI_CFAM_ID, "r");
> > - if (cfam_id_file) {
> > - uint32_t cfam_id = 0;
> > - int rc;
> > -
> > - rc = fscanf(cfam_id_file, "0x%" PRIx32, &cfam_id);
> > - if (rc != 1)
> > - pdbg_log(PDBG_ERROR, "%s", strerror(errno));
> > - fclose(cfam_id_file);
> > -
> > - switch((cfam_id >> 4) & 0xff) {
> > - case CHIP_ID_P9:
> > - return "p9";
> > - break;
> > -
> > - case CHIP_ID_P8:
> > - case CHIP_ID_P8P:
> > - return "p8";
> > - break;
> > -
> > - default:
> > - pdbg_log(PDBG_ERROR, "Unknown chip-id
> > detected\n");
> > - pdbg_log(PDBG_ERROR, "You will need to specify
> > a host type with -d <host type>\n");
> > - return NULL;
> > - }
> > - } else {
> > - /* The support for POWER8 included the cfam_id
> > - * so if it doesn't exist assume we must be on
> > - * P9 */
> > - pdbg_log(PDBG_WARNING, "Unable to determine host type,
> > defaulting to p9\n");
> > - return "p9";
> > - }
> > -
> > - return NULL;
> > -}
> > -
> > -static const char *default_fsi_target(void)
> > -{
> > - FILE *dt_compatible;
> > - char line[256];
> > - char *p;
> > -
> > - dt_compatible = fopen("/proc/device-tree/compatible", "r");
> > - if (!dt_compatible)
> > - return NULL;
> > -
> > - p = fgets(line, sizeof(line), dt_compatible);
> > - fclose(dt_compatible);
> > - if (!p) /* Uh oh*/
> > - return NULL;
> > -
> > - if (strstr(line, "witherspoon"))
> > - return "p9w";
> > -
> > - if (strstr(line, "romulus"))
> > - return "p9r";
> > -
> > - if (strstr(line, "zaius"))
> > - return "p9z";
> > -
> > - if (strstr(line, "palmetto"))
> > - return "p8";
> > -
> > - return NULL;
> > -}
> > -
> > -const char *default_target(enum backend backend)
> > -{
> > - switch(backend) {
> > - case I2C:
> > - return NULL;
> > - break;
> > -
> > - case KERNEL:
> > - return default_kernel_target();
> > - break;
> > -
> > - case FSI:
> > - return default_fsi_target();
> > - break;
> > -
> > - default:
> > - return NULL;
> > - break;
> > - }
> > -
> > - return NULL;
> > -}
> > diff --git a/src/options_def.c b/src/options_def.c
> > deleted file mode 100644
> > index 4092cea..0000000
> > --- a/src/options_def.c
> > +++ /dev/null
> > @@ -1,39 +0,0 @@
> > -/* Copyright 2017 IBM Corp.
> > - *
> > - * Licensed under the Apache License, Version 2.0 (the "License");
> > - * you may not use this file except in compliance with the License.
> > - * You may obtain a copy of the License at
> > - *
> > - * http://www.apache.org/licenses/LICENSE-2.0
> > - *
> > - * Unless required by applicable law or agreed to in writing,
> > software
> > - * distributed under the License is distributed on an "AS IS" BASIS,
> > - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > - * implied.
> > - * See the License for the specific language governing permissions
> > and
> > - * limitations under the License.
> > - */
> > -
> > -#include <stdio.h>
> > -#include "main.h"
> > -
> > -enum backend default_backend(void)
> > -{
> > - return FAKE;
> > -}
> > -
> > -void print_backends(FILE *stream)
> > -{
> > - fprintf(stream, "Valid backends: fake\n");
> > -}
> > -
> > -/* Theres no target for FAKE backend */
> > -const char *default_target(enum backend backend)
> > -{
> > - return NULL;
> > -}
> > -
> > -void print_targets(FILE *stream)
> > -{
> > - fprintf(stream, "fake: No target is necessary\n");
> > -}
> > diff --git a/src/options_ppc.c b/src/options_ppc.c
> > deleted file mode 100644
> > index 62eb7b0..0000000
> > --- a/src/options_ppc.c
> > +++ /dev/null
> > @@ -1,72 +0,0 @@
> > -/* Copyright 2017 IBM Corp.
> > - *
> > - * Licensed under the Apache License, Version 2.0 (the "License");
> > - * you may not use this file except in compliance with the License.
> > - * You may obtain a copy of the License at
> > - *
> > - * http://www.apache.org/licenses/LICENSE-2.0
> > - *
> > - * Unless required by applicable law or agreed to in writing,
> > software
> > - * distributed under the License is distributed on an "AS IS" BASIS,
> > - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > - * implied.
> > - * See the License for the specific language governing permissions
> > and
> > - * limitations under the License.
> > - */
> > -#include <stdio.h>
> > -#include <string.h>
> > -
> > -#include "main.h"
> > -
> > -static const char p8[] = "p8";
> > -static const char p9[] = "p9";
> > -
> > -enum backend default_backend(void)
> > -{
> > - return HOST;
> > -}
> > -
> > -void print_backends(FILE *stream)
> > -{
> > - fprintf(stream, "Valid backends: host fake\n");
> > -}
> > -
> > -const char *default_target(enum backend backend)
> > -{
> > - const char *pos = NULL;
> > - char line[256];
> > - FILE *cpuinfo;
> > -
> > - cpuinfo = fopen("/proc/cpuinfo", "r");
> > - if (!cpuinfo)
> > - return NULL;
> > -
> > - while ((pos = fgets(line, sizeof(line), cpuinfo)))
> > - if (strncmp(line, "cpu", 3) == 0)
> > - break;
> > - fclose(cpuinfo);
> > -
> > - if (!pos) /* Got to EOF without a break */
> > - return NULL;
> > -
> > - pos = strchr(line, ':');
> > - if (!pos)
> > - return NULL;
> > -
> > - if (*(pos + 1) == '\0')
> > - return NULL;
> > - pos += 2;
> > -
> > - if (strncmp(pos, "POWER8", 6) == 0)
> > - return p8;
> > -
> > - if (strncmp(pos, "POWER9", 6) == 0)
> > - return p9;
> > -
> > - return NULL;
> > -}
> > -
> > -void print_targets(FILE *stream)
> > -{
> > - fprintf(stream, "host: p8 p9\n");
> > -}
>
> Amitay.
More information about the Pdbg
mailing list