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

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

[Dave Jiang]

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))
+
 static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
 {
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
-	struct cxl_port *root_port __free(put_device) = NULL;
+	struct cxl_port *root_port __free(cxl_port) = NULL;
 	LIST_HEAD(__discard);
 	struct list_head *discard __free(dpa_perf) = &__discard;
 	int rc;

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

[Alison Schofield]

On Thu, Jan 04, 2024 at 09:08:05AM -0700, 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>


Reviewed-by: Alison Schofield <alison.schofield@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))
> +
>  static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>  {
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	struct cxl_port *root_port __free(put_device) = NULL;
> +	struct cxl_port *root_port __free(cxl_port) = NULL;
>  	LIST_HEAD(__discard);
>  	struct list_head *discard __free(dpa_perf) = &__discard;
>  	int rc;
> 
> 

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

[Robert Richter]

On 04.01.24 09:08:05, 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))

Let's put this in parallel with find_cxl_root() to make it part of the
api for users of find_cxl_root() to cleanup.

Nit: remove the additional newline to have the same style for all uses
of DEFINE_FREE() in this file.

-Robert

> +
>  static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>  {
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	struct cxl_port *root_port __free(put_device) = NULL;
> +	struct cxl_port *root_port __free(cxl_port) = NULL;
>  	LIST_HEAD(__discard);
>  	struct list_head *discard __free(dpa_perf) = &__discard;
>  	int rc;
> 
> 

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

[Dave Jiang]

On 1/4/24 11:22, Robert Richter wrote:
> On 04.01.24 09:08:05, 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))
> 
> Let's put this in parallel with find_cxl_root() to make it part of the
> api for users of find_cxl_root() to cleanup.

Sure that makes sense. Will do that.

> 
> Nit: remove the additional newline to have the same style for all uses
> of DEFINE_FREE() in this file.

I was contemplating that because checkpatch complained. But I guess if we are moving the function to where find_cxl_root() is then it won't matter. 

> 
> -Robert
> 
>> +
>>  static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>>  {
>>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>> -	struct cxl_port *root_port __free(put_device) = NULL;
>> +	struct cxl_port *root_port __free(cxl_port) = NULL;
>>  	LIST_HEAD(__discard);
>>  	struct list_head *discard __free(dpa_perf) = &__discard;
>>  	int rc;
>>
>>

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

[Dan Williams]

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.

> +
>  static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>  {
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	struct cxl_port *root_port __free(put_device) = NULL;
> +	struct cxl_port *root_port __free(cxl_port) = NULL;
>  	LIST_HEAD(__discard);
>  	struct list_head *discard __free(dpa_perf) = &__discard;
>  	int rc;

So in the discussion here:

http://lore.kernel.org/r/65970003ee9ef_8dc6829412@dwillia2-xfh.jf.intel.com.notmuch

...I made the assertion that if a function has multiple __free()
invocations it should make sure to arrange for it to be impossible for
declaration-order to be different from initialization order, and that
means getting rid of the "__free(x) = NULL" pattern and keep
declare/init aligned:

	struct cxl_port *root __free(put_cxl_root) = find_cxl_root(port);

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

[Jonathan Cameron]

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

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

[Robert Richter]

On 08.01.24 11:45:57, Jonathan Cameron wrote:
> 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. 

There is some description in include/linux/cleanup.h:

 * NOTE: the DEFINE_FREE()'s @free expression includes a NULL test even though                                                                                                          
 * kfree() is fine to be called with a NULL value. This is on purpose. This way                                                                                                          
 * the compiler sees the end of our alloc_obj() function as:                                                                                                          
 *                                                                                                          
 *      tmp = p;                                                                                                          
 *      p = NULL;                                                                                                          
 *      if (p)                                                                                                          
 *              kfree(p);                                                                                                          
 *      return tmp;                                                                                                          
 *                                                                                                          
 * And through the magic of value-propagation and dead-code-elimination, it                                                                                                          
 * eliminates the actual cleanup call and compiles into:                                                                                                          
 *                                                                                                          
 *      return p;                                                                                                          
 *                                                                                                          
 * Without the NULL test it turns into a mess and the compiler can't help us.                                                                                                          

So afaiu if the check is local the compiler optimizes to not call the
function at all when using return_ptr(p) (end maybe if NULL
preinitialized).

-Robert

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

[Jonathan Cameron]

On Mon, 8 Jan 2024 12:57:16 +0100
Robert Richter <rrichter@amd.com> wrote:

> On 08.01.24 11:45:57, Jonathan Cameron wrote:
> > 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.   
> 
> There is some description in include/linux/cleanup.h:
> 
>  * NOTE: the DEFINE_FREE()'s @free expression includes a NULL test even though                                                                                                          
>  * kfree() is fine to be called with a NULL value. This is on purpose. This way                                                                                                          
>  * the compiler sees the end of our alloc_obj() function as:                                                                                                          
>  *                                                                                                          
>  *      tmp = p;                                                                                                          
>  *      p = NULL;                                                                                                          
>  *      if (p)                                                                                                          
>  *              kfree(p);                                                                                                          
>  *      return tmp;                                                                                                          
>  *                                                                                                          
>  * And through the magic of value-propagation and dead-code-elimination, it                                                                                                          
>  * eliminates the actual cleanup call and compiles into:                                                                                                          
>  *                                                                                                          
>  *      return p;                                                                                                          
>  *                                                                                                          
>  * Without the NULL test it turns into a mess and the compiler can't help us.                                                                                                          
> 
> So afaiu if the check is local the compiler optimizes to not call the
> function at all when using return_ptr(p) (end maybe if NULL
> preinitialized).
> 
Thanks!  I just saw that Dan also referenced an email from Peter Z
that said the same in the PCI discussion.
Re: [PATCH v5 8/9] PCI: Define scoped based management functions

Jonathan

> -Robert