On Fri, 15 Nov 2019 14:32:47 +0000 gengdongjiu <gengdong...@huawei.com> 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/ > >
