On Thu, Jan 04, 2024 at 01:46:56PM -0800, Ira Weiny wrote: > Dan Williams wrote: > > Bjorn Helgaas wrote: > > [..] > > > > --- > > > > PCI: Introduce cleanup helpers for device reference counts and locks > > > > > > > > The "goto error" pattern is notorious for introducing subtle resource > > > > leaks. Use the new cleanup.h helpers for PCI device reference counts and > > > > locks. > > > > > > > > Similar to the new put_device() and device_lock() cleanup helpers, > > > > __free(put_device) and guard(device), define the same for PCI devices, > > > > __free(pci_dev_put) and guard(pci_dev). These helpers eliminate the > > > > need for "goto free;" and "goto unlock;" patterns. For example, A > > > > 'struct pci_dev *' instance declared as: > > > > > > > > struct pci_dev *pdev __free(pci_dev_put) = NULL; > > > > > > I see several similar __free() uses with NULL initializations in gpio, > > > but I think this idiom would be slightly improved if the __free() > > > function were more closely associated with the actual pci_dev_get(): > > > > > > struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...); > > > > > > Not always possible, I know, but easier to analyze when it is. > > > > I tend to agree, but it does lead to some long lines, for example: > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index 4fd1f207c84e..549ba4b8294e 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type, > > struct cxl_cper_event_rec *rec) > > { > > struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; > > - struct pci_dev *pdev __free(pci_dev_put) = NULL; > > enum cxl_event_log_type log_type; > > struct cxl_dev_state *cxlds; > > unsigned int devfn; > > u32 hdr_flags; > > > > devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); > > - pdev = pci_get_domain_bus_and_slot(device_id->segment_num, > > - device_id->bus_num, devfn); > > + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot( > > + device_id->segment_num, device_id->bus_num, devfn); > > if (!pdev) > > return; > > > > ...so I think people are choosing the "... __free(x) = NULL;" style for > > code density readability. > > > > Also in this case we need devfn assigned first. > > Is the above patch compliant with current style guidelines? > > Or would it be better to do? > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index b14237f824cf..8a180c6abb67 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type, > struct cxl_cper_event_rec *rec) > { > struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; > - struct pci_dev *pdev __free(pci_dev_put) = NULL; > enum cxl_event_log_type log_type; > struct cxl_dev_state *cxlds; > - unsigned int devfn; > + unsigned int devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); > + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot( > + device_id->segment_num, > + device_id->bus_num, devfn); I don't really care about this specific instance; my comment was more about the commit log for the "Define scope based management functions" patch, thinking maybe the example could encourage get/put togetherness when it's practical. Bjorn