Re: [RESEND PATCH v21 3/6] ACPI: Add APEI GHES table generation support

[Igor Mammedov]

On Fri, 15 Nov 2019 14:32:47 +0000
gengdongjiu <address@hidden> wrote:

> > > + */
> > > +static void acpi_ghes_build_notify(GArray *table, const uint8_t type)  
> > 
> > typically format should be build_WHAT(), so
> >  build_ghes_hw_error_notification()
> > 
> > And I'd move this out into its own patch.
> > this applies to other trivial in-depended sub-tables, that take all data 
> > needed to construct them from supplied arguments.  
> 
> I very used your suggested method in previous series[1], but other maintainer 
> suggested to move this function to this file, because he think only GHES used 
> it

Using this file is ok, what I've meant for you to split function from
this patch into a separate smaller patch.

With the way code split now I have to review 2 big complex patches at
the same which makes reviewing hard and I have a hard time convincing
myself that code it correct.

Moving small self-contained chunks of code in to separate smaller patches
makes review easier.

> 
> [1]:
> https://patchwork.ozlabs.org/cover/1099428/
> 
> >   
> > > +{
> > > +        /* Type */
> > > +        build_append_int_noprefix(table, type, 1);
> > > +        /*
> > > +         * Length:
> > > +         * Total length of the structure in bytes
> > > +         */
> > > +        build_append_int_noprefix(table, 28, 1);
> > > +        /* Configuration Write Enable */
> > > +        build_append_int_noprefix(table, 0, 2);
> > > +        /* Poll Interval */
> > > +        build_append_int_noprefix(table, 0, 4);
> > > +        /* Vector */
> > > +        build_append_int_noprefix(table, 0, 4);
> > > +        /* Switch To Polling Threshold Value */
> > > +        build_append_int_noprefix(table, 0, 4);
> > > +        /* Switch To Polling Threshold Window */
> > > +        build_append_int_noprefix(table, 0, 4);
> > > +        /* Error Threshold Value */
> > > +        build_append_int_noprefix(table, 0, 4);
> > > +        /* Error Threshold Window */
> > > +        build_append_int_noprefix(table, 0, 4); }
> > > +  
> > 
> > /*
> >   Initialize "etc/hardware_errors" and "etc/hardware_errors_addr" fwcfg 
> > blobs.
> >   See docs/specs/acpi_hest_ghes.rst for blobs format */  
> > > +void acpi_ghes_build_error_table(GArray *hardware_errors, BIOSLinker
> > > +*linker)  
> > build_ghes_error_table()
> > 
> > also I'd move this function into its own patch along with other related 
> > code that initializes and wires it into virt board.  
> 
> I ever use your suggested method[1], but other maintainer, it seems Michael, 
> suggested to move these functions to this file that used it, because he think 
> only GHES used it.
> 
> [1]:
> https://patchwork.ozlabs.org/patch/1099424/
> https://patchwork.ozlabs.org/patch/1099425/
> https://patchwork.ozlabs.org/patch/1099430/
> 
>