| Message ID | 170438448564.3436708.17525645430052031684.stgit@djiang5-mobl3 |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl: Clarify root_port cleanup routine for cxl_qos_class_verify() | expand |
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; > >
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; > >
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; >> >>
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);
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
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
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
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;
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(-)