Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP
From: Arnd Bergmann @ 2017-05-23 20:56 UTC (permalink / raw)
  To: Ihar Hrachyshka; +Cc: David Miller, ja, Networking
In-Reply-To: <cover.1495136258.git.ihrachys@redhat.com>

On Thu, May 18, 2017 at 9:41 PM, Ihar Hrachyshka <ihrachys@redhat.com> wrote:
> This patchset is spurred by discussion started at
> https://patchwork.ozlabs.org/patch/760372/ where we figured that there is no
> real reason for enforcing override by gratuitous ARP packets only when
> arp_accept is 1. Same should happen when it's 0 (the default value).
>
> changelog v2: handled review comments by Julian Anastasov
> - fixed a mistake in a comment;
> - postponed addr_type calculation to as late as possible.

This seems to have caused a build warning:

net/ipv4/arp.c:880:35: warning: 'addr_type' may be used uninitialized
in this function [-Wmaybe-uninitialized]

       Arnd

^ permalink raw reply

* Re: [PATCH v6 net-next 17/17] net: qualcomm: add QCA7000 UART driver
From: Lino Sanfilippo @ 2017-05-23 21:01 UTC (permalink / raw)
  To: Stefan Wahren, Rob Herring, Mark Rutland, David S. Miller
  Cc: linux-serial, Jiri Slaby, Greg Kroah-Hartman, netdev,
	linux-kernel, Jakub Kicinski, devicetree
In-Reply-To: <1059621060.236992.1495568289051@email.1und1.de>

On 23.05.2017 21:38, Stefan Wahren wrote:
> 
>> Lino Sanfilippo <LinoSanfilippo@gmx.de> hat am 23. Mai 2017 um 20:16 geschrieben:
>>
>>
>> Hi,
>>
>> On 23.05.2017 15:12, Stefan Wahren wrote:
>>
>>
>>> +}
>>> +
>>> +static void qca_uart_remove(struct serdev_device *serdev)
>>> +{
>>> +	struct qcauart *qca = serdev_device_get_drvdata(serdev);
>>> +
>>> +	netif_carrier_off(qca->net_dev);
>>> +	cancel_work_sync(&qca->tx_work);
>>> +	unregister_netdev(qca->net_dev);
>>
>> Note that it is still possible that the tx work is queued right after cancel_work_sync()
>> returned and before the net device is unregistered (and thus the check for the net device
>> being up at the beginning of the tx work function is passed and the function is executed).
> 
> Even if the carrier is off? Since i see this pattern in some drivers, can you please point me to a reference like a thread or something else?
> 

The check in the tx work function is against the "running" state not against the carrier. So why should 
the carrier matter in this case? 


>> I suggest to avoid this possible race by first unregistering the netdevice and then 
>> calling cancel_work_sync().
> 
> What makes you sure that's safe to unregister the netdev while the tx work queue is possibly active?

unregister_netdevice() calls netdev_close() if the interface is still up. netdev_close() calls flush_work()
so the unregistration is delayed until the tx work function is finished. Furthermore both close() and
tx work are synchronized by means of the qca->lock which also guarantees that unregister_netdevice() wont
be finished until the tx work is done.

But I may have missed something and if unregistering the device while the tx work could be running worries you,
we could first close and later unregister the device like in the following sequence:


dev_close();
/* the tx work wont be scheduled any more now, however we have to wait for a potentially
   earlier scheduled work */ 
cancel_work_sync(&qca->tx_work);
/* we can be sure that the tx work will neither be running nor be started again, so 
   it is safe to unregister the netdev */
unregister_netdev(qca->net_dev);
serdev_device_close(serdev);
free_netdev(qca->net_dev);
 
What do you think?

Regards,
Lino

^ permalink raw reply

* [PATCHv2 net-next 2/5] net: dsa: mv88e6xxx: Refactor mv88e6352 SERDES code into an op
From: Andrew Lunn @ 2017-05-23 21:04 UTC (permalink / raw)
  To: David Miller; +Cc: Vivien Didelot, netdev, Andrew Lunn
In-Reply-To: <1495573486-31827-1-git-send-email-andrew@lunn.ch>

The mv88e6390 family has a different SERDES implementation. Refactor
the mv88e6352 code into an ops function, so we can later add the
mv88e6390 code.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/Makefile    |  1 +
 drivers/net/dsa/mv88e6xxx/chip.c      | 64 +++++++++---------------------
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h |  6 +--
 drivers/net/dsa/mv88e6xxx/serdes.c    | 75 +++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/serdes.h    | 24 +++++++++++
 5 files changed, 122 insertions(+), 48 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/serdes.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/serdes.h

diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index e4372eaf3bc5..5cd5551461e3 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -6,3 +6,4 @@ mv88e6xxx-objs += global1_vtu.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2) += global2.o
 mv88e6xxx-objs += phy.o
 mv88e6xxx-objs += port.o
+mv88e6xxx-objs += serdes.o
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index a3c7756dc01b..b029d01a0a5f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -38,6 +38,7 @@
 #include "global2.h"
 #include "phy.h"
 #include "port.h"
+#include "serdes.h"
 
 static void assert_reg_lock(struct mv88e6xxx_chip *chip)
 {
@@ -234,18 +235,6 @@ struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip)
 	return mdio_bus->bus;
 }
 
-static int mv88e6xxx_serdes_read(struct mv88e6xxx_chip *chip, int reg, u16 *val)
-{
-	return mv88e6xxx_phy_page_read(chip, ADDR_SERDES, SERDES_PAGE_FIBER,
-				       reg, val);
-}
-
-static int mv88e6xxx_serdes_write(struct mv88e6xxx_chip *chip, int reg, u16 val)
-{
-	return mv88e6xxx_phy_page_write(chip, ADDR_SERDES, SERDES_PAGE_FIBER,
-					reg, val);
-}
-
 static void mv88e6xxx_g1_irq_mask(struct irq_data *d)
 {
 	struct mv88e6xxx_chip *chip = irq_data_get_irq_chip_data(d);
@@ -1733,24 +1722,6 @@ static int mv88e6xxx_switch_reset(struct mv88e6xxx_chip *chip)
 	return mv88e6xxx_software_reset(chip);
 }
 
-static int mv88e6xxx_serdes_power_on(struct mv88e6xxx_chip *chip)
-{
-	u16 val;
-	int err;
-
-	/* Clear Power Down bit */
-	err = mv88e6xxx_serdes_read(chip, MII_BMCR, &val);
-	if (err)
-		return err;
-
-	if (val & BMCR_PDOWN) {
-		val &= ~BMCR_PDOWN;
-		err = mv88e6xxx_serdes_write(chip, MII_BMCR, val);
-	}
-
-	return err;
-}
-
 static int mv88e6xxx_set_port_mode(struct mv88e6xxx_chip *chip, int port,
 				   enum mv88e6xxx_frame_mode frame, u16 egress,
 				   u16 etype)
@@ -1832,6 +1803,15 @@ static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
 	return 0;
 }
 
+static int mv88e6xxx_serdes_power(struct mv88e6xxx_chip *chip, int port,
+				  bool on)
+{
+	if (chip->info->ops->serdes_power)
+		return chip->info->ops->serdes_power(chip, port, on);
+
+	return 0;
+}
+
 static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 {
 	struct dsa_switch *ds = chip->ds;
@@ -1882,22 +1862,12 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	if (err)
 		return err;
 
-	/* If this port is connected to a SerDes, make sure the SerDes is not
-	 * powered down.
+	/* If this port is connected to a SerDes, make sure the SerDes is
+	 * powered up.
 	 */
-	if (mv88e6xxx_has(chip, MV88E6XXX_FLAGS_SERDES)) {
-		err = mv88e6xxx_port_read(chip, port, PORT_STATUS, &reg);
-		if (err)
-			return err;
-		reg &= PORT_STATUS_CMODE_MASK;
-		if ((reg == PORT_STATUS_CMODE_100BASE_X) ||
-		    (reg == PORT_STATUS_CMODE_1000BASE_X) ||
-		    (reg == PORT_STATUS_CMODE_SGMII)) {
-			err = mv88e6xxx_serdes_power_on(chip);
-			if (err < 0)
-				return err;
-		}
-	}
+	err = mv88e6xxx_serdes_power(chip, port, true);
+	if (err)
+		return err;
 
 	/* Port Control 2: don't force a good FCS, set the maximum frame size to
 	 * 10240 bytes, disable 802.1q tags checking, don't discard tagged or
@@ -2662,6 +2632,7 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+	.serdes_power = mv88e6352_serdes_power,
 };
 
 static const struct mv88e6xxx_ops mv88e6175_ops = {
@@ -2726,6 +2697,7 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+	.serdes_power = mv88e6352_serdes_power,
 };
 
 static const struct mv88e6xxx_ops mv88e6185_ops = {
@@ -2882,6 +2854,7 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+	.serdes_power = mv88e6352_serdes_power,
 };
 
 static const struct mv88e6xxx_ops mv88e6290_ops = {
@@ -3104,6 +3077,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+	.serdes_power = mv88e6352_serdes_power,
 };
 
 static const struct mv88e6xxx_ops mv88e6390_ops = {
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 45b387c780a8..fb996491b111 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -37,9 +37,6 @@
 #define PHY_PAGE		0x16
 #define PHY_PAGE_COPPER		0x00
 
-#define ADDR_SERDES		0x0f
-#define SERDES_PAGE_FIBER	0x01
-
 #define PORT_STATUS		0x00
 #define PORT_STATUS_PAUSE_EN	BIT(15)
 #define PORT_STATUS_MY_PAUSE	BIT(14)
@@ -884,6 +881,9 @@ struct mv88e6xxx_ops {
 	/* Can be either in g1 or g2, so don't use a prefix */
 	int (*mgmt_rsvd2cpu)(struct mv88e6xxx_chip *chip);
 
+	/* Power on/off a SERDES interface */
+	int (*serdes_power)(struct mv88e6xxx_chip *chip, int port, bool on);
+
 	/* VLAN Translation Unit operations */
 	int (*vtu_getnext)(struct mv88e6xxx_chip *chip,
 			   struct mv88e6xxx_vtu_entry *entry);
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
new file mode 100644
index 000000000000..235f5f0c30ae
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -0,0 +1,75 @@
+/*
+ * Marvell 88E6xxx SERDES manipulation, via SMI bus
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2017 Andrew Lunn <andrew@lunn.ch>
+ *
+ * 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 <linux/mii.h>
+
+#include "mv88e6xxx.h"
+#include "phy.h"
+#include "port.h"
+#include "serdes.h"
+
+static int mv88e6352_serdes_read(struct mv88e6xxx_chip *chip, int reg,
+				 u16 *val)
+{
+	return mv88e6xxx_phy_page_read(chip, MV88E6352_ADDR_SERDES,
+				       MV88E6352_SERDES_PAGE_FIBER,
+				       reg, val);
+}
+
+static int mv88e6352_serdes_write(struct mv88e6xxx_chip *chip, int reg,
+				  u16 val)
+{
+	return mv88e6xxx_phy_page_write(chip, MV88E6352_ADDR_SERDES,
+					MV88E6352_SERDES_PAGE_FIBER,
+					reg, val);
+}
+
+static int mv88e6352_serdes_power_set(struct mv88e6xxx_chip *chip, bool on)
+{
+	u16 val, new_val;
+	int err;
+
+	err = mv88e6352_serdes_read(chip, MII_BMCR, &val);
+	if (err)
+		return err;
+
+	if (on)
+		new_val = val & ~BMCR_PDOWN;
+	else
+		new_val = val | BMCR_PDOWN;
+
+	if (val != new_val)
+		err = mv88e6352_serdes_write(chip, MII_BMCR, new_val);
+
+	return err;
+}
+
+int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
+{
+	int err;
+	u8 cmode;
+
+	err = mv88e6xxx_port_get_cmode(chip, port, &cmode);
+	if (err)
+		return err;
+
+	if ((cmode == PORT_STATUS_CMODE_100BASE_X) ||
+	    (cmode == PORT_STATUS_CMODE_1000BASE_X) ||
+	    (cmode == PORT_STATUS_CMODE_SGMII)) {
+		err = mv88e6352_serdes_power_set(chip, on);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
new file mode 100644
index 000000000000..a690be09ac52
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -0,0 +1,24 @@
+/*
+ * Marvell 88E6xxx SERDES manipulation, via SMI bus
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2016 Andrew Lunn <andrew@lunn.ch>
+ *
+ * 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.
+ */
+
+#ifndef _MV88E6XXX_SERDES_H
+#define _MV88E6XXX_SERDES_H
+
+#include "mv88e6xxx.h"
+
+#define MV88E6352_ADDR_SERDES		0x0f
+#define MV88E6352_SERDES_PAGE_FIBER	0x01
+
+int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
+
+#endif
-- 
2.11.0

^ permalink raw reply related

* [PATCHv2 net-next 4/5] net: dsa: mv88e6xxx: mv88e6390X SERDES support
From: Andrew Lunn @ 2017-05-23 21:04 UTC (permalink / raw)
  To: David Miller; +Cc: Vivien Didelot, netdev, Andrew Lunn
In-Reply-To: <1495573486-31827-1-git-send-email-andrew@lunn.ch>

The mv88e6390X family has 8 SERDES lanes. These can be used for 2
10Gbps ports, ports 9 or 10. If these ports are used at slower speeds,
the SERDES lanes become available for other ports for 1000Base-X.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c   |   6 ++
 drivers/net/dsa/mv88e6xxx/serdes.c | 154 +++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/serdes.h |  24 ++++++
 3 files changed, 184 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b029d01a0a5f..fc4d93b5c59f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2757,6 +2757,7 @@ static const struct mv88e6xxx_ops mv88e6190_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
+	.serdes_power = mv88e6390_serdes_power,
 };
 
 static const struct mv88e6xxx_ops mv88e6190x_ops = {
@@ -2789,6 +2790,7 @@ static const struct mv88e6xxx_ops mv88e6190x_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
+	.serdes_power = mv88e6390_serdes_power,
 };
 
 static const struct mv88e6xxx_ops mv88e6191_ops = {
@@ -2821,6 +2823,7 @@ static const struct mv88e6xxx_ops mv88e6191_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
+	.serdes_power = mv88e6390_serdes_power,
 };
 
 static const struct mv88e6xxx_ops mv88e6240_ops = {
@@ -2888,6 +2891,7 @@ static const struct mv88e6xxx_ops mv88e6290_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
+	.serdes_power = mv88e6390_serdes_power,
 };
 
 static const struct mv88e6xxx_ops mv88e6320_ops = {
@@ -3113,6 +3117,7 @@ static const struct mv88e6xxx_ops mv88e6390_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
+	.serdes_power = mv88e6390_serdes_power,
 };
 
 static const struct mv88e6xxx_ops mv88e6390x_ops = {
@@ -3147,6 +3152,7 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
+	.serdes_power = mv88e6390_serdes_power,
 };
 
 static const struct mv88e6xxx_info mv88e6xxx_table[] = {
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 235f5f0c30ae..53795676bd70 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -13,6 +13,7 @@
 
 #include <linux/mii.h>
 
+#include "global2.h"
 #include "mv88e6xxx.h"
 #include "phy.h"
 #include "port.h"
@@ -73,3 +74,156 @@ int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
 
 	return 0;
 }
