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

["Michael D Kinney" <michael.d.kinney@intel.com>]

Which struct?  That macro only applies to global variables, not structure declarations.

Mike


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Rebecca Cran
> Sent: Friday, April 7, 2023 3:59 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>
> Cc: Rebecca Cran <rebecca@quicinc.com>
> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add new JedecJep106Lib to fetch JEDEC JEP106 manufacturer
> 
> I guess GLOBAL_REMOVE_IF_UNREFERENCED should be added to the struct too?
> 
> 
> On 4/7/23 4:42 PM, Rebecca Cran wrote:
> > 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.
> >
> >
> 
> 
> 
>