| Message ID | 170261791914.1714654.6447680285357545638.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl/pci: Kill a goto in read_cdat_data() | expand |
On 12/14/23 22:25, Dan Williams wrote: > Given there was already one bug here around passing the wrong value in > the cleanup path, remove the goto with the use of the __free() helper. > Note that since the cleanup helper only passes a single pointer as an > argument the auto @tbl variable needs to also include the @dev argument > to devm_kfree(). > > The open coded math to shift the port->cdat.table to the relevant part > of the response buffer is saved to a follow-on patch to cleanup. > > The "devm_buf" scheme can maybe move to include/linux/device.h one day, > but the expectation of using devm_kfree() in an error path for the same > function that performed the allocation is a rare pattern. Many > devm_kfree() usages are at object mid-life or end-of-life time, not init > paths. > > Cc: Robert Richter <rrichter@amd.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/pci.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 37e1652afbc7..dd938a60a2de 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -606,6 +606,12 @@ static unsigned char cdat_checksum(void *buf, size_t size) > return sum; > } > > +struct devm_buf { > + struct device *dev; > + void *p; > +}; > +DEFINE_FREE(devm, struct devm_buf, if (_T.p) devm_kfree(_T.dev, _T.p)) > + > /** > * read_cdat_data - Read the CDAT data on this port > * @port: Port to read data from > @@ -614,13 +620,13 @@ static unsigned char cdat_checksum(void *buf, size_t size) > */ > void read_cdat_data(struct cxl_port *port) > { > + struct devm_buf rsp __free(devm) = { .dev = &port->dev }; > struct device *uport = port->uport_dev; > struct device *dev = &port->dev; > struct pci_doe_mb *cdat_doe; > struct pci_dev *pdev = NULL; > struct cxl_memdev *cxlmd; > size_t cdat_length; > - void *cdat_table, *cdat_buf; > int rc; > > if (is_cxl_memdev(uport)) { > @@ -651,26 +657,19 @@ void read_cdat_data(struct cxl_port *port) > return; > } > > - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > - if (!cdat_buf) > + rsp.p = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > + if (!rsp.p) > return; > > - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length); > + rc = cxl_cdat_read_table(dev, cdat_doe, rsp.p, &cdat_length); > if (rc) > - goto err; > + return; > > - cdat_table = cdat_buf + sizeof(__le32); > - if (cdat_checksum(cdat_table, cdat_length)) > - goto err; > + if (cdat_checksum(rsp.p + sizeof(__le32), cdat_length)) > + return; > > - port->cdat.table = cdat_table; > + port->cdat.table = no_free_ptr(rsp.p) + sizeof(__le32); > port->cdat.length = cdat_length; > - return; > - > -err: > - /* Don't leave table data allocated on error */ > - devm_kfree(dev, cdat_buf); > - dev_err(dev, "Failed to read/validate CDAT.\n"); > } > EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL); > > >
Dan Williams wrote: > Given there was already one bug here around passing the wrong value in > the cleanup path, remove the goto with the use of the __free() helper. > Note that since the cleanup helper only passes a single pointer as an > argument the auto @tbl variable needs to also include the @dev argument > to devm_kfree(). > > The open coded math to shift the port->cdat.table to the relevant part > of the response buffer is saved to a follow-on patch to cleanup. > > The "devm_buf" scheme can maybe move to include/linux/device.h one day, > but the expectation of using devm_kfree() in an error path for the same > function that performed the allocation is a rare pattern. Many > devm_kfree() usages are at object mid-life or end-of-life time, not init > paths. > > Cc: Robert Richter <rrichter@amd.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/pci.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 37e1652afbc7..dd938a60a2de 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -606,6 +606,12 @@ static unsigned char cdat_checksum(void *buf, size_t size) > return sum; > } > > +struct devm_buf { > + struct device *dev; > + void *p; > +}; > +DEFINE_FREE(devm, struct devm_buf, if (_T.p) devm_kfree(_T.dev, _T.p)) I wonder if this deserves more wide spread use? [snip] > @@ -651,26 +657,19 @@ void read_cdat_data(struct cxl_port *port) > return; > } > > - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > - if (!cdat_buf) > + rsp.p = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > + if (!rsp.p) > return; > > - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length); > + rc = cxl_cdat_read_table(dev, cdat_doe, rsp.p, &cdat_length); > if (rc) > - goto err; > + return; > > - cdat_table = cdat_buf + sizeof(__le32); > - if (cdat_checksum(cdat_table, cdat_length)) > - goto err; > + if (cdat_checksum(rsp.p + sizeof(__le32), cdat_length)) > + return; > > - port->cdat.table = cdat_table; > + port->cdat.table = no_free_ptr(rsp.p) + sizeof(__le32); It is unfortunate that we can't get rid of the mystery __le32 offset here... I'm not seeing a quick way, so... Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Ira Weiny wrote: > Dan Williams wrote: > > Given there was already one bug here around passing the wrong value in > > the cleanup path, remove the goto with the use of the __free() helper. > > Note that since the cleanup helper only passes a single pointer as an > > argument the auto @tbl variable needs to also include the @dev argument > > to devm_kfree(). > > > > The open coded math to shift the port->cdat.table to the relevant part > > of the response buffer is saved to a follow-on patch to cleanup. > > > > The "devm_buf" scheme can maybe move to include/linux/device.h one day, > > but the expectation of using devm_kfree() in an error path for the same > > function that performed the allocation is a rare pattern. Many > > devm_kfree() usages are at object mid-life or end-of-life time, not init > > paths. > > > > Cc: Robert Richter <rrichter@amd.com> > > Cc: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/cxl/core/pci.c | 29 ++++++++++++++--------------- > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > > index 37e1652afbc7..dd938a60a2de 100644 > > --- a/drivers/cxl/core/pci.c > > +++ b/drivers/cxl/core/pci.c > > @@ -606,6 +606,12 @@ static unsigned char cdat_checksum(void *buf, size_t size) > > return sum; > > } > > > > +struct devm_buf { > > + struct device *dev; > > + void *p; > > +}; > > +DEFINE_FREE(devm, struct devm_buf, if (_T.p) devm_kfree(_T.dev, _T.p)) > > I wonder if this deserves more wide spread use? Yeah, I just think there might be a path to do something more sophisticated like DEFINE_LOCK_GUARD_1(), but I am not savvy enough at the moment to figure out how. Exposing the definition of devm_buf to the caller is more shortcut than global recommendation. > > [snip] > > > @@ -651,26 +657,19 @@ void read_cdat_data(struct cxl_port *port) > > return; > > } > > > > - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > > - if (!cdat_buf) > > + rsp.p = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > > + if (!rsp.p) > > return; > > > > - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length); > > + rc = cxl_cdat_read_table(dev, cdat_doe, rsp.p, &cdat_length); > > if (rc) > > - goto err; > > + return; > > > > - cdat_table = cdat_buf + sizeof(__le32); > > - if (cdat_checksum(cdat_table, cdat_length)) > > - goto err; > > + if (cdat_checksum(rsp.p + sizeof(__le32), cdat_length)) > > + return; > > > > - port->cdat.table = cdat_table; > > + port->cdat.table = no_free_ptr(rsp.p) + sizeof(__le32); > > It is unfortunate that we can't get rid of the mystery __le32 offset > here... I'm not seeing a quick way, so... That's what Robert's cleanup is all about, then maybe this can become less magical with something like this? diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index dd938a60a2de..2a05666fbdd7 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -606,9 +606,9 @@ static unsigned char cdat_checksum(void *buf, size_t size) return sum; } -struct devm_buf { +struct devm_cdat { struct device *dev; - void *p; + struct cdat_doe_response *p; }; DEFINE_FREE(devm, struct devm_buf, if (_T.p) devm_kfree(_T.dev, _T.p)) @@ -620,7 +620,7 @@ DEFINE_FREE(devm, struct devm_buf, if (_T.p) devm_kfree(_T.dev, _T.p)) */ void read_cdat_data(struct cxl_port *port) { - struct devm_buf rsp __free(devm) = { .dev = &port->dev }; + struct devm_cdat rsp __free(devm) = { .dev = &port->dev }; struct device *uport = port->uport_dev; struct device *dev = &port->dev; struct pci_doe_mb *cdat_doe; @@ -665,10 +665,10 @@ void read_cdat_data(struct cxl_port *port) if (rc) return; - if (cdat_checksum(rsp.p + sizeof(__le32), cdat_length)) + if (cdat_checksum(&rsp.p->cdat_table, cdat_length)) return; - port->cdat.table = no_free_ptr(rsp.p) + sizeof(__le32); + port->cdat.table = no_free_ptr(rsp.p) + offset_of(rsp.p->cdat_table); port->cdat.length = cdat_length; } EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
On 14.12.23 21:25:19, Dan Williams wrote: > Given there was already one bug here around passing the wrong value in > the cleanup path, remove the goto with the use of the __free() helper. > Note that since the cleanup helper only passes a single pointer as an > argument the auto @tbl variable needs to also include the @dev argument > to devm_kfree(). > > The open coded math to shift the port->cdat.table to the relevant part > of the response buffer is saved to a follow-on patch to cleanup. > > The "devm_buf" scheme can maybe move to include/linux/device.h one day, > but the expectation of using devm_kfree() in an error path for the same > function that performed the allocation is a rare pattern. Many > devm_kfree() usages are at object mid-life or end-of-life time, not init > paths. > > Cc: Robert Richter <rrichter@amd.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/pci.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 37e1652afbc7..dd938a60a2de 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -606,6 +606,12 @@ static unsigned char cdat_checksum(void *buf, size_t size) > return sum; > } > > +struct devm_buf { > + struct device *dev; > + void *p; Maybe just name that buf, the accessor would be rsp.buf then, better readable. > +}; > +DEFINE_FREE(devm, struct devm_buf, if (_T.p) devm_kfree(_T.dev, _T.p)) > + > /** > * read_cdat_data - Read the CDAT data on this port > * @port: Port to read data from > @@ -614,13 +620,13 @@ static unsigned char cdat_checksum(void *buf, size_t size) > */ > void read_cdat_data(struct cxl_port *port) > { > + struct devm_buf rsp __free(devm) = { .dev = &port->dev }; > struct device *uport = port->uport_dev; > struct device *dev = &port->dev; > struct pci_doe_mb *cdat_doe; > struct pci_dev *pdev = NULL; > struct cxl_memdev *cxlmd; > size_t cdat_length; > - void *cdat_table, *cdat_buf; > int rc; > > if (is_cxl_memdev(uport)) { > @@ -651,26 +657,19 @@ void read_cdat_data(struct cxl_port *port) > return; > } > > - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > - if (!cdat_buf) > + rsp.p = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > + if (!rsp.p) > return; > > - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length); > + rc = cxl_cdat_read_table(dev, cdat_doe, rsp.p, &cdat_length); > if (rc) > - goto err; > + return; > > - cdat_table = cdat_buf + sizeof(__le32); > - if (cdat_checksum(cdat_table, cdat_length)) > - goto err; > + if (cdat_checksum(rsp.p + sizeof(__le32), cdat_length)) > + return; > > - port->cdat.table = cdat_table; > + port->cdat.table = no_free_ptr(rsp.p) + sizeof(__le32); > port->cdat.length = cdat_length; > - return; > - > -err: > - /* Don't leave table data allocated on error */ > - devm_kfree(dev, cdat_buf); > - dev_err(dev, "Failed to read/validate CDAT.\n"); An error message is misssing now. Could be implemented after calling read_cdat_data. But that duplicates code. > } > EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL); I am not sure if using __free() is more safe and better readable here at all, it introduces more complexity hidden behind macros and compiler behavior plus an additional struct and __free() ordering issues, just to remove some gotos. I gotos are 'bad', a helper function could be implemented alternatively. On the other side, the existing code look straight forward. See the 2 direct comments to the code I made. However, the implementation itself looks correct to me, so for that: Reviewed-by: Robert Richter <rrichter@amd.com> Should I rebase my other CDAT patches on top of this one and send a v3 of it? Thanks, -Robert
Robert Richter wrote: > On 14.12.23 21:25:19, Dan Williams wrote: [snip] > > + > > /** > > * read_cdat_data - Read the CDAT data on this port > > * @port: Port to read data from > > @@ -614,13 +620,13 @@ static unsigned char cdat_checksum(void *buf, size_t size) > > */ > > void read_cdat_data(struct cxl_port *port) > > { > > + struct devm_buf rsp __free(devm) = { .dev = &port->dev }; > > struct device *uport = port->uport_dev; > > struct device *dev = &port->dev; > > struct pci_doe_mb *cdat_doe; > > struct pci_dev *pdev = NULL; > > struct cxl_memdev *cxlmd; > > size_t cdat_length; > > - void *cdat_table, *cdat_buf; > > int rc; > > > > if (is_cxl_memdev(uport)) { > > @@ -651,26 +657,19 @@ void read_cdat_data(struct cxl_port *port) > > return; > > } > > > > - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > > - if (!cdat_buf) > > + rsp.p = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > > + if (!rsp.p) > > return; > > > > - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length); > > + rc = cxl_cdat_read_table(dev, cdat_doe, rsp.p, &cdat_length); > > if (rc) > > - goto err; > > + return; > > > > - cdat_table = cdat_buf + sizeof(__le32); > > - if (cdat_checksum(cdat_table, cdat_length)) > > - goto err; > > + if (cdat_checksum(rsp.p + sizeof(__le32), cdat_length)) > > + return; > > > > - port->cdat.table = cdat_table; > > + port->cdat.table = no_free_ptr(rsp.p) + sizeof(__le32); > > port->cdat.length = cdat_length; > > - return; > > - > > -err: > > - /* Don't leave table data allocated on error */ > > - devm_kfree(dev, cdat_buf); > > - dev_err(dev, "Failed to read/validate CDAT.\n"); > > An error message is misssing now. Could be implemented after calling > read_cdat_data. But that duplicates code. > > > } > > EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL); > > I am not sure if using __free() is more safe and better readable here > at all, it introduces more complexity hidden behind macros and > compiler behavior plus an additional struct and __free() ordering > issues, just to remove some gotos. FWIW I understand your sentiment. I think it will take me a bit of time to get used to reading code like this as well. But I think the cleanup.h code style is a safer way of structuring code as it is modified over time. I think other modern languages have proven this out. Another example in the kernel is the devm_* functions, they do help in the long run even though I've had to really change my way of thinking. So, for me, I'm going to work at retraining my brain with this new stuff. Ira
Robert Richter wrote: > On 14.12.23 21:25:19, Dan Williams wrote: > > Given there was already one bug here around passing the wrong value in > > the cleanup path, remove the goto with the use of the __free() helper. > > Note that since the cleanup helper only passes a single pointer as an > > argument the auto @tbl variable needs to also include the @dev argument > > to devm_kfree(). > > > > The open coded math to shift the port->cdat.table to the relevant part > > of the response buffer is saved to a follow-on patch to cleanup. > > > > The "devm_buf" scheme can maybe move to include/linux/device.h one day, > > but the expectation of using devm_kfree() in an error path for the same > > function that performed the allocation is a rare pattern. Many > > devm_kfree() usages are at object mid-life or end-of-life time, not init > > paths. > > > > Cc: Robert Richter <rrichter@amd.com> > > Cc: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/cxl/core/pci.c | 29 ++++++++++++++--------------- > > 1 file changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > > index 37e1652afbc7..dd938a60a2de 100644 > > --- a/drivers/cxl/core/pci.c > > +++ b/drivers/cxl/core/pci.c > > @@ -606,6 +606,12 @@ static unsigned char cdat_checksum(void *buf, size_t size) > > return sum; > > } > > > > +struct devm_buf { > > + struct device *dev; > > + void *p; > > Maybe just name that buf, the accessor would be rsp.buf then, better > readable. > > > +}; > > +DEFINE_FREE(devm, struct devm_buf, if (_T.p) devm_kfree(_T.dev, _T.p)) > > + > > /** > > * read_cdat_data - Read the CDAT data on this port > > * @port: Port to read data from > > @@ -614,13 +620,13 @@ static unsigned char cdat_checksum(void *buf, size_t size) > > */ > > void read_cdat_data(struct cxl_port *port) > > { > > + struct devm_buf rsp __free(devm) = { .dev = &port->dev }; > > struct device *uport = port->uport_dev; > > struct device *dev = &port->dev; > > struct pci_doe_mb *cdat_doe; > > struct pci_dev *pdev = NULL; > > struct cxl_memdev *cxlmd; > > size_t cdat_length; > > - void *cdat_table, *cdat_buf; > > int rc; > > > > if (is_cxl_memdev(uport)) { > > @@ -651,26 +657,19 @@ void read_cdat_data(struct cxl_port *port) > > return; > > } > > > > - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > > - if (!cdat_buf) > > + rsp.p = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); > > + if (!rsp.p) > > return; > > > > - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length); > > + rc = cxl_cdat_read_table(dev, cdat_doe, rsp.p, &cdat_length); > > if (rc) > > - goto err; > > + return; > > > > - cdat_table = cdat_buf + sizeof(__le32); > > - if (cdat_checksum(cdat_table, cdat_length)) > > - goto err; > > + if (cdat_checksum(rsp.p + sizeof(__le32), cdat_length)) > > + return; > > > > - port->cdat.table = cdat_table; > > + port->cdat.table = no_free_ptr(rsp.p) + sizeof(__le32); > > port->cdat.length = cdat_length; > > - return; > > - > > -err: > > - /* Don't leave table data allocated on error */ > > - devm_kfree(dev, cdat_buf); > > - dev_err(dev, "Failed to read/validate CDAT.\n"); > > An error message is misssing now. Could be implemented after calling > read_cdat_data. But that duplicates code. > > > } > > EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL); > > I am not sure if using __free() is more safe and better readable here > at all, it introduces more complexity hidden behind macros and > compiler behavior plus an additional struct and __free() ordering > issues, just to remove some gotos. The additional struct critique is a valid concern. I think that really wants to wait until devm has native cleanup support. > I gotos are 'bad', a helper function could be implemented > alternatively. On the other side, the existing code look straight > forward. I took a look at elminating the struct, but it still turns out awkward particularly because of the fact that the buffer to release is not identical to the port->cdat.table pointer. > See the 2 direct comments to the code I made. However, the > implementation itself looks correct to me, so for that: > > Reviewed-by: Robert Richter <rrichter@amd.com> > > Should I rebase my other CDAT patches on top of this one and send a v3 > of it? I would say keep the goto for now because your cleanup to eliminate the awkward "cdat_buf + sizeof(__le32)" makes it easier to use cleanup.h helpers as a follow-on.
Dan Williams wrote: > Robert Richter wrote: [..] > > Should I rebase my other CDAT patches on top of this one and send a v3 > > of it? > > I would say keep the goto for now because your cleanup to eliminate the > awkward "cdat_buf + sizeof(__le32)" makes it easier to use cleanup.h > helpers as a follow-on. In other words, I drop this patch and await your cleanup based on the current baseline.
On 17.01.24 10:25:47, Dan Williams wrote: > Dan Williams wrote: > > Robert Richter wrote: > [..] > > > Should I rebase my other CDAT patches on top of this one and send a v3 > > > of it? > > > > I would say keep the goto for now because your cleanup to eliminate the > > awkward "cdat_buf + sizeof(__le32)" makes it easier to use cleanup.h > > helpers as a follow-on. > > In other words, I drop this patch and await your cleanup based on the > current baseline. Posted it already here, but burried in the series as I split v1 and added another (unrelated) CDAT patch: https://patchwork.kernel.org/project/cxl/list/?series=815050 That v2 is already on top of cxl/next. The first 2 patches can be applied individually independent of patch #3. Please take a look. Thanks, -Robert
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 37e1652afbc7..dd938a60a2de 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -606,6 +606,12 @@ static unsigned char cdat_checksum(void *buf, size_t size) return sum; } +struct devm_buf { + struct device *dev; + void *p; +}; +DEFINE_FREE(devm, struct devm_buf, if (_T.p) devm_kfree(_T.dev, _T.p)) + /** * read_cdat_data - Read the CDAT data on this port * @port: Port to read data from @@ -614,13 +620,13 @@ static unsigned char cdat_checksum(void *buf, size_t size) */ void read_cdat_data(struct cxl_port *port) { + struct devm_buf rsp __free(devm) = { .dev = &port->dev }; struct device *uport = port->uport_dev; struct device *dev = &port->dev; struct pci_doe_mb *cdat_doe; struct pci_dev *pdev = NULL; struct cxl_memdev *cxlmd; size_t cdat_length; - void *cdat_table, *cdat_buf; int rc; if (is_cxl_memdev(uport)) { @@ -651,26 +657,19 @@ void read_cdat_data(struct cxl_port *port) return; } - cdat_buf = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); - if (!cdat_buf) + rsp.p = devm_kzalloc(dev, cdat_length + sizeof(__le32), GFP_KERNEL); + if (!rsp.p) return; - rc = cxl_cdat_read_table(dev, cdat_doe, cdat_buf, &cdat_length); + rc = cxl_cdat_read_table(dev, cdat_doe, rsp.p, &cdat_length); if (rc) - goto err; + return; - cdat_table = cdat_buf + sizeof(__le32); - if (cdat_checksum(cdat_table, cdat_length)) - goto err; + if (cdat_checksum(rsp.p + sizeof(__le32), cdat_length)) + return; - port->cdat.table = cdat_table; + port->cdat.table = no_free_ptr(rsp.p) + sizeof(__le32); port->cdat.length = cdat_length; - return; - -err: - /* Don't leave table data allocated on error */ - devm_kfree(dev, cdat_buf); - dev_err(dev, "Failed to read/validate CDAT.\n"); } EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
Given there was already one bug here around passing the wrong value in the cleanup path, remove the goto with the use of the __free() helper. Note that since the cleanup helper only passes a single pointer as an argument the auto @tbl variable needs to also include the @dev argument to devm_kfree(). The open coded math to shift the port->cdat.table to the relevant part of the response buffer is saved to a follow-on patch to cleanup. The "devm_buf" scheme can maybe move to include/linux/device.h one day, but the expectation of using devm_kfree() in an error path for the same function that performed the allocation is a rare pattern. Many devm_kfree() usages are at object mid-life or end-of-life time, not init paths. Cc: Robert Richter <rrichter@amd.com> Cc: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/pci.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)