+
+/* Set the power on/off for 10GBASE-R and 10GBASE-X4/X2 */
+static int mv88e6390_serdes_10g(struct mv88e6xxx_chip *chip, int addr, bool on)
+{
+	u16 val, new_val;
+	int reg_c45;
+	int err;
+
+	reg_c45 = MII_ADDR_C45 | MV88E6390_SERDES_DEVICE |
+		MV88E6390_PCS_CONTROL_1;
+	err = mv88e6xxx_phy_read(chip, addr, reg_c45, &val);
+	if (err)
+		return err;
+
+	if (on)
+		new_val = val & ~(MV88E6390_PCS_CONTROL_1_RESET |
+				  MV88E6390_PCS_CONTROL_1_LOOPBACK |
+				  MV88E6390_PCS_CONTROL_1_PDOWN);
+	else
+		new_val = val | MV88E6390_PCS_CONTROL_1_PDOWN;
+
+	if (val != new_val)
+		err = mv88e6xxx_phy_write(chip, addr, reg_c45, new_val);
+
+	return err;
+}
+
+/* Set the power on/off for 10GBASE-R and 10GBASE-X4/X2 */
+static int mv88e6390_serdes_sgmii(struct mv88e6xxx_chip *chip, int addr,
+				  bool on)
+{
+	u16 val, new_val;
+	int reg_c45;
+	int err;
+
+	reg_c45 = MII_ADDR_C45 | MV88E6390_SERDES_DEVICE |
+		MV88E6390_SGMII_CONTROL;
+	err = mv88e6xxx_phy_read(chip, addr, reg_c45, &val);
+	if (err)
+		return err;
+
+	if (on)
+		new_val = val & ~(MV88E6390_SGMII_CONTROL_RESET |
+				  MV88E6390_SGMII_CONTROL_LOOPBACK |
+				  MV88E6390_SGMII_CONTROL_PDOWN);
+	else
+		new_val = val | MV88E6390_SGMII_CONTROL_PDOWN;
+
+	if (val != new_val)
+		err = mv88e6xxx_phy_write(chip, addr, reg_c45, new_val);
+
+	return err;
+}
+
+static int mv88e6390_serdes_lower(struct mv88e6xxx_chip *chip, u8 cmode,
+				  int port_donor, int lane, bool rxaui, bool on)
+{
+	int err;
+	u8 cmode_donor;
+
+	err = mv88e6xxx_port_get_cmode(chip, port_donor, &cmode_donor);
+	if (err)
+		return err;
+
+	switch (cmode_donor) {
+	case PORT_STATUS_CMODE_RXAUI:
+		if (!rxaui)
+			break;
+		/* Fall through */
+	case PORT_STATUS_CMODE_1000BASE_X:
+	case PORT_STATUS_CMODE_SGMII:
+	case PORT_STATUS_CMODE_2500BASEX:
+		if (cmode == PORT_STATUS_CMODE_1000BASE_X ||
+		    cmode == PORT_STATUS_CMODE_SGMII)
+			return	mv88e6390_serdes_sgmii(chip, lane, on);
+	}
+	return 0;
+}
+
+static int mv88e6390_serdes_port9(struct mv88e6xxx_chip *chip, u8 cmode,
+				  bool on)
+{
+	switch (cmode) {
+	case PORT_STATUS_CMODE_1000BASE_X:
+	case PORT_STATUS_CMODE_SGMII:
+		return mv88e6390_serdes_sgmii(chip, MV88E6390_PORT9_LANE0, on);
+	case PORT_STATUS_CMODE_XAUI:
+	case PORT_STATUS_CMODE_RXAUI:
+	case PORT_STATUS_CMODE_2500BASEX:
+		return mv88e6390_serdes_10g(chip, MV88E6390_PORT9_LANE0, on);
+	}
+
+	return 0;
+}
+
+static int mv88e6390_serdes_port10(struct mv88e6xxx_chip *chip, u8 cmode,
+				   bool on)
+{
+	switch (cmode) {
+	case PORT_STATUS_CMODE_SGMII:
+		return mv88e6390_serdes_sgmii(chip, MV88E6390_PORT10_LANE0, on);
+	case PORT_STATUS_CMODE_XAUI:
+	case PORT_STATUS_CMODE_RXAUI:
+	case PORT_STATUS_CMODE_1000BASE_X:
+	case PORT_STATUS_CMODE_2500BASEX:
+		return mv88e6390_serdes_10g(chip, MV88E6390_PORT10_LANE0, on);
+	}
+
+	return 0;
+}
+
+int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
+{
+	u8 cmode;
+	int err;
+
+	err = mv88e6xxx_port_get_cmode(chip, port, &cmode);
+	if (err)
+		return cmode;
+
+	switch (port) {
+	case 2:
+		return mv88e6390_serdes_lower(chip, cmode, 9,
+					      MV88E6390_PORT9_LANE1,
+					      false, on);
+	case 3:
+		return mv88e6390_serdes_lower(chip, cmode, 9,
+					      MV88E6390_PORT9_LANE2,
+					      true, on);
+	case 4:
+		return mv88e6390_serdes_lower(chip, cmode, 9,
+					      MV88E6390_PORT9_LANE3,
+					      true, on);
+	case 5:
+		return mv88e6390_serdes_lower(chip, cmode, 10,
+					      MV88E6390_PORT10_LANE1,
+					      false, on);
+	case 6:
+		return mv88e6390_serdes_lower(chip, cmode, 10,
+					      MV88E6390_PORT10_LANE2,
+					      true, on);
+	case 7:
+		return mv88e6390_serdes_lower(chip, cmode, 10,
+					      MV88E6390_PORT10_LANE3,
+					      true, on);
+	case 9:
+		return mv88e6390_serdes_port9(chip, cmode, on);
+	case 10:
+		return mv88e6390_serdes_port10(chip, cmode, on);
+	}
+
+	return 0;
+}
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index a690be09ac52..eb3ceaef790f 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -19,6 +19,30 @@
 #define MV88E6352_ADDR_SERDES		0x0f
 #define MV88E6352_SERDES_PAGE_FIBER	0x01
 
+#define MV88E6390_PORT9_LANE0		0x09
+#define MV88E6390_PORT9_LANE1		0x12
+#define MV88E6390_PORT9_LANE2		0x13
+#define MV88E6390_PORT9_LANE3		0x14
+#define MV88E6390_PORT10_LANE0		0x0a
+#define MV88E6390_PORT10_LANE1		0x15
+#define MV88E6390_PORT10_LANE2		0x16
+#define MV88E6390_PORT10_LANE3		0x17
+#define MV88E6390_SERDES_DEVICE		(4 << 16)
+
+/* 10GBASE-R and 10GBASE-X4/X2 */
+#define MV88E6390_PCS_CONTROL_1		0x1000
+#define MV88E6390_PCS_CONTROL_1_RESET		BIT(15)
+#define MV88E6390_PCS_CONTROL_1_LOOPBACK	BIT(14)
+#define MV88E6390_PCS_CONTROL_1_SPEED		BIT(13)
+#define MV88E6390_PCS_CONTROL_1_PDOWN		BIT(11)
+
+/* 1000BASE-X and SGMII */
+#define MV88E6390_SGMII_CONTROL		0x2000
+#define MV88E6390_SGMII_CONTROL_RESET		BIT(15)
+#define MV88E6390_SGMII_CONTROL_LOOPBACK	BIT(14)
+#define MV88E6390_SGMII_CONTROL_PDOWN		BIT(11)
+
 int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
+int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
 
 #endif
-- 
2.11.0

^ permalink raw reply related

* [PATCHv2 net-next 3/5] net: dsa: mv88e6xxx: Remove SERDES flag
From: Andrew Lunn @ 2017-05-23 21:04 UTC (permalink / raw)
  To: David Miller; +Cc: Vivien Didelot, netdev, Andrew Lunn
In-Reply-To: <1495573486-31827-1-git-send-email-andrew@lunn.ch>

Now that we use an op for SERDES operations, we don't need a flag for
it. Remove it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 23 ++---------------------
 drivers/net/dsa/mv88e6xxx/phy.c       |  3 ---
 2 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index fb996491b111..9087cb009cc3 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -508,14 +508,6 @@ enum mv88e6xxx_cap {
 	MV88E6XXX_CAP_SMI_CMD,		/* (0x00) SMI Command */
 	MV88E6XXX_CAP_SMI_DATA,		/* (0x01) SMI Data */
 
-	/* PHY Registers.
-	 */
-	MV88E6XXX_CAP_PHY_PAGE,		/* (0x16) Page Register */
-
-	/* Fiber/SERDES Registers (SMI address F).
-	 */
-	MV88E6XXX_CAP_SERDES,
-
 	/* Switch Global (1) Registers.
 	 */
 	MV88E6XXX_CAP_G1_ATU_FID,	/* (0x01) ATU FID Register */
@@ -550,10 +542,6 @@ enum mv88e6xxx_cap {
 #define MV88E6XXX_FLAG_SMI_CMD		BIT_ULL(MV88E6XXX_CAP_SMI_CMD)
 #define MV88E6XXX_FLAG_SMI_DATA		BIT_ULL(MV88E6XXX_CAP_SMI_DATA)
 
-#define MV88E6XXX_FLAG_PHY_PAGE		BIT_ULL(MV88E6XXX_CAP_PHY_PAGE)
-
-#define MV88E6XXX_FLAG_SERDES		BIT_ULL(MV88E6XXX_CAP_SERDES)
-
 #define MV88E6XXX_FLAG_G1_VTU_FID	BIT_ULL(MV88E6XXX_CAP_G1_VTU_FID)
 
 #define MV88E6XXX_FLAG_GLOBAL2		BIT_ULL(MV88E6XXX_CAP_GLOBAL2)
@@ -574,11 +562,6 @@ enum mv88e6xxx_cap {
 	(MV88E6XXX_FLAG_SMI_CMD |	\
 	 MV88E6XXX_FLAG_SMI_DATA)
 
-/* Fiber/SERDES Registers at SMI address F, page 1 */
-#define MV88E6XXX_FLAGS_SERDES		\
-	(MV88E6XXX_FLAG_PHY_PAGE |	\
-	 MV88E6XXX_FLAG_SERDES)
-
 #define MV88E6XXX_FLAGS_FAMILY_6095	\
 	(MV88E6XXX_FLAG_GLOBAL2 |	\
 	 MV88E6XXX_FLAG_G2_MGMT_EN_0X |	\
@@ -626,8 +609,7 @@ enum mv88e6xxx_cap {
 	 MV88E6XXX_FLAG_G2_INT |	\
 	 MV88E6XXX_FLAG_G2_POT |	\
 	 MV88E6XXX_FLAGS_IRL |		\
-	 MV88E6XXX_FLAGS_MULTI_CHIP |	\
-	 MV88E6XXX_FLAGS_SERDES)
+	 MV88E6XXX_FLAGS_MULTI_CHIP)
 
 #define MV88E6XXX_FLAGS_FAMILY_6351	\
 	(MV88E6XXX_FLAG_G1_VTU_FID |	\
@@ -648,8 +630,7 @@ enum mv88e6xxx_cap {
 	 MV88E6XXX_FLAG_G2_MGMT_EN_0X |	\
 	 MV88E6XXX_FLAG_G2_POT |	\
 	 MV88E6XXX_FLAGS_IRL |		\
-	 MV88E6XXX_FLAGS_MULTI_CHIP |	\
-	 MV88E6XXX_FLAGS_SERDES)
+	 MV88E6XXX_FLAGS_MULTI_CHIP)
 
 #define MV88E6XXX_FLAGS_FAMILY_6390	\
 	(MV88E6XXX_FLAG_EEE |		\
diff --git a/drivers/net/dsa/mv88e6xxx/phy.c b/drivers/net/dsa/mv88e6xxx/phy.c
index 2ebac599a174..cd8e0b329cd6 100644
--- a/drivers/net/dsa/mv88e6xxx/phy.c
+++ b/drivers/net/dsa/mv88e6xxx/phy.c
@@ -62,9 +62,6 @@ int mv88e6xxx_phy_write(struct mv88e6xxx_chip *chip, int phy, int reg, u16 val)
 
 int mv88e6xxx_phy_page_get(struct mv88e6xxx_chip *chip, int phy, u8 page)
 {
-	if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_PHY_PAGE))
-		return -EOPNOTSUPP;
-
 	return mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page);
 }
 
-- 
2.11.0

^ permalink raw reply related

* [PATCHv2 net-next 5/5] dsa: mv88e6xxx: Enable/Disable SERDES on port enable/disable
From: Andrew Lunn @ 2017-05-23 21:04 UTC (permalink / raw)
  To: David Miller; +Cc: Vivien Didelot, netdev, Andrew Lunn
In-Reply-To: <1495573486-31827-1-git-send-email-andrew@lunn.ch>

Implement the port enable/disable callbacks, which enable/disable the
SERDES interfaces, if applicable. This should save a bit of
power/heat.

We also need to enable SERDES on CPU and DSA ports, so keep the
existing call to the op, but make it conditional.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index fc4d93b5c59f..edab7eb306d9 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1862,12 +1862,15 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	if (err)
 		return err;
 
-	/* If this port is connected to a SerDes, make sure the SerDes is
-	 * powered up.
+	/* Enable the SERDES interface for DSA and CPU ports. Normal
+	 * ports SERDES are enabled when the port is enabled, thus
+	 * saving a bit of power.
 	 */
-	err = mv88e6xxx_serdes_power(chip, port, true);
-	if (err)
-		return err;
+	if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) {
+		err = mv88e6xxx_serdes_power(chip, port, true);
+		if (err)
+			return err;
+	}
 
 	/* Port Control 2: don't force a good FCS, set the maximum frame size to
 	 * 10240 bytes, disable 802.1q tags checking, don't discard tagged or
@@ -1969,6 +1972,31 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	return mv88e6xxx_port_write(chip, port, PORT_DEFAULT_VLAN, 0x0000);
 }
 
+static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
+				 struct phy_device *phydev)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int err = 0;
+
+	mutex_lock(&chip->reg_lock);
+	if (chip->info->ops->serdes_power)
+		err = chip->info->ops->serdes_power(chip, port, true);
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
+static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port,
+				   struct phy_device *phydev)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	mutex_lock(&chip->reg_lock);
+	if (chip->info->ops->serdes_power)
+		chip->info->ops->serdes_power(chip, port, false);
+	mutex_unlock(&chip->reg_lock);
+}
+
 static int mv88e6xxx_g1_set_switch_mac(struct mv88e6xxx_chip *chip, u8 *addr)
 {
 	int err;
@@ -3821,6 +3849,8 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
+	.port_enable		= mv88e6xxx_port_enable,
+	.port_disable		= mv88e6xxx_port_disable,
 	.set_eee		= mv88e6xxx_set_eee,
 	.get_eee		= mv88e6xxx_get_eee,
 	.get_eeprom_len		= mv88e6xxx_get_eeprom_len,
-- 
2.11.0

^ permalink raw reply related

* [PATCHv2 net-next 0/5] net: dsa: mv88e6xxx: Add basic SERDES support
From: Andrew Lunn @ 2017-05-23 21:04 UTC (permalink / raw)
  To: David Miller; +Cc: Vivien Didelot, netdev, Andrew Lunn

Some of the Marvell switches are SERDES interface, which must be
powered up before packets can be passed. This is particularly true on
the 6390, where the SERDES defaults to down, probably to save power.

This series refactors the existing SERDES support for the 6352, and
adds 6390 support.

v2:

Split phy functions out into phy.[ch]
Don't add MV88E6XXX_FLAG_G1_ATU_FID back again
Move the serdes op up in mv88e6xxx_ops
Move some #defines into serdes.h
Add a mv88e6xxx_serdes_power()
Don't keep moving calls to this helper around in the code

Andrew Lunn (5):
  net: dsa: mv88e6xxx: Move phy functions into phy.[ch]
  net: dsa: mv88e6xxx: Refactor mv88e6352 SERDES code into an op
  net: dsa: mv88e6xxx: Remove SERDES flag
  net: dsa: mv88e6xxx: mv88e6390X SERDES support
  dsa: mv88e6xxx: Enable/Disable SERDES on port enable/disable

 drivers/net/dsa/mv88e6xxx/Makefile    |   2 +
 drivers/net/dsa/mv88e6xxx/chip.c      | 315 ++++++----------------------------
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h |  31 +---
 drivers/net/dsa/mv88e6xxx/phy.c       | 232 +++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/phy.h       |  40 +++++
 drivers/net/dsa/mv88e6xxx/serdes.c    | 229 ++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/serdes.h    |  48 ++++++
 7 files changed, 611 insertions(+), 286 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/phy.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/phy.h
 create mode 100644 drivers/net/dsa/mv88e6xxx/serdes.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/serdes.h

-- 
2.11.0

^ permalink raw reply

* Re: [PATCH net-next] net: dsa: support cross-chip ageing time
From: Florian Fainelli @ 2017-05-23 21:06 UTC (permalink / raw)
  To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn
In-Reply-To: <20170523192059.1720-1-vivien.didelot@savoirfairelinux.com>

On 05/23/2017 12:20 PM, Vivien Didelot wrote:
> Now that the switchdev bridge ageing time attribute is propagated to all
> switch chips of the fabric, each switch can check if the requested value
> is valid and program itself, so that the whole fabric shares a common
> ageing time setting.
> 
> This is especially needed for switch chips in between others, containing
> no bridge port members but evidently used in the data path.
> 
> To achieve that, remove the condition which skips the other switches. We
> also don't need to identify the target switch anymore, thus remove the
> sw_index member of the dsa_notifier_ageing_time_info notifier structure.
> 
> On ZII Dev Rev B (with two 88E6352 and one 88E6185) and ZII Dev Rev C
> (with two 88E6390X), we have the following hardware configuration:
> 
>     # ip link add name br0 type bridge
>     # ip link set master br0 dev lan6
>     br0: port 1(lan6) entered blocking state
>     br0: port 1(lan6) entered disabled state
>     # echo 2000 > /sys/class/net/br0/bridge/ageing_time
> 
> Before this patch:
> 
>     zii-rev-b# cat /sys/kernel/debug/mv88e6xxx/sw*/age_time
>     300000
>     300000
>     15000
> 
>     zii-rev-c# cat /sys/kernel/debug/mv88e6xxx/sw*/age_time
>     300000
>     18750
> 
> After this patch:
> 
>     zii-rev-b# cat /sys/kernel/debug/mv88e6xxx/sw*/age_time
>     15000
>     15000
>     15000
> 
>     zii-rev-c# cat /sys/kernel/debug/mv88e6xxx/sw*/age_time
>     18750
>     18750
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply

