| Message ID | 170449246681.3779673.2288926019977963333.stgit@djiang5-mobl3 |
|---|---|
| State | Accepted |
| Commit | 98e7ab3345e105339b7d919b4918f3c1879052ed |
| Headers | show |
| Series | cxl: find_cxl_root() related cleanups | expand |
See inline. On 05.01.24 15:07:46, Dave Jiang wrote: > cxl_port_perf_data_calculate() calls find_cxl_root() and does not > dereference the 'struct device' in the cxl_root->port. find_cxl_root() > calls get_device() and takes a reference on the port 'struct device' > member. Use the __free() macro to ensure the dereference happens. > > Fixes: 7a4f148dd8d5 ("cxl: Compute the entire CXL path latency and bandwidth data") > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > v5: > - Update patch title (Dan) > --- > drivers/cxl/core/cdat.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > index 0df5379cf02f..c6208aab452f 100644 > --- a/drivers/cxl/core/cdat.c > +++ b/drivers/cxl/core/cdat.c > @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, > struct xarray *dsmas_xa) > { > struct access_coordinate c; > - struct cxl_root *cxl_root; > struct dsmas_entry *dent; > int valid_entries = 0; > unsigned long index; > @@ -174,7 +173,11 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, > return rc; > } > > - cxl_root = find_cxl_root(port); > + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); I am not quite sure here, can a definition in the middle of a block? At least this is uncommon. > + > + if (!cxl_root) > + return -ENODEV; As Dan pointed out earlier, isn't there always a root port in the hierarchy? So the check could be dropped? > + > if (!cxl_root->ops || !cxl_root->ops->qos_class) > return -EOPNOTSUPP; > > >
On 1/5/24 15:56, Robert Richter wrote: > See inline. > > On 05.01.24 15:07:46, Dave Jiang wrote: >> cxl_port_perf_data_calculate() calls find_cxl_root() and does not >> dereference the 'struct device' in the cxl_root->port. find_cxl_root() >> calls get_device() and takes a reference on the port 'struct device' >> member. Use the __free() macro to ensure the dereference happens. >> >> Fixes: 7a4f148dd8d5 ("cxl: Compute the entire CXL path latency and bandwidth data") >> Reviewed-by: Ira Weiny <ira.weiny@intel.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> v5: >> - Update patch title (Dan) >> --- >> drivers/cxl/core/cdat.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c >> index 0df5379cf02f..c6208aab452f 100644 >> --- a/drivers/cxl/core/cdat.c >> +++ b/drivers/cxl/core/cdat.c >> @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, >> struct xarray *dsmas_xa) >> { >> struct access_coordinate c; >> - struct cxl_root *cxl_root; >> struct dsmas_entry *dent; >> int valid_entries = 0; >> unsigned long index; >> @@ -174,7 +173,11 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, >> return rc; >> } >> >> - cxl_root = find_cxl_root(port); >> + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); > > I am not quite sure here, can a definition in the middle of a block? > At least this is uncommon. From what I understand with the introduction of the scope-based resource management, this kind of declaration is ok for that purpose? Dan? > >> + >> + if (!cxl_root) >> + return -ENODEV; > > As Dan pointed out earlier, isn't there always a root port in the > hierarchy? So the check could be dropped? > >> + >> if (!cxl_root->ops || !cxl_root->ops->qos_class) >> return -EOPNOTSUPP; >> >> >>
Dave Jiang wrote: > > > On 1/5/24 15:56, Robert Richter wrote: > > See inline. > > > > On 05.01.24 15:07:46, Dave Jiang wrote: > >> cxl_port_perf_data_calculate() calls find_cxl_root() and does not > >> dereference the 'struct device' in the cxl_root->port. find_cxl_root() > >> calls get_device() and takes a reference on the port 'struct device' > >> member. Use the __free() macro to ensure the dereference happens. > >> > >> Fixes: 7a4f148dd8d5 ("cxl: Compute the entire CXL path latency and bandwidth data") > >> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> [..] > >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > >> index 0df5379cf02f..c6208aab452f 100644 > >> --- a/drivers/cxl/core/cdat.c > >> +++ b/drivers/cxl/core/cdat.c > >> @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, > >> struct xarray *dsmas_xa) > >> { > >> struct access_coordinate c; > >> - struct cxl_root *cxl_root; > >> struct dsmas_entry *dent; > >> int valid_entries = 0; > >> unsigned long index; > >> @@ -174,7 +173,11 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, > >> return rc; > >> } > >> > >> - cxl_root = find_cxl_root(port); > >> + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); > > > > I am not quite sure here, can a definition in the middle of a block? > > At least this is uncommon. > > From what I understand with the introduction of the scope-based > resource management, this kind of declaration is ok for that purpose? > Dan? The relaxation of the "variables declared in the middle of a block" came with the move to compiling with -std=gnu11: e8c07082a810 Kbuild: move to -std=gnu11 ...back in v5.18, and Peter took it further in his examples of using the cleanup.h helpers to keep "get/put" in the same statement, which means combining declaration and intialization, and means initialization may need to happen after the initial variable declaration block. I agree that it is a bit jarring to see given the legacy, but I think this a good reason for an exception, along with some of the others like declaring iterator variables in for () loops.
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c index 0df5379cf02f..c6208aab452f 100644 --- a/drivers/cxl/core/cdat.c +++ b/drivers/cxl/core/cdat.c @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, struct xarray *dsmas_xa) { struct access_coordinate c; - struct cxl_root *cxl_root; struct dsmas_entry *dent; int valid_entries = 0; unsigned long index; @@ -174,7 +173,11 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port, return rc; } - cxl_root = find_cxl_root(port); + struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port); + + if (!cxl_root) + return -ENODEV; + if (!cxl_root->ops || !cxl_root->ops->qos_class) return -EOPNOTSUPP;
cxl_port_perf_data_calculate() calls find_cxl_root() and does not dereference the 'struct device' in the cxl_root->port. find_cxl_root() calls get_device() and takes a reference on the port 'struct device' member. Use the __free() macro to ensure the dereference happens. Fixes: 7a4f148dd8d5 ("cxl: Compute the entire CXL path latency and bandwidth data") Reviewed-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- v5: - Update patch title (Dan) --- drivers/cxl/core/cdat.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)