| Message ID | 20170411010436.23290-2-benh@kernel.crashing.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello! On 4/11/2017 4:04 AM, Benjamin Herrenschmidt wrote: > The documentation describes NETIF_F_IP_CSUM as deprecated > so let's switch to NETIF_F_HW_CSUM and use the helper to > handle unhandled protocols. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > drivers/net/ethernet/faraday/ftgmac100.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c > index 98b8956..85b650a 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -637,7 +637,8 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb, > csum_vlan |= FTGMAC100_TXDES1_TCP_CHKSUM; > else if (ip_proto == IPPROTO_UDP) > csum_vlan |= FTGMAC100_TXDES1_UDP_CHKSUM; > - } > + } else if (skb_checksum_help(skb)) > + goto drop; Need {} here as well since the 1st branch has it -- see Documentation/process/coding-style.rst (the end of the section 3). [...] MBR, Sergei
On Tue, 2017-04-11 at 13:57 +0300, Sergei Shtylyov wrote: > Need {} here as well since the 1st branch has it -- see > Documentation/process/coding-style.rst (the end of the section 3). Adding {} in that specific statements just makes things more cluttered and less readable. I can find a ton of examples of if (...) { multi lines ... } else if (...) single_line() In existing kernel code. I'll fix it in a next spin if Dave wants it that way but otherwise I'm keen to leave it as it is. Ben.
On Tue, 2017-04-11 at 21:13 +1000, Benjamin Herrenschmidt wrote: > On Tue, 2017-04-11 at 13:57 +0300, Sergei Shtylyov wrote: > > Need {} here as well since the 1st branch has it -- see > > Documentation/process/coding-style.rst (the end of the section 3). > > Adding {} in that specific statements just makes things more > cluttered and less readable. > > I can find a ton of examples of > > if (...) { > multi lines > ... > } else if (...) > single_line() > > In existing kernel code. > > I'll fix it in a next spin if Dave wants it that way but otherwise > I'm keen to leave it as it is. I actually took the time to read the coding-style.txt statement, and I would argue that it is about if (...) { ... } else { ... } Which is a different can of worms. I tend to agree that the else by itself followed by a single tabulated statement is a bit "odd" and I tend to use braces in that case as well. However the "} else if (...)" case is subtly different and from a code clarity point of view I prefer what I wrote. This isn't an accident, I specifically wrote that statement that way because I preferred how the code looked like that way. This tend to be my problem with coding style rules (and people who comment on patches solely on coding style issues, especially so marginal ones ;-) ... this needs to be applied along with a bit of common sense and taste. Those rules should be "general guidelines" not absolute laws. A specific piece of code might benefit from violating some of them occasionally for all sort of reasons provided the end result is both clear and more concise for example. Anyway, way too much internet bandwidth wasted today on that subject. Dave, just let me know how you want it to look like :-) Cheers, Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: Tue, 11 Apr 2017 21:13:45 +1000 > On Tue, 2017-04-11 at 13:57 +0300, Sergei Shtylyov wrote: >> Need {} here as well since the 1st branch has it -- see >> Documentation/process/coding-style.rst (the end of the section 3). > > Adding {} in that specific statements just makes things more > cluttered and less readable. > > I can find a ton of examples of > > if (...) { > multi lines > ... > } else if (...) > single_line() > > In existing kernel code. Existing practice not following the coding style rules does not dictate that it's OK to do so. > I'll fix it in a next spin if Dave wants it that way but otherwise > I'm keen to leave it as it is. Please fix this and respin. Meanwhile get the coding style rules changed if you disagree with them. A patch series review is not the place to argue about your disagreement with the coding style rules. Thanks.
On Tue, 2017-04-11 at 11:27 -0400, David Miller wrote: > > I'll fix it in a next spin if Dave wants it that way but otherwise > > I'm keen to leave it as it is. > > Please fix this and respin. > > Meanwhile get the coding style rules changed if you disagree with > them. A patch series review is not the place to argue about your > disagreement with the coding style rules. I will fix. Note that I don't disagree with the rule as stated. However I'd like to point out that the rule doesn't precisely match the construct here as it's for a dangling single else while what I had here is an else if ... so it's open to intepretation :-) I also tend to disagree that coding style rules should be firm laws, imho they should be considered in context and broken if they render a given piece of code less clear. That said, I will respin. Cheers, Ben.
On Wed, 2017-04-12 at 08:06 +1000, Benjamin Herrenschmidt wrote: > On Tue, 2017-04-11 at 11:27 -0400, David Miller wrote: > > > I'll fix it in a next spin if Dave wants it that way but > > > otherwise > > > I'm keen to leave it as it is. > > > > Please fix this and respin. > > > > Meanwhile get the coding style rules changed if you disagree with > > them. A patch series review is not the place to argue about your > > disagreement with the coding style rules. > > I will fix. Funny thing is, I think the code is wrong :) I should call the helper when I don't recognize the protocol type in the IP header, not just when the main skb protocol type is not IP. BTW. I'm not too familiar with how encapsulation works these days. I wouldn't throw at that HW anything other than unencapsulated packets for HW checksumming. Is checking skb->protocol to be IP and ip_hdr(skb)->protocol to be IP, UDP or TCP enough ? IE. Will the latter especially return the outer header ? Cheers, Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: Wed, 12 Apr 2017 09:36:05 +1000 > I should call the helper when I don't recognize the protocol type in > the IP header, not just when the main skb protocol type is not IP. That's correct. > BTW. I'm not too familiar with how encapsulation works these days. I > wouldn't throw at that HW anything other than unencapsulated packets > for HW checksumming. Is checking skb->protocol to be IP and > ip_hdr(skb)->protocol to be IP, UDP or TCP enough ? IE. Will the latter > especially return the outer header ? If skb->protocol is IP then yes ip_hdr() will point at the outermost header.
On Tue, 2017-04-11 at 20:03 -0400, David Miller wrote: > > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Date: Wed, 12 Apr 2017 09:36:05 +1000 > > > I should call the helper when I don't recognize the protocol type in > > the IP header, not just when the main skb protocol type is not IP. > > That's correct. > > > BTW. I'm not too familiar with how encapsulation works these days. I > > wouldn't throw at that HW anything other than unencapsulated packets > > for HW checksumming. Is checking skb->protocol to be IP and > > ip_hdr(skb)->protocol to be IP, UDP or TCP enough ? IE. Will the latter > > especially return the outer header ? > > If skb->protocol is IP then yes ip_hdr() will point at the outermost > header. Great thanks. I'll repost later today in case some other comments still come in. Cheers, Ben.
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index 98b8956..85b650a 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -637,7 +637,8 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb, csum_vlan |= FTGMAC100_TXDES1_TCP_CHKSUM; else if (ip_proto == IPPROTO_UDP) csum_vlan |= FTGMAC100_TXDES1_UDP_CHKSUM; - } + } else if (skb_checksum_help(skb)) + goto drop; } txdes->txdes1 = cpu_to_le32(csum_vlan); @@ -1463,11 +1464,11 @@ static int ftgmac100_probe(struct platform_device *pdev) * when NCSI is enabled on the interface. It doesn't work * in that case. */ - netdev->features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM | + netdev->features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM | NETIF_F_GRO | NETIF_F_SG; if (priv->use_ncsi && of_get_property(pdev->dev.of_node, "no-hw-checksum", NULL)) - netdev->features &= ~NETIF_F_IP_CSUM; + netdev->features &= ~NETIF_F_HW_CSUM; /* register network device */ err = register_netdev(netdev);
The documentation describes NETIF_F_IP_CSUM as deprecated so let's switch to NETIF_F_HW_CSUM and use the helper to handle unhandled protocols. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- drivers/net/ethernet/faraday/ftgmac100.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)