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(-)
--
2.14.2
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(-)
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index d53d5f168363..d5eb143040e3 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,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);
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
index e5d9d77b778e..e6fd0b024b21 100644
--- a/drivers/pci/dwc/pcie-designware.h
+++ b/drivers/pci/dwc/pcie-designware.h
@@ -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;
};
--
2.14.2
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(-)
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
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]>
---
drivers/pci/endpoint/pci-ep-cfs.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index e1f5adc9e113..f5ccd1aa2963 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -109,7 +109,11 @@ 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;
+ goto err_func_no;
+ }
set_bit(func_no, &epc_group->function_num_map);
epf->func_no = func_no;
@@ -121,6 +125,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
From: 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.
...
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[].
David
On Tue, Dec 12, 2017 at 02:33:52PM +0000, David Laight wrote:
> From: 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.
> ...
>
> 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[].
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.
Lorenzo
From: Lorenzo Pieralisi
> Sent: 12 December 2017 15:24
>
> On Tue, Dec 12, 2017 at 02:33:52PM +0000, David Laight wrote:
> > From: 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.
> > ...
> >
> > 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[].
>
> 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.
I may not have looked very closely at these patches, but IIRC some other
similar ones were using explicit foo |= 1 << bit to set the bit.
While technically correct (changes 4 or 8 to 32 or 64) it might be
better described as '8 * sizeof xxxx'.
Then the code is correct regardless of the bitmap size (unless smaller
than a long on (probably) BE systems).
David
On Tue, Dec 12, 2017 at 03:16:31PM +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 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(-)
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?
Bjorn
Hi Kishon,
On Tue, Dec 12, 2017 at 03:16:31PM +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 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(-)
I would need your ACK asap to queue this series.
Thanks,
Lorenzo
On Tuesday 12 December 2017 07:46 PM, Niklas Cassel wrote:
> 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]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
> ---
> drivers/pci/dwc/pcie-designware-ep.c | 34 ++++++++++++++++++++++++++--------
> drivers/pci/dwc/pcie-designware.h | 8 ++++++--
> 2 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index d53d5f168363..d5eb143040e3 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,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);
> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
> index e5d9d77b778e..e6fd0b024b21 100644
> --- a/drivers/pci/dwc/pcie-designware.h
> +++ b/drivers/pci/dwc/pcie-designware.h
> @@ -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;
> };
>
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 <[email protected]>
> Acked-by: Lorenzo Pieralisi <[email protected]>
> ---
> 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 Tuesday 12 December 2017 07:46 PM, 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() 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]>
I'll hold Acking this patch, since it is dependent on the previous one.
Thanks
Kishon
Hi Lorenzo,
On Thursday 14 December 2017 04:21 PM, Lorenzo Pieralisi wrote:
> Hi Kishon,
>
> On Tue, Dec 12, 2017 at 03:16:31PM +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 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(-)
>
> I would need your ACK asap to queue this series.
Sorry for the delay. I had a comment on the 2nd patch.
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 <[email protected]>
> > Acked-by: Lorenzo Pieralisi <[email protected]>
> > ---
> > 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 <[email protected]>
>>> Acked-by: Lorenzo Pieralisi <[email protected]>
>>> ---
>>> 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
On Wed, Dec 13, 2017 at 03:59:25PM -0600, Bjorn Helgaas wrote:
> On Tue, Dec 12, 2017 at 03:16:31PM +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 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(-)
>
> 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.
Regards,
Niklas
On Thu, Dec 14, 2017 at 02:32:30PM +0100, Niklas Cassel wrote:
> On Wed, Dec 13, 2017 at 03:59:25PM -0600, Bjorn Helgaas wrote:
> > On Tue, Dec 12, 2017 at 03:16:31PM +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 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(-)
> >
> > 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.
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.
Thanks,
Lorenzo
On Thu, Dec 14, 2017 at 01:47:07PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Dec 14, 2017 at 02:32:30PM +0100, Niklas Cassel wrote:
> > On Wed, Dec 13, 2017 at 03:59:25PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Dec 12, 2017 at 03:16:31PM +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 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(-)
> > >
> > > 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.
>
> 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?
Thanks!
On Thu, Dec 14, 2017 at 05:21:38PM -0600, Bjorn Helgaas wrote:
> On Thu, Dec 14, 2017 at 01:47:07PM +0000, Lorenzo Pieralisi wrote:
> > On Thu, Dec 14, 2017 at 02:32:30PM +0100, Niklas Cassel wrote:
> > > On Wed, Dec 13, 2017 at 03:59:25PM -0600, Bjorn Helgaas wrote:
> > > > On Tue, Dec 12, 2017 at 03:16:31PM +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 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(-)
> > > >
> > > > 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.
> >
> > 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?
Yes sure, I will queue it straight away.
Thanks,
Lorenzo