Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] wl1251: add a missing spin_lock_init()
From: David Miller @ 2017-08-31 17:33 UTC (permalink / raw)
  To: pavel
  Cc: kvalo, torvalds, xiyou.wangcong, akpm, netdev, linux-kernel,
	linux-wireless
In-Reply-To: <20170831144743.GA21261@amd>

From: Pavel Machek <pavel@ucw.cz>
Date: Thu, 31 Aug 2017 16:47:43 +0200

> Dave, Linus -- can you still take the patch?

Pavel, please do not bypass maintainers like this.

It's really rude, and if you do things like that instead of
trying to work properly with us, your relationship with
these maintainers will suffer in the long term.

Thank you.

^ permalink raw reply

* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-08-31 17:35 UTC (permalink / raw)
  To: David Daney, Florian Fainelli
  Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
	Andrew Lunn, Mans Rullgard
In-Reply-To: <e24693e8-d8ae-188a-2a38-c9a83fdc94e3@gmail.com>

On 31/08/2017 18:36, David Daney wrote:
> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>
>>> This reverts commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
>>> Correctly process PHY_HALTED in phy_stop_machine()") because it is
>>> creating the possibility for a NULL pointer dereference.
>>>
>>> David Daney provide the following call trace and diagram of events:
>>>
>>> When ndo_stop() is called we call:
>>>
>>>   phy_disconnect()
>>>      +---> phy_stop_interrupts() implies: phydev->irq = PHY_POLL;
>>
>> What does this mean?
> 
> I meant that after the call to phy_stop_interrupts(), phydev->irq = 
> PHY_POLL;

I must be missing something.

http://elixir.free-electrons.com/linux/latest/source/drivers/net/phy/phy.c#L868

phy_stop_interrupts() doesn't change phydev->irq right?

Only phy_start_interrupts() sets phydev->irq to
PHY_POLL if it cannot set up interrupt mode.

Regards.

^ permalink raw reply

* Re: DSA mv88e6xxx RX frame errors and TCP/IP RX failure
From: Andrew Lunn @ 2017-08-31 17:41 UTC (permalink / raw)
  To: Tim Harvey, David Miller
  Cc: netdev, Vivien Didelot, linux-kernel@vger.kernel.org, Fugang Duan,
	Ilia Mirkin
In-Reply-To: <CAJ+vNU0dq-ULUj3PbQH_biGviJURj2z5McCXx_vYxcB0k94WkA@mail.gmail.com>

> > I did fix an issue recently with that. See
> >
> > commit fbbeefdd21049fcf9437c809da3828b210577f36
> > Author: Andrew Lunn <andrew@lunn.ch>
> > Date:   Sun Jul 30 19:36:05 2017 +0200
> >
> >     net: fec: Allow reception of frames bigger than 1522 bytes
> >
> >     The FEC Receive Control Register has a 14 bit field indicating the
> >     longest frame that may be received. It is being set to 1522. Frames
> >     longer than this are discarded, but counted as being in error.
> >
> >     When using DSA, frames from the switch has an additional header,
> >     either 4 or 8 bytes if a Marvell switch is used. Thus a full MTU frame
> >     of 1522 bytes received by the switch on a port becomes 1530 bytes when
> >     passed to the host via the FEC interface.
> >
> >     Change the maximum receive size to 2048 - 64, where 64 is the maximum
> >     rx_alignment applied on the receive buffer for AVB capable FEC
> >     cores. Use this value also for the maximum receive buffer size. The
> >     driver is already allocating a receive SKB of 2048 bytes, so this
> >     change should not have any significant effects.
> >
> >     Tested on imx51, imx6, vf610.
> >
> >     Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> >     Signed-off-by: David S. Miller <davem@davemloft.net>
> >
> >
> > However, this is was of an all/nothing problem. All frames with the
> > full MTU were getting dropped, where as i think you are only seeing a
> > few dropped?
> >
> 
> Andrew,
> 
> Indeed this does resolve the issue. I see a burst of FIFO overruns
> initially when receiving an iperf bandwidth test but that would be
> caused by the IMX6 errata and should be mitigated via pause frames.
> After that short burst I see no other errors and iperf works fine.
> 
> Should we get this patch in the linux-stable tree for the 4.9 kernel?

Hi David

Please could you add

fbbeefdd2104 ("net: fec: Allow reception of frames bigger than 1522 bytes")

to stable.

Thanks
	Andrew

^ permalink raw reply

* Re: DSA mv88e6xxx RX frame errors and TCP/IP RX failure
From: David Miller @ 2017-08-31 17:44 UTC (permalink / raw)
  To: andrew; +Cc: tharvey, netdev, vivien.didelot, linux-kernel, fugang.duan,
	imirkin
In-Reply-To: <20170831174141.GP22289@lunn.ch>

From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 31 Aug 2017 19:41:41 +0200

> Please could you add
> 
> fbbeefdd2104 ("net: fec: Allow reception of frames bigger than 1522 bytes")
> 
> to stable.

Queued up, thanks.

^ permalink raw reply

* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-08-31 17:49 UTC (permalink / raw)
  To: Florian Fainelli, David Daney
  Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
	Andrew Lunn, Mans Rullgard
In-Reply-To: <931bf454-81ff-94dc-82e6-bc2b889bd43a@gmail.com>

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?

Regards.

^ permalink raw reply

* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-08-31 17:53 UTC (permalink / raw)
  To: Mason, David Daney
  Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
	Andrew Lunn, Mans Rullgard
In-Reply-To: <d6a6b552-95a7-8353-54c8-fa804f9366a1@free.fr>

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.
-- 
Florian

^ permalink raw reply

* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-08-31 18:12 UTC (permalink / raw)
  To: Florian Fainelli, David Daney
  Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
	Andrew Lunn, Mans Rullgard
In-Reply-To: <f74f1aad-3990-ae54-316f-751c3b15de41@gmail.com>

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.

Also, queue_delayed_work() is only called in polling mode.
David stated that he's using interrupt mode.

Regards.

^ permalink raw reply

* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-08-31 18:29 UTC (permalink / raw)
  To: Mason, David Daney
  Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
	Andrew Lunn, Mans Rullgard
In-Reply-To: <ebee6e5d-5bc1-1c5b-b31d-6d50618d6074@free.fr>

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")
-- 
Florian

^ permalink raw reply

* [PATCH net-next 0/4] net: dsa: add master interface
From: Vivien Didelot @ 2017-08-31 18:37 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
	Andrew Lunn, Vivien Didelot

Currently the SoC network interface (called master) to which a switch
fabric hangs, has its dsa_ptr pointing to a dsa_switch_tree instance.

This is not quite correct, because this interface is physically wired to
one of the switch ports (called CPU port), and because in a switch
fabric with multiple CPU ports, several master interfaces will point to
several CPU ports of the same dsa_switch_tree.

This patchset adds a new dsa_master structure to represent the pipe
between the SoC master interface and its switch CPU port. This structure
will store specific data such as the master ethtool_ops copy and the
tagging protocol used to pass frames with the associated slave ports.
The dsa_ptr is changed to a dsa_master instance, and each DSA slave now
has a pointer to a master port.

This is a step forward better control over the CPU conduit and support
for multiple CPU ports.

Vivien Didelot (4):
  net: dsa: introduce dsa_master
  net: dsa: move master ethtool ops in dsa_master
  net: dsa: change dsa_ptr for a dsa_master
  net: dsa: assign a master to slave ports

 drivers/net/dsa/b53/b53_common.c |   4 +-
 drivers/net/dsa/bcm_sf2.c        |   8 +--
 drivers/net/dsa/mt7530.c         |   4 +-
 drivers/net/dsa/mv88e6060.c      |   2 +-
 drivers/net/dsa/qca8k.c          |   2 +-
 include/linux/netdevice.h        |   4 +-
 include/net/dsa.h                |  42 +++++------
 net/dsa/Makefile                 |   2 +-
 net/dsa/dsa.c                    |  34 +--------
 net/dsa/dsa2.c                   |  38 +++++-----
 net/dsa/dsa_priv.h               |  24 +++----
 net/dsa/legacy.c                 |  34 +++++----
 net/dsa/master.c                 | 149 +++++++++++++++++++++++++++++++++++++++
 net/dsa/slave.c                  | 117 +++++-------------------------
 net/dsa/tag_brcm.c               |   5 +-
 net/dsa/tag_dsa.c                |   3 +-
 net/dsa/tag_edsa.c               |   3 +-
 net/dsa/tag_ksz.c                |   5 +-
 net/dsa/tag_lan9303.c            |   6 +-
 net/dsa/tag_mtk.c                |  12 +---
 net/dsa/tag_qca.c                |  12 +---
 net/dsa/tag_trailer.c            |   5 +-
 22 files changed, 265 insertions(+), 250 deletions(-)
 create mode 100644 net/dsa/master.c

-- 
2.14.1

^ permalink raw reply

* [PATCH net-next 1/4] net: dsa: introduce dsa_master
From: Vivien Didelot @ 2017-08-31 18:37 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
	Andrew Lunn, Vivien Didelot
In-Reply-To: <20170831183746.2109-1-vivien.didelot@savoirfairelinux.com>

Add a new dsa_master structure to represent the DSA switch port
physically linked to the CPU master Ethernet device.

This device, a.k.a. a CPU port, is responsible to receive/send frames
from/to the DSA slave ports, a.k.a. user ports.

For the moment, this structure only contains a pointer to a dsa_port
(i.e. the switch device side) and a pointer to the master interface
(i.e. the SoC CPU side).

