Re: [PATCH net-next v3 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY
On Fri, 30 Oct 2020 12:29:50 -0500 Dan Murphy wrote:
> The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
> that supports 10M single pair cable.
>
> The device supports both 2.4-V p2p and 1-V p2p output voltage as defined
> by IEEE 802.3cg 10Base-T1L specfications. These modes can be forced via
> the device tree or the device is defaulted to auto negotiation to
> determine the proper p2p voltage.
>
> Signed-off-by: Dan Murphy
drivers/net/phy/dp83td510.c:70:11: warning: symbol 'dp83td510_feature_array'
was not declared. Should it be static?
Also this:
WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
#429: FILE: drivers/net/phy/dp83td510.c:371:
+ return -ENOTSUPP;
WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
#524: FILE: drivers/net/phy/dp83td510.c:466:
+ return -ENOTSUPP;
ERROR: space required before the open parenthesis '('
#580: FILE: drivers/net/phy/dp83td510.c:522:
+ if(phydev->autoneg) {
ERROR: space required before the open parenthesis '('
#588: FILE: drivers/net/phy/dp83td510.c:530:
+ if(phydev->autoneg) {
And please try to wrap the code on 80 chars on the non trivial lines:
WARNING: line length of 88 exceeds 80 columns
#458: FILE: drivers/net/phy/dp83td510.c:400:
+ mst_slave_cfg = phy_read_mmd(phydev, DP83TD510_PMD_DEVADDR,
DP83TD510_PMD_CTRL);
WARNING: line length of 91 exceeds 80 columns
#505: FILE: drivers/net/phy/dp83td510.c:447:
+ linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
phydev->supported);
WARNING: line length of 93 exceeds 80 columns
#507: FILE: drivers/net/phy/dp83td510.c:449:
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
phydev->supported);
WARNING: line length of 91 exceeds 80 columns
#514: FILE: drivers/net/phy/dp83td510.c:456:
+ linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
phydev->supported);
WARNING: line length of 93 exceeds 80 columns
#516: FILE: drivers/net/phy/dp83td510.c:458:
+ linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
phydev->supported);
WARNING: line length of 92 exceeds 80 columns
#560: FILE: drivers/net/phy/dp83td510.c:502:
+ DP83TD510_MAC_CFG_1,
dp83td510->rgmii_delay);
WARNING: line length of 88 exceeds 80 columns
#574: FILE: drivers/net/phy/dp83td510.c:516:
+ mst_slave_cfg = phy_read_mmd(phydev, DP83TD510_PMD_DEVADDR,
DP83TD510_PMD_CTRL);
WARNING: line length of 84 exceeds 80 columns
#695: FILE: drivers/net/phy/dp83td510.c:637:
+ dp83td510 = devm_kzalloc(>mdio.dev, sizeof(*dp83td510),
GFP_KERNEL);
Re: [PATCH] fix for potential NULL pointer dereference with bare lan743x
On Thu, 29 Oct 2020 03:28:45 +0300 Sergej Bauer wrote:
> This is just a minor fix which prevents a kernel NULL pointer
> dereference when using phy-less lan743x.
>
> Signed-off-by: Sergej Bauer
I take you mean when the device is down netdev->phydev will be NULL?
> diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> index dcde496da7fb..354d72d550f2 100644
> --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> @@ -793,6 +795,9 @@ static int lan743x_ethtool_set_wol(struct net_device
> *netdev,
> {
> struct lan743x_adapter *adapter = netdev_priv(netdev);
>
> + if (!netdev->phydev)
> + return -EIO;
Does it make sense to just skip the phy_ethtool_set_wol() call instead?
Also doesn't the wol configuration of the PHY get lost across an
netdev up/down cycle in this driver? Should it be re-applied after phy
is connected back?
> adapter->wolopts = 0;
> if (wol->wolopts & WAKE_UCAST)
> adapter->wolopts |= WAKE_UCAST;
Re: [PATCH net-next v4] net: dec: tulip: de2104x: Add shutdown handler to stop NIC
On Wed, 28 Oct 2020 10:21:25 -0700 Moritz Fischer wrote: > The driver does not implement a shutdown handler which leads to issues > when using kexec in certain scenarios. The NIC keeps on fetching > descriptors which gets flagged by the IOMMU with errors like this: > > DMAR: DMAR:[DMA read] Request device [5e:00.0]fault addr f000 > DMAR: DMAR:[DMA read] Request device [5e:00.0]fault addr f000 > DMAR: DMAR:[DMA read] Request device [5e:00.0]fault addr f000 > DMAR: DMAR:[DMA read] Request device [5e:00.0]fault addr f000 > DMAR: DMAR:[DMA read] Request device [5e:00.0]fault addr f000 > > Signed-off-by: Moritz Fischer Applied, thanks!
Re: [PATCH v2 net 0/5] net: ipa: minor bug fixes
On Thu, 29 Oct 2020 11:50:52 -0500 Alex Elder wrote: > On 10/29/20 11:11 AM, Jakub Kicinski wrote: > > On Wed, 28 Oct 2020 14:41:43 -0500 Alex Elder wrote: > >> This series fixes several bugs. They are minor, in that the code > >> currently works on supported platforms even without these patches > >> applied, but they're bugs nevertheless and should be fixed. > > > > By which you mean "it seems to work just fine most of the time" or "the > > current code does not exercise this paths/functionally these bugs don't > > matter for current platforms". > > The latter, although for patch 3 I'm not 100% sure. > > Case by case: > Patch 1: >It works. I inquired what the consequence of passing this >wrong buffer pointer was, and for the way we are using IPA >it seems it's fine--the memory pointer we were assigning is >not used, so it's OK. But we're assigning the wrong pointer. > Patch 2: >It works. Even though the bit field is 1 bit wide (not two) >we never actually write a value greater than 1, so we don't >cause a problem. But the definition is incorrect. > Patch 3: >It works, but on the SDM845 we should be assigning the endpoints >to use resource group 1 (they are 0 by default). The way we >currently use this upstream we don't have other endpoints >competing for resources, so I think this is fine. SC7180 we >will assign endpoints to resource group 0, which is the default. > Patch 4: >It works. This is like patch 2; we define the number of these >things incorrectly, but the way we currently use them we never >exceed the limit in a broken way. > Patch 5: >It works. The maximum number of supported groups is even, >and if a (smaller) odd number are used the remainder are >programmed with 0, which is appropriate for undefined >fields. > > If you have any concerns about back-porting these fixes I > think I'm comfortable posting them for net-next instead. > I debated that before sending them out. Please request that > if it's what you think would be best. Looks like these patches apply cleanly to net-next, so I put them there. Thanks!
Re: [PATCH v4 net-next] net: bridge: mcast: add support for raw L2 multicast groups
On Thu, 29 Oct 2020 01:38:31 +0200 Vladimir Oltean wrote: > From: Nikolay Aleksandrov > > Extend the bridge multicast control and data path to configure routes > for L2 (non-IP) multicast groups. > > The uapi struct br_mdb_entry union u is extended with another variant, > mac_addr, which does not change the structure size, and which is valid > when the proto field is zero. > > To be compatible with the forwarding code that is already in place, > which acts as an IGMP/MLD snooping bridge with querier capabilities, we > need to declare that for L2 MDB entries (for which there exists no such > thing as IGMP/MLD snooping/querying), that there is always a querier. > Otherwise, these entries would be flooded to all bridge ports and not > just to those that are members of the L2 multicast group. > > Needless to say, only permanent L2 multicast groups can be installed on > a bridge port. > > Signed-off-by: Nikolay Aleksandrov > Signed-off-by: Vladimir Oltean Applied, thanks!
Re: [PATCH net-next] net: bridge: explicitly convert between mdb entry state and port group flags
On Thu, 29 Oct 2020 01:48:15 +0200 Vladimir Oltean wrote: > When creating a new multicast port group, there is implicit conversion > between the __u8 state member of struct br_mdb_entry and the unsigned > char flags member of struct net_bridge_port_group. This implicit > conversion relies on the fact that MDB_PERMANENT is equal to > MDB_PG_FLAGS_PERMANENT. > > Let's be more explicit and convert the state to flags manually. > > Signed-off-by: Vladimir Oltean Applied, thanks!
Re: [PATCH net-next 4/5] net: mscc: ocelot: make entry_type a member of struct ocelot_multicast
On Thu, 29 Oct 2020 04:27:37 +0200 Vladimir Oltean wrote:
> + mc = devm_kzalloc(ocelot->dev, sizeof(*mc), GFP_KERNEL);
> + if (!mc)
> + return -ENOMEM;
> +
> + mc->entry_type = ocelot_classify_mdb(mdb->addr);
> + ether_addr_copy(mc->addr, mdb->addr);
> + mc->vid = vid;
> +
> + pgid = ocelot_mdb_get_pgid(ocelot, mc);
>
> if (pgid < 0) {
> dev_err(ocelot->dev,
> @@ -1038,24 +1044,19 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int
> port,
> return -ENOSPC;
> }
Transitionally leaking mc here on pgid < 0
Re: [PATCH net-next 0/5] L2 multicast forwarding for Ocelot switch
On Thu, 29 Oct 2020 04:27:33 +0200 Vladimir Oltean wrote: > This series enables the mscc_ocelot switch to forward raw L2 (non-IP) > mdb entries as configured by the bridge driver after this patch: > > https://patchwork.ozlabs.org/project/netdev/patch/20201028233831.610076-1-vladimir.olt...@nxp.com/ Applied, thanks!
Re: [PATCH net-next] net: dlci: Deprecate the DLCI driver (aka the Frame Relay layer)
On Wed, 28 Oct 2020 00:05:04 -0700 Xie He wrote:
> I wish to deprecate the Frame Relay layer (dlci.c) in the kernel because
> we already have a newer and better "HDLC Frame Relay" layer (hdlc_fr.c).
>
> Reasons why hdlc_fr.c is better than dlci.c include:
>
> 1.
> dlci.c is dated 1997, while hdlc_fr.c is dated 1999 - 2006, so the later
> is newer than the former.
>
> 2.
> hdlc_fr.c is working well (tested by me). For dlci.c, I cannot even find
> the user space problem needed to use it. The link provided in
> Documentation/networking/framerelay.rst (in the last paragraph) is no
> longer valid.
>
> 3.
> dlci.c supports only one hardware driver - sdla.c, while hdlc_fr.c
> supports many hardware drivers through the generic HDLC layer (hdlc.c).
>
> WAN hardware devices are usually able to support several L2 protocols
> at the same time, so the HDLC layer is more suitable for these devices.
>
> The hardware devices that sdla.c supports are also multi-protocol
> (according to drivers/net/wan/Kconfig), so the HDLC layer is more
> suitable for these devices, too.
>
> 4.
> hdlc_fr.c supports LMI and supports Ethernet emulation. dlci.c supports
> neither according to its code.
>
> 5.
> include/uapi/linux/if_frad.h, which comes with dlci.c, contains two
> structs for ioctl configs (dlci_conf and frad_conf). According to the
> comments, these two structs are specially crafted for sdla.c (the only
> hardware driver dlci.c supports). I think this makes dlci.c not generic
> enough to support other hardware drivers.
>
> Signed-off-by: Xie He
This code has only seen cleanup patches since the git era begun -
do you think that it may still have users? Or is it completely unused?
The usual way of getting rid of old code is to move it to staging/
for a few releases then delete it, like Arnd just did with wimax.
> diff --git a/Documentation/networking/framerelay.rst
> b/Documentation/networking/framerelay.rst
> index 6d904399ec6d..92e66fc3dffc 100644
> --- a/Documentation/networking/framerelay.rst
> +++ b/Documentation/networking/framerelay.rst
> @@ -4,6 +4,9 @@
> Frame Relay (FR)
>
>
> +(Note that this Frame Relay layer is deprecated. New drivers should use the
> +HDLC Frame Relay layer instead.)
> +
> Frame Relay (FR) support for linux is built into a two tiered system of
> device
> drivers. The upper layer implements RFC1490 FR specification, and uses the
> Data Link Connection Identifier (DLCI) as its hardware address. Usually
> these
> diff --git a/drivers/net/wan/dlci.c b/drivers/net/wan/dlci.c
> index 3ca4daf63389..1f0eee10c13f 100644
> --- a/drivers/net/wan/dlci.c
> +++ b/drivers/net/wan/dlci.c
> @@ -514,6 +514,8 @@ static int __init init_dlci(void)
> register_netdevice_notifier(_notifier);
>
> printk("%s.\n", version);
> + pr_warn("The DLCI driver (the Frame Relay layer) is deprecated.\n"
> + "Please move your driver to the HDLC Frame Relay layer.\n");
>
> return 0;
> }
> diff --git a/drivers/net/wan/sdla.c b/drivers/net/wan/sdla.c
> index bc2c1c7fb1a4..21d602f698fc 100644
> --- a/drivers/net/wan/sdla.c
> +++ b/drivers/net/wan/sdla.c
> @@ -1623,6 +1623,9 @@ static int __init init_sdla(void)
> int err;
>
> printk("%s.\n", version);
> + pr_warn("The SDLA driver is deprecated.\n"
> + "If you are still using the hardware,\n"
> + "please help move this driver to the HDLC Frame Relay
> layer.\n");
>
> sdla = alloc_netdev(sizeof(struct frad_local), "sdla0",
> NET_NAME_UNKNOWN, setup_sdla);
Re: [PATCH v1 net-next] net: dsa: qca: ar9331: add ethtool stats support
On Fri, 20 Nov 2020 13:59:07 +0100 Oleksij Rempel wrote: > On Mon, Nov 16, 2020 at 04:28:44PM -0800, Jakub Kicinski wrote: > > On Tue, 17 Nov 2020 02:10:05 +0200 Vladimir Oltean wrote: > > > On Mon, Nov 16, 2020 at 04:02:13PM -0800, Jakub Kicinski wrote: > > > > For a while now we have been pushing back on stats which have a proper > > > > interface to be added to ethtool -S. So I'd expect the list of stats > > > > exposed via ethtool will end up being shorter than in this patch. > > > > > > Hmm, not sure if that's ever going to be the case. Even with drivers > > > that are going to expose standardized forms of counters, I'm not sure > > > it's going to be nice to remove them from ethtool -S. > > > > Not remove, but also not accept adding them to new drivers. > > > > > Testing teams all > > > over the world have scripts that grep for those. Unfortunately I think > > > ethtool -S will always remain a dumping ground of hell, and the place > > > where you search for a counter based on its name from the hardware block > > > guide as opposed to its standardized name/function. And that might mean > > > there's no reason to not accept Oleksij's patch right away. Even if he > > > might volunteer to actually follow up with a patch where he exposes the > > > .ndo_get_stats64 from DSA towards drivers, as well as implements > > > .ndo_has_offload_stats and .ndo_get_offload_stats within DSA, that will > > > most likely be done as separate patches to this one, and not change in > > > any way how this patch looks. > > Ok, so what is the plan for me? Implement .ndo_get_stats64 before this > one? Yup. And ethtool_ops::get_pause_stats, preferable 'cause I think you had pause stats there, too.
Re: [RFC V2 net-next 1/2] ethtool: add support for controling the type of adaptive coalescing
On Fri, 20 Nov 2020 14:24:38 +0800 Huazhong Tan wrote: > + ``ETHTOOL_A_COALESCE_USE_DIM`` boolusing DIM adaptive Sorry, I didn't get into the discussion on v1 in time. I'd prefer this to be an enum (u8 or u32) the values can already be: - device (HW / FW) - driver specific - DIM
Re: [Linux-kernel-mentees] [PATCH v5 net] rose: Fix Null pointer dereference in rose_send_frame()
On Fri, 20 Nov 2020 00:40:43 +0530 Anmol Karn wrote:
> rose_send_frame() dereferences `neigh->dev` when called from
> rose_transmit_clear_request(), and the first occurrence of the
> `neigh` is in rose_loopback_timer() as `rose_loopback_neigh`,
> and it is initialized in rose_add_loopback_neigh() as NULL.
> i.e when `rose_loopback_neigh` used in rose_loopback_timer()
> its `->dev` was still NULL and rose_loopback_timer() was calling
> rose_rx_call_request() without checking for NULL.
>
> - net/rose/rose_link.c
> This bug seems to get triggered in this line:
>
> rose_call = (ax25_address *)neigh->dev->dev_addr;
>
> Fix it by adding NULL checking for `rose_loopback_neigh->dev`
> in rose_loopback_timer().
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Suggested-by: Jakub Kicinski
> Reported-by: syzbot+a1c743815982d9496...@syzkaller.appspotmail.com
> Tested-by: syzbot+a1c743815982d9496...@syzkaller.appspotmail.com
> Link:
> https://syzkaller.appspot.com/bug?id=9d2a7ca8c7f2e4b682c97578dfa3f236258300b3
> Signed-off-by: Anmol Karn
Applied to net, thanks!
Re: [PATCH net-next v5] net/tun: Call netdev notifiers
On Wed, 18 Nov 2020 07:39:19 +0100 Martin Schiller wrote: > Call netdev notifiers before and after changing the device type. > > Signed-off-by: Martin Schiller This is a fix, right? Can you give an example of something that goes wrong without this patch?
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > This series aims to fix almost all remaining fall-through warnings in > order to enable -Wimplicit-fallthrough for Clang. > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > add multiple break/goto/return/fallthrough statements instead of just > letting the code fall through to the next case. > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > change[1] is meant to be reverted at some point. So, this patch helps > to move in that direction. > > Something important to mention is that there is currently a discrepancy > between GCC and Clang when dealing with switch fall-through to empty case > statements or to cases that only contain a break/continue/return > statement[2][3][4]. Are we sure we want to make this change? Was it discussed before? Are there any bugs Clangs puritanical definition of fallthrough helped find? IMVHO compiler warnings are supposed to warn about issues that could be bugs. Falling through to default: break; can hardly be a bug?!
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > This series aims to fix almost all remaining fall-through warnings in > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > add multiple break/goto/return/fallthrough statements instead of just > > > letting the code fall through to the next case. > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > change[1] is meant to be reverted at some point. So, this patch helps > > > to move in that direction. > > > > > > Something important to mention is that there is currently a discrepancy > > > between GCC and Clang when dealing with switch fall-through to empty case > > > statements or to cases that only contain a break/continue/return > > > statement[2][3][4]. > > > > Are we sure we want to make this change? Was it discussed before? > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > find? > > > > IMVHO compiler warnings are supposed to warn about issues that could > > be bugs. Falling through to default: break; can hardly be a bug?! > > It's certainly a place where the intent is not always clear. I think > this makes all the cases unambiguous, and doesn't impact the machine > code, since the compiler will happily optimize away any behavioral > redundancy. If none of the 140 patches here fix a real bug, and there is no change to machine code then it sounds to me like a W=2 kind of a warning. I think clang is just being annoying here, but if I'm the only one who feels this way chances are I'm wrong :)
Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error
On Fri, 20 Nov 2020 20:01:49 +0200 Vladimir Oltean wrote:
> On Thu, Nov 19, 2020 at 12:09:06PM +0100, Christian Eggers wrote:
> > If dsa_switch_ops::port_txtstamp() returns false, clone will be freed
> > immediately. Shouldn't store a pointer to freed memory.
> >
> > Signed-off-by: Christian Eggers
> > Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX
> > timestamping")
> > ---
>
> IMO this is one of the cases to which the following from
> Documentation/process/stable-kernel-rules.rst does not apply:
>
> - It must fix a real bug that bothers people (not a, "This could be a
>problem..." type thing).
>
> Therefore, specifying "net-next" as the target tree here as opposed to
> "net" is the correct choice.
The commit message doesn't really explain what happens after.
Is the dangling pointer ever accessed?
Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error
On Fri, 20 Nov 2020 23:04:36 +0200 Vladimir Oltean wrote:
> On Fri, Nov 20, 2020 at 12:59:21PM -0800, Jakub Kicinski wrote:
> > On Fri, 20 Nov 2020 20:01:49 +0200 Vladimir Oltean wrote:
> > > On Thu, Nov 19, 2020 at 12:09:06PM +0100, Christian Eggers wrote:
> > > > If dsa_switch_ops::port_txtstamp() returns false, clone will be freed
> > > > immediately. Shouldn't store a pointer to freed memory.
> > > >
> > > > Signed-off-by: Christian Eggers
> > > > Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX
> > > > timestamping")
> > > > ---
> > >
> > > IMO this is one of the cases to which the following from
> > > Documentation/process/stable-kernel-rules.rst does not apply:
> > >
> > > - It must fix a real bug that bothers people (not a, "This could be a
> > >problem..." type thing).
> > >
> > > Therefore, specifying "net-next" as the target tree here as opposed to
> > > "net" is the correct choice.
> >
> > The commit message doesn't really explain what happens after.
> >
> > Is the dangling pointer ever accessed?
>
> Nothing happens afterwards. He explained that he accessed it once while
> working on his ksz9477 PTP series. There's no code affected by this in
> mainline.
Ah, great, I'll drop the Fixes tag altogether then.
Re: [PATCHv3 net-next 0/3] Add devlink and devlink health reporters to
On Fri, 20 Nov 2020 11:57:58 +0530 George Cherian wrote: > Add basic devlink and devlink health reporters. > Devlink health reporters are added for NPA and NIX blocks. > These reporters report the error count in respective blocks. > > Address Jakub's comment to add devlink support for error reporting. > https://www.spinics.net/lists/netdev/msg670712.html This series does not apply to net-next, please rebase and repost.
Re: [PATCH] net: adaptec: remove dead code in set_vlan_mode
On Fri, 20 Nov 2020 15:50:00 +0800 xiakaixu1...@gmail.com wrote:
> From: Kaixu Xia
>
> The body of the if statement can be executed only when the variable
> vlan_count equals to 32, so the condition of the while statement can
> not be true and the while statement is dead code. Remove it.
>
> Reported-by: Tosk Robot
> Signed-off-by: Kaixu Xia
> ---
> drivers/net/ethernet/adaptec/starfire.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/adaptec/starfire.c
> b/drivers/net/ethernet/adaptec/starfire.c
> index 555299737b51..ad27a9fa5e95 100644
> --- a/drivers/net/ethernet/adaptec/starfire.c
> +++ b/drivers/net/ethernet/adaptec/starfire.c
> @@ -1754,14 +1754,9 @@ static u32 set_vlan_mode(struct netdev_private *np)
> filter_addr += 16;
> vlan_count++;
> }
> - if (vlan_count == 32) {
> + if (vlan_count == 32)
> ret |= PerfectFilterVlan;
> - while (vlan_count < 32) {
> - writew(0, filter_addr);
> - filter_addr += 16;
> - vlan_count++;
> - }
> - }
> +
> return ret;
> }
> #endif /* VLAN_SUPPORT */
This got broken back in 2011:
commit 5da96be53a16a62488316810d0c7c5d58ce3ee4f
Author: Jiri Pirko
Date: Wed Jul 20 04:54:31 2011 +
starfire: do vlan cleanup
- unify vlan and nonvlan rx path
- kill np->vlgrp and netdev_vlan_rx_register
Signed-off-by: Jiri Pirko
Signed-off-by: David S. Miller
The comparison to 32 was on a different variable before that change.
Ion, do you think anyone is still using this driver?
Maybe it's time we put it in the history book (by which I mean remove
from the kernel).
Re: [PATCH] net: adaptec: remove dead code in set_vlan_mode
On Fri, 20 Nov 2020 18:41:03 -0500 Ion Badulescu wrote: > Frankly, no, I don't know of any users, and that unfortunately includes > myself. I still have two cards in my stash, but they're 64-bit PCI-X, so > plugging them in would likely require taking a dremel to a 32-bit PCI > slot to make it open-ended. (They do work in a 32-bit slot.) > > Anyway, that filter code could use some fixing in other regards. So > either we fix it properly (which I can submit a patch for), or clean it > out for good. Entirely up to you.
Re: [PATCH net-next 4/6] net: ipa: support retries on generic GSI commands
On Thu, 19 Nov 2020 16:49:27 -0600 Alex Elder wrote: > + do > + ret = gsi_generic_command(gsi, channel_id, > + GSI_GENERIC_HALT_CHANNEL); > + while (ret == -EAGAIN && retries--); This may well be the first time I've seen someone write a do while loop without the curly brackets!
Re: [PATCH v2] mdio_bus: suppress err message for reset gpio EPROBE_DEFER
On Thu, 19 Nov 2020 22:34:46 +0200 Grygorii Strashko wrote: > The mdio_bus may have dependencies from GPIO controller and so got > deferred. Now it will print error message every time -EPROBE_DEFER is > returned which from: > __mdiobus_register() > |-devm_gpiod_get_optional() > without actually identifying error code. > > "mdio_bus 4a101000.mdio: mii_bus 4a101000.mdio couldn't get reset GPIO" > > Hence, suppress error message for devm_gpiod_get_optional() returning > -EPROBE_DEFER case by using dev_err_probe(). > > Signed-off-by: Grygorii Strashko Applied (with the line wrapped), thanks!
Re: [PATCH net-next 4/6] net: ipa: support retries on generic GSI commands
On Fri, 20 Nov 2020 21:31:09 -0600 Alex Elder wrote: > On 11/20/20 8:49 PM, Jakub Kicinski wrote: > > On Thu, 19 Nov 2020 16:49:27 -0600 Alex Elder wrote: > >> + do > >> + ret = gsi_generic_command(gsi, channel_id, > >> +GSI_GENERIC_HALT_CHANNEL); > >> + while (ret == -EAGAIN && retries--); > > > > This may well be the first time I've seen someone write a do while loop > > without the curly brackets! > > I had them at one time, then saw I could get away > without them. I don't have a preference but I see > you accepted it as-is. It was just an offhand comment, I don't have anything against it :)
Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
On Sat, 21 Nov 2020 17:52:27 +0100 Florian Westphal wrote: > Ido Schimmel wrote: > > Other suggestions? > > Aleksandr, why was this made into an skb extension in the first place? > > AFAIU this feature is usually always disabled at build time. > For debug builds (test farm /debug kernel etc) its always needed. > > If thats the case this u64 should be an sk_buff member, not an > extension. Yeah, in hindsight I should have looked at how it's used. Not a great fit for extensions. We can go back, but... In general I'm not very happy at how this is going. First of all just setting the handle in a couple of allocs seems to not be enough, skbs get cloned, reused etc. There were also build problems caused by this patch and Aleksandr & co where nowhere to be found. Now we find out this causes leaks, how was that not caught by the syzbot it's supposed to serve?! So I'm leaning towards reverting the whole thing. You can attach kretprobes and record the information you need in BPF maps.
Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote: > > So I'm leaning towards reverting the whole thing. You can attach > > kretprobes and record the information you need in BPF maps. > > I'm not going to object to reverting it (and perhaps redoing it better > later), but I will point out that kretprobe isn't going to work, you > eventually need kcov_remote_start() to be called in strategic points > before processing the skb after it bounced through the system. > > IOW, it's not really about serving userland, it's about enabling (and > later disabling) coverage collection for the bits of code it cares > about, mostly because collecting it for _everything_ is going to be too > slow and will mess up the data since for coverage guided fuzzing you > really need the reported coverage data to be only about the injected > fuzz data... All you need is make kcov_remote_start_common() be BPF-able, like the LSM hooks are now, right? And then BPF can return whatever handle it pleases. Or if you don't like BPF or what to KCOV BPF itself in the future you can roll your own mechanism. The point is - this should be relatively easily doable out of line...
Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
On Sat, 21 Nov 2020 20:30:44 +0100 Johannes Berg wrote: > On Sat, 2020-11-21 at 10:35 -0800, Jakub Kicinski wrote: > > On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote: > > > > So I'm leaning towards reverting the whole thing. You can attach > > > > kretprobes and record the information you need in BPF maps. > > > > > > I'm not going to object to reverting it (and perhaps redoing it better > > > later), but I will point out that kretprobe isn't going to work, you > > > eventually need kcov_remote_start() to be called in strategic points > > > before processing the skb after it bounced through the system. > > > > > > IOW, it's not really about serving userland, it's about enabling (and > > > later disabling) coverage collection for the bits of code it cares > > > about, mostly because collecting it for _everything_ is going to be too > > > slow and will mess up the data since for coverage guided fuzzing you > > > really need the reported coverage data to be only about the injected > > > fuzz data... > > > > All you need is make kcov_remote_start_common() be BPF-able, like > > the LSM hooks are now, right? And then BPF can return whatever handle > > it pleases. > > Not sure I understand. Are you saying something should call > "kcov_remote_start_common()" with, say, the SKB, and leave it to a mass > of bpf hooks to figure out where the SKB got cloned or copied or > whatnot, track that in a map, and then ... no, wait, I don't really see > what you mean, sorry. > > IIUC, fundamentally, you have this: > > - at the beginning, a task is tagged with "please collect coverage >data for this handle" Write the tag into task local storage, or map indexed by PID. > - this task creates an SKB, etc, and all of the code that this task >executes is captured and the coverage data is reported kprobe the right places to record the skb -> handle mapping. > - However, the SKB traverses lots of things, gets copied, cloned, or >whatnot, and eventually leaves the annotated task, say for further >processing in softirq context or elsewhere. Which is fine. > Now since the whole point is to see what chaos this SKB created from > beginning (allocation) to end (free), since it was filled with fuzzed > data, you now have to figure out where to pick back up when the SKB is > processed further. > > This is what the infrastructure was meant to solve. But note that the > SKB might be further cloned etc, so in order to track it you'd have to > (out-of-band) figure out all the possible places where it could > be reallocated, any time the skb pointer could change. Ack, you have to figure out all the places anyway, the question is whether you put probes there or calls in the source code. Shifting the maintenance burden but also BPF is flexibility. > Then, when you know you've got interesting code on your hands, like in > mac80211 that was annotated in patch 3 here, you basically say > > "oohhh, this SKB was annotated before, let's continue capturing >coverage data here" > > (and turn it off again later by the corresponding kcov_remote_stop(). Yup, the point is you can feed a raw skb pointer (and all other possible context you may want) to a BPF prog in kcov_remote_start() and let BPF/BTF give you the handle it recorded in its maps. > So the only way I could _possibly_ see how to do this would be to > > * capture all possible places where the skb pointer can change > * still call something like skb_get_kcov_handle() but let it call out >to a BPF program to query a map or something to figure out if this >SKB has a handle attached to it > > > Or if you don't like BPF or what to KCOV BPF itself in the future you > > can roll your own mechanism. The point is - this should be relatively > > easily doable out of line... > > Seems pretty complicated to me though ... It is more complicated. We can go back to an skb field if this work is expected to yield results for mac80211. Would you mind sending a patch?
Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
On Sat, 21 Nov 2020 21:58:37 +0100 Johannes Berg wrote: > On Sat, 2020-11-21 at 12:55 -0800, Jakub Kicinski wrote: > > It is more complicated. We can go back to an skb field if this work is > > expected to yield results for mac80211. Would you mind sending a patch? > > I can do that, but I'm not going to be able to do it now/tonight (GMT+1 > here), so probably only Monday/Tuesday or so, sorry. Oh yea, no worries, took someone a month to notice this is broken, as long as it's fixed before the merge window it's fine ;)
Re: [PATCH] cxgb4: Fix build failure when CONFIG_TLS=m
On Fri, 20 Nov 2020 13:25:28 -0600 Tom Seewald wrote:
> After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> whenever CONFIG_TLS=m and CONFIG_CHELSIO_T4=y, the following build
> failure occurs:
>
> ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
> `cxgb_select_queue':
> cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'
>
> Fix this by ensuring that if TLS is set to be a module, CHELSIO_T4 will
> also be compiled as a module. As otherwise the cxgb4 driver will not be
> able to access TLS' symbols.
>
> Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> Signed-off-by: Tom Seewald
Applied, thanks!
Re: [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
On Wed, 18 Nov 2020 22:50:24 -0500 min.li...@renesas.com wrote:
> From: Min Li
>
> Feed kstrtou8 with NULL terminated string.
>
> Changes since v1:
> -Only strcpy 15 characters to leave 1 space for '\0'
>
> Signed-off-by: Min Li
> -static int idtcm_strverscmp(const char *ver1, const char *ver2)
> +static int idtcm_strverscmp(const char *version1, const char *version2)
> {
> u8 num1;
> u8 num2;
> int result = 0;
> + char ver1[16];
> + char ver2[16];
> + char *cur1;
> + char *cur2;
> + char *next1;
> + char *next2;
> +
> + strncpy(ver1, version1, 15);
> + strncpy(ver2, version2, 15);
> + cur1 = ver1;
> + cur2 = ver2;
Now there is no guarantee that the buffer is null terminated.
You need to use strscpy(ver... 16) or add ver[15] = '\0';
> /* loop through each level of the version string */
> while (result == 0) {
> + next1 = strchr(cur1, '.');
> + next2 = strchr(cur2, '.');
> +
> + /* kstrtou8 could fail for dot */
> + if (next1) {
> + *next1 = '\0';
> + next1++;
> + }
> +
> + if (next2) {
> + *next2 = '\0';
> + next2++;
> + }
> +
> /* extract leading version numbers */
> - if (kstrtou8(ver1, 10, ) < 0)
> + if (kstrtou8(cur1, 10, ) < 0)
> return -1;
>
> - if (kstrtou8(ver2, 10, ) < 0)
> + if (kstrtou8(cur2, 10, ) < 0)
> return -1;
>
> /* if numbers differ, then set the result */
> - if (num1 < num2)
> + if (num1 < num2) {
> result = -1;
Why do you set the result to something instead of just returning that
value? The kstrtou8() checks above just return value directly...
If you use a return you can save yourself all the else branches.
> - else if (num1 > num2)
> + } else if (num1 > num2) {
> result = 1;
> - else {
> + } else {
> /* if numbers are the same, go to next level */
> - ver1 = strchr(ver1, '.');
> - ver2 = strchr(ver2, '.');
> - if (!ver1 && !ver2)
> + if (!next1 && !next2)
> break;
> - else if (!ver1)
> + else if (!next1) {
> result = -1;
> - else if (!ver2)
> + } else if (!next2) {
> result = 1;
> - else {
> - ver1++;
> - ver2++;
> + } else {
> + cur1 = next1;
> + cur2 = next2;
> }
> }
> }
> +
> return result;
> }
>
Re: [PATCH] usbnet: ipheth: fix connectivity with iOS 14
On Thu, 19 Nov 2020 18:24:39 +0100 Yves-Alexis Perez wrote: > Starting with iOS 14 released in September 2020, connectivity using the > personal hotspot USB tethering function of iOS devices is broken. > > Communication between the host and the device (for example ICMP traffic > or DNS resolution using the DNS service running in the device itself) > works fine, but communication to endpoints further away doesn't work. > > Investigation on the matter shows that UDP and ICMP traffic from the > tethered host is reaching the Internet at all. For TCP traffic there are > exchanges between tethered host and server but packets are modified in > transit leading to impossible communication. > > After some trials Matti Vuorela discovered that reducing the URB buffer > size by two bytes restored the previous behavior. While a better > solution might exist to fix the issue, since the protocol is not > publicly documented and considering the small size of the fix, let's do > that. > > Tested-by: Matti Vuorela > Signed-off-by: Yves-Alexis Perez > Link: > https://lore.kernel.org/linux-usb/caan0qaxmysj9vx3zemkviv_b19ju-_exn8yn_usefxpjs6g...@mail.gmail.com/ > Link: https://github.com/libimobiledevice/libimobiledevice/issues/1038 Applied to net with the typo fixed, thanks!
Re: [PATCH net] devlink: Fix reload stats structure
On Fri, 20 Nov 2020 15:40:37 +0200 Moshe Shemesh wrote:
> Fix reload stats structure exposed to the user. Change stats structure
> hierarchy to have the reload action as a parent of the stat entry and
> then stat entry includes value per limit. This will also help to avoid
> string concatenation on iproute2 output.
>
> Reload stats structure before this fix:
> "stats": {
> "reload": {
> "driver_reinit": 2,
> "fw_activate": 1,
> "fw_activate_no_reset": 0
> }
> }
>
> After this fix:
> "stats": {
> "reload": {
> "driver_reinit": {
> "unspecified": 2
> },
> "fw_activate": {
> "unspecified": 1,
> "no_reset": 0
> }
> }
>
> Fixes: a254c264267e ("devlink: Add reload stats")
> Signed-off-by: Moshe Shemesh
> Reviewed-by: Jiri Pirko
At least try to fold the core networking code at 80 characters *please*.
You folded the comments at 86 chars, neither 100 nor 80.
Re: [PATCH net-next] r8153_ecm: avoid to be prior to r8152 driver
On Tue, 17 Nov 2020 01:50:03 + Hayes Wang wrote: > Jakub Kicinski > > Sent: Tuesday, November 17, 2020 1:03 AM > [...] > > > Yes, this fixes this issue, although I would prefer a separate Kconfig > > > entry for r8153_ecm with proper dependencies instead of this ifdefs in > > > Makefile. > > > > Agreed, this is what dependency resolution is for. > > > > Let's just make this a separate Kconfig entry. > > Excuse me. I am not familiar with Kconfig. > > I wish r8153_ecm could be used, even > CONFIG_USB_RTL8152 is not defined. > > How should set it in Kconfig? Something like this? config USB_RTL8153_ECM tristate select MII select USB_NET_CDCETHER depends on USB_RTL8152 || USB_RTL8152=n help select clauses will pull in the dependencies you need, and the dependency on RTL8152 will be satisfied either when RTL8152's code is reachable (both are modules or RTL8152 is built in) or when RTL8152 is not built at all. Does that help?
Re: [PATCH net] rsi_91x: fix error return code in rsi_reset_card()
On Tue, 17 Nov 2020 11:07:34 +0800 Zhang Changzhong wrote:
> Fix to return a negative error code from the error handling
> case instead of 0, as done elsewhere in this function.
>
> Fixes: 17ff2c794f39 ("rsi: reset device changes for 9116")
> Reported-by: Hulk Robot
> Signed-off-by: Zhang Changzhong
FWIW please try to tag driver/net/wireless with [PATCH wireless],
Kalle will be taking those via the wireless drivers tree.
Re: [PATCH v2] net: phy: mscc: fix excluded_middle.cocci warnings
On Mon, 16 Nov 2020 22:15:01 +0100 Antoine Tenart wrote:
> Quoting Julia Lawall (2020-11-16 16:34:44)
> > From: kernel test robot
> >
> > Condition !A || A && B is equivalent to !A || B.
> >
> > Generated by: scripts/coccinelle/misc/excluded_middle.cocci
> >
> > Fixes: b76f0ea01312 ("coccinelle: misc: add excluded_middle.cocci script")
> > CC: Denis Efremov
> > Reported-by: kernel test robot
> > Signed-off-by: kernel test robot
> > Signed-off-by: Julia Lawall
>
> Reviewed-by: Antoine Tenart
Applied, thanks!
Re: [PATCH net-next v1] lan743x: replace devicetree phy parse code with library function
On Tue, 17 Nov 2020 03:09:56 +0100 Andrew Lunn wrote: > On Mon, Nov 16, 2020 at 12:01:55PM -0500, Sven Van Asbroeck wrote: > > From: Sven Van Asbroeck > > > > The code in this driver which parses the devicetree to determine > > the phy/fixed link setup, can be replaced by a single library > > function: of_phy_get_and_connect(). > > > > Behaviour is identical, except that the library function will > > complain when 'phy-connection-type' is omitted, instead of > > blindly using PHY_INTERFACE_MODE_NA, which would result in an > > invalid phy configuration. > > > > The library function no longer brings out the exact phy_mode, > > but the driver doesn't need this, because phy_interface_is_rgmii() > > queries the phydev directly. Remove 'phy_mode' from the private > > adapter struct. > > > > While we're here, log info about the attached phy on connect, > > this is useful because the phy type and connection method is now > > fully configurable via the devicetree. > > > > Tested on a lan7430 chip with built-in phy. Verified that adding > > fixed-link/phy-connection-type in the devicetree results in a > > fixed-link setup. Used ethtool to verify that the devicetree > > settings are used. > > > > Tested-by: Sven Van Asbroeck # lan7430 > > Signed-off-by: Sven Van Asbroeck > > Reviewed-by: Andrew Lunn Applied, thanks!
Re: [PATCH net] net: b44: fix error return code in b44_init_one()
On Mon, 16 Nov 2020 20:13:43 -0800 Michael Chan wrote:
> On Mon, Nov 16, 2020 at 7:01 PM Zhang Changzhong wrote:
> >
> > Fix to return a negative error code from the error handling
> > case instead of 0, as done elsewhere in this function.
> >
> > Fixes: 39a6f4bce6b4 ("b44: replace the ssb_dma API with the generic DMA
> > API")
> > Reported-by: Hulk Robot
> > Signed-off-by: Zhang Changzhong
>
> Reviewed-by: Michael Chan
Applied, thanks!
Re: [PATCH net-next v5] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling
On Tue, 17 Nov 2020 07:36:26 +0100 Florian Westphal wrote: > Randy Dunlap wrote: > > The previous Kconfig patch led to some other build errors as > > reported by the 0day bot and my own overnight build testing. > > > > These are all in when KCOV is enabled but > > SKB_EXTENSIONS is not enabled, so fix those by combining those conditions > > in the header file. > > Acked-by: Florian Westphal Applied, thanks1
Re: [PATCH V4 net-next 0/4] net: hns3: updates for -next
On Mon, 16 Nov 2020 16:20:50 +0800 Huazhong Tan wrote: > There are several updates relating to the interrupt coalesce for > the HNS3 ethernet driver. > > #1 adds support for QL(quantity limiting, interrupt coalesce >based on the frame quantity). > #2 queries the maximum value of GL from the firmware instead of >a fixed value in code. > #3 adds support for 1us unit GL(gap limiting, interrupt coalesce >based on the gap time). > #4 renames gl_adapt_enable in struct hns3_enet_coalesce to fit >its new usage. > > change log: > V4 - remove #5~#10 from this series, which needs more discussion. > V3 - fix a typo error in #1 reported by Jakub Kicinski. > rewrite #9 commit log. > remove #11 from this series. > V2 - reorder #2 & #3 to fix compiler error. > fix some checkpatch warnings in #10 & #11. Applied, thanks!
Re: [PATCH net-next 1/1] net: phy: Add additional logics on probing C45 PHY devices
On Thu, 12 Nov 2020 23:03:51 +0800 Wong Vee Khee wrote: > For clause 45 PHY, introduce additional logics in get_phy_c45_ids() to > check if there is at least one valid device ID, return 0 on true, and > -ENODEV otherwise. > > Signed-off-by: Wong Vee Khee I don't see any response to Andrew's questions, so I'm dropping this from patchwork.
Re: [PATCH net-next V5] net: Variable SLAAC: SLAAC with prefixes of arbitrary length in PIO
On Fri, 13 Nov 2020 20:36:58 +0100 Dmytro Shytyi wrote:
> Variable SLAAC: SLAAC with prefixes of arbitrary length in PIO (randomly
> generated hostID or stable privacy + privacy extensions).
> The main problem is that SLAAC RA or PD allocates a /64 by the Wireless
> carrier 4G, 5G to a mobile hotspot, however segmentation of the /64 via
> SLAAC is required so that downstream interfaces can be further subnetted.
> Example: uCPE device (4G + WI-FI enabled) receives /64 via Wireless, and
> assigns /72 to VNF-Firewall, /72 to WIFI, /72 to VNF-Router, /72 to
> Load-Balancer and /72 to wired connected devices.
> IETF document that defines problem statement:
> draft-mishra-v6ops-variable-slaac-problem-stmt
> IETF document that specifies variable slaac:
> draft-mishra-6man-variable-slaac
>
> Signed-off-by: Dmytro Shytyi
> ---
> diff -rupN net-next-5.10.0-rc2/include/net/if_inet6.h
> net-next-patch-5.10.0-rc2/include/net/if_inet6.h
> --- net-next-5.10.0-rc2/include/net/if_inet6.h2020-11-10
> 08:46:00.195180579 +0100
> +++ net-next-patch-5.10.0-rc2/include/net/if_inet6.h 2020-11-11
> 18:11:05.627550135 +0100
> @@ -22,6 +22,12 @@
> #define IF_RS_SENT 0x10
> #define IF_READY 0x8000
>
> +/* Variable SLAAC (Contact: Dmytro Shytyi)
> + * draft-mishra-6man-variable-slaac
> + * draft-mishra-v6ops-variable-slaac-problem-stmt
> + */
> +#define IF_RA_VAR_PLEN 0x08
> +
> /* prefix flags */
> #define IF_PREFIX_ONLINK 0x01
> #define IF_PREFIX_AUTOCONF 0x02
> diff -rupN net-next-5.10.0-rc2/include/uapi/linux/icmpv6.h
> net-next-patch-5.10.0-rc2/include/uapi/linux/icmpv6.h
> --- net-next-5.10.0-rc2/include/uapi/linux/icmpv6.h 2020-11-10
> 08:46:00.351849525 +0100
> +++ net-next-patch-5.10.0-rc2/include/uapi/linux/icmpv6.h 2020-11-11
> 18:11:05.627550135 +0100
> @@ -42,7 +42,9 @@ struct icmp6hdr {
> struct icmpv6_nd_ra {
> __u8hop_limit;
> #if defined(__LITTLE_ENDIAN_BITFIELD)
> - __u8reserved:3,
> + __u8reserved:1,
> + slaac_var_plen:1,
> + proxy:1,
What's the status of your draft? I'm not too familiar with the IETF
process, but I'm not sure we should change uAPI headers before the
draft reaches reasonable consensus.
I'd appreciate extra opinions here.
> router_pref:2,
> home_agent:1,
> other:1,
> @@ -53,7 +55,9 @@ struct icmp6hdr {
> other:1,
> home_agent:1,
> router_pref:2,
> - reserved:3;
> + proxy:1,
> + slaac_var_plen:1,
> + reserved:1;
> #else
> #error "Please fix "
> #endif
> @@ -78,9 +82,9 @@ struct icmp6hdr {
> #define icmp6_addrconf_other icmp6_dataun.u_nd_ra.other
> #define icmp6_rt_lifetimeicmp6_dataun.u_nd_ra.rt_lifetime
> #define icmp6_router_preficmp6_dataun.u_nd_ra.router_pref
> +#define icmp6_slaac_var_plen icmp6_dataun.u_nd_ra.slaac_var_plen
> };
>
> -
> #define ICMPV6_ROUTER_PREF_LOW 0x3
> #define ICMPV6_ROUTER_PREF_MEDIUM0x0
> #define ICMPV6_ROUTER_PREF_HIGH 0x1
> diff -rupN net-next-5.10.0-rc2/net/ipv6/addrconf.c
> net-next-patch-5.10.0-rc2/net/ipv6/addrconf.c
> --- net-next-5.10.0-rc2/net/ipv6/addrconf.c 2020-11-10 08:46:01.075193379
> +0100
> +++ net-next-patch-5.10.0-rc2/net/ipv6/addrconf.c 2020-11-13
> 19:50:04.401227310 +0100
> @@ -11,6 +11,8 @@
> /*
> * Changes:
> *
> + * Dmytro Shytyi : Variable SLAAC: SLAAC with
> + *prefixes of arbitrary length.
Please don't add your name to those headers. We have git now,
authorship if clearly preserved.
> * Janos Farkas: delete timer on ifdown
> *
> * Andi Kleen : kill double kfree on module
> @@ -142,7 +144,11 @@ static int ipv6_count_addresses(const st
> static int ipv6_generate_stable_address(struct in6_addr *addr,
> u8 dad_count,
> const struct inet6_dev *idev);
> -
> +static int ipv6_generate_address_variable_plen(struct in6_addr *address,
> +u8 dad_count,
> +const struct inet6_dev *idev,
> +unsigned int rcvd_prfx_len,
> +bool stable_privacy_mode);
Can you reorder the code to avoid the fwd declaration?
Also please try to shorten the name of this function.
> #define IN6_ADDR_HSIZE_SHIFT 8
> #define IN6_ADDR_HSIZE
Re: [PATCH net] atl1c: fix error return code in atl1c_probe()
On Tue, 17 Nov 2020 21:39:05 +0100 Marion & Christophe JAILLET wrote:
> Le 17/11/2020 à 03:55, Zhang Changzhong a écrit :
> > Fix to return a negative error code from the error handling
> > case instead of 0, as done elsewhere in this function.
> >
> > Fixes: 85eb5bc33717 ("net: atheros: switch from 'pci_' to 'dma_' API")
> Hi, should it have any importance, the Fixes tag is wrong.
>
> The issue was already there before 85eb5bc33717 which was just a
> mechanical update.
Can you provide the correct one, then?
I can switch it when applying.
Re: [PATCH v4 00/27]Fix several bad kernel-doc markups
On Mon, 16 Nov 2020 11:17:56 +0100 Mauro Carvalho Chehab wrote: > Kernel-doc has always be limited to a probably bad documented > rule: > > The kernel-doc markups should appear *imediatelly before* the > function or data structure that it documents. Applied 1-3 to net-next, thanks!
Re: [PATCH] cxbgb4: Fix build failure when CHELSIO_TLS_DEVICE=n
On Sun, 15 Nov 2020 20:31:40 -0600 Tom Seewald wrote:
> After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> building the kernel with CHELSIO_T4=y and CHELSIO_TLS_DEVICE=n results
> in the following error:
>
> ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
> `cxgb_select_queue':
> cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'
>
> This is caused by cxgb_select_queue() calling cxgb4_is_ktls_skb() without
> checking if CHELSIO_TLS_DEVICE=y. Fix this by calling cxgb4_is_ktls_skb()
> only when this config option is enabled.
>
> Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> Signed-off-by: Tom Seewald
> ---
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index 7fd264a6d085..8e8783afd6df 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -1176,7 +1176,9 @@ static u16 cxgb_select_queue(struct net_device *dev,
> struct sk_buff *skb,
> txq = netdev_pick_tx(dev, skb, sb_dev);
> if (xfrm_offload(skb) || is_ptp_enabled(skb, dev) ||
> skb->encapsulation ||
> - cxgb4_is_ktls_skb(skb) ||
> +#if IS_ENABLED(CONFIG_CHELSIO_TLS_DEVICE)
> + cxgb4_is_ktls_skb(skb) ||
> +#endif /* CHELSIO_TLS_DEVICE */
> (proto != IPPROTO_TCP && proto != IPPROTO_UDP))
> txq = txq % pi->nqsets;
>
The tls header already tries to solve this issue, it just does it
poorly. This is a better fix:
diff --git a/include/net/tls.h b/include/net/tls.h
index baf1e99d8193..2ff3f4f7954a 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -441,11 +441,11 @@ struct sk_buff *
tls_validate_xmit_skb(struct sock *sk, struct net_device *dev,
struct sk_buff *skb);
static inline bool tls_is_sk_tx_device_offloaded(struct sock *sk)
{
-#ifdef CONFIG_SOCK_VALIDATE_XMIT
+#ifdef CONFIG_TLS_DEVICE
return sk_fullsock(sk) &&
(smp_load_acquire(>sk_validate_xmit_skb) ==
_validate_xmit_skb);
#else
return false;
Please test this and submit if it indeed solves the problem.
Thanks!
Re: [PATCH net-next v3] net/tun: Call netdev notifiers
On Mon, 16 Nov 2020 11:41:21 +0100 Martin Schiller wrote:
> Call netdev notifiers before and after changing the device type.
>
> Signed-off-by: Martin Schiller
> ---
>
> Change from v2:
> use subject_prefix 'net-next' to fix 'fixes_present' issue
>
> Change from v1:
> fix 'subject_prefix' and 'checkpatch' warnings
>
> ---
> drivers/net/tun.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 3d45d56172cb..2d9a00f41023 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -3071,9 +3071,13 @@ static long __tun_chr_ioctl(struct file *file,
> unsigned int cmd,
> "Linktype set failed because interface is
> up\n");
> ret = -EBUSY;
> } else {
> + call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE,
> + tun->dev);
This call may return an error (which has to be unpacked with
notifier_to_errno()).
> tun->dev->type = (int) arg;
> netif_info(tun, drv, tun->dev, "linktype set to %d\n",
> tun->dev->type);
> + call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
> + tun->dev);
> ret = 0;
> }
> break;
Re: [PATCH net-next] net/nfc/nci: Support NCI 2.x initial sequence
On Tue, 17 Nov 2020 14:37:59 +0900 Bongsu Jeon wrote: > implement the NCI 2.x initial sequence to support NCI 2.x NFCC. > Since NCI 2.0, CORE_RESET and CORE_INIT sequence have been changed. > If NFCEE supports NCI 2.x, then NCI 2.x initial sequence will work. > > In NCI 1.0, Initial sequence and payloads are as below: > (DH) (NFCC) > | -- CORE_RESET_CMD --> | > | <-- CORE_RESET_RSP -- | > | -- CORE_INIT_CMD --> | > | <-- CORE_INIT_RSP -- | > CORE_RESET_RSP payloads are Status, NCI version, Configuration Status. > CORE_INIT_CMD payloads are empty. > CORE_INIT_RSP payloads are Status, NFCC Features, > Number of Supported RF Interfaces, Supported RF Interface, > Max Logical Connections, Max Routing table Size, > Max Control Packet Payload Size, Max Size for Large Parameters, > Manufacturer ID, Manufacturer Specific Information. > > In NCI 2.0, Initial Sequence and Parameters are as below: > (DH) (NFCC) > | -- CORE_RESET_CMD --> | > | <-- CORE_RESET_RSP -- | > | <-- CORE_RESET_NTF -- | > | -- CORE_INIT_CMD --> | > | <-- CORE_INIT_RSP -- | > CORE_RESET_RSP payloads are Status. > CORE_RESET_NTF payloads are Reset Trigger, > Configuration Status, NCI Version, Manufacturer ID, > Manufacturer Specific Information Length, > Manufacturer Specific Information. > CORE_INIT_CMD payloads are Feature1, Feature2. > CORE_INIT_RSP payloads are Status, NFCC Features, > Max Logical Connections, Max Routing Table Size, > Max Control Packet Payload Size, > Max Data Packet Payload Size of the Static HCI Connection, > Number of Credits of the Static HCI Connection, > Max NFC-V RF Frame Size, Number of Supported RF Interfaces, > Supported RF Interfaces. > > Signed-off-by: Bongsu Jeon Please fix the following sparse (build with C=1) warning: net/nfc/nci/ntf.c:42:17: warning: cast to restricted __le32 > + __u8 status = 0; Please don't use the __u types in the normal kernel code, those are types for user space ABI.
Re: [PATCH net-next] net: add in_softirq() debug checking in napi_consume_skb()
On Wed, 18 Nov 2020 09:57:30 +0800 Yunsheng Lin wrote:
> On 2020/11/3 3:41, Jakub Kicinski wrote:
> > On Mon, 2 Nov 2020 11:14:32 +0800 Yunsheng Lin wrote:
> >> On 2020/11/1 6:38, Jakub Kicinski wrote:
> >>> On Thu, 29 Oct 2020 19:34:48 +0800 Yunsheng Lin wrote:
> >>>> The current semantic for napi_consume_skb() is that caller need
> >>>> to provide non-zero budget when calling from NAPI context, and
> >>>> breaking this semantic will cause hard to debug problem, because
> >>>> _kfree_skb_defer() need to run in atomic context in order to push
> >>>> the skb to the particular cpu' napi_alloc_cache atomically.
> >>>>
> >>>> So add a in_softirq() debug checking in napi_consume_skb() to catch
> >>>> this kind of error.
> >>>>
> >>>> Suggested-by: Eric Dumazet
> >>>> Signed-off-by: Yunsheng Lin
> >>>
> >>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >>>> index 1ba8f01..1834007 100644
> >>>> --- a/net/core/skbuff.c
> >>>> +++ b/net/core/skbuff.c
> >>>> @@ -897,6 +897,10 @@ void napi_consume_skb(struct sk_buff *skb, int
> >>>> budget)
> >>>> return;
> >>>> }
> >>>>
> >>>> +DEBUG_NET_WARN(!in_softirq(),
> >>>> + "%s is called with non-zero budget outside
> >>>> softirq context.\n",
> >>>> + __func__);
> >>>
> >>> Can't we use lockdep instead of defining our own knobs?
> >>
> >> From the first look, using the below seems better than defining our
> >> own knobs, because there is similar lockdep_assert_in_irq() checking
> >> already and lockdep_assert_in_*() is NULL-OP when CONFIG_PROVE_LOCKING
> >> is not defined.
> >>
> >>>
> >>> Like this maybe?
> >>>
> >>> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> >>> index f5594879175a..5253a167d00c 100644
> >>> --- a/include/linux/lockdep.h
> >>> +++ b/include/linux/lockdep.h
> >>> @@ -594,6 +594,14 @@ do {
> >>> \
> >>> this_cpu_read(hardirqs_enabled)));\
> >>> } while (0)
> >>>
> >>> +#define lockdep_assert_in_softirq()\
> >>> +do { \
> >>> + WARN_ON_ONCE(__lockdep_enabled && \
> >>> +(softirq_count() == 0 || \
> >>> + this_cpu_read(hardirq_context))); \
> >>>
> >>
> >> Using in_softirq() seems more obvious then using softirq_count()?
> >> And there is below comment above avoiding the using of in_softirq(), maybe
> >> that is why you use softirq_count() directly here?
> >> "softirq_count() == 0" still mean we are not in the SoftIRQ context and
> >> BH is not disabled. right? Perhap
> >> lockdep_assert_in_softirq_or_bh_disabled()
> >> is more obvious?
> >
> > Let's add Peter to the recipients to get his opinion.
> >
> > We have a per-cpu resource which is also accessed from BH (see
> > _kfree_skb_defer()).
> >
> > We're trying to come up with the correct lockdep incantation for it.
>
> Hi, Peter
> Any suggestion?
Let's just try lockdep_assert_in_softirq() and see if anyone complains.
People are more likely to respond to a patch than a discussion.
> >> /*
> >> * Are we doing bottom half or hardware interrupt processing?
> >> *
> >> * in_irq() - We're in (hard) IRQ context
> >> * in_softirq() - We have BH disabled, or are processing softirqs
> >> * in_interrupt() - We're in NMI,IRQ,SoftIRQ context or have BH disabled
> >> * in_serving_softirq() - We're in softirq context
> >> * in_nmi() - We're in NMI context
> >> * in_task() - We're in task context
> >> *
> >> * Note: due to the BH disabled confusion: in_softirq(),in_interrupt()
> >> really
> >> * should not be used in new code.
> >> */
> >>
> >>
> >> Also, is there any particular reason we do the
> >> "this_cpu_read(hardirq_context)"
> >> checking?
> >
> > Accessing BH resources from a hard IRQ context would be a bug, we may
> > have interrupted a BH, so AFAIU softirq_count() != 0, but we can nest
> > calls to _kfree_skb_defer().
>
> In that case, maybe just call lockdep_assert_in_irq() is enough?
TBH the last sentence I wrote isn't clear even to me at this point ;D
Maybe using just the macros from preempt.h - like this?
#define lockdep_assert_in_softirq()\
do { \
WARN_ON_ONCE(__lockdep_enabled && \
(!in_softirq() || in_irq() || in_nmi()) \
} while (0)
We know what we're doing so in_softirq() should be fine (famous last
words).
Re: [PATCH net-next V5] net: Variable SLAAC: SLAAC with prefixes of arbitrary length in PIO
On Wed, 18 Nov 2020 14:41:03 +0100 Dmytro Shytyi wrote:
> > > @@ -2576,9 +2590,42 @@ int addrconf_prefix_rcv_add_addr(struct
> > > u32 addr_flags, bool sllao, bool tokenized,
> > > __u32 valid_lft, u32 prefered_lft)
> > > {
> > > -struct inet6_ifaddr *ifp = ipv6_get_ifaddr(net, addr, dev, 1);
> > > +struct inet6_ifaddr *ifp = NULL;
> > > int create = 0;
> > >
> > > +if ((in6_dev->if_flags & IF_RA_VAR_PLEN) == IF_RA_VAR_PLEN &&
> > > +in6_dev->cnf.addr_gen_mode != IN6_ADDR_GEN_MODE_STABLE_PRIVACY)
> {
> > > +struct inet6_ifaddr *result = NULL;
> > > +struct inet6_ifaddr *result_base = NULL;
> > > +struct in6_addr curr_net_prfx;
> > > +struct in6_addr net_prfx;
> > > +bool prfxs_equal;
> > > +
> > > +result_base = result;
> > > +rcu_read_lock();
> > > +list_for_each_entry_rcu(ifp, _dev->addr_list, if_list) {
> > > +if (!net_eq(dev_net(ifp->idev->dev), net))
> > > +continue;
> > > +ipv6_addr_prefix_copy(_prfx, >prefix,
> pinfo->prefix_len);
> > > +ipv6_addr_prefix_copy(_net_prfx, >addr,
> pinfo->prefix_len);
> > > +prfxs_equal =
> > > +ipv6_prefix_equal(_prfx, _net_prfx,
> pinfo->prefix_len);
> > > +
> > > +if (prfxs_equal && pinfo->prefix_len == ifp->prefix_len) {
> > > +result = ifp;
> > > +in6_ifa_hold(ifp);
> > > +break;
> > > +}
> > > +}
> > > +rcu_read_unlock();
> > > +if (result_base != result)
> > > +ifp = result;
> > > +else
> > > +ifp = NULL;
> >
> > Could this be a helper of its own?
>
> Explain the thought please. It is not clear for me.
At the look of it the code under this if statement looks relatively
self-contained, and has quite a few local variables. Rather than making
the surrounding function longer would it be possible to factor it out
into a helper function?
Re: [PATCH net-next] net: add in_softirq() debug checking in napi_consume_skb()
On Wed, 18 Nov 2020 16:57:57 +0100 Peter Zijlstra wrote:
> On Wed, Nov 18, 2020 at 07:43:48AM -0800, Jakub Kicinski wrote:
>
> > TBH the last sentence I wrote isn't clear even to me at this point ;D
> >
> > Maybe using just the macros from preempt.h - like this?
> >
> > #define lockdep_assert_in_softirq()\
> > do { \
> >WARN_ON_ONCE(__lockdep_enabled && \
> > (!in_softirq() || in_irq() || in_nmi()) \
> > } while (0)
> >
> > We know what we're doing so in_softirq() should be fine (famous last
> > words).
>
> So that's not actually using any lockdep state. But if that's what you
> need, I don't have any real complaints.
Great, thanks!
The motivation was to piggy back on lockdep rather than adding a
one-off Kconfig knob for a check in the fast path in networking.
Re: [PATCH net-next] net: phy: microchip_t1: Don't set .config_aneg
On Mon, 9 Nov 2020 18:36:22 +0100 Andrew Lunn wrote: > On Mon, Nov 09, 2020 at 09:16:05AM +0800, Jisheng Zhang wrote: > > The .config_aneg in microchip_t1 is genphy_config_aneg, so it's not > > needed, because the phy core will call genphy_config_aneg() if the > > .config_aneg is NULL. > > > > Signed-off-by: Jisheng Zhang > > Reviewed-by: Andrew Lunn Applied, thanks!
Re: [PATCH v2 net-next] net: skb_vlan_untag(): don't reset transport offset if set by GRO layer
On Mon, 09 Nov 2020 23:47:23 + Alexander Lobakin wrote:
> Similar to commit fda55eca5a33f
> ("net: introduce skb_transport_header_was_set()"), avoid resetting
> transport offsets that were already set by GRO layer. This not only
> mirrors the behavior of __netif_receive_skb_core(), but also makes
> sense when it comes to UDP GSO fraglists forwarding: transport offset
> of such skbs is set only once by GRO receive callback and remains
> untouched and correct up to the xmitting driver in 1:1 case, but
> becomes junk after untagging in ingress VLAN case and breaks UDP
> GSO offload. This does not happen after this change, and all types
> of forwarding of UDP GSO fraglists work as expected.
>
> Since v1 [1]:
> - keep the code 1:1 with __netif_receive_skb_core() (Jakub).
>
> [1]
> https://lore.kernel.org/netdev/zyurwszrn7bkqsoikwqlvqhyxz18h4lhhu4nfa...@cp4-web-038.plabs.ch
>
> Signed-off-by: Alexander Lobakin
Applied, thanks!
Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock
On Tue, 10 Nov 2020 08:19:32 -0300 Thadeu Lima de Souza Cascardo wrote: > Yeah, I agree with your initial email. The patch I submitted for that fix > needs > rework, which is what I tried and failed so far. I need to get back to some > testing of my latest fix and find out what needs fixing there. > > But I am also saying that simply doing a del_timer_sync on disconnect paths > won't do, because there are non-disconnect paths where there is a CCID that we > will remove and replace and that will still trigger a timer UAF. > > So I have been working on a fix that involves a refcnt on ccid itself. But I > want to test that it really fixes the problem and I have spent most of the > time > finding out a way to trigger the timer in a race with the disconnect path. Sounds good, thanks a lot for working on this! > And that same test has showed me that this timer UAF will happen regardless of > commit 2677d20677314101293e6da0094ede7b5526d2b1, which led me into stating > that > reverting it should be done in any case. > > I think I can find some time this week to work a little further on the fix for > the time UAF.
Re: [Linux-kernel-mentees] [PATCH v3 net] rose: Fix Null pointer dereference in rose_send_frame()
On Sun, 8 Nov 2020 00:48:35 +0530 Anmol Karn wrote:
> + dev = rose_dev_get(dest);
this calls dev_hold internally, you never release that reference in
case ..neigh->dev is NULL
> + if (rose_loopback_neigh->dev && dev) {
Re: [net-next,v2,2/5] seg6: improve management of behavior attributes
On Sat, 7 Nov 2020 16:31:36 +0100 Andrea Mayer wrote:
> Depending on the attribute (i.e.: SEG6_LOCAL_SRH, SEG6_LOCAL_TABLE, etc),
> the parse() callback performs some validity checks on the provided input
> and updates the tunnel state (slwt) with the result of the parsing
> operation. However, an attribute may also need to reserve some additional
> resources (i.e.: memory or setting up an eBPF program) in the parse()
> callback to complete the parsing operation.
Looks good, a few nit picks below.
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index eba23279912d..63a82e2fdea9 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -710,6 +710,12 @@ static int cmp_nla_srh(struct seg6_local_lwt *a, struct
> seg6_local_lwt *b)
> return memcmp(a->srh, b->srh, len);
> }
>
> +static void destroy_attr_srh(struct seg6_local_lwt *slwt)
> +{
> + kfree(slwt->srh);
> + slwt->srh = NULL;
This should never be called twice, right? No need for defensive
programming then.
> +}
> +
> static int parse_nla_table(struct nlattr **attrs, struct seg6_local_lwt
> *slwt)
> {
> slwt->table = nla_get_u32(attrs[SEG6_LOCAL_TABLE]);
> @@ -901,16 +907,33 @@ static int cmp_nla_bpf(struct seg6_local_lwt *a, struct
> seg6_local_lwt *b)
> return strcmp(a->bpf.name, b->bpf.name);
> }
>
> +static void destroy_attr_bpf(struct seg6_local_lwt *slwt)
> +{
> + kfree(slwt->bpf.name);
> + if (slwt->bpf.prog)
> + bpf_prog_put(slwt->bpf.prog);
Same - why check if prog is NULL? That doesn't seem necessary if the
code is correct.
> + slwt->bpf.name = NULL;
> + slwt->bpf.prog = NULL;
> +}
> +
> struct seg6_action_param {
> int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt);
> int (*put)(struct sk_buff *skb, struct seg6_local_lwt *slwt);
> int (*cmp)(struct seg6_local_lwt *a, struct seg6_local_lwt *b);
> +
> + /* optional destroy() callback useful for releasing resources which
> + * have been previously acquired in the corresponding parse()
> + * function.
> + */
> + void (*destroy)(struct seg6_local_lwt *slwt);
> };
>
> static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
> [SEG6_LOCAL_SRH]= { .parse = parse_nla_srh,
> .put = put_nla_srh,
> - .cmp = cmp_nla_srh },
> + .cmp = cmp_nla_srh,
> + .destroy = destroy_attr_srh },
>
> [SEG6_LOCAL_TABLE] = { .parse = parse_nla_table,
> .put = put_nla_table,
> @@ -934,13 +957,68 @@ static struct seg6_action_param
> seg6_action_params[SEG6_LOCAL_MAX + 1] = {
>
> [SEG6_LOCAL_BPF]= { .parse = parse_nla_bpf,
> .put = put_nla_bpf,
> - .cmp = cmp_nla_bpf },
> + .cmp = cmp_nla_bpf,
> + .destroy = destroy_attr_bpf },
>
> };
>
> +/* call the destroy() callback (if available) for each set attribute in
> + * @parsed_attrs, starting from attribute index @start up to @end excluded.
> + */
> +static void __destroy_attrs(unsigned long parsed_attrs, int start, int end,
You always pass 0 as start, no need for that argument.
slwt and max_parsed should be the only args this function needs.
> + struct seg6_local_lwt *slwt)
> +{
> + struct seg6_action_param *param;
> + int i;
> +
> + /* Every seg6local attribute is identified by an ID which is encoded as
> + * a flag (i.e: 1 << ID) in the @parsed_attrs bitmask; such bitmask
> + * keeps track of the attributes parsed so far.
> +
> + * We scan the @parsed_attrs bitmask, starting from the attribute
> + * identified by @start up to the attribute identified by @end
> + * excluded. For each set attribute, we retrieve the corresponding
> + * destroy() callback.
> + * If the callback is not available, then we skip to the next
> + * attribute; otherwise, we call the destroy() callback.
> + */
> + for (i = start; i < end; ++i) {
> + if (!(parsed_attrs & (1 << i)))
> + continue;
> +
> + param = _action_params[i];
> +
> + if (param->destroy)
> + param->destroy(slwt);
> + }
> +}
> +
> +/* release all the resources that may have been acquired during parsing
> + * operations.
> + */
> +static void destroy_attrs(struct seg6_local_lwt *slwt)
> +{
> + struct seg6_action_desc *desc;
> + unsigned long attrs;
> +
> + desc = slwt->desc;
> + if (!desc) {
> + WARN_ONCE(1,
> + "seg6local: seg6_action_desc* for action %d is NULL",
> + slwt->action);
> + return;
> + }
Defensive programming?
> +
> + /* get the attributes for the current
Re: [net-next,v2,1/5] vrf: add mac header for tunneled packets when sniffer is attached
On Sat, 7 Nov 2020 16:31:35 +0100 Andrea Mayer wrote: > Before this patch, a sniffer attached to a VRF used as the receiving > interface of L3 tunneled packets detects them as malformed packets and > it complains about that (i.e.: tcpdump shows bogus packets). > > The reason is that a tunneled L3 packet does not carry any L2 > information and when the VRF is set as the receiving interface of a > decapsulated L3 packet, no mac header is currently set or valid. > Therefore, the purpose of this patch consists of adding a MAC header to > any packet which is directly received on the VRF interface ONLY IF: > > i) a sniffer is attached on the VRF and ii) the mac header is not set. > > In this case, the mac address of the VRF is copied in both the > destination and the source address of the ethernet header. The protocol > type is set either to IPv4 or IPv6, depending on which L3 packet is > received. > > Signed-off-by: Andrea Mayer Please keep David's review tag since you haven't changed the code.
Re: [net-next,v2,3/5] seg6: add callbacks for customizing the creation/destruction of a behavior
On Sat, 7 Nov 2020 16:31:37 +0100 Andrea Mayer wrote:
> We introduce two callbacks used for customizing the creation/destruction of
> a SRv6 behavior. Such callbacks are defined in the new struct
> seg6_local_lwtunnel_ops and hereafter we provide a brief description of
> them:
>
> - build_state(...): used for calling the custom constructor of the
>behavior during its initialization phase and after all the attributes
>have been parsed successfully;
>
> - destroy_state(...): used for calling the custom destructor of the
>behavior before it is completely destroyed.
>
> Signed-off-by: Andrea Mayer
Looks good, minor nits.
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index 63a82e2fdea9..4b0f155d641d 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -33,11 +33,23 @@
>
> struct seg6_local_lwt;
>
> +typedef int (*slwt_build_state_t)(struct seg6_local_lwt *slwt, const void
> *cfg,
> + struct netlink_ext_ack *extack);
> +typedef void (*slwt_destroy_state_t)(struct seg6_local_lwt *slwt);
Let's avoid the typedefs. Instead of taking a pointer to the op take a
pointer to the ops struct in seg6_local_lwtunnel_build_state() etc.
> +/* callbacks used for customizing the creation and destruction of a behavior
> */
> +struct seg6_local_lwtunnel_ops {
> + slwt_build_state_t build_state;
> + slwt_destroy_state_t destroy_state;
> +};
> +
> struct seg6_action_desc {
> int action;
> unsigned long attrs;
> int (*input)(struct sk_buff *skb, struct seg6_local_lwt *slwt);
> int static_headroom;
> +
> + struct seg6_local_lwtunnel_ops slwt_ops;
> };
>
> struct bpf_lwt_prog {
> @@ -1015,6 +1027,45 @@ static void destroy_attrs(struct seg6_local_lwt *slwt)
> __destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt);
> }
>
> +/* call the custom constructor of the behavior during its initialization
> phase
> + * and after that all its attributes have been parsed successfully.
> + */
> +static int
> +seg6_local_lwtunnel_build_state(struct seg6_local_lwt *slwt, const void *cfg,
> + struct netlink_ext_ack *extack)
> +{
> + slwt_build_state_t build_func;
> + struct seg6_action_desc *desc;
> + int err = 0;
> +
> + desc = slwt->desc;
> + if (!desc)
> + return -EINVAL;
This is impossible, right?
> +
> + build_func = desc->slwt_ops.build_state;
> + if (build_func)
> + err = build_func(slwt, cfg, extack);
> +
> + return err;
no need for err, just use return directly.
if (!ops->build_state)
return 0;
return ops->build_state(...);
> +}
> +
> +/* call the custom destructor of the behavior which is invoked before the
> + * tunnel is going to be destroyed.
> + */
> +static void seg6_local_lwtunnel_destroy_state(struct seg6_local_lwt *slwt)
> +{
> + slwt_destroy_state_t destroy_func;
> + struct seg6_action_desc *desc;
> +
> + desc = slwt->desc;
> + if (!desc)
> + return;
> +
> + destroy_func = desc->slwt_ops.destroy_state;
> + if (destroy_func)
> + destroy_func(slwt);
> +}
> +
> static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt
> *slwt)
> {
> struct seg6_action_param *param;
> @@ -1090,8 +1141,16 @@ static int seg6_local_build_state(struct net *net,
> struct nlattr *nla,
>
> err = parse_nla_action(tb, slwt);
> if (err < 0)
> + /* In case of error, the parse_nla_action() takes care of
> + * releasing resources which have been acquired during the
> + * processing of attributes.
> + */
that's the normal behavior for a kernel function, comment is
unnecessary IMO
> goto out_free;
>
> + err = seg6_local_lwtunnel_build_state(slwt, cfg, extack);
> + if (err < 0)
> + goto free_attrs;
The function is called destroy_attrs, call the label out_destroy_attrs,
or err_destroy_attrs.
> newts->type = LWTUNNEL_ENCAP_SEG6_LOCAL;
> newts->flags = LWTUNNEL_STATE_INPUT_REDIRECT;
> newts->headroom = slwt->headroom;
> @@ -1100,6 +1159,9 @@ static int seg6_local_build_state(struct net *net,
> struct nlattr *nla,
>
> return 0;
>
> +free_attrs:
> + destroy_attrs(slwt);
> +
no need for empty lines on error paths
> out_free:
> kfree(newts);
> return err;
> @@ -1109,6 +1171,8 @@ static void seg6_local_destroy_state(struct
> lwtunnel_state *lwt)
> {
> struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt);
>
> + seg6_local_lwtunnel_destroy_state(slwt);
> +
> destroy_attrs(slwt);
>
> return;
Re: [net-next,v2,4/5] seg6: add support for the SRv6 End.DT4 behavior
On Sat, 7 Nov 2020 16:31:38 +0100 Andrea Mayer wrote:
> SRv6 End.DT4 is defined in the SRv6 Network Programming [1].
>
> The SRv6 End.DT4 is used to implement IPv4 L3VPN use-cases in
> multi-tenants environments. It decapsulates the received packets and it
> performs IPv4 routing lookup in the routing table of the tenant.
>
> The SRv6 End.DT4 Linux implementation leverages a VRF device in order to
> force the routing lookup into the associated routing table.
How does the behavior of DT4 compare to DT6?
The implementation looks quite different.
> To make the End.DT4 work properly, it must be guaranteed that the routing
> table used for routing lookup operations is bound to one and only one
> VRF during the tunnel creation. Such constraint has to be enforced by
> enabling the VRF strict_mode sysctl parameter, i.e:
> $ sysctl -wq net.vrf.strict_mode=1.
>
> At JANOG44, LINE corporation presented their multi-tenant DC architecture
> using SRv6 [2]. In the slides, they reported that the Linux kernel is
> missing the support of SRv6 End.DT4 behavior.
>
> The iproute2 counterpart required for configuring the SRv6 End.DT4
> behavior is already implemented along with the other supported SRv6
> behaviors [3].
>
> [1] https://tools.ietf.org/html/draft-ietf-spring-srv6-network-programming
> [2]
> https://speakerdeck.com/line_developers/line-data-center-networking-with-srv6
> [3] https://patchwork.ozlabs.org/patch/799837/
>
> Signed-off-by: Andrea Mayer
> ---
> net/ipv6/seg6_local.c | 205 ++
> 1 file changed, 205 insertions(+)
>
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index 4b0f155d641d..a41074acd43e 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -57,6 +57,14 @@ struct bpf_lwt_prog {
> char *name;
> };
>
> +struct seg6_end_dt4_info {
> + struct net *net;
> + /* VRF device associated to the routing table used by the SRv6 End.DT4
> + * behavior for routing IPv4 packets.
> + */
> + int vrf_ifindex;
> +};
> +
> struct seg6_local_lwt {
> int action;
> struct ipv6_sr_hdr *srh;
> @@ -66,6 +74,7 @@ struct seg6_local_lwt {
> int iif;
> int oif;
> struct bpf_lwt_prog bpf;
> + struct seg6_end_dt4_info dt4_info;
>
> int headroom;
> struct seg6_action_desc *desc;
> @@ -413,6 +422,194 @@ static int input_action_end_dx4(struct sk_buff *skb,
> return -EINVAL;
> }
>
> +#ifdef CONFIG_NET_L3_MASTER_DEV
> +
no need for this empty line.
> +static struct net *fib6_config_get_net(const struct fib6_config *fib6_cfg)
> +{
> + const struct nl_info *nli = _cfg->fc_nlinfo;
> +
> + return nli->nl_net;
> +}
> +
> +static int seg6_end_dt4_build(struct seg6_local_lwt *slwt, const void *cfg,
> + struct netlink_ext_ack *extack)
> +{
> + struct seg6_end_dt4_info *info = >dt4_info;
> + int vrf_ifindex;
> + struct net *net;
> +
> + net = fib6_config_get_net(cfg);
> +
> + vrf_ifindex = l3mdev_ifindex_lookup_by_table_id(L3MDEV_TYPE_VRF, net,
> + slwt->table);
> + if (vrf_ifindex < 0) {
> + if (vrf_ifindex == -EPERM) {
> + NL_SET_ERR_MSG(extack,
> +"Strict mode for VRF is disabled");
> + } else if (vrf_ifindex == -ENODEV) {
> + NL_SET_ERR_MSG(extack, "No such device");
That's what -ENODEV already says.
> + } else {
> + NL_SET_ERR_MSG(extack, "Unknown error");
Useless error.
> + pr_debug("seg6local: SRv6 End.DT4 creation error=%d\n",
> + vrf_ifindex);
> + }
> +
> + return vrf_ifindex;
> + }
> +
> + info->net = net;
> + info->vrf_ifindex = vrf_ifindex;
> +
> + return 0;
> +}
> +
> +/* The SRv6 End.DT4 behavior extracts the inner (IPv4) packet and routes the
> + * IPv4 packet by looking at the configured routing table.
> + *
> + * In the SRv6 End.DT4 use case, we can receive traffic (IPv6+Segment Routing
> + * Header packets) from several interfaces and the IPv6 destination address
> (DA)
> + * is used for retrieving the specific instance of the End.DT4 behavior that
> + * should process the packets.
> + *
> + * However, the inner IPv4 packet is not really bound to any receiving
> + * interface and thus the End.DT4 sets the VRF (associated with the
> + * corresponding routing table) as the *receiving* interface.
> + * In other words, the End.DT4 processes a packet as if it has been received
> + * directly by the VRF (and not by one of its slave devices, if any).
> + * In this way, the VRF interface is used for routing the IPv4 packet in
> + * according to the routing table configured by the End.DT4 instance.
> + *
> + * This design allows you to get some interesting features like:
> + * 1) the statistics on rx packets;
> + * 2) the
Re: [PATCH v2] net: ipv4: remove redundant initialization in inet_rtm_deladdr
On Sun, 8 Nov 2020 09:05:41 +0800 Menglong Dong wrote: > The initialization for 'err' with '-EINVAL' is redundant and > can be removed, as it is updated soon. > > Changes since v1: > - Remove redundant empty line > > Signed-off-by: Menglong Dong Applied, thanks.
Re: [PATCH v2] net: atlantic: Remove unnecessary conversion to bool
On Sun, 8 Nov 2020 09:11:59 +0800 xiakaixu1...@gmail.com wrote: > From: Kaixu Xia > > The '!=' expression itself is bool, no need to convert it to bool. > Fix the following coccicheck warning: > > ./drivers/net/ethernet/aquantia/atlantic/aq_nic.c:1477:34-39: WARNING: > conversion to bool not needed here > > Reported-by: Tosk Robot > Signed-off-by: Kaixu Xia Applied.
Re: [PATCH] net: pch_gbe: remove unneeded variable retval in __pch_gbe_suspend
On Sun, 8 Nov 2020 20:13:00 +0800 xiakaixu1...@gmail.com wrote: > From: Kaixu Xia > > Fix the following coccicheck warning: > > ./drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:2415:5-11: Unneeded > variable: "retval". Return "0" on line 2435 > > Reported-by: Tosk Robot > Signed-off-by: Kaixu Xia Applied.
Re: [PATCH net v4] lan743x: correctly handle chips with internal PHY
On Sun, 8 Nov 2020 12:12:24 -0500 Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck
>
> Commit 6f197fb63850 ("lan743x: Added fixed link and RGMII support")
> assumes that chips with an internal PHY will never have a devicetree
> entry. This is incorrect: even for these chips, a devicetree entry
> can be useful e.g. to pass the mac address from bootloader to chip:
>
> {
> status = "okay";
>
> host@0 {
> reg = <0 0 0 0 0>;
>
> #address-cells = <3>;
> #size-cells = <2>;
>
> lan7430: ethernet@0 {
> /* LAN7430 with internal PHY */
> compatible = "microchip,lan743x";
> status = "okay";
> reg = <0 0 0 0 0>;
> /* filled in by bootloader */
> local-mac-address = [00 00 00 00 00 00];
> };
> };
> };
>
> If a devicetree entry is present, the driver will not attach the chip
> to its internal phy, and the chip will be non-operational.
>
> Fix by tweaking the phy connection algorithm:
> - first try to connect to a phy specified in the devicetree
> (could be 'real' phy, or just a 'fixed-link')
> - if that doesn't succeed, try to connect to an internal phy, even
> if the chip has a devnode
>
> Tested on a LAN7430 with internal PHY. I cannot test a device using
> fixed-link, as I do not have access to one.
>
> Fixes: 6f197fb63850 ("lan743x: Added fixed link and RGMII support")
> Tested-by: Sven Van Asbroeck # lan7430
> Reviewed-by: Andrew Lunn
> Signed-off-by: Sven Van Asbroeck
Applied, thank you!
Re: [PATCH] net: sched: fix misspellings using misspell-fixer tool
On Mon, 9 Nov 2020 02:02:17 -0500 menglong8.d...@gmail.com wrote: > From: Menglong Dong > > Some typos are found out by misspell-fixer tool: > > $ misspell-fixer -rnv ./net/sched/ > ./net/sched/act_api.c:686 > ./net/sched/act_bpf.c:68 > ./net/sched/cls_rsvp.h:241 > ./net/sched/em_cmp.c:44 > ./net/sched/sch_pie.c:408 > > Fix typos found by misspell-fixer. > > Signed-off-by: Menglong Dong Applied, thanks.
Re: [PATCH V3 net] net/ethernet: Fix error return when ptp_clock is ERROR
On Mon, 9 Nov 2020 10:17:52 +0800 Wang Qing wrote: > We always have to update the value of ret, otherwise the error value > may be the previous one. And ptp_clock_register() never return NULL > when PTP_1588_CLOCK enable. > > Signed-off-by: Wang Qing Please add a Fixes tag as I requested in a reply to v2. Please CC Richard on the next version, since he gave you feedback, and he's the PTP maintainer.
Re: [PATCH V2 net-next 01/11] net: hns3: add support for configuring interrupt quantity limiting
On Mon, 9 Nov 2020 11:22:29 +0800 Huazhong Tan wrote: > + if (rx_vector->tx_group.coal.ql_enable) Is this supposed to be rx_group, not tx? > + hns3_set_vector_coalesce_rx_ql(rx_vector, > +rx_vector->rx_group.coal.int_ql);
Re: [PATCH V2 net-next 05/11] net: hns3: add support for dynamic interrupt moderation
On Mon, 9 Nov 2020 11:22:33 +0800 Huazhong Tan wrote: > Add dynamic interrupt moderation support for the HNS3 driver. > > Signed-off-by: Huazhong Tan I'm slightly confused here. What does the adaptive moderation do in your driver/device if you still need DIM on top of it?
Re: [PATCH V2 net-next 09/11] net: hns3: add support for EQ/CQ mode configuration
On Mon, 9 Nov 2020 11:22:37 +0800 Huazhong Tan wrote: > For device whose version is above V3(include V3), the GL can > select EQ or CQ mode, so adds support for it. > > In CQ mode, the coalesced timer will restart upon new completion, > while in EQ mode, the timer will not restart. > > Signed-off-by: Huazhong Tan Let's see if I understand - in CQ mode the timer is restarted very time new frame gets received/transmitted? IOW for a continuous stream of frames it will only generate an interrupt once it reaches max_frames? I think that if you need such a configuration knob we should add this as an option to the official ethtool -c/-C interface, now that we have the ability to extend the netlink API.
Re: [PATCH V2 net-next 11/11] net: hns3: add debugfs support for interrupt coalesce
On Mon, 9 Nov 2020 11:22:39 +0800 Huazhong Tan wrote: > Since user may need to check the current configuration of the > interrupt coalesce, so add debugfs support for query this info, > which includes DIM profile, coalesce configuration of both software > and hardware. > > Signed-off-by: Huazhong Tan Please create a file per vector so that users can just read it instead of dumping the info into the logs. Even better we should put as much of this information as possible into the ethtool API. dim state is hardly hardware-specific.
Re: [PATCH net v5] net: Update window_clamp if SOCK_RCVBUF is set
On Tue, 10 Nov 2020 08:32:52 +0100 Eric Dumazet wrote:
> On Tue, Nov 10, 2020 at 1:16 AM Mao Wenan wrote:
> > When net.ipv4.tcp_syncookies=1 and syn flood is happened,
> > cookie_v4_check or cookie_v6_check tries to redo what
> > tcp_v4_send_synack or tcp_v6_send_synack did,
> > rsk_window_clamp will be changed if SOCK_RCVBUF is set,
> > which will make rcv_wscale is different, the client
> > still operates with initial window scale and can overshot
> > granted window, the client use the initial scale but local
> > server use new scale to advertise window value, and session
> > work abnormally.
> >
> > Fixes: e88c64f0a425 ("tcp: allow effective reduction of TCP's rcv-buffer
> > via setsockopt")
> > Signed-off-by: Mao Wenan
>
> Signed-off-by: Eric Dumazet
Applied, thanks!
Re: [PATCH][next] net: dsa: fix unintended sign extension on a u16 left shift
On Mon, 09 Nov 2020 14:27:52 +0100 Kurt Kanzenbach wrote:
> On Mon Nov 09 2020, Colin King wrote:
> > From: Colin Ian King
> >
> > The left shift of u16 variable high is promoted to the type int and
> > then sign extended to a 64 bit u64 value. If the top bit of high is
> > set then the upper 32 bits of the result end up being set by the
> > sign extension. Fix this by explicitly casting the value in high to
> > a u64 before left shifting by 16 places.
> >
> > Also, remove the initialisation of variable value to 0 at the start
> > of each loop iteration as the value is never read and hence the
> > assignment it is redundant.
> >
> > Addresses-Coverity: ("Unintended sign extension")
> > Fixes: e4b27ebc780f ("net: dsa: Add DSA driver for Hirschmann Hellcreek
> > switches")
> > Signed-off-by: Colin Ian King
>
> Reviewed-by: Kurt Kanzenbach
Applied, thanks!
Re: [PATCH net v1] lan743x: fix "BUG: invalid wait context" when setting rx mode
On Mon, 9 Nov 2020 15:38:28 -0500 Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck
>
> In the net core, the struct net_device_ops -> ndo_set_rx_mode()
> callback is called with the dev->addr_list_lock spinlock held.
>
> However, this driver's ndo_set_rx_mode callback eventually calls
> lan743x_dp_write(), which acquires a mutex. Mutex acquisition
> may sleep, and this is not allowed when holding a spinlock.
>
> Fix by removing the dp_lock mutex entirely. Its purpose is to
> prevent concurrent accesses to the data port. No concurrent
> accesses are possible, because the dev->addr_list_lock
> spinlock in the core only lets through one thread at a time.
Please remember about fixes tags, I've added:
Fixes: 23f0703c125b ("lan743x: Add main source files for new lan743x driver")
here. No idea what this lock was trying to protect from the start.
Applied, thanks!
Re: [PATCH v8 6/6] Bluetooth: Add toggle to switch off interleave scan
On Tue, 10 Nov 2020 18:17:55 +0800 Howard Chung wrote: > This patch add a configurable parameter to switch off the interleave > scan feature. > > Reviewed-by: Alain Michaud > Signed-off-by: Howard Chung Please don't add new sparse warnings: net/bluetooth/mgmt_config.c:315:63: warning: cast to restricted __le16
Re: [PATCH] net: phy: mscc: fix excluded_middle.cocci warnings
On Sun, 15 Nov 2020 19:37:59 +0100 (CET) Julia Lawall wrote:
> From: kernel test robot
>
> Condition !A || A && B is equivalent to !A || B.
>
> Generated by: scripts/coccinelle/misc/excluded_middle.cocci
>
> Fixes: b76f0ea01312 ("coccinelle: misc: add excluded_middle.cocci script")
> CC: Denis Efremov
> Reported-by: kernel test robot
> Signed-off-by: kernel test robot
> Signed-off-by: Julia Lawall
cc netdev plz!
Re: [PATCH net-next v4] net: linux/skbuff.h: combine SKB_EXTENSIONS + KCOV handling
On Mon, 16 Nov 2020 15:31:21 +0100 Florian Westphal wrote:
> > > @@ -4151,12 +4150,11 @@ enum skb_ext_id {
> > > #if IS_ENABLED(CONFIG_MPTCP)
> > > SKB_EXT_MPTCP,
> > > #endif
> > > -#if IS_ENABLED(CONFIG_KCOV)
> > > SKB_EXT_KCOV_HANDLE,
> > > -#endif
> >
> > I don't think we should remove this #ifdef: the number of extensions are
> > currently limited to 8, we might not want to always have KCOV there even if
> > we don't want it. I think adding items in this enum only when needed was the
> > intension of Florian (+cc) when creating these SKB extensions.
> > Also, this will increase a tiny bit some structures, see "struct
> > skb_ext()".
>
> Yes, I would also prefer to retrain the ifdef.
>
> Another reason was to make sure that any skb_ext_add(..., MY_EXT) gives
> a compile error if the extension is not enabled.
Oh well, sorry for taking you down the wrong path Randy!
Re: [PATCH net-next] r8153_ecm: avoid to be prior to r8152 driver
On Mon, 16 Nov 2020 10:18:13 +0100 Marek Szyprowski wrote:
> On 16.11.2020 07:52, Hayes Wang wrote:
> > Avoid r8153_ecm is compiled as built-in, if r8152 driver is compiled
> > as modules. Otherwise, the r8153_ecm would be used, even though the
> > device is supported by r8152 driver.
> >
> > Fixes: c1aedf015ebd ("net/usb/r8153_ecm: support ECM mode for RTL8153")
> > Reported-by: Marek Szyprowski
> > Signed-off-by: Hayes Wang
>
> Yes, this fixes this issue, although I would prefer a separate Kconfig
> entry for r8153_ecm with proper dependencies instead of this ifdefs in
> Makefile.
Agreed, this is what dependency resolution is for.
Let's just make this a separate Kconfig entry.
Re: [PATCH net] net: phy: smsc: add missed clk_disable_unprepare in smsc_phy_probe()
On Mon, 16 Nov 2020 10:26:07 +0100 Marco Felsch wrote: > > > The code right above looks highly questionable as well: > > > > > > priv->refclk = clk_get_optional(dev, NULL); > > > if (IS_ERR(priv->refclk)) > > > dev_err_probe(dev, PTR_ERR(priv->refclk), "Failed to > > > request clock\n"); > > > > > > ret = clk_prepare_enable(priv->refclk); > > > if (ret) > > > return ret; > > > > > > I don't think clk_prepare_enable() will be too happy to see an error > > > pointer. This should probably be: > > > > > > priv->refclk = clk_get_optional(dev, NULL); > > > if (IS_ERR(priv->refclk)) > > > return dev_err_probe(dev, PTR_ERR(priv->refclk), > > > "Failed to request clock\n"); > > > > Right, especially if EPROBE_DEFER must be returned because the clock > > provider is not ready yet, we should have a chance to do that. > > damn.. I missed the return here. Thanks for covering that. Should I send > a fix or did you do that already? Please do, I don't see any fix for this issue in patchwork right now.
Re: [PATCH v6 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On Mon, 16 Nov 2020 16:33:26 +0100 Lukasz Stelmach wrote:
> > Please make sure the new code builds cleanly with W=1 C=1
> >
> > ../drivers/net/ethernet/asix/ax88796c_ioctl.c:221:19: warning: initialized
> > field overwritten [-Woverride-init]
> > 221 | .get_msglevel = ax88796c_ethtool_getmsglevel,
> > | ^~~~
> > ../drivers/net/ethernet/asix/ax88796c_ioctl.c:221:19: note: (near
> > initialization for ‘ax88796c_ethtool_ops.get_msglevel’)
> > ../drivers/net/ethernet/asix/ax88796c_ioctl.c:222:19: warning: initialized
> > field overwritten [-Woverride-init]
> > 222 | .set_msglevel = ax88796c_ethtool_setmsglevel,
> > | ^~~~
> > ../drivers/net/ethernet/asix/ax88796c_ioctl.c:222:19: note: (near
> > initialization for ‘ax88796c_ethtool_ops.set_msglevel’)
> > In file included from ../drivers/net/ethernet/asix/ax88796c_main.h:15,
> > from ../drivers/net/ethernet/asix/ax88796c_ioctl.c:16:
> > ../drivers/net/ethernet/asix/ax88796c_spi.h:25:17: warning: ‘tx_cmd_buf’
> > defined but not used [-Wunused-const-variable=]
> >25 | static const u8 tx_cmd_buf[4] = {AX_SPICMD_WRITE_TXQ, 0xFF, 0xFF,
> > 0xFF};
> > | ^~
>
> I fixed the problems reported by W=1, but I am afraid I can't do
> anything about C=1. sparse is is reporting
>
> [...]
> ./include/linux/atomic-fallback.h:266:16: error: Expected ; at end
> ofdeclaration
> ./include/linux/atomic-fallback.h:266:16: error: got ret
> ./include/linux/atomic-fallback.h:267:1: error: Expected ; at the end of type
> declaration
> ./include/linux/atomic-fallback.h:267:1: error: too many errors
> Segmentation fault
>
> in the headers and gets killed.
That's fine, sparse is wobbly at times, thanks!
Re: [PATCH V3 net-next 06/10] net: hns3: add ethtool priv-flag for DIM
On Mon, 16 Nov 2020 16:41:45 +0800 tanhuazhong wrote: > On 2020/11/15 2:54, Jakub Kicinski wrote: > > On Thu, 12 Nov 2020 11:33:14 +0800 Huazhong Tan wrote: > >> Add a control private flag in ethtool for enable/disable > >> DIM feature. > >> > >> Signed-off-by: Huazhong Tan > > > > Please work on a common ethtool API for the configuration instead of > > using private flags. > > > > Private flags were overused because the old IOCTL-based ethtool was > > hard to extend, but we have a netlink API now. > > > > For example here you're making a choice between device and DIM > > implementation of IRQ coalescing. You can add a new netlink attribute > > to the ETHTOOL_MSG_COALESCE_GET/ETHTOOL_MSG_COALESCE_SET commands which > > controls the type of adaptive coalescing (if enabled). > > The device's implementation of IRQ coalescing will be removed, if DIM > works ok for a long time. So could this private flag for DIM be > uptreamed as a transition scheme? And adding a new netlink attrtibute to > controls the type of adaptive coalescing seems useless for other drivers. The information whether the adaptive behavior is implemented by DIM, device or custom driver implementation is useful regardless. Right now users only see "adaptive" and don't know what implements it - device, DIM or is it a custom implementation in the driver. So regardless if you remove the priv flag, the "read"/"get" side of the information will still be useful. Besides you have another priv flag in this set that needs to be converted to a generic attribute - the one for the timer reset behavior.
Re: [PATCH net-next] MAINTAINERS: Add Martin Schiller as a maintainer for the X.25 stack
On Mon, 16 Nov 2020 10:01:20 +0100 Martin Schiller wrote: > On 2020-11-14 12:10, Xie He wrote: > > Martin Schiller is an active developer and reviewer for the X.25 code. > > His company is providing products based on the Linux X.25 stack. > > So he is a good candidate for maintainers of the X.25 code. > > > > The original maintainer of the X.25 network layer (Andrew Hendry) has > > not sent any email to the netdev mail list since 2013. So he is > > probably > > inactive now. > > > > Cc: Martin Schiller > > Cc: Andrew Hendry > > Signed-off-by: Xie He > > Acked-by: Martin Schiller Applied to net, thanks everyone!
Re: linux-next: Tree for Nov 16 (net/core/stream.o)
On Mon, 16 Nov 2020 09:46:21 -0800 Randy Dunlap wrote:
> On 11/15/20 10:59 PM, Stephen Rothwell wrote:
> > Hi all,
> >
> > Changes since 20201113:
>
> on x86_64:
>
> # CONFIG_INET is not set
>
> ld: net/core/stream.o: in function `sk_stream_write_space':
> stream.c:(.text+0x68): undefined reference to `tcp_stream_memory_free'
> ld: stream.c:(.text+0x80): undefined reference to `tcp_stream_memory_free'
> ld: net/core/stream.o: in function `sk_stream_wait_memory':
> stream.c:(.text+0x5b3): undefined reference to `tcp_stream_memory_free'
> ld: stream.c:(.text+0x5c8): undefined reference to `tcp_stream_memory_free'
> ld: stream.c:(.text+0x6f8): undefined reference to `tcp_stream_memory_free'
> ld: net/core/stream.o:stream.c:(.text+0x70d): more undefined references to
> `tcp_stream_memory_free' follow
Must be: d3cd4924e385 ("tcp: uninline tcp_stream_memory_free()")
Re: [PATCH v4 bpf-next 3/5] kbuild: build kernel module BTFs if BTF is enabled and pahole supports it
On Mon, 16 Nov 2020 12:34:17 -0800 Andrii Nakryiko wrote:
> > This change, commit 5f9ae91f7c0d ("kbuild: Build kernel module BTFs if BTF
> > is enabled and pahole
> > supports it") currently in net-next, linux-next, etc. breaks the use-case
> > of compiling only a specific
> > kernel module (both in-tree and out-of-tree, e.g. 'make
> > M=drivers/net/ethernet/intel/ice') after
> > first doing a 'make modules_prepare'. Previously, that use-case would
> > result in a warning noting
> > "Symbol info of vmlinux is missing. Unresolved symbol check will be
> > entirely skipped" but now it
> > errors out after noting "No rule to make target 'vmlinux', needed by
> > '<...>.ko'. Stop."
> >
> > Is that intentional?
>
> I wasn't aware of such a use pattern, so definitely not intentional.
> But vmlinux is absolutely necessary to generate the module BTF. So I'm
> wondering what's the proper fix here? Leave it as is (that error
> message is actually surprisingly descriptive, btw)? Force vmlinux
> build? Or skip BTF generation for that module?
For an external out-of-tree module there is no guarantee that vmlinux
will even be on the system, no? So only the last option can work in
that case.
Re: [PATCH v1 net-next] net: dsa: qca: ar9331: add ethtool stats support
On Sun, 15 Nov 2020 08:35:33 +0100 Oleksij Rempel wrote:
> +static const struct ar9331_mib_desc ar9331_mib[] = {
> + MIB_DESC(1, 0x00, "RxBroad"),
> + MIB_DESC(1, 0x04, "RxPause"),
> + MIB_DESC(1, 0x08, "RxMulti"),
> + MIB_DESC(1, 0x0c, "RxFcsErr"),
> + MIB_DESC(1, 0x10, "RxAlignErr"),
> + MIB_DESC(1, 0x14, "RxRunt"),
> + MIB_DESC(1, 0x18, "RxFragment"),
> + MIB_DESC(1, 0x1c, "Rx64Byte"),
> + MIB_DESC(1, 0x20, "Rx128Byte"),
> + MIB_DESC(1, 0x24, "Rx256Byte"),
> + MIB_DESC(1, 0x28, "Rx512Byte"),
> + MIB_DESC(1, 0x2c, "Rx1024Byte"),
> + MIB_DESC(1, 0x30, "Rx1518Byte"),
> + MIB_DESC(1, 0x34, "RxMaxByte"),
> + MIB_DESC(1, 0x38, "RxTooLong"),
> + MIB_DESC(2, 0x3c, "RxGoodByte"),
> + MIB_DESC(2, 0x44, "RxBadByte"),
> + MIB_DESC(1, 0x4c, "RxOverFlow"),
> + MIB_DESC(1, 0x50, "Filtered"),
> + MIB_DESC(1, 0x54, "TxBroad"),
> + MIB_DESC(1, 0x58, "TxPause"),
> + MIB_DESC(1, 0x5c, "TxMulti"),
> + MIB_DESC(1, 0x60, "TxUnderRun"),
> + MIB_DESC(1, 0x64, "Tx64Byte"),
> + MIB_DESC(1, 0x68, "Tx128Byte"),
> + MIB_DESC(1, 0x6c, "Tx256Byte"),
> + MIB_DESC(1, 0x70, "Tx512Byte"),
> + MIB_DESC(1, 0x74, "Tx1024Byte"),
> + MIB_DESC(1, 0x78, "Tx1518Byte"),
> + MIB_DESC(1, 0x7c, "TxMaxByte"),
> + MIB_DESC(1, 0x80, "TxOverSize"),
> + MIB_DESC(2, 0x84, "TxByte"),
> + MIB_DESC(1, 0x8c, "TxCollision"),
> + MIB_DESC(1, 0x90, "TxAbortCol"),
> + MIB_DESC(1, 0x94, "TxMultiCol"),
> + MIB_DESC(1, 0x98, "TxSingleCol"),
> + MIB_DESC(1, 0x9c, "TxExcDefer"),
> + MIB_DESC(1, 0xa0, "TxDefer"),
> + MIB_DESC(1, 0xa4, "TxLateCol"),
> +};
You must expose relevant statistics via the normal get_stats64 NDO
before you start dumping free form stuff in ethtool -S.
Re: [PATCH v1 net-next] net: dsa: qca: ar9331: add ethtool stats support
On Tue, 17 Nov 2020 00:21:46 +0200 Vladimir Oltean wrote: > On Mon, Nov 16, 2020 at 01:34:53PM -0800, Jakub Kicinski wrote: > > You must expose relevant statistics via the normal get_stats64 NDO > > before you start dumping free form stuff in ethtool -S. > > Completely agree on the point, Jakub, but to be honest we don't give him > that possibility within the DSA framework today, see .ndo_get_stats64 in > net/dsa/slave.c which returns the generic dev_get_tstats64 implementation, > and not something that hooks into the hardware counters, or into the > driver at all, for that matter. Simple matter of coding, right? I don't see a problem. Also I only mentioned .ndo_get_stats64, but now we also have stats in ethtool->get_pause_stats. > But it's good that you raise the point, I was thinking too that we > should do better in terms of keeping the software counters in sync with > the hardware. But what would be a good reference for keeping statistics > on an offloaded interface? Is it ok to just populate the netdev counters > based on the hardware statistics? IIRC the stats on the interface should be a sum of forwarded in software and in hardware. Which in practice means interface HW stats are okay, given eventually both forwarding types end up in the HW interface (/MAC block). > And what about the statistics gathered > today in software, could we return them maybe via something like ifstat > --extended=cpu_hits? Yup, exactly, that's what --extended=cpu_hits is for.
Re: [PATCH v1 net-next] net: dsa: qca: ar9331: add ethtool stats support
On Tue, 17 Nov 2020 01:00:53 +0200 Vladimir Oltean wrote: > On Mon, Nov 16, 2020 at 02:35:44PM -0800, Jakub Kicinski wrote: > > On Tue, 17 Nov 2020 00:21:46 +0200 Vladimir Oltean wrote: > > > On Mon, Nov 16, 2020 at 01:34:53PM -0800, Jakub Kicinski wrote: > > > > You must expose relevant statistics via the normal get_stats64 NDO > > > > before you start dumping free form stuff in ethtool -S. > > > > > > Completely agree on the point, Jakub, but to be honest we don't give him > > > that possibility within the DSA framework today, see .ndo_get_stats64 in > > > net/dsa/slave.c which returns the generic dev_get_tstats64 implementation, > > > and not something that hooks into the hardware counters, or into the > > > driver at all, for that matter. > > > > Simple matter of coding, right? I don't see a problem. > > > > Also I only mentioned .ndo_get_stats64, but now we also have stats in > > ethtool->get_pause_stats. > > Yes, sure we can do that. The pause stats and packet counter ops would > need to be exposed to the drivers by DSA first, though. Not sure if this > is something you expect Oleksij to do or if we could pick that up separately > afterwards. Well, I feel like unless we draw the line nobody will have the incentive to do the work. I don't mind if it's Oleksij or anyone else doing the plumbing work, but the task itself seems rather trivial. > > > But it's good that you raise the point, I was thinking too that we > > > should do better in terms of keeping the software counters in sync with > > > the hardware. But what would be a good reference for keeping statistics > > > on an offloaded interface? Is it ok to just populate the netdev counters > > > based on the hardware statistics? > > > > IIRC the stats on the interface should be a sum of forwarded in software > > and in hardware. Which in practice means interface HW stats are okay, > > given eventually both forwarding types end up in the HW interface > > (/MAC block). > > A sum? Wouldn't that count the packets sent/received by the stack twice? Note that I said _forwarded_. Frames are either forwarded by the HW or SW (former never hit the CPU, while the latter do hit the CPU or originate from it).
Re: [PATCH net] net: stmmac: dwmac-intel-plat: fix error return code in intel_eth_plat_probe()
On Fri, 13 Nov 2020 14:34:03 +0800 Zhang Changzhong wrote:
> Fix to return a negative error code from the error handling
> case instead of 0, as done elsewhere in this function.
>
> Fixes: 9efc9b2b04c7 ("net: stmmac: Add dwmac-intel-plat for GBE driver")
> Reported-by: Hulk Robot
> Signed-off-by: Zhang Changzhong
> ---
> drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> index f61cb99..82b1c7a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> @@ -113,8 +113,10 @@ static int intel_eth_plat_probe(struct platform_device
> *pdev)
> /* Enable TX clock */
> if (dwmac->data->tx_clk_en) {
> dwmac->tx_clk = devm_clk_get(>dev, "tx_clk");
> - if (IS_ERR(dwmac->tx_clk))
> + if (IS_ERR(dwmac->tx_clk)) {
> + ret = PTR_ERR(dwmac->tx_clk);
> goto err_remove_config_dt;
> + }
>
> clk_prepare_enable(dwmac->tx_clk);
Someone should take the look at the error handling later in this
function. It's not checking ret from clk_prepare_enable(), and even tho
top half of this function uses goto, the rest suddenly starts doing
direct returns :S
clk_prepare_enable(dwmac->tx_clk);
/* Check and configure TX clock rate */
rate = clk_get_rate(dwmac->tx_clk);
if (dwmac->data->tx_clk_rate &&
rate != dwmac->data->tx_clk_rate) {
rate = dwmac->data->tx_clk_rate;
ret = clk_set_rate(dwmac->tx_clk, rate);
if (ret) {
dev_err(>dev,
"Failed to set tx_clk\n");
return ret;
}
}
}
/* Check and configure PTP ref clock rate */
rate = clk_get_rate(plat_dat->clk_ptp_ref);
if (dwmac->data->ptp_ref_clk_rate &&
rate != dwmac->data->ptp_ref_clk_rate) {
rate = dwmac->data->ptp_ref_clk_rate;
ret = clk_set_rate(plat_dat->clk_ptp_ref, rate);
if (ret) {
dev_err(>dev,
"Failed to set clk_ptp_ref\n");
return ret;
}
}
}
Re: [PATCH net] net: Have netpoll bring-up DSA management interface
On Mon, 16 Nov 2020 15:20:37 -0800 Florian Fainelli wrote:
> >> Florian for you patch specifially - can't we use
> >> netdev_for_each_lower_dev()?
> >
> > Looks like I forgot to respond here, yes we could do that because we do
> > call netdev_upper_dev_link() in net/dsa/slave.c. Let me re-post with
> > that done.
>
> I remember now there was a reason for me to "open code" this, and this
> is because since the patch is intended to be a bug fix, I wanted it to
> be independent from: 2f1e8ea726e9 ("net: dsa: link interfaces with the
> DSA master to get rid of lockdep warnings")
>
> which we would be depending on and is only two-ish releases away. Let me
> know if you prefer different fixes for different branches.
Ah, makes sense, we can apply this and then clean up in net-next. Just
mention that in the commit message. FWIW you'll need to repost anyway
once the discussion with Vladimir is resolved, because this is in the
old patchwork instance :)
Re: [PATCH v1 net-next] net: dsa: qca: ar9331: add ethtool stats support
On Tue, 17 Nov 2020 01:27:31 +0200 Vladimir Oltean wrote: > > Note that I said _forwarded_. Frames are either forwarded by the HW or > > SW (former never hit the CPU, while the latter do hit the CPU or > > originate from it). > > Ah, you were just thinking out loud, I really did not understand what > you meant by the separation between "forwarded in software" and > "forwarded in hardware". > Yes, the hardware typically only gives us MAC-level counters anyway. > Another way to look at it is that the number of packets forwarded in > hardware from a given port are equal to the total number of RX packets > on that MAC minus the packets seen by the CPU coming from that port. > So all in all, it's the MAC-level counters we should expose in > .ndo_get_stats64, I'm glad you agree. As for the error packets, I > suppose that would be a driver-specific aggregate. Yup, sorry about the confusion, I was only working on those stats with SDN/OvS/tc hardware, which explains the slight difference in terminology.
Re: [PATCH v1 net-next] net: dsa: qca: ar9331: add ethtool stats support
On Mon, 16 Nov 2020 15:30:39 -0800 Florian Fainelli wrote: > > What about RMON/RFC2819 style etherStatsPkts65to127Octets? We have a > > number of switches supporting that style of counters, including the one > > that Oleksij is adding support for, apparently (but not all switches > > though). I suppose your M.O. is that anything standardizable is welcome > > to be standardized via rtnetlink? > > > > Andrew, Florian, any opinions here? > > Settling on RMON/RFC2819 statistics would work for me, and hopefully is > not too hard to get the various drivers converted to. That would be grate! For RMON stats you may have slightly more legwork to do on ethtool side, since IIRC the plumbing for stats over netlink is (intentionally?) not there until we settle on how to handle standardize-able counters. I've only done pause stats 'cause those and FEC are the primary HW stats my employer cares about :) I'm sure people would actually find use for the RMON stats once they get standardized tho! > With respect to Oleksij's patch, I would tend to accept it so we > actually have more visibility into what standardized counters are > available across switch drivers. For a while now we have been pushing back on stats which have a proper interface to be added to ethtool -S. So I'd expect the list of stats exposed via ethtool will end up being shorter than in this patch.
Re: [PATCH net] net: Have netpoll bring-up DSA management interface
On Tue, 17 Nov 2020 01:54:05 +0200 Vladimir Oltean wrote: > Yeah, I think Florian just wants netconsole to work in stable kernels, > which is a fair point. As for my 16-line patch that I suggested to him > in the initial reply, what do you think, would that be a "stable" > candidate? We would be introducing a fairly user-visible change > (removing one step that is mentioned as necessary in the documentation), > do you think it would benefit the users more to also have that behavior > change backported to all LTS kernels, or just keep it as something new > for v5.11? Yeah, I'd think that's too risky for a backport.
Re: [PATCH v1 net-next] net: dsa: qca: ar9331: add ethtool stats support
On Tue, 17 Nov 2020 02:10:05 +0200 Vladimir Oltean wrote: > On Mon, Nov 16, 2020 at 04:02:13PM -0800, Jakub Kicinski wrote: > > For a while now we have been pushing back on stats which have a proper > > interface to be added to ethtool -S. So I'd expect the list of stats > > exposed via ethtool will end up being shorter than in this patch. > > Hmm, not sure if that's ever going to be the case. Even with drivers > that are going to expose standardized forms of counters, I'm not sure > it's going to be nice to remove them from ethtool -S. Not remove, but also not accept adding them to new drivers. > Testing teams all > over the world have scripts that grep for those. Unfortunately I think > ethtool -S will always remain a dumping ground of hell, and the place > where you search for a counter based on its name from the hardware block > guide as opposed to its standardized name/function. And that might mean > there's no reason to not accept Oleksij's patch right away. Even if he > might volunteer to actually follow up with a patch where he exposes the > .ndo_get_stats64 from DSA towards drivers, as well as implements > .ndo_has_offload_stats and .ndo_get_offload_stats within DSA, that will > most likely be done as separate patches to this one, and not change in > any way how this patch looks.
Re: [PATCH net-next 1/5] ptp: clockmatrix: bug fix for idtcm_strverscmp
On Mon, 16 Nov 2020 14:27:26 -0500 min.li...@renesas.com wrote:
> From: Min Li
>
> Feed kstrtou8 with NULL terminated string.
Is this a fix? Does it need to be backported to stable?
Also please add a cover letter for the series describing the overall
goal of this work, what testing has been done etc.
> drivers/ptp/ptp_clockmatrix.c | 52
> +++
> 1 file changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
> index e020faf..bf2be50 100644
> --- a/drivers/ptp/ptp_clockmatrix.c
> +++ b/drivers/ptp/ptp_clockmatrix.c
> @@ -103,42 +103,66 @@ static int timespec_to_char_array(struct timespec64
> const *ts,
> return 0;
> }
>
> -static int idtcm_strverscmp(const char *ver1, const char *ver2)
> +static int idtcm_strverscmp(const char *version1, const char *version2)
> {
> u8 num1;
> u8 num2;
> int result = 0;
> + char ver1[16];
> + char ver2[16];
> + char *cur1;
> + char *cur2;
> + char *next1;
> + char *next2;
> +
> + strncpy(ver1, version1, 16);
> + strncpy(ver2, version2, 16);
This patch triggers the following warning on a 32bit build:
In file included from ../arch/x86/include/asm/page_32.h:35,
from ../arch/x86/include/asm/page.h:14,
from ../arch/x86/include/asm/thread_info.h:12,
from ../include/linux/thread_info.h:38,
from ../arch/x86/include/asm/preempt.h:7,
from ../include/linux/preempt.h:78,
from ../include/linux/spinlock.h:51,
from ../include/linux/mmzone.h:8,
from ../include/linux/gfp.h:6,
from ../include/linux/firmware.h:7,
from ../drivers/ptp/ptp_clockmatrix.c:8:
In function ‘strncpy’,
inlined from ‘idtcm_strverscmp.constprop’ at
../drivers/ptp/ptp_clockmatrix.c:118:2:
../include/linux/string.h:290:30: warning: ‘__builtin_strncpy’ specified bound
16 equals destination size [-Wstringop-truncation]
290 | #define __underlying_strncpy __builtin_strncpy
| ^
../include/linux/string.h:300:9: note: in expansion of macro
‘__underlying_strncpy’
300 | return __underlying_strncpy(p, q, size);
| ^~~~
Re: [PATCH v2 net-next 1/4] dt-bindings: net: nfc: s3fwrn5: Support a UART interface
On Mon, 30 Nov 2020 21:00:27 +0900 Bongsu jeon wrote: > From: Bongsu Jeon > > Since S3FWRN82 NFC Chip, The UART interface can be used. > S3FWRN82 supports I2C and UART interface. > > Signed-off-by: Bongsu Jeon All patches in the series should have the same version. If the patch was not changes in the given repost you can add: v3: - no change Or just not mention the version in the changelog. It's also best to provide a cover letter describing what the series does as a whole for series with more than 2 patches.
Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
On Sat, 21 Nov 2020 18:09:41 +0200 Ido Schimmel wrote: > + Florian > > On Thu, Oct 29, 2020 at 05:36:19PM +, Aleksandr Nogikh wrote: > > From: Aleksandr Nogikh > > > > Remote KCOV coverage collection enables coverage-guided fuzzing of the > > code that is not reachable during normal system call execution. It is > > especially helpful for fuzzing networking subsystems, where it is > > common to perform packet handling in separate work queues even for the > > packets that originated directly from the user space. > > > > Enable coverage-guided frame injection by adding kcov remote handle to > > skb extensions. Default initialization in __alloc_skb and > > __build_skb_around ensures that no socket buffer that was generated > > during a system call will be missed. > > > > Code that is of interest and that performs packet processing should be > > annotated with kcov_remote_start()/kcov_remote_stop(). > > > > An alternative approach is to determine kcov_handle solely on the > > basis of the device/interface that received the specific socket > > buffer. However, in this case it would be impossible to distinguish > > between packets that originated during normal background network > > processes or were intentionally injected from the user space. > > > > Signed-off-by: Aleksandr Nogikh > > Acked-by: Willem de Bruijn > > [...] > > > @@ -249,6 +249,9 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t > > gfp_mask, > > > > fclones->skb2.fclone = SKB_FCLONE_CLONE; > > } > > + > > + skb_set_kcov_handle(skb, kcov_common_handle()); > > Hi, > > This causes skb extensions to be allocated for the allocated skb, but > there are instances that blindly overwrite 'skb->extensions' by invoking > skb_copy_header() after __alloc_skb(). For example, skb_copy(), > __pskb_copy_fclone() and skb_copy_expand(). This results in the skb > extensions being leaked [1]. > > One possible solution is to try to patch all these instances with > skb_ext_put() before skb_copy_header(). > > Another possible solution is to convert skb_copy_header() to use > skb_ext_copy() instead of __skb_ext_copy(). It will first drop the > reference on the skb extensions of the new skb, but it assumes that > 'skb->active_extensions' is valid. This is not the case in the > skb_clone() path so we should probably zero this field in __skb_clone(). > > Other suggestions? Looking at the patch from Marco to move back to a field now I'm wondering how you run into this, Ido :D AFAIU the extension is only added if process as a KCOV handle. Are you using KCOV? > [1] > BUG: memory leak > unreferenced object 0x888027f9a490 (size 16): > comm "syz-executor.0", pid 1155, jiffies 4295996826 (age 66.927s) > hex dump (first 16 bytes): > 01 00 00 00 01 02 6b 6b 01 00 00 00 00 00 00 00 ..kk > backtrace: > [<05a5f2c4>] kmemleak_alloc_recursive include/linux/kmemleak.h:43 > [inline] > [<05a5f2c4>] slab_post_alloc_hook mm/slab.h:528 [inline] > [<05a5f2c4>] slab_alloc_node mm/slub.c:2891 [inline] > [<05a5f2c4>] slab_alloc mm/slub.c:2899 [inline] > [<05a5f2c4>] kmem_cache_alloc+0x173/0x800 mm/slub.c:2904 > [] __skb_ext_alloc+0x22/0x90 net/core/skbuff.c:6173 > [<0de35e81>] skb_ext_add+0x230/0x4a0 net/core/skbuff.c:6268 > [<3b7efba4>] skb_set_kcov_handle include/linux/skbuff.h:4622 > [inline] > [<3b7efba4>] skb_set_kcov_handle include/linux/skbuff.h:4612 > [inline] > [<3b7efba4>] __alloc_skb+0x47f/0x6a0 net/core/skbuff.c:253 > [<7f789b23>] skb_copy+0x151/0x310 net/core/skbuff.c:1512 > [<1ce26864>] mlxsw_emad_transmit+0x4e/0x620 > drivers/net/ethernet/mellanox/mlxsw/core.c:585 > [<5c732123>] mlxsw_emad_reg_access > drivers/net/ethernet/mellanox/mlxsw/core.c:829 [inline] > [<5c732123>] mlxsw_core_reg_access_emad+0xda8/0x1770 > drivers/net/ethernet/mellanox/mlxsw/core.c:2408 > [ ] mlxsw_core_reg_access+0x101/0x7f0 > drivers/net/ethernet/mellanox/mlxsw/core.c:2583 > [<7c47f30f>] mlxsw_reg_write+0x30/0x40 > drivers/net/ethernet/mellanox/mlxsw/core.c:2603 > [<675e3fc7>] mlxsw_sp_port_admin_status_set+0x8a7/0x980 > drivers/net/ethernet/mellanox/mlxsw/spectrum.c:300 > [ ] mlxsw_sp_port_stop+0x63/0x70 > drivers/net/ethernet/mellanox/mlxsw/spectrum.c:537 > [ ] __dev_close_many+0x1c7/0x300 net/core/dev.c:1607 > [<628c5987>] __dev_close net/core/dev.c:1619 [inline] > [<628c5987>] __dev_change_flags+0x2b9/0x710 net/core/dev.c:8421 > [<8cc810c6>] dev_change_flags+0x97/0x170 net/core/dev.c:8494 > [<53274a78>] do_setlink+0xa5b/0x3b80 net/core/rtnetlink.c:2706 > [ ] rtnl_group_changelink net/core/rtnetlink.c:3225 > [inline] > [ ] __rtnl_newlink+0xe06/0x17d0
Re: [PATCHv5 net-next 1/3] octeontx2-af: Add devlink suppoort to af driver
On Thu, 26 Nov 2020 19:32:49 +0530 George Cherian wrote: > Add devlink support to AF driver. Basic devlink support is added. > Currently info_get is the only supported devlink ops. > > devlink ouptput looks like this > # devlink dev > pci/0002:01:00.0 > # devlink dev info > pci/0002:01:00.0: > driver octeontx2-af > versions: > fixed: > mbox version: 9 You need to document what this version is exactly. See how other drivers document things. It's also strongly preferred to use one of the existing identifiers if any one is matching the semantics. Fixed versions are for hardware versions, like ASIC rev or PCB board id or version. Your thing looks like a SW message format. It's not even FW. It doesn't belong in devlink at all.
Re: [PATCHv5 net-next 2/3] octeontx2-af: Add devlink health reporters for NPA
On Thu, 26 Nov 2020 19:32:50 +0530 George Cherian wrote: > Add health reporters for RVU NPA block. > NPA Health reporters handle following HW event groups > - GENERAL events > - ERROR events > - RAS events > - RVU event > An event counter per event is maintained in SW. > > Output: > # devlink health > pci/0002:01:00.0: >reporter hw_npa > state healthy error 0 recover 0 > # devlink health dump show pci/0002:01:00.0 reporter hw_npa > NPA_AF_GENERAL: > Unmap PF Error: 0 > NIX: > 0: free disabled RX: 0 free disabled TX: 0 > 1: free disabled RX: 0 free disabled TX: 0 > Free Disabled for SSO: 0 > Free Disabled for TIM: 0 > Free Disabled for DPI: 0 > Free Disabled for AURA: 0 > Alloc Disabled for Resvd: 0 > NPA_AF_ERR: > Memory Fault on NPA_AQ_INST_S read: 0 > Memory Fault on NPA_AQ_RES_S write: 0 > AQ Doorbell Error: 0 > Poisoned data on NPA_AQ_INST_S read: 0 > Poisoned data on NPA_AQ_RES_S write: 0 > Poisoned data on HW context read: 0 > NPA_AF_RVU: > Unmap Slot Error: 0 You seem to have missed the feedback Saeed and I gave you on v2. Did you test this with the errors actually triggering? Devlink should store only one dump, are the counters not going to get out of sync unless something clears the dump every time it triggers?
Re: [PATCH 1/2] vxlan: Add needed_headroom for lower device
On Thu, 26 Nov 2020 13:52:46 +0100 Sven Eckelmann wrote: > It was observed that sending data via batadv over vxlan (on top of > wireguard) reduced the performance massively compared to raw ethernet or > batadv on raw ethernet. A check of perf data showed that the > vxlan_build_skb was calling all the time pskb_expand_head to allocate > enough headroom for: > > min_headroom = LL_RESERVED_SPACE(dst->dev) + dst->header_len > + VXLAN_HLEN + iphdr_len; > > But the vxlan_config_apply only requested needed headroom for: > > lowerdev->hard_header_len + VXLAN6_HEADROOM or VXLAN_HEADROOM > > So it completely ignored the needed_headroom of the lower device. The first > caller of net_dev_xmit could therefore never make sure that enough headroom > was allocated for the rest of the transmit path. > > Cc: Annika Wickert > Signed-off-by: Sven Eckelmann Applied both, thanks!
Re: [PATCH net-next v3] net/nfc/nci: Support NCI 2.x initial sequence
On Fri, 27 Nov 2020 22:36:31 +0900 bongsu.je...@gmail.com wrote:
> From: Bongsu Jeon
>
> implement the NCI 2.x initial sequence to support NCI 2.x NFCC.
> Since NCI 2.0, CORE_RESET and CORE_INIT sequence have been changed.
> If NFCEE supports NCI 2.x, then NCI 2.x initial sequence will work.
>
> In NCI 1.0, Initial sequence and payloads are as below:
> (DH) (NFCC)
> | -- CORE_RESET_CMD --> |
> | <-- CORE_RESET_RSP -- |
> | -- CORE_INIT_CMD --> |
> | <-- CORE_INIT_RSP -- |
> CORE_RESET_RSP payloads are Status, NCI version, Configuration Status.
> CORE_INIT_CMD payloads are empty.
> CORE_INIT_RSP payloads are Status, NFCC Features,
> Number of Supported RF Interfaces, Supported RF Interface,
> Max Logical Connections, Max Routing table Size,
> Max Control Packet Payload Size, Max Size for Large Parameters,
> Manufacturer ID, Manufacturer Specific Information.
>
> In NCI 2.0, Initial Sequence and Parameters are as below:
> (DH) (NFCC)
> | -- CORE_RESET_CMD --> |
> | <-- CORE_RESET_RSP -- |
> | <-- CORE_RESET_NTF -- |
> | -- CORE_INIT_CMD --> |
> | <-- CORE_INIT_RSP -- |
> CORE_RESET_RSP payloads are Status.
> CORE_RESET_NTF payloads are Reset Trigger,
> Configuration Status, NCI Version, Manufacturer ID,
> Manufacturer Specific Information Length,
> Manufacturer Specific Information.
> CORE_INIT_CMD payloads are Feature1, Feature2.
> CORE_INIT_RSP payloads are Status, NFCC Features,
> Max Logical Connections, Max Routing Table Size,
> Max Control Packet Payload Size,
> Max Data Packet Payload Size of the Static HCI Connection,
> Number of Credits of the Static HCI Connection,
> Max NFC-V RF Frame Size, Number of Supported RF Interfaces,
> Supported RF Interfaces.
>
> Signed-off-by: Bongsu Jeon
> static void nci_init_req(struct nci_dev *ndev, unsigned long opt)
> {
> - nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, 0, NULL);
> + struct nci_core_init_v2_cmd *cmd = (struct nci_core_init_v2_cmd *)opt;
> +
> + if (!cmd)
> + nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, 0, NULL);
> + else
> + /* if nci version is 2.0, then use the feature parameters */
> + nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD,
> + sizeof(struct nci_core_init_v2_cmd), cmd);
This would be better written as:
u8 plen = 0;
if (opt)
plen = sizeof(struct nci_core_init_v2_cmd);
nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, plen, (void *)opt);
> +
unnecessary empty line
> }
>
> static void nci_init_complete_req(struct nci_dev *ndev, unsigned long opt)
> @@ -497,8 +505,18 @@ static int nci_open_device(struct nci_dev *ndev)
> }
>
> if (!rc) {
> - rc = __nci_request(ndev, nci_init_req, 0,
> -msecs_to_jiffies(NCI_INIT_TIMEOUT));
> + if (!(ndev->nci_ver & NCI_VER_2_MASK)) {
> + rc = __nci_request(ndev, nci_init_req, 0,
> +msecs_to_jiffies(NCI_INIT_TIMEOUT));
> + } else {
> + struct nci_core_init_v2_cmd nci_init_v2_cmd;
> +
> + nci_init_v2_cmd.feature1 = NCI_FEATURE_DISABLE;
> + nci_init_v2_cmd.feature2 = NCI_FEATURE_DISABLE;
> +
> + rc = __nci_request(ndev, nci_init_req, (unsigned
> long)_init_v2_cmd,
> +msecs_to_jiffies(NCI_INIT_TIMEOUT));
> + }
again please try to pull out the common code:
struct nci_core_init_v2_cmd nci_init_v2_cmd = {
.feature1 = NCI_FEATURE_DISABLE;
.feature2 = NCI_FEATURE_DISABLE;
};
unsigned long opt = 0;
if (ndev->nci_ver & NCI_VER_2_MASK)
opt = (unsigned long)_init_v2_cmd;
rc = __nci_request(ndev, nci_init_req, opt,
msecs_to_jiffies(NCI_INIT_TIMEOUT));
> }
> -static void nci_core_init_rsp_packet(struct nci_dev *ndev, struct sk_buff
> *skb)
> +static unsigned char nci_core_init_rsp_packet_v1(struct nci_dev *ndev,
> struct sk_buff *skb)
> {
> struct nci_core_init_rsp_1 *rsp_1 = (void *) skb->data;
> struct nci_core_init_rsp_2 *rsp_2;
> @@ -48,16 +51,14 @@ static void nci_core_init_rsp_packet(struct nci_dev
> *ndev, struct sk_buff *skb)
> pr_debug("status 0x%x\n", rsp_1->status);
>
> if (rsp_1->status != NCI_STATUS_OK)
> - goto exit;
> + return rsp_1->status;
>
> ndev->nfcc_features = __le32_to_cpu(rsp_1->nfcc_features);
> ndev->num_supported_rf_interfaces = rsp_1->num_supported_rf_interfaces;
>
> - if (ndev->num_supported_rf_interfaces >
> - NCI_MAX_SUPPORTED_RF_INTERFACES) {
> - ndev->num_supported_rf_interfaces =
> - NCI_MAX_SUPPORTED_RF_INTERFACES;
> - }
> +
Re: [PATCH] net: bna: remove trailing semicolon in macro definition
On Fri, 27 Nov 2020 08:55:50 -0800 t...@redhat.com wrote:
> From: Tom Rix
>
> The macro use will already have a semicolon.
>
> Signed-off-by: Tom Rix
> ---
> drivers/net/ethernet/brocade/bna/bna_hw_defs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/brocade/bna/bna_hw_defs.h
> b/drivers/net/ethernet/brocade/bna/bna_hw_defs.h
> index f335b7115c1b..4b19855017d7 100644
> --- a/drivers/net/ethernet/brocade/bna/bna_hw_defs.h
> +++ b/drivers/net/ethernet/brocade/bna/bna_hw_defs.h
> @@ -218,7 +218,7 @@ do {
> \
>
> /* Set the coalescing timer for the given ib */
> #define bna_ib_coalescing_timer_set(_i_dbell, _cls_timer)\
> - ((_i_dbell)->doorbell_ack = BNA_DOORBELL_IB_INT_ACK((_cls_timer), 0));
> + ((_i_dbell)->doorbell_ack = BNA_DOORBELL_IB_INT_ACK((_cls_timer), 0))
>
> /* Acks 'events' # of events for a given ib while disabling interrupts */
> #define bna_ib_ack_disable_irq(_i_dbell, _events)\
> @@ -260,7 +260,7 @@ do {
> \
>
> #define bna_txq_prod_indx_doorbell(_tcb) \
> (writel(BNA_DOORBELL_Q_PRD_IDX((_tcb)->producer_index), \
> - (_tcb)->q_dbell));
> + (_tcb)->q_dbell))
>
> #define bna_rxq_prod_indx_doorbell(_rcb) \
> (writel(BNA_DOORBELL_Q_PRD_IDX((_rcb)->producer_index), \
Same story here as Daniel pointed out for the BPF patch.
There are 2 macros right below here which also have a semicolon at the
end. And these ones are used. So the patch appears to be pretty arbitrary.
Re: [PATCH] net: wan: remove trailing semicolon in macro definition
On Fri, 27 Nov 2020 08:57:34 -0800 t...@redhat.com wrote: > From: Tom Rix > > The macro use will already have a semicolon. > > Signed-off-by: Tom Rix This one looks fine, applied, thanks.
Re: [PATCH net-next v5] net/tun: Call netdev notifiers
On Mon, 23 Nov 2020 07:18:07 +0100 Martin Schiller wrote: > On 2020-11-20 19:28, Jakub Kicinski wrote: > > On Wed, 18 Nov 2020 07:39:19 +0100 Martin Schiller wrote: > >> Call netdev notifiers before and after changing the device type. > >> > >> Signed-off-by: Martin Schiller > > > > This is a fix, right? Can you give an example of something that goes > > wrong without this patch? > > This change is related to my latest patches to the X.25 Subsystem: > https://patchwork.kernel.org/project/netdevbpf/list/?series=388087 > > I use a tun interface in a XoT (X.25 over TCP) application and use the > TUNSETLINK ioctl to change the device type to ARPHRD_X25. > As the default device type is ARPHRD_NONE the initial NETDEV_REGISTER > event won't be catched by the X.25 Stack. > > Therefore I have to use the NETDEV_POST_TYPE_CHANGE to make sure that > the corresponding neighbour structure is created. > > I could imagine that other protocols have similar requirements. > > Whether this is a fix or a functional extension is hard to say. > > Some time ago there was also a corresponding patch for the WAN/HDLC > subsystem: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=2f8364a291e8 Thanks for this info, applied to net-next.
Re: [PATCH net-next v4 2/5] net/lapb: support netdev events
On Mon, 23 Nov 2020 03:17:54 -0800 Xie He wrote: > On Mon, Nov 23, 2020 at 2:38 AM Martin Schiller wrote: > > Well, one could argue that we would have to repair these drivers, but I > > don't think that will get us anywhere. > > Yeah... One problem I see with the Linux project is the lack of > docs/specs. Often we don't know what is right and what is wrong. More of a historic thing than a requirement AFAIK. Some software devices, e.g. loopback will not generate carrier events. But in this case looks like all the devices Martin wants to handle are lapb. > > From this point of view it will be the best to handle the NETDEV_UP in > > the lapb event handler and establish the link analog to the > > NETDEV_CHANGE event if the carrier is UP. > > Thanks! This way we can make sure LAPB would automatically connect in > all situations. > > Since we'll have a netif_carrier_ok check in NETDEV_UP handing, it > might make the code look prettier to also have a netif_carrier_ok > check in NETDEV_GOING_DOWN handing (for symmetry). Just a suggestion. > You can do whatever looks good to you :) Xie other than this the patches look good to you? Martin should I expect a respin to follow Xie's suggestion or should I apply v4?
Re: [PATCH net-next v2 1/2] lockdep: Introduce in_softirq lockdep assert
On Mon, 23 Nov 2020 15:27:25 +0100 Peter Zijlstra wrote:
> On Sat, Nov 21, 2020 at 11:06:15AM +0800, Yunsheng Lin wrote:
> > The current semantic for napi_consume_skb() is that caller need
> > to provide non-zero budget when calling from NAPI context, and
> > breaking this semantic will cause hard to debug problem, because
> > _kfree_skb_defer() need to run in atomic context in order to push
> > the skb to the particular cpu' napi_alloc_cache atomically.
> >
> > So add the lockdep_assert_in_softirq() to assert when the running
> > context is not in_softirq, in_softirq means softirq is serving or
> > BH is disabled. Because the softirq context can be interrupted by
> > hard IRQ or NMI context, so lockdep_assert_in_softirq() need to
> > assert about hard IRQ or NMI context too.
> Due to in_softirq() having a deprication notice (due to it being
> awefully ambiguous), could we have a nice big comment here that explains
> in detail understandable to !network people (me) why this is actually
> correct?
>
> I'm not opposed to the thing, if that his what you need, it's fine, but
> please put on a comment that explains that in_softirq() is ambiguous and
> when you really do need it anyway.
One liner would be:
* Acceptable for protecting per-CPU resources accessed from BH
We can add:
* Much like in_softirq() - semantics are ambiguous, use carefully. *
IIUC we basically want to protect the nc array and counter here:
static inline void _kfree_skb_defer(struct sk_buff *skb)
{
struct napi_alloc_cache *nc = this_cpu_ptr(_alloc_cache);
/* drop skb->head and call any destructors for packet */
skb_release_all(skb);
/* record skb to CPU local list */
nc->skb_cache[nc->skb_count++] = skb;
#ifdef CONFIG_SLUB
/* SLUB writes into objects when freeing */
prefetchw(skb);
#endif
/* flush skb_cache if it is filled */
if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) {
kmem_cache_free_bulk(skbuff_head_cache, NAPI_SKB_CACHE_SIZE,
nc->skb_cache);
nc->skb_count = 0;
}
}
> > +#define lockdep_assert_in_softirq()
> > \
> > +do {
> > \
> > + WARN_ON_ONCE(__lockdep_enabled && \
> > +(!in_softirq() || in_irq() || in_nmi())); \
> > +} while (0)
