| Message ID | 170476244072.115624.16860571023685051383.stgit@djiang5-mobl3 |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Dave Jiang wrote: > Move the calling find_cxl_port() and the variable declaration into the > var declaration block and remove line breaks in the declaration. There are > no ordering issues that would require the declaration to be in middle of > the code. > > Suggested-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/cdat.c | 7 ++----- > drivers/cxl/core/pmem.c | 3 +-- > drivers/cxl/port.c | 11 +++++------ > 3 files changed, 8 insertions(+), 13 deletions(-) > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > index 140935511bab..1d830d088389 100644 > --- a/drivers/cxl/core/cdat.c > +++ b/drivers/cxl/core/cdat.c > @@ -162,6 +162,7 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port, > static int cxl_port_perf_data_calculate(struct cxl_port *port, > struct xarray *dsmas_xa) > { > + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); > struct access_coordinate c; > struct dsmas_entry *dent; > int valid_entries = 0; > @@ -174,8 +175,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, > return rc; > } > > - struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); > - This looks ok, but given the comment on patch1 where it looks like a layering violation for this to assume that it is being called from cxl_port_probe() context then keeping the: if (!cxl_root) return -ENODEV; ...check means this one should stay as well. > if (!cxl_root->ops || !cxl_root->ops->qos_class) > return -EOPNOTSUPP; > > @@ -344,15 +343,13 @@ DEFINE_FREE(dpa_perf, struct list_head *, if (!list_empty(_T)) discard_dpa_perf( > > static int cxl_qos_class_verify(struct cxl_memdev *cxlmd) > { > + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(cxlmd->endpoint); > struct cxl_dev_state *cxlds = cxlmd->cxlds; > struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); > LIST_HEAD(__discard); > struct list_head *discard __free(dpa_perf) = &__discard; > int rc; > > - struct cxl_root *cxl_root __free(put_cxl_root) = > - find_cxl_root(cxlmd->endpoint); > - Similar comment as above. > /* Check that the QTG IDs are all sane between end device and root decoders */ > cxl_qos_match(cxl_root, &mds->ram_perf_list, discard); > cxl_qos_match(cxl_root, &mds->pmem_perf_list, discard); > diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c > index ed76d37e4fd9..976212e588bc 100644 > --- a/drivers/cxl/core/pmem.c > +++ b/drivers/cxl/core/pmem.c > @@ -64,8 +64,7 @@ static int match_nvdimm_bridge(struct device *dev, void *data) > > struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd) > { > - struct cxl_root *cxl_root __free(put_cxl_root) = > - find_cxl_root(cxlmd->endpoint); > + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(cxlmd->endpoint); clang-format picks the other way, I don't like futzing with line breaks if I don't have to. > struct device *dev; > > dev = device_find_child(&cxl_root->port.dev, NULL, match_nvdimm_bridge); > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > index c054e7b13bdd..2bacf48593e6 100644 > --- a/drivers/cxl/port.c > +++ b/drivers/cxl/port.c > @@ -91,6 +91,11 @@ static int cxl_switch_port_probe(struct cxl_port *port) > > static int cxl_endpoint_port_probe(struct cxl_port *port) > { > + /* > + * This can't fail in practice as CXL root exit unregisters all > + * descendant ports and that in turn synchronizes with cxl_port_probe() > + */ > + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); > struct cxl_endpoint_dvsec_info info = { .port = port }; > struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev); > struct cxl_dev_state *cxlds = cxlmd->cxlds; > @@ -125,12 +130,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) > if (rc) > return rc; > > - /* > - * This can't fail in practice as CXL root exit unregisters all > - * descendant ports and that in turn synchronizes with cxl_port_probe() > - */ > - struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); > - Given the "not a slam dunk" justification for the other changes I'm inclined to just drop this patch altogether.
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c index 140935511bab..1d830d088389 100644 --- a/drivers/cxl/core/cdat.c +++ b/drivers/cxl/core/cdat.c @@ -162,6 +162,7 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port, static int cxl_port_perf_data_calculate(struct cxl_port *port, struct xarray *dsmas_xa) { + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); struct access_coordinate c; struct dsmas_entry *dent; int valid_entries = 0; @@ -174,8 +175,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, return rc; } - struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); - if (!cxl_root->ops || !cxl_root->ops->qos_class) return -EOPNOTSUPP; @@ -344,15 +343,13 @@ DEFINE_FREE(dpa_perf, struct list_head *, if (!list_empty(_T)) discard_dpa_perf( static int cxl_qos_class_verify(struct cxl_memdev *cxlmd) { + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(cxlmd->endpoint); struct cxl_dev_state *cxlds = cxlmd->cxlds; struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds); LIST_HEAD(__discard); struct list_head *discard __free(dpa_perf) = &__discard; int rc; - struct cxl_root *cxl_root __free(put_cxl_root) = - find_cxl_root(cxlmd->endpoint); - /* Check that the QTG IDs are all sane between end device and root decoders */ cxl_qos_match(cxl_root, &mds->ram_perf_list, discard); cxl_qos_match(cxl_root, &mds->pmem_perf_list, discard); diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c index ed76d37e4fd9..976212e588bc 100644 --- a/drivers/cxl/core/pmem.c +++ b/drivers/cxl/core/pmem.c @@ -64,8 +64,7 @@ static int match_nvdimm_bridge(struct device *dev, void *data) struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd) { - struct cxl_root *cxl_root __free(put_cxl_root) = - find_cxl_root(cxlmd->endpoint); + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(cxlmd->endpoint); struct device *dev; dev = device_find_child(&cxl_root->port.dev, NULL, match_nvdimm_bridge); diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c index c054e7b13bdd..2bacf48593e6 100644 --- a/drivers/cxl/port.c +++ b/drivers/cxl/port.c @@ -91,6 +91,11 @@ static int cxl_switch_port_probe(struct cxl_port *port) static int cxl_endpoint_port_probe(struct cxl_port *port) { + /* + * This can't fail in practice as CXL root exit unregisters all + * descendant ports and that in turn synchronizes with cxl_port_probe() + */ + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); struct cxl_endpoint_dvsec_info info = { .port = port }; struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev); struct cxl_dev_state *cxlds = cxlmd->cxlds; @@ -125,12 +130,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port) if (rc) return rc; - /* - * This can't fail in practice as CXL root exit unregisters all - * descendant ports and that in turn synchronizes with cxl_port_probe() - */ - struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); - /* * Now that all endpoint decoders are successfully enumerated, try to * assemble regions from committed decoders
Move the calling find_cxl_port() and the variable declaration into the var declaration block and remove line breaks in the declaration. There are no ordering issues that would require the declaration to be in middle of the code. Suggested-by: Robert Richter <rrichter@amd.com> Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/core/cdat.c | 7 ++----- drivers/cxl/core/pmem.c | 3 +-- drivers/cxl/port.c | 11 +++++------ 3 files changed, 8 insertions(+), 13 deletions(-)