* [PATCH v4 2/7] Documentation: devicetree: add PHY lane swap binding
From: Jon Mason @ 2016-11-01 20:04 UTC (permalink / raw)
To: David Miller, Rob Herring, Mark Rutland, Florian Fainelli
Cc: rafal, bcm-kernel-feedback-list, netdev, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <1478030682-27012-1-git-send-email-jon.mason@broadcom.com>
Add the documentation for PHY lane swapping. This is a boolean entry to
notify the phy device drivers that the TX/RX lanes need to be swapped.
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
Documentation/devicetree/bindings/net/phy.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index bc1c3c8..3dce607 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -35,6 +35,9 @@ Optional Properties:
- broken-turn-around: If set, indicates the PHY device does not correctly
release the turn around line low at the end of a MDIO transaction.
+- enet-phy-lane-swap: If set, indicates the PHY device requires swapping the
+ TX/RX lanes to function properly.
+
Example:
ethernet-phy@0 {
--
2.7.4
^ permalink raw reply related
* [PATCH v4 1/7] net: phy: broadcom: add bcm54xx_auxctl_read
From: Jon Mason @ 2016-11-01 20:04 UTC (permalink / raw)
To: David Miller, Rob Herring, Mark Rutland, Florian Fainelli
Cc: devicetree, netdev, linux-kernel, bcm-kernel-feedback-list, rafal,
linux-arm-kernel
In-Reply-To: <1478030682-27012-1-git-send-email-jon.mason@broadcom.com>
Add a helper function to read the AUXCTL register for the BCM54xx. This
mirrors the bcm54xx_auxctl_write function already present in the code.
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
drivers/net/phy/broadcom.c | 10 ++++++++++
include/linux/brcmphy.h | 1 +
2 files changed, 11 insertions(+)
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 583ef8a..3a64b3d 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -30,6 +30,16 @@ MODULE_DESCRIPTION("Broadcom PHY driver");
MODULE_AUTHOR("Maciej W. Rozycki");
MODULE_LICENSE("GPL");
+static int bcm54xx_auxctl_read(struct phy_device *phydev, u16 regnum)
+{
+ /* The register must be written to both the Shadow Register Select and
+ * the Shadow Read Register Selector
+ */
+ phy_write(phydev, MII_BCM54XX_AUX_CTL, regnum |
+ regnum << MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT);
+ return phy_read(phydev, MII_BCM54XX_AUX_CTL);
+}
+
static int bcm54xx_auxctl_write(struct phy_device *phydev, u16 regnum, u16 val)
{
return phy_write(phydev, MII_BCM54XX_AUX_CTL, regnum | val);
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 60def78..0ed6691 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -110,6 +110,7 @@
#define MII_BCM54XX_AUXCTL_MISC_FORCE_AMDIX 0x0200
#define MII_BCM54XX_AUXCTL_MISC_RDSEL_MISC 0x7000
#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC 0x0007
+#define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT 12
#define MII_BCM54XX_AUXCTL_SHDWSEL_MASK 0x0007
--
2.7.4
^ permalink raw reply related
* [PATCH v4 0/7] add NS2 support to bgmac
From: Jon Mason @ 2016-11-01 20:04 UTC (permalink / raw)
To: David Miller, Rob Herring, Mark Rutland, Florian Fainelli
Cc: rafal, bcm-kernel-feedback-list, netdev, devicetree,
linux-arm-kernel, linux-kernel
Changes in v4:
* Actually send out the lane swap binding doc patch (Per Scott Branden)
* Remove unused #define (Per Andrew Lunn)
Changes in v3:
* Clean-up the bgmac DT binding doc (per Rob Herring)
* Document the lane swap binding and make it generic (Per Andrew Lunn)
Changes in v2:
* Remove the PHY power-on (per Andrew Lunn)
* Misc PHY clean-ups regarding comments and #defines (per Andrew Lunn)
This results on none of the original PHY code from Vikas being
present. So, I'm removing him as an author and giving him
"Inspired-by" credit.
* Move PHY lane swapping to PHY driver (per Andrew Lunn and Florian
Fainelli)
* Remove bgmac sleep (per Florian Fainelli)
* Re-add bgmac chip reset (per Florian Fainelli and Ray Jui)
* Rebased on latest net-next
* Added patch for bcm54xx_auxctl_read, which is used in the BCM54810
Add support for the amac found in the Broadcom Northstar2 SoC to the
bgmac driver. This necessitates adding support to connect to an
externally defined phy (as described in the device tree) in the driver.
These phy changes are in addition to the changes necessary to get NS2
working.
Jon Mason (7):
net: phy: broadcom: add bcm54xx_auxctl_read
Documentation: devicetree: add PHY lane swap binding
net: phy: broadcom: Add BCM54810 PHY entry
Documentation: devicetree: net: add NS2 bindings to amac
net: ethernet: bgmac: device tree phy enablement
net: ethernet: bgmac: add NS2 support
arm64: dts: NS2: add AMAC ethernet support
.../devicetree/bindings/net/brcm,amac.txt | 16 ++--
Documentation/devicetree/bindings/net/phy.txt | 3 +
arch/arm64/boot/dts/broadcom/ns2-svk.dts | 5 ++
arch/arm64/boot/dts/broadcom/ns2.dtsi | 12 +++
drivers/net/ethernet/broadcom/bgmac-bcma.c | 48 ++++++++++
drivers/net/ethernet/broadcom/bgmac-platform.c | 100 ++++++++++++++++++++-
drivers/net/ethernet/broadcom/bgmac.c | 55 ++----------
drivers/net/ethernet/broadcom/bgmac.h | 8 ++
drivers/net/phy/Kconfig | 2 +-
drivers/net/phy/broadcom.c | 68 +++++++++++++-
include/linux/brcmphy.h | 10 +++
11 files changed, 270 insertions(+), 57 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH v4 net-next] lan78xx: Use irq_domain for phy interrupt from USB Int. EP
From: Woojung.Huh @ 2016-11-01 20:02 UTC (permalink / raw)
To: davem, netdev; +Cc: f.fainelli, andrew, UNGLinuxDriver
From: Woojung Huh <woojung.huh@microchip.com>
To utilize phylib with interrupt fully than handling some of phy stuff in the MAC driver,
create irq_domain for USB interrupt EP of phy interrupt and
pass the irq number to phy_connect_direct() instead of PHY_IGNORE_INTERRUPT.
Idea comes from drivers/gpio/gpio-dl2.c
Signed-off-by: Woojung Huh <woojung.huh@microchip.com>
---
drivers/net/usb/lan78xx.c | 215 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 189 insertions(+), 26 deletions(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index c4e748e..bcd9010 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -30,13 +30,17 @@
#include <linux/ipv6.h>
#include <linux/mdio.h>
#include <net/ip6_checksum.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
#include <linux/microchipphy.h>
#include "lan78xx.h"
#define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@microchip.com>"
#define DRIVER_DESC "LAN78XX USB 3.0 Gigabit Ethernet Devices"
#define DRIVER_NAME "lan78xx"
-#define DRIVER_VERSION "1.0.4"
+#define DRIVER_VERSION "1.0.5"
#define TX_TIMEOUT_JIFFIES (5 * HZ)
#define THROTTLE_JIFFIES (HZ / 8)
@@ -89,6 +93,38 @@
/* statistic update interval (mSec) */
#define STAT_UPDATE_TIMER (1 * 1000)
+/* defines interrupts from interrupt EP */
+#define MAX_INT_EP (32)
+#define INT_EP_INTEP (31)
+#define INT_EP_OTP_WR_DONE (28)
+#define INT_EP_EEE_TX_LPI_START (26)
+#define INT_EP_EEE_TX_LPI_STOP (25)
+#define INT_EP_EEE_RX_LPI (24)
+#define INT_EP_MAC_RESET_TIMEOUT (23)
+#define INT_EP_RDFO (22)
+#define INT_EP_TXE (21)
+#define INT_EP_USB_STATUS (20)
+#define INT_EP_TX_DIS (19)
+#define INT_EP_RX_DIS (18)
+#define INT_EP_PHY (17)
+#define INT_EP_DP (16)
+#define INT_EP_MAC_ERR (15)
+#define INT_EP_TDFU (14)
+#define INT_EP_TDFO (13)
+#define INT_EP_UTX (12)
+#define INT_EP_GPIO_11 (11)
+#define INT_EP_GPIO_10 (10)
+#define INT_EP_GPIO_9 (9)
+#define INT_EP_GPIO_8 (8)
+#define INT_EP_GPIO_7 (7)
+#define INT_EP_GPIO_6 (6)
+#define INT_EP_GPIO_5 (5)
+#define INT_EP_GPIO_4 (4)
+#define INT_EP_GPIO_3 (3)
+#define INT_EP_GPIO_2 (2)
+#define INT_EP_GPIO_1 (1)
+#define INT_EP_GPIO_0 (0)
+
static const char lan78xx_gstrings[][ETH_GSTRING_LEN] = {
"RX FCS Errors",
"RX Alignment Errors",
@@ -296,6 +332,15 @@ struct statstage {
struct lan78xx_statstage64 curr_stat;
};
+struct irq_domain_data {
+ struct irq_domain *irqdomain;
+ unsigned int phyirq;
+ struct irq_chip *irqchip;
+ irq_flow_handler_t irq_handler;
+ u32 irqenable;
+ struct mutex irq_lock; /* for irq bus access */
+};
+
struct lan78xx_net {
struct net_device *net;
struct usb_device *udev;
@@ -351,6 +396,8 @@ struct lan78xx_net {
int delta;
struct statstage stats;
+
+ struct irq_domain_data domain_data;
};
/* use ethtool to change the level for any given device */
@@ -1096,11 +1143,6 @@ static int lan78xx_link_reset(struct lan78xx_net *dev)
int ladv, radv, ret;
u32 buf;
- /* clear PHY interrupt status */
- ret = phy_read(phydev, LAN88XX_INT_STS);
- if (unlikely(ret < 0))
- return -EIO;
-
/* clear LAN78xx interrupt status */
ret = lan78xx_write_reg(dev, INT_STS, INT_STS_PHY_INT_);
if (unlikely(ret < 0))
@@ -1120,16 +1162,12 @@ static int lan78xx_link_reset(struct lan78xx_net *dev)
if (unlikely(ret < 0))
return -EIO;
- phy_mac_interrupt(phydev, 0);
-
del_timer(&dev->stat_monitor);
} else if (phydev->link && !dev->link_on) {
dev->link_on = true;
phy_ethtool_ksettings_get(phydev, &ecmd);
- ret = phy_read(phydev, LAN88XX_INT_STS);
-
if (dev->udev->speed == USB_SPEED_SUPER) {
if (ecmd.base.speed == 1000) {
/* disable U2 */
@@ -1163,7 +1201,6 @@ static int lan78xx_link_reset(struct lan78xx_net *dev)
ret = lan78xx_update_flowcontrol(dev, ecmd.base.duplex, ladv,
radv);
- phy_mac_interrupt(phydev, 1);
if (!timer_pending(&dev->stat_monitor)) {
dev->delta = 1;
@@ -1202,7 +1239,10 @@ static void lan78xx_status(struct lan78xx_net *dev, struct urb *urb)
if (intdata & INT_ENP_PHY_INT) {
netif_dbg(dev, link, dev->net, "PHY INTR: 0x%08x\n", intdata);
- lan78xx_defer_kevent(dev, EVENT_LINK_RESET);
+ lan78xx_defer_kevent(dev, EVENT_LINK_RESET);
+
+ if (dev->domain_data.phyirq > 0)
+ generic_handle_irq(dev->domain_data.phyirq);
} else
netdev_warn(dev->net,
"unexpected interrupt: 0x%08x\n", intdata);
@@ -1844,6 +1884,127 @@ static void lan78xx_link_status_change(struct net_device *net)
}
}
+static int irq_map(struct irq_domain *d, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ struct irq_domain_data *data = d->host_data;
+
+ irq_set_chip_data(irq, data);
+ irq_set_chip_and_handler(irq, data->irqchip, data->irq_handler);
+ irq_set_noprobe(irq);
+
+ return 0;
+}
+
+static void irq_unmap(struct irq_domain *d, unsigned int irq)
+{
+ irq_set_chip_and_handler(irq, NULL, NULL);
+ irq_set_chip_data(irq, NULL);
+}
+
+static const struct irq_domain_ops chip_domain_ops = {
+ .map = irq_map,
+ .unmap = irq_unmap,
+};
+
+static void lan78xx_irq_mask(struct irq_data *irqd)
+{
+ struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
+
+ data->irqenable &= ~BIT(irqd_to_hwirq(irqd));
+}
+
+static void lan78xx_irq_unmask(struct irq_data *irqd)
+{
+ struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
+
+ data->irqenable |= BIT(irqd_to_hwirq(irqd));
+}
+
+static void lan78xx_irq_bus_lock(struct irq_data *irqd)
+{
+ struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
+
+ mutex_lock(&data->irq_lock);
+}
+
+static void lan78xx_irq_bus_sync_unlock(struct irq_data *irqd)
+{
+ struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
+ struct lan78xx_net *dev =
+ container_of(data, struct lan78xx_net, domain_data);
+ u32 buf;
+ int ret;
+
+ /* call register access here because irq_bus_lock & irq_bus_sync_unlock
+ * are only two callbacks executed in non-atomic contex.
+ */
+ ret = lan78xx_read_reg(dev, INT_EP_CTL, &buf);
+ if (buf != data->irqenable)
+ ret = lan78xx_write_reg(dev, INT_EP_CTL, data->irqenable);
+
+ mutex_unlock(&data->irq_lock);
+}
+
+static struct irq_chip lan78xx_irqchip = {
+ .name = "lan78xx-irqs",
+ .irq_mask = lan78xx_irq_mask,
+ .irq_unmask = lan78xx_irq_unmask,
+ .irq_bus_lock = lan78xx_irq_bus_lock,
+ .irq_bus_sync_unlock = lan78xx_irq_bus_sync_unlock,
+};
+
+static int lan78xx_setup_irq_domain(struct lan78xx_net *dev)
+{
+ struct device_node *of_node;
+ struct irq_domain *irqdomain;
+ unsigned int irqmap = 0;
+ u32 buf;
+ int ret = 0;
+
+ of_node = dev->udev->dev.parent->of_node;
+
+ mutex_init(&dev->domain_data.irq_lock);
+
+ lan78xx_read_reg(dev, INT_EP_CTL, &buf);
+ dev->domain_data.irqenable = buf;
+
+ dev->domain_data.irqchip = &lan78xx_irqchip;
+ dev->domain_data.irq_handler = handle_simple_irq;
+
+ irqdomain = irq_domain_add_simple(of_node, MAX_INT_EP, 0,
+ &chip_domain_ops, &dev->domain_data);
+ if (irqdomain) {
+ /* create mapping for PHY interrupt */
+ irqmap = irq_create_mapping(irqdomain, INT_EP_PHY);
+ if (!irqmap) {
+ irq_domain_remove(irqdomain);
+
+ irqdomain = NULL;
+ ret = -EINVAL;
+ }
+ } else {
+ ret = -EINVAL;
+ }
+
+ dev->domain_data.irqdomain = irqdomain;
+ dev->domain_data.phyirq = irqmap;
+
+ return ret;
+}
+
+static void lan78xx_remove_irq_domain(struct lan78xx_net *dev)
+{
+ if (dev->domain_data.phyirq > 0) {
+ irq_dispose_mapping(dev->domain_data.phyirq);
+
+ if (dev->domain_data.irqdomain)
+ irq_domain_remove(dev->domain_data.irqdomain);
+ }
+ dev->domain_data.phyirq = 0;
+ dev->domain_data.irqdomain = NULL;
+}
+
static int lan78xx_phy_init(struct lan78xx_net *dev)
{
int ret;
@@ -1856,15 +2017,12 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
return -EIO;
}
- /* Enable PHY interrupts.
- * We handle our own interrupt
- */
- ret = phy_read(phydev, LAN88XX_INT_STS);
- ret = phy_write(phydev, LAN88XX_INT_MASK,
- LAN88XX_INT_MASK_MDINTPIN_EN_ |
- LAN88XX_INT_MASK_LINK_CHANGE_);
-
- phydev->irq = PHY_IGNORE_INTERRUPT;
+ /* if phyirq is not set, use polling mode in phylib */
+ if (dev->domain_data.phyirq > 0)
+ phydev->irq = dev->domain_data.phyirq;
+ else
+ phydev->irq = 0;
+ netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
ret = phy_connect_direct(dev->net, phydev,
lan78xx_link_status_change,
@@ -2255,11 +2413,6 @@ static int lan78xx_reset(struct lan78xx_net *dev)
buf |= MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_;
ret = lan78xx_write_reg(dev, MAC_CR, buf);
- /* enable PHY interrupts */
- ret = lan78xx_read_reg(dev, INT_EP_CTL, &buf);
- buf |= INT_ENP_PHY_INT;
- ret = lan78xx_write_reg(dev, INT_EP_CTL, buf);
-
ret = lan78xx_read_reg(dev, MAC_TX, &buf);
buf |= MAC_TX_TXEN_;
ret = lan78xx_write_reg(dev, MAC_TX, buf);
@@ -2668,6 +2821,14 @@ static int lan78xx_bind(struct lan78xx_net *dev, struct usb_interface *intf)
dev->net->hw_features = dev->net->features;
+ ret = lan78xx_setup_irq_domain(dev);
+ if (ret < 0) {
+ netdev_warn(dev->net,
+ "lan78xx_setup_irq_domain() failed : %d", ret);
+ kfree(pdata);
+ return ret;
+ }
+
/* Init all registers */
ret = lan78xx_reset(dev);
@@ -2684,6 +2845,8 @@ static void lan78xx_unbind(struct lan78xx_net *dev, struct usb_interface *intf)
{
struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
+ lan78xx_remove_irq_domain(dev);
+
lan78xx_remove_mdio(dev);
if (pdata) {
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3 0/6] add NS2 support to bgmac
From: Jon Mason @ 2016-11-01 20:01 UTC (permalink / raw)
To: Scott Branden
Cc: David Miller, Rob Herring, Mark Rutland, Florian Fainelli, rafal,
bcm-kernel-feedback-list, netdev, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <cc2554bf-2700-9d9b-b012-15e82681b31b@broadcom.com>
On Tue, Nov 01, 2016 at 11:01:19AM -0700, Scott Branden wrote:
> Hi Jon,
>
> On 16-11-01 10:51 AM, Jon Mason wrote:
> >Changes in v3:
> >* Clean-up the bgmac DT binding doc (per Rob Herring)
> >* Document the lane swap binding and make it generic (Per Andrew Lunn)
> Where is the documentation of the lane swap binding?
Sent out the wrong branch :(
Sending out v4 shortly with this patch.
>
> >
> >
> >Changes in v2:
> >* Remove the PHY power-on (per Andrew Lunn)
> >* Misc PHY clean-ups regarding comments and #defines (per Andrew Lunn)
> > This results on none of the original PHY code from Vikas being
> > present. So, I'm removing him as an author and giving him
> > "Inspired-by" credit.
> >* Move PHY lane swapping to PHY driver (per Andrew Lunn and Florian
> > Fainelli)
> >* Remove bgmac sleep (per Florian Fainelli)
> >* Re-add bgmac chip reset (per Florian Fainelli and Ray Jui)
> >* Rebased on latest net-next
> >* Added patch for bcm54xx_auxctl_read, which is used in the BCM54810
> >
> >
> >Add support for the amac found in the Broadcom Northstar2 SoC to the
> >bgmac driver. This necessitates adding support to connect to an
> >externally defined phy (as described in the device tree) in the driver.
> >These phy changes are in addition to the changes necessary to get NS2
> >working.
> >
> >
> >Jon Mason (6):
> > net: phy: broadcom: add bcm54xx_auxctl_read
> > net: phy: broadcom: Add BCM54810 PHY entry
> > Documentation: devicetree: net: add NS2 bindings to amac
> > net: ethernet: bgmac: device tree phy enablement
> > net: ethernet: bgmac: add NS2 support
> > arm64: dts: NS2: add AMAC ethernet support
> >
> > .../devicetree/bindings/net/brcm,amac.txt | 16 ++--
> > arch/arm64/boot/dts/broadcom/ns2-svk.dts | 5 ++
> > arch/arm64/boot/dts/broadcom/ns2.dtsi | 12 +++
> > drivers/net/ethernet/broadcom/bgmac-bcma.c | 48 ++++++++++
> > drivers/net/ethernet/broadcom/bgmac-platform.c | 100 ++++++++++++++++++++-
> > drivers/net/ethernet/broadcom/bgmac.c | 55 ++----------
> > drivers/net/ethernet/broadcom/bgmac.h | 8 ++
> > drivers/net/phy/Kconfig | 2 +-
> > drivers/net/phy/broadcom.c | 68 +++++++++++++-
> > include/linux/brcmphy.h | 11 +++
> > 10 files changed, 268 insertions(+), 57 deletions(-)
> >
^ permalink raw reply
* Re: [PATCH v3 2/6] net: phy: broadcom: Add BCM54810 PHY entry
From: Jon Mason @ 2016-11-01 19:59 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Rob Herring, Mark Rutland, Florian Fainelli,
devicetree, netdev, linux-kernel, bcm-kernel-feedback-list, rafal,
linux-arm-kernel
In-Reply-To: <20161101184639.GI10785@lunn.ch>
On Tue, Nov 01, 2016 at 07:46:39PM +0100, Andrew Lunn wrote:
> Hi Jon
>
> > @@ -56,6 +57,8 @@
> > #define PHY_BRCM_EXT_IBND_TX_ENABLE 0x00002000
> > #define PHY_BRCM_CLEAR_RGMII_MODE 0x00004000
> > #define PHY_BRCM_DIS_TXCRXC_NOENRGY 0x00008000
> > +#define PHY_BRCM_EXP_LANE_SWAP 0x00010000
> > +
>
> You define this, but don't use it...
Good catch. Cruft from the last patch. I'll sendout a quick v4 to
address this and the missing Documentation patch.
>
> Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 0/5] bpf: BPF for lightweight tunnel encapsulation
From: Thomas Graf @ 2016-11-01 19:59 UTC (permalink / raw)
To: Tom Herbert
Cc: David S. Miller, Alexei Starovoitov, Daniel Borkmann, roopa,
Linux Kernel Network Developers
In-Reply-To: <CALx6S35FYAQWk1p_PztBJ_ojio6OOdeYDb-AYQwZ0juxPj5gYQ@mail.gmail.com>
On 1 November 2016 at 12:27, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Nov 1, 2016 at 12:11 PM, Thomas Graf <tgraf@suug.ch> wrote:
>> You can do the same with act_pedit or cls_bpf at dev_queue_xmit()
>> before or after you go into the encapsulation device. This is a tool
>> for root, if you install a drop all ssh rule then you won't be able to
>> log into the box anymore. If you modify the packet and render it
>> invalid, the packet will be dropped.
>> If you attach a VLAN header while VLAN offload is already set up, then
>> the hardware will add another VLAN header, this is what I would
>> expect. I truly hope that we don't have code which crashes if we
>> dev_queue_xmit() garbage into any device. That would be horrible.
>>
>> Do you have a specific example that could be a problem?
>
> I believe Alexander was dealing with problems where drivers were not
> properly handling IP extension headers. We regularly get reports about
There are many ways to cause IP extension headers to be inserted. How
is this specific to BPF or this series?
> driver checksum failures on edge conditions. Making a fully functional
Not sure what an "edge condition" is? Untrusted networks? How is
drivers crashing on receive related to this series?
> and efficient transmit data path is nontrivial, there are many
> assumptions being made some documented some not. When drivers crash we
> either fix the driver or the protocol stack that is doing something
> bad.
Tom, we have a dozen ways to modify packet content already and we have
multiple ways to take raw packet data from userspace and inject them
at dev_queue_xmit() level and some even above. What is different for
lwtunnel_xmit()?
This is entirely optional and nobody is forced to use any of this. If
you don't trust the BPF program then you better not run in. It's about
the same as trusting a random tc filter or iptables ruleset. The
important point is that integrity is maintained at all times.I would
love to address any specific concern.
^ permalink raw reply
* Re: [PATCH v2 net-next] tcp: enhance tcp collapsing
From: Neal Cardwell @ 2016-11-01 19:35 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ilya Lesokhin, David Miller, netdev@vger.kernel.org,
Ilpo Järvinen, Yuchung Cheng
In-Reply-To: <1478022822.7065.351.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, Nov 1, 2016 at 1:53 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> As Ilya Lesokhin suggested, we can collapse two skbs at retransmit
> time even if the skb at the right has fragments.
>
> We simply have to use more generic skb_copy_bits() instead of
> skb_copy_from_linear_data() in tcp_collapse_retrans()
>
> Also need to guard this skb_copy_bits() in case there is nothing to
> copy, otherwise skb_put() could panic if left skb has frags.
>
> Tested:
>
> Used following packetdrill test
>
> // Establish a connection.
> 0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> +0 bind(3, ..., ...) = 0
> +0 listen(3, 1) = 0
>
> +0 < S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 8>
> +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8>
> +.100 < . 1:1(0) ack 1 win 257
> +0 accept(3, ..., ...) = 4
>
> +0 setsockopt(4, SOL_TCP, TCP_NODELAY, [1], 4) = 0
> +0 write(4, ..., 200) = 200
> +0 > P. 1:201(200) ack 1
> +.001 write(4, ..., 200) = 200
> +0 > P. 201:401(200) ack 1
> +.001 write(4, ..., 200) = 200
> +0 > P. 401:601(200) ack 1
> +.001 write(4, ..., 200) = 200
> +0 > P. 601:801(200) ack 1
> +.001 write(4, ..., 200) = 200
> +0 > P. 801:1001(200) ack 1
> +.001 write(4, ..., 100) = 100
> +0 > P. 1001:1101(100) ack 1
> +.001 write(4, ..., 100) = 100
> +0 > P. 1101:1201(100) ack 1
> +.001 write(4, ..., 100) = 100
> +0 > P. 1201:1301(100) ack 1
> +.001 write(4, ..., 100) = 100
> +0 > P. 1301:1401(100) ack 1
>
> +.100 < . 1:1(0) ack 1 win 257 <nop,nop,sack 1001:1401>
> // Check that TCP collapse works :
> +0 > P. 1:1001(1000) ack 1
>
>
> Reported-by: Ilya Lesokhin <ilyal@mellanox.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> ---
Acked-by: Neal Cardwell <ncardwell@google.com>
Thanks, Eric. :-)
neal
^ permalink raw reply
* Re: [PATCH net-next v2 0/5] bpf: BPF for lightweight tunnel encapsulation
From: Tom Herbert @ 2016-11-01 19:27 UTC (permalink / raw)
To: Thomas Graf
Cc: David S. Miller, Alexei Starovoitov, Daniel Borkmann, roopa,
Linux Kernel Network Developers
In-Reply-To: <CACby=pnLzihLCFzT-HvB3Ovo-hC1b79Dr6iO74y5mMqPspHFBg@mail.gmail.com>
On Tue, Nov 1, 2016 at 12:11 PM, Thomas Graf <tgraf@suug.ch> wrote:
> On 1 November 2016 at 11:51, Tom Herbert <tom@herbertland.com> wrote:
>> On Tue, Nov 1, 2016 at 11:20 AM, Thomas Graf <tgraf@suug.ch> wrote:
>>> The headers cannot be extended or reduced so the offsets always remain
>>> correct. What can happen is that the header contains invalid data.
>>>
>> If we can't add/remove headers then doesn't that really limit the
>> utility of these patches? My assumption was that BPF+LWT is needed to
>> allow users to define and implement their own encapsulations, EH
>> insertion, packet modification, etc.
>
> I thought you were specifically referring to lwtunnel_output(). You
> can extend headers at lwtunnel_xmit() but you can't for
> lwtunnel_output() and lwtunnel_input(). The xmit hook is in
> ip_finish_output2() where it is fine to do so. We have existing code
> doing this there.
>
>>> You keep saying this "middle in the stack" but the point is exactly
>>> the same as a raw socket with IPPROTO_RAW and hdrincl, see
>>> rawv6_sendmsg() and rawv6_send_hdrincl(). An IPv6 raw socket can feed
>>> arbitrary garbage into dst_output(). IPv4 does some minimal sanity
>>> checks.
>>>
>> What I mean is that an admin can create a BPF program that run on any
>> user packets (for instance default route could be set). This would be
>> in the path of TCP, UDP, and other protocols tightly integrated with
>> the stack. Packets being routed may be encapsulated, VLAN, have
>> checksum offload, GORed set etc. They also might be looped back in
>> which case the settings in skbuff become receive parameters.
>
> You can do the same with act_pedit or cls_bpf at dev_queue_xmit()
> before or after you go into the encapsulation device. This is a tool
> for root, if you install a drop all ssh rule then you won't be able to
> log into the box anymore. If you modify the packet and render it
> invalid, the packet will be dropped.
> If you attach a VLAN header while VLAN offload is already set up, then
> the hardware will add another VLAN header, this is what I would
> expect. I truly hope that we don't have code which crashes if we
> dev_queue_xmit() garbage into any device. That would be horrible.
>
> Do you have a specific example that could be a problem?
I believe Alexander was dealing with problems where drivers were not
properly handling IP extension headers. We regularly get reports about
driver checksum failures on edge conditions. Making a fully functional
and efficient transmit data path is nontrivial, there are many
assumptions being made some documented some not. When drivers crash we
either fix the driver or the protocol stack that is doing something
bad.
Tom
^ permalink raw reply
* Re: [PATCH] [RFC] net: phy: phy drivers should not set SUPPORTED_Pause or SUPPORTED_Asym_Pause
From: Timur Tabi @ 2016-11-01 19:19 UTC (permalink / raw)
To: Florian Fainelli, David Miller, netdev
In-Reply-To: <be1c80d3-654a-65c8-3c5b-9a7bfbf8817d@gmail.com>
On 11/01/2016 02:03 PM, Florian Fainelli wrote:
> The standard (IEEE Std 802.3-2008) describes this as the
> Auto-negotiation advertisement register and these bits are part of the
> Technology ability field register. These two bits A5 (PAUSE operation
> for full duplex links) and A6 (Asymetric PAUSE operation for full duplex
> links) have no cabling requirement, and earlier the paragraph mentions
> this is to be resolved by the "management" entity, so I don't think the
> PHY has any business in that other than reflecting what the MAC is
> capable of doing towards the link partner, but I could be reading the
> specification incorrectly.
Fair enough. I'll post a V2 patch that enables those bits in phy.c.
On a side note, is there any reason for the MAC to ever disable support
for accepting pause frames? If the PHY is configured to not advertise
support for receiving pause frames, then does it matter if the MAC is
configured to accept them? The MAC would never receive such a frame.
If so, then I can modify my other patch (net: qcom/emac: enable flow
control if requested) to always set RXFC, and have it set TXFC only if
the PHY will transmit pause frames.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH net-next v2 0/5] bpf: BPF for lightweight tunnel encapsulation
From: Thomas Graf @ 2016-11-01 19:11 UTC (permalink / raw)
To: Tom Herbert
Cc: David S. Miller, Alexei Starovoitov, Daniel Borkmann, roopa,
Linux Kernel Network Developers
In-Reply-To: <CALx6S34ZVK1uHu_dYyh2w=k+8gP-H-nSPw-OgsMk9D2f4uE7jA@mail.gmail.com>
On 1 November 2016 at 11:51, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Nov 1, 2016 at 11:20 AM, Thomas Graf <tgraf@suug.ch> wrote:
>> The headers cannot be extended or reduced so the offsets always remain
>> correct. What can happen is that the header contains invalid data.
>>
> If we can't add/remove headers then doesn't that really limit the
> utility of these patches? My assumption was that BPF+LWT is needed to
> allow users to define and implement their own encapsulations, EH
> insertion, packet modification, etc.
I thought you were specifically referring to lwtunnel_output(). You
can extend headers at lwtunnel_xmit() but you can't for
lwtunnel_output() and lwtunnel_input(). The xmit hook is in
ip_finish_output2() where it is fine to do so. We have existing code
doing this there.
>> You keep saying this "middle in the stack" but the point is exactly
>> the same as a raw socket with IPPROTO_RAW and hdrincl, see
>> rawv6_sendmsg() and rawv6_send_hdrincl(). An IPv6 raw socket can feed
>> arbitrary garbage into dst_output(). IPv4 does some minimal sanity
>> checks.
>>
> What I mean is that an admin can create a BPF program that run on any
> user packets (for instance default route could be set). This would be
> in the path of TCP, UDP, and other protocols tightly integrated with
> the stack. Packets being routed may be encapsulated, VLAN, have
> checksum offload, GORed set etc. They also might be looped back in
> which case the settings in skbuff become receive parameters.
You can do the same with act_pedit or cls_bpf at dev_queue_xmit()
before or after you go into the encapsulation device. This is a tool
for root, if you install a drop all ssh rule then you won't be able to
log into the box anymore. If you modify the packet and render it
invalid, the packet will be dropped.
If you attach a VLAN header while VLAN offload is already set up, then
the hardware will add another VLAN header, this is what I would
expect. I truly hope that we don't have code which crashes if we
dev_queue_xmit() garbage into any device. That would be horrible.
Do you have a specific example that could be a problem?
^ permalink raw reply
* Re: [PATCH 2/2] net: qcom/emac: enable flow control if requested
From: Timur Tabi @ 2016-11-01 19:05 UTC (permalink / raw)
To: David Miller, Florian Fainelli, netdev
In-Reply-To: <1478025055-13167-2-git-send-email-timur@codeaurora.org>
On 11/01/2016 01:30 PM, Timur Tabi wrote:
> + if (!phydev->pause && phydev->asym_pause)
> + mac |= TXFC;
I think this might be wrong, and nb8800_pause_config() has it right.
This should be:
if (!phydev->pause != !phydev->asym_pause)
which is a logical-xor.
Do you concur?
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH] [RFC] net: phy: phy drivers should not set SUPPORTED_Pause or SUPPORTED_Asym_Pause
From: Florian Fainelli @ 2016-11-01 19:03 UTC (permalink / raw)
To: Timur Tabi, David Miller, netdev
In-Reply-To: <5818E2B0.80902@codeaurora.org>
On 11/01/2016 11:45 AM, Timur Tabi wrote:
> Florian Fainelli wrote:
>> So in premise, this is good, and is exactly what I have in mind for the
>> series that I am cooking, but if we apply this alone, without a change
>> in drivers/net/phy/phy.c which adds SUPPORTED_Pause |
>> SUPPORTED_AsymPause to phydev->features, we are basically breaking the
>> Ethernet MAC drivers that don't explicitly override phydev->features and
>> yet rely on that to get flow control to work.
>
> That's what I figured, but I wasn't sure how to handle that. However,
> isn't the ability to pass pause frames a feature that some PHYs do not
> have? That is, does every PHY support bits 10 and 11 in register 4?
>
The standard (IEEE Std 802.3-2008) describes this as the
Auto-negotiation advertisement register and these bits are part of the
Technology ability field register. These two bits A5 (PAUSE operation
for full duplex links) and A6 (Asymetric PAUSE operation for full duplex
links) have no cabling requirement, and earlier the paragraph mentions
this is to be resolved by the "management" entity, so I don't think the
PHY has any business in that other than reflecting what the MAC is
capable of doing towards the link partner, but I could be reading the
specification incorrectly.
^ permalink raw reply
* Re: new kmemleak reports (was: Re: [PATCH 0/5] genetlink improvements)
From: Jakub Kicinski @ 2016-11-01 18:56 UTC (permalink / raw)
To: Cong Wang
Cc: Johannes Berg, Dmitry Torokhov, Maciej Żenczykowski,
Linux Kernel Network Developers
In-Reply-To: <CAM_iQpVeB+2M1MPxjRx++E=q4mDuo7XQqfQn3-160PqG8bNLdQ@mail.gmail.com>
On Tue, 1 Nov 2016 11:32:52 -0700, Cong Wang wrote:
> On Tue, Nov 1, 2016 at 10:28 AM, Jakub Kicinski <kubakici@wp.pl> wrote:
> > unreferenced object 0xffff8807389cba28 (size 128):
> > comm "swapper/0", pid 1, jiffies 4294898463 (age 781.332s)
> > hex dump (first 32 bytes):
> > 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > backtrace:
> > [<ffffffff85decad8>] kmemleak_alloc+0x28/0x50
> > [<ffffffff84771246>] __kmalloc+0x206/0x5a0
> > [<ffffffff859e1261>] genl_register_family+0x711/0x11d0
> > [<ffffffff888d9524>] netlbl_mgmt_genl_init+0x10/0x12
> > [<ffffffff888d91e8>] netlbl_netlink_init+0x9/0x26
> > [<ffffffff888d9254>] netlbl_init+0x4f/0x85
> > [<ffffffff840022b7>] do_one_initcall+0xb7/0x2a0
> > [<ffffffff887f9102>] kernel_init_freeable+0x597/0x636
> > [<ffffffff85de7793>] kernel_init+0x13/0x140
> > [<ffffffff85e0246a>] ret_from_fork+0x2a/0x40
>
> Looks like we are missing a kfree(family->attrbuf); on error path,
> but it is not related to Johannes' recent patches.
>
> Could the attached patch help?
>
> Thanks.
Still there:
unreferenced object 0xffff88073fb204e8 (size 64):
comm "swapper/0", pid 1, jiffies 4294898455 (age 88.528s)
hex dump (first 32 bytes):
6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
backtrace:
[<ffffffff93decbf8>] kmemleak_alloc+0x28/0x50
[<ffffffff92771246>] __kmalloc+0x206/0x5a0
[<ffffffff939e1471>] genl_register_family+0x921/0x1270
[<ffffffff968d0ecf>] genl_init+0x11/0x43
[<ffffffff920022b7>] do_one_initcall+0xb7/0x2a0
[<ffffffff967f9102>] kernel_init_freeable+0x597/0x636
[<ffffffff93de78b3>] kernel_init+0x13/0x140
[<ffffffff93e0256a>] ret_from_fork+0x2a/0x40
[<ffffffffffffffff>] 0xffffffffffffffff
etc.
I'm getting this quite reliably on every boot.
^ permalink raw reply
* Re: [PATCH net-next v2 0/5] bpf: BPF for lightweight tunnel encapsulation
From: Thomas Graf @ 2016-11-01 18:51 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: David S. Miller, Alexei Starovoitov, Daniel Borkmann, Tom Herbert,
roopa, netdev
In-Reply-To: <c220b6b2-3f20-09ff-2068-6ecfe0b47b9e@stressinduktion.org>
On 1 November 2016 at 03:54, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> I do fear the complexity and debugability introduced by this patch set
> quite a bit.
What is the complexity concern? This is pretty straight forward. I
agree on debugability. This is being worked on separately as Alexei
mentioned, to address this for all BPF integrations.
> I wonder if architecturally it would be more feasible to add a generic
> (bfp-)hook into into dst_output(_sk) and allow arbitrary metadata to be
> added into the dsts.
>
> BPF could then be able to access parts of the metadata in the attached
> metadata dst entries and performing the matching this way?
If I understand you correctly then a single BPF program would be
loaded which then applies to all dst_output() calls? This has a huge
drawback, instead of multiple small BPF programs which do exactly what
is required per dst, a large BPF program is needed which matches on
metadata. That's way slower and renders one of the biggest advantages
of BPF invalid, the ability to generate a a small program tailored to
a particular use. See Cilium.
> The reason why I would prefer an approach like this: irregardless of the
> routing lookup we would process the skb with bpf or not. This gives a
> single point to debug, where instead in your approach we first must
> figure out the corresponding bpf program and then check for it specifically.
Not sure I see what kind of advantage this actually provides. You can
dump the routes and see which programs get invoked and which section.
If it's based on metadata then you need to know the program logic and
associate it with the metadata in the dst. It actually doesn't get
much easier than to debug one of the samples, they are completely
static once compiled and it's very simple to verify if they do what
they are supposed to do.
If you like the single program approach, feel free to load the same
program for every dst. Perfectly acceptable but I don't see why we
should force everybody to use that model.
^ permalink raw reply
* Re: [PATCH net-next v2 0/5] bpf: BPF for lightweight tunnel encapsulation
From: Tom Herbert @ 2016-11-01 18:51 UTC (permalink / raw)
To: Thomas Graf
Cc: David S. Miller, Alexei Starovoitov, Daniel Borkmann, roopa,
Linux Kernel Network Developers
In-Reply-To: <CACby=pkPQC92t3Y9-p9RRUv1_WL2uZMM6DRJFUc5-KP4B41hAA@mail.gmail.com>
On Tue, Nov 1, 2016 at 11:20 AM, Thomas Graf <tgraf@suug.ch> wrote:
> On 1 November 2016 at 09:17, Tom Herbert <tom@herbertland.com> wrote:
>> On Mon, Oct 31, 2016 at 5:37 PM, Thomas Graf <tgraf@suug.ch> wrote:
>>> {Open question:
>>> Tom brought up the question on whether it is safe to modify the packet
>>> in artbirary ways before dst_output(). This is the equivalent to a raw
>>> socket injecting illegal headers. This v2 currently assumes that
>>> dst_output() is ready to accept invalid header values. This needs to be
>>> verified and if not the case, then raw sockets or dst_output() handlers
>>> must be fixed as well. Another option is to mark lwtunnel_output() as
>>> read-only for now.}
>>>
>> The question might not be so much about illegal headers but whether
>> fields in the skbuff related to the packet contents are kept correct.
>> We have protocol, header offsets, offsets for inner protocols also,
>> encapsulation settings, checksum status, checksum offset, checksum
>
> The headers cannot be extended or reduced so the offsets always remain
> correct. What can happen is that the header contains invalid data.
>
If we can't add/remove headers then doesn't that really limit the
utility of these patches? My assumption was that BPF+LWT is needed to
allow users to define and implement their own encapsulations, EH
insertion, packet modification, etc.
>> complete value, vlan information. Any or all of which I believe could
>> be turned into being incorrect if we allow the packet to be
>> arbitrarily modified by BPF. This problem is different than raw
>> sockets because LWT operates in the middle of the stack, the skbuff
>> has already been set up which such things.
>
> You keep saying this "middle in the stack" but the point is exactly
> the same as a raw socket with IPPROTO_RAW and hdrincl, see
> rawv6_sendmsg() and rawv6_send_hdrincl(). An IPv6 raw socket can feed
> arbitrary garbage into dst_output(). IPv4 does some minimal sanity
> checks.
>
What I mean is that an admin can create a BPF program that run on any
user packets (for instance default route could be set). This would be
in the path of TCP, UDP, and other protocols tightly integrated with
the stack. Packets being routed may be encapsulated, VLAN, have
checksum offload, GORed set etc. They also might be looped back in
which case the settings in skbuff become receive parameters.
> If this is a concern I'm fine with making the dst_output path read-only for now.
>
The might be good. The ramifications around allowing an open ended
method for users to modify L3/L2 packets needs more consideration.
>>> This series implements BPF program invocation from dst entries via the
>>> lightweight tunnels infrastructure. The BPF program can be attached to
>>> lwtunnel_input(), lwtunnel_output() or lwtunnel_xmit() and sees an L3
>>> skb as context. input is read-only, output can write, xmit can write,
>>> push headers, and redirect.
>>>
>>> Motiviation for this work:
>>> - Restricting outgoing routes beyond what the route tuple supports
>>> - Per route accounting byond realms
>>> - Fast attachment of L2 headers where header does not require resolving
>>> L2 addresses
>>> - ILA like uses cases where L3 addresses are resolved and then routed
>>> in an async manner
>>> - Fast encapsulation + redirect. For now limited to use cases where not
>>> setting inner and outer offset/protocol is OK.
>>>
>> Is checksum offload supported? By default, at least for Linux, we
>> offload the outer UDP checksum in VXLAN and the other UDP
>> encapsulations for performance.
>
> No. UDP encap is done by setting a tunnel key through a helper and
> letting the encapsulation device handle this. I don't currently see a
> point in replicating all of that logic.
^ permalink raw reply
* Re: Why do we need tasklet in IFB?
From: Michael Ma @ 2016-11-01 18:49 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: David Miller, Stephen Hemminger, Linux Kernel Network Developers
In-Reply-To: <dc98e4d7-b076-66ca-a22f-5199b6d13ea9@mojatatu.com>
2016-11-01 4:38 GMT-07:00 Jamal Hadi Salim <jhs@mojatatu.com>:
> On 16-10-31 02:10 PM, David Miller wrote:
>>
>> From: Michael Ma <make0818@gmail.com>
>> Date: Mon, 31 Oct 2016 11:02:28 -0700
>>
>>> 2016-10-28 14:52 GMT-07:00 Michael Ma <make0818@gmail.com>:
>>>>
>>>> 2016-10-28 14:48 GMT-07:00 Stephen Hemminger
>>>> <stephen@networkplumber.org>:
>>>>>
>>>>> On Fri, 28 Oct 2016 14:45:07 -0700
>>>>> Michael Ma <make0818@gmail.com> wrote:
>>>>>
>>>>>> 2016-10-28 14:38 GMT-07:00 Stephen Hemminger
>>>>>> <stephen@networkplumber.org>:
>>>>>>>
>>>>>>> On Fri, 28 Oct 2016 14:36:27 -0700
>>>>>>> Michael Ma <make0818@gmail.com> wrote:
>>>>>>>
>>>>>>>> Hi -
>>>>>>>>
>>>>>>>> Currently IFB uses tasklet to process tx/rx on the interface that
>>>>>>>> forwarded the packet to IFB. My understanding on why we're doing
>>>>>>>> this
>>>>>>>> is that since dev_queue_xmit() can be invoked in interrupt, we want
>>>>>>>> to
>>>>>>>> defer the processing of original tx/rx in case ifb_xmit() is called
>>>>>>>> from interrupt.
>>>>>>>
>>>>>>>
>>>>>>> dev_queue_xmit is only called from interrupt if doing netconsole.
>>>>>>
>>>>>>
>>>
>>> In fact this doesn't seem to explain since if the original path is tx
>>> and the context is interrupt, IFB will call dev_queue_xmit as well so
>>> the context can be interrupt in that case.
>>>
>>> Then tasklet is still unnecessary.
>>
>>
>> Perhaps it's necessary to deal with potential recursion, that's my only
>> guess at this point.
>>
>> Jamal, do you remember what went through your mind back in 2005? :-)
>>
>
> Certainly recursions (which were a huge issue in the original
> implementations) as well as the need to have qdisc which shape traffic
> (which implies they will sit on the queue for some period of time and
> where accuracy of timing is important; therefore low latency of
> scheduling was needed).
>
Thanks for the explanation - I can get the point of avoiding
recursion. However latency wise if we don't have queue/tasklet in IFB
and rely on the upstream/downstream part (either qdisc or tx/rx) to do
the queueing, latency wouldn't be a problem anyway, or did I miss
anything here?
> I didnt follow the context of "tasklets are unnecessary":
> Are tasklets bad or is the OP interested in replacing them with
> something s/he thinks is better? IIRC, at some point the idea was to
> infact use a softirq but it became obvious it was overkill.
I was thinking of directly forwarding the packet in the thread context
of IFB since it's really an intermediate forwarding component and
additional queueing/tasklet doesn't seem to be necessary (let's put
the consideration of too deep call stack/recursion problem aside).
The problem that I have found is that queue selection on NIC will
actually be inherited in IFB and that affects how packets are handled
in tasklet since the tasklet is associated with each queue. We don't
really have a strict one to one mapping of cpu core to queue in NIC,
whereas in IFB case the queue is strictly mapped to tasklet/cpu core
which can cause undesirable contention.
The question here was raised mostly because I wanted to adopt the
thread context of packet in IFB.
>
> cheers,
> jamal
^ permalink raw reply
* Re: [PATCH v3 2/6] net: phy: broadcom: Add BCM54810 PHY entry
From: Andrew Lunn @ 2016-11-01 18:46 UTC (permalink / raw)
To: Jon Mason
Cc: David Miller, Rob Herring, Mark Rutland, Florian Fainelli,
devicetree, netdev, linux-kernel, bcm-kernel-feedback-list, rafal,
linux-arm-kernel
In-Reply-To: <1478022694-25308-3-git-send-email-jon.mason@broadcom.com>
Hi Jon
> @@ -56,6 +57,8 @@
> #define PHY_BRCM_EXT_IBND_TX_ENABLE 0x00002000
> #define PHY_BRCM_CLEAR_RGMII_MODE 0x00004000
> #define PHY_BRCM_DIS_TXCRXC_NOENRGY 0x00008000
> +#define PHY_BRCM_EXP_LANE_SWAP 0x00010000
> +
You define this, but don't use it...
Andrew
^ permalink raw reply
* Re: [PATCH] [RFC] net: phy: phy drivers should not set SUPPORTED_Pause or SUPPORTED_Asym_Pause
From: Timur Tabi @ 2016-11-01 18:45 UTC (permalink / raw)
To: Florian Fainelli, David Miller, netdev
In-Reply-To: <aa0224f1-814a-d281-ad26-6c8086e40658@gmail.com>
Florian Fainelli wrote:
> So in premise, this is good, and is exactly what I have in mind for the
> series that I am cooking, but if we apply this alone, without a change
> in drivers/net/phy/phy.c which adds SUPPORTED_Pause |
> SUPPORTED_AsymPause to phydev->features, we are basically breaking the
> Ethernet MAC drivers that don't explicitly override phydev->features and
> yet rely on that to get flow control to work.
That's what I figured, but I wasn't sure how to handle that. However,
isn't the ability to pass pause frames a feature that some PHYs do not
have? That is, does every PHY support bits 10 and 11 in register 4?
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: ixgbe 4.4.6 detected crash
From: Alexander Duyck @ 2016-11-01 18:43 UTC (permalink / raw)
To: Matias Vara; +Cc: e1000-devel@lists.sourceforge.net, Netdev, David Miller
In-Reply-To: <CA+dKekAEc1YcXhDMYrRNHEJPO-sadjB4u0-8+vt51iOabx-C4g@mail.gmail.com>
On Tue, Nov 1, 2016 at 8:16 AM, Matias Vara <matiasevara@gmail.com> wrote:
> Hi everyone, I am facing an issue with the ixgbe 10G Network card version
> 4.4.6. I am in XenServer which is based on CentOS and the issue only appers
> on FCOE machines. I am getting the following message during crash:
>
> [2016-10-28 09:43:58 UTC] [ 68.789174] kernel BUG at
> /obj/RPM_BUILD_DIRECTORY/driver-intel-ixgbe-4.4.6/src/ixgbe_fcoe.c:440!
>
> Someone knows what is happening?To track this bug, I opened the ticket
> #543. Please go to the ticket for more details.
>
> Thanks in advance, Matias.
I just replied to your bug but I will reply here as well.
The 4.4 kernel has a bug that is fixed by the following commit:
https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=224516b3a798
I would recommend back-porting it into your kernel and we probably
need to have this queued up for linux-stable as well.
>From what I can tell I overlooked adding the following which should
have probably been included in the original patch.
Fixes: a6e544b0a88b ("flow_dissector: Jump to exit code in __skb_flow_dissect")
Thanks.
- Alex
------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: ixgbe 4.4.6 detected crash
From: Alexander Duyck @ 2016-11-01 18:40 UTC (permalink / raw)
To: Matias Vara; +Cc: e1000-devel@lists.sourceforge.net, Netdev, David Miller
In-Reply-To: <CA+dKekAEc1YcXhDMYrRNHEJPO-sadjB4u0-8+vt51iOabx-C4g@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1140 bytes --]
On Tue, Nov 1, 2016 at 8:16 AM, Matias Vara <matiasevara@gmail.com> wrote:
> Hi everyone, I am facing an issue with the ixgbe 10G Network card version
> 4.4.6. I am in XenServer which is based on CentOS and the issue only appers
> on FCOE machines. I am getting the following message during crash:
>
> [2016-10-28 09:43:58 UTC] [ 68.789174] kernel BUG at
> /obj/RPM_BUILD_DIRECTORY/driver-intel-ixgbe-4.4.6/src/ixgbe_fcoe.c:440!
>
> Someone knows what is happening?To track this bug, I opened the ticket
> #543. Please go to the ticket for more details.
>
> Thanks in advance, Matias.
I just replied to your bug but I will reply here as well.
The 4.4 kernel has a bug that is fixed by the following commit:
https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.
git/commit/?id=224516b3a798
I would recommend back-porting it into your kernel and we probably need to
have this queued up for linux-stable as well.
>From what I can tell I overlooked adding the following which should have
probably been included in the original patch.
Fixes: a6e544b0a88b ("flow_dissector: Jump to exit code in
__skb_flow_dissect")
Thanks.
- Alex
[-- Attachment #2: Type: text/plain, Size: 323 bytes --]
------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
[-- Attachment #3: Type: text/plain, Size: 257 bytes --]
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* [PATCH 0/2] net: qcom/emac: the emac driver requires pause frames
From: Timur Tabi @ 2016-11-01 18:40 UTC (permalink / raw)
To: David Miller, Florian Fainelli, netdev
In-Reply-To: <1478025055-13167-1-git-send-email-timur@codeaurora.org>
The qcom emac driver experiences significant packet loss (through frame
check sequence errors) if flow control is not enabled and the phy is not
configured to allow pause frames to pass through it. Therefore, we need
to force enable flow control and force the phy to pass pause frames.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH] [RFC] net: phy: phy drivers should not set SUPPORTED_Pause or SUPPORTED_Asym_Pause
From: Florian Fainelli @ 2016-11-01 18:35 UTC (permalink / raw)
To: Timur Tabi, David Miller, netdev
In-Reply-To: <1478020715-25578-1-git-send-email-timur@codeaurora.org>
On 11/01/2016 10:18 AM, Timur Tabi wrote:
> Pause frames are a feature that is supported by the MAC. It is the MAC
> that generates the frames and that processes them. Thy PHY can only be
> configured to allow them to pass. Therefore, pause frame support in the
> PHY should only be enabled if the MAC wants it enabled. It should not be
> forced on all the time.
>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
So in premise, this is good, and is exactly what I have in mind for the
series that I am cooking, but if we apply this alone, without a change
in drivers/net/phy/phy.c which adds SUPPORTED_Pause |
SUPPORTED_AsymPause to phydev->features, we are basically breaking the
Ethernet MAC drivers that don't explicitly override phydev->features and
yet rely on that to get flow control to work.
--
Florian
^ permalink raw reply
* Re: [PATCH 2/2] net: qcom/emac: enable flow control if requested
From: Florian Fainelli @ 2016-11-01 18:34 UTC (permalink / raw)
To: Timur Tabi, David Miller, netdev
In-Reply-To: <1478025055-13167-2-git-send-email-timur@codeaurora.org>
On 11/01/2016 11:30 AM, Timur Tabi wrote:
> If the PHY has been configured to allow pause frames, then the MAC
> should be configured to generate and/or accept those frames.
>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH 1/2] net: qcom/emac: configure the external phy to allow pause frames
From: Timur Tabi @ 2016-11-01 18:34 UTC (permalink / raw)
To: David Miller, Florian Fainelli, netdev
In-Reply-To: <1478025055-13167-1-git-send-email-timur@codeaurora.org>
Timur Tabi wrote:
> Pause frames are used to enable flow control. A MAC can send and
> receive pause frames in order to throttle traffic. However, the PHY
> must be configured to allow those frames to pass through.
>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
Ugh, I forgot to include an introductory post. Coming right up.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ 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