| Message ID | 20230914192324.672997-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2,1/1] PCI: mediatek: Correct type for virt_to_phys() | expand |
On Thu, Sep 14, 2023 at 10:23:24PM +0300, Andy Shevchenko wrote: > virt_to_phys() takes a regular pointer, while driver supplies __iomem > annotated one. Force type to void to make sparse happy, otherwise > > pcie-mediatek.c:400:40: sparse: expected void volatile *address > pcie-mediatek.c:400:40: sparse: got void [noderef] __iomem * > > pcie-mediatek.c:523:44: sparse: expected void volatile *address > pcie-mediatek.c:523:44: sparse: got void [noderef] __iomem * > > Reported-by: Huacai Chen <chenhuacai@kernel.org> > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202309072237.9zxMv4MZ-lkp@intel.com/ > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pci/controller/pcie-mediatek.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 66a8f73296fc..5e795afd1cee 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -397,7 +397,7 @@ static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) > phys_addr_t addr; > > /* MT2712/MT7622 only support 32-bit MSI addresses */ > - addr = virt_to_phys(port->base + PCIE_MSI_VECTOR); > + addr = virt_to_phys((__force void *)port->base + PCIE_MSI_VECTOR); Lots of these drivers use either virt_to_phys() or platform_get_resource_byname() to get a physical address that they then use as the MSI target. But I don't think that's quite right -- the MSI is a DMA transaction on PCI, and in general there's no guarantee that bus addresses are identical to CPU physical addresses, so shouldn't we use a dma_addr_t obtained from the DMA API? dw_pcie_msi_host_init() has a complicated version of this that uses dmam_alloc_coherent(). > msg->address_hi = 0; > msg->address_lo = lower_32_bits(addr); > > @@ -520,7 +520,7 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port) > u32 val; > phys_addr_t msg_addr; > > - msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR); > + msg_addr = virt_to_phys((__force void *)port->base + PCIE_MSI_VECTOR); > val = lower_32_bits(msg_addr); > writel(val, port->base + PCIE_IMSI_ADDR); > > -- > 2.40.0.1.gaa8946217a0b > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Sep 14, 2023 at 03:31:46PM -0500, Bjorn Helgaas wrote: > On Thu, Sep 14, 2023 at 10:23:24PM +0300, Andy Shevchenko wrote: ... > > - addr = virt_to_phys(port->base + PCIE_MSI_VECTOR); > > + addr = virt_to_phys((__force void *)port->base + PCIE_MSI_VECTOR); > > Lots of these drivers use either virt_to_phys() or > platform_get_resource_byname() to get a physical address that they > then use as the MSI target. > > But I don't think that's quite right -- the MSI is a DMA transaction > on PCI, and in general there's no guarantee that bus addresses are > identical to CPU physical addresses, so shouldn't we use a dma_addr_t > obtained from the DMA API? > > dw_pcie_msi_host_init() has a complicated version of this that uses > dmam_alloc_coherent(). Fair enough. I leave it to you then to get it fixed properly. Consider this as just a heads up for the old report.
On Sat, Sep 16, 2023 at 01:41:59PM +0300, Andy Shevchenko wrote: > On Thu, Sep 14, 2023 at 03:31:46PM -0500, Bjorn Helgaas wrote: > > On Thu, Sep 14, 2023 at 10:23:24PM +0300, Andy Shevchenko wrote: > > ... > > > > - addr = virt_to_phys(port->base + PCIE_MSI_VECTOR); > > > + addr = virt_to_phys((__force void *)port->base + PCIE_MSI_VECTOR); > > > > Lots of these drivers use either virt_to_phys() or > > platform_get_resource_byname() to get a physical address that they > > then use as the MSI target. > > > > But I don't think that's quite right -- the MSI is a DMA transaction > > on PCI, and in general there's no guarantee that bus addresses are > > identical to CPU physical addresses, so shouldn't we use a dma_addr_t > > obtained from the DMA API? > > > > dw_pcie_msi_host_init() has a complicated version of this that uses > > dmam_alloc_coherent(). > > Fair enough. I leave it to you then to get it fixed properly. > Consider this as just a heads up for the old report. Sounds good, thanks! Bjorn
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c index 66a8f73296fc..5e795afd1cee 100644 --- a/drivers/pci/controller/pcie-mediatek.c +++ b/drivers/pci/controller/pcie-mediatek.c @@ -397,7 +397,7 @@ static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) phys_addr_t addr; /* MT2712/MT7622 only support 32-bit MSI addresses */ - addr = virt_to_phys(port->base + PCIE_MSI_VECTOR); + addr = virt_to_phys((__force void *)port->base + PCIE_MSI_VECTOR); msg->address_hi = 0; msg->address_lo = lower_32_bits(addr); @@ -520,7 +520,7 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port) u32 val; phys_addr_t msg_addr; - msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR); + msg_addr = virt_to_phys((__force void *)port->base + PCIE_MSI_VECTOR); val = lower_32_bits(msg_addr); writel(val, port->base + PCIE_IMSI_ADDR);
virt_to_phys() takes a regular pointer, while driver supplies __iomem annotated one. Force type to void to make sparse happy, otherwise pcie-mediatek.c:400:40: sparse: expected void volatile *address pcie-mediatek.c:400:40: sparse: got void [noderef] __iomem * pcie-mediatek.c:523:44: sparse: expected void volatile *address pcie-mediatek.c:523:44: sparse: got void [noderef] __iomem * Reported-by: Huacai Chen <chenhuacai@kernel.org> Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202309072237.9zxMv4MZ-lkp@intel.com/ Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pci/controller/pcie-mediatek.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)