Netdev List
 help / color / mirror / Atom feed
* [PATCH net 1/3] tcp_bbr: record "full bw reached" decision in new full_bw_reached bit
From: Neal Cardwell @ 2017-12-07 17:43 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Neal Cardwell
In-Reply-To: <20171207174332.17689-1-ncardwell@google.com>

This commit records the "full bw reached" decision in a new
full_bw_reached bit. This is a pure refactor that does not change the
current behavior, but enables subsequent fixes and improvements.

In particular, this enables simple and clean fixes because the full_bw
and full_bw_cnt can be unconditionally zeroed without worrying about
forgetting that we estimated we filled the pipe in Startup. And it
enables future improvements because multiple code paths can be used
for estimating that we filled the pipe in Startup; any new code paths
only need to set this bit when they think the pipe is full.

Note that this fix intentionally reduces the width of the full_bw_cnt
counter, since we have never used the most significant bit.

Signed-off-by: Neal Cardwell <ncardwell@google.com>
Reviewed-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
---
 net/ipv4/tcp_bbr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
index 69ee877574d0..3089c956b9f9 100644
--- a/net/ipv4/tcp_bbr.c
+++ b/net/ipv4/tcp_bbr.c
@@ -110,7 +110,8 @@ struct bbr {
 	u32	lt_last_lost;	     /* LT intvl start: tp->lost */
 	u32	pacing_gain:10,	/* current gain for setting pacing rate */
 		cwnd_gain:10,	/* current gain for setting cwnd */
-		full_bw_cnt:3,	/* number of rounds without large bw gains */
+		full_bw_reached:1,   /* reached full bw in Startup? */
+		full_bw_cnt:2,	/* number of rounds without large bw gains */
 		cycle_idx:3,	/* current index in pacing_gain cycle array */
 		has_seen_rtt:1, /* have we seen an RTT sample yet? */
 		unused_b:5;
@@ -180,7 +181,7 @@ static bool bbr_full_bw_reached(const struct sock *sk)
 {
 	const struct bbr *bbr = inet_csk_ca(sk);
 
-	return bbr->full_bw_cnt >= bbr_full_bw_cnt;
+	return bbr->full_bw_reached;
 }
 
 /* Return the windowed max recent bandwidth sample, in pkts/uS << BW_SCALE. */
@@ -717,6 +718,7 @@ static void bbr_check_full_bw_reached(struct sock *sk,
 		return;
 	}
 	++bbr->full_bw_cnt;
+	bbr->full_bw_reached = bbr->full_bw_cnt >= bbr_full_bw_cnt;
 }
 
 /* If pipe is probably full, drain the queue and then enter steady-state. */
@@ -850,6 +852,7 @@ static void bbr_init(struct sock *sk)
 	bbr->restore_cwnd = 0;
 	bbr->round_start = 0;
 	bbr->idle_restart = 0;
+	bbr->full_bw_reached = 0;
 	bbr->full_bw = 0;
 	bbr->full_bw_cnt = 0;
 	bbr->cycle_mstamp = 0;
-- 
2.15.1.424.g9478a66081-goog

^ permalink raw reply related

* [PATCH net 0/3] TCP BBR sampling fixes for loss recovery undo
From: Neal Cardwell @ 2017-12-07 17:43 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Neal Cardwell

This patch series has a few minor bug fixes for cases where spurious
loss recoveries can trick BBR estimators into estimating that the
available bandwidth is much lower than the true available bandwidth.
In both cases the fix here is to just reset the estimator upon loss
recovery undo.

Neal Cardwell (3):
  tcp_bbr: record "full bw reached" decision in new full_bw_reached bit
  tcp_bbr: reset full pipe detection on loss recovery undo
  tcp_bbr: reset long-term bandwidth sampling on loss recovery undo

 net/ipv4/tcp_bbr.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.15.1.424.g9478a66081-goog

^ permalink raw reply

* Re: [PATCH v4.1] phylib: Add device reset GPIO support
From: Sergei Shtylyov @ 2017-12-07 17:40 UTC (permalink / raw)
  To: Geert Uytterhoeven, David S . Miller, Andrew Lunn,
	Florian Fainelli, Simon Horman, Magnus Damm
  Cc: Rob Herring, Mark Rutland, Nicolas Ferre, Richard Leitner,
	netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <69fe9a6d-4039-6f80-b80d-7dc0de31e5bf-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>

On 12/07/2017 08:20 PM, Sergei Shtylyov wrote:

>> The PHY devices sometimes do have their reset signal (maybe even power
>> supply?) tied to some GPIO and sometimes it also does happen that a boot
>> loader does not leave it deasserted. So far this issue has been attacked
>> from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
>> the GPIO in question; that solution, when applied to the device trees, led
>> to adding the PHY reset GPIO properties to the MAC device node, with one
>> exception: Cadence MACB driver which could handle the "reset-gpios" prop
>> in a PHY device subnode. I believe that the correct approach is to teach
>> the 'phylib' to get the MDIO device reset GPIO from the device tree node
>> corresponding to this device -- which this patch is doing...
>>
>> Note that I had to modify the AT803x PHY driver as it would stop working
>> otherwise -- it made use of the reset GPIO for its own purposes...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> [geert: Propagate actual errors from fwnode_get_named_gpiod()]
>> [geert: Avoid destroying initial setup]
>> [geert: Consolidate GPIO descriptor acquiring code]
>> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> [...]
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index 2df7b62c1a36811e..8f8b7747c54bc478 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
> [...]
>> @@ -48,9 +49,26 @@
>>   int mdiobus_register_device(struct mdio_device *mdiodev)
>>   {
>> +    struct gpio_desc *gpiod = NULL;
>> +
>>       if (mdiodev->bus->mdio_map[mdiodev->addr])
>>           return -EBUSY;
>> +    /* Deassert the optional reset signal */
> 
>     Umm, but why deassert it here for such a short time?
> 
>> +    if (mdiodev->dev.of_node)
>> +        gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
>> +                           "reset-gpios", 0, GPIOD_OUT_LOW,
>> +                           "PHY reset");
>> +    if (PTR_ERR(gpiod) == -ENOENT)
>> +        gpiod = NULL;
>> +    else if (IS_ERR(gpiod))
>> +        return PTR_ERR(gpiod);
> 
>     Hm, returning on error with reset deasserted?

    Oops, error means we couldn't drive the GPIO at all...
    The 1st question remains though...

[...]

MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* David Lamparter
From: Алексей Болдырев @ 2017-12-07 17:30 UTC (permalink / raw)
  To: netdev

And what happened to him?

^ permalink raw reply

* Re: [Intel-wired-lan] [next-queue 07/10] ixgbe: process the Rx ipsec offload
From: Alexander Duyck @ 2017-12-07 17:20 UTC (permalink / raw)
  To: Shannon Nelson
  Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
	Netdev
In-Reply-To: <eb6a3c29-ba18-ec42-0a95-76f0af0cd1b4@oracle.com>

