[bug report] PCI: rockchip: Fix window mapping and address translation for endpoint

[Rick Wertenbroek]

On Tue, Jun 27, 2023 at 9:19 AM Dan Carpenter <dan.carpenter at linaro.org> 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