The structure will be extended with master-specific data such as the
tagging operations or the master ethtool_ops copies.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/b53/b53_common.c |  4 ++--
 drivers/net/dsa/bcm_sf2.c        |  8 ++++----
 drivers/net/dsa/mt7530.c         |  4 ++--
 drivers/net/dsa/mv88e6060.c      |  2 +-
 drivers/net/dsa/qca8k.c          |  2 +-
 include/net/dsa.h                | 12 ++++++++----
 net/dsa/Makefile                 |  2 +-
 net/dsa/dsa2.c                   | 27 ++++++++++++++-------------
 net/dsa/dsa_priv.h               |  8 ++++++--
 net/dsa/legacy.c                 | 21 ++++++++++++---------
 net/dsa/master.c                 | 30 ++++++++++++++++++++++++++++++
 net/dsa/slave.c                  |  6 +-----
 12 files changed, 82 insertions(+), 44 deletions(-)
 create mode 100644 net/dsa/master.c

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 274f3679f33d..798045939281 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1280,7 +1280,7 @@ EXPORT_SYMBOL(b53_fdb_dump);
 int b53_br_join(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct b53_device *dev = ds->priv;
-	s8 cpu_port = ds->dst->cpu_dp->index;
+	s8 cpu_port = ds->dst->master->port->index;
 	u16 pvlan, reg;
 	unsigned int i;
 
@@ -1326,7 +1326,7 @@ void b53_br_leave(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct b53_device *dev = ds->priv;
 	struct b53_vlan *vl = &dev->vlans[0];
-	s8 cpu_port = ds->dst->cpu_dp->index;
+	s8 cpu_port = ds->dst->master->port->index;
 	unsigned int i;
 	u16 pvlan, reg, pvid;
 
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 8492c9d64004..39468a0c1cee 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -227,7 +227,7 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
 			      struct phy_device *phy)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
-	s8 cpu_port = ds->dst->cpu_dp->index;
+	s8 cpu_port = ds->dst->master->port->index;
 	unsigned int i;
 	u32 reg;
 
@@ -788,7 +788,7 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds)
 static void bcm_sf2_sw_get_wol(struct dsa_switch *ds, int port,
 			       struct ethtool_wolinfo *wol)
 {
-	struct net_device *p = ds->dst->cpu_dp->netdev;
+	struct net_device *p = ds->dst->master->netdev;
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	struct ethtool_wolinfo pwol;
 
@@ -811,9 +811,9 @@ static void bcm_sf2_sw_get_wol(struct dsa_switch *ds, int port,
 static int bcm_sf2_sw_set_wol(struct dsa_switch *ds, int port,
 			      struct ethtool_wolinfo *wol)
 {
-	struct net_device *p = ds->dst->cpu_dp->netdev;
+	struct net_device *p = ds->dst->master->netdev;
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
-	s8 cpu_port = ds->dst->cpu_dp->index;
+	s8 cpu_port = ds->dst->master->port->index;
 	struct ethtool_wolinfo pwol;
 
 	p->ethtool_ops->get_wol(p, &pwol);
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index c142b97add2c..4998c05dba1b 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -928,11 +928,11 @@ mt7530_setup(struct dsa_switch *ds)
 	struct device_node *dn;
 	struct mt7530_dummy_poll p;
 
-	/* The parent node of cpu_dp->netdev which holds the common system
+	/* The parent node of master->netdev which holds the common system
 	 * controller also is the container for two GMACs nodes representing
 	 * as two netdev instances.
 	 */
-	dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
+	dn = ds->dst->master->netdev->dev.of_node->parent;
 	priv->ethernet = syscon_node_to_regmap(dn);
 	if (IS_ERR(priv->ethernet))
 		return PTR_ERR(priv->ethernet);
diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index dce7fa57eb55..6a30e15b9c01 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -176,7 +176,7 @@ static int mv88e6060_setup_port(struct dsa_switch *ds, int p)
 		  ((p & 0xf) << PORT_VLAN_MAP_DBNUM_SHIFT) |
 		   (dsa_is_cpu_port(ds, p) ?
 			ds->enabled_port_mask :
-			BIT(ds->dst->cpu_dp->index)));
+			BIT(ds->dst->master->port->index)));
 
 	/* Port Association Vector: when learning source addresses
 	 * of packets, add the address to the address database using
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 5ada7a41449c..c34d6c82efb3 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -506,7 +506,7 @@ qca8k_setup(struct dsa_switch *ds)
 		pr_warn("regmap initialization failed");
 
 	/* Initialize CPU port pad mode (xMII type, delays...) */
-	phy_mode = of_get_phy_mode(ds->dst->cpu_dp->dn);
+	phy_mode = of_get_phy_mode(ds->dst->master->port->dn);
 	if (phy_mode < 0) {
 		pr_err("Can't find phy-mode for master device\n");
 		return phy_mode;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 398ca8d70ccd..217e36cfc69f 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -138,7 +138,7 @@ struct dsa_switch_tree {
 	/*
 	 * The switch port to which the CPU is attached.
 	 */
-	struct dsa_port		*cpu_dp;
+	struct dsa_master	*master;
 
 	/*
 	 * Data for the individual switch chips.
@@ -173,6 +173,10 @@ struct dsa_mall_tc_entry {
 	};
 };
 
+struct dsa_master {
+	struct dsa_port *port;
+	struct net_device *netdev;
+};
 
 struct dsa_port {
 	struct dsa_switch	*ds;
@@ -273,10 +277,10 @@ static inline u8 dsa_upstream_port(struct dsa_switch *ds)
 	 * Else return the (DSA) port number that connects to the
 	 * switch that is one hop closer to the cpu.
 	 */
-	if (dst->cpu_dp->ds == ds)
-		return dst->cpu_dp->index;
+	if (dst->master->port->ds == ds)
+		return dst->master->port->index;
 	else
-		return ds->rtable[dst->cpu_dp->ds->index];
+		return ds->rtable[dst->master->port->ds->index];
 }
 
 typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index fcce25da937c..2e7ac8bab19d 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -1,6 +1,6 @@
 # the core
 obj-$(CONFIG_NET_DSA) += dsa_core.o
-dsa_core-y += dsa.o dsa2.o legacy.o port.o slave.o switch.o
+dsa_core-y += dsa.o dsa2.o legacy.o master.o port.o slave.o switch.o
 
 # tagging formats
 dsa_core-$(CONFIG_NET_DSA_TAG_BRCM) += tag_brcm.o
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index cceaa4dd9f53..4c4381b7aafb 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -337,7 +337,7 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 		return err;
 
 	if (ds->ops->set_addr) {
-		err = ds->ops->set_addr(ds, dst->cpu_dp->netdev->dev_addr);
+		err = ds->ops->set_addr(ds, dst->master->netdev->dev_addr);
 		if (err < 0)
 			return err;
 	}
@@ -433,8 +433,8 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst)
 			return err;
 	}
 
-	if (dst->cpu_dp) {
-		err = dsa_cpu_port_ethtool_setup(dst->cpu_dp);
+	if (dst->master) {
+		err = dsa_cpu_port_ethtool_setup(dst->master->port);
 		if (err)
 			return err;
 	}
@@ -444,7 +444,7 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst)
 	 * sent to the tag format's receive function.
 	 */
 	wmb();
-	dst->cpu_dp->netdev->dsa_ptr = dst;
+	dst->master->netdev->dsa_ptr = dst;
 	dst->applied = true;
 
 	return 0;
@@ -458,7 +458,7 @@ static void dsa_dst_unapply(struct dsa_switch_tree *dst)
 	if (!dst->applied)
 		return;
 
-	dst->cpu_dp->netdev->dsa_ptr = NULL;
+	dst->master->netdev->dsa_ptr = NULL;
 
 	/* If we used a tagging format that doesn't have an ethertype
 	 * field, make sure that all packets from this point get sent
@@ -474,9 +474,9 @@ static void dsa_dst_unapply(struct dsa_switch_tree *dst)
 		dsa_ds_unapply(dst, ds);
 	}
 
-	if (dst->cpu_dp) {
-		dsa_cpu_port_ethtool_restore(dst->cpu_dp);
-		dst->cpu_dp = NULL;
+	if (dst->master) {
+		dsa_cpu_port_ethtool_restore(dst->master->port);
+		dst->master = NULL;
 	}
 
 	pr_info("DSA: tree %d unapplied\n", dst->tree);
@@ -504,9 +504,10 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
 	if (!ethernet_dev)
 		return -EPROBE_DEFER;
 
-	if (!dst->cpu_dp) {
-		dst->cpu_dp = port;
-		dst->cpu_dp->netdev = ethernet_dev;
+	if (!dst->master) {
+		dst->master = dsa_master_create(port, ethernet_dev);
+		if (!dst->master)
+			return -ENOMEM;
 	}
 
 	/* Initialize cpu_port_mask now for drv->setup()
@@ -577,7 +578,7 @@ static int dsa_dst_parse(struct dsa_switch_tree *dst)
 			return err;
 	}
 
-	if (!dst->cpu_dp->netdev) {
+	if (!dst->master) {
 		pr_warn("Tree has no master device\n");
 		return -EINVAL;
 	}
@@ -595,7 +596,7 @@ static int dsa_dst_parse(struct dsa_switch_tree *dst)
 			    dsa_port_is_cpu(dp))
 				continue;
 
-			dp->cpu_dp = dst->cpu_dp;
+			dp->cpu_dp = dst->master->port;
 		}
 	}
 
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 9c3eeb72462d..70f576b3d6fb 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -112,6 +112,10 @@ int dsa_legacy_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
 		       struct net_device *dev,
 		       const unsigned char *addr, u16 vid);
 
+/* master.c */
+struct dsa_master *dsa_master_create(struct dsa_port *port,
+				     struct net_device *netdev);
+
 /* port.c */
 int dsa_port_set_state(struct dsa_port *dp, u8 state,
 		       struct switchdev_trans *trans);