* [PATCHv2 net-next 1/5] net: dsa: mv88e6xxx: Move phy functions into phy.[ch]
From: Andrew Lunn @ 2017-05-23 21:04 UTC (permalink / raw)
  To: David Miller; +Cc: Vivien Didelot, netdev, Andrew Lunn
In-Reply-To: <1495573486-31827-1-git-send-email-andrew@lunn.ch>

The upcoming SERDES support will need to make use of PHY functions. Move
them out into a file of there own. No code changes.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/Makefile    |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c      | 221 +-------------------------------
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h |   2 +-
 drivers/net/dsa/mv88e6xxx/phy.c       | 235 ++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/phy.h       |  40 ++++++
 5 files changed, 279 insertions(+), 220 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/phy.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/phy.h

diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index 6edd869c8d6f..e4372eaf3bc5 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -4,4 +4,5 @@ mv88e6xxx-objs += global1.o
 mv88e6xxx-objs += global1_atu.o
 mv88e6xxx-objs += global1_vtu.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2) += global2.o
+mv88e6xxx-objs += phy.o
 mv88e6xxx-objs += port.o
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 41de250dbcc3..a3c7756dc01b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -36,6 +36,7 @@
 #include "mv88e6xxx.h"
 #include "global1.h"
 #include "global2.h"
+#include "phy.h"
 #include "port.h"
 
 static void assert_reg_lock(struct mv88e6xxx_chip *chip)
@@ -221,21 +222,7 @@ int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val)
 	return 0;
 }
 
-static int mv88e6165_phy_read(struct mv88e6xxx_chip *chip,
-			      struct mii_bus *bus,
-			      int addr, int reg, u16 *val)
-{
-	return mv88e6xxx_read(chip, addr, reg, val);
-}
-
-static int mv88e6165_phy_write(struct mv88e6xxx_chip *chip,
-			       struct mii_bus *bus,
-			       int addr, int reg, u16 val)
-{
-	return mv88e6xxx_write(chip, addr, reg, val);
-}
-
-static struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip)
+struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip)
 {
 	struct mv88e6xxx_mdio_bus *mdio_bus;
 
@@ -247,94 +234,6 @@ static struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip)
 	return mdio_bus->bus;
 }
 
-static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
-			      int reg, u16 *val)
-{
-	int addr = phy; /* PHY devices addresses start at 0x0 */
-	struct mii_bus *bus;
-
-	bus = mv88e6xxx_default_mdio_bus(chip);
-	if (!bus)
-		return -EOPNOTSUPP;
-
-	if (!chip->info->ops->phy_read)
-		return -EOPNOTSUPP;
-
-	return chip->info->ops->phy_read(chip, bus, addr, reg, val);
-}
-
-static int mv88e6xxx_phy_write(struct mv88e6xxx_chip *chip, int phy,
-			       int reg, u16 val)
-{
-	int addr = phy; /* PHY devices addresses start at 0x0 */
-	struct mii_bus *bus;
-
-	bus = mv88e6xxx_default_mdio_bus(chip);
-	if (!bus)
-		return -EOPNOTSUPP;
-
-	if (!chip->info->ops->phy_write)
-		return -EOPNOTSUPP;
-
-	return chip->info->ops->phy_write(chip, bus, addr, reg, val);
-}
-
-static int mv88e6xxx_phy_page_get(struct mv88e6xxx_chip *chip, int phy, u8 page)
-{
-	if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_PHY_PAGE))
-		return -EOPNOTSUPP;
-
-	return mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page);
-}
-
-static void mv88e6xxx_phy_page_put(struct mv88e6xxx_chip *chip, int phy)
-{
-	int err;
-
-	/* Restore PHY page Copper 0x0 for access via the registered MDIO bus */
-	err = mv88e6xxx_phy_write(chip, phy, PHY_PAGE, PHY_PAGE_COPPER);
-	if (unlikely(err)) {
-		dev_err(chip->dev, "failed to restore PHY %d page Copper (%d)\n",
-			phy, err);
-	}
-}
-
-static int mv88e6xxx_phy_page_read(struct mv88e6xxx_chip *chip, int phy,
-				   u8 page, int reg, u16 *val)
-{
-	int err;
-
-	/* There is no paging for registers 22 */
-	if (reg == PHY_PAGE)
-		return -EINVAL;
-
-	err = mv88e6xxx_phy_page_get(chip, phy, page);
-	if (!err) {
-		err = mv88e6xxx_phy_read(chip, phy, reg, val);
-		mv88e6xxx_phy_page_put(chip, phy);
-	}
-
-	return err;
-}
-
-static int mv88e6xxx_phy_page_write(struct mv88e6xxx_chip *chip, int phy,
-				    u8 page, int reg, u16 val)
-{
-	int err;
-
-	/* There is no paging for registers 22 */
-	if (reg == PHY_PAGE)
-		return -EINVAL;
-
-	err = mv88e6xxx_phy_page_get(chip, phy, page);
-	if (!err) {
-		err = mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page);
-		mv88e6xxx_phy_page_put(chip, phy);
-	}
-
-	return err;
-}
-
 static int mv88e6xxx_serdes_read(struct mv88e6xxx_chip *chip, int reg, u16 *val)
 {
 	return mv88e6xxx_phy_page_read(chip, ADDR_SERDES, SERDES_PAGE_FIBER,
@@ -560,122 +459,6 @@ int mv88e6xxx_update(struct mv88e6xxx_chip *chip, int addr, int reg, u16 update)
 	return mv88e6xxx_write(chip, addr, reg, val);
 }
 
-static int mv88e6xxx_ppu_disable(struct mv88e6xxx_chip *chip)
-{
-	if (!chip->info->ops->ppu_disable)
-		return 0;
-
-	return chip->info->ops->ppu_disable(chip);
-}
-
-static int mv88e6xxx_ppu_enable(struct mv88e6xxx_chip *chip)
-{
-	if (!chip->info->ops->ppu_enable)
-		return 0;
-
-	return chip->info->ops->ppu_enable(chip);
-}
-
-static void mv88e6xxx_ppu_reenable_work(struct work_struct *ugly)
-{
-	struct mv88e6xxx_chip *chip;
-
-	chip = container_of(ugly, struct mv88e6xxx_chip, ppu_work);
-
-	mutex_lock(&chip->reg_lock);
-
-	if (mutex_trylock(&chip->ppu_mutex)) {
-		if (mv88e6xxx_ppu_enable(chip) == 0)
-			chip->ppu_disabled = 0;
-		mutex_unlock(&chip->ppu_mutex);
-	}
-
-	mutex_unlock(&chip->reg_lock);
-}
-
-static void mv88e6xxx_ppu_reenable_timer(unsigned long _ps)
-{
-	struct mv88e6xxx_chip *chip = (void *)_ps;
-
-	schedule_work(&chip->ppu_work);
-}
-
-static int mv88e6xxx_ppu_access_get(struct mv88e6xxx_chip *chip)
-{
-	int ret;
-
-	mutex_lock(&chip->ppu_mutex);
-
-	/* If the PHY polling unit is enabled, disable it so that
-	 * we can access the PHY registers.  If it was already
-	 * disabled, cancel the timer that is going to re-enable
-	 * it.
-	 */
-	if (!chip->ppu_disabled) {
-		ret = mv88e6xxx_ppu_disable(chip);
-		if (ret < 0) {
-			mutex_unlock(&chip->ppu_mutex);
-			return ret;
-		}
-		chip->ppu_disabled = 1;
-	} else {
-		del_timer(&chip->ppu_timer);
-		ret = 0;
-	}
-
-	return ret;
-}
-
-static void mv88e6xxx_ppu_access_put(struct mv88e6xxx_chip *chip)
-{
-	/* Schedule a timer to re-enable the PHY polling unit. */
-	mod_timer(&chip->ppu_timer, jiffies + msecs_to_jiffies(10));
-	mutex_unlock(&chip->ppu_mutex);
-}
-
-static void mv88e6xxx_ppu_state_init(struct mv88e6xxx_chip *chip)
-{
-	mutex_init(&chip->ppu_mutex);
-	INIT_WORK(&chip->ppu_work, mv88e6xxx_ppu_reenable_work);
-	setup_timer(&chip->ppu_timer, mv88e6xxx_ppu_reenable_timer,
-		    (unsigned long)chip);
-}
-
-static void mv88e6xxx_ppu_state_destroy(struct mv88e6xxx_chip *chip)
-{
-	del_timer_sync(&chip->ppu_timer);
-}
-
-static int mv88e6xxx_phy_ppu_read(struct mv88e6xxx_chip *chip,
-				  struct mii_bus *bus,
-				  int addr, int reg, u16 *val)
-{
-	int err;
-
-	err = mv88e6xxx_ppu_access_get(chip);
-	if (!err) {
-		err = mv88e6xxx_read(chip, addr, reg, val);
-		mv88e6xxx_ppu_access_put(chip);
-	}
-
-	return err;
-}
-
-static int mv88e6xxx_phy_ppu_write(struct mv88e6xxx_chip *chip,
-				   struct mii_bus *bus,
-				   int addr, int reg, u16 val)
-{
-	int err;
-
-	err = mv88e6xxx_ppu_access_get(chip);
-	if (!err) {
-		err = mv88e6xxx_write(chip, addr, reg, val);
-		mv88e6xxx_ppu_access_put(chip);
-	}
-
-	return err;
-}
-
 static int mv88e6xxx_port_setup_mac(struct mv88e6xxx_chip *chip, int port,
 				    int link, int speed, int duplex,
 				    phy_interface_t mode)
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 77236cd72df2..45b387c780a8 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -942,5 +942,5 @@ int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val);
 int mv88e6xxx_update(struct mv88e6xxx_chip *chip, int addr, int reg,
 		     u16 update);
 int mv88e6xxx_wait(struct mv88e6xxx_chip *chip, int addr, int reg, u16 mask);
-
+struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip);
 #endif
