Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v3 net-next 3/6] dt-bindings: net: Add lantiq,xrx200-net DT bindings
From: Andrew Lunn @ 2018-09-10 12:53 UTC (permalink / raw)
  To: Hauke Mehrtens
  Cc: davem, netdev, vivien.didelot, f.fainelli, john, linux-mips, dev,
	hauke.mehrtens, devicetree
In-Reply-To: <20180909201647.32727-4-hauke@hauke-m.de>

On Sun, Sep 09, 2018 at 10:16:44PM +0200, Hauke Mehrtens wrote:
> This adds the binding for the PMAC core between the CPU and the GSWIP
> switch found on the xrx200 / VR9 Lantiq / Intel SoC.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> Cc: devicetree@vger.kernel.org
> ---
>  .../devicetree/bindings/net/lantiq,xrx200-net.txt   | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/lantiq,xrx200-net.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/lantiq,xrx200-net.txt b/Documentation/devicetree/bindings/net/lantiq,xrx200-net.txt
> new file mode 100644
> index 000000000000..8a2fe5200cdc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/lantiq,xrx200-net.txt
> @@ -0,0 +1,21 @@
> +Lantiq xRX200 GSWIP PMAC Ethernet driver
> +==================================
> +
> +Required properties:
> +
> +- compatible	: "lantiq,xrx200-net" for the PMAC of the embedded
> +		: GSWIP in the xXR200
> +- reg		: memory range of the PMAC core inside of the GSWIP core
> +- interrupts	: TX and RX DMA interrupts. Use interrupt-names "tx" for
> +		: the TX interrupt and "rx" for the RX interrupt.
> +
> +Example:
> +
> +eth0: eth@E10B308 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "lantiq,xrx200-net";
> +	reg = <0xE10B308 0x30>;

Hi Hauke

This binding itself looks fine. I just find this address range a bit
odd. What are 0xe10b300-0xe10b307 used for? Are all 0x30 bytes used in
the range? The address range ending at 0xe10b338 seems a bit
odd. 0xe10b33f would be more typical.

I'm asking because it can be messy when you find out you need to
change the address range, and not break backwards compatibility.

     Andrew

^ permalink raw reply

* Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races
From: Jose Abreu @ 2018-09-10 12:52 UTC (permalink / raw)
  To: Neil Armstrong, Jose Abreu, netdev
  Cc: Jerome Brunet, Martin Blumenstingl, David S. Miller, Joao Pinto,
	Giuseppe Cavallaro, Alexandre Torgue
In-Reply-To: <ff2f8f89-134e-b7f8-1a40-d87f371be303@baylibre.com>

