[PATCH v2] net/phy: micrel: configure intterupts after autoneg workaround

[Zach Brown]

The commit ("net/phy: micrel: Add workaround for bad autoneg") fixes an
autoneg failure case by resetting the hardware. This turns off
intterupts. Things will work themselves out if the phy polls, as it will
figure out it's state during a poll. However if the phy uses only
intterupts, the phy will stall, since interrupts are off. This patch
fixes the issue by calling config_intr after resetting the phy.

Fixes: d2fd719bcb0e ("net/phy: micrel: Add workaround for bad autoneg ")
Signed-off-by: Zach Brown <zach.br...@ni.com>
---
v2:
 * Check phy_intterupt_is_valid before calling config_intr

 drivers/net/phy/micrel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 9365b07..fdb43dd 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -620,6 +620,8 @@ static int ksz9031_read_status(struct phy_device *phydev)
if ((regval & 0xFF) == 0xFF) {
phy_init_hw(phydev);
phydev->link = 0;
+   if (phydev->drv->config_intr && phy_interrupt_is_valid(phydev))
+   phydev->drv->config_intr(phydev);
}
 
return 0;
-- 
2.7.4

[PATCH] net/phy: micrel: configure intterupts after autoneg workaround

[Zach Brown]

The commit ("net/phy: micrel: Add workaround for bad autoneg") fixes
an autoneg failure case by resetting the hardware. This turns off
intterupts. Things will work themselves out if the phy
polls, as it will figure out it's state during a poll. However if the
phy uses only intterupts, the phy will stall, since interrupts
are off.

This patch fixes the issue by calling config_intr after resetting the
phy.

Fixes: d2fd719bcb0e ("net/phy: micrel: Add workaround for bad autoneg ")
Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/micrel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 9365b07..bc2a0a4 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -620,6 +620,8 @@ static int ksz9031_read_status(struct phy_device *phydev)
if ((regval & 0xFF) == 0xFF) {
phy_init_hw(phydev);
phydev->link = 0;
+   if (phydev->drv->config_intr)
+   phydev->drv->config_intr(phydev);
}
 
return 0;
-- 
2.7.4

[PATCH] net: phy: handle state correctly in phy_stop_machine

[Zach Brown]

From: Nathan Sullivan 

If the PHY is halted on stop, then do not set the state to PHY_UP.  This
ensures the phy will be restarted later in phy_start when the machine is
started again.

Signed-off-by: Nathan Sullivan 
Signed-off-by: Brad Mouring 
Acked-by: Xander Huff 
Acked-by: Kyle Roeschley 
---
 drivers/net/phy/phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7cc1b7d..fe2d4c4 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -678,7 +678,7 @@ void phy_stop_machine(struct phy_device *phydev)
cancel_delayed_work_sync(>state_queue);
 
mutex_lock(>lock);
-   if (phydev->state > PHY_UP)
+   if (phydev->state > PHY_UP && phydev->state != PHY_HALTED)
phydev->state = PHY_UP;
mutex_unlock(>lock);
 }
-- 
2.7.4

Re: [PATCH v2] net: phy: Fix use after free in phy_detach()

[Zach Brown]

On Mon, Nov 28, 2016 at 03:18:31PM +0100, Geert Uytterhoeven wrote:
> If device_release_driver(>mdio.dev) is called, it releases all
> resources belonging to the PHY device. Hence the subsequent call to
> phy_led_triggers_unregister() will access already freed memory when
> unregistering the LEDs.
> 
> Move the call to phy_led_triggers_unregister() before the possible call
> to device_release_driver() to fix this.
> 
> Fixes: 2e0bc452f4721520 ("net: phy: leds: add support for led triggers on phy 
> link state change")
> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
> ---
> This is v2 of "[RFC] net: phy: Fix double free in phy_detach()".
> 
> v2:
>   - Dropped RFC,
>   - Reworded, as commit a7dac9f9c1695d74 ("phy: fix error case of
> phy_led_triggers_(un)register") fixed the double free, and thus the
> warning I was seeing during "poweroff" on sh73a0/kzm9g,
>   - Verified use after free using CONFIG_DEBUG_DEVRES, log_devres = 1,
> and additional debug code printing the address of
> phy->phy_led_triggers. Adding poisoning of freed memory to
> devres_log() will cause a crash.
> ---
>  drivers/net/phy/phy_device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index ba86c191a13ea81c..a1d6e13b1b4113a4 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -981,6 +981,8 @@ void phy_detach(struct phy_device *phydev)
>   phydev->attached_dev = NULL;
>   phy_suspend(phydev);
>  
> + phy_led_triggers_unregister(phydev);
> +
>   /* If the device had no specific driver before (i.e. - it
>* was using the generic driver), we unbind the device
>* from the generic driver so that there's a chance a
> @@ -994,8 +996,6 @@ void phy_detach(struct phy_device *phydev)
>   }
>   }
>  
> - phy_led_triggers_unregister(phydev);
> -
>   /*
>* The phydev might go away on the put_device() below, so avoid
>* a use-after-free bug by reading the underlying bus first.
> -- 
> 1.9.1
> 

I was able to recreate the issue and confirmed this patch fixes it.
Tested-by: Zach Brown <zach.br...@ni.com>

--Zach

Re: Pure polling mode for netdevices

[Zach Brown]

On Fri, Oct 21, 2016 at 11:41:46AM -0700, Eric Dumazet wrote:
> On Fri, 2016-10-21 at 13:03 -0500, Zach Brown wrote:
> > Is there a way to get NAPI to poll all the time?
> > Or just any way to get netdevices to use only polling and no interrupts?
> > 
> > We have some rt targets where the jitter can be improved by disabling
> > interrupts and using just polling. In these cases we're okay with the
> > performance downsides that come with polling.
> > 
> > In particular we already have an implementation for the cadence macb driver
> > that does only polling mode and have verified that it improves the
> > jitter.
> > 
> > We're hoping for a more general existing solution or at the very least a
> > solution that would be accepted upstream. Any thoughts?
> 
> This is not yet done, although you could use busy poll infrastructure to
> get this without a kernel change.
> 
> Open as many TCP flows are necessary to traverse all the queues you care
> about, then loop on recvmsg() to trigger NAPI polling.
> 
> 

I looked into the busy poll infrastructure and I don't think it meets our
needs. We're not interested in minimizing the latency of the ethernet device,
but rather diminishing if not eliminating the system jitter cause by ethernet
interrupts. Not to mention that some interfaces like macb don't provide a lot
of the config options to make busy polling really work well. For example,
interrupt coalescing configuration. 

I was thinking more along the lines of an extenstion to the NAPI interface that
drivers can opt into that allows them to enter NAPI polling mode and never
leave when configured to do so.  

Pure polling mode for netdevices

[Zach Brown]

Is there a way to get NAPI to poll all the time?
Or just any way to get netdevices to use only polling and no interrupts?

We have some rt targets where the jitter can be improved by disabling
interrupts and using just polling. In these cases we're okay with the
performance downsides that come with polling.

In particular we already have an implementation for the cadence macb driver
that does only polling mode and have verified that it improves the
jitter.

We're hoping for a more general existing solution or at the very least a
solution that would be accepted upstream. Any thoughts?

[PATCH 0/2] Add ethtool get_ringparam and set_ringparam to cadence

[Zach Brown]

There are use cases like RT that would benefit from being able to tune the
macb rx/tx ring sizes. The ethtool set_ringparam function is the standard way
of doing so.

The first patch changes the hardcoded tx/rx ring sizes to variables that are
set to a hardcoded default.

The second patch implements the get_ringparam and set_ringparam fucntions.

Zach Brown (2):
  net: macb: Use variables with defaults for tx/rx ring sizes instead of
hardcoded values
  net: macb: Add ethtool get_ringparam and set_ringparam functionality

 drivers/net/ethernet/cadence/macb.c | 173 +---
 drivers/net/ethernet/cadence/macb.h |   3 +
 2 files changed, 124 insertions(+), 52 deletions(-)

--
2.7.4

[PATCH 2/2] net: macb: Add ethtool get_ringparam and set_ringparam functionality

[Zach Brown]

Some applications want to tune the size of the macb rx/tx ring buffers.
The ethtool set_ringparam function is the standard way of doing it.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/ethernet/cadence/macb.c | 59 +
 1 file changed, 59 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index 644975d..e1847ce 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -37,11 +37,16 @@

 #define MACB_RX_BUFFER_SIZE128
 #define RX_BUFFER_MULTIPLE 64  /* bytes */
+
 #define DEFAULT_RX_RING_SIZE   512 /* must be power of 2 */
+#define MIN_RX_RING_SIZE   64
+#define MAX_RX_RING_SIZE   8192
 #define RX_RING_BYTES(bp)  (sizeof(struct macb_dma_desc)   \
 * (bp)->rx_ring_size)

 #define DEFAULT_TX_RING_SIZE   512 /* must be power of 2 */
+#define MIN_TX_RING_SIZE   64
+#define MAX_TX_RING_SIZE   4096
 #define TX_RING_BYTES(bp)  (sizeof(struct macb_dma_desc)   \
 * (bp)->tx_ring_size)

@@ -2211,6 +2216,56 @@ static int macb_set_wol(struct net_device *netdev, 
struct ethtool_wolinfo *wol)
return 0;
 }

+static void macb_get_ringparam(struct net_device *netdev,
+  struct ethtool_ringparam *ring)
+{
+   struct macb *bp = netdev_priv(netdev);
+
+   ring->rx_max_pending = MAX_RX_RING_SIZE;
+   ring->tx_max_pending = MAX_TX_RING_SIZE;
+
+   ring->rx_pending = bp->rx_ring_size;
+   ring->tx_pending = bp->tx_ring_size;
+}
+
+static int macb_set_ringparam(struct net_device *netdev,
+ struct ethtool_ringparam *ring)
+{
+   struct macb *bp = netdev_priv(netdev);
+   u32 new_rx_size, new_tx_size;
+   unsigned int reset = 0;
+
+   if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
+   return -EINVAL;
+
+   new_rx_size = clamp_t(u32, ring->rx_pending,
+ MIN_RX_RING_SIZE, MAX_RX_RING_SIZE);
+   new_rx_size = roundup_pow_of_two(new_rx_size);
+
+   new_tx_size = clamp_t(u32, ring->tx_pending,
+ MIN_TX_RING_SIZE, MAX_TX_RING_SIZE);
+   new_tx_size = roundup_pow_of_two(new_tx_size);
+
+   if ((new_tx_size == bp->tx_ring_size) &&
+   (new_rx_size == bp->rx_ring_size)) {
+   /* nothing to do */
+   return 0;
+   }
+
+   if (netif_running(bp->dev)) {
+   reset = 1;
+   macb_close(bp->dev);
+   }
+
+   bp->rx_ring_size = new_rx_size;
+   bp->tx_ring_size = new_tx_size;
+
+   if (reset)
+   macb_open(bp->dev);
+
+   return 0;
+}
+
 static const struct ethtool_ops macb_ethtool_ops = {
.get_regs_len   = macb_get_regs_len,
.get_regs   = macb_get_regs,
@@ -2220,6 +2275,8 @@ static const struct ethtool_ops macb_ethtool_ops = {
.set_wol= macb_set_wol,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
+   .get_ringparam  = macb_get_ringparam,
+   .set_ringparam  = macb_set_ringparam,
 };

 static const struct ethtool_ops gem_ethtool_ops = {
@@ -2232,6 +2289,8 @@ static const struct ethtool_ops gem_ethtool_ops = {
.get_sset_count = gem_get_sset_count,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
+   .get_ringparam  = macb_get_ringparam,
+   .set_ringparam  = macb_set_ringparam,
 };

 static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
--
2.7.4

[PATCH 1/2] net: macb: Use variables with defaults for tx/rx ring sizes instead of hardcoded values

[Zach Brown]

The macb driver hardcoded the tx/rx ring sizes. This made it
impossible to change the sizes at run time.

Add tx_ring_size, and rx_ring_size variables to macb object, which
are initilized with default vales during macb_init. Change all
references to RX_RING_SIZE and TX_RING_SIZE to their respective
replacements.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/ethernet/cadence/macb.c | 114 
 drivers/net/ethernet/cadence/macb.h |   3 +
 2 files changed, 65 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index e83781a..644975d 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -37,14 +37,16 @@
 
 #define MACB_RX_BUFFER_SIZE128
 #define RX_BUFFER_MULTIPLE 64  /* bytes */
-#define RX_RING_SIZE   512 /* must be power of 2 */
-#define RX_RING_BYTES  (sizeof(struct macb_dma_desc) * RX_RING_SIZE)
+#define DEFAULT_RX_RING_SIZE   512 /* must be power of 2 */
+#define RX_RING_BYTES(bp)  (sizeof(struct macb_dma_desc)   \
+* (bp)->rx_ring_size)
 
-#define TX_RING_SIZE   128 /* must be power of 2 */
-#define TX_RING_BYTES  (sizeof(struct macb_dma_desc) * TX_RING_SIZE)
+#define DEFAULT_TX_RING_SIZE   512 /* must be power of 2 */
+#define TX_RING_BYTES(bp)  (sizeof(struct macb_dma_desc)   \
+* (bp)->tx_ring_size)
 
 /* level of occupied TX descriptors under which we wake up TX process */
-#define MACB_TX_WAKEUP_THRESH  (3 * TX_RING_SIZE / 4)
+#define MACB_TX_WAKEUP_THRESH(bp)  (3 * (bp)->tx_ring_size / 4)
 
 #define MACB_RX_INT_FLAGS  (MACB_BIT(RCOMP) | MACB_BIT(RXUBR)  \
 | MACB_BIT(ISR_ROVR))
@@ -67,45 +69,47 @@
 #define MACB_HALT_TIMEOUT  1230
 
 /* Ring buffer accessors */
-static unsigned int macb_tx_ring_wrap(unsigned int index)
+static unsigned int macb_tx_ring_wrap(struct macb *bp, unsigned int index)
 {
-   return index & (TX_RING_SIZE - 1);
+   return index & (bp->tx_ring_size - 1);
 }
 
 static struct macb_dma_desc *macb_tx_desc(struct macb_queue *queue,
  unsigned int index)
 {
-   return >tx_ring[macb_tx_ring_wrap(index)];
+   return >tx_ring[macb_tx_ring_wrap(queue->bp, index)];
 }
 
 static struct macb_tx_skb *macb_tx_skb(struct macb_queue *queue,
   unsigned int index)
 {
-   return >tx_skb[macb_tx_ring_wrap(index)];
+   return >tx_skb[macb_tx_ring_wrap(queue->bp, index)];
 }
 
 static dma_addr_t macb_tx_dma(struct macb_queue *queue, unsigned int index)
 {
dma_addr_t offset;
 
-   offset = macb_tx_ring_wrap(index) * sizeof(struct macb_dma_desc);
+   offset = macb_tx_ring_wrap(queue->bp, index) *
+sizeof(struct macb_dma_desc);
 
return queue->tx_ring_dma + offset;
 }
 
-static unsigned int macb_rx_ring_wrap(unsigned int index)
+static unsigned int macb_rx_ring_wrap(struct macb *bp, unsigned int index)
 {
-   return index & (RX_RING_SIZE - 1);
+   return index & (bp->rx_ring_size - 1);
 }
 
 static struct macb_dma_desc *macb_rx_desc(struct macb *bp, unsigned int index)
 {
-   return >rx_ring[macb_rx_ring_wrap(index)];
+   return >rx_ring[macb_rx_ring_wrap(bp, index)];
 }
 
 static void *macb_rx_buffer(struct macb *bp, unsigned int index)
 {
-   return bp->rx_buffers + bp->rx_buffer_size * macb_rx_ring_wrap(index);
+   return bp->rx_buffers + bp->rx_buffer_size *
+  macb_rx_ring_wrap(bp, index);
 }
 
 /* I/O accessors */
@@ -608,7 +612,8 @@ static void macb_tx_error_task(struct work_struct *work)
 */