On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> On 12/5/2017 9:40 AM, Alexander Duyck wrote:
>>
>> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
>> <shannon.nelson@oracle.com> wrote:
>>>
>>> If the chip sees and decrypts an ipsec offload, set up the skb
>>> sp pointer with the ralated SA info.  Since the chip is rude
>>> enough to keep to itself the table index it used for the
>>> decryption, we have to do our own table lookup, using the
>>> hash for speed.
>>>
>>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>>> ---
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe.h       |  6 ++
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 89
>>> ++++++++++++++++++++++++++
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  3 +
>>>   3 files changed, 98 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> index 7e8bca7..77f07dc 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> @@ -1009,9 +1009,15 @@ s32 ixgbe_negotiate_fc(struct ixgbe_hw *hw, u32
>>> adv_reg, u32 lp_reg,
>>>                         u32 adv_sym, u32 adv_asm, u32 lp_sym, u32
>>> lp_asm);
>>>   #ifdef CONFIG_XFRM_OFFLOAD
>>>   void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter);
>>> +void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>>> +                   union ixgbe_adv_rx_desc *rx_desc,
>>> +                   struct sk_buff *skb);
>>>   void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
>>>   #else
>>>   static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter
>>> *adapter) { };
>>> +static inline void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>>> +                                 union ixgbe_adv_rx_desc *rx_desc,
>>> +                                 struct sk_buff *skb) { };
>>>   static inline void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter) {
>>> };
>>>   #endif /* CONFIG_XFRM_OFFLOAD */
>>>   #endif /* _IXGBE_H_ */
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> index b93ee7f..fd06d9b 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> @@ -379,6 +379,35 @@ static int ixgbe_ipsec_find_empty_idx(struct
>>> ixgbe_ipsec *ipsec, bool rxtable)
>>>   }
>>>
>>>   /**
>>> + * ixgbe_ipsec_find_rx_state - find the state that matches
>>> + * @ipsec: pointer to ipsec struct
>>> + * @daddr: inbound address to match
>>> + * @proto: protocol to match
>>> + * @spi: SPI to match
>>> + *
>>> + * Returns a pointer to the matching SA state information
>>> + **/
>>> +static struct xfrm_state *ixgbe_ipsec_find_rx_state(struct ixgbe_ipsec
>>> *ipsec,
>>> +                                                   __be32 daddr, u8
>>> proto,
>>> +                                                   __be32 spi)
>>> +{
>>> +       struct rx_sa *rsa;
>>> +       struct xfrm_state *ret = NULL;
>>> +
>>> +       rcu_read_lock();
>>> +       hash_for_each_possible_rcu(ipsec->rx_sa_list, rsa, hlist, spi)
>>> +               if (spi == rsa->xs->id.spi &&
>>> +                   daddr == rsa->xs->id.daddr.a4 &&
>>> +                   proto == rsa->xs->id.proto) {
>>> +                       ret = rsa->xs;
>>> +                       xfrm_state_hold(ret);
>>> +                       break;
>>> +               }
>>> +       rcu_read_unlock();
>>> +       return ret;
>>> +}
>>> +
>>
>>
>> You need to choose a bucket, not just walk through all buckets.
>
>
> I may be wrong, but I believe that is what is happening here, where the spi
> is the hash key.  As the function description says "iterate over all
> possible objects hashing to the same bucket".  Besides, I basically cribbed
> this directly from our Mellanox friends (thanks!).

You're right. I misread that.

^ permalink raw reply

* Re: [PATCH v5 2/2] sock: Move the socket inuse to namespace.
From: Eric Dumazet @ 2017-12-07 17:20 UTC (permalink / raw)
  To: Tonghao Zhang, davem, xiyou.wangcong, edumazet, willemb; +Cc: netdev
In-Reply-To: <1512665148-2413-2-git-send-email-xiangxia.m.yue@gmail.com>

On Thu, 2017-12-07 at 08:45 -0800, Tonghao Zhang wrote:
> In some case, we want to know how many sockets are in use in
> different _net_ namespaces. It's a key resource metric.
> 

...

> +static void sock_inuse_add(struct net *net, int val)
> +{
> +	if (net->core.prot_inuse)
> +		this_cpu_add(*net->core.sock_inuse, val);
> +}

This is very confusing.

Why testing net->core.prot_inuse for NULL is needed at all ?

Why not testing net->core.sock_inuse instead ?

^ permalink raw reply

* Re: [PATCH v4.1] phylib: Add device reset GPIO support
From: Sergei Shtylyov @ 2017-12-07 17:20 UTC (permalink / raw)
  To: Geert Uytterhoeven, David S . Miller, Andrew Lunn,
	Florian Fainelli, Simon Horman, Magnus Damm
  Cc: Rob Herring, Mark Rutland, Nicolas Ferre, Richard Leitner, netdev,
	devicetree, linux-renesas-soc, linux-kernel
In-Reply-To: <1512390905-28094-1-git-send-email-geert+renesas@glider.be>

Hello!

On 12/04/2017 03:35 PM, Geert Uytterhoeven wrote:

> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> The PHY devices sometimes do have their reset signal (maybe even power
> supply?) tied to some GPIO and sometimes it also does happen that a boot
> loader does not leave it deasserted. So far this issue has been attacked
> from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
> the GPIO in question; that solution, when applied to the device trees, led
> to adding the PHY reset GPIO properties to the MAC device node, with one
> exception: Cadence MACB driver which could handle the "reset-gpios" prop
> in a PHY device subnode. I believe that the correct approach is to teach
> the 'phylib' to get the MDIO device reset GPIO from the device tree node
> corresponding to this device -- which this patch is doing...
> 
> Note that I had to modify the AT803x PHY driver as it would stop working
> otherwise -- it made use of the reset GPIO for its own purposes...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Acked-by: Rob Herring <robh@kernel.org>
> [geert: Propagate actual errors from fwnode_get_named_gpiod()]
> [geert: Avoid destroying initial setup]
> [geert: Consolidate GPIO descriptor acquiring code]
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
[...]
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 2df7b62c1a36811e..8f8b7747c54bc478 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
[...]
> @@ -48,9 +49,26 @@
>   
>   int mdiobus_register_device(struct mdio_device *mdiodev)
>   {
> +	struct gpio_desc *gpiod = NULL;
> +
>   	if (mdiodev->bus->mdio_map[mdiodev->addr])
>   		return -EBUSY;
>   
> +	/* Deassert the optional reset signal */

    Umm, but why deassert it here for such a short time?

> +	if (mdiodev->dev.of_node)
> +		gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
> +					       "reset-gpios", 0, GPIOD_OUT_LOW,
> +					       "PHY reset");
> +	if (PTR_ERR(gpiod) == -ENOENT)
> +		gpiod = NULL;
> +	else if (IS_ERR(gpiod))
> +		return PTR_ERR(gpiod);

    Hm, returning on error with reset deasserted?

> +
> +	mdiodev->reset = gpiod;
> +
> +	/* Assert the reset signal again */
> +	mdio_device_reset(mdiodev, 1);
> +
>   	mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev;
>   
>   	return 0;
[...]

MBR, Sergei

^ permalink raw reply

* [PATCH net] sfc: pass valid pointers from efx_enqueue_unwind
From: Bert Kenward @ 2017-12-07 17:18 UTC (permalink / raw)
  To: Dave Miller; +Cc: netdev, linux-net-drivers, Jarod Wilson

The bytes_compl and pkts_compl pointers passed to efx_dequeue_buffers
cannot be NULL. Add a paranoid warning to check this condition and fix
the one case where they were NULL.

efx_enqueue_unwind() is called very rarely, during error handling.
Without this fix it would fail with a NULL pointer dereference in
efx_dequeue_buffer, with efx_enqueue_skb in the call stack.

