| Message ID | 20240313083602.239201-6-ming4.li@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for root port RAS error handling | expand |
Li Ming wrote: > Introduce a new helper function called put_cxl_port() to instead of the > put_device() in order to release the device reference of struct cxl_port > got via get_device() in cxl_pci/mem_find_port(). > > Besides, use scope-based resource management __free() to drop the open > coded put_device() for each cxl_pci/mem_find_port(). > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Li Ming <ming4.li@intel.com> > --- > drivers/cxl/core/pci.c | 6 ++---- > drivers/cxl/core/port.c | 9 +++++++++ > drivers/cxl/cxl.h | 2 ++ > drivers/cxl/mem.c | 5 ++--- > drivers/cxl/pci.c | 12 +++++------- > 5 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 6c9c8d92f8f7..7254484330d2 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -902,15 +902,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds) > struct pci_dev *pdev = to_pci_dev(cxlds->dev); > struct aer_capability_regs aer_regs; > struct cxl_dport *dport; > - struct cxl_port *port; > int severity; > + struct cxl_port *port __free(put_cxl_port) = > + cxl_pci_find_port(pdev, &dport); > > - port = cxl_pci_find_port(pdev, &dport); > if (!port) Keep the declaration separated from the rest of the variable declarations like this: diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 0df09bd79408..2d1ef35fe99c 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -916,12 +916,11 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds) struct cxl_port *port; int severity; - port = cxl_pci_find_port(pdev, &dport); + struct cxl_port *port __free(put_cxl_port) = + cxl_pci_find_port(pdev, &dport); if (!port) return; - put_device(&port->dev); - if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs)) return; > return; > > - put_device(&port->dev); > - > if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs)) > return; > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index e59d9d37aa65..6e2fc2fce7c9 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1671,6 +1671,15 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, > } > EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL); > > +void put_cxl_port(struct cxl_port *port) > +{ > + if (!port) > + return; > + > + put_device(&port->dev); > +} > +EXPORT_SYMBOL_NS_GPL(put_cxl_port, CXL); > + > static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd, > struct cxl_port *port, int *target_map) > { > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index b6017c0c57b4..476158782e3e 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -743,6 +743,8 @@ struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev, > struct cxl_dport **dport); > struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, > struct cxl_dport **dport); > +void put_cxl_port(struct cxl_port *port); > +DEFINE_FREE(put_cxl_port, struct cxl_port *, if (_T) put_cxl_port(_T)) > bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd); > > struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port, > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index c5c9d8e0d88d..5aaa8ee2a46d 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev) > struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); > struct cxl_dev_state *cxlds = cxlmd->cxlds; > struct device *endpoint_parent; > - struct cxl_port *parent_port; > struct cxl_dport *dport; > struct dentry *dentry; > int rc; > @@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev) > if (rc) > return rc; > > - parent_port = cxl_mem_find_port(cxlmd, &dport); > + struct cxl_port *parent_port __free(put_cxl_port) = > + cxl_mem_find_port(cxlmd, &dport); > if (!parent_port) { > dev_err(dev, "CXL port topology not found\n"); > return -ENXIO; > @@ -170,7 +170,6 @@ static int cxl_mem_probe(struct device *dev) > rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); > unlock: > device_unlock(endpoint_parent); > - put_device(&parent_port->dev); > if (rc) > return rc; When converting a function to use cleanup helpers, it should try to remove all gotos, something like: @@ -159,21 +159,18 @@ static int cxl_mem_probe(struct device *dev) cxl_setup_parent_dport(dev, dport); - device_lock(endpoint_parent); - if (!endpoint_parent->driver) { - dev_err(dev, "CXL port topology %s not enabled\n", - dev_name(endpoint_parent)); - rc = -ENXIO; - goto unlock; + scoped_guard(device, endpoint_parent) { + if (!endpoint_parent->driver) { + dev_err(dev, "CXL port topology %s not enabled\n", + dev_name(endpoint_parent)); + return -ENXIO; + } + + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); + if (rc) + return rc; } - rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); -unlock: - device_unlock(endpoint_parent); - put_device(&parent_port->dev); - if (rc) - return rc; - if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) { rc = devm_cxl_add_nvdimm(cxlmd); if (rc == -ENODEV) > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 4fd1f207c84e..d0ec8c5b1e99 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -473,23 +473,21 @@ static bool is_cxl_restricted(struct pci_dev *pdev) > static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev, > struct cxl_register_map *map) > { > - struct cxl_port *port; > struct cxl_dport *dport; > resource_size_t component_reg_phys; > + struct cxl_port *port __free(put_cxl_port) = > + cxl_pci_find_port(pdev, &dport); > + > + if (!port) > + return -EPROBE_DEFER; > > *map = (struct cxl_register_map) { > .host = &pdev->dev, > .resource = CXL_RESOURCE_NONE, > }; > > - port = cxl_pci_find_port(pdev, &dport); > - if (!port) > - return -EPROBE_DEFER; > - Here again don't move the cxl_pci_find_port() earlier in the function, keep it after the assignment of @map.
On 3/15/2024 10:24 AM, Dan Williams wrote: > Li Ming wrote: >> Introduce a new helper function called put_cxl_port() to instead of the >> put_device() in order to release the device reference of struct cxl_port >> got via get_device() in cxl_pci/mem_find_port(). >> >> Besides, use scope-based resource management __free() to drop the open >> coded put_device() for each cxl_pci/mem_find_port(). >> >> Suggested-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Li Ming <ming4.li@intel.com> >> --- >> drivers/cxl/core/pci.c | 6 ++---- >> drivers/cxl/core/port.c | 9 +++++++++ >> drivers/cxl/cxl.h | 2 ++ >> drivers/cxl/mem.c | 5 ++--- >> drivers/cxl/pci.c | 12 +++++------- >> 5 files changed, 20 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >> index 6c9c8d92f8f7..7254484330d2 100644 >> --- a/drivers/cxl/core/pci.c >> +++ b/drivers/cxl/core/pci.c >> @@ -902,15 +902,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds) >> struct pci_dev *pdev = to_pci_dev(cxlds->dev); >> struct aer_capability_regs aer_regs; >> struct cxl_dport *dport; >> - struct cxl_port *port; >> int severity; >> + struct cxl_port *port __free(put_cxl_port) = >> + cxl_pci_find_port(pdev, &dport); >> >> - port = cxl_pci_find_port(pdev, &dport); >> if (!port) > > Keep the declaration separated from the rest of the variable > declarations like this: > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index 0df09bd79408..2d1ef35fe99c 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -916,12 +916,11 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds) > struct cxl_port *port; > int severity; > > - port = cxl_pci_find_port(pdev, &dport); > + struct cxl_port *port __free(put_cxl_port) = > + cxl_pci_find_port(pdev, &dport); > if (!port) > return; > > - put_device(&port->dev); > - > if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs)) > return; > >> return; >> >> - put_device(&port->dev); >> - >> if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs)) >> return; >> >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c >> index e59d9d37aa65..6e2fc2fce7c9 100644 >> --- a/drivers/cxl/core/port.c >> +++ b/drivers/cxl/core/port.c >> @@ -1671,6 +1671,15 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, >> } >> EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL); >> >> +void put_cxl_port(struct cxl_port *port) >> +{ >> + if (!port) >> + return; >> + >> + put_device(&port->dev); >> +} >> +EXPORT_SYMBOL_NS_GPL(put_cxl_port, CXL); >> + >> static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd, >> struct cxl_port *port, int *target_map) >> { >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index b6017c0c57b4..476158782e3e 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -743,6 +743,8 @@ struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev, >> struct cxl_dport **dport); >> struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, >> struct cxl_dport **dport); >> +void put_cxl_port(struct cxl_port *port); >> +DEFINE_FREE(put_cxl_port, struct cxl_port *, if (_T) put_cxl_port(_T)) >> bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd); >> >> struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port, >> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c >> index c5c9d8e0d88d..5aaa8ee2a46d 100644 >> --- a/drivers/cxl/mem.c >> +++ b/drivers/cxl/mem.c >> @@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev) >> struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); >> struct cxl_dev_state *cxlds = cxlmd->cxlds; >> struct device *endpoint_parent; >> - struct cxl_port *parent_port; >> struct cxl_dport *dport; >> struct dentry *dentry; >> int rc; >> @@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev) >> if (rc) >> return rc; >> >> - parent_port = cxl_mem_find_port(cxlmd, &dport); >> + struct cxl_port *parent_port __free(put_cxl_port) = >> + cxl_mem_find_port(cxlmd, &dport); >> if (!parent_port) { >> dev_err(dev, "CXL port topology not found\n"); >> return -ENXIO; >> @@ -170,7 +170,6 @@ static int cxl_mem_probe(struct device *dev) >> rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); >> unlock: >> device_unlock(endpoint_parent); >> - put_device(&parent_port->dev); >> if (rc) >> return rc; > > When converting a function to use cleanup helpers, it should try to > remove all gotos, something like: > > @@ -159,21 +159,18 @@ static int cxl_mem_probe(struct device *dev) > > cxl_setup_parent_dport(dev, dport); > > - device_lock(endpoint_parent); > - if (!endpoint_parent->driver) { > - dev_err(dev, "CXL port topology %s not enabled\n", > - dev_name(endpoint_parent)); > - rc = -ENXIO; > - goto unlock; > + scoped_guard(device, endpoint_parent) { > + if (!endpoint_parent->driver) { > + dev_err(dev, "CXL port topology %s not enabled\n", > + dev_name(endpoint_parent)); > + return -ENXIO; > + } > + > + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); > + if (rc) > + return rc; > } > > - rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); > -unlock: > - device_unlock(endpoint_parent); > - put_device(&parent_port->dev); > - if (rc) > - return rc; > - > if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) { > rc = devm_cxl_add_nvdimm(cxlmd); > if (rc == -ENODEV) > > >> >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c >> index 4fd1f207c84e..d0ec8c5b1e99 100644 >> --- a/drivers/cxl/pci.c >> +++ b/drivers/cxl/pci.c >> @@ -473,23 +473,21 @@ static bool is_cxl_restricted(struct pci_dev *pdev) >> static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev, >> struct cxl_register_map *map) >> { >> - struct cxl_port *port; >> struct cxl_dport *dport; >> resource_size_t component_reg_phys; >> + struct cxl_port *port __free(put_cxl_port) = >> + cxl_pci_find_port(pdev, &dport); >> + >> + if (!port) >> + return -EPROBE_DEFER; >> >> *map = (struct cxl_register_map) { >> .host = &pdev->dev, >> .resource = CXL_RESOURCE_NONE, >> }; >> >> - port = cxl_pci_find_port(pdev, &dport); >> - if (!port) >> - return -EPROBE_DEFER; >> - > > Here again don't move the cxl_pci_find_port() earlier in the function, > keep it after the assignment of @map. Thanks, I will do that.
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index 6c9c8d92f8f7..7254484330d2 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -902,15 +902,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds) struct pci_dev *pdev = to_pci_dev(cxlds->dev); struct aer_capability_regs aer_regs; struct cxl_dport *dport; - struct cxl_port *port; int severity; + struct cxl_port *port __free(put_cxl_port) = + cxl_pci_find_port(pdev, &dport); - port = cxl_pci_find_port(pdev, &dport); if (!port) return; - put_device(&port->dev); - if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs)) return; diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index e59d9d37aa65..6e2fc2fce7c9 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1671,6 +1671,15 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, } EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL); +void put_cxl_port(struct cxl_port *port) +{ + if (!port) + return; + + put_device(&port->dev); +} +EXPORT_SYMBOL_NS_GPL(put_cxl_port, CXL); + static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd, struct cxl_port *port, int *target_map) { diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index b6017c0c57b4..476158782e3e 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -743,6 +743,8 @@ struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev, struct cxl_dport **dport); struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, struct cxl_dport **dport); +void put_cxl_port(struct cxl_port *port); +DEFINE_FREE(put_cxl_port, struct cxl_port *, if (_T) put_cxl_port(_T)) bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd); struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port, diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index c5c9d8e0d88d..5aaa8ee2a46d 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev) struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); struct cxl_dev_state *cxlds = cxlmd->cxlds; struct device *endpoint_parent; - struct cxl_port *parent_port; struct cxl_dport *dport; struct dentry *dentry; int rc; @@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev) if (rc) return rc; - parent_port = cxl_mem_find_port(cxlmd, &dport); + struct cxl_port *parent_port __free(put_cxl_port) = + cxl_mem_find_port(cxlmd, &dport); if (!parent_port) { dev_err(dev, "CXL port topology not found\n"); return -ENXIO; @@ -170,7 +170,6 @@ static int cxl_mem_probe(struct device *dev) rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); unlock: device_unlock(endpoint_parent); - put_device(&parent_port->dev); if (rc) return rc; diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 4fd1f207c84e..d0ec8c5b1e99 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -473,23 +473,21 @@ static bool is_cxl_restricted(struct pci_dev *pdev) static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev, struct cxl_register_map *map) { - struct cxl_port *port; struct cxl_dport *dport; resource_size_t component_reg_phys; + struct cxl_port *port __free(put_cxl_port) = + cxl_pci_find_port(pdev, &dport); + + if (!port) + return -EPROBE_DEFER; *map = (struct cxl_register_map) { .host = &pdev->dev, .resource = CXL_RESOURCE_NONE, }; - port = cxl_pci_find_port(pdev, &dport); - if (!port) - return -EPROBE_DEFER; - component_reg_phys = cxl_rcd_component_reg_phys(&pdev->dev, dport); - put_device(&port->dev); - if (component_reg_phys == CXL_RESOURCE_NONE) return -ENXIO;
Introduce a new helper function called put_cxl_port() to instead of the put_device() in order to release the device reference of struct cxl_port got via get_device() in cxl_pci/mem_find_port(). Besides, use scope-based resource management __free() to drop the open coded put_device() for each cxl_pci/mem_find_port(). Suggested-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Li Ming <ming4.li@intel.com> --- drivers/cxl/core/pci.c | 6 ++---- drivers/cxl/core/port.c | 9 +++++++++ drivers/cxl/cxl.h | 2 ++ drivers/cxl/mem.c | 5 ++--- drivers/cxl/pci.c | 12 +++++------- 5 files changed, 20 insertions(+), 14 deletions(-)