[PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

[PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

[Moon Yeounsu]

`ip_hdr(skb)->ihl << 2` are the same as `ip_hdrlen(skb)`
Therefore, we should use a well-defined function not a bit shift
to find the header length.

It also compress two lines at a single line.

Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
---
 drivers/net/ethernet/jme.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
index b06e24562973..83b185c995df 100644
--- a/drivers/net/ethernet/jme.c
+++ b/drivers/net/ethernet/jme.c
@@ -946,15 +946,13 @@ jme_udpsum(struct sk_buff *skb)
 	if (skb->protocol != htons(ETH_P_IP))
 		return csum;
 	skb_set_network_header(skb, ETH_HLEN);
+
 	if ((ip_hdr(skb)->protocol != IPPROTO_UDP) ||
-	    (skb->len < (ETH_HLEN +
-			(ip_hdr(skb)->ihl << 2) +
-			sizeof(struct udphdr)))) {
+	    (skb->len < (ETH_HLEN + (ip_hdrlen(skb)) + sizeof(struct udphdr)))) {
 		skb_reset_network_header(skb);
 		return csum;
 	}
-	skb_set_transport_header(skb,
-			ETH_HLEN + (ip_hdr(skb)->ihl << 2));
+	skb_set_transport_header(skb, ETH_HLEN + (ip_hdrlen(skb)));
 	csum = udp_hdr(skb)->check;
 	skb_reset_transport_header(skb);
 	skb_reset_network_header(skb);
-- 
2.45.2

Re: [PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

[Christophe JAILLET]

Le 02/08/2024 à 07:44, Moon Yeounsu a écrit :
> `ip_hdr(skb)->ihl << 2` are the same as `ip_hdrlen(skb)`
> Therefore, we should use a well-defined function not a bit shift
> to find the header length.
> 
> It also compress two lines at a single line.
> 
> Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
> ---
>   drivers/net/ethernet/jme.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 

Hi,

> diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
> index b06e24562973..83b185c995df 100644
> --- a/drivers/net/ethernet/jme.c
> +++ b/drivers/net/ethernet/jme.c
> @@ -946,15 +946,13 @@ jme_udpsum(struct sk_buff *skb)
>   	if (skb->protocol != htons(ETH_P_IP))
>   		return csum;
>   	skb_set_network_header(skb, ETH_HLEN);
> +
>   	if ((ip_hdr(skb)->protocol != IPPROTO_UDP) ||
> -	    (skb->len < (ETH_HLEN +
> -			(ip_hdr(skb)->ihl << 2) +
> -			sizeof(struct udphdr)))) {
> +	    (skb->len < (ETH_HLEN + (ip_hdrlen(skb)) + sizeof(struct udphdr)))) {

The extra () around "ip_hdrlen(skb)" can be remove.
Also maybe the ones around "ETH_HLEN + ip_hdrlen(skb)" could also be 
removed.

>   		skb_reset_network_header(skb);
>   		return csum;
>   	}
> -	skb_set_transport_header(skb,
> -			ETH_HLEN + (ip_hdr(skb)->ihl << 2));
> +	skb_set_transport_header(skb, ETH_HLEN + (ip_hdrlen(skb)));

Same here, the extra () around "ip_hdrlen(skb)" can be remove.

CJ

>   	csum = udp_hdr(skb)->check;
>   	skb_reset_transport_header(skb);
>   	skb_reset_network_header(skb);

Re: [PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

[Moon Yeounsu]

On Fri, Aug 2, 2024 at 10:35 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> The extra () around "ip_hdrlen(skb)" can be remove.
> Also maybe the ones around "ETH_HLEN + ip_hdrlen(skb)" could also be
> removed.
Okay, I'll send the next patch which the parenthesis are removed!
But... The parenthesis around `ETH_HLEN + ip_hdrlen(skb) +
sizeof(struct udphdr)`
should be retained, because it makes a clear boundary.
>
> >               skb_reset_network_header(skb);
> >               return csum;
> >       }
> > -     skb_set_transport_header(skb,
> > -                     ETH_HLEN + (ip_hdr(skb)->ihl << 2));
> > +     skb_set_transport_header(skb, ETH_HLEN + (ip_hdrlen(skb)));
>
> Same here, the extra () around "ip_hdrlen(skb)" can be remove.
I'll remove it also.
>
> CJ
>
> >       csum = udp_hdr(skb)->check;
> >       skb_reset_transport_header(skb);
> >       skb_reset_network_header(skb);
>

Thank you for reviewing ^오^

Re: [PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

[Simon Horman]

On Fri, Aug 02, 2024 at 02:44:21PM +0900, Moon Yeounsu wrote:
> `ip_hdr(skb)->ihl << 2` are the same as `ip_hdrlen(skb)`
> Therefore, we should use a well-defined function not a bit shift
> to find the header length.
> 
> It also compress two lines at a single line.
> 
> Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>

Firstly, I think this clean-up is both correct and safe.  Safe because
ip_hdrlen() only relies on ip_hdr(), which is already used in the same code
path. And correct because ip_hdrlen multiplies ihl by 4, which is clearly
equivalent to a left shift of 2 bits.

However, I do wonder about the value of clean-ups for what appears to be a
very old driver, which hasn't received a new feature for quite sometime

And further, I wonder if we should update this driver from "Maintained" to
"Odd Fixes" as the maintainer, "Guo-Fu Tseng" <cooldavid@cooldavid.org>,
doesn't seem to have been seen by lore since early 2020.

https://lore.kernel.org/netdev/20200219034801.M31679@cooldavid.org/

> ---
>  drivers/net/ethernet/jme.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
> index b06e24562973..83b185c995df 100644
> --- a/drivers/net/ethernet/jme.c
> +++ b/drivers/net/ethernet/jme.c
> @@ -946,15 +946,13 @@ jme_udpsum(struct sk_buff *skb)
>  	if (skb->protocol != htons(ETH_P_IP))
>  		return csum;
>  	skb_set_network_header(skb, ETH_HLEN);
> +
>  	if ((ip_hdr(skb)->protocol != IPPROTO_UDP) ||
> -	    (skb->len < (ETH_HLEN +
> -			(ip_hdr(skb)->ihl << 2) +
> -			sizeof(struct udphdr)))) {
> +	    (skb->len < (ETH_HLEN + (ip_hdrlen(skb)) + sizeof(struct udphdr)))) {

The parentheses around the call to ip_hdrlen are unnecessary.
And this line is now too long: networking codes till prefers
code to be 80 columns wide or less.

>  		skb_reset_network_header(skb);
>  		return csum;
>  	}
> -	skb_set_transport_header(skb,
> -			ETH_HLEN + (ip_hdr(skb)->ihl << 2));
> +	skb_set_transport_header(skb, ETH_HLEN + (ip_hdrlen(skb)));

Unnecessary parentheses here too.

>  	csum = udp_hdr(skb)->check;
>  	skb_reset_transport_header(skb);
>  	skb_reset_network_header(skb);

-- 
pw-bot: cr

Re: [PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

[Simon Horman]

On Fri, Aug 02, 2024 at 03:15:34PM +0100, Simon Horman wrote:
> On Fri, Aug 02, 2024 at 02:44:21PM +0900, Moon Yeounsu wrote:
> > `ip_hdr(skb)->ihl << 2` are the same as `ip_hdrlen(skb)`
> > Therefore, we should use a well-defined function not a bit shift
> > to find the header length.
> > 
> > It also compress two lines at a single line.
> > 
> > Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
> 
> Firstly, I think this clean-up is both correct and safe.  Safe because
> ip_hdrlen() only relies on ip_hdr(), which is already used in the same code
> path. And correct because ip_hdrlen multiplies ihl by 4, which is clearly
> equivalent to a left shift of 2 bits.
> 
> However, I do wonder about the value of clean-ups for what appears to be a
> very old driver, which hasn't received a new feature for quite sometime
> 
> And further, I wonder if we should update this driver from "Maintained" to
> "Odd Fixes" as the maintainer, "Guo-Fu Tseng" <cooldavid@cooldavid.org>,
> doesn't seem to have been seen by lore since early 2020.
> 
> https://lore.kernel.org/netdev/20200219034801.M31679@cooldavid.org/

By "Odd Fixes" I meant "Orphan"

...

Re: [PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

[Moon Yeounsu]

On Fri, Aug 2, 2024 at 11:15 PM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Aug 02, 2024 at 02:44:21PM +0900, Moon Yeounsu wrote:
> > `ip_hdr(skb)->ihl << 2` are the same as `ip_hdrlen(skb)`
> > Therefore, we should use a well-defined function not a bit shift
> > to find the header length.
> >
> > It also compress two lines at a single line.
> >
> > Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
>
> Firstly, I think this clean-up is both correct and safe.  Safe because
> ip_hdrlen() only relies on ip_hdr(), which is already used in the same code
> path. And correct because ip_hdrlen multiplies ihl by 4, which is clearly
> equivalent to a left shift of 2 bits.
Firstly, Thank you for reviewing my patch!
>
> However, I do wonder about the value of clean-ups for what appears to be a
> very old driver, which hasn't received a new feature for quite sometime
Oh, I don't know that...
>
> And further, I wonder if we should update this driver from "Maintained" to
> "Odd Fixes" as the maintainer, "Guo-Fu Tseng" <cooldavid@cooldavid.org>,
> doesn't seem to have been seen by lore since early 2020.
>
> https://lore.kernel.org/netdev/20200219034801.M31679@cooldavid.org/
Then, how about deleting the file from the kernel if the driver isn't
maintained?
Many people think like that (At least I think so)
There are files, and if there are issues, then have to fix them.
Who can think unmanaged files remain in the kernel?

> > ---
> >  drivers/net/ethernet/jme.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
> > index b06e24562973..83b185c995df 100644
> > --- a/drivers/net/ethernet/jme.c
> > +++ b/drivers/net/ethernet/jme.c
> > @@ -946,15 +946,13 @@ jme_udpsum(struct sk_buff *skb)
> >       if (skb->protocol != htons(ETH_P_IP))
> >               return csum;
> >       skb_set_network_header(skb, ETH_HLEN);
> > +
> >       if ((ip_hdr(skb)->protocol != IPPROTO_UDP) ||
> > -         (skb->len < (ETH_HLEN +
> > -                     (ip_hdr(skb)->ihl << 2) +
> > -                     sizeof(struct udphdr)))) {
> > +         (skb->len < (ETH_HLEN + (ip_hdrlen(skb)) + sizeof(struct udphdr)))) {
>
> The parentheses around the call to ip_hdrlen are unnecessary.
> And this line is now too long: networking codes till prefers
> code to be 80 columns wide or less.
Okay, I'll keep the kernel coding style too!
>
> >               skb_reset_network_header(skb);
> >               return csum;
> >       }
> > -     skb_set_transport_header(skb,
> > -                     ETH_HLEN + (ip_hdr(skb)->ihl << 2));
> > +     skb_set_transport_header(skb, ETH_HLEN + (ip_hdrlen(skb)));
>
> Unnecessary parentheses here too.
Also fix it :)
>
> >       csum = udp_hdr(skb)->check;
> >       skb_reset_transport_header(skb);
> >       skb_reset_network_header(skb);
>
> --
> pw-bot: cr

Thank you for paying attention to my patch! I'm a beginner who just
came to the kernel.
So... Sorry if I sounded presumptuous and didn't know much about kernels.
But I don't understand why we have to pay attention to unmanaged kernel files.
And why do we have to check whether the file is managed or not?

Thank you for reading my email ^오^

Re: [PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

[Simon Horman]

On Sat, Aug 03, 2024 at 10:47:35AM +0900, Moon Yeounsu wrote:
> On Fri, Aug 2, 2024 at 11:15 PM Simon Horman <horms@kernel.org> wrote:
> >
> > On Fri, Aug 02, 2024 at 02:44:21PM +0900, Moon Yeounsu wrote:
> > > `ip_hdr(skb)->ihl << 2` are the same as `ip_hdrlen(skb)`
> > > Therefore, we should use a well-defined function not a bit shift
> > > to find the header length.
> > >
> > > It also compress two lines at a single line.
> > >
> > > Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
> >
> > Firstly, I think this clean-up is both correct and safe.  Safe because
> > ip_hdrlen() only relies on ip_hdr(), which is already used in the same code
> > path. And correct because ip_hdrlen multiplies ihl by 4, which is clearly
> > equivalent to a left shift of 2 bits.
> Firstly, Thank you for reviewing my patch!
> >
> > However, I do wonder about the value of clean-ups for what appears to be a
> > very old driver, which hasn't received a new feature for quite sometime
> Oh, I don't know that...
> >
> > And further, I wonder if we should update this driver from "Maintained" to
> > "Odd Fixes" as the maintainer, "Guo-Fu Tseng" <cooldavid@cooldavid.org>,
> > doesn't seem to have been seen by lore since early 2020.
> >
> > https://lore.kernel.org/netdev/20200219034801.M31679@cooldavid.org/
> Then, how about deleting the file from the kernel if the driver isn't
> maintained?

That is a bit more severe than marking it as being unmaintained
in MAINTAINERS. But I do agree that it should be considered.

> Many people think like that (At least I think so)
> There are files, and if there are issues, then have to fix them.
> Who can think unmanaged files remain in the kernel?

And yet, they do exist. ☯

Re: [PATCH] net: ethernet: use ip_hdrlen() instead of bit shift

[Guo-Fu Tseng]

On Sun, 4 Aug 2024 11:18:58 +0100, Simon Horman wrote
> On Sat, Aug 03, 2024 at 10:47:35AM +0900, Moon Yeounsu wrote:
> > On Fri, Aug 2, 2024 at 11:15 PM Simon Horman <horms@kernel.org> wrote:
> > >
> > > On Fri, Aug 02, 2024 at 02:44:21PM +0900, Moon Yeounsu wrote:
> > > > `ip_hdr(skb)->ihl << 2` are the same as `ip_hdrlen(skb)`
> > > > Therefore, we should use a well-defined function not a bit shift
> > > > to find the header length.
> > > >
> > > > It also compress two lines at a single line.
> > > >
> > > > Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
> > >
> > > Firstly, I think this clean-up is both correct and safe.  Safe because
> > > ip_hdrlen() only relies on ip_hdr(), which is already used in the same code
> > > path. And correct because ip_hdrlen multiplies ihl by 4, which is clearly
> > > equivalent to a left shift of 2 bits.
> > Firstly, Thank you for reviewing my patch!
> > >
> > > However, I do wonder about the value of clean-ups for what appears to be a
> > > very old driver, which hasn't received a new feature for quite sometime
> > Oh, I don't know that...
> > >
> > > And further, I wonder if we should update this driver from "Maintained" to
> > > "Odd Fixes" as the maintainer, "Guo-Fu Tseng" <cooldavid@cooldavid.org>,
> > > doesn't seem to have been seen by lore since early 2020.

The device has been EOLed for a lone time.
Since there is no new related chip and no new updates from the chip maker, I do agreed to make the status as "Odd
Fixes".

> > >
> > > https://lore.kernel.org/netdev/20200219034801.M31679@cooldavid.org/
> > Then, how about deleting the file from the kernel if the driver isn't
> > maintained?
> 
> That is a bit more severe than marking it as being unmaintained
> in MAINTAINERS. But I do agree that it should be considered.
> 
> > Many people think like that (At least I think so)
> > There are files, and if there are issues, then have to fix them.
> > Who can think unmanaged files remain in the kernel?
> 
> And yet, they do exist. ☯


Guo-Fu Tseng

[PATCH v2] net: ethernet: remove unnecessary parentheses

[Moon Yeounsu]

remove unnecessary parentheses surrounding `ip_hdrlen()`,
And keep it under 80 columns long to follow the kernel coding style.

Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
---
 drivers/net/ethernet/jme.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
index 83b185c995df..d8be0e4dcb07 100644
--- a/drivers/net/ethernet/jme.c
+++ b/drivers/net/ethernet/jme.c
@@ -947,12 +947,12 @@ jme_udpsum(struct sk_buff *skb)
 		return csum;
 	skb_set_network_header(skb, ETH_HLEN);
 
-	if ((ip_hdr(skb)->protocol != IPPROTO_UDP) ||
-	    (skb->len < (ETH_HLEN + (ip_hdrlen(skb)) + sizeof(struct udphdr)))) {
+	if (ip_hdr(skb)->protocol != IPPROTO_UDP ||
+	    skb->len < (ETH_HLEN + ip_hdrlen(skb) + sizeof(struct udphdr))) {
 		skb_reset_network_header(skb);
 		return csum;
 	}
-	skb_set_transport_header(skb, ETH_HLEN + (ip_hdrlen(skb)));
+	skb_set_transport_header(skb, ETH_HLEN + ip_hdrlen(skb));
 	csum = udp_hdr(skb)->check;
 	skb_reset_transport_header(skb);
 	skb_reset_network_header(skb);
-- 
2.45.2

Re: [PATCH v2] net: ethernet: remove unnecessary parentheses

[Christophe JAILLET]

Hi,

this is not how things work.
This v2 depends on the previous v1. It shouldn't.

v2 should be a standalone patch which includes v1 and all modifications 
done afterward.


When you send a new patch you should:
   - give an history of changes, below the ---. See [1]
   - eventually give links to previous version
   - your new version should not be threaded with the previous mails. It 
should start a new thread.


Le 03/08/2024 à 04:29, Moon Yeounsu a écrit :
> remove unnecessary parentheses surrounding `ip_hdrlen()`,
> And keep it under 80 columns long to follow the kernel coding style.
> 
> Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
> ---

[1]: here.

*As an example*, I would have done as follow, but there is no rule 
(AFAIK) for this section.


Changes in v2:
   - Remove extra ()   [Christophe Jaillet, Simon Horman]
   - Break long lines   [Simon Horman]

v1: https://lore.kernel.org/all/20240802054421.5428-1-yyyynoom@gmail.com/

>   drivers/net/ethernet/jme.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
> index 83b185c995df..d8be0e4dcb07 100644
> --- a/drivers/net/ethernet/jme.c
> +++ b/drivers/net/ethernet/jme.c
> @@ -947,12 +947,12 @@ jme_udpsum(struct sk_buff *skb)
>   		return csum;
>   	skb_set_network_header(skb, ETH_HLEN);
>   
> -	if ((ip_hdr(skb)->protocol != IPPROTO_UDP) ||
> -	    (skb->len < (ETH_HLEN + (ip_hdrlen(skb)) + sizeof(struct udphdr)))) {
> +	if (ip_hdr(skb)->protocol != IPPROTO_UDP ||
> +	    skb->len < (ETH_HLEN + ip_hdrlen(skb) + sizeof(struct udphdr))) {
>   		skb_reset_network_header(skb);
>   		return csum;
>   	}
> -	skb_set_transport_header(skb, ETH_HLEN + (ip_hdrlen(skb)));
> +	skb_set_transport_header(skb, ETH_HLEN + ip_hdrlen(skb));
>   	csum = udp_hdr(skb)->check;
>   	skb_reset_transport_header(skb);
>   	skb_reset_network_header(skb);