Re: [PATCH] PCI: mediatek: Fix sparse warning caused to virt_to_phys() prototype change

Re: [PATCH] PCI: mediatek: Fix sparse warning caused to virt_to_phys() prototype change

[Bjorn Helgaas]

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);

Re: [PATCH] PCI: mediatek: Fix sparse warning caused to virt_to_phys() prototype change

[Jianjun Wang (王建军)]

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);

Re: [PATCH] PCI: mediatek: Fix sparse warning caused to virt_to_phys() prototype change

[Bjorn Helgaas]

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);
>  
> 
> 
>