[PATCH] wifi：mac80211: Replace the ternary conditional operator with max()

[PATCH] wifi：mac80211: Replace the ternary conditional operator with max()

[You Kangren]

Replace the ternary conditional operator with max() to make the code clean

Signed-off-by: You Kangren <youkangren@vivo.com>
---
 net/mac80211/tdls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c
index a4af3b7675ef..9f8b0842a616 100644
--- a/net/mac80211/tdls.c
+++ b/net/mac80211/tdls.c
@@ -946,7 +946,7 @@ ieee80211_tdls_build_mgmt_packet_data(struct ieee80211_sub_if_data *sdata,
 	int ret;
 	struct ieee80211_link_data *link;
 
-	link_id = link_id >= 0 ? link_id : 0;
+	link_id = max(link_id, 0);
 	rcu_read_lock();
 	link = rcu_dereference(sdata->link[link_id]);
 	if (WARN_ON(!link))
-- 
2.39.0

RE: [PATCH] wifi：mac80211: Replace the ternary conditional operator with max()

[Ping-Ke Shih]

> -----Original Message-----
> From: You Kangren <youkangren@vivo.com>
> Sent: Monday, June 26, 2023 6:48 PM
> To: Johannes Berg <johannes@sipsolutions.net>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; open
> list:MAC80211 <linux-wireless@vger.kernel.org>; open list:NETWORKING [GENERAL] <netdev@vger.kernel.org>;
> open list <linux-kernel@vger.kernel.org>
> Cc: opensource.kernel@vivo.com; youkangren@vivo.com
> Subject: [PATCH] wifi：mac80211: Replace the ternary conditional operator with max()

The semicolon of "wifi：" is different from others.

> 
> Replace the ternary conditional operator with max() to make the code clean
> 
> Signed-off-by: You Kangren <youkangren@vivo.com>
> ---
>  net/mac80211/tdls.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c
> index a4af3b7675ef..9f8b0842a616 100644
> --- a/net/mac80211/tdls.c
> +++ b/net/mac80211/tdls.c
> @@ -946,7 +946,7 @@ ieee80211_tdls_build_mgmt_packet_data(struct ieee80211_sub_if_data *sdata,
>         int ret;
>         struct ieee80211_link_data *link;
> 
> -       link_id = link_id >= 0 ? link_id : 0;
> +       link_id = max(link_id, 0);

Original logic means "if link_id < 0, then use default link (0)" instead of
"always use link_id larger than or equal to 0". So, I think max(link_id, 0) could
cause misunderstanding. 

>         rcu_read_lock();
>         link = rcu_dereference(sdata->link[link_id]);
>         if (WARN_ON(!link))
> --
> 2.39.0
> 
> 
> ------Please consider the environment before printing this e-mail.

Re: [PATCH] wifi：mac80211: Replace the ternary conditional operator with max()

[Nicolas Escande]

On Wed Jun 28, 2023 at 3:48 AM CEST, Ping-Ke Shih wrote:
>
>
> > -----Original Message-----
> > From: You Kangren <youkangren@vivo.com>
> > Sent: Monday, June 26, 2023 6:48 PM
> > To: Johannes Berg <johannes@sipsolutions.net>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; open
> > list:MAC80211 <linux-wireless@vger.kernel.org>; open list:NETWORKING [GENERAL] <netdev@vger.kernel.org>;
> > open list <linux-kernel@vger.kernel.org>
> > Cc: opensource.kernel@vivo.com; youkangren@vivo.com
> > Subject: [PATCH] wifi：mac80211: Replace the ternary conditional operator with max()
>
> The semicolon of "wifi：" is different from others.
>
> > 
> > Replace the ternary conditional operator with max() to make the code clean
> > 
> > Signed-off-by: You Kangren <youkangren@vivo.com>
> > ---
> >  net/mac80211/tdls.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c
> > index a4af3b7675ef..9f8b0842a616 100644
> > --- a/net/mac80211/tdls.c
> > +++ b/net/mac80211/tdls.c
> > @@ -946,7 +946,7 @@ ieee80211_tdls_build_mgmt_packet_data(struct ieee80211_sub_if_data *sdata,
> >         int ret;
> >         struct ieee80211_link_data *link;
> > 
> > -       link_id = link_id >= 0 ? link_id : 0;
> > +       link_id = max(link_id, 0);
>
> Original logic means "if link_id < 0, then use default link (0)" instead of
> "always use link_id larger than or equal to 0". So, I think max(link_id, 0) could
> cause misunderstanding. 

I feel the same way, max() implies we want the 'highest' link whereas for me the
actual code really means 'prefer the non default' (zero) link.

>
> >         rcu_read_lock();
> >         link = rcu_dereference(sdata->link[link_id]);
> >         if (WARN_ON(!link))
> > --
> > 2.39.0
> > 
> > 
> > ------Please consider the environment before printing this e-mail.

RE: [PATCH] wifi：mac80211: Replace the ternary conditional operator with max()

[David Laight]

From: Ping-Ke Shih
> Sent: 28 June 2023 02:49
> 
> > -----Original Message-----
> > From: You Kangren <youkangren@vivo.com>
> > Sent: Monday, June 26, 2023 6:48 PM
> > To: Johannes Berg <johannes@sipsolutions.net>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; open
> > list:MAC80211 <linux-wireless@vger.kernel.org>; open list:NETWORKING [GENERAL]
> <netdev@vger.kernel.org>;
> > open list <linux-kernel@vger.kernel.org>
> > Cc: opensource.kernel@vivo.com; youkangren@vivo.com
> > Subject: [PATCH] wifi：mac80211: Replace the ternary conditional operator with max()
> 
> The semicolon of "wifi：" is different from others.
> 
> >
> > Replace the ternary conditional operator with max() to make the code clean
> >
> > Signed-off-by: You Kangren <youkangren@vivo.com>
> > ---
> >  net/mac80211/tdls.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c
> > index a4af3b7675ef..9f8b0842a616 100644
> > --- a/net/mac80211/tdls.c
> > +++ b/net/mac80211/tdls.c
> > @@ -946,7 +946,7 @@ ieee80211_tdls_build_mgmt_packet_data(struct ieee80211_sub_if_data *sdata,
> >         int ret;
> >         struct ieee80211_link_data *link;
> >
> > -       link_id = link_id >= 0 ? link_id : 0;
> > +       link_id = max(link_id, 0);
> 
> Original logic means "if link_id < 0, then use default link (0)" instead of
> "always use link_id larger than or equal to 0". So, I think max(link_id, 0) could
> cause misunderstanding.

The clearest is probably:
	if (link_id < 0)
		link_id = 0;

The compiler could easily generate the same code (compare and conditional
move).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] wifi：mac80211: Replace the ternary conditional operator with max()

[Ping-Ke Shih]

> -----Original Message-----
> From: David Laight <David.Laight@ACULAB.COM>
> Sent: Thursday, June 29, 2023 9:28 PM
> To: Ping-Ke Shih <pkshih@realtek.com>; You Kangren <youkangren@vivo.com>; Johannes Berg
> <johannes@sipsolutions.net>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; open list:MAC80211
> <linux-wireless@vger.kernel.org>; open list:NETWORKING [GENERAL] <netdev@vger.kernel.org>; open list
> <linux-kernel@vger.kernel.org>
> Cc: opensource.kernel@vivo.com
> Subject: RE: [PATCH] wifi：mac80211: Replace the ternary conditional operator with max()
> 
> From: Ping-Ke Shih
> > Sent: 28 June 2023 02:49
> >
> > > -----Original Message-----
> > > From: You Kangren <youkangren@vivo.com>
> > > Sent: Monday, June 26, 2023 6:48 PM
> > > To: Johannes Berg <johannes@sipsolutions.net>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> > > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; open
> > > list:MAC80211 <linux-wireless@vger.kernel.org>; open list:NETWORKING [GENERAL]
> > <netdev@vger.kernel.org>;
> > > open list <linux-kernel@vger.kernel.org>
> > > Cc: opensource.kernel@vivo.com; youkangren@vivo.com
> > > Subject: [PATCH] wifi：mac80211: Replace the ternary conditional operator with max()
> >
> > The semicolon of "wifi：" is different from others.
> >
> > >
> > > Replace the ternary conditional operator with max() to make the code clean
> > >
> > > Signed-off-by: You Kangren <youkangren@vivo.com>
> > > ---
> > >  net/mac80211/tdls.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c
> > > index a4af3b7675ef..9f8b0842a616 100644
> > > --- a/net/mac80211/tdls.c
> > > +++ b/net/mac80211/tdls.c
> > > @@ -946,7 +946,7 @@ ieee80211_tdls_build_mgmt_packet_data(struct ieee80211_sub_if_data *sdata,
> > >         int ret;
> > >         struct ieee80211_link_data *link;
> > >
> > > -       link_id = link_id >= 0 ? link_id : 0;
> > > +       link_id = max(link_id, 0);
> >
> > Original logic means "if link_id < 0, then use default link (0)" instead of
> > "always use link_id larger than or equal to 0". So, I think max(link_id, 0) could
> > cause misunderstanding.
> 
> The clearest is probably:
>         if (link_id < 0)
>                 link_id = 0;
> 
> The compiler could easily generate the same code (compare and conditional
> move).
> 

They would be the same, but personally I prefer original single one line statement
that is clear to me.