[-- Attachment #1: Type: text/plain, Size: 19320 bytes --]

Hi Neil,

On 10-09-2018 12:43, Neil Armstrong wrote:
> Hi Jose,
>
> On 10/09/2018 11:14, Jose Abreu wrote:
>> This follows David Miller advice and tries to fix coalesce timer in
>> multi-queue scenarios.
>>
>> We are now using per-queue coalesce values and per-queue TX timer.
>>
>> Coalesce timer default values was changed to 1ms and the coalesce frames
>> to 25.
>>
>> Tested in B2B setup between XGMAC2 and GMAC5.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Jerome Brunet <jbrunet@baylibre.com>
>> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@st.com>
>> ---
>> Jerome,
> Jerome is in holidays...
>
>> Can you please test if this one is okay ?
>
> I tested this patch on top of 4.18.7 with the previous patch (4ae0169fd1b3) reverted.
>
> The TX or RX stopped a few seconds after iperf3 started :
> (iperf3 is running in server mode on the Amlogic A113D board)
>
> $ iperf3 -c 10.1.4.95
> Connecting to host 10.1.4.95, port 5201
> [  4] local 10.1.2.12 port 39906 connected to 10.1.4.95 port 5201
> [ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
> [  4]   0.00-1.00   sec  56.2 MBytes   472 Mbits/sec    0    660 KBytes
> [  4]   1.00-2.00   sec  56.2 MBytes   472 Mbits/sec    0    660 KBytes
> [  4]   2.00-3.00   sec  38.8 MBytes   325 Mbits/sec    1   1.41 KBytes
> [  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
> [  4]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
> ^C[  4]   5.00-5.61   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bandwidth       Retr
> [  4]   0.00-5.61   sec   151 MBytes   226 Mbits/sec    3             sender
> [  4]   0.00-5.61   sec  0.00 Bytes  0.00 bits/sec                  receiver
> iperf3: interrupt - the client has terminated
>
> $ iperf3 -c 10.1.4.95 -R
> Connecting to host 10.1.4.95, port 5201
> Reverse mode, remote host 10.1.4.95 is sending
> [  4] local 10.1.2.12 port 39894 connected to 10.1.4.95 port 5201
> [ ID] Interval           Transfer     Bandwidth
> [  4]   0.00-1.00   sec  82.2 MBytes   690 Mbits/sec
> [  4]   1.00-2.00   sec  82.6 MBytes   693 Mbits/sec
> [  4]   2.00-3.00   sec  24.2 MBytes   203 Mbits/sec
> [  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec
> [  4]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec
> ^C[  4]   5.00-5.53   sec  0.00 Bytes  0.00 bits/sec
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bandwidth
> [  4]   0.00-5.53   sec  0.00 Bytes  0.00 bits/sec                  sender
> [  4]   0.00-5.53   sec   189 MBytes   287 Mbits/sec                  receiver
> iperf3: interrupt - the client has terminated
>
> These works for hours without this patch applied.

Damn... I'm able to replicate the issue if I turn SMP off but
it's not consistent ...

Can you please try attached follow-up patch ? It's been running
for 10min now and I'm getting ~700Mbps on the same GMAC as you
have (3.71).

Thanks and Best Regards,
Jose Miguel Abreu

>
> Neil
>
>> Thanks and Best Regards,
>> Jose Miguel Abreu
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
>>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   6 +-
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 207 ++++++++++++++--------
>>  3 files changed, 135 insertions(+), 82 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
>> index 1854f270ad66..b1b305f8f414 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
>> @@ -258,10 +258,10 @@ struct stmmac_safety_stats {
>>  #define MAX_DMA_RIWT		0xff
>>  #define MIN_DMA_RIWT		0x20
>>  /* Tx coalesce parameters */
>> -#define STMMAC_COAL_TX_TIMER	40000
>> +#define STMMAC_COAL_TX_TIMER	1000
>>  #define STMMAC_MAX_COAL_TX_TICK	100000
>>  #define STMMAC_TX_MAX_FRAMES	256
>> -#define STMMAC_TX_FRAMES	64
>> +#define STMMAC_TX_FRAMES	25
>>  
>>  /* Packets types */
>>  enum packets_types {
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> index c0a855b7ab3b..957030cfb833 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> @@ -48,6 +48,9 @@ struct stmmac_tx_info {
>>  
>>  /* Frequently used values are kept adjacent for cache effect */
>>  struct stmmac_tx_queue {
>> +	u32 tx_count_frames;
>> +	int tx_timer_active;
>> +	struct timer_list txtimer;
>>  	u32 queue_index;
>>  	struct stmmac_priv *priv_data;
>>  	struct dma_extended_desc *dma_etx ____cacheline_aligned_in_smp;
>> @@ -59,6 +62,7 @@ struct stmmac_tx_queue {
>>  	dma_addr_t dma_tx_phy;
>>  	u32 tx_tail_addr;
>>  	u32 mss;
>> +	struct napi_struct napi ____cacheline_aligned_in_smp;
>>  };
>>  
>>  struct stmmac_rx_queue {
>> @@ -109,14 +113,12 @@ struct stmmac_pps_cfg {
>>  
>>  struct stmmac_priv {
>>  	/* Frequently used values are kept adjacent for cache effect */
>> -	u32 tx_count_frames;
>>  	u32 tx_coal_frames;
>>  	u32 tx_coal_timer;
>>  
>>  	int tx_coalesce;
>>  	int hwts_tx_en;
>>  	bool tx_path_in_lpi_mode;
>> -	struct timer_list txtimer;
>>  	bool tso;
>>  
>>  	unsigned int dma_buf_sz;
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 9f458bb16f2a..9809c2b319fe 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -148,6 +148,7 @@ static void stmmac_verify_args(void)
>>  static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>>  {
>>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
>> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>>  	u32 queue;
>>  
>>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
>> @@ -155,6 +156,12 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>>  
>>  		napi_disable(&rx_q->napi);
>>  	}
>> +
>> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		napi_disable(&tx_q->napi);
>> +	}
>>  }
>>  
>>  /**
>> @@ -164,6 +171,7 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>>  static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>>  {
>>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
>> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>>  	u32 queue;
>>  
>>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
>> @@ -171,6 +179,12 @@ static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>>  
>>  		napi_enable(&rx_q->napi);
>>  	}
>> +
>> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		napi_enable(&tx_q->napi);
>> +	}
>>  }
>>  
>>  /**
>> @@ -1843,7 +1857,8 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
>>   * @queue: TX queue index
>>   * Description: it reclaims the transmit resources after transmission completes.
>>   */
>> -static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>> +static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
>> +			   bool *more)
>>  {
>>  	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>>  	unsigned int bytes_compl = 0, pkts_compl = 0;
>> @@ -1851,10 +1866,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>>  
>>  	netif_tx_lock(priv->dev);
>>  
>> +	if (more)
>> +		*more = false;
>> +
>>  	priv->xstats.tx_clean++;
>>  
>>  	entry = tx_q->dirty_tx;
>> -	while (entry != tx_q->cur_tx) {
>> +	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
>>  		struct sk_buff *skb = tx_q->tx_skbuff[entry];
>>  		struct dma_desc *p;
>>  		int status;
>> @@ -1937,7 +1955,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>>  		stmmac_enable_eee_mode(priv);
>>  		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
>>  	}
>> +
>> +	if (more && (tx_q->dirty_tx != tx_q->cur_tx))
>> +		*more = true;
>> +
>>  	netif_tx_unlock(priv->dev);
>> +
>> +	return pkts_compl;
>>  }
>>  
>>  /**
>> @@ -2020,6 +2044,34 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
>>  	return false;
>>  }
>>  
>> +static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan)
>> +{
>> +	int status = stmmac_dma_interrupt_status(priv, priv->ioaddr,
>> +						 &priv->xstats, chan);
>> +
>> +	if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
>> +		struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
>> +
>> +		if (likely(napi_schedule_prep(&rx_q->napi))) {
>> +			stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
>> +			__napi_schedule(&rx_q->napi);
>> +		}
>> +	} else {
>> +		status &= ~handle_rx;
>> +	}
>> +
>> +	if ((status & handle_tx) && (chan < priv->plat->tx_queues_to_use)) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
>> +
>> +		if (likely(napi_schedule_prep(&tx_q->napi)))
>> +			__napi_schedule(&tx_q->napi);
>> +	} else {
>> +		status &= ~handle_tx;
>> +	}
>> +
>> +	return status;
>> +}
>> +
>>  /**
>>   * stmmac_dma_interrupt - DMA ISR
>>   * @priv: driver private structure
>> @@ -2034,57 +2086,14 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
>>  	u32 channels_to_check = tx_channel_count > rx_channel_count ?
>>  				tx_channel_count : rx_channel_count;
>>  	u32 chan;
>> -	bool poll_scheduled = false;
>>  	int status[max_t(u32, MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES)];
>>  
>>  	/* Make sure we never check beyond our status buffer. */
>>  	if (WARN_ON_ONCE(channels_to_check > ARRAY_SIZE(status)))
>>  		channels_to_check = ARRAY_SIZE(status);
>>  
>> -	/* Each DMA channel can be used for rx and tx simultaneously, yet
>> -	 * napi_struct is embedded in struct stmmac_rx_queue rather than in a
>> -	 * stmmac_channel struct.
>> -	 * Because of this, stmmac_poll currently checks (and possibly wakes)
>> -	 * all tx queues rather than just a single tx queue.
>> -	 */
>>  	for (chan = 0; chan < channels_to_check; chan++)
>> -		status[chan] = stmmac_dma_interrupt_status(priv, priv->ioaddr,
>> -				&priv->xstats, chan);
>> -
>> -	for (chan = 0; chan < rx_channel_count; chan++) {
>> -		if (likely(status[chan] & handle_rx)) {
>> -			struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
>> -
>> -			if (likely(napi_schedule_prep(&rx_q->napi))) {
>> -				stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
>> -				__napi_schedule(&rx_q->napi);
>> -				poll_scheduled = true;
>> -			}
>> -		}
>> -	}
>> -
>> -	/* If we scheduled poll, we already know that tx queues will be checked.
>> -	 * If we didn't schedule poll, see if any DMA channel (used by tx) has a
>> -	 * completed transmission, if so, call stmmac_poll (once).
>> -	 */
>> -	if (!poll_scheduled) {
>> -		for (chan = 0; chan < tx_channel_count; chan++) {
>> -			if (status[chan] & handle_tx) {
>> -				/* It doesn't matter what rx queue we choose
>> -				 * here. We use 0 since it always exists.
>> -				 */
>> -				struct stmmac_rx_queue *rx_q =
>> -					&priv->rx_queue[0];
>> -
>> -				if (likely(napi_schedule_prep(&rx_q->napi))) {
>> -					stmmac_disable_dma_irq(priv,
>> -							priv->ioaddr, chan);
>> -					__napi_schedule(&rx_q->napi);
>> -				}
>> -				break;
>> -			}
>> -		}
>> -	}
>> +		status[chan] = stmmac_napi_check(priv, chan);
>>  
>>  	for (chan = 0; chan < tx_channel_count; chan++) {
>>  		if (unlikely(status[chan] & tx_hard_error_bump_tc)) {
>> @@ -2241,13 +2250,11 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
>>   */
>>  static void stmmac_tx_timer(struct timer_list *t)
>>  {
>> -	struct stmmac_priv *priv = from_timer(priv, t, txtimer);
>> -	u32 tx_queues_count = priv->plat->tx_queues_to_use;
>> -	u32 queue;
>> +	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
>>  
>> -	/* let's scan all the tx queues */
>> -	for (queue = 0; queue < tx_queues_count; queue++)
>> -		stmmac_tx_clean(priv, queue);
>> +	if (likely(napi_schedule_prep(&tx_q->napi)))
>> +		__napi_schedule(&tx_q->napi);
>> +	tx_q->tx_timer_active = 0;
>>  }
>>  
>>  /**
>> @@ -2260,11 +2267,17 @@ static void stmmac_tx_timer(struct timer_list *t)
>>   */
>>  static void stmmac_init_tx_coalesce(struct stmmac_priv *priv)
>>  {
>> +	u32 tx_channel_count = priv->plat->tx_queues_to_use;
>> +	u32 chan;
>> +
>>  	priv->tx_coal_frames = STMMAC_TX_FRAMES;
>>  	priv->tx_coal_timer = STMMAC_COAL_TX_TIMER;
>> -	timer_setup(&priv->txtimer, stmmac_tx_timer, 0);
>> -	priv->txtimer.expires = STMMAC_COAL_TIMER(priv->tx_coal_timer);
>> -	add_timer(&priv->txtimer);
>> +
>> +	for (chan = 0; chan < tx_channel_count; chan++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
>> +
>> +		timer_setup(&tx_q->txtimer, stmmac_tx_timer, 0);
>> +	}
>>  }
>>  
>>  static void stmmac_set_rings_length(struct stmmac_priv *priv)
>> @@ -2592,6 +2605,7 @@ static void stmmac_hw_teardown(struct net_device *dev)
>>  static int stmmac_open(struct net_device *dev)
>>  {
>>  	struct stmmac_priv *priv = netdev_priv(dev);
>> +	u32 chan;
>>  	int ret;
>>  
>>  	stmmac_check_ether_addr(priv);
>> @@ -2688,7 +2702,9 @@ static int stmmac_open(struct net_device *dev)
>>  	if (dev->phydev)
>>  		phy_stop(dev->phydev);
>>  
>> -	del_timer_sync(&priv->txtimer);
>> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
>> +
>>  	stmmac_hw_teardown(dev);
>>  init_error:
>>  	free_dma_desc_resources(priv);
>> @@ -2708,6 +2724,7 @@ static int stmmac_open(struct net_device *dev)
>>  static int stmmac_release(struct net_device *dev)
>>  {
>>  	struct stmmac_priv *priv = netdev_priv(dev);
>> +	u32 chan;
>>  
>>  	if (priv->eee_enabled)
>>  		del_timer_sync(&priv->eee_ctrl_timer);
>> @@ -2722,7 +2739,8 @@ static int stmmac_release(struct net_device *dev)
>>  
>>  	stmmac_disable_all_queues(priv);
>>  
>> -	del_timer_sync(&priv->txtimer);
>> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
>>  
>>  	/* Free the IRQ lines */
>>  	free_irq(dev->irq, dev);
>> @@ -2936,14 +2954,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	priv->xstats.tx_tso_nfrags += nfrags;
>>  
>>  	/* Manage tx mitigation */
>> -	priv->tx_count_frames += nfrags + 1;
>> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>> -		mod_timer(&priv->txtimer,
>> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> -	} else {
>> -		priv->tx_count_frames = 0;
>> +	tx_q->tx_count_frames += nfrags + 1;
>> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>>  		stmmac_set_tx_ic(priv, desc);
>>  		priv->xstats.tx_set_ic_bit++;
>> +		tx_q->tx_count_frames = 0;
>>  	}
>>  
>>  	skb_tx_timestamp(skb);
>> @@ -2994,6 +3009,12 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>>  
>>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>>  
>> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
>> +		tx_q->tx_timer_active = 1;
>> +		mod_timer(&tx_q->txtimer,
>> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> +	}
>> +
>>  	return NETDEV_TX_OK;
>>  
>>  dma_map_err:
>> @@ -3146,14 +3167,11 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	 * This approach takes care about the fragments: desc is the first
>>  	 * element in case of no SG.
>>  	 */
>> -	priv->tx_count_frames += nfrags + 1;
>> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>> -		mod_timer(&priv->txtimer,
>> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> -	} else {
>> -		priv->tx_count_frames = 0;
>> +	tx_q->tx_count_frames += nfrags + 1;
>> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>>  		stmmac_set_tx_ic(priv, desc);
>>  		priv->xstats.tx_set_ic_bit++;
>> +		tx_q->tx_count_frames = 0;
>>  	}
>>  
>>  	skb_tx_timestamp(skb);
>> @@ -3199,8 +3217,15 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
>>  
>>  	stmmac_enable_dma_transmission(priv, priv->ioaddr);
>> +
>>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>>  
>> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
>> +		tx_q->tx_timer_active = 1;
>> +		mod_timer(&tx_q->txtimer,
>> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
>> +	}
>> +
>>  	return NETDEV_TX_OK;
>>  
>>  dma_map_err:
>> @@ -3514,27 +3539,41 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>>   *  Description :
>>   *  To look at the incoming frames and clear the tx resources.
>>   */
>> -static int stmmac_poll(struct napi_struct *napi, int budget)
>> +static int stmmac_rx_poll(struct napi_struct *napi, int budget)
>>  {
>>  	struct stmmac_rx_queue *rx_q =
>>  		container_of(napi, struct stmmac_rx_queue, napi);
>>  	struct stmmac_priv *priv = rx_q->priv_data;
>> -	u32 tx_count = priv->plat->tx_queues_to_use;
>>  	u32 chan = rx_q->queue_index;
>>  	int work_done = 0;
>> -	u32 queue;
>>  
>>  	priv->xstats.napi_poll++;
>>  
>> -	/* check all the queues */
>> -	for (queue = 0; queue < tx_count; queue++)
>> -		stmmac_tx_clean(priv, queue);
>> -
>>  	work_done = stmmac_rx(priv, budget, rx_q->queue_index);
>> +	if (work_done < budget && napi_complete_done(napi, work_done))
>> +		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
>> +
>> +	return work_done;
>> +}
>> +
>> +static int stmmac_tx_poll(struct napi_struct *napi, int budget)
>> +{
>> +	struct stmmac_tx_queue *tx_q =
>> +		container_of(napi, struct stmmac_tx_queue, napi);
>> +	struct stmmac_priv *priv = tx_q->priv_data;
>> +	u32 chan = tx_q->queue_index;
>> +	int work_done = 0;
>> +	bool more;
>> +
>> +	priv->xstats.napi_poll++;
>> +
>> +	work_done = stmmac_tx_clean(priv, budget, chan, &more);
>>  	if (work_done < budget) {
>>  		napi_complete_done(napi, work_done);
>> -		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
>> +		if (more)
>> +			napi_reschedule(napi);
>>  	}
>> +
>>  	return work_done;
>>  }
>>  
>> @@ -4325,10 +4364,17 @@ int stmmac_dvr_probe(struct device *device,
>>  	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
>>  		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
>>  
>> -		netif_napi_add(ndev, &rx_q->napi, stmmac_poll,
>> +		netif_napi_add(ndev, &rx_q->napi, stmmac_rx_poll,
>>  			       (8 * priv->plat->rx_queues_to_use));
>>  	}
>>  
>> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		netif_napi_add(ndev, &tx_q->napi, stmmac_tx_poll,
>> +			       (8 * priv->plat->tx_queues_to_use));
>> +	}
>> +
>>  	mutex_init(&priv->lock);
>>  
>>  	/* If a specific clk_csr value is passed from the platform
>> @@ -4377,6 +4423,11 @@ int stmmac_dvr_probe(struct device *device,
>>  
>>  		netif_napi_del(&rx_q->napi);
>>  	}
>> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
>> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>> +
>> +		netif_napi_del(&tx_q->napi);
>> +	}
>>  error_hw_init:
>>  	destroy_workqueue(priv->wq);
>>  error_wq:
>>


[-- Attachment #2: 0001-fixup_coalesce.patch --]
[-- Type: text/x-patch, Size: 3982 bytes --]

>From 35a5e33f4edcc663511d615e61a1ea119dfc77ee Mon Sep 17 00:00:00 2001
Message-Id: <35a5e33f4edcc663511d615e61a1ea119dfc77ee.1536583587.git.joabreu@synopsys.com>
From: Jose Abreu <joabreu@synopsys.com>
Date: Mon, 10 Sep 2018 14:46:09 +0200
Subject: [PATCH] fixup_coalesce

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 39 +++++++++++++----------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 97268769186e..7875e81966fb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1872,7 +1872,7 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
 	priv->xstats.tx_clean++;
 
 	entry = tx_q->dirty_tx;
-	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
+	while (entry != tx_q->cur_tx) {
 		struct sk_buff *skb = tx_q->tx_skbuff[entry];
 		struct dma_desc *p;
 		int status;
@@ -2241,6 +2241,17 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
 	return ret;
 }
 
+static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
+{
+	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
+
+	if (tx_q->tx_timer_active)
+		return;
+
+	mod_timer(&tx_q->txtimer, STMMAC_COAL_TIMER(priv->tx_coal_timer));
+	tx_q->tx_timer_active = true;
+}
+
 /**
  * stmmac_tx_timer - mitigation sw timer for tx.
  * @data: data pointer
@@ -2250,10 +2261,10 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
 static void stmmac_tx_timer(struct timer_list *t)
 {
 	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
+	struct stmmac_priv *priv = tx_q->priv_data;
 
-	if (likely(napi_schedule_prep(&tx_q->napi)))
-		__napi_schedule(&tx_q->napi);
-	tx_q->tx_timer_active = 0;
+	stmmac_tx_clean(priv, ~0, tx_q->queue_index, NULL);
+	tx_q->tx_timer_active = false;
 }
 
 /**
@@ -2852,6 +2863,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Compute header lengths */
 	proto_hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
 
+	/* Start coalesce timer earlier in case TX Queue is stopped */
+	stmmac_tx_timer_arm(priv, queue);
+
 	/* Desc availability based on threshold should be enough safe */
 	if (unlikely(stmmac_tx_avail(priv, queue) <
 		(((skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1)))) {
@@ -3009,12 +3023,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	tx_q->tx_tail_addr = tx_q->dma_tx_phy + (tx_q->cur_tx * sizeof(*desc));
 	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
 
-	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
-		tx_q->tx_timer_active = 1;
-		mod_timer(&tx_q->txtimer,
-				STMMAC_COAL_TIMER(priv->tx_coal_timer));
-	}
-
 	return NETDEV_TX_OK;
 
 dma_map_err:
@@ -3054,6 +3062,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 			return stmmac_tso_xmit(skb, dev);
 	}
 
+	/* Start coalesce timer earlier in case TX Queue is stopped */
+	stmmac_tx_timer_arm(priv, queue);
+
 	if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
 		if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
 			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
@@ -3221,12 +3232,6 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	tx_q->tx_tail_addr = tx_q->dma_tx_phy + (tx_q->cur_tx * sizeof(*desc));
 	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
 
-	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
-		tx_q->tx_timer_active = 1;
-		mod_timer(&tx_q->txtimer,
-				STMMAC_COAL_TIMER(priv->tx_coal_timer));
-	}
-
 	return NETDEV_TX_OK;
 
 dma_map_err:
@@ -3575,7 +3580,7 @@ static int stmmac_tx_poll(struct napi_struct *napi, int budget)
 			napi_reschedule(napi);
 	}
 
-	return work_done;
+	return min(work_done, budget);
 }
 
 /**
-- 
2.7.4


^ permalink raw reply related

* Re: [PATCH v2 net] MIPS: lantiq: dma: add dev pointer
From: Andrew Lunn @ 2018-09-10 12:45 UTC (permalink / raw)
  To: Hauke Mehrtens
  Cc: davem, netdev, f.fainelli, john, linux-mips, hauke.mehrtens,
	paul.burton
In-Reply-To: <20180909192623.14998-1-hauke@hauke-m.de>

On Sun, Sep 09, 2018 at 09:26:23PM +0200, Hauke Mehrtens wrote:
> dma_zalloc_coherent() now crashes if no dev pointer is given.
> Add a dev pointer to the ltq_dma_channel structure and fill it in the
> driver using it.
> 
> This fixes a bug introduced in kernel 4.19.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
> 
> no changes since v1.
> 
> This should go into kernel 4.19 and I have some other patches adding new 
> features for kernel 4.20 which are depending on this, so I would prefer 
> if this goes through the net tree. 

Hi Hauke

Is this a build time dependency, or a runtime dependency?

What we don't want to do is add the switch driver to net-next and find
it does not compile because this change is not in net-next yet.

   Andrew

^ permalink raw reply

* libbpf build broken on musl libc (Alpine Linux)
From: Arnaldo Carvalho de Melo @ 2018-09-10 17:29 UTC (permalink / raw)
  To: Daniel Borkmann, Thomas Richter
  Cc: Jakub Kicinski, Hendrik Brueckner, Linux Kernel Mailing List,
	Linux Networking Development Mailing List

Hi guys,

	Ingo updated tip/perf/core and this made me notice that perf is
not building on systems using !glibc, like Alpine Linux, that uses musl
libc. This ends up as:

  # dm
   1 alpine:3.4                    : FAIL gcc (Alpine 5.3.0) 5.3.0
   2 alpine:3.5                    : FAIL gcc (Alpine 6.2.1) 6.2.1 20160822
   3 alpine:3.6                    : FAIL gcc (Alpine 6.3.0) 6.3.0
   4 alpine:3.7                    : FAIL gcc (Alpine 6.4.0) 6.4.0
   5 alpine:3.8                    : FAIL gcc (Alpine 6.4.0) 6.4.0
   6 alpine:edge                   : FAIL gcc (Alpine 6.4.0) 6.4.0
   7 amazonlinux:1                 : Ok   gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-28)
   8 amazonlinux:2                 : Ok   gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
   9 android-ndk:r12b-arm          : Ok   arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease)
  10 android-ndk:r15c-arm          : Ok   arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease)
  11 centos:5                      : Ok   gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-55)
  12 centos:6                      : Ok   gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23)
  13 centos:7                      : Ok   gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-28)
  14 debian:7                      : Ok   gcc (Debian 4.7.2-5) 4.7.2
  15 debian:8                      : Ok   gcc (Debian 4.9.2-10+deb8u1) 4.9.2
  17 debian:9                      : Ok   gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
  18 debian:experimental           : Ok   gcc (Debian 8.2.0-4) 8.2.0
<still building on the other containers, 65 in total>

  CC       /tmp/build/perf/libbpf.o
  CC       /tmp/build/perf/event-plugin.o
  CC       /tmp/build/perf/parse-options.o
libbpf.c: In function 'bpf_object__elf_init':
libbpf.c:472:15: error: initialization makes pointer from integer without a cast [-Werror=int-conversion]
    char *cp = strerror_r(errno, errmsg, sizeof(errmsg));
               ^
libbpf.c: In function 'bpf_object__elf_collect':
libbpf.c:813:16: error: initialization makes pointer from integer without a cast [-Werror=int-conversion]
     char *cp = strerror_r(-err, errmsg,
                ^
libbpf.c: In function 'bpf_object__create_maps':
libbpf.c:1143:7: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
    cp = strerror_r(errno, errmsg, sizeof(errmsg));
       ^
libbpf.c:1158:7: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
    cp = strerror_r(errno, errmsg, sizeof(errmsg));
       ^
libbpf.c: In function 'load_program':
libbpf.c:1342:5: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
  cp = strerror_r(errno, errmsg, sizeof(errmsg));
     ^
libbpf.c: In function 'check_path':
libbpf.c:1657:6: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
   cp = strerror_r(errno, errmsg, sizeof(errmsg));
      ^
libbpf.c: In function 'bpf_program__pin_instance':
libbpf.c:1693:6: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
   cp = strerror_r(errno, errmsg, sizeof(errmsg));
      ^
libbpf.c: In function 'make_dir':
libbpf.c:1711:6: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
   cp = strerror_r(-err, errmsg, sizeof(errmsg));
      ^
libbpf.c: In function 'bpf_map__pin':
libbpf.c:1773:6: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
   cp = strerror_r(errno, errmsg, sizeof(errmsg));
      ^
  CC       /tmp/build/perf/trace-seq.o
  CC       /tmp/build/perf/parse-filter.o
  CC       /tmp/build/perf/parse-utils.o
cc1: all warnings being treated as errors
mv: can't rename '/tmp/build/perf/.libbpf.o.tmp': No such file or directory

This is handled in tools/perf/ by using tools/lib/str_error_r.c, that
was introduced with the cset at the end of this message.

After lunch I'll work on a patch to fix this, 

Thanks,

- Arnaldo

commit c8b5f2c96d1bf6cefcbe12f67dce0b892fe20512
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Wed Jul 6 11:56:20 2016 -0300

    tools: Introduce str_error_r()
    
    The tools so far have been using the strerror_r() GNU variant, that
    returns a string, be it the buffer passed or something else.
    
    But that, besides being tricky in cases where we expect that the
    function using strerror_r() returns the error formatted in a provided
    buffer (we have to check if it returned something else and copy that
    instead), breaks the build on systems not using glibc, like Alpine
    Linux, where musl libc is used.
    
    So, introduce yet another wrapper, str_error_r(), that has the GNU
    interface, but uses the portable XSI variant of strerror_r(), so that
    users rest asured that the provided buffer is used and it is what is
    returned.
    
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: Jiri Olsa <jolsa@kernel.org>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Wang Nan <wangnan0@huawei.com>
    Link: http://lkml.kernel.org/n/tip-d4t42fnf48ytlk8rjxs822tf@git.kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/include/linux/string.h b/tools/include/linux/string.h
index e26223f1f287..b466d0228b57 100644
--- a/tools/include/linux/string.h
+++ b/tools/include/linux/string.h
@@ -12,4 +12,6 @@ int strtobool(const char *s, bool *res);
 extern size_t strlcpy(char *dest, const char *src, size_t size);
 #endif
 
+char *str_error_r(int errnum, char *buf, size_t buflen);
<SNIP>

^ permalink raw reply related

* Re: Offloaded u32 classifier tables WAS (Re: [PATCH net 00/13] cls_u32 cleanups and fixes.
From: Jamal Hadi Salim @ 2018-09-10 12:31 UTC (permalink / raw)
  To: Al Viro
  Cc: netdev, Cong Wang, Jiri Pirko, Rahul Lakkireddy, Kumar A S,
	Sridhar Samudrala, Alexander Duyck, Amritha Nambiar, Jose.Abreu,
	peppe.cavallaro, alexandre.torgue, Jeff Kirsher,
	Anjali Singhai Jain
In-Reply-To: <3c72ac9e-12d6-0244-d380-5803607c1dae@mojatatu.com>

On 2018-09-10 8:25 a.m., Jamal Hadi Salim wrote:
> On 2018-09-09 11:48 a.m., Al Viro wrote:
> 
>>
>> BTW, shouldn't we issue u32_clear_hw_hnode() every time
>> we destroy an hnode?  It's done on u32_delete(), it's
>> done (for root ht) on u32_destroy(), but it's not done
>> for any other hnodes when you remove the entire (not shared)
>> filter.  Looks fishy...
>>
> 
> What you are saying makes sense, but that doesnt seem
> like a new thing.
> All hardware offload examples I have seen use root tables
> only[1]. So I am not sure if they use any other tables
> although the intel hardware at least seems very capable.
> Look at ixgbe_main.c for example. Theres an explicit assumption
> that root is always 0x800 but oresence of
>

"..but presence of other tables can be handled"

cheers,
jamal

^ permalink raw reply

* Offloaded u32 classifier tables WAS (Re: [PATCH net 00/13] cls_u32 cleanups and fixes.
From: Jamal Hadi Salim @ 2018-09-10 12:25 UTC (permalink / raw)
  To: Al Viro
  Cc: netdev, Cong Wang, Jiri Pirko, Rahul Lakkireddy, Kumar A S,
	Sridhar Samudrala, Alexander Duyck, Amritha Nambiar, Jose.Abreu,
	peppe.cavallaro, alexandre.torgue, Jeff Kirsher,
	Anjali Singhai Jain
In-Reply-To: <20180909154827.GH19965@ZenIV.linux.org.uk>

On 2018-09-09 11:48 a.m., Al Viro wrote:

>
> BTW, shouldn't we issue u32_clear_hw_hnode() every time
> we destroy an hnode?  It's done on u32_delete(), it's
> done (for root ht) on u32_destroy(), but it's not done
> for any other hnodes when you remove the entire (not shared)
> filter.  Looks fishy...
> 

What you are saying makes sense, but that doesnt seem
like a new thing.
All hardware offload examples I have seen use root tables
only[1]. So I am not sure if they use any other tables
although the intel hardware at least seems very capable.
Look at ixgbe_main.c for example. Theres an explicit assumption
that root is always 0x800 but oresence of

+Cc some of the NIC vendor folks..



cheers,
jamal

[1]
Here's a script posted by someone at Intel(Sridhar?) a while back
that adds 2 filters, one with skip-sw and the other with skip-hw
flag.

---
    # add ingress qdisc
    tc qdisc add dev p4p1 ingress

    # enable hw tc offload.
    ethtool -K p4p1 hw-tc-offload on

    # add u32 filter with skip-sw flag.
    tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
       handle 800:0:1 u32 ht 800: flowid 800:1 \
       skip-sw \
       match ip src 192.168.1.0/24 \
       action drop

    # add u32 filter with skip-hw flag.
    tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
       handle 800:0:2 u32 ht 800: flowid 800:2 \
       skip-hw \
       match ip src 192.168.2.0/24 \
       action drop
----

^ permalink raw reply

* [PATCH][next] cxgb4: remove redundant assignment to vlan_cmd.dropnovlan_fm
From: Colin King @ 2018-09-10 12:17 UTC (permalink / raw)
  To: Ganesh Goudar, David S . Miller, netdev; +Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

A recent commit updated vlan_cmd.dropnovlan_fm but failed to remove
the older assignment.  Fix this by removing the former redundant
assignment.

Detected by CoverityScan, CID#1473290 ("Unused value")

Fixes: a89cdd8e7c74 ("cxgb4: impose mandatory VLAN usage when non-zero TAG ID")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index 6f1bd7bf1a11..c28a1d8b7f33 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -10209,7 +10209,6 @@ int t4_set_vlan_acl(struct adapter *adap, unsigned int mbox, unsigned int vf,
 					 FW_ACL_VLAN_CMD_VFN_V(vf));
 	vlan_cmd.en_to_len16 = cpu_to_be32(enable | FW_LEN16(vlan_cmd));
 	/* Drop all packets that donot match vlan id */
-	vlan_cmd.dropnovlan_fm = FW_ACL_VLAN_CMD_FM_F;
 	vlan_cmd.dropnovlan_fm = (enable
 				  ? (FW_ACL_VLAN_CMD_DROPNOVLAN_F |
 				     FW_ACL_VLAN_CMD_FM_F) : 0);
-- 
2.17.1

^ permalink raw reply related

* Re: [PATCH][next] cxgb4: remove redundant assignment to vlan_cmd.dropnovlan_fm
From: David Miller @ 2018-09-10 17:01 UTC (permalink / raw)
  To: colin.king; +Cc: ganeshgr, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20180910121722.15524-1-colin.king@canonical.com>

From: Colin King <colin.king@canonical.com>
Date: Mon, 10 Sep 2018 13:17:22 +0100

> From: Colin Ian King <colin.king@canonical.com>
> 
> A recent commit updated vlan_cmd.dropnovlan_fm but failed to remove
> the older assignment.  Fix this by removing the former redundant
> assignment.
> 
> Detected by CoverityScan, CID#1473290 ("Unused value")
> 
> Fixes: a89cdd8e7c74 ("cxgb4: impose mandatory VLAN usage when non-zero TAG ID")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Applied, thanks.

^ permalink raw reply

* Re: [PATCH iproute2 3/3] bridge: fix vlan show formatting
From: Tobias Jungel @ 2018-09-10 11:52 UTC (permalink / raw)
  To: Stephen Hemminger, netdev
In-Reply-To: <20180906153057.5379-3-stephen@networkplumber.org>

Hi Stephen,

I just have seen this patch, hence please ignore the "bridge: Correct
json output" I sent. This one solves the issue in a way more elegant
manor.

I tested the JSON output in this series and it works as intended.

Thanks


Reviewed-by: Tobias Jungel <tobias.jungel@bisdn.de>


On Thu, 2018-09-06 at 16:30 +0100, Stephen Hemminger wrote:
> The output of vlan show was broken previous change to use json_print.
> Clean the code up and return to original format.
> 
> Note: the JSON syntax has changed to make the bridge vlan
> show more like other outputs (e.g. ip -j li show).
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  bridge/br_common.h |  2 +-
>  bridge/link.c      |  6 ++---
>  bridge/vlan.c      | 61 +++++++++++++++++++++++++++-----------------
> --
>  3 files changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/bridge/br_common.h b/bridge/br_common.h
> index 2f1cb8fd9f3d..69665fde32b6 100644
> --- a/bridge/br_common.h
> +++ b/bridge/br_common.h
> @@ -6,7 +6,7 @@
>  #define MDB_RTR_RTA(r) \
>  		((struct rtattr *)(((char *)(r)) +
> RTA_ALIGN(sizeof(__u32))))
>  
> -extern void print_vlan_info(FILE *fp, struct rtattr *tb);
> +extern void print_vlan_info(struct rtattr *tb, int ifindex);

nitpick, this does not apply to master.

>  extern int print_linkinfo(const struct sockaddr_nl *who,
>  			  struct nlmsghdr *n,
>  			  void *arg);
> diff --git a/bridge/link.c b/bridge/link.c
> index 8d89aca2e638..a5ee9a5c58e6 100644
> --- a/bridge/link.c
> +++ b/bridge/link.c
> @@ -161,7 +161,7 @@ static void print_protinfo(FILE *fp, struct
> rtattr *attr)
>   * This is reported by HW devices that have some bridging
>   * capabilities.
>   */
> -static void print_af_spec(FILE *fp, struct rtattr *attr)
> +static void print_af_spec(struct rtattr *attr, int ifindex)
>  {
>  	struct rtattr *aftb[IFLA_BRIDGE_MAX+1];
>  
> @@ -174,7 +174,7 @@ static void print_af_spec(FILE *fp, struct rtattr
> *attr)
>  		return;
>  
>  	if (aftb[IFLA_BRIDGE_VLAN_INFO])
> -		print_vlan_info(fp, aftb[IFLA_BRIDGE_VLAN_INFO]);
> +		print_vlan_info(aftb[IFLA_BRIDGE_VLAN_INFO], ifindex);
>  }
>  
>  int print_linkinfo(const struct sockaddr_nl *who,
> @@ -229,7 +229,7 @@ int print_linkinfo(const struct sockaddr_nl *who,
>  		print_protinfo(fp, tb[IFLA_PROTINFO]);
>  
>  	if (tb[IFLA_AF_SPEC])
> -		print_af_spec(fp, tb[IFLA_AF_SPEC]);
> +		print_af_spec(tb[IFLA_AF_SPEC], ifi->ifi_index);
>  
>  	print_string(PRINT_FP, NULL, "%s", "\n");
>  	close_json_object();
> diff --git a/bridge/vlan.c b/bridge/vlan.c
> index 19a36b804069..bdce55ae4e14 100644
> --- a/bridge/vlan.c
> +++ b/bridge/vlan.c
> @@ -252,10 +252,18 @@ static int filter_vlan_check(__u16 vid, __u16
> flags)
>  	return 1;
>  }
>  
> -static void print_vlan_port(FILE *fp, int ifi_index)
> +static void open_vlan_port(int ifi_index)
>  {
> -	print_string(PRINT_ANY, NULL, "%s",
> +	open_json_object(NULL);
> +	print_string(PRINT_ANY, "ifname", "%s",
>  		     ll_index_to_name(ifi_index));
> +	open_json_array(PRINT_JSON, "vlans");
> +}
> +
> +static void close_vlan_port(void)
> +{
> +	close_json_array(PRINT_JSON, NULL);
> +	close_json_object();
>  }
>  
>  static void print_range(const char *name, __u16 start, __u16 id)
> @@ -278,7 +286,7 @@ static void print_vlan_tunnel_info(FILE *fp,
> struct rtattr *tb, int ifindex)
>  	__u32 last_tunid_start = 0;
>  
>  	if (!filter_vlan)
> -		print_vlan_port(fp, ifindex);
> +		open_vlan_port(ifindex);
>  
>  	open_json_array(PRINT_JSON, "tunnel");
>  	for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem))
> {
> @@ -323,18 +331,20 @@ static void print_vlan_tunnel_info(FILE *fp,
> struct rtattr *tb, int ifindex)
>  			continue;
>  
>  		if (filter_vlan)
> -			print_vlan_port(fp, ifindex);
> +			open_vlan_port(ifindex);
>  
>  		open_json_object(NULL);
>  		print_range("vlan", last_vid_start, tunnel_vid);
>  		print_range("tunid", last_tunid_start, tunnel_id);
>  		close_json_object();
>  
> -		if (!is_json_context())
> -			fprintf(fp, "\n");
> -
> +		print_string(PRINT_FP, NULL, "%s", _SL_);
> +		if (filter_vlan)
> +			close_vlan_port();
>  	}
> -	close_json_array(PRINT_JSON, NULL);
> +
> +	if (!filter_vlan)
> +		close_vlan_port();
>  }
>  
>  static int print_vlan_tunnel(const struct sockaddr_nl *who,
> @@ -421,8 +431,8 @@ static int print_vlan(const struct sockaddr_nl
> *who,
>  		return 0;
>  	}
>  
> -	print_vlan_port(fp, ifm->ifi_index);
> -	print_vlan_info(fp, tb[IFLA_AF_SPEC]);
> +	print_vlan_info(tb[IFLA_AF_SPEC], ifm->ifi_index);
> +	print_string(PRINT_FP, NULL, "%s", _SL_);
>  
>  	fflush(fp);
>  	return 0;
> @@ -430,11 +440,16 @@ static int print_vlan(const struct sockaddr_nl
> *who,
>  
>  static void print_vlan_flags(__u16 flags)
>  {
> +	if (flags == 0)
> +		return;
> +
> +	open_json_array(PRINT_JSON, "flags");
>  	if (flags & BRIDGE_VLAN_INFO_PVID)
> -		print_null(PRINT_ANY, "pvid", " %s", "PVID");
> +		print_string(PRINT_ANY, NULL, " %s", "PVID");
>  
>  	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
> -		print_null(PRINT_ANY, "untagged", " %s", "untagged");
> +		print_string(PRINT_ANY, NULL, " %s", "Egress
> Untagged");
> +	close_json_array(PRINT_JSON, NULL);
>  }
>  
>  static void print_one_vlan_stats(const struct bridge_vlan_xstats
> *vstats)
> @@ -461,6 +476,7 @@ static void print_vlan_stats_attr(struct rtattr
> *attr, int ifindex)
>  {
>  	struct rtattr *brtb[LINK_XSTATS_TYPE_MAX+1];
>  	struct rtattr *i, *list;
> +	const char *ifname;
>  	int rem;
>  
>  	parse_rtattr(brtb, LINK_XSTATS_TYPE_MAX, RTA_DATA(attr),
> @@ -471,13 +487,12 @@ static void print_vlan_stats_attr(struct rtattr
> *attr, int ifindex)
>  	list = brtb[LINK_XSTATS_TYPE_BRIDGE];
>  	rem = RTA_PAYLOAD(list);
>  
> -	open_json_object(NULL);
> +	ifname = ll_index_to_name(ifindex);
> +	open_json_object(ifname);
>  
> -	print_color_string(PRINT_ANY, COLOR_IFNAME,
> -			   "dev", "%-16s",
> -			   ll_index_to_name(ifindex));
> +	print_color_string(PRINT_FP, COLOR_IFNAME,
> +			   NULL, "%-16s", ifname);
>  
> -	open_json_array(PRINT_JSON, "xstats");
>  	for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem))
> {
>  		const struct bridge_vlan_xstats *vstats = RTA_DATA(i);
>  
> @@ -494,7 +509,6 @@ static void print_vlan_stats_attr(struct rtattr
> *attr, int ifindex)
>  
>  		print_one_vlan_stats(vstats);
>  	}
> -	close_json_array(PRINT_ANY, "\n");
>  	close_json_object();
>  
>  }
> @@ -623,16 +637,13 @@ static int vlan_show(int argc, char **argv)
>  	return 0;
>  }
>  
> -void print_vlan_info(FILE *fp, struct rtattr *tb)
> +void print_vlan_info(struct rtattr *tb, int ifindex)
>  {
>  	struct rtattr *i, *list = tb;
>  	int rem = RTA_PAYLOAD(list);
>  	__u16 last_vid_start = 0;
>  
> -	if (!is_json_context())
> -		fprintf(fp, "%s", _SL_);
> -
> -	open_json_array(PRINT_JSON, "vlan");
> +	open_vlan_port(ifindex);
>  
>  	for (i = RTA_DATA(list); RTA_OK(i, rem); i = RTA_NEXT(i, rem))
> {
>  		struct bridge_vlan_info *vinfo;
> @@ -656,9 +667,9 @@ void print_vlan_info(FILE *fp, struct rtattr *tb)
>  
>  		print_vlan_flags(vinfo->flags);
>  		close_json_object();
> +		print_string(PRINT_FP, NULL, "%s", _SL_);
>  	}
> -
> -	close_json_array(PRINT_ANY, "\n");
> +	close_vlan_port();
>  }
>  
>  int do_vlan(int argc, char **argv)

^ permalink raw reply

* Re: [PATCH] r8169: Clear RTL_FLAG_TASK_*_PENDING when clearing RTL_FLAG_TASK_ENABLED
From: Florian Fainelli @ 2018-09-10 16:42 UTC (permalink / raw)
  To: Kai-Heng Feng, nic_swsd; +Cc: davem, netdev, linux-kernel, Heiner Kallweit
In-Reply-To: <20180910063436.2326-1-kai.heng.feng@canonical.com>

On 09/09/2018 11:34 PM, Kai-Heng Feng wrote:
> After system suspend, sometimes the r8169 doesn't work when ethernet
> cable gets pluggued.
> 
> This issue happens because rtl_reset_work() doesn't get called from
> rtl8169_runtime_resume(), after system suspend.
> 
> In rtl_task(), RTL_FLAG_TASK_* only gets cleared if this condition is
> met:
> if (!netif_running(dev) ||
>     !test_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags))
>     ...
> 
> If RTL_FLAG_TASK_ENABLED was cleared during system suspend while
> RTL_FLAG_TASK_RESET_PENDING was set, the next rtl_schedule_task() won't
> schedule task as the flag is still there.
> 
> So in addition to clearing RTL_FLAG_TASK_ENABLED, also clears other
> flags.
> 
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/net/ethernet/realtek/r8169.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index b08d51bf7a20..20593245ef53 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -6648,6 +6648,7 @@ static int rtl8169_close(struct net_device *dev)
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
>  	struct pci_dev *pdev = tp->pci_dev;
> +	int i;
>  
>  	pm_runtime_get_sync(&pdev->dev);
>  
> @@ -6655,7 +6656,9 @@ static int rtl8169_close(struct net_device *dev)
>  	rtl8169_update_counters(tp);
>  
>  	rtl_lock_work(tp);
> -	clear_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
> +	/* Clear all task flags */
> +	for (i = 0; i < RTL_FLAG_MAX; i++)
> +		clear_bit(i, tp->wk.flags);

bitmap_zero(&tp->wk.,flagsm RTL_FLAG_MAX)

>  
>  	rtl8169_down(dev);
>  	rtl_unlock_work(tp);
> @@ -6828,6 +6831,7 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
>  static void rtl8169_net_suspend(struct net_device *dev)
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
> +	int i;
>  
>  	if (!netif_running(dev))
>  		return;
> @@ -6838,7 +6842,10 @@ static void rtl8169_net_suspend(struct net_device *dev)
>  
>  	rtl_lock_work(tp);
>  	napi_disable(&tp->napi);
> -	clear_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
> +	/* Clear all task flags */
> +	for (i = 0; i < RTL_FLAG_MAX; i++)
> +		clear_bit(i, tp->wk.flags);

Likewise.

> +
>  	rtl_unlock_work(tp);
>  
>  	rtl_pll_power_down(tp);
> 


-- 
Florian

^ permalink raw reply

* Re: [net-next, v2, 1/2] net: stmmac: Rework coalesce timer and fix multi-queue races
From: Neil Armstrong @ 2018-09-10 11:43 UTC (permalink / raw)
  To: Jose Abreu, netdev
  Cc: Jerome Brunet, Martin Blumenstingl, David S. Miller, Joao Pinto,
	Giuseppe Cavallaro, Alexandre Torgue
In-Reply-To: <c54230d2dc7e2740c039e0f5e8f306a7544bbe5e.1536570319.git.joabreu@synopsys.com>

Hi Jose,

On 10/09/2018 11:14, Jose Abreu wrote:
> This follows David Miller advice and tries to fix coalesce timer in
> multi-queue scenarios.
> 
> We are now using per-queue coalesce values and per-queue TX timer.
> 
> Coalesce timer default values was changed to 1ms and the coalesce frames
> to 25.
> 
> Tested in B2B setup between XGMAC2 and GMAC5.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> ---
> Jerome,

Jerome is in holidays...

> 
> Can you please test if this one is okay ?


I tested this patch on top of 4.18.7 with the previous patch (4ae0169fd1b3) reverted.

The TX or RX stopped a few seconds after iperf3 started :
(iperf3 is running in server mode on the Amlogic A113D board)

$ iperf3 -c 10.1.4.95
Connecting to host 10.1.4.95, port 5201
[  4] local 10.1.2.12 port 39906 connected to 10.1.4.95 port 5201
[ ID] Interval           Transfer     Bandwidth       Retr  Cwnd
[  4]   0.00-1.00   sec  56.2 MBytes   472 Mbits/sec    0    660 KBytes
[  4]   1.00-2.00   sec  56.2 MBytes   472 Mbits/sec    0    660 KBytes
[  4]   2.00-3.00   sec  38.8 MBytes   325 Mbits/sec    1   1.41 KBytes
[  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
[  4]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec    1   1.41 KBytes
^C[  4]   5.00-5.61   sec  0.00 Bytes  0.00 bits/sec    0   1.41 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bandwidth       Retr
[  4]   0.00-5.61   sec   151 MBytes   226 Mbits/sec    3             sender
[  4]   0.00-5.61   sec  0.00 Bytes  0.00 bits/sec                  receiver
iperf3: interrupt - the client has terminated

$ iperf3 -c 10.1.4.95 -R
Connecting to host 10.1.4.95, port 5201
Reverse mode, remote host 10.1.4.95 is sending
[  4] local 10.1.2.12 port 39894 connected to 10.1.4.95 port 5201
[ ID] Interval           Transfer     Bandwidth
[  4]   0.00-1.00   sec  82.2 MBytes   690 Mbits/sec
[  4]   1.00-2.00   sec  82.6 MBytes   693 Mbits/sec
[  4]   2.00-3.00   sec  24.2 MBytes   203 Mbits/sec
[  4]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec
[  4]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec
^C[  4]   5.00-5.53   sec  0.00 Bytes  0.00 bits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bandwidth
[  4]   0.00-5.53   sec  0.00 Bytes  0.00 bits/sec                  sender
[  4]   0.00-5.53   sec   189 MBytes   287 Mbits/sec                  receiver
iperf3: interrupt - the client has terminated

These works for hours without this patch applied.

Neil

> 
> Thanks and Best Regards,
> Jose Miguel Abreu
> ---
>  drivers/net/ethernet/stmicro/stmmac/common.h      |   4 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   6 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 207 ++++++++++++++--------
>  3 files changed, 135 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index 1854f270ad66..b1b305f8f414 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -258,10 +258,10 @@ struct stmmac_safety_stats {
>  #define MAX_DMA_RIWT		0xff
>  #define MIN_DMA_RIWT		0x20
>  /* Tx coalesce parameters */
> -#define STMMAC_COAL_TX_TIMER	40000
> +#define STMMAC_COAL_TX_TIMER	1000
>  #define STMMAC_MAX_COAL_TX_TICK	100000
>  #define STMMAC_TX_MAX_FRAMES	256
> -#define STMMAC_TX_FRAMES	64
> +#define STMMAC_TX_FRAMES	25
>  
>  /* Packets types */
>  enum packets_types {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index c0a855b7ab3b..957030cfb833 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -48,6 +48,9 @@ struct stmmac_tx_info {
>  
>  /* Frequently used values are kept adjacent for cache effect */
>  struct stmmac_tx_queue {
> +	u32 tx_count_frames;
> +	int tx_timer_active;
> +	struct timer_list txtimer;
>  	u32 queue_index;
>  	struct stmmac_priv *priv_data;
>  	struct dma_extended_desc *dma_etx ____cacheline_aligned_in_smp;
> @@ -59,6 +62,7 @@ struct stmmac_tx_queue {
>  	dma_addr_t dma_tx_phy;
>  	u32 tx_tail_addr;
>  	u32 mss;
> +	struct napi_struct napi ____cacheline_aligned_in_smp;
>  };
>  
>  struct stmmac_rx_queue {
> @@ -109,14 +113,12 @@ struct stmmac_pps_cfg {
>  
>  struct stmmac_priv {
>  	/* Frequently used values are kept adjacent for cache effect */
> -	u32 tx_count_frames;
>  	u32 tx_coal_frames;
>  	u32 tx_coal_timer;
>  
>  	int tx_coalesce;
>  	int hwts_tx_en;
>  	bool tx_path_in_lpi_mode;
> -	struct timer_list txtimer;
>  	bool tso;
>  
>  	unsigned int dma_buf_sz;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9f458bb16f2a..9809c2b319fe 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -148,6 +148,7 @@ static void stmmac_verify_args(void)
>  static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>  {
>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>  	u32 queue;
>  
>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
> @@ -155,6 +156,12 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>  
>  		napi_disable(&rx_q->napi);
>  	}
> +
> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		napi_disable(&tx_q->napi);
> +	}
>  }
>  
>  /**
> @@ -164,6 +171,7 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
>  static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>  {
>  	u32 rx_queues_cnt = priv->plat->rx_queues_to_use;
> +	u32 tx_queues_cnt = priv->plat->tx_queues_to_use;
>  	u32 queue;
>  
>  	for (queue = 0; queue < rx_queues_cnt; queue++) {
> @@ -171,6 +179,12 @@ static void stmmac_enable_all_queues(struct stmmac_priv *priv)
>  
>  		napi_enable(&rx_q->napi);
>  	}
> +
> +	for (queue = 0; queue < tx_queues_cnt; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		napi_enable(&tx_q->napi);
> +	}
>  }
>  
>  /**
> @@ -1843,7 +1857,8 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
>   * @queue: TX queue index
>   * Description: it reclaims the transmit resources after transmission completes.
>   */
> -static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
> +static int stmmac_tx_clean(struct stmmac_priv *priv, int limit, u32 queue,
> +			   bool *more)
>  {
>  	struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
>  	unsigned int bytes_compl = 0, pkts_compl = 0;
> @@ -1851,10 +1866,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>  
>  	netif_tx_lock(priv->dev);
>  
> +	if (more)
> +		*more = false;
> +
>  	priv->xstats.tx_clean++;
>  
>  	entry = tx_q->dirty_tx;
> -	while (entry != tx_q->cur_tx) {
> +	while ((entry != tx_q->cur_tx) && (pkts_compl < limit)) {
>  		struct sk_buff *skb = tx_q->tx_skbuff[entry];
>  		struct dma_desc *p;
>  		int status;
> @@ -1937,7 +1955,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv, u32 queue)
>  		stmmac_enable_eee_mode(priv);
>  		mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(eee_timer));
>  	}
> +
> +	if (more && (tx_q->dirty_tx != tx_q->cur_tx))
> +		*more = true;
> +
>  	netif_tx_unlock(priv->dev);
> +
> +	return pkts_compl;
>  }
>  
>  /**
> @@ -2020,6 +2044,34 @@ static bool stmmac_safety_feat_interrupt(struct stmmac_priv *priv)
>  	return false;
>  }
>  
> +static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan)
> +{
> +	int status = stmmac_dma_interrupt_status(priv, priv->ioaddr,
> +						 &priv->xstats, chan);
> +
> +	if ((status & handle_rx) && (chan < priv->plat->rx_queues_to_use)) {
> +		struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
> +
> +		if (likely(napi_schedule_prep(&rx_q->napi))) {
> +			stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
> +			__napi_schedule(&rx_q->napi);
> +		}
> +	} else {
> +		status &= ~handle_rx;
> +	}
> +
> +	if ((status & handle_tx) && (chan < priv->plat->tx_queues_to_use)) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
> +
> +		if (likely(napi_schedule_prep(&tx_q->napi)))
> +			__napi_schedule(&tx_q->napi);
> +	} else {
> +		status &= ~handle_tx;
> +	}
> +
> +	return status;
> +}
> +
>  /**
>   * stmmac_dma_interrupt - DMA ISR
>   * @priv: driver private structure
> @@ -2034,57 +2086,14 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
>  	u32 channels_to_check = tx_channel_count > rx_channel_count ?
>  				tx_channel_count : rx_channel_count;
>  	u32 chan;
> -	bool poll_scheduled = false;
>  	int status[max_t(u32, MTL_MAX_TX_QUEUES, MTL_MAX_RX_QUEUES)];
>  
>  	/* Make sure we never check beyond our status buffer. */
>  	if (WARN_ON_ONCE(channels_to_check > ARRAY_SIZE(status)))
>  		channels_to_check = ARRAY_SIZE(status);
>  
> -	/* Each DMA channel can be used for rx and tx simultaneously, yet
> -	 * napi_struct is embedded in struct stmmac_rx_queue rather than in a
> -	 * stmmac_channel struct.
> -	 * Because of this, stmmac_poll currently checks (and possibly wakes)
> -	 * all tx queues rather than just a single tx queue.
> -	 */
>  	for (chan = 0; chan < channels_to_check; chan++)
> -		status[chan] = stmmac_dma_interrupt_status(priv, priv->ioaddr,
> -				&priv->xstats, chan);
> -
> -	for (chan = 0; chan < rx_channel_count; chan++) {
> -		if (likely(status[chan] & handle_rx)) {
> -			struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
> -
> -			if (likely(napi_schedule_prep(&rx_q->napi))) {
> -				stmmac_disable_dma_irq(priv, priv->ioaddr, chan);
> -				__napi_schedule(&rx_q->napi);
> -				poll_scheduled = true;
> -			}
> -		}
> -	}
> -
> -	/* If we scheduled poll, we already know that tx queues will be checked.
> -	 * If we didn't schedule poll, see if any DMA channel (used by tx) has a
> -	 * completed transmission, if so, call stmmac_poll (once).
> -	 */
> -	if (!poll_scheduled) {
> -		for (chan = 0; chan < tx_channel_count; chan++) {
> -			if (status[chan] & handle_tx) {
> -				/* It doesn't matter what rx queue we choose
> -				 * here. We use 0 since it always exists.
> -				 */
> -				struct stmmac_rx_queue *rx_q =
> -					&priv->rx_queue[0];
> -
> -				if (likely(napi_schedule_prep(&rx_q->napi))) {
> -					stmmac_disable_dma_irq(priv,
> -							priv->ioaddr, chan);
> -					__napi_schedule(&rx_q->napi);
> -				}
> -				break;
> -			}
> -		}
> -	}
> +		status[chan] = stmmac_napi_check(priv, chan);
>  
>  	for (chan = 0; chan < tx_channel_count; chan++) {
>  		if (unlikely(status[chan] & tx_hard_error_bump_tc)) {
> @@ -2241,13 +2250,11 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
>   */
>  static void stmmac_tx_timer(struct timer_list *t)
>  {
> -	struct stmmac_priv *priv = from_timer(priv, t, txtimer);
> -	u32 tx_queues_count = priv->plat->tx_queues_to_use;
> -	u32 queue;
> +	struct stmmac_tx_queue *tx_q = from_timer(tx_q, t, txtimer);
>  
> -	/* let's scan all the tx queues */
> -	for (queue = 0; queue < tx_queues_count; queue++)
> -		stmmac_tx_clean(priv, queue);
> +	if (likely(napi_schedule_prep(&tx_q->napi)))
> +		__napi_schedule(&tx_q->napi);
> +	tx_q->tx_timer_active = 0;
>  }
>  
>  /**
> @@ -2260,11 +2267,17 @@ static void stmmac_tx_timer(struct timer_list *t)
>   */
>  static void stmmac_init_tx_coalesce(struct stmmac_priv *priv)
>  {
> +	u32 tx_channel_count = priv->plat->tx_queues_to_use;
> +	u32 chan;
> +
>  	priv->tx_coal_frames = STMMAC_TX_FRAMES;
>  	priv->tx_coal_timer = STMMAC_COAL_TX_TIMER;
> -	timer_setup(&priv->txtimer, stmmac_tx_timer, 0);
> -	priv->txtimer.expires = STMMAC_COAL_TIMER(priv->tx_coal_timer);
> -	add_timer(&priv->txtimer);
> +
> +	for (chan = 0; chan < tx_channel_count; chan++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
> +
> +		timer_setup(&tx_q->txtimer, stmmac_tx_timer, 0);
> +	}
>  }
>  
>  static void stmmac_set_rings_length(struct stmmac_priv *priv)
> @@ -2592,6 +2605,7 @@ static void stmmac_hw_teardown(struct net_device *dev)
>  static int stmmac_open(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> +	u32 chan;
>  	int ret;
>  
>  	stmmac_check_ether_addr(priv);
> @@ -2688,7 +2702,9 @@ static int stmmac_open(struct net_device *dev)
>  	if (dev->phydev)
>  		phy_stop(dev->phydev);
>  
> -	del_timer_sync(&priv->txtimer);
> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
> +
>  	stmmac_hw_teardown(dev);
>  init_error:
>  	free_dma_desc_resources(priv);
> @@ -2708,6 +2724,7 @@ static int stmmac_open(struct net_device *dev)
>  static int stmmac_release(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> +	u32 chan;
>  
>  	if (priv->eee_enabled)
>  		del_timer_sync(&priv->eee_ctrl_timer);
> @@ -2722,7 +2739,8 @@ static int stmmac_release(struct net_device *dev)
>  
>  	stmmac_disable_all_queues(priv);
>  
> -	del_timer_sync(&priv->txtimer);
> +	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> +		del_timer_sync(&priv->tx_queue[chan].txtimer);
>  
>  	/* Free the IRQ lines */
>  	free_irq(dev->irq, dev);
> @@ -2936,14 +2954,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	priv->xstats.tx_tso_nfrags += nfrags;
>  
>  	/* Manage tx mitigation */
> -	priv->tx_count_frames += nfrags + 1;
> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> -		mod_timer(&priv->txtimer,
> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> -	} else {
> -		priv->tx_count_frames = 0;
> +	tx_q->tx_count_frames += nfrags + 1;
> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>  		stmmac_set_tx_ic(priv, desc);
>  		priv->xstats.tx_set_ic_bit++;
> +		tx_q->tx_count_frames = 0;
>  	}
>  
>  	skb_tx_timestamp(skb);
> @@ -2994,6 +3009,12 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>  
> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
> +		tx_q->tx_timer_active = 1;
> +		mod_timer(&tx_q->txtimer,
> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
> +	}
> +
>  	return NETDEV_TX_OK;
>  
>  dma_map_err:
> @@ -3146,14 +3167,11 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	 * This approach takes care about the fragments: desc is the first
>  	 * element in case of no SG.
>  	 */
> -	priv->tx_count_frames += nfrags + 1;
> -	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> -		mod_timer(&priv->txtimer,
> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> -	} else {
> -		priv->tx_count_frames = 0;
> +	tx_q->tx_count_frames += nfrags + 1;
> +	if (priv->tx_coal_frames <= tx_q->tx_count_frames) {
>  		stmmac_set_tx_ic(priv, desc);
>  		priv->xstats.tx_set_ic_bit++;
> +		tx_q->tx_count_frames = 0;
>  	}
>  
>  	skb_tx_timestamp(skb);
> @@ -3199,8 +3217,15 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
>  
>  	stmmac_enable_dma_transmission(priv, priv->ioaddr);
> +
>  	stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr, queue);
>  
> +	if (priv->tx_coal_timer && !tx_q->tx_timer_active) {
> +		tx_q->tx_timer_active = 1;
> +		mod_timer(&tx_q->txtimer,
> +				STMMAC_COAL_TIMER(priv->tx_coal_timer));
> +	}
> +
>  	return NETDEV_TX_OK;
>  
>  dma_map_err:
> @@ -3514,27 +3539,41 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>   *  Description :
>   *  To look at the incoming frames and clear the tx resources.
>   */
> -static int stmmac_poll(struct napi_struct *napi, int budget)
> +static int stmmac_rx_poll(struct napi_struct *napi, int budget)
>  {
>  	struct stmmac_rx_queue *rx_q =
>  		container_of(napi, struct stmmac_rx_queue, napi);
>  	struct stmmac_priv *priv = rx_q->priv_data;
> -	u32 tx_count = priv->plat->tx_queues_to_use;
>  	u32 chan = rx_q->queue_index;
>  	int work_done = 0;
> -	u32 queue;
>  
>  	priv->xstats.napi_poll++;
>  
> -	/* check all the queues */
> -	for (queue = 0; queue < tx_count; queue++)
> -		stmmac_tx_clean(priv, queue);
> -
>  	work_done = stmmac_rx(priv, budget, rx_q->queue_index);
> +	if (work_done < budget && napi_complete_done(napi, work_done))
> +		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
> +
> +	return work_done;
> +}
> +
> +static int stmmac_tx_poll(struct napi_struct *napi, int budget)
> +{
> +	struct stmmac_tx_queue *tx_q =
> +		container_of(napi, struct stmmac_tx_queue, napi);
> +	struct stmmac_priv *priv = tx_q->priv_data;
> +	u32 chan = tx_q->queue_index;
> +	int work_done = 0;
> +	bool more;
> +
> +	priv->xstats.napi_poll++;
> +
> +	work_done = stmmac_tx_clean(priv, budget, chan, &more);
>  	if (work_done < budget) {
>  		napi_complete_done(napi, work_done);
> -		stmmac_enable_dma_irq(priv, priv->ioaddr, chan);
> +		if (more)
> +			napi_reschedule(napi);
>  	}
> +
>  	return work_done;
>  }
>  
> @@ -4325,10 +4364,17 @@ int stmmac_dvr_probe(struct device *device,
>  	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
>  		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
>  
> -		netif_napi_add(ndev, &rx_q->napi, stmmac_poll,
> +		netif_napi_add(ndev, &rx_q->napi, stmmac_rx_poll,
>  			       (8 * priv->plat->rx_queues_to_use));
>  	}
>  
> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		netif_napi_add(ndev, &tx_q->napi, stmmac_tx_poll,
> +			       (8 * priv->plat->tx_queues_to_use));
> +	}
> +
>  	mutex_init(&priv->lock);
>  
>  	/* If a specific clk_csr value is passed from the platform
> @@ -4377,6 +4423,11 @@ int stmmac_dvr_probe(struct device *device,
>  
>  		netif_napi_del(&rx_q->napi);
>  	}
> +	for (queue = 0; queue < priv->plat->tx_queues_to_use; queue++) {
> +		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +
> +		netif_napi_del(&tx_q->napi);
> +	}
>  error_hw_init:
>  	destroy_workqueue(priv->wq);
>  error_wq:
> 

^ permalink raw reply

* Re: [PATCH net 00/13] cls_u32 cleanups and fixes.
From: Jamal Hadi Salim @ 2018-09-10 11:30 UTC (permalink / raw)
  To: Al Viro; +Cc: netdev, Cong Wang, Jiri Pirko
In-Reply-To: <20180909141538.GG19965@ZenIV.linux.org.uk>

On 2018-09-09 10:15 a.m., Al Viro wrote:

[..]

> Umm...  Interesting - TCA_U32_SEL is not the only thing that
> gets ignored there; TCA_U32_MARK gets the same treatment.
> And then there's a lovely question what to do with n->pf -
> it's an array of n->sel.nkeys counters, and apparently we
> want (at least in common cases) to avoid resetting those.

> *If* we declare that ->nkeys mismatch means failure, it's
> all relatively easy to implement.  Alternatively, we could
> declare that selector change means resetting the stats.
> Preferences?

I am now conflicted. I have sample scripts which showed
that at one point that worked. All of them seem to be
indicating the nkeys and stats never changed. So that
could be a starting point; however, if a policy changes
the match tuples then it is no longer the same rule imo.

Note: you can change the "actions" - of which the most
primitive is "set classid x:y" without changing what is
being matched. i.e changing the classid in that example
would work.

cheers,
jamal

^ permalink raw reply

* Re: [net-next, PATCH 2/2, v1] net: socionext: add AF_XDP support
From: Ilias Apalodimas @ 2018-09-10 11:20 UTC (permalink / raw)
  To: Toshiaki Makita
  Cc: netdev, jaswinder.singh, ard.biesheuvel, masami.hiramatsu, arnd,
	mykyta.iziumtsev, bjorn.topel, magnus.karlsson
In-Reply-To: <8bfd8219-acea-8b63-b6be-d17a7e3b6e24@lab.ntt.co.jp>

On Mon, Sep 10, 2018 at 07:56:49PM +0900, Toshiaki Makita wrote:
> On 2018/09/10 17:24, Ilias Apalodimas wrote:
> > Add basic AF_XDP support without zero-copy
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> ...
> > @@ -707,6 +731,26 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
> >  		if (unlikely(!buf_addr))
> >  			break;
> >  
> > +		if (xdp_prog) {
> > +			xdp_result = netsec_run_xdp(desc, priv, xdp_prog,
> > +						    pkt_len);
> > +			if (xdp_result != NETSEC_XDP_PASS) {
> > +				xdp_flush |= xdp_result & NETSEC_XDP_REDIR;
> > +
> > +				dma_unmap_single_attrs(priv->dev,
> > +						       desc->dma_addr,
> > +						       desc->len, DMA_TO_DEVICE,
> > +						       DMA_ATTR_SKIP_CPU_SYNC);
> > +
> > +				desc->len = desc_len;
> > +				desc->dma_addr = dma_handle;
> > +				desc->addr = buf_addr;
> > +				netsec_rx_fill(priv, idx, 1);
> > +				nsetsec_adv_desc(&dring->tail);
> > +			}
> > +			continue;
> 
> Continue even on XDP_PASS? Is this really correct?
> 
> Also seems there is no handling of adjust_head/tail for XDP_PASS case.
No continue is wrong there, thanks for the catch. This should return the packet
to the network stack. I'll fix it on v2.

> 
> > +		}
> > +
> >  		skb = build_skb(desc->addr, desc->len);
> >  		if (unlikely(!skb)) {
> >  			dma_unmap_single(priv->dev, dma_handle, desc_len,
> > @@ -740,6 +784,9 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
> >  		nsetsec_adv_desc(&dring->tail);
> >  	}
> >  
> > +	if (xdp_flush & NETSEC_XDP_REDIR)
> > +		xdp_do_flush_map();
> > +
> >  	return done;
> >  }
> ...
> > +static u32 netsec_run_xdp(struct netsec_desc *desc, struct netsec_priv *priv,
> > +			  struct bpf_prog *prog, u16 len)
> > +
> > +{
> > +	struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_RX];
> > +	struct xdp_buff xdp;
> > +	u32 ret = NETSEC_XDP_PASS;
> > +	int err;
> > +	u32 act;
> > +
> > +	xdp.data_hard_start = desc->addr;
> > +	xdp.data = desc->addr;
> 
> There is no headroom. REDIRECT using devmap/cpumap will fail due to
> this. Generally we need XDP_PACKET_HEADROOM.
> 
Ok, i'll modify the patch to include XDP_PACKET_HEADROOM
> > +	xdp_set_data_meta_invalid(&xdp);
> > +	xdp.data_end = xdp.data + len;
> > +	xdp.rxq = &dring->xdp_rxq;
> > +
> > +	rcu_read_lock();
> > +	act = bpf_prog_run_xdp(prog, &xdp);
> > +
> > +	switch (act) {
> > +	case XDP_PASS:
> > +		ret = NETSEC_XDP_PASS;
> > +		break;
> > +	case XDP_TX:
> > +		ret = netsec_xmit_xdp(priv, &xdp, desc);
> > +		break;
> > +	case XDP_REDIRECT:
> > +		err = xdp_do_redirect(priv->ndev, &xdp, prog);
> > +		if (!err) {
> > +			ret = NETSEC_XDP_REDIR;
> > +		} else {
> > +			ret = NETSEC_XDP_CONSUMED;
> > +			xdp_return_buff(&xdp);
> > +		}
> > +		break;
> > +	default:
> > +		bpf_warn_invalid_xdp_action(act);
> > +		/* fall through */
> > +	case XDP_ABORTED:
> > +		trace_xdp_exception(priv->ndev, prog, act);
> > +		/* fall through -- handle aborts by dropping packet */
> > +	case XDP_DROP:
> > +		ret = NETSEC_XDP_CONSUMED;
> > +		break;
> > +	}
> > +
> > +	rcu_read_unlock();
> > +
> > +	return ret;
> > +}
> > +
> > +static int netsec_xdp_setup(struct netsec_priv *priv, struct bpf_prog *prog)
> > +{
> > +	struct net_device *dev = priv->ndev;
> > +	struct bpf_prog *old_prog;
> > +
> > +	/* For now just support only the usual MTU sized frames */
> > +	if (prog && dev->mtu > 1500) {
> > +		netdev_warn(dev, "Jumbo frames not yet supported with XDP\n");
> 
> Why not using extack?
> 
Wasn't aware that this should use the extack for errors. I just saw the same
check on an existing driver and thought that was the way to go
> > +		return -EOPNOTSUPP;
> > +	}
> > +

Thanks for having a look!

/Ilias

^ permalink raw reply

* 3% Loan Offer Apply Now
From: Mr. Passy Ben @ 2018-09-09 17:00 UTC (permalink / raw)
  To: Recipients

ARE YOU IN NEED OF LOAN @3% INTEREST RATE FOR BUSINESS AND PRIVATE
PURPOSES? IF YES:
FILL AND RETURN
Name:===
Amount needed:===
Duration:==
country:===
Purpose:===
Mobile number

^ permalink raw reply

* Re: [net-next, PATCH 2/2, v1] net: socionext: add AF_XDP support
From: Toshiaki Makita @ 2018-09-10 10:56 UTC (permalink / raw)
  To: Ilias Apalodimas, netdev, jaswinder.singh
  Cc: ard.biesheuvel, masami.hiramatsu, arnd, mykyta.iziumtsev,
	bjorn.topel, magnus.karlsson
In-Reply-To: <1536567880-15097-3-git-send-email-ilias.apalodimas@linaro.org>

On 2018/09/10 17:24, Ilias Apalodimas wrote:
> Add basic AF_XDP support without zero-copy
> 
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
...
> @@ -707,6 +731,26 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
>  		if (unlikely(!buf_addr))
>  			break;
>  
> +		if (xdp_prog) {
> +			xdp_result = netsec_run_xdp(desc, priv, xdp_prog,
> +						    pkt_len);
> +			if (xdp_result != NETSEC_XDP_PASS) {
> +				xdp_flush |= xdp_result & NETSEC_XDP_REDIR;
> +
> +				dma_unmap_single_attrs(priv->dev,
> +						       desc->dma_addr,
> +						       desc->len, DMA_TO_DEVICE,
> +						       DMA_ATTR_SKIP_CPU_SYNC);
> +
> +				desc->len = desc_len;
> +				desc->dma_addr = dma_handle;
> +				desc->addr = buf_addr;
> +				netsec_rx_fill(priv, idx, 1);
> +				nsetsec_adv_desc(&dring->tail);
> +			}
> +			continue;

Continue even on XDP_PASS? Is this really correct?

Also seems there is no handling of adjust_head/tail for XDP_PASS case.

> +		}
> +
>  		skb = build_skb(desc->addr, desc->len);
>  		if (unlikely(!skb)) {
>  			dma_unmap_single(priv->dev, dma_handle, desc_len,
> @@ -740,6 +784,9 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
>  		nsetsec_adv_desc(&dring->tail);
>  	}
>  
> +	if (xdp_flush & NETSEC_XDP_REDIR)
> +		xdp_do_flush_map();
> +
>  	return done;
>  }
...
> +static u32 netsec_run_xdp(struct netsec_desc *desc, struct netsec_priv *priv,
> +			  struct bpf_prog *prog, u16 len)
> +
> +{
> +	struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_RX];
> +	struct xdp_buff xdp;
> +	u32 ret = NETSEC_XDP_PASS;
> +	int err;
> +	u32 act;
> +
> +	xdp.data_hard_start = desc->addr;
> +	xdp.data = desc->addr;

There is no headroom. REDIRECT using devmap/cpumap will fail due to
this. Generally we need XDP_PACKET_HEADROOM.

> +	xdp_set_data_meta_invalid(&xdp);
> +	xdp.data_end = xdp.data + len;
> +	xdp.rxq = &dring->xdp_rxq;
> +
> +	rcu_read_lock();
> +	act = bpf_prog_run_xdp(prog, &xdp);
> +
> +	switch (act) {
> +	case XDP_PASS:
> +		ret = NETSEC_XDP_PASS;
> +		break;
> +	case XDP_TX:
> +		ret = netsec_xmit_xdp(priv, &xdp, desc);
> +		break;
> +	case XDP_REDIRECT:
> +		err = xdp_do_redirect(priv->ndev, &xdp, prog);
> +		if (!err) {
> +			ret = NETSEC_XDP_REDIR;
> +		} else {
> +			ret = NETSEC_XDP_CONSUMED;
> +			xdp_return_buff(&xdp);
> +		}
> +		break;
> +	default:
> +		bpf_warn_invalid_xdp_action(act);
> +		/* fall through */
> +	case XDP_ABORTED:
> +		trace_xdp_exception(priv->ndev, prog, act);
> +		/* fall through -- handle aborts by dropping packet */
> +	case XDP_DROP:
> +		ret = NETSEC_XDP_CONSUMED;
> +		break;
> +	}
> +
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> +
> +static int netsec_xdp_setup(struct netsec_priv *priv, struct bpf_prog *prog)
> +{
> +	struct net_device *dev = priv->ndev;
> +	struct bpf_prog *old_prog;
> +
> +	/* For now just support only the usual MTU sized frames */
> +	if (prog && dev->mtu > 1500) {
> +		netdev_warn(dev, "Jumbo frames not yet supported with XDP\n");

Why not using extack?

> +		return -EOPNOTSUPP;
> +	}
> +

-- 
Toshiaki Makita

^ permalink raw reply

* Re: [PATCH] net: ipv4: Use BUG_ON directly instead of a if condition followed by BUG
From: Jiri Benc @ 2018-09-10 15:39 UTC (permalink / raw)
  To: zhong jiang; +Cc: davem, edumazet, kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <1536590282-23899-1-git-send-email-zhongjiang@huawei.com>

On Mon, 10 Sep 2018 22:38:02 +0800, zhong jiang wrote:
> +			BUG(skb_copy_bits(skb, ptr, &tmp, 1));

I doubt this builds.

 Jiri

^ permalink raw reply

* Re: kernels > v4.12 oops/crash with ipsec-traffic: bisected to b838d5e1c5b6e57b10ec8af2268824041e3ea911: ipv4: mark DST_NOGC and remove the operation of dst_free()
From: Wolfgang Walter @ 2018-09-10 10:46 UTC (permalink / raw)
  To: Kristian Evensen
  Cc: Steffen Klassert, Network Development, weiwan, Tobias Hommel,
	edumazet
In-Reply-To: <CAKfDRXg7EJeoO--QFFEJLtSPqRVngy4etSQHjSkQwNnhc4RFRA@mail.gmail.com>

Am Montag, 10. September 2018, 10:18:47 schrieb Kristian Evensen:
> Hi,
> 
> Thanks everyone for all the effort in debugging this issue.
> 
> On Mon, Sep 10, 2018 at 8:39 AM Steffen Klassert
> 
> <steffen.klassert@secunet.com> wrote:
> > The easy fix that could be backported to stable would be
> > to check skb->dst for NULL and drop the packet in that case.
> 
> Thought I should just chime in and say that we deployed this
> work-around when we started observing the error back in June. Since
> then we have not seen any crashes. Also, we have instrumented some of
> our kernels to count the number of times the error is hit (overall +
> consecutive). Compared to the overall number of packets, the error
> happens very rarely. With our workloads, we on average see the error
> once every couple of days.
> 

Would you mind send us yout patch (with the accounting) so that we can check 
how often that happens here?

Regards,
-- 
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts

^ permalink raw reply

* 3% Loan Offer Apply Now
From: Mr. Passy Ben @ 2018-09-09 15:10 UTC (permalink / raw)
  To: Recipients

ARE YOU IN NEED OF LOAN @3% INTEREST RATE FOR BUSINESS AND PRIVATE
PURPOSES? IF YES:
FILL AND RETURN
Name:===
Amount needed:===
Duration:==
country:===
Purpose:===
Mobile number

^ permalink raw reply

* [PATCH v2 1/3][can-next] can: rcar_can: Fix erroneous registration
From: Fabrizio Castro @ 2018-09-10 10:43 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: Fabrizio Castro, David S. Miller, Sergei Shtylyov, Chris Paterson,
	linux-can, netdev, Simon Horman, Geert Uytterhoeven, Biju Das,
	linux-renesas-soc
In-Reply-To: <1536576195-11520-1-git-send-email-fabrizio.castro@bp.renesas.com>

Assigning 2 to "renesas,can-clock-select" tricks the driver into
registering the CAN interface, even though we don't want that.
This patch improves one of the checks to prevent that from happening.

Fixes: 862e2b6af9413b43 ("can: rcar_can: support all input clocks")
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
---

v1->v2:
* dropped id specific data as per Geert's comments

This patch applies on top of linux-can-next-for-4.19-20180727.

 drivers/net/can/rcar/rcar_can.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
index 11662f4..771a460 100644
--- a/drivers/net/can/rcar/rcar_can.c
+++ b/drivers/net/can/rcar/rcar_can.c
@@ -24,6 +24,9 @@
 
 #define RCAR_CAN_DRV_NAME	"rcar_can"
 
+#define RCAR_SUPPORTED_CLOCKS	(BIT(CLKR_CLKP1) | BIT(CLKR_CLKP2) | \
+				 BIT(CLKR_CLKEXT))
+
 /* Mailbox configuration:
  * mailbox 60 - 63 - Rx FIFO mailboxes
  * mailbox 56 - 59 - Tx FIFO mailboxes
@@ -789,7 +792,7 @@ static int rcar_can_probe(struct platform_device *pdev)
 		goto fail_clk;
 	}
 
-	if (clock_select >= ARRAY_SIZE(clock_names)) {
+	if (!(BIT(clock_select) & RCAR_SUPPORTED_CLOCKS)) {
 		err = -EINVAL;
 		dev_err(&pdev->dev, "invalid CAN clock selected\n");
 		goto fail_clk;
-- 
2.7.4

^ permalink raw reply related

* Re: [PATCH] ip6_gre: simplify gre header parsing in ip6gre_err
From: Jiri Benc @ 2018-09-10 15:36 UTC (permalink / raw)
  To: Haishuang Yan; +Cc: David S. Miller, Alexey Kuznetsov, netdev, linux-kernel
In-Reply-To: <1536567909-25089-1-git-send-email-yanhaishuang@cmss.chinamobile.com>

On Mon, 10 Sep 2018 16:25:09 +0800, Haishuang Yan wrote:
> +	if (gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IPV6),
> +			     offset) < 0) {
> +		if (!csum_err)		/* ignore csum errors. */
> +			return;
>  	}

gre_parse_header stops parsing when csum_err is encountered. Which
means tpi.key is undefined...

>  
> -	if (!pskb_may_pull(skb, offset + grehlen))
> -		return;
>  	ipv6h = (const struct ipv6hdr *)skb->data;
> -	greh = (const struct gre_base_hdr *)(skb->data + offset);
> -	key = key_off ? *(__be32 *)(skb->data + key_off) : 0;
> -
>  	t = ip6gre_tunnel_lookup(skb->dev, &ipv6h->daddr, &ipv6h->saddr,
> -				 key, greh->protocol);
> +				 tpi.key, tpi.proto);

...and can't be used here.

 Jiri

^ permalink raw reply

* [PATCH net-next] net: bridge: add support for sticky fdb entries
From: Nikolay Aleksandrov @ 2018-09-10 10:16 UTC (permalink / raw)
  To: netdev; +Cc: roopa, davem, stephen, bridge, Nikolay Aleksandrov

Add support for entries which are "sticky", i.e. will not change their port
if they show up from a different one. A new ndm flag is introduced for that
purpose - NTF_STICKY. We allow to set it only to non-local entries.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
I'll send the selftest for sticky with the iproute2 patch if this one is
accepted. We've had multiple requests to support such flag and now it's
also needed for some eVPN and clag setups.

 include/uapi/linux/neighbour.h |  1 +
 net/bridge/br_fdb.c            | 19 ++++++++++++++++---
 net/bridge/br_private.h        |  1 +
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 904db6148476..998155444e0d 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -43,6 +43,7 @@ enum {
 #define NTF_PROXY	0x08	/* == ATF_PUBL */
 #define NTF_EXT_LEARNED	0x10
 #define NTF_OFFLOADED   0x20
+#define NTF_STICKY	0x40
 #define NTF_ROUTER	0x80
 
 /*
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 502f66349530..26569ed06a4d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -584,7 +584,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 			unsigned long now = jiffies;
 
 			/* fastpath: update of existing entry */
-			if (unlikely(source != fdb->dst)) {
+			if (unlikely(source != fdb->dst && !fdb->is_sticky)) {
 				fdb->dst = source;
 				fdb_modified = true;
 				/* Take over HW learned entry */
@@ -656,6 +656,8 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
 		ndm->ndm_flags |= NTF_OFFLOADED;
 	if (fdb->added_by_external_learn)
 		ndm->ndm_flags |= NTF_EXT_LEARNED;
+	if (fdb->is_sticky)
+		ndm->ndm_flags |= NTF_STICKY;
 
 	if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
 		goto nla_put_failure;
@@ -772,7 +774,8 @@ int br_fdb_dump(struct sk_buff *skb,
 
 /* Update (create or replace) forwarding database entry */
 static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
-			 const __u8 *addr, __u16 state, __u16 flags, __u16 vid)
+			 const u8 *addr, u16 state, u16 flags, u16 vid,
+			 u8 is_sticky)
 {
 	struct net_bridge_fdb_entry *fdb;
 	bool modified = false;
@@ -789,6 +792,9 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 		return -EINVAL;
 	}
 
+	if (is_sticky && (state & NUD_PERMANENT))
+		return -EINVAL;
+
 	fdb = br_fdb_find(br, addr, vid);
 	if (fdb == NULL) {
 		if (!(flags & NLM_F_CREATE))
@@ -832,6 +838,12 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 
 		modified = true;
 	}
+
+	if (is_sticky != fdb->is_sticky) {
+		fdb->is_sticky = is_sticky;
+		modified = true;
+	}
+
 	fdb->added_by_user = 1;
 
 	fdb->used = jiffies;
@@ -865,7 +877,8 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
 	} else {
 		spin_lock_bh(&br->hash_lock);
 		err = fdb_add_entry(br, p, addr, ndm->ndm_state,
-				    nlh_flags, vid);
+				    nlh_flags, vid,
+				    !!(ndm->ndm_flags & NTF_STICKY));
 		spin_unlock_bh(&br->hash_lock);
 	}
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 11ed2029985f..d21035a17f4c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -181,6 +181,7 @@ struct net_bridge_fdb_entry {
 	struct hlist_node		fdb_node;
 	unsigned char			is_local:1,
 					is_static:1,
+					is_sticky:1,
 					added_by_user:1,
 					added_by_external_learn:1,
 					offloaded:1;
-- 
2.11.0

^ permalink raw reply related

* Re: [PATCH net-next] tcp: show number of network segments in some SNMP counters
From: kbuild test robot @ 2018-09-10 15:07 UTC (permalink / raw)
  To: Yafang Shao
  Cc: kbuild-all, edumazet, davem, netdev, linux-kernel, Yafang Shao
In-Reply-To: <1536425898-12059-1-git-send-email-laoar.shao@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2444 bytes --]

Hi Yafang,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Yafang-Shao/tcp-show-number-of-network-segments-in-some-SNMP-counters/20180910-225108
config: x86_64-randconfig-x019-201836 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   In file included from net/ipv4/tcp_ipv4.c:69:0:
   net/ipv4/tcp_ipv4.c: In function 'tcp_v4_err':
>> include/net/tcp.h:917:24: warning: 'skb' may be used uninitialized in this function [-Wmaybe-uninitialized]
     return TCP_SKB_CB(skb)->tcp_gso_segs;
                           ^
   net/ipv4/tcp_ipv4.c:436:18: note: 'skb' was declared here
     struct sk_buff *skb;
                     ^~~
--
   In file included from net//ipv4/tcp_ipv4.c:69:0:
   net//ipv4/tcp_ipv4.c: In function 'tcp_v4_err':
>> include/net/tcp.h:917:24: warning: 'skb' may be used uninitialized in this function [-Wmaybe-uninitialized]
     return TCP_SKB_CB(skb)->tcp_gso_segs;
                           ^
   net//ipv4/tcp_ipv4.c:436:18: note: 'skb' was declared here
     struct sk_buff *skb;
                     ^~~

vim +/skb +917 include/net/tcp.h

3fa6f616 David Ahern    2017-08-07  911  
^1da177e Linus Torvalds 2005-04-16  912  /* Due to TSO, an SKB can be composed of multiple actual
^1da177e Linus Torvalds 2005-04-16  913   * packets.  To keep these tracked properly, we use this.
bd14b1b2 Eric Dumazet   2012-05-04  914   */
^1da177e Linus Torvalds 2005-04-16  915  static inline int tcp_skb_pcount(const struct sk_buff *skb)
bd14b1b2 Eric Dumazet   2012-05-04  916  {
cd7d8498 Eric Dumazet   2014-09-24 @917  	return TCP_SKB_CB(skb)->tcp_gso_segs;
cd7d8498 Eric Dumazet   2014-09-24  918  }
bd14b1b2 Eric Dumazet   2012-05-04  919  

:::::: The code at line 917 was first introduced by commit
:::::: cd7d8498c9a5d510c64db38d9f4f4fbc41790f09 tcp: change tcp_skb_pcount() location

:::::: TO: Eric Dumazet <edumazet@google.com>
:::::: CC: David S. Miller <davem@davemloft.net>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30784 bytes --]

^ permalink raw reply

* Re: [net-next, PATCH 0/2, v1] net: socionext: add AF_XDP support
From: Björn Töpel @ 2018-09-10 10:06 UTC (permalink / raw)
  To: Ilias Apalodimas
  Cc: Netdev, jaswinder.singh, ard.biesheuvel, masami.hiramatsu,
	Arnd Bergmann, MykytaI Iziumtsev, Björn Töpel,
	Karlsson, Magnus
In-Reply-To: <1536567880-15097-1-git-send-email-ilias.apalodimas@linaro.org>

Den mån 10 sep. 2018 kl 10:26 skrev Ilias Apalodimas
<ilias.apalodimas@linaro.org>:
>
> This patch series adds AF_XDP support socionext netsec driver
>
> - patch [1/2]: Use a different allocation scheme for Rx DMA buffers to prepare
> the driver for AF_XDP support
> - patch [2/2]: Add AF_XDP support without zero-copy
>
> Ilias Apalodimas (2):
>   net: socionext: different approach on DMA
>   net: socionext: add AF_XDP support
>

You should probably rephrase patch #2. You are adding XDP support, and
AF_XDP just follows from that. Nice to see more XDP support!


Björn

>  drivers/net/ethernet/socionext/netsec.c | 444 +++++++++++++++++++++++---------
>  1 file changed, 329 insertions(+), 115 deletions(-)
>
> --
> 2.7.4
>

^ permalink raw reply

* [afaerber:lora-next 14445/14606] net//smc/smc_ib.c:152:2: warning: 'ib_query_gid' is deprecated (declared at include/rdma/ib_cache.h:139)
From: kbuild test robot @ 2018-09-10 10:04 UTC (permalink / raw)
  To: David S. Miller; +Cc: kbuild-all, netdev

[-- Attachment #1: Type: text/plain, Size: 3301 bytes --]

tree:   git://github.com/afaerber/linux lora-next
head:   51325f8cd6dd80cba7ac6e881fe523ad0475c927
commit: 6a991b35a2fa0b75d99f03c564b40b6b9a5d2aed [14445/14606] Merge remote-tracking branch 'net-next/master'
config: i386-randconfig-j0-09101243 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        git checkout 6a991b35a2fa0b75d99f03c564b40b6b9a5d2aed
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   net//smc/smc_ib.c: In function 'smc_ib_fill_mac':
>> net//smc/smc_ib.c:152:2: warning: 'ib_query_gid' is deprecated (declared at include/rdma/ib_cache.h:139) [-Wdeprecated-declarations]
     rc = ib_query_gid(smcibdev->ibdev, ibport, 0, &gid, &gattr);
     ^
   net//smc/smc_ib.c: In function 'smc_ib_determine_gid':
   net//smc/smc_ib.c:190:3: warning: 'ib_query_gid' is deprecated (declared at include/rdma/ib_cache.h:139) [-Wdeprecated-declarations]
      if (ib_query_gid(smcibdev->ibdev, ibport, i, &_gid, &gattr))
      ^
   net//smc/smc_ib.c:190:3: warning: 'ib_query_gid' is deprecated (declared at include/rdma/ib_cache.h:139) [-Wdeprecated-declarations]
   In file included from include/linux/kernel.h:10:0,
                    from include/linux/list.h:9,
                    from include/linux/random.h:10,
                    from net//smc/smc_ib.c:15:
   include/linux/compiler.h:61:17: warning: 'ib_query_gid' is deprecated (declared at include/rdma/ib_cache.h:139) [-Wdeprecated-declarations]
      static struct ftrace_branch_data   \
                    ^
   include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^
   net//smc/smc_ib.c:190:3: note: in expansion of macro 'if'
      if (ib_query_gid(smcibdev->ibdev, ibport, i, &_gid, &gattr))
      ^

vim +/ib_query_gid +152 net//smc/smc_ib.c

bd4ad577 Ursula Braun 2017-01-09  145  
7005ada6 Ursula Braun 2018-07-25  146  static int smc_ib_fill_mac(struct smc_ib_device *smcibdev, u8 ibport)
be6a3f38 Ursula Braun 2018-06-28  147  {
be6a3f38 Ursula Braun 2018-06-28  148  	struct ib_gid_attr gattr;
7005ada6 Ursula Braun 2018-07-25  149  	union ib_gid gid;
be6a3f38 Ursula Braun 2018-06-28  150  	int rc;
be6a3f38 Ursula Braun 2018-06-28  151  
7005ada6 Ursula Braun 2018-07-25 @152  	rc = ib_query_gid(smcibdev->ibdev, ibport, 0, &gid, &gattr);
be6a3f38 Ursula Braun 2018-06-28  153  	if (rc || !gattr.ndev)
be6a3f38 Ursula Braun 2018-06-28  154  		return -ENODEV;
be6a3f38 Ursula Braun 2018-06-28  155  
be6a3f38 Ursula Braun 2018-06-28  156  	memcpy(smcibdev->mac[ibport - 1], gattr.ndev->dev_addr, ETH_ALEN);
be6a3f38 Ursula Braun 2018-06-28  157  	dev_put(gattr.ndev);
be6a3f38 Ursula Braun 2018-06-28  158  	return 0;
be6a3f38 Ursula Braun 2018-06-28  159  }
be6a3f38 Ursula Braun 2018-06-28  160  

:::::: The code at line 152 was first introduced by commit
:::::: 7005ada68d1774d7c1109deaba0c2cd8e46f5091 net/smc: use correct vlan gid of RoCE device

:::::: TO: Ursula Braun <ubraun@linux.ibm.com>
:::::: CC: David S. Miller <davem@davemloft.net>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29420 bytes --]

^ permalink raw reply

* RE: [PATCH 3/3] dt-bindings: can: rcar_can: Add r8a774a1 support
From: Fabrizio Castro @ 2018-09-10  9:54 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, Rob Herring, Mark Rutland,
	Sergei Shtylyov, David S. Miller, linux-can@vger.kernel.org,
	netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
	Linux-Renesas, Linux Kernel Mailing List
In-Reply-To: <CAMuHMdWPuqfXOgh_-qp8q6wATU3Lj_WpX0bAOxPBX76SKvyXCQ@mail.gmail.com>

Hello Geert,

Thank you for your feedback.

> Subject: Re: [PATCH 3/3] dt-bindings: can: rcar_can: Add r8a774a1 support
>
> Hi Fabrizio,
>
> On Thu, Aug 23, 2018 at 3:08 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:>
> > Document RZ/G2M (r8a774a1) SoC specific bindings and RZ/G2
> > generic bindings.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Chris Paterson <Chris.Paterson2@renesas.com>
> > Reviewed-by: Biju Das <biju.das@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
> > +++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> > @@ -4,6 +4,7 @@ Renesas R-Car CAN controller Device Tree Bindings
> >  Required properties:
> >  - compatible: "renesas,can-r8a7743" if CAN controller is a part of R8A7743 SoC.
> >               "renesas,can-r8a7745" if CAN controller is a part of R8A7745 SoC.
> > +             "renesas,can-r8a774a1" if CAN controller is a part of R8A774A1 SoC.
>
> Looks good to me.
>
> >               "renesas,can-r8a7778" if CAN controller is a part of R8A7778 SoC.
> >               "renesas,can-r8a7779" if CAN controller is a part of R8A7779 SoC.
> >               "renesas,can-r8a7790" if CAN controller is a part of R8A7790 SoC.
> > @@ -17,6 +18,7 @@ Required properties:
> >               "renesas,rcar-gen2-can" for a generic R-Car Gen2 or RZ/G1
> >               compatible device.
> >               "renesas,rcar-gen3-can" for a generic R-Car Gen3 compatible device.
> > +             "renesas,rzg-gen2-can" for a generic RZ/G2 compatible device.
>
> AFAIK, the actual CAN module in RZ/G2M is fully compatible with the CAN
> module in R-Car Gen3 SoCs. The lack of clkp2 is merely an integration
> difference: as RZ/G2 SoCs do not have the CANFD module, and their CPG block
> doesn't provide the CANFD clock (so the CAN device node in DT cannot refer
> to that clock anyway).
>
> Hence I don't think there's a need to introduce a "renesas,rzg-gen2-can"
> compatible value.

Agreed, will drop RZ/G2 specific compatible string.

>
> >               When compatible with the generic version, nodes must list the
> >               SoC-specific version corresponding to the platform first
> >               followed by the generic version.
> > @@ -24,7 +26,9 @@ Required properties:
> >  - reg: physical base address and size of the R-Car CAN register map.
> >  - interrupts: interrupt specifier for the sole interrupt.
> >  - clocks: phandles and clock specifiers for 3 CAN clock inputs.
>
> You still have "3" here. Perhaps
> "Must contain a phandle and clock-specifier pair for each entry in
> clock-names."?

Good spot, we overlooked it.

>
> > -- clock-names: 3 clock input name strings: "clkp1", "clkp2", "can_clk".
> > +- clock-names: 2 clock input name strings for RZ/G2: "clkp1", "can_clk".
> > +              3 clock input name strings for every other SoC: "clkp1", "clkp2",
> > +              "can_clk".
>
> OK.
>
> > @@ -41,8 +45,9 @@ using the below properties:
> >  Optional properties:
> >  - renesas,can-clock-select: R-Car CAN Clock Source Select. Valid values are:
> >                             <0x0> (default) : Peripheral clock (clkp1)
> > -                           <0x1> : Peripheral clock (clkp2)
> > -                           <0x3> : Externally input clock
> > +                           <0x1> : Peripheral clock (clkp2) (not supported by
> > +                                   RZ/G2 devices)
> > +                           <0x3> : External input clock
>
> I already expressed my feelings about this property in my reply to the first
> patch ;-)

I know, I am not super happy about it either, maybe we will get a proper solution for this at some point in the future.

Thanks,
Fab

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

^ 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