Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201026213040.3889546-1-arnd@kernel.org> <20201027035558.16864-1-xie.he.0141@gmail.com>
 <CAJht_EPSs6W-r6kpWUNQDPzCjL-+_8mqq2JBoY=qhsQREgn92g@mail.gmail.com> <CAK8P3a3JTg5Mi2XC9AEC+YwH552M_TXDY4BaULZz5WmEb3woRQ@mail.gmail.com>
In-Reply-To: <CAK8P3a3JTg5Mi2XC9AEC+YwH552M_TXDY4BaULZz5WmEb3woRQ@mail.gmail.com>
From:   Xie He <xie.he.0141@gmail.com>
Date:   Tue, 27 Oct 2020 14:46:59 -0700
Message-ID: <CAJht_EPoXR9K3WoY5iNDNhzMgqtd=iS=mQsMQKHiGh7xRrYwHA@mail.gmail.com>
Subject: Re: [PATCH net-next 01/11] atm: horizon: shut up clang null pointer
 arithmetic warning
To:     Arnd Bergmann <arnd@kernel.org>
Cc:     Chas Williams <3chas3@gmail.com>,
        Nathan Chancellor <natechancellor@gmail.com>,
        Nick Desaulniers <ndesaulniers@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        linux-atm-general@lists.sourceforge.net,
        Linux Kernel Network Developers <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        clang-built-linux <clang-built-linux@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 27, 2020 at 6:24 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> Ah, of course. I had looked up the types but mixed up the memmap
> and HDW definitions, but then got confused trying to understand the
> logic in wr_mem() that operates on bytes but expands them into
> multiples of 4.

I think wr_mem() doesn't try to expand the address into multiples of
4. The address is multiplied by "sizeof(HDW)", which is 1. So the
address is not expanded.

I think this driver uses 0-based pointers not as byte-addresses to
access the host memory, but as (32-bit) word-addresses to access the
special hardware address space.

So using pointers in this case is confusing because it makes people
incorrectly consider they are used to access the host memory. It'd be
better that we just use integers.

> I've modified it as below now, will resend along with the other patches
> if you think this makes sense.
>
>         Arnd
>
> --- a/drivers/atm/horizon.c
> +++ b/drivers/atm/horizon.c
> @@ -1815,7 +1815,7 @@ static int hrz_init(hrz_dev *dev)
>
>    int buff_count;
>
> -  HDW * mem;
> +  uintptr_t offset;
>
>    cell_buf * tx_desc;
>    cell_buf * rx_desc;
> @@ -1841,8 +1841,8 @@ static int hrz_init(hrz_dev *dev)
>
>    printk (" clearing memory");
>
> -  for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem)
> -    wr_mem (dev, mem, 0);
> +  for (offset = 0; offset < sizeof(struct MEMMAP); offset++)
> +    wr_mem (dev, (HDW *)offset, 0);
>
>    printk (" tx channels");

This looks good to me. Thanks!