[PATCH rc] s390: Use the correct count for __iowrite64_copy()

[PATCH rc] s390: Use the correct count for __iowrite64_copy()

[Jason Gunthorpe]

The signature for __iowrite64_copy() requires the number of 64 bit
quantities, not bytes. Multiple by 8 to get to a byte length before
invoking zpci_memcpy_toio()

Fixes: 87bc359b9822 ("s390/pci: speed up __iowrite64_copy by using pci store block insn")
Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 arch/s390/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 676ac74026a82b..52a44e353796c0 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -252,7 +252,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 /* combine single writes by using store-block insn */
 void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
 {
-       zpci_memcpy_toio(to, from, count);
+	zpci_memcpy_toio(to, from, count * 8);
 }
 
 void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,

base-commit: cf4f722d8b48e66de348f2fe8d0e19c01d3e6b72
-- 
2.43.2

Re: [PATCH rc] s390: Use the correct count for __iowrite64_copy()

[Heiko Carstens]

On Fri, Feb 16, 2024 at 08:48:14PM -0400, Jason Gunthorpe wrote:
> The signature for __iowrite64_copy() requires the number of 64 bit
> quantities, not bytes. Multiple by 8 to get to a byte length before
> invoking zpci_memcpy_toio()
> 
> Fixes: 87bc359b9822 ("s390/pci: speed up __iowrite64_copy by using pci store block insn")
> Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  arch/s390/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 676ac74026a82b..52a44e353796c0 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -252,7 +252,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  /* combine single writes by using store-block insn */
>  void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
>  {
> -       zpci_memcpy_toio(to, from, count);
> +	zpci_memcpy_toio(to, from, count * 8);
>  }

Odd, this bug is 11 years old, and there was never any bug report.
Is this never called, or how is this possible?

Niklas, would you happen to have an idea?

Re: [PATCH rc] s390: Use the correct count for __iowrite64_copy()

[Niklas Schnelle]

On Mon, 2024-02-19 at 11:08 +0100, Heiko Carstens wrote:
> On Fri, Feb 16, 2024 at 08:48:14PM -0400, Jason Gunthorpe wrote:
> > The signature for __iowrite64_copy() requires the number of 64 bit
> > quantities, not bytes. Multiple by 8 to get to a byte length before
> > invoking zpci_memcpy_toio()
> > 
> > Fixes: 87bc359b9822 ("s390/pci: speed up __iowrite64_copy by using pci store block insn")
> > Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  arch/s390/pci/pci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > index 676ac74026a82b..52a44e353796c0 100644
> > --- a/arch/s390/pci/pci.c
> > +++ b/arch/s390/pci/pci.c
> > @@ -252,7 +252,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> >  /* combine single writes by using store-block insn */
> >  void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
> >  {
> > -       zpci_memcpy_toio(to, from, count);
> > +	zpci_memcpy_toio(to, from, count * 8);
> >  }
> 
> Odd, this bug is 11 years old, and there was never any bug report.
> Is this never called, or how is this possible?
> 
> Niklas, would you happen to have an idea?

I did ask myself the same question when I first saw this change but
still don't have a satisfactory answer.

As far as I can tell there is only one call that could be relevant for
us in drivers/net/ethernet/mellanox/mlx4/en_tx.c:mlx4_bf_copy(). That
call looks like it would be done during transmit. Not sure under what
exact circumstances or if a short write would directly cause observable
issues though. Note also that contrary to what the mlx4 name suggests
even ConnectX-4 based devices already use the mlx5 driver and that
doesn't call this helper at the moment. Jason has plans to use it there
in the future which is what caused him to even stumble over this
though.

Re: [PATCH rc] s390: Use the correct count for __iowrite64_copy()

[Jason Gunthorpe]

On Tue, Feb 20, 2024 at 01:22:11PM +0100, Niklas Schnelle wrote:
> On Mon, 2024-02-19 at 11:08 +0100, Heiko Carstens wrote:
> > On Fri, Feb 16, 2024 at 08:48:14PM -0400, Jason Gunthorpe wrote:
> > > The signature for __iowrite64_copy() requires the number of 64 bit
> > > quantities, not bytes. Multiple by 8 to get to a byte length before
> > > invoking zpci_memcpy_toio()
> > > 
> > > Fixes: 87bc359b9822 ("s390/pci: speed up __iowrite64_copy by using pci store block insn")
> > > Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > > ---
> > >  arch/s390/pci/pci.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > > index 676ac74026a82b..52a44e353796c0 100644
> > > --- a/arch/s390/pci/pci.c
> > > +++ b/arch/s390/pci/pci.c
> > > @@ -252,7 +252,7 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> > >  /* combine single writes by using store-block insn */
> > >  void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
> > >  {
> > > -       zpci_memcpy_toio(to, from, count);
> > > +	zpci_memcpy_toio(to, from, count * 8);
> > >  }
> > 
> > Odd, this bug is 11 years old, and there was never any bug report.
> > Is this never called, or how is this possible?
> > 
> > Niklas, would you happen to have an idea?
> 
> I did ask myself the same question when I first saw this change but
> still don't have a satisfactory answer.
> 
> As far as I can tell there is only one call that could be relevant for
> us in drivers/net/ethernet/mellanox/mlx4/en_tx.c:mlx4_bf_copy(). That
> call looks like it would be done during transmit.

I'm pretty sure a short write to even mlx4 will trigger the NIC to do
the slow path anyhow, so it would have been hidden. (assuming anyone
ever tested mlx4 on a kernel with this, it sure is old at this point)

Jason

Re: [PATCH rc] s390: Use the correct count for __iowrite64_copy()

[Heiko Carstens]

On Fri, Feb 16, 2024 at 08:48:14PM -0400, Jason Gunthorpe wrote:
> The signature for __iowrite64_copy() requires the number of 64 bit
> quantities, not bytes. Multiple by 8 to get to a byte length before
> invoking zpci_memcpy_toio()
> 
> Fixes: 87bc359b9822 ("s390/pci: speed up __iowrite64_copy by using pci store block insn")
> Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  arch/s390/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks!