Re: [PATCH] cxl: Clarify root_port cleanup routine for cxl_qos_class_verify()

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 4 Jan 2024 11:52:55 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Dave Jiang wrote:
> > The current __free() call for root_port in cxl_qos_class_verify() depends
> > on 'struct device' to be the first member of 'struct cxl_port' and calls
> > put_device() directly with the root_port pointer passed in. Add a helper
> > function put_cxl_port() to handle the put_device() properly and avoid
> > future issues if the 'struct device' member moves.
> > 
> > Suggested-by: Robert Richter <rrichter@amd.com>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> >  drivers/cxl/core/cdat.c |   12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > index cd84d87f597a..d6e64570032f 100644
> > --- a/drivers/cxl/core/cdat.c
> > +++ b/drivers/cxl/core/cdat.c
> > @@ -345,11 +345,21 @@ static void discard_dpa_perf(struct list_head *list)
> >  }
> >  DEFINE_FREE(dpa_perf, struct list_head *, if (!list_empty(_T)) discard_dpa_perf(_T))
> >  
> > +static void put_cxl_port(struct cxl_port *port)
> > +{
> > +	if (!port)
> > +		return;
> > +
> > +	put_device(&port->dev);
> > +}
> > +
> > +DEFINE_FREE(cxl_port, struct cxl_port *, put_cxl_port(_T))  
> 
> I don't think there are other cases where a port reference is acquired
> at runtime, so this should be root specific, i.e. put_cxl_root().
> 
> This also merits a NULL check to skip calling put_cxl_root() if the
> pointer is already NULL:
> 
> 	DEFINE_FREE(put_cxl_root, struct cxl_port *, if (_T) put_cxl_root(_T))
> 
> ...for example if someone used no_free_ptr() to return the found root
> port.
Hi Dan,

Sorry for late reply - been distracted and only now playing catch up.


I'm curious on this mostly because I got similar review feedback on another
case without the if (_T) and conversely yet another review asking me
to drop it as pointless (totally unrelated bits of the kernel ;)
Why do we care given put_cxl_port() has that check? It's clearly harmless
but also at first glance pointless. 

Jonathan