Fixes: e9117e5099ea ("sfc: Firmware-Assisted TSO version 2")
Reported-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Bert Kenward <bkenward@solarflare.com>
---
 drivers/net/ethernet/sfc/tx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 0ea7e16f2e6e..9937a2450e57 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -77,6 +77,7 @@ static void efx_dequeue_buffer(struct efx_tx_queue *tx_queue,
 	}
 
 	if (buffer->flags & EFX_TX_BUF_SKB) {
+		EFX_WARN_ON_PARANOID(!pkts_compl || !bytes_compl);
 		(*pkts_compl)++;
 		(*bytes_compl) += buffer->skb->len;
 		dev_consume_skb_any((struct sk_buff *)buffer->skb);
@@ -426,12 +427,14 @@ static int efx_tx_map_data(struct efx_tx_queue *tx_queue, struct sk_buff *skb,
 static void efx_enqueue_unwind(struct efx_tx_queue *tx_queue)
 {
 	struct efx_tx_buffer *buffer;
+	unsigned int bytes_compl = 0;
+	unsigned int pkts_compl = 0;
 
 	/* Work backwards until we hit the original insert pointer value */
 	while (tx_queue->insert_count != tx_queue->write_count) {
 		--tx_queue->insert_count;
 		buffer = __efx_tx_queue_get_insert_buffer(tx_queue);
-		efx_dequeue_buffer(tx_queue, buffer, NULL, NULL);
+		efx_dequeue_buffer(tx_queue, buffer, &pkts_compl, &bytes_compl);
 	}
 }
 
-- 
2.13.6

^ permalink raw reply related

* Re: [Intel-wired-lan] [next-queue 06/10] ixgbe: restore offloaded SAs after a reset
From: Alexander Duyck @ 2017-12-07 17:16 UTC (permalink / raw)
  To: Shannon Nelson
  Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
	Netdev
In-Reply-To: <90efb41d-2cab-f623-8b3d-318f3ad6e135@oracle.com>

On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> On 12/5/2017 9:30 AM, Alexander Duyck wrote:
>>
>> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
>> <shannon.nelson@oracle.com> wrote:
>>>
>>> On a chip reset most of the table contents are lost, so must be
>>> restored.  This scans the driver's ipsec tables and restores both
>>> the filled and empty table slots to their pre-reset values.
>>>
>>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>>> ---
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe.h       |  2 +
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 53
>>> ++++++++++++++++++++++++++
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  1 +
>>>   3 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> index 9487750..7e8bca7 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>> @@ -1009,7 +1009,9 @@ s32 ixgbe_negotiate_fc(struct ixgbe_hw *hw, u32
>>> adv_reg, u32 lp_reg,
>>>                         u32 adv_sym, u32 adv_asm, u32 lp_sym, u32
>>> lp_asm);
>>>   #ifdef CONFIG_XFRM_OFFLOAD
>>>   void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter);
>>> +void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
>>>   #else
>>>   static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter
>>> *adapter) { };
>>> +static inline void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter) {
>>> };
>>>   #endif /* CONFIG_XFRM_OFFLOAD */
>>>   #endif /* _IXGBE_H_ */
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> index 7b01d92..b93ee7f 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> @@ -292,6 +292,59 @@ static void ixgbe_ipsec_start_engine(struct
>>> ixgbe_adapter *adapter)
>>>   }
>>>
>>>   /**
>>> + * ixgbe_ipsec_restore - restore the ipsec HW settings after a reset
>>> + * @adapter: board private structure
>>> + **/
>>> +void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter)
>>> +{
>>> +       struct ixgbe_ipsec *ipsec = adapter->ipsec;
>>> +       struct ixgbe_hw *hw = &adapter->hw;
>>> +       u32 zbuf[4] = {0, 0, 0, 0};
>>
>>
>> zbuf should be a static const.
>
>
> Yep
>
>>
>>> +       int i;
>>> +
>>> +       if (!(adapter->flags2 & IXGBE_FLAG2_IPSEC_ENABLED))
>>> +               return;
>>> +
>>> +       /* clean up the engine settings */
>>> +       ixgbe_ipsec_stop_engine(adapter);
>>> +
>>> +       /* start the engine */
>>> +       ixgbe_ipsec_start_engine(adapter);
>>> +
>>> +       /* reload the IP addrs */
>>> +       for (i = 0; i < IXGBE_IPSEC_MAX_RX_IP_COUNT; i++) {
>>> +               struct rx_ip_sa *ipsa = &ipsec->ip_tbl[i];
>>> +
>>> +               if (ipsa->used)
>>> +                       ixgbe_ipsec_set_rx_ip(hw, i, ipsa->ipaddr);
>>> +               else
>>> +                       ixgbe_ipsec_set_rx_ip(hw, i, zbuf);
>>
>>
>> If we are doing a restore do we actually need to write the zero
>> values? If we did a reset I thought you had a function that was going
>> through and zeroing everything out. If so this now becomes redundant.
>
>
> Currently ixgbe_ipsec_clear_hw_tables() only gets run at at probe.  It
> should probably get run at remove as well.  Doing this is a bit of safety
> paranoia, and making sure the CAM memory structures that don't get cleared
> on reset have exactly what I expect in them.

You might just move ixgbe_ipsec_clear_hw_tables into the rest logic
itself. Then it covers all cases where you would be resetting the
hardware and expecting a consistent state. It will mean writing some
registers twice during the reset but it is probably better just to
make certain everything stays in a known good state after a reset.

>>
>>> +       }
>>> +
>>> +       /* reload the Rx keys */
>>> +       for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>>> +               struct rx_sa *rsa = &ipsec->rx_tbl[i];
>>> +
>>> +               if (rsa->used)
>>> +                       ixgbe_ipsec_set_rx_sa(hw, i, rsa->xs->id.spi,
>>> +                                             rsa->key, rsa->salt,
>>> +                                             rsa->mode, rsa->iptbl_ind);
>>> +               else
>>> +                       ixgbe_ipsec_set_rx_sa(hw, i, 0, zbuf, 0, 0, 0);
>>
>>
>> same here
>>
>>> +       }
>>> +
>>> +       /* reload the Tx keys */
>>> +       for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>>> +               struct tx_sa *tsa = &ipsec->tx_tbl[i];
>>> +
>>> +               if (tsa->used)
>>> +                       ixgbe_ipsec_set_tx_sa(hw, i, tsa->key,
>>> tsa->salt);
>>> +               else
>>> +                       ixgbe_ipsec_set_tx_sa(hw, i, zbuf, 0);
>>
>>
>> and here
>>
>>> +       }
>>> +}
>>> +
>>> +/**
>>>    * ixgbe_ipsec_find_empty_idx - find the first unused security
>>> parameter index
>>>    * @ipsec: pointer to ipsec struct
>>>    * @rxtable: true if we need to look in the Rx table
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 01fd89b..6eabf92 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -5347,6 +5347,7 @@ static void ixgbe_configure(struct ixgbe_adapter
>>> *adapter)
>>>
>>>          ixgbe_set_rx_mode(adapter->netdev);
>>>          ixgbe_restore_vlan(adapter);
>>> +       ixgbe_ipsec_restore(adapter);
>>>
>>>          switch (hw->mac.type) {
>>>          case ixgbe_mac_82599EB:
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> Intel-wired-lan mailing list
>>> Intel-wired-lan@osuosl.org
>>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply

* Re: [Intel-wired-lan] [next-queue 02/10] ixgbe: add ipsec register access routines
From: Shannon Nelson @ 2017-12-07 17:03 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
	Netdev
In-Reply-To: <CAKgT0Uci9TLVcaYzfGr=yVXDhVc+sbQr01cUeWiypN7-7faYmw@mail.gmail.com>

On 12/7/2017 8:02 AM, Alexander Duyck wrote:
> On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
> <shannon.nelson@oracle.com> wrote:
>> Thanks, Alex, for your detailed comments, I do appreciate the time and
>> thought you put into them.
>>
>> Responses below...
>>
>> sln
>>
>> On 12/5/2017 8:56 AM, Alexander Duyck wrote:

<snip>

>>>> +       reg = IXGBE_READ_REG(hw, IXGBE_IPSRXIDX);
>>>> +       reg &= IXGBE_RXTXIDX_IPS_EN;
>>>> +       reg |= tbl | idx << 3 | IXGBE_RXTXIDX_IDX_WRITE;
>>>> +       IXGBE_WRITE_REG(hw, IXGBE_IPSRXIDX, reg);
>>>> +       IXGBE_WRITE_FLUSH(hw);
>>>> +}
>>>> +
>>>
>>>
>>> The Rx version of this gets a bit trickier since the datasheet
>>> actually indicates there are a few different types of tables that can
>>> be indexed via this. Also why is the tbl value not being shifted? It
>>> seems like it should be shifted by 1 to avoid overwriting the IPS_EN
>>> bit. Really I would like to see the tbl value converted to an enum and
>>> shifted by 1 in order to generate the table reference.
>>
>>
>> I would have done this, but we can't use an enum shifted bit because the
>> field values are 01, 10, and 11.  I used the direct 2, 4, and 6 values
>> rather than shifting by one, but I can reset them and shift by 1.
> 
> I didn't mean 1 << enum I was referring to enum << 1. Right now you
> can be given a table value of 3 if somebody incorrectly used the
> function and the side effect is that it overwrites the enable bit.

Okay, sure, that makes sense.

> 
>>>

<snip>

>>>> +       /* store the SPI (in bigendian) and IPidx */
>>>> +       IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, spi);
>>>> +       IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, ip_idx);
>>>> +       IXGBE_WRITE_FLUSH(hw);
>>>> +
>>>> +       ixgbe_ipsec_set_rx_item(hw, idx, IXGBE_RXIDX_TBL_SPI);
>>>> +
>>>> +       /* store the key, salt, and mode */
>>>> +       for (i = 0; i < 4; i++)
>>>> +               IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(i),
>>>> cpu_to_be32(key[3-i]));
>>>> +       IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, cpu_to_be32(salt));
>>>> +       IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, mode);
>>>> +       IXGBE_WRITE_FLUSH(hw);
>>>> +
>>>> +       ixgbe_ipsec_set_rx_item(hw, idx, IXGBE_RXIDX_TBL_KEY);
>>>> +}
>>>
>>>
>>> Is there any reason why you could write the SPI, key, salt, and mode,
>>> then flush, and trigger the writes via the IPSRXIDX? Just wondering
>>> since it would likely save you a few cycles avoiding PCIe bus stalls.
>>
>>
>> See note above about religiously flushing everything to make a persnickety
>> chip work.
> 
> I get the flushing. What I am saying is that as far as I can tell the
> SPI, salt, and mode don't overlap so you could update all 3, flush,
> and then call set_rx_item twice.

I'll check that for here and a possibly a couple other places.

> 
>>>
>>>
>>>> +
>>>> +/**
>>>> + * ixgbe_ipsec_set_rx_ip - set up the register bits to save SA IP addr
>>>> info
>>>> + * @hw: hw specific details
>>>> + * @idx: register index to write
>>>> + * @addr: IP address byte array
>>>> + **/
>>>> +static void ixgbe_ipsec_set_rx_ip(struct ixgbe_hw *hw, u16 idx, u32
>>>> addr[])
>>>> +{
>>>> +       int i;
>>>> +
>>>> +       /* store the ip address */
>>>> +       for (i = 0; i < 4; i++)
>>>> +               IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(i), addr[i]);
>>>> +       IXGBE_WRITE_FLUSH(hw);
>>>> +
>>>> +       ixgbe_ipsec_set_rx_item(hw, idx, IXGBE_RXIDX_TBL_IP);
>>>> +}
>>>> +
>>>
>>>
>>> This piece is kind of confusing. I would suggest storing the address
>>> as a __be32 pointer instead of a u32 array. That way you start with
>>> either an IPv6 or an IPv4 address at offset 0 instead of the way the
>>> hardware is defined which has you writing it at either 0 or 3
>>> depending on if the address is IPv6 or IPv4.
>>
>>
>> Using a __be32 rather than u32 is fine here, it doesn't make much
>> difference.
>>
>> If I understand your suggestion correctly, we would also need an additional
>> function parameter to tell us if we were pointing to an ipv6 or ipv4
>> address.  Since the driver's SW tables are modeling the HW, I think it is
>> simpler to leave it in the array.
> 
> Actually I am not too concerned about needing a flag, but the __be32
> usage addresses another problem. If I am not mistaken in order to
> store an IPv6 value you will have to write addr[3] to IPADDR(0) and so
> forth since the hardware is storing the IPv6 address as little endian.
> So if you store the IPv4 address in addr[0] as a __be32 value and
> leave the rest as zero you should get the correct ordering in either
> setup when you store either IPv6 or IPv4 values.

The datasheet says
   n=0 contains the MSB for an IPv6 IP Address.
   n=3 contains an IPv4 IP Address or the LSB for an IPv6 IP Address.
If the ipv6 address is handed to us in bigendian, then I think we're okay.

Obviously this is something that will get tested when I get around to 
fixing up support for ipv6 in the future, and perhaps I'll be surprised.

sln

^ permalink raw reply

* Re: [PATCH v4 2/2] sock: Move the socket inuse to namespace.
From: Tonghao Zhang @ 2017-12-07 16:56 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Kernel Network Developers, Cong Wang
In-Reply-To: <20171125.235302.781017870270039369.davem@davemloft.net>

On Sat, Nov 25, 2017 at 10:53 PM, David Miller <davem@davemloft.net> wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Date: Wed, 22 Nov 2017 17:51:25 -0800
>
>> This patch add a member in struct netns_core. And this is
>> a counter for socket_inuse in the _net_ namespace. The patch
>> will add/sub counter in the sk_alloc or sk_free. Because socket and
>> sock is in pair. It's a easy way to maintain the code and help
>> developer to review. More important, it avoids holding the _net_
>> namespace again.
>>
>> Signed-off-by: Martin Zhang <zhangjunweimartin@didichuxing.com>
>> Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
>
> First, it is extremely unclear why this is better.  You do not explain
> the reason at all.
Sorry, I am so later to reply. I send  the v5 and the comment may
explain the reason.

> Second:
>
>> @@ -2646,17 +2646,8 @@ static int __init sock_init(void)
>>  #ifdef CONFIG_PROC_FS
>>  void socket_seq_show(struct seq_file *seq)
>>  {
>> -     int cpu;
>> -     int counter = 0;
>> -
>> -     for_each_possible_cpu(cpu)
>> -         counter += per_cpu(sockets_in_use, cpu);
>> -
>> -     /* It can be negative, by the way. 8) */
>> -     if (counter < 0)
>> -             counter = 0;
>> -
>> -     seq_printf(seq, "sockets: used %d\n", counter);
>
> You've deleted the only use of "sockets_in_use" but you have not
> removed that per-cpu variable and it's maintainence.
So sorry :(

> But do not even bother fixing this if you cannot explain properly
> why these changes are an improvement.  I do not understant it,
> and until I do I cannot consider these changes seriously.
Yes, I should check the patch carefully and add more detail. I may do it better.
The v5 patch may be useful. Thanks so much.

> Thank you.

^ permalink raw reply

* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-12-07 16:53 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Jakub Kicinski, Hannes Frederic Sowa, achiad shochat,
	Sridhar Samudrala, virtualization, Shannon Nelson, Achiad,
	Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
	Or Gerlitz
In-Reply-To: <CAKgT0UfC2o4P8DeRHnnPV9fvUzJYbkCKVHr=zHOfbEQdLn=3nA@mail.gmail.com>

On Thu, Dec 07, 2017 at 08:45:33AM -0800, Alexander Duyck wrote:
> As far as indicating that the interfaces are meant to be enslaved I
> wonder if we couldn't look at tweaking the PCI layout of the guest and
> use that to indicate that a given set of interfaces are meant to be
> bonded. For example the VFs are all meant to work as a part of a
> multi-function device. What if we were to make virtio-net function 0
> of a PCI/PCIe device, and then place any direct assigned VFs that are
> meant to be a part of the bond in functions 1-7 of the device? Then it
> isn't too far off from the model we have on the host where if the VF
> goes away we would expect to see the traffic on the PF that is usually
> occupying function 0 of a given device.

This pretty much precludes removing them with hotplug.

But as long as we are happy to limit this to pci devices,
maybe we should put them behind a pci bridge.

All devices behind a bridge would be assumed to have
the same backend.

QEMU has pci-bridge-seat which we could reuse for this -
just need to build a similar pci express bridge.

-- 
MST

^ permalink raw reply

* [PATCH v5 2/2] sock: Move the socket inuse to namespace.
From: Tonghao Zhang @ 2017-12-07 16:45 UTC (permalink / raw)
  To: davem, xiyou.wangcong, edumazet, willemb; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1512665148-2413-1-git-send-email-xiangxia.m.yue@gmail.com>

In some case, we want to know how many sockets are in use in
different _net_ namespaces. It's a key resource metric.

This patch add a member in struct netns_core. This is a counter
for socket-inuse in the _net_ namespace. The patch will add/sub
counter in the sk_alloc, sk_clone_lock and __sk_free.

The main reasons for doing this are that:

1. When linux calls the 'do_exit' for process to exit, the functions
'exit_task_namespaces' and 'exit_task_work' will be called sequentially.
'exit_task_namespaces' may have destroyed the _net_ namespace, but
'sock_release' called in 'exit_task_work' may use the _net_ namespace
if we counter the socket-inuse in sock_release.

2. socket and sock are in pair. More important, sock holds the _net_
namespace. We counter the socket-inuse in sock, for avoiding holding
_net_ namespace again in socket. It's a easy way to maintain the code.

Signed-off-by: Martin Zhang <zhangjunweimartin@didichuxing.com>
Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
---
 include/net/netns/core.h |  1 +
 include/net/sock.h       |  1 +
 net/core/sock.c          | 52 ++++++++++++++++++++++++++++++++++++++++++++++--
 net/socket.c             | 21 ++-----------------
 4 files changed, 54 insertions(+), 21 deletions(-)

diff --git a/include/net/netns/core.h b/include/net/netns/core.h
index 45cfb5d..d1b4748f 100644
--- a/include/net/netns/core.h
+++ b/include/net/netns/core.h
@@ -11,6 +11,7 @@ struct netns_core {
 
 	int	sysctl_somaxconn;
 
+	int __percpu *sock_inuse;
 	struct prot_inuse __percpu *prot_inuse;
 };
 
diff --git a/include/net/sock.h b/include/net/sock.h
index 79e1a2c..0809b31 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1266,6 +1266,7 @@ static inline void sk_sockets_allocated_inc(struct sock *sk)
 /* Called with local bh disabled */
 void sock_prot_inuse_add(struct net *net, struct proto *prot, int inc);
 int sock_prot_inuse_get(struct net *net, struct proto *proto);
+int sock_inuse_get(struct net *net);
 #else
 static inline void sock_prot_inuse_add(struct net *net, struct proto *prot,
 		int inc)
diff --git a/net/core/sock.c b/net/core/sock.c
index c2dd2d3..a11680a 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -145,6 +145,8 @@
 static DEFINE_MUTEX(proto_list_mutex);
 static LIST_HEAD(proto_list);
 
+static void sock_inuse_add(struct net *net, int val);
+
 /**
  * sk_ns_capable - General socket capability test
  * @sk: Socket to use a capability on or through
@@ -1534,6 +1536,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 		if (likely(sk->sk_net_refcnt))
 			get_net(net);
 		sock_net_set(sk, net);
+		sock_inuse_add(net, 1);
 		refcount_set(&sk->sk_wmem_alloc, 1);
 
 		mem_cgroup_sk_alloc(sk);
@@ -1595,6 +1598,8 @@ void sk_destruct(struct sock *sk)
 
 static void __sk_free(struct sock *sk)
 {
+	sock_inuse_add(sock_net(sk), -1);
+
 	if (unlikely(sock_diag_has_destroy_listeners(sk) && sk->sk_net_refcnt))
 		sock_diag_broadcast_destroy(sk);
 	else
@@ -1716,6 +1721,7 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
 		newsk->sk_priority = 0;
 		newsk->sk_incoming_cpu = raw_smp_processor_id();
 		atomic64_set(&newsk->sk_cookie, 0);
+		sock_inuse_add(sock_net(newsk), 1);
 
 		/*
 		 * Before updating sk_refcnt, we must commit prior changes to memory
@@ -3061,15 +3067,53 @@ int sock_prot_inuse_get(struct net *net, struct proto *prot)
 }
 EXPORT_SYMBOL_GPL(sock_prot_inuse_get);
 
+static void sock_inuse_add(struct net *net, int val)
+{
+	if (net->core.prot_inuse)
+		this_cpu_add(*net->core.sock_inuse, val);
+}
+
+int sock_inuse_get(struct net *net)
+{
+	int cpu, res = 0;
+
+	if (!net->core.prot_inuse)
+		return 0;
+
+	for_each_possible_cpu(cpu)
+		res += *per_cpu_ptr(net->core.sock_inuse, cpu);
+
+	return res >= 0 ? res : 0;
+}
+EXPORT_SYMBOL_GPL(sock_inuse_get);
+
 static int __net_init sock_inuse_init_net(struct net *net)
 {
 	net->core.prot_inuse = alloc_percpu(struct prot_inuse);
-	return net->core.prot_inuse ? 0 : -ENOMEM;
+	if (!net->core.prot_inuse)
+		return -ENOMEM;
+
+	net->core.sock_inuse = alloc_percpu(int);
+	if (!net->core.sock_inuse)
+		goto out;
+
+	return 0;
+out:
+	free_percpu(net->core.prot_inuse);
+	return -ENOMEM;
 }
 
 static void __net_exit sock_inuse_exit_net(struct net *net)
 {
-	free_percpu(net->core.prot_inuse);
+	if (net->core.prot_inuse) {
+		free_percpu(net->core.prot_inuse);
+		net->core.prot_inuse = NULL;
+	}
+
+	if (net->core.sock_inuse) {
+		free_percpu(net->core.sock_inuse);
+		net->core.prot_inuse = NULL;
+	}
 }
 
 static struct pernet_operations net_inuse_ops = {
@@ -3112,6 +3156,10 @@ static inline void assign_proto_idx(struct proto *prot)
 static inline void release_proto_idx(struct proto *prot)
 {
 }
+
+static void sock_inuse_add(struct net *net, int val)
+{
+}
 #endif
 
 static void req_prot_cleanup(struct request_sock_ops *rsk_prot)
diff --git a/net/socket.c b/net/socket.c
index 42d8e9c..183de8f01 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -163,12 +163,6 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
 static const struct net_proto_family __rcu *net_families[NPROTO] __read_mostly;
 
 /*
- *	Statistics counters of the socket lists
- */
-
-static DEFINE_PER_CPU(int, sockets_in_use);
-
-/*
  * Support routines.
  * Move socket addresses back and forth across the kernel/user
  * divide and look after the messy bits.
@@ -574,7 +568,6 @@ struct socket *sock_alloc(void)
 	inode->i_gid = current_fsgid();
 	inode->i_op = &sockfs_inode_ops;
 
-	this_cpu_add(sockets_in_use, 1);
 	return sock;
 }
 EXPORT_SYMBOL(sock_alloc);
@@ -601,7 +594,6 @@ void sock_release(struct socket *sock)
 	if (rcu_dereference_protected(sock->wq, 1)->fasync_list)
 		pr_err("%s: fasync list not empty!\n", __func__);
 
-	this_cpu_sub(sockets_in_use, 1);
 	if (!sock->file) {
 		iput(SOCK_INODE(sock));
 		return;
@@ -2644,17 +2636,8 @@ static int __init sock_init(void)
 #ifdef CONFIG_PROC_FS
 void socket_seq_show(struct seq_file *seq)
 {
-	int cpu;
-	int counter = 0;
-
-	for_each_possible_cpu(cpu)
-	    counter += per_cpu(sockets_in_use, cpu);
-
-	/* It can be negative, by the way. 8) */
-	if (counter < 0)
-		counter = 0;
-
-	seq_printf(seq, "sockets: used %d\n", counter);
+	seq_printf(seq, "sockets: used %d\n",
+		   sock_inuse_get(seq->private));
 }
 #endif				/* CONFIG_PROC_FS */
 
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH v5 1/2] sock: Change the netns_core member name.
From: Tonghao Zhang @ 2017-12-07 16:45 UTC (permalink / raw)
  To: davem, xiyou.wangcong, edumazet, willemb; +Cc: netdev, Tonghao Zhang

Change the member name will make the code more readable.
This patch will be used in next patch.

Signed-off-by: Martin Zhang <zhangjunweimartin@didichuxing.com>
Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
---
 include/net/netns/core.h |  2 +-
 net/core/sock.c          | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/net/netns/core.h b/include/net/netns/core.h
index 0ad4d0c..45cfb5d 100644
--- a/include/net/netns/core.h
+++ b/include/net/netns/core.h
@@ -11,7 +11,7 @@ struct netns_core {
 
 	int	sysctl_somaxconn;
 
-	struct prot_inuse __percpu *inuse;
+	struct prot_inuse __percpu *prot_inuse;
 };
 
 #endif
diff --git a/net/core/sock.c b/net/core/sock.c
index c0b5b2f..c2dd2d3 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3045,7 +3045,7 @@ struct prot_inuse {
 
 void sock_prot_inuse_add(struct net *net, struct proto *prot, int val)
 {
-	__this_cpu_add(net->core.inuse->val[prot->inuse_idx], val);
+	__this_cpu_add(net->core.prot_inuse->val[prot->inuse_idx], val);
 }
 EXPORT_SYMBOL_GPL(sock_prot_inuse_add);
 
@@ -3055,7 +3055,7 @@ int sock_prot_inuse_get(struct net *net, struct proto *prot)
 	int res = 0;
 
 	for_each_possible_cpu(cpu)
-		res += per_cpu_ptr(net->core.inuse, cpu)->val[idx];
+		res += per_cpu_ptr(net->core.prot_inuse, cpu)->val[idx];
 
 	return res >= 0 ? res : 0;
 }
