* Re: [PATCH v5 2/2] net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341
From: Andrew Lunn @ 2017-01-20 0:12 UTC (permalink / raw)
To: Vivien Didelot
Cc: Thomas Petazzoni, Florian Fainelli, Jason Cooper, Joe Zhou,
netdev, linux-kernel, Jon Pannell, Nadav Haklai,
Kostya Porotchkin, Gregory CLEMENT, Wilson Ding, David S. Miller,
linux-arm-kernel, Sebastian Hesselbarth
In-Reply-To: <87y3y6k7pg.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On Thu, Jan 19, 2017 at 05:26:03PM -0500, Vivien Didelot wrote:
> Gregory CLEMENT <gregory.clement@free-electrons.com> writes:
>
> > +static bool mv88e6xxx_6341_family(struct mv88e6xxx_chip *chip)
> > +{
> > + return chip->info->family == MV88E6XXX_FAMILY_6341;
> > +}
>
> I don't want to see these erronous family checks anymore, but I cannot
> blame you for adding it since not all the code is moved to ops yet ;)
>
> > MV88E6XXX_FAMILY_6165, /* 6123 6161 6165 */
> > MV88E6XXX_FAMILY_6185, /* 6108 6121 6122 6131 6152 6155 6182 6185 */
> > MV88E6XXX_FAMILY_6320, /* 6320 6321 */
> > + MV88E6XXX_FAMILY_6341, /* 6141 6341 */
>
> Maybe I missed it, Andrew, can you confirm that 6341 is a proper Marvell
> family of switch chips?
My understand is that it is. Marvell have not added it to DSDT. There
is a new SDK called UMSD, also GPLv2 and BSD. They call this family
Topaz, and the 6390 is Peridot.
Andrew
^ permalink raw reply
* [PATCH v2 0/2] net: dsa: Move temperature sensor code into PHY.
From: Andrew Lunn @ 2017-01-20 0:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Vivien Didelot, Andrew Lunn
Marvell Ethernet switches contain a temperature sensor. There appears
to be one sensor, which is shared by each of the internal PHYs. Each
PHY has independent registers to read this sensor, and to set a limit
for when an alarm should be raised.
Some Marvell discrete PHY also have the same sensor and registers.
Moving the HWMON code from DSA into the PHY makes the sensor available
in discrete PHYs, and removes the layering violation, the switch
driver poking around in PHY registers.
While moving the code into the PHY driver, it has been re-written to
use the new HWMON APIs.
v2:
Better Cover note explaining one sensor, but multiple independent
registers
Simply error checking.
Andrew Lunn (2):
phy: marvell: Add support for temperature sensor
net: dsa: Remove hwmon support
drivers/net/dsa/mv88e6xxx/chip.c | 154 -------------
drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 16 --
drivers/net/phy/marvell.c | 423 +++++++++++++++++++++++++++++++++-
include/net/dsa.h | 8 -
net/dsa/Kconfig | 11 -
net/dsa/Makefile | 1 -
net/dsa/dsa.c | 4 -
net/dsa/dsa_priv.h | 9 -
net/dsa/hwmon.c | 147 ------------
9 files changed, 420 insertions(+), 353 deletions(-)
delete mode 100644 net/dsa/hwmon.c
--
2.11.0
^ permalink raw reply
* [PATCH v2 2/2] net: dsa: Remove hwmon support
From: Andrew Lunn @ 2017-01-20 0:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Vivien Didelot, Andrew Lunn
In-Reply-To: <1484872670-32585-1-git-send-email-andrew@lunn.ch>
Only the Marvell mv88e6xxx DSA driver made use of the HWMON support in
DSA. The temperature sensor registers are actually in the embedded
PHYs, and the PHY driver now supports it. So remove all HWMON support
from DSA and drivers.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/dsa/mv88e6xxx/chip.c | 154 ----------------------------------
drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 16 ----
include/net/dsa.h | 8 --
net/dsa/Kconfig | 11 ---
net/dsa/Makefile | 1 -
net/dsa/dsa.c | 4 -
net/dsa/dsa_priv.h | 9 --
net/dsa/hwmon.c | 147 --------------------------------
8 files changed, 350 deletions(-)
delete mode 100644 net/dsa/hwmon.c
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 987b2dbbd35a..c7e08e13bb54 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2964,154 +2964,6 @@ static void mv88e6xxx_mdio_unregister(struct mv88e6xxx_chip *chip)
of_node_put(chip->mdio_np);
}
-#ifdef CONFIG_NET_DSA_HWMON
-
-static int mv88e61xx_get_temp(struct dsa_switch *ds, int *temp)
-{
- struct mv88e6xxx_chip *chip = ds->priv;
- u16 val;
- int ret;
-
- *temp = 0;
-
- mutex_lock(&chip->reg_lock);
-
- ret = mv88e6xxx_phy_write(chip, 0x0, 0x16, 0x6);
- if (ret < 0)
- goto error;
-
- /* Enable temperature sensor */
- ret = mv88e6xxx_phy_read(chip, 0x0, 0x1a, &val);
- if (ret < 0)
- goto error;
-
- ret = mv88e6xxx_phy_write(chip, 0x0, 0x1a, val | (1 << 5));
- if (ret < 0)
- goto error;
-
- /* Wait for temperature to stabilize */
- usleep_range(10000, 12000);
-
- ret = mv88e6xxx_phy_read(chip, 0x0, 0x1a, &val);
- if (ret < 0)
- goto error;
-
- /* Disable temperature sensor */
- ret = mv88e6xxx_phy_write(chip, 0x0, 0x1a, val & ~(1 << 5));
- if (ret < 0)
- goto error;
-
- *temp = ((val & 0x1f) - 5) * 5;
-
-error:
- mv88e6xxx_phy_write(chip, 0x0, 0x16, 0x0);
- mutex_unlock(&chip->reg_lock);
- return ret;
-}
-
-static int mv88e63xx_get_temp(struct dsa_switch *ds, int *temp)
-{
- struct mv88e6xxx_chip *chip = ds->priv;
- int phy = mv88e6xxx_6320_family(chip) ? 3 : 0;
- u16 val;
- int ret;
-
- *temp = 0;
-
- mutex_lock(&chip->reg_lock);
- ret = mv88e6xxx_phy_page_read(chip, phy, 6, 27, &val);
- mutex_unlock(&chip->reg_lock);
- if (ret < 0)
- return ret;
-
- *temp = (val & 0xff) - 25;
-
- return 0;
-}
-
-static int mv88e6xxx_get_temp(struct dsa_switch *ds, int *temp)
-{
- struct mv88e6xxx_chip *chip = ds->priv;
-
- if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_TEMP))
- return -EOPNOTSUPP;
-
- if (mv88e6xxx_6320_family(chip) || mv88e6xxx_6352_family(chip))
- return mv88e63xx_get_temp(ds, temp);
-
- return mv88e61xx_get_temp(ds, temp);
-}
-
-static int mv88e6xxx_get_temp_limit(struct dsa_switch *ds, int *temp)
-{
- struct mv88e6xxx_chip *chip = ds->priv;
- int phy = mv88e6xxx_6320_family(chip) ? 3 : 0;
- u16 val;
- int ret;
-
- if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_TEMP_LIMIT))
- return -EOPNOTSUPP;
-
- *temp = 0;
-
- mutex_lock(&chip->reg_lock);
- ret = mv88e6xxx_phy_page_read(chip, phy, 6, 26, &val);
- mutex_unlock(&chip->reg_lock);
- if (ret < 0)
- return ret;
-
- *temp = (((val >> 8) & 0x1f) * 5) - 25;
-
- return 0;
-}
-
-static int mv88e6xxx_set_temp_limit(struct dsa_switch *ds, int temp)
-{
- struct mv88e6xxx_chip *chip = ds->priv;
- int phy = mv88e6xxx_6320_family(chip) ? 3 : 0;
- u16 val;
- int err;
-
- if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_TEMP_LIMIT))
- return -EOPNOTSUPP;
-
- mutex_lock(&chip->reg_lock);
- err = mv88e6xxx_phy_page_read(chip, phy, 6, 26, &val);
- if (err)
- goto unlock;
- temp = clamp_val(DIV_ROUND_CLOSEST(temp, 5) + 5, 0, 0x1f);
- err = mv88e6xxx_phy_page_write(chip, phy, 6, 26,
- (val & 0xe0ff) | (temp << 8));
-unlock:
- mutex_unlock(&chip->reg_lock);
-
- return err;
-}
-
-static int mv88e6xxx_get_temp_alarm(struct dsa_switch *ds, bool *alarm)
-{
- struct mv88e6xxx_chip *chip = ds->priv;
- int phy = mv88e6xxx_6320_family(chip) ? 3 : 0;
- u16 val;
- int ret;
-
- if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_TEMP_LIMIT))
- return -EOPNOTSUPP;
-
- *alarm = false;
-
- mutex_lock(&chip->reg_lock);
- ret = mv88e6xxx_phy_page_read(chip, phy, 6, 26, &val);
- mutex_unlock(&chip->reg_lock);
- if (ret < 0)
- return ret;
-
- *alarm = !!(val & 0x40);
-
- return 0;
-}
-#endif /* CONFIG_NET_DSA_HWMON */
-
static int mv88e6xxx_get_eeprom_len(struct dsa_switch *ds)
{
struct mv88e6xxx_chip *chip = ds->priv;
@@ -4386,12 +4238,6 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
.get_sset_count = mv88e6xxx_get_sset_count,
.set_eee = mv88e6xxx_set_eee,
.get_eee = mv88e6xxx_get_eee,
-#ifdef CONFIG_NET_DSA_HWMON
- .get_temp = mv88e6xxx_get_temp,
- .get_temp_limit = mv88e6xxx_get_temp_limit,
- .set_temp_limit = mv88e6xxx_set_temp_limit,
- .get_temp_alarm = mv88e6xxx_get_temp_alarm,
-#endif
.get_eeprom_len = mv88e6xxx_get_eeprom_len,
.get_eeprom = mv88e6xxx_get_eeprom,
.set_eeprom = mv88e6xxx_set_eeprom,
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 466cfdadb7bd..ce8b43b14e96 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -497,12 +497,6 @@ enum mv88e6xxx_cap {
*/
MV88E6XXX_CAP_STU,
- /* Internal temperature sensor.
- * Available from any enabled port's PHY register 26, page 6.
- */
- MV88E6XXX_CAP_TEMP,
- MV88E6XXX_CAP_TEMP_LIMIT,
-
/* VLAN Table Unit.
* The VTU is used to program 802.1Q VLANs. See GLOBAL_VTU_OP.
*/
@@ -533,8 +527,6 @@ enum mv88e6xxx_cap {
#define MV88E6XXX_FLAG_G2_POT BIT_ULL(MV88E6XXX_CAP_G2_POT)
#define MV88E6XXX_FLAG_STU BIT_ULL(MV88E6XXX_CAP_STU)
-#define MV88E6XXX_FLAG_TEMP BIT_ULL(MV88E6XXX_CAP_TEMP)
-#define MV88E6XXX_FLAG_TEMP_LIMIT BIT_ULL(MV88E6XXX_CAP_TEMP_LIMIT)
#define MV88E6XXX_FLAG_VTU BIT_ULL(MV88E6XXX_CAP_VTU)
/* Ingress Rate Limit unit */
@@ -586,7 +578,6 @@ enum mv88e6xxx_cap {
MV88E6XXX_FLAG_G2_MGMT_EN_0X | \
MV88E6XXX_FLAG_G2_POT | \
MV88E6XXX_FLAG_STU | \
- MV88E6XXX_FLAG_TEMP | \
MV88E6XXX_FLAG_VTU | \
MV88E6XXX_FLAGS_IRL | \
MV88E6XXX_FLAGS_MULTI_CHIP | \
@@ -605,8 +596,6 @@ enum mv88e6xxx_cap {
MV88E6XXX_FLAG_G2_MGMT_EN_2X | \
MV88E6XXX_FLAG_G2_MGMT_EN_0X | \
MV88E6XXX_FLAG_G2_POT | \
- MV88E6XXX_FLAG_TEMP | \
- MV88E6XXX_FLAG_TEMP_LIMIT | \
MV88E6XXX_FLAG_VTU | \
MV88E6XXX_FLAGS_IRL | \
MV88E6XXX_FLAGS_MULTI_CHIP | \
@@ -621,7 +610,6 @@ enum mv88e6xxx_cap {
MV88E6XXX_FLAG_G2_MGMT_EN_0X | \
MV88E6XXX_FLAG_G2_POT | \
MV88E6XXX_FLAG_STU | \
- MV88E6XXX_FLAG_TEMP | \
MV88E6XXX_FLAG_VTU | \
MV88E6XXX_FLAGS_IRL | \
MV88E6XXX_FLAGS_MULTI_CHIP | \
@@ -637,8 +625,6 @@ enum mv88e6xxx_cap {
MV88E6XXX_FLAG_G2_MGMT_EN_0X | \
MV88E6XXX_FLAG_G2_POT | \
MV88E6XXX_FLAG_STU | \
- MV88E6XXX_FLAG_TEMP | \
- MV88E6XXX_FLAG_TEMP_LIMIT | \
MV88E6XXX_FLAG_VTU | \
MV88E6XXX_FLAGS_IRL | \
MV88E6XXX_FLAGS_MULTI_CHIP | \
@@ -651,8 +637,6 @@ struct mv88e6xxx_ops;
(MV88E6XXX_FLAG_EEE | \
MV88E6XXX_FLAG_GLOBAL2 | \
MV88E6XXX_FLAG_STU | \
- MV88E6XXX_FLAG_TEMP | \
- MV88E6XXX_FLAG_TEMP_LIMIT | \
MV88E6XXX_FLAG_VTU | \
MV88E6XXX_FLAGS_IRL | \
MV88E6XXX_FLAGS_MULTI_CHIP | \
diff --git a/include/net/dsa.h b/include/net/dsa.h
index c72ed7af2a2a..9d6cd923c48c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -307,14 +307,6 @@ struct dsa_switch_ops {
int (*get_eee)(struct dsa_switch *ds, int port,
struct ethtool_eee *e);
-#ifdef CONFIG_NET_DSA_HWMON
- /* Hardware monitoring */
- int (*get_temp)(struct dsa_switch *ds, int *temp);
- int (*get_temp_limit)(struct dsa_switch *ds, int *temp);
- int (*set_temp_limit)(struct dsa_switch *ds, int temp);
- int (*get_temp_alarm)(struct dsa_switch *ds, bool *alarm);
-#endif
-
/* EEPROM access */
int (*get_eeprom_len)(struct dsa_switch *ds);
int (*get_eeprom)(struct dsa_switch *ds,
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 39bb5b3a82f2..9649238eef40 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -15,17 +15,6 @@ config NET_DSA
if NET_DSA
-config NET_DSA_HWMON
- bool "Distributed Switch Architecture HWMON support"
- default y
- depends on HWMON && !(NET_DSA=y && HWMON=m)
- ---help---
- Say Y if you want to expose thermal sensor data on switches supported
- by the Distributed Switch Architecture.
-
- Some of those switches contain thermal sensors. This data is available
- via the hwmon sysfs interface and exposes the onboard sensors.
-
# tagging formats
config NET_DSA_TAG_BRCM
bool
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 560b6747c276..a3380ed0e0be 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -1,7 +1,6 @@
# the core
obj-$(CONFIG_NET_DSA) += dsa_core.o
dsa_core-y += dsa.o slave.o dsa2.o
-dsa_core-$(CONFIG_NET_DSA_HWMON) += hwmon.o
# tagging formats
dsa_core-$(CONFIG_NET_DSA_TAG_BRCM) += tag_brcm.o
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 91f96e1bd2ec..77cb78767f1d 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -316,8 +316,6 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
if (ret)
return ret;
- dsa_hwmon_register(ds);
-
return 0;
}
@@ -376,8 +374,6 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
{
int port;
- dsa_hwmon_unregister(ds);
-
/* Destroy network devices for physical switch ports. */
for (port = 0; port < DSA_MAX_PORTS; port++) {
if (!(ds->enabled_port_mask & (1 << port)))
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 7e3385ec73f4..63ae1484abae 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -56,15 +56,6 @@ const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol);
int dsa_cpu_port_ethtool_setup(struct dsa_switch *ds);
void dsa_cpu_port_ethtool_restore(struct dsa_switch *ds);
-/* hwmon.c */
-#ifdef CONFIG_NET_DSA_HWMON
-void dsa_hwmon_register(struct dsa_switch *ds);
-void dsa_hwmon_unregister(struct dsa_switch *ds);
-#else
-static inline void dsa_hwmon_register(struct dsa_switch *ds) { }
-static inline void dsa_hwmon_unregister(struct dsa_switch *ds) { }
-#endif
-
/* slave.c */
extern const struct dsa_device_ops notag_netdev_ops;
void dsa_slave_mii_bus_init(struct dsa_switch *ds);
diff --git a/net/dsa/hwmon.c b/net/dsa/hwmon.c
deleted file mode 100644
index 08831a811278..000000000000
--- a/net/dsa/hwmon.c
+++ /dev/null
@@ -1,147 +0,0 @@
-/*
- * net/dsa/hwmon.c - HWMON subsystem support
- *
- * 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/ctype.h>
-#include <linux/hwmon.h>
-#include <net/dsa.h>
-
-#include "dsa_priv.h"
-
-static ssize_t temp1_input_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct dsa_switch *ds = dev_get_drvdata(dev);
- int temp, ret;
-
- ret = ds->ops->get_temp(ds, &temp);
- if (ret < 0)
- return ret;
-
- return sprintf(buf, "%d\n", temp * 1000);
-}
-static DEVICE_ATTR_RO(temp1_input);
-
-static ssize_t temp1_max_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct dsa_switch *ds = dev_get_drvdata(dev);
- int temp, ret;
-
- ret = ds->ops->get_temp_limit(ds, &temp);
- if (ret < 0)
- return ret;
-
- return sprintf(buf, "%d\n", temp * 1000);
-}
-
-static ssize_t temp1_max_store(struct device *dev,
- struct device_attribute *attr, const char *buf,
- size_t count)
-{
- struct dsa_switch *ds = dev_get_drvdata(dev);
- int temp, ret;
-
- ret = kstrtoint(buf, 0, &temp);
- if (ret < 0)
- return ret;
-
- ret = ds->ops->set_temp_limit(ds, DIV_ROUND_CLOSEST(temp, 1000));
- if (ret < 0)
- return ret;
-
- return count;
-}
-static DEVICE_ATTR_RW(temp1_max);
-
-static ssize_t temp1_max_alarm_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct dsa_switch *ds = dev_get_drvdata(dev);
- bool alarm;
- int ret;
-
- ret = ds->ops->get_temp_alarm(ds, &alarm);
- if (ret < 0)
- return ret;
-
- return sprintf(buf, "%d\n", alarm);
-}
-static DEVICE_ATTR_RO(temp1_max_alarm);
-
-static struct attribute *dsa_hwmon_attrs[] = {
- &dev_attr_temp1_input.attr, /* 0 */
- &dev_attr_temp1_max.attr, /* 1 */
- &dev_attr_temp1_max_alarm.attr, /* 2 */
- NULL
-};
-
-static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj,
- struct attribute *attr, int index)
-{
- struct device *dev = container_of(kobj, struct device, kobj);
- struct dsa_switch *ds = dev_get_drvdata(dev);
- const struct dsa_switch_ops *ops = ds->ops;
- umode_t mode = attr->mode;
-
- if (index == 1) {
- if (!ops->get_temp_limit)
- mode = 0;
- else if (!ops->set_temp_limit)
- mode &= ~S_IWUSR;
- } else if (index == 2 && !ops->get_temp_alarm) {
- mode = 0;
- }
- return mode;
-}
-
-static const struct attribute_group dsa_hwmon_group = {
- .attrs = dsa_hwmon_attrs,
- .is_visible = dsa_hwmon_attrs_visible,
-};
-__ATTRIBUTE_GROUPS(dsa_hwmon);
-
-void dsa_hwmon_register(struct dsa_switch *ds)
-{
- const char *netname = netdev_name(ds->dst->master_netdev);
- char hname[IFNAMSIZ + 1];
- int i, j;
-
- /* If the switch provides temperature accessors, register with hardware
- * monitoring subsystem. Treat registration error as non-fatal.
- */
- if (!ds->ops->get_temp)
- return;
-
- /* Create valid hwmon 'name' attribute */
- for (i = j = 0; i < IFNAMSIZ && netname[i]; i++) {
- if (isalnum(netname[i]))
- hname[j++] = netname[i];
- }
- hname[j] = '\0';
- scnprintf(ds->hwmon_name, sizeof(ds->hwmon_name), "%s_dsa%d", hname,
- ds->index);
- ds->hwmon_dev = hwmon_device_register_with_groups(NULL, ds->hwmon_name,
- ds, dsa_hwmon_groups);
- if (IS_ERR(ds->hwmon_dev)) {
- pr_warn("DSA: failed to register HWMON subsystem for switch %d\n",
- ds->index);
- ds->hwmon_dev = NULL;
- } else {
- pr_info("DSA: registered HWMON subsystem for switch %d\n",
- ds->index);
- }
-}
-
-void dsa_hwmon_unregister(struct dsa_switch *ds)
-{
- if (ds->hwmon_dev) {
- hwmon_device_unregister(ds->hwmon_dev);
- ds->hwmon_dev = NULL;
- }
-}
--
2.11.0
^ permalink raw reply related
* [PATCH v2 1/2] phy: marvell: Add support for temperature sensor
From: Andrew Lunn @ 2017-01-20 0:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Vivien Didelot, Andrew Lunn
In-Reply-To: <1484872670-32585-1-git-send-email-andrew@lunn.ch>
Some Marvell PHYs have an inbuilt temperature sensor. Add hwmon
support for this sensor.
There are two different variants. The simpler, older chips have a 5
degree accuracy. The newer devices have 1 degree accuracy.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2: Simplify using PTR_ERR_OR_ZERO()
---
drivers/net/phy/marvell.c | 423 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 420 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 0b78210c0fa7..64229976ace1 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -17,8 +17,10 @@
*/
#include <linux/kernel.h>
#include <linux/string.h>
+#include <linux/ctype.h>
#include <linux/errno.h>
#include <linux/unistd.h>
+#include <linux/hwmon.h>
#include <linux/interrupt.h>
#include <linux/init.h>
#include <linux/delay.h>
@@ -90,6 +92,17 @@
#define MII_88E1121_PHY_MSCR_TX_DELAY BIT(4)
#define MII_88E1121_PHY_MSCR_DELAY_MASK (~(0x3 << 4))
+#define MII_88E1121_MISC_TEST 0x1a
+#define MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK 0x1f00
+#define MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT 8
+#define MII_88E1510_MISC_TEST_TEMP_IRQ_EN BIT(7)
+#define MII_88E1510_MISC_TEST_TEMP_IRQ BIT(6)
+#define MII_88E1121_MISC_TEST_TEMP_SENSOR_EN BIT(5)
+#define MII_88E1121_MISC_TEST_TEMP_MASK 0x1f
+
+#define MII_88E1510_TEMP_SENSOR 0x1b
+#define MII_88E1510_TEMP_SENSOR_MASK 0xff
+
#define MII_88E1318S_PHY_MSCR1_REG 16
#define MII_88E1318S_PHY_MSCR1_PAD_ODD BIT(6)
@@ -172,6 +185,8 @@ static struct marvell_hw_stat marvell_hw_stats[] = {
struct marvell_priv {
u64 stats[ARRAY_SIZE(marvell_hw_stats)];
+ char *hwmon_name;
+ struct device *hwmon_dev;
};
static int marvell_ack_interrupt(struct phy_device *phydev)
@@ -1468,6 +1483,371 @@ static void marvell_get_stats(struct phy_device *phydev,
data[i] = marvell_get_stat(phydev, i);
}
+#ifdef CONFIG_HWMON
+static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
+{
+ int ret;
+ int val;
+
+ *temp = 0;
+
+ mutex_lock(&phydev->lock);
+
+ ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+ if (ret < 0)
+ goto error;
+
+ /* Enable temperature sensor */
+ ret = phy_read(phydev, MII_88E1121_MISC_TEST);
+ if (ret < 0)
+ goto error;
+
+ ret = phy_write(phydev, MII_88E1121_MISC_TEST,
+ ret | MII_88E1121_MISC_TEST_TEMP_SENSOR_EN);
+ if (ret < 0)
+ goto error;
+
+ /* Wait for temperature to stabilize */
+ usleep_range(10000, 12000);
+
+ val = phy_read(phydev, MII_88E1121_MISC_TEST);
+ if (val < 0) {
+ ret = val;
+ goto error;
+ }
+
+ /* Disable temperature sensor */
+ ret = phy_write(phydev, MII_88E1121_MISC_TEST,
+ ret & ~MII_88E1121_MISC_TEST_TEMP_SENSOR_EN);
+ if (ret < 0)
+ goto error;
+
+ *temp = ((val & MII_88E1121_MISC_TEST_TEMP_MASK) - 5) * 5000;
+
+error:
+ phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+ mutex_unlock(&phydev->lock);
+
+ return ret;
+}
+
+static int m88e1121_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel, long *temp)
+{
+ struct phy_device *phydev = dev_get_drvdata(dev);
+ int err;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ err = m88e1121_get_temp(phydev, temp);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return err;
+}
+
+static umode_t m88e1121_hwmon_is_visible(const void *data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ if (type != hwmon_temp)
+ return 0;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ return 0444;
+ default:
+ return 0;
+ }
+}
+
+static u32 m88e1121_hwmon_chip_config[] = {
+ HWMON_C_REGISTER_TZ,
+ 0
+};
+
+static const struct hwmon_channel_info m88e1121_hwmon_chip = {
+ .type = hwmon_chip,
+ .config = m88e1121_hwmon_chip_config,
+};
+
+static u32 m88e1121_hwmon_temp_config[] = {
+ HWMON_T_INPUT,
+ 0
+};
+
+static const struct hwmon_channel_info m88e1121_hwmon_temp = {
+ .type = hwmon_temp,
+ .config = m88e1121_hwmon_temp_config,
+};
+
+static const struct hwmon_channel_info *m88e1121_hwmon_info[] = {
+ &m88e1121_hwmon_chip,
+ &m88e1121_hwmon_temp,
+ NULL
+};
+
+static const struct hwmon_ops m88e1121_hwmon_hwmon_ops = {
+ .is_visible = m88e1121_hwmon_is_visible,
+ .read = m88e1121_hwmon_read,
+};
+
+static const struct hwmon_chip_info m88e1121_hwmon_chip_info = {
+ .ops = &m88e1121_hwmon_hwmon_ops,
+ .info = m88e1121_hwmon_info,
+};
+
+static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
+{
+ int ret;
+
+ *temp = 0;
+
+ mutex_lock(&phydev->lock);
+
+ ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+ if (ret < 0)
+ goto error;
+
+ ret = phy_read(phydev, MII_88E1510_TEMP_SENSOR);
+ if (ret < 0)
+ goto error;
+
+ *temp = ((ret & MII_88E1510_TEMP_SENSOR_MASK) - 25) * 1000;
+
+error:
+ phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+ mutex_unlock(&phydev->lock);
+
+ return ret;
+}
+
+int m88e1510_get_temp_critical(struct phy_device *phydev, long *temp)
+{
+ int ret;
+
+ *temp = 0;
+
+ mutex_lock(&phydev->lock);
+
+ ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+ if (ret < 0)
+ goto error;
+
+ ret = phy_read(phydev, MII_88E1121_MISC_TEST);
+ if (ret < 0)
+ goto error;
+
+ *temp = (((ret & MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK) >>
+ MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT) * 5) - 25;
+ /* convert to mC */
+ *temp *= 1000;
+
+error:
+ phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+ mutex_unlock(&phydev->lock);
+
+ return ret;
+}
+
+int m88e1510_set_temp_critical(struct phy_device *phydev, long temp)
+{
+ int ret;
+
+ mutex_lock(&phydev->lock);
+
+ ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+ if (ret < 0)
+ goto error;
+
+ ret = phy_read(phydev, MII_88E1121_MISC_TEST);
+ if (ret < 0)
+ goto error;
+
+ temp = temp / 1000;
+ temp = clamp_val(DIV_ROUND_CLOSEST(temp, 5) + 5, 0, 0x1f);
+ ret = phy_write(phydev, MII_88E1121_MISC_TEST,
+ (ret & ~MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK) |
+ (temp << MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT));
+
+error:
+ phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+ mutex_unlock(&phydev->lock);
+
+ return ret;
+}
+
+int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
+{
+ int ret;
+
+ *alarm = false;
+
+ mutex_lock(&phydev->lock);
+
+ ret = phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x6);
+ if (ret < 0)
+ goto error;
+
+ ret = phy_read(phydev, MII_88E1121_MISC_TEST);
+ if (ret < 0)
+ goto error;
+ *alarm = !!(ret & MII_88E1510_MISC_TEST_TEMP_IRQ);
+
+error:
+ phy_write(phydev, MII_M1145_PHY_EXT_ADDR_PAGE, 0x0);
+ mutex_unlock(&phydev->lock);
+
+ return ret;
+}
+
+static int m88e1510_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel, long *temp)
+{
+ struct phy_device *phydev = dev_get_drvdata(dev);
+ int err;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ err = m88e1510_get_temp(phydev, temp);
+ break;
+ case hwmon_temp_crit:
+ err = m88e1510_get_temp_critical(phydev, temp);
+ break;
+ case hwmon_temp_max_alarm:
+ err = m88e1510_get_temp_alarm(phydev, temp);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return err;
+}
+
+static int m88e1510_hwmon_write(struct device *dev,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel, long temp)
+{
+ struct phy_device *phydev = dev_get_drvdata(dev);
+ int err;
+
+ switch (attr) {
+ case hwmon_temp_crit:
+ err = m88e1510_set_temp_critical(phydev, temp);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ return err;
+}
+
+static umode_t m88e1510_hwmon_is_visible(const void *data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ if (type != hwmon_temp)
+ return 0;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ case hwmon_temp_max_alarm:
+ return 0444;
+ case hwmon_temp_crit:
+ return 0644;
+ default:
+ return 0;
+ }
+}
+
+static u32 m88e1510_hwmon_temp_config[] = {
+ HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_MAX_ALARM,
+ 0
+};
+
+static const struct hwmon_channel_info m88e1510_hwmon_temp = {
+ .type = hwmon_temp,
+ .config = m88e1510_hwmon_temp_config,
+};
+
+static const struct hwmon_channel_info *m88e1510_hwmon_info[] = {
+ &m88e1121_hwmon_chip,
+ &m88e1510_hwmon_temp,
+ NULL
+};
+
+static const struct hwmon_ops m88e1510_hwmon_hwmon_ops = {
+ .is_visible = m88e1510_hwmon_is_visible,
+ .read = m88e1510_hwmon_read,
+ .write = m88e1510_hwmon_write,
+};
+
+static const struct hwmon_chip_info m88e1510_hwmon_chip_info = {
+ .ops = &m88e1510_hwmon_hwmon_ops,
+ .info = m88e1510_hwmon_info,
+};
+
+static int marvell_hwmon_name(struct phy_device *phydev)
+{
+ struct marvell_priv *priv = phydev->priv;
+ struct device *dev = &phydev->mdio.dev;
+ const char *devname = dev_name(dev);
+ size_t len = strlen(devname);
+ int i, j;
+
+ priv->hwmon_name = devm_kzalloc(dev, len, GFP_KERNEL);
+ if (!priv->hwmon_name)
+ return -ENOMEM;
+
+ for (i = j = 0; i < len && devname[i]; i++) {
+ if (isalnum(devname[i]))
+ priv->hwmon_name[j++] = devname[i];
+ }
+
+ return 0;
+}
+
+static int marvell_hwmon_probe(struct phy_device *phydev,
+ const struct hwmon_chip_info *chip)
+{
+ struct marvell_priv *priv = phydev->priv;
+ struct device *dev = &phydev->mdio.dev;
+ int err;
+
+ err = marvell_hwmon_name(phydev);
+ if (err)
+ return err;
+
+ priv->hwmon_dev = devm_hwmon_device_register_with_info(
+ dev, priv->hwmon_name, phydev, chip, NULL);
+
+ return PTR_ERR_OR_ZERO(priv->hwmon_dev);
+}
+
+static int m88e1121_hwmon_probe(struct phy_device *phydev)
+{
+ return marvell_hwmon_probe(phydev, &m88e1121_hwmon_chip_info);
+}
+
+static int m88e1510_hwmon_probe(struct phy_device *phydev)
+{
+ return marvell_hwmon_probe(phydev, &m88e1510_hwmon_chip_info);
+}
+#else
+static int m88e1121_hwmon_probe(struct phy_device *phydev)
+{
+ return 0;
+}
+
+static int m88e1510_hwmon_probe(struct phy_device *phydev)
+{
+ return 0;
+}
+#endif
+
static int marvell_probe(struct phy_device *phydev)
{
struct marvell_priv *priv;
@@ -1481,6 +1861,39 @@ static int marvell_probe(struct phy_device *phydev)
return 0;
}
+static int m88e1121_probe(struct phy_device *phydev)
+{
+ int err;
+
+ err = marvell_probe(phydev);
+ if (err)
+ return err;
+
+ return m88e1121_hwmon_probe(phydev);
+}
+
+static int m88e1510_probe(struct phy_device *phydev)
+{
+ int err;
+
+ err = marvell_probe(phydev);
+ if (err)
+ return err;
+
+ return m88e1510_hwmon_probe(phydev);
+}
+
+static void marvell_remove(struct phy_device *phydev)
+{
+#ifdef CONFIG_HWMON
+
+ struct marvell_priv *priv = phydev->priv;
+
+ if (priv && priv->hwmon_dev)
+ hwmon_device_unregister(priv->hwmon_dev);
+#endif
+}
+
static struct phy_driver marvell_drivers[] = {
{
.phy_id = MARVELL_PHY_ID_88E1101,
@@ -1558,9 +1971,10 @@ static struct phy_driver marvell_drivers[] = {
.phy_id = MARVELL_PHY_ID_88E1121R,
.phy_id_mask = MARVELL_PHY_ID_MASK,
.name = "Marvell 88E1121R",
+ .probe = &m88e1121_probe,
+ .remove = &marvell_remove,
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .probe = marvell_probe,
.config_init = &m88e1121_config_init,
.config_aneg = &m88e1121_config_aneg,
.read_status = &marvell_read_status,
@@ -1671,8 +2085,10 @@ static struct phy_driver marvell_drivers[] = {
.phy_id_mask = MARVELL_PHY_ID_MASK,
.name = "Marvell 88E1510",
.features = PHY_GBIT_FEATURES | SUPPORTED_FIBRE,
+ .probe = &m88e1510_probe,
+ .remove = &marvell_remove,
+ .features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .probe = marvell_probe,
.config_init = &m88e1510_config_init,
.config_aneg = &m88e1510_config_aneg,
.read_status = &marvell_read_status,
@@ -1689,9 +2105,10 @@ static struct phy_driver marvell_drivers[] = {
.phy_id = MARVELL_PHY_ID_88E1540,
.phy_id_mask = MARVELL_PHY_ID_MASK,
.name = "Marvell 88E1540",
+ .probe = m88e1510_probe,
+ .remove = &marvell_remove,
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .probe = marvell_probe,
.config_init = &marvell_config_init,
.config_aneg = &m88e1510_config_aneg,
.read_status = &marvell_read_status,
--
2.11.0
^ permalink raw reply related
* [PATCH net] net: mpls: Fix multipath selection for LSR use case
From: David Ahern @ 2017-01-20 0:51 UTC (permalink / raw)
To: netdev; +Cc: rshearma, roopa, David Ahern
MPLS multipath for LSR is broken -- always selecting the first nexthop
in the one label case. For example:
$ ip netns exec ns1 ip -f mpls ro ls
100
nexthop as to 200 via inet 172.16.2.2 dev virt12
nexthop as to 300 via inet 172.16.3.2 dev virt13
101
nexthop as to 201 via inet6 2000:2::2 dev virt12
nexthop as to 301 via inet6 2000:3::2 dev virt13
In this example incoming packets have a single MPLS labels which means
BOS bit is set. The BOS bit is passed from mpls_forward down to
mpls_multipath_hash which never processes the hash loop because BOS is 1.
Removing the bos arg from mpls_multipath_hash uncovers a number of other
problems with the hash loop that processes the MPLS label stack -- from
incorrect assumptions on the skb (skb has already pulled the first mpls
label in mpls_forward yet loop assumes it is there) to incorrect
pskb_may_pull checks (label_index starts at 0 and pskb_may_pull checks
all use sizeof() * label_index).
This patch addresses all problems by moving the skb_pull in mpls_forward
after mpls_select_multipath. This allows mpls_multipath_hash to see the
skb with the entire label stack as it arrived.
>From there mpls_multipath_hash is modified to additively compute the
total mpls header length on each pass (on pass N mpls_hdr_len is
N * sizeof(mpls_shim_hdr)). When the label is found with the BOS set it
verifies the skb has sufficient header for ipv4 or ipv6, and find the
IPv4 and IPv6 header by using the last mpls_hdr pointer and adding 1 to
advance past it.
With these changes I have verified the code correctly sees the label,
BOS, IPv4 and IPv6 addresses in the network header and icmp/tcp/udp
traffic for ipv4 and ipv6 are distributed across the nexthops.
Fixes: 1c78efa8319ca ("mpls: flow-based multipath selection")
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
net/mpls/af_mpls.c | 48 +++++++++++++++++++++++++-----------------------
1 file changed, 25 insertions(+), 23 deletions(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 4dc81963af8f..64d3bf269a26 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -119,18 +119,19 @@ void mpls_stats_inc_outucastpkts(struct net_device *dev,
}
EXPORT_SYMBOL_GPL(mpls_stats_inc_outucastpkts);
-static u32 mpls_multipath_hash(struct mpls_route *rt,
- struct sk_buff *skb, bool bos)
+static u32 mpls_multipath_hash(struct mpls_route *rt, struct sk_buff *skb)
{
struct mpls_entry_decoded dec;
+ unsigned int mpls_hdr_len = 0;
struct mpls_shim_hdr *hdr;
bool eli_seen = false;
int label_index;
u32 hash = 0;
- for (label_index = 0; label_index < MAX_MP_SELECT_LABELS && !bos;
+ for (label_index = 0; label_index < MAX_MP_SELECT_LABELS;
label_index++) {
- if (!pskb_may_pull(skb, sizeof(*hdr) * label_index))
+ mpls_hdr_len += sizeof(*hdr);
+ if (!pskb_may_pull(skb, mpls_hdr_len))
break;
/* Read and decode the current label */
@@ -155,37 +156,38 @@ static u32 mpls_multipath_hash(struct mpls_route *rt,
eli_seen = true;
}
- bos = dec.bos;
- if (bos && pskb_may_pull(skb, sizeof(*hdr) * label_index +
- sizeof(struct iphdr))) {
+ if (!dec.bos)
+ continue;
+
+ /* found bottom label; does skb have room for a header? */
+ if (pskb_may_pull(skb, mpls_hdr_len + sizeof(struct iphdr))) {
const struct iphdr *v4hdr;
- v4hdr = (const struct iphdr *)(mpls_hdr(skb) +
- label_index);
+ v4hdr = (const struct iphdr *)(hdr + 1);
if (v4hdr->version == 4) {
hash = jhash_3words(ntohl(v4hdr->saddr),
ntohl(v4hdr->daddr),
v4hdr->protocol, hash);
} else if (v4hdr->version == 6 &&
- pskb_may_pull(skb, sizeof(*hdr) * label_index +
- sizeof(struct ipv6hdr))) {
+ pskb_may_pull(skb, mpls_hdr_len +
+ sizeof(struct ipv6hdr))) {
const struct ipv6hdr *v6hdr;
- v6hdr = (const struct ipv6hdr *)(mpls_hdr(skb) +
- label_index);
-
+ v6hdr = (const struct ipv6hdr *)(hdr + 1);
hash = __ipv6_addr_jhash(&v6hdr->saddr, hash);
hash = __ipv6_addr_jhash(&v6hdr->daddr, hash);
hash = jhash_1word(v6hdr->nexthdr, hash);
}
}
+
+ break;
}
return hash;
}
static struct mpls_nh *mpls_select_multipath(struct mpls_route *rt,
- struct sk_buff *skb, bool bos)
+ struct sk_buff *skb)
{
int alive = ACCESS_ONCE(rt->rt_nhn_alive);
u32 hash = 0;
@@ -201,7 +203,7 @@ static struct mpls_nh *mpls_select_multipath(struct mpls_route *rt,
if (alive <= 0)
return NULL;
- hash = mpls_multipath_hash(rt, skb, bos);
+ hash = mpls_multipath_hash(rt, skb);
nh_index = hash % alive;
if (alive == rt->rt_nhn)
goto out;
@@ -308,22 +310,22 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
hdr = mpls_hdr(skb);
dec = mpls_entry_decode(hdr);
- /* Pop the label */
- skb_pull(skb, sizeof(*hdr));
- skb_reset_network_header(skb);
-
- skb_orphan(skb);
-
rt = mpls_route_input_rcu(net, dec.label);
if (!rt) {
MPLS_INC_STATS(mdev, rx_noroute);
goto drop;
}
- nh = mpls_select_multipath(rt, skb, dec.bos);
+ nh = mpls_select_multipath(rt, skb);
if (!nh)
goto err;
+ /* Pop the label */
+ skb_pull(skb, sizeof(*hdr));
+ skb_reset_network_header(skb);
+
+ skb_orphan(skb);
+
if (skb_warn_if_lro(skb))
goto err;
--
2.1.4
^ permalink raw reply related
* Re: [PATCH v2 0/2] net: dsa: Move temperature sensor code into PHY.
From: Andrew Lunn @ 2017-01-20 0:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Vivien Didelot
In-Reply-To: <1484872670-32585-1-git-send-email-andrew@lunn.ch>
Hi Dave
Sorry, i forget the net-next in the subject line.
Andrew
^ permalink raw reply
* Re: [PATCH v3 net-next 1/2] net/ipv6: allow sysctl to change link-local address generation mode
From: Stephen Hemminger @ 2017-01-20 1:27 UTC (permalink / raw)
To: Felix Jia; +Cc: network dev, Hermin Anggawijaya
In-Reply-To: <20170119224619.18656-1-felix.jia@alliedtelesis.co.nz>
On Fri, 20 Jan 2017 11:46:19 +1300
Felix Jia <felix.jia@alliedtelesis.co.nz> wrote:
> diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h
> index 0fa4c324b713..48e2a34a6ed3 100644
> --- a/include/net/if_inet6.h
> +++ b/include/net/if_inet6.h
> @@ -1,9 +1,9 @@
> /*
> * inet6 interface/address list definitions
> - * Linux INET6 implementation
> + * Linux INET6 implementation
> *
> * Authors:
> - * Pedro Roque <roque@di.fc.ul.pt>
> + * Pedro Roque <roque@di.fc.ul.pt>
Please don't add unnecessary whitespace changes to your patch.
^ permalink raw reply
* Setting skb->csum in shared skb with csum-complete
From: Tom Herbert @ 2017-01-20 1:29 UTC (permalink / raw)
To: Herbert Xu, Linux Kernel Network Developers
Hi Herbert,
I am looking at "net: Fix skb csum races when peeking". The context is
that I'm seeing checksum faults after backporting mlx5 from driver.
The handling of the checksum faults in interesting in itself. One side
effect is that we end up recalculating and doing the dump twice when
GRO is enabled. Another is that we validate checksum complete in a
shared skbuf with a knowingly bad skb->csum value.
In the checksum fault case we recalculate the checksum and find it was
actually correct in the packet. If the skb is not shared then we set
the skb->csum to the correct value and also set skb->csum_complete_sw
and skb->csum_valid. But if skb is shared we don't do any of that.
This is kind of odd since __skb_checksum_complete_head then returns
zero for an skb with checksum_complete where the checksum complete
value is not correct. I'm not sure what the consequences of doing that
are (I don't think it's related to issues I'm seeing) but this doesn't
seem like a good thing. Maybe we should set skb->csum in this case
since we know it's already wrong and the fact that skbuf is shared
can't make it more wrong...
Tom
^ permalink raw reply
* Re: [PATCH v5 1/2] net: dsa: mv88e6xxx: Don't forbid MDIO I/Os for PHY addr >= num_of_ports
From: Vivien Didelot @ 2017-01-20 1:54 UTC (permalink / raw)
To: Andrew Lunn
Cc: Gregory CLEMENT, Florian Fainelli, netdev, linux-kernel,
David S. Miller, Jason Cooper, Sebastian Hesselbarth,
Thomas Petazzoni, linux-arm-kernel, Nadav Haklai, Wilson Ding,
Kostya Porotchkin, Joe Zhou, Jon Pannell
In-Reply-To: <20170120000645.GA31483@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
>> Isn't there an hardware table used to map the PHY addresses on such chip?
>
> The 6390 has something like this. But if we can avoid it, lets keep it
> KISS.
I definitely agree. I was just curious about this new chip.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH v5 2/2] net: dsa: mv88e6xxx: Add support for ethernet switch 88E6341
From: Vivien Didelot @ 2017-01-20 1:55 UTC (permalink / raw)
To: Andrew Lunn
Cc: Gregory CLEMENT, Florian Fainelli, netdev, linux-kernel,
David S. Miller, Jason Cooper, Sebastian Hesselbarth,
Thomas Petazzoni, linux-arm-kernel, Nadav Haklai, Wilson Ding,
Kostya Porotchkin, Joe Zhou, Jon Pannell
In-Reply-To: <20170120001203.GB31483@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
>> Maybe I missed it, Andrew, can you confirm that 6341 is a proper Marvell
>> family of switch chips?
>
> My understand is that it is. Marvell have not added it to DSDT. There
> is a new SDK called UMSD, also GPLv2 and BSD. They call this family
> Topaz, and the 6390 is Peridot.
At least we could rename it anytime. They don't really make sense nor or
reliable anyway...
Thanks,
Vivien
^ permalink raw reply
* Re: Setting skb->csum in shared skb with csum-complete
From: Eric Dumazet @ 2017-01-20 2:00 UTC (permalink / raw)
To: Tom Herbert; +Cc: Herbert Xu, Linux Kernel Network Developers
In-Reply-To: <CALx6S34ZfFzPOmzfLoverXQ215AvasYejyJUv8wtWAVRumi-eg@mail.gmail.com>
On Thu, 2017-01-19 at 17:29 -0800, Tom Herbert wrote:
> Hi Herbert,
>
> I am looking at "net: Fix skb csum races when peeking". The context is
> that I'm seeing checksum faults after backporting mlx5 from driver.
> The handling of the checksum faults in interesting in itself. One side
> effect is that we end up recalculating and doing the dump twice when
> GRO is enabled. Another is that we validate checksum complete in a
> shared skbuf with a knowingly bad skb->csum value.
>
> In the checksum fault case we recalculate the checksum and find it was
> actually correct in the packet. If the skb is not shared then we set
> the skb->csum to the correct value and also set skb->csum_complete_sw
> and skb->csum_valid. But if skb is shared we don't do any of that.
> This is kind of odd since __skb_checksum_complete_head then returns
> zero for an skb with checksum_complete where the checksum complete
> value is not correct. I'm not sure what the consequences of doing that
> are (I don't think it's related to issues I'm seeing) but this doesn't
> seem like a good thing. Maybe we should set skb->csum in this case
> since we know it's already wrong and the fact that skbuf is shared
> can't make it more wrong...
Why GRO can not change skb fields ?
skb can not be shared at GRO layer, so really this should be done there
if you want to avoid a double checksumming later.
You added all these skb fields, you know better than us ;)
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Timur Tabi @ 2017-01-20 2:38 UTC (permalink / raw)
To: Zefir Kurtisi, netdev; +Cc: andrew, f.fainelli
In-Reply-To: <759d6323-ffac-ebe8-b197-4e1165be5673@neratec.com>
Zefir Kurtisi wrote:
> It always operates at 675MHz, which with two lines gives 1.25Gbps,
> which at 10/8 coding gives exactly 1Gbps net data rate. If the
> at803x's copper side autonegotiates to 1Gbps, the bits traversing
> over the SGMII match the copper side 1:1. In case the copper side
> autonegotiates to e.g. 100Mbps, each bit at the copper side on the
> SGMII bus is replicated and sent 10x times - or 100x times in case of
> 10Mbps. The MAC side of the ETH needs to be aware of how the SGMII
> data has to be interpreted, and this is why you have to set the bits
> you are referring to.
So does this mean that the SGMII link should not be autonegotiated? I
currently have this code:
if (phydev->autoneg == AUTONEG_ENABLE) {
val &= ~(FORCE_AN_RX_CFG | FORCE_AN_TX_CFG);
val |= AN_ENABLE;
writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
} else {
...
So if the external PHY is set to autonegotiate, then the SGMII block is
set to also negotiate. Now that I think about it, this seems wrong.
And in fact, I'm not sure how it works. It seems that the this only
makes sense if the SGMII block is configured to act as the only PHY.
This is an option that the hardware supports but my driver does not. So
perhaps I should remove this part, and just do the rest:
u32 speed_cfg;
switch (phydev->speed) {
case SPEED_10:
speed_cfg = SPDMODE_10;
break;
case SPEED_100:
speed_cfg = SPDMODE_100;
break;
case SPEED_1000:
speed_cfg = SPDMODE_1000;
break;
default:
return -EINVAL;
}
if (phydev->duplex == DUPLEX_FULL)
speed_cfg |= DUPLEX_MODE;
val &= ~AN_ENABLE;
writel(speed_cfg, phy->base + EMAC_SGMII_PHY_SPEED_CFG1);
writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
Should I be doing this all the time?
> To track down who is causing the additional message, I would proceed
> with following technique that helped me dig down a similar problem:
> since you control the events in question and there is no risk of
> flooding the kernel log, in the top of phy.c::phy_print_status() add
> a dump_stack() call. In the debug log ensure that all of the traces
> end up in the same phydev->adjust_link() callback handler (in your
> case emac_adjust_link()).
That's a good idea, thanks.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply
* Re: Potential issues (security and otherwise) with the current cgroup-bpf API
From: Alexei Starovoitov @ 2017-01-20 2:39 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tejun Heo, Michal Hocko, Peter Zijlstra, David Ahern,
Andy Lutomirski, Daniel Mack, Mickaël Salaün, Kees Cook,
Jann Horn, David S. Miller, Thomas Graf, Michael Kerrisk,
Linux API, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Network Development
In-Reply-To: <CALCETrVrEnL4cvkdDu2LUhxmeOZ+SMEmF=yKKsm9OYoW2y1Kpg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jan 18, 2017 at 06:29:22PM -0800, Andy Lutomirski wrote:
> I think it could work by making a single socket cgroup controller that
> handles all cgroup things that are bound to a socket. Using
Such 'socket cgroup controller' would limit usability of the feature
to sockets and force all other use cases like landlock to invent
their own wheel, which is undesirable. Everyone will be
inventing new 'foo cgroup controller', while all of them
are really bpf features. They are different bpf program
types that attach to different hooks and use cgroup for scoping.
> Having thought about this some more, I think that making it would
> alleviate a bunch of my concerns, as it would make the semantics if
> the capable() check were relaxed to ns_capable() be sane. Here's what
here we're on the same page. For any meaningful discussion about
'bpf cgroup controller' to happen bpf itself needs to become
delegatable in cgroup sense. In other words BPF_PROG_TYPE_CGROUP*
program types need to become available for unprivileged users.
The only unprivileged prog type today is BPF_PROG_TYPE_SOCKET_FILTER.
To make it secure we severely limited its functionality.
All bpf advances since then (like new map types and verifier extensions)
were done for root only. If early on the priv vs unpriv bpf features
were 80/20. Now it's close to 95/5. No work has been done to
make socket filter type more powerful. It still has to use
slow-ish ld_abs skb access while tc/xdp have direct packet access.
Things like register value tracking is root only as well and so on
and so forth.
We cannot just flip the switch and allow type_cgroup* to unpriv
and I don't see any volunteers willing to do this work.
Until that happens there is no point coming up with designs
for 'cgroup bpf controller'... whatever that means.
> I currently should happen before bpf+cgroup is enabled in a release:
>
> 1. Make it netns-aware. This could be as simple as making it only
> work in the root netns because then real netns awareness can be added
> later without breaking anything. The current situation is bad in that
> network namespaces are just ignored and it's plausible that people
> will start writing user code that depends on having network namespaces
> be ignored.
nothing in bpf today is netns-aware and frankly I don't see
how cgroup+bpf has anything to do with netns.
For regular sockets+bpf we don't check netns.
When tcpdump opens raw socket and attaches bpf there are no netns
checks, since socket itself gives a scope for the program to run.
Same thing applies to cgroup+bpf. cgroup gives a scope for the program.
But, say, we indeed add 'if !root ns' check to BPF_CGROUP_INET_*
hooks. Then if the hooks are used for security, the process
only needs to do setns() to escape security sandbox. Obviously
broken semantics.
> 2. Make it inherit properly. Inner cgroups should not override outer
> hooks. As in (1), this could be simplified by preventing the same
> hook from being configured in both an ancestor and a descendent
> cgroup. Then inheritance could be added for real later on.
In general it sounds fine, but it seems the reasoning to add
such restriction now (instead of later), so that program chain can
be added without breaking abi, since if we don't restrict it now
there will be no way to add it without breaking abi?!
That is incorrect assumption. We can add chaining and can add
'do not override' logic without breaking existing semantics.
For example, we can add 'priority' field to
struct { /* anonymous struct used by BPF_PROG_ATTACH/DETACH commands */}
which would indicate relative position of multiple chained programs
applied to the same cgroup+hook pair. Multiple programs with
the same priority will be executed in the order they were added.
Programs with different priorities will execute in the priority order.
Such scheme will be more generic and flexible than earlier proposals.
Similarly we can add another flag that will say 'dissallow override
of bpf program in descendent cgroup'. It's all trivial to do,
since bpf syscall was designed for extensibility.
Also until bpf_type_cgroup* becomes unprivileged there is no reason
to add this 'priority/prog chaining' feature, since if it's
used for security the root can always override it no matter cgroup
hierarchy.
> 3. Give cgroup delegation support some serious thought. In
> particular, if delegation would be straightforward but the current API
> wouldn't work well with delegation, then at least consider whether the
> API should change before it becomes stable so that two APIs don't need
> to be supported going forward.
please see example above. Since we went with bpf syscall (instead of
inextensible ioctl) we can add any new cgroup+bpf logic without
breaking current abi.
No matter how you twist it the cgroup+bpf is bpf specific feature.
^ permalink raw reply
* Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags
From: Larry Finger @ 2017-01-20 2:41 UTC (permalink / raw)
To: Bharat Kumar Gogada, chaoming_li, linux-wireless, linux-kernel
Cc: kvalo, netdev, rgummal, Bharat Kumar Gogada
In-Reply-To: <1484820854-16719-1-git-send-email-bharatku@xilinx.com>
On 01/19/2017 04:14 AM, Bharat Kumar Gogada wrote:
> -Realtek 8192CE chipset maintains local irq flags after enabling/disabling
> hardware interrupts.
> -Hardware interrupts are enabled before enabling the local irq
> flags(these flags are being checked in interrupt handler),
> leading to race condition on some RP, where the irq line between
> bridge and GIC goes high at ASSERT_INTx and goes low only
> at DEASSERT_INTx. In this kind of RP by the time ASSERT_INTx is seen
> irq_enable flag is still set to false, resulting in continuous
> interrupts seen by CPU as DEASSERT_INTx cannot be sent since
> flag is still false and making CPU stall.
> -Changing the sequence of setting these irq flags.
>
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
This patch should be enhanced with the smb_xx() calls as suggested by by Lino.
The subject should be changed. I would suggest something like "rtlwifi:
rtl8192ce: Prevent race condition when enabling interrupts", as it explains the
condition you are preventing.
The other PCI drivers also have the same problem. Do you want to prepare the
patches, or should I do it?
Larry
^ permalink raw reply
* Re: [net PATCH v5 6/6] virtio_net: XDP support for adjust_head
From: Jason Wang @ 2017-01-20 3:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: John Fastabend, john.r.fastabend, netdev, alexei.starovoitov,
daniel
In-Reply-To: <20170119231027-mutt-send-email-mst@kernel.org>
On 2017年01月20日 05:11, Michael S. Tsirkin wrote:
> On Thu, Jan 19, 2017 at 11:05:40AM +0800, Jason Wang wrote:
>>
>> On 2017年01月18日 23:15, Michael S. Tsirkin wrote:
>>> On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
>>>> Add support for XDP adjust head by allocating a 256B header region
>>>> that XDP programs can grow into. This is only enabled when a XDP
>>>> program is loaded.
>>>>
>>>> In order to ensure that we do not have to unwind queue headroom push
>>>> queue setup below bpf_prog_add. It reads better to do a prog ref
>>>> unwind vs another queue setup call.
>>>>
>>>> At the moment this code must do a full reset to ensure old buffers
>>>> without headroom on program add or with headroom on program removal
>>>> are not used incorrectly in the datapath. Ideally we would only
>>>> have to disable/enable the RX queues being updated but there is no
>>>> API to do this at the moment in virtio so use the big hammer. In
>>>> practice it is likely not that big of a problem as this will only
>>>> happen when XDP is enabled/disabled changing programs does not
>>>> require the reset. There is some risk that the driver may either
>>>> have an allocation failure or for some reason fail to correctly
>>>> negotiate with the underlying backend in this case the driver will
>>>> be left uninitialized. I have not seen this ever happen on my test
>>>> systems and for what its worth this same failure case can occur
>>>> from probe and other contexts in virtio framework.
>>>>
>>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>> I've been thinking about it - can't we drop
>>> old buffers without the head room which were posted before
>>> xdp attached?
>>>
>>> Avoiding the reset would be much nicer.
>>>
>>> Thoughts?
>>>
>> As been discussed before, device may use them in the same time so it's not
>> safe. Or do you mean detect them after xdp were set and drop the buffer
>> without head room, this looks sub-optimal.
>>
>> Thanks
> Yes, this is what I mean. Why is this suboptimal? It's a single branch
> in code. Yes we might lose some packets but the big hammer of device
> reset will likely lose more.
>
Maybe I was wrong but I think driver should try their best to avoid
dropping packets. (And look at mlx4, it did something similar to this
patch).
Thanks
^ permalink raw reply
* Re: [PATCH v2 0/2] net: dsa: Move temperature sensor code into PHY.
From: Vivien Didelot @ 2017-01-20 3:26 UTC (permalink / raw)
To: Andrew Lunn, David Miller; +Cc: netdev
In-Reply-To: <20170120004328.GA381@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> Sorry, i forget the net-next in the subject line.
Maybe use this opportunity to resend this thread with the good subject
prefix and the people involved in v1?
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit
From: Jason Wang @ 2017-01-20 3:29 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Rolf Neugebauer, netdev, davem, rppt
In-Reply-To: <20170120002916-mutt-send-email-mst@kernel.org>
On 2017年01月20日 06:30, Michael S. Tsirkin wrote:
> On Thu, Jan 19, 2017 at 11:06:59AM +0800, Jason Wang wrote:
>>
>> On 2017年01月18日 23:18, Michael S. Tsirkin wrote:
>>> On Wed, Jan 18, 2017 at 11:54:21AM +0800, Jason Wang wrote:
>>>> On 2017年01月18日 02:27, Michael S. Tsirkin wrote:
>>>>> On Tue, Jan 17, 2017 at 06:13:51PM +0000, Rolf Neugebauer wrote:
>>>>>> This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
>>>>>> subtle change in how the virtio_net flags are derived from the SKBs
>>>>>> ip_summed field.
>>>>>>
>>>>>> With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
>>>>>> when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
>>>>>> ip_summed == CHECKSUM_NONE, which should be the same.
>>>>>>
>>>>>> Further, the virtio spec 1.0 / CS04 explicitly says that
>>>>>> VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
>>>>>>
>>>>>> Signed-off-by: Rolf Neugebauer<rolf.neugebauer@docker.com>
>>>>> Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
>>>>> Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
>>>>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>>>>>
>>>>> Should be backported into stable as well.
>>>> Looks like a side effect is that we will never see this on receive path?
>>> I thought receive path is using virtio_net_hdr_to_skb - isn't it?
>>>
>> The problem is tun use this on xmit.
>>
>> Thanks
> Oh I missed that. I agree, let's add a flag that allows this.
>
Will post a fix soon.
Thanks
^ permalink raw reply
* Re: [net PATCH v5 6/6] virtio_net: XDP support for adjust_head
From: John Fastabend @ 2017-01-20 3:38 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang
Cc: john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <20170119231027-mutt-send-email-mst@kernel.org>
On 17-01-19 01:11 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 19, 2017 at 11:05:40AM +0800, Jason Wang wrote:
>>
>>
>> On 2017年01月18日 23:15, Michael S. Tsirkin wrote:
>>> On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
>>>> Add support for XDP adjust head by allocating a 256B header region
>>>> that XDP programs can grow into. This is only enabled when a XDP
>>>> program is loaded.
>>>>
>>>> In order to ensure that we do not have to unwind queue headroom push
>>>> queue setup below bpf_prog_add. It reads better to do a prog ref
>>>> unwind vs another queue setup call.
>>>>
>>>> At the moment this code must do a full reset to ensure old buffers
>>>> without headroom on program add or with headroom on program removal
>>>> are not used incorrectly in the datapath. Ideally we would only
>>>> have to disable/enable the RX queues being updated but there is no
>>>> API to do this at the moment in virtio so use the big hammer. In
>>>> practice it is likely not that big of a problem as this will only
>>>> happen when XDP is enabled/disabled changing programs does not
>>>> require the reset. There is some risk that the driver may either
>>>> have an allocation failure or for some reason fail to correctly
>>>> negotiate with the underlying backend in this case the driver will
>>>> be left uninitialized. I have not seen this ever happen on my test
>>>> systems and for what its worth this same failure case can occur
>>>> from probe and other contexts in virtio framework.
>>>>
>>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>> I've been thinking about it - can't we drop
>>> old buffers without the head room which were posted before
>>> xdp attached?
>>>
>>> Avoiding the reset would be much nicer.
>>>
>>> Thoughts?
>>>
>>
>> As been discussed before, device may use them in the same time so it's not
>> safe. Or do you mean detect them after xdp were set and drop the buffer
>> without head room, this looks sub-optimal.
>>
>> Thanks
>
> Yes, this is what I mean. Why is this suboptimal? It's a single branch
> in code. Yes we might lose some packets but the big hammer of device
> reset will likely lose more.
>
Maybe I'm not following, is the suggestion to drop the packets after XDP
is setup for all outstanding buffers until we have done a reallocation of
all the buffers? In this case we can't just detach the buffers we have to
wait until the backend retires them by using them correct?
But when XDP setup call returns we need to guarantee that buffers and
driver are setup. Otherwise the next n packets get dropped in the future.
If there is no traffic currently this could be at some undetermined point
in the future. This will be very buggy.
Did I miss something?
Thanks,
John
^ permalink raw reply
* Re: [net PATCH v5 6/6] virtio_net: XDP support for adjust_head
From: John Fastabend @ 2017-01-20 3:39 UTC (permalink / raw)
To: Jason Wang, Michael S. Tsirkin
Cc: john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <d1c43849-0da9-5f03-1eae-09a27c2e9985@redhat.com>
On 17-01-19 07:26 PM, Jason Wang wrote:
>
>
> On 2017年01月20日 05:11, Michael S. Tsirkin wrote:
>> On Thu, Jan 19, 2017 at 11:05:40AM +0800, Jason Wang wrote:
>>>
>>> On 2017年01月18日 23:15, Michael S. Tsirkin wrote:
>>>> On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
>>>>> Add support for XDP adjust head by allocating a 256B header region
>>>>> that XDP programs can grow into. This is only enabled when a XDP
>>>>> program is loaded.
>>>>>
>>>>> In order to ensure that we do not have to unwind queue headroom push
>>>>> queue setup below bpf_prog_add. It reads better to do a prog ref
>>>>> unwind vs another queue setup call.
>>>>>
>>>>> At the moment this code must do a full reset to ensure old buffers
>>>>> without headroom on program add or with headroom on program removal
>>>>> are not used incorrectly in the datapath. Ideally we would only
>>>>> have to disable/enable the RX queues being updated but there is no
>>>>> API to do this at the moment in virtio so use the big hammer. In
>>>>> practice it is likely not that big of a problem as this will only
>>>>> happen when XDP is enabled/disabled changing programs does not
>>>>> require the reset. There is some risk that the driver may either
>>>>> have an allocation failure or for some reason fail to correctly
>>>>> negotiate with the underlying backend in this case the driver will
>>>>> be left uninitialized. I have not seen this ever happen on my test
>>>>> systems and for what its worth this same failure case can occur
>>>>> from probe and other contexts in virtio framework.
>>>>>
>>>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>>> I've been thinking about it - can't we drop
>>>> old buffers without the head room which were posted before
>>>> xdp attached?
>>>>
>>>> Avoiding the reset would be much nicer.
>>>>
>>>> Thoughts?
>>>>
>>> As been discussed before, device may use them in the same time so it's not
>>> safe. Or do you mean detect them after xdp were set and drop the buffer
>>> without head room, this looks sub-optimal.
>>>
>>> Thanks
>> Yes, this is what I mean. Why is this suboptimal? It's a single branch
>> in code. Yes we might lose some packets but the big hammer of device
>> reset will likely lose more.
>>
>
> Maybe I was wrong but I think driver should try their best to avoid dropping
> packets. (And look at mlx4, it did something similar to this patch).
>
> Thanks
+1 sorry didn't see your reply as I was typing mine. Bottom line when XDP
returns I believe the driver must be ready to accept packets or managing
XDP will be problematic.
.John
^ permalink raw reply
* Re: [PATCH RFC net-next 2/7] net: add dst_pending_confirm flag to skbuff
From: YueHaibing @ 2017-01-20 3:39 UTC (permalink / raw)
To: Julian Anastasov; +Cc: Eric Dumazet, netdev, linux-sctp
In-Reply-To: <alpine.LFD.2.11.1612192203480.3105@ja.home.ssi.bg>
On 2016/12/20 4:37, Julian Anastasov wrote:
>
> Hello,
>
> On Mon, 19 Dec 2016, Eric Dumazet wrote:
>
>> I am still digesting this awesome patch series ;)
>
> Thanks. I don't feel quite comfortable with some
> of the changes (mostly XFRM, dst_confirm usage in CXGB) and
> I hope the discussion can provide adequate solution.
>
>> Not sure why you used an unlikely() here. TCP for example would hit this
>> path quite often.
>
> I was not sure, may be because ACKs can come with lower
> rate than the sent packets. Also because non-TCP rarely uses
> MSG_CONFIRM. If you still think it is better without unlikely,
> I'll remove it.
>
>> So considering sk_dst_pending_confirm might be dirtied quite often,further
>>
>> I am not sure why you placed it in the cache line that contains
>> sk_rx_dst (in 1st patch)
>
> I saw your recent changes and was worried if the
> sk_dst_confirm() calling on RX can cause unwanted dirtying of
> additional cache line. My preliminary analyze pointed
> sk_omem_alloc as good candidate for moving to next cache
> line. I know how critical is to properly place the new flags,
> so I really need recommendations about this.
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
>
> .
>
Sorry for so late reply.
I have test your new patch, It works well in my scene.
Everybody,Is there any further comment about this awesome patch series?
^ permalink raw reply
* Re: Setting skb->csum in shared skb with csum-complete
From: Tom Herbert @ 2017-01-20 3:50 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Herbert Xu, Linux Kernel Network Developers
In-Reply-To: <1484877634.16328.71.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Jan 19, 2017 at 6:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2017-01-19 at 17:29 -0800, Tom Herbert wrote:
>> Hi Herbert,
>>
>> I am looking at "net: Fix skb csum races when peeking". The context is
>> that I'm seeing checksum faults after backporting mlx5 from driver.
>> The handling of the checksum faults in interesting in itself. One side
>> effect is that we end up recalculating and doing the dump twice when
>> GRO is enabled. Another is that we validate checksum complete in a
>> shared skbuf with a knowingly bad skb->csum value.
>>
>> In the checksum fault case we recalculate the checksum and find it was
>> actually correct in the packet. If the skb is not shared then we set
>> the skb->csum to the correct value and also set skb->csum_complete_sw
>> and skb->csum_valid. But if skb is shared we don't do any of that.
>> This is kind of odd since __skb_checksum_complete_head then returns
>> zero for an skb with checksum_complete where the checksum complete
>> value is not correct. I'm not sure what the consequences of doing that
>> are (I don't think it's related to issues I'm seeing) but this doesn't
>> seem like a good thing. Maybe we should set skb->csum in this case
>> since we know it's already wrong and the fact that skbuf is shared
>> can't make it more wrong...
>
> Why GRO can not change skb fields ?
>
Sure, I just noted this fact. We don't typically expect the device to
be giving us miscomputed checksum complete values ;-)
> skb can not be shared at GRO layer, so really this should be done there
> if you want to avoid a double checksumming later.
>
> You added all these skb fields, you know better than us ;)
>
>
>
^ permalink raw reply
* Re: Potential issues (security and otherwise) with the current cgroup-bpf API
From: Andy Lutomirski @ 2017-01-20 4:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Tejun Heo, Michal Hocko, Peter Zijlstra, David Ahern,
Andy Lutomirski, Daniel Mack, Mickaël Salaün, Kees Cook,
Jann Horn, David S. Miller, Thomas Graf, Michael Kerrisk,
Linux API, linux-kernel@vger.kernel.org, Network Development
In-Reply-To: <20170120023907.GA3294@ast-mbp.thefacebook.com>
On Thu, Jan 19, 2017 at 6:39 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Wed, Jan 18, 2017 at 06:29:22PM -0800, Andy Lutomirski wrote:
>> I think it could work by making a single socket cgroup controller that
>> handles all cgroup things that are bound to a socket. Using
>
> Such 'socket cgroup controller' would limit usability of the feature
> to sockets and force all other use cases like landlock to invent
> their own wheel, which is undesirable. Everyone will be
> inventing new 'foo cgroup controller', while all of them
> are really bpf features. They are different bpf program
> types that attach to different hooks and use cgroup for scoping.
Can you elaborate on why that would be a problem? In a cgroup v1
world, users who want different hierarchies for different types of
control could easily want one hierarchy for socket hooks and a
different hierarchy for lsm hooks. In a cgroup v2 delegation world, I
could easily imagine the decision to delegate socket hooks being
different from the decision to delegate lsm hooks. Almost all of the
code would be shared between different bpf-using cgroup controllers.
>
>> Having thought about this some more, I think that making it would
>> alleviate a bunch of my concerns, as it would make the semantics if
>> the capable() check were relaxed to ns_capable() be sane. Here's what
>
> here we're on the same page. For any meaningful discussion about
> 'bpf cgroup controller' to happen bpf itself needs to become
> delegatable in cgroup sense. In other words BPF_PROG_TYPE_CGROUP*
> program types need to become available for unprivileged users.
> The only unprivileged prog type today is BPF_PROG_TYPE_SOCKET_FILTER.
> To make it secure we severely limited its functionality.
> All bpf advances since then (like new map types and verifier extensions)
> were done for root only. If early on the priv vs unpriv bpf features
> were 80/20. Now it's close to 95/5. No work has been done to
> make socket filter type more powerful. It still has to use
> slow-ish ld_abs skb access while tc/xdp have direct packet access.
> Things like register value tracking is root only as well and so on
> and so forth.
> We cannot just flip the switch and allow type_cgroup* to unpriv
> and I don't see any volunteers willing to do this work.
> Until that happens there is no point coming up with designs
> for 'cgroup bpf controller'... whatever that means.
Sure there is. If delegation can be turned on without changing the
API, then the result will be easier to work with and have fewer
compatibility issues.
>
>> I currently should happen before bpf+cgroup is enabled in a release:
>>
>> 1. Make it netns-aware. This could be as simple as making it only
>> work in the root netns because then real netns awareness can be added
>> later without breaking anything. The current situation is bad in that
>> network namespaces are just ignored and it's plausible that people
>> will start writing user code that depends on having network namespaces
>> be ignored.
>
> nothing in bpf today is netns-aware and frankly I don't see
> how cgroup+bpf has anything to do with netns.
> For regular sockets+bpf we don't check netns.
> When tcpdump opens raw socket and attaches bpf there are no netns
> checks, since socket itself gives a scope for the program to run.
> Same thing applies to cgroup+bpf. cgroup gives a scope for the program.
> But, say, we indeed add 'if !root ns' check to BPF_CGROUP_INET_*
> hooks.
Here I completely disagree with you. tcpdump sees packets in its
network namespace. Regular sockets apply bpf filters to the packets
seen by that socket, and the socket itself is scoped to a netns.
Meanwhile, cgroup+bpf actually appears to be buggy in this regard even
regardless of what semantics you think are better. sk_bound_dev_if is
exposed as a u32 value, but sk_bound_dev_if only has meaning within a
given netns. The "ip vrf" stuff will straight-up malfunction if a
process affected by its hook runs in a different netns from the netns
that "ip vrf" was run in.
IOW, the current code is buggy.
> Then if the hooks are used for security, the process
> only needs to do setns() to escape security sandbox. Obviously
> broken semantics.
This could go both ways. If the goal is to filter packets, then it's
not really important to have the filter keep working if the sandboxed
task unshares netns -- in the new netns, there isn't any access to the
network at all. If the goal is to reduce attack surface by
restricting the types of sockets that can be created, then you do want
the filter to work across namespaces, but seccomp can do that too and
the current code doesn't handle netns correctly.
>
>> 2. Make it inherit properly. Inner cgroups should not override outer
>> hooks. As in (1), this could be simplified by preventing the same
>> hook from being configured in both an ancestor and a descendent
>> cgroup. Then inheritance could be added for real later on.
>
> In general it sounds fine, but it seems the reasoning to add
> such restriction now (instead of later), so that program chain can
> be added without breaking abi, since if we don't restrict it now
> there will be no way to add it without breaking abi?!
Adding it the straightforward way (by simply making all the hooks in
scope run) would break ABI. Of course there are more complicated ways
to do it, but those are more complicated and messier. Do actually
have a use case in which overriding of hooks is a good thing? As far
as I can tell, the original version of the patch set didn't have hooks
get overridden by descendents, and I don't know why this changed in
the first place.
>
> Also until bpf_type_cgroup* becomes unprivileged there is no reason
> to add this 'priority/prog chaining' feature, since if it's
> used for security the root can always override it no matter cgroup
> hierarchy.
>
>> 3. Give cgroup delegation support some serious thought. In
>> particular, if delegation would be straightforward but the current API
>> wouldn't work well with delegation, then at least consider whether the
>> API should change before it becomes stable so that two APIs don't need
>> to be supported going forward.
>
> please see example above. Since we went with bpf syscall (instead of
> inextensible ioctl) we can add any new cgroup+bpf logic without
> breaking current abi.
But then you end up with two ABIs. Ideally delegation would just work
-- then programs written now could run unmodified in unprivileged
containers in future kernels.
--Andy
^ permalink raw reply
* Re: [PATCH] Revert "net: qcom/emac: configure the external phy to allow pause frames"
From: David Miller @ 2017-01-20 4:15 UTC (permalink / raw)
To: timur; +Cc: netdev, alokc
In-Reply-To: <1484692279-18289-1-git-send-email-timur@codeaurora.org>
From: Timur Tabi <timur@codeaurora.org>
Date: Tue, 17 Jan 2017 16:31:19 -0600
> This reverts commit 3e884493448131179a5b7cae1ddca1028ffaecc8.
>
> With commit 529ed1275263 ("net: phy: phy drivers should not set
> SUPPORTED_[Asym_]Pause"), phylib now handles automatically enabling
> pause frame support in the PHY, and the MAC driver should follow suit.
>
> Since the EMAC driver driver does this, we no longer need to force
> pause frames support.
>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
Applied to net-next.
^ permalink raw reply
* Re: [PATCH net-next] mlx4: support __GFP_MEMALLOC for rx
From: David Miller @ 2017-01-20 4:36 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, khlebnikov, tariqt
In-Reply-To: <1484712850.13165.86.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 17 Jan 2017 20:14:10 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> Commit 04aeb56a1732 ("net/mlx4_en: allocate non 0-order pages for RX
> ring with __GFP_NOMEMALLOC") added code that appears to be not needed at
> that time, since mlx4 never used __GFP_MEMALLOC allocations anyway.
>
> As using memory reserves is a must in some situations (swap over NFS or
> iSCSI), this patch adds this flag.
>
> Note that this driver does not reuse pages (yet) so we do not have to
> add anything else.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: [PATCH net-next] net: ipv6: Keep nexthop of multipath route on admin down
From: David Miller @ 2017-01-20 4:40 UTC (permalink / raw)
To: dsa; +Cc: netdev
In-Reply-To: <1484754036-26807-1-git-send-email-dsa@cumulusnetworks.com>
From: David Ahern <dsa@cumulusnetworks.com>
Date: Wed, 18 Jan 2017 07:40:36 -0800
> IPv6 deletes route entries associated with multipath routes on an
> admin down where IPv4 does not. For example:
...
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
Seems reasonable, applied, thanks David.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox