[Pdbg] [PATCH v2 2/2] pdbg: Remove default_backend() selection
Amitay Isaacs
amitay at ozlabs.org
Tue Jul 2 17:57:28 AEST 2019
On Tue, 2019-07-02 at 16:02 +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 | 13 +----
> 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(+), 400 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 5cb0b6f..287c294 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -106,16 +106,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 +119,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..121934f 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 false;
> + }
This should return 1 rather than 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 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");
> -}
> --
> 2.11.0
>
Amitay.
--
A wise man will make more opportunities than he finds. - Francis Bacon
More information about the Pdbg
mailing list