Re: [ndctl PATCH v2 3/5] cxl/list: collect and parse the poison list records

[Alison Schofield <alison.schofield@intel.com>]

On Wed, Nov 15, 2023 at 02:09:15AM -0800, Vishal Verma wrote:
> On Sun, 2023-10-01 at 15:31 -0700, alison.schofield@intel.com wrote:

snip

> >
> > +/* CXL 8.2.9.5.4.1 Get Poison List: Poison Source */
> 
> These usually have a spec version too - "CXL 3.0 8.2.9... "

Got it. Fixed up the two like this and also a commit log reference.
Moved them all to the now released CXL 3.1 spec.

> > +static struct json_object *
> > +util_cxl_poison_events_to_json(struct tracefs_instance *inst, bool is_region,
> > +                              unsigned long flags)
> > +{
> > +       struct json_object *jerrors, *jmedia, *jobj = NULL;
> 
> Since everything else is now 'poison', might be good to also
> s/jmedia/jpoison/ everywhere.

Done.

> > +
> > +       list_for_each_safe(&ectx.jlist_head, jnode, next, list) {
> > +               struct json_object *jval = NULL;
> > +               struct json_object *jp = NULL;
> 
> Are the NULL assignments needed? At least for @jp, it is
> unconditionally assigned below, and isn't used before that. I suspect
> json-c probably doesn't care about what's in @jval either before
> writing it.
>

jp init, obvious not init needed.
For jval init, I guess I worried about garbage in jval, but the way the
trace event is built, it's not a concern.

Removed both those inits.

snip

> > +               if (json_object_object_get_ex(jnode->jobj, "source", &jval)) {
> > +                       source = json_object_get_int(jval);
> > +                       if (source == CXL_POISON_SOURCE_UNKNOWN)
> > +                               jobj = json_object_new_string("Unknown");
> > +                       else if (source == CXL_POISON_SOURCE_EXTERNAL)
> > +                               jobj = json_object_new_string("External");
> > +                       else if (source == CXL_POISON_SOURCE_INTERNAL)
> > +                               jobj = json_object_new_string("Internal");
> > +                       else if (source == CXL_POISON_SOURCE_INJECTED)
> > +                               jobj = json_object_new_string("Injected");
> > +                       else if (source == CXL_POISON_SOURCE_VENDOR)
> > +                               jobj = json_object_new_string("Vendor");
> > +                       else
> > +                               jobj = json_object_new_string("Reserved");
> 
> Minor nit, but maybe 'switch (source) ...' would look a bit cleaner?

Done.

> 
> > +
> > +       /* Always include the count. If count is zero, no records follow. */
> > +       jobj = json_object_new_int(count);
> > +       if (jobj)
> > +               json_object_object_add(jmedia, "nr_poison_records", jobj);
> > +       if (count)
> > +               json_object_object_add(jmedia, "poison_records", jerrors);
> 
> Since these are already nested under a 'poison' JSON object, I'm
> tempted to say these can just be 'nr_records' and 'records'
> respectively.
> 

Done.

> > +
> > +       return jmedia;
> > +}
> > +
> > +struct cxl_poison_ctx {
> > +       void *dev;
> > +       bool is_region;
> > +};
> 
> This structure is a bit awkward - what do you think about creating
> different wrappers for the memdev and region case -
> 
>   util_cxl_memdev_poison_list_to_json(), and
>   util_cxl_region_poison_list_to_json() that are called respectively by
> 
> util_cxl_{memdev,region}_to_json(), and internally they can call:
> 
>   util_cxl_poison_list_to_json(NULL, memdev, flags), or
>   util_cxl_poison_list_to_json(region, NULL, flags)
> 
> For the next level down, i.e. poison_events_to_json, the @is_region
> bool passed in directly is fine as it doesn't need the memdev or region
> objects passed in via void *.

Thanks! I did something like this but didn't actually add wrappers.
Please look in v3 and let me know.

> 
snip

> > +       if (pctx->is_region)
> > +               rc = cxl_region_trigger_poison_list(pctx->dev);
> > +       else
> > +               rc = cxl_memdev_trigger_poison_list(pctx->dev);
> > +       if (rc) {
> > +               fprintf(stderr, "Failed write of sysfs attribute: %d\n", rc);
> 
> This would be incorrect if the memdev trigger reported an ENOMEM, and
> then this reported a sysfs write failure.
> 
> It should at least be something like 'failed to trigger poison" - but
> since the memdev trigger helper has prints for every failure case,
> maybe this can just be omitted?

Removed it.

>
snip

> > diff --git a/util/json.h b/util/json.h
> > index ea370df4d1b7..3ae4074a95c3 100644
> > --- a/util/json.h
> > +++ b/util/json.h
> > @@ -21,6 +21,7 @@ enum util_json_flags {
> >         UTIL_JSON_TARGETS       = (1 << 11),
> >         UTIL_JSON_PARTITION     = (1 << 12),
> >         UTIL_JSON_ALERT_CONFIG  = (1 << 13),
> > +       UTIL_JSON_POISON_LIST   = (1 << 14),
> 
> There's already a UTIL_JSON_MEDIA_ERRORS, can we just reuse that (in
> spite of the name :))

Since it's not visible to user, changed it.

Thanks for the review Vishal!

>