if (!(ctrl & MACB_BIT(TX_BUF_EXHAUSTED))) {
netdev_vdbg(bp->dev, "txerr skb %u (data %p) TX 
complete\n",
-   macb_tx_ring_wrap(tail), skb->data);
+   macb_tx_ring_wrap(bp, tail),
+   skb->data);
bp->stats.tx_packets++;
bp->stats.tx_bytes += skb->len;
}
@@ -700,7 +705,8 @@ static void macb_tx_interrupt(struct macb_queue *queue)
/* First, update TX stats if needed */
if (skb) {
netdev_vdbg(bp->dev, "skb %u (data %p) TX 
complete\n",
-   macb_tx_ring_wrap(tail), skb->data);
+   macb_tx_ring_wrap(bp, tail),
+   skb->data);
bp->stats.tx_packets++;
   

Re: [PATCH v5 4/4] net: phy: leds: add support for led triggers on phy link state change

[Zach Brown]

On Tue, Oct 18, 2016 at 09:13:50AM +0200, Andrew Lunn wrote:
> On Mon, Oct 17, 2016 at 10:49:55AM -0500, Zach Brown wrote:
> > Create an option CONFIG_LED_TRIGGER_PHY (default n), which will create a
> > set of led triggers for each instantiated PHY device. There is one LED
> > trigger per link-speed, per-phy.
> > The triggers are registered during phy_attach and unregistered during
> > phy_detach.
> >
> > This allows for a user to configure their system to allow a set of LEDs
> > not controlled by the phy to represent link state changes on the phy.
> > LEDS controlled by the phy are unaffected.
> >
> > For example, we have a board where some of the leds in the
> > RJ45 socket are controlled by the phy, but others are not. Using the
> > triggers provided by this patch the leds not controlled by the phy can
> > be configured to show the current speed of the ethernet connection. The
> > leds controlled by the phy are unaffected.
>
> Is there a clear path how we generalise this, so it could also be used
> to control PHY LEDs?
>

No, this patch set is only concerned with generic LEDs not PHY LEDs.

> The idea i had a while ago was that the PHY LEDS would also have a
> list of triggers which mapped to what the PHY controller could do. So
> to enable the PHY LED to show RxTx activity, you would enable the RxTx
> trigger on the PHY LED.
>
> This needs an extension to the PHY code, in that these triggers are
> specific to the LED, not general across all LEDs. But this is not too
> big an issue.
>
> We could end up that if a PHY LED can be used as a generic LED, your
> 'manual' trigger could be used on it. But it could also support the
> PHY driven 'automatic' link status indication trigger. Do we want two
> triggers for the same or similar information?
>

If generic LEDs can only use the 'manual' triggers then having two triggers
for the same or similar information would be okay. The focus of this patch set
was to allow generic LEDs to represent the current speed of the phy, which is
very useful when the PHY LEDs don't for whatever reason i.e they don't exist or
are busy representing another aspect.

[PATCH v5 2/4] net: phy: Encapsulate actions performed during link state changes into function phy_adjust_link

[Zach Brown]

During phy state machine state transitions some set of actions should
occur whenever the link state changes. These actions should be
encapsulated into a single function

This patch adds the phy_adjust_link function, which is called whenever
phydev->adjust_link would have been called before. Actions that should
occur whenever the phy link is adjusted can now be added to the
phy_adjust_link function.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/phy.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c6f6683..f5721db 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -893,6 +893,11 @@ void phy_start(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_start);
 
+static void phy_adjust_link(struct phy_device *phydev)
+{
+   phydev->adjust_link(phydev->attached_dev);
+}
+
 /**
  * phy_state_machine - Handle the state machine
  * @work: work_struct that describes the work to be done
@@ -935,7 +940,7 @@ void phy_state_machine(struct work_struct *work)
if (!phydev->link) {
phydev->state = PHY_NOLINK;
netif_carrier_off(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
break;
}
 
@@ -948,7 +953,7 @@ void phy_state_machine(struct work_struct *work)
if (err > 0) {
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
 
} else if (0 == phydev->link_timeout--)
needs_aneg = true;
@@ -975,7 +980,7 @@ void phy_state_machine(struct work_struct *work)
}
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
}
break;
case PHY_FORCING:
@@ -991,7 +996,7 @@ void phy_state_machine(struct work_struct *work)
needs_aneg = true;
}
 
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
break;
case PHY_RUNNING:
/* Only register a CHANGE if we are polling and link changed
@@ -1020,7 +1025,7 @@ void phy_state_machine(struct work_struct *work)
netif_carrier_off(phydev->attached_dev);
}
 
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
 
if (phy_interrupt_is_valid(phydev))
err = phy_config_interrupt(phydev,
@@ -1030,7 +1035,7 @@ void phy_state_machine(struct work_struct *work)
if (phydev->link) {
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
do_suspend = true;
}
break;
@@ -1054,7 +1059,7 @@ void phy_state_machine(struct work_struct *work)
} else  {
phydev->state = PHY_NOLINK;
}
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
} else {
phydev->state = PHY_AN;
phydev->link_timeout = PHY_AN_TIMEOUT;
@@ -1070,7 +1075,7 @@ void phy_state_machine(struct work_struct *work)
} else  {
phydev->state = PHY_NOLINK;
}
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
}
break;
}
-- 
2.7.4

[PATCH v5 4/4] net: phy: leds: add support for led triggers on phy link state change

[Zach Brown]

Create an option CONFIG_LED_TRIGGER_PHY (default n), which will create a
set of led triggers for each instantiated PHY device. There is one LED
trigger per link-speed, per-phy.
The triggers are registered during phy_attach and unregistered during
phy_detach.

This allows for a user to configure their system to allow a set of LEDs
not controlled by the phy to represent link state changes on the phy.
LEDS controlled by the phy are unaffected.

For example, we have a board where some of the leds in the
RJ45 socket are controlled by the phy, but others are not. Using the
triggers provided by this patch the leds not controlled by the phy can
be configured to show the current speed of the ethernet connection. The
leds controlled by the phy are unaffected.

Signed-off-by: Josh Cartwright <josh.cartwri...@ni.com>
Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com>
Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/Kconfig|  13 
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/phy.c  |   1 +
 drivers/net/phy/phy_device.c   |   5 ++
 drivers/net/phy/phy_led_triggers.c | 136 +
 include/linux/phy.h|   7 ++
 include/linux/phy_led_triggers.h   |  51 ++
 7 files changed, 214 insertions(+)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 5078a0d..54c8eb8 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -15,6 +15,19 @@ if PHYLIB
 config SWPHY
bool
 
+config LED_TRIGGER_PHY
+   bool "Support LED triggers for tracking link state"
+   depends on LEDS_TRIGGERS
+   ---help---
+ Adds support for a set of LED trigger events per-PHY.  Link
+ state change will trigger the events, for consumption by an
+ LED class driver.  There are triggers for each link speed currently
+ supported by the phy, and are of the form:
+  ::
+
+ Where speed is in the form:
+   Mbps or Gbps
+
 comment "MDIO bus device drivers"
 
 config MDIO_BCM_IPROC
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index e58667d..86d12cd 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -2,6 +2,7 @@
 
 libphy-y   := phy.o phy_device.o mdio_bus.o mdio_device.o
 libphy-$(CONFIG_SWPHY) += swphy.o
+libphy-$(CONFIG_LED_TRIGGER_PHY)   += phy_led_triggers.o
 
 obj-$(CONFIG_PHYLIB)   += libphy.o
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 82ee233..ef0e3d0 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -931,6 +931,7 @@ EXPORT_SYMBOL(phy_start);
 static void phy_adjust_link(struct phy_device *phydev)
 {
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 }
 
 /**
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e977ba9..b41ebd5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -916,6 +917,8 @@ int phy_attach_direct(struct net_device *dev, struct 
phy_device *phydev,
else
phy_resume(phydev);
 
+   phy_led_triggers_register(phydev);
+
return err;
 
 error:
@@ -989,6 +992,8 @@ void phy_detach(struct phy_device *phydev)
}
}
 
+   phy_led_triggers_unregister(phydev);
+
/*
 * The phydev might go away on the put_device() below, so avoid
 * a use-after-free bug by reading the underlying bus first.
diff --git a/drivers/net/phy/phy_led_triggers.c 
b/drivers/net/phy/phy_led_triggers.c
new file mode 100644
index 000..cda600a
--- /dev/null
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -0,0 +1,136 @@
+/* Copyright (C) 2016 National Instruments Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+  * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+
+static struct phy_led_trigger *phy_speed_to_led_trigger(struct phy_device *phy,
+   unsigned int speed)
+{
+   unsigned int i;
+
+   for (i = 0; i < phy->phy_num_led_triggers; i++) {
+   if (phy->phy_led_triggers[i].speed == speed)
+   return >phy_led_triggers[i];
+   }
+   return NULL;
+

[PATCH v5 0/4] Add support for led triggers on phy link state change

[Zach Brown]

Fix skge driver that declared enum contants that conflicted with enum
constants in linux/leds.h

Create function that encapsulates actions taken during the adjust phy link step
of phy state changes.

Create function that provides list of speeds currently supported by the phy.

Add support for led triggers on phy link state changes by adding
a config option. When set the config option will create a set of led triggers
for each phy device. Users can use the led triggers to represent link state
changes on the phy.

v2:
 * New patch that creates phy_adjust_link function to encapsulate actions taken
   when adjusting phy link during phy state changes
 * led trigger speed strings changed to match existing phy speed strings
 * New function that maps speeds to led triggers
 * Replace magic constants with definitions when declaring trigger name
   buffer and number of triggers.
v3:
 * Changed LED_ON to LED_REG_ON in skge driver to avoid possible future
   conflict and improve consistency.
 * Dropped rtl8712 patch that was accepted separately.
v4:
 * tweaked commit message
v5
 * Changed commit message to explain relationship between the new triggers and
   leds driven by phys.
 * Added new patch that creates phy_supported_speeds function.
 * Moved phy_leds_triggers_register and phy_leds_triggers_unregister to
   phy_attach and phy_detach respectively. This change is so the
   phydev->supported field will be filled by the time the triggers are
   registered.
 * Changed hardcoded list of triggers to dynamic list determined by speeds
   return by phy_supported_speeds.

Zach Brown (4):
  skge: Rename LED_OFF and LED_ON in marvel skge driver to avoid
conflicts with leds namespace
  net: phy: Encapsulate actions performed during link state changes into
function phy_adjust_link
  net: phy: Create phy_supported_speeds function which lists speeds
currently supported by a phydevice
  net: phy: leds: add support for led triggers on phy link state change

 drivers/net/ethernet/marvell/skge.c |   6 +-
 drivers/net/ethernet/marvell/skge.h |   4 +-
 drivers/net/phy/Kconfig |  13 
 drivers/net/phy/Makefile|   1 +
 drivers/net/phy/phy.c   |  57 ---
 drivers/net/phy/phy_device.c|   5 ++
 drivers/net/phy/phy_led_triggers.c  | 136 
 include/linux/phy.h |  22 ++
 include/linux/phy_led_triggers.h|  51 ++
 9 files changed, 282 insertions(+), 13 deletions(-)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

-- 
2.7.4

[PATCH v5 1/4] skge: Rename LED_OFF and LED_ON in marvel skge driver to avoid conflicts with leds namespace

[Zach Brown]

Adding led support for phy causes namespace conflicts for some
phy drivers.

The marvel skge driver declared an enum for representing the states of
Link LED Register. The enum contained constant LED_OFF which conflicted
with declartation found in linux/leds.h.
LED_OFF changed to LED_REG_OFF
Also changed LED_ON to LED_REG_ON to avoid possible future conflict and
for consistency.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/ethernet/marvell/skge.c | 6 +++---
 drivers/net/ethernet/marvell/skge.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/skge.c 
b/drivers/net/ethernet/marvell/skge.c
index 7173836..783df01 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -1048,7 +1048,7 @@ static const char *skge_pause(enum pause_status status)
 static void skge_link_up(struct skge_port *skge)
 {
skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG),
-   LED_BLK_OFF|LED_SYNC_OFF|LED_ON);
+   LED_BLK_OFF|LED_SYNC_OFF|LED_REG_ON);
 
netif_carrier_on(skge->netdev);
netif_wake_queue(skge->netdev);
@@ -1062,7 +1062,7 @@ static void skge_link_up(struct skge_port *skge)
 
 static void skge_link_down(struct skge_port *skge)
 {
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
netif_carrier_off(skge->netdev);
netif_stop_queue(skge->netdev);
 
@@ -2668,7 +2668,7 @@ static int skge_down(struct net_device *dev)
if (hw->ports == 1)
free_irq(hw->pdev->irq, hw);
 
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
if (is_genesis(hw))
genesis_stop(skge);
else
diff --git a/drivers/net/ethernet/marvell/skge.h 
b/drivers/net/ethernet/marvell/skge.h
index a2eb341..3ea151f 100644
--- a/drivers/net/ethernet/marvell/skge.h
+++ b/drivers/net/ethernet/marvell/skge.h
@@ -662,8 +662,8 @@ enum {
LED_BLK_OFF = 1<<4, /* Link LED Blinking Off */
LED_SYNC_ON = 1<<3, /* Use Sync Wire to switch LED */
LED_SYNC_OFF= 1<<2, /* Disable Sync Wire Input */
-   LED_ON  = 1<<1, /* switch LED on */
-   LED_OFF = 1<<0, /* switch LED off */
+   LED_REG_ON  = 1<<1, /* switch LED on */
+   LED_REG_OFF = 1<<0, /* switch LED off */
 };
 
 /* Receive GMAC FIFO (YUKON) */
-- 
2.7.4

[PATCH v5 3/4] net: phy: Create phy_supported_speeds function which lists speeds currently supported by a phydevice

[Zach Brown]

phy_supported_speeds provides a means to get a list of all the speeds a
phy device currently supports.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/phy.c | 35 +++
 include/linux/phy.h   | 15 +++
 2 files changed, 50 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f5721db..82ee233 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -261,6 +261,41 @@ static inline unsigned int phy_find_valid(unsigned int 
idx, u32 features)
 }
 
 /**
+ * phy_supported_speeds - return all speeds currently supported by a phy device
+ * @phy: The phy device to return supported speeds of.
+ * @speeds: buffer to store supported speeds in.
+ * @size:   size of speeds buffer.
+ *
+ * Description: Returns the number of supported speeds, and fills the speeds
+ * buffer with the supported speeds. If speeds buffer is too small to contain
+ * all currently supported speeds, will return as many speeds as can fit.
+ */
+unsigned int phy_supported_speeds(struct phy_device *phy,
+ unsigned int *speeds,
+ unsigned int size)
+{
+   unsigned int count = 0;
+   unsigned int idx = 0;
+
+   while (idx < MAX_NUM_SETTINGS && count < size) {
+   idx = phy_find_valid(idx, phy->supported);
+
+   if (!(settings[idx].setting & phy->supported))
+   break;
+
+   /* Assumes settings are grouped by speed */
+   if ((count == 0) ||
+   (speeds[count - 1] != settings[idx].speed)) {
+   speeds[count] = settings[idx].speed;
+   count++;
+   }
+   idx++;
+   }
+
+   return count;
+}
+
+/**
  * phy_check_valid - check if there is a valid PHY setting which matches
  *  speed, duplex, and feature mask
  * @speed: speed to match
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e25f183..8761f30 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -85,6 +85,21 @@ typedef enum {
 } phy_interface_t;
 
 /**
+ * phy_supported_speeds - return all speeds currently supported by a phy device
+ * @phy: The phy device to return supported speeds of.
+ * @speeds: buffer to store supported speeds in.
+ * @size: size of speeds buffer.
+ *
+ * Description: Returns the number of supported speeds, and
+ * fills the speeds * buffer with the supported speeds. If speeds buffer is
+ * too small to contain * all currently supported speeds, will return as
+ * many speeds as can fit.
+ */
+unsigned int phy_supported_speeds(struct phy_device *phy,
+ unsigned int *speeds,
+ unsigned int size);
+
+/**
  * It maps 'enum phy_interface_t' found in include/linux/phy.h
  * into the device tree binding of 'phy-mode', so that Ethernet
  * device driver can get phy interface from device tree.
-- 
2.7.4

Re: [PATCH v4 3/3] net: phy: leds: add support for led triggers on phy link state change

[Zach Brown]

On Thu, Oct 13, 2016 at 10:46:34AM -0400, David Miller wrote:
> From: Zach Brown <zach.br...@ni.com>
> Date: Tue, 11 Oct 2016 15:26:20 -0500
> 
> > From: Josh Cartwright <josh.cartwri...@ni.com>
> > 
> > Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
> > create a set of led triggers for each instantiated PHY device.  There is
> > one LED trigger per link-speed, per-phy.
> > 
> > This allows for a user to configure their system to allow a set of LEDs
> > to represent link state changes on the phy.
> > 
> > Signed-off-by: Josh Cartwright <josh.cartwri...@ni.com>
> > Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com>
> > Signed-off-by: Zach Brown <zach.br...@ni.com>
>  ...
> > +   static const char * const name_suffix[] = {
> > +   "10Mbps",
> > +   "100Mbps",
> > +   "1Gbps",
> > +   "2.5Gbps",
> > +   "10Gbps",
> 
> This choice of both the array size and the speeds to support seems
> entirely arbitrary and is inappropriate for a generic driver of this
> kind.
> 
> This seems to be hard coding this to support the list of speeds
> supported by whatever driver you want to use with this new LED
> facility, and sorry that's not how we build nice generic pieces of
> infrastructure.
> 
> Thanks.

The speeds listed are the speeds found in the phy_speed_to_str function in 
phy.c.
They are also the speeds found in the struct phy_setting settings array,
which is commented with
"/* A mapping of all SUPPORTED settings to speed/duplex */"
We believed they represented the commonly supported speeds of phys.