diff --git a/drivers/net/dsa/mv88e6xxx/phy.c b/drivers/net/dsa/mv88e6xxx/phy.c
new file mode 100644
index 000000000000..2ebac599a174
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/phy.c
@@ -0,0 +1,235 @@
+/*
+ * Marvell 88e6xxx Ethernet switch PHY and PPU support
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2017 Andrew Lunn <andrew@lunn.ch>
+ *
+ * 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 <linux/mdio.h>
+#include <linux/module.h>
+#include <net/dsa.h>
+
+#include "mv88e6xxx.h"
+#include "phy.h"
+
+int mv88e6165_phy_read(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
+		       int addr, int reg, u16 *val)
+{
+	return mv88e6xxx_read(chip, addr, reg, val);
+}
+
+int mv88e6165_phy_write(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
+			int addr, int reg, u16 val)
+{
+	return mv88e6xxx_write(chip, addr, reg, val);
+}
+
+int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy, int reg, u16 *val)
+{
+	int addr = phy; /* PHY devices addresses start at 0x0 */
+	struct mii_bus *bus;
+
+	bus = mv88e6xxx_default_mdio_bus(chip);
+	if (!bus)
+		return -EOPNOTSUPP;
+
+	if (!chip->info->ops->phy_read)
+		return -EOPNOTSUPP;
+
+	return chip->info->ops->phy_read(chip, bus, addr, reg, val);
+}
+
+int mv88e6xxx_phy_write(struct mv88e6xxx_chip *chip, int phy, int reg, u16 val)
+{
+	int addr = phy; /* PHY devices addresses start at 0x0 */
+	struct mii_bus *bus;
+
+	bus = mv88e6xxx_default_mdio_bus(chip);
+	if (!bus)
+		return -EOPNOTSUPP;
+
+	if (!chip->info->ops->phy_write)
+		return -EOPNOTSUPP;
+
+	return chip->info->ops->phy_write(chip, bus, addr, reg, val);
+}
+
+int mv88e6xxx_phy_page_get(struct mv88e6xxx_chip *chip, int phy, u8 page)
+{
+	if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_PHY_PAGE))
+		return -EOPNOTSUPP;
+
+	return mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page);
+}
+
+void mv88e6xxx_phy_page_put(struct mv88e6xxx_chip *chip, int phy)
+{
+	int err;
+
+	/* Restore PHY page Copper 0x0 for access via the registered
+	 * MDIO bus
+	 */
+	err = mv88e6xxx_phy_write(chip, phy, PHY_PAGE, PHY_PAGE_COPPER);
+	if (unlikely(err)) {
+		dev_err(chip->dev,
+			"failed to restore PHY %d page Copper (%d)\n",
+			phy, err);
+	}
+}
+
+int mv88e6xxx_phy_page_read(struct mv88e6xxx_chip *chip, int phy,
+			    u8 page, int reg, u16 *val)
+{
+	int err;
+
+	/* There is no paging for registers 22 */
+	if (reg == PHY_PAGE)
+		return -EINVAL;
+
+	err = mv88e6xxx_phy_page_get(chip, phy, page);
+	if (!err) {
+		err = mv88e6xxx_phy_read(chip, phy, reg, val);
+		mv88e6xxx_phy_page_put(chip, phy);
+	}
+
+	return err;
+}
+
+int mv88e6xxx_phy_page_write(struct mv88e6xxx_chip *chip, int phy,
+			     u8 page, int reg, u16 val)
+{
+	int err;
+
+	/* There is no paging for registers 22 */
+	if (reg == PHY_PAGE)
+		return -EINVAL;
+
+	err = mv88e6xxx_phy_page_get(chip, phy, page);
+	if (!err) {
+		err = mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page);
+		mv88e6xxx_phy_page_put(chip, phy);
+	}
+
+	return err;
+}
+
+int mv88e6xxx_ppu_disable(struct mv88e6xxx_chip *chip)
+{
+	if (!chip->info->ops->ppu_disable)
+		return 0;
+
+	return chip->info->ops->ppu_disable(chip);
+}
+
+int mv88e6xxx_ppu_enable(struct mv88e6xxx_chip *chip)
+{
+	if (!chip->info->ops->ppu_enable)
+		return 0;
+
+	return chip->info->ops->ppu_enable(chip);
+}
+
+static void mv88e6xxx_ppu_reenable_work(struct work_struct *ugly)
+{
+	struct mv88e6xxx_chip *chip;
+
+	chip = container_of(ugly, struct mv88e6xxx_chip, ppu_work);
+
+	mutex_lock(&chip->reg_lock);
+
+	if (mutex_trylock(&chip->ppu_mutex)) {
+		if (mv88e6xxx_ppu_enable(chip) == 0)
+			chip->ppu_disabled = 0;
+		mutex_unlock(&chip->ppu_mutex);
+	}
+
+	mutex_unlock(&chip->reg_lock);
+}
+
+static void mv88e6xxx_ppu_reenable_timer(unsigned long _ps)
+{
+	struct mv88e6xxx_chip *chip = (void *)_ps;
+
+	schedule_work(&chip->ppu_work);
+}
+
+static int mv88e6xxx_ppu_access_get(struct mv88e6xxx_chip *chip)
+{
+	int ret;
+
+	mutex_lock(&chip->ppu_mutex);
+
+	/* If the PHY polling unit is enabled, disable it so that
+	 * we can access the PHY registers.  If it was already
+	 * disabled, cancel the timer that is going to re-enable
+	 * it.
+	 */
+	if (!chip->ppu_disabled) {
+		ret = mv88e6xxx_ppu_disable(chip);
+		if (ret < 0) {
+			mutex_unlock(&chip->ppu_mutex);
+			return ret;
+		}
+		chip->ppu_disabled = 1;
+	} else {
+		del_timer(&chip->ppu_timer);
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static void mv88e6xxx_ppu_access_put(struct mv88e6xxx_chip *chip)
+{
+	/* Schedule a timer to re-enable the PHY polling unit. */
+	mod_timer(&chip->ppu_timer, jiffies + msecs_to_jiffies(10));
+	mutex_unlock(&chip->ppu_mutex);
+}
+
+void mv88e6xxx_ppu_state_init(struct mv88e6xxx_chip *chip)
+{
+	mutex_init(&chip->ppu_mutex);
+	INIT_WORK(&chip->ppu_work, mv88e6xxx_ppu_reenable_work);
+	setup_timer(&chip->ppu_timer, mv88e6xxx_ppu_reenable_timer,
+		    (unsigned long)chip);
+}
+
+void mv88e6xxx_ppu_state_destroy(struct mv88e6xxx_chip *chip)
+{
+	del_timer_sync(&chip->ppu_timer);
+}
+
+int mv88e6xxx_phy_ppu_read(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
+			   int addr, int reg, u16 *val)
+{
+	int err;
+
+	err = mv88e6xxx_ppu_access_get(chip);
+	if (!err) {
+		err = mv88e6xxx_read(chip, addr, reg, val);
+		mv88e6xxx_ppu_access_put(chip);
+	}
+
+	return err;
+}
+
+int mv88e6xxx_phy_ppu_write(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
+			    int addr, int reg, u16 val)
+{
+	int err;
+
+	err = mv88e6xxx_ppu_access_get(chip);
+	if (!err) {
+		err = mv88e6xxx_write(chip, addr, reg, val);
+		mv88e6xxx_ppu_access_put(chip);
+	}
+
+	return err;
+}
+
diff --git a/drivers/net/dsa/mv88e6xxx/phy.h b/drivers/net/dsa/mv88e6xxx/phy.h
new file mode 100644
index 000000000000..b90949cd3ed1
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/phy.h
@@ -0,0 +1,40 @@
+/*
+ * Marvell 88E6xxx PHY access
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2017 Andrew Lunn <andrew@lunn.ch>
+ *
+ * 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.
+ */
+
+#ifndef _MV88E6XXX_PHY_H
+#define _MV88E6XXX_PHY_H
+
+int mv88e6165_phy_read(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
+		       int addr, int reg, u16 *val);
+int mv88e6165_phy_write(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
+			int addr, int reg, u16 val);
+int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
+		       int reg, u16 *val);
+int mv88e6xxx_phy_write(struct mv88e6xxx_chip *chip, int phy,
+			int reg, u16 val);
+int mv88e6xxx_phy_page_get(struct mv88e6xxx_chip *chip, int phy, u8 page);
+void mv88e6xxx_phy_page_put(struct mv88e6xxx_chip *chip, int phy);
+int mv88e6xxx_phy_page_read(struct mv88e6xxx_chip *chip, int phy,
+			    u8 page, int reg, u16 *val);
+int mv88e6xxx_phy_page_write(struct mv88e6xxx_chip *chip, int phy,
+			     u8 page, int reg, u16 val);
+int mv88e6xxx_ppu_disable(struct mv88e6xxx_chip *chip);
+int mv88e6xxx_ppu_enable(struct mv88e6xxx_chip *chip);
+void mv88e6xxx_ppu_state_init(struct mv88e6xxx_chip *chip);
+void mv88e6xxx_ppu_state_destroy(struct mv88e6xxx_chip *chip);
+int mv88e6xxx_phy_ppu_read(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
+			   int addr, int reg, u16 *val);
+int mv88e6xxx_phy_ppu_write(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
+			    int addr, int reg, u16 val);
+
+#endif /*_MV88E6XXX_PHY_H */
-- 
2.11.0

^ permalink raw reply related

* [PATCH v2 net-next 1/2] Documentation: sysfs-class-net-statistics: Clarify rx_bytes and tx_bytes
From: Andrew Lunn @ 2017-05-23 21:10 UTC (permalink / raw)
  To: David Miller; +Cc: Florian Fainelli, hayeswang, netdev, Andrew Lunn
In-Reply-To: <1495573858-32346-1-git-send-email-andrew@lunn.ch>

Document what is expected for the rx_bytes and tx_bytes statistics in
/sys/class/net/<device>/statistics. The FCS should be included in the
statistics. However, since this has been unclear until now, it is
expected a number of drivers don't. But maybe with time they will.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/ABI/testing/sysfs-class-net-statistics | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-net-statistics b/Documentation/ABI/testing/sysfs-class-net-statistics
index 397118de7b5e..a487cbb79458 100644
--- a/Documentation/ABI/testing/sysfs-class-net-statistics
+++ b/Documentation/ABI/testing/sysfs-class-net-statistics
@@ -21,7 +21,8 @@ Contact:	netdev@vger.kernel.org
 Description:
 		Indicates the number of bytes received by this network device.
 		See the network driver for the exact meaning of when this
-		value is incremented.
+		value is incremented. However, for an Ethernet frame, it should
+		include the Ethernet header, data, and frame check sum.
 
 What:		/sys/class/<iface>/statistics/rx_compressed
 Date:		April 2005
@@ -125,7 +126,8 @@ Description:
 		device. See the network driver for the exact meaning of this
 		value, in particular whether this accounts for all successfully
 		transmitted packets or all packets that have been queued for
-		transmission.
+		transmission. For an Ethernet frame, it should include the
+		Ethernet header, data, and frame check sum.
 
 What:		/sys/class/<iface>/statistics/tx_carrier_errors
 Date:		April 2005
-- 
2.11.0

^ permalink raw reply related

* [PATCH v2 net-next 2/2] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS
From: Andrew Lunn @ 2017-05-23 21:10 UTC (permalink / raw)
  To: David Miller; +Cc: Florian Fainelli, hayeswang, netdev, Andrew Lunn
In-Reply-To: <1495573858-32346-1-git-send-email-andrew@lunn.ch>

The statistics counters rx_bytes and tx_bytes don't include the
Ethernet Frame Check Sequence. Fix the rx_bytes/tx_bytes counters to
include the FCS, and include the received FCS in the skb.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/usb/r8152.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ddc62cb69be8..4081a2cd8b1b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1228,7 +1228,7 @@ static void write_bulk_callback(struct urb *urb)
 		stats->tx_errors += agg->skb_num;
 	} else {
 		stats->tx_packets += agg->skb_num;
-		stats->tx_bytes += agg->skb_len;
+		stats->tx_bytes += agg->skb_len + CRC_SIZE;
 	}
 
 	spin_lock(&tp->tx_lock);
@@ -1826,7 +1826,6 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			if (urb->actual_length < len_used)
 				break;
 
-			pkt_len -= CRC_SIZE;
 			rx_data += sizeof(struct rx_desc);
 
 			skb = napi_alloc_skb(napi, pkt_len);
@@ -1850,7 +1849,7 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			}
 
 find_next_rx:
-			rx_data = rx_agg_align(rx_data + pkt_len + CRC_SIZE);
+			rx_data = rx_agg_align(rx_data + pkt_len);
 			rx_desc = (struct rx_desc *)rx_data;
 			len_used = (int)(rx_data - (u8 *)agg->head);
 			len_used += sizeof(struct rx_desc);
-- 
2.11.0

^ permalink raw reply related

* [PATCH v2 net-next 0/2] Start making stats consistent
From: Andrew Lunn @ 2017-05-23 21:10 UTC (permalink / raw)
  To: David Miller; +Cc: Florian Fainelli, hayeswang, netdev, Andrew Lunn

It does not appear that the sysfs class net stats have been clearly
defined, resulting in different network interfaces implementing them
slightly different. Define that the rx_bytes and tx_bytes counters
should include the Ethernet header, data and FEC. Modify the r8152 USB
Ethernet driver to include the FEC in its counters.

Andrew Lunn (2):
  Documentation: sysfs-class-net-statistics: Clarify rx_bytes and
    tx_bytes
  net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS

 Documentation/ABI/testing/sysfs-class-net-statistics | 6 ++++--
 drivers/net/usb/r8152.c                              | 5 ++---
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.11.0

^ permalink raw reply

* Re: bond link state mismatch, rtnl_trylock() vs rtnl_lock()
From: Nithin Sujir @ 2017-05-23 21:10 UTC (permalink / raw)
  To: Jay Vosburgh; +Cc: netdev
In-Reply-To: <1638.1495570412@famine>



