> -----Original Message----- > From: Jean Delvare [mailto:jdelvare@xxxxxxx] > Sent: Friday, June 2, 2017 7:43 PM > To: K, Narendra <Narendra_K@xxxxxxxx> > Cc: x86@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Hargrave, Jordan > <Jordan_Hargrave@xxxxxxxx>; Iyer, Shyam <Shyam_Iyer@xxxxxxxx>; > bhelgaas@xxxxxxxxxx > Subject: Re: dmi type 0xB1 record - unknown flag [...] > > The value 0x00 is not a valid value. When the flag is 0x00, the sort method > will be the default that is decided by the kernel. > > How can it be invalid? I have DMI dumps of several Dell systems with a type > 0xB1 DMI structure, with value 0x00 for these bits. If Dell ships such systems, > then this is valid by definition. > > > There is no additional handling required for such a system. > > I don't understand the complexity of the code. There are 4 possible values > for these bits, the code treats all of them differently: > * 0x01, you set smbios_type_b1_flag to 1 and this later triggers a call > to set_bf_sort(). > * 0x02, you set smbios_type_b1_flag to 2 but this has no effect. > * 0x00, you print a rather cryptic info message which serves no purpose. > * 0x03, you do nothing. > > On top of that, I can't see the value of the intermediate variable > smbios_type_b1_flag. The only situations where it would make a difference > is if multiple type 0xB1 structures with conflicting information were present; > but I don't think this is supposed to happen in the first place. > > Lastly I'm not sure why you continue processing the list of DMI matches, > when smbios_type_b1_flag == 1, and stop processing it if not. > This seems needlessly inconsistent. > > I think the whole thing can be simplified like this: > > From: Jean Delvare <jdelvare@xxxxxxx> > Subject: x86/PCI: Simplify Dell DMI B1 quirk > > No need for such convoluted code, when all we need is to call one function in > one specific case. > > Signed-off-by: Jean Delvare <jdelvare@xxxxxxx> > --- > arch/x86/pci/common.c | 27 +++++---------------------- > 1 file changed, 5 insertions(+), 22 deletions(-) > > --- linux-4.11.orig/arch/x86/pci/common.c 2017-05-01 > 04:47:48.000000000 +0200 > +++ linux-4.11/arch/x86/pci/common.c 2017-06-02 16:04:05.737889598 +0200 > @@ -24,7 +24,6 @@ unsigned int pci_probe = PCI_PROBE_BIOS > > unsigned int pci_early_dump_regs; > static int pci_bf_sort; > -static int smbios_type_b1_flag; > int pci_routeirq; > int noioapicquirk; > #ifdef CONFIG_X86_REROUTE_FOR_BROKEN_BOOT_IRQS > @@ -197,34 +196,18 @@ static int __init set_bf_sort(const stru static void > __init read_dmi_type_b1(const struct dmi_header *dm, > void *private_data) > { > - u8 *d = (u8 *)dm + 4; > + u8 *data = (u8 *)dm + 4; > > if (dm->type != 0xB1) > return; > - switch (((*(u32 *)d) >> 9) & 0x03) { > - case 0x00: > - printk(KERN_INFO "dmi type 0xB1 record - unknown flag\n"); > - break; > - case 0x01: /* set pci=bfsort */ > - smbios_type_b1_flag = 1; > - break; > - case 0x02: /* do not set pci=bfsort */ > - smbios_type_b1_flag = 2; > - break; > - default: > - break; > - } > + if ((((*(u32 *)data) >> 9) & 0x03) == 0x01) > + set_bf_sort((const struct dmi_system_id *)private_data); > } > > static int __init find_sort_method(const struct dmi_system_id *d) { > - dmi_walk(read_dmi_type_b1, NULL); > - > - if (smbios_type_b1_flag == 1) { > - set_bf_sort(d); > - return 0; > - } > - return -1; > + dmi_walk(read_dmi_type_b1, (void *)d); > + return 0; > } > > /* > > What do you think? Hi Jean, Thank you for reviewing and providing feedback. We will look into this and provide feedback. With regards, Narendra K
