| Message ID | 170439743417.3519628.11381251582844385622.stgit@djiang5-mobl3 |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] cxl: Add check for NULL ptr after calling find_cxl_root() | expand |
Dave Jiang wrote: > Add a helper function put_cxl_root() to maintain symmetry for > find_cxl_root() function instead of relying on open coding of the > put_device() in order to dereference the 'struct device' that happens via > get_device() in find_cxl_root(). Add cleanups for all code paths that > calls put_device() after find_cxl_root(). > > In the process also clean up cdat code that uses __free() on the root_port. > The current __free() call for root_port in cxl_qos_class_verify() and > cxl_port_perf_data_calculate() 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. Use the put_cxl_root() call instead to make > the code more precise. > > Suggested-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > v2: > - Make put_cxl_root() an exported function to be symmetric to > find_cxl_root(). (Robert) > --- > drivers/cxl/core/cdat.c | 6 ++++-- > drivers/cxl/core/pmem.c | 2 +- > drivers/cxl/core/port.c | 9 +++++++++ > drivers/cxl/cxl.h | 1 + > drivers/cxl/port.c | 2 +- > 5 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > index 701ed53ce337..18399baa45b8 100644 > --- a/drivers/cxl/core/cdat.c > +++ b/drivers/cxl/core/cdat.c > @@ -158,11 +158,13 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port, > return cdat_table_parse_output(rc); > } > > +DEFINE_FREE(put_root_port, struct cxl_port *, put_cxl_root(_T)) Lets keep the free routine name and the __free() type the same name, so it would be put_cxl_root() throughout. Overall, this exercise also highlights that the commit that added to_cxl_root(): 790815902ec6 cxl: Add support for _DSM Function for retrieving QTG ID ...really should have been broken out first to rework find_cxl_root() to return a 'struct cxl_root *'. We can clean that up now on the backend. Callers of find_cxl_root() that need the port should be doing something like: root_port = &cxl_root->port; > + > static int cxl_port_perf_data_calculate(struct cxl_port *port, > struct xarray *dsmas_xa) > { > struct access_coordinate c; > - struct cxl_port *root_port __free(put_device) = NULL; > + struct cxl_port *root_port __free(put_root_port) = NULL; Where possible assign to find_cxl_root() immediately, or move the declaration down to where find_cxl_root() is called. > struct cxl_root *cxl_root; > struct dsmas_entry *dent; > int valid_entries = 0; > @@ -352,7 +354,7 @@ 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(put_root_port) = NULL; > LIST_HEAD(__discard); > struct list_head *discard __free(dpa_perf) = &__discard; > int rc; > diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c > index fc94f5240327..1b8d7f812f1a 100644 > --- a/drivers/cxl/core/pmem.c > +++ b/drivers/cxl/core/pmem.c > @@ -71,7 +71,7 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd) > return NULL; > > dev = device_find_child(&port->dev, NULL, match_nvdimm_bridge); > - put_device(&port->dev); > + put_cxl_root(port); I would say nothing should be calling put_cxl_root() directly. The fixup here is to convert this to use __free(put_cxl_root) at declaration time. > > if (!dev) > return NULL; > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 8c00fd6be730..f989f88960bb 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -986,6 +986,15 @@ struct cxl_port *find_cxl_root(struct cxl_port *port) > } > EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL); > > +void put_cxl_root(struct cxl_port *port) > +{ > + if (!port || !is_cxl_root(port)) > + return; The !is_cxl_root() can go away if this function starts taking a 'struct cxl_root *' argument directly. Let the type safety happen at compile time now that 'struct cxl_root' is a distinct type. > + > + put_device(&port->dev); > +} > +EXPORT_SYMBOL_NS_GPL(put_cxl_root, CXL); > + > static struct cxl_dport *find_dport(struct cxl_port *port, int id) > { > struct cxl_dport *dport; > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 492dbf63935f..ba82b2f89898 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -735,6 +735,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, > struct cxl_root *devm_cxl_add_root(struct device *host, > const struct cxl_root_ops *ops); > struct cxl_port *find_cxl_root(struct cxl_port *port); > +void put_cxl_root(struct cxl_port *port); > int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd); > void cxl_bus_rescan(void); > void cxl_bus_drain(void); > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > index da3c3a08bd62..398f5d50e942 100644 > --- a/drivers/cxl/port.c > +++ b/drivers/cxl/port.c > @@ -137,7 +137,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) > * assemble regions from committed decoders > */ > device_for_each_child(&port->dev, root, discover_region); > - put_device(&root->dev); > + put_cxl_root(root); Another one to convert to __free(put_cxl_root).
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c index 701ed53ce337..18399baa45b8 100644 --- a/drivers/cxl/core/cdat.c +++ b/drivers/cxl/core/cdat.c @@ -158,11 +158,13 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port, return cdat_table_parse_output(rc); } +DEFINE_FREE(put_root_port, struct cxl_port *, put_cxl_root(_T)) + static int cxl_port_perf_data_calculate(struct cxl_port *port, struct xarray *dsmas_xa) { struct access_coordinate c; - struct cxl_port *root_port __free(put_device) = NULL; + struct cxl_port *root_port __free(put_root_port) = NULL; struct cxl_root *cxl_root; struct dsmas_entry *dent; int valid_entries = 0; @@ -352,7 +354,7 @@ 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(put_root_port) = NULL; LIST_HEAD(__discard); struct list_head *discard __free(dpa_perf) = &__discard; int rc; diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c index fc94f5240327..1b8d7f812f1a 100644 --- a/drivers/cxl/core/pmem.c +++ b/drivers/cxl/core/pmem.c @@ -71,7 +71,7 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd) return NULL; dev = device_find_child(&port->dev, NULL, match_nvdimm_bridge); - put_device(&port->dev); + put_cxl_root(port); if (!dev) return NULL; diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 8c00fd6be730..f989f88960bb 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -986,6 +986,15 @@ struct cxl_port *find_cxl_root(struct cxl_port *port) } EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL); +void put_cxl_root(struct cxl_port *port) +{ + if (!port || !is_cxl_root(port)) + return; + + put_device(&port->dev); +} +EXPORT_SYMBOL_NS_GPL(put_cxl_root, CXL); + static struct cxl_dport *find_dport(struct cxl_port *port, int id) { struct cxl_dport *dport; diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 492dbf63935f..ba82b2f89898 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -735,6 +735,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct cxl_root *devm_cxl_add_root(struct device *host, const struct cxl_root_ops *ops); struct cxl_port *find_cxl_root(struct cxl_port *port); +void put_cxl_root(struct cxl_port *port); int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd); void cxl_bus_rescan(void); void cxl_bus_drain(void); diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c index da3c3a08bd62..398f5d50e942 100644 --- a/drivers/cxl/port.c +++ b/drivers/cxl/port.c @@ -137,7 +137,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) * assemble regions from committed decoders */ device_for_each_child(&port->dev, root, discover_region); - put_device(&root->dev); + put_cxl_root(root); return 0; }
Add a helper function put_cxl_root() to maintain symmetry for find_cxl_root() function instead of relying on open coding of the put_device() in order to dereference the 'struct device' that happens via get_device() in find_cxl_root(). Add cleanups for all code paths that calls put_device() after find_cxl_root(). In the process also clean up cdat code that uses __free() on the root_port. The current __free() call for root_port in cxl_qos_class_verify() and cxl_port_perf_data_calculate() 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. Use the put_cxl_root() call instead to make the code more precise. Suggested-by: Robert Richter <rrichter@amd.com> Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- v2: - Make put_cxl_root() an exported function to be symmetric to find_cxl_root(). (Robert) --- drivers/cxl/core/cdat.c | 6 ++++-- drivers/cxl/core/pmem.c | 2 +- drivers/cxl/core/port.c | 9 +++++++++ drivers/cxl/cxl.h | 1 + drivers/cxl/port.c | 2 +- 5 files changed, 16 insertions(+), 4 deletions(-)