On 5/23/2017 1:13 PM, Jay Vosburgh wrote:
> Nithin Sujir <nsujir@tintri.com> wrote:
>
>> Hi,
>> We're encountering a problem in 4.4 LTS where, rarely, the bond link state
>> is not updated when the slave link changes.
>>
>> I've traced the issue to the arp monitor unable to get the rtnl lock. The
>> sequence resulting in failure is as below.
>>
>> bond_loadbalance_arp_mon() periodically called, if slave link is _down_,
>> it checks if the slave is sending/receiving packets. If it is, it sets
>> flags to be processed later down the function for bond link
>> update. However, it sets the slave->link right away.
>>
>>                 if (slave->link != BOND_LINK_UP) {
>>                         if (bond_time_in_interval(bond, trans_start, 1) &&
>>                             bond_time_in_interval(bond, slave->last_rx,
>> 1)) {
>>
>>                                 slave->link  = BOND_LINK_UP;
>>                                 slave_state_changed = 1;
>>
>>
>> Later down the function, it tries to get the rtnl_lock. If it doesn't get
>> it, it rearms and returns.
>>
>>         if (do_failover || slave_state_changed) {
>>                 if (!rtnl_trylock())
>>                         goto re_arm; <-- returns here
>>
>>                 if (slave_state_changed) {
>>                         bond_slave_state_change(bond);
>>
>> This is the problem. The next time this function is called, the
>> slave->link is already marked UP. And we will never update the bond link
>> state to UP.
> 	This looks like an ARP monitor version of
>
> commit de77ecd4ef02ca783f7762e04e92b3d0964be66b
> Author: Mahesh Bandewar <maheshb@google.com>
> Date:   Mon Mar 27 11:37:33 2017 -0700
>
>      bonding: improve link-status update in mii-monitoring
>
> 	and probably needs a similar fix (possibly for both the
> loadbalance and active-backup ARP monitor cases).
Thanks for the explanation and the pointer to this patch. I will take a 
look.

Thanks, Jay!

Nithin.

>> Changing the rtnl_trylock() -> rtnl_lock() _does_ fix the issue.
>>
>> Is this the right way to fix it? If it is, I can submit this formally.
> 	It's not the right way, unfortunately.
>
> 	The reason for the rtnl_trylock is that there's a possible race
> against bond_close() -> bond_work_cancel_all() trying to cancel the
> arp_work workqueue item while it's running.  bond_close is called with
> RTNL held, so if it has RTNL and is waiting for the work function to
> complete, an rtnl_lock call here will deadlock.  Some of the trylock
> calls in bonding are commented to this effect, but not this one.
>
> 	-J
>
>> What are the guidelines around using rtnl_lock() vs rtnl_trylock()? Some
>> places are using rtnl_lock() and other rtnl_trylock(). Sorry, I couldn't
>> find much via a google search or in Documentation/.
>>
>> Thanks,
>> Nithin.
>>
>> --------------------
>>
>> diff --git a/drivers/net/bonding/bond_main.c
>> b/drivers/net/bonding/bond_main.c
>> index 5dca77e..1f60503 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2614,8 +2614,7 @@ static void bond_loadbalance_arp_mon(struct
>> work_struct *work)
>>         rcu_read_unlock();
>>
>>         if (do_failover || slave_state_changed) {
>> -               if (!rtnl_trylock())
>> -                       goto re_arm;
>> +               rtnl_lock();
>>
>>                 if (slave_state_changed) {
>>                         bond_slave_state_change(bond);
>>
>>
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com

^ permalink raw reply

* [PATCH net-next 3/4] net: phy: marvell: helper to get and set page
From: Andrew Lunn @ 2017-05-23 21:24 UTC (permalink / raw)
  To: David Miller; +Cc: Florian Fainelli, netdev, Andrew Lunn
In-Reply-To: <1495574697-501-1-git-send-email-andrew@lunn.ch>

There is a common pattern of first reading the currently selected page
and then changing to another page. Add a helper to do this.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/marvell.c | 75 ++++++++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 44 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 44ea1dd89ce5..ab00609af2be 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -199,6 +199,19 @@ static int marvell_set_page(struct phy_device *phydev, int page)
 	return phy_write(phydev, MII_MARVELL_PHY_PAGE, page);
 }
 
+static int marvell_get_set_page(struct phy_device *phydev, int page)
+{
+	int oldpage = marvell_get_page(phydev);
+
+	if (oldpage < 0)
+		return oldpage;
+
+	if (page != oldpage)
+		return marvell_set_page(phydev, page);
+
+	return 0;
+}
+
 static int marvell_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -452,11 +465,9 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
 {
 	int err, oldpage, mscr;
 
-	oldpage = marvell_get_page(phydev);
-
-	err = marvell_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
-	if (err < 0)
-		return err;
+	oldpage = marvell_get_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
+	if (oldpage < 0)
+		return oldpage;
 
 	if (phy_interface_is_rgmii(phydev)) {
 		mscr = phy_read(phydev, MII_88E1121_PHY_MSCR_REG) &
@@ -493,11 +504,9 @@ static int m88e1318_config_aneg(struct phy_device *phydev)
 {
 	int err, oldpage, mscr;
 
-	oldpage = marvell_get_page(phydev);
-
-	err = marvell_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
-	if (err < 0)
-		return err;
+	oldpage = marvell_get_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
+	if (oldpage < 0)
+		return oldpage;
 
 	mscr = phy_read(phydev, MII_88E1318S_PHY_MSCR1_REG);
 	mscr |= MII_88E1318S_PHY_MSCR1_PAD_ODD;
@@ -843,11 +852,9 @@ static int m88e1121_config_init(struct phy_device *phydev)
 {
 	int err, oldpage;
 
-	oldpage = marvell_get_page(phydev);
-
-	err = marvell_set_page(phydev, MII_88E1121_PHY_LED_PAGE);
-	if (err < 0)
-		return err;
+	oldpage = marvell_get_set_page(phydev, MII_88E1121_PHY_LED_PAGE);
+	if (oldpage < 0)
+		return oldpage;
 
 	/* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
 	err = phy_write(phydev, MII_88E1121_PHY_LED_CTRL,
@@ -1516,12 +1523,11 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
 {
 	struct marvell_hw_stat stat = marvell_hw_stats[i];
 	struct marvell_priv *priv = phydev->priv;
-	int err, oldpage, val;
+	int oldpage, val;
 	u64 ret;
 
-	oldpage = marvell_get_page(phydev);
-	err = marvell_set_page(phydev, stat.page);
-	if (err < 0)
+	oldpage = marvell_get_set_page(phydev, stat.page);
+	if (oldpage < 0)
 		return UINT64_MAX;
 
 	val = phy_read(phydev, stat.reg);
@@ -1558,16 +1564,12 @@ static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
 
 	mutex_lock(&phydev->lock);
 
-	oldpage = marvell_get_page(phydev);
+	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
 	}
 
-	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
-	if (ret < 0)
-		goto error;
-
 	/* Enable temperature sensor */
 	ret = phy_read(phydev, MII_88E1121_MISC_TEST);
 	if (ret < 0)
@@ -1680,16 +1682,12 @@ static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
 
 	mutex_lock(&phydev->lock);
 
-	oldpage = marvell_get_page(phydev);
+	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
 	}
 
-	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
-	if (ret < 0)
-		goto error;
-
 	ret = phy_read(phydev, MII_88E1510_TEMP_SENSOR);
 	if (ret < 0)
 		goto error;
@@ -1711,16 +1709,13 @@ int m88e1510_get_temp_critical(struct phy_device *phydev, long *temp)
 	*temp = 0;
 
 	mutex_lock(&phydev->lock);
-	oldpage = marvell_get_page(phydev);
+
+	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
 	}
 
-	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
-	if (ret < 0)
-		goto error;
-
 	ret = phy_read(phydev, MII_88E1121_MISC_TEST);
 	if (ret < 0)
 		goto error;
@@ -1744,16 +1739,12 @@ int m88e1510_set_temp_critical(struct phy_device *phydev, long temp)
 
 	mutex_lock(&phydev->lock);
 
-	oldpage = marvell_get_page(phydev);
+	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
 	}
 
-	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
-	if (ret < 0)
-		goto error;
-
 	ret = phy_read(phydev, MII_88E1121_MISC_TEST);
 	if (ret < 0)
 		goto error;
@@ -1780,16 +1771,12 @@ int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
 
 	mutex_lock(&phydev->lock);
 
-	oldpage = marvell_get_page(phydev);
+	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
 	}
 
-	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
-	if (ret < 0)
-		goto error;
-
 	ret = phy_read(phydev, MII_88E1121_MISC_TEST);
 	if (ret < 0)
 		goto error;
-- 
2.11.0

^ permalink raw reply related

* [PATCH net-next 4/4] net: phy: marvell: Uniform page names
From: Andrew Lunn @ 2017-05-23 21:24 UTC (permalink / raw)
  To: David Miller; +Cc: Florian Fainelli, netdev, Andrew Lunn
In-Reply-To: <1495574697-501-1-git-send-email-andrew@lunn.ch>

Bring all the page names together, remove the repeats, and make them
uniform.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/marvell.c | 94 +++++++++++++++++++++++------------------------
 1 file changed, 46 insertions(+), 48 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index ab00609af2be..0569614ec236 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -41,6 +41,12 @@
 #include <linux/uaccess.h>
 
 #define MII_MARVELL_PHY_PAGE		22
+#define MII_MARVELL_COPPER_PAGE		0x00
+#define MII_MARVELL_FIBER_PAGE		0x01
+#define MII_MARVELL_MSCR_PAGE		0x02
+#define MII_MARVELL_LED_PAGE		0x03
+#define MII_MARVELL_MISC_TEST_PAGE	0x06
+#define MII_MARVELL_WOL_PAGE		0x11
 
 #define MII_M1011_IEVENT		0x13
 #define MII_M1011_IEVENT_CLEAR		0x0000
@@ -82,16 +88,11 @@
 #define MII_M1111_HWCFG_FIBER_COPPER_AUTO	0x8000
 #define MII_M1111_HWCFG_FIBER_COPPER_RES	0x2000
 
-#define MII_M1111_COPPER		0
-#define MII_M1111_FIBER			1
-
-#define MII_88E1121_PHY_MSCR_PAGE	2
 #define MII_88E1121_PHY_MSCR_REG	21
 #define MII_88E1121_PHY_MSCR_RX_DELAY	BIT(5)
 #define MII_88E1121_PHY_MSCR_TX_DELAY	BIT(4)
 #define MII_88E1121_PHY_MSCR_DELAY_MASK	(~(0x3 << 4))
 
-#define MII_88E1121_MISC_TEST_PAGE			6
 #define MII_88E1121_MISC_TEST				0x1a
 #define MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK	0x1f00
 #define MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT	8
@@ -112,7 +113,6 @@
 #define MII_88E1318S_PHY_CSIER_WOL_EIE                      BIT(7)
 
 /* LED Timer Control Register */
-#define MII_88E1318S_PHY_LED_PAGE                           0x03
 #define MII_88E1318S_PHY_LED_TCR                            0x12
 #define MII_88E1318S_PHY_LED_TCR_FORCE_INT                  BIT(15)
 #define MII_88E1318S_PHY_LED_TCR_INTn_ENABLE                BIT(7)
@@ -123,13 +123,11 @@
 #define MII_88E1318S_PHY_MAGIC_PACKET_WORD1                 0x18
 #define MII_88E1318S_PHY_MAGIC_PACKET_WORD0                 0x19
 
-#define MII_88E1318S_PHY_WOL_PAGE                           0x11
 #define MII_88E1318S_PHY_WOL_CTRL                           0x10
 #define MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS          BIT(12)
 #define MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE BIT(14)
 
 #define MII_88E1121_PHY_LED_CTRL	16
-#define MII_88E1121_PHY_LED_PAGE	3
 #define MII_88E1121_PHY_LED_DEF		0x0030
 
 #define MII_M1011_PHY_STATUS		0x11
@@ -465,7 +463,7 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
 {
 	int err, oldpage, mscr;
 
-	oldpage = marvell_get_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
+	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MSCR_PAGE);
 	if (oldpage < 0)
 		return oldpage;
 
@@ -504,7 +502,7 @@ static int m88e1318_config_aneg(struct phy_device *phydev)
 {
 	int err, oldpage, mscr;
 
-	oldpage = marvell_get_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
+	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MSCR_PAGE);
 	if (oldpage < 0)
 		return oldpage;
 
@@ -615,7 +613,7 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 {
 	int err;
 
-	err = marvell_set_page(phydev, MII_M1111_COPPER);
+	err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 	if (err < 0)
 		goto error;
 
@@ -625,7 +623,7 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 		goto error;
 
 	/* Then the fiber link */
-	err = marvell_set_page(phydev, MII_M1111_FIBER);
+	err = marvell_set_page(phydev, MII_MARVELL_FIBER_PAGE);
 	if (err < 0)
 		goto error;
 
@@ -633,10 +631,10 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 	if (err < 0)
 		goto error;
 
-	return marvell_set_page(phydev, MII_M1111_COPPER);
+	return marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 
 error:
-	marvell_set_page(phydev, MII_M1111_COPPER);
+	marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 	return err;
 }
 
@@ -659,7 +657,7 @@ static int m88e1116r_config_init(struct phy_device *phydev)
 
 	mdelay(500);
 
-	err = marvell_set_page(phydev, MII_M1111_COPPER);
+	err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 	if (err < 0)
 		return err;
 
@@ -671,7 +669,7 @@ static int m88e1116r_config_init(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
-	err = marvell_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
+	err = marvell_set_page(phydev, MII_MARVELL_MSCR_PAGE);
 	if (err < 0)
 		return err;
 	temp = phy_read(phydev, MII_M1116R_CONTROL_REG_MAC);
@@ -680,7 +678,7 @@ static int m88e1116r_config_init(struct phy_device *phydev)
 	err = phy_write(phydev, MII_M1116R_CONTROL_REG_MAC, temp);
 	if (err < 0)
 		return err;
-	err = marvell_set_page(phydev, MII_M1111_COPPER);
+	err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 	if (err < 0)
 		return err;
 
@@ -769,7 +767,7 @@ static int m88e1111_config_init_sgmii(struct phy_device *phydev)
 		return err;
 
 	/* make sure copper is selected */
-	return marvell_set_page(phydev, MII_M1111_COPPER);
+	return marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 }
 
 static int m88e1111_config_init_rtbi(struct phy_device *phydev)
@@ -852,7 +850,7 @@ static int m88e1121_config_init(struct phy_device *phydev)
 {
 	int err, oldpage;
 
-	oldpage = marvell_get_set_page(phydev, MII_88E1121_PHY_LED_PAGE);
+	oldpage = marvell_get_set_page(phydev, MII_MARVELL_LED_PAGE);
 	if (oldpage < 0)
 		return oldpage;
 
@@ -895,7 +893,7 @@ static int m88e1510_config_init(struct phy_device *phydev)
 			return err;
 
 		/* Reset page selection */
-		err = marvell_set_page(phydev, MII_M1111_COPPER);
+		err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 		if (err < 0)
 			return err;
 	}
@@ -925,7 +923,7 @@ static int m88e1118_config_init(struct phy_device *phydev)
 	int err;
 
 	/* Change address */
-	err = marvell_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
+	err = marvell_set_page(phydev, MII_MARVELL_MSCR_PAGE);
 	if (err < 0)
 		return err;
 
@@ -935,7 +933,7 @@ static int m88e1118_config_init(struct phy_device *phydev)
 		return err;
 
 	/* Change address */
-	err = marvell_set_page(phydev, MII_88E1318S_PHY_LED_PAGE);
+	err = marvell_set_page(phydev, MII_MARVELL_LED_PAGE);
 	if (err < 0)
 		return err;
 
@@ -952,7 +950,7 @@ static int m88e1118_config_init(struct phy_device *phydev)
 		return err;
 
 	/* Reset address */
-	err = marvell_set_page(phydev, MII_M1111_COPPER);
+	err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 	if (err < 0)
 		return err;
 
@@ -964,7 +962,7 @@ static int m88e1149_config_init(struct phy_device *phydev)
 	int err;
 
 	/* Change address */
-	err = marvell_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
+	err = marvell_set_page(phydev, MII_MARVELL_MSCR_PAGE);
 	if (err < 0)
 		return err;
 
@@ -978,7 +976,7 @@ static int m88e1149_config_init(struct phy_device *phydev)
 		return err;
 
 	/* Reset address */
-	err = marvell_set_page(phydev, MII_M1111_COPPER);
+	err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 	if (err < 0)
 		return err;
 
@@ -1248,7 +1246,7 @@ static int marvell_read_status_page(struct phy_device *phydev, int page)
 	/* Detect and update the link, but return if there
 	 * was an error
 	 */
-	if (page == MII_M1111_FIBER)
+	if (page == MII_MARVELL_FIBER_PAGE)
 		fiber = 1;
 	else
 		fiber = 0;
@@ -1281,11 +1279,11 @@ static int marvell_read_status(struct phy_device *phydev)
 	/* Check the fiber mode first */
 	if (phydev->supported & SUPPORTED_FIBRE &&
 	    phydev->interface != PHY_INTERFACE_MODE_SGMII) {
-		err = marvell_set_page(phydev, MII_M1111_FIBER);
+		err = marvell_set_page(phydev, MII_MARVELL_FIBER_PAGE);
 		if (err < 0)
 			goto error;
 
-		err = marvell_read_status_page(phydev, MII_M1111_FIBER);
+		err = marvell_read_status_page(phydev, MII_MARVELL_FIBER_PAGE);
 		if (err < 0)
 			goto error;
 
@@ -1300,15 +1298,15 @@ static int marvell_read_status(struct phy_device *phydev)
 			return 0;
 
 		/* If fiber link is down, check and save copper mode state */
-		err = marvell_set_page(phydev, MII_M1111_COPPER);
+		err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 		if (err < 0)
 			goto error;
 	}
 
-	return marvell_read_status_page(phydev, MII_M1111_COPPER);
+	return marvell_read_status_page(phydev, MII_MARVELL_COPPER_PAGE);
 
 error:
-	marvell_set_page(phydev, MII_M1111_COPPER);
+	marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 	return err;
 }
 
@@ -1323,7 +1321,7 @@ static int marvell_suspend(struct phy_device *phydev)
 
 	/* Suspend the fiber mode first */
 	if (!(phydev->supported & SUPPORTED_FIBRE)) {
-		err = marvell_set_page(phydev, MII_M1111_FIBER);
+		err = marvell_set_page(phydev, MII_MARVELL_FIBER_PAGE);
 		if (err < 0)
 			goto error;
 
@@ -1333,7 +1331,7 @@ static int marvell_suspend(struct phy_device *phydev)
 			goto error;
 
 		/* Then, the copper link */
-		err = marvell_set_page(phydev, MII_M1111_COPPER);
+		err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 		if (err < 0)
 			goto error;
 	}
@@ -1342,7 +1340,7 @@ static int marvell_suspend(struct phy_device *phydev)
 	return genphy_suspend(phydev);
 
 error:
-	marvell_set_page(phydev, MII_M1111_COPPER);
+	marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 	return err;
 }
 
@@ -1357,7 +1355,7 @@ static int marvell_resume(struct phy_device *phydev)
 
 	/* Resume the fiber mode first */
 	if (!(phydev->supported & SUPPORTED_FIBRE)) {
-		err = marvell_set_page(phydev, MII_M1111_FIBER);
+		err = marvell_set_page(phydev, MII_MARVELL_FIBER_PAGE);
 		if (err < 0)
 			goto error;
 
@@ -1367,7 +1365,7 @@ static int marvell_resume(struct phy_device *phydev)
 			goto error;
 
 		/* Then, the copper link */
-		err = marvell_set_page(phydev, MII_M1111_COPPER);
+		err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 		if (err < 0)
 			goto error;
 	}
@@ -1376,7 +1374,7 @@ static int marvell_resume(struct phy_device *phydev)
 	return genphy_resume(phydev);
 
 error:
-	marvell_set_page(phydev, MII_M1111_COPPER);
+	marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 	return err;
 }
 
@@ -1405,14 +1403,14 @@ static void m88e1318_get_wol(struct phy_device *phydev,
 	wol->supported = WAKE_MAGIC;
 	wol->wolopts = 0;
 
-	if (marvell_set_page(phydev, MII_88E1318S_PHY_WOL_PAGE) < 0)
+	if (marvell_set_page(phydev, MII_MARVELL_WOL_PAGE) < 0)
 		return;
 
 	if (phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL) &
 	    MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE)
 		wol->wolopts |= WAKE_MAGIC;
 
-	if (marvell_set_page(phydev, MII_M1111_COPPER) < 0)
+	if (marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE) < 0)
 		return;
 }
 
