| Message ID | 20170602161311.27eab72d@endymion (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
> -----Original Message----- > From: Jean Delvare [mailto:jdelvare@suse.de] > Sent: Friday, June 2, 2017 7:43 PM > To: K, Narendra <Narendra_K@Dell.com> > Cc: x86@kernel.org; linux-pci@vger.kernel.org; Hargrave, Jordan > <Jordan_Hargrave@Dell.com>; Iyer, Shyam <Shyam_Iyer@Dell.com>; > bhelgaas@google.com > 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@suse.de> > 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@suse.de> > --- > 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
On Fri, Jun 02, 2017 at 04:13:11PM +0200, Jean Delvare wrote: > ... > I think the whole thing can be simplified like this: > > From: Jean Delvare <jdelvare@suse.de> > 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@suse.de> Applied to pci/misc for v4.13, thanks! As far as I can tell, the new code is functionally equivalent to the old code, so I don't think there's any risk here. > --- > 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? > > Thanks, > -- > Jean Delvare > SUSE L3 Support
> -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: Friday, June 16, 2017 3:10 AM > To: Jean Delvare <jdelvare@suse.de> > Cc: K, Narendra <Narendra_K@Dell.com>; x86@kernel.org; linux- > pci@vger.kernel.org; Hargrave, Jordan <Jordan_Hargrave@Dell.com>; Iyer, > Shyam <Shyam_Iyer@Dell.com>; bhelgaas@google.com > Subject: Re: dmi type 0xB1 record - unknown flag > > On Fri, Jun 02, 2017 at 04:13:11PM +0200, Jean Delvare wrote: > > ... > > I think the whole thing can be simplified like this: > > > > From: Jean Delvare <jdelvare@suse.de> > > 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@suse.de> > > Applied to pci/misc for v4.13, thanks! > > As far as I can tell, the new code is functionally equivalent to the old code, so > I don't think there's any risk here. > Hi Jean/Bjorn, Sorry for the delay. I applied the patch to the mainline kernel version 4.12-rc5 and performed sanity test on a DellEMC PowerEdge R730XD system. It is working as expected. 'dmesg' shows the message that 'pci=bfsort' is enabled. I also booted kernel version 4.12-rc5 with patch applied on a DellEMC PowerEdge 1950 system which is listed in the ' pciprobe_dmi_table' quicks table. It is working as expected and has not caused regression on this system. 'dmesg' shows the message that 'pci=bfsort' is enabled. [...] > > > > What do you think? Thank you for simplifying the code and for the patch. With regards, Narendra K
On ven., 2017-06-16 at 09:36 +0000, Narendra.K@dell.com wrote: > Sorry for the delay. I applied the patch to the mainline kernel version 4.12-rc5 and performed sanity test on a DellEMC PowerEdge R730XD system. It is working as > expected. 'dmesg' shows the message that 'pci=bfsort' is enabled. > > I also booted kernel version 4.12-rc5 with patch applied on a DellEMC PowerEdge 1950 system which is listed in the ' pciprobe_dmi_table' quicks table. > It is working as expected and has not caused regression on this system. 'dmesg' shows the message that 'pci=bfsort' is enabled. Thank you for testing it on real systems, this is very useful and appreciated.
--- 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; } /*