[PATCH net] rds: Fix endian annotations across various assignments

[PATCH net] rds: Fix endian annotations across various assignments

[Ujwal Kundur]

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.2

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Andrew Lunn]

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

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Al Viro]

On 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,

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Al Viro]

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"...

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Al Viro]

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.

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Al Viro]

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.

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Andrew Lunn]

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

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Al Viro]

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.

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Ujwal Kundur]

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

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Andrew Lunn]

> > 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

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Al Viro]

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...

Re: [PATCH net] rds: Fix endian annotations across various assignments

[Al Viro]

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