@@ -1425,7 +1423,7 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 
 	if (wol->wolopts & WAKE_MAGIC) {
 		/* Explicitly switch to page 0x00, just to be sure */
-		err = marvell_set_page(phydev, MII_M1111_COPPER);
+		err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
 		if (err < 0)
 			return err;
 
@@ -1436,7 +1434,7 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 		if (err < 0)
 			return err;
 
-		err = marvell_set_page(phydev, MII_88E1318S_PHY_LED_PAGE);
+		err = marvell_set_page(phydev, MII_MARVELL_LED_PAGE);
 		if (err < 0)
 			return err;
 
@@ -1449,7 +1447,7 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 		if (err < 0)
 			return err;
 
-		err = marvell_set_page(phydev, MII_88E1318S_PHY_WOL_PAGE);
+		err = marvell_set_page(phydev, MII_MARVELL_WOL_PAGE);
 		if (err < 0)
 			return err;
 
@@ -1478,7 +1476,7 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 		if (err < 0)
 			return err;
 	} else {
-		err = marvell_set_page(phydev, MII_88E1318S_PHY_WOL_PAGE);
+		err = marvell_set_page(phydev, MII_MARVELL_WOL_PAGE);
 		if (err < 0)
 			return err;
 
@@ -1564,7 +1562,7 @@ static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
 
 	mutex_lock(&phydev->lock);
 
-	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
+	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
@@ -1682,7 +1680,7 @@ static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
 
 	mutex_lock(&phydev->lock);
 
-	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
+	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
@@ -1710,7 +1708,7 @@ int m88e1510_get_temp_critical(struct phy_device *phydev, long *temp)
 
 	mutex_lock(&phydev->lock);
 
-	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
+	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
@@ -1739,7 +1737,7 @@ int m88e1510_set_temp_critical(struct phy_device *phydev, long temp)
 
 	mutex_lock(&phydev->lock);
 
-	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
+	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
@@ -1771,7 +1769,7 @@ int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
 
 	mutex_lock(&phydev->lock);
 
-	oldpage = marvell_get_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
+	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
 	if (oldpage < 0) {
 		mutex_unlock(&phydev->lock);
 		return oldpage;
-- 
2.11.0

^ permalink raw reply related

* [PATCH net-next 2/4] net: phy: marvell: More hidden page changes refactored
From: Andrew Lunn @ 2017-05-23 21:24 UTC (permalink / raw)
  To: David Miller; +Cc: Florian Fainelli, netdev, Andrew Lunn
In-Reply-To: <1495574697-501-1-git-send-email-andrew@lunn.ch>

EXT_ADDR_PAGE is the same meaning as MII_MARVELL_PHY_PAGE, i.e. change
page. Replace it will calls to the helpers.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/marvell.c | 62 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb067026353a..44ea1dd89ce5 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -54,7 +54,6 @@
 #define MII_M1011_PHY_SCR_MDI_X		0x0020
 #define MII_M1011_PHY_SCR_AUTO_CROSS	0x0060
 
-#define MII_M1145_PHY_EXT_ADDR_PAGE	0x16
 #define MII_M1145_PHY_EXT_SR		0x1b
 #define MII_M1145_PHY_EXT_CR		0x14
 #define MII_M1145_RGMII_RX_DELAY	0x0080
@@ -92,6 +91,7 @@
 #define MII_88E1121_PHY_MSCR_TX_DELAY	BIT(4)
 #define MII_88E1121_PHY_MSCR_DELAY_MASK	(~(0x3 << 4))
 
+#define MII_88E1121_MISC_TEST_PAGE			6
 #define MII_88E1121_MISC_TEST				0x1a
 #define MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK	0x1f00
 #define MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT	8
@@ -760,11 +760,7 @@ static int m88e1111_config_init_sgmii(struct phy_device *phydev)
 		return err;
 
 	/* make sure copper is selected */
-	err = phy_read(phydev, MII_M1145_PHY_EXT_ADDR_PAGE);
-	if (err < 0)
-		return err;
-
-	return phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, err & (~0xff));
+	return marvell_set_page(phydev, MII_M1111_COPPER);
 }
 
 static int m88e1111_config_init_rtbi(struct phy_device *phydev)
