Re: [PATCH v2 15/16] powerpc/powernv/sriov: Make single PE mode a per-BAR setting

[Michael Ellerman <mpe@ellerman.id.au>]

Nathan Chancellor <natechancellor@gmail.com> writes:
> On Sun, Aug 02, 2020 at 11:12:23PM +1000, Michael Ellerman wrote:
>> Nathan Chancellor <natechancellor@gmail.com> writes:
>> > On Wed, Jul 22, 2020 at 04:57:14PM +1000, Oliver O'Halloran wrote:
>> >> Using single PE BARs to map an SR-IOV BAR is really a choice about what
>> >> strategy to use when mapping a BAR. It doesn't make much sense for this to
>> >> be a global setting since a device might have one large BAR which needs to
>> >> be mapped with single PE windows and another smaller BAR that can be mapped
>> >> with a regular segmented window. Make the segmented vs single decision a
>> >> per-BAR setting and clean up the logic that decides which mode to use.
>> >> 
>> >> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
>> >> ---
>> >> v2: Dropped unused total_vfs variables in pnv_pci_ioda_fixup_iov_resources()
>> >>     Dropped bar_no from pnv_pci_iov_resource_alignment()
>> >>     Minor re-wording of comments.
>> >> ---
>> >>  arch/powerpc/platforms/powernv/pci-sriov.c | 131 ++++++++++-----------
>> >>  arch/powerpc/platforms/powernv/pci.h       |  11 +-
>> >>  2 files changed, 73 insertions(+), 69 deletions(-)
>> >> 
>> >> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
>> >> index ce8ad6851d73..76215d01405b 100644
>> >> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
>> >> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
>> >> @@ -260,42 +256,40 @@ void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev)
>> >>  resource_size_t pnv_pci_iov_resource_alignment(struct pci_dev *pdev,
>> >>  						      int resno)
>> >>  {
>> >> -	struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
>> >>  	struct pnv_iov_data *iov = pnv_iov_get(pdev);
>> >>  	resource_size_t align;
>> >>  
>> >> +	/*
>> >> +	 * iov can be null if we have an SR-IOV device with IOV BAR that can't
>> >> +	 * be placed in the m64 space (i.e. The BAR is 32bit or non-prefetch).
>> >> +	 * In that case we don't allow VFs to be enabled since one of their
>> >> +	 * BARs would not be placed in the correct PE.
>> >> +	 */
>> >> +	if (!iov)
>> >> +		return align;
>> >> +	if (!iov->vfs_expanded)
>> >> +		return align;
>> >> +
>> >> +	align = pci_iov_resource_size(pdev, resno);
>> 
>> That's, oof.
>> 
>> > I am not sure if it has been reported yet but clang points out that
>> > align is initialized after its use:
>> >
>> > arch/powerpc/platforms/powernv/pci-sriov.c:267:10: warning: variable 'align' is uninitialized when used here [-Wuninitialized]
>> >                 return align;
>> >                        ^~~~~
>> > arch/powerpc/platforms/powernv/pci-sriov.c:258:23: note: initialize the variable 'align' to silence this warning
>> >         resource_size_t align;
>> >                              ^
>> >                               = 0
>> > 1 warning generated.
>> 
>> But I can't get gcc to warn about it?
>> 
>> It produces some code, so it's not like the whole function has been
>> elided or something. I'm confused.
>
> -Wmaybe-uninitialized was disabled in commit 78a5255ffb6a ("Stop the
> ad-hoc games with -Wno-maybe-initialized") upstream so GCC won't warn on
> stuff like this anymore.

Seems so. Just that there's no "maybe" here, it's very uninitialised.

> I would assume the function should still be generated since those checks
> are relevant, just the return value is bogus.

Yeah, just sometimes missing warnings boil down to the compiler eliding
whole sections of code, if it can convince itself they're unreachable.

AFAICS there's nothing weird going on here that should confuse GCC, it's
about as straight forward as it gets.

Actually I can reproduce it with:

$ cat > test.c <<EOF
int foo(void *p)
{
        unsigned align;

        if (!p)
                return align;

        return 0;
}
EOF

$ gcc -Wall -Wno-maybe-uninitialized -c test.c
$

No warning.

But I guess that's behaving as documented. The compiler can't prove that
foo() will be called with p == NULL, so it doesn't warn.

cheers