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.
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.
The original function had the mask constant as a u64 variable (with inverted
logic) :
https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pcie-rockchip-ep.c#L415
I replaced this with a constant and thought it would be promoted, but since
it is promoted to unsigned it isn't sign-extended, therefore the upper 32-bits
are zeroed which is not the intended behavior
The simplest way to fix this would be to revert to the initial logic with the
variable as linked above. Or to replace set the constant to 0xffffffffffffff00
and revert the two changes in drivers/pci/controller/pcie-rockchip.h :
-#define PCIE_CORE_OB_REGION_ADDR0_LO_ADDR 0xffffff00
+#define PCIE_CORE_OB_REGION_ADDR0_LO_ADDR PCIE_ADDR_MASK
-#define PCIE_CORE_IB_REGION_ADDR0_LO_ADDR 0xffffff00
+#define PCIE_CORE_IB_REGION_ADDR0_LO_ADDR PCIE_ADDR_MASK
>
> 406 return 0;
> 407 }
>
> regards,
> dan carpenter
Thank you very much for spotting this.
I will prepare the patch for this in a few days (I am currently out of office),
I'll add the "reported-by" tag and fix this, unless you want to fix and submit
it right away.
Thanks.
Best regards,
Rick Wertenbroek
