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); 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); if (!pdev) return; Ira