Fix find_first_zero_bit() usage

find_first_zero_bit()'s parameter 'size' is defined in bits, not in bytes.

find_first_zero_bit() is called with size in bytes rather than bits, which thus defines a too low upper limit, causing dw_pcie_ep_inbound_atu() to assign iatu index #4 to both bar 4 and bar 5, which makes bar 5 overwrite the settings set by bar 4.

Since the sizes of the bitmaps are known, dynamically allocate the bitmaps, and use the correct size when calling find_first_zero_bit().

Additionally, make sure that ep->num_ob_windows and ep->num_ib_windows, which are obtained from device tree, are smaller than the maximum number of iATUs (MAX_IATU_IN/MAX_IATU_OUT).

Fixes f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/pci/dwc/pcie-designware-ep.c | 34 ++++++++++++++++++++++++++--------
 drivers/pci/dwc/pcie-designware.h    |  8 ++++++--
 2 files changed, 32 insertions(+), 10 deletions(-)

drivers/pci/dwc/pcie-designware-ep.c +26 -8

index d53d5f168363..d5eb143040e3 100644
@@ -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,32 @@ 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_IATU_IN) {
+		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_IATU_OUT) {
+		dev_err(dev, "invalid *num-ob-windows*\n");
+		return -EINVAL;
+	}
+
+	ep->ib_window_map = devm_kzalloc(dev, sizeof(long) *
+					 BITS_TO_LONGS(ep->num_ib_windows),
+					 GFP_KERNEL);
+	if (!ep->ib_window_map)
+		return -ENOMEM;
+
+	ep->ob_window_map = devm_kzalloc(dev, sizeof(long) *
+					 BITS_TO_LONGS(ep->num_ob_windows),
+					 GFP_KERNEL);
+	if (!ep->ob_window_map)
+		return -ENOMEM;
 
 	addr = devm_kzalloc(dev, sizeof(phys_addr_t) * ep->num_ob_windows,
 			    GFP_KERNEL);

drivers/pci/dwc/pcie-designware.h +6 -2

index e5d9d77b778e..e6fd0b024b21 100644
@@ -113,6 +113,10 @@
 #define MAX_MSI_IRQS			32
 #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / 32)
 
+/* Maximum number of inbound/outbound iATUs */
+#define MAX_IATU_IN			256
+#define MAX_IATU_OUT			256
+
 struct pcie_port;
 struct dw_pcie;
 struct dw_pcie_ep;

@@ -192,8 +196,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;
+	unsigned long		*ib_window_map;
+	unsigned long		*ob_window_map;
 	u32			num_ib_windows;
 	u32			num_ob_windows;
 };

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 <[email protected]>

Acked-by Lorenzo Pieralisi <[email protected]>

---
 drivers/pci/endpoint/pci-ep-cfs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/pci/endpoint/pci-ep-cfs.c +0 -0

index 4f74386c1ced..e1f5adc9e113 100644

Hi Niklas,

Quoted 14 lines · Niklas Cassel

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 <[email protected]>

Acked-by Lorenzo Pieralisi <[email protected]>

