| Message ID | 20240802054421.5428-1-yyyynoom@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: use ip_hdrlen() instead of bit shift | expand |
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);
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);
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" ...
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 ^오^
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 ^오^
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. ☯
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
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);
`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(-)