Re: [RFC PATCH 1/2] lspci: CCIX DVSEC initial support

Martin Mareš
2020-01-22T09:48:26Z

Hello!

A couple of remarks to the code:

> +#define ARRAY_SIZE(x) sizeof(x)/sizeof(*(x))

This can be useful in other parts, too, so please move it to pciutils.h.

> +struct ccix_state {
> + int portagg_capable:1;
> + int sw_portal_capable:1;
> + int forwarding:1;

Please avoid bit fields. There is no need to save data size at the expense
of increasing code size.

> +static const char * ccix_sub_mem_types[8] = {

This should be "static const char * const ccix_sub_mem_types[8]".

> + printf("\t\t\t\t\tMemErr:\t");

Could you please try decreasing the overall amount of indent to make
the output narrower?

> + switch (value) {
> + case 0 ... 2:

Please avoid GCC extensions.

> +static void
> +cap_dvsec(struct device *d, int where)
> +{
> + u32 buff;
> +
> + printf("Designated Vendor-Specific <>\n");
> +
> + if (verbose < 2)
> + return;

The "<>" does not make much sense now :)

If verbose < 2, I would print

Designated Vendor-Specific: vendor XXXX, version NNN

> + buff = get_conf_long(d, where + 4);
> + printf("\t\tVendor:%4x Version:%u\n", buff & 0xFFFF, (buff >> 16) & 0xF);
> + switch (buff & 0xffff) {

Maybe read two 16-bit numbers instead?

Have a nice fortnight
--
Martin `MJ' Mareš <mj@ucw.cz> http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe
"Snow falling on Perl. White noise covering line noise. Hides all the bugs too." -- J. Putnam