| Message ID | 20171212141634.5985-1-niklas.cassel@axis.com |
|---|---|
| Headers | show |
| Series | Fix find_first_zero_bit() usage | expand |
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
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 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