* Re: [patch net-next v2 10/11] mlxsw: spectrum_router: Request a dump of FIB tables during init
From: Jiri Pirko @ 2016-11-23 17:04 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: netdev, davem, idosch, eladr, yotamg, nogahf, arkadis, ogerlitz,
roopa, dsa, nikolay, andy, vivien.didelot, andrew, f.fainelli,
alexander.h.duyck, kaber
In-Reply-To: <1479920345.4035504.797158425.2C10AA0C@webmail.messagingengine.com>
Wed, Nov 23, 2016 at 05:59:05PM CET, hannes@stressinduktion.org wrote:
>On Wed, Nov 23, 2016, at 17:04, Jiri Pirko wrote:
>> Wed, Nov 23, 2016 at 05:00:00PM CET, hannes@stressinduktion.org wrote:
>> >On Wed, Nov 23, 2016, at 15:48, Jiri Pirko wrote:
>> >> From: Ido Schimmel <idosch@mellanox.com>
>> >>
>> >> Make sure the device has a complete view of the FIB tables by invoking
>> >> their dump during module init.
>> >>
>> >> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> >> ---
>> >> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 16
>> >> ++++++++++++++++
>> >> 1 file changed, 16 insertions(+)
>> >>
>> >> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> >> b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> >> index 14bed1d..36a71d2 100644
>> >> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> >> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> >> @@ -2027,6 +2027,21 @@ static int mlxsw_sp_router_fib_event(struct
>> >> notifier_block *nb,
>> >> return NOTIFY_DONE;
>> >> }
>> >>
>> >> +static void mlxsw_sp_router_fib_dump(struct mlxsw_sp *mlxsw_sp)
>> >> +{
>> >> + while (!fib_notifier_dump(&mlxsw_sp->fib_nb)) {
>> >> + /* Flush pending FIB notifications and then flush the
>> >> + * device's table before requesting another dump. Do
>> >> + * that with RTNL held, as FIB notification block is
>> >> + * already registered.
>> >> + */
>> >> + mlxsw_core_flush_owq();
>> >> + rtnl_lock();
>> >> + mlxsw_sp_router_fib_flush(mlxsw_sp);
>> >> + rtnl_unlock();
>> >> + }
>> >> +}
>> >
>> >I think it is fine to use this kind of synchronization.
>> >
>> >But I think that this part of the logic still belongs into the core
>>
>> Core does not know how driver handles the offloaded fibs. So only driver
>> knows how/if he needs to do flush in case of retry.
>
>Sure, but an abort function can be provided to the kernel anyway and the
>driver can care about that.
Ok, how?
>
>> >kernel. I still think it could happen that we will loop here
>> >indefinitely because of a lot of routing updates and as such would need
>> >to abort this loop after a number of tries.
>>
>> In theory, it is possible, howevery quite unlikely.
>
>I think the "quite unlikely" already got us down the path to not using
>rtnl_lock in the first place.
>
>As I said, I am not sure about this as I didn't try any hardware
>offloading before and delays how long it needs to be transferred to
>hardware, but having a fail case for that seems like a nice improvement.
>At the same time I know of Linux boxes running in internet exchanges
>having several peers. The high update rates actually led to bgp
>implementation specifying flap damping which is actually nowadays
>considered harmful.
>
>Seriously, while most of the time convergence in routing protocols is
>good and most updates only hit the BGP user space table anyway and the
>change is suppressed because recursive routing lookup idempotence, quite
>unlikely events happen to the internet now and then:
>http://research.dyn.com/2009/02/longer-is-not-better/, which caused *a
>lot* of flapping and ongoing events on BGP routers throughout the world.
>
>I agree it is unlikely that you have to refresh your hw dump during this
>time, but who knows what customers do and what admins do in case
>something like this happens. I just don't favor to looping endlessly
>trying to sync up and getting into a stable state but tell the admin to
>detach the control plane from the forwarding plane and sync up then.
>
>That said, I think a sysctl for a maximum number of loops respected by
>drivers that needs to do so, should be enough for the time being.
Okay. Point taken.
^ permalink raw reply
* Re: [patch net-next v2 10/11] mlxsw: spectrum_router: Request a dump of FIB tables during init
From: Hannes Frederic Sowa @ 2016-11-23 17:08 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, idosch, eladr, yotamg, nogahf, arkadis, ogerlitz,
roopa, dsa, nikolay, andy, vivien.didelot, andrew, f.fainelli,
alexander.h.duyck, kaber
In-Reply-To: <20161123170436.GC1873@nanopsycho>
On Wed, Nov 23, 2016, at 18:04, Jiri Pirko wrote:
> >Sure, but an abort function can be provided to the kernel anyway and the
> >driver can care about that.
>
> Ok, how?
I think just a sysctl ontop of this series is enough plus a pr_warn.
Rocker and mlxsw are responsible to loop for a maximum amount of time.
Otherwise, if more fancy, can we provide an
fib_inconsistency_notification function pointer in netdev_ops?
Bye and thanks,
Hannes
^ permalink raw reply
* [PATCH v3] net: dsa: mv88e6xxx: enable EDSA on mv88e6097
From: Stefan Eichenberger @ 2016-11-23 17:11 UTC (permalink / raw)
To: andrew, vivien.didelot; +Cc: f.fainelli, netdev, Stefan Eichenberger
In-Reply-To: <20161123165949.GB8760@lunn.ch>
EDSA is currently disabled on mv88e6097 devices, this commit enables it.
Signed-off-by: Stefan Eichenberger <stefan.eichenberger@netmodule.com>
---
drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index ab52c37..a2ff1fc 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -543,7 +543,8 @@ enum mv88e6xxx_cap {
MV88E6XXX_FLAGS_MULTI_CHIP)
#define MV88E6XXX_FLAGS_FAMILY_6097 \
- (MV88E6XXX_FLAG_G1_ATU_FID | \
+ (MV88E6XXX_FLAG_EDSA | \
+ MV88E6XXX_FLAG_G1_ATU_FID | \
MV88E6XXX_FLAG_G1_VTU_FID | \
MV88E6XXX_FLAG_GLOBAL2 | \
MV88E6XXX_FLAG_G2_MGMT_EN_2X | \
--
2.9.3
^ permalink raw reply related
* Re: [PATCH net-next] net: properly flush delay-freed skbs
From: Alexander Duyck @ 2016-11-23 17:12 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Jesper Dangaard Brouer, Alexander Duyck
In-Reply-To: <1479919496.8455.509.camel@edumazet-glaptop3.roam.corp.google.com>
On Wed, Nov 23, 2016 at 8:44 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Typical NAPI drivers use napi_consume_skb(skb) at TX completion time.
> This put skb in a percpu special queue, napi_alloc_cache, to get bulk
> frees.
>
> It turns out the queue is not flushed and hits the NAPI_SKB_CACHE_SIZE
> limit quite often, with skbs that were queued hundreds of usec earlier.
> I measured this can take ~6000 nsec to perform one flush.
>
> __kfree_skb_flush() can be called from two points right now :
>
> 1) From net_tx_action(), but only for skbs that were queued to
> sd->completion_queue.
>
> -> Irrelevant for NAPI drivers in normal operation.
>
> 2) From net_rx_action(), but only under high stress or if RPS/RFS has a
> pending action.
>
> This patch changes net_rx_action() to perform the flush in all cases and
> after more urgent operations happened (like kicking remote CPUS for
> RPS/RFS).
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
Yeah, we didn't intent the data to be sitting around that long. The
change looks good to me.
Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>
^ permalink raw reply
* Re: [PATCH v3] net: dsa: mv88e6xxx: enable EDSA on mv88e6097
From: Andrew Lunn @ 2016-11-23 17:13 UTC (permalink / raw)
To: Stefan Eichenberger
Cc: vivien.didelot, f.fainelli, netdev, Stefan Eichenberger
In-Reply-To: <20161123171135.9768-1-stefan.eichenberger@netmodule.com>
On Wed, Nov 23, 2016 at 06:11:35PM +0100, Stefan Eichenberger wrote:
> EDSA is currently disabled on mv88e6097 devices, this commit enables it.
And was that sufficient to fix all your issues?
Andrew
^ permalink raw reply
* Re: [PATCH v2] net: dsa: mv88e6xxx: forward unknown mc packets on mv88e6097
From: Stefan Eichenberger @ 2016-11-23 17:14 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Stefan Eichenberger, vivien.didelot, f.fainelli, netdev
In-Reply-To: <20161123165949.GB8760@lunn.ch>
On Wed, Nov 23, 2016 at 05:59:49PM +0100, Andrew Lunn wrote:
> On Wed, Nov 23, 2016 at 05:54:40PM +0100, Stefan Eichenberger wrote:
> > Packets with unknown destination addresses are not forwarded to the cpu
> > port on mv88e6097 based switches (e.g. MV88E6097) at the moment. This
> > commit enables PORT_CONTROL_FORWARD_UNKNOWN_MC for this family.
>
> Please try adding MV88E6XXX_FLAG_EDSA to
> MV88E6XXX_FLAGS_FAMILY_6097. That is the better fix if it works.
I was even wondering what EDSA means:) Thanks this solved the problem!
Regards
Stefan
^ permalink raw reply
* Re: [PATCH] drivers: net: davinci_mdio: use builtin_platform_driver
From: Grygorii Strashko @ 2016-11-23 17:22 UTC (permalink / raw)
To: Geliang Tang, Mugunthan V N; +Cc: linux-omap, netdev, linux-kernel
In-Reply-To: <055763562f90fd7e2d311308e1d731ba93c3eea9.1479912302.git.geliangtang@gmail.com>
On 11/23/2016 08:45 AM, Geliang Tang wrote:
> Use builtin_platform_driver() helper to simplify the code.
Not sure about this. We do support this driver to be a module.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> drivers/net/ethernet/ti/davinci_mdio.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
> index 33df340..b3f0a12 100644
> --- a/drivers/net/ethernet/ti/davinci_mdio.c
> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
> @@ -536,11 +536,7 @@ static struct platform_driver davinci_mdio_driver = {
> .remove = davinci_mdio_remove,
> };
>
> -static int __init davinci_mdio_init(void)
> -{
> - return platform_driver_register(&davinci_mdio_driver);
> -}
> -device_initcall(davinci_mdio_init);
> +builtin_platform_driver(davinci_mdio_driver);
>
> static void __exit davinci_mdio_exit(void)
> {
>
--
regards,
-grygorii
^ permalink raw reply
* Re: [PATCH v2] net: dsa: mv88e6xxx: forward unknown mc packets on mv88e6097
From: Andrew Lunn @ 2016-11-23 17:32 UTC (permalink / raw)
To: Stefan Eichenberger
Cc: Stefan Eichenberger, vivien.didelot, f.fainelli, netdev
In-Reply-To: <20161123171441.GE12698@gruene.netmodule.intranet>
On Wed, Nov 23, 2016 at 06:14:41PM +0100, Stefan Eichenberger wrote:
> On Wed, Nov 23, 2016 at 05:59:49PM +0100, Andrew Lunn wrote:
> > On Wed, Nov 23, 2016 at 05:54:40PM +0100, Stefan Eichenberger wrote:
> > > Packets with unknown destination addresses are not forwarded to the cpu
> > > port on mv88e6097 based switches (e.g. MV88E6097) at the moment. This
> > > commit enables PORT_CONTROL_FORWARD_UNKNOWN_MC for this family.
> >
> > Please try adding MV88E6XXX_FLAG_EDSA to
> > MV88E6XXX_FLAGS_FAMILY_6097. That is the better fix if it works.
>
> I was even wondering what EDSA means:) Thanks this solved the problem!
Great.
We should fix up a few minor issues and resubmit.
What is the status of the first patch, which added 6097 to the driver?
I don't think David accepted it yet. So lets make one patchset
containing the two patches.
The subject line of the patches need to have net-next in it. e.g.
[PATCH net-next 0/2] Add support for the MV88e6097
Include a cover node, saying what the patchset as a whole does.
This gets used as the merge commit message.
Then the two patches.
When posting the patchset, please start a new thread. A new version of
a patchset or patch should be a new thread.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers
From: Florian Fainelli @ 2016-11-23 17:33 UTC (permalink / raw)
To: yegorslists, netdev
Cc: linux-omap, grygorii.strashko, mugunthanvnm, roszenrami
In-Reply-To: <1479911913-1761-1-git-send-email-yegorslists@googlemail.com>
On 11/23/2016 06:38 AM, yegorslists@googlemail.com wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
>
> Add the ability to query and set Energy Efficient Ethernet parameters
> via ethtool for applicable devices.
Are you sure this is enough to actually enable EEE? I don't see where
phy_init_eee() is called here, nor is the cpsw Ethernet controller part
configured to enable/disable EEE. EEE is not just a PHY thing, it
usually also needs to be configured properly at the Ethernet MAC/switch
level as well.
Just curious here.
--
Florian
^ permalink raw reply
* Re: [PATCH v2] net: dsa: mv88e6xxx: forward unknown mc packets on mv88e6097
From: Andrew Lunn @ 2016-11-23 17:40 UTC (permalink / raw)
To: Stefan Eichenberger
Cc: Stefan Eichenberger, vivien.didelot, f.fainelli, netdev
In-Reply-To: <20161123171441.GE12698@gruene.netmodule.intranet>
On Wed, Nov 23, 2016 at 06:14:41PM +0100, Stefan Eichenberger wrote:
> On Wed, Nov 23, 2016 at 05:59:49PM +0100, Andrew Lunn wrote:
> > On Wed, Nov 23, 2016 at 05:54:40PM +0100, Stefan Eichenberger wrote:
> > > Packets with unknown destination addresses are not forwarded to the cpu
> > > port on mv88e6097 based switches (e.g. MV88E6097) at the moment. This
> > > commit enables PORT_CONTROL_FORWARD_UNKNOWN_MC for this family.
> >
> > Please try adding MV88E6XXX_FLAG_EDSA to
> > MV88E6XXX_FLAGS_FAMILY_6097. That is the better fix if it works.
>
> I was even wondering what EDSA means:) Thanks this solved the problem!
Plain DSA puts four bytes of header between the MAC source address and
the EtherType/Length.
EDSA puts in an 8 byte header, and includes an Ethertype value of
0xdada. Having that ethertype value makes it more obvious what is
going on. And if you have a recent version of tcpdump, it will decode
the header.
Andrew
^ permalink raw reply
* [PATCH net-next] mlx4: do not use priv->stats_lock in mlx4_en_auto_moderation()
From: Eric Dumazet @ 2016-11-23 17:46 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Tariq Toukan
From: Eric Dumazet <edumazet@google.com>
Per RX ring packets/bytes counters are not protected by global
priv->stats_lock.
Better not confuse the reader, and use READ_ONCE() to show we read
these counters without surrounding synchronization.
Interrupt moderation is best effort, and we do not really care of
ultra precise counters.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 9a807e93c9fdd81e61e561208aa1480a244d0bdb..b964bdcd4ae509a7e693215e8b32f040218e252c 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1391,10 +1391,8 @@ static void mlx4_en_auto_moderation(struct mlx4_en_priv *priv)
return;
for (ring = 0; ring < priv->rx_ring_num; ring++) {
- spin_lock_bh(&priv->stats_lock);
- rx_packets = priv->rx_ring[ring]->packets;
- rx_bytes = priv->rx_ring[ring]->bytes;
- spin_unlock_bh(&priv->stats_lock);
+ rx_packets = READ_ONCE(priv->rx_ring[ring]->packets);
+ rx_bytes = READ_ONCE(priv->rx_ring[ring]->bytes);
rx_pkt_diff = ((unsigned long) (rx_packets -
priv->last_moder_packets[ring]));
^ permalink raw reply related
* Re: [patch net-next v2 09/11] ipv4: fib: Add an API to request a FIB dump
From: Hannes Frederic Sowa @ 2016-11-23 17:47 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: davem, idosch, eladr, yotamg, nogahf, arkadis, ogerlitz, roopa,
dsa, nikolay, andy, vivien.didelot, andrew, f.fainelli,
alexander.h.duyck, kaber
In-Reply-To: <1479911670-4525-10-git-send-email-jiri@resnulli.us>
On 23.11.2016 15:34, Jiri Pirko wrote:
> From: Ido Schimmel <idosch@mellanox.com>
>
> Commit b90eb7549499 ("fib: introduce FIB notification infrastructure")
> introduced a new notification chain to notify listeners (f.e., switchdev
> drivers) about addition and deletion of routes.
>
> However, upon registration to the chain the FIB tables can already be
> populated, which means potential listeners will have an incomplete view
> of the tables.
>
> Solve that by adding an API to request a FIB dump. The dump itself it
> done using RCU in order not to starve consumers that need RTNL to make
> progress.
>
> For each net namespace the integrity of the dump is ensured by reading
> the atomic change sequence counter before and after the dump. This
> allows us to avoid the problematic situation in which the dumping
> process sends a ENTRY_ADD notification following ENTRY_DEL generated by
> another process holding RTNL.
>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> include/net/ip_fib.h | 1 +
> net/ipv4/fib_trie.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 118 insertions(+)
>
> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
> index 6c67b93..c76303e 100644
> --- a/include/net/ip_fib.h
> +++ b/include/net/ip_fib.h
> @@ -221,6 +221,7 @@ enum fib_event_type {
> FIB_EVENT_RULE_DEL,
> };
>
> +bool fib_notifier_dump(struct notifier_block *nb);
> int register_fib_notifier(struct notifier_block *nb);
> int unregister_fib_notifier(struct notifier_block *nb);
> int call_fib_notifiers(struct net *net, enum fib_event_type event_type,
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index b1d2d09..9770edfe 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -86,6 +86,67 @@
>
> static ATOMIC_NOTIFIER_HEAD(fib_chain);
>
> +static int call_fib_notifier(struct notifier_block *nb, struct net *net,
> + enum fib_event_type event_type,
> + struct fib_notifier_info *info)
> +{
> + info->net = net;
> + return nb->notifier_call(nb, event_type, info);
> +}
> +
> +static void fib_rules_notify(struct net *net, struct notifier_block *nb,
> + enum fib_event_type event_type)
> +{
> +#ifdef CONFIG_IP_MULTIPLE_TABLES
> + struct fib_notifier_info info;
> +
> + if (net->ipv4.fib_has_custom_rules)
> + call_fib_notifier(nb, net, event_type, &info);
> +#endif
> +}
> +
> +static void fib_notify(struct net *net, struct notifier_block *nb,
> + enum fib_event_type event_type);
> +
> +static int call_fib_entry_notifier(struct notifier_block *nb, struct net *net,
> + enum fib_event_type event_type, u32 dst,
> + int dst_len, struct fib_info *fi,
> + u8 tos, u8 type, u32 tb_id, u32 nlflags)
> +{
> + struct fib_entry_notifier_info info = {
> + .dst = dst,
> + .dst_len = dst_len,
> + .fi = fi,
> + .tos = tos,
> + .type = type,
> + .tb_id = tb_id,
> + .nlflags = nlflags,
> + };
> + return call_fib_notifier(nb, net, event_type, &info.info);
> +}
> +
> +bool fib_notifier_dump(struct notifier_block *nb)
> +{
> + struct net *net;
> + bool ret = true;
> + rcu_read_lock();
> + for_each_net_rcu(net) {
> + int fib_seq = atomic_read(&net->ipv4.fib_seq);
> +
> + fib_rules_notify(net, nb, FIB_EVENT_RULE_ADD);
> + fib_notify(net, nb, FIB_EVENT_ENTRY_ADD);
> + if (atomic_read(&net->ipv4.fib_seq) != fib_seq) {
> + ret = false;
> + goto out_unlock;
> + }
Hmm, I think you need to read the sequence counter under rtnl_lock to
have an ordering with the rest of the updates to the RCU trie. Otherwise
you don't know if the fib trie has the correct view regarding to the
incoming notifications as a whole. This is also necessary during restarts.
You can also try to register the notifier after the dump and check for
the sequence number after registering the notifier, maybe that is easier
(and restart unregisters and does the same).
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH v2] net: dsa: mv88e6xxx: forward unknown mc packets on mv88e6097
From: Stefan Eichenberger @ 2016-11-23 17:49 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Stefan Eichenberger, vivien.didelot, f.fainelli, netdev
In-Reply-To: <20161123173230.GD8760@lunn.ch>
On Wed, Nov 23, 2016 at 06:32:30PM +0100, Andrew Lunn wrote:
> On Wed, Nov 23, 2016 at 06:14:41PM +0100, Stefan Eichenberger wrote:
> > On Wed, Nov 23, 2016 at 05:59:49PM +0100, Andrew Lunn wrote:
> > > On Wed, Nov 23, 2016 at 05:54:40PM +0100, Stefan Eichenberger wrote:
> > > > Packets with unknown destination addresses are not forwarded to the cpu
> > > > port on mv88e6097 based switches (e.g. MV88E6097) at the moment. This
> > > > commit enables PORT_CONTROL_FORWARD_UNKNOWN_MC for this family.
> > >
> > > Please try adding MV88E6XXX_FLAG_EDSA to
> > > MV88E6XXX_FLAGS_FAMILY_6097. That is the better fix if it works.
> >
> > I was even wondering what EDSA means:) Thanks this solved the problem!
>
> Great.
>
> We should fix up a few minor issues and resubmit.
>
> What is the status of the first patch, which added 6097 to the driver?
> I don't think David accepted it yet. So lets make one patchset
> containing the two patches.
>
> The subject line of the patches need to have net-next in it. e.g.
>
> [PATCH net-next 0/2] Add support for the MV88e6097
>
> Include a cover node, saying what the patchset as a whole does.
> This gets used as the merge commit message.
>
> Then the two patches.
Perfect, thanks a lot for the help! The patchset will follow.
Thanks
Stefan
^ permalink raw reply
* Re: [PATCH v2] net: dsa: mv88e6xxx: forward unknown mc packets on mv88e6097
From: Vivien Didelot @ 2016-11-23 17:52 UTC (permalink / raw)
To: Andrew Lunn, Stefan Eichenberger; +Cc: Stefan Eichenberger, f.fainelli, netdev
In-Reply-To: <20161123174040.GE8760@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> And if you have a recent version of tcpdump, it will decode
> the header.
Since d729eb4, thanks to you Andrew ;-)
I move up the cleanup of ports setup in my priority list. The code is
quite cluttered at the moment and it's hard to read through it. We need
proper helpers for egress floods, (E)DSA setup, etc. like what is being
done for the other devices.
Thanks,
Vivien
^ permalink raw reply
* [PATCH net-next 0/2] Add support for the MV88e6097
From: Stefan Eichenberger @ 2016-11-23 17:55 UTC (permalink / raw)
To: andrew, vivien.didelot, davem; +Cc: netdev, Stefan Eichenberger
This patchset will add support for the MV88E6097 DSA switch and enable
EDSA on MV88E6097 family devices.
Stefan Eichenberger (2):
net: dsa: mv88e6xxx: add MV88E6097 switch
net: dsa: mv88e6xxx: enable EDSA on mv88e6097
drivers/net/dsa/mv88e6xxx/chip.c | 26 ++++++++++++++++++++++++++
drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 5 ++++-
2 files changed, 30 insertions(+), 1 deletion(-)
--
2.9.3
^ permalink raw reply
* [PATCH 1/2] net: dsa: mv88e6xxx: add MV88E6097 switch
From: Stefan Eichenberger @ 2016-11-23 17:55 UTC (permalink / raw)
To: andrew, vivien.didelot, davem; +Cc: netdev, Stefan Eichenberger
In-Reply-To: <20161123175546.31416-1-stefan.eichenberger@netmodule.com>
Add support for the MV88E6097 switch. The change was tested on an Armada
based platform with a MV88E6097 switch.
Signed-off-by: Stefan Eichenberger <stefan.eichenberger@netmodule.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 26 ++++++++++++++++++++++++++
drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 2 ++
2 files changed, 28 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index bada646..b14b3d5 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3209,6 +3209,19 @@ static const struct mv88e6xxx_ops mv88e6095_ops = {
.stats_get_stats = mv88e6095_stats_get_stats,
};
+static const struct mv88e6xxx_ops mv88e6097_ops = {
+ .set_switch_mac = mv88e6xxx_g2_set_switch_mac,
+ .phy_read = mv88e6xxx_g2_smi_phy_read,
+ .phy_write = mv88e6xxx_g2_smi_phy_write,
+ .port_set_link = mv88e6xxx_port_set_link,
+ .port_set_duplex = mv88e6xxx_port_set_duplex,
+ .port_set_speed = mv88e6185_port_set_speed,
+ .stats_snapshot = mv88e6xxx_g1_stats_snapshot,
+ .stats_get_sset_count = mv88e6095_stats_get_sset_count,
+ .stats_get_strings = mv88e6095_stats_get_strings,
+ .stats_get_stats = mv88e6095_stats_get_stats,
+};
+
static const struct mv88e6xxx_ops mv88e6123_ops = {
/* MV88E6XXX_FAMILY_6165 */
.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
@@ -3580,6 +3593,19 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.ops = &mv88e6095_ops,
},
+ [MV88E6097] = {
+ .prod_num = PORT_SWITCH_ID_PROD_NUM_6097,
+ .family = MV88E6XXX_FAMILY_6097,
+ .name = "Marvell 88E6097/88E6097F",
+ .num_databases = 4096,
+ .num_ports = 11,
+ .port_base_addr = 0x10,
+ .global1_addr = 0x1b,
+ .age_time_coeff = 15000,
+ .flags = MV88E6XXX_FLAGS_FAMILY_6097,
+ .ops = &mv88e6097_ops,
+ },
+
[MV88E6123] = {
.prod_num = PORT_SWITCH_ID_PROD_NUM_6123,
.family = MV88E6XXX_FAMILY_6165,
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 9298faa..ab52c37 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -81,6 +81,7 @@
#define PORT_SWITCH_ID 0x03
#define PORT_SWITCH_ID_PROD_NUM_6085 0x04a
#define PORT_SWITCH_ID_PROD_NUM_6095 0x095
+#define PORT_SWITCH_ID_PROD_NUM_6097 0x099
#define PORT_SWITCH_ID_PROD_NUM_6131 0x106
#define PORT_SWITCH_ID_PROD_NUM_6320 0x115
#define PORT_SWITCH_ID_PROD_NUM_6123 0x121
@@ -378,6 +379,7 @@
enum mv88e6xxx_model {
MV88E6085,
MV88E6095,
+ MV88E6097,
MV88E6123,
MV88E6131,
MV88E6161,
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v2] net: dsa: mv88e6xxx: forward unknown mc packets on mv88e6097
From: Andrew Lunn @ 2016-11-23 18:01 UTC (permalink / raw)
To: Vivien Didelot; +Cc: Stefan Eichenberger, f.fainelli, netdev
In-Reply-To: <877f7uxevf.fsf@ketchup.i-did-not-set--mail-host-address--so-tickle-me>
On Wed, Nov 23, 2016 at 12:52:52PM -0500, Vivien Didelot wrote:
> Hi Andrew,
>
> Andrew Lunn <andrew@lunn.ch> writes:
>
> > And if you have a recent version of tcpdump, it will decode
> > the header.
>
> Since d729eb4, thanks to you Andrew ;-)
>
> I move up the cleanup of ports setup in my priority list.
Hi Vivien
Please take a look at my mv88e6390 branch. I already refactored this
code, because the mv88e6390 does something slightly different...
I hope to post another batch of mv88e6390 patches soon, and they will
include this cleanup. Since they will clash with these patches, i will
post them first as RFC.
Andrew
^ permalink raw reply
* Re: [PATCH 12/20] net/iucv: Convert to hotplug state machine
From: Ursula Braun @ 2016-11-23 18:04 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, linux-kernel
Cc: rt, David S. Miller, linux-s390, netdev
In-Reply-To: <20161117183541.8588-13-bigeasy@linutronix.de>
Sebastian,
your patch looks good to me. I run successfully some small tests with it.
I want to suggest a small change in iucv_init() to keep the uniform technique
of undo labels below. Do you agree?
Kind regards, Ursula
On 11/17/2016 07:35 PM, Sebastian Andrzej Siewior wrote:
> Install the callbacks via the state machine and let the core invoke the
> callbacks on the already online CPUs. The smp function calls in the
> online/downprep callbacks are not required as the callback is guaranteed to
> be invoked on the upcoming/outgoing cpu.
>
> Cc: Ursula Braun <ubraun@linux.vnet.ibm.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-s390@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> include/linux/cpuhotplug.h | 1 +
> net/iucv/iucv.c | 118 +++++++++++++++++----------------------------
> 2 files changed, 45 insertions(+), 74 deletions(-)
>
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index fd5598b8353a..69abf2c09f6c 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -63,6 +63,7 @@ enum cpuhp_state {
> CPUHP_X86_THERM_PREPARE,
> CPUHP_X86_CPUID_PREPARE,
> CPUHP_X86_MSR_PREPARE,
> + CPUHP_NET_IUCV_PREPARE,
> CPUHP_TIMERS_DEAD,
> CPUHP_NOTF_ERR_INJ_PREPARE,
> CPUHP_MIPS_SOC_PREPARE,
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 88a2a3ba4212..f0d6afc5d4a9 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -639,7 +639,7 @@ static void iucv_disable(void)
> put_online_cpus();
> }
>
> -static void free_iucv_data(int cpu)
> +static int iucv_cpu_dead(unsigned int cpu)
> {
> kfree(iucv_param_irq[cpu]);
> iucv_param_irq[cpu] = NULL;
> @@ -647,9 +647,10 @@ static void free_iucv_data(int cpu)
> iucv_param[cpu] = NULL;
> kfree(iucv_irq_data[cpu]);
> iucv_irq_data[cpu] = NULL;
> + return 0;
> }
>
> -static int alloc_iucv_data(int cpu)
> +static int iucv_cpu_prepare(unsigned int cpu)
> {
> /* Note: GFP_DMA used to get memory below 2G */
> iucv_irq_data[cpu] = kmalloc_node(sizeof(struct iucv_irq_data),
> @@ -671,58 +672,38 @@ static int alloc_iucv_data(int cpu)
> return 0;
>
> out_free:
> - free_iucv_data(cpu);
> + iucv_cpu_dead(cpu);
> return -ENOMEM;
> }
>
> -static int iucv_cpu_notify(struct notifier_block *self,
> - unsigned long action, void *hcpu)
> +static int iucv_cpu_online(unsigned int cpu)
> {
> - cpumask_t cpumask;
> - long cpu = (long) hcpu;
> -
> - switch (action) {
> - case CPU_UP_PREPARE:
> - case CPU_UP_PREPARE_FROZEN:
> - if (alloc_iucv_data(cpu))
> - return notifier_from_errno(-ENOMEM);
> - break;
> - case CPU_UP_CANCELED:
> - case CPU_UP_CANCELED_FROZEN:
> - case CPU_DEAD:
> - case CPU_DEAD_FROZEN:
> - free_iucv_data(cpu);
> - break;
> - case CPU_ONLINE:
> - case CPU_ONLINE_FROZEN:
> - case CPU_DOWN_FAILED:
> - case CPU_DOWN_FAILED_FROZEN:
> - if (!iucv_path_table)
> - break;
> - smp_call_function_single(cpu, iucv_declare_cpu, NULL, 1);
> - break;
> - case CPU_DOWN_PREPARE:
> - case CPU_DOWN_PREPARE_FROZEN:
> - if (!iucv_path_table)
> - break;
> - cpumask_copy(&cpumask, &iucv_buffer_cpumask);
> - cpumask_clear_cpu(cpu, &cpumask);
> - if (cpumask_empty(&cpumask))
> - /* Can't offline last IUCV enabled cpu. */
> - return notifier_from_errno(-EINVAL);
> - smp_call_function_single(cpu, iucv_retrieve_cpu, NULL, 1);
> - if (cpumask_empty(&iucv_irq_cpumask))
> - smp_call_function_single(
> - cpumask_first(&iucv_buffer_cpumask),
> - iucv_allow_cpu, NULL, 1);
> - break;
> - }
> - return NOTIFY_OK;
> + if (!iucv_path_table)
> + return 0;
> + iucv_declare_cpu(NULL);
> + return 0;
> }
>
> -static struct notifier_block __refdata iucv_cpu_notifier = {
> - .notifier_call = iucv_cpu_notify,
> -};
> +static int iucv_cpu_down_prep(unsigned int cpu)
> +{
> + cpumask_t cpumask;
> +
> + if (!iucv_path_table)
> + return 0;
> +
> + cpumask_copy(&cpumask, &iucv_buffer_cpumask);
> + cpumask_clear_cpu(cpu, &cpumask);
> + if (cpumask_empty(&cpumask))
> + /* Can't offline last IUCV enabled cpu. */
> + return -EINVAL;
> +
> + iucv_retrieve_cpu(NULL);
> + if (!cpumask_empty(&iucv_irq_cpumask))
> + return 0;
> + smp_call_function_single(cpumask_first(&iucv_buffer_cpumask),
> + iucv_allow_cpu, NULL, 1);
> + return 0;
> +}
>
> /**
> * iucv_sever_pathid
> @@ -2027,6 +2008,7 @@ struct iucv_interface iucv_if = {
> };
> EXPORT_SYMBOL(iucv_if);
>
> +static enum cpuhp_state iucv_online;
> /**
> * iucv_init
> *
> @@ -2035,7 +2017,6 @@ EXPORT_SYMBOL(iucv_if);
> static int __init iucv_init(void)
> {
> int rc;
> - int cpu;
>
> if (!MACHINE_IS_VM) {
> rc = -EPROTONOSUPPORT;
> @@ -2054,23 +2035,19 @@ static int __init iucv_init(void)
> goto out_int;
> }
>
> - cpu_notifier_register_begin();
> -
> - for_each_online_cpu(cpu) {
> - if (alloc_iucv_data(cpu)) {
> - rc = -ENOMEM;
> - goto out_free;
> - }
> - }
> - rc = __register_hotcpu_notifier(&iucv_cpu_notifier);
> + rc = cpuhp_setup_state(CPUHP_NET_IUCV_PREPARE, "net/iucv:prepare",
> + iucv_cpu_prepare, iucv_cpu_dead);
> if (rc)
> goto out_free;
> -
> - cpu_notifier_register_done();
> + rc = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "net/iucv:online",
> + iucv_cpu_online, iucv_cpu_down_prep);
> + if (rc < 0)
> + goto out_free;
> + iucv_online = rc;
>
> rc = register_reboot_notifier(&iucv_reboot_notifier);
> if (rc)
> - goto out_cpu;
> + goto out_free;
> ASCEBC(iucv_error_no_listener, 16);
> ASCEBC(iucv_error_no_memory, 16);
> ASCEBC(iucv_error_pathid, 16);
> @@ -2084,14 +2061,10 @@ static int __init iucv_init(void)
>
> out_reboot:
> unregister_reboot_notifier(&iucv_reboot_notifier);
> -out_cpu:
> - cpu_notifier_register_begin();
> - __unregister_hotcpu_notifier(&iucv_cpu_notifier);
> out_free:
> - for_each_possible_cpu(cpu)
> - free_iucv_data(cpu);
> -
> - cpu_notifier_register_done();
> + if (iucv_online)
> + cpuhp_remove_state(iucv_online);
> + cpuhp_remove_state(CPUHP_NET_IUCV_PREPARE);
>
> root_device_unregister(iucv_root);
> out_int:
I prefer to keep the technique of cascaded undo labels here, like this:
@@ -2054,23 +2035,19 @@ static int __init iucv_init(void)
goto out_int;
}
- cpu_notifier_register_begin();
-
- for_each_online_cpu(cpu) {
- if (alloc_iucv_data(cpu)) {
- rc = -ENOMEM;
- goto out_free;
- }
- }
- rc = __register_hotcpu_notifier(&iucv_cpu_notifier);
+ rc = cpuhp_setup_state(CPUHP_NET_IUCV_PREPARE, "net/iucv:prepare",
+ iucv_cpu_prepare, iucv_cpu_dead);
if (rc)
- goto out_free;
-
- cpu_notifier_register_done();
+ goto out_dev;
+ rc = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "net/iucv:online",
+ iucv_cpu_online, iucv_cpu_down_prep);
+ if (rc < 0)
+ goto out_prep;
+ iucv_online = rc;
rc = register_reboot_notifier(&iucv_reboot_notifier);
if (rc)
- goto out_cpu;
+ goto out_remove;
ASCEBC(iucv_error_no_listener, 16);
ASCEBC(iucv_error_no_memory, 16);
ASCEBC(iucv_error_pathid, 16);
@@ -2084,15 +2061,12 @@ static int __init iucv_init(void)
out_reboot:
unregister_reboot_notifier(&iucv_reboot_notifier);
-out_cpu:
- cpu_notifier_register_begin();
- __unregister_hotcpu_notifier(&iucv_cpu_notifier);
-out_free:
- for_each_possible_cpu(cpu)
- free_iucv_data(cpu);
-
- cpu_notifier_register_done();
-
+out_remove:
+ if (iucv_online)
+ cpuhp_remove_state(iucv_online);
+out_prep:
+ cpuhp_remove_state(CPUHP_NET_IUCV_PREPARE);
+out_dev:
root_device_unregister(iucv_root);
out_int:
unregister_external_irq(EXT_IRQ_IUCV, iucv_external_interrupt);
> @@ -2110,7 +2083,6 @@ static int __init iucv_init(void)
> static void __exit iucv_exit(void)
> {
> struct iucv_irq_list *p, *n;
> - int cpu;
>
> spin_lock_irq(&iucv_queue_lock);
> list_for_each_entry_safe(p, n, &iucv_task_queue, list)
> @@ -2119,11 +2091,9 @@ static void __exit iucv_exit(void)
> kfree(p);
> spin_unlock_irq(&iucv_queue_lock);
> unregister_reboot_notifier(&iucv_reboot_notifier);
> - cpu_notifier_register_begin();
> - __unregister_hotcpu_notifier(&iucv_cpu_notifier);
> - for_each_possible_cpu(cpu)
> - free_iucv_data(cpu);
> - cpu_notifier_register_done();
> +
> + cpuhp_remove_state_nocalls(iucv_online);
> + cpuhp_remove_state(CPUHP_NET_IUCV_PREPARE);
> root_device_unregister(iucv_root);
> bus_unregister(&iucv_bus);
> unregister_external_irq(EXT_IRQ_IUCV, iucv_external_interrupt);
>
^ permalink raw reply
* Re: [PATCH net-next 0/2] Add support for the MV88e6097
From: Vivien Didelot @ 2016-11-23 18:09 UTC (permalink / raw)
To: Stefan Eichenberger, andrew, davem; +Cc: netdev, Stefan Eichenberger
In-Reply-To: <20161123175546.31416-1-stefan.eichenberger@netmodule.com>
Hi Stefan,
Stefan Eichenberger <eichest@gmail.com> writes:
> This patchset will add support for the MV88E6097 DSA switch and enable
> EDSA on MV88E6097 family devices.
>
> Stefan Eichenberger (2):
> net: dsa: mv88e6xxx: add MV88E6097 switch
> net: dsa: mv88e6xxx: enable EDSA on mv88e6097
>
> drivers/net/dsa/mv88e6xxx/chip.c | 26 ++++++++++++++++++++++++++
> drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 5 ++++-
> 2 files changed, 30 insertions(+), 1 deletion(-)
Ideally I'd put 2/2 first, because right after 1/2 your switch won't
work as expected.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH 1/2] net: dsa: mv88e6xxx: add MV88E6097 switch
From: Andrew Lunn @ 2016-11-23 18:10 UTC (permalink / raw)
To: Stefan Eichenberger; +Cc: vivien.didelot, davem, netdev, Stefan Eichenberger
In-Reply-To: <20161123175546.31416-2-stefan.eichenberger@netmodule.com>
> + [MV88E6097] = {
> + .prod_num = PORT_SWITCH_ID_PROD_NUM_6097,
> + .family = MV88E6XXX_FAMILY_6097,
> + .name = "Marvell 88E6097/88E6097F",
> + .num_databases = 4096,
> + .num_ports = 11,
> + .port_base_addr = 0x10,
> + .global1_addr = 0x1b,
> + .age_time_coeff = 15000,
> + .flags = MV88E6XXX_FLAGS_FAMILY_6097,
> + .ops = &mv88e6097_ops,
Upps. Sorry, i missed something when you rebased onto net-next. You
are missing .g1_irqs = . It is probably 9. You can check the
datasheet, global 1, register 0. If bit 8 is AVBInt, you need 9. If
bit 8 is reserved, then 8.
Andrew
^ permalink raw reply
* Re: [PATCH 2/2] net: dsa: mv88e6xxx: enable EDSA on mv88e6097
From: Andrew Lunn @ 2016-11-23 18:10 UTC (permalink / raw)
To: Stefan Eichenberger; +Cc: vivien.didelot, davem, netdev, Stefan Eichenberger
In-Reply-To: <20161123175546.31416-3-stefan.eichenberger@netmodule.com>
On Wed, Nov 23, 2016 at 06:55:46PM +0100, Stefan Eichenberger wrote:
> EDSA is currently disabled on mv88e6097 devices, this commit enables it.
>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@netmodule.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next] net: properly flush delay-freed skbs
From: Jesper Dangaard Brouer @ 2016-11-23 18:11 UTC (permalink / raw)
To: Alexander Duyck
Cc: Eric Dumazet, David Miller, netdev, Alexander Duyck, brouer
In-Reply-To: <CAKgT0UerbiZ8cWk3hkVO2SckxBgiG3zsLn_P05jM=-Cr6B3qAg@mail.gmail.com>
On Wed, 23 Nov 2016 09:12:50 -0800
Alexander Duyck <alexander.duyck@gmail.com> wrote:
> On Wed, Nov 23, 2016 at 8:44 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Typical NAPI drivers use napi_consume_skb(skb) at TX completion time.
> > This put skb in a percpu special queue, napi_alloc_cache, to get bulk
> > frees.
> >
> > It turns out the queue is not flushed and hits the NAPI_SKB_CACHE_SIZE
> > limit quite often, with skbs that were queued hundreds of usec earlier.
> > I measured this can take ~6000 nsec to perform one flush.
> >
> > __kfree_skb_flush() can be called from two points right now :
> >
> > 1) From net_tx_action(), but only for skbs that were queued to
> > sd->completion_queue.
> >
> > -> Irrelevant for NAPI drivers in normal operation.
> >
> > 2) From net_rx_action(), but only under high stress or if RPS/RFS has a
> > pending action.
> >
> > This patch changes net_rx_action() to perform the flush in all cases and
> > after more urgent operations happened (like kicking remote CPUS for
> > RPS/RFS).
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> > Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> > ---
>
> Yeah, we didn't intent the data to be sitting around that long. The
> change looks good to me.
>
> Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>
Also looks good to me! Thanks for catching this.
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
From: Florian Fainelli @ 2016-11-23 18:16 UTC (permalink / raw)
To: Andrew Lunn, Allan W. Nielsen
Cc: netdev, davem, bcm-kernel-feedback-list, raju.lakkaraju,
vivien.didelot
In-Reply-To: <20161123144636.GK14947@lunn.ch>
On 11/23/2016 06:46 AM, Andrew Lunn wrote:
>>>> Maybe we should think about this locking a bit. It is normal for the
>>>> lock to be held when using ops in the phy driver structure. The
>>>> exception is suspend/resume. Maybe we should also take the lock before
>>>> calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?
>>>
>>> Yes, that certainly seems like a good approach to me, let me cook a
>>> patch doing that.
>>
>> Just for my understanding (such that I will not make the same mistake again)...
>>
>> Why is it that phy functions such as get_wol needs to take the phy_lock and
>> others like get_tunable does not.
>>
>> I do understand the arguments on why the lock should be held by the caller of
>> get_tunable, but I do not understand why the same argument does not apply for
>> get_wol.
>
> Hi Allan
>
> phy_ethtool_get_wol and friends probably should take the
> phy_lock. This inconsistency is probably leading to locking
> bugs. e.g. at803x_set_wol() does a read-modify-write, and does not
> take the lock.
>
> There is no comment in the patch adding phy_ethtool_set_wol() to say
> why the lock is not taken, and a quick look at the code does not
> suggest a reason why it could not be taken/released by
> phy_ethtool_set_wol().
Yes, this should happen. I don't see how we cannot have two user-space
processes not racing with each other here for instance, see
mv643xx_eth_get_wol and cpsw_get_wol.
>
> I think it would be a good idea to change this.
>
> phy_suspend()/phy_resume() might have good reasons to avoid the lock,
> i've no idea how it is supposed to work. Is there a danger something
> else is holding the lock and has already been suspended? I guess not,
> otherwise there is little hope suspend would work at all.
phy_suspend() and phy_resume() usually get called after phy_disconnect()
or phy_stop() have been invoked, and even then this is during the
Ethernet driver's suspend resume/resume path, so there is no room for
concurrency to occur (user space is quiesced, and the PHY state machine
is stopped/halted), but still, if we were to change the calling context
it would be a good idea to acquire phydev->lock.
--
Florian
^ permalink raw reply
* [PATCH 2/2] net: dsa: mv88e6xxx: enable EDSA on mv88e6097
From: Stefan Eichenberger @ 2016-11-23 17:55 UTC (permalink / raw)
To: andrew, vivien.didelot, davem; +Cc: netdev, Stefan Eichenberger
In-Reply-To: <20161123175546.31416-1-stefan.eichenberger@netmodule.com>
EDSA is currently disabled on mv88e6097 devices, this commit enables it.
Signed-off-by: Stefan Eichenberger <stefan.eichenberger@netmodule.com>
---
drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index ab52c37..a2ff1fc 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -543,7 +543,8 @@ enum mv88e6xxx_cap {
MV88E6XXX_FLAGS_MULTI_CHIP)
#define MV88E6XXX_FLAGS_FAMILY_6097 \
- (MV88E6XXX_FLAG_G1_ATU_FID | \
+ (MV88E6XXX_FLAG_EDSA | \
+ MV88E6XXX_FLAG_G1_ATU_FID | \
MV88E6XXX_FLAG_G1_VTU_FID | \
MV88E6XXX_FLAG_GLOBAL2 | \
MV88E6XXX_FLAG_G2_MGMT_EN_2X | \
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v2] net: dsa: mv88e6xxx: forward unknown mc packets on mv88e6097
From: Vivien Didelot @ 2016-11-23 18:18 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Stefan Eichenberger, f.fainelli, netdev
In-Reply-To: <20161123180125.GF8760@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> On Wed, Nov 23, 2016 at 12:52:52PM -0500, Vivien Didelot wrote:
>> Hi Andrew,
>>
>> Andrew Lunn <andrew@lunn.ch> writes:
>>
>> > And if you have a recent version of tcpdump, it will decode
>> > the header.
>>
>> Since d729eb4, thanks to you Andrew ;-)
>>
>> I move up the cleanup of ports setup in my priority list.
>
> Hi Vivien
>
> Please take a look at my mv88e6390 branch. I already refactored this
> code, because the mv88e6390 does something slightly different...
>
> I hope to post another batch of mv88e6390 patches soon, and they will
> include this cleanup. Since they will clash with these patches, i will
> post them first as RFC.
Perfect. Please split an RFC only including this cleanup if
possible. Fewer patches will be easier to review, since the first port
registers differs a lot.
Thanks,
Vivien
^ 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