| Message ID | 20201026213040.3889546-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,01/11] atm: horizon: shut up clang null pointer arithmetic warning | expand |
> - for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem) > + for (mem = (HDW *) memmap; mem < (HDW *) ((uintptr_t)memmap + 1); ++mem) Note that these two lines are semantically different. In the first line, "+ 1" moves the pointer by (sizeof memmap) bytes. However in the second line, "+ 1" moves the pointer by only 1 byte. This driver is old, but let's still keep its code correct!
On Mon, Oct 26, 2020 at 8:56 PM Xie He <xie.he.0141@gmail.com> wrote: > > > - for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem) > > + for (mem = (HDW *) memmap; mem < (HDW *) ((uintptr_t)memmap + 1); ++mem) > > Note that these two lines are semantically different. In the first line, > "+ 1" moves the pointer by (sizeof memmap) bytes. However in the second > line, "+ 1" moves the pointer by only 1 byte. Correction: in the first line "+ 1" moves the pointer by (sizeof *memmap) bytes.
On Tue, Oct 27, 2020 at 5:02 AM Xie He <xie.he.0141@gmail.com> wrote: > > On Mon, Oct 26, 2020 at 8:56 PM Xie He <xie.he.0141@gmail.com> wrote: > > > > > - for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem) > > > + for (mem = (HDW *) memmap; mem < (HDW *) ((uintptr_t)memmap + 1); ++mem) > > > > Note that these two lines are semantically different. In the first line, > > "+ 1" moves the pointer by (sizeof memmap) bytes. However in the second > > line, "+ 1" moves the pointer by only 1 byte. > > Correction: in the first line "+ 1" moves the pointer by (sizeof *memmap) bytes. 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'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");
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!
On Mon, 26 Oct 2020 22:29:48 +0100 Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Building a "W=1" kernel with clang produces a warning about > suspicous pointer arithmetic: > > drivers/atm/horizon.c:1844:52: warning: performing pointer arithmetic > on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] > for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem) > > The way that the addresses are handled is very obscure, and > rewriting it to be more conventional seems fairly pointless, given > that this driver probably has no users. > Shut up this warning by adding a cast to uintptr_t. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Hi! I'm not sure what your plan is for re-spinning but when you do could you please split the wireless changes out? Also we never got patch 3 IDK if that's a coincidence or if it wasn't for networking...
On Wed, Oct 28, 2020 at 1:42 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 26 Oct 2020 22:29:48 +0100 Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > Building a "W=1" kernel with clang produces a warning about > > suspicous pointer arithmetic: > > > > drivers/atm/horizon.c:1844:52: warning: performing pointer arithmetic > > on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] > > for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem) > > > > The way that the addresses are handled is very obscure, and > > rewriting it to be more conventional seems fairly pointless, given > > that this driver probably has no users. > > Shut up this warning by adding a cast to uintptr_t. > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Hi! > > I'm not sure what your plan is for re-spinning but when you do could > you please split the wireless changes out? Sure, will do. The easiest for me would be if you just merge the ones that have been acked or that look obvious enough for you, and I'll then resend whatever is left after addressing the review comments. If that causes you extra work, I'll just send everything that should go through your tree. > Also we never got patch 3 > IDK if that's a coincidence or if it wasn't for networking... Yes, that one slipped in when I was sorting my longer series, it was a block driver change. Arnd
diff --git a/drivers/atm/horizon.c b/drivers/atm/horizon.c index 4f2951cbe69c..cd368786b216 100644 --- a/drivers/atm/horizon.c +++ b/drivers/atm/horizon.c @@ -1841,7 +1841,7 @@ static int hrz_init(hrz_dev *dev) printk (" clearing memory"); - for (mem = (HDW *) memmap; mem < (HDW *) (memmap + 1); ++mem) + for (mem = (HDW *) memmap; mem < (HDW *) ((uintptr_t)memmap + 1); ++mem) wr_mem (dev, mem, 0); printk (" tx channels");