* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Vivien Didelot @ 2017-09-06 13:56 UTC (permalink / raw)
To: Andrew Lunn, netdev, Florian Fainelli, Woojung.Huh, jbe,
sean.wang, john
In-Reply-To: <20170906004703.GB27385@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> So there is the third issue. It affects just DSA, but it possible
> affects all DSA drivers.
>
> This patchset broken broadcast with the Marvell drivers. It could
> break broadcast on others drivers as well.
>
> What i found is that the Marvell chips don't flood broadcast frames
> between bridged ports. What appears to happen is there is a fdb miss,
> so it gets forwarded to the CPU port for the host to deal with. The
> software bridge when floods it out all ports of the bridge.
Do you have this issue on a single switch?
I do expect FDB (not MDB) miss on a multi-chip fabric, not on a single
chip though.
> But the set offload_fwd_mark patch changes this. The software bridge
> now assumes the hardware has already flooded broadcast out all ports
> of the switch as needed. So it does not do any flooding itself. As a
> result, on Marvell devices, broadcast packets don't get flooded at
> all.
>
> The issue can be fixed. I just need to add an mdb entry for the
> broadcast address to each port of the bridge in the switch, and the
> CPU port. But i don't know at what level to do this.
>
> Should this be done at the DSA level, or at the driver level? Do any
> chips do broadcast flooding in hardware already? Hence they currently
> see broadcast duplication? If i add a broadcast mdb at the DSA level,
> and the chip is already hard wired to flooding broadcast, is it going
> to double flood?
It should be done at the DSA level. DSA core must be the single entry
point to handle all the switch logic, calling into (dumb) DSA drivers.
If it is buried into a specific driver, we'll likely lose track of the
problem and make it harder to maintain.
Thanks!
Vivien
^ permalink raw reply
* Re: [PATCH net 2/4] lan78xx: Add fixed_phy device support for LAN7801 device
From: Andrew Lunn @ 2017-09-06 14:02 UTC (permalink / raw)
To: Nisar.Sayed; +Cc: davem, UNGLinuxDriver, netdev
In-Reply-To: <CE371C1263339941885964188A0225FA333381@CHN-SV-EXMX03.mchp-main.com>
On Wed, Sep 06, 2017 at 10:51:44AM +0000, Nisar.Sayed@microchip.com wrote:
> From: Nisar Sayed <Nisar.Sayed@microchip.com>
>
> Add fixed_phy device support for LAN7801 device
>
> When LAN7801 device connected to PHY Device which does not have
> MDIO/MDC access, fixex_phy device will be added.
Please try to find a way to do this without all the #ifdefs. They can
be acceptable in header files, but should be avoided in .c files.
Andrew
^ permalink raw reply
* Re: [PATCH net 3/4] lan78xx: Fix for eeprom read/write when device autosuspend
From: Andrew Lunn @ 2017-09-06 14:06 UTC (permalink / raw)
To: Nisar.Sayed; +Cc: davem, UNGLinuxDriver, netdev
In-Reply-To: <CE371C1263339941885964188A0225FA333391@CHN-SV-EXMX03.mchp-main.com>
Hi Nisar
> + else if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
> + (ee->offset >= 0 && ee->offset < MAX_EEPROM_SIZE) &&
> + (ee->len > 0 && (ee->offset + ee->len) <= MAX_EEPROM_SIZE))
> + ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
This change does not appear to have anything to do with auto suspend.
Please make it a separate patch.
Andrew
^ permalink raw reply
* Re: [PATCH] vsock: only load vmci transport on VMware hypervisor by default
From: Jorgen S. Hansen @ 2017-09-06 14:11 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Dexuan Cui, davem@davemloft.net, netdev@vger.kernel.org,
gregkh@linuxfoundation.org, devel@linuxdriverproject.org,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, George Zhang,
Michal Kubecek, Asias He, Vitaly Kuznetsov, Cathy Avery,
jasowang@redhat.com, Rolf Neugebauer, Dave Scott, Marcelo Cerri,
apw@canonical.com
In-Reply-To: <20170831115455.GP13619@stefanha-x1.localdomain>
> On Aug 31, 2017, at 1:54 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, Aug 29, 2017 at 03:37:07PM +0000, Jorgen S. Hansen wrote:
>>> On Aug 29, 2017, at 4:36 AM, Dexuan Cui <decui@microsoft.com> wrote:
>> If we allow multiple host side transports, virtio host side support and
>> vmci should be able to coexist regardless of the order of initialization.
>
> That sounds good to me.
>
> This means af_vsock.c needs to be aware of CID allocation. Currently the
> vhost_vsock.ko driver handles this itself (it keeps a list of CIDs and
> checks that they are not used twice). It should be possible to move
> that state into af_vsock.c so we have <cid, host_transport> pairs.
>
Yes, that was my thinking as well.
> I'm currently working on NFS over AF_VSOCK and sock_diag support (for
> ss(8) and netstat-like tools).
>
> Multi-transport support is lower priority for me at the moment. I'm
> happy to review patches though. If there is no progress on this by the
> end of the year then I will have time to work on it.
>
I’ll try to find time to write a more coherent proposal in the coming weeks,
and we can discuss that.
> Are either of you are in Prague, Czech Republic on October 25-27 for
> Linux Kernel Summit, Open Source Summit Europe, Embedded Linux
> Conference Europe, KVM Forum, or MesosCon Europe?
No, I won’t be there.
Thanks,
Jorgen
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Andrew Lunn @ 2017-09-06 14:16 UTC (permalink / raw)
To: Vivien Didelot
Cc: netdev, Florian Fainelli, Woojung.Huh, jbe, sean.wang, john
In-Reply-To: <87y3pskk1p.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
> > What i found is that the Marvell chips don't flood broadcast frames
> > between bridged ports. What appears to happen is there is a fdb miss,
> > so it gets forwarded to the CPU port for the host to deal with. The
> > software bridge when floods it out all ports of the bridge.
>
> Do you have this issue on a single switch?
Yes. I have two boards with a single switch in my test setup. They
fail the broadcast tests in the same way as boards with multiple
switches.
> It should be done at the DSA level. DSA core must be the single entry
> point to handle all the switch logic, calling into (dumb) DSA drivers.
> If it is buried into a specific driver, we'll likely lose track of the
> problem and make it harder to maintain.
I think we need to wait and see what driver writers report their chips
are doing. If this is just a Marvell issue, we probably should fix it
in the Marvell driver only.
If i remember correctly, Woojung said he was seeing duplication of
broadcasts. So it could be the Microchip hardware is forwarding
broadcast in the hardware.
Andrew
^ permalink raw reply
* Re: [RFC net-next] net: sch_clsact: add support for global per-netns classifier mode
From: Roopa Prabhu @ 2017-09-06 14:19 UTC (permalink / raw)
To: Jiri Pirko
Cc: Daniel Borkmann, Cong Wang, Nikolay Aleksandrov,
Linux Kernel Network Developers, David Ahern, Jamal Hadi Salim
In-Reply-To: <20170906072413.GC2523@nanopsycho>
On Wed, Sep 6, 2017 at 12:24 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Sep 06, 2017 at 06:04:17AM CEST, roopa@cumulusnetworks.com wrote:
>>On Tue, Sep 5, 2017 at 3:45 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> On 09/06/2017 12:01 AM, Roopa Prabhu wrote:
>>>>
>>>> On Tue, Sep 5, 2017 at 11:18 AM, Cong Wang <xiyou.wangcong@gmail.com>
>>>> wrote:
>>>>>
>>>>> On Tue, Sep 5, 2017 at 5:48 AM, Nikolay Aleksandrov
>>>>> <nikolay@cumulusnetworks.com> wrote:
>>>>>>
>>>>>> Hi all,
>>>>>> This RFC adds a new mode for clsact which designates a device's egress
>>>>>> classifier as global per netns. The packets that are not classified for
>>>>>> a particular device will be classified using the global classifier.
>>>>>> We have needed a global classifier for some time now for various
>>>>>> purposes and setting the single bridge or loopback/vrf device as the
>>>
>>>
>>> Can you elaborate a bit more on the ... "we have needed a global
>>> classifier for some time now for various purposes".
>>
>>Most of our acl's are global or use a wildcard. eg iptables supports
>>global rules without an dev. We do end up having hundreds of netdevs.
>>Another use case for the future is use of tc for policy based routing
>>which requires global rules.
>
> That is not how TC works. There are devices, qdiscs, blocks, chains. The
> global approach does not fit. The block sharing gets you what you need,
> without need for any ugly hack.
We know how TC works. And we are only trying to see if the classifiers
and actions can be used
in other use-cases. Note that the patch was an RFC to start the
discussion and refine
it. We will explore your block sharing patches when they are in.
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: John Crispin @ 2017-09-06 14:27 UTC (permalink / raw)
To: Andrew Lunn, netdev, Florian Fainelli, Vivien Didelot,
Woojung.Huh, jbe, sean.wang
In-Reply-To: <20170906004703.GB27385@lunn.ch>
On 06/09/17 02:47, Andrew Lunn wrote:
> Should this be done at the DSA level, or at the driver level? Do any
> chips do broadcast flooding in hardware already? Hence they currently
> see broadcast duplication? If i add a broadcast mdb at the DSA level,
> and the chip is already hard wired to flooding broadcast, is it going
> to double flood?
Hi Andrew,
MT7530 and QCA8K only have a "unknown mac fwd to port X" feature. both
use the same HW table for FDB and MDB tables. so this should ideally be
fixed on DSA level rather than fixing up those 2 drivers.
John
^ permalink raw reply
* Re: hung task in mac80211
From: Stefano Brivio @ 2017-09-06 14:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <1504702728.13457.17.camel@sipsolutions.net>
On Wed, 06 Sep 2017 14:58:48 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> +void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> + u8 dialog_token, u16 timeout,
> + u16 start_seq_num, u16 ba_policy, u16 tid,
> + u16 buf_size, bool tx, bool auto_seq)
> +{
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + ___ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
> + start_seq_num, ba_policy, tid,
> + buf_size, tx, auto_seq);
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> +}
> +
Sorry for the extended bothering :) but here, you're extending quite a
bit the scope of the lock also when__ieee80211_start_rx_ba_session() is
called by ieee80211_process_addba_request(). No idea what the hit can
be, but we can't safely assume it's nothing either. What about simply
introducing a 'ampdu_mlme_lock_held' argument instead? Something like:
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 2b36eff5d97e..35a9eff1ec66 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -248,7 +248,8 @@ static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *d
void __ieee80211_start_rx_ba_session(struct sta_info *sta,
u8 dialog_token, u16 timeout,
u16 start_seq_num, u16 ba_policy, u16 tid,
- u16 buf_size, bool tx, bool auto_seq)
+ u16 buf_size, bool tx, bool auto_seq,
+ bool ampdu_mlme_lock_held)
{
struct ieee80211_local *local = sta->sdata->local;
struct tid_ampdu_rx *tid_agg_rx;
@@ -311,7 +312,8 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
buf_size, sta->sta.addr);
/* examine state machine */
- mutex_lock(&sta->ampdu_mlme.mtx);
+ if (!ampdu_mlme_lock_held)
+ mutex_lock(&sta->ampdu_mlme.mtx);
if (test_bit(tid, sta->ampdu_mlme.agg_session_valid)) {
if (sta->ampdu_mlme.tid_rx_token[tid] == dialog_token) {
@@ -415,7 +417,8 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
__clear_bit(tid, sta->ampdu_mlme.unexpected_agg);
sta->ampdu_mlme.tid_rx_token[tid] = dialog_token;
}
- mutex_unlock(&sta->ampdu_mlme.mtx);
+ if (!ampdu_mlme_lock_held)
+ mutex_unlock(&sta->ampdu_mlme.mtx);
end_no_lock:
if (tx)
@@ -445,7 +448,7 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
__ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
start_seq_num, ba_policy, tid,
- buf_size, true, false);
+ buf_size, true, false, false);
}
void ieee80211_manage_rx_ba_offl(struct ieee80211_vif *vif,
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..59ba67e8942f 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -335,7 +335,7 @@ void ieee80211_ba_session_work(struct work_struct *work)
sta->ampdu_mlme.tid_rx_manage_offl))
__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
IEEE80211_MAX_AMPDU_BUF,
- false, true);
+ false, true, true);
if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
sta->ampdu_mlme.tid_rx_manage_offl))
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2197c62a0a6e..5d494ac65853 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1759,7 +1759,8 @@ void __ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
void __ieee80211_start_rx_ba_session(struct sta_info *sta,
u8 dialog_token, u16 timeout,
u16 start_seq_num, u16 ba_policy, u16 tid,
- u16 buf_size, bool tx, bool auto_seq);
+ u16 buf_size, bool tx, bool auto_seq,
+ bool ampdu_mlme_lock_held);
void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
enum ieee80211_agg_stop_reason reason);
void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
--
Stefano
^ permalink raw reply related
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 14:30 UTC (permalink / raw)
To: Stefano Brivio; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <20170906162748.071b040e@elisabeth>
On Wed, 2017-09-06 at 16:27 +0200, Stefano Brivio wrote:
>
> Sorry for the extended bothering :) but here, you're extending quite
> a bit the scope of the lock also
> when__ieee80211_start_rx_ba_session() is called by
> ieee80211_process_addba_request().
I know, but it doesn't matter.
> No idea what the hit can be, but we can't safely assume it's
> nothing either.
We don't really have to assume anything, we can read the code :-)
Trust me, I probably wrote most of it. It's fine, just sanity checks.
> What about simply introducing a 'ampdu_mlme_lock_held' argument
> instead?
Eww, no.
johannes
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Vivien Didelot @ 2017-09-06 14:29 UTC (permalink / raw)
To: Andrew Lunn, netdev; +Cc: jiri, nikolay, Florian Fainelli, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> Getting the frames to the bridge as requested turned up an issue or
> three. The offload_fwd_mark is not being set by DSA, so the bridge
> floods the received frames back to the switch ports, resulting in
> duplication since the hardware has already flooded the packet. Fixing
> that turned up an issue with the meaning of
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID in DSA. A DSA fabric of three
> switches needs to look to the software bridge as a single
> switch. Otherwise the offload_fwd_mark does not work, and we get
> duplication on the non-ingress switch. But each switch returned a
> different value. And they were not unique.
[...]
> Is the SWITCHDEV_ATTR_ID_PORT_PARENT_ID change acceptable?
So here we need to return the switch fabric (tree) ID instead of a
single switch chip ID. I think we are not supposed to change it, but
there's definitly something wrong with them and we must fix it.
The same issue happens with the physical port IDs, where the net devices
of two disjoint trees on a system will have the same phys_*_id
attributes, because we do not include the tree ID in them.
(not sure if this affects your patchset though.)
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-09-06 14:33 UTC (permalink / raw)
To: Florian Fainelli, David Daney
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard, Thibaud Cornic
In-Reply-To: <4ea8b432-4968-1616-eff9-48a2689dd3ce@gmail.com>
On 31/08/2017 20:29, Florian Fainelli wrote:
> On 08/31/2017 11:12 AM, Mason wrote:
>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>> netif_carrier_off().
>>>>
>>>> I must be missing something.
>>>> (Since a thread cannot race against itself.)
>>>>
>>>> phy_disconnect calls phy_stop_machine which
>>>> 1) stops the work queue from running in a separate thread
>>>> 2) calls phy_state_machine *synchronously*
>>>> which runs the PHY_HALTED case with everything well-defined
>>>> end of phy_stop_machine
>>>>
>>>> phy_disconnect only then calls phy_detach()
>>>> which makes future calls of phy_state_machine perilous.
>>>>
>>>> This all happens in the same thread, so I'm not yet
>>>> seeing where the race happens?
>>>
>>> The race is as described in David's earlier email, so let's recap:
>>>
>>> Thread 1 Thread 2
>>> phy_disconnect()
>>> phy_stop_interrupts()
>>> phy_stop_machine()
>>> phy_state_machine()
>>> -> queue_delayed_work()
>>> phy_detach()
>>> phy_state_machine()
>>> -> netif_carrier_off()
>>>
>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>> scheduled and process PHY_HALTED again, then we trigger the NULL pointer
>>> de-reference.
>>>
>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>> they will run on the same CPU.
>>
>> Something does not add up.
>>
>> The synchronous call to phy_state_machine() does:
>>
>> case PHY_HALTED:
>> if (phydev->link) {
>> phydev->link = 0;
>> netif_carrier_off(phydev->attached_dev);
>> phy_adjust_link(phydev);
>> do_suspend = true;
>> }
>>
>> then sets phydev->link = 0; therefore subsequent calls to
>> phy_state_machin() will be no-op.
>
> Actually you are right, once phydev->link is set to 0 these would become
> no-ops. Still scratching my head as to what happens for David then...
>
>>
>> Also, queue_delayed_work() is only called in polling mode.
>> David stated that he's using interrupt mode.
>
> Right that's confusing too now. David can you check if you tree has:
>
> 49d52e8108a21749dc2114b924c907db43358984 ("net: phy: handle state
> correctly in phy_stop_machine")
Hello David,
A week ago, you wrote about my patch:
"This is broken. Please revert."
I assume you tested the revert locally, and that reverting did make
the crash disappear. Is that correct?
The reason I ask is because the analysis you provided contains some
flaws, as noted above. But, if reverting my patch did fix your issue,
then perhaps understanding *why* is unimportant.
I'm a bit baffled that it took less than 90 minutes for your request
to be approved, and the patch reverted in all branches, before I even
had a chance to comment.
Regards.
^ permalink raw reply
* Re: [PATCH v2 rfc 8/8] net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID
From: Vivien Didelot @ 2017-09-06 14:46 UTC (permalink / raw)
To: Andrew Lunn, netdev; +Cc: jiri, nikolay, Florian Fainelli, Andrew Lunn
In-Reply-To: <1504654510-31004-9-git-send-email-andrew@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID is used by the software bridge when
> determining which ports to flood a packet out. If the packet
> originated from a switch, it assumes the switch has already flooded
> the packet out the switches ports, so the bridge should not flood the
> packet itself out switch ports. Ports on the same switch are expected
> to return the same parent ID when SWITCHDEV_ATTR_ID_PORT_PARENT_ID is
> called.
>
> DSA gets this wrong with clusters of switches. As far as the software
> bridge is concerned, the cluster is all one switch. A packet from any
> switch in the cluster can be assumed to of been flooded as needed out
> all ports of the cluster, not just the switch it originated
> from. Hence all ports of a cluster should return the same parent. The
> old implementation did not, each switch in the cluster had its own ID.
>
> Also wrong was that the ID was not unique if multiple DSA instances
> are in operation.
>
> Use the MAC address of the master interface as the parent ID. This is
> the same for all switches in a cluster, and should be unique if there
> are multiple clusters.
That is not correct. Support for multiple CPU ports is coming and in
this case, you can have two CPU host interfaces wired to two switch
ports of the same tree. So two different master MAC addresses.
Only the tree ID assigned by DSA core is unique to a given tree.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Matthias May @ 2017-09-06 14:46 UTC (permalink / raw)
To: Andrew Lunn, netdev, Florian Fainelli, Vivien Didelot,
Woojung.Huh, jbe, sean.wang, john
In-Reply-To: <20170906004703.GB27385@lunn.ch>
On 06/09/17 02:47, Andrew Lunn wrote:
>> The third and last issue will be explained in a followup email.
>
> Hi DSA hackers
>
> So there is the third issue. It affects just DSA, but it possible
> affects all DSA drivers.
>
> This patchset broken broadcast with the Marvell drivers. It could
> break broadcast on others drivers as well.
>
> What i found is that the Marvell chips don't flood broadcast frames
> between bridged ports. What appears to happen is there is a fdb miss,
> so it gets forwarded to the CPU port for the host to deal with. The
> software bridge when floods it out all ports of the bridge.
>
> But the set offload_fwd_mark patch changes this. The software bridge
> now assumes the hardware has already flooded broadcast out all ports
> of the switch as needed. So it does not do any flooding itself. As a
> result, on Marvell devices, broadcast packets don't get flooded at
> all.
>
> The issue can be fixed. I just need to add an mdb entry for the
> broadcast address to each port of the bridge in the switch, and the
> CPU port. But i don't know at what level to do this.
>
> Should this be done at the DSA level, or at the driver level? Do any
> chips do broadcast flooding in hardware already? Hence they currently
> see broadcast duplication? If i add a broadcast mdb at the DSA level,
> and the chip is already hard wired to flooding broadcast, is it going
> to double flood?
>
> Andrew
>
Hi Andrew
We are using the 88E6321.
In our setup we are using openvswitch and not a bridge, however the problem you describe seems to be the same.
We had to configure the switch to flood unknown multicast (Egress Floods = 0x3, bits 3:2, offset 0x4 in port control)
and
unset FloodBC (FloodBC = 0x0, bit 12, offset 0x5 in global 2) which defines if a broadcast should be considered as
multicast for the above config.
Regarding the looping problem:
Since OVS isn't aware of the fdb of the switch, we configure the ports representing the switch ports (in ovs) as
"protected" which prevents looping them back between (even when flooding) see [1].
I'm not sure if the bridge has a similar feature.
BR
Matthias
[1]http://openvswitch.org/support/dist-docs/ovs-vswitchd.conf.db.5.txt ctrl-f: "protected: boolean"
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-09-06 14:55 UTC (permalink / raw)
To: Florian Fainelli
Cc: Marc Gonzalez, David Daney, netdev, Geert Uytterhoeven,
David Miller, Andrew Lunn, Mans Rullgard, Thibaud Cornic
In-Reply-To: <7b1c1dc9-b6e3-a1bd-2e36-474946741a79@gmail.com>
On 31/08/2017 21:18, Florian Fainelli wrote:
> On 08/31/2017 12:09 PM, Mason wrote:
>
>> 1) nb8800_link_reconfigure() calls phy_print_status()
>> which prints the "Link down" and "Link up" messages
>> to the console. With the patch reverted, nothing is
>> printed when the link goes down, and the result is
>> random when the link comes up. Sometimes, we get
>> down + up, sometimes just up.
>
> Nothing printed when you bring down the network interface as a result of
> not signaling the link down, there is a small nuance here.
Let me first focus on the "Link down" message.
Do you agree that such a message should be printed when the
link goes down, not when the link comes up?
Perhaps the issue is that the 2 following cases need to be
handled differently:
A) operator sets link down on the command-line
B) asynchronous event makes link go down (peer is dead, cable is cut, etc)
In B) the PHY state machine keeps on running, and eventually
calls adjust_link()
In A) the driver calls phy_stop() and phy_disconnect() and
therefore adjust_link() will not be called?
Regards.
^ permalink raw reply
* [PATCH 2/2] net/9p: Switch to wait_event_killable()
From: Tuomas Tynkkynen @ 2017-09-06 14:59 UTC (permalink / raw)
To: Al Viro
Cc: v9fs-developer, Eric Van Hensbergen, Ron Minnich,
Latchesar Ionkov, David S. Miller, linux-kernel, netdev,
linux-fsdevel, Tuomas Tynkkynen, stable
In-Reply-To: <20170906145908.8082-1-tuomas@tuxera.com>
Because userspace gets Very Unhappy when calls like stat() and execve()
return -EINTR on 9p filesystem mounts. For instance, when bash is
looking in PATH for things to execute and some SIGCHLD interrupts
stat(), bash can throw a spurious 'command not found' since it doesn't
retry the stat().
In practice, hitting the problem is rare and needs a really
slow/bogged down 9p server.
Cc: stable@vger.kernel.org
Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>
---
net/9p/client.c | 3 +--
net/9p/trans_virtio.c | 13 ++++++-------
net/9p/trans_xen.c | 4 ++--
3 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/net/9p/client.c b/net/9p/client.c
index 4674235b0d9b..1beb131dd3e1 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -773,8 +773,7 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
}
again:
/* Wait for the response */
- err = wait_event_interruptible(*req->wq,
- req->status >= REQ_STATUS_RCVD);
+ err = wait_event_killable(*req->wq, req->status >= REQ_STATUS_RCVD);
/*
* Make sure our req is coherent with regard to updates in other
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index f24b25c25106..f3a4efcf1456 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -286,8 +286,8 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
if (err == -ENOSPC) {
chan->ring_bufs_avail = 0;
spin_unlock_irqrestore(&chan->lock, flags);
- err = wait_event_interruptible(*chan->vc_wq,
- chan->ring_bufs_avail);
+ err = wait_event_killable(*chan->vc_wq,
+ chan->ring_bufs_avail);
if (err == -ERESTARTSYS)
return err;
@@ -327,7 +327,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
* Other zc request to finish here
*/
if (atomic_read(&vp_pinned) >= chan->p9_max_pages) {
- err = wait_event_interruptible(vp_wq,
+ err = wait_event_killable(vp_wq,
(atomic_read(&vp_pinned) < chan->p9_max_pages));
if (err == -ERESTARTSYS)
return err;
@@ -471,8 +471,8 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
if (err == -ENOSPC) {
chan->ring_bufs_avail = 0;
spin_unlock_irqrestore(&chan->lock, flags);
- err = wait_event_interruptible(*chan->vc_wq,
- chan->ring_bufs_avail);
+ err = wait_event_killable(*chan->vc_wq,
+ chan->ring_bufs_avail);
if (err == -ERESTARTSYS)
goto err_out;
@@ -489,8 +489,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
virtqueue_kick(chan->vq);
spin_unlock_irqrestore(&chan->lock, flags);
p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
- err = wait_event_interruptible(*req->wq,
- req->status >= REQ_STATUS_RCVD);
+ err = wait_event_killable(*req->wq, req->status >= REQ_STATUS_RCVD);
/*
* Non kernel buffers are pinned, unpin them
*/
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 6ad3e043c617..325c56043007 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -156,8 +156,8 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
ring = &priv->rings[num];
again:
- while (wait_event_interruptible(ring->wq,
- p9_xen_write_todo(ring, size)) != 0)
+ while (wait_event_killable(ring->wq,
+ p9_xen_write_todo(ring, size)) != 0)
;
spin_lock_irqsave(&ring->lock, flags);
--
2.13.0
^ permalink raw reply related
* Re: hung task in mac80211
From: Matteo Croce @ 2017-09-06 15:04 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <1504702728.13457.17.camel@sipsolutions.net>
On Wed, Sep 6, 2017 at 2:58 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2017-09-06 at 13:57 +0200, Matteo Croce wrote:
>
>> I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
>> The problem is present both on my AP and on my notebook,
>> so it seems it affects AP and STA mode as well.
>> The generated messages are:
>>
>> INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
>> Not tainted 4.13.0 #57
>> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
>> message.
>> kworker/u16:6 D 0 120 2 0x00000000
>> Workqueue: phy0 ieee80211_ba_session_work [mac80211]
>> Call Trace:
>> ? __schedule+0x174/0x5b0
>> ? schedule+0x31/0x80
>> ? schedule_preempt_disabled+0x9/0x10
>> ? __mutex_lock.isra.2+0x163/0x480
>> ? select_task_rq_fair+0xb9f/0xc60
>> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
>> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
>
> Yeah - obviously as Stefano found, both take &sta->ampdu_mlme.mtx.
>
> Can you try this?
>
> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
> index 2b36eff5d97e..d8d32776031e 100644
> --- a/net/mac80211/agg-rx.c
> +++ b/net/mac80211/agg-rx.c
> @@ -245,10 +245,10 @@ static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *d
> ieee80211_tx_skb(sdata, skb);
> }
>
> -void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> - u8 dialog_token, u16 timeout,
> - u16 start_seq_num, u16 ba_policy, u16 tid,
> - u16 buf_size, bool tx, bool auto_seq)
> +void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
> + u8 dialog_token, u16 timeout,
> + u16 start_seq_num, u16 ba_policy, u16 tid,
> + u16 buf_size, bool tx, bool auto_seq)
> {
> struct ieee80211_local *local = sta->sdata->local;
> struct tid_ampdu_rx *tid_agg_rx;
> @@ -267,7 +267,7 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> ht_dbg(sta->sdata,
> "STA %pM requests BA session on unsupported tid %d\n",
> sta->sta.addr, tid);
> - goto end_no_lock;
> + goto end;
> }
>
> if (!sta->sta.ht_cap.ht_supported) {
> @@ -275,14 +275,14 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> "STA %pM erroneously requests BA session on tid %d w/o QoS\n",
> sta->sta.addr, tid);
> /* send a response anyway, it's an error case if we get here */
> - goto end_no_lock;
> + goto end;
> }
>
> if (test_sta_flag(sta, WLAN_STA_BLOCK_BA)) {
> ht_dbg(sta->sdata,
> "Suspend in progress - Denying ADDBA request (%pM tid %d)\n",
> sta->sta.addr, tid);
> - goto end_no_lock;
> + goto end;
> }
>
> /* sanity check for incoming parameters:
> @@ -296,7 +296,7 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> ht_dbg_ratelimited(sta->sdata,
> "AddBA Req with bad params from %pM on tid %u. policy %d, buffer size %d\n",
> sta->sta.addr, tid, ba_policy, buf_size);
> - goto end_no_lock;
> + goto end;
> }
> /* determine default buffer size */
> if (buf_size == 0)
> @@ -311,7 +311,6 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> buf_size, sta->sta.addr);
>
> /* examine state machine */
> - mutex_lock(&sta->ampdu_mlme.mtx);
>
> if (test_bit(tid, sta->ampdu_mlme.agg_session_valid)) {
> if (sta->ampdu_mlme.tid_rx_token[tid] == dialog_token) {
> @@ -415,15 +414,25 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> __clear_bit(tid, sta->ampdu_mlme.unexpected_agg);
> sta->ampdu_mlme.tid_rx_token[tid] = dialog_token;
> }
> - mutex_unlock(&sta->ampdu_mlme.mtx);
>
> -end_no_lock:
> if (tx)
> ieee80211_send_addba_resp(sta->sdata, sta->sta.addr, tid,
> dialog_token, status, 1, buf_size,
> timeout);
> }
>
> +void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> + u8 dialog_token, u16 timeout,
> + u16 start_seq_num, u16 ba_policy, u16 tid,
> + u16 buf_size, bool tx, bool auto_seq)
> +{
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + ___ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
> + start_seq_num, ba_policy, tid,
> + buf_size, tx, auto_seq);
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> +}
> +
> void ieee80211_process_addba_request(struct ieee80211_local *local,
> struct sta_info *sta,
> struct ieee80211_mgmt *mgmt,
> diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
> index c92df492e898..198b2d3e56fd 100644
> --- a/net/mac80211/ht.c
> +++ b/net/mac80211/ht.c
> @@ -333,9 +333,9 @@ void ieee80211_ba_session_work(struct work_struct *work)
>
> if (test_and_clear_bit(tid,
> sta->ampdu_mlme.tid_rx_manage_offl))
> - __ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
> - IEEE80211_MAX_AMPDU_BUF,
> - false, true);
> + ___ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
> + IEEE80211_MAX_AMPDU_BUF,
> + false, true);
>
> if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
> sta->ampdu_mlme.tid_rx_manage_offl))
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 2197c62a0a6e..9675814f64db 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -1760,6 +1760,10 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> u8 dialog_token, u16 timeout,
> u16 start_seq_num, u16 ba_policy, u16 tid,
> u16 buf_size, bool tx, bool auto_seq);
> +void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
> + u8 dialog_token, u16 timeout,
> + u16 start_seq_num, u16 ba_policy, u16 tid,
> + u16 buf_size, bool tx, bool auto_seq);
> void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
> enum ieee80211_agg_stop_reason reason);
> void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
>
> johannes
I confirm that this patch fixes the hang too.
I'm curious to see if there are noticeable performance differences
between the two solutions.
Cheers,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* Re: [PATCH v2 rfc 8/8] net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID
From: Andrew Lunn @ 2017-09-06 15:08 UTC (permalink / raw)
To: Vivien Didelot; +Cc: netdev, jiri, nikolay, Florian Fainelli
In-Reply-To: <87d17351gn.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
> > Use the MAC address of the master interface as the parent ID. This is
> > the same for all switches in a cluster, and should be unique if there
> > are multiple clusters.
>
> That is not correct. Support for multiple CPU ports is coming and in
> this case, you can have two CPU host interfaces wired to two switch
> ports of the same tree. So two different master MAC addresses.
Yes, you are correct. I will change this.
Andrew
^ permalink raw reply
* [PATCH 0/2] 9p: Fixes for hard-to-hit bugs
From: Tuomas Tynkkynen @ 2017-09-06 14:59 UTC (permalink / raw)
To: Al Viro
Cc: v9fs-developer, Eric Van Hensbergen, Ron Minnich,
Latchesar Ionkov, David S. Miller, linux-kernel, netdev,
linux-fsdevel, Tuomas Tynkkynen
These two patches fix two hard-to-hit (but really annoying) bugs in 9p.
The first one was posted earlier in February (with one R-b), the second
is a new one.
Both of these have had soaking in NixOS distribution kernels for
a couple of months with no ill effects.
Tuomas Tynkkynen (2):
fs/9p: Compare qid.path in v9fs_test_inode
net/9p: Switch to wait_event_killable()
fs/9p/vfs_inode.c | 3 +++
fs/9p/vfs_inode_dotl.c | 3 +++
net/9p/client.c | 3 +--
net/9p/trans_virtio.c | 13 ++++++-------
net/9p/trans_xen.c | 4 ++--
5 files changed, 15 insertions(+), 11 deletions(-)
--
2.13.0
^ permalink raw reply
* [PATCH 1/2] fs/9p: Compare qid.path in v9fs_test_inode
From: Tuomas Tynkkynen @ 2017-09-06 14:59 UTC (permalink / raw)
To: Al Viro
Cc: v9fs-developer, Eric Van Hensbergen, Ron Minnich,
Latchesar Ionkov, David S. Miller, linux-kernel, netdev,
linux-fsdevel, Tuomas Tynkkynen, stable
In-Reply-To: <20170906145908.8082-1-tuomas@tuxera.com>
Commit fd2421f54423 ("fs/9p: When doing inode lookup compare qid details
and inode mode bits.") transformed v9fs_qid_iget() to use iget5_locked()
instead of iget_locked(). However, the test() callback is not checking
fid.path at all, which means that a lookup in the inode cache can now
accidentally locate a completely wrong inode from the same inode hash
bucket if the other fields (qid.type and qid.version) match.
Fixes: fd2421f54423 ("fs/9p: When doing inode lookup compare qid details and inode mode bits.")
Cc: stable@vger.kernel.org
Reviewed-by: Latchesar Ionkov <lucho@ionkov.net>
Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>
---
fs/9p/vfs_inode.c | 3 +++
fs/9p/vfs_inode_dotl.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 2a5de610dd8f..bdabb2765d1b 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -483,6 +483,9 @@ static int v9fs_test_inode(struct inode *inode, void *data)
if (v9inode->qid.type != st->qid.type)
return 0;
+
+ if (v9inode->qid.path != st->qid.path)
+ return 0;
return 1;
}
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 70f9887c59a9..7f6ae21a27b3 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -87,6 +87,9 @@ static int v9fs_test_inode_dotl(struct inode *inode, void *data)
if (v9inode->qid.type != st->qid.type)
return 0;
+
+ if (v9inode->qid.path != st->qid.path)
+ return 0;
return 1;
}
--
2.13.0
^ permalink raw reply related
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 15:11 UTC (permalink / raw)
To: Matteo Croce; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <CAGnkfhyBMKT-Tf10NxjN40Znuj7OK2pSM3qLUidPSkQqh8gjwQ@mail.gmail.com>
On Wed, 2017-09-06 at 17:04 +0200, Matteo Croce wrote:
>
> I confirm that this patch fixes the hang too.
Cool, I'll go apply it.
> I'm curious to see if there are noticeable performance differences
> between the two solutions.
Nope, you hit this code path essentially once.
johannes
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Andrew Lunn @ 2017-09-06 15:16 UTC (permalink / raw)
To: Matthias May
Cc: netdev, Florian Fainelli, Vivien Didelot, Woojung.Huh, jbe,
sean.wang, john
In-Reply-To: <edf02eb7-e6bf-e88c-f655-40f6f71a3446@neratec.com>
On Wed, Sep 06, 2017 at 04:46:51PM +0200, Matthias May wrote:
>
> Hi Andrew
> We are using the 88E6321.
> In our setup we are using openvswitch and not a bridge, however the problem you describe seems to be the same.
>
> We had to configure the switch to flood unknown multicast (Egress Floods = 0x3, bits 3:2, offset 0x4 in port control)
> and
> unset FloodBC (FloodBC = 0x0, bit 12, offset 0x5 in global 2) which defines if a broadcast should be considered as
> multicast for the above config.
Hi Matthias
I might look at this.
But architecturally it seems better to add an mdb entry for the
broadcast address. I hope that work across all switches, where as what
you suggests only works for Marvell devices.
Andrew
^ permalink raw reply
* Re: [PATCH 6/10] ixgbe: Use ARRAY_SIZE macro
From: Joe Perches @ 2017-09-06 15:19 UTC (permalink / raw)
To: Thomas Meyer, David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20170906090817.f7qir5okeeajfcsc@olymp>
On Wed, 2017-09-06 at 11:08 +0200, Thomas Meyer wrote:
> On Tue, Sep 05, 2017 at 02:22:05PM -0700, David Miller wrote:
> > nothing prevented you from CC:'ing the maintainer,
> > Jeff Kirscher.
[]
> That's the other condition in my script. I only send to the role
> "maintainer" from the output of get_maintainer.pl. But Mr Jeff
> Kirscher is only listed as supporter...
Supporter means he gets paid to look after that
subsystem so he is something other than a volunteer.
from MAINTAINERS:
S: Status, one of the following:
Supported: Someone is actually paid to look after this.
Maintained: Someone actually looks after it.
Odd Fixes: It has a maintainer but they don't have time to do
much other than throw the odd patch in. See below..
Orphan: No current maintainer [but maybe you could take the
role as you write your new code].
Obsolete: Old code. Something tagged obsolete generally means
it has been replaced by a better system and you
should be using that.
^ permalink raw reply
* [PATCH net-next 0/1] netvsc hot plug fix
From: Stephen Hemminger @ 2017-09-06 15:19 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
This is one fix for problems with hot add of network device
on Windows Server.
Stephen Hemminger (1):
hv_netvsc: fix deadlock on hotplug
drivers/net/hyperv/hyperv_net.h | 1 +
drivers/net/hyperv/netvsc_drv.c | 8 +--
drivers/net/hyperv/rndis_filter.c | 106 ++++++++++++++++++++++++++------------
3 files changed, 74 insertions(+), 41 deletions(-)
--
2.11.0
^ permalink raw reply
* [PATCH net-next 1/1] hv_netvsc: fix deadlock on hotplug
From: Stephen Hemminger @ 2017-09-06 15:19 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170906151925.15221-1-sthemmin@microsoft.com>
When a virtual device is added dynamically (via host console), then
the vmbus sends an offer message for the primary channel. The processing
of this message for networking causes the network device to then
initialize the sub channels.
The problem is that setting up the sub channels needs to wait until
the subsequent subchannel offers have been processed. These offers
come in on the same ring buffer and work queue as where the primary
offer is being processed; leading to a deadlock.
This did not happen in older kernels, because the sub channel waiting
logic was broken (it wasn't really waiting).
The solution is to do the sub channel setup in its own work queue
context that is scheduled by the primary channel setup; and then
happens later.
Fixes: 732e49850c5e ("netvsc: fix race on sub channel creation")
Reported-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
Should also go to stable, but this version does not apply cleanly
to 4.13. Have another patch for that.
drivers/net/hyperv/hyperv_net.h | 1 +
drivers/net/hyperv/netvsc_drv.c | 8 +--
drivers/net/hyperv/rndis_filter.c | 106 ++++++++++++++++++++++++++------------
3 files changed, 74 insertions(+), 41 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index ec546da86683..d1df5050adfb 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -172,6 +172,7 @@ struct rndis_device {
struct list_head req_list;
struct work_struct mcast_work;
+ struct work_struct subchan_work;
bool link_state; /* 0 - link up, 1 - link down */
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 165ba4b3b423..80f45e0f81c8 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -853,10 +853,7 @@ static int netvsc_set_channels(struct net_device *net,
rndis_filter_device_remove(dev, nvdev);
nvdev = rndis_filter_device_add(dev, &device_info);
- if (!IS_ERR(nvdev)) {
- netif_set_real_num_tx_queues(net, nvdev->num_chn);
- netif_set_real_num_rx_queues(net, nvdev->num_chn);
- } else {
+ if (IS_ERR(nvdev)) {
ret = PTR_ERR(nvdev);
device_info.num_chn = orig;
nvdev = rndis_filter_device_add(dev, &device_info);
@@ -1954,9 +1951,6 @@ static int netvsc_probe(struct hv_device *dev,
NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
net->vlan_features = net->features;
- netif_set_real_num_tx_queues(net, nvdev->num_chn);
- netif_set_real_num_rx_queues(net, nvdev->num_chn);
-
netdev_lockdep_set_classes(net);
/* MTU range: 68 - 1500 or 65521 */
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 69c40b8fccc3..c240125eed48 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -33,6 +33,7 @@
#include "hyperv_net.h"
static void rndis_set_multicast(struct work_struct *w);
+static void rndis_set_subchannel(struct work_struct *w);
#define RNDIS_EXT_LEN PAGE_SIZE
struct rndis_request {
@@ -79,6 +80,7 @@ static struct rndis_device *get_rndis_device(void)
INIT_LIST_HEAD(&device->req_list);
INIT_WORK(&device->mcast_work, rndis_set_multicast);
+ INIT_WORK(&device->subchan_work, rndis_set_subchannel);
device->state = RNDIS_DEV_UNINITIALIZED;
@@ -1002,6 +1004,7 @@ static int rndis_filter_close_device(struct rndis_device *dev)
/* Make sure rndis_set_multicast doesn't re-enable filter! */
cancel_work_sync(&dev->mcast_work);
+ cancel_work_sync(&dev->subchan_work);
ret = rndis_filter_set_packet_filter(dev, 0);
if (ret == -ENODEV)
@@ -1054,6 +1057,71 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
wake_up(&nvscdev->subchan_open);
}
+/*
+ * Open sub-channels after completing the handling of the device probe.
+ * This breaks overlap of processing the host message for the
+ * new primary channel with the initialization of sub-channels.
+ */
+static void rndis_set_subchannel(struct work_struct *w)
+{
+ struct rndis_device *rdev
+ = container_of(w, struct rndis_device, subchan_work);
+ struct net_device *net = rdev->ndev;
+ struct net_device_context *ndev_ctx = netdev_priv(net);
+ struct hv_device *dev = ndev_ctx->device_ctx;
+ struct nvsp_message *init_packet;
+ struct netvsc_device *nvdev;
+ int ret = -ENODEV;
+
+ if (!rtnl_trylock()) {
+ schedule_work(w);
+ return;
+ }
+
+ nvdev = rtnl_dereference(ndev_ctx->nvdev);
+ if (!nvdev)
+ goto out;
+
+ init_packet = &nvdev->channel_init_pkt;
+ memset(init_packet, 0, sizeof(struct nvsp_message));
+ init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
+ init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
+ init_packet->msg.v5_msg.subchn_req.num_subchannels =
+ nvdev->num_chn - 1;
+ ret = vmbus_sendpacket(dev->channel, init_packet,
+ sizeof(struct nvsp_message),
+ (unsigned long)init_packet,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret)
+ goto out;
+
+ wait_for_completion(&nvdev->channel_init_wait);
+ if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
+ netdev_err(net, "sub channel request failed\n");
+ goto out;
+ }
+
+ nvdev->num_chn = 1 +
+ init_packet->msg.v5_msg.subchn_comp.num_subchannels;
+
+ /* wait for all sub channels to open */
+ wait_event(nvdev->subchan_open,
+ atomic_read(&nvdev->open_chn) == nvdev->num_chn);
+
+ /* ignore failues from setting rss parameters, still have channels */
+ rndis_filter_set_rss_param(rdev, netvsc_hash_key);
+out:
+ if (ret) {
+ nvdev->max_chn = 1;
+ nvdev->num_chn = 1;
+ }
+
+ netif_set_real_num_tx_queues(net, nvdev->num_chn);
+ netif_set_real_num_rx_queues(net, nvdev->num_chn);
+ rtnl_unlock();
+}
+
struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
struct netvsc_device_info *device_info)
{
@@ -1063,7 +1131,6 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
struct rndis_device *rndis_device;
struct ndis_offload hwcaps;
struct ndis_offload_params offloads;
- struct nvsp_message *init_packet;
struct ndis_recv_scale_cap rsscap;
u32 rsscap_size = sizeof(struct ndis_recv_scale_cap);
unsigned int gso_max_size = GSO_MAX_SIZE;
@@ -1215,9 +1282,7 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
net_device->num_chn);
atomic_set(&net_device->open_chn, 1);
-
- if (net_device->num_chn == 1)
- return net_device;
+ vmbus_set_sc_create_callback(dev->channel, netvsc_sc_open);
for (i = 1; i < net_device->num_chn; i++) {
ret = netvsc_alloc_recv_comp_ring(net_device, i);
@@ -1228,38 +1293,11 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
}
}
- vmbus_set_sc_create_callback(dev->channel, netvsc_sc_open);
-
- init_packet = &net_device->channel_init_pkt;
- memset(init_packet, 0, sizeof(struct nvsp_message));
- init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
- init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
- init_packet->msg.v5_msg.subchn_req.num_subchannels =
- net_device->num_chn - 1;
- ret = vmbus_sendpacket(dev->channel, init_packet,
- sizeof(struct nvsp_message),
- (unsigned long)init_packet,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (ret)
- goto out;
-
- wait_for_completion(&net_device->channel_init_wait);
- if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
- ret = -ENODEV;
- goto out;
- }
-
- net_device->num_chn = 1 +
- init_packet->msg.v5_msg.subchn_comp.num_subchannels;
-
- /* wait for all sub channels to open */
- wait_event(net_device->subchan_open,
- atomic_read(&net_device->open_chn) == net_device->num_chn);
+ if (net_device->num_chn > 1)
+ schedule_work(&rndis_device->subchan_work);
- /* ignore failues from setting rss parameters, still have channels */
- rndis_filter_set_rss_param(rndis_device, netvsc_hash_key);
out:
+ /* if unavailable, just proceed with one queue */
if (ret) {
net_device->max_chn = 1;
net_device->num_chn = 1;
--
2.11.0
^ permalink raw reply related
* Re: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()
From: Stephen Hemminger @ 2017-09-06 15:25 UTC (permalink / raw)
To: David Laight; +Cc: Phil Sutter, netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD006EC26@AcuExch.aculab.com>
On Wed, 6 Sep 2017 13:59:27 +0000
David Laight <David.Laight@ACULAB.COM> wrote:
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: 04 September 2017 19:25
> > On Mon, 4 Sep 2017 17:00:15 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> >
> > > On Mon, Sep 04, 2017 at 02:49:20PM +0000, David Laight wrote:
> > > > From: Phil Sutter
> > > > > Sent: 01 September 2017 17:53
> > > > > By making use of strncpy(), both implementations are really simple so
> > > > > there is no need to add libbsd as additional dependency.
> > > > >
> > > > ...
> > > > > +
> > > > > +size_t strlcpy(char *dst, const char *src, size_t size)
> > > > > +{
> > > > > + if (size) {
> > > > > + strncpy(dst, src, size - 1);
> > > > > + dst[size - 1] = '\0';
> > > > > + }
> > > > > + return strlen(src);
> > > > > +}
> > > >
> > > > Except that isn't really strlcpy().
> > > > Better would be:
> > > > len = strlen(src) + 1;
> > > > if (len <= size)
> > > > memcpy(dst, src, len);
> > > > else if (size) {
> > > > dst[size - 1] = 0;
> > > > memcpy(dst, src, size - 1);
> > > > }
> > > > return len - 1;
> > >
> > > Please elaborate: Why isn't my version "really" strlcpy()? Why is your
> > > proposed version better?
> > >
> > > Thanks, Phil
> >
> > Linux kernel:
> > size_t strlcpy(char *dest, const char *src, size_t size)
> > {
> > size_t ret = strlen(src);
> >
> > if (size) {
> > size_t len = (ret >= size) ? size - 1 : ret;
> > memcpy(dest, src, len);
> > dest[len] = '\0';
> > }
> > return ret;
> > }
> >
> > FreeBSD:
> > size_t
> > strlcpy(char * __restrict dst, const char * __restrict src, size_t dsize)
> > {
> > const char *osrc = src;
> > size_t nleft = dsize;
> >
> > /* Copy as many bytes as will fit. */
> > if (nleft != 0) {
> > while (--nleft != 0) {
> > if ((*dst++ = *src++) == '\0')
> > break;
> > }
> > }
> >
> > /* Not enough room in dst, add NUL and traverse rest of src. */
> > if (nleft == 0) {
> > if (dsize != 0)
> > *dst = '\0'; /* NUL-terminate dst */
> > while (*src++)
> > ;
> > }
> >
> > return(src - osrc - 1); /* count does not include NUL */
> > }
> >
> >
> > They all give the same results for some basic tests.
> > Test FreeBSD Linux Iproute2
> > "",0: 0 "JUNK" 0 "JUNK" 0 "JUNK"
> > "",1: 0 "" 0 "" 0 ""
> > "",8: 0 "" 0 "" 0 ""
> > "foo",0: 3 "JUNK" 3 "JUNK" 3 "JUNK"
> > "foo",3: 3 "fo" 3 "fo" 3 "fo"
> > "foo",4: 3 "foo" 3 "foo" 3 "foo"
> > "foo",8: 3 "foo" 3 "foo" 3 "foo"
> > "longstring",0: 10 "JUNK" 10 "JUNK" 10 "JUNK"
> > "longstring",8: 10 "longstr" 10 "longstr" 10 "longstr"
>
> You need to look at the contents of the destination buffer after the
> first '\0'.
> strlcpy() shouldn't change it.
>
> David
Zeroing the bytes after the first null character should not be a big issue
other than a few nanoseconds extra work.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox