dmi type 0xB1 record - unknown flag

dmi type 0xB1 record - unknown flag

[Jean Delvare]

Hi all,

I see the following message in my kernel log:

dmi type 0xB1 record - unknown flag

This is on a Dell Optiplex 9020 workstation. I see the message comes
from:

static void __init read_dmi_type_b1(const struct dmi_header *dm,
                                    void *private_data)
{
	(...)
        switch (((*(u32 *)d) >> 9) & 0x03) {
        case 0x00:
                printk(KERN_INFO "dmi type 0xB1 record - unknown flag\n");
                break;

What is the value of this message? Is there anything which needs to be
done to properly support such systems?

Thanks,
-- 
Jean Delvare
SUSE L3 Support

RE: dmi type 0xB1 record - unknown flag

[Narendra.K]

> -----Original Message-----
> From: Jean Delvare [mailto:jdelvare@suse.de]
> Sent: Thursday, June 1, 2017 6:08 PM
> To: x86@kernel.org; linux-pci@vger.kernel.org
> Cc: K, Narendra <Narendra_K@Dell.com>; Hargrave, Jordan
> <Jordan_Hargrave@Dell.com>; Iyer, Shyam <Shyam_Iyer@Dell.com>; Bjorn
> Helgaas <bhelgaas@google.com>
> Subject: dmi type 0xB1 record - unknown flag
>=20
> Hi all,
>=20

Hi Jean,

> I see the following message in my kernel log:
>=20
> dmi type 0xB1 record - unknown flag
>=20
> This is on a Dell Optiplex 9020 workstation. I see the message comes
> from:
>=20
> static void __init read_dmi_type_b1(const struct dmi_header *dm,
>                                     void *private_data) {
> 	(...)
>         switch (((*(u32 *)d) >> 9) & 0x03) {
>         case 0x00:
>                 printk(KERN_INFO "dmi type 0xB1 record - unknown flag\n")=
;
>                 break;
>=20
> What is the value of this message? Is there anything which needs to be do=
ne
> to properly support such systems?

This function was added to avoid adding systems to the 'pciprobe_dmi_table'=
 and set breadth first sorting in a generic way.

This flag is a hint to indicate the sort method to be used. The value 0x01 =
indicates that PCI breadth first sort be used.
' find_sort_method' function checks if smbios_type_b1_flag is set to 1 and =
if yes, calls 'set_bf_sort '. This function sets ' pci_bf_sort' to 'pci_dmi=
_bf'.

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.

There is no additional handling required for such a system.

With regards,
Narendra K

Re: dmi type 0xB1 record - unknown flag

[Bjorn Helgaas]

On Thu, Jun 01, 2017 at 01:28:31PM +0000, Narendra.K@dell.com wrote:
> > -----Original Message-----
> > From: Jean Delvare [mailto:jdelvare@suse.de]
> > Sent: Thursday, June 1, 2017 6:08 PM
> > To: x86@kernel.org; linux-pci@vger.kernel.org
> > Cc: K, Narendra <Narendra_K@Dell.com>; Hargrave, Jordan
> > <Jordan_Hargrave@Dell.com>; Iyer, Shyam <Shyam_Iyer@Dell.com>; Bjorn
> > Helgaas <bhelgaas@google.com>
> > Subject: dmi type 0xB1 record - unknown flag
> > 
> > Hi all,
> > 
> 
> Hi Jean,
> 
> > I see the following message in my kernel log:
> > 
> > dmi type 0xB1 record - unknown flag
> > 
> > This is on a Dell Optiplex 9020 workstation. I see the message comes
> > from:
> > 
> > static void __init read_dmi_type_b1(const struct dmi_header *dm,
> >                                     void *private_data) {
> > 	(...)
> >         switch (((*(u32 *)d) >> 9) & 0x03) {
> >         case 0x00:
> >                 printk(KERN_INFO "dmi type 0xB1 record - unknown flag\n");
> >                 break;
> > 
> > What is the value of this message? Is there anything which needs
> > to be done to properly support such systems?
> 
> This function was added to avoid adding systems to the
> 'pciprobe_dmi_table' and set breadth first sorting in a generic way.
> 
> This flag is a hint to indicate the sort method to be used. The
> value 0x01 indicates that PCI breadth first sort be used.  '
> find_sort_method' function checks if smbios_type_b1_flag is set to 1
> and if yes, calls 'set_bf_sort '. This function sets ' pci_bf_sort'
> to 'pci_dmi_bf'.
> 
> 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.
> 
> There is no additional handling required for such a system.

That was added by 6e8af08dfa40 ("PCI: enable pci=bfsort by default on
future Dell systems").

The fact that BIOS supplied a B1 record with an unknown value (0)
looks like a Dell BIOS defect, which is ironic, considering that Dell
added this thing in the first place.

I think we should remove the message, since it is alarming to users
and they can't do anything about it anyway.

Bjorn

RE: dmi type 0xB1 record - unknown flag

[Narendra.K]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Thursday, June 1, 2017 10:30 PM
> To: K, Narendra <Narendra_K@Dell.com>
> Cc: jdelvare@suse.de; 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.
> >
> > There is no additional handling required for such a system.
>=20
> That was added by 6e8af08dfa40 ("PCI: enable pci=3Dbfsort by default on
> future Dell systems").
>=20
> The fact that BIOS supplied a B1 record with an unknown value (0) looks l=
ike a
> Dell BIOS defect, which is ironic, considering that Dell added this thing=
 in the
> first place.
>=20
> I think we should remove the message, since it is alarming to users and t=
hey
> can't do anything about it anyway.
>=20

Hi Bjorn,

Thank you for the feedback. We will submit a patch to remove the message.=20

With regards,
Narendra K

Re: dmi type 0xB1 record - unknown flag

[Jean Delvare]

Hi Narendra,

Thanks for your answer.

On Thu, 1 Jun 2017 13:28:31 +0000, Narendra.K@dell.com wrote:
> > -----Original Message-----
> > From: Jean Delvare [mailto:jdelvare@suse.de]
> > I see the following message in my kernel log:
> > 
> > dmi type 0xB1 record - unknown flag
> > 
> > This is on a Dell Optiplex 9020 workstation. I see the message comes
> > from:
> > 
> > static void __init read_dmi_type_b1(const struct dmi_header *dm,
> >                                     void *private_data) {
> > 	(...)
> >         switch (((*(u32 *)d) >> 9) & 0x03) {
> >         case 0x00:
> >                 printk(KERN_INFO "dmi type 0xB1 record - unknown flag\n");
> >                 break;
> > 
> > What is the value of this message? Is there anything which needs to be done
> > to properly support such systems?  
> 
> This function was added to avoid adding systems to the 'pciprobe_dmi_table' and set breadth first sorting in a generic way.
> 
> This flag is a hint to indicate the sort method to be used. The value 0x01 indicates that PCI breadth first sort be used.
> ' find_sort_method' function checks if smbios_type_b1_flag is set to 1 and if yes, calls 'set_bf_sort '. This function sets ' pci_bf_sort' to 'pci_dmi_bf'.

I can read the code, thank you ;-)

> 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?

Thanks,
-- 
Jean Delvare
SUSE L3 Support

RE: dmi type 0xB1 record - unknown flag

[Narendra.K]

> -----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 me=
thod
> will be the default that is decided by the kernel.
>=20
> How can it be invalid? I have DMI dumps of several Dell systems with a ty=
pe
> 0xB1 DMI structure, with value 0x00 for these bits. If Dell ships such sy=
stems,
> then this is valid by definition.
>=20
> > There is no additional handling required for such a system.
>=20
> I don't understand the complexity of the code. There are 4 possible value=
s
> 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.
>=20
> 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 pre=
sent;
> but I don't think this is supposed to happen in the first place.
>=20
> Lastly I'm not sure why you continue processing the list of DMI matches,
> when smbios_type_b1_flag =3D=3D 1, and stop processing it if not.
> This seems needlessly inconsistent.
>=20
> I think the whole thing can be simplified like this:
>=20
> From: Jean Delvare <jdelvare@suse.de>
> Subject: x86/PCI: Simplify Dell DMI B1 quirk
>=20
> No need for such convoluted code, when all we need is to call one functio=
n in
> one specific case.
>=20
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> ---
>  arch/x86/pci/common.c |   27 +++++----------------------
>  1 file changed, 5 insertions(+), 22 deletions(-)
>=20
> --- 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 =3D PCI_PROBE_BIOS
>=20
>  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 vo=
id
> __init read_dmi_type_b1(const struct dmi_header *dm,
>  				    void *private_data)
>  {
> -	u8 *d =3D (u8 *)dm + 4;
> +	u8 *data =3D (u8 *)dm + 4;
>=20
>  	if (dm->type !=3D 0xB1)
>  		return;
> -	switch (((*(u32 *)d) >> 9) & 0x03) {
> -	case 0x00:
> -		printk(KERN_INFO "dmi type 0xB1 record - unknown flag\n");
> -		break;
> -	case 0x01: /* set pci=3Dbfsort */
> -		smbios_type_b1_flag =3D 1;
> -		break;
> -	case 0x02: /* do not set pci=3Dbfsort */
> -		smbios_type_b1_flag =3D 2;
> -		break;
> -	default:
> -		break;
> -	}
> +	if ((((*(u32 *)data) >> 9) & 0x03) =3D=3D 0x01)
> +		set_bf_sort((const struct dmi_system_id *)private_data);
>  }
>=20
>  static int __init find_sort_method(const struct dmi_system_id *d)  {
> -	dmi_walk(read_dmi_type_b1, NULL);
> -
> -	if (smbios_type_b1_flag =3D=3D 1) {
> -		set_bf_sort(d);
> -		return 0;
> -	}
> -	return -1;
> +	dmi_walk(read_dmi_type_b1, (void *)d);
> +	return 0;
>  }
>=20
>  /*
>=20
> 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

Re: dmi type 0xB1 record - unknown flag

[Bjorn Helgaas]

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

RE: dmi type 0xB1 record - unknown flag

[Narendra.K]

> -----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
>=20
> 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>
>=20
> Applied to pci/misc for v4.13, thanks!
>=20
> 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.
>=20

Hi Jean/Bjorn,

Sorry for the delay. I applied the patch to the mainline kernel version 4.1=
2-rc5 and performed sanity test on  a  DellEMC PowerEdge R730XD system. It =
is working as=20
expected.  'dmesg' shows the message that 'pci=3Dbfsort' is enabled.

I also booted kernel version 4.12-rc5 with patch applied on a DellEMC Power=
Edge 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. 'dm=
esg' shows the message that 'pci=3Dbfsort' is enabled.

[...]
> >
> > What do you think?
=20
Thank you for simplifying the code and for the patch.=20

With regards,
Narendra K

Re: dmi type 0xB1 record - unknown flag

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support