* [bug report] PCI: rockchip: Fix window mapping and address translation for endpoint
@ 2023-06-27 7:19 Dan Carpenter
2023-06-27 9:39 ` Rick Wertenbroek
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2023-06-27 7:19 UTC (permalink / raw)
To: rick.wertenbroek; +Cc: linux-pci, linux-rockchip
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.
406 return 0;
407 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] PCI: rockchip: Fix window mapping and address translation for endpoint
2023-06-27 7:19 [bug report] PCI: rockchip: Fix window mapping and address translation for endpoint Dan Carpenter
@ 2023-06-27 9:39 ` Rick Wertenbroek
2023-06-27 11:18 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Rick Wertenbroek @ 2023-06-27 9:39 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-pci, linux-rockchip
On Tue, Jun 27, 2023 at 9:19 AM Dan Carpenter <dan.carpenter@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] PCI: rockchip: Fix window mapping and address translation for endpoint
2023-06-27 9:39 ` Rick Wertenbroek
@ 2023-06-27 11:18 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2023-06-27 11:18 UTC (permalink / raw)
To: Rick Wertenbroek; +Cc: linux-pci, linux-rockchip
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@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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-27 11:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-27 7:19 [bug report] PCI: rockchip: Fix window mapping and address translation for endpoint Dan Carpenter
2023-06-27 9:39 ` Rick Wertenbroek
2023-06-27 11:18 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).