[1/2] dmi: dmicheck: Add BMC Interface Type definitions from SMBIOS spec

Message ID 1505165777-17121-1-git-send-email-jhugo@codeaurora.org
State Superseded
Headers show
Series
  • [1/2] dmi: dmicheck: Add BMC Interface Type definitions from SMBIOS spec
Related show

Commit Message

Jeffrey Hugo Sept. 11, 2017, 9:36 p.m.
The SMBIOS spec defines BMC Interface Types in section 7.39.1.  Value 0 is
defined as "Unknown" (might be a platform specific interface), and value 3
is "BT: Block Transfer".  Update the test to accept these definitions.

Fixes: 00445adbcb5a ("dmi: dmicheck: add SBBR compliance tests")
Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 src/dmi/dmicheck/dmicheck.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alex Hung Sept. 12, 2017, 5:04 a.m. | #1
On 2017-09-11 02:36 PM, Jeffrey Hugo wrote:
> The SMBIOS spec defines BMC Interface Types in section 7.39.1.  Value 0 is
> defined as "Unknown" (might be a platform specific interface), and value 3
> is "BT: Block Transfer".  Update the test to accept these definitions.
> 
> Fixes: 00445adbcb5a ("dmi: dmicheck: add SBBR compliance tests")
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
>   src/dmi/dmicheck/dmicheck.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c
> index 61c6b81..c0e78f0 100644
> --- a/src/dmi/dmicheck/dmicheck.c
> +++ b/src/dmi/dmicheck/dmicheck.c
> @@ -1772,7 +1772,7 @@ static void dmicheck_entry(fwts_framework *fw,
>   
>   			dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 2, 2);
>   			dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 5, 5);
> -			dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr Modifier/Interrupt Info)", hdr, 0x10, 0x1, 0x2, 6, 0x3);
> +			dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr Modifier/Interrupt Info)", hdr, 0x10, 0x0, 0x3, 6, 0x3);
>   			break;
>   
>   		case 39: /* 7.40 */
> 


Acked-by: Alex Hung <alex.hung@canonical.com>
Sakar Arora Sept. 12, 2017, 5:21 a.m. | #2
-----Original Message-----
From: fwts-devel [mailto:fwts-devel-bounces@lists.ubuntu.com] On Behalf Of Alex Hung

Sent: Tuesday, September 12, 2017 10:34 AM
To: fwts-devel@lists.ubuntu.com
Subject: ACK: [PATCH 1/2] dmi: dmicheck: Add BMC Interface Type definitions from SMBIOS spec

On 2017-09-11 02:36 PM, Jeffrey Hugo wrote:
> The SMBIOS spec defines BMC Interface Types in section 7.39.1.  Value

> 0 is defined as "Unknown" (might be a platform specific interface),

> and value 3 is "BT: Block Transfer".  Update the test to accept these definitions.

>

> Fixes: 00445adbcb5a ("dmi: dmicheck: add SBBR compliance tests")

> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>

> ---

>   src/dmi/dmicheck/dmicheck.c | 2 +-

>   1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c

> index 61c6b81..c0e78f0 100644

> --- a/src/dmi/dmicheck/dmicheck.c

> +++ b/src/dmi/dmicheck/dmicheck.c

> @@ -1772,7 +1772,7 @@ static void dmicheck_entry(fwts_framework *fw,

>

>   dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 2, 2);

>   dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 5, 5);

> -dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr Modifier/Interrupt Info)", hdr, 0x10, 0x1, 0x2, 6, 0x3);

> +dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr

> +Modifier/Interrupt Info)", hdr, 0x10, 0x0, 0x3, 6, 0x3);


The above test checks the range of the bits 6:7 of Base Addr Modifier info. As per SMBIOS spec that value 0x3 is reserved for these 2 bits. Hence the accepted values should be 0x0 to 0x2. The BMC interface type field is at offset 0x4 which is being correctly tested for range 0x0 to 0x3 in the first line of this switch case block.

dmi_min_max_uint8_check(fw, table, addr, "Interface Type", hdr, 0x4, 0x0, 0x3);

IMO this change re-introduces the bug, that was fixed in : 00445adbcb5a ("dmi: dmicheck: add SBBR compliance tests")

>   break;

>

>   case 39: /* 7.40 */

>



Acked-by: Alex Hung <alex.hung@canonical.com>


--
fwts-devel mailing list
fwts-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Colin King Sept. 12, 2017, 8 a.m. | #3
On 11/09/17 22:36, Jeffrey Hugo wrote:
> The SMBIOS spec defines BMC Interface Types in section 7.39.1.  Value 0 is
> defined as "Unknown" (might be a platform specific interface), and value 3
> is "BT: Block Transfer".  Update the test to accept these definitions.
> 
> Fixes: 00445adbcb5a ("dmi: dmicheck: add SBBR compliance tests")
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
>  src/dmi/dmicheck/dmicheck.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c
> index 61c6b81..c0e78f0 100644
> --- a/src/dmi/dmicheck/dmicheck.c
> +++ b/src/dmi/dmicheck/dmicheck.c
> @@ -1772,7 +1772,7 @@ static void dmicheck_entry(fwts_framework *fw,
>  
>  			dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 2, 2);
>  			dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 5, 5);
> -			dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr Modifier/Interrupt Info)", hdr, 0x10, 0x1, 0x2, 6, 0x3);
> +			dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr Modifier/Interrupt Info)", hdr, 0x10, 0x0, 0x3, 6, 0x3);
>  			break;
>  
>  		case 39: /* 7.40 */
> 
Acked-by: Colin Ian King <colin.king@canonical.com>
Sakar Arora Sept. 12, 2017, 11:04 a.m. | #4
Hi,

My two cents on this patch. Any comments?

Thanks,
Sakar

-----Original Message-----
From: fwts-devel [mailto:fwts-devel-bounces@lists.ubuntu.com] On Behalf Of Colin Ian King

Sent: Tuesday, September 12, 2017 1:31 PM
To: fwts-devel@lists.ubuntu.com
Subject: ACK: [PATCH 1/2] dmi: dmicheck: Add BMC Interface Type definitions from SMBIOS spec

On 11/09/17 22:36, Jeffrey Hugo wrote:
> The SMBIOS spec defines BMC Interface Types in section 7.39.1.  Value

> 0 is defined as "Unknown" (might be a platform specific interface),

> and value 3 is "BT: Block Transfer".  Update the test to accept these definitions.

>

> Fixes: 00445adbcb5a ("dmi: dmicheck: add SBBR compliance tests")

> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>

> ---

>  src/dmi/dmicheck/dmicheck.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c

> index 61c6b81..c0e78f0 100644

> --- a/src/dmi/dmicheck/dmicheck.c

> +++ b/src/dmi/dmicheck/dmicheck.c

> @@ -1772,7 +1772,7 @@ static void dmicheck_entry(fwts_framework *fw,

>

>  dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 2, 2);

>  dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 5, 5);

> -dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr Modifier/Interrupt Info)", hdr, 0x10, 0x1, 0x2, 6, 0x3);

> +dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr

> +Modifier/Interrupt Info)", hdr, 0x10, 0x0, 0x3, 6, 0x3);


The above test checks the range of the bits 6:7 of Base Addr Modifier/Interrupt Info at offset 0x10. As per SMBIOS spec that value 0x3 is reserved for these 2 bits. Hence the accepted values should be 0x0 to 0x2. The BMC interface type field is at offset 0x4 which is being correctly tested for range 0x0 to 0x3 in the first line of this switch case block.

dmi_min_max_uint8_check(fw, table, addr, "Interface Type", hdr, 0x4, 0x0, 0x3);

This change re-introduces the bug, that was fixed in : 00445adbcb5a ("dmi: dmicheck: add SBBR compliance tests")

>  break;

>

>  case 39: /* 7.40 */

>

Acked-by: Colin Ian King <colin.king@canonical.com>


--
fwts-devel mailing list
fwts-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Jeffrey Hugo Sept. 12, 2017, 1:45 p.m. | #5
On 9/12/2017 5:04 AM, Sakar Arora wrote:
> Hi,
> 
> My two cents on this patch. Any comments?
> 
> Thanks,
> Sakar
> 
> -----Original Message-----
> From: fwts-devel [mailto:fwts-devel-bounces@lists.ubuntu.com] On Behalf Of Colin Ian King
> Sent: Tuesday, September 12, 2017 1:31 PM
> To: fwts-devel@lists.ubuntu.com
> Subject: ACK: [PATCH 1/2] dmi: dmicheck: Add BMC Interface Type definitions from SMBIOS spec
> 
> On 11/09/17 22:36, Jeffrey Hugo wrote:
>> The SMBIOS spec defines BMC Interface Types in section 7.39.1.  Value
>> 0 is defined as "Unknown" (might be a platform specific interface),
>> and value 3 is "BT: Block Transfer".  Update the test to accept these definitions.
>>
>> Fixes: 00445adbcb5a ("dmi: dmicheck: add SBBR compliance tests")
>> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
>> ---
>>   src/dmi/dmicheck/dmicheck.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c
>> index 61c6b81..c0e78f0 100644
>> --- a/src/dmi/dmicheck/dmicheck.c
>> +++ b/src/dmi/dmicheck/dmicheck.c
>> @@ -1772,7 +1772,7 @@ static void dmicheck_entry(fwts_framework *fw,
>>
>>   dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 2, 2);
>>   dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 5, 5);
>> -dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr Modifier/Interrupt Info)", hdr, 0x10, 0x1, 0x2, 6, 0x3);
>> +dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr
>> +Modifier/Interrupt Info)", hdr, 0x10, 0x0, 0x3, 6, 0x3);
> 
> The above test checks the range of the bits 6:7 of Base Addr Modifier/Interrupt Info at offset 0x10. As per SMBIOS spec that value 0x3 is reserved for these 2 bits. Hence the accepted values should be 0x0 to 0x2. The BMC interface type field is at offset 0x4 which is being correctly tested for range 0x0 to 0x3 in the first line of this switch case block.
> 
> dmi_min_max_uint8_check(fw, table, addr, "Interface Type", hdr, 0x4, 0x0, 0x3);
> 
> This change re-introduces the bug, that was fixed in : 00445adbcb5a ("dmi: dmicheck: add SBBR compliance tests")

You are right, partially.  3 is not valid here, but 0 is, which you 
didn't fix last time (and your commit message wasn't clear exactly about 
what you were fixing).

I'll rework this series.

Patch

diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c
index 61c6b81..c0e78f0 100644
--- a/src/dmi/dmicheck/dmicheck.c
+++ b/src/dmi/dmicheck/dmicheck.c
@@ -1772,7 +1772,7 @@  static void dmicheck_entry(fwts_framework *fw,
 
 			dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 2, 2);
 			dmi_reserved_bits_check(fw, table, addr, "Base Addr Modifier/Interrupt Info", hdr, sizeof(uint8_t), 0x10, 5, 5);
-			dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr Modifier/Interrupt Info)", hdr, 0x10, 0x1, 0x2, 6, 0x3);
+			dmi_min_max_mask_uint8_check(fw, table, addr, "Base Addr Modifier/Interrupt Info)", hdr, 0x10, 0x0, 0x3, 6, 0x3);
 			break;
 
 		case 39: /* 7.40 */