| Message ID | 20250207153753.418849-6-rrichter@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Address translation support, part 1: Cleanups and refactoring | expand |
On Fri, Feb 07, 2025 at 04:37:40PM +0100, Robert Richter wrote: > Often a parent port must be determined. Introduce the parent_port_of() > helper function for this. Would this be simpler with less touchpoints: Make next_port() available to the port driver by moving it from region.c to port.c. Note that simply exporting from the region driver is not an option since region driver is not guaranteed to be configured. (Basically I'm suggesting keep the name and touch region.c less) > > Signed-off-by: Robert Richter <rrichter@amd.com> > Reviewed-by: Gregory Price <gourry@gourry.net> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/port.c | 15 +++++++++------ > drivers/cxl/core/region.c | 11 ++--------- > drivers/cxl/cxl.h | 1 + > 3 files changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index f9501a16b390..d19930c009ce 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -606,17 +606,20 @@ struct cxl_port *to_cxl_port(const struct device *dev) > } > EXPORT_SYMBOL_NS_GPL(to_cxl_port, "CXL"); > > +struct cxl_port *parent_port_of(struct cxl_port *port) > +{ > + if (!port || !port->parent_dport) > + return NULL; > + return port->parent_dport->port; > +} > +EXPORT_SYMBOL_NS_GPL(parent_port_of, "CXL"); > + > static void unregister_port(void *_port) > { > struct cxl_port *port = _port; > - struct cxl_port *parent; > + struct cxl_port *parent = parent_port_of(port); > struct device *lock_dev; > > - if (is_cxl_root(port)) > - parent = NULL; > - else > - parent = to_cxl_port(port->dev.parent); > - > /* > * CXL root port's and the first level of ports are unregistered > * under the platform firmware device lock, all other ports are > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 5d252dfae138..54afdb0fa61c 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1734,13 +1734,6 @@ static int cmp_interleave_pos(const void *a, const void *b) > return cxled_a->pos - cxled_b->pos; > } > > -static struct cxl_port *next_port(struct cxl_port *port) > -{ > - if (!port->parent_dport) > - return NULL; > - return port->parent_dport->port; > -} > - > static int match_switch_decoder_by_range(struct device *dev, > const void *data) > { > @@ -1767,7 +1760,7 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range, > struct device *dev; > int rc = -ENXIO; > > - parent = next_port(port); > + parent = parent_port_of(port); > if (!parent) > return rc; > > @@ -1847,7 +1840,7 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled) > */ > > /* Iterate from endpoint to root_port refining the position */ > - for (iter = port; iter; iter = next_port(iter)) { > + for (iter = port; iter; iter = parent_port_of(iter)) { > if (is_cxl_root(iter)) > break; > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 6baec4ba9141..0d7aff8b97b3 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -721,6 +721,7 @@ static inline bool is_cxl_root(struct cxl_port *port) > int cxl_num_decoders_committed(struct cxl_port *port); > bool is_cxl_port(const struct device *dev); > struct cxl_port *to_cxl_port(const struct device *dev); > +struct cxl_port *parent_port_of(struct cxl_port *port); > void cxl_port_commit_reap(struct cxl_decoder *cxld); > struct pci_bus; > int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev, > -- > 2.39.5 >
On 07.02.25 15:08:11, Alison Schofield wrote: > On Fri, Feb 07, 2025 at 04:37:40PM +0100, Robert Richter wrote: > > Often a parent port must be determined. Introduce the parent_port_of() > > helper function for this. > > Would this be simpler with less touchpoints: > > Make next_port() available to the port driver by moving it from > region.c to port.c. Note that simply exporting from the region > driver is not an option since region driver is not guaranteed > to be configured. > > (Basically I'm suggesting keep the name and touch region.c less) As long as next_port() is used in a local context the function name works well as its direct use is visible. But when exporting it, the name "next_port()" is not very specific. So I renamed it to better describe the function similar to other helpers like this. Changes for the rename are just 2 lines in region.c. The helper is only used in the core module, there is no strict need to export it from there. Anyway, I implemented it the same way as the to_cxl_port() helper and it can be used outside of core too. This does not introduce more code (except for the EXPORT_SYMBOL_NS_GPL line). Thus, I rather would like to keep the patch as it is. Thanks for review, -Robert > > > > > Signed-off-by: Robert Richter <rrichter@amd.com> > > Reviewed-by: Gregory Price <gourry@gourry.net> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index f9501a16b390..d19930c009ce 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -606,17 +606,20 @@ struct cxl_port *to_cxl_port(const struct device *dev) } EXPORT_SYMBOL_NS_GPL(to_cxl_port, "CXL"); +struct cxl_port *parent_port_of(struct cxl_port *port) +{ + if (!port || !port->parent_dport) + return NULL; + return port->parent_dport->port; +} +EXPORT_SYMBOL_NS_GPL(parent_port_of, "CXL"); + static void unregister_port(void *_port) { struct cxl_port *port = _port; - struct cxl_port *parent; + struct cxl_port *parent = parent_port_of(port); struct device *lock_dev; - if (is_cxl_root(port)) - parent = NULL; - else - parent = to_cxl_port(port->dev.parent); - /* * CXL root port's and the first level of ports are unregistered * under the platform firmware device lock, all other ports are diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 5d252dfae138..54afdb0fa61c 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1734,13 +1734,6 @@ static int cmp_interleave_pos(const void *a, const void *b) return cxled_a->pos - cxled_b->pos; } -static struct cxl_port *next_port(struct cxl_port *port) -{ - if (!port->parent_dport) - return NULL; - return port->parent_dport->port; -} - static int match_switch_decoder_by_range(struct device *dev, const void *data) { @@ -1767,7 +1760,7 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range, struct device *dev; int rc = -ENXIO; - parent = next_port(port); + parent = parent_port_of(port); if (!parent) return rc; @@ -1847,7 +1840,7 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled) */ /* Iterate from endpoint to root_port refining the position */ - for (iter = port; iter; iter = next_port(iter)) { + for (iter = port; iter; iter = parent_port_of(iter)) { if (is_cxl_root(iter)) break; diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 6baec4ba9141..0d7aff8b97b3 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -721,6 +721,7 @@ static inline bool is_cxl_root(struct cxl_port *port) int cxl_num_decoders_committed(struct cxl_port *port); bool is_cxl_port(const struct device *dev); struct cxl_port *to_cxl_port(const struct device *dev); +struct cxl_port *parent_port_of(struct cxl_port *port); void cxl_port_commit_reap(struct cxl_decoder *cxld); struct pci_bus; int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,