[flashrom] [PATCH 1/3] Refactor PCI and USB device status printing.
Stefan Tauner
stefan.tauner at student.tuwien.ac.at
Wed Dec 12 04:14:18 CET 2012
On Sun, 18 Nov 2012 20:45:38 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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 <stefan.tauner at student.tuwien.ac.at>
>
> 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 "
> > + "flashrom at flashrom.org\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).
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
More information about the flashrom
mailing list