@@ -3063,13 +3063,13 @@ int sock_prot_inuse_get(struct net *net, struct proto *prot)
 
 static int __net_init sock_inuse_init_net(struct net *net)
 {
-	net->core.inuse = alloc_percpu(struct prot_inuse);
-	return net->core.inuse ? 0 : -ENOMEM;
+	net->core.prot_inuse = alloc_percpu(struct prot_inuse);
+	return net->core.prot_inuse ? 0 : -ENOMEM;
 }
 
 static void __net_exit sock_inuse_exit_net(struct net *net)
 {
-	free_percpu(net->core.inuse);
+	free_percpu(net->core.prot_inuse);
 }
 
 static struct pernet_operations net_inuse_ops = {
-- 
1.8.3.1

^ permalink raw reply related

* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Alexander Duyck @ 2017-12-07 16:45 UTC (permalink / raw)
  To: achiad shochat
  Cc: Michael S. Tsirkin, Stephen Hemminger, Jakub Kicinski,
	Hannes Frederic Sowa, Sridhar Samudrala, netdev, virtualization,
	Achiad, Peter Waskiewicz Jr, Singhai, Anjali, Shannon Nelson,
	Andy Gospodarek, Or Gerlitz
In-Reply-To: <CAEHy93JG-X-5qtrU2wUmxfMzKc9G3GDNL_H-FXsmAmfwvoO+vg@mail.gmail.com>

On Wed, Dec 6, 2017 at 11:28 PM, achiad shochat
<achiad.mellanox@gmail.com> wrote:
> On 5 December 2017 at 21:20, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Tue, Dec 05, 2017 at 11:59:17AM +0200, achiad shochat wrote:
>>> Then we'll have a single solution for both netvsc and virtio (and any
>>> other PV device).
>>> And we could handle the VF DMA dirt issue agnostically.
>>
>> For the record, I won't block patches adding this kist to virtio
>> on the basis that they must be generic. It's not a lot
>> of code, implementation can come first, prettify later.
>
> It's not a lot of code either way.
> So I fail to understand why not to do it right from the beginning.
> For the record...

What isn't a lot of code? If you are talking about the DMA dirtying
then I would have to disagree. The big problem with the DMA is that we
have to mark a page as dirty and non-migratable as soon as it is
mapped for Rx DMA. It isn't until the driver has either unmapped the
page or the device has been disabled that we can then allow the page
to be migrated for being dirty. That ends up being the way we have to
support this if we don't have the bonding solution.

With the bonding solution we could look at doing a lightweight DMA
dirtying which would just require flagging pages as dirty after an
unmap or sync call is performed. However it requires that we shut down
the driver/device before we can complete the migration which means we
have to have the paravirtualized fail-over approach.

As far as indicating that the interfaces are meant to be enslaved I
wonder if we couldn't look at tweaking the PCI layout of the guest and
use that to indicate that a given set of interfaces are meant to be
bonded. For example the VFs are all meant to work as a part of a
multi-function device. What if we were to make virtio-net function 0
of a PCI/PCIe device, and then place any direct assigned VFs that are
meant to be a part of the bond in functions 1-7 of the device? Then it
isn't too far off from the model we have on the host where if the VF
goes away we would expect to see the traffic on the PF that is usually
occupying function 0 of a given device.

^ permalink raw reply

* Re: [PATCH v2 08/35] nds32: Process management
From: Al Viro @ 2017-12-07 16:45 UTC (permalink / raw)
  To: Greentime Hu
  Cc: greentime, linux-kernel, arnd, linux-arch, tglx, jason,
	marc.zyngier, robh+dt, netdev, deanbo422, devicetree, dhowells,
	will.deacon, daniel.lezcano, linux-serial, Vincent Chen
In-Reply-To: <53789f9af98217d24580479c55e550301651d4a8.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 08:27:55PM +0800, Greentime Hu wrote:

> +#define start_thread(regs,pc,stack)			\
> +({							\
> +	set_fs(USER_DS);				\

Not the job of start_thread() - its users (->load_binary() methods of
assorted binfmt) must (and do) call flush_old_exec() first.  And
that will switch to USER_DS just fine.

^ permalink raw reply

* Re: [PATCH v2 iproute2 net-next] gre6: add collect metadata support
From: William Tu @ 2017-12-07 16:43 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: Stephen Hemminger, Linux Kernel Network Developers
In-Reply-To: <f378f631-8cb5-db11-1c3f-7e293d87d9fa@iogearbox.net>

On Tue, Dec 5, 2017 at 5:40 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 12/06/2017 02:07 AM, Stephen Hemminger wrote:
>> On Tue,  5 Dec 2017 15:10:37 -0800
>> William Tu <u9012063@gmail.com> wrote:
>>
>>> diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
>>> index a6a10e577b1f..eb04f887c940 100644
>>> --- a/man/man8/ip-link.8.in
>>> +++ b/man/man8/ip-link.8.in
>>> @@ -755,6 +755,8 @@ the following additional arguments are supported:
>>>  .BI "dscp inherit"
>>>  ] [
>>>  .BI dev " PHYS_DEV "
>>> +] [
>>> +.RB external
>>>  ]
>>>
>>>  .in +8
>>> @@ -833,6 +835,10 @@ or
>>>  .IR 00 ".." ff
>>>  when tunneling non-IP packets. The default value is 00.
>>>
>>> +.sp
>>> +.RB external
>>> +- make this tunnel externally controlled (or not, which is the default).
>>> +
>>>  .in -8
>>
>> I don't have any  direct involvement in offload, so would like some feedback
>> from others that are.
>>
>> Not a big fan of opaque "metadata" what exactly does it mean?
>> Also "external" is already used to mean something else on other parts of
>> the link command. Also the option, and the value in JSON should be the same.
>
> The keyword "external" to set the device into collect metadata mode
> is already used heavily throughout iproute2, e.g. vxlan, geneve, ipip,
> and many other device types. Special casing gre6 to not having it set
> up in such way through "external" would seem quite confusing.
>
>> Please reconsider the naming and resubmit
>>
>> The wording in the man page here could be better
>>
>
Hi Stephen and Daniel,
Thanks for the feedbacks.

For the next version, I will remain using the keyword "external" since
it consists with other tunnels.
As for JSON, since geneve and vxlan is useing "collect_metadata" for
external keyword, should I keep using "collect_metadata" for ip6gre?
root@:~/iproute2# git grep collect_metadata
ip/iplink_geneve.c:             print_bool(PRINT_ANY,
"collect_metadata", "external ", true);
ip/iplink_vxlan.c:              print_bool(PRINT_ANY,
"collect_metadata", "external ", true);

Or I can submit another patch to make them all use "external".
Thanks
William

^ permalink raw reply

* [PATCH net] gianfar: Disable EEE autoneg by default
From: Claudiu Manoil @ 2017-12-07 16:44 UTC (permalink / raw)
  To: netdev; +Cc: Andrew Lunn, David S . Miller, Shaohui Xie
In-Reply-To: <20171128062901.22210-1-yangbo.lu@nxp.com>

This controller does not support EEE, but it may connect to a PHY
which supports EEE and advertises EEE by default, while its link
partner also advertises EEE. If this happens, the PHY enters low
power mode when the traffic rate is low and causes packet loss.
This patch disables EEE advertisement by default for any PHY that
gianfar connects to, to prevent the above unwanted outcome.

Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
Tested-by: Yangbo Lu <Yangbo.lu@nxp.com>
Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 81a73af..7f83700 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1792,6 +1792,7 @@ static int init_phy(struct net_device *dev)
 		GFAR_SUPPORTED_GBIT : 0;
 	phy_interface_t interface;
 	struct phy_device *phydev;
+	struct ethtool_eee edata;
 
 	priv->oldlink = 0;
 	priv->oldspeed = 0;
@@ -1816,6 +1817,10 @@ static int init_phy(struct net_device *dev)
 	/* Add support for flow control, but don't advertise it by default */
 	phydev->supported |= (SUPPORTED_Pause | SUPPORTED_Asym_Pause);
 
+	/* disable EEE autoneg, EEE not supported by eTSEC */
+	memset(&edata, 0, sizeof(struct ethtool_eee));
+	phy_ethtool_set_eee(phydev, &edata);
+
 	return 0;
 }
 
-- 
2.7.4

^ permalink raw reply related

* Re: [crypto 4/8] chtls: CPL handler definition
From: Stefano Brivio @ 2017-12-07 16:42 UTC (permalink / raw)
  To: Atul Gupta
  Cc: herbert@gondor.apana.org.au, linux-crypto@vger.kernel.org,
	netdev@vger.kernel.org, davem@davemloft.net, davejwatson@fb.com,
	Ganesh GR, Harsh Jain
In-Reply-To: <MWHPR1201MB0238491AAE21EA436B966AC997330@MWHPR1201MB0238.namprd12.prod.outlook.com>

Hi Atul,

On Thu, 7 Dec 2017 14:50:37 +0000
Atul Gupta <atul.gupta@chelsio.com> wrote:

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Stefano Brivio
> Sent: Tuesday, December 5, 2017 8:54 PM
> To: Atul Gupta <atul.gupta@chelsio.com>
> Cc: herbert@gondor.apana.org.au; linux-crypto@vger.kernel.org; netdev@vger.kernel.org; davem@davemloft.net; davejwatson@fb.com; Ganesh GR <ganeshgr@chelsio.com>; Harsh Jain <Harsh@chelsio.com>
> Subject: Re: [crypto 4/8] chtls: CPL handler definition

