Re: [PATCH v2 1/1] MdePkg: Add new JedecJep106Lib to fetch JEDEC JEP106 manufacturer

["Rebecca Cran" <rebecca@bsdio.com>]

On 4/7/23 2:25 PM, Kinney, Michael D wrote:
> Comments below.
>
> Hopefully this lib would only be used by modules that get compressed.
I guess so, but that's for the user to decide.
>
> Might add GLOBAL_REMOVE_IF_UNREFERENCED to the arrays of strings to
> help the optimizer remove the data that is not referenced.

Good idea - I'll add that.

>> +CONST CHAR8 *
>> +EFIAPI
>> +Jep106GetManufacturerName (
>> +  IN UINT8  Code,
>> +  IN UINT8  ContinuationBytes
>> +  )
>> +{
>> +  UINTN                      Index;
>> +  CONST JEDEC_MANUFACTURERS  *ManufacturersBank;
>> +
>> +  Index = 0;
>> +
>> +  if (ContinuationBytes >= JEP106_MANUFACTURERS_NUM_BANKS) {
>> +    ASSERT (0);
> Do you really want an ASSERT() from this?  If this is data from a DIMM
> I doubt we want platform to ASSERT().  Perhaps just return NULL?
I'll remove it. I added it for validation when I was writing the SPD 
parsing library.
>> +UINTN
>> +EFIAPI
>> +Jep106GetLongestManufacturerName (
>> +  VOID
>> +  )
> Why is this API needed?  Wouldn’t you really just need the
> longest of the ones present in the current boot to build the
> SMBIOS record?.  Not the longest of all in the banks?

I added it because it's useful to know the maximum possible size of the 
strings that could be in the SMBIOS Type 17 table before doing the SPD 
parsing.

I'll remove it for now, and if it's really needed I can add it back in.


-- 
Rebecca Cran