On Wed, Nov 15, 2023 at 02:09:15AM -0800, Vishal Verma wrote:
> On Sun, 2023-10-01 at 15:31 -0700, [email protected] 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!
>