Do you have suggestions on how to better handle the choice of the array size
and the speeds?

Thanks.

Re: [RFC v3 3/3] phy,leds: add support for led triggers on phy link state change

[Zach Brown]

On Mon, Oct 10, 2016 at 02:03:32AM -0700, Florian Fainelli wrote:
> > +
> > +#ifdef CONFIG_LED_TRIGGER_PHY
> > +
> > +#include 
> > +#include 
> > +
> > +#define PHY_LINK_LED_MAX_TRIGGERS  5
> > +#define PHY_LED_TRIGGER_SPEED_SUFFIX_SIZE  7
> > +#define PHY_MII_BUS_ID_SIZE(20 - 3)
> 
> This particular constant may be something worth moving to
> include/linux/phy.h eventually.
> -- 
> Florian

MII_BUS_ID_SIZE is defined in include/linux/phy.h but it's defined after
phy_led_triggers.h is included so phy_led_triggers.h doesn't have access.
I could move the definition of MII_BUS_ID_SIZE above the include, but that
seemed ugly. Do you have any suggestions?

Re: [PATCH v4 1/3] skge: Rename LED_OFF and LED_ON in marvel skge driver to avoid conflicts with leds namespace

[Zach Brown]

On Tue, Oct 11, 2016 at 02:14:07PM -0700, Stephen Hemminger wrote:
> On Tue, 11 Oct 2016 15:26:18 -0500
> Zach Brown <zach.br...@ni.com> wrote:
>
> > Adding led support for phy causes namespace conflicts for some
> > phy drivers.
> >
> > The marvel skge driver declared an enum for representing the states of
> > Link LED Register. The enum contained constant LED_OFF which conflicted
> > with declartation found in linux/leds.h.
> > LED_OFF changed to LED_REG_OFF
> > Also changed LED_ON to LED_REG_ON to avoid possible future conflict and
> > for consistency.
> >
> > Signed-off-by: Zach Brown <zach.br...@ni.com>
>
> Sure, that's fine but not sure why skge would be including linux/leds.h
> anyway.

It's pretty convoluted. Here's the chain of includes.
skge -> netdevice -> dsa -> phy -> phy_led_triggers -> leds

[PATCH v4 1/3] skge: Rename LED_OFF and LED_ON in marvel skge driver to avoid conflicts with leds namespace

[Zach Brown]

Adding led support for phy causes namespace conflicts for some
phy drivers.

The marvel skge driver declared an enum for representing the states of
Link LED Register. The enum contained constant LED_OFF which conflicted
with declartation found in linux/leds.h.
LED_OFF changed to LED_REG_OFF
Also changed LED_ON to LED_REG_ON to avoid possible future conflict and
for consistency.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/ethernet/marvell/skge.c | 6 +++---
 drivers/net/ethernet/marvell/skge.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/skge.c 
b/drivers/net/ethernet/marvell/skge.c
index 7173836..783df01 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -1048,7 +1048,7 @@ static const char *skge_pause(enum pause_status status)
 static void skge_link_up(struct skge_port *skge)
 {
skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG),
-   LED_BLK_OFF|LED_SYNC_OFF|LED_ON);
+   LED_BLK_OFF|LED_SYNC_OFF|LED_REG_ON);
 
netif_carrier_on(skge->netdev);
netif_wake_queue(skge->netdev);
@@ -1062,7 +1062,7 @@ static void skge_link_up(struct skge_port *skge)
 
 static void skge_link_down(struct skge_port *skge)
 {
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
netif_carrier_off(skge->netdev);
netif_stop_queue(skge->netdev);
 
@@ -2668,7 +2668,7 @@ static int skge_down(struct net_device *dev)
if (hw->ports == 1)
free_irq(hw->pdev->irq, hw);
 
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
if (is_genesis(hw))
genesis_stop(skge);
else
diff --git a/drivers/net/ethernet/marvell/skge.h 
b/drivers/net/ethernet/marvell/skge.h
index a2eb341..3ea151f 100644
--- a/drivers/net/ethernet/marvell/skge.h
+++ b/drivers/net/ethernet/marvell/skge.h
@@ -662,8 +662,8 @@ enum {
LED_BLK_OFF = 1<<4, /* Link LED Blinking Off */
LED_SYNC_ON = 1<<3, /* Use Sync Wire to switch LED */
LED_SYNC_OFF= 1<<2, /* Disable Sync Wire Input */
-   LED_ON  = 1<<1, /* switch LED on */
-   LED_OFF = 1<<0, /* switch LED off */
+   LED_REG_ON  = 1<<1, /* switch LED on */
+   LED_REG_OFF = 1<<0, /* switch LED off */
 };
 
 /* Receive GMAC FIFO (YUKON) */
-- 
2.7.4

[PATCH v4 0/3] Add support for led triggers on phy link state change

[Zach Brown]

Fix skge driver that declared enum contants that conflicted with enum
constants in linux/leds.h

Create function that encapsulates actions taken during the adjust phy link step
of phy state changes.

Add support for led triggers on phy link state changes by adding
a config option. When set the config option will create a set of led triggers
for each phy device. Users can use the led triggers to represent link state
changes on the phy.

v2:
 * New patch that creates phy_adjust_link function to encapsulate actions taken
   when adjusting phy link during phy state changes
 * led trigger speed strings changed to match existing phy speed strings
 * New function that maps speeds to led triggers
 * Replace magic constants with definitions when declaring trigger name
   buffer and number of triggers.
v3:
 * Changed LED_ON to LED_REG_ON in skge driver to avoid possible future
   conflict and improve consistency.
 * Dropped rtl8712 patch that was accepted separately.
v4:
 * tweaked commit message

Josh Cartwright (1):
  net: phy: leds: add support for led triggers on phy link state change

Zach Brown (2):
  skge: Rename LED_OFF and LED_ON in marvel skge driver to avoid
conflicts with leds namespace
  net: phy: Encapsulate actions performed during link state changes into
function phy_adjust_link

 drivers/net/ethernet/marvell/skge.c |   6 +-
 drivers/net/ethernet/marvell/skge.h |   4 +-
 drivers/net/phy/Kconfig |  13 +++-
 drivers/net/phy/Makefile|   1 +
 drivers/net/phy/phy.c   |  22 ---
 drivers/net/phy/phy_device.c|   4 ++
 drivers/net/phy/phy_led_triggers.c  | 121 
 include/linux/phy.h |   9 +++
 include/linux/phy_led_triggers.h|  52 
 9 files changed, 218 insertions(+), 14 deletions(-)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

-- 
2.7.4

[PATCH v4 3/3] net: phy: leds: add support for led triggers on phy link state change

[Zach Brown]

From: Josh Cartwright <josh.cartwri...@ni.com>

Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
create a set of led triggers for each instantiated PHY device.  There is
one LED trigger per link-speed, per-phy.

This allows for a user to configure their system to allow a set of LEDs
to represent link state changes on the phy.

Signed-off-by: Josh Cartwright <josh.cartwri...@ni.com>
Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com>
Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/Kconfig|  13 +++-
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/phy.c  |   1 +
 drivers/net/phy/phy_device.c   |   4 ++
 drivers/net/phy/phy_led_triggers.c | 121 +
 include/linux/phy.h|   9 +++
 include/linux/phy_led_triggers.h   |  52 
 7 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 5078a0d..4fd912d 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -25,6 +25,18 @@ config MDIO_BCM_IPROC
  This module provides a driver for the MDIO busses found in the
  Broadcom iProc SoC's.
 
+config LED_TRIGGER_PHY
+   bool "Support LED triggers for tracking link state"
+   depends on LEDS_TRIGGERS
+   ---help---
+ Adds support for a set of LED trigger events per-PHY.  Link
+ state change will trigger the events, for consumption by an
+ LED class driver.  There are triggers for each link speed,
+ and are of the form:
+  ::
+
+ Where speed is one of: 10Mbps, 100Mbps, 1Gbps, 2.5Gbps, or 10Gbps.
+
 config MDIO_BCM_UNIMAC
tristate "Broadcom UniMAC MDIO bus controller"
depends on HAS_IOMEM
@@ -40,7 +52,6 @@ config MDIO_BITBANG
  This module implements the MDIO bus protocol in software,
  for use by low level drivers that export the ability to
  drive the relevant pins.
-
  If in doubt, say N.
 
 config MDIO_BUS_MUX
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index e58667d..86d12cd 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -2,6 +2,7 @@
 
 libphy-y   := phy.o phy_device.o mdio_bus.o mdio_device.o
 libphy-$(CONFIG_SWPHY) += swphy.o
+libphy-$(CONFIG_LED_TRIGGER_PHY)   += phy_led_triggers.o
 
 obj-$(CONFIG_PHYLIB)   += libphy.o
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f5721db..e5f9fee7 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -896,6 +896,7 @@ EXPORT_SYMBOL(phy_start);
 static void phy_adjust_link(struct phy_device *phydev)
 {
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 }
 
 /**
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e977ba9..4671c13 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -57,6 +58,7 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
 
 static void phy_device_release(struct device *dev)
 {
+   phy_led_triggers_unregister(to_phy_device(dev));
kfree(to_phy_device(dev));
 }
 
@@ -345,6 +347,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, 
int addr, int phy_id,
 
dev->state = PHY_DOWN;
 
+   phy_led_triggers_register(dev);
+
mutex_init(>lock);
INIT_DELAYED_WORK(>state_queue, phy_state_machine);
INIT_WORK(>phy_queue, phy_change);
diff --git a/drivers/net/phy/phy_led_triggers.c 
b/drivers/net/phy/phy_led_triggers.c
new file mode 100644
index 000..32326d7
--- /dev/null
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -0,0 +1,121 @@
+/* Copyright (C) 2016 National Instruments Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+  * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+
+static struct phy_led_trigger *phy_speed_to_led_trigger(struct phy_device *phy,
+   unsigned int speed)
+{
+   switch (speed) {
+   case SPEED_10:
+   return >phy_led_trigger[0];
+   case SPEED_100:
+   return >phy_led_trigger[1];
+   case SPEED_1000:
+   return 

[PATCH v4 2/3] net: phy: Encapsulate actions performed during link state changes into function phy_adjust_link

[Zach Brown]

During phy state machine state transitions some set of actions should
occur whenever the link state changes. These actions should be
encapsulated into a single function

This patch adds the phy_adjust_link function, which is called whenever
phydev->adjust_link would have been called before. Actions that should
occur whenever the phy link is adjusted can now be added to the
phy_adjust_link function.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/phy.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c6f6683..f5721db 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -893,6 +893,11 @@ void phy_start(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_start);
 
+static void phy_adjust_link(struct phy_device *phydev)
+{
+   phydev->adjust_link(phydev->attached_dev);
+}
+
 /**
  * phy_state_machine - Handle the state machine
  * @work: work_struct that describes the work to be done
@@ -935,7 +940,7 @@ void phy_state_machine(struct work_struct *work)
if (!phydev->link) {
phydev->state = PHY_NOLINK;
netif_carrier_off(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
break;
}
 
@@ -948,7 +953,7 @@ void phy_state_machine(struct work_struct *work)
if (err > 0) {
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
 
} else if (0 == phydev->link_timeout--)
needs_aneg = true;
@@ -975,7 +980,7 @@ void phy_state_machine(struct work_struct *work)
}
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
}
break;
case PHY_FORCING:
@@ -991,7 +996,7 @@ void phy_state_machine(struct work_struct *work)
needs_aneg = true;
}
 
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
break;
case PHY_RUNNING:
/* Only register a CHANGE if we are polling and link changed
@@ -1020,7 +1025,7 @@ void phy_state_machine(struct work_struct *work)
netif_carrier_off(phydev->attached_dev);
}
 
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
 
if (phy_interrupt_is_valid(phydev))
err = phy_config_interrupt(phydev,
@@ -1030,7 +1035,7 @@ void phy_state_machine(struct work_struct *work)
if (phydev->link) {
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
do_suspend = true;
}
break;
@@ -1054,7 +1059,7 @@ void phy_state_machine(struct work_struct *work)
} else  {
phydev->state = PHY_NOLINK;
}
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
} else {
phydev->state = PHY_AN;
phydev->link_timeout = PHY_AN_TIMEOUT;
@@ -1070,7 +1075,7 @@ void phy_state_machine(struct work_struct *work)
} else  {
phydev->state = PHY_NOLINK;
}
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
}
break;
}
-- 
2.7.4

[RFC v3 2/3] phy: Encapsulate actions performed during link state changes into function phy_adjust_link

[Zach Brown]

During phy state machine state transitions some set of actions should
occur whenever the link state changes. These actions should be
encapsulated into a single function

This patch adds the phy_adjust_link function, which is called whenever
phydev->adjust_link would have been called before. Actions that should
occur whenever the phy link is adjusted can now be added to the
phy_adjust_link function.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/phy.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c6f6683..f5721db 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -893,6 +893,11 @@ void phy_start(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_start);
 
+static void phy_adjust_link(struct phy_device *phydev)
+{
+   phydev->adjust_link(phydev->attached_dev);
+}
+
 /**
  * phy_state_machine - Handle the state machine
  * @work: work_struct that describes the work to be done
@@ -935,7 +940,7 @@ void phy_state_machine(struct work_struct *work)
if (!phydev->link) {
phydev->state = PHY_NOLINK;
netif_carrier_off(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
break;
}
 
@@ -948,7 +953,7 @@ void phy_state_machine(struct work_struct *work)
if (err > 0) {
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
 
} else if (0 == phydev->link_timeout--)
needs_aneg = true;
@@ -975,7 +980,7 @@ void phy_state_machine(struct work_struct *work)
}
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
}
break;
case PHY_FORCING:
@@ -991,7 +996,7 @@ void phy_state_machine(struct work_struct *work)
needs_aneg = true;
}
 
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
break;
case PHY_RUNNING:
/* Only register a CHANGE if we are polling and link changed
@@ -1020,7 +1025,7 @@ void phy_state_machine(struct work_struct *work)
netif_carrier_off(phydev->attached_dev);
}
 
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
 
if (phy_interrupt_is_valid(phydev))
err = phy_config_interrupt(phydev,
@@ -1030,7 +1035,7 @@ void phy_state_machine(struct work_struct *work)
if (phydev->link) {
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
do_suspend = true;
}
break;
@@ -1054,7 +1059,7 @@ void phy_state_machine(struct work_struct *work)
} else  {
phydev->state = PHY_NOLINK;
}
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
} else {
phydev->state = PHY_AN;
phydev->link_timeout = PHY_AN_TIMEOUT;
@@ -1070,7 +1075,7 @@ void phy_state_machine(struct work_struct *work)
} else  {
phydev->state = PHY_NOLINK;
}
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
}
break;
}
-- 
2.7.4

[RFC v3 1/3] skge: Rename LED_OFF and LED_ON in marvel skge driver to avoid conflicts with leds namespace

[Zach Brown]

Adding led support for phy causes namespace conflicts for some
phy drivers.

The marvel skge driver declared an enum for representing the states of
Link LED Register. The enum contained constant LED_OFF which conflicted
with declartation found in linux/leds.h.
LED_OFF changed to LED_REG_OFF
Also changed LED_ON to LED_REG_ON to avoid possible future conflict and
for consistency.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/ethernet/marvell/skge.c | 6 +++---
 drivers/net/ethernet/marvell/skge.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/skge.c 
b/drivers/net/ethernet/marvell/skge.c
index 7173836..783df01 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -1048,7 +1048,7 @@ static const char *skge_pause(enum pause_status status)
 static void skge_link_up(struct skge_port *skge)
 {
skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG),
-   LED_BLK_OFF|LED_SYNC_OFF|LED_ON);
+   LED_BLK_OFF|LED_SYNC_OFF|LED_REG_ON);
 
netif_carrier_on(skge->netdev);
netif_wake_queue(skge->netdev);
@@ -1062,7 +1062,7 @@ static void skge_link_up(struct skge_port *skge)
 
 static void skge_link_down(struct skge_port *skge)
 {
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
netif_carrier_off(skge->netdev);
netif_stop_queue(skge->netdev);
 
@@ -2668,7 +2668,7 @@ static int skge_down(struct net_device *dev)
if (hw->ports == 1)
free_irq(hw->pdev->irq, hw);
 
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
if (is_genesis(hw))
genesis_stop(skge);
else
diff --git a/drivers/net/ethernet/marvell/skge.h 
b/drivers/net/ethernet/marvell/skge.h
index a2eb341..3ea151f 100644
--- a/drivers/net/ethernet/marvell/skge.h
+++ b/drivers/net/ethernet/marvell/skge.h
@@ -662,8 +662,8 @@ enum {
LED_BLK_OFF = 1<<4, /* Link LED Blinking Off */
LED_SYNC_ON = 1<<3, /* Use Sync Wire to switch LED */
LED_SYNC_OFF= 1<<2, /* Disable Sync Wire Input */
-   LED_ON  = 1<<1, /* switch LED on */
-   LED_OFF = 1<<0, /* switch LED off */
+   LED_REG_ON  = 1<<1, /* switch LED on */
+   LED_REG_OFF = 1<<0, /* switch LED off */
 };
 
 /* Receive GMAC FIFO (YUKON) */
