Re: [PATCH 08/15] acpi/ghes: Prepare to support multiple sources on ghes

[Mauro Carvalho Chehab]

Em Wed, 25 Sep 2024 15:23:33 +0100
Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> escreveu:

> On Wed, 25 Sep 2024 06:04:13 +0200
> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> 
> > The current code is actually dependent on having just one
> > error structure with a single source.
> > 
> > As the number of sources should be arch-dependent, as it
> > will depend on what kind of synchronous/assynchronous
> > notifications will exist, change the logic to dynamically
> > build the table.  
> Not really arch dependent.  Depends on both arch and some
> firmware implementation choices, but I guess that detail
> doesn't matter here.
> 
> > 
> > Yet, for a proper support, we need to get the number of
> > sources by reading the number from the HEST table. However,
> > bios currently doesn't store a pointer to it.
> > 
> > For now just change the logic at table build time, while
> > enforcing that it will behave like before with a single
> > source ID.
> > 
> > A future patch will add a HEST table bios pointer and
> > change the logic at acpi_ghes_record_errors() to
> > dynamically use the new size.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>  
> Trivial comment inline
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> 
> > @@ -335,9 +346,10 @@ static void build_ghes_v2(GArray *table_data,
> >      build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0,
> >                       4 /* QWord access */, 0);
> >      bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> > -        address_offset + GAS_ADDR_OFFSET,
> > -        sizeof(uint64_t), ACPI_GHES_ERRORS_FW_CFG_FILE,
> > -        (ACPI_GHES_ERROR_SOURCE_COUNT + source_id) * sizeof(uint64_t));
> > +                                   address_offset + GAS_ADDR_OFFSET,  
> 
> I'd prefer if we avoided realigning unless absolutely necessary or
> that it is split into a separate patch.
> Makes things a tiny bit harder to review.

Heh, Igor nacked a patch doing the alignment change on a separate patch,
so let's do it at the patches that are actually changing the code.

At least for me, it is a low easier to review patches that are properly
aligned with parenthesis. So, yeah it may be a little more painful to
review a patch changing alignments, but IMO it pays off on future
revisions, specially if we place one argument per line, like in this
function.

> 
> > +                                   sizeof(uint64_t),
> > +                                   ACPI_GHES_ERRORS_FW_CFG_FILE,
> > +                                   (num_sources + index) * sizeof(uint64_t));
> >    
> 



Thanks,
Mauro