| Message ID | 20201117012552.262149-2-matthew.gerlach@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | fpga: dfl: optional VSEC for start of dfl | expand |
> Subject: [PATCH 1/2] fpga: dfl: refactor cci_enumerate_feature_devs() > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > In preparation of looking for dfls based on a vendor > specific pcie capability, move code that assumes > Bar0/offset0 as start of DFL to its own function. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > --- > drivers/fpga/dfl-pci.c | 86 ++++++++++++++++++++++++------------------ > 1 file changed, 49 insertions(+), 37 deletions(-) > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c > index a2203d03c9e2..b1b157b41942 100644 > --- a/drivers/fpga/dfl-pci.c > +++ b/drivers/fpga/dfl-pci.c > @@ -119,49 +119,20 @@ static int *cci_pci_create_irq_table(struct pci_dev > *pcidev, unsigned int nvec) > return table; > } > > -/* enumerate feature devices under pci device */ > -static int cci_enumerate_feature_devs(struct pci_dev *pcidev) > +static int find_dfl_in_bar0(struct pci_dev *pcidev, > + struct dfl_fpga_enum_info *info) > { > - struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); > - int port_num, bar, i, nvec, ret = 0; > - struct dfl_fpga_enum_info *info; > - struct dfl_fpga_cdev *cdev; > resource_size_t start, len; > + int port_num, bar, i; > void __iomem *base; > - int *irq_table; > + int ret = 0; > u32 offset; > u64 v; > > - /* allocate enumeration info via pci_dev */ > - info = dfl_fpga_enum_info_alloc(&pcidev->dev); > - if (!info) > - return -ENOMEM; > - > - /* add irq info for enumeration if the device support irq */ > - nvec = cci_pci_alloc_irq(pcidev); > - if (nvec < 0) { > - dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec); > - ret = nvec; > - goto enum_info_free_exit; > - } else if (nvec) { > - irq_table = cci_pci_create_irq_table(pcidev, nvec); > - if (!irq_table) { > - ret = -ENOMEM; > - goto irq_free_exit; > - } > - > - ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table); > - kfree(irq_table); > - if (ret) > - goto irq_free_exit; > - } > - > - /* start to find Device Feature List in Bar 0 */ > + /* start to find Device Feature List from Bar 0 */ > base = cci_pci_ioremap_bar0(pcidev); > - if (!base) { > - ret = -ENOMEM; > - goto irq_free_exit; > - } > + if (!base) > + return -ENOMEM; > > /* > * PF device has FME and Ports/AFUs, and VF device only has one > @@ -208,12 +179,53 @@ static int cci_enumerate_feature_devs(struct > pci_dev *pcidev) > dfl_fpga_enum_info_add_dfl(info, start, len); > } else { > ret = -ENODEV; > - goto irq_free_exit; > } > > /* release I/O mappings for next step enumeration */ > pcim_iounmap_regions(pcidev, BIT(0)); > > + We don't need 2 blank line here, remove one please. > + return ret; > +} > + > +/* enumerate feature devices under pci device */ > +static int cci_enumerate_feature_devs(struct pci_dev *pcidev) > +{ > + struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); > + struct dfl_fpga_enum_info *info; > + struct dfl_fpga_cdev *cdev; > + int nvec, ret = 0; > + int *irq_table; > + > + /* allocate enumeration info via pci_dev */ > + info = dfl_fpga_enum_info_alloc(&pcidev->dev); > + if (!info) > + return -ENOMEM; > + > + /* add irq info for enumeration if the device support irq */ > + nvec = cci_pci_alloc_irq(pcidev); > + if (nvec < 0) { > + dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec); > + ret = nvec; > + goto enum_info_free_exit; > + } else if (nvec) { > + irq_table = cci_pci_create_irq_table(pcidev, nvec); > + if (!irq_table) { > + ret = -ENOMEM; > + goto irq_free_exit; > + } > + > + ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table); > + kfree(irq_table); > + if (ret) > + goto irq_free_exit; > + } > + > + ret = find_dfl_in_bar0(pcidev, info); > + Remove this blank line, and maybe switch to a better function name here. Thanks Hao > + if (ret) > + goto irq_free_exit; > + > /* start enumeration with prepared enumeration information */ > cdev = dfl_fpga_feature_devs_enumerate(info); > if (IS_ERR(cdev)) { > -- > 2.25.2
On Tue, 17 Nov 2020, Wu, Hao wrote: >> Subject: [PATCH 1/2] fpga: dfl: refactor cci_enumerate_feature_devs() >> >> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> >> In preparation of looking for dfls based on a vendor >> specific pcie capability, move code that assumes >> Bar0/offset0 as start of DFL to its own function. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- >> drivers/fpga/dfl-pci.c | 86 ++++++++++++++++++++++++------------------ >> 1 file changed, 49 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c >> index a2203d03c9e2..b1b157b41942 100644 >> --- a/drivers/fpga/dfl-pci.c >> +++ b/drivers/fpga/dfl-pci.c >> @@ -119,49 +119,20 @@ static int *cci_pci_create_irq_table(struct pci_dev >> *pcidev, unsigned int nvec) >> return table; >> } >> >> -/* enumerate feature devices under pci device */ >> -static int cci_enumerate_feature_devs(struct pci_dev *pcidev) >> +static int find_dfl_in_bar0(struct pci_dev *pcidev, >> + struct dfl_fpga_enum_info *info) >> { >> - struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); >> - int port_num, bar, i, nvec, ret = 0; >> - struct dfl_fpga_enum_info *info; >> - struct dfl_fpga_cdev *cdev; >> resource_size_t start, len; >> + int port_num, bar, i; >> void __iomem *base; >> - int *irq_table; >> + int ret = 0; >> u32 offset; >> u64 v; >> >> - /* allocate enumeration info via pci_dev */ >> - info = dfl_fpga_enum_info_alloc(&pcidev->dev); >> - if (!info) >> - return -ENOMEM; >> - >> - /* add irq info for enumeration if the device support irq */ >> - nvec = cci_pci_alloc_irq(pcidev); >> - if (nvec < 0) { >> - dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec); >> - ret = nvec; >> - goto enum_info_free_exit; >> - } else if (nvec) { >> - irq_table = cci_pci_create_irq_table(pcidev, nvec); >> - if (!irq_table) { >> - ret = -ENOMEM; >> - goto irq_free_exit; >> - } >> - >> - ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table); >> - kfree(irq_table); >> - if (ret) >> - goto irq_free_exit; >> - } >> - >> - /* start to find Device Feature List in Bar 0 */ >> + /* start to find Device Feature List from Bar 0 */ >> base = cci_pci_ioremap_bar0(pcidev); >> - if (!base) { >> - ret = -ENOMEM; >> - goto irq_free_exit; >> - } >> + if (!base) >> + return -ENOMEM; >> >> /* >> * PF device has FME and Ports/AFUs, and VF device only has one >> @@ -208,12 +179,53 @@ static int cci_enumerate_feature_devs(struct >> pci_dev *pcidev) >> dfl_fpga_enum_info_add_dfl(info, start, len); >> } else { >> ret = -ENODEV; >> - goto irq_free_exit; >> } >> >> /* release I/O mappings for next step enumeration */ >> pcim_iounmap_regions(pcidev, BIT(0)); >> >> + > > We don't need 2 blank line here, remove one please. I will remove this line in v2. > >> + return ret; >> +} >> + >> +/* enumerate feature devices under pci device */ >> +static int cci_enumerate_feature_devs(struct pci_dev *pcidev) >> +{ >> + struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); >> + struct dfl_fpga_enum_info *info; >> + struct dfl_fpga_cdev *cdev; >> + int nvec, ret = 0; >> + int *irq_table; >> + >> + /* allocate enumeration info via pci_dev */ >> + info = dfl_fpga_enum_info_alloc(&pcidev->dev); >> + if (!info) >> + return -ENOMEM; >> + >> + /* add irq info for enumeration if the device support irq */ >> + nvec = cci_pci_alloc_irq(pcidev); >> + if (nvec < 0) { >> + dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec); >> + ret = nvec; >> + goto enum_info_free_exit; >> + } else if (nvec) { >> + irq_table = cci_pci_create_irq_table(pcidev, nvec); >> + if (!irq_table) { >> + ret = -ENOMEM; >> + goto irq_free_exit; >> + } >> + >> + ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table); >> + kfree(irq_table); >> + if (ret) >> + goto irq_free_exit; >> + } >> + >> + ret = find_dfl_in_bar0(pcidev, info); >> + > > Remove this blank line, and maybe switch to a better function name here. I will remove blank line in v2 and use your suggested function name, find_dfls_by_default. > > Thanks > Hao > >> + if (ret) >> + goto irq_free_exit; >> + >> /* start enumeration with prepared enumeration information */ >> cdev = dfl_fpga_feature_devs_enumerate(info); >> if (IS_ERR(cdev)) { >> -- >> 2.25.2 > >
On 11/16/20 5:25 PM, matthew.gerlach@linux.intel.com wrote: > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > In preparation of looking for dfls based on a vendor > specific pcie capability, move code that assumes > Bar0/offset0 as start of DFL to its own function. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > --- > drivers/fpga/dfl-pci.c | 86 ++++++++++++++++++++++++------------------ > 1 file changed, 49 insertions(+), 37 deletions(-) > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c > index a2203d03c9e2..b1b157b41942 100644 > --- a/drivers/fpga/dfl-pci.c > +++ b/drivers/fpga/dfl-pci.c > @@ -119,49 +119,20 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec) > return table; > } > > -/* enumerate feature devices under pci device */ > -static int cci_enumerate_feature_devs(struct pci_dev *pcidev) > +static int find_dfl_in_bar0(struct pci_dev *pcidev, > + struct dfl_fpga_enum_info *info) > { > - struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); > - int port_num, bar, i, nvec, ret = 0; > - struct dfl_fpga_enum_info *info; > - struct dfl_fpga_cdev *cdev; > resource_size_t start, len; > + int port_num, bar, i; > void __iomem *base; > - int *irq_table; > + int ret = 0; > u32 offset; > u64 v; > > - /* allocate enumeration info via pci_dev */ > - info = dfl_fpga_enum_info_alloc(&pcidev->dev); > - if (!info) > - return -ENOMEM; > - > - /* add irq info for enumeration if the device support irq */ > - nvec = cci_pci_alloc_irq(pcidev); > - if (nvec < 0) { > - dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec); > - ret = nvec; > - goto enum_info_free_exit; > - } else if (nvec) { > - irq_table = cci_pci_create_irq_table(pcidev, nvec); > - if (!irq_table) { > - ret = -ENOMEM; > - goto irq_free_exit; > - } > - > - ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table); > - kfree(irq_table); > - if (ret) > - goto irq_free_exit; > - } > - > - /* start to find Device Feature List in Bar 0 */ > + /* start to find Device Feature List from Bar 0 */ > base = cci_pci_ioremap_bar0(pcidev); > - if (!base) { > - ret = -ENOMEM; > - goto irq_free_exit; > - } > + if (!base) > + return -ENOMEM; > > /* > * PF device has FME and Ports/AFUs, and VF device only has one > @@ -208,12 +179,53 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev) > dfl_fpga_enum_info_add_dfl(info, start, len); > } else { > ret = -ENODEV; > - goto irq_free_exit; > } > > /* release I/O mappings for next step enumeration */ > pcim_iounmap_regions(pcidev, BIT(0)); This ws was already commented on. > > + > + return ret; > +} > + > +/* enumerate feature devices under pci device */ > +static int cci_enumerate_feature_devs(struct pci_dev *pcidev) > +{ > + struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); > + struct dfl_fpga_enum_info *info; > + struct dfl_fpga_cdev *cdev; > + int nvec, ret = 0; > + int *irq_table; > + > + /* allocate enumeration info via pci_dev */ > + info = dfl_fpga_enum_info_alloc(&pcidev->dev); > + if (!info) > + return -ENOMEM; > + > + /* add irq info for enumeration if the device support irq */ > + nvec = cci_pci_alloc_irq(pcidev); > + if (nvec < 0) { > + dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec); > + ret = nvec; > + goto enum_info_free_exit; > + } else if (nvec) { > + irq_table = cci_pci_create_irq_table(pcidev, nvec); > + if (!irq_table) { > + ret = -ENOMEM; > + goto irq_free_exit; > + } > + > + ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table); > + kfree(irq_table); > + if (ret) > + goto irq_free_exit; > + } > + > + ret = find_dfl_in_bar0(pcidev, info); > + > + if (ret) > + goto irq_free_exit; > + > /* start enumeration with prepared enumeration information */ > cdev = dfl_fpga_feature_devs_enumerate(info); > if (IS_ERR(cdev)) { This looks fine. Reviewed-by: Tom Rix <trix@redhat.com>
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index a2203d03c9e2..b1b157b41942 100644 --- a/drivers/fpga/dfl-pci.c +++ b/drivers/fpga/dfl-pci.c @@ -119,49 +119,20 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec) return table; } -/* enumerate feature devices under pci device */ -static int cci_enumerate_feature_devs(struct pci_dev *pcidev) +static int find_dfl_in_bar0(struct pci_dev *pcidev, + struct dfl_fpga_enum_info *info) { - struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); - int port_num, bar, i, nvec, ret = 0; - struct dfl_fpga_enum_info *info; - struct dfl_fpga_cdev *cdev; resource_size_t start, len; + int port_num, bar, i; void __iomem *base; - int *irq_table; + int ret = 0; u32 offset; u64 v; - /* allocate enumeration info via pci_dev */ - info = dfl_fpga_enum_info_alloc(&pcidev->dev); - if (!info) - return -ENOMEM; - - /* add irq info for enumeration if the device support irq */ - nvec = cci_pci_alloc_irq(pcidev); - if (nvec < 0) { - dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec); - ret = nvec; - goto enum_info_free_exit; - } else if (nvec) { - irq_table = cci_pci_create_irq_table(pcidev, nvec); - if (!irq_table) { - ret = -ENOMEM; - goto irq_free_exit; - } - - ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table); - kfree(irq_table); - if (ret) - goto irq_free_exit; - } - - /* start to find Device Feature List in Bar 0 */ + /* start to find Device Feature List from Bar 0 */ base = cci_pci_ioremap_bar0(pcidev); - if (!base) { - ret = -ENOMEM; - goto irq_free_exit; - } + if (!base) + return -ENOMEM; /* * PF device has FME and Ports/AFUs, and VF device only has one @@ -208,12 +179,53 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev) dfl_fpga_enum_info_add_dfl(info, start, len); } else { ret = -ENODEV; - goto irq_free_exit; } /* release I/O mappings for next step enumeration */ pcim_iounmap_regions(pcidev, BIT(0)); + + return ret; +} + +/* enumerate feature devices under pci device */ +static int cci_enumerate_feature_devs(struct pci_dev *pcidev) +{ + struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); + struct dfl_fpga_enum_info *info; + struct dfl_fpga_cdev *cdev; + int nvec, ret = 0; + int *irq_table; + + /* allocate enumeration info via pci_dev */ + info = dfl_fpga_enum_info_alloc(&pcidev->dev); + if (!info) + return -ENOMEM; + + /* add irq info for enumeration if the device support irq */ + nvec = cci_pci_alloc_irq(pcidev); + if (nvec < 0) { + dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec); + ret = nvec; + goto enum_info_free_exit; + } else if (nvec) { + irq_table = cci_pci_create_irq_table(pcidev, nvec); + if (!irq_table) { + ret = -ENOMEM; + goto irq_free_exit; + } + + ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table); + kfree(irq_table); + if (ret) + goto irq_free_exit; + } + + ret = find_dfl_in_bar0(pcidev, info); + + if (ret) + goto irq_free_exit; + /* start enumeration with prepared enumeration information */ cdev = dfl_fpga_feature_devs_enumerate(info); if (IS_ERR(cdev)) {