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

[Stefan Tauner]

On Sun, 18 Nov 2012 20:45:38 +0100
Carl-Daniel Hailfinger  wrote:
> 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.

no idea if it is possible, but i have put it on my todo list. i am
not sure we really want these programmer-specific bits here though. i
would rather want to declare the complete programmer structs in the
programmer files and just add pointers to them to this array... i
think... now, and need to look at it in detail. in any case this is
nothing we want to do soon imho. i (i.e. the todo.txt :) wont forget
about it.
> > .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.

Not the one in trunk:
"Supported devices for the dediprog programmer:
Dediprog SF100"
the corresponding hunk contains:
-#if CONFIG_DEDIPROG == 1
-	msg_ginfo("\nSupported devices for the %s programmer:\n",
-	       programmer_table[PROGRAMMER_DEDIPROG].name);
-	/* FIXME */
-	msg_ginfo("Dediprog SF100\n");
-#endif
> > 			/* 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.

still waiting for that... :/
> > +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.

hm well, it is more like an error/fail safe case, i just did not bother
to add a check for it yet (or at least i thought so ;). i have done so
now and there is a programmer loop now in selfcheck() that checks for
valid (i.e. non-null for most of the cases)
 - name
 - vendor... wait what? our programmers have a vendor? no! they do not!
             i have removed this cruft from the struct. i wonder why
             you did not notice that when writing your programmer
             generator.
 - type
 - devices list/note
 - init function
 - delay function
 - map_flash_region function
 - unmap_flash_region function
basically every field is checked.

and then i found out why i really did what i did back then. the
internal programmer is special and to allow the print code to easily
skip it i added the note == null option. damn :)
i have now added a special case for the internal programmer in the
checking code so that it is the only one where type = OTHER and note =
NULL is expected and allowed.
> > 	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.

ah you suggested that... my well-behaved unconsciousness made me
implement that before i re-read the complete mail :)
> > 	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
> > @@ -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.

didnt that come up already once? dunno. my rule of thumb is: if it would
require to add a new #ifdef that was not there before, then skip that
and add the definition without it, else add it to the existing #ifdef.

in this case however it is actually needed because i include the wiki
print functions for devices unconditionally, and in theory that would
not compile then (likewise for usbdev_status):
CONFIG_INTERNAL=no CONFIG_SERPROG=yes CONFIG_RAYER_SPI=no CONFIG_PONY_SPI=no CONFIG_NIC3COM=no CONFIG_GFXNVIDIA=no CONFIG_SATASII=no CONFIG_FT2232_SPI=no CONFIG_DUMMY=no CONFIG_DRKAISER=no CONFIG_NICREALTEK=no CONFIG_NICINTEL=no CONFIG_NICINTEL_SPI=no CONFIG_OGP_SPI=no CONFIG_BUSPIRATE_SPI=no CONFIG_SATAMV=no CONFIG_LINUX_SPI=no CONFIG_PRINT_WIKI=yes make

cc -MMD -Os -Wall -Wshadow -Werror   -D'CONFIG_DEFAULT_PROGRAMMER=PROGRAMMER_INVALID' -D'CONFIG_SERPROG=1' -D'CONFIG_PRINT_WIKI=1' -D'HAVE_UTSNAME=1' -D'FLASHROM_VERSION="0.9.6.1-r1627"' -o print_wiki.o -c print_wiki.c
print_wiki.c: In function ‘count_supported_pcidevs_wiki’:
print_wiki.c:307:2: error: invalid use of undefined type ‘struct pcidev_status’

yes, i am mad. :P

the code will come later together with the other patches (refactored/partially merged).