| Message ID | 20221012182120.174142-4-gregory.price@memverge.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] hw/mem/cxl_type3: fix checkpatch errors | expand |
On Wed, 12 Oct 2022 14:21:18 -0400 Gregory Price <gourry.memverge@gmail.com> wrote: > Makes the size of the allocated cdat table static (6 entries), > flattens the code, and reduces the number of exit conditions > > Signed-off-by: Gregory Price <gregory.price@memverge.com> Hmm. I don't entirely like this as it stands because it leads to more fragile code as we don't have clear association between number of entries and actual assignments. So, what I've done (inspired by this) is moved to a local enum in the factored out building function that has an element for each of the entries (used ultimately to assign them) and a trailing NUM_ENTRIES element we can then use in place of the CT3_CDAT_SUBTABLE_SIZE define you have here. I went with the 2 pass approach mentioned in a later patch, so if cdat_table passed to the factored out code is NULL, we just return NUM_ENTRIES directly. > --- > hw/mem/cxl_type3.c | 52 ++++++++++++++++++++-------------------------- > 1 file changed, 22 insertions(+), 30 deletions(-) > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 43b2b9e041..0e0ea70387 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -17,6 +17,7 @@ > #include "hw/pci/msix.h" > > #define DWORD_BYTE 4 > +#define CT3_CDAT_SUBTABLE_SIZE 6 > > static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > void *priv) > @@ -25,7 +26,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > g_autofree CDATDslbis *dslbis_nonvolatile = NULL; > g_autofree CDATDsemts *dsemts_nonvolatile = NULL; > CXLType3Dev *ct3d = priv; > - int len = 0; > int i = 0; > int next_dsmad_handle = 0; > int nonvolatile_dsmad = -1; > @@ -33,7 +33,7 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > MemoryRegion *mr; > > if (!ct3d->hostmem) { > - return len; > + return 0; > } > > mr = host_memory_backend_get_memory(ct3d->hostmem); > @@ -41,11 +41,22 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > return -EINVAL; > } > > + *cdat_table = g_malloc0(CT3_CDAT_SUBTABLE_SIZE * sizeof(*cdat_table)); > + if (!*cdat_table) { > + return -ENOMEM; > + } > + > /* Non volatile aspects */ > dsmas_nonvolatile = g_malloc(sizeof(*dsmas_nonvolatile)); > - if (!dsmas_nonvolatile) { > + dslbis_nonvolatile = > + g_malloc(sizeof(*dslbis_nonvolatile) * dslbis_nonvolatile_num); > + dsemts_nonvolatile = g_malloc(sizeof(*dsemts_nonvolatile)); > + if (!dsmas_nonvolatile || !dslbis_nonvolatile || !dsemts_nonvolatile) { I don't like aggregated error checking. It saves lines of code, but leads to generally less mantainable code. I prefer to do one thing, check it and handle necessary errors - provides a small localized chunk of code that is easy to review and maintain. 1. Allocate structure 2. Fill structure. We have to leave the assignment till later as only want to steal the pointers once we know there are no error paths. > + g_free(*cdat_table); We have auto free to clean this up. So if this did make sense, use a local g_autofree CDATSubHeader **cdat_table = NULL; and steal the pointer when assigning *cdat_table at the end of this function after all the failure paths. This code all ends up in the caller of the factored out code anyway so that comment becomes irrelevant on the version I've ended up with. Jonathan > + *cdat_table = NULL; > return -ENOMEM; > } > + > nonvolatile_dsmad = next_dsmad_handle++; > *dsmas_nonvolatile = (CDATDsmas) { > .header = { > @@ -57,15 +68,8 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > .DPA_base = 0, > .DPA_length = int128_get64(mr->size), > }; > - len++; > > /* For now, no memory side cache, plausiblish numbers */ > - dslbis_nonvolatile = > - g_malloc(sizeof(*dslbis_nonvolatile) * dslbis_nonvolatile_num); > - if (!dslbis_nonvolatile) { > - return -ENOMEM; > - } > - > dslbis_nonvolatile[0] = (CDATDslbis) { > .header = { > .type = CDAT_TYPE_DSLBIS, > @@ -77,7 +81,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > .entry_base_unit = 10000, /* 10ns base */ > .entry[0] = 15, /* 150ns */ > }; > - len++; > > dslbis_nonvolatile[1] = (CDATDslbis) { > .header = { > @@ -90,7 +93,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > .entry_base_unit = 10000, > .entry[0] = 25, /* 250ns */ > }; > - len++; > > dslbis_nonvolatile[2] = (CDATDslbis) { > .header = { > @@ -103,7 +105,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > .entry_base_unit = 1000, /* GB/s */ > .entry[0] = 16, > }; > - len++; > > dslbis_nonvolatile[3] = (CDATDslbis) { > .header = { > @@ -116,9 +117,7 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > .entry_base_unit = 1000, /* GB/s */ > .entry[0] = 16, > }; > - len++; > > - dsemts_nonvolatile = g_malloc(sizeof(*dsemts_nonvolatile)); > *dsemts_nonvolatile = (CDATDsemts) { > .header = { > .type = CDAT_TYPE_DSEMTS, > @@ -130,26 +129,19 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, > .DPA_offset = 0, > .DPA_length = int128_get64(mr->size), > }; > - len++; > > - *cdat_table = g_malloc0(len * sizeof(*cdat_table)); > /* Header always at start of structure */ > - if (dsmas_nonvolatile) { > - (*cdat_table)[i++] = g_steal_pointer(&dsmas_nonvolatile); > - } > - if (dslbis_nonvolatile) { > - CDATDslbis *dslbis = g_steal_pointer(&dslbis_nonvolatile); > - int j; > + (*cdat_table)[i++] = g_steal_pointer(&dsmas_nonvolatile); > > - for (j = 0; j < dslbis_nonvolatile_num; j++) { > - (*cdat_table)[i++] = (CDATSubHeader *)&dslbis[j]; > - } > - } > - if (dsemts_nonvolatile) { > - (*cdat_table)[i++] = g_steal_pointer(&dsemts_nonvolatile); > + CDATDslbis *dslbis = g_steal_pointer(&dslbis_nonvolatile); Removing the paranoid checking makes sense if we are going to handle the volatile / non volatile as 'whole sets of tables'. > + int j; > + for (j = 0; j < dslbis_nonvolatile_num; j++) { > + (*cdat_table)[i++] = (CDATSubHeader *)&dslbis[j]; > } > > - return len; > + (*cdat_table)[i++] = g_steal_pointer(&dsemts_nonvolatile); > + > + return CT3_CDAT_SUBTABLE_SIZE; > } > > static void ct3_free_cdat_table(CDATSubHeader **cdat_table, int num, void *priv)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 43b2b9e041..0e0ea70387 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -17,6 +17,7 @@ #include "hw/pci/msix.h" #define DWORD_BYTE 4 +#define CT3_CDAT_SUBTABLE_SIZE 6 static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv) @@ -25,7 +26,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, g_autofree CDATDslbis *dslbis_nonvolatile = NULL; g_autofree CDATDsemts *dsemts_nonvolatile = NULL; CXLType3Dev *ct3d = priv; - int len = 0; int i = 0; int next_dsmad_handle = 0; int nonvolatile_dsmad = -1; @@ -33,7 +33,7 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, MemoryRegion *mr; if (!ct3d->hostmem) { - return len; + return 0; } mr = host_memory_backend_get_memory(ct3d->hostmem); @@ -41,11 +41,22 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, return -EINVAL; } + *cdat_table = g_malloc0(CT3_CDAT_SUBTABLE_SIZE * sizeof(*cdat_table)); + if (!*cdat_table) { + return -ENOMEM; + } + /* Non volatile aspects */ dsmas_nonvolatile = g_malloc(sizeof(*dsmas_nonvolatile)); - if (!dsmas_nonvolatile) { + dslbis_nonvolatile = + g_malloc(sizeof(*dslbis_nonvolatile) * dslbis_nonvolatile_num); + dsemts_nonvolatile = g_malloc(sizeof(*dsemts_nonvolatile)); + if (!dsmas_nonvolatile || !dslbis_nonvolatile || !dsemts_nonvolatile) { + g_free(*cdat_table); + *cdat_table = NULL; return -ENOMEM; } + nonvolatile_dsmad = next_dsmad_handle++; *dsmas_nonvolatile = (CDATDsmas) { .header = { @@ -57,15 +68,8 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, .DPA_base = 0, .DPA_length = int128_get64(mr->size), }; - len++; /* For now, no memory side cache, plausiblish numbers */ - dslbis_nonvolatile = - g_malloc(sizeof(*dslbis_nonvolatile) * dslbis_nonvolatile_num); - if (!dslbis_nonvolatile) { - return -ENOMEM; - } - dslbis_nonvolatile[0] = (CDATDslbis) { .header = { .type = CDAT_TYPE_DSLBIS, @@ -77,7 +81,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, .entry_base_unit = 10000, /* 10ns base */ .entry[0] = 15, /* 150ns */ }; - len++; dslbis_nonvolatile[1] = (CDATDslbis) { .header = { @@ -90,7 +93,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, .entry_base_unit = 10000, .entry[0] = 25, /* 250ns */ }; - len++; dslbis_nonvolatile[2] = (CDATDslbis) { .header = { @@ -103,7 +105,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, .entry_base_unit = 1000, /* GB/s */ .entry[0] = 16, }; - len++; dslbis_nonvolatile[3] = (CDATDslbis) { .header = { @@ -116,9 +117,7 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, .entry_base_unit = 1000, /* GB/s */ .entry[0] = 16, }; - len++; - dsemts_nonvolatile = g_malloc(sizeof(*dsemts_nonvolatile)); *dsemts_nonvolatile = (CDATDsemts) { .header = { .type = CDAT_TYPE_DSEMTS, @@ -130,26 +129,19 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, .DPA_offset = 0, .DPA_length = int128_get64(mr->size), }; - len++; - *cdat_table = g_malloc0(len * sizeof(*cdat_table)); /* Header always at start of structure */ - if (dsmas_nonvolatile) { - (*cdat_table)[i++] = g_steal_pointer(&dsmas_nonvolatile); - } - if (dslbis_nonvolatile) { - CDATDslbis *dslbis = g_steal_pointer(&dslbis_nonvolatile); - int j; + (*cdat_table)[i++] = g_steal_pointer(&dsmas_nonvolatile); - for (j = 0; j < dslbis_nonvolatile_num; j++) { - (*cdat_table)[i++] = (CDATSubHeader *)&dslbis[j]; - } - } - if (dsemts_nonvolatile) { - (*cdat_table)[i++] = g_steal_pointer(&dsemts_nonvolatile); + CDATDslbis *dslbis = g_steal_pointer(&dslbis_nonvolatile); + int j; + for (j = 0; j < dslbis_nonvolatile_num; j++) { + (*cdat_table)[i++] = (CDATSubHeader *)&dslbis[j]; } - return len; + (*cdat_table)[i++] = g_steal_pointer(&dsemts_nonvolatile); + + return CT3_CDAT_SUBTABLE_SIZE; } static void ct3_free_cdat_table(CDATSubHeader **cdat_table, int num, void *priv)
Makes the size of the allocated cdat table static (6 entries), flattens the code, and reduces the number of exit conditions Signed-off-by: Gregory Price <gregory.price@memverge.com> --- hw/mem/cxl_type3.c | 52 ++++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 30 deletions(-)