-- 
2.7.4

[PATCH 0/3] Add support for led triggers on phy link state change

[Zach Brown]

Fix skge driver that declared enum contants that conflicted with enum
constants in linux/leds.h

Create function that encapsulates actions taken during the adjust phy link step
of phy state changes.

Add support for led triggers on phy link state changes by adding
a config option. When set the config option will create a set of led triggers
for each phy device. Users can use the led triggers to represent link state
changes on the phy.

v2:
 * New patch that creates phy_adjust_link function to encapsulate actions taken
   when adjusting phy link during phy state changes
 * led trigger speed strings changed to match existing phy speed strings
 * New function that maps speeds to led triggers
 * Replace magic constants with definitions when declaring trigger name
   buffer and number of triggers.
v3:
 * Changed LED_ON to LED_REG_ON in skge driver to avoid possible future
   conflict and improve consistency.
 * Dropped rtl8712 patch that was accepted separately.

Josh Cartwright (1):
  phy,leds: add support for led triggers on phy link state change

Zach Brown (2):
  skge: Change LED_OFF to LED_REG_OFF in marvel skge driver to avoid
conflicts with leds namespace
  phy: Encapsulate actions performed during link state changes into
function phy_adjust_link

 drivers/net/ethernet/marvell/skge.c |   6 +-
 drivers/net/ethernet/marvell/skge.h |   4 +-
 drivers/net/phy/Kconfig |  13 +++-
 drivers/net/phy/Makefile|   1 +
 drivers/net/phy/phy.c   |  22 ---
 drivers/net/phy/phy_device.c|   4 ++
 drivers/net/phy/phy_led_triggers.c  | 121 
 include/linux/phy.h |   9 +++
 include/linux/phy_led_triggers.h|  52 
 9 files changed, 218 insertions(+), 14 deletions(-)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

--
2.7.4

[RFC v3 3/3] phy,leds: add support for led triggers on phy link state change

[Zach Brown]

From: Josh Cartwright <josh.cartwri...@ni.com>

Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
create a set of led triggers for each instantiated PHY device.  There is
one LED trigger per link-speed, per-phy.

This allows for a user to configure their system to allow a set of LEDs
to represent link state changes on the phy.

Signed-off-by: Josh Cartwright <josh.cartwri...@ni.com>
Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com>
Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/Kconfig|  13 +++-
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/phy.c  |   1 +
 drivers/net/phy/phy_device.c   |   4 ++
 drivers/net/phy/phy_led_triggers.c | 121 +
 include/linux/phy.h|   9 +++
 include/linux/phy_led_triggers.h   |  52 
 7 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 5078a0d..4fd912d 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -25,6 +25,18 @@ config MDIO_BCM_IPROC
  This module provides a driver for the MDIO busses found in the
  Broadcom iProc SoC's.
 
+config LED_TRIGGER_PHY
+   bool "Support LED triggers for tracking link state"
+   depends on LEDS_TRIGGERS
+   ---help---
+ Adds support for a set of LED trigger events per-PHY.  Link
+ state change will trigger the events, for consumption by an
+ LED class driver.  There are triggers for each link speed,
+ and are of the form:
+  ::
+
+ Where speed is one of: 10Mbps, 100Mbps, 1Gbps, 2.5Gbps, or 10Gbps.
+
 config MDIO_BCM_UNIMAC
tristate "Broadcom UniMAC MDIO bus controller"
depends on HAS_IOMEM
@@ -40,7 +52,6 @@ config MDIO_BITBANG
  This module implements the MDIO bus protocol in software,
  for use by low level drivers that export the ability to
  drive the relevant pins.
-
  If in doubt, say N.
 
 config MDIO_BUS_MUX
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index e58667d..86d12cd 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -2,6 +2,7 @@
 
 libphy-y   := phy.o phy_device.o mdio_bus.o mdio_device.o
 libphy-$(CONFIG_SWPHY) += swphy.o
+libphy-$(CONFIG_LED_TRIGGER_PHY)   += phy_led_triggers.o
 
 obj-$(CONFIG_PHYLIB)   += libphy.o
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f5721db..e5f9fee7 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -896,6 +896,7 @@ EXPORT_SYMBOL(phy_start);
 static void phy_adjust_link(struct phy_device *phydev)
 {
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 }
 
 /**
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e977ba9..4671c13 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -57,6 +58,7 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
 
 static void phy_device_release(struct device *dev)
 {
+   phy_led_triggers_unregister(to_phy_device(dev));
kfree(to_phy_device(dev));
 }
 
@@ -345,6 +347,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, 
int addr, int phy_id,
 
dev->state = PHY_DOWN;
 
+   phy_led_triggers_register(dev);
+
mutex_init(>lock);
INIT_DELAYED_WORK(>state_queue, phy_state_machine);
INIT_WORK(>phy_queue, phy_change);
diff --git a/drivers/net/phy/phy_led_triggers.c 
b/drivers/net/phy/phy_led_triggers.c
new file mode 100644
index 000..32326d7
--- /dev/null
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -0,0 +1,121 @@
+/* Copyright (C) 2016 National Instruments Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+  * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+
+static struct phy_led_trigger *phy_speed_to_led_trigger(struct phy_device *phy,
+   unsigned int speed)
+{
+   switch (speed) {
+   case SPEED_10:
+   return >phy_led_trigger[0];
+   case SPEED_100:
+   return >phy_led_trigger[1];
+   case SPEED_1000:
+   return 

[RFC v2 1/4] skge: Change LED_OFF to LED_REG_OFF in marvel skge driver to avoid conflicts with leds namespace

[Zach Brown]

Adding led support for phy causes namespace conflicts for some
phy drivers.

The marvel skge driver declared an enum for representing the states of
Link LED Register. The enum contained constant LED_OFF which conflicted
with declartation found in linux/leds.h.
LED_OFF changed to LED_REG_OFF

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/ethernet/marvell/skge.c | 4 ++--
 drivers/net/ethernet/marvell/skge.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/skge.c 
b/drivers/net/ethernet/marvell/skge.c
index 7173836..ff5a087 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -1062,7 +1062,7 @@ static void skge_link_up(struct skge_port *skge)
 
 static void skge_link_down(struct skge_port *skge)
 {
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
netif_carrier_off(skge->netdev);
netif_stop_queue(skge->netdev);
 
@@ -2668,7 +2668,7 @@ static int skge_down(struct net_device *dev)
if (hw->ports == 1)
free_irq(hw->pdev->irq, hw);
 
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
if (is_genesis(hw))
genesis_stop(skge);
else
diff --git a/drivers/net/ethernet/marvell/skge.h 
b/drivers/net/ethernet/marvell/skge.h
index a2eb341..ec054d3 100644
--- a/drivers/net/ethernet/marvell/skge.h
+++ b/drivers/net/ethernet/marvell/skge.h
@@ -663,7 +663,7 @@ enum {
LED_SYNC_ON = 1<<3, /* Use Sync Wire to switch LED */
LED_SYNC_OFF= 1<<2, /* Disable Sync Wire Input */
LED_ON  = 1<<1, /* switch LED on */
-   LED_OFF = 1<<0, /* switch LED off */
+   LED_REG_OFF = 1<<0, /* switch LED off */
 };
 
 /* Receive GMAC FIFO (YUKON) */
-- 
2.7.4

[RFC v2 2/4] staging: rtl8712: Change _LED_STATE enum in rtl871x driver to avoid conflicts with LED namespace

[Zach Brown]

Adding led support for phy causes namespace conflicts for some
phy drivers.

The rtl871 driver declared an enum for representing LED states. The enum
contains constant LED_OFF which conflicted with declaration found in
linux/leds.h. LED_OFF changed to LED_STATE_OFF
In order to avoid a possible future collision LED_ON was changed to
LED_STATE_ON as well.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/staging/rtl8712/rtl8712_led.c | 388 +-
 1 file changed, 194 insertions(+), 194 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl8712_led.c 
