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

[David Miller <davem@davemloft.net>]

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.