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

Dan Carpenter dan.carpenter at linaro.org
Tue Jun 27 04:18:00 PDT 2023


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 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.
> 

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




More information about the Linux-rockchip mailing list