b/drivers/staging/rtl8712/rtl8712_led.c
index 9055827..8a524ac 100644
--- a/drivers/staging/rtl8712/rtl8712_led.c
+++ b/drivers/staging/rtl8712/rtl8712_led.c
@@ -51,8 +51,8 @@
  */
 enum _LED_STATE_871x {
LED_UNKNOWN = 0,
-   LED_ON = 1,
-   LED_OFF = 2,
+   LED_STATE_ON = 1,
+   LED_STATE_OFF = 2,
LED_BLINK_NORMAL = 3,
LED_BLINK_SLOWLY = 4,
LED_POWER_ON_BLINK = 5,
@@ -92,7 +92,7 @@ static void InitLed871x(struct _adapter *padapter, struct 
LED_871x *pLed,
nic = padapter->pnetdev;
pLed->padapter = padapter;
pLed->LedPin = LedPin;
-   pLed->CurrLedState = LED_OFF;
+   pLed->CurrLedState = LED_STATE_OFF;
pLed->bLedOn = false;
pLed->bLedBlinkInProgress = false;
pLed->BlinkTimes = 0;
@@ -208,7 +208,7 @@ static void SwLedBlink(struct LED_871x *pLed)
u8 bStopBlinking = false;
 
/* Change LED according to BlinkingLedState specified. */
-   if (pLed->BlinkingLedState == LED_ON)
+   if (pLed->BlinkingLedState == LED_STATE_ON)
SwLedOn(padapter, pLed);
else
SwLedOff(padapter, pLed);
@@ -248,10 +248,10 @@ static void SwLedBlink(struct LED_871x *pLed)
pLed->bLedBlinkInProgress = false;
} else {
/* Assign LED state to toggle. */
-   if (pLed->BlinkingLedState == LED_ON)
-   pLed->BlinkingLedState = LED_OFF;
+   if (pLed->BlinkingLedState == LED_STATE_ON)
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
-   pLed->BlinkingLedState = LED_ON;
+   pLed->BlinkingLedState = LED_STATE_ON;
 
/* Schedule a timer to toggle LED state. */
switch (pLed->CurrLedState) {
@@ -288,7 +288,7 @@ static void SwLedBlink1(struct LED_871x *pLed)
if (peeprompriv->CustomerID == RT_CID_819x_CAMEO)
pLed = &(ledpriv->SwLed1);
/* Change LED according to BlinkingLedState specified. */
-   if (pLed->BlinkingLedState == LED_ON)
+   if (pLed->BlinkingLedState == LED_STATE_ON)
SwLedOn(padapter, pLed);
else
SwLedOff(padapter, pLed);
@@ -312,17 +312,17 @@ static void SwLedBlink1(struct LED_871x *pLed)
switch (pLed->CurrLedState) {
case LED_BLINK_SLOWLY:
if (pLed->bLedOn)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
-   pLed->BlinkingLedState = LED_ON;
+   pLed->BlinkingLedState = LED_STATE_ON;
mod_timer(>BlinkTimer, jiffies +
  msecs_to_jiffies(LED_BLINK_NO_LINK_INTERVAL_ALPHA));
break;
case LED_BLINK_NORMAL:
if (pLed->bLedOn)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
-   pLed->BlinkingLedState = LED_ON;
+   pLed->BlinkingLedState = LED_STATE_ON;
mod_timer(>BlinkTimer, jiffies +
  msecs_to_jiffies(LED_BLINK_LINK_INTERVAL_ALPHA));
break;
@@ -335,27 +335,27 @@ static void SwLedBlink1(struct LED_871x *pLed)
pLed->bLedLinkBlinkInProgress = true;
pLed->CurrLedState = LED_BLINK_NORMAL;
if (pLed->bLedOn)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
-   pLed->BlinkingLedState = LED_ON;
+   pLed->BlinkingLedState = LED_STATE_ON;
mod_timer(>BlinkTimer, jiffies +
  
msecs_to_jiffies(LED_BLINK_LINK_INTERVAL_ALPHA));
} else if (!check_fwstate(pmlmepriv, _FW_LINKED)) {
pLed->bLedNoLinkBlinkInProgress = true;
   

[RFC v2 0/4] Add support for led triggers on phy link state change

[Zach Brown]

Fix two net drivers that declared enum constants that conflict with enum
constants in linux/leds.h

Create function that encapsulates actions taken during the adjust phy link step
of phy state changes.

Add support for led triggers on phy link state changes by adding
a config option. When set the config option will create a set of led triggers
for each phy device. Users can use the led triggers to represent link state
changes on the phy.

v2:
 * New patch that creates phy_adjust_link function to encapsulate actions taken
   when adjusting phy link during phy state changes
 * led trigger speed strings changed to match existing phy speed strings
 * New function that maps speeds to led triggers
 * Replace magic constants with definitions when declaring trigger name buffer
   and number of triggers.

Josh Cartwright (1):
  phy,leds: add support for led triggers on phy link state change

Zach Brown (3):
  skge: Change LED_OFF to LED_REG_OFF in marvel skge driver to avoid
conflicts with leds namespace
  staging: rtl8712: Change _LED_STATE enum in rtl871x driver to avoid
conflicts with LED namespace
  phy: Encapsulate actions performed during link state changes into
function phy_adjust_link

 drivers/net/ethernet/marvell/skge.c   |   4 +-
 drivers/net/ethernet/marvell/skge.h   |   2 +-
 drivers/net/phy/Kconfig   |  13 +-
 drivers/net/phy/Makefile  |   1 +
 drivers/net/phy/phy.c |  22 +-
 drivers/net/phy/phy_device.c  |   4 +
 drivers/net/phy/phy_led_triggers.c| 121 +++
 drivers/staging/rtl8712/rtl8712_led.c | 388 +-
 include/linux/phy.h   |   9 +
 include/linux/phy_led_triggers.h  |  52 +
 10 files changed, 410 insertions(+), 206 deletions(-)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

--
2.7.4

[RFC v2 4/4] phy,leds: add support for led triggers on phy link state change

[Zach Brown]

From: Josh Cartwright <josh.cartwri...@ni.com>

Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
create a set of led triggers for each instantiated PHY device.  There is
one LED trigger per link-speed, per-phy.

This allows for a user to configure their system to allow a set of LEDs
to represent link state changes on the phy.

Signed-off-by: Josh Cartwright <josh.cartwri...@ni.com>
Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com>
Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/Kconfig|  13 +++-
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/phy.c  |   1 +
 drivers/net/phy/phy_device.c   |   4 ++
 drivers/net/phy/phy_led_triggers.c | 121 +
 include/linux/phy.h|   9 +++
 include/linux/phy_led_triggers.h   |  52 
 7 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 1c3e07c..f233625 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -25,6 +25,18 @@ config MDIO_BCM_IPROC
  This module provides a driver for the MDIO busses found in the
  Broadcom iProc SoC's.
 
+config LED_TRIGGER_PHY
+   bool "Support LED triggers for tracking link state"
+   depends on LEDS_TRIGGERS
+   ---help---
+ Adds support for a set of LED trigger events per-PHY.  Link
+ state change will trigger the events, for consumption by an
+ LED class driver.  There are triggers for each link speed,
+ and are of the form:
+  ::
+
+ Where speed is one of: 10Mbps, 100Mbps, 1Gbps, 2.5Gbps, or 10Gbps.
+
 config MDIO_BCM_UNIMAC
tristate "Broadcom UniMAC MDIO bus controller"
depends on HAS_IOMEM
@@ -40,7 +52,6 @@ config MDIO_BITBANG
  This module implements the MDIO bus protocol in software,
  for use by low level drivers that export the ability to
  drive the relevant pins.
-
  If in doubt, say N.
 
 config MDIO_BUS_MUX
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index e58667d..86d12cd 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -2,6 +2,7 @@
 
 libphy-y   := phy.o phy_device.o mdio_bus.o mdio_device.o
 libphy-$(CONFIG_SWPHY) += swphy.o
+libphy-$(CONFIG_LED_TRIGGER_PHY)   += phy_led_triggers.o
 
 obj-$(CONFIG_PHYLIB)   += libphy.o
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f5721db..e5f9fee7 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -896,6 +896,7 @@ EXPORT_SYMBOL(phy_start);
 static void phy_adjust_link(struct phy_device *phydev)
 {
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 }
 
 /**
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e977ba9..4671c13 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -57,6 +58,7 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
 
 static void phy_device_release(struct device *dev)
 {
+   phy_led_triggers_unregister(to_phy_device(dev));
kfree(to_phy_device(dev));
 }
 
@@ -345,6 +347,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, 
int addr, int phy_id,
 
dev->state = PHY_DOWN;
 
+   phy_led_triggers_register(dev);
+
mutex_init(>lock);
INIT_DELAYED_WORK(>state_queue, phy_state_machine);
INIT_WORK(>phy_queue, phy_change);
diff --git a/drivers/net/phy/phy_led_triggers.c 
b/drivers/net/phy/phy_led_triggers.c
new file mode 100644
index 000..32326d7
--- /dev/null
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -0,0 +1,121 @@
+/* Copyright (C) 2016 National Instruments Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+  * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+
+static struct phy_led_trigger *phy_speed_to_led_trigger(struct phy_device *phy,
+   unsigned int speed)
+{
+   switch (speed) {
+   case SPEED_10:
+   return >phy_led_trigger[0];
+   case SPEED_100:
+   return >phy_led_trigger[1];
+   case SPEED_1000:
+   return 

[RFC v2 3/4] phy: Encapsulate actions performed during link state changes into function phy_adjust_link

[Zach Brown]

During phy state machine state transitions some set of actions should
occur whenever the link state changes. These actions should be
encapsulated into a single function.

This patch adds the phy_adjust_link function, which is called whenever
phydev->adjust_link would have been called before. Actions that should
occur whenever the phy link is adjusted can now be added to the
phy_adjust_link function.

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/phy.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c6f6683..f5721db 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -893,6 +893,11 @@ void phy_start(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_start);
 
+static void phy_adjust_link(struct phy_device *phydev)
+{
+   phydev->adjust_link(phydev->attached_dev);
+}
+
 /**
  * phy_state_machine - Handle the state machine
  * @work: work_struct that describes the work to be done
@@ -935,7 +940,7 @@ void phy_state_machine(struct work_struct *work)
if (!phydev->link) {
phydev->state = PHY_NOLINK;
netif_carrier_off(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
break;
}
 
@@ -948,7 +953,7 @@ void phy_state_machine(struct work_struct *work)
if (err > 0) {
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
 
} else if (0 == phydev->link_timeout--)
needs_aneg = true;
@@ -975,7 +980,7 @@ void phy_state_machine(struct work_struct *work)
}
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
}
break;
case PHY_FORCING:
@@ -991,7 +996,7 @@ void phy_state_machine(struct work_struct *work)
needs_aneg = true;
}
 
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
break;
case PHY_RUNNING:
/* Only register a CHANGE if we are polling and link changed
@@ -1020,7 +1025,7 @@ void phy_state_machine(struct work_struct *work)
netif_carrier_off(phydev->attached_dev);
}
 
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
 
if (phy_interrupt_is_valid(phydev))
err = phy_config_interrupt(phydev,
@@ -1030,7 +1035,7 @@ void phy_state_machine(struct work_struct *work)
if (phydev->link) {
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
do_suspend = true;
}
break;
@@ -1054,7 +1059,7 @@ void phy_state_machine(struct work_struct *work)
} else  {
phydev->state = PHY_NOLINK;
}
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
} else {
phydev->state = PHY_AN;
phydev->link_timeout = PHY_AN_TIMEOUT;
@@ -1070,7 +1075,7 @@ void phy_state_machine(struct work_struct *work)
} else  {
phydev->state = PHY_NOLINK;
}
-   phydev->adjust_link(phydev->attached_dev);
+   phy_adjust_link(phydev);
}
break;
}
-- 
2.7.4

Re: [RFC 3/3] phy,leds: add support for led triggers on phy link state change

[Zach Brown]

On Wed, Sep 14, 2016 at 04:16:15PM -0700, Florian Fainelli wrote:
> On 09/14/2016 02:55 PM, Zach Brown wrote:
> > From: Josh Cartwright <josh.cartwri...@ni.com>
> > 
> > Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
> > create a set of led triggers for each instantiated PHY device.  There is
> > one LED trigger per link-speed, per-phy.
> > 
> > This allows for a user to configure their system to allow a set of LEDs
> > to represent link state changes on the phy.
> 
> The part that seems to be missing from this changeset (not an issue) is
> how you can "accelerate" the triggers, or rather make sure that the
> trigger configuration potentially calls back into the PHY driver when
> the requested trigger is actually supported by the on-PHY LEDs.
> 
> Other than that, just minor nits here and there.
> 
> > 
> > Signed-off-by: Josh Cartwright <josh.cartwri...@ni.com>
> > Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com>
> > Signed-off-by: Zach Brown <zach.br...@ni.com>
> > ---
> 
> > +config LED_TRIGGER_PHY
> > +   bool "Support LED triggers for tracking link state"
> > +   depends on LEDS_TRIGGERS
> > +   ---help---
> > + Adds support for a set of LED trigger events per-PHY.  Link
> > + state change will trigger the events, for consumption by an
> > + LED class driver.  There are triggers for each link speed,
> > + and are of the form:
> > +  ::
> > +
> > + Where speed is one of: 10Mb, 100Mb, Gb, 2.5Gb, or 10GbE.
> 
> I would do s/10GbE/10Gb/ just to be consistent with the other speeds, or
> maybe, to avoid too much duplicationg of how we represent speeds, use
> the same set of strings that drivers/net/phy/phy.c::phy_speed_to_str uses.
> 
> 
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index c6f6683..3345767 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -936,6 +936,7 @@ void phy_state_machine(struct work_struct *work)
> > phydev->state = PHY_NOLINK;
> > netif_carrier_off(phydev->attached_dev);
> > phydev->adjust_link(phydev->attached_dev);
> > +   phy_led_trigger_change_speed(phydev);
> 
> Since we have essentially two actions to take when performing link state
> changes:
> 
> - call the adjust_link callback
> - call into the LED trigger
> 
> we might consider encapsulating this into a function that could be named
> phy_adjust_link() and does both of these. That would be a preliminary
> patch to this this one.
> 
> >  
> > @@ -345,6 +347,8 @@ struct phy_device *phy_device_create(struct mii_bus 
> > *bus, int addr, int phy_id,
> >  
> > dev->state = PHY_DOWN;
> >  
> > +   phy_led_triggers_register(dev);
> > +
> > mutex_init(>lock);
> > INIT_DELAYED_WORK(>state_queue, phy_state_machine);
> > INIT_WORK(>phy_queue, phy_change);
> 
> Humm, should it be before the PHY state machine workqueue creation or
> after? Just wondering if there is a remote chance we could get an user
> to immediately program a trigger and this could create a problem, maybe
> not so much.
> 
I'm not sure, but I don't think there would be an issue since the interaction
between the triggers and the phy system only goes in one direction. The
triggers don't influence the phy system to my understanding.

> > diff --git a/drivers/net/phy/phy_led_triggers.c 
> > b/drivers/net/phy/phy_led_triggers.c
> > new file mode 100644
> > index 000..6c40414
> > --- /dev/null
> > +++ b/drivers/net/phy/phy_led_triggers.c
> > @@ -0,0 +1,109 @@
> > +/* Copyright (C) 2016 National Instruments Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +#include 
> > +#include 
> > +
> > +void phy_led_trigger_change_speed(struct phy_device *phy)
> > +{
> > +   struct phy_led_trigger *plt;
> > +
> > +   if (!phy->link) {
> > +   if (phy->last_tri

[RFC 1/3] skge: Change LED_OFF to LED_REG_OFF in marvel skge driver to avoid conflicts with leds namespace

[Zach Brown]

Adding led support for phy causes namespace conflicts for some
phy drivers.

The marvel skge driver declared an enum for representing the states of
Link LED Register. The enum contained constant LED_OFF which conflicted
with declartation found in linux/leds.h.
LED_OFF changed to LED_REG_OFF

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/ethernet/marvell/skge.c | 4 ++--
 drivers/net/ethernet/marvell/skge.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/skge.c 
b/drivers/net/ethernet/marvell/skge.c
index 7173836..ff5a087 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -1062,7 +1062,7 @@ static void skge_link_up(struct skge_port *skge)
 
 static void skge_link_down(struct skge_port *skge)
 {
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
netif_carrier_off(skge->netdev);
netif_stop_queue(skge->netdev);
 
@@ -2668,7 +2668,7 @@ static int skge_down(struct net_device *dev)
if (hw->ports == 1)
free_irq(hw->pdev->irq, hw);
 
-   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_OFF);
+   skge_write8(skge->hw, SK_REG(skge->port, LNK_LED_REG), LED_REG_OFF);
if (is_genesis(hw))
genesis_stop(skge);
else
diff --git a/drivers/net/ethernet/marvell/skge.h 
b/drivers/net/ethernet/marvell/skge.h
index a2eb341..ec054d3 100644
--- a/drivers/net/ethernet/marvell/skge.h
+++ b/drivers/net/ethernet/marvell/skge.h
@@ -663,7 +663,7 @@ enum {
LED_SYNC_ON = 1<<3, /* Use Sync Wire to switch LED */
LED_SYNC_OFF= 1<<2, /* Disable Sync Wire Input */
LED_ON  = 1<<1, /* switch LED on */
-   LED_OFF = 1<<0, /* switch LED off */
+   LED_REG_OFF = 1<<0, /* switch LED off */
 };
 
 /* Receive GMAC FIFO (YUKON) */
-- 
2.7.4

[RFC 2/3] staging: rtl8712: Change _LED_STATE enum in rtl871x driver to avoid conflicts with LED namespace

[Zach Brown]

Adding led support for phy causes namespace conflicts for some
phy drivers.

The rtl871 driver declared an enum for representing LED states. The enum
contains constant LED_OFF which conflicted with declartation found in
linux/leds.h. LED_OFF changed to LED_STATE_OFF

Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/staging/rtl8712/rtl8712_led.c | 192 +-
 1 file changed, 96 insertions(+), 96 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl8712_led.c 
b/drivers/staging/rtl8712/rtl8712_led.c
index 9055827..d53ad76 100644
--- a/drivers/staging/rtl8712/rtl8712_led.c
+++ b/drivers/staging/rtl8712/rtl8712_led.c
@@ -52,7 +52,7 @@
 enum _LED_STATE_871x {
LED_UNKNOWN = 0,
LED_ON = 1,
-   LED_OFF = 2,
+   LED_STATE_OFF = 2,
LED_BLINK_NORMAL = 3,
LED_BLINK_SLOWLY = 4,
LED_POWER_ON_BLINK = 5,
@@ -92,7 +92,7 @@ static void InitLed871x(struct _adapter *padapter, struct 
LED_871x *pLed,
nic = padapter->pnetdev;
pLed->padapter = padapter;
pLed->LedPin = LedPin;
-   pLed->CurrLedState = LED_OFF;
+   pLed->CurrLedState = LED_STATE_OFF;
pLed->bLedOn = false;
pLed->bLedBlinkInProgress = false;
pLed->BlinkTimes = 0;
@@ -249,7 +249,7 @@ static void SwLedBlink(struct LED_871x *pLed)
} else {
/* Assign LED state to toggle. */
if (pLed->BlinkingLedState == LED_ON)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
pLed->BlinkingLedState = LED_ON;
 
@@ -312,7 +312,7 @@ static void SwLedBlink1(struct LED_871x *pLed)
switch (pLed->CurrLedState) {
case LED_BLINK_SLOWLY:
if (pLed->bLedOn)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
pLed->BlinkingLedState = LED_ON;
mod_timer(>BlinkTimer, jiffies +
@@ -320,7 +320,7 @@ static void SwLedBlink1(struct LED_871x *pLed)
break;
case LED_BLINK_NORMAL:
if (pLed->bLedOn)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
pLed->BlinkingLedState = LED_ON;
mod_timer(>BlinkTimer, jiffies +
@@ -335,7 +335,7 @@ static void SwLedBlink1(struct LED_871x *pLed)
pLed->bLedLinkBlinkInProgress = true;
pLed->CurrLedState = LED_BLINK_NORMAL;
if (pLed->bLedOn)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
pLed->BlinkingLedState = LED_ON;
mod_timer(>BlinkTimer, jiffies +
@@ -344,7 +344,7 @@ static void SwLedBlink1(struct LED_871x *pLed)
pLed->bLedNoLinkBlinkInProgress = true;
pLed->CurrLedState = LED_BLINK_SLOWLY;
if (pLed->bLedOn)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
pLed->BlinkingLedState = LED_ON;
mod_timer(>BlinkTimer, jiffies +
@@ -353,7 +353,7 @@ static void SwLedBlink1(struct LED_871x *pLed)
pLed->bLedScanBlinkInProgress = false;
} else {
 if (pLed->bLedOn)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
pLed->BlinkingLedState = LED_ON;
mod_timer(>BlinkTimer, jiffies +
@@ -369,7 +369,7 @@ static void SwLedBlink1(struct LED_871x *pLed)
pLed->bLedLinkBlinkInProgress = true;
pLed->CurrLedState = LED_BLINK_NORMAL;
if (pLed->bLedOn)
-   pLed->BlinkingLedState = LED_OFF;
+   pLed->BlinkingLedState = LED_STATE_OFF;
else
pLed->BlinkingLedState = LED_ON;
mod_timer(>BlinkTimer, jiffies +
@@ -378,7 +378,7 @@ static void SwLedBlink1(struct LED_871x *pLed)
pLe

[RFC 3/3] phy,leds: add support for led triggers on phy link state change

[Zach Brown]

From: Josh Cartwright <josh.cartwri...@ni.com>

Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
create a set of led triggers for each instantiated PHY device.  There is
one LED trigger per link-speed, per-phy.

This allows for a user to configure their system to allow a set of LEDs
to represent link state changes on the phy.

Signed-off-by: Josh Cartwright <josh.cartwri...@ni.com>
Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com>
Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/Kconfig|  12 
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/phy.c  |   8 +++
 drivers/net/phy/phy_device.c   |   4 ++
 drivers/net/phy/phy_led_triggers.c | 109 +
 include/linux/phy.h|   9 +++
 include/linux/phy_led_triggers.h   |  42 ++
 7 files changed, 185 insertions(+)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 1c3e07c..4aeaba4 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -15,6 +15,18 @@ if PHYLIB
 config SWPHY
bool
 
+config LED_TRIGGER_PHY
+   bool "Support LED triggers for tracking link state"
+   depends on LEDS_TRIGGERS
+   ---help---
+ Adds support for a set of LED trigger events per-PHY.  Link
+ state change will trigger the events, for consumption by an
+ LED class driver.  There are triggers for each link speed,
+ and are of the form:
+  ::
+
+ Where speed is one of: 10Mb, 100Mb, Gb, 2.5Gb, or 10GbE.
+
 comment "MDIO bus device drivers"
 
 config MDIO_BCM_IPROC
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index e58667d..86d12cd 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -2,6 +2,7 @@
 
 libphy-y   := phy.o phy_device.o mdio_bus.o mdio_device.o
 libphy-$(CONFIG_SWPHY) += swphy.o
+libphy-$(CONFIG_LED_TRIGGER_PHY)   += phy_led_triggers.o
 
 obj-$(CONFIG_PHYLIB)   += libphy.o
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c6f6683..3345767 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -936,6 +936,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_NOLINK;
netif_carrier_off(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
break;
}
 
@@ -949,6 +950,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 
} else if (0 == phydev->link_timeout--)
needs_aneg = true;
@@ -976,6 +978,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
}
break;
case PHY_FORCING:
@@ -992,6 +995,7 @@ void phy_state_machine(struct work_struct *work)
}
 
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
break;
case PHY_RUNNING:
/* Only register a CHANGE if we are polling and link changed
@@ -1021,6 +1025,7 @@ void phy_state_machine(struct work_struct *work)
}
 
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 
if (phy_interrupt_is_valid(phydev))
err = phy_config_interrupt(phydev,
@@ -1031,6 +1036,7 @@ void phy_state_machine(struct work_struct *work)
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
do_suspend = true;
}
break;
@@ -1055,6 +1061,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_NOLINK;
}
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
} else {
 

[RFC 0/3] Add support for led triggers on phy link state change

[Zach Brown]

Some drivers that include phy.h defined LED_OFF which conflicts with
definition in leds.h. phy led support uses leds.h so the two namespaces are no
longer isolated.
The first two patches fix the two net drivers that declared enum constants that
conflict with enum constants in linux/leds.h.

The final patch adds support for led triggers on phy link state changes by
adding a config option. When set the config option will create a set of led
triggers for each phy device. Users can use the led triggers to represent link
state changes on the phy.
The changes assumes that there are 5 speed options
10Mb,100Mb,1Gb,2.5Gb,10Gb
The assumption makes mapping a phy_device's current speed to a trigger easy,
but means there are triggers made that aren't used if the phy doesn't support
the corresponding speeds.
Thoughts on how to better manage the triggers created would be appreciated
if is important to do so.

Josh Cartwright (1):
  phy,leds: add support for led triggers on phy link state change

Zach Brown (2):
  skge: Change LED_OFF to LED_REG_OFF in marvel skge driver to avoid
conflicts with leds namespace
  staging: rtl8712: Change _LED_STATE enum in rtl871x driver to avoid
conflicts with LED namespace

 drivers/net/ethernet/marvell/skge.c   |   4 +-
 drivers/net/ethernet/marvell/skge.h   |   2 +-
 drivers/net/phy/Kconfig   |  12 +++
 drivers/net/phy/Makefile  |   1 +
 drivers/net/phy/phy.c |   8 ++
 drivers/net/phy/phy_device.c  |   4 +
 drivers/net/phy/phy_led_triggers.c| 109 +++
 drivers/staging/rtl8712/rtl8712_led.c | 192 +-
 include/linux/phy.h   |   9 ++
 include/linux/phy_led_triggers.h  |  42 
 10 files changed, 284 insertions(+), 99 deletions(-)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

--
2.7.4

[PATCH] phy, leds: add support for led triggers on phy link state change

[Zach Brown]

From: Josh Cartwright <josh.cartwri...@ni.com>

This introduces an option CONFIG_LED_TRIGGER_PHY (default n), which will
create a set of led triggers for each instantiated PHY device.  There is
one LED trigger per link-speed, per-phy.

This allows for a user to configure their system to allow a set of LEDs
to represent link state changes on the phy.

Signed-off-by: Josh Cartwright <josh.cartwri...@ni.com>
Signed-off-by: Nathan Sullivan <nathan.sulli...@ni.com>
Signed-off-by: Zach Brown <zach.br...@ni.com>
---
 drivers/net/phy/Kconfig|  12 +
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/phy.c  |   8 +++
 drivers/net/phy/phy_device.c   |   4 ++
 drivers/net/phy/phy_led_triggers.c | 107 +
 include/linux/phy.h|   9 
 include/linux/phy_led_triggers.h   |  42 +++
 7 files changed, 183 insertions(+)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index d66133bf..ebab7cc 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -169,6 +169,18 @@ config FIXED_PHY
 
  Currently tested with mpc866ads and mpc8349e-mitx.
 
+config LED_TRIGGER_PHY
+   bool "Support LED triggers for tracking link state"
+   depends on LEDS_TRIGGERS
+   ---help---
+ Adds support for a set of LED trigger events per-PHY.  Link
+ state change will trigger the events, for consumption by an
+ LED class driver.  There are triggers for each link speed,
+ and are of the form:
+  ::
+
+ Where speed is one of: 10Mb, 100Mb, Gb, 2.5Gb, or 10GbE.
+
 config MDIO_BITBANG
tristate "Support for bitbanged MDIO buses"
help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 73d65ce..0d69da7 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_ICPLUS_PHY)  += icplus.o
 obj-$(CONFIG_REALTEK_PHY)  += realtek.o
 obj-$(CONFIG_LSI_ET1011C_PHY)  += et1011c.o
 obj-$(CONFIG_FIXED_PHY)+= fixed_phy.o
+obj-$(CONFIG_LED_TRIGGER_PHY)  += phy_led_triggers.o
 obj-$(CONFIG_MDIO_BITBANG) += mdio-bitbang.o
 obj-$(CONFIG_MDIO_GPIO)+= mdio-gpio.o
 obj-$(CONFIG_NATIONAL_PHY) += national.o
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c5dc2c36..1baaf0f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -934,6 +934,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_NOLINK;
netif_carrier_off(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
break;
}
 
@@ -947,6 +948,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 
} else if (0 == phydev->link_timeout--)
needs_aneg = true;
@@ -974,6 +976,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
}
break;
case PHY_FORCING:
@@ -990,6 +993,7 @@ void phy_state_machine(struct work_struct *work)
}
 
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
break;
case PHY_RUNNING:
/* Only register a CHANGE if we are polling and link changed
@@ -1019,6 +1023,7 @@ void phy_state_machine(struct work_struct *work)
}
 
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 
if (phy_interrupt_is_valid(phydev))
err = phy_config_interrupt(phydev,
@@ -1029,6 +1034,7 @@ void phy_state_machine(struct work_struct *work)
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
do_suspend = true;
}
break;
@@ -1053,6 +1059,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_NOLI

[PATCH] sctp: lock_sock_nested in sctp_sock_migrate

[Zach Brown]

I'm not sure that I've gotten either the sctp or lockdep details right,
but with this patch I don't get lockdep yelling at me any more :)

--

sctp: lock_sock_nested in sctp_sock_migrate

sctp_sock_migrate() grabs the socket lock on a newly allocated socket while
holding the socket lock on an old socket.  lockdep worries that this might
be a recursive lock attempt.

 task/3026 is trying to acquire lock:
  (sk_lock-AF_INET){--..}, at: [88105b8c] 
sctp_sock_migrate+0x2e3/0x327 [sctp]
 but task is already holding lock:
  (sk_lock-AF_INET){--..}, at: [8810891f] sctp_accept+0xdf/0x1e3 
[sctp]

This patch tells lockdep that this locking is safe by using
lock_sock_nested().

Signed-off-by: Zach Brown [EMAIL PROTECTED]

diff -r 8adcfdf2545b net/sctp/socket.c
--- a/net/sctp/socket.c Fri Jun 22 11:11:33 2007 -0700
+++ b/net/sctp/socket.c Fri Jun 22 15:05:22 2007 -0700
@@ -6084,8 +6084,11 @@ static void sctp_sock_migrate(struct soc
 * queued to the backlog.  This prevents a potential race between
 * backlog processing on the old socket and new-packet processing
 * on the new socket.
-*/
-   sctp_lock_sock(newsk);
+*
+* The caller has just allocated newsk so we can guarantee that other
+* paths won't try to lock it and then oldsk.
+*/
+   lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
sctp_assoc_migrate(assoc, newsk);
 
/* If the association on the newsk is already closed before accept()

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [take9 1/2] kevent: Core files.

[Zach Brown]

 +   for (i=0; iARRAY_SIZE(u-kevent_list); ++i)
  for (i = 0; i  ARRAY_SIZE(u-kevent_list); i++)
 
 Ugh, no. It reduces readability due to exessive number of spaces.

Ihavetoverystronglydisagree.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [take9 0/2] kevent: Generic event handling mechanism.

[Zach Brown]

 It is done by scripts using list of files generated by git-diff, but I
 can reformat them to be in a way:

Perhaps you should think about maintaining them as an explicit series of
patches, say with quilt or mq, instead of as one repository that you try
and cut up into separate patches.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [take5 0/4] kevent: Generic event handling mechanism.

[Zach Brown]

Evgeniy Polyakov wrote:
 Generic event handling mechanism.
 
 I send this patchset for comments and review, it still contains AIO and 
 aio_sendfile() implementation on top of get_block() abstraction, which was
 decided to postpone for a while (it is simpler right now to generate patchset 
 as a whole,
 when kevent will be ready for merge, I will generate patchset without AIO 
 stuff).

I think that's the wrong order.  Let's see the clean patch before
deciding to merge it.  Just remove the current aio_sendfile
implementation.  We can build up one that is suitable for merging once
the kevent core is ready for merging.

 Since number of suggested changes goes from 'several' to 'very small', I'm 
 asking for 
 inclusion or declining.

Well, I'm a little confused.  It still seems like we have a long way to
go before having something that we'll want to merge.  There's still
trivial things that need fixing like using LIST_POISON outside list.h
and that nutty 1 second default timeout thing, both previously
mentioned.  Then there's much more fundamental stuff like still
returning events from the _wait() syscall.  I thought the notion was
that the wait syscall would only wait and that events were always to be
collected from the ring.

David, what do you think about the networking calls in here?  Are they
suitable for merging?  I've been assuming that you'll want much more
code re-use, but I don't remember seeing you say either way.

There's also something that's been bugging me that I want to ask the
crowd.  The kevent work is doing the same thing fs/aio.c did: for an API
to support async operation we duplicate its existing system call API.
In fs/aio.c it was IO_CMD_* and struct uiocb members, with kevent it's a
system call like sys_aio_sendfile().  This seems like it might get to be
a little much if we bring async support to much of networking, disk io,
and say timers.  That's a lot of interfaces to duplicate.  Is this the
direction we want to take?  One could imagine doing a wrapper syscall
that might allocate a kevent struct and hang it off task_struct for
subsystems to recognize and work with, but that'd bring different
trade-offs.

 Should I prepare final patchset and what should it include?

Can you address the things I mentioned last time I looked through the
code and remove the parts of the patch that we know aren't going to be
merged?

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [take5 1/4] kevent: Core files.

[Zach Brown]

 +++ b/include/linux/kevent.h

 ...

 +#ifdef CONFIG_KEVENT_SOCKET
 +
 +extern struct file_operations socket_file_ops;

This doesn't build because socket_file_ops was left static in net/socket.c.

In any case, kevent.h has no business exposing socket_file_ops to users
of the kevent api just so the kevent core can test files as being backed
by sockets.  It'd be more appropriate to call into the socket layer with
the filp and let it return -EINVAL or -ESOCKNOOPT instead of trying to
do that in the kevent layer.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/4] kevent: core files.

[Zach Brown]

 I do not think if we do a ring buffer that events should be obtainable
 via a syscall at all.  Rather, I think this system call should be
 purely sleep until ring is not empty.

Mmm, yeah, of course.  That's much simpler.  I'm looking forward to
Evgeniy's next patch set.

 The ring buffer size, as Evgeniy also tried to describe, is bounded
 purely by the number of registered events.

Yeah.  fwiw, fs/aio.c has this property today.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [take2 1/4] kevent: core files.

[Zach Brown]

OK, here's some of my reactions to the core part.

 +#define KEVENT_SOCKET0
 +#define KEVENT_INODE 1
 +#define KEVENT_TIMER 2
 +#define KEVENT_POLL  3
 +#define KEVENT_NAIO  4
 +#define KEVENT_AIO   5

I guess we can't really avoid some form of centralized list of the
constants in the API if we're going for a flat constant namespace.
It'll be irritating to manage this list over time, just like it's
irritating to manage syscall numbers now.

 +/*
 + * Socket/network asynchronous IO events.
 + */
 +#define  KEVENT_SOCKET_RECV  0x1
 +#define  KEVENT_SOCKET_ACCEPT0x2
 +#define  KEVENT_SOCKET_SEND  0x4

I wonder if these shouldn't live in the subsystems instead of in kevent.h.

 +/*
 + * Poll events.
 + */
 +#define  KEVENT_POLL_POLLIN  0x0001
 +#define  KEVENT_POLL_POLLPRI 0x0002
 +#define  KEVENT_POLL_POLLOUT 0x0004
 +#define  KEVENT_POLL_POLLERR 0x0008
 +#define  KEVENT_POLL_POLLHUP 0x0010
 +#define  KEVENT_POLL_POLLNVAL0x0020
 +
 +#define  KEVENT_POLL_POLLRDNORM  0x0040
 +#define  KEVENT_POLL_POLLRDBAND  0x0080
 +#define  KEVENT_POLL_POLLWRNORM  0x0100
 +#define  KEVENT_POLL_POLLWRBAND  0x0200
 +#define  KEVENT_POLL_POLLMSG 0x0400
 +#define  KEVENT_POLL_POLLREMOVE  0x1000

And couldn't we just use the existing poll bit definitions for this?

 +struct kevent_id
 +{
 + __u32   raw[2];
 +};

Why not a simple u64?  Users can play games with packing it into other
types if they want.

 + __u32   user[2];/* User's data. It is 
 not used, just copied to/from user. */
 + void*ptr;
 + };

Again just a u64 seems like it would be simpler.  userspace library
wrappers can help massage it, but the kernel is just treating it as an
opaque data blob.

 +};
 +
 +#define  KEVENT_CTL_ADD  0
 +#define  KEVENT_CTL_REMOVE   1
 +#define  KEVENT_CTL_MODIFY   2
 +#define  KEVENT_CTL_INIT 3
 +
 +struct kevent_user_control
 +{
 + unsigned intcmd;/* Control command, 
 e.g. KEVENT_ADD, KEVENT_REMOVE... */
 + unsigned intnum;/* Number of ukevents 
 this strucutre controls. */
 + unsigned inttimeout;/* Timeout in 
 milliseconds waiting for num events to become ready. */
 +};

Even if we only have one syscall with a cmd multiplexer (which I'm not
thrilled with), we should at least make these arguments explicit in the
system call.  It's weird to hide them in a struct.  We could also think
about making them u32 or u64 so that we don't need compat wrappers, but
maybe that's overkill.

Also, can we please use a struct timespec for the timeout?  Then the
kernel will have the luxury of using whatever mechanism it wants to
satisfy the user's precision desires.  Just like sys_nanosleep() uses
timespec and so can be implemented with hrtimers.

 +struct kevent
 +{

(trivial nit, struct kevent { is the preferred form.)

 + struct ukevent  event;
 + spinlock_t  lock;   /* This lock protects 
 ukevent manipulations, e.g. ret_flags changes. */


It'd be great if these struct members could get a prefix (ala: inode -
i_, socket - sk_) so that it's less painful getting tags helpers to
look up instances for us.  Asking for 'lock' is hilarious.

 +struct kevent_list
 +{
 + struct list_headkevent_list;/* List of all kevents. 
 */
 + spinlock_t  kevent_lock;/* Protects all 
 manipulations with queue of kevents. */
 +};
 +
 +struct kevent_user
 +{
 + struct kevent_list  kqueue[KEVENT_HASH_MASK+1];

Hmm.  I think the current preference is not to have a lock per bucket.
It doesn't scale nearly as well as it seems like it should as the cache
footprint is higher and as cacheline contention hits as there are
multiple buckets per cacheline.  For now I'd simplify the hash into a
single lock and an array of struct hlist_head.  In the future it could
be another user of some kind of relatively-generic hash implementation
based on rcu that has been talked about for a while.

 +#define KEVENT_MAX_REQUESTS  PAGE_SIZE/sizeof(struct kevent)

This is unused?

 +#define list_for_each_entry_reverse_safe(pos, n, head, member)   
 \
 + for (pos = list_entry((head)-prev, typeof(*pos), member),  \
 + n = list_entry(pos-member.prev, typeof(*pos), member); \
 +  prefetch(pos-member.prev), pos-member != (head);\
 +  pos = n, n = list_entry(pos-member.prev, typeof(*pos), member))

If anyone was calling this they could use
list_for_each_entry_safe_reverse() in list.h but nothing is calling it?
 Either way, it should be removed :).

 +#define sock_async(__sk) 0

It's a minor complaint, but these kinds of ifdefs that drop arguments
can cause unused 

Re: [RFC 1/4] kevent: core files.

[Zach Brown]

 I completely agree that existing kevent interface is not the best, so
 I'm opened for any suggestions.
 Should kevent creation/removing/modification be separated too?

Yeah, I think so.

 Hmm, it looks like I'm lost here...
 Yeah, it seems my description might not have sunk in :).  We're giving
 userspace a way to collect events without performing a system call.
 
 And why do we want this?

So that event collection can be very efficient.

 How glibc is supposed to determine, that some events already fired and
 such requests will return immediately, or for example how timer events
 will be managed?

...

That was what my previous mail was all about!

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/4] kevent: core files.

[Zach Brown]

 Things were like that at one point in time, but file descriptors turn out 
 to introduce a huge gaping security hole with SUID programs.  The problem 
 is that any event context is closely tied to the address space of the 
 thread issuing the syscalls, and file descriptors do not have this close 
 binding.

Can you go into that hole in more detail?

 Except that you're not usually pulling a full ring worth of events at a 
 time, more often just one.

OK, but then to wait for it you were already sleeping in the kernel, right?

Clearly we should port httpd to kevents and take some measurements :)

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/4] kevent: core files.

[Zach Brown]

 So, I'm going to create kevent_create/destroy/control and kevent_get_events()
 Or any better names?

Yeah, that sounds good.

 Some events are impossible to create in userspace (like timer
 notification, which requires timer start and check when timer
 completed).

We're not talking about *creating* events in userspace, we're talking
about checking for their completion events in the ring.

 and get ready), and I do not see how, for exmple, glibc can avoid them
 when user requested POLLIN or similar event for network dataflow?

There are events that can be generated by kernel code paths as the event
completes.  Network sockets have the hooks to do this with SIGIO, it's
very natural for the storage completion paths, etc.  So that kernel code
would update the ring which userspace could check.  AIO does this today.
 Userspace would still have to use the syscall to sleep waiting for new
events when the ring is empty.

 According to syscall speed on Linux, last time I checked empty syscall 
 took about 100ns on AMD Athlon 3500+.

Oh, sure, but still nice to avoid.

I'm mostly pursuing this because Ulrich seemed so insistent on it in his
paper and talk.  I will be very sad if we don't have aggressive glibc
support for this generic event collection interface and so I want very
much to keep him engaged.  Ulrich, would you be satisfied if we didn't
have the userspace mapped ring on the first pass and only had a
collection syscall?

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/4] kevent: core files.

[Zach Brown]

Evgeniy Polyakov wrote:
 On Fri, Jul 28, 2006 at 12:01:28PM -0700, Zach Brown ([EMAIL PROTECTED]) 
 wrote:
 Clearly we should port httpd to kevents and take some measurements :)
 
 One of my main kevent benchmarks (socket notifications for
 accept/receive) is handmade http server.

Yeah, so I noticed.  That's a good starting point but I'm more
interested in seeing the work integrated with servers that have to
survive outside of benchmarking runs.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/4] kevent: core files.

