| Message ID | 170052362584.21270.8345708191142620624.stgit@skinsburskii. (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: mediatek: Fix sparse warning caused to virt_to_phys() prototype change | expand |
On Mon, Nov 20, 2023 at 03:40:33PM -0800, Stanislav Kinsburskii wrote: > Explicitly cast __iomem pointer to const void* with __force to fix the > following warning: > > warning: incorrect type in argument 1 (different address spaces) > expected void const volatile *address > got void [noderef] __iomem * I have two questions about this: 1) There's no other use of __force in drivers/pci, so I don't know what's special about pcie-mediatek.c. There should be a way to fix the types so it's not needed. 2) virt_to_phys() is not quite right to begin with because what we want is a *bus* address, not the CPU physical address we get from virt_to_phys(). Obviously the current platforms that use this must not apply any offset between bus and CPU physical addresses, but it's not something we should rely on. There are only three drivers (pci-aardvark.c, pcie-xilinx.c, and this one) that use virt_to_phys(), and they're all slightly wrong here. The *_compose_msi_msg() methods could use a little more consistency across the board. > Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.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..27f0f79810a1 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 const 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 const void *)port->base + PCIE_MSI_VECTOR); > val = lower_32_bits(msg_addr); > writel(val, port->base + PCIE_IMSI_ADDR); > > > >
On Tue, Nov 21, 2023 at 03:37:55PM -0600, Bjorn Helgaas wrote: > On Mon, Nov 20, 2023 at 03:40:33PM -0800, Stanislav Kinsburskii wrote: > > Explicitly cast __iomem pointer to const void* with __force to fix the > > following warning: > > > > warning: incorrect type in argument 1 (different address spaces) > > expected void const volatile *address > > got void [noderef] __iomem * > > I have two questions about this: > > 1) There's no other use of __force in drivers/pci, so I don't know > what's special about pcie-mediatek.c. There should be a way to fix > the types so it's not needed. > __force suppreses the following sparse warning: warning: cast removes address space '__iomem' of expression > 2) virt_to_phys() is not quite right to begin with because what we > want is a *bus* address, not the CPU physical address we get from > virt_to_phys(). Obviously the current platforms that use this must > not apply any offset between bus and CPU physical addresses, but > it's not something we should rely on. > > There are only three drivers (pci-aardvark.c, pcie-xilinx.c, and > this one) that use virt_to_phys(), and they're all slightly wrong > here. > > The *_compose_msi_msg() methods could use a little more consistency > across the board. > Could you elaborate on what do you suggest? Should virt_to_phys() be simply removed? > > Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.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..27f0f79810a1 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 const 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 const void *)port->base + PCIE_MSI_VECTOR); > > val = lower_32_bits(msg_addr); > > writel(val, port->base + PCIE_IMSI_ADDR); > > > > > > > >
On Tue, Nov 21, 2023 at 02:05:56PM -0800, Stanislav Kinsburskii wrote: > On Tue, Nov 21, 2023 at 03:37:55PM -0600, Bjorn Helgaas wrote: > > On Mon, Nov 20, 2023 at 03:40:33PM -0800, Stanislav Kinsburskii wrote: > > > Explicitly cast __iomem pointer to const void* with __force to fix the > > > following warning: > > > > > > warning: incorrect type in argument 1 (different address spaces) > > > expected void const volatile *address > > > got void [noderef] __iomem * > > > > I have two questions about this: > > > > 1) There's no other use of __force in drivers/pci, so I don't know > > what's special about pcie-mediatek.c. There should be a way to fix > > the types so it's not needed. > > __force suppreses the following sparse warning: > > warning: cast removes address space '__iomem' of expression I'm suggesting that the cast is a band-aid that covers up a type mismatch, and there shouldn't be a mismatch in the first place. > > 2) virt_to_phys() is not quite right to begin with because what we > > want is a *bus* address, not the CPU physical address we get from > > virt_to_phys(). Obviously the current platforms that use this must > > not apply any offset between bus and CPU physical addresses, but > > it's not something we should rely on. > > > > There are only three drivers (pci-aardvark.c, pcie-xilinx.c, and > > this one) that use virt_to_phys(), and they're all slightly wrong > > here. > > > > The *_compose_msi_msg() methods could use a little more consistency > > across the board. > > Could you elaborate on what do you suggest? > Should virt_to_phys() be simply removed? The DMA API (Documentation/core-api/dma-api.rst) is the usual way to get bus addresses, since an MSI is basically a DMA on the PCI bus. https://lore.kernel.org/linux-pci/20230914203146.GA77870@bhelgaas/ Nobody is very motivated to fix these, I guess ;) I sort of hate to just throw in a cast to shut up the warning because it doesn't really solve the problem. > > > Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.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..27f0f79810a1 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 const 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 const void *)port->base + PCIE_MSI_VECTOR); > > > val = lower_32_bits(msg_addr); > > > writel(val, port->base + PCIE_IMSI_ADDR);
On Tue, Nov 21, 2023 at 04:23:25PM -0600, Bjorn Helgaas wrote: > On Tue, Nov 21, 2023 at 02:05:56PM -0800, Stanislav Kinsburskii wrote: > > On Tue, Nov 21, 2023 at 03:37:55PM -0600, Bjorn Helgaas wrote: > > > On Mon, Nov 20, 2023 at 03:40:33PM -0800, Stanislav Kinsburskii wrote: > > > > Explicitly cast __iomem pointer to const void* with __force to fix the > > > > following warning: > > > > > > > > warning: incorrect type in argument 1 (different address spaces) > > > > expected void const volatile *address > > > > got void [noderef] __iomem * > > > > > > I have two questions about this: > > > > > > 1) There's no other use of __force in drivers/pci, so I don't know > > > what's special about pcie-mediatek.c. There should be a way to fix > > > the types so it's not needed. > > > > __force suppreses the following sparse warning: > > > > warning: cast removes address space '__iomem' of expression > > I'm suggesting that the cast is a band-aid that covers up a type > mismatch, and there shouldn't be a mismatch in the first place. > > > > 2) virt_to_phys() is not quite right to begin with because what we > > > want is a *bus* address, not the CPU physical address we get from > > > virt_to_phys(). Obviously the current platforms that use this must > > > not apply any offset between bus and CPU physical addresses, but > > > it's not something we should rely on. > > > > > > There are only three drivers (pci-aardvark.c, pcie-xilinx.c, and > > > this one) that use virt_to_phys(), and they're all slightly wrong > > > here. > > > > > > The *_compose_msi_msg() methods could use a little more consistency > > > across the board. > > > > Could you elaborate on what do you suggest? > > Should virt_to_phys() be simply removed? > > The DMA API (Documentation/core-api/dma-api.rst) is the usual way to > get bus addresses, since an MSI is basically a DMA on the PCI bus. > > https://lore.kernel.org/linux-pci/20230914203146.GA77870@bhelgaas/ > > Nobody is very motivated to fix these, I guess ;) I sort of hate to > just throw in a cast to shut up the warning because it doesn't really > solve the problem. > This is fair. However I have neither expertize to assess, no hardware to test such intrusive changes. I guess it's a no-op then, is it? > > > > Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.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..27f0f79810a1 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 const 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 const void *)port->base + PCIE_MSI_VECTOR); > > > > val = lower_32_bits(msg_addr); > > > > writel(val, port->base + PCIE_IMSI_ADDR);
Hi, On Tue, 2023-11-21 at 14:34 -0800, Stanislav Kinsburskii wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Tue, Nov 21, 2023 at 04:23:25PM -0600, Bjorn Helgaas wrote: > > On Tue, Nov 21, 2023 at 02:05:56PM -0800, Stanislav Kinsburskii > wrote: > > > On Tue, Nov 21, 2023 at 03:37:55PM -0600, Bjorn Helgaas wrote: > > > > On Mon, Nov 20, 2023 at 03:40:33PM -0800, Stanislav Kinsburskii > wrote: > > > > > Explicitly cast __iomem pointer to const void* with __force > to fix the > > > > > following warning: > > > > > > > > > > warning: incorrect type in argument 1 (different address > spaces) > > > > > expected void const volatile *address > > > > > got void [noderef] __iomem * > > > > > > > > I have two questions about this: > > > > > > > > 1) There's no other use of __force in drivers/pci, so I don't > know > > > > what's special about pcie-mediatek.c. There should be a way > to fix > > > > the types so it's not needed. > > > > > > __force suppreses the following sparse warning: > > > > > > warning: cast removes address space '__iomem' of expression > > > > I'm suggesting that the cast is a band-aid that covers up a type > > mismatch, and there shouldn't be a mismatch in the first place. > > > > > > 2) virt_to_phys() is not quite right to begin with because > what we > > > > want is a *bus* address, not the CPU physical address we get > from > > > > virt_to_phys(). Obviously the current platforms that use > this must > > > > not apply any offset between bus and CPU physical addresses, > but > > > > it's not something we should rely on. > > > > > > > > There are only three drivers (pci-aardvark.c, pcie-xilinx.c, > and > > > > this one) that use virt_to_phys(), and they're all slightly > wrong > > > > here. > > > > > > > > The *_compose_msi_msg() methods could use a little more > consistency > > > > across the board. > > > > > > Could you elaborate on what do you suggest? > > > Should virt_to_phys() be simply removed? > > > > The DMA API (Documentation/core-api/dma-api.rst) is the usual way > to > > get bus addresses, since an MSI is basically a DMA on the PCI bus. > > > > https://lore.kernel.org/linux-pci/20230914203146.GA77870@bhelgaas/ > > > > Nobody is very motivated to fix these, I guess ;) I sort of hate > to > > just throw in a cast to shut up the warning because it doesn't > really > > solve the problem. > > > > This is fair. > However I have neither expertize to assess, no hardware to test such > intrusive > changes. > I guess it's a no-op then, is it? I think I can try and test 'dmam_alloc_coherent()' or other proper APIs on the MediaTek platform, and if it works as expected, I can send the patch to fix this. Thanks. > > > > > > Signed-off-by: Stanislav Kinsburskii < > skinsburskii@linux.microsoft.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..27f0f79810a1 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 const 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 const void *)port->base + > PCIE_MSI_VECTOR); > > > > > val = lower_32_bits(msg_addr); > > > > > writel(val, port->base + PCIE_IMSI_ADDR);
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c index 66a8f73296fc..27f0f79810a1 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 const 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 const void *)port->base + PCIE_MSI_VECTOR); val = lower_32_bits(msg_addr); writel(val, port->base + PCIE_IMSI_ADDR);
Explicitly cast __iomem pointer to const void* with __force to fix the following warning: warning: incorrect type in argument 1 (different address spaces) expected void const volatile *address got void [noderef] __iomem * Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> --- drivers/pci/controller/pcie-mediatek.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)