* [BUG] micrel phy suspend/resume
From: Russell King - ARM Linux @ 2017-12-10 16:47 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli; +Cc: netdev
Guys,
I've just tripped over a bug with the Micrel PHY driver, but it
really isn't specific to the Micrel PHY driver.
When we suspend, we suspend the PHY and then the MAC driver (eg,
on the ZII board):
[ 198.822751] 400d0000.ethernet-1:00: bus : mdio_bus_suspend+0x0/0x34
[ 198.822859] __mdiobus_read: 117 400d0000.ethernet-1 00 00 => 3100
[ 198.822878] __mdiobus_write: 117 400d0000.ethernet-1 00 00 <= 3900
...
[ 198.826235] 400d0000.ethernet: bus : platform_pm_freeze+0x0/0x5c
[ 198.826354] __mdiobus_read: 117 400d0000.ethernet-1 00 1f => 9198
[ 198.826374] __mdiobus_write: 117 400d0000.ethernet-1 00 1f <= 9198
[ 198.826503] __mdiobus_write: 117 400d0000.ethernet-1 00 1b <= 0000
[ 198.826699] __mdiobus_read: 117 400d0000.ethernet-1 00 1b => 0000
When we resume, the order is reversed:
[ 198.848300] 400d0000.ethernet: bus : platform_pm_thaw+0x0/0x54
[ 198.849024] __mdiobus_read: 117 400d0000.ethernet-1 00 1b => 0000
[ 198.849120] __mdiobus_read: 117 400d0000.ethernet-1 00 1f => 9198
[ 198.849141] __mdiobus_write: 117 400d0000.ethernet-1 00 1f <= 9198
[ 198.849243] __mdiobus_write: 117 400d0000.ethernet-1 00 1b <= 0500
[ 198.849401] __mdiobus_read: 117 400d0000.ethernet-1 00 00 => 3900
[ 198.849419] __mdiobus_write: 117 400d0000.ethernet-1 00 00 <= 3100
[ 198.849637] __mdiobus_read: 61 400d0000.ethernet-1 00 01 => 7849
...
[ 198.852677] 400d0000.ethernet-1:00: bus : mdio_bus_resume+0x0/0x34
[ 198.852778] __mdiobus_read: 117 400d0000.ethernet-1 00 00 => 3100
[ 198.852797] __mdiobus_write: 117 400d0000.ethernet-1 00 00 <= 3100
Now, the MAC driver calls phy_stop() and phy_start() from within its
own suspend/resume methods, and at this is done while the PHY is,
as far as the kernel PM code is concerned, suspended.
However, phylib works around that by resuming the PHY itself when
phy_start() is called in this situation. That looks good, but it
really isn't in this case. Given the above sequence, we will be in
PHY_HALTED state.
So, when phy_start() is called, the first thing it does is check the
state, and finds PHY_HALTED. It then tries to enable the PHY
interrupts. This cause the Micrel driver to write to the PHY to
enable interrupts - it reads 0x1b to ack any pre-existing interrupt,
modifies 0x1f to set the interrupt pin level, and then supposedly
writes 0x1b to enable interrupts.
However, at this point, the PHY is still powered down, as we can see
in the following read of the BMCR - containing 0x3900. The write to
enable interrupts is ignored, and so interrupts remain disabled.
The resume process continues, and the system resumes, but interrupts
on the PHY remain disabled, and the phylib state machine never
advances. You can do anything you like with cables etc, as far as
phylib is concerned, the link steadfastly remains "down".
That's a bit of a problem if your platform is running root-NFS through
that network interface.
It looks like some variants of the Micrel phy code work around this by
including in their resume path code to enable PHY interrupts
independently of phylib.
phy_resume() can't be called inside the locked region in phy_start(),
where we enable interrupts, because genphy_resume() also wants to take
phydev->lock - and it wants to do that to safely read-modify-write the
BMCR register. This, I feel, comes back to an abuse of the phy state
machine lock to also protect atomic bus operations.
If we move over to using the bus lock to protect bus atomic operations
(as I believe we should) then phy_resume() can be called while holding
phydev->lock from within bits of the code that are protecting themselves
from concurrency with the phylib state machine. It also means that we
can get rid of some of these boolean variables, and most importantly
in this particular case, call phy_resume() before we enable interrupts.
With that arrangement, things look a lot better:
[ 74.545584] 400d0000.ethernet: bus : platform_pm_thaw+0x0/0x54
[ 74.546010] __mdiobus_read: 117 400d0000.ethernet-1 00 00 => 3900
[ 74.546029] __mdiobus_write: 117 400d0000.ethernet-1 00 00 <= 3100
[ 74.546264] __mdiobus_read: 117 400d0000.ethernet-1 00 1b => 0000
[ 74.546354] __mdiobus_read: 117 400d0000.ethernet-1 00 1f => 8100
[ 74.546373] __mdiobus_write: 117 400d0000.ethernet-1 00 1f <= 8100
[ 74.546651] __mdiobus_write: 117 400d0000.ethernet-1 00 1b <= 0500
The patch for this is slightly larger than it needs to be because I've
converted more places that do read-modify-write to the new xxx_modify()
accessor, and it also ensures that phy_resume() is consistently called
with the phy state machine lock held. It also means we can get rid of
the interrupt enable hack for some micrel PHYs, which I suspect comes
from this same root cause.
Something similar ought to be done for phy_suspend() as well, but that's
a little more complex because it also calls get_wol() functions.
More places could probably be converted to phy_modify() too.
drivers/net/phy/at803x.c | 24 +++--------------
drivers/net/phy/mdio_bus.c | 32 ++++++++++++++++++++++
drivers/net/phy/micrel.c | 15 +----------
drivers/net/phy/phy.c | 9 +++----
drivers/net/phy/phy_device.c | 63 +++++++++++++-------------------------------
include/linux/mdio.h | 1 +
include/linux/phy.h | 18 +++++++++++++
7 files changed, 77 insertions(+), 85 deletions(-)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 4c75cfcdeaec..a20d1f5e4dbf 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -258,38 +258,22 @@ static int at803x_suspend(struct phy_device *phydev)
int value;
int wol_enabled;
- mutex_lock(&phydev->lock);
-
value = phy_read(phydev, AT803X_INTR_ENABLE);
wol_enabled = value & AT803X_INTR_ENABLE_WOL;
- value = phy_read(phydev, MII_BMCR);
-
if (wol_enabled)
- value |= BMCR_ISOLATE;
+ value = BMCR_ISOLATE;
else
- value |= BMCR_PDOWN;
-
- phy_write(phydev, MII_BMCR, value);
+ value = BMCR_PDOWN;
- mutex_unlock(&phydev->lock);
+ phy_modify(phydev, MII_BMCR, value, value);
return 0;
}
static int at803x_resume(struct phy_device *phydev)
{
- int value;
-
- mutex_lock(&phydev->lock);
-
- value = phy_read(phydev, MII_BMCR);
- value &= ~(BMCR_PDOWN | BMCR_ISOLATE);
- phy_write(phydev, MII_BMCR, value);
-
- mutex_unlock(&phydev->lock);
-
- return 0;
+ return phy_modify(phydev, MII_BMCR, BMCR_PDOWN | BMCR_ISOLATE, 0);
}
static int at803x_probe(struct phy_device *phydev)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 1bf3adcdcbac..d5adb00eafdf 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -651,6 +651,38 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
EXPORT_SYMBOL(mdiobus_write);
/**
+ * mdiobus_modify - Convenience function for writing a given MII mgmt register
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to write
+ * @mask: bits to mask off from the register @regnum
+ * @val: new value of bits set in mask to write to @regnum
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask, u16 val)
+{
+ int retval;
+ int err;
+
+ BUG_ON(in_interrupt());
+
+ mutex_lock(&bus->mdio_lock);
+ err = retval = __mdiobus_read(bus, addr, regnum);
+ if (err >= 0) {
+ retval &= ~mask;
+ retval |= val & mask;
+ err = __mdiobus_write(bus, addr, regnum, retval);
+ }
+ mutex_unlock(&bus->mdio_lock);
+
+ return err;
+}
+EXPORT_SYMBOL(mdiobus_modify);
+
+/**
* mdio_bus_match - determine if given MDIO driver supports the given
* MDIO device
* @dev: target MDIO device
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index fdb43dd9b5cd..9e9438caa2b7 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -711,22 +711,9 @@ static int kszphy_suspend(struct phy_device *phydev)
static int kszphy_resume(struct phy_device *phydev)
{
- int ret;
-
genphy_resume(phydev);
- ret = kszphy_config_reset(phydev);
- if (ret)
- return ret;
-
- /* Enable PHY Interrupts */
- if (phy_interrupt_is_valid(phydev)) {
- phydev->interrupts = PHY_INTERRUPT_ENABLED;
- if (phydev->drv->config_intr)
- phydev->drv->config_intr(phydev);
- }
-
- return 0;
+ return kszphy_config_reset(phydev);
}
static int kszphy_probe(struct phy_device *phydev)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f2a83ab00e71..874740957606 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -844,7 +844,6 @@ EXPORT_SYMBOL(phy_stop);
*/
void phy_start(struct phy_device *phydev)
{
- bool do_resume = false;
int err = 0;
mutex_lock(&phydev->lock);
@@ -857,6 +856,9 @@ void phy_start(struct phy_device *phydev)
phydev->state = PHY_UP;
break;
case PHY_HALTED:
+ /* if phy was suspended, bring the physical link up again */
+ phy_resume(phydev);
+
/* make sure interrupts are re-enabled for the PHY */
if (phydev->irq != PHY_POLL) {
err = phy_enable_interrupts(phydev);
@@ -865,17 +867,12 @@ void phy_start(struct phy_device *phydev)
}
phydev->state = PHY_RESUMING;
- do_resume = true;
break;
default:
break;
}
mutex_unlock(&phydev->lock);
- /* if phy was suspended, bring the physical link up again */
- if (do_resume)
- phy_resume(phydev);
-
phy_trigger_machine(phydev, true);
}
EXPORT_SYMBOL(phy_start);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 67f25ac29025..d0d98e6f6406 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -135,7 +135,9 @@ static int mdio_bus_phy_resume(struct device *dev)
if (!mdio_bus_phy_may_suspend(phydev))
goto no_resume;
+ mutex_lock(&phydev->lock);
ret = phy_resume(phydev);
+ mutex_unlock(&phydev->lock);
if (ret < 0)
return ret;
@@ -1026,7 +1028,9 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
if (err)
goto error;
+ mutex_lock(&phydev->lock);
phy_resume(phydev);
+ mutex_unlock(&phydev->lock);
phy_led_triggers_register(phydev);
return err;
@@ -1157,6 +1161,8 @@ int phy_resume(struct phy_device *phydev)
struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
int ret = 0;
+ WARN_ON(!mutex_is_locked(&phydev->lock));
+
if (phydev->drv && phydrv->resume)
ret = phydrv->resume(phydev);
@@ -1322,9 +1328,8 @@ static int genphy_config_eee_advert(struct phy_device *phydev)
*/
int genphy_setup_forced(struct phy_device *phydev)
{
- int ctl = phy_read(phydev, MII_BMCR);
+ u16 ctl = 0;
- ctl &= BMCR_LOOPBACK | BMCR_ISOLATE | BMCR_PDOWN;
phydev->pause = 0;
phydev->asym_pause = 0;
@@ -1336,7 +1341,10 @@ int genphy_setup_forced(struct phy_device *phydev)
if (DUPLEX_FULL == phydev->duplex)
ctl |= BMCR_FULLDPLX;
- return phy_write(phydev, MII_BMCR, ctl);
+ return phy_modify(phydev, MII_BMCR,
+ BMCR_LOOPBACK | BMCR_ISOLATE | BMCR_PDOWN |
+ BMCR_SPEED1000 | BMCR_SPEED100 | BMCR_FULLDPLX,
+ ctl);
}
EXPORT_SYMBOL(genphy_setup_forced);
@@ -1346,17 +1354,10 @@ EXPORT_SYMBOL(genphy_setup_forced);
*/
int genphy_restart_aneg(struct phy_device *phydev)
{
- int ctl = phy_read(phydev, MII_BMCR);
-
- if (ctl < 0)
- return ctl;
-
- ctl |= BMCR_ANENABLE | BMCR_ANRESTART;
-
/* Don't isolate the PHY if we're negotiating */
- ctl &= ~BMCR_ISOLATE;
-
- return phy_write(phydev, MII_BMCR, ctl);
+ return phy_modify(phydev, MII_BMCR,
+ BMCR_ANENABLE | BMCR_ANRESTART | BMCR_ISOLATE,
+ BMCR_ANENABLE | BMCR_ANRESTART);
}
EXPORT_SYMBOL(genphy_restart_aneg);
@@ -1622,48 +1623,20 @@ EXPORT_SYMBOL(genphy_config_init);
int genphy_suspend(struct phy_device *phydev)
{
- int value;
-
- mutex_lock(&phydev->lock);
-
- value = phy_read(phydev, MII_BMCR);
- phy_write(phydev, MII_BMCR, value | BMCR_PDOWN);
-
- mutex_unlock(&phydev->lock);
-
- return 0;
+ return phy_modify(phydev, MII_BMCR, BMCR_PDOWN, BMCR_PDOWN);
}
EXPORT_SYMBOL(genphy_suspend);
int genphy_resume(struct phy_device *phydev)
{
- int value;
-
- mutex_lock(&phydev->lock);
-
- value = phy_read(phydev, MII_BMCR);
- phy_write(phydev, MII_BMCR, value & ~BMCR_PDOWN);
-
- mutex_unlock(&phydev->lock);
-
- return 0;
+ return phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
}
EXPORT_SYMBOL(genphy_resume);
int genphy_loopback(struct phy_device *phydev, bool enable)
{
- int value;
-
- value = phy_read(phydev, MII_BMCR);
- if (value < 0)
- return value;
-
- if (enable)
- value |= BMCR_LOOPBACK;
- else
- value &= ~BMCR_LOOPBACK;
-
- return phy_write(phydev, MII_BMCR, value);
+ return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
+ enable ? BMCR_LOOPBACK : 0);
}
EXPORT_SYMBOL(genphy_loopback);
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 4be30adc033b..8e69211685be 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -264,6 +264,7 @@ int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
int mdiobus_read_nested(struct mii_bus *bus, int addr, u32 regnum);
int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
int mdiobus_write_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val);
+int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask, u16 val);
int mdiobus_register_device(struct mdio_device *mdiodev);
int mdiobus_unregister_device(struct mdio_device *mdiodev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 03a259f128f9..afe5d963a98c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -766,6 +766,24 @@ static inline int __phy_write(struct phy_device *phydev, u32 regnum, u16 val)
}
/**
+ * phy_modify - Convenience function for modifying a given PHY register
+ * @phydev: the phy_device struct
+ * @regnum: register number to write
+ * @mask: bits to mask off from the register @regnum
+ * @val: new value of bits set in mask to write to @regnum
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+static inline int phy_modify(struct phy_device *phydev, u32 regnum, u16 mask,
+ u16 val)
+{
+ return mdiobus_modify(phydev->mdio.bus, phydev->mdio.addr, regnum,
+ mask, val);
+}
+
+/**
* phy_interrupt_is_valid - Convenience function for testing a given PHY irq
* @phydev: the phy_device struct
*
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply related
* [benet] possible endianness bug in be_cmd_txq_create()
From: Al Viro @ 2017-12-10 16:41 UTC (permalink / raw)
To: netdev; +Cc: Vasundhara Volam, Sathya Perla
In be_cmd_txq_create() we have
if (req->hdr.version > 0)
req->if_id = cpu_to_le16(adapter->if_handle);
req->num_pages = PAGES_4K_SPANNED(q_mem->va, q_mem->size);
req->ulp_num = BE_ULP1_NUM;
req->type = BE_ETH_TX_RING_TYPE_STANDARD;
req->cq_id = cpu_to_le16(cq->id);
req->queue_size = be_encoded_q_len(txq->len);
be_cmd_page_addrs_prepare(req->pages, ARRAY_SIZE(req->pages), q_mem);
ver = req->hdr.version;
req points to
struct be_cmd_req_eth_tx_create {
struct be_cmd_req_hdr hdr;
u8 num_pages;
u8 ulp_num;
u16 type;
u16 if_id;
u8 queue_size;
u8 rsvd0;
u32 rsvd1;
u16 cq_id;
u16 rsvd2;
u32 rsvd3[13];
struct phys_addr pages[8];
} __packed;
Everything appears to be consistent with little-endian data - direct
assignments to u8 fields, cpu_to_le16 for cq_id and if_id, phys_addr
array is also filled with little-endian data, so's ->hdr (several
lines prior, by be_wrb_cmd_hdr_prepare()).
The only exception is
req->type = BE_ETH_TX_RING_TYPE_STANDARD;
where we set a 16bit field with host-endian constant (2).
benet is playing silly buggers with swap-in-place in some places, but
it's always 32bit values getting swapped, so this can't be happening
here (num_pages, ulp_num and type form a 32bit-aligned word, and
on big-endian cpu_to_le32() done to it would've ended up with num_pages = 2,
ulp_num = 0, type = 256 + PAGES_4K_SPANNED(q_mem->va, q_mem->size), which is
unlikely to do anything good).
So it really smells like this line should've been
req->type = cpu_to_le16(BE_ETH_TX_RING_TYPE_STANDARD);
I don't have the hardware, so the above is completely untested (caught by
sparse when trying to do endianness annotations in drivers/net), but it
does look like it might be worth a look from benet maintainers.
^ permalink raw reply
* [PATCH v4] leds: trigger: Introduce a NETDEV trigger
From: Ben Whitten @ 2017-12-10 16:24 UTC (permalink / raw)
To: rpurdie, pavel, jacek.anaszewski
Cc: linux-leds, linux-kernel, netdev, Ben Whitten
This commit introduces a NETDEV trigger for named device
activity. Available triggers are link, rx, and tx.
Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
Changes in v4:
Adopt SPDX licence header
Changes in v3:
Cancel the software blink prior to a oneshot re-queue
Changes in v2:
Sort includes and redate documentation
Correct licence
Remove macro and replace with generic function using enums
Convert blink logic in stats work to use led_blink_oneshot
Uses configured brightness instead of FULL
---
.../ABI/testing/sysfs-class-led-trigger-netdev | 45 ++
drivers/leds/trigger/Kconfig | 7 +
drivers/leds/trigger/Makefile | 1 +
drivers/leds/trigger/ledtrig-netdev.c | 498 +++++++++++++++++++++
4 files changed, 551 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-netdev
create mode 100644 drivers/leds/trigger/ledtrig-netdev.c
diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
new file mode 100644
index 0000000..451af6d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
@@ -0,0 +1,45 @@
+What: /sys/class/leds/<led>/device_name
+Date: Dec 2017
+KernelVersion: 4.16
+Contact: linux-leds@vger.kernel.org
+Description:
+ Specifies the network device name to monitor.
+
+What: /sys/class/leds/<led>/interval
+Date: Dec 2017
+KernelVersion: 4.16
+Contact: linux-leds@vger.kernel.org
+Description:
+ Specifies the duration of the LED blink in milliseconds.
+ Defaults to 50 ms.
+
+What: /sys/class/leds/<led>/link
+Date: Dec 2017
+KernelVersion: 4.16
+Contact: linux-leds@vger.kernel.org
+Description:
+ Signal the link state of the named network device.
+ If set to 0 (default), the LED's normal state is off.
+ If set to 1, the LED's normal state reflects the link state
+ of the named network device.
+ Setting this value also immediately changes the LED state.
+
+What: /sys/class/leds/<led>/tx
+Date: Dec 2017
+KernelVersion: 4.16
+Contact: linux-leds@vger.kernel.org
+Description:
+ Signal transmission of data on the named network device.
+ If set to 0 (default), the LED will not blink on transmission.
+ If set to 1, the LED will blink for the milliseconds specified
+ in interval to signal transmission.
+
+What: /sys/class/leds/<led>/rx
+Date: Dec 2017
+KernelVersion: 4.16
+Contact: linux-leds@vger.kernel.org
+Description:
+ Signal reception of data on the named network device.
+ If set to 0 (default), the LED will not blink on reception.
+ If set to 1, the LED will blink for the milliseconds specified
+ in interval to signal reception.
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 3f9ddb9..4ec1853 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -126,4 +126,11 @@ config LEDS_TRIGGER_PANIC
a different trigger.
If unsure, say Y.
+config LEDS_TRIGGER_NETDEV
+ tristate "LED Netdev Trigger"
+ depends on NET && LEDS_TRIGGERS
+ help
+ This allows LEDs to be controlled by network device activity.
+ If unsure, say Y.
+
endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 9f2e868..59e163d 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o
obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o
obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o
obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
+obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
new file mode 100644
index 0000000..3d24573
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -0,0 +1,498 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2017 Ben Whitten <ben.whitten@gmail.com>
+// Copyright 2007 Oliver Jowett <oliver@opencloud.com>
+/*
+ * LED Kernel Netdev Trigger
+ *
+ * Toggles the LED to reflect the link and traffic state of a named net device
+ *
+ * Derived from ledtrig-timer.c which is:
+ * Copyright 2005-2006 Openedhand Ltd.
+ * Author: Richard Purdie <rpurdie@openedhand.com>
+ *
+ */
+
+#include <linux/atomic.h>
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
+#include "../leds.h"
+
+/*
+ * Configurable sysfs attributes:
+ *
+ * device_name - network device name to monitor
+ * interval - duration of LED blink, in milliseconds
+ * link - LED's normal state reflects whether the link is up
+ * (has carrier) or not
+ * tx - LED blinks on transmitted data
+ * rx - LED blinks on receive data
+ *
+ */
+
+struct led_netdev_data {
+ spinlock_t lock;
+
+ struct delayed_work work;
+ struct notifier_block notifier;
+
+ struct led_classdev *led_cdev;
+ struct net_device *net_dev;
+
+ char device_name[IFNAMSIZ];
+ atomic_t interval;
+ unsigned int last_activity;
+
+ unsigned long mode;
+#define NETDEV_LED_LINK 0
+#define NETDEV_LED_TX 1
+#define NETDEV_LED_RX 2
+#define NETDEV_LED_MODE_LINKUP 3
+};
+
+enum netdev_led_attr {
+ NETDEV_ATTR_LINK,
+ NETDEV_ATTR_TX,
+ NETDEV_ATTR_RX
+};
+
+static void set_baseline_state(struct led_netdev_data *trigger_data)
+{
+ int current_brightness;
+ struct led_classdev *led_cdev = trigger_data->led_cdev;
+
+ current_brightness = led_cdev->brightness;
+ if (current_brightness)
+ led_cdev->blink_brightness = current_brightness;
+ if (!led_cdev->blink_brightness)
+ led_cdev->blink_brightness = led_cdev->max_brightness;
+
+ if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode))
+ led_set_brightness(led_cdev, LED_OFF);
+ else {
+ if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
+ led_set_brightness(led_cdev,
+ led_cdev->blink_brightness);
+ else
+ led_set_brightness(led_cdev, LED_OFF);
+
+ /* If we are looking for RX/TX start periodically
+ * checking stats
+ */
+ if (test_bit(NETDEV_LED_TX, &trigger_data->mode) ||
+ test_bit(NETDEV_LED_RX, &trigger_data->mode))
+ schedule_delayed_work(&trigger_data->work, 0);
+ }
+}
+
+static ssize_t device_name_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_netdev_data *trigger_data = led_cdev->trigger_data;
+ ssize_t len;
+
+ spin_lock_bh(&trigger_data->lock);
+ len = sprintf(buf, "%s\n", trigger_data->device_name);
+ spin_unlock_bh(&trigger_data->lock);
+
+ return len;
+}
+
+static ssize_t device_name_store(struct device *dev,
+ struct device_attribute *attr, const char *buf,
+ size_t size)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_netdev_data *trigger_data = led_cdev->trigger_data;
+
+ if (size >= IFNAMSIZ)
+ return -EINVAL;
+
+ cancel_delayed_work_sync(&trigger_data->work);
+
+ spin_lock_bh(&trigger_data->lock);
+
+ if (trigger_data->net_dev) {
+ dev_put(trigger_data->net_dev);
+ trigger_data->net_dev = NULL;
+ }
+
+ strncpy(trigger_data->device_name, buf, size);
+ if (size > 0 && trigger_data->device_name[size - 1] == '\n')
+ trigger_data->device_name[size - 1] = 0;
+
+ if (trigger_data->device_name[0] != 0)
+ trigger_data->net_dev =
+ dev_get_by_name(&init_net, trigger_data->device_name);
+
+ clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+ if (trigger_data->net_dev != NULL)
+ if (netif_carrier_ok(trigger_data->net_dev))
+ set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+
+ trigger_data->last_activity = 0;
+
+ set_baseline_state(trigger_data);
+ spin_unlock_bh(&trigger_data->lock);
+
+ return size;
+}
+
+static DEVICE_ATTR_RW(device_name);
+
+static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
+ enum netdev_led_attr attr)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_netdev_data *trigger_data = led_cdev->trigger_data;
+ int bit;
+
+ switch (attr) {
+ case NETDEV_ATTR_LINK:
+ bit = NETDEV_LED_LINK;
+ break;
+ case NETDEV_ATTR_TX:
+ bit = NETDEV_LED_TX;
+ break;
+ case NETDEV_ATTR_RX:
+ bit = NETDEV_LED_RX;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
+}
+
+static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
+ size_t size, enum netdev_led_attr attr)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_netdev_data *trigger_data = led_cdev->trigger_data;
+ unsigned long state;
+ int ret;
+ int bit;
+
+ ret = kstrtoul(buf, 0, &state);
+ if (ret)
+ return ret;
+
+ switch (attr) {
+ case NETDEV_ATTR_LINK:
+ bit = NETDEV_LED_LINK;
+ break;
+ case NETDEV_ATTR_TX:
+ bit = NETDEV_LED_TX;
+ break;
+ case NETDEV_ATTR_RX:
+ bit = NETDEV_LED_RX;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ cancel_delayed_work_sync(&trigger_data->work);
+
+ if (state)
+ set_bit(bit, &trigger_data->mode);
+ else
+ clear_bit(bit, &trigger_data->mode);
+
+ set_baseline_state(trigger_data);
+
+ return size;
+}
+
+static ssize_t link_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return netdev_led_attr_show(dev, buf, NETDEV_ATTR_LINK);
+}
+
+static ssize_t link_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_LINK);
+}
+
+static DEVICE_ATTR_RW(link);
+
+static ssize_t tx_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return netdev_led_attr_show(dev, buf, NETDEV_ATTR_TX);
+}
+
+static ssize_t tx_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_TX);
+}
+
+static DEVICE_ATTR_RW(tx);
+
+static ssize_t rx_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return netdev_led_attr_show(dev, buf, NETDEV_ATTR_RX);
+}
+
+static ssize_t rx_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_RX);
+}
+
+static DEVICE_ATTR_RW(rx);
+
+static ssize_t interval_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_netdev_data *trigger_data = led_cdev->trigger_data;
+
+ return sprintf(buf, "%u\n",
+ jiffies_to_msecs(atomic_read(&trigger_data->interval)));
+}
+
+static ssize_t interval_store(struct device *dev,
+ struct device_attribute *attr, const char *buf,
+ size_t size)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_netdev_data *trigger_data = led_cdev->trigger_data;
+ unsigned long value;
+ int ret;
+
+ ret = kstrtoul(buf, 0, &value);
+ if (ret)
+ return ret;
+
+ /* impose some basic bounds on the timer interval */
+ if (value >= 5 && value <= 10000) {
+ cancel_delayed_work_sync(&trigger_data->work);
+
+ atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
+ set_baseline_state(trigger_data); /* resets timer */
+ }
+
+ return size;
+}
+
+static DEVICE_ATTR_RW(interval);
+
+static int netdev_trig_notify(struct notifier_block *nb,
+ unsigned long evt, void *dv)
+{
+ struct net_device *dev =
+ netdev_notifier_info_to_dev((struct netdev_notifier_info *)dv);
+ struct led_netdev_data *trigger_data = container_of(nb,
+ struct
+ led_netdev_data,
+ notifier);
+
+ if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
+ && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
+ && evt != NETDEV_CHANGENAME)
+ return NOTIFY_DONE;
+
+ if (strcmp(dev->name, trigger_data->device_name))
+ return NOTIFY_DONE;
+
+ cancel_delayed_work_sync(&trigger_data->work);
+
+ spin_lock_bh(&trigger_data->lock);
+
+ clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+ switch (evt) {
+ case NETDEV_REGISTER:
+ if (trigger_data->net_dev)
+ dev_put(trigger_data->net_dev);
+ dev_hold(dev);
+ trigger_data->net_dev = dev;
+ break;
+ case NETDEV_CHANGENAME:
+ case NETDEV_UNREGISTER:
+ if (trigger_data->net_dev) {
+ dev_put(trigger_data->net_dev);
+ trigger_data->net_dev = NULL;
+ }
+ break;
+ case NETDEV_UP:
+ case NETDEV_CHANGE:
+ if (netif_carrier_ok(dev))
+ set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+ break;
+ }
+
+ set_baseline_state(trigger_data);
+
+ spin_unlock_bh(&trigger_data->lock);
+
+ return NOTIFY_DONE;
+}
+
+/* here's the real work! */
+static void netdev_trig_work(struct work_struct *work)
+{
+ struct led_netdev_data *trigger_data = container_of(work,
+ struct
+ led_netdev_data,
+ work.work);
+ struct rtnl_link_stats64 *dev_stats;
+ unsigned int new_activity;
+ struct rtnl_link_stats64 temp;
+ unsigned long interval;
+ int invert;
+
+ /* If we dont have a device, insure we are off */
+ if (!trigger_data->net_dev) {
+ led_set_brightness(trigger_data->led_cdev, LED_OFF);
+ return;
+ }
+
+ /* If we are not looking for RX/TX then return */
+ if (!test_bit(NETDEV_LED_TX, &trigger_data->mode) &&
+ !test_bit(NETDEV_LED_RX, &trigger_data->mode))
+ return;
+
+ dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
+ new_activity =
+ (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
+ dev_stats->tx_packets : 0) +
+ (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
+ dev_stats->rx_packets : 0);
+
+ if (trigger_data->last_activity != new_activity) {
+ led_stop_software_blink(trigger_data->led_cdev);
+
+ invert = test_bit(NETDEV_LED_LINK, &trigger_data->mode);
+ interval = jiffies_to_msecs(
+ atomic_read(&trigger_data->interval));
+ /* base state is ON (link present) */
+ led_blink_set_oneshot(trigger_data->led_cdev,
+ &interval,
+ &interval,
+ invert);
+ trigger_data->last_activity = new_activity;
+ }
+
+ schedule_delayed_work(&trigger_data->work,
+ (atomic_read(&trigger_data->interval)*2));
+}
+
+static void netdev_trig_activate(struct led_classdev *led_cdev)
+{
+ struct led_netdev_data *trigger_data;
+ int rc;
+
+ trigger_data = kzalloc(sizeof(struct led_netdev_data), GFP_KERNEL);
+ if (!trigger_data)
+ return;
+
+ spin_lock_init(&trigger_data->lock);
+
+ trigger_data->notifier.notifier_call = netdev_trig_notify;
+ trigger_data->notifier.priority = 10;
+
+ INIT_DELAYED_WORK(&trigger_data->work, netdev_trig_work);
+
+ trigger_data->led_cdev = led_cdev;
+ trigger_data->net_dev = NULL;
+ trigger_data->device_name[0] = 0;
+
+ trigger_data->mode = 0;
+ atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
+ trigger_data->last_activity = 0;
+
+ led_cdev->trigger_data = trigger_data;
+
+ rc = device_create_file(led_cdev->dev, &dev_attr_device_name);
+ if (rc)
+ goto err_out;
+ rc = device_create_file(led_cdev->dev, &dev_attr_link);
+ if (rc)
+ goto err_out_device_name;
+ rc = device_create_file(led_cdev->dev, &dev_attr_rx);
+ if (rc)
+ goto err_out_link;
+ rc = device_create_file(led_cdev->dev, &dev_attr_tx);
+ if (rc)
+ goto err_out_rx;
+ rc = device_create_file(led_cdev->dev, &dev_attr_interval);
+ if (rc)
+ goto err_out_tx;
+ rc = register_netdevice_notifier(&trigger_data->notifier);
+ if (rc)
+ goto err_out_interval;
+ return;
+
+err_out_interval:
+ device_remove_file(led_cdev->dev, &dev_attr_interval);
+err_out_tx:
+ device_remove_file(led_cdev->dev, &dev_attr_tx);
+err_out_rx:
+ device_remove_file(led_cdev->dev, &dev_attr_rx);
+err_out_link:
+ device_remove_file(led_cdev->dev, &dev_attr_link);
+err_out_device_name:
+ device_remove_file(led_cdev->dev, &dev_attr_device_name);
+err_out:
+ led_cdev->trigger_data = NULL;
+ kfree(trigger_data);
+}
+
+static void netdev_trig_deactivate(struct led_classdev *led_cdev)
+{
+ struct led_netdev_data *trigger_data = led_cdev->trigger_data;
+
+ if (trigger_data) {
+ unregister_netdevice_notifier(&trigger_data->notifier);
+
+ device_remove_file(led_cdev->dev, &dev_attr_device_name);
+ device_remove_file(led_cdev->dev, &dev_attr_link);
+ device_remove_file(led_cdev->dev, &dev_attr_rx);
+ device_remove_file(led_cdev->dev, &dev_attr_tx);
+ device_remove_file(led_cdev->dev, &dev_attr_interval);
+
+ cancel_delayed_work_sync(&trigger_data->work);
+
+ if (trigger_data->net_dev)
+ dev_put(trigger_data->net_dev);
+
+ kfree(trigger_data);
+ }
+}
+
+static struct led_trigger netdev_led_trigger = {
+ .name = "netdev",
+ .activate = netdev_trig_activate,
+ .deactivate = netdev_trig_deactivate,
+};
+
+static int __init netdev_trig_init(void)
+{
+ return led_trigger_register(&netdev_led_trigger);
+}
+
+static void __exit netdev_trig_exit(void)
+{
+ led_trigger_unregister(&netdev_led_trigger);
+}
+
+module_init(netdev_trig_init);
+module_exit(netdev_trig_exit);
+
+MODULE_AUTHOR("Ben Whitten <ben.whitten@gmail.com>");
+MODULE_AUTHOR("Oliver Jowett <oliver@opencloud.com>");
+MODULE_DESCRIPTION("Netdev LED trigger");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 2/3] net:tracepoint: using sock_set_state tracepoint to trace DCCP state transition
From: Yafang Shao @ 2017-12-10 15:31 UTC (permalink / raw)
To: davem, songliubraving, marcelo.leitner
Cc: edumazet, xiyou.wangcong, mingo, kuznet, yoshfuji, rostedt,
bgregg, netdev, linux-kernel, Yafang Shao
In-Reply-To: <1512919904-14166-1-git-send-email-laoar.shao@gmail.com>
With changes in inet_ files, DCCP state transitions are traced with
sock_set_state tracepoint.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
net/dccp/proto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/dccp/proto.c b/net/dccp/proto.c
index b68168f..92b9613 100644
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -110,7 +110,7 @@ void dccp_set_state(struct sock *sk, const int state)
/* Change state AFTER socket is unhashed to avoid closed
* socket sitting in hash tables.
*/
- sk->sk_state = state;
+ sk_set_state(sk, state);
}
EXPORT_SYMBOL_GPL(dccp_set_state);
^ permalink raw reply related
* [PATCH net-next 0/3] replace tcp_set_state tracepoint with sock_set_state tracepoint
From: Yafang Shao @ 2017-12-10 15:31 UTC (permalink / raw)
To: davem, songliubraving, marcelo.leitner
Cc: edumazet, xiyou.wangcong, mingo, kuznet, yoshfuji, rostedt,
bgregg, netdev, linux-kernel, Yafang Shao
Hi,
According to the discussion in the mail thread
https://patchwork.kernel.org/patch/10099243/,
tcp_set_state tracepoint is renamed to sock_set_state tracepoint and is moved
to include/trace/events/sock.h.
Using this new tracepoint to trace TCP/DCCP/SCTP state transition.
Yafang Shao (3):
net:tracepoint: replace tcp_set_state tracepoint with
sock_set_state tracepoint
net:tracepoint: using sock_set_state tracepoint to trace DCCP
state transition
net:tracepoint: using sock_set_state tracepoint to trace SCTP state
transition
include/net/sock.h | 15 ++-----
include/trace/events/sock.h | 95 +++++++++++++++++++++++++++++++++++++++++
include/trace/events/tcp.h | 76 ---------------------------------
net/core/sock.c | 13 ++++++
net/dccp/proto.c | 2 +-
net/ipv4/inet_connection_sock.c | 4 +-
net/ipv4/inet_hashtables.c | 2 +-
net/ipv4/tcp.c | 4 --
net/sctp/endpointola.c | 2 +-
net/sctp/sm_sideeffect.c | 4 +-
net/sctp/socket.c | 14 +++---
11 files changed, 125 insertions(+), 106 deletions(-)
^ permalink raw reply
* [PATCH net-next 3/3] net:tracepoint: using sock_set_state tracepoint to trace SCTP state transition
From: Yafang Shao @ 2017-12-10 15:31 UTC (permalink / raw)
To: davem, songliubraving, marcelo.leitner
Cc: edumazet, xiyou.wangcong, mingo, kuznet, yoshfuji, rostedt,
bgregg, netdev, linux-kernel, Yafang Shao
In-Reply-To: <1512919904-14166-1-git-send-email-laoar.shao@gmail.com>
With changes in inet_ files, SCTP state transitions are traced with
sockt_set_state tracepoint.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
net/sctp/endpointola.c | 2 +-
net/sctp/sm_sideeffect.c | 4 ++--
net/sctp/socket.c | 14 +++++++-------
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index ee1e601..52d2bb3 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -232,7 +232,7 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
{
ep->base.dead = true;
- ep->base.sk->sk_state = SCTP_SS_CLOSED;
+ sk_set_state(ep->base.sk, SCTP_SS_CLOSED);
/* Unlink this endpoint, so we can't find it again! */
sctp_unhash_endpoint(ep);
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index df94d77..dd2d7f8 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -878,12 +878,12 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
* successfully completed a connect() call.
*/
if (sctp_state(asoc, ESTABLISHED) && sctp_sstate(sk, CLOSED))
- sk->sk_state = SCTP_SS_ESTABLISHED;
+ sk_set_state(sk, SCTP_SS_ESTABLISHED);
/* Set the RCV_SHUTDOWN flag when a SHUTDOWN is received. */
if (sctp_state(asoc, SHUTDOWN_RECEIVED) &&
sctp_sstate(sk, ESTABLISHED)) {
- sk->sk_state = SCTP_SS_CLOSING;
+ sk_set_state(sk, SCTP_SS_CLOSING);
sk->sk_shutdown |= RCV_SHUTDOWN;
}
}
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 014847e..51ebb38 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1528,7 +1528,7 @@ static void sctp_close(struct sock *sk, long timeout)
lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
sk->sk_shutdown = SHUTDOWN_MASK;
- sk->sk_state = SCTP_SS_CLOSING;
+ sk_set_state(sk, SCTP_SS_CLOSING);
ep = sctp_sk(sk)->ep;
@@ -4582,7 +4582,7 @@ static void sctp_shutdown(struct sock *sk, int how)
if (how & SEND_SHUTDOWN && !list_empty(&ep->asocs)) {
struct sctp_association *asoc;
- sk->sk_state = SCTP_SS_CLOSING;
+ sk_set_state(sk, SCTP_SS_CLOSING);
asoc = list_entry(ep->asocs.next,
struct sctp_association, asocs);
sctp_primitive_SHUTDOWN(net, asoc, NULL);
@@ -7405,13 +7405,13 @@ static int sctp_listen_start(struct sock *sk, int backlog)
* sockets.
*
*/
- sk->sk_state = SCTP_SS_LISTENING;
+ sk_set_state(sk, SCTP_SS_LISTENING);
if (!ep->base.bind_addr.port) {
if (sctp_autobind(sk))
return -EAGAIN;
} else {
if (sctp_get_port(sk, inet_sk(sk)->inet_num)) {
- sk->sk_state = SCTP_SS_CLOSED;
+ sk_set_state(sk, SCTP_SS_CLOSED);
return -EADDRINUSE;
}
}
@@ -7463,7 +7463,7 @@ int sctp_inet_listen(struct socket *sock, int backlog)
err = 0;
sctp_unhash_endpoint(ep);
- sk->sk_state = SCTP_SS_CLOSED;
+ sk_set_state(sk, SCTP_SS_CLOSED);
if (sk->sk_reuse)
sctp_sk(sk)->bind_hash->fastreuse = 1;
goto out;
@@ -8438,10 +8438,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
* is called, set RCV_SHUTDOWN flag.
*/
if (sctp_state(assoc, CLOSED) && sctp_style(newsk, TCP)) {
- newsk->sk_state = SCTP_SS_CLOSED;
+ sk_set_state(newsk, SCTP_SS_CLOSED);
newsk->sk_shutdown |= RCV_SHUTDOWN;
} else {
- newsk->sk_state = SCTP_SS_ESTABLISHED;
+ sk_set_state(newsk, SCTP_SS_ESTABLISHED);
}
release_sock(newsk);
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 1/3] net:tracepoint: replace tcp_set_state tracepoint with sock_set_state tracepoint
From: Yafang Shao @ 2017-12-10 15:31 UTC (permalink / raw)
To: davem, songliubraving, marcelo.leitner
Cc: edumazet, xiyou.wangcong, mingo, kuznet, yoshfuji, rostedt,
bgregg, netdev, linux-kernel, Yafang Shao
In-Reply-To: <1512919904-14166-1-git-send-email-laoar.shao@gmail.com>
As sk_state is a common field for struct sock, so the state
transition should not be a TCP specific feature.
So I rename tcp_set_state tracepoint to sock_set_state tracepoint with
some minor changes and move it into file trace/events/sock.h.
The minor changes against on the original tcp_set_state tracepoint:
- Protocol name is printed to distinguish which protocol it is belonging
to
- The macros defined in the file are undefed at the end of this file as
they are only used in this file
Two helpers are introduced to trace sk_state transition
- void sk_state_store(struct sock *sk, int state);
- void sk_set_state(struct sock *sk, int state);
As trace header should not be included in other header files,
so they are defined in sock.c.
The protocol such as SCTP maybe compiled as a ko, hence export
sk_set_state().
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
include/net/sock.h | 15 ++-----
include/trace/events/sock.h | 95 +++++++++++++++++++++++++++++++++++++++++
include/trace/events/tcp.h | 76 ---------------------------------
net/core/sock.c | 13 ++++++
net/ipv4/inet_connection_sock.c | 4 +-
net/ipv4/inet_hashtables.c | 2 +-
net/ipv4/tcp.c | 4 --
7 files changed, 114 insertions(+), 95 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 79e1a2c..b307b60 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2348,18 +2348,9 @@ static inline int sk_state_load(const struct sock *sk)
return smp_load_acquire(&sk->sk_state);
}
-/**
- * sk_state_store - update sk->sk_state
- * @sk: socket pointer
- * @newstate: new state
- *
- * Paired with sk_state_load(). Should be used in contexts where
- * state change might impact lockless readers.
- */
-static inline void sk_state_store(struct sock *sk, int newstate)
-{
- smp_store_release(&sk->sk_state, newstate);
-}
+/* For sock_set_state tracepoint */
+void sk_state_store(struct sock *sk, int newstate);
+void sk_set_state(struct sock *sk, int state);
void sock_enable_timestamp(struct sock *sk, int flag);
int sock_get_timestamp(struct sock *, struct timeval __user *);
diff --git a/include/trace/events/sock.h b/include/trace/events/sock.h
index ec4dade..2728892 100644
--- a/include/trace/events/sock.h
+++ b/include/trace/events/sock.h
@@ -6,7 +6,33 @@
#define _TRACE_SOCK_H
#include <net/sock.h>
+#include <net/ipv6.h>
#include <linux/tracepoint.h>
+#include <linux/ipv6.h>
+#include <linux/tcp.h>
+
+#define inet_protocol_name(protocol) {IPPROTO_##protocol, #protocol}
+#define show_inet_protocol_name(val) \
+ __print_symbolic(val, \
+ inet_protocol_name(TCP), \
+ inet_protocol_name(DCCP), \
+ inet_protocol_name(SCTP))
+
+#define tcp_state_name(state) { state, #state }
+#define show_tcp_state_name(val) \
+ __print_symbolic(val, \
+ tcp_state_name(TCP_ESTABLISHED), \
+ tcp_state_name(TCP_SYN_SENT), \
+ tcp_state_name(TCP_SYN_RECV), \
+ tcp_state_name(TCP_FIN_WAIT1), \
+ tcp_state_name(TCP_FIN_WAIT2), \
+ tcp_state_name(TCP_TIME_WAIT), \
+ tcp_state_name(TCP_CLOSE), \
+ tcp_state_name(TCP_CLOSE_WAIT), \
+ tcp_state_name(TCP_LAST_ACK), \
+ tcp_state_name(TCP_LISTEN), \
+ tcp_state_name(TCP_CLOSING), \
+ tcp_state_name(TCP_NEW_SYN_RECV))
TRACE_EVENT(sock_rcvqueue_full,
@@ -63,6 +89,75 @@
__entry->rmem_alloc)
);
+TRACE_EVENT(sock_set_state,
+
+ TP_PROTO(const struct sock *sk, const int oldstate, const int newstate),
+
+ TP_ARGS(sk, oldstate, newstate),
+
+ TP_STRUCT__entry(
+ __field(const void *, skaddr)
+ __field(int, oldstate)
+ __field(int, newstate)
+ __field(__u16, sport)
+ __field(__u16, dport)
+ __field(__u8, protocol);
+ __array(__u8, saddr, 4)
+ __array(__u8, daddr, 4)
+ __array(__u8, saddr_v6, 16)
+ __array(__u8, daddr_v6, 16)
+ ),
+
+ TP_fast_assign(
+ struct inet_sock *inet = inet_sk(sk);
+ struct in6_addr *pin6;
+ __be32 *p32;
+
+ __entry->skaddr = sk;
+ __entry->oldstate = oldstate;
+ __entry->newstate = newstate;
+
+ __entry->protocol = sk->sk_protocol;
+ __entry->sport = ntohs(inet->inet_sport);
+ __entry->dport = ntohs(inet->inet_dport);
+
+ p32 = (__be32 *) __entry->saddr;
+ *p32 = inet->inet_saddr;
+
+ p32 = (__be32 *) __entry->daddr;
+ *p32 = inet->inet_daddr;
+
+#if IS_ENABLED(CONFIG_IPV6)
+ if (sk->sk_family == AF_INET6) {
+ pin6 = (struct in6_addr *)__entry->saddr_v6;
+ *pin6 = sk->sk_v6_rcv_saddr;
+ pin6 = (struct in6_addr *)__entry->daddr_v6;
+ *pin6 = sk->sk_v6_daddr;
+ } else
+#endif
+ {
+ pin6 = (struct in6_addr *)__entry->saddr_v6;
+ ipv6_addr_set_v4mapped(inet->inet_saddr, pin6);
+ pin6 = (struct in6_addr *)__entry->daddr_v6;
+ ipv6_addr_set_v4mapped(inet->inet_daddr, pin6);
+ }
+ ),
+
+ TP_printk("protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4"
+ "saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
+ show_inet_protocol_name(__entry->protocol),
+ __entry->sport, __entry->dport,
+ __entry->saddr, __entry->daddr,
+ __entry->saddr_v6, __entry->daddr_v6,
+ show_tcp_state_name(__entry->oldstate),
+ show_tcp_state_name(__entry->newstate))
+);
+
+#undef show_tcp_state_name
+#undef tcp_state_name
+#undef show_inet_protocol_name
+#undef inet_protocol_name
+
#endif /* _TRACE_SOCK_H */
/* This part must be outside protection */
diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index 07cccca..7399399 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -9,22 +9,6 @@
#include <linux/tracepoint.h>
#include <net/ipv6.h>
-#define tcp_state_name(state) { state, #state }
-#define show_tcp_state_name(val) \
- __print_symbolic(val, \
- tcp_state_name(TCP_ESTABLISHED), \
- tcp_state_name(TCP_SYN_SENT), \
- tcp_state_name(TCP_SYN_RECV), \
- tcp_state_name(TCP_FIN_WAIT1), \
- tcp_state_name(TCP_FIN_WAIT2), \
- tcp_state_name(TCP_TIME_WAIT), \
- tcp_state_name(TCP_CLOSE), \
- tcp_state_name(TCP_CLOSE_WAIT), \
- tcp_state_name(TCP_LAST_ACK), \
- tcp_state_name(TCP_LISTEN), \
- tcp_state_name(TCP_CLOSING), \
- tcp_state_name(TCP_NEW_SYN_RECV))
-
/*
* tcp event with arguments sk and skb
*
@@ -177,66 +161,6 @@
TP_ARGS(sk)
);
-TRACE_EVENT(tcp_set_state,
-
- TP_PROTO(const struct sock *sk, const int oldstate, const int newstate),
-
- TP_ARGS(sk, oldstate, newstate),
-
- TP_STRUCT__entry(
- __field(const void *, skaddr)
- __field(int, oldstate)
- __field(int, newstate)
- __field(__u16, sport)
- __field(__u16, dport)
- __array(__u8, saddr, 4)
- __array(__u8, daddr, 4)
- __array(__u8, saddr_v6, 16)
- __array(__u8, daddr_v6, 16)
- ),
-
- TP_fast_assign(
- struct inet_sock *inet = inet_sk(sk);
- struct in6_addr *pin6;
- __be32 *p32;
-
- __entry->skaddr = sk;
- __entry->oldstate = oldstate;
- __entry->newstate = newstate;
-
- __entry->sport = ntohs(inet->inet_sport);
- __entry->dport = ntohs(inet->inet_dport);
-
- p32 = (__be32 *) __entry->saddr;
- *p32 = inet->inet_saddr;
-
- p32 = (__be32 *) __entry->daddr;
- *p32 = inet->inet_daddr;
-
-#if IS_ENABLED(CONFIG_IPV6)
- if (sk->sk_family == AF_INET6) {
- pin6 = (struct in6_addr *)__entry->saddr_v6;
- *pin6 = sk->sk_v6_rcv_saddr;
- pin6 = (struct in6_addr *)__entry->daddr_v6;
- *pin6 = sk->sk_v6_daddr;
- } else
-#endif
- {
- pin6 = (struct in6_addr *)__entry->saddr_v6;
- ipv6_addr_set_v4mapped(inet->inet_saddr, pin6);
- pin6 = (struct in6_addr *)__entry->daddr_v6;
- ipv6_addr_set_v4mapped(inet->inet_daddr, pin6);
- }
- ),
-
- TP_printk("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
- __entry->sport, __entry->dport,
- __entry->saddr, __entry->daddr,
- __entry->saddr_v6, __entry->daddr_v6,
- show_tcp_state_name(__entry->oldstate),
- show_tcp_state_name(__entry->newstate))
-);
-
TRACE_EVENT(tcp_retransmit_synack,
TP_PROTO(const struct sock *sk, const struct request_sock *req),
diff --git a/net/core/sock.c b/net/core/sock.c
index c0b5b2f..ee0c1bc 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2859,6 +2859,19 @@ int sock_get_timestampns(struct sock *sk, struct timespec __user *userstamp)
}
EXPORT_SYMBOL(sock_get_timestampns);
+void sk_state_store(struct sock *sk, int state)
+{
+ trace_sock_set_state(sk, sk->sk_state, state);
+ smp_store_release(&sk->sk_state, state);
+}
+
+void sk_set_state(struct sock *sk, int state)
+{
+ trace_sock_set_state(sk, sk->sk_state, state);
+ sk->sk_state = state;
+}
+EXPORT_SYMBOL(sk_set_state);
+
void sock_enable_timestamp(struct sock *sk, int flag)
{
if (!sock_flag(sk, flag)) {
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 4ca46dc..001f7b0 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -783,7 +783,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk,
if (newsk) {
struct inet_connection_sock *newicsk = inet_csk(newsk);
- newsk->sk_state = TCP_SYN_RECV;
+ sk_set_state(newsk, TCP_SYN_RECV);
newicsk->icsk_bind_hash = NULL;
inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port;
@@ -888,7 +888,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
return 0;
}
- sk->sk_state = TCP_CLOSE;
+ sk_set_state(sk, TCP_CLOSE);
return err;
}
EXPORT_SYMBOL_GPL(inet_csk_listen_start);
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index f6f5810..5973693 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -544,7 +544,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk)
sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
} else {
percpu_counter_inc(sk->sk_prot->orphan_count);
- sk->sk_state = TCP_CLOSE;
+ sk_set_state(sk, TCP_CLOSE);
sock_set_flag(sk, SOCK_DEAD);
inet_csk_destroy_sock(sk);
}
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1803116..000504f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -283,8 +283,6 @@
#include <asm/ioctls.h>
#include <net/busy_poll.h>
-#include <trace/events/tcp.h>
-
struct percpu_counter tcp_orphan_count;
EXPORT_SYMBOL_GPL(tcp_orphan_count);
@@ -2040,8 +2038,6 @@ void tcp_set_state(struct sock *sk, int state)
{
int oldstate = sk->sk_state;
- trace_tcp_set_state(sk, oldstate, state);
-
switch (state) {
case TCP_ESTABLISHED:
if (oldstate != TCP_ESTABLISHED)
--
1.8.3.1
^ permalink raw reply related
* [PATCH v6 3/3] sock: Hide unused variable when !CONFIG_PROC_FS.
From: Tonghao Zhang @ 2017-12-10 15:12 UTC (permalink / raw)
To: davem, xiyou.wangcong, edumazet, willemb, xemul; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1512918726-2731-1-git-send-email-xiangxia.m.yue@gmail.com>
When CONFIG_PROC_FS is disabled, we will not use the prot_inuse
counter. This adds an #ifdef to hide the variable definition in
that case. This is not a bugfix. But we can save bytes when there
are many network namespace.
Cc: Pavel Emelyanov <xemul@openvz.org>
Signed-off-by: Martin Zhang <zhangjunweimartin@didichuxing.com>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
include/net/netns/core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/netns/core.h b/include/net/netns/core.h
index a5e8a66..36c2d99 100644
--- a/include/net/netns/core.h
+++ b/include/net/netns/core.h
@@ -13,8 +13,8 @@ struct netns_core {
#ifdef CONFIG_PROC_FS
int __percpu *sock_inuse;
-#endif
struct prot_inuse __percpu *prot_inuse;
+#endif
};
#endif
--
1.8.3.1
^ permalink raw reply related
* [PATCH v6 2/3] sock: Move the socket inuse to namespace.
From: Tonghao Zhang @ 2017-12-10 15:12 UTC (permalink / raw)
To: davem, xiyou.wangcong, edumazet, willemb, xemul; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1512918726-2731-1-git-send-email-xiangxia.m.yue@gmail.com>
In some case, we want to know how many sockets are in use in
different _net_ namespaces. It's a key resource metric.
This patch adds a member in struct netns_core. This is a counter
for socket-inuse in the _net_ namespace. The patch will add/sub
counter in the sk_alloc, sk_clone_lock and __sk_free.
The main reasons for doing this are that:
1. When linux calls the 'do_exit' for processes to exit, the functions
'exit_task_namespaces' and 'exit_task_work' will be called sequentially.
'exit_task_namespaces' may have destroyed the _net_ namespace, but
'sock_release' called in 'exit_task_work' may use the _net_ namespace
if we counter the socket-inuse in sock_release.
2. socket and sock are in pair. More important, sock holds the _net_
namespace. We counter the socket-inuse in sock, for avoiding holding
_net_ namespace again in socket. It's a easy way to maintain the code.
3. We alloc the sock_inuse in net_alloc() and free it in net_free()
because we should make sure that the sock_inuse will not be used anymore
after we release it. Notice that some sockets (e.g netlink socket created
in kernel) will be released after all of the network namespace exit methods.
For more details, see the cleanup_net. Then, we should not use the per
network namespace operations to malloc the sock_inuse.
Signed-off-by: Martin Zhang <zhangjunweimartin@didichuxing.com>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
include/net/netns/core.h | 3 +++
include/net/sock.h | 1 +
net/core/net_namespace.c | 10 ++++++++++
net/core/sock.c | 26 ++++++++++++++++++++++++++
net/socket.c | 21 ++-------------------
5 files changed, 42 insertions(+), 19 deletions(-)
diff --git a/include/net/netns/core.h b/include/net/netns/core.h
index 45cfb5d..a5e8a66 100644
--- a/include/net/netns/core.h
+++ b/include/net/netns/core.h
@@ -11,6 +11,9 @@ struct netns_core {
int sysctl_somaxconn;
+#ifdef CONFIG_PROC_FS
+ int __percpu *sock_inuse;
+#endif
struct prot_inuse __percpu *prot_inuse;
};
diff --git a/include/net/sock.h b/include/net/sock.h
index 9155da4..44f4890 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1262,6 +1262,7 @@ static inline void sk_sockets_allocated_inc(struct sock *sk)
/* Called with local bh disabled */
void sock_prot_inuse_add(struct net *net, struct proto *prot, int inc);
int sock_prot_inuse_get(struct net *net, struct proto *proto);
+int sock_inuse_get(struct net *net);
#else
static inline void sock_prot_inuse_add(struct net *net, struct proto *prot,
int inc)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index b797832..6c191fb 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -363,6 +363,13 @@ static struct net *net_alloc(void)
if (!net)
goto out_free;
+#ifdef CONFIG_PROC_FS
+ net->core.sock_inuse = alloc_percpu(int);
+ if (!net->core.sock_inuse) {
+ kmem_cache_free(net_cachep, net);
+ goto out_free;
+ }
+#endif
rcu_assign_pointer(net->gen, ng);
out:
return net;
@@ -374,6 +381,9 @@ static struct net *net_alloc(void)
static void net_free(struct net *net)
{
+#ifdef CONFIG_PROC_FS
+ free_percpu(net->core.sock_inuse);
+#endif
kfree(rcu_access_pointer(net->gen));
kmem_cache_free(net_cachep, net);
}
diff --git a/net/core/sock.c b/net/core/sock.c
index c2dd2d3..f6974eb 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -145,6 +145,8 @@
static DEFINE_MUTEX(proto_list_mutex);
static LIST_HEAD(proto_list);
+static void sock_inuse_add(struct net *net, int val);
+
/**
* sk_ns_capable - General socket capability test
* @sk: Socket to use a capability on or through
@@ -1534,6 +1536,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
if (likely(sk->sk_net_refcnt))
get_net(net);
sock_net_set(sk, net);
+ sock_inuse_add(net, 1);
refcount_set(&sk->sk_wmem_alloc, 1);
mem_cgroup_sk_alloc(sk);
@@ -1595,6 +1598,8 @@ void sk_destruct(struct sock *sk)
static void __sk_free(struct sock *sk)
{
+ sock_inuse_add(sock_net(sk), -1);
+
if (unlikely(sock_diag_has_destroy_listeners(sk) && sk->sk_net_refcnt))
sock_diag_broadcast_destroy(sk);
else
@@ -1716,6 +1721,7 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
newsk->sk_priority = 0;
newsk->sk_incoming_cpu = raw_smp_processor_id();
atomic64_set(&newsk->sk_cookie, 0);
+ sock_inuse_add(sock_net(newsk), 1);
/*
* Before updating sk_refcnt, we must commit prior changes to memory
@@ -3061,6 +3067,22 @@ int sock_prot_inuse_get(struct net *net, struct proto *prot)
}
EXPORT_SYMBOL_GPL(sock_prot_inuse_get);
+static void sock_inuse_add(struct net *net, int val)
+{
+ this_cpu_add(*net->core.sock_inuse, val);
+}
+
+int sock_inuse_get(struct net *net)
+{
+ int cpu, res = 0;
+
+ for_each_possible_cpu(cpu)
+ res += *per_cpu_ptr(net->core.sock_inuse, cpu);
+
+ return res >= 0 ? res : 0;
+}
+EXPORT_SYMBOL_GPL(sock_inuse_get);
+
static int __net_init sock_inuse_init_net(struct net *net)
{
net->core.prot_inuse = alloc_percpu(struct prot_inuse);
@@ -3112,6 +3134,10 @@ static inline void assign_proto_idx(struct proto *prot)
static inline void release_proto_idx(struct proto *prot)
{
}
+
+static void sock_inuse_add(struct net *net, int val)
+{
+}
#endif
static void req_prot_cleanup(struct request_sock_ops *rsk_prot)
diff --git a/net/socket.c b/net/socket.c
index 05f361f..bbd2e9c 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -163,12 +163,6 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
static const struct net_proto_family __rcu *net_families[NPROTO] __read_mostly;
/*
- * Statistics counters of the socket lists
- */
-
-static DEFINE_PER_CPU(int, sockets_in_use);
-
-/*
* Support routines.
* Move socket addresses back and forth across the kernel/user
* divide and look after the messy bits.
@@ -578,7 +572,6 @@ struct socket *sock_alloc(void)
inode->i_gid = current_fsgid();
inode->i_op = &sockfs_inode_ops;
- this_cpu_add(sockets_in_use, 1);
return sock;
}
EXPORT_SYMBOL(sock_alloc);
@@ -605,7 +598,6 @@ void sock_release(struct socket *sock)
if (rcu_dereference_protected(sock->wq, 1)->fasync_list)
pr_err("%s: fasync list not empty!\n", __func__);
- this_cpu_sub(sockets_in_use, 1);
if (!sock->file) {
iput(SOCK_INODE(sock));
return;
@@ -2622,17 +2614,8 @@ static int __init sock_init(void)
#ifdef CONFIG_PROC_FS
void socket_seq_show(struct seq_file *seq)
{
- int cpu;
- int counter = 0;
-
- for_each_possible_cpu(cpu)
- counter += per_cpu(sockets_in_use, cpu);
-
- /* It can be negative, by the way. 8) */
- if (counter < 0)
- counter = 0;
-
- seq_printf(seq, "sockets: used %d\n", counter);
+ seq_printf(seq, "sockets: used %d\n",
+ sock_inuse_get(seq->private));
}
#endif /* CONFIG_PROC_FS */
--
1.8.3.1
^ permalink raw reply related
* [PATCH v6 1/3] sock: Change the netns_core member name.
From: Tonghao Zhang @ 2017-12-10 15:12 UTC (permalink / raw)
To: davem, xiyou.wangcong, edumazet, willemb, xemul; +Cc: netdev, Tonghao Zhang
Change the member name will make the code more readable.
This patch will be used in next patch.
Signed-off-by: Martin Zhang <zhangjunweimartin@didichuxing.com>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
include/net/netns/core.h | 2 +-
net/core/sock.c | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/net/netns/core.h b/include/net/netns/core.h
index 0ad4d0c..45cfb5d 100644
--- a/include/net/netns/core.h
+++ b/include/net/netns/core.h
@@ -11,7 +11,7 @@ struct netns_core {
int sysctl_somaxconn;
- struct prot_inuse __percpu *inuse;
+ struct prot_inuse __percpu *prot_inuse;
};
#endif
diff --git a/net/core/sock.c b/net/core/sock.c
index c0b5b2f..c2dd2d3 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3045,7 +3045,7 @@ struct prot_inuse {
void sock_prot_inuse_add(struct net *net, struct proto *prot, int val)
{
- __this_cpu_add(net->core.inuse->val[prot->inuse_idx], val);
+ __this_cpu_add(net->core.prot_inuse->val[prot->inuse_idx], val);
}
EXPORT_SYMBOL_GPL(sock_prot_inuse_add);
@@ -3055,7 +3055,7 @@ int sock_prot_inuse_get(struct net *net, struct proto *prot)
int res = 0;
for_each_possible_cpu(cpu)
- res += per_cpu_ptr(net->core.inuse, cpu)->val[idx];
+ res += per_cpu_ptr(net->core.prot_inuse, cpu)->val[idx];
return res >= 0 ? res : 0;
}
@@ -3063,13 +3063,13 @@ int sock_prot_inuse_get(struct net *net, struct proto *prot)
static int __net_init sock_inuse_init_net(struct net *net)
{
- net->core.inuse = alloc_percpu(struct prot_inuse);
- return net->core.inuse ? 0 : -ENOMEM;
+ net->core.prot_inuse = alloc_percpu(struct prot_inuse);
+ return net->core.prot_inuse ? 0 : -ENOMEM;
}
static void __net_exit sock_inuse_exit_net(struct net *net)
{
- free_percpu(net->core.inuse);
+ free_percpu(net->core.prot_inuse);
}
static struct pernet_operations net_inuse_ops = {
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams
From: Marcelo Ricardo Leitner @ 2017-12-10 13:51 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman, syzkaller
In-Reply-To: <a6c86b9cb13c2b6e2ed2539c028557a6b1a713ef.1512891651.git.lucien.xin@gmail.com>
On Sun, Dec 10, 2017 at 03:40:51PM +0800, Xin Long wrote:
> Now in sctp_setsockopt_reset_streams, it only does the check
> optlen < sizeof(*params) for optlen. But it's not enough, as
> params->srs_number_streams should also match optlen.
>
> If the streams in params->srs_stream_list are less than stream
> nums in params->srs_number_streams, later when dereferencing
> the stream list, it could cause a slab-out-of-bounds crash, as
> reported by syzbot.
>
> This patch is to fix it by also checking the stream numbers in
> sctp_setsockopt_reset_streams to make sure at least it's not
> greater than the streams in the list.
>
> Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/socket.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 014847e..dbf140d 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3891,13 +3891,17 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
> struct sctp_association *asoc;
> int retval = -EINVAL;
>
> - if (optlen < sizeof(struct sctp_reset_streams))
> + if (optlen < sizeof(*params))
> return -EINVAL;
>
> params = memdup_user(optval, optlen);
> if (IS_ERR(params))
> return PTR_ERR(params);
>
> + if (params->srs_number_streams * sizeof(__u16) >
> + optlen - sizeof(*params))
> + goto out;
> +
> asoc = sctp_id2assoc(sk, params->srs_assoc_id);
> if (!asoc)
> goto out;
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH 1/2] net: sh_eth: add support for SH7786
From: Sergei Shtylyov @ 2017-12-10 12:46 UTC (permalink / raw)
To: Thomas Petazzoni, David S. Miller, Niklas Söderlund,
Geert Uytterhoeven, Simon Horman
Cc: netdev, linux-renesas-soc
In-Reply-To: <86b47029-6474-31c9-b48a-9f5215b73378@cogentembedded.com>
On 12/10/2017 03:20 PM, Sergei Shtylyov wrote:
>> This commit adds the sh_eth_cpu_data structure that describes the
>> SH7786 variant of the IP.
>>
>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> ---
>> drivers/net/ethernet/renesas/sh_eth.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>> b/drivers/net/ethernet/renesas/sh_eth.c
>> index 0074c5998481..a3c48b2a713c 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> @@ -710,6 +710,30 @@ static struct sh_eth_cpu_data sh7724_data = {
>> .rpadir_value = 0x00020000, /* NET_IP_ALIGN assumed to be 2 */
>> };
>> +static struct sh_eth_cpu_data sh7786_data = {
>> + .set_duplex = sh_eth_set_duplex,
>
> Hm, no bitrate switching?
> (ECMR.OLB is said to be unused indeed in the manual...)
I meant CXR20.OLB -- as it's called in the SH7786 and R-Car manuals.
MBR, Sergei
^ permalink raw reply
* Re: v4.15-rc2 on thinkpad x60: ethernet stopped working
From: Gabriel C @ 2017-12-10 12:44 UTC (permalink / raw)
To: Pavel Machek, kernel list; +Cc: jeffrey.t.kirsher, intel-wired-lan, netdev
In-Reply-To: <20171210083949.GA3872@amd>
On 10.12.2017 09:39, Pavel Machek wrote:
> Hi!
Hi,
> In v4.15-rc2+, network manager can not see my ethernet card, and
> manual attempts to ifconfig it up did not really help, either.
>
> Card is:
>
> 02:00.0 Ethernet controller: Intel Corporation 82573L Gigabit Ethernet
> Controller
>
> Dmesg says:
>
> dmesg | grep eth
> [ 0.648931] e1000e 0000:02:00.0 eth0: (PCI Express:2.5GT/s:Width
> x1) 00:16:d3:25:19:04
> [ 0.648934] e1000e 0000:02:00.0 eth0: Intel(R) PRO/1000 Network
> Connection
> [ 0.649012] e1000e 0000:02:00.0 eth0: MAC: 2, PHY: 2, PBA No:
> 005302-003
> [ 0.706510] usbcore: registered new interface driver cdc_ether
> [ 6.557022] e1000e 0000:02:00.0 eth1: renamed from eth0
> [ 6.577554] systemd-udevd[2363]: renamed network interface eth0 to
> eth1
>
> Any ideas ?
Yes , 19110cfbb34d4af0cdfe14cd243f3b09dc95b013 broke it.
See:
https://bugzilla.kernel.org/show_bug.cgi?id=198047
Fix there :
https://marc.info/?l=linux-kernel&m=151272209903675&w=2
Regards,
Gabriel C
^ permalink raw reply
* Re: [PATCH 1/2] net: sh_eth: add support for SH7786
From: Sergei Shtylyov @ 2017-12-10 12:41 UTC (permalink / raw)
To: Thomas Petazzoni, David S. Miller, Niklas Söderlund,
Geert Uytterhoeven, Simon Horman
Cc: netdev, linux-renesas-soc
In-Reply-To: <86b47029-6474-31c9-b48a-9f5215b73378@cogentembedded.com>
On 12/10/2017 03:20 PM, Sergei Shtylyov wrote:
[...]
> The reset looks good...
Sorry, I meant to type "rest". :-)
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 1/2] net: sh_eth: add support for SH7786
From: Sergei Shtylyov @ 2017-12-10 12:20 UTC (permalink / raw)
To: Thomas Petazzoni, David S. Miller, Niklas Söderlund,
Geert Uytterhoeven, Simon Horman
Cc: netdev, linux-renesas-soc
In-Reply-To: <20171204141744.18599-2-thomas.petazzoni@free-electrons.com>
On 12/04/2017 05:17 PM, Thomas Petazzoni wrote:
> This commit adds the sh_eth_cpu_data structure that describes the
> SH7786 variant of the IP.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> drivers/net/ethernet/renesas/sh_eth.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 0074c5998481..a3c48b2a713c 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -710,6 +710,30 @@ static struct sh_eth_cpu_data sh7724_data = {
> .rpadir_value = 0x00020000, /* NET_IP_ALIGN assumed to be 2 */
> };
>
> +static struct sh_eth_cpu_data sh7786_data = {
> + .set_duplex = sh_eth_set_duplex,
Hm, no bitrate switching?
(ECMR.OLB is said to be unused indeed in the manual...)
> +
> + .register_type = SH_ETH_REG_FAST_SH4,
SH_ETH_REG_FAST_RCAR.
> +
> + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD,
> + .ecsipr_value = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP,
Good enough (though magic packet interrupt should work too)...
> + .eesipr_value = EESIPR_RFCOFIP | EESIPR_ADEIP | EESIPR_ECIIP |
> + EESIPR_FTCIP | EESIPR_TDEIP | EESIPR_TFUFIP |
> + EESIPR_FRIP | EESIPR_RDEIP | EESIPR_RFOFIP |
> + EESIPR_RMAFIP | EESIPR_RRFIP |
> + EESIPR_RTLFIP | EESIPR_RTSFIP |
> + EESIPR_PREIP | EESIPR_CERFIP,
> +
> + .tx_check = EESR_FTC | EESR_CND | EESR_DLC | EESR_CD | EESR_RTO,
> + .eesr_err_check = EESR_TWB | EESR_TABT | EESR_RABT | EESR_RFE |
> + EESR_RDE | EESR_RFRMER | EESR_TFE | EESR_TDE,
I think you also need:
.fdr_value = 0x00000f0f,
like on R-Car gen1 SoCs...
[...]
The reset looks good...
MBR, Sergei
^ permalink raw reply
* Module compile error
From: Алексей Болдырев @ 2017-12-10 11:58 UTC (permalink / raw)
To: netdev
CC [M] drivers/net/ethernet/intel/ixgbe/ixgbe_main.o
In file included from ./include/net/vxlan.h:6:0,
from drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:60:
./include/net/dst_metadata.h: In function ‘skb_vpls_info’:
./include/net/dst_metadata.h:36:9: error: implicit declaration of function ‘skb_metadata_dst’ [-Werror=implicit-function-declaration]
struct metadata_dst *md_dst = skb_metadata_dst(skb);
^
./include/net/dst_metadata.h:36:32: warning: initialization makes pointer from integer without a cast
struct metadata_dst *md_dst = skb_metadata_dst(skb);
^
./include/net/dst_metadata.h: At top level:
./include/net/dst_metadata.h:42:36: error: conflicting types for ‘skb_metadata_dst’
static inline struct metadata_dst *skb_metadata_dst(struct sk_buff *skb)
^
./include/net/dst_metadata.h:36:32: note: previous implicit declaration of ‘skb_metadata_dst’ was here
struct metadata_dst *md_dst = skb_metadata_dst(skb);
^
cc1: some warnings being treated as errors
scripts/Makefile.build:302: recipe for target 'drivers/net/ethernet/intel/ixgbe/ixgbe_main.o' failed
make[5]: *** [drivers/net/ethernet/intel/ixgbe/ixgbe_main.o] Error 1
scripts/Makefile.build:561: recipe for target 'drivers/net/ethernet/intel/ixgbe' failed
make[4]: *** [drivers/net/ethernet/intel/ixgbe] Error 2
scripts/Makefile.build:561: recipe for target 'drivers/net/ethernet/intel' failed
make[3]: *** [drivers/net/ethernet/intel] Error 2
scripts/Makefile.build:561: recipe for target 'drivers/net/ethernet' failed
make[2]: *** [drivers/net/ethernet] Error 2
scripts/Makefile.build:561: recipe for target 'drivers/net' failed
make[1]: *** [drivers/net] Error 2
Makefile:1019: recipe for target 'drivers' failed
make: *** [drivers] Error 2
This patch is https://github.com/eqvinox/vpls-linux-kernel/commit/d9d8efec0760bcea54496e7a3ed45c72316ffb6b#diff-00c78c934b7e227c38ff1b1b571db9e6
What could be the problem?
Kernel: 4.13.16
^ permalink raw reply
* Re: [PATCH 1/2] net: sh_eth: add support for SH7786
From: Sergei Shtylyov @ 2017-12-10 11:55 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: David S. Miller, Niklas Söderlund, Geert Uytterhoeven,
Simon Horman, netdev, linux-renesas-soc
In-Reply-To: <20171208164017.3e9a7cc6@windsurf.lan>
Hello!
On 12/08/2017 06:40 PM, Thomas Petazzoni wrote:
>>>>>> This commit adds the sh_eth_cpu_data structure that describes the
>>>>>> SH7786 variant of the IP.
>>>>>
>>>>> The manual seems to be unavailable, so I have to trust you. :-)
>>>>
>>>> Yes, sadly. However, if you tell me what to double check, I'd be happy
>>>> to do so.
>>>
>>> I have the manual now, will check against it...
>>> DaveM, I'm retracting my ACK for the time being.
>>
>> Starting to look into the manual, the current patch is wrong. SH7786 SoC
>> was probably the 1st one to use what we thought was R-Car specific register
>> layout. Definite NAK on this version.
>
> Thanks for the feedback. How do we proceed from there ? I don't have
Please use SH_ETH_REG_FAST_RCAR for the register layout.
> access to a lot of datasheets of the different Renesas SoCs, so it's
> not easy to figure out which IP variant the SH7786 is using compared to
> other Renesas SoCs.
I've already done that for you. :-)
> Just out of curiosity, which specific aspect makes you think the
> proposed patch is wrong ?
Total Ether register/bit documentation rehaul done for SH7786/R-Car --
including the register (and bit) rename and moving the registers to different
offsets...
> Have you noticed a specific register or field
> that isn't compatible with SH_ETH_REG_FAST_SH4 layout ?
There are surely SH4 registers that don't exist on SH7786 -- like BCFRR,
RTRATE, RPADIR, RBWAR, RDFAR, TBRAR, TDFAR...
> Note that my patch makes Ethernet work in practice on SH7784, I have
> root over NFS working as we speak.
I don't doubt it...
> This certainly doesn't mean that the
> patch is entirely correct, but it definitely means that the
> SH_ETH_REG_FAST_SH4 is close enough to what the SH7786 is using :-)
SH_ETH_REG_FAST_RCAR is definitely closer. :-)
> Thanks!
>
> Thomas
MBR, Sergei
^ permalink raw reply
* [PATCH net] fou: fix some member types in guehdr
From: Xin Long @ 2017-12-10 8:56 UTC (permalink / raw)
To: network dev; +Cc: davem, Tom Herbert
guehdr struct is used to build or parse gue packets, which
are always in big endian. It's better to define all guehdr
members as __beXX types.
Also, in validate_gue_flags it's not good to use a __be32
variable for both Standard flags(__be16) and Private flags
(__be32), and pass it to other funcions.
This patch could fix a bunch of sparse warnings from fou.
Fixes: 5024c33ac354 ("gue: Add infrastructure for flags and options")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
include/net/gue.h | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/net/gue.h b/include/net/gue.h
index 2fdb29c..fdad414 100644
--- a/include/net/gue.h
+++ b/include/net/gue.h
@@ -44,10 +44,10 @@ struct guehdr {
#else
#error "Please fix <asm/byteorder.h>"
#endif
- __u8 proto_ctype;
- __u16 flags;
+ __u8 proto_ctype;
+ __be16 flags;
};
- __u32 word;
+ __be32 word;
};
};
@@ -84,11 +84,10 @@ static inline size_t guehdr_priv_flags_len(__be32 flags)
* if there is an unknown standard or private flags, or the options length for
* the flags exceeds the options length specific in hlen of the GUE header.
*/
-static inline int validate_gue_flags(struct guehdr *guehdr,
- size_t optlen)
+static inline int validate_gue_flags(struct guehdr *guehdr, size_t optlen)
{
+ __be16 flags = guehdr->flags;
size_t len;
- __be32 flags = guehdr->flags;
if (flags & ~GUE_FLAGS_ALL)
return 1;
@@ -101,12 +100,13 @@ static inline int validate_gue_flags(struct guehdr *guehdr,
/* Private flags are last four bytes accounted in
* guehdr_flags_len
*/
- flags = *(__be32 *)((void *)&guehdr[1] + len - GUE_LEN_PRIV);
+ __be32 pflags = *(__be32 *)((void *)&guehdr[1] +
+ len - GUE_LEN_PRIV);
- if (flags & ~GUE_PFLAGS_ALL)
+ if (pflags & ~GUE_PFLAGS_ALL)
return 1;
- len += guehdr_priv_flags_len(flags);
+ len += guehdr_priv_flags_len(pflags);
if (len > optlen)
return 1;
}
--
2.1.0
^ permalink raw reply related
* v4.15-rc2 on thinkpad x60: ethernet stopped working
From: Pavel Machek @ 2017-12-10 8:39 UTC (permalink / raw)
To: kernel list; +Cc: jeffrey.t.kirsher, intel-wired-lan, netdev
[-- Attachment #1: Type: text/plain, Size: 899 bytes --]
Hi!
In v4.15-rc2+, network manager can not see my ethernet card, and
manual attempts to ifconfig it up did not really help, either.
Card is:
02:00.0 Ethernet controller: Intel Corporation 82573L Gigabit Ethernet
Controller
Dmesg says:
dmesg | grep eth
[ 0.648931] e1000e 0000:02:00.0 eth0: (PCI Express:2.5GT/s:Width
x1) 00:16:d3:25:19:04
[ 0.648934] e1000e 0000:02:00.0 eth0: Intel(R) PRO/1000 Network
Connection
[ 0.649012] e1000e 0000:02:00.0 eth0: MAC: 2, PHY: 2, PBA No:
005302-003
[ 0.706510] usbcore: registered new interface driver cdc_ether
[ 6.557022] e1000e 0000:02:00.0 eth1: renamed from eth0
[ 6.577554] systemd-udevd[2363]: renamed network interface eth0 to
eth1
Any ideas?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams
From: Xin Long @ 2017-12-10 7:40 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Neil Horman, syzkaller
Now in sctp_setsockopt_reset_streams, it only does the check
optlen < sizeof(*params) for optlen. But it's not enough, as
params->srs_number_streams should also match optlen.
If the streams in params->srs_stream_list are less than stream
nums in params->srs_number_streams, later when dereferencing
the stream list, it could cause a slab-out-of-bounds crash, as
reported by syzbot.
This patch is to fix it by also checking the stream numbers in
sctp_setsockopt_reset_streams to make sure at least it's not
greater than the streams in the list.
Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/socket.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 014847e..dbf140d 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3891,13 +3891,17 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
struct sctp_association *asoc;
int retval = -EINVAL;
- if (optlen < sizeof(struct sctp_reset_streams))
+ if (optlen < sizeof(*params))
return -EINVAL;
params = memdup_user(optval, optlen);
if (IS_ERR(params))
return PTR_ERR(params);
+ if (params->srs_number_streams * sizeof(__u16) >
+ optlen - sizeof(*params))
+ goto out;
+
asoc = sctp_id2assoc(sk, params->srs_assoc_id);
if (!asoc)
goto out;
--
2.1.0
^ permalink raw reply related
* Re: [PATCH net-next v3 2/5] net: Disable GRO_HW when generic XDP is installed on a device.
From: Michael Chan @ 2017-12-10 6:49 UTC (permalink / raw)
To: Alexander Duyck
Cc: David Miller, Netdev, Andrew Gospodarek, Ariel Elior,
everest-linux-l2
In-Reply-To: <CAKgT0UeqWjHEFYLzbSk5O_7xV=QYYtPd-HdnRaGgpYy=d3yhLg@mail.gmail.com>
On Sat, Dec 9, 2017 at 2:37 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Sat, Dec 9, 2017 at 1:40 PM, Michael Chan <michael.chan@broadcom.com> wrote:
>> On Sat, Dec 9, 2017 at 10:56 AM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> I think these two lines are redundant in dev_disable_lro, since
>>> netdev_update_features should propagate the disable to all of the
>>> lower devices.
>>
>> Right. But for GRO_HW, there is no automatic propagation.
>
> Right, but that is also an issue since the automatic propagation is
> what prevents LRO from being re-enabled on the lower devices.
>
>>> Also this doesn't prevent the lower devices from
>>> re-enabling gro_hw.
>>
>> Right. You can re-enable LRO on the upper device as well.
>
> On the upper device yes, but not on the lower devices. That was what I
> was getting at. With LRO there is netdev_sync_upper_features() and
> that prevents you from enabling it if the upper device has it
> disabled. The problem is right now there is nothing that sets it for
> the upper devices when they are added to something like a bond so that
> is one of the pieces that still has to be worked on before we can just
> use the existing sync logic.
I understand. But if the user really wants to re-enable LRO, he can
just re-enable LRO on the upper device first and then re-enable LRO on
the lower devices.
To permanently disable a feature, I think additional infrastructure
may be required so that the feature can be cleared in hw_features and
then re-enabled later when it is allowed.
^ permalink raw reply
* Re: [PATCH net-next v3 1/5] net: Introduce NETIF_F_GRO_HW.
From: Michael Chan @ 2017-12-10 6:40 UTC (permalink / raw)
To: Alexander Duyck
Cc: David Miller, Netdev, Andrew Gospodarek, Ariel Elior,
everest-linux-l2
In-Reply-To: <CAKgT0UdKcVb+Qpmbdv=TfFTdnO9PGDgcTXpXf+eJ_y9k4wz_CA@mail.gmail.com>
On Sat, Dec 9, 2017 at 2:04 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Sat, Dec 9, 2017 at 1:31 PM, Michael Chan <michael.chan@broadcom.com> wrote:
>> On Sat, Dec 9, 2017 at 10:50 AM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> So I would disagree with it being a subset of NETIF_F_GRO. If anything
>>> it is an alternative to NETIF_F_GRO. It is performing GRO much earlier
>>> at the device level in the case of hardware drivers. My concern is
>>> this is probably going to end up applying to things other than just
>>> hardware drivers though. For example what is to prevent this from
>>> being applied to something like a virtio/tap interface? It seems like
>>> this should be something that would be easy to implement in software.
>>
>> If you do it in software, it's called NETIF_F_GRO. We already have
>> it. The whole point of the new flag is that if the device has
>> software GRO enabled, and if the device supports GRO_HW, then we can
>> do a subset of GRO in hardware (hopefully faster).
>
> I can see what you are getting at. But GRO_HW with GRO stacked on top
> of it won't necessarily be the fastest form of GRO. If you have a
> GRO_HW implementation that is complete enough people may want to
> disable Software GRO in order to avoid the extra overhead involved
> with using it.
It is possible that if you have incoming packets 1, 2, 3, 4, 5 for a
TCP connection, HW_GRO can aggregate packets 1, 2, 3, but cannot
aggregate packets 4 and 5 due to hardware resource limitation.
Software GRO aggregates 4 and 5. So it works well together.
>>> I'm going to back off on my requirement for you to handle propagation
>>> since after spending a couple hours working on it I did find it was
>>> more complex then I originally thought it would be. With that said
>>> however I would want to see this feature implemented in such a way
>>> that we can deal with propagating the bits in the future if we need to
>>> and that is what I am basing my comments on.
>>
>> Nothing stops anyone from propagating the flag. Just add
>> NETIF_F_GRO_HW to NETIF_F_UPPER_DISABLES and it will be propagated
>> just like LRO.
>
> Yes, but the problem then is it doesn't solve the secondary issue of
> no way to propagate down the desire to disable GRO as well. That is
> why I am thinking that the new bit could be used to indicate that we
> want GRO to be supported either in the driver or below it instead of
> only in "hardware". We are much better off with a generic solution and
> that is why I think it might be better to use more of a pipeline or
> staged type definition for this. Basically with GRO it occurs in the
> GRO logic just after the driver hands off the packet, while this new
> bit indicates that GRO happens somewhere before then. If we use that
> definition for this then it becomes usable to deal with things like
> the stacked devices problem where the stacked devices normally have
> the GRO flag disabled since we don't want to run GRO multiple times,
> but as a result the stacked devices have no way of saying they don't
> want GRO. If we tweak the definition of this bit it solves that
> problem since it would allow for us disabling GRO, GRO_HW, and LRO on
> any devices below a given device.
I just don't follow your logic. First of all, GRO on an upper device
doesn't mean that we are doing GRO on the upper device. The bonding
driver cannot do GRO because it doesn't call napi_gro_receive(). GRO
always happens on the lower device. Propagation of GRO can only mean
that if GRO is set on the upper device, GRO is propagated and allowed
on lower devices. Nothing stops you from doing that if you want to do
that.
>>> I still disagree with this bit. I think GRO is a pure software
>>> offload, whereas GRO_HW can represent either a software offload of
>>> some sort occurring in or before the driver, or in the hardware.
>>> Basically the difference between the two as I view it is where the GRO
>>> is occurring. I would like to keep that distinction and make use of
>>> it. As I mentioned before in the case of bonding we currently have no
>>> way to disable GRO on the lower devices partially because GRO is a
>>> pure software feature and always happens at each device along the way.
>>> The nice thing about this new bit is the assumption is that it is
>>> pushing GRO to the lowest possible level and not triggering any side
>>> effects like GRO currently does. I hope to use that logic with stacked
>>> devices so that we could clear the bit and have it disable GRO,
>>> GRO_HW, and LRO on all devices below the device that cleared it.
>>>
>>> I think this linking of GRO and GRO_HW is something that would be
>>> better served by moving it into the driver if you are wanting to
>>> maintain the behavior of how this was previously linked to GRO.
>>
>> If you insist, I can move this to the driver's ndo_fix_features().
>> But I feel it is much better to enforce this dependency system wide.
>> Once again, GRO_HW is hardware accelerated GRO and should depend on
>> it.
>
> The question I would have is why? Where is the dependency? I don't see
> it. It is GRO in one spot and/or GRO in the other. The two don't
> interract directly and I don't believe you can do software GRO on a
> frame that has already been coalesced in hardware,
Right. But hardware can do a series of frames and software can do a
different series of frames that have not been aggregated.
>> This is a logical feature dependency that Yuval Mintz suggested. For
>> GRO_HW to work, hardware must verify the checksum of a packet before
>> the packet can be merged.
>>
>> So if the user does not want to do RXCSUM on this device for whatever
>> reason, it logically means that he also doesn't want to do GRO_HW with
>> implied RXCSUM performed on each packet that is merged.
>>
>> So I agree with Yuval that this dependency makes sense.
>
> Okay then, so if we are going to go that route we may want to be
> complete on this and just disable GRO_HW and LRO if RXCSUM is not
> enabled. We might also want to add a comment indicating that we don't
> support anything that might mangle a packet at the driver level if
> RXCSUM is not enabled. Comments explaining all this would be a good
> thing just to keep someone from grabbing GRO and lumping it in at some
> point in the future.
>
> I'm still working on trying to propagate the Rx checksum properly
> since it should probably follow the same UPPER_DISABLES behavior as
> LRO, but I will probably only have a few hours over the next week to
> really work on any code and there end up being a number of stacked
> drivers that have to be updated. I would be good with just flipping
> this logic for now and if RXCSUM is not set, and GRO_HW (just noticed
> the typo in your message) is set, then print your message and clear
> the bit. I can probably come back later and add LRO once I get the
> propagation bits worked out.
Just fix the netdev_dbg() typo, right? I don't understand what you
mean by flipping the logic. It's the same whether you check RXCSUM
first or GRO_HW first.
May be you meant put the RXCSUM check in the outer if statement so
that someone could add more inner checks? OK, I think that's what you
meant.
>
> As far as patch 2 in the set it would probably be better to either
> drop it and just accept it as an outstanding issue, or you could take
> on the propagation problems with GRO_HW and RXCSUM since we really
> need to get those solved in order for this functionality to fully
> work.
We need patch #2 otherwise generic GRO won't work on these 3 drivers.
I don't think I fully understand your concerns about propagation. To
me propagation is just a usage model where an upper device will
control the common features of lower devices. It is more convenient
to have propagation, but requires upper devices to be aware of all
features that propagate (GRO, RXCSUM). Without propagation, it is
still fine.
^ permalink raw reply
* Re: kernel BUG at net/core/skbuff.c:LINE! (2)
From: Eric Dumazet @ 2017-12-10 5:12 UTC (permalink / raw)
To: Xin Long, Cong Wang
Cc: syzbot, davem, kuznet, LKML, linux-sctp, network dev, Neil Horman,
syzkaller-bugs, Vlad Yasevich, yoshfuji
In-Reply-To: <CADvbK_cAeiW-Tt0OnTBrzHObKOaNjRZnB1Uu+GSRJhhFpHTU2w@mail.gmail.com>
On Sun, 2017-12-10 at 12:38 +0800, Xin Long wrote:
> On Sun, Dec 10, 2017 at 3:36 AM, Cong Wang <xiyou.wangcong@gmail.com>
> wrote:
> > On Fri, Dec 8, 2017 at 12:45 AM, Xin Long <lucien.xin@gmail.com>
> > wrote:
> > > This isn't a sctp problem, but mld's, seems when lo's mtu became
> > > 0,
> > > it allocs a skb without enough space in add_grec():
> >
> > Shouldn't we just set its min_mtu to ETH_MIN_MTU?
>
> No idea why there's no min_mtu limitation for lo dev.
Because it wont solve this bug.
ETH_MIN_MTU is really about IPv4.
^ permalink raw reply
* [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac
From: Al Viro @ 2017-12-10 4:53 UTC (permalink / raw)
To: Jie Deng; +Cc: netdev
In xlgmac_dev_xmit():
/* Mark it as a CONTEXT descriptor */
dma_desc->desc3 = XLGMAC_SET_REG_BITS_LE(
dma_desc->desc3,
TX_CONTEXT_DESC3_CTXT_POS,
TX_CONTEXT_DESC3_CTXT_LEN,
1);
Looking at XLGMAC_SET_REG_BITS_LE() we see this:
#define XLGMAC_SET_REG_BITS_LE(var, pos, len, val) ({ \
typeof(var) _var = (var); \
typeof(pos) _pos = (pos); \
typeof(len) _len = (len); \
typeof(val) _val = (val); \
_val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos); \
_var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val; \
cpu_to_le32(_var); \
})
That thing assumes var to be host-endian and has a little-endian result.
Unfortunately, we feed it a little-endian and store the result back into
the same place. That might work if the original values *was* host-endian
and we wanted to end up with little-endian. However, that is immediately
followed by
/* Indicate this descriptor contains the MSS */
dma_desc->desc3 = XLGMAC_SET_REG_BITS_LE(
dma_desc->desc3,
TX_CONTEXT_DESC3_TCMSSV_POS,
TX_CONTEXT_DESC3_TCMSSV_LEN,
1);
where we operate on the now definitely little-endian value. That really
can't be right. I don't have the hardware in question, so I can't test
that, but it smells like this needs something like diff below, making
XLGMAC_SET_REG_BITS_LE take le32 and return le32. GET side of things
is le32 -> u32; definition looks correct, but slightly misannotated.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/net/ethernet/synopsys/dwc-xlgmac.h b/drivers/net/ethernet/synopsys/dwc-xlgmac.h
index cab3e40a86b9..e95c4c250e16 100644
--- a/drivers/net/ethernet/synopsys/dwc-xlgmac.h
+++ b/drivers/net/ethernet/synopsys/dwc-xlgmac.h
@@ -106,7 +106,7 @@
#define XLGMAC_GET_REG_BITS_LE(var, pos, len) ({ \
typeof(pos) _pos = (pos); \
typeof(len) _len = (len); \
- typeof(var) _var = le32_to_cpu((var)); \
+ u32 _var = le32_to_cpu((var)); \
((_var) & GENMASK(_pos + _len - 1, _pos)) >> (_pos); \
})
@@ -125,8 +125,8 @@
typeof(len) _len = (len); \
typeof(val) _val = (val); \
_val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos); \
- _var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val; \
- cpu_to_le32(_var); \
+ (_var & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | \
+ cpu_to_le32(_val); \
})
struct xlgmac_pdata;
^ permalink raw reply related
* Re: kernel BUG at net/core/skbuff.c:LINE! (2)
From: Eric Dumazet @ 2017-12-10 4:51 UTC (permalink / raw)
To: Xin Long
Cc: syzbot, davem, kuznet, LKML, linux-sctp, network dev, Neil Horman,
syzkaller-bugs, Vlad Yasevich, yoshfuji
In-Reply-To: <CADvbK_dMfCf82LYZWeWp1EOzStwuTyjP7SzbUwa0vnT5sc38Qw@mail.gmail.com>
On Sun, 2017-12-10 at 12:36 +0800, Xin Long wrote:
> The new patch works to me, just two questions:
> 1. should it use "idev->cnf.mtu6" here for mld ?
No idea why some parts of IPv6 would use a different view of device
mtu. Maybe others can comment.
>
> 2. 'if (int < unsigned int)' is still not nice, though in 'if
> (AVAILABLE(skb) < sizeof())'
> AVAILABLE(skb) seems always to return >= 0 after your patch.
Really if AVAILABLE(skb) was negative, a bug already happened.
^ 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