@@ -177,12 +181,12 @@ extern const struct dsa_device_ops trailer_netdev_ops;
 
 static inline struct net_device *dsa_master_netdev(struct dsa_slave_priv *p)
 {
-	return p->dp->cpu_dp->netdev;
+	return p->dp->ds->dst->master->netdev;
 }
 
 static inline struct dsa_port *dsa_get_cpu_port(struct dsa_switch_tree *dst)
 {
-	return dst->cpu_dp;
+	return dst->master->port;
 }
 
 #endif
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index 91e6f7981d39..7a21415d2a81 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -114,13 +114,16 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
 			continue;
 
 		if (!strcmp(name, "cpu")) {
-			if (dst->cpu_dp) {
+			if (dst->master) {
 				netdev_err(master,
 					   "multiple cpu ports?!\n");
 				return -EINVAL;
 			}
-			dst->cpu_dp = &ds->ports[i];
-			dst->cpu_dp->netdev = master;
+
+			dst->master = dsa_master_create(&ds->ports[i], master);
+			if (!dst->master)
+				return -ENOMEM;
+
 			ds->cpu_port_mask |= 1 << i;
 		} else if (!strcmp(name, "dsa")) {
 			ds->dsa_port_mask |= 1 << i;
@@ -143,7 +146,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
 	 * tagging protocol to the preferred tagging format of this
 	 * switch.
 	 */
-	if (dst->cpu_dp->ds == ds) {
+	if (dst->master->port->ds == ds) {
 		enum dsa_tag_protocol tag_protocol;
 
 		tag_protocol = ops->get_tag_protocol(ds);
@@ -189,7 +192,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
 	 */
 	for (i = 0; i < ds->num_ports; i++) {
 		ds->ports[i].dn = cd->port_dn[i];
-		ds->ports[i].cpu_dp = dst->cpu_dp;
+		ds->ports[i].cpu_dp = dst->master->port;
 
 		if (!(ds->enabled_port_mask & (1 << i)))
 			continue;
@@ -206,7 +209,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
 		netdev_err(master, "[%d] : can't configure CPU and DSA ports\n",
 			   index);
 
-	ret = dsa_cpu_port_ethtool_setup(ds->dst->cpu_dp);
+	ret = dsa_cpu_port_ethtool_setup(ds->dst->master->port);
 	if (ret)
 		return ret;
 
@@ -671,7 +674,7 @@ static void dsa_remove_dst(struct dsa_switch_tree *dst)
 {
 	int i;
 
-	dst->cpu_dp->netdev->dsa_ptr = NULL;
+	dst->master->netdev->dsa_ptr = NULL;
 
 	/* If we used a tagging format that doesn't have an ethertype
 	 * field, make sure that all packets from this point get sent
@@ -686,9 +689,9 @@ static void dsa_remove_dst(struct dsa_switch_tree *dst)
 			dsa_switch_destroy(ds);
 	}
 
-	dsa_cpu_port_ethtool_restore(dst->cpu_dp);
+	dsa_cpu_port_ethtool_restore(dst->master->port);
 
-	dev_put(dst->cpu_dp->netdev);
+	dev_put(dst->master->netdev);
 }
 
 static int dsa_remove(struct platform_device *pdev)
diff --git a/net/dsa/master.c b/net/dsa/master.c
new file mode 100644
index 000000000000..294b82b9c114
--- /dev/null
+++ b/net/dsa/master.c
@@ -0,0 +1,30 @@
+/*
+ * Handling of a master port, receiving/sending frames from/to slave ports
+ *
+ * Copyright (c) 2017 Savoir-faire Linux Inc.
+ *	Vivien Didelot <vivien.didelot@savoirfairelinux.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "dsa_priv.h"
+
+struct dsa_master *dsa_master_create(struct dsa_port *port,
+				     struct net_device *netdev)
+{
+	struct device *dev = port->ds->dev;
+	struct dsa_master *master;
+
+	master = devm_kzalloc(dev, sizeof(struct dsa_master), GFP_KERNEL);
+	if (!master)
+		return NULL;
+
+	master->port = port;
+	master->netdev = netdev;
+	master->port->netdev = netdev;
+
+	return master;
+}
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 78e78a6e6833..6d47eca47a0c 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1250,15 +1250,11 @@ int dsa_slave_create(struct dsa_port *port, const char *name)
 {
 	struct dsa_switch *ds = port->ds;
 	struct dsa_switch_tree *dst = ds->dst;
-	struct net_device *master;
+	struct net_device *master = dst->master->netdev;
 	struct net_device *slave_dev;
 	struct dsa_slave_priv *p;
-	struct dsa_port *cpu_dp;
 	int ret;
 
-	cpu_dp = ds->dst->cpu_dp;
-	master = cpu_dp->netdev;
-
 	slave_dev = alloc_netdev(sizeof(struct dsa_slave_priv), name,
 				 NET_NAME_UNKNOWN, ether_setup);
 	if (slave_dev == NULL)
-- 
2.14.1

^ permalink raw reply related

* [PATCH net-next 2/4] net: dsa: move master ethtool ops in dsa_master
From: Vivien Didelot @ 2017-08-31 18:37 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
	Andrew Lunn, Vivien Didelot
In-Reply-To: <20170831183746.2109-1-vivien.didelot@savoirfairelinux.com>

The copy of the master netdev ethtool_ops is currently stored in the
dsa_port structure, but it is specific to the DSA master (CPU) port.

Move it in the new dsa_master structure. While modifying the ethtool
functions to take this structure as argument, move them in a new
master.c DSA core file.

Also the ethtool ops were the only remaining users for
master->port->netdev thus remove this assignment now.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/net/dsa.h  |   9 ++---
 net/dsa/dsa.c      |  28 --------------
 net/dsa/dsa2.c     |   4 +-
 net/dsa/dsa_priv.h |   5 +--
 net/dsa/legacy.c   |   4 +-
 net/dsa/master.c   | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 net/dsa/slave.c    |  80 ----------------------------------------
 7 files changed, 114 insertions(+), 121 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 217e36cfc69f..f4a5afc4255b 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -176,6 +176,10 @@ struct dsa_mall_tc_entry {
 struct dsa_master {
 	struct dsa_port *port;
 	struct net_device *netdev;
+
+	/* Original copy of the master netdev ethtool_ops */
+	const struct ethtool_ops *orig_ethtool_ops;
+	struct ethtool_ops ethtool_ops;
 };
 
 struct dsa_port {
@@ -189,11 +193,6 @@ struct dsa_port {
 	u8			stp_state;
 	struct net_device	*bridge_dev;
 	struct devlink_port	devlink_port;
-	/*
-	 * Original copy of the master netdev ethtool_ops
-	 */
-	struct ethtool_ops	ethtool_ops;
-	const struct ethtool_ops *orig_ethtool_ops;
 };
 
 struct dsa_switch {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 03c58b0eb082..81c852e32821 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -112,34 +112,6 @@ const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol)
 	return ops;
 }
 
-int dsa_cpu_port_ethtool_setup(struct dsa_port *cpu_dp)
-{
-	struct dsa_switch *ds = cpu_dp->ds;
-	struct net_device *master;
-	struct ethtool_ops *cpu_ops;
-
-	master = cpu_dp->netdev;
-
-	cpu_ops = devm_kzalloc(ds->dev, sizeof(*cpu_ops), GFP_KERNEL);
-	if (!cpu_ops)
-		return -ENOMEM;
-
-	memcpy(&cpu_dp->ethtool_ops, master->ethtool_ops,
-	       sizeof(struct ethtool_ops));
-	cpu_dp->orig_ethtool_ops = master->ethtool_ops;
-	memcpy(cpu_ops, &cpu_dp->ethtool_ops,
-	       sizeof(struct ethtool_ops));
-	dsa_cpu_port_ethtool_init(cpu_ops);
-	master->ethtool_ops = cpu_ops;
-
-	return 0;
-}
-
-void dsa_cpu_port_ethtool_restore(struct dsa_port *cpu_dp)
-{
-	cpu_dp->netdev->ethtool_ops = cpu_dp->orig_ethtool_ops;
-}
-
 void dsa_cpu_dsa_destroy(struct dsa_port *port)
 {
 	struct device_node *port_dn = port->dn;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 4c4381b7aafb..d60f681b96cc 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -434,7 +434,7 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst)
 	}
 
 	if (dst->master) {
-		err = dsa_cpu_port_ethtool_setup(dst->master->port);
+		err = dsa_master_ethtool_setup(dst->master);
 		if (err)
 			return err;
 	}
@@ -475,7 +475,7 @@ static void dsa_dst_unapply(struct dsa_switch_tree *dst)
 	}
 
 	if (dst->master) {
-		dsa_cpu_port_ethtool_restore(dst->master->port);
+		dsa_master_ethtool_restore(dst->master);
 		dst->master = NULL;
 	}
 
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 70f576b3d6fb..4fa14b4305b9 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -97,8 +97,6 @@ struct dsa_slave_priv {
 int dsa_cpu_dsa_setup(struct dsa_port *port);
 void dsa_cpu_dsa_destroy(struct dsa_port *dport);
 const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol);
-int dsa_cpu_port_ethtool_setup(struct dsa_port *cpu_dp);
-void dsa_cpu_port_ethtool_restore(struct dsa_port *cpu_dp);
 bool dsa_schedule_work(struct work_struct *work);
 
 /* legacy.c */
@@ -115,6 +113,8 @@ int dsa_legacy_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
 /* master.c */
 struct dsa_master *dsa_master_create(struct dsa_port *port,
 				     struct net_device *netdev);
+int dsa_master_ethtool_setup(struct dsa_master *master);
+void dsa_master_ethtool_restore(struct dsa_master *master);
 
 /* port.c */
 int dsa_port_set_state(struct dsa_port *dp, u8 state,
@@ -143,7 +143,6 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
 void dsa_slave_mii_bus_init(struct dsa_switch *ds);
-void dsa_cpu_port_ethtool_init(struct ethtool_ops *ops);
 int dsa_slave_create(struct dsa_port *port, const char *name);
 void dsa_slave_destroy(struct net_device *slave_dev);
 int dsa_slave_suspend(struct net_device *slave_dev);
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index 7a21415d2a81..acd81c488bf4 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -209,7 +209,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
 		netdev_err(master, "[%d] : can't configure CPU and DSA ports\n",
 			   index);
 
-	ret = dsa_cpu_port_ethtool_setup(ds->dst->master->port);
+	ret = dsa_master_ethtool_setup(ds->dst->master);
 	if (ret)
 		return ret;
 
@@ -689,7 +689,7 @@ static void dsa_remove_dst(struct dsa_switch_tree *dst)
 			dsa_switch_destroy(ds);
 	}
 
-	dsa_cpu_port_ethtool_restore(dst->master->port);
+	dsa_master_ethtool_restore(dst->master);
 
 	dev_put(dst->master->netdev);
 }
diff --git a/net/dsa/master.c b/net/dsa/master.c
index 294b82b9c114..9ecce3e7c8df 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -12,6 +12,110 @@
 
 #include "dsa_priv.h"
 
+static void dsa_master_get_ethtool_stats(struct net_device *dev,
+					 struct ethtool_stats *stats,
+					 uint64_t *data)
+{
+	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_master *master = dst->master;
+	struct dsa_port *port = master->port;
+	struct dsa_switch *ds = port->ds;
+	int count = 0;
+
+	if (master->ethtool_ops.get_sset_count) {
+		count = master->ethtool_ops.get_sset_count(dev, ETH_SS_STATS);
+		master->ethtool_ops.get_ethtool_stats(dev, stats, data);
+	}
+
+	if (ds->ops->get_ethtool_stats)
+		ds->ops->get_ethtool_stats(ds, port->index, data + count);
+}
+
+static int dsa_master_get_sset_count(struct net_device *dev, int sset)
+{
+	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_master *master = dst->master;
+	struct dsa_port *port = master->port;
+	struct dsa_switch *ds = port->ds;
+	int count = 0;
+
+	if (master->ethtool_ops.get_sset_count)
+		count += master->ethtool_ops.get_sset_count(dev, sset);
+
+	if (sset == ETH_SS_STATS && ds->ops->get_sset_count)
+		count += ds->ops->get_sset_count(ds);
+
+	return count;
+}
+
+static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
+				   uint8_t *data)
+{
+	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_master *master = dst->master;
+	struct dsa_port *port = master->port;
+	struct dsa_switch *ds = port->ds;
+	int len = ETH_GSTRING_LEN;
+	int mcount = 0, count;
+	unsigned int i;
+	uint8_t pfx[4];
+	uint8_t *ndata;
+
+	snprintf(pfx, sizeof(pfx), "p%.2d", port->index);
+	/* We do not want to be NULL-terminated, since this is a prefix */
+	pfx[sizeof(pfx) - 1] = '_';
+
+	if (master->ethtool_ops.get_sset_count) {
+		mcount = master->ethtool_ops.get_sset_count(dev, ETH_SS_STATS);
+		master->ethtool_ops.get_strings(dev, stringset, data);
+	}
+
+	if (stringset == ETH_SS_STATS && ds->ops->get_strings) {
+		ndata = data + mcount * len;
+		/* This function copies ETH_GSTRINGS_LEN bytes, we will mangle
+		 * the output after to prepend our CPU port prefix we
+		 * constructed earlier
+		 */
+		ds->ops->get_strings(ds, port->index, ndata);
+		count = ds->ops->get_sset_count(ds);
+		for (i = 0; i < count; i++) {
+			memmove(ndata + (i * len + sizeof(pfx)),
+				ndata + i * len, len - sizeof(pfx));
+			memcpy(ndata + i * len, pfx, sizeof(pfx));
+		}
+	}
+}
+
+int dsa_master_ethtool_setup(struct dsa_master *master)
+{
+	struct device *dev = master->port->ds->dev;
+	struct net_device *netdev = master->netdev;
+	struct ethtool_ops *ops;
+
+	ops = devm_kzalloc(dev, sizeof(*ops), GFP_KERNEL);
+	if (!ops)
+		return -ENOMEM;
+
+	/* Back up the original master netdev ethtool_ops */
+	master->orig_ethtool_ops = netdev->ethtool_ops;
+	memcpy(&master->ethtool_ops, master->orig_ethtool_ops, sizeof(*ops));
+	memcpy(ops, &master->ethtool_ops, sizeof(*ops));
+
+	/* Change the master netdev ethtool_ops */
+	ops->get_sset_count = dsa_master_get_sset_count;
+	ops->get_ethtool_stats = dsa_master_get_ethtool_stats;
+	ops->get_strings = dsa_master_get_strings;
+	netdev->ethtool_ops = ops;
+
+	return 0;
+}
+
+void dsa_master_ethtool_restore(struct dsa_master *master)
+{
+	master->netdev->ethtool_ops = master->orig_ethtool_ops;
+	master->orig_ethtool_ops = NULL;
+}
+
 struct dsa_master *dsa_master_create(struct dsa_port *port,
 				     struct net_device *netdev)
 {
@@ -24,7 +128,6 @@ struct dsa_master *dsa_master_create(struct dsa_port *port,
 
 	master->port = port;
 	master->netdev = netdev;
-	master->port->netdev = netdev;
 
 	return master;
 }
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6d47eca47a0c..99f9e48a6cee 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -567,79 +567,6 @@ static void dsa_slave_get_strings(struct net_device *dev,
 	}
 }
 
-static void dsa_cpu_port_get_ethtool_stats(struct net_device *dev,
-					   struct ethtool_stats *stats,
-					   uint64_t *data)
-{
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
-	struct dsa_switch *ds = cpu_dp->ds;
-	s8 cpu_port = cpu_dp->index;
-	int count = 0;
-
-	if (cpu_dp->ethtool_ops.get_sset_count) {
-		count = cpu_dp->ethtool_ops.get_sset_count(dev, ETH_SS_STATS);
-		cpu_dp->ethtool_ops.get_ethtool_stats(dev, stats, data);
-	}
-
-	if (ds->ops->get_ethtool_stats)
-		ds->ops->get_ethtool_stats(ds, cpu_port, data + count);
-}
-
-static int dsa_cpu_port_get_sset_count(struct net_device *dev, int sset)
-{
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
-	struct dsa_switch *ds = cpu_dp->ds;
-	int count = 0;
-
-	if (cpu_dp->ethtool_ops.get_sset_count)
-		count += cpu_dp->ethtool_ops.get_sset_count(dev, sset);
-
-	if (sset == ETH_SS_STATS && ds->ops->get_sset_count)
-		count += ds->ops->get_sset_count(ds);
-
-	return count;
-}
-
-static void dsa_cpu_port_get_strings(struct net_device *dev,
-				     uint32_t stringset, uint8_t *data)
-{
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
-	struct dsa_switch *ds = cpu_dp->ds;
-	s8 cpu_port = cpu_dp->index;
-	int len = ETH_GSTRING_LEN;
-	int mcount = 0, count;
-	unsigned int i;
-	uint8_t pfx[4];
-	uint8_t *ndata;
-
-	snprintf(pfx, sizeof(pfx), "p%.2d", cpu_port);
-	/* We do not want to be NULL-terminated, since this is a prefix */
-	pfx[sizeof(pfx) - 1] = '_';
-
-	if (cpu_dp->ethtool_ops.get_sset_count) {
-		mcount = cpu_dp->ethtool_ops.get_sset_count(dev, ETH_SS_STATS);
-		cpu_dp->ethtool_ops.get_strings(dev, stringset, data);
-	}
-
-	if (stringset == ETH_SS_STATS && ds->ops->get_strings) {
-		ndata = data + mcount * len;
-		/* This function copies ETH_GSTRINGS_LEN bytes, we will mangle
-		 * the output after to prepend our CPU port prefix we
-		 * constructed earlier
-		 */
-		ds->ops->get_strings(ds, cpu_port, ndata);
-		count = ds->ops->get_sset_count(ds);
-		for (i = 0; i < count; i++) {
-			memmove(ndata + (i * len + sizeof(pfx)),
-				ndata + i * len, len - sizeof(pfx));
-			memcpy(ndata + i * len, pfx, sizeof(pfx));
-		}
-	}
-}
-
 static void dsa_slave_get_ethtool_stats(struct net_device *dev,
 					struct ethtool_stats *stats,
 					uint64_t *data)
@@ -976,13 +903,6 @@ static void dsa_slave_get_stats64(struct net_device *dev,
 	}
 }
 