[Zach Brown]

 Clearly we should port httpd to kevents and take some measurements :)

oh, I see, I forgot the 't' in 'thttpd'.  My mistake.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [3/4] kevent: AIO, aio_sendfile() implementation.

[Zach Brown]

 Suparna mentioned at Ulrich wants us to concentrate on kernel-side 
 support, so that he can look at glibc side of things (along with
 other work he is already doing). So, if we can get an agreement on
 what kind of kernel support is needed - we can focus our efforts on
 kernel side first and leave glibc enablement to capable hands of Uli
 :)

Yeah, and the existing patches still need some cleanup.  Badari, did you
still want me to look into that?

We need someone to claim ultimate responsibility for getting these
patches suitable for merging :).  I'm happy to do that if Suparna isn't
already on it.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/4] kevent: core files.

[Zach Brown]

 I like this work a lot, as I've stated before.

Yeah, me too.  I think we're very close to having a workable system
here.  A few weeks of some restructuring and we all might be very happy.

 The data structures
 look like they will scale well and it takes care of all the limitations
 that networking in particular seems to have in this area.
 
 I have to say that the user API is not the nicest in the world.  Yet,
 at the same time, I cannot think of a better one :)

I want to first focus on the event collection side of the API because I
think we can definitely do better there :).  I hope we all agree that
there is huge value in having one place where an application can wait
for notification from many different sources.  If we get the collection
side right we can later worry about generating events down the pipe from
subsystems in a way that works best for them.

