Re: [PATCH v3 4/5] acpi/nvdimm: Implement ACPI NVDIMM Label Methods

[Igor Mammedov]

On Fri, 16 Sep 2022 10:27:08 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> On Fri, 2022-09-09 at 15:39 +0200, Igor Mammedov wrote:
> ...
> > looks more or less fine except of excessive use of named variables
> > which creates global scope variables.
> > 
> > I'd suggest to store temporary buffers/packages in LocalX variales,
> > you should be able to do that for everything modulo
> > aml_create_dword_field().
> > 
> > see an example below
> >   
> ...
> > >  
> > > +        /*
> > > +         * ACPI v6.4: Section 6.5.10 NVDIMM Label Methods
> > > +         */
> > > +        /* _LSI */
> > > +        method = aml_method("_LSI", 0, AML_SERIALIZED);
> > > +        com_call = aml_call5(NVDIMM_COMMON_DSM,
> > > +                            aml_touuid(NVDIMM_DEVICE_DSM_UUID),
> > > +                            aml_int(1), aml_int(4), aml_int(0),
> > > +                            aml_int(handle));
> > > +        aml_append(method, aml_store(com_call, aml_local(0)));
> > > +
> > > +        aml_append(method, aml_create_dword_field(aml_local(0),
> > > +                                                  aml_int(0),
> > > "STTS"));
> > > +        aml_append(method, aml_create_dword_field(aml_local(0),
> > > aml_int(4),
> > > +                                                  "SLSA"));
> > > +        aml_append(method, aml_create_dword_field(aml_local(0),
> > > aml_int(8),
> > > +                                                  "MAXT"));
> > > +
> > > +        pkg = aml_package(3);
> > > +        aml_append(pkg, aml_name("STTS"));
> > > +        aml_append(pkg, aml_name("SLSA"));
> > > +        aml_append(pkg, aml_name("MAXT"));
> > > +        aml_append(method, aml_name_decl("RET", pkg));  
> > 
> > ex: put it in local instead of named variable and return that
> > the same applies to other named temporary named variables.
> >   
> Fine, get your point now.
> In ASL it will look like this:
>                     Local1 = Package (0x3) {STTS, SLSA, MAXT}
>                     Return (Local1)


> 
> But as for 
>                     CreateDWordField (Local0, Zero, STTS)  // Status
>                     CreateDWordField (Local0, 0x04, SLSA)  // SizeofLSA
>                     CreateDWordField (Local0, 0x08, MAXT)  // Max Trans
> 
> I cannot figure out how to substitute with LocalX. Can you shed more
> light?

Leave this as is, there is no way to make it anonymous/local with FooField.

(well one might try to use Index and copy field's bytes into a buffer and
then explicitly convert to Integer, but that's a rather convoluted way
around limitation so I'd not go this route)

> 
> CreateQWordFieldTerm :=
> CreateQWordField (
> SourceBuffer, // TermArg => Buffer
> ByteIndex, // TermArg => Integer
> QWordFieldName // NameString
> )
> NameString :=
> <RootChar NamePath> | <ParentPrefixChar PrefixPath NamePath> |
> NonEmptyNamePath
> 
> > > +        aml_append(method, aml_return(aml_name("RET")));
> > > +  
> ...
> > > +        field = aml_create_dword_field(aml_local(3), aml_int(0),
> > > "STTS");
> > > +        aml_append(method, field);
> > > +        aml_append(method,
> > > aml_return(aml_to_integer(aml_name("STTS"))));  
> > 
> > why do you need explicitly convert DWORD field to integer?
> > it should be fine to return STTS directly (implicit conversion should
> > take care of the rest)  
> 
> Explicit convert eases my anxiety on uncertainty. ;)
> 
> >   
> > > +        aml_append(nvdimm_dev, method);
> > > +  
> ...
>