On Thu, Oct 13, 2022 at 12:26:58PM -0400, Gregory Price wrote:
> Other than the nitpicks below, lgtm. Not sure if you need a sign off
> from me given the contributions:
>
> Signed-off-by: Gregory Price <[email protected]>
>
> > +/* If no cdat_table == NULL returns number of entries */
> > +static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> > + int dsmad_handle, MemoryRegion
> > *mr)
> > +{
> > + enum {
> > + DSMAS,
> > + DSLBIS0,
> > + DSLBIS1,
> > + DSLBIS2,
> > + DSLBIS3,
> > + DSEMTS,
> > + NUM_ENTRIES
> > + };
> // ...
> > + if (!cdat_table) {
> > + return NUM_ENTRIES;
> > + }
>
> the only thing that i would recommend is making this enum global (maybe
> renaming them CT3_CDAT_[ENTRY_NAME]) and using CT3_CDAT_NUM_ENTRIES
> directly in the function that allocates the cdat buffer itself.
Yes I think I agree here.
> I do
> understand the want to keep the enum and the code creating the entries
> co-located though, so i'm just nitpicking here i guess.
>
> Generally I dislike the pattern of passing a NULL into a function to get
> configuration data, and then calling that function again. This function
> wants to be named...
>
> ct3_build_cdat_entries_for_mr_or_get_table_size_if_cdat_is_null(...)
>
> to accurately describe what it does. Just kinda feels like an extra
> function call for no reason.
>
> But either way, it works, this is just a nitpick on my side.
>
> > +static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> > +{
> > + g_autofree CDATSubHeader **table = NULL;
> > + CXLType3Dev *ct3d = priv;
> > + MemoryRegion *volatile_mr;
> > + /* ... snip ... */
> > +}
>
> s/volatile/nonvolatile
