net/rds/af_rds.c | 2 +- net/rds/connection.c | 6 +++--- net/rds/rdma.c | 1 - net/rds/rds.h | 2 +- net/rds/recv.c | 4 ++-- net/rds/send.c | 4 ++-- 6 files changed, 9 insertions(+), 10 deletions(-)
Sparse reports the following warnings:
net/rds/af_rds.c:245:22: warning: invalid assignment: |=
net/rds/af_rds.c:245:22: left side has type restricted __poll_t
net/rds/af_rds.c:245:22: right side has type int
__poll_t is typedef'ed to __bitwise while POLLERR is defined as 0x0008,
force conversion.
net/rds/recv.c:218:42: warning: cast to restricted __be16
net/rds/recv.c:222:44: warning: cast to restricted __be32
Replace be{16,32}_to_cpu with explicit __force to force conversion. The
value remains the same either way so not a bug.
net/rds/send.c:1050:24: warning: incorrect type in argument 1 (different base types)
net/rds/send.c:1050:24: expected unsigned int [usertype] a
net/rds/send.c:1050:24: got restricted __be16 [usertype] sin6_port
net/rds/send.c:1052:24: warning: incorrect type in argument 1 (different base types)
net/rds/send.c:1052:24: expected unsigned int [usertype] a
net/rds/send.c:1052:24: got restricted __be16 [usertype] sin6_port
net/rds/send.c:1457:30: warning: incorrect type in initializer (different base types)
net/rds/send.c:1457:30: expected unsigned short [usertype] npaths
net/rds/send.c:1457:30: got restricted __be16 [usertype]
net/rds/send.c:1458:34: warning: incorrect type in initializer (different base types)
net/rds/send.c:1458:34: expected unsigned int [usertype] my_gen_num
net/rds/send.c:1458:34: got restricted __be32 [usertype]
Use correct endianness. Replace cpu_to_be{16,32} for the same reason as
above.
net/rds/connection.c:71:31: warning: incorrect type in argument 1 (different base types)
net/rds/connection.c:71:31: expected restricted __be32 const [usertype] laddr
net/rds/connection.c:71:31: got unsigned int [assigned] [usertype] lhash
net/rds/connection.c:71:41: warning: incorrect type in argument 3 (different base types)
net/rds/connection.c:71:41: expected restricted __be32 const [usertype] faddr
net/rds/connection.c:71:41: got unsigned int [assigned] [usertype] fhash
Signed-off-by: Ujwal Kundur <ujwal.kundur@gmail.com>
---
net/rds/af_rds.c | 2 +-
net/rds/connection.c | 6 +++---
net/rds/rdma.c | 1 -
net/rds/rds.h | 2 +-
net/rds/recv.c | 4 ++--
net/rds/send.c | 4 ++--
6 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index 086a13170e09..9cd5905d916a 100644
--- a/net/rds/af_rds.c
+++ b/net/rds/af_rds.c
@@ -242,7 +242,7 @@ static __poll_t rds_poll(struct file *file, struct socket *sock,
if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))
mask |= (EPOLLOUT | EPOLLWRNORM);
if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
- mask |= POLLERR;
+ mask |= (__force __poll_t)POLLERR;
read_unlock_irqrestore(&rs->rs_recv_lock, flags);
/* clear state any time we wake a seen-congested socket */
diff --git a/net/rds/connection.c b/net/rds/connection.c
index d62f486ab29f..0047b76c3c0b 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -62,13 +62,13 @@ static struct hlist_head *rds_conn_bucket(const struct in6_addr *laddr,
net_get_random_once(&rds_hash_secret, sizeof(rds_hash_secret));
net_get_random_once(&rds6_hash_secret, sizeof(rds6_hash_secret));
- lhash = (__force u32)laddr->s6_addr32[3];
+ lhash = (__force __u32)laddr->s6_addr32[3];
#if IS_ENABLED(CONFIG_IPV6)
fhash = __ipv6_addr_jhash(faddr, rds6_hash_secret);
#else
- fhash = (__force u32)faddr->s6_addr32[3];
+ fhash = (__force __u32)(faddr->s6_addr32[3]);
#endif
- hash = __inet_ehashfn(lhash, 0, fhash, 0, rds_hash_secret);
+ hash = __inet_ehashfn((__force __be32)lhash, 0, (__force __be32)fhash, 0, rds_hash_secret);
return &rds_conn_hash[hash & RDS_CONNECTION_HASH_MASK];
}
diff --git a/net/rds/rdma.c b/net/rds/rdma.c
index 00dbcd4d28e6..f9bcec8f745a 100644
--- a/net/rds/rdma.c
+++ b/net/rds/rdma.c
@@ -39,7 +39,6 @@
/*
* XXX
- * - build with sparse
* - should we detect duplicate keys on a socket? hmm.
* - an rdma is an mlock, apply rlimit?
*/
diff --git a/net/rds/rds.h b/net/rds/rds.h
index dc360252c515..c2fb47e1fe07 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -93,7 +93,7 @@ enum {
/* Max number of multipaths per RDS connection. Must be a power of 2 */
#define RDS_MPATH_WORKERS 8
-#define RDS_MPATH_HASH(rs, n) (jhash_1word((rs)->rs_bound_port, \
+#define RDS_MPATH_HASH(rs, n) (jhash_1word((__force __u16)(rs)->rs_bound_port, \
(rs)->rs_hash_initval) & ((n) - 1))
#define IS_CANONICAL(laddr, faddr) (htonl(laddr) < htonl(faddr))
diff --git a/net/rds/recv.c b/net/rds/recv.c
index 5627f80013f8..7fc7a3850a7b 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -216,10 +216,10 @@ static void rds_recv_hs_exthdrs(struct rds_header *hdr,
switch (type) {
case RDS_EXTHDR_NPATHS:
conn->c_npaths = min_t(int, RDS_MPATH_WORKERS,
- be16_to_cpu(buffer.rds_npaths));
+ (__force __u16)buffer.rds_npaths);
break;
case RDS_EXTHDR_GEN_NUM:
- new_peer_gen_num = be32_to_cpu(buffer.rds_gen_num);
+ new_peer_gen_num = (__force __u32)buffer.rds_gen_num;
break;
default:
pr_warn_ratelimited("ignoring unknown exthdr type "
diff --git a/net/rds/send.c b/net/rds/send.c
index 42d991bc8543..0d79455c9721 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -1454,8 +1454,8 @@ rds_send_probe(struct rds_conn_path *cp, __be16 sport,
if (RDS_HS_PROBE(be16_to_cpu(sport), be16_to_cpu(dport)) &&
cp->cp_conn->c_trans->t_mp_capable) {
- u16 npaths = cpu_to_be16(RDS_MPATH_WORKERS);
- u32 my_gen_num = cpu_to_be32(cp->cp_conn->c_my_gen_num);
+ u16 npaths = (__force __u16)RDS_MPATH_WORKERS;
+ u32 my_gen_num = (__force __u32)cp->cp_conn->c_my_gen_num;
rds_message_add_extension(&rm->m_inc.i_hdr,
RDS_EXTHDR_NPATHS, &npaths,
--
2.30.2On Sun, Aug 10, 2025 at 10:41:55PM +0530, Ujwal Kundur wrote:
> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index 086a13170e09..9cd5905d916a 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c
> @@ -242,7 +242,7 @@ static __poll_t rds_poll(struct file *file, struct socket *sock,
> if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))
> mask |= (EPOLLOUT | EPOLLWRNORM);
> if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
> - mask |= POLLERR;
> + mask |= (__force __poll_t)POLLERR;
EPOLLERR.
> read_unlock_irqrestore(&rs->rs_recv_lock, flags);
>
> /* clear state any time we wake a seen-congested socket */
> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index d62f486ab29f..0047b76c3c0b 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -62,13 +62,13 @@ static struct hlist_head *rds_conn_bucket(const struct in6_addr *laddr,
> net_get_random_once(&rds_hash_secret, sizeof(rds_hash_secret));
> net_get_random_once(&rds6_hash_secret, sizeof(rds6_hash_secret));
>
> - lhash = (__force u32)laddr->s6_addr32[3];
> + lhash = (__force __u32)laddr->s6_addr32[3];
> #if IS_ENABLED(CONFIG_IPV6)
> fhash = __ipv6_addr_jhash(faddr, rds6_hash_secret);
> #else
> - fhash = (__force u32)faddr->s6_addr32[3];
> + fhash = (__force __u32)(faddr->s6_addr32[3]);
> #endif
> - hash = __inet_ehashfn(lhash, 0, fhash, 0, rds_hash_secret);
> + hash = __inet_ehashfn((__force __be32)lhash, 0, (__force __be32)fhash, 0, rds_hash_secret);
what the... You have lhash and fhash set to __be32 values, then
feed them to function that expects __be32 argument. Just turn
these two variables into __be32 and lose the pointless casts.
> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index dc360252c515..c2fb47e1fe07 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h
> @@ -93,7 +93,7 @@ enum {
>
> /* Max number of multipaths per RDS connection. Must be a power of 2 */
> #define RDS_MPATH_WORKERS 8
> -#define RDS_MPATH_HASH(rs, n) (jhash_1word((rs)->rs_bound_port, \
> +#define RDS_MPATH_HASH(rs, n) (jhash_1word((__force __u16)(rs)->rs_bound_port, \
> (rs)->rs_hash_initval) & ((n) - 1))
Reasonable.
> #define IS_CANONICAL(laddr, faddr) (htonl(laddr) < htonl(faddr))
> diff --git a/net/rds/recv.c b/net/rds/recv.c
> index 5627f80013f8..7fc7a3850a7b 100644
> --- a/net/rds/recv.c
> +++ b/net/rds/recv.c
> @@ -216,10 +216,10 @@ static void rds_recv_hs_exthdrs(struct rds_header *hdr,
> switch (type) {
> case RDS_EXTHDR_NPATHS:
> conn->c_npaths = min_t(int, RDS_MPATH_WORKERS,
> - be16_to_cpu(buffer.rds_npaths));
> + (__force __u16)buffer.rds_npaths);
No. It will break on little-endian. That's over-the-wire data you are
dealing with; it's *NOT* going to be host-endian. Fix the buggered
annotations instead.
> break;
> case RDS_EXTHDR_GEN_NUM:
> - new_peer_gen_num = be32_to_cpu(buffer.rds_gen_num);
> + new_peer_gen_num = (__force __u32)buffer.rds_gen_num;
> break;
Ditto.
> diff --git a/net/rds/send.c b/net/rds/send.c
> index 42d991bc8543..0d79455c9721 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -1454,8 +1454,8 @@ rds_send_probe(struct rds_conn_path *cp, __be16 sport,
>
> if (RDS_HS_PROBE(be16_to_cpu(sport), be16_to_cpu(dport)) &&
> cp->cp_conn->c_trans->t_mp_capable) {
> - u16 npaths = cpu_to_be16(RDS_MPATH_WORKERS);
> - u32 my_gen_num = cpu_to_be32(cp->cp_conn->c_my_gen_num);
> + u16 npaths = (__force __u16)RDS_MPATH_WORKERS;
> + u32 my_gen_num = (__force __u32)cp->cp_conn->c_my_gen_num;
Again, over-the-wire data; you are breaking it on l-e.
> rds_message_add_extension(&rm->m_inc.i_hdr,
> RDS_EXTHDR_NPATHS, &npaths,
On Sun, Aug 10, 2025 at 06:47:05PM +0100, Al Viro wrote: > > diff --git a/net/rds/connection.c b/net/rds/connection.c > > index d62f486ab29f..0047b76c3c0b 100644 > > --- a/net/rds/connection.c > > +++ b/net/rds/connection.c > > @@ -62,13 +62,13 @@ static struct hlist_head *rds_conn_bucket(const struct in6_addr *laddr, > > net_get_random_once(&rds_hash_secret, sizeof(rds_hash_secret)); > > net_get_random_once(&rds6_hash_secret, sizeof(rds6_hash_secret)); > > > > - lhash = (__force u32)laddr->s6_addr32[3]; > > + lhash = (__force __u32)laddr->s6_addr32[3]; > > #if IS_ENABLED(CONFIG_IPV6) > > fhash = __ipv6_addr_jhash(faddr, rds6_hash_secret); > > #else > > - fhash = (__force u32)faddr->s6_addr32[3]; > > + fhash = (__force __u32)(faddr->s6_addr32[3]); > > #endif > > - hash = __inet_ehashfn(lhash, 0, fhash, 0, rds_hash_secret); > > + hash = __inet_ehashfn((__force __be32)lhash, 0, (__force __be32)fhash, 0, rds_hash_secret); > > what the... You have lhash and fhash set to __be32 values, then > feed them to function that expects __be32 argument. Just turn > these two variables into __be32 and lose the pointless casts. FWIW, here you do have a missing cast - fhash = __ipv6_addr_jhash(faddr, rds6_hash_secret); should be fhash = (__force __be32)__ipv6_addr_jhash(faddr, rds6_hash_secret); With both fhash and lhash declared as __be32.
On Sun, Aug 10, 2025 at 06:47:05PM +0100, Al Viro wrote:
> > switch (type) {
> > case RDS_EXTHDR_NPATHS:
> > conn->c_npaths = min_t(int, RDS_MPATH_WORKERS,
> > - be16_to_cpu(buffer.rds_npaths));
> > + (__force __u16)buffer.rds_npaths);
>
> No. It will break on little-endian. That's over-the-wire data you are
> dealing with; it's *NOT* going to be host-endian. Fix the buggered
> annotations instead.
PS: be16_to_cpu() is not the same thing as a cast. On a big-endian box,
a 16bit number 1000 (0x3e8) is stored as {3, 0xe8}; on a little-endian it's
{3, 0xe8} instead; {0xe8, 3} means 59395 there (0xe8 * 256 + 3).
be16_to_cpu() is a no-op on big-endian; on little-endian it converts the
big-endian 16bit to host-endian (internally it's a byteswap).
If over-the-wire data is stored in big-endian order (bits 8..15 in the
first byte, bits 0..7 in the second one) and you want to do any kind
of arithmetics with the value you've received, you can't blindly treat
it as u16 (unsigned short) field of structure - on big-endian boxen
it would work, but on little-endian it would give the wrong values
(59395 when sender had meant 1000). be16_to_cpu() adjusts for that.
In the above, you have ->c_npaths definitely used as a number - this
net/rds/connection.c:924: } while (++i < conn->c_npaths);
alone is enough to be convincing. All other uses are of the same
sort - it's used in comparisons, so places using it are expecting
host-endian integer.
Assignment you've modified sets it to lesser of RDS_MPATH_WORKERS (8,
apparently) and the value of buffer.rds_npaths decoded as big-endian.
"buffer" is declared as
union {
struct rds_ext_header_version version;
u16 rds_npaths;
u32 rds_gen_num;
} buffer;
and the value in it comes from
type = rds_message_next_extension(hdr, &pos, &buffer, &len);
Then we look at the return value of that rds_message_next_extension() thing
and if it's RDS_EXTHDR_NPATHS we hit that branch.
That smells like parsing the incoming package, doesn't it? A look at
rds_message_next_extension() seems to confirm that - we fetch the next
byte (that's our return value), then pick the corresponding size from
rds_exthdr_size[that_byte] and copy that many following bytes.
That code clearly expects the data to be stored in big-endian order -
it is not entirely impossible that somehow it gets host-endian (in
which case be16_to_cpu() would be a bug), but that's very unlikely.
*IF* that's over-the-wire data, the code is actually correct and the
problem is with wrong annotations -
union {
struct rds_ext_header_version version;
__be16 rds_npaths;
__be32 rds_gen_num;
} buffer;
to reflect the actual data layout to be found in there. Probably
static unsigned int rds_exthdr_size[__RDS_EXTHDR_MAX] = {
[RDS_EXTHDR_NONE] = 0,
[RDS_EXTHDR_VERSION] = sizeof(struct rds_ext_header_version),
[RDS_EXTHDR_RDMA] = sizeof(struct rds_ext_header_rdma),
[RDS_EXTHDR_RDMA_DEST] = sizeof(struct rds_ext_header_rdma_dest),
[RDS_EXTHDR_NPATHS] = sizeof(__be16),
[RDS_EXTHDR_GEN_NUM] = sizeof(__be32),
};
for consistency sake. Note that e.g. struct rds_ext_header_rdma_dest
is
struct rds_ext_header_rdma_dest {
__be32 h_rdma_rkey;
__be32 h_rdma_offset;
};
which also points towards "protocol data, fixed-endian"...
On Sun, Aug 10, 2025 at 07:25:06PM +0100, Al Viro wrote:
> On Sun, Aug 10, 2025 at 06:47:05PM +0100, Al Viro wrote:
>
> > > switch (type) {
> > > case RDS_EXTHDR_NPATHS:
> > > conn->c_npaths = min_t(int, RDS_MPATH_WORKERS,
> > > - be16_to_cpu(buffer.rds_npaths));
> > > + (__force __u16)buffer.rds_npaths);
> >
> > No. It will break on little-endian. That's over-the-wire data you are
> > dealing with; it's *NOT* going to be host-endian. Fix the buggered
> > annotations instead.
>
> PS: be16_to_cpu() is not the same thing as a cast. On a big-endian box,
> a 16bit number 1000 (0x3e8) is stored as {3, 0xe8}; on a little-endian it's
> {3, 0xe8} instead; {0xe8, 3} means 59395 there (0xe8 * 256 + 3).
Hi Al
This smells of an LLM generated patch. So i think you are somewhat
wasting your time explaining in detail why this is wrong. Well, maybe
in a few generations of LLM it might learn from what you said, but
that does not address the immediate problem.
We need developers using LLM to accept they have often wrong, and you
need to spend time and effort:
1) Proving it got is wrong.
2) That after a lot of effort, failing to prove it wrong, accept it might be right.
3) Proving it actually got it right.
It took me about 60 seconds to prove the POLLERR change was wrong, and
i know nothing about this code base. So it is in fact not a lot of
effort.
Andrew
Thanks a lot for the explanation Al! I was apprehensive about breaking things and in hindsight, should've understood why the cast was present rather than accepting sparse's report as the whole truth; Will go through the code more thoroughly and send a v2 patchset. > This smells of an LLM generated patch. So i think you are somewhat > wasting your time explaining in detail why this is wrong. I have never used (and will not use) LLMs :( I intend to learn more about the networking stack through contributions and I __strongly__ believe using LLMs / AI won't help me get there. > It took me about 60 seconds to prove the POLLERR change was wrong, and > i know nothing about this code base. So it is in fact not a lot of > effort. I looked up the definition of POLLERR on Elixir [1] and it seemed like a valid Sparse report to me. I wasn't aware of EPOLLERR, and now realize all the other operations are prefixed with EPOLL* in af_rds.c. I look forward to reviews/critiques to learn from them but being accused of using LLMs is kinda disheartening. P.S: I'm still learning the ropes as a contributor so please pardon my ignorance. [1] - https://elixir.bootlin.com/linux/v6.16/source/include/uapi/asm-generic/poll.h#L9
On Mon, Aug 11, 2025 at 01:01:01AM +0530, Ujwal Kundur wrote: > > It took me about 60 seconds to prove the POLLERR change was wrong, and > > i know nothing about this code base. So it is in fact not a lot of > > effort. > I looked up the definition of POLLERR on Elixir [1] and it seemed like > a valid Sparse report to me. I wasn't aware of EPOLLERR, and now > realize all the other operations are prefixed with EPOLL* in af_rds.c. > I look forward to reviews/critiques to learn from them but being > accused of using LLMs is kinda disheartening. As for the POLLERR part of that, the thing about POLL* constants is that beyond the first 6 (IN/PRI/OUT/ERR/HUP/NVAL) they are arch-dependent, and not just in a sense of bit assignments. generic: IN PRI OUT ERR HUP NVAL RDNORM RDBAND WRNORM WRBAND MSG REMOVE RDHUP 0 1 2 3 4 5 6 7 8 9 10 11 12 sparc: 0 1 2 3 4 5 6 7 =OUT 8 9 10 11 mips,m68k: 0 1 2 3 4 5 6 7 =OUT 8 10 11 12 xtensa: 0 1 2 3 4 5 6 7 =OUT 8 10 13 12 So these get mapped from/to by poll(2) (mangle_poll() and demangle_poll() resp.) and __poll_t serves as a tool for catching the places that might be confused. The internal values (also used by eventpoll(2)) are 0 1 2 3 4 5 6 7 8 9 10 --- 12 POLLREMOVE is Solaris-only thing and we do not even attempt to implement it. So's POLLMSG, but there's a bit of a twist - nobody ever returns that shit from ->poll(), but SIGIO from dnotify and from lease breaking comes with ->si_band in siginfo set to POLLIN|POLLRDNORM|POLLMSG; Warning about __poll_t is usually "mixing POLL... and EPOLL... is a bad idea". Here it's not a bug (note that ERR gets the same bit in all of the above), but it's trivial to annotate properly...
On Sun, Aug 10, 2025 at 10:00:58PM +0100, Al Viro wrote: > On Mon, Aug 11, 2025 at 01:01:01AM +0530, Ujwal Kundur wrote: > > > > It took me about 60 seconds to prove the POLLERR change was wrong, and > > > i know nothing about this code base. So it is in fact not a lot of > > > effort. > > I looked up the definition of POLLERR on Elixir [1] and it seemed like > > a valid Sparse report to me. I wasn't aware of EPOLLERR, and now > > realize all the other operations are prefixed with EPOLL* in af_rds.c. > > I look forward to reviews/critiques to learn from them but being > > accused of using LLMs is kinda disheartening. > > As for the POLLERR part of that, the thing about POLL* constants is that > beyond the first 6 (IN/PRI/OUT/ERR/HUP/NVAL) they are arch-dependent, > and not just in a sense of bit assignments. > generic: > IN PRI OUT ERR HUP NVAL RDNORM RDBAND WRNORM WRBAND MSG REMOVE RDHUP > 0 1 2 3 4 5 6 7 8 9 10 11 12 > sparc: > 0 1 2 3 4 5 6 7 =OUT 8 9 10 11 > mips,m68k: > 0 1 2 3 4 5 6 7 =OUT 8 10 11 13 > xtensa: > 0 1 2 3 4 5 6 7 =OUT 8 10 11 12 Ugh... My apologies - messed table above in the last two columns. REMOVE is 10 for sparc, 11 for xtensa and 12 for everybody else. RDHUP is 11 for sparc and 13 for everybody else. generic: IN PRI OUT ERR HUP NVAL RDNORM RDBAND WRNORM WRBAND MSG REMOVE RDHUP 0 1 2 3 4 5 6 7 8 9 10 12 13 sparc: 0 1 2 3 4 5 6 7 =OUT 8 9 10 11 mips,m68k: 0 1 2 3 4 5 6 7 =OUT 8 10 12 13 xtensa: 0 1 2 3 4 5 6 7 =OUT 8 10 11 13
> > This smells of an LLM generated patch. So i think you are somewhat > > wasting your time explaining in detail why this is wrong. > I have never used (and will not use) LLMs :( Sorry, I need to refine my sense of smell. > P.S: I'm still learning the ropes as a contributor so please pardon my > ignorance. You might find this interesting: https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html You made a few process errors as well. Since you submitted this to net, it needs a Fixes: tag. Also: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html Particularly: It must either fix a real bug that bothers people Sparse warnings don't really both people. But is the sparse warning indicating a read bug which does bother people? If so, net is O.K, but please add to the commit message what the real bug is. If this is simply cleanup, not a bug fix, please submit to net-next. You can also learn a lot by subscribing to the netdev mailing list, and reading other peoples patches, and review comments they get. Andrew
On Sun, Aug 10, 2025 at 08:41:49PM +0200, Andrew Lunn wrote: > This smells of an LLM generated patch. Maybe, maybe not. > So i think you are somewhat > wasting your time explaining in detail why this is wrong. Well, maybe > in a few generations of LLM it might learn from what you said, but > that does not address the immediate problem. You do realize that there _are_ humans out there, right? Ones capable of learning, that is... > We need developers using LLM to accept they have often wrong, and you > need to spend time and effort: > > 1) Proving it got is wrong. > 2) That after a lot of effort, failing to prove it wrong, accept it might be right. > 3) Proving it actually got it right. > > It took me about 60 seconds to prove the POLLERR change was wrong, and > i know nothing about this code base. So it is in fact not a lot of > effort.
On Sun, Aug 10, 2025 at 07:25:06PM +0100, Al Viro wrote:
> On Sun, Aug 10, 2025 at 06:47:05PM +0100, Al Viro wrote:
>
> > > switch (type) {
> > > case RDS_EXTHDR_NPATHS:
> > > conn->c_npaths = min_t(int, RDS_MPATH_WORKERS,
> > > - be16_to_cpu(buffer.rds_npaths));
> > > + (__force __u16)buffer.rds_npaths);
> >
> > No. It will break on little-endian. That's over-the-wire data you are
> > dealing with; it's *NOT* going to be host-endian. Fix the buggered
> > annotations instead.
>
> PS: be16_to_cpu() is not the same thing as a cast. On a big-endian box,
> a 16bit number 1000 (0x3e8) is stored as {3, 0xe8}; on a little-endian it's
> {3, 0xe8} instead; {0xe8, 3} means 59395 there (0xe8 * 256 + 3).
^^^^^^^^ ^^^^^^^^^
{0xe8, 3} {3, 0xe8}
Sorry, buggered editing.
On Sun, Aug 10, 2025 at 10:41:55PM +0530, Ujwal Kundur wrote:
> Sparse reports the following warnings:
>
> net/rds/af_rds.c:245:22: warning: invalid assignment: |=
> net/rds/af_rds.c:245:22: left side has type restricted __poll_t
> net/rds/af_rds.c:245:22: right side has type int
>
> __poll_t is typedef'ed to __bitwise while POLLERR is defined as 0x0008,
> force conversion.
Please could you split this up, one patch per type of problem.
> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index 086a13170e09..9cd5905d916a 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c
> @@ -242,7 +242,7 @@ static __poll_t rds_poll(struct file *file, struct socket *sock,
> if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))
> mask |= (EPOLLOUT | EPOLLWRNORM);
> if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
> - mask |= POLLERR;
> + mask |= (__force __poll_t)POLLERR;
I don't like __force, it suggests something is wrong with the
design. If it is needed, it should be hidden away.
However:
~/linux/net$ grep -r POLLERR
caif/caif_socket.c: wake_up_interruptible_poll(sk_sleep(sk), EPOLLERR|EPOLLHUP);
caif/caif_socket.c: mask |= EPOLLERR;
rds/af_rds.c: mask |= POLLERR;
bluetooth/af_bluetooth.c: mask |= EPOLLERR |
sctp/socket.c: mask |= EPOLLERR |
vmw_vsock/af_vsock.c: mask |= EPOLLERR;
vmw_vsock/af_vsock.c: mask |= EPOLLERR;
vmw_vsock/af_vsock.c: mask |= EPOLLERR;
9p/trans_fd.c: return EPOLLERR;
9p/trans_fd.c: if (n & (EPOLLERR | EPOLLHUP | EPOLLNVAL)) {
mptcp/protocol.c: mask |= EPOLLERR;
core/datagram.c: if (key && !(key_to_poll(key) & (EPOLLIN | EPOLLERR)))
core/datagram.c: mask |= EPOLLERR |
core/sock.c: wake_up_interruptible_poll(&wq->wait, EPOLLERR);
nfc/llcp_sock.c: mask |= EPOLLERR |
smc/af_smc.c: else if (flags & EPOLLERR)
smc/af_smc.c: mask |= EPOLLERR;
phonet/socket.c: return EPOLLERR;
iucv/af_iucv.c: mask |= EPOLLERR |
unix/af_unix.c: mask |= EPOLLERR;
unix/af_unix.c: mask |= EPOLLERR |
ipv4/tcp.c: mask |= EPOLLERR;
sunrpc/rpc_pipe.c: mask |= EPOLLERR | EPOLLHUP;
atm/common.c: mask = EPOLLERR;
So why is af_rds.c special? Or do all these files also give the same
warning?
Also:
https://elixir.bootlin.com/linux/v6.16/source/include/uapi/linux/eventpoll.h#L34
#define EPOLLERR (__force __poll_t)0x00000008
So your patch does nothing.
Andrew
---
pw-bot: cr
© 2016 - 2025 Red Hat, Inc.