I'll sort of ramble about my thoughts here.

The easy part is fixing up the somewhat obfuscated collection call.
Instead of coming in through a multiplexer that magically treats a void
* as a struct kevent_user_control followed by N ukevents (as specified
in the kevent_user_control!) we'd turn it into a more explicit
collection syscall:

int kevent_getevents(int event_fd, struct ukevent *events,
int min_events, int max_events,
struct timeval *timeout);

This would look a lot less nutty in strace.  It lets apps specify if
there is some minimum number of events they'd like the opportunity to
process rather than waiting for the timeout to expire before the max
number arrives.  (the latter is what kevent_user_wait() does today).  We
can have the usual argument about whether *timeout is updated on a
partial wake-up :).

That'd be a fine syscall collection interface, but we should try hard to
explore being able to collect events without hitting the kernel.

Say we have a ring of event structs.  AIO has this today, but it sort of
gets it wrong because each event element doesn't specify whether it is
owned by the kernel or userspace.  (It really gets it wrong because it
doesn't flush_dcache_page() after updating the ring via kmap(), but
never mind that!  No one actually uses this mmap() AIO ring.)  In AIO
today there is also a control struct mapped along with the ring that has
head and tail pointers.  We don't want to bounce that cacheline around.
 net/socket/af_packet.c gets this right with it's tp_status member of
tpacket_hdr.

So as the kernel generates events in the ring it only produces an event
if the ownership field says that userspace has consumed it and in doing
so it sets the ownership field to tell userspace that an event is
waiting.  userspace and the kernel now each follow their index around
the ring as the ownership field lets them produce or consume the event
at their index.  Can someone tell me if the cache coherence costs of
this are extreme?  I'm hoping they're not.

So, great, glibc can now find pending events very quickly if they're
waiting in the ring and can fall back to the collection syscall if it
wants to wait and the ring is empty.  If it consumes events via the
syscall it increases its ring index by the number the syscall returned.

There's two things we should address: level events and the notion of
only submitting as much as fits in the ring.

epoll and kevent both have the notion of an event type that always
creates an event at the time of the collection syscall while the event
source is on a ready list.  Think of epoll calling -poll(POLLOUT) for
an empty socket buffer at every sys_epoll_wait() call.  We can't have
some source constantly spewing into the ring :/.  We could fix this by
the API requiring that level events can *only* be collected through the
syscall interface.  userspace could call into the collection syscall
every N events collected through the ring, say.  N would be tuned to
amortize the syscall cost and still provide fairness or latency for the
level sources.  I'd be fine with that, especially when it's hidden off
in glibc.

Today AIO only allows submission of as many events as there are space in
the ring.  It mostly does this so its completion can drop an event in
the ring from any context.  If we back away from this so that we can
have long-lived source registration generate multiple edge events (and I
think we want to!), we have to be kind of careful.  A source could
generate an event while the ring is full.  The event could go in a list
but if userspace is collecting events in userspace the kernel won't be
told when there's space.  We'd first have to check this ready list when
later events are generated so that pending events on the list aren't
overlooked.  Userspace would also want to use the collection syscall as
the ring empties.  Neither seem hard.

So how does this sound?  It wouldn't take me long to build this off of
the current kevent patches.  We could see how it looks..

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a 

Re: [RFC 1/4] kevent: core files.

[Zach Brown]

  int kevent_getevents(int event_fd, struct ukevent *events,
  int min_events, int max_events,
  struct timeval *timeout);
 
 I used only one syscall for all operations, above syscall is
 essentially what kevent_user_wait() does.

Essentially, yes, but the differences are important.  It's important to
have a clear syscall interface instead of nesting through multiplexers.
 And we should get the batching/latency inputs right.  (I'm for both
min/max elements and arguably timeouts, but I could understand not
wanting to go *that* far.)

 Hmm, it looks like I'm lost here...

Yeah, it seems my description might not have sunk in :).  We're giving
userspace a way to collect events without performing a system call.

 I especially like idea about world happinness in a week or so :)

A few weeks! :)

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/4] kevent: core files.

[Zach Brown]

  int kevent_getevents(int event_fd, struct ukevent *events,
  int min_events, int max_events,
  struct timeval *timeout);
 
 You've just reinvented io_getevents().

Well, that's certainly one inflammatory way to put it.  I would describe
it as suggesting that the kevents collection interface not lose the
nicer properties of io_getevents().

 What exactly are we getting from 
 reinventing this (aside from breaking existing apps and creating more of 
 an API mess)?

A generic event collection interface that isn't so strongly bound to the
existing semantics of io_setup() and io_submit().  It can be a file
descriptor instead of a mysterious cookie/pointer to the mapped region,
to start.

 incompat bits for changing the structure layout.  As for bouncing the 
 cacheline of head/tail around, I don't think it matters on real machines, 
 as the multithreaded/SMP case will hit that cacheline bouncing if the 
 user is sharing the event ring between multiple threads on multiple CPUs.  
 The only way around that is to use multiple event rings, say one per node, 
 at which point you have to do load balancing of io requests explicitely 
 between queues (which might be worth it).

Sure, so maybe we experiment with these things in the context of the
kevent patches and maybe merge them back into the AIO paths if in the
end that's the right thing to do.  I see no problem with separating
development from the existing code.

 epoll and kevent both have the notion of an event type that always
 creates an event at the time of the collection syscall while the event
 source is on a ready list.  Think of epoll calling -poll(POLLOUT) for
 an empty socket buffer at every sys_epoll_wait() call.  We can't have
 some source constantly spewing into the ring :/.  We could fix this by
 the API requiring that level events can *only* be collected through the
 syscall interface.  userspace could call into the collection syscall
 every N events collected through the ring, say.  N would be tuned to
 amortize the syscall cost and still provide fairness or latency for the
 level sources.  I'd be fine with that, especially when it's hidden off
 in glibc.
 
 This is exactly why I think level triggered events are nasty.  It's 
 impossible to do cleanly without requiring a syscall.

I'm not convinced that it isn't possible to get a sufficiently clean
interface that involves the mix.

 As soon as you allow queueing events up in kernel space, it becomes 
 necessary to do another syscall after pulling events out of the queue, 
 which is a waste of CPU cycles when you're under heavy load (exactly the 
 point at which you want the system to be its most efficient).

If we've just consumed a full ring worth of events, and done real work
with them, I'm not convinced that an empty syscall is going to be that
painful.  If we're really under load it might well return some newly
arrived events.  It becomes a mix of ring completions and syscall
completions.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] IB/mthca: comment fix

[Zach Brown]

Michael S. Tsirkin wrote:
 OK, this is fine with both Arjan van de Ven and Roland Dreier, so -
 Andrew, could you take this into -mm please?

 Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]

Acked-by: Zach Brown [EMAIL PROTECTED]

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tcp_slow_start_after_idle

[Zach Brown]

David Miller wrote:
 Bringing back up this old topic:
 
   http://marc.theaimsgroup.com/?l=linux-netdevm=114564962420171w=2
 
 I've decided to add this tunable to the net-2.6.18 tree, patch below.

Nice, thanks for the heads-up.  I'll pass the notice on to the guys who
were asking about this in that thread.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tune back idle cwnd closing?

[Zach Brown]

 Given that RFC2681 is Experimental (and I'm not aware of any current 
 efforts in the IETF to push it to the standard track), IHMO it would not 
 be inappropriate to make this behavior controlled via sysctl.
 
 I have to respectfully disagree.

OK, thanks for taking the time to look at it.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

tune back idle cwnd closing?

[Zach Brown]

My apologies if this is a FAQ, I couldn't find it in the archives.

We have some dudes who are syncing large amounts of data across a
dedicated long fat pipe at somewhat irregular intervals that are, sadly,
longer than the rto.  They feel the pain of having to reopen the window
between transmissions.

Is there room for a compromise tunable that would be less aggressive
about closing cwnd during idle periods but which wouldn't violate the
spirit of 2861?  No one wants broken TCP here.

They mention that Solaris has the tcp_slow_start_after_idle tunable and
that it helps their situation.  I mention that only as a data point, I
wouldn't be foolish enough to try and use the presence of something in
Solaris as justification :)

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] Avoid multi-page allocs for large mtu datagram sends

[Zach Brown]

Herbert Xu wrote:
 On Wed, Apr 05, 2006 at 05:51:02PM +, Zach Brown wrote:
 2) I changed the final-frag test to be length + fraggap as the math later on
seemed to match that.. is that OK?
 
 Yes that's a real bug introduced by a previous rework.  Could you
 please split that off into a separate patch?

Sure, here it is by itself.  I lightly tested this but didn't actually
use anything that has a non-zero trailer len, I don't think.

Did you have an opinion of the rest of the original patch?

- z



[PATCH] ip_output: account for fraggap when checking to add trailer_len

During other work I noticed that ip_append_data() seemed to be forgetting to
include the frag gap in its calculation of a fragment that consumes the rest of
the payload.  Herbert confirmed that this was a bug that snuck in during a
previous rework.

Signed-off-by: Zach Brown [EMAIL PROTECTED]

Index: 2.6.17-rc1-mm2-fraggap/net/ipv4/ip_output.c
===
--- 2.6.17-rc1-mm2-fraggap.orig/net/ipv4/ip_output.c
+++ 2.6.17-rc1-mm2-fraggap/net/ipv4/ip_output.c
@@ -904,7 +904,7 @@ alloc_new_skb:
 			 * because we have no idea what fragment will be
 			 * the last.
 			 */
-			if (datalen == length)
+			if (datalen == length + fraggap)
 alloclen += rt-u.dst.trailer_len;
 
 			if (transhdrlen) {

Re: ipfrag calculating its hash outside lock

[Zach Brown]

 I got tired of waiting for Zach to cook up a patch so I tossed
 the following into my tree :-)

