Re: [flashrom] [PATCH 1/3] Refactor PCI and USB device status printing.

[Carl-Daniel Hailfinger]

Am 17.11.2012 20:09 schrieb Stefan Tauner:
> To be able to get rid of lots of #ifdefs and centralize programmer-specific
> data more...
> introduce two new fields to struct programmer_entry, namely
> enum type (OTHER, USB, PCI) and union devices (pcidev_status, usbdev_status
> or char *note).
> use those fields to generate device listings in print.c and print_wiki.c.
>
> Bonus: add printing of USB devices to print_wiki.c and count supported PCI
> and USB devices.
>
> Signed-off-by: Stefan Tauner

Good idea.
I have a few comments, otherwise it looks OK.
Note: I have not checked the wiki output, I only reviewed the code. I
trust you have checked the wiki output for changes.
> flashrom.c   |   48 ++++++++++++++++++-
>  ft2232_spi.c |   13 ------
>  pcidev.c     |   13 ------
>  print.c      |  148 +++++++++++++++++++---------------------------------------
>  print_wiki.c |  141 ++++++++++++++++++++++++++++++++++---------------------
>  programmer.h |   41 ++++++++++------
>  6 files changed, 211 insertions(+), 193 deletions(-)
>
> diff --git a/flashrom.c b/flashrom.c
> index 2299b06..d873276 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -64,6 +64,8 @@ const struct programmer_entry programmer_table[] = {
>  #if CONFIG_INTERNAL == 1
>    {
>    	.name			= "internal",
> .type			= OTHER,
> .devices.note		= NULL,
> .init			= internal_init,
> .map_flash_region	= physmap,
> .unmap_flash_region	= physunmap,
>
> @@ -84,6 +89,8 @@ const struct programmer_entry programmer_table[] = {
>  #if CONFIG_NIC3COM == 1
>    {
>    	.name			= "nic3com",
> .type			= PCI,
> .devices.pci		= nics_3com,

If we reference the device list here, can we kill the code in each
programmer which mentions the device list explicitly and use
pgm->devices->pci there? Otherwise there's a good chance someone will
forget one or the other reference and wonder why --list and actual
recognized programmer devices differ.
> .init			= nic3com_init,
> .map_flash_region	= fallback_map,
> .unmap_flash_region	= fallback_unmap,
>
> @@ -185,6 +212,9 @@ const struct programmer_entry programmer_table[] = {
>  #if CONFIG_DEDIPROG == 1
>    {
>    	.name			= "dediprog",
> .type			= OTHER,

Regression. The old code lists all devices.
> 			/* FIXME */
> .devices.note		= "Dediprog SF100\n",
> .init			= dediprog_init,
> .map_flash_region	= fallback_map,
> .unmap_flash_region	= fallback_unmap,
>
> diff --git a/print.c b/print.c
> index d7bdbfe..4b23ca0 100644
> --- a/print.c
> +++ b/print.c
> @@ -433,8 +433,32 @@ static void print_supported_boards_helper(const struct board_info *boards,
>  }
>  #endif

For consistency, we should introduce NEED_USB here. That would need
Makefile glue, but I think it's worth it, especially now that there are
Dediprog patches which change the libusb-0 dependency to a libusb-1
dependency.
> +void print_supported_usbdevs(const struct usbdev_status *devs)
> +{
> int i;
> msg_pinfo("USB devices:\n");
> for (i = 0; devs[i].vendor_name != NULL; i++) {
> msg_pinfo("%s %s [%04x:%04x]%s\n", devs[i].vendor_name, devs[i].device_name, devs[i].vendor_id,
> 	  devs[i].device_id, (devs[i].status == NT) ? " (untested)" : "");
> }
>
> +}
>
> +#if NEED_PCI == 1
> +void print_supported_pcidevs(const struct pcidev_status *devs)
> +{
> int i;
> for (i = 0; devs[i].vendor_name != NULL; i++) {
> msg_pinfo("%s %s [%04x:%04x]%s\n", devs[i].vendor_name, devs[i].device_name, devs[i].vendor_id,
>           devs[i].device_id, (devs[i].status == NT) ? " (untested)" : "");
> }
>
> +}
> +#endif
>
> int print_supported(void)
>  {
> unsigned int i;
> if (print_supported_chips())
> return 1;
>
> @@ -449,107 +473,31 @@ int print_supported(void)
>    msg_ginfo("\n");
>    print_supported_boards_helper(laptops_known, "laptops");
>  #endif
> -#if CONFIG_DUMMY == 1
> msg_ginfo("\nSupported devices for the %s programmer:\n",
>       programmer_table[PROGRAMMER_DUMMY].name);
> /* FIXME */
> msg_ginfo("Dummy device, does nothing and logs all accesses\n");
>
> -#endif
> [...]
> -#if CONFIG_LINUX_SPI == 1
> msg_ginfo("\nSupported devices for the %s programmer:\n",
>       programmer_table[PROGRAMMER_LINUX_SPI].name);
> msg_ginfo("Device files /dev/spidev*.*\n");
> for (i = 0; i < PROGRAMMER_INVALID; i++) {
> const struct programmer_entry prog = programmer_table[i];
> switch (prog.type) {

NEED_USB again.
> case USB:
> 	msg_ginfo("\nSupported USB devices for the %s programmer:\n", prog.name);
> 	print_supported_usbdevs(prog.devices.usb);
> 	break;
>
> +#if NEED_PCI == 1
> case PCI:
> 	msg_ginfo("\nSupported PCI devices for the %s programmer:\n", prog.name);
> 	print_supported_pcidevs(prog.devices.pci);
> 	break;
>
> #endif
> case OTHER:
> 	if (prog.devices.note == NULL)
> 		break;

Interesting logic. I'm not saying it is wrong, but it's not immediately
obvious that .note=NULL means the programmer won't be listed at all. I
fear this might be forgotten by future programmer driver authors, but
then again I should update my programmer code generation script and push
it to svn so this won't be a problem in the future.
> 	msg_ginfo("\nSupported devices for the %s programmer:\n", prog.name);
> 	msg_ginfo("%s", prog.devices.note);
> 	break;
> default:
> 	msg_gerr("\n%s: %s: Uninitialized programmer type! Please report a bug at "
> 		 "[e-mail]\n", __func__, prog.name);
> 	break;
> }
> }
> return 0;
>
> }
>
> diff --git a/print_wiki.c b/print_wiki.c
> index 617053c..17e2e85 100644
> --- a/print_wiki.c
> +++ b/print_wiki.c
> @@ -302,9 +298,18 @@ static void print_supported_chips_wiki(int cols)
>    printf("|}\n\n");
>  }
>
> -/* Not needed for CONFIG_INTERNAL, but for all other PCI-based programmers. */
> -#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV >= 1
> -static void print_supported_pcidevs_wiki(const struct pcidev_status *devs)
> +/* Following functions are not needed when no PCI/USB programmers are compiled in,
> but since print_wiki code has no size constraints we include it unconditionally. */

Heh.
> +static int count_supported_pcidevs_wiki(const struct pcidev_status *devs)
> +{
> unsigned int count = 0;
> unsigned int i = 0;
> for (i = 0; devs[i].vendor_name != NULL; i++)

AFAICS this should segfault in case a driver has prog.type=PCI and
prog.devices.pci==NULL. Not a problem, but maybe we should catch this
case (as well as the USB case) in a selftest function during startup?
That would also help during programmer development.
> 	count++;
> return count;
>
> +}
>
> +static void print_supported_pcidevs_wiki_helper(const struct pcidev_status *devs)
>  {
>    int i = 0;
>    static int c = 0;
> @@ -313,14 +318,81 @@ static void print_supported_pcidevs_wiki(const struct pcidev_status *devs)
>    c = !c;
>
> for (i = 0; devs[i].vendor_name != NULL; i++) {
> printf("|- bgcolor=\"#%s\"\n| %s || %s || "
>        "%04x:%04x || {{%s}}\n", (c) ? "eeeeee" : "dddddd",
>        devs[i].vendor_name, devs[i].device_name,
>        devs[i].vendor_id, devs[i].device_id,
> printf("|- bgcolor=\"#%s\"\n| %s || %s || %04x:%04x || {{%s}}\n", (c) ? "eeeeee" : "dddddd",
>        devs[i].vendor_name, devs[i].device_name, devs[i].vendor_id, devs[i].device_id,
>   (devs[i].status == NT) ? "?3" : "OK");
> }
>
> }
> -#endif
>
> +static int count_supported_usbdevs_wiki(const struct usbdev_status *devs)
> +{
> unsigned int count = 0;
> unsigned int i = 0;
> for (i = 0; devs[i].vendor_name != NULL; i++)
> 	count++;
> return count;
>
> +}
>
> +static void print_supported_usbdevs_wiki_helper(const struct usbdev_status *devs)
> +{
> int i = 0;
> static int c = 0;
> /* Alternate colors if the vendor changes. */
> c = !c;
> for (i = 0; devs[i].vendor_name != NULL; i++) {
> printf("|- bgcolor=\"#%s\"\n| %s || %s || %04x:%04x || {{%s}}\n", (c) ? "eeeeee" : "dddddd",
>        devs[i].vendor_name, devs[i].device_name, devs[i].vendor_id, devs[i].device_id,
>        (devs[i].status == NT) ? "?3" : "OK");
> }
>
> +}
>
> +static void print_supported_devs_wiki()
> +{
> unsigned int pci_count = 0;
> unsigned int usb_count = 0;
> unsigned int i;
> for (i = 0; i < PROGRAMMER_INVALID; i++) {
> const struct programmer_entry prog = programmer_table[i];
> switch (prog.type) {
> case USB:
> 	usb_count += count_supported_usbdevs_wiki(prog.devices.usb);
> 	break;
> case PCI:
> 	pci_count += count_supported_pcidevs_wiki(prog.devices.pci);
> 	break;
> case OTHER:
> default:
> 	break;
> }
> }
> printf("\n== PCI Devices ==\n\n"
>       "Total amount of supported PCI devices flashrom can use as a programmer: '''%d'''\n\n"
>       "{%s%s", pci_count, th_start, programmer_th);
> for (i = 0; i < PROGRAMMER_INVALID; i++) {
> const struct programmer_entry prog = programmer_table[i];
> if (prog.type == PCI) {
> 	print_supported_pcidevs_wiki_helper(prog.devices.pci);
> }
> }
> printf("\n|}\n\n|}\n");
> printf("\n== USB Devices ==\n\n"
>       "Total amount of supported USB devices flashrom can use as a programmer: '''%d'''\n\n"
>       "{%s%s", usb_count, th_start, programmer_th);
> for (i = 0; i < PROGRAMMER_INVALID; i++) {
> const struct programmer_entry prog = programmer_table[i];
> if (prog.type == USB) {
> 	print_supported_usbdevs_wiki_helper(prog.devices.usb);
> }
> }
> printf("\n|}\n\n|}\n");
>
> +}
>
> void print_supported_wiki(void)
>  {
> @@ -332,41 +404,6 @@ void print_supported_wiki(void)
>    print_supported_chipsets_wiki(3);
>    print_supported_boards_wiki();
>  #endif
> printf("%s%s%s", programmer_intro, th_start, programmer_th);
>
> -#if CONFIG_NIC3COM == 1
> print_supported_pcidevs_wiki(nics_3com);
>
> -#endif
> -#if CONFIG_NICREALTEK == 1
> print_supported_pcidevs_wiki(nics_realtek);
>
> -#endif
> -#if CONFIG_NICNATSEMI == 1
> print_supported_pcidevs_wiki(nics_natsemi);
>
> -#endif
> -#if CONFIG_GFXNVIDIA == 1
> print_supported_pcidevs_wiki(gfx_nvidia);
>
> -#endif
> -#if CONFIG_DRKAISER == 1
> print_supported_pcidevs_wiki(drkaiser_pcidev);
>
> -#endif
> -#if CONFIG_SATASII == 1
> print_supported_pcidevs_wiki(satas_sii);
>
> -#endif
> -#if CONFIG_ATAHPT == 1
> print_supported_pcidevs_wiki(ata_hpt);
>
> -#endif
> -#if CONFIG_NICINTEL == 1
> print_supported_pcidevs_wiki(nics_intel);
>
> -#endif
> -#if CONFIG_NICINTEL_SPI == 1
> print_supported_pcidevs_wiki(nics_intel_spi);
>
> -#endif
> -#if CONFIG_OGP_SPI == 1
> print_supported_pcidevs_wiki(ogp_spi);
>
> -#endif
> -#if CONFIG_SATAMV == 1
> print_supported_pcidevs_wiki(satas_mv);
>
> -#endif
> printf("\n|}\n\n|}\n");
> print_supported_devs_wiki();
>
> }
>
> diff --git a/programmer.h b/programmer.h
> index dedec67..21fa707 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -90,9 +90,21 @@ enum programmer {
>    PROGRAMMER_INVALID /* This must always be the last entry. */
>  };
>
> +enum programmer_type {
> PCI = 1, /* to detect uninitialized values */
> USB,
> OTHER,
>
> +};
>
> struct programmer_entry {
>    const char *vendor;
>    const char *name;
> const enum programmer_type type;
> union {
> const struct pcidev_status *const pci;
> const struct usbdev_status *const usb;
> const char *const note;

Neat!
> } devices;
>
> int (*init) (void);
>
> @@ -221,13 +233,6 @@ void internal_delay(int usecs);
>  extern uint32_t io_base_addr;
>  extern struct pci_access *pacc;
>  extern struct pci_dev *pcidev_dev;
> -struct pcidev_status {
> uint16_t vendor_id;
> uint16_t device_id;
> const enum test_state status;
> const char *vendor_name;
> const char *device_name;
>
> -};
>  uintptr_t pcidev_readbar(struct pci_dev *dev, int bar);
>  uintptr_t pcidev_init(int bar, const struct pcidev_status *devs);
>  /* rpci_write_* are reversible writes. The original PCI config space register
> @@ -244,6 +249,21 @@ int rpci_write_long(struct pci_dev *dev, int reg, uint32_t data);
>  void print_supported_pcidevs(const struct pcidev_status *devs);
>  #endif
>
> +struct usbdev_status {
> uint16_t vendor_id;
> uint16_t device_id;
> const enum test_state status;
> const char *vendor_name;
> const char *device_name;
>
> +};
> +struct pcidev_status {
> uint16_t vendor_id;
> uint16_t device_id;
> const enum test_state status;
> const char *vendor_name;
> const char *device_name;
>
> +};

Hm. You moved the struct pcidev_status and usbdev_status outside the
#ifdef protecting it. Given that this has no impact on code generation,
I don't have a big problem with it.

This is a question we should eventually decide: Do we want unused struct
declarations inside #ifdef or not? Such a decision should not hold up
merging this code, I just wanted to mention it.
> #if CONFIG_INTERNAL == 1
>  /* board_enable.c */
>  int board_parse_parameter(const char *boardstring, const char **vendor, const char **model);
> @@ -422,13 +442,6 @@ extern const struct pcidev_status ata_hpt[];
>
> /* ft2232_spi.c */
>  #if CONFIG_FT2232_SPI == 1
> -struct usbdev_status {
> uint16_t vendor_id;
> uint16_t device_id;
> const enum test_state status;
> const char *vendor_name;
> const char *device_name;
>
> -};
>  int ft2232_spi_init(void);
>  extern const struct usbdev_status devs_ft2232spi[];
>  void print_supported_usbdevs(const struct usbdev_status *devs);

Regards,
Carl-Daniel