First off, it would help immensely if you used an e-mail client with
sane settings for line lengths limit and quoting as described by
RFC3676. Otherwise, this will get quite unreadable, quite soon.

> [...]
>
> > +void get_tcp_symbol(void)
> > +{
> > +	tcp_time_wait_p = (void *)kallsyms_lookup_name("tcp_time_wait");
> > +	if (!tcp_time_wait_p)
> > +		pr_info("could not locate tcp_time_wait");  
> 
> Probably not something that should be used here. Why do you need this?
> [Atul] using it to call tcp_time_wait, as used in tcp_rcv_state_process

Indeed, but why do you need to call tcp_time_wait() directly by looking
it up by symbol name, especially from a network driver? This is really
against any kind of accepted API practice or architecture consideration
whatsoever.

> [...]
>
> > +int chtls_send_reset(struct sock *sk, int mode, struct sk_buff *skb)
> > +{
> > +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> > +
> > +	if (unlikely(csk_flag_nochk(csk, CSK_ABORT_SHUTDOWN) ||
> > +		     !csk->cdev)) {
> > +		if (sk->sk_state == TCP_SYN_RECV)
> > +			csk_set_flag(csk, CSK_RST_ABORTED);
> > +		goto out;
> > +	}
> > +
> > +	if (!csk_flag_nochk(csk, CSK_TX_DATA_SENT)) {
> > +		struct tcp_sock *tp = tcp_sk(sk);
> > +
> > +		if (send_tx_flowc_wr(sk, 0, tp->snd_nxt, tp->rcv_nxt) < 0)
> > +			WARN_ONCE(1, "send tx flowc error");
> > +		csk_set_flag(csk, CSK_TX_DATA_SENT);
> > +	}
> > +
> > +	csk_set_flag(csk, CSK_ABORT_RPL_PENDING);
> > +	chtls_purge_write_queue(sk);
> > +
> > +	csk_set_flag(csk, CSK_ABORT_SHUTDOWN);
> > +	if (sk->sk_state != TCP_SYN_RECV)
> > +		chtls_send_abort(sk, mode, skb);  
> 
> If sk->sk_state == TCP_SYN_RECV, aren't we leaking skb, coming e.g.
> from reset_listen_child()?
> [Atul] If (sk->sk_state == TCP_SYN_RECV) we free the skb, else we call the send abort where skb is freed on completion.

That will only happen if, additionally:

	csk_flag_nochk(csk, CSK_ABORT_SHUTDOWN) || !csk->cdev

but otherwise, you can probably end up here with (sk->sk_state ==
TCP_SYN_RECV) and leak the skb.

> [...]
> > +int chtls_listen_start(struct chtls_dev *cdev, struct sock *sk)
>
> [...]
>
> > +	if (cdev->lldi->enable_fw_ofld_conn) {
> > +		ret = cxgb4_create_server_filter(ndev, stid,
> > +						 inet_sk(sk)->inet_rcv_saddr,
> > +						 inet_sk(sk)->inet_sport, 0,
> > +						 cdev->lldi->rxq_ids[0], 0, 0);
> > +	} else {
> > +		ret = cxgb4_create_server(ndev, stid,
> > +					  inet_sk(sk)->inet_rcv_saddr,
> > +					  inet_sk(sk)->inet_sport, 0,
> > +					  cdev->lldi->rxq_ids[0]);
> > +	}
> > +	if (ret > 0)
> > +		ret = net_xmit_errno(ret);
> > +	if (ret)
> > +		goto del_hash;
> > +
> > +	if (!ret)  
> 
> Not needed I guess?
> [Atul] its required, cxgb4_create_server calls net_xmit_eval where ret can be NET_XMIT_SUCCESS/DROP/CN. 
> net_xmit_eval can return 0 or 1.
> If 1, net_xmit_errno is called which returns ENOBUF or 0. If ENOBUF goto del_hash else return 0

You are doing something like:

	if (x)
		goto y;
	if (!x)
		return 0;
y:

hence the if (!x) clause appears indeed to be quite useless, because
you will never reach that clause if 'x' is true, which voids the whole
purpose of checking whether it's false.

> [...]
> > +static struct sock *chtls_recv_sock(struct sock *lsk,
> > +				    struct request_sock *oreq,
> > +				    void *network_hdr,
> > +				    const struct cpl_pass_accept_req *req,
> > +				    struct chtls_dev *cdev)
> > +
> > +{
> > +	struct sock *newsk;
> > +	struct dst_entry *dst = NULL;
> > +	const struct tcphdr *tcph;
> > +	struct neighbour *n;
> > +	struct net_device *ndev;
> > +	struct chtls_sock *csk;
> > +	struct tcp_sock *tp;
> > +	struct inet_sock *newinet;
> > +	u16 port_id;
> > +	int step;
> > +	int rxq_idx;
> > +	const struct iphdr *iph = (const struct iphdr *)network_hdr;  
> 
> Reverse christmas tree format?
> [Atul] will take care in v2
> 
> > +
> > +	newsk = tcp_create_openreq_child(lsk, oreq, cdev->askb);
> > +	if (!newsk)
> > +		goto free_oreq;
> > +
> > +	dst = inet_csk_route_child_sock(lsk, newsk, oreq);
> > +	if (!dst)
> > +		goto free_sk;
> > +
> > +	tcph = (struct tcphdr *)(iph + 1);
> > +	n = dst_neigh_lookup(dst, &iph->saddr);
> > +	if (!n)
> > +		goto free_sk;
> > +
> > +	ndev = n->dev;
> > +	if (!ndev)
> > +		goto free_sk;
> > +	port_id = cxgb4_port_idx(ndev);
> > +
> > +	csk = chtls_sock_create(cdev);
> > +	if (!csk)
> > +		goto free_sk;
> > +
> > +	csk->l2t_entry = cxgb4_l2t_get(cdev->lldi->l2t, n, ndev, 0);
> > +	if (!csk->l2t_entry)
> > +		goto free_csk;
> > +
> > +	newsk->sk_user_data = csk;
> > +	newsk->sk_backlog_rcv = chtls_backlog_rcv;
> > +
> > +	tp = tcp_sk(newsk);
> > +	newinet = inet_sk(newsk);
> > +
> > +	newinet->inet_daddr = iph->saddr;
> > +	newinet->inet_rcv_saddr = iph->daddr;
> > +	newinet->inet_saddr = iph->daddr;
> > +
> > +	oreq->ts_recent = PASS_OPEN_TID_G(ntohl(req->tos_stid));
> > +	sk_setup_caps(newsk, dst);
> > +	csk->sk = newsk;
> > +	csk->passive_reap_next = oreq;
> > +	csk->tx_chan = cxgb4_port_chan(ndev);
> > +	csk->port_id = port_id;
> > +	csk->egress_dev = ndev;
> > +	csk->tos = PASS_OPEN_TOS_G(ntohl(req->tos_stid));
> > +	csk->ulp_mode = ULP_MODE_TLS;
> > +	step = cdev->lldi->nrxq / cdev->lldi->nchan;
> > +	csk->rss_qid = cdev->lldi->rxq_ids[port_id * step];
> > +	rxq_idx = port_id * step;
> > +	csk->txq_idx = (rxq_idx < cdev->lldi->ntxq) ? rxq_idx :
> > +			port_id * step;
> > +	csk->sndbuf = newsk->sk_sndbuf;
> > +	csk->smac_idx = cxgb4_tp_smt_idx(cdev->lldi->adapter_type,
> > +					 cxgb4_port_viid(ndev));
> > +	tp->rcv_wnd = select_rcv_wnd(csk);
> > +
> > +	neigh_release(n);
> > +	lsk->sk_prot->hash(newsk);
> > +	inet_inherit_port(&tcp_hashinfo, lsk, newsk);
> > +	bh_unlock_sock(newsk);  
> 
> Where is this locked?
> [Atul] tcp_create_openreq_child ->sk_clone_lock

Doesn't this mean that if we hit an error after
tcp_create_openreq_child(), and, say, reach free_sk:

> > +
> > +	return newsk;
> > +free_csk:
> > +	chtls_sock_release(&csk->kref);
> > +free_sk:
> > +	dst_release(dst);
> > +free_oreq:
> > +	chtls_reqsk_free(oreq);
> > +	return NULL;
> > +}

the lock on newsk is never released?

> [...]
>
> > +	if (skb_queue_len(&csk->txq) && chtls_push_frames(csk, 0))
> > +		sk->sk_write_space(sk);
> > +		kfree_skb(skb);  
> 
> I guess you actually always want to kfree_skb(skb) here, right?
> [Atul] yes

Then please fix the indentation. :)

I would also suggest that, given the complexity of the changes, and the
fact that they appear in some parts to challenge the usual practices in
both implementation and structure of typical, existing Linux networking
components, you should mark this as RFC (request for comments) starting
from v2 and try to really split it down in smaller changes, if possible.

-- 
Stefano

^ permalink raw reply

* Re: [PATCH v2 06/35] nds32: MMU fault handling and page table management
From: Al Viro @ 2017-12-07 16:40 UTC (permalink / raw)
  To: Greentime Hu
  Cc: greentime, linux-kernel, arnd, linux-arch, tglx, jason,
	marc.zyngier, robh+dt, netdev, deanbo422, devicetree, dhowells,
	will.deacon, daniel.lezcano, linux-serial, Vincent Chen
In-Reply-To: <ba92adae5d20d99c7c18e75146642a2ccbd5d047.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 08:27:53PM +0800, Greentime Hu wrote:
> +void do_page_fault(unsigned long entry, unsigned long addr,
> +		   unsigned int error_code, struct pt_regs *regs)
[snip]
> +	/*
> +	 * If we're in an interrupt or have no user
> +	 * context, we must not take the fault..
> +	 */
> +	if (unlikely(in_atomic() || !mm))

Broken.  in_atomic() is wrong here - it should be faulthandler_disabled().

^ permalink raw reply

* Re: [PATCH] netfilter: fix clusterip_net_exit build regression
From: Vasily Averin @ 2017-12-07 16:29 UTC (permalink / raw)
  To: Arnd Bergmann, Pablo Neira Ayuso, Jozsef Kadlecsik,
	Florian Westphal, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI
  Cc: Xin Long, netfilter-devel, coreteam, netdev, linux-kernel
In-Reply-To: <20171207132636.259066-1-arnd@arndb.de>

thank you,
it was mu fault.

Reviewed-by: Vasily Averin <vvs@virtuozzo.com>

On 2017-12-07 16:26, Arnd Bergmann wrote:
> The added check produces a build error when CONFIG_PROC_FS is
> disabled:
> 
> net/ipv4/netfilter/ipt_CLUSTERIP.c: In function 'clusterip_net_exit':
> net/ipv4/netfilter/ipt_CLUSTERIP.c:822:28: error: 'cn' undeclared (first use in this function)
> 
> This moves the variable declaration out of the #ifdef to make it
> available to the WARN_ON_ONCE().
> 
> Fixes: 613d0776d3fe ("netfilter: exit_net cleanup check added")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  net/ipv4/netfilter/ipt_CLUSTERIP.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
> index e35b8d074f06..69060e3abe85 100644
> --- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
> +++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
> @@ -813,8 +813,8 @@ static int clusterip_net_init(struct net *net)
>  
>  static void clusterip_net_exit(struct net *net)
>  {
> -#ifdef CONFIG_PROC_FS
>  	struct clusterip_net *cn = net_generic(net, clusterip_net_id);
> +#ifdef CONFIG_PROC_FS
>  	proc_remove(cn->procdir);
>  	cn->procdir = NULL;
>  #endif
> 

^ permalink raw reply

* Re: [PATCH net-next v2 5/8] net: phy: meson-gxl: detect LPA corruption
From: Jerome Brunet @ 2017-12-07 16:22 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Florian Fainelli, Kevin Hilman, netdev, linux-arm-kernel,
	linux-amlogic, linux-kernel
In-Reply-To: <20171207161248.GN24750@lunn.ch>

On Thu, 2017-12-07 at 17:12 +0100, Andrew Lunn wrote:
> > Would it be Ok if send patches 1 to 5 to net ?
> > and 6 to 8 separately on net-next ?
> 
> No. The rules for stable is that a patch must really fix something and
> be minimal.
> 
> Documentation/process/stable-kernel-rules.rst 
> 
> What might be best is to develop a minimal, but ugly patch for stable.
> Get it applied. Around a week later, net will be merged into
> net-next. You can then have a 'revert' patch, followed by this series
> making it nice and clean.

Looks like a plan. Will do.
Thanks Andrew.

> 
>        Andrew

^ permalink raw reply

* Re: Waiting for the PHY to complete auto-negotiation
From: Mason @ 2017-12-07 16:17 UTC (permalink / raw)
  To: Florian Fainelli, Andrew Lunn; +Cc: netdev, David Miller
In-Reply-To: <31ba2a2d-99f4-64d7-b9e3-057cdaa1618c@gmail.com>

On 07/12/2017 00:00, Florian Fainelli wrote:

> On 12/06/2017 11:25 AM, Mason wrote:
>
>> When we detect link down, we put the ethernet HW block in reset,
>> and repeat initialization when the link comes back up.
>>
>> Hmmm, however, at the moment, I only reset on an administrative
>> (user-requested) link down, i.e. through ndo_stop. I would probably
>> have to handle cable unplug/replug events as well.
>>
>> Or just consider the quirk to make flow control too complicated
>> to implement correctly...
> 
> I suppose your procedure is fine, but don't you have a better way to
> resolve that by trying to place a special RX DMA ring entry that allows
> your RX DMA not to be entirely stopped, but intentionally looped through
> a buffer that you control? As long as you can stop the Ethernet MAC RX,
> working with such a limitation is probably fine, but this really sounds
> like a huge pain in the butt and a major HW flaw.

Could you elaborate a bit on your suggestion?
(Special ring entry, looped through a buffer under my control)
Is this a typical thing to do to stop DMA?

Currently the driver tries to stop DMA in nb8800_dma_stop(),
which does the following:

http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/aurora/nb8800.c#L881

1) poll until TX finishes (I assume the system no longer accepts new
   frames to send at this point)
2) set the EOC (end of chain) bit on all descriptors (could there be
   a problem if we receive a frame at that moment? Don't we need some
   kind of lock?)
