Re: [PATCH v12 4/6] cxl/region: Provide region info to the cxl_poison trace event

[Dan Williams <dan.j.williams@intel.com>]

Alison Schofield wrote:
[..]
> > Ah, cxl_get_poison_by_endpoint() is a function pointer to
> > device_for_each_child(), that really feels like a detail that's private
> > to the implementation.
> > 
> > I would reorganize this to something like:
> > 
> >      if (port->commit_end == -1) {
> >              /* No regions mapped to this memdev */
> >              rc = cxl_get_poison_by_memdev(cxlmd);
> >      } else {
> >              /* Regions mapped, collect poison by endpoint */
> >              rc = cxl_get_poison_by_endpoint(endpoint);
> >      }
> > 
> > ...and then internal to cxl_get_poison_by_endpoint() do:
> > 	    
> >              ctx = (struct cxl_trigger_poison_context) {
> >                      .port = endpoint,
> >              };
> >              rc = device_for_each_child(&port->dev, &ctx,
> >                                         poison_by_decoder);
> >              if (rc == 1)
> >                      rc = cxl_get_poison_unmapped(cxlmd, &ctx);
> > 
> > ...then the header file reads more calmly with the added type-safety.
> 
> I'll take another stab at this.
> 
> A couple of versions back, I pulled cxl_get_poison_unmapped() out of 
> cxl_get_poison_by_endpoint() since it was not really work of the 
> region driver. (Ira called that out in a review)

I think it's ok to have poison_by_decoder() and
cxl_get_poison_unmapped() within cxl_get_poison_by_endpoint() because
decoder-mapped and unmapped-capacity are both a part of the endpoint.
Given that the region core and the region driver are both glommed into
drivers/cxl/core/region.c I would not worry too much about the unmapped
scanning being in that file if it keeps all the poison code better
colocated.