[net-next,V1,09/10] net/mlx5: Fix global UAR mapping

From: Moshe Lazer <moshel@mellanox.com>

Remove the global WC mapping of the total UARs since
UAR mapping should be decided per UAR (e.g we want
different mappings for EQs, CQs vs QPs).

We use ARCH_HAS_IOREMAP_WC to know if the current arch supports WC
(Write combining) IO memory mapping, if it is not supported
"uar->bf_map" will be NULL, thus we will use NC (Non Cached) mapping
"uar->map".

Fixes: 88a85f99e51f ('TX latency optimization to save DMA reads')
Signed-off-by: Moshe Lazer <moshel@mellanox.com>
Reviewed-by: Achiad Shochat <achiad@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/main.c |   28 +-----------------------
 drivers/net/ethernet/mellanox/mlx5/core/uar.c  |   12 +++++-----
 include/linux/mlx5/driver.h                    |    2 -
 3 files changed, 7 insertions(+), 35 deletions(-)

From: Saeed Mahameed <saeedm@mellanox.com>
Date: Sun, 28 Feb 2016 17:09:10 +0200

> We use ARCH_HAS_IOREMAP_WC to know if the current arch supports WC
> (Write combining) IO memory mapping, if it is not supported
> "uar->bf_map" will be NULL, thus we will use NC (Non Cached) mapping
> "uar->map".

This description sucks.

You're just saying what will happen if the CPP is defined or not
(uar->bf_map ends up being NULL).

Well anyone can see that from the code.

You have to explain why.

And BTW, ARCH_HAS_IOREMAP_WC doesn't even tell you if the platform
will actually give you a write-combining mapping.

So if it's the driver operates properly if a non-WC mapping is used
for uar->bf_map, then get rid of this CPP test altogether PLEASE!

Otherwise your driver is buggy, because ARCH_HAS_IOREMAP_WC only says
whether the default implementation of ioremap_wc() needs to be
provided by include/asm-generic/iomap.h It does not guarantee that a
write-combining mapping will be provided.

I really can't think of any reason why you absolutely require a
WC mapping, and the CPP test just makes your driver look more
ugly than it needs to me.

So can you please explain what the hell is happening here and why you
are doing things this way rather than just reading the code to me?

Thanks.

>
> Well anyone can see that from the code.
>
> You have to explain why.

In a simple words as partially explained in the commit message we want
to have both mappings (NC and WC) available so upper layer can decide
which to choose e.g. for SQs/QPs in some cases (Small Packets) and
only when WC is supported we would like to write TX descriptors (WQEs)
using ConnectX BlueFlame feature via WC mapping and if WC is not
supported the TX descriptors would be posted in the usual way
(doorbell) via NC mapping.
this would give a latency boost for small packets.

The problem is when posting BlueFlame buffers when the mapping is not
WC i.e via NC mapping the latency will get worst than writing using
the usual way (doorbell).

so this is why we use ARCH_HAS_IOREMAP_WC to give a hint to upper
layer whether to use BlueFlame writes (WC) or doorbell writes (NC).

>
> And BTW, ARCH_HAS_IOREMAP_WC doesn't even tell you if the platform
> will actually give you a write-combining mapping.

We did some research after your comment and we are considering
removing ARCH_HAS_IOREMAP_WC from the code, we will update the patches
soon.

>
> So if it's the driver operates properly if a non-WC mapping is used
> for uar->bf_map, then get rid of this CPP test altogether PLEASE!
>
> Otherwise your driver is buggy, because ARCH_HAS_IOREMAP_WC only says
> whether the default implementation of ioremap_wc() needs to be
> provided by include/asm-generic/iomap.h It does not guarantee that a
> write-combining mapping will be provided.
>
> I really can't think of any reason why you absolutely require a
> WC mapping, and the CPP test just makes your driver look more
> ugly than it needs to me.

WC mapping is required in order to know if BlueFlame writes would give
a better latency or not.

>
> So can you please explain what the hell is happening here and why you
> are doing things this way rather than just reading the code to me?

I hope the above explains what we are trying to do here, I know it is
not perfect, but as you know the kernel IO mapping API doesn't tell if
the WC mapping was successful or not, so we used the CPP test.
but after your comment we understood it is not perfect, and we are
looking into it.

Thanks