Haha, sorry, I didn't realize I was racing the clock :).  We disappeared
this weekend.

That matches what I came up with, but did we miss the IPv6 part?  Here's
that half if you still need it.

Herbert, does it look OK?  I don't have v6 stuff setup here..

- z

[IPv6] reassembly: Always compute hash under the fragment lock.

This closes a race where an ipq6hashfn() caller could get a hash value
and race with the cycling of the random seed.  By the time they got to the
read_lock they'd have a stale hash value and might not find previous fragments
of their datagram.

This matches the previous patch to IPv4.

Signed-off-by: Zach Brown [EMAIL PROTECTED]

Index: 2.6.17-rc1-mm1-fraghash/net/ipv6/reassembly.c
===
--- 2.6.17-rc1-mm1-fraghash.orig/net/ipv6/reassembly.c
+++ 2.6.17-rc1-mm1-fraghash/net/ipv6/reassembly.c
@@ -121,6 +121,10 @@ static __inline__ void fq_unlink(struct 
 	write_unlock(ip6_frag_lock);
 }
 
+/*
+ * callers should be careful not to use the hash value outside the ipfrag_lock
+ * as doing so could race with ipfrag_hash_rnd being recalculated.
+ */
 static unsigned int ip6qhashfn(u32 id, struct in6_addr *saddr,
 			   struct in6_addr *daddr)
 {
@@ -322,15 +326,16 @@ out:
 /* Creation primitives. */
 
 
-static struct frag_queue *ip6_frag_intern(unsigned int hash,
-	  struct frag_queue *fq_in)
+static struct frag_queue *ip6_frag_intern(struct frag_queue *fq_in)
 {
 	struct frag_queue *fq;
+	unsigned int hash;
 #ifdef CONFIG_SMP
 	struct hlist_node *n;
 #endif
 
 	write_lock(ip6_frag_lock);
+	hash = ip6qhashfn(fq_in-id, fq_in-saddr, fq_in-daddr);
 #ifdef CONFIG_SMP
 	hlist_for_each_entry(fq, n, ip6_frag_hash[hash], list) {
 		if (fq-id == fq_in-id  
@@ -360,7 +365,7 @@ static struct frag_queue *ip6_frag_inter
 
 
 static struct frag_queue *
-ip6_frag_create(unsigned int hash, u32 id, struct in6_addr *src, struct in6_addr *dst)
+ip6_frag_create(u32 id, struct in6_addr *src, struct in6_addr *dst)
 {
 	struct frag_queue *fq;
 
@@ -377,7 +382,7 @@ ip6_frag_create(unsigned int hash, u32 i
 	spin_lock_init(fq-lock);
 	atomic_set(fq-refcnt, 1);
 
-	return ip6_frag_intern(hash, fq);
+	return ip6_frag_intern(fq);
 
 oom:
 	IP6_INC_STATS_BH(IPSTATS_MIB_REASMFAILS);
@@ -389,9 +394,10 @@ fq_find(u32 id, struct in6_addr *src, st
 {
 	struct frag_queue *fq;
 	struct hlist_node *n;
-	unsigned int hash = ip6qhashfn(id, src, dst);
+	unsigned int hash;
 
 	read_lock(ip6_frag_lock);
+	hash = ip6qhashfn(id, src, dst);
 	hlist_for_each_entry(fq, n, ip6_frag_hash[hash], list) {
 		if (fq-id == id  
 		ipv6_addr_equal(src, fq-saddr) 
@@ -403,7 +409,7 @@ fq_find(u32 id, struct in6_addr *src, st
 	}
 	read_unlock(ip6_frag_lock);
 
-	return ip6_frag_create(hash, id, src, dst);
+	return ip6_frag_create(id, src, dst);
 }
 
 

ipfrag calculating its hash outside lock

[Zach Brown]

I noticed that ip_find() calculates the hash bucket for the incoming
fragment using ipfrag_hash_rnd outside the ipfrag_lock.  So it can race
with ipfrag_secret_rebuild() and end up putting a frag in the previous
bucket instead of the new bucket that ipfrag_secret_rebuild() has put
the previous frags in.  The unlock and write lock acquiry in the
allocating case could also hit this.

I verified this by spraying a box with 16k udp writes and turning the
secret interval way down to hz/100.  It didn't take long before it hit.
 It resulted in reasm failures due to frag timeouts as expected.

Before worrying about fixing this I thought I'd sample the crowd.

a) Who cares?  The interval is huge and it'd be a few packets at most.

b) Just calculate the hashes under the lock, we're already doing lots of
work there anyway.

c) Sample the random value when calculating the hash outside the lock
and only recalculate the hash inside the lock if it changes.  (Maybe
with some memory barrier help).

d) Go insane redoing the serialization so that we get rid of this and
other suboptimal behaviour.  (please no :)).

I vote for c) for it's compromise of minimal invasiveness and impact to
the unracey case.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ipfrag calculating its hash outside lock

[Zach Brown]

 I think this is the best way to go.  Then we don't need to think
 about it, and frankly I think the recheck hash rnd after getting
 lock idea would turn out to be more expensive :)

OK :).  I'll throw that together..

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH] Avoid multi-page allocs for large mtu datagram sends

[Zach Brown]

OK, here's a stronger attempt.  I might have gone a little wild with the patch
description :).  Two questions and a data point:

1) It occurs to me as I write this that I don't know how the trailer_len stuff
   works if a fragments final data bits are page frags in the skb.  Did I
   screw this up?  How did the old code handle it?  Maybe I'm just paranoid.

2) I changed the final-frag test to be length + fraggap as the math later on
   seemed to match that.. is that OK?

I tested this with two back to back e1000s.. ping ponging udp messages of sizes
1 ... 65507 across mtus of 576, 1500, 9000, and 16110.  That did shake out a
few bugs.

So how's this looking?

- z



Avoid multi-page allocs for large mtu datagram sends 

Previously the size of the first skb allocated for an IP fragment was bounded
by the MTU of the outgoing device.  With large MTUs (32k, say) this results in
multi order allocations.  We were seeing these fail on machines that were
heavily loaded after about a week of uptime.

This patch clamps the size of the first skb allocated to a single page,
including the headers and skb accounting overhead.  SKB_DATA_KMALLOC_BYTES() is
introduced to let callers know how large an allocation will be.
ip_append_data() uses this to restrict the amount of payload that it puts in
the initial skb to just fit a page-sized allocation.  This is only done if the
device supports scatter-gather so that the rest of the fragment can be built of
more page-sized allocs and hung off the skb.

The confusing case is the inclusion of the trailer_len overhead in the
allocation for the final fragment.  The risk is in having a page-sized initial
skb that has room for the full payload but not enough room for the trailer_len.
We avoid this by always including the trailer_len overhead in the allocaiton if
the payload fits the MTU.  If we shrink datalen then it won't be the final
fragment and it won't have a trailer, but we've allocated space for it.  We
reuse that space for payload only if doing so doesn't increase datalen so much
that it consumes the entire payload again.  So in that case we'll have an
initial fragment that doesn't fill the mtu and a second fragment that has a
small amount of payload and the trailer.  Sheesh.

And finally, while we introduce SKB_DATA_KMALLOC_BYTES() we use it in some
other code paths that previously had the same functionality coded by hand.

Index: 2.6.16-mm2-bigmtu/net/ipv4/ip_output.c
===
--- 2.6.16-mm2-bigmtu.orig/net/ipv4/ip_output.c
+++ 2.6.16-mm2-bigmtu/net/ipv4/ip_output.c
@@ -891,32 +891,50 @@ alloc_new_skb:
datalen = length + fraggap;
if (datalen  mtu - fragheaderlen)
datalen = maxfraglen - fragheaderlen;
-   fraglen = datalen + fragheaderlen;
 
-   if ((flags  MSG_MORE)  
-   !(rt-u.dst.dev-featuresNETIF_F_SG))
-   alloclen = mtu;
-   else
-   alloclen = datalen + fragheaderlen;
+   alloclen = fragheaderlen + hh_len + 15;
 
/* The last fragment gets additional space at tail.
 * Note, with MSG_MORE we overallocate on fragments,
 * because we have no idea what fragment will be
 * the last.
 */
-   if (datalen == length)
+   if (datalen == length + fraggap)
alloclen += rt-u.dst.trailer_len;
 
+   if ((rt-u.dst.dev-featuresNETIF_F_SG) 
+   (SKB_DATA_KMALLOC_BYTES(alloclen + datalen)
+PAGE_SIZE)) {
+   /*
+* datalen is shrinking so there won't be a
+* trailer, but alloclen already accounted for
+* it.  use that space for payload but only if
+* it wasn't the trailer_len itself that pushed
+* the alloc beyond a single page.
+*/
+   if (datalen - SKB_MAX_ORDER(alloclen, 0) 
+   rt-u.dst.trailer_len)
+   alloclen -= rt-u.dst.trailer_len;
+
+   datalen = SKB_MAX_ORDER(alloclen, 0);
+   }
+
+   fraglen = datalen + fragheaderlen;
+
+   if ((flags  MSG_MORE) 
+   !(rt-u.dst.dev-featuresNETIF_F_SG))
+   alloclen += mtu - fragheaderlen;
+   else
+   alloclen += datalen;
+
if 

Re: [RFC][PATCH] avoid multi page allocs in ip_append_data

[Zach Brown]

 What exactly are you trying to do with that watch out for trailer_len
 thing where you subtract 2 from datalen?

The danger is in chosing a datalen that just fits in a page but then
finding that it == length and so we want to add on trailer_len, bringing
the alloc back over a page.

The logic seemed simpler, if hackier, to discover this case after the
fact rather than trying to bring the length/trailer_len logic up into
the initial estimation of the datalen that will fit in a page.  It
obviously doesn't deal well with cases where datalen = 2 :/ :/

I'm not thrilled with it, either.  I can try harder to bring it up into
the initial datalen estimation if we want to see what that would really
look like..

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] avoid multi page allocs in ip_append_data

[Zach Brown]

 For the else clause wouldn't we be counting fragheaderlen twice if
 the multi page if falls through above despite NETIF_F_SG being true?

Looks like, yeah.  I'll fix up and resend.  Thanks for the sharp eyes.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH] avoid multi page allocs in ip_append_data

[Zach Brown]

We've had reports from customers of boxes with a week's uptime and 32k MTUs
getting failed order-3 allocs under:

udp_sendmsg .. ip_append_data .. {sock_wmalloc, sock_alloc_send_skb}

I came up with the following which seems to do the right thing in a trivial
test.  It clamps the size of the first frag's allocation to a single page
if the dev supports _SG.  The rest of the message is copied into page frags
as usual. 

I'm *pretty sure* I got the math right, but this is some twisty, twisty, code.
Please yell at me if I've messed something up.

Is this sort of thing mergable?  I'm happy to tweak the patch into whatever for
is acceptable.  Maybe a knob for the largest order that it should allow would
be appropriate?

- z

Index: 2.6.16-mm2-bigmtu/net/ipv4/ip_output.c
===
--- 2.6.16-mm2-bigmtu.orig/net/ipv4/ip_output.c
+++ 2.6.16-mm2-bigmtu/net/ipv4/ip_output.c
@@ -891,13 +891,26 @@ alloc_new_skb:
datalen = length + fraggap;
if (datalen  mtu - fragheaderlen)
datalen = maxfraglen - fragheaderlen;
+
+   alloclen = fragheaderlen + hh_len + 15;
+
+   /* avoid multi page allocs */
+   if ((rt-u.dst.dev-featuresNETIF_F_SG) 
+   (SKB_DATA_KMALLOC_BYTES(alloclen + datalen)
+PAGE_SIZE)) {
+   datalen = SKB_MAX_ORDER(alloclen, 0);
+   /* watch out for trailer_len */
+   if (datalen == length)
+   datalen -= 2;
+   }
+
fraglen = datalen + fragheaderlen;
 
if ((flags  MSG_MORE)  
!(rt-u.dst.dev-featuresNETIF_F_SG))
-   alloclen = mtu;
+   alloclen += mtu - fragheaderlen;
else
-   alloclen = datalen + fragheaderlen;
+   alloclen += datalen + fragheaderlen;
 
/* The last fragment gets additional space at tail.
 * Note, with MSG_MORE we overallocate on fragments,
@@ -908,15 +921,13 @@ alloc_new_skb:
alloclen += rt-u.dst.trailer_len;
 
if (transhdrlen) {
-   skb = sock_alloc_send_skb(sk, 
-   alloclen + hh_len + 15,
+   skb = sock_alloc_send_skb(sk, alloclen,
(flags  MSG_DONTWAIT), err);
} else {
skb = NULL;
if (atomic_read(sk-sk_wmem_alloc) =
2 * sk-sk_sndbuf)
-   skb = sock_wmalloc(sk, 
-  alloclen + hh_len + 
15, 1,
+   skb = sock_wmalloc(sk, alloclen, 1,
   sk-sk_allocation);
if (unlikely(skb == NULL))
err = -ENOBUFS;
Index: 2.6.16-mm2-bigmtu/include/linux/skbuff.h
===
--- 2.6.16-mm2-bigmtu.orig/include/linux/skbuff.h
+++ 2.6.16-mm2-bigmtu/include/linux/skbuff.h
@@ -39,6 +39,8 @@
 
 #define SKB_DATA_ALIGN(X)  (((X) + (SMP_CACHE_BYTES - 1))  \
 ~(SMP_CACHE_BYTES - 1))
+#define SKB_DATA_KMALLOC_BYTES(X) (SKB_DATA_ALIGN(X) + \
+ sizeof(struct skb_shared_info))
 #define SKB_MAX_ORDER(X, ORDER)(((PAGE_SIZE  (ORDER)) - (X) - \
  sizeof(struct skb_shared_info))  \
  ~(SMP_CACHE_BYTES - 1))
Index: 2.6.16-mm2-bigmtu/net/core/skbuff.c
===
--- 2.6.16-mm2-bigmtu.orig/net/core/skbuff.c
+++ 2.6.16-mm2-bigmtu/net/core/skbuff.c
@@ -148,8 +148,7 @@ struct sk_buff *__alloc_skb(unsigned int
goto out;
 
/* Get the DATA. Size must match skb_add_mtu(). */
-   size = SKB_DATA_ALIGN(size);
-   data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
+   data = kmalloc(SKB_DATA_KMALLOC_BYTES(size), gfp_mask);
if (!data)
goto nodata;
 
@@ -1486,7 +1485,7 @@ void skb_insert(struct sk_buff *old, str
 void skb_add_mtu(int mtu)
 {
/* Must match allocation in alloc_skb */
-   mtu = SKB_DATA_ALIGN(mtu) + sizeof(struct skb_shared_info);
+   mtu = SKB_DATA_KMALLOC_BYTES(mtu);
 
kmem_add_cache_size(mtu);
 }
Index: 

Re: [RFC][PATCH] avoid multi page allocs in ip_append_data

[Zach Brown]

 + /* watch out for trailer_len */
 + if (datalen == length)
 + datalen -= 2;

Hmm, that probably needs to be more careful :/

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ES-API?

[Zach Brown]

 Yes, this is how these standard groups try to force a particular
 approach to high speed networking down everyone's throats.

Yeah, from what I understand there's a lot of RDMA/IB people involved in
that ``ICSC'' thing.

 I think we'll pass on this stuff.

OK, so noted.  Thanks.

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ES-API?

[Zach Brown]

 Also, the following scares me:
 
 This specification has not been verified for avoidance of possible
 third-party proprietary rights. In implementing this specification,
 usual procedures to ensure the respect of possible third-party
 intellectual property rights should be followed.

Scary, indeed.  I'll try and ask people about that bit.

- z

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RP filter support for IPv6, kernel 2.6.15 (fwd)

[Zach Brown]

 @@ -151,6 +152,7 @@
  /* index values for the variables in ipv6_devconf */
  enum {
 DEVCONF_FORWARDING = 0,
 +   DEVCONF_RPFILTER = 0,
 DEVCONF_HOPLIMIT,
 DEVCONF_MTU6,
 DEVCONF_ACCEPT_RA,

That doesn't look quite right :)

- z
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html