| Message ID | 20171212141634.5985-3-niklas.cassel@axis.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Niklas, On Tuesday 12 December 2017 07:46 PM, Niklas Cassel wrote: > The error handling in pci_epc_epf_link() is broken, > since the clean up code for pci_epc_add_epf() calls clear_bit(), > even though the matching set_bit() is done after pci_epc_add_epf(). > > Also, clear_bit() should be done before pci_epc_remove_epf(), > since clean up code should normally do things in the reverse order. > > Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions") > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > --- > drivers/pci/endpoint/pci-ep-cfs.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c > index 4f74386c1ced..e1f5adc9e113 100644 > --- a/drivers/pci/endpoint/pci-ep-cfs.c > +++ b/drivers/pci/endpoint/pci-ep-cfs.c > @@ -106,7 +106,7 @@ static int pci_epc_epf_link(struct config_item *epc_item, > epf = epf_group->epf; > ret = pci_epc_add_epf(epc, epf); > if (ret) > - goto err_add_epf; > + return ret; Actually the func_no should be populated before invoking pci_epc_add_epf. Once that is done, the error handling should be fine. Thanks Kishon
On Thu, Dec 14, 2017 at 04:37:22PM +0530, Kishon Vijay Abraham I wrote: > Hi Niklas, > > On Tuesday 12 December 2017 07:46 PM, Niklas Cassel wrote: > > The error handling in pci_epc_epf_link() is broken, > > since the clean up code for pci_epc_add_epf() calls clear_bit(), > > even though the matching set_bit() is done after pci_epc_add_epf(). > > > > Also, clear_bit() should be done before pci_epc_remove_epf(), > > since clean up code should normally do things in the reverse order. > > > > Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions") > > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > --- > > drivers/pci/endpoint/pci-ep-cfs.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c > > index 4f74386c1ced..e1f5adc9e113 100644 > > --- a/drivers/pci/endpoint/pci-ep-cfs.c > > +++ b/drivers/pci/endpoint/pci-ep-cfs.c > > @@ -106,7 +106,7 @@ static int pci_epc_epf_link(struct config_item *epc_item, > > epf = epf_group->epf; > > ret = pci_epc_add_epf(epc, epf); > > if (ret) > > - goto err_add_epf; > > + return ret; > > Actually the func_no should be populated before invoking pci_epc_add_epf. Once > that is done, the error handling should be fine. Which means that current code works because pci_epc_add_epf() is called with epf->func_no == 0 right ? I agree that the correct fix consists in setting the epf->func_no before calling pci_epc_add_epf(), this means that this patch and patch 3 need updating. Thanks, Lorenzo
On Thursday 14 December 2017 05:37 PM, Lorenzo Pieralisi wrote: > On Thu, Dec 14, 2017 at 04:37:22PM +0530, Kishon Vijay Abraham I wrote: >> Hi Niklas, >> >> On Tuesday 12 December 2017 07:46 PM, Niklas Cassel wrote: >>> The error handling in pci_epc_epf_link() is broken, >>> since the clean up code for pci_epc_add_epf() calls clear_bit(), >>> even though the matching set_bit() is done after pci_epc_add_epf(). >>> >>> Also, clear_bit() should be done before pci_epc_remove_epf(), >>> since clean up code should normally do things in the reverse order. >>> >>> Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions") >>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> >>> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>> --- >>> drivers/pci/endpoint/pci-ep-cfs.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c >>> index 4f74386c1ced..e1f5adc9e113 100644 >>> --- a/drivers/pci/endpoint/pci-ep-cfs.c >>> +++ b/drivers/pci/endpoint/pci-ep-cfs.c >>> @@ -106,7 +106,7 @@ static int pci_epc_epf_link(struct config_item *epc_item, >>> epf = epf_group->epf; >>> ret = pci_epc_add_epf(epc, epf); >>> if (ret) >>> - goto err_add_epf; >>> + return ret; >> >> Actually the func_no should be populated before invoking pci_epc_add_epf. Once >> that is done, the error handling should be fine. > > Which means that current code works because pci_epc_add_epf() is called > with epf->func_no == 0 right ? I agree that the correct fix consists in that's right Lorenzo. Thanks Kishon
diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c index 4f74386c1ced..e1f5adc9e113 100644 --- a/drivers/pci/endpoint/pci-ep-cfs.c +++ b/drivers/pci/endpoint/pci-ep-cfs.c @@ -106,7 +106,7 @@ static int pci_epc_epf_link(struct config_item *epc_item, epf = epf_group->epf; ret = pci_epc_add_epf(epc, epf); if (ret) - goto err_add_epf; + return ret; func_no = find_first_zero_bit(&epc_group->function_num_map, sizeof(epc_group->function_num_map)); @@ -120,10 +120,8 @@ static int pci_epc_epf_link(struct config_item *epc_item, return 0; err_epf_bind: - pci_epc_remove_epf(epc, epf); - -err_add_epf: clear_bit(func_no, &epc_group->function_num_map); + pci_epc_remove_epf(epc, epf); return ret; }
The error handling in pci_epc_epf_link() is broken, since the clean up code for pci_epc_add_epf() calls clear_bit(), even though the matching set_bit() is done after pci_epc_add_epf(). Also, clear_bit() should be done before pci_epc_remove_epf(), since clean up code should normally do things in the reverse order. Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions") Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- drivers/pci/endpoint/pci-ep-cfs.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)