On Tue, Jun 27, 2023 at 11:39:16AM +0200, Rick Wertenbroek wrote:
> On Tue, Jun 27, 2023 at 9:19 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> >
> > Hello Rick Wertenbroek,
> >
> > The patch dc73ed0f1b8b: "PCI: rockchip: Fix window mapping and
> > address translation for endpoint" from Apr 18, 2023, leads to the
> > following Smatch static checker warning:
> >
> > drivers/pci/controller/pcie-rockchip-ep.c:405 rockchip_pcie_ep_send_msi_irq()
> > warn: was expecting a 64 bit value instead of '~4294967040'
> >
> > drivers/pci/controller/pcie-rockchip-ep.c
> > 351 static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> > 352 u8 interrupt_num)
> > 353 {
> > 354 struct rockchip_pcie *rockchip = &ep->rockchip;
> > 355 u32 flags, mme, data, data_mask;
> > 356 u8 msi_count;
> > 357 u64 pci_addr;
> > ^^^^^^^^^^^^^
> >
> > 358 u32 r;
> > 359
> > 360 /* Check MSI enable bit */
> > 361 flags = rockchip_pcie_read(&ep->rockchip,
> > 362 ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> > 363 ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> > 364 if (!(flags & ROCKCHIP_PCIE_EP_MSI_CTRL_ME))
> > 365 return -EINVAL;
> > 366
> > 367 /* Get MSI numbers from MME */
> > 368 mme = ((flags & ROCKCHIP_PCIE_EP_MSI_CTRL_MME_MASK) >>
> > 369 ROCKCHIP_PCIE_EP_MSI_CTRL_MME_OFFSET);
> > 370 msi_count = 1 << mme;
> > 371 if (!interrupt_num || interrupt_num > msi_count)
> > 372 return -EINVAL;
> > 373
> > 374 /* Set MSI private data */
> > 375 data_mask = msi_count - 1;
> > 376 data = rockchip_pcie_read(rockchip,
> > 377 ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> > 378 ROCKCHIP_PCIE_EP_MSI_CTRL_REG +
> > 379 PCI_MSI_DATA_64);
> > 380 data = (data & ~data_mask) | ((interrupt_num - 1) & data_mask);
> > 381
> > 382 /* Get MSI PCI address */
> > 383 pci_addr = rockchip_pcie_read(rockchip,
> > 384 ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> > 385 ROCKCHIP_PCIE_EP_MSI_CTRL_REG +
> > 386 PCI_MSI_ADDRESS_HI);
> > 387 pci_addr <<= 32;
> >
> > The high 32 bits are definitely set.
> >
> > 388 pci_addr |= rockchip_pcie_read(rockchip,
> > 389 ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> > 390 ROCKCHIP_PCIE_EP_MSI_CTRL_REG +
> > 391 PCI_MSI_ADDRESS_LO);
> > 392
> > 393 /* Set the outbound region if needed. */
> > 394 if (unlikely(ep->irq_pci_addr != (pci_addr & PCIE_ADDR_MASK) ||
> > 395 ep->irq_pci_fn != fn)) {
> > 396 r = rockchip_ob_region(ep->irq_phys_addr);
> > 397 rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
> > 398 ep->irq_phys_addr,
> > 399 pci_addr & PCIE_ADDR_MASK,
> > 400 ~PCIE_ADDR_MASK + 1);
> > 401 ep->irq_pci_addr = (pci_addr & PCIE_ADDR_MASK);
> > 402 ep->irq_pci_fn = fn;
> > 403 }
> > 404
> > --> 405 writew(data, ep->irq_cpu_addr + (pci_addr & ~PCIE_ADDR_MASK));
> >
> > PCIE_ADDR_MASK is 0xffffff00 (which is unsigned int). What Smatch is
> > saying here is that pci_addr is a u64 it looks like the intention was to
> > zero out the last byte, but instead we are zeroing the high 32 bits as
> > well as the last 8 bits.
>
> Hello,
> You are right there is a problem.
>
> The warning at line 405, however, seems to be a false positive. Because the
> mask is 0xffffff00 (which is unsigned int) and the ~ is applied before promotion
> this results in 0xff, which is then promoted to 0x00000000000000ff when applied
> to pci_addr so this is fine.
>
Hm... Yeah. This warning message is quite old and I haven't thought
about it properly in some time. Probably I should silence the warning
if BIT(31) is zero and only the lowest bits are 1.
I'll take a look at all these warnings again.
> What you describe about zeroing the upper 32 bits and not only the lower 8
> refers to line 399 where we apply the PCI_ADDR_MASK 0xffffff00 with the
> & operator to pci_addr, this is the real issue, as you said this
> zeroes the upper
> 32-bits, which is not the intended behavior. Thank you for pointing this out.
Heh. No, I don't get credit for spotting line 399. That's all you.
I don't know how to identify the issue on line 399 through static
analysis. It think it's impossible to tell unintentional masking from
intentional masking on that line.
regards,
dan carpenter