3) disable address filtering (need to check what this does)
4) enable loop-back mode
5) send up to 5 "fake" packets in order to hit an EOC descriptor

The reason I'm trying to move away from this method is that it doesn't
work on our new SoC; and when pressed, the HW dev said it had never been
supported. (Also I find it somewhat hackish, but that's a matter of taste.)

Regards.

^ permalink raw reply

* Re: VPLS Support
From: Amine Kherbouche @ 2017-12-07 16:12 UTC (permalink / raw)
  To: Алексей Болдырев
  Cc: netdev
In-Reply-To: <1332291512476269@web26g.yandex.ru>


On 12/05/2017 01:17 PM, Алексей Болдырев wrote:
> Hello, when will VPLS be implemented in Linux? August 15, 2017 were patches from ekoinoks, which in Linux added support for VPLS. But, these patches for some reason did not pass. Also, for some reason, the author of these patches has no activity on, for two months. I think so, if activity does not appear in the near future, I will have to take his patches and send it back to the kernel.
> 

Hi Алексей Болдырев,

Thanks for your interest in this topic, but unfortunately David 
Lamparter is offline and he may not be able to be back soon. Moreover I 
do not have time to work on VPLS anymore.

Your are welcome to send your new version starting from the previous 
one, I'll be pleased to review it.

Regards,
   Amine.

^ permalink raw reply

* Re: [PATCH net-next v2 5/8] net: phy: meson-gxl: detect LPA corruption
From: Andrew Lunn @ 2017-12-07 16:12 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: Florian Fainelli, Kevin Hilman, netdev, linux-arm-kernel,
	linux-amlogic, linux-kernel
In-Reply-To: <1512661332.7042.5.camel@baylibre.com>

> Would it be Ok if send patches 1 to 5 to net ?
> and 6 to 8 separately on net-next ?

No. The rules for stable is that a patch must really fix something and
be minimal.

Documentation/process/stable-kernel-rules.rst 

What might be best is to develop a minimal, but ugly patch for stable.
Get it applied. Around a week later, net will be merged into
net-next. You can then have a 'revert' patch, followed by this series
making it nice and clean.

       Andrew

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox