On Fri, Jun 18, 2021 at 03:03:24PM +0100, Jon Hunter wrote: > Hi Bjorn, > > On 18/06/2021 14:13, Bjorn Helgaas wrote: > > [+to Jon, +cc Thierry] > > > > On Fri, Jun 18, 2021 at 06:34:20PM +0800, kernel test robot wrote: > >> tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git for-linus > >> branch HEAD: 15ac366c3d20ce1e08173f1de393a8ce95a1facf PCI: aardvark: Fix kernel panic during PIO transfer > >> > >> possible Warning in current branch: > >> > >> drivers/pci/controller/dwc/pcie-tegra194.c:1829:23: warning: Shifting signed 32-bit value by 31 bits is undefined behaviour. See condition at line 1826. [shiftTooManyBitsSigned] > > > > This looks like a legit warning, but I think the only commit that > > could be related is this one: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=for-linus&id=99ab5996278379a02d5a84c4a7ac33a2ebfdb29e > > > > which doesn't touch that code. > > > > It does *move* some code, so maybe this was an existing warning that > > moved enough that the robot thought it was new? > > > I guessing that this is now happening because we are now compiling the > bulk of the code in the driver. However, yes looks like it has been > there for a while. > > I wonder if the '(1 << irq)' is being treated as a signed type. > > > How can we reproduce this to make sure we fix it? > > I was able to reproduce it by ... > > $ cppcheck --force --enable=all drivers/pci/controller/dwc/pcie-tegra194.c > Checking drivers/pci/controller/dwc/pcie-tegra194.c ... > > drivers/pci/controller/dwc/pcie-tegra194.c:1829:23: portability: Shifting signed 32-bit value by 31 bits is implementation-defined behaviour. See condition at line 1826. [shiftTooManyBitsSigned] > appl_writel(pcie, (1 << irq), APPL_MSI_CTRL_1); > ^ > drivers/pci/controller/dwc/pcie-tegra194.c:1826:19: note: Assuming that condition 'irq>31' is not redundant > if (unlikely(irq > 31)) > ^ > drivers/pci/controller/dwc/pcie-tegra194.c:1829:23: note: Shift > appl_writel(pcie, (1 << irq), APPL_MSI_CTRL_1); > > > I was able to fix this by ... > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > index 8fc08336f76e..05d6a8da190b 100644 > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > @@ -1826,7 +1826,7 @@ static int tegra_pcie_ep_raise_msi_irq(struct tegra_pcie_dw *pcie, u16 irq) > if (unlikely(irq > 31)) > return -EINVAL; > > - appl_writel(pcie, (1 << irq), APPL_MSI_CTRL_1); > + appl_writel(pcie, (BIT(1) << irq), APPL_MSI_CTRL_1); > > return 0; > } > > I can send this as a patch. I think that's not the same anymore. The equivalent would rather be: appl_writel(pcie, (BIT(0) << irq), APPL_MSI_CTRL_1); But I think this can be achieved more easily by doing this: appl_writel(pcie, 1UL << irq, APPL_MSI_CTRL_1); Which is what BIT(0) would effectively end up doing. Actually, this sounds like it should really have been this all along: appl_writel(pcie, BIT(irq), APPL_MSI_CTRL_1); Which should also get rid of that warning. Thierry
Attachment:
signature.asc
Description: PGP signature
