* [PATCH v3 0/3] Fix find_first_zero_bit() usage
@ 2017-11-27 15:49 Niklas Cassel
2017-11-27 15:49 ` [PATCH v3 1/3] PCI: designware-ep: " Niklas Cassel
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Niklas Cassel @ 2017-11-27 15:49 UTC (permalink / raw)
To: linux-pci; +Cc: kishon, Niklas Cassel, linux-kernel
find_first_zero_bit()'s parameter 'size' is defined in bits,
not in bytes.
Calling find_first_zero_bit() with the wrong size unit
will lead to insidious bugs.
Fix all uses of find_first_zero_bit() called with
sizeof() as size argument in drivers/pci.
Also had to fix broken error handling in pci_epc_epf_link()
in order to do proper error handling for find_first_zero_bit().
Niklas Cassel (3):
PCI: designware-ep: Fix find_first_zero_bit() usage
PCI: endpoint: Fix error handling in pci_epc_epf_link()
PCI: endpoint: Fix find_first_zero_bit() usage
drivers/pci/dwc/pcie-designware-ep.c | 22 ++++++++++++++--------
drivers/pci/dwc/pcie-designware.h | 7 +++++--
drivers/pci/endpoint/pci-ep-cfs.c | 14 +++++++++-----
3 files changed, 28 insertions(+), 15 deletions(-)
--
2.14.2
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 1/3] PCI: designware-ep: Fix find_first_zero_bit() usage 2017-11-27 15:49 [PATCH v3 0/3] Fix find_first_zero_bit() usage Niklas Cassel @ 2017-11-27 15:49 ` Niklas Cassel 2017-11-27 16:55 ` Lorenzo Pieralisi 2017-11-27 15:49 ` [PATCH v3 2/3] PCI: endpoint: Fix error handling in pci_epc_epf_link() Niklas Cassel 2017-11-27 15:49 ` [PATCH v3 3/3] PCI: endpoint: Fix find_first_zero_bit() usage Niklas Cassel 2 siblings, 1 reply; 10+ messages in thread From: Niklas Cassel @ 2017-11-27 15:49 UTC (permalink / raw) To: Jingoo Han, Joao Pinto, Lorenzo Pieralisi, Bjorn Helgaas Cc: kishon, Niklas Cassel, linux-pci, linux-kernel find_first_zero_bit()'s parameter 'size' is defined in bits, not in bytes. find_first_zero_bit() was called with size in bytes rather than bits, which thus defined a too low upper limit, causing dw_pcie_ep_inbound_atu() to assign iatu index #4 to both bar 4 and bar 5, which made bar 5 overwrite the settings set by bar 4. Fix this by using bitmaps with a static upper limit (MAX_IATUS). 256 was chosen since according to the databook, that is the maximum number of iATUs supported by the IP. Additionally, make sure that ep->num_ob_windows and ep->num_ib_windows, which are obtained from device tree, are less than the maximum number of iATUs (MAX_IATUS). Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support") Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> --- drivers/pci/dwc/pcie-designware-ep.c | 22 ++++++++++++++-------- drivers/pci/dwc/pcie-designware.h | 7 +++++-- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c index d53d5f168363..8b14e7db5487 100644 --- a/drivers/pci/dwc/pcie-designware-ep.c +++ b/drivers/pci/dwc/pcie-designware-ep.c @@ -70,8 +70,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum pci_barno bar, u32 free_win; struct dw_pcie *pci = to_dw_pcie_from_ep(ep); - free_win = find_first_zero_bit(&ep->ib_window_map, - sizeof(ep->ib_window_map)); + free_win = find_first_zero_bit(ep->ib_window_map, ep->num_ib_windows); if (free_win >= ep->num_ib_windows) { dev_err(pci->dev, "no free inbound window\n"); return -EINVAL; @@ -85,7 +84,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum pci_barno bar, } ep->bar_to_atu[bar] = free_win; - set_bit(free_win, &ep->ib_window_map); + set_bit(free_win, ep->ib_window_map); return 0; } @@ -96,8 +95,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr, u32 free_win; struct dw_pcie *pci = to_dw_pcie_from_ep(ep); - free_win = find_first_zero_bit(&ep->ob_window_map, - sizeof(ep->ob_window_map)); + free_win = find_first_zero_bit(ep->ob_window_map, ep->num_ob_windows); if (free_win >= ep->num_ob_windows) { dev_err(pci->dev, "no free outbound window\n"); return -EINVAL; @@ -106,7 +104,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr, dw_pcie_prog_outbound_atu(pci, free_win, PCIE_ATU_TYPE_MEM, phys_addr, pci_addr, size); - set_bit(free_win, &ep->ob_window_map); + set_bit(free_win, ep->ob_window_map); ep->outbound_addr[free_win] = phys_addr; return 0; @@ -121,7 +119,7 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, enum pci_barno bar) dw_pcie_ep_reset_bar(pci, bar); dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND); - clear_bit(atu_index, &ep->ib_window_map); + clear_bit(atu_index, ep->ib_window_map); } static int dw_pcie_ep_set_bar(struct pci_epc *epc, enum pci_barno bar, @@ -175,7 +173,7 @@ static void dw_pcie_ep_unmap_addr(struct pci_epc *epc, phys_addr_t addr) return; dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND); - clear_bit(atu_index, &ep->ob_window_map); + clear_bit(atu_index, ep->ob_window_map); } static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, @@ -298,12 +296,20 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) dev_err(dev, "unable to read *num-ib-windows* property\n"); return ret; } + if (ep->num_ib_windows > MAX_IATUS) { + dev_err(dev, "invalid *num-ib-windows*\n"); + return -EINVAL; + } ret = of_property_read_u32(np, "num-ob-windows", &ep->num_ob_windows); if (ret < 0) { dev_err(dev, "unable to read *num-ob-windows* property\n"); return ret; } + if (ep->num_ob_windows > MAX_IATUS) { + dev_err(dev, "invalid *num-ob-windows*\n"); + return -EINVAL; + } addr = devm_kzalloc(dev, sizeof(phys_addr_t) * ep->num_ob_windows, GFP_KERNEL); diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h index e5d9d77b778e..189f45264c2a 100644 --- a/drivers/pci/dwc/pcie-designware.h +++ b/drivers/pci/dwc/pcie-designware.h @@ -113,6 +113,9 @@ #define MAX_MSI_IRQS 32 #define MAX_MSI_CTRLS (MAX_MSI_IRQS / 32) +/* Maximum number of inbound/outbound iATUs */ +#define MAX_IATUS 256 + struct pcie_port; struct dw_pcie; struct dw_pcie_ep; @@ -192,8 +195,8 @@ struct dw_pcie_ep { size_t page_size; u8 bar_to_atu[6]; phys_addr_t *outbound_addr; - unsigned long ib_window_map; - unsigned long ob_window_map; + DECLARE_BITMAP(ib_window_map, MAX_IATUS); + DECLARE_BITMAP(ob_window_map, MAX_IATUS); u32 num_ib_windows; u32 num_ob_windows; }; -- 2.14.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] PCI: designware-ep: Fix find_first_zero_bit() usage 2017-11-27 15:49 ` [PATCH v3 1/3] PCI: designware-ep: " Niklas Cassel @ 2017-11-27 16:55 ` Lorenzo Pieralisi 2017-12-08 11:21 ` Lorenzo Pieralisi 0 siblings, 1 reply; 10+ messages in thread From: Lorenzo Pieralisi @ 2017-11-27 16:55 UTC (permalink / raw) To: Niklas Cassel Cc: Jingoo Han, Joao Pinto, Bjorn Helgaas, kishon, Niklas Cassel, linux-pci, linux-kernel On Mon, Nov 27, 2017 at 04:49:53PM +0100, Niklas Cassel wrote: > find_first_zero_bit()'s parameter 'size' is defined in bits, > not in bytes. > > find_first_zero_bit() was called with size in bytes rather than bits, > which thus defined a too low upper limit, causing > dw_pcie_ep_inbound_atu() to assign iatu index #4 to both bar 4 > and bar 5, which made bar 5 overwrite the settings set by bar 4. Same comments about past tense usage. > Fix this by using bitmaps with a static upper limit (MAX_IATUS). I think that given that we have the size from FW (num_ib/ob_windows), you should allocate them dynamically (sorry for asking another respin). > 256 was chosen since according to the databook, that is the Nit: I think "datasheet" is more appropriate than "databook" here. > maximum number of iATUs supported by the IP. > > Additionally, make sure that ep->num_ob_windows and ep->num_ib_windows, > which are obtained from device tree, are less than the maximum number of s/less/smaller > iATUs (MAX_IATUS). > > Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support") > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > --- > drivers/pci/dwc/pcie-designware-ep.c | 22 ++++++++++++++-------- > drivers/pci/dwc/pcie-designware.h | 7 +++++-- > 2 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c > index d53d5f168363..8b14e7db5487 100644 > --- a/drivers/pci/dwc/pcie-designware-ep.c > +++ b/drivers/pci/dwc/pcie-designware-ep.c > @@ -70,8 +70,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum pci_barno bar, > u32 free_win; > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > - free_win = find_first_zero_bit(&ep->ib_window_map, > - sizeof(ep->ib_window_map)); > + free_win = find_first_zero_bit(ep->ib_window_map, ep->num_ib_windows); > if (free_win >= ep->num_ib_windows) { > dev_err(pci->dev, "no free inbound window\n"); > return -EINVAL; > @@ -85,7 +84,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum pci_barno bar, > } > > ep->bar_to_atu[bar] = free_win; > - set_bit(free_win, &ep->ib_window_map); > + set_bit(free_win, ep->ib_window_map); > > return 0; > } > @@ -96,8 +95,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr, > u32 free_win; > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > - free_win = find_first_zero_bit(&ep->ob_window_map, > - sizeof(ep->ob_window_map)); > + free_win = find_first_zero_bit(ep->ob_window_map, ep->num_ob_windows); > if (free_win >= ep->num_ob_windows) { > dev_err(pci->dev, "no free outbound window\n"); > return -EINVAL; > @@ -106,7 +104,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr, > dw_pcie_prog_outbound_atu(pci, free_win, PCIE_ATU_TYPE_MEM, > phys_addr, pci_addr, size); > > - set_bit(free_win, &ep->ob_window_map); > + set_bit(free_win, ep->ob_window_map); > ep->outbound_addr[free_win] = phys_addr; > > return 0; > @@ -121,7 +119,7 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, enum pci_barno bar) > dw_pcie_ep_reset_bar(pci, bar); > > dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND); > - clear_bit(atu_index, &ep->ib_window_map); > + clear_bit(atu_index, ep->ib_window_map); > } > > static int dw_pcie_ep_set_bar(struct pci_epc *epc, enum pci_barno bar, > @@ -175,7 +173,7 @@ static void dw_pcie_ep_unmap_addr(struct pci_epc *epc, phys_addr_t addr) > return; > > dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND); > - clear_bit(atu_index, &ep->ob_window_map); > + clear_bit(atu_index, ep->ob_window_map); > } > > static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, > @@ -298,12 +296,20 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > dev_err(dev, "unable to read *num-ib-windows* property\n"); > return ret; > } > + if (ep->num_ib_windows > MAX_IATUS) { > + dev_err(dev, "invalid *num-ib-windows*\n"); > + return -EINVAL; > + } > > ret = of_property_read_u32(np, "num-ob-windows", &ep->num_ob_windows); > if (ret < 0) { > dev_err(dev, "unable to read *num-ob-windows* property\n"); > return ret; > } > + if (ep->num_ob_windows > MAX_IATUS) { > + dev_err(dev, "invalid *num-ob-windows*\n"); > + return -EINVAL; > + } > > addr = devm_kzalloc(dev, sizeof(phys_addr_t) * ep->num_ob_windows, > GFP_KERNEL); > diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h > index e5d9d77b778e..189f45264c2a 100644 > --- a/drivers/pci/dwc/pcie-designware.h > +++ b/drivers/pci/dwc/pcie-designware.h > @@ -113,6 +113,9 @@ > #define MAX_MSI_IRQS 32 > #define MAX_MSI_CTRLS (MAX_MSI_IRQS / 32) > > +/* Maximum number of inbound/outbound iATUs */ > +#define MAX_IATUS 256 > + > struct pcie_port; > struct dw_pcie; > struct dw_pcie_ep; > @@ -192,8 +195,8 @@ struct dw_pcie_ep { > size_t page_size; > u8 bar_to_atu[6]; > phys_addr_t *outbound_addr; > - unsigned long ib_window_map; > - unsigned long ob_window_map; > + DECLARE_BITMAP(ib_window_map, MAX_IATUS); > + DECLARE_BITMAP(ob_window_map, MAX_IATUS); Only comment I have on the patch is, make the bitmap dynamic ie allocate them using eg: ib_window_map = kzalloc(BITS_TO_LONG(num_ib_windows) * sizeof(long), GFP_KERNEL); it is a common pattern in the kernel. I just do not like preallocating static arrays if we know the real size. It is correct to check the number of ib/ob windows parsed from DT against MAX_IATUS, since that's a HW limitation therefore a FW (DT) bug if they are exceeded. find_first_zero_bit() should use num_ib/ob_windows as size parameter (ie current patch is correct). Thanks, Lorenzo > u32 num_ib_windows; > u32 num_ob_windows; > }; > -- > 2.14.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] PCI: designware-ep: Fix find_first_zero_bit() usage 2017-11-27 16:55 ` Lorenzo Pieralisi @ 2017-12-08 11:21 ` Lorenzo Pieralisi 0 siblings, 0 replies; 10+ messages in thread From: Lorenzo Pieralisi @ 2017-12-08 11:21 UTC (permalink / raw) To: Niklas Cassel Cc: Jingoo Han, Joao Pinto, Bjorn Helgaas, kishon, Niklas Cassel, linux-pci, linux-kernel On Mon, Nov 27, 2017 at 04:55:05PM +0000, Lorenzo Pieralisi wrote: > On Mon, Nov 27, 2017 at 04:49:53PM +0100, Niklas Cassel wrote: > > find_first_zero_bit()'s parameter 'size' is defined in bits, > > not in bytes. > > > > find_first_zero_bit() was called with size in bytes rather than bits, > > which thus defined a too low upper limit, causing > > dw_pcie_ep_inbound_atu() to assign iatu index #4 to both bar 4 > > and bar 5, which made bar 5 overwrite the settings set by bar 4. > > Same comments about past tense usage. > > > Fix this by using bitmaps with a static upper limit (MAX_IATUS). > > I think that given that we have the size from FW (num_ib/ob_windows), > you should allocate them dynamically (sorry for asking another respin). > > > 256 was chosen since according to the databook, that is the > > Nit: I think "datasheet" is more appropriate than "databook" here. > > > maximum number of iATUs supported by the IP. > > > > Additionally, make sure that ep->num_ob_windows and ep->num_ib_windows, > > which are obtained from device tree, are less than the maximum number of > > s/less/smaller > > > iATUs (MAX_IATUS). > > > > Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support") > > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > > --- > > drivers/pci/dwc/pcie-designware-ep.c | 22 ++++++++++++++-------- > > drivers/pci/dwc/pcie-designware.h | 7 +++++-- > > 2 files changed, 19 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c > > index d53d5f168363..8b14e7db5487 100644 > > --- a/drivers/pci/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/dwc/pcie-designware-ep.c > > @@ -70,8 +70,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum pci_barno bar, > > u32 free_win; > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > - free_win = find_first_zero_bit(&ep->ib_window_map, > > - sizeof(ep->ib_window_map)); > > + free_win = find_first_zero_bit(ep->ib_window_map, ep->num_ib_windows); > > if (free_win >= ep->num_ib_windows) { > > dev_err(pci->dev, "no free inbound window\n"); > > return -EINVAL; > > @@ -85,7 +84,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum pci_barno bar, > > } > > > > ep->bar_to_atu[bar] = free_win; > > - set_bit(free_win, &ep->ib_window_map); > > + set_bit(free_win, ep->ib_window_map); > > > > return 0; > > } > > @@ -96,8 +95,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr, > > u32 free_win; > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > - free_win = find_first_zero_bit(&ep->ob_window_map, > > - sizeof(ep->ob_window_map)); > > + free_win = find_first_zero_bit(ep->ob_window_map, ep->num_ob_windows); > > if (free_win >= ep->num_ob_windows) { > > dev_err(pci->dev, "no free outbound window\n"); > > return -EINVAL; > > @@ -106,7 +104,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr, > > dw_pcie_prog_outbound_atu(pci, free_win, PCIE_ATU_TYPE_MEM, > > phys_addr, pci_addr, size); > > > > - set_bit(free_win, &ep->ob_window_map); > > + set_bit(free_win, ep->ob_window_map); > > ep->outbound_addr[free_win] = phys_addr; > > > > return 0; > > @@ -121,7 +119,7 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, enum pci_barno bar) > > dw_pcie_ep_reset_bar(pci, bar); > > > > dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND); > > - clear_bit(atu_index, &ep->ib_window_map); > > + clear_bit(atu_index, ep->ib_window_map); > > } > > > > static int dw_pcie_ep_set_bar(struct pci_epc *epc, enum pci_barno bar, > > @@ -175,7 +173,7 @@ static void dw_pcie_ep_unmap_addr(struct pci_epc *epc, phys_addr_t addr) > > return; > > > > dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND); > > - clear_bit(atu_index, &ep->ob_window_map); > > + clear_bit(atu_index, ep->ob_window_map); > > } > > > > static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, > > @@ -298,12 +296,20 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > dev_err(dev, "unable to read *num-ib-windows* property\n"); > > return ret; > > } > > + if (ep->num_ib_windows > MAX_IATUS) { > > + dev_err(dev, "invalid *num-ib-windows*\n"); > > + return -EINVAL; > > + } > > > > ret = of_property_read_u32(np, "num-ob-windows", &ep->num_ob_windows); > > if (ret < 0) { > > dev_err(dev, "unable to read *num-ob-windows* property\n"); > > return ret; > > } > > + if (ep->num_ob_windows > MAX_IATUS) { > > + dev_err(dev, "invalid *num-ob-windows*\n"); > > + return -EINVAL; > > + } > > > > addr = devm_kzalloc(dev, sizeof(phys_addr_t) * ep->num_ob_windows, > > GFP_KERNEL); > > diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h > > index e5d9d77b778e..189f45264c2a 100644 > > --- a/drivers/pci/dwc/pcie-designware.h > > +++ b/drivers/pci/dwc/pcie-designware.h > > @@ -113,6 +113,9 @@ > > #define MAX_MSI_IRQS 32 > > #define MAX_MSI_CTRLS (MAX_MSI_IRQS / 32) > > > > +/* Maximum number of inbound/outbound iATUs */ > > +#define MAX_IATUS 256 > > + > > struct pcie_port; > > struct dw_pcie; > > struct dw_pcie_ep; > > @@ -192,8 +195,8 @@ struct dw_pcie_ep { > > size_t page_size; > > u8 bar_to_atu[6]; > > phys_addr_t *outbound_addr; > > - unsigned long ib_window_map; > > - unsigned long ob_window_map; > > + DECLARE_BITMAP(ib_window_map, MAX_IATUS); > > + DECLARE_BITMAP(ob_window_map, MAX_IATUS); > > Only comment I have on the patch is, make the bitmap dynamic ie allocate > them using eg: > > ib_window_map = kzalloc(BITS_TO_LONG(num_ib_windows) * sizeof(long), > GFP_KERNEL); > > it is a common pattern in the kernel. I just do not like preallocating > static arrays if we know the real size. > > It is correct to check the number of ib/ob windows parsed from DT > against MAX_IATUS, since that's a HW limitation therefore a FW (DT) > bug if they are exceeded. > > find_first_zero_bit() should use num_ib/ob_windows as size parameter > (ie current patch is correct). Hi Niklas, May I ask you please to update this series to the latest review comments and send out a v4 so that Bjorn can pull it please ? Thanks, Lorenzo ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/3] PCI: endpoint: Fix error handling in pci_epc_epf_link() 2017-11-27 15:49 [PATCH v3 0/3] Fix find_first_zero_bit() usage Niklas Cassel 2017-11-27 15:49 ` [PATCH v3 1/3] PCI: designware-ep: " Niklas Cassel @ 2017-11-27 15:49 ` Niklas Cassel 2017-11-27 16:16 ` Lorenzo Pieralisi 2017-11-27 15:49 ` [PATCH v3 3/3] PCI: endpoint: Fix find_first_zero_bit() usage Niklas Cassel 2 siblings, 1 reply; 10+ messages in thread From: Niklas Cassel @ 2017-11-27 15:49 UTC (permalink / raw) To: Kishon Vijay Abraham I, Lorenzo Pieralisi, Bjorn Helgaas Cc: Niklas Cassel, linux-pci, linux-kernel The error handling in pci_epc_epf_link() was broken, since the clean up code for pci_epc_add_epf() did 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> --- 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; 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; } -- 2.14.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/3] PCI: endpoint: Fix error handling in pci_epc_epf_link() 2017-11-27 15:49 ` [PATCH v3 2/3] PCI: endpoint: Fix error handling in pci_epc_epf_link() Niklas Cassel @ 2017-11-27 16:16 ` Lorenzo Pieralisi 0 siblings, 0 replies; 10+ messages in thread From: Lorenzo Pieralisi @ 2017-11-27 16:16 UTC (permalink / raw) To: Niklas Cassel Cc: Kishon Vijay Abraham I, Bjorn Helgaas, Niklas Cassel, linux-pci, linux-kernel On Mon, Nov 27, 2017 at 04:49:54PM +0100, Niklas Cassel wrote: > The error handling in pci_epc_epf_link() was broken, s/was/is (ie commit logs should be written in the present tense) > since the clean up code for pci_epc_add_epf() did clear_bit(), s/did/calls (see above) > 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> > --- > drivers/pci/endpoint/pci-ep-cfs.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > 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; > } > -- > 2.14.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 3/3] PCI: endpoint: Fix find_first_zero_bit() usage 2017-11-27 15:49 [PATCH v3 0/3] Fix find_first_zero_bit() usage Niklas Cassel 2017-11-27 15:49 ` [PATCH v3 1/3] PCI: designware-ep: " Niklas Cassel 2017-11-27 15:49 ` [PATCH v3 2/3] PCI: endpoint: Fix error handling in pci_epc_epf_link() Niklas Cassel @ 2017-11-27 15:49 ` Niklas Cassel 2017-11-27 17:44 ` Lorenzo Pieralisi 2017-11-28 9:53 ` David Laight 2 siblings, 2 replies; 10+ messages in thread From: Niklas Cassel @ 2017-11-27 15:49 UTC (permalink / raw) To: Kishon Vijay Abraham I, Lorenzo Pieralisi, Bjorn Helgaas Cc: Niklas Cassel, linux-pci, linux-kernel find_first_zero_bit()'s parameter 'size' is defined in bits, not in bytes. Calling find_first_zero_bit() with the wrong size unit will lead to insidious bugs. Fix this by calling find_first_zero_bit() with size BITS_PER_LONG, rather than sizeof(). Also add proper error handling for find_first_zero_bit(), since this was missing. Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions") Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> --- drivers/pci/endpoint/pci-ep-cfs.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c index e1f5adc9e113..0a22a7976580 100644 --- a/drivers/pci/endpoint/pci-ep-cfs.c +++ b/drivers/pci/endpoint/pci-ep-cfs.c @@ -109,7 +109,12 @@ static int pci_epc_epf_link(struct config_item *epc_item, return ret; func_no = find_first_zero_bit(&epc_group->function_num_map, - sizeof(epc_group->function_num_map)); + BITS_PER_LONG); + if (func_no >= BITS_PER_LONG) { + ret = -EINVAL; + dev_err(&epc->dev, "failed to link endpoint function device\n"); + goto err_func_no; + } set_bit(func_no, &epc_group->function_num_map); epf->func_no = func_no; @@ -121,6 +126,7 @@ static int pci_epc_epf_link(struct config_item *epc_item, err_epf_bind: clear_bit(func_no, &epc_group->function_num_map); +err_func_no: pci_epc_remove_epf(epc, epf); return ret; -- 2.14.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] PCI: endpoint: Fix find_first_zero_bit() usage 2017-11-27 15:49 ` [PATCH v3 3/3] PCI: endpoint: Fix find_first_zero_bit() usage Niklas Cassel @ 2017-11-27 17:44 ` Lorenzo Pieralisi 2017-11-28 9:53 ` David Laight 1 sibling, 0 replies; 10+ messages in thread From: Lorenzo Pieralisi @ 2017-11-27 17:44 UTC (permalink / raw) To: Niklas Cassel Cc: Kishon Vijay Abraham I, Bjorn Helgaas, Niklas Cassel, linux-pci, linux-kernel On Mon, Nov 27, 2017 at 04:49:55PM +0100, Niklas Cassel wrote: > find_first_zero_bit()'s parameter 'size' is defined in bits, > not in bytes. > > Calling find_first_zero_bit() with the wrong size unit > will lead to insidious bugs. > > Fix this by calling find_first_zero_bit() with size BITS_PER_LONG, > rather than sizeof(). > > Also add proper error handling for find_first_zero_bit(), since this > was missing. "Fix this by calling find_first_zero_bit() with size BITS_PER_LONG, rather than sizeof() and add missing find_first_zero_bit() return handling" > Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions") > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > --- > drivers/pci/endpoint/pci-ep-cfs.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c > index e1f5adc9e113..0a22a7976580 100644 > --- a/drivers/pci/endpoint/pci-ep-cfs.c > +++ b/drivers/pci/endpoint/pci-ep-cfs.c > @@ -109,7 +109,12 @@ static int pci_epc_epf_link(struct config_item *epc_item, > return ret; > > func_no = find_first_zero_bit(&epc_group->function_num_map, > - sizeof(epc_group->function_num_map)); > + BITS_PER_LONG); > + if (func_no >= BITS_PER_LONG) { > + ret = -EINVAL; > + dev_err(&epc->dev, "failed to link endpoint function device\n"); I would not log the error for the time being or make it more explicit: dev_error(&epc->dev, "unsupported number of functions, failed to link endpoint function device\n"); Side note: we _do_ know the max number of functions the epc supports (epc->max_functions) when this code is called so we do know the bitmap size - we can rework this code path based on that (@kishon, thoughts ?). Other than that: Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > + goto err_func_no; > + } > set_bit(func_no, &epc_group->function_num_map); > epf->func_no = func_no; > > @@ -121,6 +126,7 @@ static int pci_epc_epf_link(struct config_item *epc_item, > > err_epf_bind: > clear_bit(func_no, &epc_group->function_num_map); > +err_func_no: > pci_epc_remove_epf(epc, epf); > > return ret; > -- > 2.14.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v3 3/3] PCI: endpoint: Fix find_first_zero_bit() usage 2017-11-27 15:49 ` [PATCH v3 3/3] PCI: endpoint: Fix find_first_zero_bit() usage Niklas Cassel 2017-11-27 17:44 ` Lorenzo Pieralisi @ 2017-11-28 9:53 ` David Laight 2017-11-28 12:13 ` Lorenzo Pieralisi 1 sibling, 1 reply; 10+ messages in thread From: David Laight @ 2017-11-28 9:53 UTC (permalink / raw) To: 'Niklas Cassel', Kishon Vijay Abraham I, Lorenzo Pieralisi, Bjorn Helgaas Cc: Niklas Cassel, linux-pci, linux-kernel From: Niklas Cassel > Sent: 27 November 2017 15:50 > find_first_zero_bit()'s parameter 'size' is defined in bits, > not in bytes. > > Calling find_first_zero_bit() with the wrong size unit > will lead to insidious bugs. > > Fix this by calling find_first_zero_bit() with size > BITS_PER_LONG, rather than sizeof(). > > Also add proper error handling for find_first_zero_bit(), > since this was missing. > > Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions") > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > --- > drivers/pci/endpoint/pci-ep-cfs.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c > index e1f5adc9e113..0a22a7976580 100644 > --- a/drivers/pci/endpoint/pci-ep-cfs.c > +++ b/drivers/pci/endpoint/pci-ep-cfs.c > @@ -109,7 +109,12 @@ static int pci_epc_epf_link(struct config_item *epc_item, > return ret; > > func_no = find_first_zero_bit(&epc_group->function_num_map, > - sizeof(epc_group->function_num_map)); > + BITS_PER_LONG); Surely this should be either 8 * sizeof() or you should use ffz() that takes a numeric argument rather than the function designed for arbitrary size bitmaps. David ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] PCI: endpoint: Fix find_first_zero_bit() usage 2017-11-28 9:53 ` David Laight @ 2017-11-28 12:13 ` Lorenzo Pieralisi 0 siblings, 0 replies; 10+ messages in thread From: Lorenzo Pieralisi @ 2017-11-28 12:13 UTC (permalink / raw) To: David Laight Cc: 'Niklas Cassel', Kishon Vijay Abraham I, Bjorn Helgaas, Niklas Cassel, linux-pci, linux-kernel On Tue, Nov 28, 2017 at 09:53:12AM +0000, David Laight wrote: > From: Niklas Cassel > > Sent: 27 November 2017 15:50 > > find_first_zero_bit()'s parameter 'size' is defined in bits, > > not in bytes. > > > > Calling find_first_zero_bit() with the wrong size unit > > will lead to insidious bugs. > > > > Fix this by calling find_first_zero_bit() with size > > BITS_PER_LONG, rather than sizeof(). > > > > Also add proper error handling for find_first_zero_bit(), > > since this was missing. > > > > Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions") > > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > > --- > > drivers/pci/endpoint/pci-ep-cfs.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c > > index e1f5adc9e113..0a22a7976580 100644 > > --- a/drivers/pci/endpoint/pci-ep-cfs.c > > +++ b/drivers/pci/endpoint/pci-ep-cfs.c > > @@ -109,7 +109,12 @@ static int pci_epc_epf_link(struct config_item *epc_item, > > return ret; > > > > func_no = find_first_zero_bit(&epc_group->function_num_map, > > - sizeof(epc_group->function_num_map)); > > + BITS_PER_LONG); > > Surely this should be either 8 * sizeof() or you should use ffz() > that takes a numeric argument rather than the function designed > for arbitrary size bitmaps. Do you see a problem with this code ? It can be made a bitmap by allocating it using epc->max_functions as bitmap number of bits and that can be made on top of the code above that would go in as a fix unless you strongly object to it, as Joe mentioned both ffz+| and find_first_zero_bit()+set_bit() are ok with me as long as we choose one and fix the issue. Thanks, Lorenzo ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-12-08 11:20 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-27 15:49 [PATCH v3 0/3] Fix find_first_zero_bit() usage Niklas Cassel 2017-11-27 15:49 ` [PATCH v3 1/3] PCI: designware-ep: " Niklas Cassel 2017-11-27 16:55 ` Lorenzo Pieralisi 2017-12-08 11:21 ` Lorenzo Pieralisi 2017-11-27 15:49 ` [PATCH v3 2/3] PCI: endpoint: Fix error handling in pci_epc_epf_link() Niklas Cassel 2017-11-27 16:16 ` Lorenzo Pieralisi 2017-11-27 15:49 ` [PATCH v3 3/3] PCI: endpoint: Fix find_first_zero_bit() usage Niklas Cassel 2017-11-27 17:44 ` Lorenzo Pieralisi 2017-11-28 9:53 ` David Laight 2017-11-28 12:13 ` Lorenzo Pieralisi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.