| Message ID | 20230330162434.35055-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add pci_dev_for_each_resource() helper and update users | expand |
On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: > Provide two new helper macros to iterate over PCI device resources and > convert users. > > Looking at it, refactor existing pci_bus_for_each_resource() and convert > users accordingly. > > Note, the amount of lines grew due to the documentation update. > > Changelog v8: > - fixed issue with pci_bus_for_each_resource() macro (LKP) > - due to above added a new patch to document how it works > - moved the last patch to be #2 (Philippe) > - added tags (Philippe) > > Changelog v7: > - made both macros to share same name (Bjorn) I didn't actually request the same name for both; I would have had no idea how to even do that :) v6 had: pci_dev_for_each_resource_p(dev, res) pci_dev_for_each_resource(dev, res, i) and I suggested: pci_dev_for_each_resource(dev, res) pci_dev_for_each_resource_idx(dev, res, i) because that pattern is used elsewhere. But you figured out how to do it, and having one name is even better, so thanks for that extra work! > - split out the pci_resource_n() conversion (Bjorn) > > Changelog v6: > - dropped unused variable in PPC code (LKP) > > Changelog v5: > - renamed loop variable to minimize the clash (Keith) > - addressed smatch warning (Dan) > - addressed 0-day bot findings (LKP) > > Changelog v4: > - rebased on top of v6.3-rc1 > - added tag (Krzysztof) > > Changelog v3: > - rebased on top of v2 by Mika, see above > - added tag to pcmcia patch (Dominik) > > Changelog v2: > - refactor to have two macros > - refactor existing pci_bus_for_each_resource() in the same way and > convert users > > Andy Shevchenko (6): > kernel.h: Split out COUNT_ARGS() and CONCATENATE() > PCI: Introduce pci_resource_n() > PCI: Document pci_bus_for_each_resource() to avoid confusion > PCI: Allow pci_bus_for_each_resource() to take less arguments > EISA: Convert to use less arguments in pci_bus_for_each_resource() > pcmcia: Convert to use less arguments in pci_bus_for_each_resource() > > Mika Westerberg (1): > PCI: Introduce pci_dev_for_each_resource() > > .clang-format | 1 + > arch/alpha/kernel/pci.c | 5 +- > arch/arm/kernel/bios32.c | 16 +++-- > arch/arm/mach-dove/pcie.c | 10 ++-- > arch/arm/mach-mv78xx0/pcie.c | 10 ++-- > arch/arm/mach-orion5x/pci.c | 10 ++-- > arch/mips/pci/ops-bcm63xx.c | 8 +-- > arch/mips/pci/pci-legacy.c | 3 +- > arch/powerpc/kernel/pci-common.c | 21 +++---- > arch/powerpc/platforms/4xx/pci.c | 8 +-- > arch/powerpc/platforms/52xx/mpc52xx_pci.c | 5 +- > arch/powerpc/platforms/pseries/pci.c | 16 ++--- > arch/sh/drivers/pci/pcie-sh7786.c | 10 ++-- > arch/sparc/kernel/leon_pci.c | 5 +- > arch/sparc/kernel/pci.c | 10 ++-- > arch/sparc/kernel/pcic.c | 5 +- > drivers/eisa/pci_eisa.c | 4 +- > drivers/pci/bus.c | 7 +-- > drivers/pci/hotplug/shpchp_sysfs.c | 8 +-- > drivers/pci/pci.c | 3 +- > drivers/pci/probe.c | 2 +- > drivers/pci/remove.c | 5 +- > drivers/pci/setup-bus.c | 37 +++++------- > drivers/pci/setup-res.c | 4 +- > drivers/pci/vgaarb.c | 17 ++---- > drivers/pci/xen-pcifront.c | 4 +- > drivers/pcmcia/rsrc_nonstatic.c | 9 +-- > drivers/pcmcia/yenta_socket.c | 3 +- > drivers/pnp/quirks.c | 29 ++++----- > include/linux/args.h | 13 ++++ > include/linux/kernel.h | 8 +-- > include/linux/pci.h | 72 +++++++++++++++++++---- > 32 files changed, 190 insertions(+), 178 deletions(-) > create mode 100644 include/linux/args.h Applied 2-7 to pci/resource for v6.4, thanks, I really like this! I omitted [1/7] kernel.h: Split out COUNT_ARGS() and CONCATENATE()" only because it's not essential to this series and has only a trivial one-line impact on include/linux/pci.h. Bjorn
On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote: > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: > > Provide two new helper macros to iterate over PCI device resources and > > convert users. > > > > Looking at it, refactor existing pci_bus_for_each_resource() and convert > > users accordingly. > > > > Note, the amount of lines grew due to the documentation update. > > > > Changelog v8: > > - fixed issue with pci_bus_for_each_resource() macro (LKP) > > - due to above added a new patch to document how it works > > - moved the last patch to be #2 (Philippe) > > - added tags (Philippe) > > > > Changelog v7: > > - made both macros to share same name (Bjorn) > > I didn't actually request the same name for both; I would have had no > idea how to even do that :) > > v6 had: > > pci_dev_for_each_resource_p(dev, res) > pci_dev_for_each_resource(dev, res, i) > > and I suggested: > > pci_dev_for_each_resource(dev, res) > pci_dev_for_each_resource_idx(dev, res, i) > > because that pattern is used elsewhere. Ah, sorry I misinterpreted your suggestion (I thought that at the end of the day you wanted the macro to be less intrusive, so we change less code, that's why I interpreted it the way described in the Changelog). > But you figured out how to do > it, and having one name is even better, so thanks for that extra work! You are welcome! > > - split out the pci_resource_n() conversion (Bjorn) > > > > Changelog v6: > > - dropped unused variable in PPC code (LKP) > > > > Changelog v5: > > - renamed loop variable to minimize the clash (Keith) > > - addressed smatch warning (Dan) > > - addressed 0-day bot findings (LKP) > > > > Changelog v4: > > - rebased on top of v6.3-rc1 > > - added tag (Krzysztof) > > > > Changelog v3: > > - rebased on top of v2 by Mika, see above > > - added tag to pcmcia patch (Dominik) > > > > Changelog v2: > > - refactor to have two macros > > - refactor existing pci_bus_for_each_resource() in the same way and > > convert users > > > > Andy Shevchenko (6): > > kernel.h: Split out COUNT_ARGS() and CONCATENATE() > > PCI: Introduce pci_resource_n() > > PCI: Document pci_bus_for_each_resource() to avoid confusion > > PCI: Allow pci_bus_for_each_resource() to take less arguments > > EISA: Convert to use less arguments in pci_bus_for_each_resource() > > pcmcia: Convert to use less arguments in pci_bus_for_each_resource() ... > Applied 2-7 to pci/resource for v6.4, thanks, I really like this! Btw, can you actually drop patch 7, please? After I have updated the documentation I have realised that why the first chunk is invalid. It needs mode careful check and rework. > I omitted > > [1/7] kernel.h: Split out COUNT_ARGS() and CONCATENATE()" > > only because it's not essential to this series and has only a trivial > one-line impact on include/linux/pci.h. I'm not sure I understood what exactly "essentiality" means to you, but I included that because it makes the split which can be used later by others and not including kernel.h in the header is the objective I want to achieve. Without this patch the achievement is going to be deferred. Yet, this, as you have noticed, allows to compile and use the macros in the rest of the patches. P.S. Thank you for the review and application of the rest!
On Wed, Apr 05, 2023 at 11:28:27AM +0300, Andy Shevchenko wrote: > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote: > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: > > > Provide two new helper macros to iterate over PCI device resources and > > > convert users. > > > > > > Looking at it, refactor existing pci_bus_for_each_resource() and convert > > > users accordingly. > > Applied 2-7 to pci/resource for v6.4, thanks, I really like this! > > Btw, can you actually drop patch 7, please? Done. > > I omitted > > > > [1/7] kernel.h: Split out COUNT_ARGS() and CONCATENATE()" > > > > only because it's not essential to this series and has only a trivial > > one-line impact on include/linux/pci.h. > > I'm not sure I understood what exactly "essentiality" means to you, but > I included that because it makes the split which can be used later by > others and not including kernel.h in the header is the objective I want > to achieve. Without this patch the achievement is going to be deferred. > Yet, this, as you have noticed, allows to compile and use the macros in > the rest of the patches. I haven't followed the kernel.h splitting, and I try to avoid incidental changes outside of the files I maintain, so I just wanted to keep this series purely PCI and avoid any possible objections to a new include file or discussion about how it should be done.
On Wed, Apr 05, 2023 at 03:18:32PM -0500, Bjorn Helgaas wrote: > On Wed, Apr 05, 2023 at 11:28:27AM +0300, Andy Shevchenko wrote: > > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote: > > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: ... > > > I omitted > > > > > > [1/7] kernel.h: Split out COUNT_ARGS() and CONCATENATE()" > > > > > > only because it's not essential to this series and has only a trivial > > > one-line impact on include/linux/pci.h. > > > > I'm not sure I understood what exactly "essentiality" means to you, but > > I included that because it makes the split which can be used later by > > others and not including kernel.h in the header is the objective I want > > to achieve. Without this patch the achievement is going to be deferred. > > Yet, this, as you have noticed, allows to compile and use the macros in > > the rest of the patches. > > I haven't followed the kernel.h splitting, and I try to avoid > incidental changes outside of the files I maintain, so I just wanted > to keep this series purely PCI and avoid any possible objections to a > new include file or discussion about how it should be done. Okay, fair enough :-) Thank you for elaboration, I will send the new version of patch 7 separately.
On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote: > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: > > Provide two new helper macros to iterate over PCI device resources and > > convert users. > Applied 2-7 to pci/resource for v6.4, thanks, I really like this! This is 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()") upstream now. Coverity complains about each use, sample below from drivers/pci/vgaarb.c. I didn't investigate at all, so it might be a false positive; just FYI. 1. Condition screen_info.capabilities & (2U /* 1 << 1 */), taking true branch. 556 if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) 557 base |= (u64)screen_info.ext_lfb_base << 32; 558 559 limit = base + size; 560 561 /* Does firmware framebuffer belong to us? */ 2. Condition __b < PCI_NUM_RESOURCES, taking true branch. 3. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. 6. Condition __b < PCI_NUM_RESOURCES, taking true branch. 7. cond_at_most: Checking __b < PCI_NUM_RESOURCES implies that __b may be up to 16 on the true branch. 8. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. 11. incr: Incrementing __b. The value of __b may now be up to 17. 12. alias: Assigning: r = &pdev->resource[__b]. r may now point to as high as element 17 of pdev->resource (which consists of 17 64-byte elements). 13. Condition __b < PCI_NUM_RESOURCES, taking true branch. 14. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. 562 pci_dev_for_each_resource(pdev, r) { 4. Condition resource_type(r) != 512, taking true branch. 9. Condition resource_type(r) != 512, taking true branch. CID 1529911 (#1 of 1): Out-of-bounds read (OVERRUN) 15. overrun-local: Overrunning array of 1088 bytes at byte offset 1088 by dereferencing pointer r. [show details] 563 if (resource_type(r) != IORESOURCE_MEM) 5. Continuing loop. 10. Continuing loop. 564 continue;
On Tue, May 09, 2023 at 01:21:22PM -0500, Bjorn Helgaas wrote: > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote: > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: > > > Provide two new helper macros to iterate over PCI device resources and > > > convert users. > > > Applied 2-7 to pci/resource for v6.4, thanks, I really like this! > > This is 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()") > upstream now. > > Coverity complains about each use, It needs more clarification here. Use of reduced variant of the macro or all of them? If the former one, then I can speculate that Coverity (famous for false positives) simply doesn't understand `for (type var; var ...)` code. > sample below from > drivers/pci/vgaarb.c. I didn't investigate at all, so it might be a > false positive; just FYI. > > 1. Condition screen_info.capabilities & (2U /* 1 << 1 */), taking true branch. > 556 if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > 557 base |= (u64)screen_info.ext_lfb_base << 32; > 558 > 559 limit = base + size; > 560 > 561 /* Does firmware framebuffer belong to us? */ > 2. Condition __b < PCI_NUM_RESOURCES, taking true branch. > 3. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. > 6. Condition __b < PCI_NUM_RESOURCES, taking true branch. > 7. cond_at_most: Checking __b < PCI_NUM_RESOURCES implies that __b may be up to 16 on the true branch. > 8. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. > 11. incr: Incrementing __b. The value of __b may now be up to 17. > 12. alias: Assigning: r = &pdev->resource[__b]. r may now point to as high as element 17 of pdev->resource (which consists of 17 64-byte elements). > 13. Condition __b < PCI_NUM_RESOURCES, taking true branch. > 14. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. > 562 pci_dev_for_each_resource(pdev, r) { > 4. Condition resource_type(r) != 512, taking true branch. > 9. Condition resource_type(r) != 512, taking true branch. > > CID 1529911 (#1 of 1): Out-of-bounds read (OVERRUN) > 15. overrun-local: Overrunning array of 1088 bytes at byte offset 1088 by dereferencing pointer r. [show details] > 563 if (resource_type(r) != IORESOURCE_MEM) > 5. Continuing loop. > 10. Continuing loop. > 564 continue;
On Fri, May 12, 2023 at 01:56:29PM +0300, Andy Shevchenko wrote: > On Tue, May 09, 2023 at 01:21:22PM -0500, Bjorn Helgaas wrote: > > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote: > > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: > > > > Provide two new helper macros to iterate over PCI device resources and > > > > convert users. > > > > > Applied 2-7 to pci/resource for v6.4, thanks, I really like this! > > > > This is 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()") > > upstream now. > > > > Coverity complains about each use, > > It needs more clarification here. Use of reduced variant of the > macro or all of them? If the former one, then I can speculate that > Coverity (famous for false positives) simply doesn't understand `for > (type var; var ...)` code. True, Coverity finds false positives. It flagged every use in drivers/pci and drivers/pnp. It didn't mention the arch/alpha, arm, mips, powerpc, sh, or sparc uses, but I think it just didn't look at those. It flagged both: pbus_size_io pci_dev_for_each_resource(dev, r) pbus_size_mem pci_dev_for_each_resource(dev, r, i) Here's a spreadsheet with a few more details (unfortunately I don't know how to make it dump the actual line numbers or analysis like I pasted below, so "pci_dev_for_each_resource" doesn't appear). These are mostly in the "Drivers-PCI" component. https://docs.google.com/spreadsheets/d/1ohOJwxqXXoDUA0gwopgk-z-6ArLvhN7AZn4mIlDkHhQ/edit?usp=sharing These particular reports are in the "High Impact Outstanding" tab. > > sample below from > > drivers/pci/vgaarb.c. I didn't investigate at all, so it might be a > > false positive; just FYI. > > > > 1. Condition screen_info.capabilities & (2U /* 1 << 1 */), taking true branch. > > 556 if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > > 557 base |= (u64)screen_info.ext_lfb_base << 32; > > 558 > > 559 limit = base + size; > > 560 > > 561 /* Does firmware framebuffer belong to us? */ > > 2. Condition __b < PCI_NUM_RESOURCES, taking true branch. > > 3. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. > > 6. Condition __b < PCI_NUM_RESOURCES, taking true branch. > > 7. cond_at_most: Checking __b < PCI_NUM_RESOURCES implies that __b may be up to 16 on the true branch. > > 8. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. > > 11. incr: Incrementing __b. The value of __b may now be up to 17. > > 12. alias: Assigning: r = &pdev->resource[__b]. r may now point to as high as element 17 of pdev->resource (which consists of 17 64-byte elements). > > 13. Condition __b < PCI_NUM_RESOURCES, taking true branch. > > 14. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. > > 562 pci_dev_for_each_resource(pdev, r) { > > 4. Condition resource_type(r) != 512, taking true branch. > > 9. Condition resource_type(r) != 512, taking true branch. > > > > CID 1529911 (#1 of 1): Out-of-bounds read (OVERRUN) > > 15. overrun-local: Overrunning array of 1088 bytes at byte offset 1088 by dereferencing pointer r. [show details] > > 563 if (resource_type(r) != IORESOURCE_MEM) > > 5. Continuing loop. > > 10. Continuing loop. > > 564 continue; > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, May 12, 2023 at 02:48:51PM -0500, Bjorn Helgaas wrote: > On Fri, May 12, 2023 at 01:56:29PM +0300, Andy Shevchenko wrote: > > On Tue, May 09, 2023 at 01:21:22PM -0500, Bjorn Helgaas wrote: > > > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote: > > > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: > > > > > Provide two new helper macros to iterate over PCI device resources and > > > > > convert users. > > > > > > > Applied 2-7 to pci/resource for v6.4, thanks, I really like this! > > > > > > This is 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()") > > > upstream now. > > > > > > Coverity complains about each use, > > > > It needs more clarification here. Use of reduced variant of the > > macro or all of them? If the former one, then I can speculate that > > Coverity (famous for false positives) simply doesn't understand `for > > (type var; var ...)` code. > > True, Coverity finds false positives. It flagged every use in > drivers/pci and drivers/pnp. It didn't mention the arch/alpha, arm, > mips, powerpc, sh, or sparc uses, but I think it just didn't look at > those. > > It flagged both: > > pbus_size_io pci_dev_for_each_resource(dev, r) > pbus_size_mem pci_dev_for_each_resource(dev, r, i) > > Here's a spreadsheet with a few more details (unfortunately I don't > know how to make it dump the actual line numbers or analysis like I > pasted below, so "pci_dev_for_each_resource" doesn't appear). These > are mostly in the "Drivers-PCI" component. > > https://docs.google.com/spreadsheets/d/1ohOJwxqXXoDUA0gwopgk-z-6ArLvhN7AZn4mIlDkHhQ/edit?usp=sharing > > These particular reports are in the "High Impact Outstanding" tab. Where are we at? Are we going to ignore this because some Coverity reports are false positives? Bjorn
Hi, On Tue, 30 May 2023 at 23:34, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Fri, May 12, 2023 at 02:48:51PM -0500, Bjorn Helgaas wrote: > > On Fri, May 12, 2023 at 01:56:29PM +0300, Andy Shevchenko wrote: > > > On Tue, May 09, 2023 at 01:21:22PM -0500, Bjorn Helgaas wrote: > > > > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote: > > > > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: > > > > > > Provide two new helper macros to iterate over PCI device resources and > > > > > > convert users. > > > > > > > > > Applied 2-7 to pci/resource for v6.4, thanks, I really like this! > > > > > > > > This is 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()") > > > > upstream now. > > > > > > > > Coverity complains about each use, > > > > > > It needs more clarification here. Use of reduced variant of the > > > macro or all of them? If the former one, then I can speculate that > > > Coverity (famous for false positives) simply doesn't understand `for > > > (type var; var ...)` code. > > > > True, Coverity finds false positives. It flagged every use in > > drivers/pci and drivers/pnp. It didn't mention the arch/alpha, arm, > > mips, powerpc, sh, or sparc uses, but I think it just didn't look at > > those. > > > > It flagged both: > > > > pbus_size_io pci_dev_for_each_resource(dev, r) > > pbus_size_mem pci_dev_for_each_resource(dev, r, i) > > > > Here's a spreadsheet with a few more details (unfortunately I don't > > know how to make it dump the actual line numbers or analysis like I > > pasted below, so "pci_dev_for_each_resource" doesn't appear). These > > are mostly in the "Drivers-PCI" component. > > > > https://docs.google.com/spreadsheets/d/1ohOJwxqXXoDUA0gwopgk-z-6ArLvhN7AZn4mIlDkHhQ/edit?usp=sharing > > > > These particular reports are in the "High Impact Outstanding" tab. > > Where are we at? Are we going to ignore this because some Coverity > reports are false positives? Looking at the code I understand where coverity is coming from: #define __pci_dev_for_each_res0(dev, res, ...) \ for (unsigned int __b = 0; \ res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES; \ __b++) res will be assigned before __b is checked for being less than PCI_NUM_RESOURCES, making it point to behind the array at the end of the last loop iteration. Rewriting the test expression as __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b)); should avoid the (coverity) warning by making use of lazy evaluation. It probably makes the code slightly less performant as res will now be checked for being not NULL (which will always be true), but I doubt it will be significant (or in any hot paths). Regards, Jonas
On Wed, 31 May 2023 at 23:30, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, May 31, 2023 at 08:48:35PM +0200, Jonas Gorski wrote: > > ... > > > Looking at the code I understand where coverity is coming from: > > > > #define __pci_dev_for_each_res0(dev, res, ...) \ > > for (unsigned int __b = 0; \ > > res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES; \ > > __b++) > > > > res will be assigned before __b is checked for being less than > > PCI_NUM_RESOURCES, making it point to behind the array at the end of > > the last loop iteration. > > > > Rewriting the test expression as > > > > __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b)); > > > > should avoid the (coverity) warning by making use of lazy evaluation. > > > > It probably makes the code slightly less performant as res will now be > > checked for being not NULL (which will always be true), but I doubt it > > will be significant (or in any hot paths). > > Thanks a lot for looking into this! I think you're right, and I think > the rewritten expression is more logical as well. Do you want to post > a patch for it? Not sure when I'll come around to, so I have no strong feeling here. So feel free to just borrow my suggestion, especially since I won't be able to test it (don't have a kernel tree ready I can build and boot). Also looking more closely at the Coverity output, I think it might not handle the comma operator well in the loop condition: > 11. incr: Incrementing __b. The value of __b may now be up to 17. > 12. alias: Assigning: r = &pdev->resource[__b]. r may now point to as high as element 17 of pdev->resource (which consists of 17 64-byte elements). > 13. Condition __b < PCI_NUM_RESOURCES, taking true branch. > 14. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch. 13 If __b is 17 ( = PCI_NUM_RESOURCES) we wouldn't taking the true branch, but somehow Coverity thinks that we do. No idea if it is worth reporting to Coverity. The changed condition statement should hopefully silence the warning though. Regards Jonas
On Wed, May 31, 2023 at 08:48:35PM +0200, Jonas Gorski wrote: > On Tue, 30 May 2023 at 23:34, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, May 12, 2023 at 02:48:51PM -0500, Bjorn Helgaas wrote: > > > On Fri, May 12, 2023 at 01:56:29PM +0300, Andy Shevchenko wrote: > > > > On Tue, May 09, 2023 at 01:21:22PM -0500, Bjorn Helgaas wrote: > > > > > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote: > > > > > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote: > > > > > > > Provide two new helper macros to iterate over PCI device resources and > > > > > > > convert users. > > > > > > > > > > > Applied 2-7 to pci/resource for v6.4, thanks, I really like this! > > > > > > > > > > This is 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()") > > > > > upstream now. > > > > > > > > > > Coverity complains about each use, > > > > > > > > It needs more clarification here. Use of reduced variant of the > > > > macro or all of them? If the former one, then I can speculate that > > > > Coverity (famous for false positives) simply doesn't understand `for > > > > (type var; var ...)` code. > > > > > > True, Coverity finds false positives. It flagged every use in > > > drivers/pci and drivers/pnp. It didn't mention the arch/alpha, arm, > > > mips, powerpc, sh, or sparc uses, but I think it just didn't look at > > > those. > > > > > > It flagged both: > > > > > > pbus_size_io pci_dev_for_each_resource(dev, r) > > > pbus_size_mem pci_dev_for_each_resource(dev, r, i) > > > > > > Here's a spreadsheet with a few more details (unfortunately I don't > > > know how to make it dump the actual line numbers or analysis like I > > > pasted below, so "pci_dev_for_each_resource" doesn't appear). These > > > are mostly in the "Drivers-PCI" component. > > > > > > https://docs.google.com/spreadsheets/d/1ohOJwxqXXoDUA0gwopgk-z-6ArLvhN7AZn4mIlDkHhQ/edit?usp=sharing > > > > > > These particular reports are in the "High Impact Outstanding" tab. > > > > Where are we at? Are we going to ignore this because some Coverity > > reports are false positives? > > Looking at the code I understand where coverity is coming from: > > #define __pci_dev_for_each_res0(dev, res, ...) \ > for (unsigned int __b = 0; \ > res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES; \ > __b++) > > res will be assigned before __b is checked for being less than > PCI_NUM_RESOURCES, making it point to behind the array at the end of > the last loop iteration. Which is fine and you stumbled over the same mistake I made, that's why the documentation has been added to describe why the heck this macro is written the way it's written. Coverity sucks. > Rewriting the test expression as > > __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b)); > > should avoid the (coverity) warning by making use of lazy evaluation. Obviously NAK. > It probably makes the code slightly less performant as res will now be > checked for being not NULL (which will always be true), but I doubt it > will be significant (or in any hot paths).
On Thu, Jun 01, 2023 at 07:25:46PM +0300, Andy Shevchenko wrote: > On Wed, May 31, 2023 at 08:48:35PM +0200, Jonas Gorski wrote: > > On Tue, 30 May 2023 at 23:34, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Fri, May 12, 2023 at 02:48:51PM -0500, Bjorn Helgaas wrote: ... > > > Where are we at? Are we going to ignore this because some Coverity > > > reports are false positives? > > > > Looking at the code I understand where coverity is coming from: > > > > #define __pci_dev_for_each_res0(dev, res, ...) \ > > for (unsigned int __b = 0; \ > > res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES; \ > > __b++) > > > > res will be assigned before __b is checked for being less than > > PCI_NUM_RESOURCES, making it point to behind the array at the end of > > the last loop iteration. > > Which is fine and you stumbled over the same mistake I made, that's why the > documentation has been added to describe why the heck this macro is written > the way it's written. > > Coverity sucks. > > > Rewriting the test expression as > > > > __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b)); > > > > should avoid the (coverity) warning by making use of lazy evaluation. > > Obviously NAK. > > > It probably makes the code slightly less performant as res will now be > > checked for being not NULL (which will always be true), but I doubt it > > will be significant (or in any hot paths). Oh my god, I mistakenly read this as bus macro, sorry for my rant, it's simply wrong.
On Wed, May 31, 2023 at 04:30:28PM -0500, Bjorn Helgaas wrote: > On Wed, May 31, 2023 at 08:48:35PM +0200, Jonas Gorski wrote: ... > > Looking at the code I understand where coverity is coming from: > > > > #define __pci_dev_for_each_res0(dev, res, ...) \ > > for (unsigned int __b = 0; \ > > res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES; \ > > __b++) > > > > res will be assigned before __b is checked for being less than > > PCI_NUM_RESOURCES, making it point to behind the array at the end of > > the last loop iteration. > > > > Rewriting the test expression as > > > > __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b)); > > > > should avoid the (coverity) warning by making use of lazy evaluation. > > > > It probably makes the code slightly less performant as res will now be > > checked for being not NULL (which will always be true), but I doubt it > > will be significant (or in any hot paths). > > Thanks a lot for looking into this! I think you're right, and I think > the rewritten expression is more logical as well. Do you want to post > a patch for it? Gimme some time, I was on a long leave and now it's a pile to handle.