@@ -1556,12 +1552,19 @@ static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
 {
 	int ret;
 	int val;
+	int oldpage;
 
 	*temp = 0;
 
 	mutex_lock(&phydev->lock);
 
-	ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+	oldpage = marvell_get_page(phydev);
+	if (oldpage < 0) {
+		mutex_unlock(&phydev->lock);
+		return oldpage;
+	}
+
+	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (ret < 0)
 		goto error;
 
@@ -1593,7 +1596,7 @@ static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
 	*temp = ((val & MII_88E1121_MISC_TEST_TEMP_MASK) - 5) * 5000;
 
 error:
-	phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+	marvell_set_page(phydev, oldpage);
 	mutex_unlock(&phydev->lock);
 
 	return ret;
@@ -1671,12 +1674,19 @@ static const struct hwmon_chip_info m88e1121_hwmon_chip_info = {
 static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
 {
 	int ret;
+	int oldpage;
 
 	*temp = 0;
 
 	mutex_lock(&phydev->lock);
 
-	ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+	oldpage = marvell_get_page(phydev);
+	if (oldpage < 0) {
+		mutex_unlock(&phydev->lock);
+		return oldpage;
+	}
+
+	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (ret < 0)
 		goto error;
 
@@ -1687,7 +1697,7 @@ static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
 	*temp = ((ret & MII_88E1510_TEMP_SENSOR_MASK) - 25) * 1000;
 
 error:
-	phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+	marvell_set_page(phydev, oldpage);
 	mutex_unlock(&phydev->lock);
 
 	return ret;
@@ -1696,12 +1706,18 @@ static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
 int m88e1510_get_temp_critical(struct phy_device *phydev, long *temp)
 {
 	int ret;
+	int oldpage;
 
 	*temp = 0;
 
 	mutex_lock(&phydev->lock);
+	oldpage = marvell_get_page(phydev);
+	if (oldpage < 0) {
+		mutex_unlock(&phydev->lock);
+		return oldpage;
+	}
 
-	ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (ret < 0)
 		goto error;
 
@@ -1715,7 +1731,7 @@ int m88e1510_get_temp_critical(struct phy_device *phydev, long *temp)
 	*temp *= 1000;
 
 error:
-	phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+	marvell_set_page(phydev, oldpage);
 	mutex_unlock(&phydev->lock);
 
 	return ret;
@@ -1724,10 +1740,17 @@ int m88e1510_get_temp_critical(struct phy_device *phydev, long *temp)
 int m88e1510_set_temp_critical(struct phy_device *phydev, long temp)
 {
 	int ret;
+	int oldpage;
 
 	mutex_lock(&phydev->lock);
 
-	ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+	oldpage = marvell_get_page(phydev);
+	if (oldpage < 0) {
+		mutex_unlock(&phydev->lock);
+		return oldpage;
+	}
+
+	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (ret < 0)
 		goto error;
 
@@ -1742,7 +1765,7 @@ int m88e1510_set_temp_critical(struct phy_device *phydev, long temp)
 			(temp << MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT));
 
 error:
-	phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+	marvell_set_page(phydev, oldpage);
 	mutex_unlock(&phydev->lock);
 
 	return ret;
@@ -1751,12 +1774,19 @@ int m88e1510_set_temp_critical(struct phy_device *phydev, long temp)
 int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
 {
 	int ret;
+	int oldpage;
 
 	*alarm = false;
 
 	mutex_lock(&phydev->lock);
 
-	ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+	oldpage = marvell_get_page(phydev);
+	if (oldpage < 0) {
+		mutex_unlock(&phydev->lock);
+		return oldpage;
+	}
+
+	ret = marvell_set_page(phydev, MII_88E1121_MISC_TEST_PAGE);
 	if (ret < 0)
 		goto error;
 
@@ -1766,7 +1796,7 @@ int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
 	*alarm = !!(ret & MII_88E1510_MISC_TEST_TEMP_IRQ);
 
 error:
-	phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+	marvell_set_page(phydev, oldpage);
 	mutex_unlock(&phydev->lock);
 
 	return ret;
-- 
2.11.0

^ permalink raw reply related

* [PATCH net-next 1/4] net: phy: marvell: #defines for copper and fibre pages
From: Andrew Lunn @ 2017-05-23 21:24 UTC (permalink / raw)
  To: David Miller; +Cc: Florian Fainelli, netdev, Andrew Lunn
In-Reply-To: <1495574697-501-1-git-send-email-andrew@lunn.ch>

Replace magic numbers for PHY pages with symbolic names.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/marvell.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 88cd97b44ba6..bb067026353a 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -650,7 +650,7 @@ static int m88e1116r_config_init(struct phy_device *phydev)
 
 	mdelay(500);
 
-	err = marvell_set_page(phydev, 0);
+	err = marvell_set_page(phydev, MII_M1111_COPPER);
 	if (err < 0)
 		return err;
 
@@ -662,7 +662,7 @@ static int m88e1116r_config_init(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
-	err = marvell_set_page(phydev, 2);
+	err = marvell_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
 	if (err < 0)
 		return err;
 	temp = phy_read(phydev, MII_M1116R_CONTROL_REG_MAC);
@@ -671,7 +671,7 @@ static int m88e1116r_config_init(struct phy_device *phydev)
 	err = phy_write(phydev, MII_M1116R_CONTROL_REG_MAC, temp);
 	if (err < 0)
 		return err;
-	err = marvell_set_page(phydev, 0);
+	err = marvell_set_page(phydev, MII_M1111_COPPER);
 	if (err < 0)
 		return err;
 
@@ -892,7 +892,7 @@ static int m88e1510_config_init(struct phy_device *phydev)
 			return err;
 
 		/* Reset page selection */
-		err = marvell_set_page(phydev, 0);
+		err = marvell_set_page(phydev, MII_M1111_COPPER);
 		if (err < 0)
 			return err;
 	}
@@ -922,7 +922,7 @@ static int m88e1118_config_init(struct phy_device *phydev)
 	int err;
 
 	/* Change address */
-	err = marvell_set_page(phydev, 2);
+	err = marvell_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
 	if (err < 0)
 		return err;
 
@@ -932,7 +932,7 @@ static int m88e1118_config_init(struct phy_device *phydev)
 		return err;
 
 	/* Change address */
-	err = marvell_set_page(phydev, 3);
+	err = marvell_set_page(phydev, MII_88E1318S_PHY_LED_PAGE);
 	if (err < 0)
 		return err;
 
@@ -949,7 +949,7 @@ static int m88e1118_config_init(struct phy_device *phydev)
 		return err;
 
 	/* Reset address */
-	err = marvell_set_page(phydev, 0);
+	err = marvell_set_page(phydev, MII_M1111_COPPER);
 	if (err < 0)
 		return err;
 
@@ -961,7 +961,7 @@ static int m88e1149_config_init(struct phy_device *phydev)
 	int err;
 
 	/* Change address */
-	err = marvell_set_page(phydev, 2);
+	err = marvell_set_page(phydev, MII_88E1121_PHY_MSCR_PAGE);
 	if (err < 0)
 		return err;
 
@@ -975,7 +975,7 @@ static int m88e1149_config_init(struct phy_device *phydev)
 		return err;
 
 	/* Reset address */
-	err = marvell_set_page(phydev, 0);
+	err = marvell_set_page(phydev, MII_M1111_COPPER);
 	if (err < 0)
 		return err;
 
@@ -1409,7 +1409,7 @@ static void m88e1318_get_wol(struct phy_device *phydev,
 	    MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE)
 		wol->wolopts |= WAKE_MAGIC;
 
-	if (marvell_set_page(phydev, 0x00) < 0)
+	if (marvell_set_page(phydev, MII_M1111_COPPER) < 0)
 		return;
 }
 
@@ -1422,7 +1422,7 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 
 	if (wol->wolopts & WAKE_MAGIC) {
 		/* Explicitly switch to page 0x00, just to be sure */
-		err = marvell_set_page(phydev, 0x00);
+		err = marvell_set_page(phydev, MII_M1111_COPPER);
 		if (err < 0)
 			return err;
 
-- 
2.11.0

^ permalink raw reply related

* Re: [PATCH net] netfilter: do not hold dev in ipt_CLUSTERIP
From: Pablo Neira Ayuso @ 2017-05-23 21:26 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, netfilter-devel, davem, fw
In-Reply-To: <cc6ebc3f8784d8d537737a1e18b443a2e0c7313a.1495271286.git.lucien.xin@gmail.com>

On Sat, May 20, 2017 at 05:08:06PM +0800, Xin Long wrote:
> It's a terrible thing to hold dev in iptables target. When the dev is
> being removed, unregister_netdevice has to wait for the dev to become
> free. dmesg will keep logging the err:
> 
>   kernel:unregister_netdevice: waiting for veth0_in to become free. \
>   Usage count = 1
> 
> until iptables rules with this target are removed manually.
> 
> The worse thing is when deleting a netns, a virtual nic will be deleted
> instead of reset to init_net in default_device_ops exit/exit_batch. As
> it is earlier than to flush the iptables rules in iptable_filter_net_ops
> exit, unregister_netdevice will block to wait for the nic to become free.
> 
> As unregister_netdevice is actually waiting for iptables rules flushing
> while iptables rules have to be flushed after unregister_netdevice. This
> 'dead lock' will cause unregister_netdevice to block there forever. As
> the netns is not available to operate at that moment, iptables rules can
> not even be flushed manually either.
> 
> The reproducer can be:
> 
>   # ip netns add test
>   # ip link add veth0_in type veth peer name veth0_out
>   # ip link set veth0_in netns test
>   # ip netns exec test ip link set lo up
>   # ip netns exec test ip link set veth0_in up
>   # ip netns exec test iptables -I INPUT -d 1.2.3.4 -i veth0_in -j \
>     CLUSTERIP --new --clustermac 89:d4:47:eb:9a:fa --total-nodes 3 \
>     --local-node 1 --hashmode sourceip-sourceport
>   # ip netns del test
> 
> This issue can be triggered by all virtual nics with ipt_CLUSTERIP.
> 
> This patch is to fix it by not holding dev in ipt_CLUSTERIP, but only
> save dev->ifindex instead of dev. When removing the mc from the dev,
> it will get dev by c->ifindex through dev_get_by_index.
> 
> Note that it doesn't save dev->name but dev->ifindex, as a dev->name
> can be changed, it will confuse ipt_CLUSTERIP.
> 
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

OK. Let's fix this finally... One comment below.

>  net/ipv4/netfilter/ipt_CLUSTERIP.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
> index 038f293..d1adb2f 100644
> --- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
> +++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
> @@ -47,7 +47,7 @@ struct clusterip_config {
>  
>  	__be32 clusterip;			/* the IP address */
>  	u_int8_t clustermac[ETH_ALEN];		/* the MAC address */
> -	struct net_device *dev;			/* device */
> +	int ifindex;				/* device ifindex */
>  	u_int16_t num_total_nodes;		/* total number of nodes */
>  	unsigned long local_nodes;		/* node number array */
>  
> @@ -98,19 +98,23 @@ clusterip_config_put(struct clusterip_config *c)
>   * entry(rule) is removed, remove the config from lists, but don't free it
>   * yet, since proc-files could still be holding references */
>  static inline void
> -clusterip_config_entry_put(struct clusterip_config *c)
> +clusterip_config_entry_put(struct net *net, struct clusterip_config *c)
>  {
> -	struct net *net = dev_net(c->dev);
>  	struct clusterip_net *cn = net_generic(net, clusterip_net_id);
>  
>  	local_bh_disable();
>  	if (refcount_dec_and_lock(&c->entries, &cn->lock)) {
> +		struct net_device *dev;
> +
>  		list_del_rcu(&c->list);
>  		spin_unlock(&cn->lock);
>  		local_bh_enable();
>  
> -		dev_mc_del(c->dev, c->clustermac);
> -		dev_put(c->dev);
> +		dev = dev_get_by_index(net, c->ifindex);
> +		if (dev) {
> +			dev_mc_del(dev, c->clustermac);
> +			dev_put(dev);
> +		}
>  
>  		/* In case anyone still accesses the file, the open/close
>  		 * functions are also incrementing the refcount on their own,
> @@ -182,7 +186,7 @@ clusterip_config_init(const struct ipt_clusterip_tgt_info *i, __be32 ip,
>  	if (!c)
>  		return ERR_PTR(-ENOMEM);
>  
> -	c->dev = dev;
> +	c->ifindex = dev->ifindex;
>  	c->clusterip = ip;
>  	memcpy(&c->clustermac, &i->clustermac, ETH_ALEN);
>  	c->num_total_nodes = i->num_total_nodes;
> @@ -427,12 +431,14 @@ static int clusterip_tg_check(const struct xt_tgchk_param *par)
>  			}
>  
>  			config = clusterip_config_init(cipinfo,
> -							e->ip.dst.s_addr, dev);
> +						       e->ip.dst.s_addr, dev);
>  			if (IS_ERR(config)) {
>  				dev_put(dev);
>  				return PTR_ERR(config);
>  			}
> -			dev_mc_add(config->dev, config->clustermac);
> +
> +			dev_mc_add(dev, config->clustermac);
> +			dev_put(dev);
>  		}
>  	}
>  	cipinfo->config = config;
> @@ -458,7 +464,7 @@ static void clusterip_tg_destroy(const struct xt_tgdtor_param *par)
>  
>  	/* if no more entries are referencing the config, remove it
>  	 * from the list and destroy the proc entry */
> -	clusterip_config_entry_put(cipinfo->config);
> +	clusterip_config_entry_put(par->net, cipinfo->config);
>  
>  	clusterip_config_put(cipinfo->config);
>  
> @@ -558,10 +564,9 @@ arp_mangle(void *priv,
>  	 * addresses on different interfacs.  However, in the CLUSTERIP case
>  	 * this wouldn't work, since we didn't subscribe the mcast group on
>  	 * other interfaces */
> -	if (c->dev != state->out) {
> -		pr_debug("not mangling arp reply on different "
> -			 "interface: cip'%s'-skb'%s'\n",
> -			 c->dev->name, state->out->name);
> +	if (c->ifindex != state->out->ifindex) {

I'm missing the code to register_netdevice_notifier() so we can
refresh c->ifindex.

Just like we do in net/netfilter/xt_TEE.c

^ permalink raw reply

* [PATCH net-next 0/4] More marvell phy cleanups
From: Andrew Lunn @ 2017-05-23 21:24 UTC (permalink / raw)
  To: David Miller; +Cc: Florian Fainelli, netdev, Andrew Lunn

This patchset continues the cleanup of the Marvell PHY driver.  These
phys use pages to allow more than the 32 registers that fit into the
MDIO address space. Cleanup the code used for changing pages.

Andrew Lunn (4):
  net: phy: marvell: #defines for copper and fibre pages
  net: phy: marvell: More hidden page changes refactored
  net: phy: marvell: helper to get and set page
  net: phy: marvell: Uniform page names

 drivers/net/phy/marvell.c | 181 +++++++++++++++++++++++++---------------------
 1 file changed, 98 insertions(+), 83 deletions(-)

-- 
2.11.0

^ permalink raw reply

* Re: Alignment in BPF verifier
From: Daniel Borkmann @ 2017-05-23 21:27 UTC (permalink / raw)
  To: Alexei Starovoitov, Edward Cree, David Miller; +Cc: alexei.starovoitov, netdev
In-Reply-To: <ad2162c3-1286-ab35-464a-8d21a1adc94f@fb.com>

On 05/23/2017 09:45 PM, Alexei Starovoitov wrote:
> On 5/23/17 7:41 AM, Edward Cree wrote:
>> I'm still plugging away at this... it's going to be quite a big patch and
>>  rewrite a lot of stuff (and I'm not sure I'll be able to break it into
>>  smaller bisectable patches).
>> And of course I have more questions.  In check_packet_ptr_add(), we
>>  forbid adding a negative constant to a packet ptr.  Is there some
>>  principled reason for that, or is it just because the bounds checking is
>>  hard?  It seems like, if imm + reg->off > 0 (suitably carefully checked
>>  to avoid overflow etc.), then the subtraction should be legal.  Indeed,
>>  even if the reg->off (fixed part of offset) is zero, if the variable part
>>  is known (min_value) to be >= -imm, the subtraction should be safe.
>
> adding negative imm to pkt_ptr is ok, but what is the use case?
> Do you see llvm generating such code?

Btw, currently, you kind of have it in a limited way via BPF_STX_MEM()
and BPF_LDX_MEM() when accessing pkt data through the offset, which
can be negative on the insn level.

> I think if we try to track everything with the current shape of
> state pruning, the verifier will stop accepting old programs
> because it reaches complexity limit.
>
> I think we need to rearchitect the whole thing.
> I was thinking of doing it compiler-style. Convert to ssa and
> do traditional data flow analysis, use-def chains, register liveness
> then pruning heuristics won't be necessary and verifier should be
> able to check everything in more or less single pass.
> Things like register liveness can be done without ssa. It can
> be used to augment existing pruning, since it will know which
> registers are dead, so they don't have to be compared, but it
> feels half-way. I'd rather go all the way.
>
>> On 20/05/17 00:05, Daniel Borkmann wrote:
>>> Besides PTR_TO_PACKET also PTR_TO_MAP_VALUE_OR_NULL uses it to
>>> track all registers (incl. spilled ones) with the same reg->id
>>> that originated from the same map lookup. After the reg type is
>>> then migrated to either PTR_TO_MAP_VALUE (resp. CONST_PTR_TO_MAP
>>> for map in map) or UNKNOWN_VALUE depending on the branch, the
>>> reg->id is then reset to 0 again. Whole reason for this is that
>>> LLVM generates code where it can move and/or spill a reg of type
>>> PTR_TO_MAP_VALUE_OR_NULL to other regs before we do the NULL
>>> test on it, and later on it expects that the spilled or moved
>>> regs work wrt access. So they're marked with an id and then all
>>> of them are type migrated. So here meaning of reg->id is different
>>> than in PTR_TO_PACKET case.
>> Hmm, that means that we can't do arithmetic on a
>>  PTR_TO_MAP_VALUE_OR_NULL, we have to convert it to a PTR_TO_MAP_VALUE
>>  first by NULL-checking it.  That's probably fine, but I can just about
>>  imagine some compiler optimisation reordering them.  Any reason not to
>>  split this out into a different reg->field, rather than overloading id?
>
> 'id' is sort of like 'version' of a pointer and has the same meaning in
> both cases. How exactly do you see this split?

Also, same id is never reused once generated and later propagated
through regs. So far we haven't run into this kind of optimization
from llvm side yet, but others which led to requiring the id marker
(see 57a09bf0a416). I could imagine it might be needed at some point,
though where we later transition directly to PTR_TO_MAP_VALUE_ADJ
after NULL check. Out of curiosity, did you run into it with llvm?

>> Of course that would need (more) caution wrt. states_equal(), but it
>>  looks like I'll be mangling that a lot anyway - for instance, we don't
>>  want to just use memcmp() to compare alignments, we want to check that
>>  our alignment is stricter than the old alignment.  (Of course memcmp()
>>  is a conservative check, so the "memcmp() the whole reg_state" fast
>>  path can remain.)
>
> yes. that would be good improvement. Not sure how much it will help
> the pruning though.

^ permalink raw reply

* Re: bond link state mismatch, rtnl_trylock() vs rtnl_lock()
From: Mahesh Bandewar (महेश बंडेवार) @ 2017-05-23 21:30 UTC (permalink / raw)
  To: Nithin Sujir; +Cc: linux-netdev
In-Reply-To: <24c0747b-df09-66da-f3ac-a393a5902a72@tintri.com>

On Tue, May 23, 2017 at 12:32 PM, Nithin Sujir <nsujir@tintri.com> wrote:
> Hi,
> We're encountering a problem in 4.4 LTS where, rarely, the bond link state
> is not updated when the slave link changes.
>
> I've traced the issue to the arp monitor unable to get the rtnl lock. The
> sequence resulting in failure is as below.
>
> bond_loadbalance_arp_mon() periodically called, if slave link is _down_, it
> checks if the slave is sending/receiving packets. If it is, it sets flags to
> be processed later down the function for bond link update. However, it sets
> the slave->link right away.
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, trans_start, 1) &&
>                             bond_time_in_interval(bond, slave->last_rx, 1))
> {
>
>                                 slave->link  = BOND_LINK_UP;
>                                 slave_state_changed = 1;
>
>
> Later down the function, it tries to get the rtnl_lock. If it doesn't get
> it, it rearms and returns.
>
>         if (do_failover || slave_state_changed) {
>                 if (!rtnl_trylock())
>                         goto re_arm; <-- returns here
>
>                 if (slave_state_changed) {
>                         bond_slave_state_change(bond);
>
> This is the problem. The next time this function is called, the slave->link
> is already marked UP. And we will never update the bond link state to UP.
>
> Changing the rtnl_trylock() -> rtnl_lock() _does_ fix the issue.
>
> Is this the right way to fix it? If it is, I can submit this formally.
>
> What are the guidelines around using rtnl_lock() vs rtnl_trylock()? Some
> places are using rtnl_lock() and other rtnl_trylock(). Sorry, I couldn't
> find much via a google search or in Documentation/.
>
> Thanks,
> Nithin.
>
> --------------------
>
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index 5dca77e..1f60503 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2614,8 +2614,7 @@ static void bond_loadbalance_arp_mon(struct
> work_struct *work)
>         rcu_read_unlock();
>
>         if (do_failover || slave_state_changed) {
> -               if (!rtnl_trylock())
> -                       goto re_arm;
> +               rtnl_lock();

Nitin, you can't do this. The tryRTNL code is to prevent deadlock
during work-cancellation during bond_close().

>
>                 if (slave_state_changed) {
>                         bond_slave_state_change(bond);
>
>

^ permalink raw reply

* Re: bond link state mismatch, rtnl_trylock() vs rtnl_lock()
From: Nithin Sujir @ 2017-05-23 21:35 UTC (permalink / raw)
  To: Mahesh Bandewar (महेश बंडेवार)
  Cc: linux-netdev
In-Reply-To: <CAF2d9jiUh2iVG4=igP0+e6dzZkff_h8u02taQts6hC2B2qsM2g@mail.gmail.com>



On 5/23/2017 2:30 PM, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Tue, May 23, 2017 at 12:32 PM, Nithin Sujir <nsujir@tintri.com> wrote:
>> diff --git a/drivers/net/bonding/bond_main.c
>> b/drivers/net/bonding/bond_main.c
>> index 5dca77e..1f60503 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2614,8 +2614,7 @@ static void bond_loadbalance_arp_mon(struct
>> work_struct *work)
>>          rcu_read_unlock();
>>
>>          if (do_failover || slave_state_changed) {
>> -               if (!rtnl_trylock())
>> -                       goto re_arm;
>> +               rtnl_lock();
> Nitin, you can't do this. The tryRTNL code is to prevent deadlock
> during work-cancellation during bond_close().
Thanks, Mahesh. Yes, Jay pointed me to your patch and I will take a look 
at how to use a similar approach.

Nithin.

>>                  if (slave_state_changed) {
>>                          bond_slave_state_change(bond);
>>
>>

^ permalink raw reply

* [patch iproute2] tc: flower: add support for tcp flags
From: Jiri Pirko @ 2017-05-23 21:51 UTC (permalink / raw)
  To: netdev; +Cc: davem, idosch, yotamg, mlxsw, Yuval.Mintz
In-Reply-To: <20170523164048.16514-1-jiri@resnulli.us>

From: Jiri Pirko <jiri@mellanox.com>

Allow user to insert a flower classifier filter rule which includes
match for tcp flags.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- removed forgotten debug printout
- fixed mask parsing as reported by Or
---
 include/linux/pkt_cls.h |  3 +++
 man/man8/tc-flower.8    |  8 +++++++
 tc/f_flower.c           | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+)

diff --git a/include/linux/pkt_cls.h b/include/linux/pkt_cls.h
index d613be3..ce9dfb9 100644
--- a/include/linux/pkt_cls.h
+++ b/include/linux/pkt_cls.h
@@ -450,6 +450,9 @@ enum {
 	TCA_FLOWER_KEY_MPLS_TC,		/* u8 - 3 bits */
 	TCA_FLOWER_KEY_MPLS_LABEL,	/* be32 - 20 bits */
 
+	TCA_FLOWER_KEY_TCP_FLAGS,	/* be16 */
+	TCA_FLOWER_KEY_TCP_FLAGS_MASK,	/* be16 */
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/man/man8/tc-flower.8 b/man/man8/tc-flower.8
index ba29065..7648079 100644
--- a/man/man8/tc-flower.8
+++ b/man/man8/tc-flower.8
@@ -35,6 +35,8 @@ flower \- flow based traffic control filter
 .IR PREFIX " | { "
 .BR dst_port " | " src_port " } "
 .IR port_number " } | "
+.B tcp_flags
+.IR MASKED_TCP_FLAGS " | "
 .B type
 .IR MASKED_TYPE " | "
 .B code
@@ -136,6 +138,12 @@ Match on layer 4 protocol source or destination port number. Only available for
 .BR ip_proto " values " udp ", " tcp  " and " sctp
 which have to be specified in beforehand.
 .TP
+.BI tcp_flags " MASKED_TCP_FLAGS"
+Match on TCP flags represented as 12bit bitfield in in hexadecimal format.
+A mask may be optionally provided to limit the bits which are matched. A mask
+is provided by following the value with a slash and then the mask. If the mask
+is missing then a match on all bits is assumed.
+.TP
 .BI type " MASKED_TYPE"
 .TQ
 .BI code " MASKED_CODE"
diff --git a/tc/f_flower.c b/tc/f_flower.c
index ebc63ca..1b6b46e 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -57,6 +57,7 @@ static void explain(void)
 		"                       src_ip PREFIX |\n"
 		"                       dst_port PORT-NUMBER |\n"
 		"                       src_port PORT-NUMBER |\n"
+		"                       tcp_flags MASKED-TCP_FLAGS |\n"
 		"                       type MASKED-ICMP-TYPE |\n"
 		"                       code MASKED-ICMP-CODE |\n"
 		"                       arp_tip IPV4-PREFIX |\n"
@@ -474,6 +475,41 @@ static int flower_parse_port(char *str, __u8 ip_proto,
 	return 0;
 }
 
+#define TCP_FLAGS_MAX_MASK 0xfff
+
+static int flower_parse_tcp_flags(char *str, int flags_type, int mask_type,
+				  struct nlmsghdr *n)
+{
+	char *slash;
+	int ret, err = -1;
+	__u16 flags;
+
+	slash = strchr(str, '/');
+	if (slash)
+		*slash = '\0';
+
+	ret = get_u16(&flags, str, 16);
+	if (ret < 0 || flags & ~TCP_FLAGS_MAX_MASK)
+		goto err;
+
+	addattr16(n, MAX_MSG, flags_type, htons(flags));
+
+	if (slash) {
+		ret = get_u16(&flags, slash + 1, 16);
+		if (ret < 0 || flags & ~TCP_FLAGS_MAX_MASK)
+			goto err;
+	} else {
+		flags = TCP_FLAGS_MAX_MASK;
+	}
+	addattr16(n, MAX_MSG, mask_type, htons(flags));
+
+	err = 0;
+err:
+	if (slash)
+		*slash = '/';
+	return err;
+}
+
 static int flower_parse_key_id(const char *str, int type, struct nlmsghdr *n)
 {
 	int ret;
@@ -671,6 +707,16 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 				fprintf(stderr, "Illegal \"src_port\"\n");
 				return -1;
 			}
+		} else if (matches(*argv, "tcp_flags") == 0) {
+			NEXT_ARG();
+			ret = flower_parse_tcp_flags(*argv,
+						     TCA_FLOWER_KEY_TCP_FLAGS,
+						     TCA_FLOWER_KEY_TCP_FLAGS_MASK,
+						     n);
+			if (ret < 0) {
+				fprintf(stderr, "Illegal \"tcp_flags\"\n");
+				return -1;
+			}
 		} else if (matches(*argv, "type") == 0) {
 			NEXT_ARG();
 			ret = flower_parse_icmp(*argv, eth_type, ip_proto,
@@ -1000,6 +1046,19 @@ static void flower_print_port(FILE *f, char *name, struct rtattr *attr)
 		fprintf(f, "\n  %s %d", name, rta_getattr_be16(attr));
 }
 
+static void flower_print_tcp_flags(FILE *f, char *name,
+				  struct rtattr *flags_attr,
+				  struct rtattr *mask_attr)
+{
+	if (!flags_attr)
+		return;
+	fprintf(f, "\n  %s %x", name, rta_getattr_be16(flags_attr));
+	if (!mask_attr)
+		return;
+	fprintf(f, "/%x", rta_getattr_be16(mask_attr));
+}
+
+
 static void flower_print_key_id(FILE *f, const char *name,
 				struct rtattr *attr)
 {
@@ -1110,6 +1169,9 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
 	if (nl_type >= 0)
 		flower_print_port(f, "src_port", tb[nl_type]);
 
+	flower_print_tcp_flags(f, "tcp_flags", tb[TCA_FLOWER_KEY_TCP_FLAGS],
+			       tb[TCA_FLOWER_KEY_TCP_FLAGS_MASK]);
+
 	nl_type = flower_icmp_attr_type(eth_type, ip_proto,
 					FLOWER_ICMP_FIELD_TYPE);
 	nl_mask_type = flower_icmp_attr_mask_type(eth_type, ip_proto,
-- 
2.9.3

^ permalink raw reply related

* [PATCH net-next] tcp: better validation of received ack sequences
From: Eric Dumazet @ 2017-05-23 22:24 UTC (permalink / raw)
  To: Paul Fiterau Brostean, David Miller
  Cc: netdev, Frits Vaandrager, Neal Cardwell, Yuchung Cheng,
	Soheil Hassas Yeganeh
In-Reply-To: <fbb92f49-5eca-f411-dcab-253d2a222623@science.ru.nl>

From: Eric Dumazet <edumazet@google.com>

Paul Fiterau Brostean reported :

<quote>
Linux TCP stack we analyze exhibits behavior that seems odd to me.
The scenario is as follows (all packets have empty payloads, no window
scaling, rcv/snd window size should not be a factor):

       TEST HARNESS (CLIENT)                        LINUX SERVER

   1.  -                                          LISTEN (server listen,
then accepts)

   2.  - --> <SEQ=100><CTL=SYN>               --> SYN-RECEIVED

   3.  - <-- <SEQ=300><ACK=101><CTL=SYN,ACK>  <-- SYN-RECEIVED

   4.  - --> <SEQ=101><ACK=301><CTL=ACK>      --> ESTABLISHED
  
   5.  - <-- <SEQ=301><ACK=101><CTL=FIN,ACK>  <-- FIN WAIT-1 (server
opts to close the data connection calling "close" on the connection
socket)

   6.  - --> <SEQ=101><ACK=99999><CTL=FIN,ACK> --> CLOSING (client sends
FIN,ACK with not yet sent acknowledgement number)

   7.  - <-- <SEQ=302><ACK=102><CTL=ACK>      <-- CLOSING (ACK is 102
instead of 101, why?)
  
... (silence from CLIENT)

   8.  - <-- <SEQ=301><ACK=102><CTL=FIN,ACK>  <-- CLOSING
(retransmission, again ACK is 102)


Now, note that packet 6 while having the expected sequence number, 
acknowledges something that wasn't sent by the server. So I would
expect 
the packet to maybe prompt an ACK response from the server, and then be 
ignored. Yet it is not ignored and actually leads to an increase of the 
acknowledgement number in the server's retransmission of the FIN,ACK 
packet. The explanation I found is that the FIN  in packet 6 was 
processed, despite the acknowledgement number being unacceptable. 
Further experiments indeed show that the server processes this FIN, 
transitioning to CLOSING, then on receiving an ACK for the FIN it had 
send in packet 5, the server (or better said connection) transitions 
from CLOSING to TIME_WAIT (as signaled by netstat).

</quote>

Indeed, tcp_rcv_state_process() calls tcp_ack() but
does not exploit the @acceptable status but for TCP_SYN_RECV
state.

What we want here is to send a challenge ACK, if not in TCP_SYN_RECV
state. TCP_FIN_WAIT1 state is not the only state we should fix.

Add a FLAG_NO_CHALLENGE_ACK so that tcp_rcv_state_process()
can choose to send a challenge ACK and discard the packet instead
of wrongly change socket state.

With help from Neal Cardwell.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Paul Fiterau Brostean <p.fiterau-brostean@science.ru.nl>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
---
Note for Googlers : Google-Bug-Id: 37204158

 net/ipv4/tcp_input.c |   24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2fa55f57ac06584bfd9b555799ceb3bbfb7e1b4e..c3bdcbcf544793ba410c618130586bf7d3963da6 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -112,6 +112,7 @@ int sysctl_tcp_invalid_ratelimit __read_mostly = HZ/2;
 #define FLAG_DSACKING_ACK	0x800 /* SACK blocks contained D-SACK info */
 #define FLAG_SACK_RENEGING	0x2000 /* snd_una advanced to a sacked seq */
 #define FLAG_UPDATE_TS_RECENT	0x4000 /* tcp_replace_ts_recent() */
+#define FLAG_NO_CHALLENGE_ACK	0x8000 /* do not call tcp_send_challenge_ack()	*/
 
 #define FLAG_ACKED		(FLAG_DATA_ACKED|FLAG_SYN_ACKED)
 #define FLAG_NOT_DUP		(FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED)
@@ -3568,7 +3569,8 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
 	if (before(ack, prior_snd_una)) {
 		/* RFC 5961 5.2 [Blind Data Injection Attack].[Mitigation] */
 		if (before(ack, prior_snd_una - tp->max_window)) {
-			tcp_send_challenge_ack(sk, skb);
+			if (!(flag & FLAG_NO_CHALLENGE_ACK))
+				tcp_send_challenge_ack(sk, skb);
 			return -1;
 		}
 		goto old_ack;
@@ -5951,13 +5953,17 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 
 	/* step 5: check the ACK field */
 	acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH |
-				      FLAG_UPDATE_TS_RECENT) > 0;
+				      FLAG_UPDATE_TS_RECENT |
+				      FLAG_NO_CHALLENGE_ACK) > 0;
 
+	if (!acceptable) {
+		if (sk->sk_state == TCP_SYN_RECV)
+			return 1;	/* send one RST */
+		tcp_send_challenge_ack(sk, skb);
+		goto discard;
+	}
 	switch (sk->sk_state) {
 	case TCP_SYN_RECV:
-		if (!acceptable)
-			return 1;
-
 		if (!tp->srtt_us)
 			tcp_synack_rtt_meas(sk, req);
 
@@ -6026,14 +6032,6 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 		 * our SYNACK so stop the SYNACK timer.
 		 */
 		if (req) {
-			/* Return RST if ack_seq is invalid.
-			 * Note that RFC793 only says to generate a
-			 * DUPACK for it but for TCP Fast Open it seems
-			 * better to treat this case like TCP_SYN_RECV
-			 * above.
-			 */
-			if (!acceptable)
-				return 1;
 			/* We no longer need the request sock. */
 			reqsk_fastopen_remove(sk, req, false);
 			tcp_rearm_rto(sk);

^ permalink raw reply related

* [PATCH] net: phy: put genphy_config_init's EXPORT_SYMBOL directly after the function
From: Uwe Kleine-König @ 2017-05-23 22:26 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, David S. Miller; +Cc: netdev, kernel

Commit af6b6967d6e1 ("net: phy: export genphy_config_init()") introduced
this EXPORT_SYMBOL and put it after gen10g_soft_reset() instead of
directly after genphy_config_init. Probably this happend when the patch
was applied because http://patchwork.ozlabs.org/patch/339622/ looks ok.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/phy/phy_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1219eeab69d1..0780e9f9e167 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1571,13 +1571,13 @@ int genphy_config_init(struct phy_device *phydev)
 
 	return 0;
 }
+EXPORT_SYMBOL(genphy_config_init);
 
 static int gen10g_soft_reset(struct phy_device *phydev)
 {
 	/* Do nothing for now */
 	return 0;
 }
-EXPORT_SYMBOL(genphy_config_init);
 
 static int gen10g_config_init(struct phy_device *phydev)
 {
-- 
2.11.0

^ permalink raw reply related


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