| Message ID | 1556518178-13786-1-git-send-email-hofrat@osadl.org |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | rds: ib: force endiannes annotation | expand |
On 29/04/2019 07:09, Nicholas Mc Guire wrote: > diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c > index 7055985..a070a2d 100644 > --- a/net/rds/ib_recv.c > +++ b/net/rds/ib_recv.c > @@ -824,7 +824,7 @@ static void rds_ib_cong_recv(struct rds_connection *conn, > } > > /* the congestion map is in little endian order */ > - uncongested = le64_to_cpu(uncongested); > + uncongested = le64_to_cpu((__force __le64)uncongested); > > rds_cong_map_updated(map, uncongested); > } Again, a __force cast doesn't seem necessary here. It looks like the code is just using the wrong types; if all of src, dst and uncongested were __le64 instead of uint64_t, and the last two lines replaced with rds_cong_map_updated(map, le64_to_cpu(uncongested)); then the semantics would be kept with neither sparse errors nor __force. __force is almost never necessary and mostly just masks other bugs or endianness confusion in the surrounding code. Instead of adding a __force, either fix the code to be sparse-clean or leave the sparse warning in place so that future developers know there's something not right. -Ed
On Mon, Apr 29, 2019 at 12:00:06PM +0100, Edward Cree wrote: > On 29/04/2019 07:09, Nicholas Mc Guire wrote: > > diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c > > index 7055985..a070a2d 100644 > > --- a/net/rds/ib_recv.c > > +++ b/net/rds/ib_recv.c > > @@ -824,7 +824,7 @@ static void rds_ib_cong_recv(struct rds_connection *conn, > > } > > > > /* the congestion map is in little endian order */ > > - uncongested = le64_to_cpu(uncongested); > > + uncongested = le64_to_cpu((__force __le64)uncongested); > > > > rds_cong_map_updated(map, uncongested); > > } > Again, a __force cast doesn't seem necessary here. It looks like the > code is just using the wrong types; if all of src, dst and uncongested > were __le64 instead of uint64_t, and the last two lines replaced with > rds_cong_map_updated(map, le64_to_cpu(uncongested)); then the semantics > would be kept with neither sparse errors nor __force. > > __force is almost never necessary and mostly just masks other bugs or > endianness confusion in the surrounding code. Instead of adding a > __force, either fix the code to be sparse-clean or leave the sparse > warning in place so that future developers know there's something not > right. > changing uncongested to __le64 is not an option here - it would only move the sparse warnings to those other locatoins where the ports that became uncongested are being or'ed into uncongested. I'm not using __force as the prime way to silence sparse - I try to find an alternative first - the problem is in line 805 for (k = 0; k < to_copy; k += 8) { /* Record ports that became uncongested, ie * bits that changed from 0 to 1. */ uncongested |= ~(*src) & *dst; *dst++ = *src++; } And in this case the endianness handling does seem right. But ok with me to leave it in as it is - if you think that the __force here is not justified. thanks for your comments and notably the explainations ! thx! hofrat alternative
On 29/04/2019 12:18, Nicholas Mc Guire wrote: > On Mon, Apr 29, 2019 at 12:00:06PM +0100, Edward Cree wrote: >> Again, a __force cast doesn't seem necessary here. It looks like the >> code is just using the wrong types; if all of src, dst and uncongested >> were __le64 instead of uint64_t, and the last two lines replaced with >> rds_cong_map_updated(map, le64_to_cpu(uncongested)); then the semantics >> would be kept with neither sparse errors nor __force. >> >> __force is almost never necessary and mostly just masks other bugs or >> endianness confusion in the surrounding code. Instead of adding a >> __force, either fix the code to be sparse-clean or leave the sparse >> warning in place so that future developers know there's something not >> right. >> > changing uncongested to __le64 is not an option here - it would only move > the sparse warnings to those other locatoins where the ports that > became uncongested are being or'ed into uncongested. That's why I say to change *src and *dst too. Sparse won't mind the conversion from void * to __le64 * when they're assigned, and the only operations we do on them... > uncongested |= ~(*src) & *dst; > *dst++ = *src++; ... are some bitwise ops on the values (bitwise ops are legal in any endianness) and incrementation of the pointers (which cares only about the pointee size, not type). -Ed
On Mon, Apr 29, 2019 at 01:18:36PM +0200, Nicholas Mc Guire wrote: > changing uncongested to __le64 is not an option here - it would only move > the sparse warnings to those other locatoins where the ports that > became uncongested are being or'ed into uncongested. Than fix that a well. Either by throwing in a conversion, or add {be,le}XX_{and,or} helpers.
On Mon, Apr 29, 2019 at 01:02:31PM +0100, Edward Cree wrote: > ... are some bitwise ops on the values (bitwise ops are legal in any > endianness) and incrementation of the pointers (which cares only about > the pointee size, not type). Oh, true. That is why the underlying annotation is called __bitwise :) I'll take my previous comment back.
On Mon, Apr 29, 2019 at 05:21:32AM -0700, Christoph Hellwig wrote: > On Mon, Apr 29, 2019 at 01:18:36PM +0200, Nicholas Mc Guire wrote: > > changing uncongested to __le64 is not an option here - it would only move > > the sparse warnings to those other locatoins where the ports that > > became uncongested are being or'ed into uncongested. > > Than fix that a well. Either by throwing in a conversion, or > add {be,le}XX_{and,or} helpers. ok - that is an option in that case - will try that route thx! hofrat
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c index 7055985..a070a2d 100644 --- a/net/rds/ib_recv.c +++ b/net/rds/ib_recv.c @@ -824,7 +824,7 @@ static void rds_ib_cong_recv(struct rds_connection *conn, } /* the congestion map is in little endian order */ - uncongested = le64_to_cpu(uncongested); + uncongested = le64_to_cpu((__force __le64)uncongested); rds_cong_map_updated(map, uncongested); }
While the endiannes is being handled correctly as indicated by the comment above the offending line - sparse was unhappy with the missing annotation as be64_to_cpu() expects a __be64 argument. To mitigate this annotation issue forced annotation is introduced. Note that this patch has no impact on the generated binary. Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> --- Problem located by an experimental coccinelle script to locate patters that make sparse unhappy (false positives): net/rds/ib_recv.c:827:23: warning: cast to restricted __le64 Patch was compile-tested with: x86_64_defconfig + RDS_RDMA=m Patch was verified not to change the binary by diffing the generated object code before and after applying the patch. Patch is against 5.1-rc6 (localversion-next is 20190426) net/rds/ib_recv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)