---
 drivers/pci/endpoint/pci-ep-cfs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/pci/endpoint/pci-ep-cfs.c static int pci_epc_epf_link(struct config_item *epc_item, · line 109

Quoted 10 lines

drivers/pci/endpoint/pci-ep-cfs.c +1 -1

index 4f74386c1ced..e1f5adc9e113 100644
@@ -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.

Quoted 17 lines · Kishon Vijay Abraham I

Hi Niklas,

Quoted 14 lines · Niklas Cassel

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 <[email protected]>

Acked-by Lorenzo Pieralisi <[email protected]>

---
 drivers/pci/endpoint/pci-ep-cfs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Quoted 13 lines

drivers/pci/endpoint/pci-ep-cfs.c static int pci_epc_epf_link(struct config_item *epc_item, · line 109

Quoted 10 lines

drivers/pci/endpoint/pci-ep-cfs.c +1 -1

index 4f74386c1ced..e1f5adc9e113 100644
@@ -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

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() 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 <[email protected]>

Acked-by Lorenzo Pieralisi <[email protected]>

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.

From: Niklas Cassel

Quoted 8 lines

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.

...

Isn't all this code just using the wrong function. Shouldn't they be using ffz() (or whatever it is called) to find the first zero in a numeric argument rather that find_first_zero_bit() which is intended for large bitmaps.

Perhaps the type for 'large bitmaps' should be:

struct {
	unsigned long bitmap_bits[0];
} bitmap;

rather than unsigned long[].

Sent: 12 December 2017 15:24

Quoted 16 lines · David Laight

From: Niklas Cassel

Quoted 8 lines

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.

...

Isn't all this code just using the wrong function. Shouldn't they be using ffz() (or whatever it is called) to find the first zero in a numeric argument rather that find_first_zero_bit() which is intended for large bitmaps.

Quoted 5 lines

Perhaps the type for 'large bitmaps' should be:

struct {
	unsigned long bitmap_bits[0];
} bitmap;

rather than unsigned long[].

David,

I think you are referring to patch 3, which is a fix for the current find_first_zero_bit() usage. The point is, I think that struct pci_epc_group.function_num_map should actually be converted to a bitmap (and therefore using find_first_zero_bit() on it is the right API); patch 3 is just a fix for current code.

Unless you think patch 3 is technically wrong I would go ahead with the series as-is for fixes and we will refactor struct pci_epc_group.function_num_map usage to a proper bitmap for the upcoming merge window.

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 | 34 ++++++++++++++++++++++++++--------
 drivers/pci/dwc/pcie-designware.h    |  8 ++++++--
 drivers/pci/endpoint/pci-ep-cfs.c    | 13 ++++++++-----
 3 files changed, 40 insertions(+), 15 deletions(-)

Quoted 17 lines · Niklas Cassel

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

Quoted 4 lines

 drivers/pci/dwc/pcie-designware-ep.c | 34 ++++++++++++++++++++++++++--------
 drivers/pci/dwc/pcie-designware.h    |  8 ++++++--
 drivers/pci/endpoint/pci-ep-cfs.c    | 13 ++++++++-----
 3 files changed, 40 insertions(+), 15 deletions(-)

In the interest of making forward progress, I applied these to for-linus for v4.15.

The issues apparently have been there since v4.12-rc1, but I guess this is proposed for for-linus because (a) it fixes insidious bugs and (b) the endpoint framework is relatively little-used yet so low-risk. Right?

Quoted 23 lines · Bjorn Helgaas

Quoted 17 lines · Niklas Cassel

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

Quoted 4 lines

 drivers/pci/dwc/pcie-designware-ep.c | 34 ++++++++++++++++++++++++++--------
 drivers/pci/dwc/pcie-designware.h    |  8 ++++++--
 drivers/pci/endpoint/pci-ep-cfs.c    | 13 ++++++++-----
 3 files changed, 40 insertions(+), 15 deletions(-)

Quoted 8 lines

In the interest of making forward progress, I applied these to for-linus for v4.15.

The issues apparently have been there since v4.12-rc1, but I guess this is proposed for for-linus because (a) it fixes insidious bugs and (b) the endpoint framework is relatively little-used yet so low-risk. Right?

Hello Bjorn,

As far as I know, dra7xx is the only in-tree user of the endpoint framework. Therefore, I see no real need to rush these patches.

One benefit of sending them to v4.15 would be if anyone starts developing endpoint support for their driver (with v4.15 as a base), we eliminate the risk that they might get hit by these bugs, and potentially waste time finding bugs that have already been found.

Please note that Kishon had some last minute review comments, so I had to submit a V5 of the patch series.

Quoted 43 lines · Niklas Cassel

Quoted 23 lines · Bjorn Helgaas

Quoted 17 lines · Niklas Cassel

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

Quoted 4 lines

 drivers/pci/dwc/pcie-designware-ep.c | 34 ++++++++++++++++++++++++++--------
 drivers/pci/dwc/pcie-designware.h    |  8 ++++++--
 drivers/pci/endpoint/pci-ep-cfs.c    | 13 ++++++++-----
 3 files changed, 40 insertions(+), 15 deletions(-)

Quoted 8 lines

In the interest of making forward progress, I applied these to for-linus for v4.15.

The issues apparently have been there since v4.12-rc1, but I guess this is proposed for for-linus because (a) it fixes insidious bugs and (b) the endpoint framework is relatively little-used yet so low-risk. Right?

Hello Bjorn,

As far as I know, dra7xx is the only in-tree user of the endpoint framework. Therefore, I see no real need to rush these patches.

One benefit of sending them to v4.15 would be if anyone starts developing endpoint support for their driver (with v4.15 as a base), we eliminate the risk that they might get hit by these bugs, and potentially waste time finding bugs that have already been found.

Quoted 2 lines

Please note that Kishon had some last minute review comments, so I had to submit a V5 of the patch series.

I missed this message (please CC me on the cover letter too next time) I hope Bjorn can drop v4 and replace it with v5 - or I can just queue v5 for v4.16 - just let me know please how you want to handle it.

Quoted 47 lines · Lorenzo Pieralisi

Quoted 45 lines · Niklas Cassel

Quoted 23 lines · Bjorn Helgaas

Quoted 17 lines · Niklas Cassel

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

Quoted 4 lines

 drivers/pci/dwc/pcie-designware-ep.c | 34 ++++++++++++++++++++++++++--------
 drivers/pci/dwc/pcie-designware.h    |  8 ++++++--
 drivers/pci/endpoint/pci-ep-cfs.c    | 13 ++++++++-----
 3 files changed, 40 insertions(+), 15 deletions(-)

Quoted 8 lines

In the interest of making forward progress, I applied these to for-linus for v4.15.

The issues apparently have been there since v4.12-rc1, but I guess this is proposed for for-linus because (a) it fixes insidious bugs and (b) the endpoint framework is relatively little-used yet so low-risk. Right?

Hello Bjorn,

As far as I know, dra7xx is the only in-tree user of the endpoint framework. Therefore, I see no real need to rush these patches.

One benefit of sending them to v4.15 would be if anyone starts developing endpoint support for their driver (with v4.15 as a base), we eliminate the risk that they might get hit by these bugs, and potentially waste time finding bugs that have already been found.

Please note that Kishon had some last minute review comments, so I had to submit a V5 of the patch series.

Quoted 3 lines

I missed this message (please CC me on the cover letter too next time) I hope Bjorn can drop v4 and replace it with v5 - or I can just queue v5 for v4.16 - just let me know please how you want to handle it.

I dropped them from my for-linus tree. Lorenzo, can you take care of v5 for v4.16?

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 | 34 ++++++++++++++++++++++++++--------
 drivers/pci/dwc/pcie-designware.h    |  8 ++++++--
 drivers/pci/endpoint/pci-ep-cfs.c    | 13 ++++++++-----
 3 files changed, 40 insertions(+), 15 deletions(-)

Hi Kishon,

Quoted 17 lines · Niklas Cassel

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

Quoted 4 lines

 drivers/pci/dwc/pcie-designware-ep.c | 34 ++++++++++++++++++++++++++--------
 drivers/pci/dwc/pcie-designware.h    |  8 ++++++--
 drivers/pci/endpoint/pci-ep-cfs.c    | 13 ++++++++-----
 3 files changed, 40 insertions(+), 15 deletions(-)

I would need your ACK asap to queue this series.