-void dsa_cpu_port_ethtool_init(struct ethtool_ops *ops)
-{
-	ops->get_sset_count = dsa_cpu_port_get_sset_count;
-	ops->get_ethtool_stats = dsa_cpu_port_get_ethtool_stats;
-	ops->get_strings = dsa_cpu_port_get_strings;
-}
-
 static int dsa_slave_get_rxnfc(struct net_device *dev,
 			       struct ethtool_rxnfc *nfc, u32 *rule_locs)
 {
-- 
2.14.1

^ permalink raw reply related

* [PATCH net-next 3/4] net: dsa: change dsa_ptr for a dsa_master
From: Vivien Didelot @ 2017-08-31 18:37 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
	Andrew Lunn, Vivien Didelot
In-Reply-To: <20170831183746.2109-1-vivien.didelot@savoirfairelinux.com>

The dsa_ptr of a net_device currently points to a dsa_switch_tree.

This is not correct because the interface is physically linked to a
switch port, and because several network interface could point to
several (so called "CPU") ports of the same DSA switch tree.

Because the tagging operations are specific to the DSA master port and
not the whole switch tree, move the tag_ops pointer in dsa_master.

Finally change the dsa_ptr from dsa_switch_tree to a dsa_master.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/linux/netdevice.h |  4 ++--
 include/net/dsa.h         | 21 ++++++++++-----------
 net/dsa/dsa.c             |  6 +++---
 net/dsa/dsa2.c            | 13 +++++--------
 net/dsa/dsa_priv.h        |  8 ++------
 net/dsa/legacy.c          | 13 ++++---------
 net/dsa/master.c          | 28 ++++++++++++++++++++++------
 net/dsa/slave.c           |  2 +-
 net/dsa/tag_brcm.c        |  5 ++---
 net/dsa/tag_dsa.c         |  3 ++-
 net/dsa/tag_edsa.c        |  3 ++-
 net/dsa/tag_ksz.c         |  5 ++---
 net/dsa/tag_lan9303.c     |  6 ++----
 net/dsa/tag_mtk.c         | 12 ++----------
 net/dsa/tag_qca.c         | 12 ++----------
 net/dsa/tag_trailer.c     |  5 ++---
 16 files changed, 65 insertions(+), 81 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 29bf06ff157c..eb0501c9afc5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -55,7 +55,7 @@
 struct netpoll_info;
 struct device;
 struct phy_device;
-struct dsa_switch_tree;
+struct dsa_master;
 
 /* 802.11 specific */
 struct wireless_dev;
@@ -1752,7 +1752,7 @@ struct net_device {
 	struct vlan_info __rcu	*vlan_info;
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA)
-	struct dsa_switch_tree	*dsa_ptr;
+	struct dsa_master	*dsa_ptr;
 #endif
 #if IS_ENABLED(CONFIG_TIPC)
 	struct tipc_bearer __rcu *tipc_ptr;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index f4a5afc4255b..d5b24cd10f79 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -130,11 +130,6 @@ struct dsa_switch_tree {
 	 */
 	struct dsa_platform_data	*pd;
 
-	/* Copy of tag_ops->rcv for faster access in hot path */
-	struct sk_buff *	(*rcv)(struct sk_buff *skb,
-				       struct net_device *dev,
-				       struct packet_type *pt);
-
 	/*
 	 * The switch port to which the CPU is attached.
 	 */
@@ -144,12 +139,6 @@ struct dsa_switch_tree {
 	 * Data for the individual switch chips.
 	 */
 	struct dsa_switch	*ds[DSA_MAX_SWITCHES];
-
-	/*
-	 * Tagging protocol operations for adding and removing an
-	 * encapsulation tag.
-	 */
-	const struct dsa_device_ops *tag_ops;
 };
 
 /* TC matchall action types, only mirroring for now */
@@ -177,6 +166,16 @@ struct dsa_master {
 	struct dsa_port *port;
 	struct net_device *netdev;
 
+	/*
+	 * Tagging protocol operations for adding and removing an
+	 * encapsulation tag.
+	 */
+	const struct dsa_device_ops *tag_ops;
+
+	/* Copy of tag_ops->rcv for faster access in hot path */
+	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev,
+			       struct packet_type *pt);
+
 	/* Original copy of the master netdev ethtool_ops */
 	const struct ethtool_ops *orig_ethtool_ops;
 	struct ethtool_ops ethtool_ops;
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 81c852e32821..f2ebd3572d6f 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -160,12 +160,12 @@ EXPORT_SYMBOL_GPL(dsa_dev_to_net_device);
 static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 			  struct packet_type *pt, struct net_device *unused)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_master *master = dev->dsa_ptr;
 	struct sk_buff *nskb = NULL;
 	struct pcpu_sw_netstats *s;
 	struct dsa_slave_priv *p;
 
-	if (unlikely(dst == NULL)) {
+	if (unlikely(!master)) {
 		kfree_skb(skb);
 		return 0;
 	}
@@ -174,7 +174,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb)
 		return 0;
 
-	nskb = dst->rcv(skb, dev, pt);
+	nskb = master->rcv(skb, dev, pt);
 	if (!nskb) {
 		kfree_skb(skb);
 		return 0;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index d60f681b96cc..80a9706324f2 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -444,7 +444,7 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst)
 	 * sent to the tag format's receive function.
 	 */
 	wmb();
-	dst->master->netdev->dsa_ptr = dst;
+	dst->master->netdev->dsa_ptr = dst->master;
 	dst->applied = true;
 
 	return 0;
@@ -487,9 +487,9 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
 			 struct dsa_switch_tree *dst,
 			 struct dsa_switch *ds)
 {
-	enum dsa_tag_protocol tag_protocol;
 	struct net_device *ethernet_dev;
 	struct device_node *ethernet;
+	int err;
 
 	if (port->dn) {
 		ethernet = of_parse_phandle(port->dn, "ethernet", 0);
@@ -516,16 +516,13 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
 	 */
 	ds->cpu_port_mask |= BIT(index);
 
-	tag_protocol = ds->ops->get_tag_protocol(ds);
-	dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
-	if (IS_ERR(dst->tag_ops)) {
+	err = dsa_master_tag_protocol(dst->master);
+	if (err) {
 		dev_warn(ds->dev, "No tagger for this switch\n");
 		ds->cpu_port_mask &= ~BIT(index);
-		return PTR_ERR(dst->tag_ops);
+		return err;
 	}
 
-	dst->rcv = dst->tag_ops->rcv;
-
 	return 0;
 }
 
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 4fa14b4305b9..59f155cbbe87 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -66,7 +66,7 @@ struct dsa_notifier_vlan_info {
 };
 
 struct dsa_slave_priv {
-	/* Copy of dp->ds->dst->tag_ops->xmit for faster access in hot path */
+	/* Copy of the master xmit tagging op for faster access in hot path */
 	struct sk_buff *	(*xmit)(struct sk_buff *skb,
 					struct net_device *dev);
 
@@ -115,6 +115,7 @@ struct dsa_master *dsa_master_create(struct dsa_port *port,
 				     struct net_device *netdev);
 int dsa_master_ethtool_setup(struct dsa_master *master);
 void dsa_master_ethtool_restore(struct dsa_master *master);
+int dsa_master_tag_protocol(struct dsa_master *master);
 
 /* port.c */
 int dsa_port_set_state(struct dsa_port *dp, u8 state,
@@ -183,9 +184,4 @@ static inline struct net_device *dsa_master_netdev(struct dsa_slave_priv *p)
 	return p->dp->ds->dst->master->netdev;
 }
 
-static inline struct dsa_port *dsa_get_cpu_port(struct dsa_switch_tree *dst)
-{
-	return dst->master->port;
-}
-
 #endif
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index acd81c488bf4..3afedd9758c5 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -147,14 +147,9 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
 	 * switch.
 	 */
 	if (dst->master->port->ds == ds) {
-		enum dsa_tag_protocol tag_protocol;
-
-		tag_protocol = ops->get_tag_protocol(ds);
-		dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
-		if (IS_ERR(dst->tag_ops))
-			return PTR_ERR(dst->tag_ops);
-
-		dst->rcv = dst->tag_ops->rcv;
+		ret = dsa_master_tag_protocol(dst->master);
+		if (ret)
+			return ret;
 	}
 
 	memcpy(ds->rtable, cd->rtable, sizeof(ds->rtable));
@@ -607,7 +602,7 @@ static int dsa_setup_dst(struct dsa_switch_tree *dst, struct net_device *dev,
 	 * sent to the tag format's receive function.
 	 */
 	wmb();
-	dev->dsa_ptr = dst;
+	dev->dsa_ptr = dst->master;
 
 	return 0;
 }
diff --git a/net/dsa/master.c b/net/dsa/master.c
index 9ecce3e7c8df..6714750ee13f 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -16,8 +16,7 @@ static void dsa_master_get_ethtool_stats(struct net_device *dev,
 					 struct ethtool_stats *stats,
 					 uint64_t *data)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_master *master = dst->master;
+	struct dsa_master *master = dev->dsa_ptr;
 	struct dsa_port *port = master->port;
 	struct dsa_switch *ds = port->ds;
 	int count = 0;
@@ -33,8 +32,7 @@ static void dsa_master_get_ethtool_stats(struct net_device *dev,
 
 static int dsa_master_get_sset_count(struct net_device *dev, int sset)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_master *master = dst->master;
+	struct dsa_master *master = dev->dsa_ptr;
 	struct dsa_port *port = master->port;
 	struct dsa_switch *ds = port->ds;
 	int count = 0;
@@ -51,8 +49,7 @@ static int dsa_master_get_sset_count(struct net_device *dev, int sset)
 static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
 				   uint8_t *data)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_master *master = dst->master;
+	struct dsa_master *master = dev->dsa_ptr;
 	struct dsa_port *port = master->port;
 	struct dsa_switch *ds = port->ds;
 	int len = ETH_GSTRING_LEN;
@@ -116,6 +113,25 @@ void dsa_master_ethtool_restore(struct dsa_master *master)
 	master->orig_ethtool_ops = NULL;
 }
 
+int dsa_master_tag_protocol(struct dsa_master *master)
+{
+	struct dsa_switch *ds = master->port->ds;
+	enum dsa_tag_protocol proto;
+
+	if (!ds->ops->get_tag_protocol)
+		return -EOPNOTSUPP;
+
+	proto = ds->ops->get_tag_protocol(ds);
+
+	master->tag_ops = dsa_resolve_tag_protocol(proto);
+	if (IS_ERR(master->tag_ops))
+		return PTR_ERR(master->tag_ops);
+
+	master->rcv = master->tag_ops->rcv;
+
+	return 0;
+}
+
 struct dsa_master *dsa_master_create(struct dsa_port *port,
 				     struct net_device *netdev)
 {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 99f9e48a6cee..d5a03ef8d0e0 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1206,7 +1206,7 @@ int dsa_slave_create(struct dsa_port *port, const char *name)
 	}
 	p->dp = port;
 	INIT_LIST_HEAD(&p->mall_tc_list);
-	p->xmit = dst->tag_ops->xmit;
+	p->xmit = dst->master->tag_ops->xmit;
 
 	p->old_pause = -1;
 	p->old_link = -1;
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index de74c3f77818..417266f23bed 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -91,9 +91,8 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
 static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 				    struct packet_type *pt)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
-	struct dsa_switch *ds = cpu_dp->ds;
+	struct dsa_master *master = dev->dsa_ptr;
+	struct dsa_switch *ds = master->port->ds;
 	int source_port;
 	u8 *brcm_tag;
 
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index fbf9ca954773..7897bbd1a110 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -67,7 +67,8 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
 			       struct packet_type *pt)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_master *master = dev->dsa_ptr;
+	struct dsa_switch_tree *dst = master->port->ds->dst;
 	struct dsa_switch *ds;
 	u8 *dsa_header;
 	int source_device;
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 76367ba1b2e2..12f9efed6251 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -80,7 +80,8 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
 				struct packet_type *pt)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_master *master = dev->dsa_ptr;
+	struct dsa_switch_tree *dst = master->port->ds->dst;
 	struct dsa_switch *ds;
 	u8 *edsa_header;
 	int source_device;
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 17f30675c15c..7043d51a7a80 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -78,9 +78,8 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
 static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev,
 			       struct packet_type *pt)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
-	struct dsa_switch *ds = cpu_dp->ds;
+	struct dsa_master *master = dev->dsa_ptr;
+	struct dsa_switch *ds = master->port->ds;
 	u8 *tag;
 	int source_port;
 
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 0b9826105e42..afb3df46712a 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -70,13 +70,11 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
 static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
 			struct packet_type *pt)
 {
+	struct dsa_master *master = dev->dsa_ptr;
+	struct dsa_switch *ds = master->port->ds;
 	u16 *lan9303_tag;
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_switch *ds;
 	unsigned int source_port;
 
-	ds = dst->ds[0];
-
 	if (unlikely(!ds)) {
 		dev_warn_ratelimited(&dev->dev, "Dropping packet, due to missing DSA switch device\n");
 		return NULL;
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index ec8ee5f43255..7af26afaa8ac 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -46,8 +46,8 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 				   struct packet_type *pt)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_switch *ds;
+	struct dsa_master *master = dev->dsa_ptr;
+	struct dsa_switch *ds = master->port->ds;
 	int port;
 	__be16 *phdr, hdr;
 
@@ -68,14 +68,6 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 		skb->data - ETH_HLEN - MTK_HDR_LEN,
 		2 * ETH_ALEN);
 
-	/* This protocol doesn't support cascading multiple
-	 * switches so it's safe to assume the switch is first
-	 * in the tree.
-	 */
-	ds = dst->ds[0];
-	if (!ds)
-		return NULL;
-
 	/* Get source port information */
 	port = (hdr & MTK_HDR_RECV_SOURCE_PORT_MASK);
 	if (!ds->ports[port].netdev)
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 1d4c70711c0f..a49cc7c6fac9 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -65,9 +65,8 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 				   struct packet_type *pt)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
-	struct dsa_switch *ds;
+	struct dsa_master *master = dev->dsa_ptr;
+	struct dsa_switch *ds = master->port->ds;
 	u8 ver;
 	int port;
 	__be16 *phdr, hdr;
@@ -92,13 +91,6 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 	memmove(skb->data - ETH_HLEN, skb->data - ETH_HLEN - QCA_HDR_LEN,
 		ETH_HLEN - QCA_HDR_LEN);
 
-	/* This protocol doesn't support cascading multiple switches so it's
-	 * safe to assume the switch is first in the tree
-	 */
-	ds = cpu_dp->ds;
-	if (!ds)
-		return NULL;
-
 	/* Get source port information */
 	port = (hdr & QCA_HDR_RECV_SOURCE_PORT_MASK);
 	if (!ds->ports[port].netdev)
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index 8707157dea32..a3dacfb48dc0 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -58,9 +58,8 @@ static struct sk_buff *trailer_xmit(struct sk_buff *skb, struct net_device *dev)
 static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev,
 				   struct packet_type *pt)
 {
-	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
-	struct dsa_switch *ds = cpu_dp->ds;
+	struct dsa_master *master = dev->dsa_ptr;
+	struct dsa_switch *ds = master->port->ds;
 	u8 *trailer;
 	int source_port;
 
-- 
2.14.1

^ permalink raw reply related

* Re: [Patch net-next] net_sched: add reverse binding for tc class
From: David Miller @ 2017-08-31 18:41 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev, jhs
In-Reply-To: <20170830213036.24250-1-xiyou.wangcong@gmail.com>

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 30 Aug 2017 14:30:36 -0700

> TC filters when used as classifiers are bound to TC classes.
> However, there is a hidden difference when adding them in different
> orders:
> 
> 1. If we add tc classes before its filters, everything is fine.
>    Logically, the classes exist before we specify their ID's in
>    filters, it is easy to bind them together, just as in the current
>    code base.
> 
> 2. If we add tc filters before the tc classes they bind, we have to
>    do dynamic lookup in fast path. What's worse, this happens all
>    the time not just once, because on fast path tcf_result is passed
>    on stack, there is no way to propagate back to the one in tc filters.
> 
> This hidden difference hurts performance silently if we have many tc
> classes in hierarchy.
> 
> This patch intends to close this gap by doing the reverse binding when
> we create a new class, in this case we can actually search all the
> filters in its parent, match and fixup by classid. And because
> tcf_result is specific to each type of tc filter, we have to introduce
> a new ops for each filter to tell how to bind the class.
> 
> Note, we still can NOT totally get rid of those class lookup in
> ->enqueue() because cgroup and flow filters have no way to determine
> the classid at setup time, they still have to go through dynamic lookup.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied, thanks.

^ permalink raw reply

* (unknown), 
From: helga.brickl @ 2017-08-31 18:41 UTC (permalink / raw)
  To: netdev

[-- Attachment #1: 66881.doc --]
[-- Type: application/msword, Size: 41431 bytes --]

^ permalink raw reply

* [PATCH net-next 4/4] net: dsa: assign a master to slave ports
From: Vivien Didelot @ 2017-08-31 18:37 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
	Andrew Lunn, Vivien Didelot
In-Reply-To: <20170831183746.2109-1-vivien.didelot@savoirfairelinux.com>

Because each DSA slave port may use a different DSA master port, add a
pointer to a master in the slave structure. This is a preparatory patch
for multiple CPU ports.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa_priv.h |  7 ++-----
 net/dsa/slave.c    | 33 ++++++++++++++++++---------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 59f155cbbe87..a8cd6cbe4061 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -66,6 +66,8 @@ struct dsa_notifier_vlan_info {
 };
 
 struct dsa_slave_priv {
+	struct dsa_master *master;
+
 	/* Copy of the master xmit tagging op for faster access in hot path */
 	struct sk_buff *	(*xmit)(struct sk_buff *skb,
 					struct net_device *dev);
@@ -179,9 +181,4 @@ extern const struct dsa_device_ops qca_netdev_ops;
 /* tag_trailer.c */
 extern const struct dsa_device_ops trailer_netdev_ops;
 
-static inline struct net_device *dsa_master_netdev(struct dsa_slave_priv *p)
-{
-	return p->dp->ds->dst->master->netdev;
-}
-
 #endif
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d5a03ef8d0e0..107ed0c4a850 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -66,7 +66,7 @@ static int dsa_slave_get_iflink(const struct net_device *dev)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 
-	return dsa_master_netdev(p)->ifindex;
+	return p->master->netdev->ifindex;
 }
 
 static int dsa_slave_open(struct net_device *dev)
@@ -74,7 +74,7 @@ static int dsa_slave_open(struct net_device *dev)
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_port *dp = p->dp;
 	struct dsa_switch *ds = dp->ds;
-	struct net_device *master = dsa_master_netdev(p);
+	struct net_device *master = p->master->netdev;
 	u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING;
 	int err;
 
@@ -127,7 +127,7 @@ static int dsa_slave_open(struct net_device *dev)
 static int dsa_slave_close(struct net_device *dev)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct net_device *master = dsa_master_netdev(p);
+	struct net_device *master = p->master->netdev;
 	struct dsa_switch *ds = p->dp->ds;
 
 	if (p->phy)
@@ -154,7 +154,7 @@ static int dsa_slave_close(struct net_device *dev)
 static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct net_device *master = dsa_master_netdev(p);
+	struct net_device *master = p->master->netdev;
 
 	if (change & IFF_ALLMULTI)
 		dev_set_allmulti(master, dev->flags & IFF_ALLMULTI ? 1 : -1);
@@ -165,7 +165,7 @@ static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
 static void dsa_slave_set_rx_mode(struct net_device *dev)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct net_device *master = dsa_master_netdev(p);
+	struct net_device *master = p->master->netdev;
 
 	dev_mc_sync(master, dev);
 	dev_uc_sync(master, dev);
@@ -174,7 +174,7 @@ static void dsa_slave_set_rx_mode(struct net_device *dev)
 static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct net_device *master = dsa_master_netdev(p);
+	struct net_device *master = p->master->netdev;
 	struct sockaddr *addr = a;
 	int err;
 
@@ -427,7 +427,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Queue the SKB for transmission on the parent interface, but
 	 * do not modify its EtherType
 	 */
-	nskb->dev = dsa_master_netdev(p);
+	nskb->dev = p->master->netdev;
 	dev_queue_xmit(nskb);
 
 	return NETDEV_TX_OK;
@@ -687,7 +687,7 @@ static int dsa_slave_netpoll_setup(struct net_device *dev,
 				   struct netpoll_info *ni)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct net_device *master = dsa_master_netdev(p);
+	struct net_device *master = p->master->netdev;
 	struct netpoll *netpoll;
 	int err = 0;
 
@@ -1170,7 +1170,7 @@ int dsa_slave_create(struct dsa_port *port, const char *name)
 {
 	struct dsa_switch *ds = port->ds;
 	struct dsa_switch_tree *dst = ds->dst;
-	struct net_device *master = dst->master->netdev;
+	struct dsa_master *master = dst->master;
 	struct net_device *slave_dev;
 	struct dsa_slave_priv *p;
 	int ret;
@@ -1180,10 +1180,10 @@ int dsa_slave_create(struct dsa_port *port, const char *name)
 	if (slave_dev == NULL)
 		return -ENOMEM;
 
-	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
+	slave_dev->features = master->netdev->vlan_features | NETIF_F_HW_TC;
 	slave_dev->hw_features |= NETIF_F_HW_TC;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
-	eth_hw_addr_inherit(slave_dev, master);
+	eth_hw_addr_inherit(slave_dev, master->netdev);
 	slave_dev->priv_flags |= IFF_NO_QUEUE;
 	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
 	slave_dev->switchdev_ops = &dsa_slave_switchdev_ops;
@@ -1196,7 +1196,7 @@ int dsa_slave_create(struct dsa_port *port, const char *name)
 
 	SET_NETDEV_DEV(slave_dev, port->ds->dev);
 	slave_dev->dev.of_node = port->dn;
-	slave_dev->vlan_features = master->vlan_features;
+	slave_dev->vlan_features = master->netdev->vlan_features;
 
 	p = netdev_priv(slave_dev);
 	p->stats64 = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
@@ -1206,7 +1206,8 @@ int dsa_slave_create(struct dsa_port *port, const char *name)
 	}
 	p->dp = port;
 	INIT_LIST_HEAD(&p->mall_tc_list);
-	p->xmit = dst->master->tag_ops->xmit;
+	p->master = master;
+	p->xmit = master->tag_ops->xmit;
 
 	p->old_pause = -1;
 	p->old_link = -1;
@@ -1215,7 +1216,8 @@ int dsa_slave_create(struct dsa_port *port, const char *name)
 	port->netdev = slave_dev;
 	ret = register_netdev(slave_dev);
 	if (ret) {
-		netdev_err(master, "error %d registering interface %s\n",
+		netdev_err(master->netdev,
+			   "error %d registering interface %s\n",
 			   ret, slave_dev->name);
 		port->netdev = NULL;
 		free_percpu(p->stats64);
@@ -1227,7 +1229,8 @@ int dsa_slave_create(struct dsa_port *port, const char *name)
 
 	ret = dsa_slave_phy_setup(p, slave_dev);
 	if (ret) {
-		netdev_err(master, "error %d setting up slave phy\n", ret);
+		netdev_err(master->netdev,
+			   "error %d setting up slave phy\n", ret);
 		unregister_netdev(slave_dev);
 		free_percpu(p->stats64);
 		free_netdev(slave_dev);
-- 
2.14.1

^ permalink raw reply related

* Re: [PATCH net-next] bridge: add tracepoint in br_fdb_update
From: David Miller @ 2017-08-31 18:43 UTC (permalink / raw)
  To: roopa; +Cc: netdev, nikolay, f.fainelli, andrew, bridge
In-Reply-To: <1504156693-24692-1-git-send-email-roopa@cumulusnetworks.com>

From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Wed, 30 Aug 2017 22:18:13 -0700

> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> This extends bridge fdb table tracepoints to also cover
> learned fdb entries in the br_fdb_update path. Note that
> unlike other tracepoints I have moved this to when the fdb
> is modified because this is in the datapath and can generate
> a lot of noise in the trace output. br_fdb_update is also called
> from added_by_user context in the NTF_USE case which is already
> traced ..hence the !added_by_user check.
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Applied.

Let's use dev->name for now and if the tooling can eventually
do transparent ifindex->name then we can consider redoing
a bunch of networking tracepoints.

Thanks.

^ permalink raw reply

* Re: [PATCH] net: dccp: Add handling of IPV6_PKTOPTIONS to dccp_v6_do_rcv()
From: David Miller @ 2017-08-31 18:44 UTC (permalink / raw)
  To: tulup; +Cc: gerrit, netdev, linux-kernel, dccp
In-Reply-To: <d782f2bb-ade5-62ed-f20f-4854e68a2b93@mail.ru>

From: Andrii <tulup@mail.ru>
Date: Thu, 31 Aug 2017 08:28:01 +0300

> Add handling of IPV6_PKTOPTIONS to dccp_v6_do_rcv() in
> net/dccp/ipv6.c, similar
> to the handling in net/ipv6/tcp_ipv6.c
> 
> Signed-off-by: Andrii Vladyka <tulup@mail.ru>

Applied.

^ permalink raw reply

* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection
From: Kees Cook @ 2017-08-31 18:45 UTC (permalink / raw)
  To: Mike Galbraith
  Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar,
	Reshetova, Elena, Network Development
In-Reply-To: <1504199967.666.16.camel@gmx.de>

On Thu, Aug 31, 2017 at 10:19 AM, Mike Galbraith <efault@gmx.de> wrote:
> On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote:
>>
>> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct:
>>
>> 4.8.5: WARN, eventual kernel hang
>> 6.3.1, 7.0.1: WARN, but continues working
>
> Yeah, that's correct.  I find that troubling, simply because this gcc
> version has been through one hell of a lot of kernels with me.  Yeah, I
> know, that doesn't exempt it from having bugs, but color me suspicious.

I still can't hit this with a 4.8.5 build. :(

With _RATELIMIT removed, this should, in theory, report whatever goes
negative first...

diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index c076f710de4c..d4fc6b91c0e6 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -72,6 +72,10 @@ bool ex_handler_refcount(const struct
exception_table_entry *fixup,
                bool zero = regs->flags & X86_EFLAGS_ZF;

                refcount_error_report(regs, zero ? "hit zero" : "overflow");
+       } else if (regs->flags & X86_EFLAGS_SF) {
+               refcount_error_report(regs, "result was negative");
+       } else {
+               refcount_error_report(regs, "unknown state");
        }

        return true;
diff --git a/kernel/panic.c b/kernel/panic.c
index bdd18afa19a4..966ade491543 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -605,7 +605,7 @@ EXPORT_SYMBOL(__stack_chk_fail);
 #ifdef CONFIG_ARCH_HAS_REFCOUNT
 void refcount_error_report(struct pt_regs *regs, const char *err)
 {
-       WARN_RATELIMIT(1, "refcount_t %s at %pB in %s[%d], uid/euid: %u/%u\n",
+       WARN(1, "refcount_t %s at %pB in %s[%d], uid/euid: %u/%u\n",
                err, (void *)instruction_pointer(regs),
                current->comm, task_pid_nr(current),
                from_kuid_munged(&init_user_ns, current_uid()),

And if it is still a refcount_inc(), and only on gcc 4.8.5, then I
think we need to study the resulting assembly...

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply related

* Re: [PATCH net-next] bridge: add tracepoint in br_fdb_update
From: Arnaldo Carvalho de Melo @ 2017-08-31 18:56 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: David Ahern, Roopa Prabhu, davem@davemloft.net,
	netdev@vger.kernel.org, Nikolay Aleksandrov, Florian Fainelli,
	Andrew Lunn, bridge, Peter Zijlstra
In-Reply-To: <20170831182012.5d321c6a@redhat.com>

Em Thu, Aug 31, 2017 at 06:20:12PM +0200, Jesper Dangaard Brouer escreveu:
> On Thu, 31 Aug 2017 09:30:05 -0600 David Ahern <dsahern@gmail.com> wrote:
> > > On Thu, Aug 31, 2017 at 5:38 AM, Jesper Dangaard Brouer <brouer@redhat.com> wrote:  
> > > These bridge tracepoints in context are primarily for debugging fdb
> > > updates only, not for every packet and hence not in the performance
> > > path.
> > > In large scale deployments with thousands of bridge ports and fdb
> > > entries, dev->name will definately make it easier to trouble-shoot.
> > > So, I did like to leave these with dev->name unless there are strong objections.  

> > +1 for user friendliness for debugging tracepoints. The device name is
> > also more user friendly when adding filters to the data collection.

> > Being able to add bpf everywhere certainly changes the game a bit, but
> > we should not relinquish ease of use and understanding for the potential
> > that someone might want to put a bpf program on the tracepoint and want
> > to maintain high performance.
 
> (Cc. Acme and Peterz)
> I wonder if we can create a special perf-tracepoint type for ifindex'es
> and the tool reading (e.g. perf-script) can perform the name lookup in
> userspace (calling if_indextoname(3)) ?
 
> I don't know the perf tools well enough to know if this is possible?

Yeah, there are libtraceevent plugins, and that gets used by trace-cmd
and perf script, perf trace.

[root@jouet ~]# ls -la ~/.traceevent/plugins/
total 192
drwxr-xr-x. 2 acme acme  4096 Aug 31 15:29 .
drwxr-xr-x. 3 acme acme  4096 Jan 27  2017 ..
-rwxr-xr-x. 1 acme acme 13744 Aug 31 15:29 plugin_cfg80211.so
-rwxr-xr-x. 1 acme acme 20192 Aug 31 15:29 plugin_function.so
-rwxr-xr-x. 1 acme acme 13680 Aug 31 15:29 plugin_hrtimer.so
-rwxr-xr-x. 1 acme acme 13760 Aug 31 15:29 plugin_jbd2.so
-rwxr-xr-x. 1 acme acme 13704 Aug 31 15:29 plugin_kmem.so
-rwxr-xr-x. 1 acme acme 28568 Aug 31 15:29 plugin_kvm.so
-rwxr-xr-x. 1 acme acme 14184 Aug 31 15:29 plugin_mac80211.so
-rwxr-xr-x. 1 acme acme 14424 Aug 31 15:29 plugin_sched_switch.so
-rwxr-xr-x. 1 acme acme 20136 Aug 31 15:29 plugin_scsi.so
-rwxr-xr-x. 1 acme acme 14504 Aug 31 15:29 plugin_xen.so
[root@jouet ~]#

But... that index is something that is mutable, i.e. you'd have to
somehow record all the assignments of an index to an interface and then,
when processing the events, get from that state the mapping you want.

So you don't store the device name by doing lookups at each of those
high volume tracepoints, store just the index, but then, when
establishing the mapping, collect that as well and we come up with some
infrastructure to get that mapping in a place where a plugin can do the
lookups at post processing time.

For instance, the hrtimer plugin will get an address from the kernel,
and, from a state recorded at the same time as the trace file, will
lookup elf symbol tables for the kernel or modules and resolve that
symbol, etc.

- Arnaldo

^ permalink raw reply

* Re: [PATCH] wl1251: add a missing spin_lock_init()
From: Pavel Machek @ 2017-08-31 18:57 UTC (permalink / raw)
  To: David Miller
  Cc: kvalo-sgV2jX0FEOL9JmXXK+q4OQ,
	torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	xiyou.wangcong-Re5JQEeQqe8AvxtiuMwx3w,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170831.103305.1809212990680774811.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

Hi!

> From: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> Date: Thu, 31 Aug 2017 16:47:43 +0200
> 
> > Dave, Linus -- can you still take the patch?
> 
> Pavel, please do not bypass maintainers like this.
> 
> It's really rude, and if you do things like that instead of
> trying to work properly with us, your relationship with
> these maintainers will suffer in the long term.

Do you mean I'm being rude to Kalle, or rude to you?

In the part you snipped, Kalle asked me to do just that:

# I'm not planning to submit pull requests to 4.13 anymore. If you think
# this is so important that it needs to be applied in the last minute (I
# don't) you could always try to convince Dave to take it directly.

..and as I still believe patch should go in, that's what I did.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* Re: [PATCH net-next] net: fix two typos in net_device_ops documentation.
From: David Miller @ 2017-08-31 18:57 UTC (permalink / raw)
  To: rami.rosen; +Cc: netdev, jasowang
In-Reply-To: <1504176100-28914-1-git-send-email-rami.rosen@intel.com>

From: Rami Rosen <rami.rosen@intel.com>
Date: Thu, 31 Aug 2017 13:41:40 +0300

> This patch fixes two trivial typos in net_device_ops documentation,
> related to ndo_xdp_flush callback.
> 
> Signed-off-by: Rami Rosen <rami.rosen@intel.com>

Applied.

^ permalink raw reply

* Re: [PATCH net-next] samples/bpf: Fix compilation issue in redirect dummy program
From: David Miller @ 2017-08-31 18:58 UTC (permalink / raw)
  To: tariqt; +Cc: netdev, eranbe, brouer
In-Reply-To: <1504178199-12410-1-git-send-email-tariqt@mellanox.com>

From: Tariq Toukan <tariqt@mellanox.com>
Date: Thu, 31 Aug 2017 14:16:39 +0300

> Fix compilation error below:
> 
> $ make samples/bpf/
> 
> LLVM ERROR: 'xdp_redirect_dummy' label emitted multiple times to
> assembly file
> make[1]: *** [samples/bpf/xdp_redirect_kern.o] Error 1
> make: *** [samples/bpf/] Error 2
> 
> Fixes: 306da4e685b4 ("samples/bpf: xdp_redirect load XDP dummy prog on TX device")
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>

Applied.

^ permalink raw reply

* Re: [PATCH net-next] x86: bpf_jit: small optimization in emit_bpf_tail_call()
From: David Miller @ 2017-08-31 18:58 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev, ast, daniel
In-Reply-To: <1504180422.15310.12.camel@edumazet-glaptop3.roam.corp.google.com>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 31 Aug 2017 04:53:42 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Saves 4 bytes replacing following instructions :
> 
> lea rax, [rsi + rdx * 8 + offsetof(...)] 
> mov rax, qword ptr [rax]
> cmp rax, 0
> 
> by :
> 
> mov rax, [rsi + rdx * 8 + offsetof(...)] 
> test rax, rax
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.

^ permalink raw reply

* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-08-31 19:09 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Marc Gonzalez, David Daney, netdev, Geert Uytterhoeven,
	David Miller, Andrew Lunn, Mans Rullgard
In-Reply-To: <a75691d9-c22a-9b89-2cce-604315062739@gmail.com>

On 31/08/2017 19:03, Florian Fainelli wrote:

> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>
>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>
>>> The original motivation for this change originated from Marc Gonzalez
>>> indicating that his network driver did not have its adjust_link callback
>>> executing with phydev->link = 0 while he was expecting it.
>>
>> I expect the core to call phy_adjust_link() for link changes.
>> This used to work back in 3.4 and was broken somewhere along
>> the way.
> 
> If that was working correctly in 3.4 surely we can look at the diff and
> figure out what changed, even maybe find the offending commit, can you
> do that?

Bisecting would a be a huge pain because my platform was
not upstream until v4.4

You mentioned the guarantees made by PHYLIB.
When is the adjust_link callback guaranteed to be called?

>>> PHYLIB has never made any such guarantees ever because phy_stop() merely
>>> just tells the workqueue to move into PHY_HALTED state which will happen
>>> asynchronously.
>>
>> My original proposal was to fix the issue in the driver.
>> I'll try locating it in my archives.
> 
> Yes I remember you telling that, by the way I don't think you ever
> provided a clear explanation why this is absolutely necessary for your
> driver though?

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.

2) nb8800_link_reconfigure() does some HW init when
the link state changes. If we miss some notifications,
we might not perform some HW init, and stuff breaks.

Regards.

^ permalink raw reply

* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-08-31 19:18 UTC (permalink / raw)
  To: Mason
  Cc: Marc Gonzalez, David Daney, netdev, Geert Uytterhoeven,
	David Miller, Andrew Lunn, Mans Rullgard
In-Reply-To: <d54d0555-c448-741c-eb63-5a773bed5a30@free.fr>

On 08/31/2017 12:09 PM, Mason wrote:
> On 31/08/2017 19:03, Florian Fainelli wrote:
> 
>> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>>
>>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>>
>>>> The original motivation for this change originated from Marc Gonzalez
>>>> indicating that his network driver did not have its adjust_link callback
>>>> executing with phydev->link = 0 while he was expecting it.
>>>
>>> I expect the core to call phy_adjust_link() for link changes.
>>> This used to work back in 3.4 and was broken somewhere along
>>> the way.
>>
>> If that was working correctly in 3.4 surely we can look at the diff and
>> figure out what changed, even maybe find the offending commit, can you
>> do that?
> 
> Bisecting would a be a huge pain because my platform was
> not upstream until v4.4

Then just diff the file and try to pinpoint which commit may have
changed that?

> 
> You mentioned the guarantees made by PHYLIB.
> When is the adjust_link callback guaranteed to be called?

As long as the state machine is running after a call to phy_start()
adjust_link will be called if there a change in link and/or link
settings. Once you call phy_stop() no such guarantees are made.

> 
>>>> PHYLIB has never made any such guarantees ever because phy_stop() merely
>>>> just tells the workqueue to move into PHY_HALTED state which will happen
>>>> asynchronously.
>>>
>>> My original proposal was to fix the issue in the driver.
>>> I'll try locating it in my archives.
>>
>> Yes I remember you telling that, by the way I don't think you ever
>> provided a clear explanation why this is absolutely necessary for your
>> driver though?
> 
> 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.

Seeing a random message upon bringing the interface back up suggests you
may not be re-initialization your old vs. new link state book keeping
variables and state transitions are not properly detected and therefore
not printed. In fact, I don't see where priv->link is ever set to say -1
to force the comparison between phydev->link != priv->link to be true,
oversight?

> 
> 2) nb8800_link_reconfigure() does some HW init when
> the link state changes. If we miss some notifications,
> we might not perform some HW init, and stuff breaks.

Care to be more specific? What specific HW init is required during link
notification that if not done breaks the HW? There is both
nb8800_mac_config() and nb8800_pause_config() that are both called in
the adjust_link callback both could presumably be deferred until the
link is detected, so why do you need it during ndo_stop() absolutely?
-- 
Florian

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox