patchdmidecode - Patches: patch #9642, ThinkPad-specific patches

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #9642: ThinkPad-specific patches

Submitter:  Joe <c0d3st0rm>
Submitted:  Sat 26 May 2018 10:52:02 PM UTC
   
 
Category:  None Priority:  4
Status:  Done Privacy:  Public
Assigned to:  khali Open/Closed:  Closed

Sun 02 Sep 2018 08:40:04 PM UTC, comment #4: 
Jean Delvare <khali>
Group administrator
Thu 09 Aug 2018 06:29:58 PM UTC, comment #3: 

I reworked your patch based on my own comments, and posted the result on the list:

http://lists.nongnu.org/archive/html/dmidecode-devel/2018-08/msg00028.html

Jean Delvare <khali>
Group administrator
Wed 04 Jul 2018 04:04:43 PM UTC, comment #2: 

Type 135:

For this one too, I have multiple records with various lengths in my samples. The one documented in the Compal manual is only one of the possible subtypes (OEM structure number = 0x03).

Your test for the "TP" signature is incorrect, it only fails if both bytes are wrong, while it should fail if either is wrong.

This:

> oem.data = data + offset;

looks wrong to me. This means that oem.data points to the header of the OEM structure. However later in the code you try to access byte at global offset 0x09 as oem.data[0], which would work if oem.data points to the OEM structure payload, not header.

This test:

> if (oem.num == 0x01)

is wrong, you actually want to check oem.rev, not oem.num (which is already checked on the previous line).

This test:

> if (oem.len != 3)

seems too strict to me, anything >= 3 is OK, and you have already tested for that earlier.

This:

> ((oem.data[0] >> 7) & 1)

is testing the wrong bit (MSB), the fingerprint device is bit 0 (LSB).

After fixing all this, the code finally reports "Fingerprint sensor: Present" on 3 samples I have.

By the way, the usual term is "Fingerprint Reader", not "Fingerprint sensor".

Type 140:

The handle is expected to vary, it is an arbitrary unique identifier with no special meaning. Same holds for every other record so no need to mention it.

Hard-coding

> \x0b\x07\x01\x01\x02

is pretty harsh. You can easily imagine that these bytes have a meaning. In fact I believe that the structure of type 140 is very close to the structure of type 135:

  • The signature is "LENOVO" instead of "TP".
  • Follows the offset to the OEM structure, which is 11 instead of 7 because "LENOVO" is 4 bytes longer than "TP".
  • Follows the 2-byte OEM header, Number = 0x07 and Revision = 0x01 in our case. I have type 140 records with other Number values in my samples, from 0x00 to 0x08.
  • Lastly we have 2 bytes of payload for this specific subtype, which appear to be DMI string references, 1 for the Version ID and 2 for the Release Date.


It would be much better to express this with code, as you did for type 135, than blindly matching an arbitrary binary string.

No need to explicitly check for the presence of DMI strings at first, we know they will be there, and the rest of the code would survive just fine in case they are not.

No C++-style comments please.

Jean Delvare <khali>
Group administrator
Wed 04 Jul 2018 02:56:35 PM UTC, comment #1: 

This is not really the best interface for a patch review, the mailing list is much more convenient for the purpose. I'll still try...

General comments first:

You don't need to "export" bad_index. strcmp() will fail naturally if the index was bad, no need to special-case it. There is also no need to explicitly test for bad_index before printing strings, if they were bad then "<BAD INDEX>" will be displayed, as is the case everywhere else in dmidecode.

Please respect the coding style used in the rest of the code (specifically the placement of curly braces). No typedef please, especially for a struct you use only once. I don't even get the point of the structure, to be honest, we did not define a struct for any other DMI type. No blocks around cases in switch, and declare your variables early. (We aim at maximum portability.)

I'm not strict in the 80-columns rule, but please avoid overlong lines.

The patch adds the following warning:

dmioem.c: In function ‘dmi_decode_ibm_lenovo’:
dmioem.c:296:6: warning: unused variable ‘cap’ [-Wunused-variable]
  u16 cap;
      ^

That's easy to fix, and you should not post patches which add warnings.

Type 131:

I admit I'm curious why a Compal manual contains information about IBM/Lenovo-specific DMI types... Oh well.

Checking the version would seem safer, as presumably the rest of the structure as documented is only valid for version 1.

  ((data[0x15] & 1) == 1)
can be written more simply
  (data[0x15] & 0x01)
when used as a boolean value.

Lastly, your definition of the type 131 record is incomplete. You define a total of 21 bytes (4 for header, 1 for version, 16 for feature bits) but the total length is 22 bytes. There is one more byte at the end of the record (offset 0x15), which is the reference to the DMI string with index 1 ("TVT-Enablement"). The last feature byte is at offset 0x14, not 0x15, so your code is testing the wrong bit.

Unfortunately, the document doesn't make it clear in which order the bits are being counted, so I would not be able to tell for sure which is "bit 127". In all my samples, all the feature bits are set to 0 anyway, which seriously limits both my testing capabilities and the interest of decoding this record at all. Do you have any system where any feature bit is actually set?

For completeness, note that in my samples I have several type 131 records on IBM/Lenovo machines with length different from 22 (I have seen 11, 17 and 64) and different strings as well (like "IBM System Metrics" for length 11 for example).

Jean Delvare <khali>
Group administrator
Sat 26 May 2018 10:52:02 PM UTC, original submission:  

Hi,

This patch introduces the following ThinkPad-specific DMI entries, which have been either sourced from manuals online or assumed:

  • ThinkVantage Technologies feature bits (a somewhat outdated entry?)
  • Device Presence Detection bits (unable to test this one, taken from the referenced source)
  • ThinkPad Embedded Controller firmware version
Joe <c0d3st0rm>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #44223:  dmidecode_thinkpad_oem_types.patch added by c0d3st0rm (7KiB - text/x-patch - Patch based on latest sources.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by khali (Posted a comment)
  • -email is unavailable- added by c0d3st0rm (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-09-02 khali StatusIn Progress Done
        Open/ClosedOpen Closed
    2018-07-04 khali Priority5 - Normal 4
        StatusNone In Progress
        Assigned toNone khali
    2018-05-26 c0d3st0rm Carbon-CopyRemoved khali -
    2018-05-26 c0d3st0rm Attached File- Added dmidecode_thinkpad_oem_types.patch, #44223
        Carbon-Copy- Added khali

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code