* Re: [PATCH net-next 4/4] net/sched: act_mirred: Implement ingress actions
From: Shmulik Ladkani @ 2016-09-25 17:59 UTC (permalink / raw)
To: Cong Wang
Cc: Jamal Hadi Salim, David S. Miller, Eric Dumazet,
Linux Kernel Network Developers
In-Reply-To: <CAM_iQpU+7NDoGGPkkDHA=v5XW5z98GHQhstFg5H=WCyiBzA-eA@mail.gmail.com>
Hi,
On Sat, 24 Sep 2016 17:07:12 -0700 Cong Wang <xiyou.wangcong@gmail.com> wrote:
> One problem to use your code for us is that, the RX side of veth
> is inside containers, not visible to outside, perhaps we need some
> more parameter to tell the netns before the device name/index?
> Thoughts?
Well, this is way trickier...
tc_mirred doesn't cope with netns movement of the target device.
See 'mirred_device_event': upon NETDEV_UNREGISTER the 'tcfm_dev' gets
nullified.
(dev_change_net_namespace sequence includes NETDEV_UNREGISTER,
dev_net_set, NETDEV_REGISTER).
As upposed to veth, which keeps the peer netdev pointer (since veth peers
lifetime is coupled), here in act_mirred we can't easily distinguish a
"real" NETDEV_UNREGISTER vs a namespace change...
^ permalink raw reply
* Re: [PATCH net-next 4/4] net/sched: act_mirred: Implement ingress actions
From: Shmulik Ladkani @ 2016-09-25 17:33 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: David S. Miller, WANG Cong, Eric Dumazet, netdev,
Florian Westphal, Daniel Borkmann
In-Reply-To: <6d2bd45a-a8a0-846d-5934-5e246522cab8@mojatatu.com>
On Sun, 25 Sep 2016 09:05:08 -0400 Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 16-09-23 11:40 AM, Shmulik Ladkani wrote:
> >
> > [off topic]
>
> I think this is still on topic!
Sorry, wasn't too clear on that.
What I meant is that _existing_ "egress redirect" already gets us into
crazy loops - the veth misconfig being just one example of, but
many more exist (many device stacking constructs, with lower dev issuing
an egress-redirect back to the topmost dev).
Point is, IMO loop detection (whether/how addressed), is orthogonal to
this series implementing "ingress redirect", and doesn't seem as a
strict prerequisite to adding the "ingress redirect" functionality to
act_mirred.
We can later address any loop-detection improvements in mirred.
WDYT?
Thanks,
Shmulik
^ permalink raw reply
* [PATCH v2 net-next 3/3] net: bcmgenet: use new api ethtool_{get|set}_link_ksettings
From: Philippe Reynes @ 2016-09-25 16:29 UTC (permalink / raw)
To: f.fainelli, jaedon.shin, davem; +Cc: netdev, linux-kernel, Philippe Reynes
In-Reply-To: <1474820967-1130-1-git-send-email-tremyfr@gmail.com>
The ethtool api {get|set}_settings is deprecated.
We move this driver to new api {get|set}_link_ksettings.
Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 24 ++++++++++--------------
1 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 47d0a2b..2c5d9d3 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -450,32 +450,28 @@ static inline void bcmgenet_rdma_ring_writel(struct bcmgenet_priv *priv,
genet_dma_ring_regs[r]);
}
-static int bcmgenet_get_settings(struct net_device *dev,
- struct ethtool_cmd *cmd)
+static int bcmgenet_get_link_ksettings(struct net_device *dev,
+ struct ethtool_link_ksettings *cmd)
{
- struct bcmgenet_priv *priv = netdev_priv(dev);
-
if (!netif_running(dev))
return -EINVAL;
- if (!priv->phydev)
+ if (!dev->phydev)
return -ENODEV;
- return phy_ethtool_gset(priv->phydev, cmd);
+ return phy_ethtool_ksettings_get(dev->phydev, cmd);
}
-static int bcmgenet_set_settings(struct net_device *dev,
- struct ethtool_cmd *cmd)
+static int bcmgenet_set_link_ksettings(struct net_device *dev,
+ const struct ethtool_link_ksettings *cmd)
{
- struct bcmgenet_priv *priv = netdev_priv(dev);
-
if (!netif_running(dev))
return -EINVAL;
- if (!priv->phydev)
+ if (!dev->phydev)
return -ENODEV;
- return phy_ethtool_sset(priv->phydev, cmd);
+ return phy_ethtool_ksettings_set(dev->phydev, cmd);
}
static int bcmgenet_set_rx_csum(struct net_device *dev,
@@ -983,8 +979,6 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
.get_strings = bcmgenet_get_strings,
.get_sset_count = bcmgenet_get_sset_count,
.get_ethtool_stats = bcmgenet_get_ethtool_stats,
- .get_settings = bcmgenet_get_settings,
- .set_settings = bcmgenet_set_settings,
.get_drvinfo = bcmgenet_get_drvinfo,
.get_link = ethtool_op_get_link,
.get_msglevel = bcmgenet_get_msglevel,
@@ -996,6 +990,8 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
.nway_reset = bcmgenet_nway_reset,
.get_coalesce = bcmgenet_get_coalesce,
.set_coalesce = bcmgenet_set_coalesce,
+ .get_link_ksettings = bcmgenet_get_link_ksettings,
+ .set_link_ksettings = bcmgenet_set_link_ksettings,
};
/* Power down the unimac, based on mode. */
--
1.7.4.4
^ permalink raw reply related
* [PATCH v2 net-next 2/3] Revert "net: ethernet: bcmgenet: use phydev from struct net_device"
From: Philippe Reynes @ 2016-09-25 16:29 UTC (permalink / raw)
To: f.fainelli, jaedon.shin, davem; +Cc: netdev, linux-kernel, Philippe Reynes
In-Reply-To: <1474820967-1130-1-git-send-email-tremyfr@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
This reverts commit 62469c76007e ("net: ethernet: bcmgenet: use phydev
from struct net_device") because it causes GENETv1/2/3 adapters to
expose the following behavior after an ifconfig down/up sequence:
PING fainelli-linux (10.112.156.244): 56 data bytes
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.352 ms
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.472 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.496 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.517 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.536 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.557 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=752.448 ms (DUP!)
This was previously fixed by commit 5dbebbb44a6a ("net: bcmgenet:
Software reset EPHY after power on") but the commit we are reverting was
essentially making this previous commit void, here is why.
Without commit 62469c76007e we would have the following scenario after
an ifconfig down then up sequence:
- bcmgenet_open() calls bcmgenet_power_up() to make sure the PHY is
initialized *before* we get to initialize the UniMAC, this is
critical to ensure the PHY is in a correct state, priv->phydev is
valid, this code executes fine
- second time from bcmgenet_mii_probe(), through the normal
phy_init_hw() call (which arguably could be optimized out)
Everything is fine in that case. With commit 62469c76007e, we would have
the following scenario to happen after an ifconfig down then up
sequence:
- bcmgenet_close() calls phy_disonnect() which makes dev->phydev become
NULL
- when bcmgenet_open() executes again and calls bcmgenet_mii_reset() from
bcmgenet_power_up() to initialize the internal PHY, the NULL check
becomes true, so we do not reset the PHY, yet we keep going on and
initialize the UniMAC, causing MAC activity to occur
- we call bcmgenet_mii_reset() from bcmgenet_mii_probe(), but this is
too late, the PHY is botched, and causes the above bogus pings/packets
transmission/reception to occur
Reported-by: Jaedon Shin <jaedon.shin@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 45 +++++++++++++----------
drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
drivers/net/ethernet/broadcom/genet/bcmmii.c | 24 +++++++------
3 files changed, 39 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 46f9043..47d0a2b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -453,25 +453,29 @@ static inline void bcmgenet_rdma_ring_writel(struct bcmgenet_priv *priv,
static int bcmgenet_get_settings(struct net_device *dev,
struct ethtool_cmd *cmd)
{
+ struct bcmgenet_priv *priv = netdev_priv(dev);
+
if (!netif_running(dev))
return -EINVAL;
- if (!dev->phydev)
+ if (!priv->phydev)
return -ENODEV;
- return phy_ethtool_gset(dev->phydev, cmd);
+ return phy_ethtool_gset(priv->phydev, cmd);
}
static int bcmgenet_set_settings(struct net_device *dev,
struct ethtool_cmd *cmd)
{
+ struct bcmgenet_priv *priv = netdev_priv(dev);
+
if (!netif_running(dev))
return -EINVAL;
- if (!dev->phydev)
+ if (!priv->phydev)
return -ENODEV;
- return phy_ethtool_sset(dev->phydev, cmd);
+ return phy_ethtool_sset(priv->phydev, cmd);
}
static int bcmgenet_set_rx_csum(struct net_device *dev,
@@ -937,7 +941,7 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
e->eee_active = p->eee_active;
e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
- return phy_ethtool_get_eee(dev->phydev, e);
+ return phy_ethtool_get_eee(priv->phydev, e);
}
static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
@@ -954,7 +958,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
if (!p->eee_enabled) {
bcmgenet_eee_enable_set(dev, false);
} else {
- ret = phy_init_eee(dev->phydev, 0);
+ ret = phy_init_eee(priv->phydev, 0);
if (ret) {
netif_err(priv, hw, dev, "EEE initialization failed\n");
return ret;
@@ -964,12 +968,14 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
bcmgenet_eee_enable_set(dev, true);
}
- return phy_ethtool_set_eee(dev->phydev, e);
+ return phy_ethtool_set_eee(priv->phydev, e);
}
static int bcmgenet_nway_reset(struct net_device *dev)
{
- return genphy_restart_aneg(dev->phydev);
+ struct bcmgenet_priv *priv = netdev_priv(dev);
+
+ return genphy_restart_aneg(priv->phydev);
}
/* standard ethtool support functions. */
@@ -996,13 +1002,12 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
static int bcmgenet_power_down(struct bcmgenet_priv *priv,
enum bcmgenet_power_mode mode)
{
- struct net_device *ndev = priv->dev;
int ret = 0;
u32 reg;
switch (mode) {
case GENET_POWER_CABLE_SENSE:
- phy_detach(ndev->phydev);
+ phy_detach(priv->phydev);
break;
case GENET_POWER_WOL_MAGIC:
@@ -1063,6 +1068,7 @@ static void bcmgenet_power_up(struct bcmgenet_priv *priv,
/* ioctl handle special commands that are not present in ethtool. */
static int bcmgenet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
{
+ struct bcmgenet_priv *priv = netdev_priv(dev);
int val = 0;
if (!netif_running(dev))
@@ -1072,10 +1078,10 @@ static int bcmgenet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
case SIOCGMIIPHY:
case SIOCGMIIREG:
case SIOCSMIIREG:
- if (!dev->phydev)
+ if (!priv->phydev)
val = -ENODEV;
else
- val = phy_mii_ioctl(dev->phydev, rq, cmd);
+ val = phy_mii_ioctl(priv->phydev, rq, cmd);
break;
default:
@@ -2458,7 +2464,6 @@ static void bcmgenet_irq_task(struct work_struct *work)
{
struct bcmgenet_priv *priv = container_of(
work, struct bcmgenet_priv, bcmgenet_irq_work);
- struct net_device *ndev = priv->dev;
netif_dbg(priv, intr, priv->dev, "%s\n", __func__);
@@ -2471,7 +2476,7 @@ static void bcmgenet_irq_task(struct work_struct *work)
/* Link UP/DOWN event */
if (priv->irq0_stat & UMAC_IRQ_LINK_EVENT) {
- phy_mac_interrupt(ndev->phydev,
+ phy_mac_interrupt(priv->phydev,
!!(priv->irq0_stat & UMAC_IRQ_LINK_UP));
priv->irq0_stat &= ~UMAC_IRQ_LINK_EVENT;
}
@@ -2833,7 +2838,7 @@ static void bcmgenet_netif_start(struct net_device *dev)
/* Monitor link interrupts now */
bcmgenet_link_intr_enable(priv);
- phy_start(dev->phydev);
+ phy_start(priv->phydev);
}
static int bcmgenet_open(struct net_device *dev)
@@ -2932,7 +2937,7 @@ static void bcmgenet_netif_stop(struct net_device *dev)
struct bcmgenet_priv *priv = netdev_priv(dev);
netif_tx_stop_all_queues(dev);
- phy_stop(dev->phydev);
+ phy_stop(priv->phydev);
bcmgenet_intr_disable(priv);
bcmgenet_disable_rx_napi(priv);
bcmgenet_disable_tx_napi(priv);
@@ -2958,7 +2963,7 @@ static int bcmgenet_close(struct net_device *dev)
bcmgenet_netif_stop(dev);
/* Really kill the PHY state machine and disconnect from it */
- phy_disconnect(dev->phydev);
+ phy_disconnect(priv->phydev);
/* Disable MAC receive */
umac_enable_set(priv, CMD_RX_EN, false);
@@ -3517,7 +3522,7 @@ static int bcmgenet_suspend(struct device *d)
bcmgenet_netif_stop(dev);
- phy_suspend(dev->phydev);
+ phy_suspend(priv->phydev);
netif_device_detach(dev);
@@ -3581,7 +3586,7 @@ static int bcmgenet_resume(struct device *d)
if (priv->wolopts)
clk_disable_unprepare(priv->clk_wol);
- phy_init_hw(dev->phydev);
+ phy_init_hw(priv->phydev);
/* Speed settings must be restored */
bcmgenet_mii_config(priv->dev);
@@ -3614,7 +3619,7 @@ static int bcmgenet_resume(struct device *d)
netif_device_attach(dev);
- phy_resume(dev->phydev);
+ phy_resume(priv->phydev);
if (priv->eee.eee_enabled)
bcmgenet_eee_enable_set(dev, true);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 0f0868c..1e2dc34 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -597,6 +597,7 @@ struct bcmgenet_priv {
/* MDIO bus variables */
wait_queue_head_t wq;
+ struct phy_device *phydev;
bool internal_phy;
struct device_node *phy_dn;
struct device_node *mdio_dn;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index e907acd..457c3bc 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -86,7 +86,7 @@ static int bcmgenet_mii_write(struct mii_bus *bus, int phy_id,
void bcmgenet_mii_setup(struct net_device *dev)
{
struct bcmgenet_priv *priv = netdev_priv(dev);
- struct phy_device *phydev = dev->phydev;
+ struct phy_device *phydev = priv->phydev;
u32 reg, cmd_bits = 0;
bool status_changed = false;
@@ -183,9 +183,9 @@ void bcmgenet_mii_reset(struct net_device *dev)
if (GENET_IS_V4(priv))
return;
- if (dev->phydev) {
- phy_init_hw(dev->phydev);
- phy_start_aneg(dev->phydev);
+ if (priv->phydev) {
+ phy_init_hw(priv->phydev);
+ phy_start_aneg(priv->phydev);
}
}
@@ -236,7 +236,6 @@ static void bcmgenet_internal_phy_setup(struct net_device *dev)
static void bcmgenet_moca_phy_setup(struct bcmgenet_priv *priv)
{
- struct net_device *ndev = priv->dev;
u32 reg;
/* Speed settings are set in bcmgenet_mii_setup() */
@@ -245,14 +244,14 @@ static void bcmgenet_moca_phy_setup(struct bcmgenet_priv *priv)
bcmgenet_sys_writel(priv, reg, SYS_PORT_CTRL);
if (priv->hw_params->flags & GENET_HAS_MOCA_LINK_DET)
- fixed_phy_set_link_update(ndev->phydev,
+ fixed_phy_set_link_update(priv->phydev,
bcmgenet_fixed_phy_link_update);
}
int bcmgenet_mii_config(struct net_device *dev)
{
struct bcmgenet_priv *priv = netdev_priv(dev);
- struct phy_device *phydev = dev->phydev;
+ struct phy_device *phydev = priv->phydev;
struct device *kdev = &priv->pdev->dev;
const char *phy_name = NULL;
u32 id_mode_dis = 0;
@@ -303,7 +302,7 @@ int bcmgenet_mii_config(struct net_device *dev)
* capabilities, use that knowledge to also configure the
* Reverse MII interface correctly.
*/
- if ((phydev->supported & PHY_BASIC_FEATURES) ==
+ if ((priv->phydev->supported & PHY_BASIC_FEATURES) ==
PHY_BASIC_FEATURES)
port_ctrl = PORT_MODE_EXT_RVMII_25;
else
@@ -372,7 +371,7 @@ int bcmgenet_mii_probe(struct net_device *dev)
return -ENODEV;
}
} else {
- phydev = dev->phydev;
+ phydev = priv->phydev;
phydev->dev_flags = phy_flags;
ret = phy_connect_direct(dev, phydev, bcmgenet_mii_setup,
@@ -383,6 +382,8 @@ int bcmgenet_mii_probe(struct net_device *dev)
}
}
+ priv->phydev = phydev;
+
/* Configure port multiplexer based on what the probed PHY device since
* reading the 'max-speed' property determines the maximum supported
* PHY speed which is needed for bcmgenet_mii_config() to configure
@@ -390,7 +391,7 @@ int bcmgenet_mii_probe(struct net_device *dev)
*/
ret = bcmgenet_mii_config(dev);
if (ret) {
- phy_disconnect(phydev);
+ phy_disconnect(priv->phydev);
return ret;
}
@@ -400,7 +401,7 @@ int bcmgenet_mii_probe(struct net_device *dev)
* Ethernet MAC ISRs
*/
if (priv->internal_phy)
- phydev->irq = PHY_IGNORE_INTERRUPT;
+ priv->phydev->irq = PHY_IGNORE_INTERRUPT;
return 0;
}
@@ -605,6 +606,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
}
+ priv->phydev = phydev;
priv->phy_interface = pd->phy_interface;
return 0;
--
1.7.4.4
^ permalink raw reply related
* [PATCH v2 net-next 1/3] Revert "net: ethernet: bcmgenet: use new api ethtool_{get|set}_link_ksettings"
From: Philippe Reynes @ 2016-09-25 16:29 UTC (permalink / raw)
To: f.fainelli, jaedon.shin, davem; +Cc: netdev, linux-kernel, Philippe Reynes
In-Reply-To: <1474820967-1130-1-git-send-email-tremyfr@gmail.com>
This reverts commit 6b352ebccbcf ("net: ethernet: broadcom: bcmgenet:
use new api ethtool_{get|set}_link_ksettings").
We needs to revert the commit 62469c76007e ("net: ethernet: bcmgenet:
use phydev from struct net_device"), because this commit add a
regression. As the commit 6b352ebccbcf ("net: ethernet: broadcom:
bcmgenet: use new api ethtool_{get|set}_link_ksettings") depend
on the first one, we also need to revert it first.
Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 2013474..46f9043 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -450,8 +450,8 @@ static inline void bcmgenet_rdma_ring_writel(struct bcmgenet_priv *priv,
genet_dma_ring_regs[r]);
}
-static int bcmgenet_get_link_ksettings(struct net_device *dev,
- struct ethtool_link_ksettings *cmd)
+static int bcmgenet_get_settings(struct net_device *dev,
+ struct ethtool_cmd *cmd)
{
if (!netif_running(dev))
return -EINVAL;
@@ -459,11 +459,11 @@ static int bcmgenet_get_link_ksettings(struct net_device *dev,
if (!dev->phydev)
return -ENODEV;
- return phy_ethtool_ksettings_get(dev->phydev, cmd);
+ return phy_ethtool_gset(dev->phydev, cmd);
}
-static int bcmgenet_set_link_ksettings(struct net_device *dev,
- const struct ethtool_link_ksettings *cmd)
+static int bcmgenet_set_settings(struct net_device *dev,
+ struct ethtool_cmd *cmd)
{
if (!netif_running(dev))
return -EINVAL;
@@ -471,7 +471,7 @@ static int bcmgenet_set_link_ksettings(struct net_device *dev,
if (!dev->phydev)
return -ENODEV;
- return phy_ethtool_ksettings_set(dev->phydev, cmd);
+ return phy_ethtool_sset(dev->phydev, cmd);
}
static int bcmgenet_set_rx_csum(struct net_device *dev,
@@ -977,6 +977,8 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
.get_strings = bcmgenet_get_strings,
.get_sset_count = bcmgenet_get_sset_count,
.get_ethtool_stats = bcmgenet_get_ethtool_stats,
+ .get_settings = bcmgenet_get_settings,
+ .set_settings = bcmgenet_set_settings,
.get_drvinfo = bcmgenet_get_drvinfo,
.get_link = ethtool_op_get_link,
.get_msglevel = bcmgenet_get_msglevel,
@@ -988,8 +990,6 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
.nway_reset = bcmgenet_nway_reset,
.get_coalesce = bcmgenet_get_coalesce,
.set_coalesce = bcmgenet_set_coalesce,
- .get_link_ksettings = bcmgenet_get_link_ksettings,
- .set_link_ksettings = bcmgenet_set_link_ksettings,
};
/* Power down the unimac, based on mode. */
--
1.7.4.4
^ permalink raw reply related
* [PATCH v2 net-next 0/3] net: bcmgenet: only use new api ethtool_{get|set}_link_ksettings
From: Philippe Reynes @ 2016-09-25 16:29 UTC (permalink / raw)
To: f.fainelli, jaedon.shin, davem; +Cc: netdev, linux-kernel, Philippe Reynes
Some times ago, a serie of patches were committed :
- commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from struct net_device")
- commit 6b352ebccbcf ("net: ethernet: broadcom: bcmgenet: use new api ethtool_{get|set}_link_ksettings")
The first patch add a regression on this driver, so it should be reverted.
As the second patch depend on the former, it should be reverted too.
The first patch is buggy because there is a "trick" in this driver.
The structure phydev is kept in the private data when the interface
go down, and used when the interface go up to enable the phy before
the function phy_connect is called.
I don't have this hardware, neither the datasheet. So I won't
update the driver to avoid this trick.
But the real goal of the first serie was to move to the new api
ethtool_{get|set}_link_ksettings. So I provide a new version of
the patch without the "cleaning" of driver to use the phydev
store in the net_device structure.
Changelog:
v2:
- use Florian Fainelli patches for the revert instead of Jaedon Shin
- simply use net: bcmgenet: for the prefix of the patch
Florian Fainelli (1):
Revert "net: ethernet: bcmgenet: use phydev from struct net_device"
Philippe Reynes (2):
Revert "net: ethernet: bcmgenet: use new api
ethtool_{get|set}_link_ksettings"
net: bcmgenet: use new api ethtool_{get|set}_link_ksettings
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 33 ++++++++++++-----------
drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
drivers/net/ethernet/broadcom/genet/bcmmii.c | 24 +++++++++--------
3 files changed, 31 insertions(+), 27 deletions(-)
--
1.7.4.4
^ permalink raw reply
* Re: [PATCH net-next 4/4] net/sched: act_mirred: Implement ingress actions
From: Daniel Borkmann @ 2016-09-25 16:26 UTC (permalink / raw)
To: Jamal Hadi Salim, Shmulik Ladkani
Cc: David S. Miller, WANG Cong, Eric Dumazet, netdev,
Florian Westphal
In-Reply-To: <6d2bd45a-a8a0-846d-5934-5e246522cab8@mojatatu.com>
On 09/25/2016 03:05 PM, Jamal Hadi Salim wrote:
> On 16-09-23 11:40 AM, Shmulik Ladkani wrote:
>> On Fri, 23 Sep 2016 08:48:33 -0400 Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>> Even today, one may create loops using existing 'egress redirect',
>>>> e.g. this rediculously errorneous construct:
>>>>
>>>> # ip l add v0 type veth peer name v0p
>>>> # tc filter add dev v0p parent ffff: basic \
>>>> action mirred egress redirect dev v0
>>>
>>> I think we actually recover from this one by eventually
>>> dropping (theres a ttl field).
>>
>> [off topic]
>>
>> Don't know about that :) cpu fan got very noisy, 3 of 4 cores at 100%,
>> and after one second I got:
>>
>> # ip -s l show type veth
>> 16: v0p@v0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>> link/ether a2:64:ff:10:dd:85 brd ff:ff:ff:ff:ff:ff
>> RX: bytes packets errors dropped overrun mcast
>> 71660305923 469890864 0 0 0 0
>> TX: bytes packets errors dropped carrier collsns
>> 3509 24 0 0 0 0
>> 17: v0@v0p: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>> link/ether 52:a2:34:f6:7c:ec brd ff:ff:ff:ff:ff:ff
>> RX: bytes packets errors dropped overrun mcast
>> 3509 24 0 0 0 0
>> TX: bytes packets errors dropped carrier collsns
>> 71660713017 469893555 0 0 0 0
>>
>
> I think this is still on topic!
>
> Now I realize that code we took out around 4.2.x is still useful
> for such a use case (I wasnt thinking about veth when Florian was
> slimming the skb);
> +Cc Florian W.
>
> This snippet from 4.2:
> -------------
> 3525 static int ing_filter(struct sk_buff *skb, struct netdev_queue *rxq)
> 3526 {
> 3527 struct net_device *dev = skb->dev;
> 3528 u32 ttl = G_TC_RTTL(skb->tc_verd);
> 3529 int result = TC_ACT_OK;
> 3530 struct Qdisc *q;
> 3531
> 3532 if (unlikely(MAX_RED_LOOP < ttl++)) {
> 3533 net_warn_ratelimited("Redir loop detected Dropping packet (%d->%d)\n",
> 3534 skb->skb_iif, dev->ifindex);
> 3535 return TC_ACT_SHOT;
> 3536 }
> 3537
> 3538 skb->tc_verd = SET_TC_RTTL(skb->tc_verd, ttl);
> 3539 skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_INGRESS);
> 3540
> 3541 q = rcu_dereference(rxq->qdisc);
> 3542 if (q != &noop_qdisc) {
> 3543 spin_lock(qdisc_lock(q));
> 3544 if (likely(!test_bit(__QDISC_STATE_DEACTIVATED, &q->state)))
> 3545 result = qdisc_enqueue_root(skb, q);
> 3546 spin_unlock(qdisc_lock(q));
> 3547 }
> 3548
> 3549 return result;
> 3550 }
> --------------------
>
> MAX_RED_LOOP (stands for "Maximum Redirect loop") still exists in
> current code. The idea above was that we would increment the rttl
> counter once and if we saw it again upto MAX_RED_LOOP we would assume
> a loop and drop the packet (at the time i didnt think it was wise to
> let the actions be in charge of setting the RTTL; it had to be central
> core code - but it may not be neccessary)
>
> Florian, when we discussed I said it was fine to reclaim those 3 bits
> on tc verdict for RTTL at the time because i had taken out the
> feature and never added it back. Your comment at the time was we can
> add it back when someone shows up with the feature.
> Shmulik is looking to add it.
Why not just reuse xmit_recursion, which is what we did in tc cls_bpf
programs f.e. see __bpf_tx_skb()? Would be a pity to waste 3 bits on
this in the skb.
>> Similarly to all constructs injecting skbs to device rx (bond/team,
>> vlan, macvlan, tunnels, ifb, __dev_forward_skb callers, etc..), we are
>> obligated to assign 'skb2->dev' as the new rx device.
>>
>> Regarding 'skb2->skb_iif', original act_mirred code already has:
>>
>> skb2->skb_iif = skb->dev->ifindex; <--- THIS IS ORIG DEV IIF
>> skb2->dev = dev; <--- THIS IS TARGET DEV
>> err = dev_queue_xmit(skb2);
>>
>> I'm preserving this; OTOH the suggested modification in the patch is
>>
>> - err = dev_queue_xmit(skb2);
>> + if (tcf_mirred_act_direction(m->tcfm_eaction) & AT_EGRESS)
>> + err = dev_queue_xmit(skb2);
>> + else
>> + netif_receive_skb(skb2);
>>
>> now, the call to 'netif_receive_skb' will eventually override skb_iif to
>> the target RX dev's index, upon entry to __netif_receive_skb_core.
>>
>> I think this IS the expected behavior - as done by other "rx injection"
>> constructs.
>
> Sounds fine.
> I am wondering if we can have a tracing feature to show the lifetime of
> the packet as it is being cycled around the kernel? It would help
> debugging if some policy misbehaves.
>
>> My doubts were around whether we should call 'dev_forward_skb' instead
>> of 'netif_receive_skb'.
>> The former does some things I assumed we're not interested of, like
>> testing 'is_skb_forwardable' and re-running 'eth_type_trans'.
>> OTOH, it DOES scrub the skb.
>> Maybe we should scrub it as well prior the netif_receive_skb call?
>
> Scrubbing the skb could be a bad idea if it gets rid of global state
> like the RTTL if you add it back.
>
> cheers,
> jamal
>
^ permalink raw reply
* RE: [PATCH net v2 0/2] Fix tc-ife bugs
From: Yotam Gigi @ 2016-09-25 15:55 UTC (permalink / raw)
To: Jamal Hadi Salim, Yotam Gigi, davem@davemloft.net,
netdev@vger.kernel.org
Cc: Jiri Pirko, Elad Raz, mlxsw
In-Reply-To: <2b01c982-76b7-d380-807c-1fe1df62d285@mojatatu.com>
>-----Original Message-----
>From: Jamal Hadi Salim [mailto:jhs@mojatatu.com]
>Sent: Sunday, September 25, 2016 4:46 PM
>To: Yotam Gigi <yotam.gi@gmail.com>; davem@davemloft.net;
>netdev@vger.kernel.org; Yotam Gigi <yotamg@mellanox.com>
>Subject: Re: [PATCH net v2 0/2] Fix tc-ife bugs
>
>On 16-09-25 08:31 AM, Yotam Gigi wrote:
>> This patch-set contains two bugfixes in the tc-ife action, one fixing some
>> random behaviour in encode side, and one fixing the decode side packet
>> parsing logic.
>>
>> Yotam Gigi (2):
>> act_ife: Fix external mac header on encode
>> act_ife: Fix false parsing on decode side
>>
>> net/sched/act_ife.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>
>Yotam, did you run the test i proposed? I am worried about the TLV one.
>Note:
>Encoder includes the length of the TLV header length in the L part.
>If you are reaching a conclusion that you need this in the decoding:
>+ tlvdata += alen + sizeof(struct meta_tlvhdr);
>
>then very likely whoever is sending that packet is not encoding
>correctly.
The one encoding the packets I get is the ife action. You can look at the code:
int ife_tlv_meta_encode(void *skbdata, u16 attrtype, u16 dlen, const void *dval)
{
u32 *tlv = (u32 *)(skbdata);
u16 totlen = nla_total_size(dlen); /*alignment + hdr */
char *dptr = (char *)tlv + NLA_HDRLEN;
u32 htlv = attrtype << 16 | dlen;
*tlv = htonl(htlv);
memset(dptr, 0, totlen - NLA_HDRLEN);
memcpy(dptr, dval, dlen);
return totlen;
}
As you can see, the data that is actually written into the packet is the htlv
var, which has the 'dlen' in it, and not the totlen which corresponds the tlv
header size. You can see that in the following example:
I ran the tc command you proposed:
$TC filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action skbedit prio 33 \
action ife encode \
type 0xDEAD \
use mark 12 \
allow prio \
dst 02:15:15:15:15:15
And this is the packet I got:
0x0000: 0215 1515 1515 da23 d209 8644 dead 0012
0x0010: 0001 0004 0000 000c 0003 0004 0000 0033
0x0020: fa30 7418 593a da23 d209 8644 0800 4500
0x0030: 0054 da3c 4000 4001 486a 0c00 0001 0c00
0x0040: 0002 0800 9fa2 1562 0008 aeec e757 0000
0x0050: 0000 ecdb 0100 0000 0000 1011 1213 1415
0x0060: 1617 1819 1a1b 1c1d 1e1f 2021 2223 2425
0x0070: 2627 2829 2a2b 2c2d 2e2f 3031 3233 3435
0x0080: 3637
You can see that there are two tlvs there, one for mark (with value 0xc=12) and
one for prio (with value 0x33). In the packets, you can see the on offsets 0x12
and 0x1a, that the size in the tlv is 4 which is the datalen and not 8 which is
the total tlv size.
When I ran the decode without the fix, my kernel went into infinite loop which
was caused by:
- The loop stopping condition was checking that an unsigned variable is greater
than zero.
- The parsing assumes that in the tlv header, the size refers to the whole tlv
size, but it refers to the size of the data only.
To fix those problems, I fixed the decode side to assume that the tlv->size
refers to the data len and not the whole tlv, and changed the variable to be
signed.
Do you prefer that I will fix the encode side to encode the whole tlv header
size instead of fixing the decode side?
Thanks :)
>
>cheers,
>jamal
^ permalink raw reply
* Re: [PATCH] nfp: bpf: improve handling for disabled BPF syscall
From: Jakub Kicinski @ 2016-09-25 15:52 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: David S. Miller, oss-drivers, netdev, linux-kernel
In-Reply-To: <20160923202443.4130858-1-arnd@arndb.de>
On Fri, 23 Sep 2016 22:23:59 +0200, Arnd Bergmann wrote:
> I stumbled over a new warning during randconfig testing,
> with CONFIG_BPF_SYSCALL disabled:
>
> drivers/net/ethernet/netronome/nfp/nfp_net_offload.c: In function 'nfp_net_bpf_offload':
> drivers/net/ethernet/netronome/nfp/nfp_net_offload.c:263:3: error: '*((void *)&res+4)' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/net/ethernet/netronome/nfp/nfp_net_offload.c:263:3: error: 'res.n_instr' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> As far as I can tell, this is a false positive caused by the compiler
> getting confused about a function that is partially inlined, but it's
> easy to avoid while improving the code:
>
> The nfp_bpf_jit() stub helper for that configuration is unusual as it
> is defined in a header file but not marked 'static inline'. By moving
> the compile-time check into the caller using the IS_ENABLED() macro,
> we can remove that stub and simplify the nfp_net_bpf_offload_prepare()
> function enough to unconfuse the compiler.
>
> Fixes: 7533fdc0f77f ("nfp: bpf: add hardware bpf offload")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Thanks Arnd! static inline definitely missing there but if we can
depend on dead code elimination to remove the symbol that's even cooler!
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
^ permalink raw reply
* Re: [PATCH 0/3] ethernet: net: bcmgenet: only use new api ethtool_{get|set}_link_ksettings
From: Florian Fainelli @ 2016-09-25 15:50 UTC (permalink / raw)
To: Philippe Reynes, jaedon.shin, davem; +Cc: netdev, linux-kernel
In-Reply-To: <ec5bc7b8-43dc-7b33-50db-98eb7f4bc3e8@gmail.com>
Le 25/09/2016 à 08:49, Florian Fainelli a écrit :
> Le 25/09/2016 à 08:36, Philippe Reynes a écrit :
>> Some times ago, a serie of patches were committed :
>> - commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from struct net_device")
>> - commit 6b352ebccbcf ("net: ethernet: broadcom: bcmgenet: use new api ethtool_{get|set}_link_ksettings")
>> The first patch add a regression on this driver, so it should be reverted.
>> As the second patch depend on the former, it should be reverted too.
>>
>> The first patch is buggy because there is a "trick" in this driver.
>> The structure phydev is kept in the private data when the interface
>> go down, and used when the interface go up to enable the phy before
>> the function phy_connect is called.
>>
>> I don't have this hardware, neither the datasheet. So I won't
>> update the driver to avoid this trick.
>>
>> But the real goal of the first serie was to move to the new api
>> ethtool_{get|set}_link_ksettings. So I provide a new version of
>> the patch without the "cleaning" of driver to use the phydev
>> store in the net_device structure.
>>
>> Jaedon Shin (1):
>> Revert "net: ethernet: bcmgenet: use phydev from struct net_device"
>
> Please replace Jaedon's patch with mine which contains more background
> as to what the problem was and how it gets fixed.
>
>>
>> Philippe Reynes (2):
>> Revert "net: ethernet: bcmgenet: use new api
>> ethtool_{get|set}_link_ksettings"
>> net: ethernet: broadcom: bcmgenet: use new api
>> ethtool_{get|set}_link_ksettings
>
> Can you be consistent in the subject and just use "net: bcmgenet: " as a
> prefix here?
An finally, indicate in the subject which tree you are targeting,
"net-next" or "net".
--
Florian
^ permalink raw reply
* Re: [PATCH 0/3] ethernet: net: bcmgenet: only use new api ethtool_{get|set}_link_ksettings
From: Florian Fainelli @ 2016-09-25 15:49 UTC (permalink / raw)
To: Philippe Reynes, jaedon.shin, davem; +Cc: netdev, linux-kernel
In-Reply-To: <1474817794-31791-1-git-send-email-tremyfr@gmail.com>
Le 25/09/2016 à 08:36, Philippe Reynes a écrit :
> Some times ago, a serie of patches were committed :
> - commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from struct net_device")
> - commit 6b352ebccbcf ("net: ethernet: broadcom: bcmgenet: use new api ethtool_{get|set}_link_ksettings")
> The first patch add a regression on this driver, so it should be reverted.
> As the second patch depend on the former, it should be reverted too.
>
> The first patch is buggy because there is a "trick" in this driver.
> The structure phydev is kept in the private data when the interface
> go down, and used when the interface go up to enable the phy before
> the function phy_connect is called.
>
> I don't have this hardware, neither the datasheet. So I won't
> update the driver to avoid this trick.
>
> But the real goal of the first serie was to move to the new api
> ethtool_{get|set}_link_ksettings. So I provide a new version of
> the patch without the "cleaning" of driver to use the phydev
> store in the net_device structure.
>
> Jaedon Shin (1):
> Revert "net: ethernet: bcmgenet: use phydev from struct net_device"
Please replace Jaedon's patch with mine which contains more background
as to what the problem was and how it gets fixed.
>
> Philippe Reynes (2):
> Revert "net: ethernet: bcmgenet: use new api
> ethtool_{get|set}_link_ksettings"
> net: ethernet: broadcom: bcmgenet: use new api
> ethtool_{get|set}_link_ksettings
Can you be consistent in the subject and just use "net: bcmgenet: " as a
prefix here?
>
> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 33 ++++++++++++-----------
> drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
> drivers/net/ethernet/broadcom/genet/bcmmii.c | 24 +++++++++--------
> 3 files changed, 31 insertions(+), 27 deletions(-)
>
--
Florian
^ permalink raw reply
* [PATCH -next] net: dsa: mv88e6xxx: fix non static symbol warnings
From: Wei Yongjun @ 2016-09-25 15:43 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli; +Cc: Wei Yongjun, netdev
From: Wei Yongjun <weiyongjun1@huawei.com>
Fixes the following sparse warnings:
drivers/net/dsa/mv88e6xxx/chip.c:219:5: warning:
symbol 'mv88e6xxx_port_read' was not declared. Should it be static?
drivers/net/dsa/mv88e6xxx/chip.c:227:5: warning:
symbol 'mv88e6xxx_port_write' was not declared. Should it be static?
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 25bd3fa..6f28f19 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -216,16 +216,16 @@ int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val)
return 0;
}
-int mv88e6xxx_port_read(struct mv88e6xxx_chip *chip, int port, int reg,
- u16 *val)
+static int mv88e6xxx_port_read(struct mv88e6xxx_chip *chip, int port, int reg,
+ u16 *val)
{
int addr = chip->info->port_base_addr + port;
return mv88e6xxx_read(chip, addr, reg, val);
}
-int mv88e6xxx_port_write(struct mv88e6xxx_chip *chip, int port, int reg,
- u16 val)
+static int mv88e6xxx_port_write(struct mv88e6xxx_chip *chip, int port, int reg,
+ u16 val)
{
int addr = chip->info->port_base_addr + port;
^ permalink raw reply related
* [PATCH -next] be2net: fix non static symbol warnings
From: Wei Yongjun @ 2016-09-25 15:40 UTC (permalink / raw)
To: Sathya Perla, Ajit Khaparde, Sriharsha Basavapatna, Somnath Kotur
Cc: Wei Yongjun, netdev
From: Wei Yongjun <weiyongjun1@huawei.com>
Fixes the following sparse warnings:
drivers/net/ethernet/emulex/benet/be_main.c:47:25: warning:
symbol 'be_err_recovery_workq' was not declared. Should it be static?
drivers/net/ethernet/emulex/benet/be_main.c:63:25: warning:
symbol 'be_wq' was not declared. Should it be static?
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
drivers/net/ethernet/emulex/benet/be_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 9a94840..0d1ae3d 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -44,7 +44,7 @@ MODULE_PARM_DESC(rx_frag_size, "Size of a fragment that holds rcvd data.");
/* Per-module error detection/recovery workq shared across all functions.
* Each function schedules its own work request on this shared workq.
*/
-struct workqueue_struct *be_err_recovery_workq;
+static struct workqueue_struct *be_err_recovery_workq;
static const struct pci_device_id be_dev_ids[] = {
{ PCI_DEVICE(BE_VENDOR_ID, BE_DEVICE_ID1) },
@@ -60,7 +60,7 @@ static const struct pci_device_id be_dev_ids[] = {
MODULE_DEVICE_TABLE(pci, be_dev_ids);
/* Workqueue used by all functions for defering cmd calls to the adapter */
-struct workqueue_struct *be_wq;
+static struct workqueue_struct *be_wq;
/* UE Status Low CSR */
static const char * const ue_status_low_desc[] = {
^ permalink raw reply related
* [PATCH 3/3] net: ethernet: broadcom: bcmgenet: use new api ethtool_{get|set}_link_ksettings
From: Philippe Reynes @ 2016-09-25 15:36 UTC (permalink / raw)
To: f.fainelli, jaedon.shin, davem; +Cc: netdev, linux-kernel, Philippe Reynes
In-Reply-To: <1474817794-31791-1-git-send-email-tremyfr@gmail.com>
The ethtool api {get|set}_settings is deprecated.
We move this driver to new api {get|set}_link_ksettings.
Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 24 ++++++++++--------------
1 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 47d0a2b..2c5d9d3 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -450,32 +450,28 @@ static inline void bcmgenet_rdma_ring_writel(struct bcmgenet_priv *priv,
genet_dma_ring_regs[r]);
}
-static int bcmgenet_get_settings(struct net_device *dev,
- struct ethtool_cmd *cmd)
+static int bcmgenet_get_link_ksettings(struct net_device *dev,
+ struct ethtool_link_ksettings *cmd)
{
- struct bcmgenet_priv *priv = netdev_priv(dev);
-
if (!netif_running(dev))
return -EINVAL;
- if (!priv->phydev)
+ if (!dev->phydev)
return -ENODEV;
- return phy_ethtool_gset(priv->phydev, cmd);
+ return phy_ethtool_ksettings_get(dev->phydev, cmd);
}
-static int bcmgenet_set_settings(struct net_device *dev,
- struct ethtool_cmd *cmd)
+static int bcmgenet_set_link_ksettings(struct net_device *dev,
+ const struct ethtool_link_ksettings *cmd)
{
- struct bcmgenet_priv *priv = netdev_priv(dev);
-
if (!netif_running(dev))
return -EINVAL;
- if (!priv->phydev)
+ if (!dev->phydev)
return -ENODEV;
- return phy_ethtool_sset(priv->phydev, cmd);
+ return phy_ethtool_ksettings_set(dev->phydev, cmd);
}
static int bcmgenet_set_rx_csum(struct net_device *dev,
@@ -983,8 +979,6 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
.get_strings = bcmgenet_get_strings,
.get_sset_count = bcmgenet_get_sset_count,
.get_ethtool_stats = bcmgenet_get_ethtool_stats,
- .get_settings = bcmgenet_get_settings,
- .set_settings = bcmgenet_set_settings,
.get_drvinfo = bcmgenet_get_drvinfo,
.get_link = ethtool_op_get_link,
.get_msglevel = bcmgenet_get_msglevel,
@@ -996,6 +990,8 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
.nway_reset = bcmgenet_nway_reset,
.get_coalesce = bcmgenet_get_coalesce,
.set_coalesce = bcmgenet_set_coalesce,
+ .get_link_ksettings = bcmgenet_get_link_ksettings,
+ .set_link_ksettings = bcmgenet_set_link_ksettings,
};
/* Power down the unimac, based on mode. */
--
1.7.4.4
^ permalink raw reply related
* [PATCH 2/3] Revert "net: ethernet: bcmgenet: use phydev from struct net_device"
From: Philippe Reynes @ 2016-09-25 15:36 UTC (permalink / raw)
To: f.fainelli, jaedon.shin, davem; +Cc: netdev, linux-kernel, Philippe Reynes
In-Reply-To: <1474817794-31791-1-git-send-email-tremyfr@gmail.com>
From: Jaedon Shin <jaedon.shin@gmail.com>
This reverts commit 62469c76007e ("net: ethernet: bcmgenet: use phydev
from struct net_device")
without this patch, we call twice bcmgenet_mii_reset, and that is intended:
- first time from bcmgenet_power_up() to make sure the PHY is initialized
*before* we get to initialize the UniMAC, this is critical
- second time from bcmgenet_mii_probe(), through the normal phy_init_hw()
with this patch, we only get to call bcmgenet_mii_reset once, in
bcmgenet_mii_probe() because the first time in bcmgenet_power_up(),
dev->phydev is NULL, because of a prior call to phy_disconnect() in
bcmgenet_close(), unfortunately, there has been MAC activity, so the PHY
gets in a bad state
Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 45 +++++++++++++----------
drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
drivers/net/ethernet/broadcom/genet/bcmmii.c | 24 +++++++------
3 files changed, 39 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 46f9043..47d0a2b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -453,25 +453,29 @@ static inline void bcmgenet_rdma_ring_writel(struct bcmgenet_priv *priv,
static int bcmgenet_get_settings(struct net_device *dev,
struct ethtool_cmd *cmd)
{
+ struct bcmgenet_priv *priv = netdev_priv(dev);
+
if (!netif_running(dev))
return -EINVAL;
- if (!dev->phydev)
+ if (!priv->phydev)
return -ENODEV;
- return phy_ethtool_gset(dev->phydev, cmd);
+ return phy_ethtool_gset(priv->phydev, cmd);
}
static int bcmgenet_set_settings(struct net_device *dev,
struct ethtool_cmd *cmd)
{
+ struct bcmgenet_priv *priv = netdev_priv(dev);
+
if (!netif_running(dev))
return -EINVAL;
- if (!dev->phydev)
+ if (!priv->phydev)
return -ENODEV;
- return phy_ethtool_sset(dev->phydev, cmd);
+ return phy_ethtool_sset(priv->phydev, cmd);
}
static int bcmgenet_set_rx_csum(struct net_device *dev,
@@ -937,7 +941,7 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
e->eee_active = p->eee_active;
e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
- return phy_ethtool_get_eee(dev->phydev, e);
+ return phy_ethtool_get_eee(priv->phydev, e);
}
static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
@@ -954,7 +958,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
if (!p->eee_enabled) {
bcmgenet_eee_enable_set(dev, false);
} else {
- ret = phy_init_eee(dev->phydev, 0);
+ ret = phy_init_eee(priv->phydev, 0);
if (ret) {
netif_err(priv, hw, dev, "EEE initialization failed\n");
return ret;
@@ -964,12 +968,14 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
bcmgenet_eee_enable_set(dev, true);
}
- return phy_ethtool_set_eee(dev->phydev, e);
+ return phy_ethtool_set_eee(priv->phydev, e);
}
static int bcmgenet_nway_reset(struct net_device *dev)
{
- return genphy_restart_aneg(dev->phydev);
+ struct bcmgenet_priv *priv = netdev_priv(dev);
+
+ return genphy_restart_aneg(priv->phydev);
}
/* standard ethtool support functions. */
@@ -996,13 +1002,12 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
static int bcmgenet_power_down(struct bcmgenet_priv *priv,
enum bcmgenet_power_mode mode)
{
- struct net_device *ndev = priv->dev;
int ret = 0;
u32 reg;
switch (mode) {
case GENET_POWER_CABLE_SENSE:
- phy_detach(ndev->phydev);
+ phy_detach(priv->phydev);
break;
case GENET_POWER_WOL_MAGIC:
@@ -1063,6 +1068,7 @@ static void bcmgenet_power_up(struct bcmgenet_priv *priv,
/* ioctl handle special commands that are not present in ethtool. */
static int bcmgenet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
{
+ struct bcmgenet_priv *priv = netdev_priv(dev);
int val = 0;
if (!netif_running(dev))
@@ -1072,10 +1078,10 @@ static int bcmgenet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
case SIOCGMIIPHY:
case SIOCGMIIREG:
case SIOCSMIIREG:
- if (!dev->phydev)
+ if (!priv->phydev)
val = -ENODEV;
else
- val = phy_mii_ioctl(dev->phydev, rq, cmd);
+ val = phy_mii_ioctl(priv->phydev, rq, cmd);
break;
default:
@@ -2458,7 +2464,6 @@ static void bcmgenet_irq_task(struct work_struct *work)
{
struct bcmgenet_priv *priv = container_of(
work, struct bcmgenet_priv, bcmgenet_irq_work);
- struct net_device *ndev = priv->dev;
netif_dbg(priv, intr, priv->dev, "%s\n", __func__);
@@ -2471,7 +2476,7 @@ static void bcmgenet_irq_task(struct work_struct *work)
/* Link UP/DOWN event */
if (priv->irq0_stat & UMAC_IRQ_LINK_EVENT) {
- phy_mac_interrupt(ndev->phydev,
+ phy_mac_interrupt(priv->phydev,
!!(priv->irq0_stat & UMAC_IRQ_LINK_UP));
priv->irq0_stat &= ~UMAC_IRQ_LINK_EVENT;
}
@@ -2833,7 +2838,7 @@ static void bcmgenet_netif_start(struct net_device *dev)
/* Monitor link interrupts now */
bcmgenet_link_intr_enable(priv);
- phy_start(dev->phydev);
+ phy_start(priv->phydev);
}
static int bcmgenet_open(struct net_device *dev)
@@ -2932,7 +2937,7 @@ static void bcmgenet_netif_stop(struct net_device *dev)
struct bcmgenet_priv *priv = netdev_priv(dev);
netif_tx_stop_all_queues(dev);
- phy_stop(dev->phydev);
+ phy_stop(priv->phydev);
bcmgenet_intr_disable(priv);
bcmgenet_disable_rx_napi(priv);
bcmgenet_disable_tx_napi(priv);
@@ -2958,7 +2963,7 @@ static int bcmgenet_close(struct net_device *dev)
bcmgenet_netif_stop(dev);
/* Really kill the PHY state machine and disconnect from it */
- phy_disconnect(dev->phydev);
+ phy_disconnect(priv->phydev);
/* Disable MAC receive */
umac_enable_set(priv, CMD_RX_EN, false);
@@ -3517,7 +3522,7 @@ static int bcmgenet_suspend(struct device *d)
bcmgenet_netif_stop(dev);
- phy_suspend(dev->phydev);
+ phy_suspend(priv->phydev);
netif_device_detach(dev);
@@ -3581,7 +3586,7 @@ static int bcmgenet_resume(struct device *d)
if (priv->wolopts)
clk_disable_unprepare(priv->clk_wol);
- phy_init_hw(dev->phydev);
+ phy_init_hw(priv->phydev);
/* Speed settings must be restored */
bcmgenet_mii_config(priv->dev);
@@ -3614,7 +3619,7 @@ static int bcmgenet_resume(struct device *d)
netif_device_attach(dev);
- phy_resume(dev->phydev);
+ phy_resume(priv->phydev);
if (priv->eee.eee_enabled)
bcmgenet_eee_enable_set(dev, true);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 0f0868c..1e2dc34 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -597,6 +597,7 @@ struct bcmgenet_priv {
/* MDIO bus variables */
wait_queue_head_t wq;
+ struct phy_device *phydev;
bool internal_phy;
struct device_node *phy_dn;
struct device_node *mdio_dn;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index e907acd..457c3bc 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -86,7 +86,7 @@ static int bcmgenet_mii_write(struct mii_bus *bus, int phy_id,
void bcmgenet_mii_setup(struct net_device *dev)
{
struct bcmgenet_priv *priv = netdev_priv(dev);
- struct phy_device *phydev = dev->phydev;
+ struct phy_device *phydev = priv->phydev;
u32 reg, cmd_bits = 0;
bool status_changed = false;
@@ -183,9 +183,9 @@ void bcmgenet_mii_reset(struct net_device *dev)
if (GENET_IS_V4(priv))
return;
- if (dev->phydev) {
- phy_init_hw(dev->phydev);
- phy_start_aneg(dev->phydev);
+ if (priv->phydev) {
+ phy_init_hw(priv->phydev);
+ phy_start_aneg(priv->phydev);
}
}
@@ -236,7 +236,6 @@ static void bcmgenet_internal_phy_setup(struct net_device *dev)
static void bcmgenet_moca_phy_setup(struct bcmgenet_priv *priv)
{
- struct net_device *ndev = priv->dev;
u32 reg;
/* Speed settings are set in bcmgenet_mii_setup() */
@@ -245,14 +244,14 @@ static void bcmgenet_moca_phy_setup(struct bcmgenet_priv *priv)
bcmgenet_sys_writel(priv, reg, SYS_PORT_CTRL);
if (priv->hw_params->flags & GENET_HAS_MOCA_LINK_DET)
- fixed_phy_set_link_update(ndev->phydev,
+ fixed_phy_set_link_update(priv->phydev,
bcmgenet_fixed_phy_link_update);
}
int bcmgenet_mii_config(struct net_device *dev)
{
struct bcmgenet_priv *priv = netdev_priv(dev);
- struct phy_device *phydev = dev->phydev;
+ struct phy_device *phydev = priv->phydev;
struct device *kdev = &priv->pdev->dev;
const char *phy_name = NULL;
u32 id_mode_dis = 0;
@@ -303,7 +302,7 @@ int bcmgenet_mii_config(struct net_device *dev)
* capabilities, use that knowledge to also configure the
* Reverse MII interface correctly.
*/
- if ((phydev->supported & PHY_BASIC_FEATURES) ==
+ if ((priv->phydev->supported & PHY_BASIC_FEATURES) ==
PHY_BASIC_FEATURES)
port_ctrl = PORT_MODE_EXT_RVMII_25;
else
@@ -372,7 +371,7 @@ int bcmgenet_mii_probe(struct net_device *dev)
return -ENODEV;
}
} else {
- phydev = dev->phydev;
+ phydev = priv->phydev;
phydev->dev_flags = phy_flags;
ret = phy_connect_direct(dev, phydev, bcmgenet_mii_setup,
@@ -383,6 +382,8 @@ int bcmgenet_mii_probe(struct net_device *dev)
}
}
+ priv->phydev = phydev;
+
/* Configure port multiplexer based on what the probed PHY device since
* reading the 'max-speed' property determines the maximum supported
* PHY speed which is needed for bcmgenet_mii_config() to configure
@@ -390,7 +391,7 @@ int bcmgenet_mii_probe(struct net_device *dev)
*/
ret = bcmgenet_mii_config(dev);
if (ret) {
- phy_disconnect(phydev);
+ phy_disconnect(priv->phydev);
return ret;
}
@@ -400,7 +401,7 @@ int bcmgenet_mii_probe(struct net_device *dev)
* Ethernet MAC ISRs
*/
if (priv->internal_phy)
- phydev->irq = PHY_IGNORE_INTERRUPT;
+ priv->phydev->irq = PHY_IGNORE_INTERRUPT;
return 0;
}
@@ -605,6 +606,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
}
+ priv->phydev = phydev;
priv->phy_interface = pd->phy_interface;
return 0;
--
1.7.4.4
^ permalink raw reply related
* [PATCH 1/3] Revert "net: ethernet: bcmgenet: use new api ethtool_{get|set}_link_ksettings"
From: Philippe Reynes @ 2016-09-25 15:36 UTC (permalink / raw)
To: f.fainelli, jaedon.shin, davem; +Cc: netdev, linux-kernel, Philippe Reynes
In-Reply-To: <1474817794-31791-1-git-send-email-tremyfr@gmail.com>
This reverts commit 6b352ebccbcf ("net: ethernet: broadcom: bcmgenet:
use new api ethtool_{get|set}_link_ksettings").
We needs to revert the commit 62469c76007e ("net: ethernet: bcmgenet:
use phydev from struct net_device"), because this commit add a
regression. As the commit 6b352ebccbcf ("net: ethernet: broadcom:
bcmgenet: use new api ethtool_{get|set}_link_ksettings") depend
on the first one, we also need to revert it first.
Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 2013474..46f9043 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -450,8 +450,8 @@ static inline void bcmgenet_rdma_ring_writel(struct bcmgenet_priv *priv,
genet_dma_ring_regs[r]);
}
-static int bcmgenet_get_link_ksettings(struct net_device *dev,
- struct ethtool_link_ksettings *cmd)
+static int bcmgenet_get_settings(struct net_device *dev,
+ struct ethtool_cmd *cmd)
{
if (!netif_running(dev))
return -EINVAL;
@@ -459,11 +459,11 @@ static int bcmgenet_get_link_ksettings(struct net_device *dev,
if (!dev->phydev)
return -ENODEV;
- return phy_ethtool_ksettings_get(dev->phydev, cmd);
+ return phy_ethtool_gset(dev->phydev, cmd);
}
-static int bcmgenet_set_link_ksettings(struct net_device *dev,
- const struct ethtool_link_ksettings *cmd)
+static int bcmgenet_set_settings(struct net_device *dev,
+ struct ethtool_cmd *cmd)
{
if (!netif_running(dev))
return -EINVAL;
@@ -471,7 +471,7 @@ static int bcmgenet_set_link_ksettings(struct net_device *dev,
if (!dev->phydev)
return -ENODEV;
- return phy_ethtool_ksettings_set(dev->phydev, cmd);
+ return phy_ethtool_sset(dev->phydev, cmd);
}
static int bcmgenet_set_rx_csum(struct net_device *dev,
@@ -977,6 +977,8 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
.get_strings = bcmgenet_get_strings,
.get_sset_count = bcmgenet_get_sset_count,
.get_ethtool_stats = bcmgenet_get_ethtool_stats,
+ .get_settings = bcmgenet_get_settings,
+ .set_settings = bcmgenet_set_settings,
.get_drvinfo = bcmgenet_get_drvinfo,
.get_link = ethtool_op_get_link,
.get_msglevel = bcmgenet_get_msglevel,
@@ -988,8 +990,6 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
.nway_reset = bcmgenet_nway_reset,
.get_coalesce = bcmgenet_get_coalesce,
.set_coalesce = bcmgenet_set_coalesce,
- .get_link_ksettings = bcmgenet_get_link_ksettings,
- .set_link_ksettings = bcmgenet_set_link_ksettings,
};
/* Power down the unimac, based on mode. */
--
1.7.4.4
^ permalink raw reply related
* [PATCH 0/3] ethernet: net: bcmgenet: only use new api ethtool_{get|set}_link_ksettings
From: Philippe Reynes @ 2016-09-25 15:36 UTC (permalink / raw)
To: f.fainelli, jaedon.shin, davem; +Cc: netdev, linux-kernel, Philippe Reynes
Some times ago, a serie of patches were committed :
- commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from struct net_device")
- commit 6b352ebccbcf ("net: ethernet: broadcom: bcmgenet: use new api ethtool_{get|set}_link_ksettings")
The first patch add a regression on this driver, so it should be reverted.
As the second patch depend on the former, it should be reverted too.
The first patch is buggy because there is a "trick" in this driver.
The structure phydev is kept in the private data when the interface
go down, and used when the interface go up to enable the phy before
the function phy_connect is called.
I don't have this hardware, neither the datasheet. So I won't
update the driver to avoid this trick.
But the real goal of the first serie was to move to the new api
ethtool_{get|set}_link_ksettings. So I provide a new version of
the patch without the "cleaning" of driver to use the phydev
store in the net_device structure.
Jaedon Shin (1):
Revert "net: ethernet: bcmgenet: use phydev from struct net_device"
Philippe Reynes (2):
Revert "net: ethernet: bcmgenet: use new api
ethtool_{get|set}_link_ksettings"
net: ethernet: broadcom: bcmgenet: use new api
ethtool_{get|set}_link_ksettings
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 33 ++++++++++++-----------
drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
drivers/net/ethernet/broadcom/genet/bcmmii.c | 24 +++++++++--------
3 files changed, 31 insertions(+), 27 deletions(-)
--
1.7.4.4
^ permalink raw reply
* Re: [PATCH net-next] Revert "net: ethernet: bcmgenet: use phydev from struct net_device"
From: Florian Fainelli @ 2016-09-25 15:30 UTC (permalink / raw)
To: David Miller; +Cc: netdev, tremyfr, jaedon.shin
In-Reply-To: <20160925.053531.2196576594458792866.davem@davemloft.net>
Le 25/09/2016 à 02:35, David Miller a écrit :
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Sat, 24 Sep 2016 20:06:12 -0700
>
>> This reverts commit 62469c76007e ("net: ethernet: bcmgenet: use phydev
>> from struct net_device") because it causes GENETv1/2/3 adapters to
>> expose the following behavior after an ifconfig down/up sequence:
>
> This breaks the conversion of this driver to ethtool ksettings.
I must be missing something here, the proposed patch passes priv->phydev
to the ethtool ksettings helpers, is there something else that needs to
be taken care of?
Sorry for being slow to understand here...
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] net/sched: pkt_cls: change tc actions order to be as the user sets
From: Jamal Hadi Salim @ 2016-09-25 14:39 UTC (permalink / raw)
To: Hadar Hen Zion, David S. Miller; +Cc: netdev, Cong Wang, Or Gerlitz
In-Reply-To: <1474812524-29089-1-git-send-email-hadarh@mellanox.com>
On 16-09-25 10:08 AM, Hadar Hen Zion wrote:
> Currently the created tc actions list is reversed against the order
> set by the user.
> Change the actions list order to be the same as was set by the user.
>
Did something break? It seems to matter most for dumping. But even that
didnt breaking. Looking at the latest net tree, i tried:
====
sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 u32 \
match ip protocol 1 0xff flowid 1:2 \
action skbedit prio 33 \
action ife encode type 0xDEAD \
use mark 12 allow prio dst 02:15:15:15:15:15
Then I dump:
=====
jhs@jhs-foobar:~$ sudo $TC -s filter ls dev $ETH parent 1: protocol ip
filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:2
(rule hit 12 success 2)
match 00010000/00ff0000 at 8 (success 2 )
action order 1: skbedit priority :33
index 2 ref 1 bind 1 installed 604 sec used first 589 sec last 583 sec
Action statistics:
Sent 196 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
action order 2: ife encode action pipe type 0xDEAD
use mark 12 allow prio dst 02:15:15:15:15:15
index 2 ref 1 bind 1 installed 604 sec used first 589 sec last 583 sec
Action statistics:
Sent 196 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
-----
cheers,
jamal
^ permalink raw reply
* [PATCH net-next] net/sched: pkt_cls: change tc actions order to be as the user sets
From: Hadar Hen Zion @ 2016-09-25 14:08 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Jamal Hadi Salim, Cong Wang, Or Gerlitz, Hadar Hen Zion
Currently the created tc actions list is reversed against the order
set by the user.
Change the actions list order to be the same as was set by the user.
Signed-off-by: Hadar Hen Zion <hadarh@mellanox.com>
---
include/net/pkt_cls.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 5ccaa4b..767b03a 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -123,7 +123,7 @@ static inline void tcf_exts_to_list(const struct tcf_exts *exts,
for (i = 0; i < exts->nr_actions; i++) {
struct tc_action *a = exts->actions[i];
- list_add(&a->list, actions);
+ list_add_tail(&a->list, actions);
}
#endif
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net v2 0/2] Fix tc-ife bugs
From: Jamal Hadi Salim @ 2016-09-25 13:46 UTC (permalink / raw)
To: Yotam Gigi, davem, netdev, yotamg
In-Reply-To: <1474806711-64660-1-git-send-email-yotamg@mellanox.com>
On 16-09-25 08:31 AM, Yotam Gigi wrote:
> This patch-set contains two bugfixes in the tc-ife action, one fixing some
> random behaviour in encode side, and one fixing the decode side packet
> parsing logic.
>
> Yotam Gigi (2):
> act_ife: Fix external mac header on encode
> act_ife: Fix false parsing on decode side
>
> net/sched/act_ife.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
Yotam, did you run the test i proposed? I am worried about the TLV one.
Note:
Encoder includes the length of the TLV header length in the L part.
If you are reaching a conclusion that you need this in the decoding:
+ tlvdata += alen + sizeof(struct meta_tlvhdr);
then very likely whoever is sending that packet is not encoding
correctly.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next 4/4] net/sched: act_mirred: Implement ingress actions
From: Jamal Hadi Salim @ 2016-09-25 13:39 UTC (permalink / raw)
To: Cong Wang, Shmulik Ladkani
Cc: David S. Miller, Eric Dumazet, Linux Kernel Network Developers,
Florian Westphal
In-Reply-To: <CAM_iQpU+7NDoGGPkkDHA=v5XW5z98GHQhstFg5H=WCyiBzA-eA@mail.gmail.com>
On 16-09-24 08:07 PM, Cong Wang wrote:
> On Thu, Sep 22, 2016 at 10:11 PM, Shmulik Ladkani
>
> One problem to use your code for us is that, the RX side of veth
> is inside containers, not visible to outside, perhaps we need some
> more parameter to tell the netns before the device name/index?
> Thoughts?
>
Intriguing - but this would apply for only veth?
>>
>>> It may be around preventing loops maybe.
>>
>> Could be, but personally, I treat these constructs as (powerful)
>> building blocks, and "with great power comes great responsibility".
>>
>> Even today, one may create loops using existing 'egress redirect',
>> e.g. this rediculously errorneous construct:
>>
>> # ip l add v0 type veth peer name v0p
>> # tc filter add dev v0p parent ffff: basic \
>> action mirred egress redirect dev v0
>
> Detecting such loops should not be hard technically, like we do
> for reclassification. We might need some bits in skb to detect
> this specific case.
Note my other email. We had the feature but we took it out to save bits
on the skb.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next 4/4] net/sched: act_mirred: Implement ingress actions
From: Jamal Hadi Salim @ 2016-09-25 13:05 UTC (permalink / raw)
To: Shmulik Ladkani
Cc: David S. Miller, WANG Cong, Eric Dumazet, netdev,
Florian Westphal
In-Reply-To: <20160923184030.75124289@halley>
On 16-09-23 11:40 AM, Shmulik Ladkani wrote:
> On Fri, 23 Sep 2016 08:48:33 -0400 Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> Even today, one may create loops using existing 'egress redirect',
>>> e.g. this rediculously errorneous construct:
>>>
>>> # ip l add v0 type veth peer name v0p
>>> # tc filter add dev v0p parent ffff: basic \
>>> action mirred egress redirect dev v0
>>
>> I think we actually recover from this one by eventually
>> dropping (theres a ttl field).
>
> [off topic]
>
> Don't know about that :) cpu fan got very noisy, 3 of 4 cores at 100%,
> and after one second I got:
>
> # ip -s l show type veth
> 16: v0p@v0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> link/ether a2:64:ff:10:dd:85 brd ff:ff:ff:ff:ff:ff
> RX: bytes packets errors dropped overrun mcast
> 71660305923 469890864 0 0 0 0
> TX: bytes packets errors dropped carrier collsns
> 3509 24 0 0 0 0
> 17: v0@v0p: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> link/ether 52:a2:34:f6:7c:ec brd ff:ff:ff:ff:ff:ff
> RX: bytes packets errors dropped overrun mcast
> 3509 24 0 0 0 0
> TX: bytes packets errors dropped carrier collsns
> 71660713017 469893555 0 0 0 0
>
I think this is still on topic!
Now I realize that code we took out around 4.2.x is still useful
for such a use case (I wasnt thinking about veth when Florian was
slimming the skb);
+Cc Florian W.
This snippet from 4.2:
-------------
3525 static int ing_filter(struct sk_buff *skb, struct netdev_queue *rxq)
3526 {
3527 struct net_device *dev = skb->dev;
3528 u32 ttl = G_TC_RTTL(skb->tc_verd);
3529 int result = TC_ACT_OK;
3530 struct Qdisc *q;
3531
3532 if (unlikely(MAX_RED_LOOP < ttl++)) {
3533 net_warn_ratelimited("Redir loop detected Dropping
packet (%d->%d)\n",
3534 skb->skb_iif, dev->ifindex);
3535 return TC_ACT_SHOT;
3536 }
3537
3538 skb->tc_verd = SET_TC_RTTL(skb->tc_verd, ttl);
3539 skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_INGRESS);
3540
3541 q = rcu_dereference(rxq->qdisc);
3542 if (q != &noop_qdisc) {
3543 spin_lock(qdisc_lock(q));
3544 if (likely(!test_bit(__QDISC_STATE_DEACTIVATED,
&q->state)))
3545 result = qdisc_enqueue_root(skb, q);
3546 spin_unlock(qdisc_lock(q));
3547 }
3548
3549 return result;
3550 }
--------------------
MAX_RED_LOOP (stands for "Maximum Redirect loop") still exists in
current code. The idea above was that we would increment the rttl
counter once and if we saw it again upto MAX_RED_LOOP we would assume
a loop and drop the packet (at the time i didnt think it was wise to
let the actions be in charge of setting the RTTL; it had to be central
core code - but it may not be neccessary)
Florian, when we discussed I said it was fine to reclaim those 3 bits
on tc verdict for RTTL at the time because i had taken out the
feature and never added it back. Your comment at the time was we can
add it back when someone shows up with the feature.
Shmulik is looking to add it.
> Similarly to all constructs injecting skbs to device rx (bond/team,
> vlan, macvlan, tunnels, ifb, __dev_forward_skb callers, etc..), we are
> obligated to assign 'skb2->dev' as the new rx device.
>
> Regarding 'skb2->skb_iif', original act_mirred code already has:
>
> skb2->skb_iif = skb->dev->ifindex; <--- THIS IS ORIG DEV IIF
> skb2->dev = dev; <--- THIS IS TARGET DEV
> err = dev_queue_xmit(skb2);
>
> I'm preserving this; OTOH the suggested modification in the patch is
>
> - err = dev_queue_xmit(skb2);
> + if (tcf_mirred_act_direction(m->tcfm_eaction) & AT_EGRESS)
> + err = dev_queue_xmit(skb2);
> + else
> + netif_receive_skb(skb2);
>
> now, the call to 'netif_receive_skb' will eventually override skb_iif to
> the target RX dev's index, upon entry to __netif_receive_skb_core.
>
> I think this IS the expected behavior - as done by other "rx injection"
> constructs.
>
Sounds fine.
I am wondering if we can have a tracing feature to show the lifetime of
the packet as it is being cycled around the kernel? It would help
debugging if some policy misbehaves.
> My doubts were around whether we should call 'dev_forward_skb' instead
> of 'netif_receive_skb'.
> The former does some things I assumed we're not interested of, like
> testing 'is_skb_forwardable' and re-running 'eth_type_trans'.
> OTOH, it DOES scrub the skb.
> Maybe we should scrub it as well prior the netif_receive_skb call?
>
Scrubbing the skb could be a bad idea if it gets rid of global state
like the RTTL if you add it back.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH v3 2/2] netfilter: Create revision 2 of xt_hashlimit to support higher pps rates
From: Pablo Neira Ayuso @ 2016-09-25 12:58 UTC (permalink / raw)
To: Vishwanath Pai
Cc: Jan Engelhardt, kaber, kadlec, johunt, netfilter-devel, coreteam,
netdev, pai.vishwain
In-Reply-To: <20160925113501.GD8331@salvia>
On Sun, Sep 25, 2016 at 01:35:01PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 22, 2016 at 02:39:45PM -0400, Vishwanath Pai wrote:
> > Thanks for pointing this out, I will reorder the fields to:
> >
> > struct hashlimit_cfg2 {
> > __u64 avg; /* Average secs between packets * scale */
> > __u64 burst;
> > __u32 mode; /* bitmask of XT_HASHLIMIT_HASH_* */
> >
> > This should fix the hole and avoid padding.
>
> I have manually mangled this here, and applied, to get this change in
> this in merge window.
>
> I also have to revert the rename of XT_HASHLIMIT_SCALE to
> XT_HASHLIMIT_SCALE_v1. This is exposed through uapi, this change would
> break userspace.
This requires that you rebase and resubmit your userspace patches for
iptables though.
Thanks for your patience to get this in, btw.
^ permalink raw reply
* (unknown)
From: David Miller @ 2016-09-25 12:51 UTC (permalink / raw)
To: zenczykowski; +Cc: maze, netdev, ek, lorenzo
In-Reply-To: <1474800749-2306-1-git-send-email-zenczykowski@gmail.com>
This posting needs an actual Subject line, saying something like:
[PATCH net-next v3 0/7] Add RFC7559 style ipv6 soliciation backoff support
This text will go into the merge commit I create should I apply
this patch series.
In any event, using a blank Subject line is never appropriate.
^ 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