[PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[Ding Tianhong]

The patches series was used to fix the issues of the hip04 driver, and added
some optimizations according to some good suggestion. 

Thanks.

Ding Tianhong (6):
  net: hip04: Set more appropriate value for tx coalesce num
  net: hip04: use the big endian for tx and rx desc
  net: hip04: Solve the problem of the skb memory allocation failure
  net: hip04: Don't free the tx skb when the buffer is not ready for
    xmit
  net: hip04: remove the unnecessary check
  net: hip04: add ratelimit for rx/tx drops skb

 drivers/net/ethernet/hisilicon/hip04_eth.c | 109 ++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 33 deletions(-)

-- 
1.8.0

[PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[Ding Tianhong]

The patches series was used to fix the issues of the hip04 driver, and added
some optimizations according to some good suggestion. 

Thanks.

Ding Tianhong (6):
  net: hip04: Set more appropriate value for tx coalesce num
  net: hip04: use the big endian for tx and rx desc
  net: hip04: Solve the problem of the skb memory allocation failure
  net: hip04: Don't free the tx skb when the buffer is not ready for
    xmit
  net: hip04: remove the unnecessary check
  net: hip04: add ratelimit for rx/tx drops skb

 drivers/net/ethernet/hisilicon/hip04_eth.c | 109 ++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 33 deletions(-)

-- 
1.8.0

[PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[Ding Tianhong]

The patches series was used to fix the issues of the hip04 driver, and added
some optimizations according to some good suggestion. 

Thanks.

Ding Tianhong (6):
  net: hip04: Set more appropriate value for tx coalesce num
  net: hip04: use the big endian for tx and rx desc
  net: hip04: Solve the problem of the skb memory allocation failure
  net: hip04: Don't free the tx skb when the buffer is not ready for
    xmit
  net: hip04: remove the unnecessary check
  net: hip04: add ratelimit for rx/tx drops skb

 drivers/net/ethernet/hisilicon/hip04_eth.c | 109 ++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 33 deletions(-)

-- 
1.8.0

[PATCH net-next 1/6] net: hip04: Set more appropriate value for tx coalesce num

[Ding Tianhong]

According Arnd's suggestion, the user might set the tx coalesce value
in large range for different workloads, so we should define them to
appropriate value.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index b72d238..b0a7f03 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -126,10 +126,10 @@
 #define DRV_NAME			"hip04-ether"
 #define DRV_VERSION			"v1.0"
 
-#define HIP04_MAX_TX_COALESCE_USECS	200
-#define HIP04_MIN_TX_COALESCE_USECS	100
-#define HIP04_MAX_TX_COALESCE_FRAMES	200
-#define HIP04_MIN_TX_COALESCE_FRAMES	100
+#define HIP04_MAX_TX_COALESCE_USECS	100000
+#define HIP04_MIN_TX_COALESCE_USECS	1
+#define HIP04_MAX_TX_COALESCE_FRAMES	(TX_DESC_NUM - 1)
+#define HIP04_MIN_TX_COALESCE_FRAMES	1
 
 struct tx_desc {
 	u32 send_addr;
-- 
1.8.0

[PATCH net-next 1/6] net: hip04: Set more appropriate value for tx coalesce num

[Ding Tianhong]

According Arnd's suggestion, the user might set the tx coalesce value
in large range for different workloads, so we should define them to
appropriate value.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index b72d238..b0a7f03 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -126,10 +126,10 @@
 #define DRV_NAME			"hip04-ether"
 #define DRV_VERSION			"v1.0"
 
-#define HIP04_MAX_TX_COALESCE_USECS	200
-#define HIP04_MIN_TX_COALESCE_USECS	100
-#define HIP04_MAX_TX_COALESCE_FRAMES	200
-#define HIP04_MIN_TX_COALESCE_FRAMES	100
+#define HIP04_MAX_TX_COALESCE_USECS	100000
+#define HIP04_MIN_TX_COALESCE_USECS	1
+#define HIP04_MAX_TX_COALESCE_FRAMES	(TX_DESC_NUM - 1)
+#define HIP04_MIN_TX_COALESCE_FRAMES	1
 
 struct tx_desc {
 	u32 send_addr;
-- 
1.8.0

[PATCH net-next 1/6] net: hip04: Set more appropriate value for tx coalesce num

[Ding Tianhong]

According Arnd's suggestion, the user might set the tx coalesce value
in large range for different workloads, so we should define them to
appropriate value.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index b72d238..b0a7f03 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -126,10 +126,10 @@
 #define DRV_NAME			"hip04-ether"
 #define DRV_VERSION			"v1.0"
 
-#define HIP04_MAX_TX_COALESCE_USECS	200
-#define HIP04_MIN_TX_COALESCE_USECS	100
-#define HIP04_MAX_TX_COALESCE_FRAMES	200
-#define HIP04_MIN_TX_COALESCE_FRAMES	100
+#define HIP04_MAX_TX_COALESCE_USECS	100000
+#define HIP04_MIN_TX_COALESCE_USECS	1
+#define HIP04_MAX_TX_COALESCE_FRAMES	(TX_DESC_NUM - 1)
+#define HIP04_MIN_TX_COALESCE_FRAMES	1
 
 struct tx_desc {
 	u32 send_addr;
-- 
1.8.0

Re: [PATCH net-next 1/6] net: hip04: Set more appropriate value for tx coalesce num

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:03 Ding Tianhong wrote:
> According Arnd's suggestion, the user might set the tx coalesce value
> in large range for different workloads, so we should define them to
> appropriate value.
> 
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Joe Perches <joe@perches.com>
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH net-next 1/6] net: hip04: Set more appropriate value for tx coalesce num

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:03 Ding Tianhong wrote:
> According Arnd's suggestion, the user might set the tx coalesce value
> in large range for different workloads, so we should define them to
> appropriate value.
> 
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Joe Perches <joe@perches.com>
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Ding Tianhong]

Eric pointed out the problem that the tx skb might already have be freed
by tx completion before enter the xmit queue, so don't notice the tx completion
until the skb is ready to be freed.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 7533858..ff9d19c 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -480,8 +480,6 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	skb_tx_timestamp(skb);
 
 	hip04_set_xmit_desc(priv, phys);
-	priv->tx_head = TX_NEXT(tx_head);
-	count++;
 	netdev_sent_queue(ndev, skb->len);
 
 	stats->tx_bytes += skb->len;
@@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	/* Ensure tx_head update visible to tx reclaim */
 	smp_wmb();
+	count++;
+	priv->tx_head = TX_NEXT(tx_head);
 
 	/* queue is getting full, better start cleaning up now */
 	if (count >= priv->tx_coalesce_frames) {
-- 
1.8.0

[PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Ding Tianhong]

Eric pointed out the problem that the tx skb might already have be freed
by tx completion before enter the xmit queue, so don't notice the tx completion
until the skb is ready to be freed.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 7533858..ff9d19c 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -480,8 +480,6 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	skb_tx_timestamp(skb);
 
 	hip04_set_xmit_desc(priv, phys);
-	priv->tx_head = TX_NEXT(tx_head);
-	count++;
 	netdev_sent_queue(ndev, skb->len);
 
 	stats->tx_bytes += skb->len;
@@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	/* Ensure tx_head update visible to tx reclaim */
 	smp_wmb();
+	count++;
+	priv->tx_head = TX_NEXT(tx_head);
 
 	/* queue is getting full, better start cleaning up now */
 	if (count >= priv->tx_coalesce_frames) {
-- 
1.8.0

[PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Ding Tianhong]

Eric pointed out the problem that the tx skb might already have be freed
by tx completion before enter the xmit queue, so don't notice the tx completion
until the skb is ready to be freed.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 7533858..ff9d19c 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -480,8 +480,6 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	skb_tx_timestamp(skb);
 
 	hip04_set_xmit_desc(priv, phys);
-	priv->tx_head = TX_NEXT(tx_head);
-	count++;
 	netdev_sent_queue(ndev, skb->len);
 
 	stats->tx_bytes += skb->len;
@@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	/* Ensure tx_head update visible to tx reclaim */
 	smp_wmb();
+	count++;
+	priv->tx_head = TX_NEXT(tx_head);
 
 	/* queue is getting full, better start cleaning up now */
 	if (count >= priv->tx_coalesce_frames) {
-- 
1.8.0

Re: [PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>         /* Ensure tx_head update visible to tx reclaim */
>         smp_wmb();
> +       count++;
> +       priv->tx_head = TX_NEXT(tx_head);
>  
> 

Something is wrong here, the comment does not match the code any more, because
the smp_wmb() won't actually make the tx_head update visible.

	Arnd

[PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>         /* Ensure tx_head update visible to tx reclaim */
>         smp_wmb();
> +       count++;
> +       priv->tx_head = TX_NEXT(tx_head);
>  
> 

Something is wrong here, the comment does not match the code any more, because
the smp_wmb() won't actually make the tx_head update visible.

	Arnd

Re: [PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Ding Tianhong]

On 2015/4/15 22:19, Arnd Bergmann wrote:
> On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
>> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>  
>>         /* Ensure tx_head update visible to tx reclaim */
>>         smp_wmb();
>> +       count++;
>> +       priv->tx_head = TX_NEXT(tx_head);
>>  
>>
> 
> Something is wrong here, the comment does not match the code any more, because
> the smp_wmb() won't actually make the tx_head update visible.
> 
> 	Arnd
> 
Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit.

> .
> 


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

[PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Ding Tianhong]

On 2015/4/15 22:19, Arnd Bergmann wrote:
> On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
>> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>  
>>         /* Ensure tx_head update visible to tx reclaim */
>>         smp_wmb();
>> +       count++;
>> +       priv->tx_head = TX_NEXT(tx_head);
>>  
>>
> 
> Something is wrong here, the comment does not match the code any more, because
> the smp_wmb() won't actually make the tx_head update visible.
> 
> 	Arnd
> 
Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit.

> .
> 

Re: [PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Ding Tianhong]

On 2015/4/15 22:19, Arnd Bergmann wrote:
> On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
>> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>  
>>         /* Ensure tx_head update visible to tx reclaim */
>>         smp_wmb();
>> +       count++;
>> +       priv->tx_head = TX_NEXT(tx_head);
>>  
>>
> 
> Something is wrong here, the comment does not match the code any more, because
> the smp_wmb() won't actually make the tx_head update visible.
> 
> 	Arnd
> 
Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit.

> .
> 


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

Re: [PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Arnd Bergmann]

On Thursday 16 April 2015 14:26:21 Ding Tianhong wrote:
> On 2015/4/15 22:19, Arnd Bergmann wrote:
> > On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
> >> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> >>  
> >>         /* Ensure tx_head update visible to tx reclaim */
> >>         smp_wmb();
> >> +       count++;
> >> +       priv->tx_head = TX_NEXT(tx_head);
> >>  
> >>
> > 
> > Something is wrong here, the comment does not match the code any more, because
> > the smp_wmb() won't actually make the tx_head update visible.
> > 
> >       Arnd
> > 
> Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit.

The problem you need to avoid is that the tx reclaim function thinks it's
done with all outstanding packets and does not retrigger, while the
start_xmit thinks it will still get to that. This means you
need a barrier between the priv->tx_head update and the
napi_schedule_prep() call.

	Arnd

[PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Arnd Bergmann]

On Thursday 16 April 2015 14:26:21 Ding Tianhong wrote:
> On 2015/4/15 22:19, Arnd Bergmann wrote:
> > On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
> >> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> >>  
> >>         /* Ensure tx_head update visible to tx reclaim */
> >>         smp_wmb();
> >> +       count++;
> >> +       priv->tx_head = TX_NEXT(tx_head);
> >>  
> >>
> > 
> > Something is wrong here, the comment does not match the code any more, because
> > the smp_wmb() won't actually make the tx_head update visible.
> > 
> >       Arnd
> > 
> Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit.

The problem you need to avoid is that the tx reclaim function thinks it's
done with all outstanding packets and does not retrigger, while the
start_xmit thinks it will still get to that. This means you
need a barrier between the priv->tx_head update and the
napi_schedule_prep() call.

	Arnd

Re: [PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Ding Tianhong]

On 2015/4/16 16:41, Arnd Bergmann wrote:
> On Thursday 16 April 2015 14:26:21 Ding Tianhong wrote:
>> On 2015/4/15 22:19, Arnd Bergmann wrote:
>>> On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
>>>> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>>>  
>>>>         /* Ensure tx_head update visible to tx reclaim */
>>>>         smp_wmb();
>>>> +       count++;
>>>> +       priv->tx_head = TX_NEXT(tx_head);
>>>>  
>>>>
>>>
>>> Something is wrong here, the comment does not match the code any more, because
>>> the smp_wmb() won't actually make the tx_head update visible.
>>>
>>>       Arnd
>>>
>> Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit.
> 
> The problem you need to avoid is that the tx reclaim function thinks it's
> done with all outstanding packets and does not retrigger, while the
> start_xmit thinks it will still get to that. This means you
> need a barrier between the priv->tx_head update and the
> napi_schedule_prep() call.
> 

But I still doubt about that, when the buffer is ready to xmit, it will add to fifo queue, but it
doesn't mean it is finished xmit yet, the tx reclaim function will free the skb only
depend on whether tx_head is updated.

Ding

> 	Arnd
> 
> .
> 


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

[PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Ding Tianhong]

On 2015/4/16 16:41, Arnd Bergmann wrote:
> On Thursday 16 April 2015 14:26:21 Ding Tianhong wrote:
>> On 2015/4/15 22:19, Arnd Bergmann wrote:
>>> On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
>>>> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>>>  
>>>>         /* Ensure tx_head update visible to tx reclaim */
>>>>         smp_wmb();
>>>> +       count++;
>>>> +       priv->tx_head = TX_NEXT(tx_head);
>>>>  
>>>>
>>>
>>> Something is wrong here, the comment does not match the code any more, because
>>> the smp_wmb() won't actually make the tx_head update visible.
>>>
>>>       Arnd
>>>
>> Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit.
> 
> The problem you need to avoid is that the tx reclaim function thinks it's
> done with all outstanding packets and does not retrigger, while the
> start_xmit thinks it will still get to that. This means you
> need a barrier between the priv->tx_head update and the
> napi_schedule_prep() call.
> 

But I still doubt about that, when the buffer is ready to xmit, it will add to fifo queue, but it
doesn't mean it is finished xmit yet, the tx reclaim function will free the skb only
depend on whether tx_head is updated.

Ding

> 	Arnd
> 
> .
> 

Re: [PATCH net-next 4/6] net: hip04: Don't free the tx skb when the buffer is not ready for xmit

[Ding Tianhong]

On 2015/4/16 16:41, Arnd Bergmann wrote:
> On Thursday 16 April 2015 14:26:21 Ding Tianhong wrote:
>> On 2015/4/15 22:19, Arnd Bergmann wrote:
>>> On Wednesday 15 April 2015 20:30:06 Ding Tianhong wrote:
>>>> @@ -489,6 +487,8 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>>>  
>>>>         /* Ensure tx_head update visible to tx reclaim */
>>>>         smp_wmb();
>>>> +       count++;
>>>> +       priv->tx_head = TX_NEXT(tx_head);
>>>>  
>>>>
>>>
>>> Something is wrong here, the comment does not match the code any more, because
>>> the smp_wmb() won't actually make the tx_head update visible.
>>>
>>>       Arnd
>>>
>> Yes, the smp_wmb() could only make sure the tx buffer was ready to xmit.
> 
> The problem you need to avoid is that the tx reclaim function thinks it's
> done with all outstanding packets and does not retrigger, while the
> start_xmit thinks it will still get to that. This means you
> need a barrier between the priv->tx_head update and the
> napi_schedule_prep() call.
> 

But I still doubt about that, when the buffer is ready to xmit, it will add to fifo queue, but it
doesn't mean it is finished xmit yet, the tx reclaim function will free the skb only
depend on whether tx_head is updated.

Ding

> 	Arnd
> 
> .
> 


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

[PATCH net-next 2/6] net: hip04: use the big endian for tx and rx desc

[Ding Tianhong]

The hip04 ethernet use the big endian for tx and rx, so set desc to
big endian and remove the unused next_addr.

Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index b0a7f03..6473462 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -132,19 +132,18 @@
 #define HIP04_MIN_TX_COALESCE_FRAMES	1
 
 struct tx_desc {
-	u32 send_addr;
-	u32 send_size;
-	u32 next_addr;
-	u32 cfg;
-	u32 wb_addr;
+	__be32 send_addr;
+	__be32 send_size;
+	__be32 cfg;
+	__be32 wb_addr;
 } __aligned(64);
 
 struct rx_desc {
-	u16 reserved_16;
-	u16 pkt_len;
-	u32 reserve1[3];
-	u32 pkt_err;
-	u32 reserve2[4];
+	__be16 reserved_16;
+	__be16 pkt_len;
+	__be32 reserve1[3];
+	__be32 pkt_err;
+	__be32 reserve2[4];
 };
 
 struct hip04_priv {
-- 
1.8.0


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

[PATCH net-next 2/6] net: hip04: use the big endian for tx and rx desc

[Ding Tianhong]

The hip04 ethernet use the big endian for tx and rx, so set desc to
big endian and remove the unused next_addr.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index b0a7f03..6473462 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -132,19 +132,18 @@
 #define HIP04_MIN_TX_COALESCE_FRAMES	1
 
 struct tx_desc {
-	u32 send_addr;
-	u32 send_size;
-	u32 next_addr;
-	u32 cfg;
-	u32 wb_addr;
+	__be32 send_addr;
+	__be32 send_size;
+	__be32 cfg;
+	__be32 wb_addr;
 } __aligned(64);
 
 struct rx_desc {
-	u16 reserved_16;
-	u16 pkt_len;
-	u32 reserve1[3];
-	u32 pkt_err;
-	u32 reserve2[4];
+	__be16 reserved_16;
+	__be16 pkt_len;
+	__be32 reserve1[3];
+	__be32 pkt_err;
+	__be32 reserve2[4];
 };
 
 struct hip04_priv {
-- 
1.8.0

[PATCH net-next 2/6] net: hip04: use the big endian for tx and rx desc

[Ding Tianhong]

The hip04 ethernet use the big endian for tx and rx, so set desc to
big endian and remove the unused next_addr.

Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index b0a7f03..6473462 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -132,19 +132,18 @@
 #define HIP04_MIN_TX_COALESCE_FRAMES	1
 
 struct tx_desc {
-	u32 send_addr;
-	u32 send_size;
-	u32 next_addr;
-	u32 cfg;
-	u32 wb_addr;
+	__be32 send_addr;
+	__be32 send_size;
+	__be32 cfg;
+	__be32 wb_addr;
 } __aligned(64);
 
 struct rx_desc {
-	u16 reserved_16;
-	u16 pkt_len;
-	u32 reserve1[3];
-	u32 pkt_err;
-	u32 reserve2[4];
+	__be16 reserved_16;
+	__be16 pkt_len;
+	__be32 reserve1[3];
+	__be32 pkt_err;
+	__be32 reserve2[4];
 };
 
 struct hip04_priv {
-- 
1.8.0


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

Re: [PATCH net-next 2/6] net: hip04: use the big endian for tx and rx desc

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:04 Ding Tianhong wrote:
> The hip04 ethernet use the big endian for tx and rx, so set desc to
> big endian and remove the unused next_addr.

I don't understand:

> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index b0a7f03..6473462 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -132,19 +132,18 @@
>  #define HIP04_MIN_TX_COALESCE_FRAMES   1
>  
>  struct tx_desc {
> -       u32 send_addr;
> -       u32 send_size;
> -       u32 next_addr;
> -       u32 cfg;
> -       u32 wb_addr;
> +       __be32 send_addr;
> +       __be32 send_size;
> +       __be32 cfg;
> +       __be32 wb_addr;
>  } __aligned(64);

I would think this is a hardware structure, does this not break
access to the cfg and wb_addr fields if you remove next_addr?

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

[PATCH net-next 2/6] net: hip04: use the big endian for tx and rx desc

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:04 Ding Tianhong wrote:
> The hip04 ethernet use the big endian for tx and rx, so set desc to
> big endian and remove the unused next_addr.

I don't understand:

> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index b0a7f03..6473462 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -132,19 +132,18 @@
>  #define HIP04_MIN_TX_COALESCE_FRAMES   1
>  
>  struct tx_desc {
> -       u32 send_addr;
> -       u32 send_size;
> -       u32 next_addr;
> -       u32 cfg;
> -       u32 wb_addr;
> +       __be32 send_addr;
> +       __be32 send_size;
> +       __be32 cfg;
> +       __be32 wb_addr;
>  } __aligned(64);

I would think this is a hardware structure, does this not break
access to the cfg and wb_addr fields if you remove next_addr?

	Arnd

Re: [PATCH net-next 2/6] net: hip04: use the big endian for tx and rx desc

[Ding Tianhong]

On 2015/4/15 22:13, Arnd Bergmann wrote:
> On Wednesday 15 April 2015 20:30:04 Ding Tianhong wrote:
>> The hip04 ethernet use the big endian for tx and rx, so set desc to
>> big endian and remove the unused next_addr.
> 
> I don't understand:
> 
>> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
>> index b0a7f03..6473462 100644
>> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
>> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
>> @@ -132,19 +132,18 @@
>>  #define HIP04_MIN_TX_COALESCE_FRAMES   1
>>  
>>  struct tx_desc {
>> -       u32 send_addr;
>> -       u32 send_size;
>> -       u32 next_addr;
>> -       u32 cfg;
>> -       u32 wb_addr;
>> +       __be32 send_addr;
>> +       __be32 send_size;
>> +       __be32 cfg;
>> +       __be32 wb_addr;
>>  } __aligned(64);
> 
> I would think this is a hardware structure, does this not break
> access to the cfg and wb_addr fields if you remove next_addr?
> 
> 	Arnd
> 
good cache, I make a mistake here, thanks.

> .
> 

[PATCH net-next 2/6] net: hip04: use the big endian for tx and rx desc

[Ding Tianhong]

On 2015/4/15 22:13, Arnd Bergmann wrote:
> On Wednesday 15 April 2015 20:30:04 Ding Tianhong wrote:
>> The hip04 ethernet use the big endian for tx and rx, so set desc to
>> big endian and remove the unused next_addr.
> 
> I don't understand:
> 
>> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
>> index b0a7f03..6473462 100644
>> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
>> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
>> @@ -132,19 +132,18 @@
>>  #define HIP04_MIN_TX_COALESCE_FRAMES   1
>>  
>>  struct tx_desc {
>> -       u32 send_addr;
>> -       u32 send_size;
>> -       u32 next_addr;
>> -       u32 cfg;
>> -       u32 wb_addr;
>> +       __be32 send_addr;
>> +       __be32 send_size;
>> +       __be32 cfg;
>> +       __be32 wb_addr;
>>  } __aligned(64);
> 
> I would think this is a hardware structure, does this not break
> access to the cfg and wb_addr fields if you remove next_addr?
> 
> 	Arnd
> 
good cache, I make a mistake here, thanks.

> .
> 

Re: [PATCH net-next 2/6] net: hip04: use the big endian for tx and rx desc

[Ding Tianhong]

On 2015/4/15 22:13, Arnd Bergmann wrote:
> On Wednesday 15 April 2015 20:30:04 Ding Tianhong wrote:
>> The hip04 ethernet use the big endian for tx and rx, so set desc to
>> big endian and remove the unused next_addr.
> 
> I don't understand:
> 
>> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
>> index b0a7f03..6473462 100644
>> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
>> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
>> @@ -132,19 +132,18 @@
>>  #define HIP04_MIN_TX_COALESCE_FRAMES   1
>>  
>>  struct tx_desc {
>> -       u32 send_addr;
>> -       u32 send_size;
>> -       u32 next_addr;
>> -       u32 cfg;
>> -       u32 wb_addr;
>> +       __be32 send_addr;
>> +       __be32 send_size;
>> +       __be32 cfg;
>> +       __be32 wb_addr;
>>  } __aligned(64);
> 
> I would think this is a hardware structure, does this not break
> access to the cfg and wb_addr fields if you remove next_addr?
> 
> 	Arnd
> 
good cache, I make a mistake here, thanks.

> .
> 

[PATCH net-next 3/6] net: hip04: Solve the problem of the skb memory allocation failure

[Ding Tianhong]

The driver will alloc some skb buffer for hareware queue, but without
considering the case of memory allocation failure, when memory is low,
the skb may be null and panic the system, so break the loop when skb is
null and try to alloc the memory again to fix this problem.

Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 68 ++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 6473462..7533858 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -131,6 +131,8 @@
 #define HIP04_MAX_TX_COALESCE_FRAMES	(TX_DESC_NUM - 1)
 #define HIP04_MIN_TX_COALESCE_FRAMES	1
 
+#define HIP04_RX_BUFFER_WRITE		16
+
 struct tx_desc {
 	__be32 send_addr;
 	__be32 send_size;
@@ -180,6 +182,7 @@ struct hip04_priv {
 
 	/* written only by tx cleanup */
 	unsigned int tx_tail ____cacheline_aligned_in_smp;
+	unsigned int rx_tail ____cacheline_aligned_in_smp;
 };
 
 static inline unsigned int tx_count(unsigned int head, unsigned int tail)
@@ -187,6 +190,11 @@ static inline unsigned int tx_count(unsigned int head, unsigned int tail)
 	return (head - tail) % (TX_DESC_NUM - 1);
 }
 
+static inline unsigned int rx_count(unsigned int head, unsigned int tail)
+{
+	return (head - tail) % (RX_DESC_NUM - 1);
+}
+
 static void hip04_config_port(struct net_device *ndev, u32 speed, u32 duplex)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);
@@ -363,6 +371,35 @@ static int hip04_set_mac_address(struct net_device *ndev, void *addr)
 	return 0;
 }
 
+static int hip04_alloc_rx_buffers(struct net_device *ndev, int cleaned_count)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	unsigned char *buf;
+	dma_addr_t phys;
+	int i = priv->rx_tail;
+
+	while (cleaned_count) {
+		buf = netdev_alloc_frag(priv->rx_buf_size);
+		if (!buf)
+			break;
+
+		phys = dma_map_single(&ndev->dev, buf,
+				      RX_BUF_SIZE, DMA_FROM_DEVICE);
+		if (dma_mapping_error(&ndev->dev, phys))
+			break;
+
+		priv->rx_buf[i] = buf;
+		priv->rx_phys[i] = phys;
+		hip04_set_recv_desc(priv, phys);
+		i = RX_NEXT(i);
+		cleaned_count--;
+	}
+
+	priv->rx_tail = i;
+
+	return 0;
+}
+
 static int hip04_tx_reclaim(struct net_device *ndev, bool force)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);
@@ -482,8 +519,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 	struct sk_buff *skb;
 	unsigned char *buf;
 	bool last = false;
-	dma_addr_t phys;
-	int rx = 0;
+	int rx = 0, cleaned_count = 0;
 	int tx_remaining;
 	u16 len;
 	u32 err;
@@ -491,8 +527,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 	while (cnt && !last) {
 		buf = priv->rx_buf[priv->rx_head];
 		skb = build_skb(buf, priv->rx_buf_size);
-		if (unlikely(!skb))
+		if (unlikely(!skb)) {
 			net_dbg_ratelimited("build_skb failed\n");
+			goto done;
+		}
 
 		dma_unmap_single(&ndev->dev, priv->rx_phys[priv->rx_head],
 				 RX_BUF_SIZE, DMA_FROM_DEVICE);
@@ -519,18 +557,15 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 			rx++;
 		}
 
-		buf = netdev_alloc_frag(priv->rx_buf_size);
-		if (!buf)
-			goto done;
-		phys = dma_map_single(&ndev->dev, buf,
-				      RX_BUF_SIZE, DMA_FROM_DEVICE);
-		if (dma_mapping_error(&ndev->dev, phys))
-			goto done;
-		priv->rx_buf[priv->rx_head] = buf;
-		priv->rx_phys[priv->rx_head] = phys;
-		hip04_set_recv_desc(priv, phys);
-
 		priv->rx_head = RX_NEXT(priv->rx_head);
+
+		cleaned_count = rx_count(priv->rx_head, priv->rx_tail);
+		/* return some buffers to hardware , one at a time is too slow */
+		if (++cleaned_count >= HIP04_RX_BUFFER_WRITE) {
+			hip04_alloc_rx_buffers(ndev, cleaned_count);
+			cleaned_count = 0;
+		}
+
 		if (rx >= budget)
 			goto done;
 
@@ -545,6 +580,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 	}
 	napi_complete(napi);
 done:
+	cleaned_count = rx_count(priv->rx_head, priv->rx_tail);
+	if (cleaned_count)
+		hip04_alloc_rx_buffers(ndev, cleaned_count);
+
 	/* clean up tx descriptors and start a new timer if necessary */
 	tx_remaining = hip04_tx_reclaim(ndev, false);
 	if (rx < budget && tx_remaining)
@@ -621,6 +660,7 @@ static int hip04_mac_open(struct net_device *ndev)
 	int i;
 
 	priv->rx_head = 0;
+	priv->rx_tail = 0;
 	priv->tx_head = 0;
 	priv->tx_tail = 0;
 	hip04_reset_ppe(priv);
-- 
1.8.0


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

[PATCH net-next 3/6] net: hip04: Solve the problem of the skb memory allocation failure

[Ding Tianhong]

The driver will alloc some skb buffer for hareware queue, but without
considering the case of memory allocation failure, when memory is low,
the skb may be null and panic the system, so break the loop when skb is
null and try to alloc the memory again to fix this problem.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 68 ++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 6473462..7533858 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -131,6 +131,8 @@
 #define HIP04_MAX_TX_COALESCE_FRAMES	(TX_DESC_NUM - 1)
 #define HIP04_MIN_TX_COALESCE_FRAMES	1
 
+#define HIP04_RX_BUFFER_WRITE		16
+
 struct tx_desc {
 	__be32 send_addr;
 	__be32 send_size;
@@ -180,6 +182,7 @@ struct hip04_priv {
 
 	/* written only by tx cleanup */
 	unsigned int tx_tail ____cacheline_aligned_in_smp;
+	unsigned int rx_tail ____cacheline_aligned_in_smp;
 };
 
 static inline unsigned int tx_count(unsigned int head, unsigned int tail)
@@ -187,6 +190,11 @@ static inline unsigned int tx_count(unsigned int head, unsigned int tail)
 	return (head - tail) % (TX_DESC_NUM - 1);
 }
 
+static inline unsigned int rx_count(unsigned int head, unsigned int tail)
+{
+	return (head - tail) % (RX_DESC_NUM - 1);
+}
+
 static void hip04_config_port(struct net_device *ndev, u32 speed, u32 duplex)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);
@@ -363,6 +371,35 @@ static int hip04_set_mac_address(struct net_device *ndev, void *addr)
 	return 0;
 }
 
+static int hip04_alloc_rx_buffers(struct net_device *ndev, int cleaned_count)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	unsigned char *buf;
+	dma_addr_t phys;
+	int i = priv->rx_tail;
+
+	while (cleaned_count) {
+		buf = netdev_alloc_frag(priv->rx_buf_size);
+		if (!buf)
+			break;
+
+		phys = dma_map_single(&ndev->dev, buf,
+				      RX_BUF_SIZE, DMA_FROM_DEVICE);
+		if (dma_mapping_error(&ndev->dev, phys))
+			break;
+
+		priv->rx_buf[i] = buf;
+		priv->rx_phys[i] = phys;
+		hip04_set_recv_desc(priv, phys);
+		i = RX_NEXT(i);
+		cleaned_count--;
+	}
+
+	priv->rx_tail = i;
+
+	return 0;
+}
+
 static int hip04_tx_reclaim(struct net_device *ndev, bool force)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);
@@ -482,8 +519,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 	struct sk_buff *skb;
 	unsigned char *buf;
 	bool last = false;
-	dma_addr_t phys;
-	int rx = 0;
+	int rx = 0, cleaned_count = 0;
 	int tx_remaining;
 	u16 len;
 	u32 err;
@@ -491,8 +527,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 	while (cnt && !last) {
 		buf = priv->rx_buf[priv->rx_head];
 		skb = build_skb(buf, priv->rx_buf_size);
-		if (unlikely(!skb))
+		if (unlikely(!skb)) {
 			net_dbg_ratelimited("build_skb failed\n");
+			goto done;
+		}
 
 		dma_unmap_single(&ndev->dev, priv->rx_phys[priv->rx_head],
 				 RX_BUF_SIZE, DMA_FROM_DEVICE);
@@ -519,18 +557,15 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 			rx++;
 		}
 
-		buf = netdev_alloc_frag(priv->rx_buf_size);
-		if (!buf)
-			goto done;
-		phys = dma_map_single(&ndev->dev, buf,
-				      RX_BUF_SIZE, DMA_FROM_DEVICE);
-		if (dma_mapping_error(&ndev->dev, phys))
-			goto done;
-		priv->rx_buf[priv->rx_head] = buf;
-		priv->rx_phys[priv->rx_head] = phys;
-		hip04_set_recv_desc(priv, phys);
-
 		priv->rx_head = RX_NEXT(priv->rx_head);
+
+		cleaned_count = rx_count(priv->rx_head, priv->rx_tail);
+		/* return some buffers to hardware , one at a time is too slow */
+		if (++cleaned_count >= HIP04_RX_BUFFER_WRITE) {
+			hip04_alloc_rx_buffers(ndev, cleaned_count);
+			cleaned_count = 0;
+		}
+
 		if (rx >= budget)
 			goto done;
 
@@ -545,6 +580,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 	}
 	napi_complete(napi);
 done:
+	cleaned_count = rx_count(priv->rx_head, priv->rx_tail);
+	if (cleaned_count)
+		hip04_alloc_rx_buffers(ndev, cleaned_count);
+
 	/* clean up tx descriptors and start a new timer if necessary */
 	tx_remaining = hip04_tx_reclaim(ndev, false);
 	if (rx < budget && tx_remaining)
@@ -621,6 +660,7 @@ static int hip04_mac_open(struct net_device *ndev)
 	int i;
 
 	priv->rx_head = 0;
+	priv->rx_tail = 0;
 	priv->tx_head = 0;
 	priv->tx_tail = 0;
 	hip04_reset_ppe(priv);
-- 
1.8.0

[PATCH net-next 3/6] net: hip04: Solve the problem of the skb memory allocation failure

[Ding Tianhong]

The driver will alloc some skb buffer for hareware queue, but without
considering the case of memory allocation failure, when memory is low,
the skb may be null and panic the system, so break the loop when skb is
null and try to alloc the memory again to fix this problem.

Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 68 ++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 6473462..7533858 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -131,6 +131,8 @@
 #define HIP04_MAX_TX_COALESCE_FRAMES	(TX_DESC_NUM - 1)
 #define HIP04_MIN_TX_COALESCE_FRAMES	1
 
+#define HIP04_RX_BUFFER_WRITE		16
+
 struct tx_desc {
 	__be32 send_addr;
 	__be32 send_size;
@@ -180,6 +182,7 @@ struct hip04_priv {
 
 	/* written only by tx cleanup */
 	unsigned int tx_tail ____cacheline_aligned_in_smp;
+	unsigned int rx_tail ____cacheline_aligned_in_smp;
 };
 
 static inline unsigned int tx_count(unsigned int head, unsigned int tail)
@@ -187,6 +190,11 @@ static inline unsigned int tx_count(unsigned int head, unsigned int tail)
 	return (head - tail) % (TX_DESC_NUM - 1);
 }
 
+static inline unsigned int rx_count(unsigned int head, unsigned int tail)
+{
+	return (head - tail) % (RX_DESC_NUM - 1);
+}
+
 static void hip04_config_port(struct net_device *ndev, u32 speed, u32 duplex)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);
@@ -363,6 +371,35 @@ static int hip04_set_mac_address(struct net_device *ndev, void *addr)
 	return 0;
 }
 
+static int hip04_alloc_rx_buffers(struct net_device *ndev, int cleaned_count)
+{
+	struct hip04_priv *priv = netdev_priv(ndev);
+	unsigned char *buf;
+	dma_addr_t phys;
+	int i = priv->rx_tail;
+
+	while (cleaned_count) {
+		buf = netdev_alloc_frag(priv->rx_buf_size);
+		if (!buf)
+			break;
+
+		phys = dma_map_single(&ndev->dev, buf,
+				      RX_BUF_SIZE, DMA_FROM_DEVICE);
+		if (dma_mapping_error(&ndev->dev, phys))
+			break;
+
+		priv->rx_buf[i] = buf;
+		priv->rx_phys[i] = phys;
+		hip04_set_recv_desc(priv, phys);
+		i = RX_NEXT(i);
+		cleaned_count--;
+	}
+
+	priv->rx_tail = i;
+
+	return 0;
+}
+
 static int hip04_tx_reclaim(struct net_device *ndev, bool force)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);
@@ -482,8 +519,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 	struct sk_buff *skb;
 	unsigned char *buf;
 	bool last = false;
-	dma_addr_t phys;
-	int rx = 0;
+	int rx = 0, cleaned_count = 0;
 	int tx_remaining;
 	u16 len;
 	u32 err;
@@ -491,8 +527,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 	while (cnt && !last) {
 		buf = priv->rx_buf[priv->rx_head];
 		skb = build_skb(buf, priv->rx_buf_size);
-		if (unlikely(!skb))
+		if (unlikely(!skb)) {
 			net_dbg_ratelimited("build_skb failed\n");
+			goto done;
+		}
 
 		dma_unmap_single(&ndev->dev, priv->rx_phys[priv->rx_head],
 				 RX_BUF_SIZE, DMA_FROM_DEVICE);
@@ -519,18 +557,15 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 			rx++;
 		}
 
-		buf = netdev_alloc_frag(priv->rx_buf_size);
-		if (!buf)
-			goto done;
-		phys = dma_map_single(&ndev->dev, buf,
-				      RX_BUF_SIZE, DMA_FROM_DEVICE);
-		if (dma_mapping_error(&ndev->dev, phys))
-			goto done;
-		priv->rx_buf[priv->rx_head] = buf;
-		priv->rx_phys[priv->rx_head] = phys;
-		hip04_set_recv_desc(priv, phys);
-
 		priv->rx_head = RX_NEXT(priv->rx_head);
+
+		cleaned_count = rx_count(priv->rx_head, priv->rx_tail);
+		/* return some buffers to hardware , one at a time is too slow */
+		if (++cleaned_count >= HIP04_RX_BUFFER_WRITE) {
+			hip04_alloc_rx_buffers(ndev, cleaned_count);
+			cleaned_count = 0;
+		}
+
 		if (rx >= budget)
 			goto done;
 
@@ -545,6 +580,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
 	}
 	napi_complete(napi);
 done:
+	cleaned_count = rx_count(priv->rx_head, priv->rx_tail);
+	if (cleaned_count)
+		hip04_alloc_rx_buffers(ndev, cleaned_count);
+
 	/* clean up tx descriptors and start a new timer if necessary */
 	tx_remaining = hip04_tx_reclaim(ndev, false);
 	if (rx < budget && tx_remaining)
@@ -621,6 +660,7 @@ static int hip04_mac_open(struct net_device *ndev)
 	int i;
 
 	priv->rx_head = 0;
+	priv->rx_tail = 0;
 	priv->tx_head = 0;
 	priv->tx_tail = 0;
 	hip04_reset_ppe(priv);
-- 
1.8.0


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

Re: [PATCH net-next 3/6] net: hip04: Solve the problem of the skb memory allocation failure

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:05 Ding Tianhong wrote:
> The driver will alloc some skb buffer for hareware queue, but without
> considering the case of memory allocation failure, when memory is low,
> the skb may be null and panic the system, so break the loop when skb is
> null and try to alloc the memory again to fix this problem.
> 
> Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
> 

Looks reasonable to me, though I haven't done a deep analysis
to see if there are remaining problems in this area.

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

[PATCH net-next 3/6] net: hip04: Solve the problem of the skb memory allocation failure

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:05 Ding Tianhong wrote:
> The driver will alloc some skb buffer for hareware queue, but without
> considering the case of memory allocation failure, when memory is low,
> the skb may be null and panic the system, so break the loop when skb is
> null and try to alloc the memory again to fix this problem.
> 
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Joe Perches <joe@perches.com>
> 

Looks reasonable to me, though I haven't done a deep analysis
to see if there are remaining problems in this area.

	Arnd

[PATCH net-next 5/6] net: hip04: remove the unnecessary check

[Ding Tianhong]

Testing bytes_compl should be enough, because there is no way
that pkt_compl could be 0 if bytes_compl is not 0.

Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index ff9d19c..a7ab1d9 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -440,7 +440,7 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
 	smp_wmb(); /* Ensure tx_tail visible to xmit */
 
 out:
-	if (pkts_compl || bytes_compl)
+	if (bytes_compl)
 		netdev_completed_queue(ndev, pkts_compl, bytes_compl);
 
 	if (unlikely(netif_queue_stopped(ndev)) && (count < (TX_DESC_NUM - 1)))
-- 
1.8.0


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

[PATCH net-next 5/6] net: hip04: remove the unnecessary check

[Ding Tianhong]

Testing bytes_compl should be enough, because there is no way
that pkt_compl could be 0 if bytes_compl is not 0.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index ff9d19c..a7ab1d9 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -440,7 +440,7 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
 	smp_wmb(); /* Ensure tx_tail visible to xmit */
 
 out:
-	if (pkts_compl || bytes_compl)
+	if (bytes_compl)
 		netdev_completed_queue(ndev, pkts_compl, bytes_compl);
 
 	if (unlikely(netif_queue_stopped(ndev)) && (count < (TX_DESC_NUM - 1)))
-- 
1.8.0

[PATCH net-next 5/6] net: hip04: remove the unnecessary check

[Ding Tianhong]

Testing bytes_compl should be enough, because there is no way
that pkt_compl could be 0 if bytes_compl is not 0.

Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index ff9d19c..a7ab1d9 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -440,7 +440,7 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
 	smp_wmb(); /* Ensure tx_tail visible to xmit */
 
 out:
-	if (pkts_compl || bytes_compl)
+	if (bytes_compl)
 		netdev_completed_queue(ndev, pkts_compl, bytes_compl);
 
 	if (unlikely(netif_queue_stopped(ndev)) && (count < (TX_DESC_NUM - 1)))
-- 
1.8.0


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

Re: [PATCH net-next 5/6] net: hip04: remove the unnecessary check

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:07 Ding Tianhong wrote:
> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index ff9d19c..a7ab1d9 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -440,7 +440,7 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
>         smp_wmb(); /* Ensure tx_tail visible to xmit */
>  
>  out:
> -       if (pkts_compl || bytes_compl)
> +       if (bytes_compl)
>                 netdev_completed_queue(ndev, pkts_compl, bytes_compl);
>  
>         if (unlikely(netif_queue_stopped(ndev)) && (count < (TX_DESC_NUM - 1)))
> -- 
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH net-next 5/6] net: hip04: remove the unnecessary check

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:07 Ding Tianhong wrote:
> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index ff9d19c..a7ab1d9 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -440,7 +440,7 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
>         smp_wmb(); /* Ensure tx_tail visible to xmit */
>  
>  out:
> -       if (pkts_compl || bytes_compl)
> +       if (bytes_compl)
>                 netdev_completed_queue(ndev, pkts_compl, bytes_compl);
>  
>         if (unlikely(netif_queue_stopped(ndev)) && (count < (TX_DESC_NUM - 1)))
> -- 
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH net-next 6/6] net: hip04: add ratelimit for rx/tx drops skb

[Ding Tianhong]

There can be quite a lot of rx/tx drops message and affect
useful message, so need to ratelimit them to not overwhelm
logging.

Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index a7ab1d9..b19830d 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -608,11 +608,15 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
 		if (ists & (RCV_NOBUF | RCV_DROP)) {
 			stats->rx_errors++;
 			stats->rx_dropped++;
-			netdev_err(ndev, "rx drop\n");
+			if (net_ratelimit())
+				netdev_dbg(ndev, "rx drop: %lu\n",
+					   stats->rx_dropped);
 		}
 		if (ists & TX_DROP) {
 			stats->tx_dropped++;
-			netdev_err(ndev, "tx drop\n");
+			if (net_ratelimit())
+				netdev_dbg(ndev, "tx drop: %lu\n",
+					   stats->rx_dropped);
 		}
 	}
 
-- 
1.8.0


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

[PATCH net-next 6/6] net: hip04: add ratelimit for rx/tx drops skb

[Ding Tianhong]

There can be quite a lot of rx/tx drops message and affect
useful message, so need to ratelimit them to not overwhelm
logging.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index a7ab1d9..b19830d 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -608,11 +608,15 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
 		if (ists & (RCV_NOBUF | RCV_DROP)) {
 			stats->rx_errors++;
 			stats->rx_dropped++;
-			netdev_err(ndev, "rx drop\n");
+			if (net_ratelimit())
+				netdev_dbg(ndev, "rx drop: %lu\n",
+					   stats->rx_dropped);
 		}
 		if (ists & TX_DROP) {
 			stats->tx_dropped++;
-			netdev_err(ndev, "tx drop\n");
+			if (net_ratelimit())
+				netdev_dbg(ndev, "tx drop: %lu\n",
+					   stats->rx_dropped);
 		}
 	}
 
-- 
1.8.0

[PATCH net-next 6/6] net: hip04: add ratelimit for rx/tx drops skb

[Ding Tianhong]

There can be quite a lot of rx/tx drops message and affect
useful message, so need to ratelimit them to not overwhelm
logging.

Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index a7ab1d9..b19830d 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -608,11 +608,15 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
 		if (ists & (RCV_NOBUF | RCV_DROP)) {
 			stats->rx_errors++;
 			stats->rx_dropped++;
-			netdev_err(ndev, "rx drop\n");
+			if (net_ratelimit())
+				netdev_dbg(ndev, "rx drop: %lu\n",
+					   stats->rx_dropped);
 		}
 		if (ists & TX_DROP) {
 			stats->tx_dropped++;
-			netdev_err(ndev, "tx drop\n");
+			if (net_ratelimit())
+				netdev_dbg(ndev, "tx drop: %lu\n",
+					   stats->rx_dropped);
 		}
 	}
 
-- 
1.8.0


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

Re: [PATCH net-next 6/6] net: hip04: add ratelimit for rx/tx drops skb

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:08 Ding Tianhong wrote:
> There can be quite a lot of rx/tx drops message and affect
> useful message, so need to ratelimit them to not overwhelm
> logging.
> 
> Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Cc: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
> 

Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 6/6] net: hip04: add ratelimit for rx/tx drops skb

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:08 Ding Tianhong wrote:
> There can be quite a lot of rx/tx drops message and affect
> useful message, so need to ratelimit them to not overwhelm
> logging.
> 
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Joe Perches <joe@perches.com>
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:02 Ding Tianhong wrote:
> The patches series was used to fix the issues of the hip04 driver, and added
> some optimizations according to some good suggestion. 
> 
> 

Thanks, that looks much better, except for patch 4 that I commented on.

I had at some point sent a patch that is archived at
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318120.html

I believe that one is also still needed, but I have not tested whether it
is correct. Can you have a look at the patch from back then and see if it
works, of if you find something wrong about it?

I'm sending the unmodified patch from then here again for you to apply
or comment. It will have to be rebased on top of your current changes.

	Arnd

8<----
Subject: [PATCH] net/hip04: refactor interrupt masking

The hip04 ethernet driver currently acknowledges all interrupts directly
in the interrupt handler, and leaves all interrupts except the RX data
enabled the whole time. This causes multiple problems:

- When more packets come in between the original interrupt and the
  NAPI poll function, we will get an extraneous RX interrupt as soon
  as interrupts are enabled again.

- The two error interrupts are by definition combining all errors that
  may have happened since the last time they were handled, but just
  acknowledging the irq without dealing with the cause of the condition
  makes it come back immediately. In particular, when NAPI is intentionally
  stalling the rx queue, this causes a storm of "rx dropped" messages.  

- The access to the 'reg_inten' field in hip_priv is used for serializing
  access, but is in fact racy itself.

To deal with these issues, the driver is changed to only acknowledge
the IRQ at the point when it is being handled. The RX interrupts now get
acked right before reading the number of received frames to keep spurious
interrupts to the absolute minimum without losing interrupts.

As there is no tx-complete interrupt, only an informational tx-dropped
one, we now ack that in the tx reclaim handler, hoping that clearing
the descriptors has also eliminated the error condition.

The only place that reads the reg_inten now just relies on
napi_schedule_prep() to return whether NAPI is active or not, and
the poll() function is then going to ack and reenabled the IRQ if
necessary.

Finally, when we disable interrupts, they are now all disabled
together, in order to simplify the logic and to avoid getting
rx-dropped interrupts when NAPI is in control of the rx queue.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 525214ef5984..83773247a368 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -56,6 +56,8 @@
 #define RCV_DROP                       BIT(7)
 #define TX_DROP                                BIT(6)
 #define DEF_INT_ERR                    (RCV_NOBUF | RCV_DROP | TX_DROP)
+#define DEF_INT_RX                     (RCV_INT | RCV_NOBUF | RCV_DROP)
+#define DEF_INT_TX                     (TX_DROP)
 #define DEF_INT_MASK                   (RCV_INT | DEF_INT_ERR)
 
 /* TX descriptor config */
@@ -154,7 +156,6 @@ struct hip04_priv {
        unsigned int port;
        unsigned int speed;
        unsigned int duplex;
-       unsigned int reg_inten;
 
        struct napi_struct napi;
        struct net_device *ndev;
@@ -303,17 +304,15 @@ static void hip04_mac_enable(struct net_device *ndev)
        val |= GE_RX_PORT_EN | GE_TX_PORT_EN;
        writel_relaxed(val, priv->base + GE_PORT_EN);
 
-       /* clear rx int */
-       val = RCV_INT;
-       writel_relaxed(val, priv->base + PPE_RINT);
+       /* clear prior interrupts */
+       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
 
        /* config recv int */
        val = GE_RX_INT_THRESHOLD | GE_RX_TIMEOUT;
        writel_relaxed(val, priv->base + PPE_CFG_RX_PKT_INT);
 
        /* enable interrupt */
-       priv->reg_inten = DEF_INT_MASK;
-       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
+       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
 }
 
 static void hip04_mac_disable(struct net_device *ndev)
@@ -322,8 +321,7 @@ static void hip04_mac_disable(struct net_device *ndev)
        u32 val;
 
        /* disable int */
-       priv->reg_inten &= ~(DEF_INT_MASK);
-       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
+       writel_relaxed(0, priv->base + PPE_INTEN);
 
        /* disable tx & rx */
        val = readl_relaxed(priv->base + GE_PORT_EN);
@@ -403,6 +401,8 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
        priv->tx_tail = tx_tail;
        smp_wmb(); /* Ensure tx_tail visible to xmit */
 
+       writel_relaxed(DEF_INT_TX, priv->base + PPE_RINT);
+
 out:
        if (pkts_compl || bytes_compl)
                netdev_completed_queue(ndev, pkts_compl, bytes_compl);
@@ -458,9 +458,7 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
        if (count >= priv->tx_coalesce_frames) {
                if (napi_schedule_prep(&priv->napi)) {
                        /* disable rx interrupt and timer */
-                       priv->reg_inten &= ~(RCV_INT);
-                       writel_relaxed(DEF_INT_MASK & ~RCV_INT,
-                                      priv->base + PPE_INTEN);
+                       writel_relaxed(0, priv->base + PPE_INTEN);
                        hrtimer_cancel(&priv->tx_coalesce_timer);
                        __napi_schedule(&priv->napi);
                }
@@ -478,7 +476,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
        struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
        struct net_device *ndev = priv->ndev;
        struct net_device_stats *stats = &ndev->stats;
-       unsigned int cnt = hip04_recv_cnt(priv);
+       unsigned int cnt;
        struct rx_desc *desc;
        struct sk_buff *skb;
        unsigned char *buf;
@@ -489,6 +487,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
        u16 len;
        u32 err;
 
+       /* acknowledge interrupts and read status */
+       writel_relaxed(DEF_INT_RX, priv->base + PPE_RINT);
+       cnt = hip04_recv_cnt(priv);
+
        while (cnt && !last) {
                buf = priv->rx_buf[priv->rx_head];
                skb = build_skb(buf, priv->rx_buf_size);
@@ -539,11 +541,8 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
                        cnt = hip04_recv_cnt(priv);
        }
 
-       if (!(priv->reg_inten & RCV_INT)) {
-               /* enable rx interrupt */
-               priv->reg_inten |= RCV_INT;
-               writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
-       }
+       /* enable rx interrupt */
+       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
        napi_complete(napi);
 done:
        /* clean up tx descriptors and start a new timer if necessary */
@@ -564,23 +563,21 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
        if (!ists)
                return IRQ_NONE;
 
-       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
-
        if (unlikely(ists & DEF_INT_ERR)) {
-               if (ists & (RCV_NOBUF | RCV_DROP))
+               if (ists & (RCV_NOBUF | RCV_DROP)) {
                        stats->rx_errors++;
                        stats->rx_dropped++;
-                       netdev_err(ndev, "rx drop\n");
+                       netdev_dbg(ndev, "rx drop\n");
+               }
                if (ists & TX_DROP) {
                        stats->tx_dropped++;
-                       netdev_err(ndev, "tx drop\n");
+                       netdev_dbg(ndev, "tx drop\n");
                }
        }
 
-       if (ists & RCV_INT && napi_schedule_prep(&priv->napi)) {
-               /* disable rx interrupt */
-               priv->reg_inten &= ~(RCV_INT);
-               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
+       if (napi_schedule_prep(&priv->napi)) {
+               /* disable interrupt */
+               writel_relaxed(0, priv->base + PPE_INTEN);
                hrtimer_cancel(&priv->tx_coalesce_timer);
                __napi_schedule(&priv->napi);
        }
@@ -596,8 +593,7 @@ enum hrtimer_restart tx_done(struct hrtimer *hrtimer)
 
        if (napi_schedule_prep(&priv->napi)) {
                /* disable rx interrupt */
-               priv->reg_inten &= ~(RCV_INT);
-               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
+               writel_relaxed(0, priv->base + PPE_INTEN);
                __napi_schedule(&priv->napi);
        }
 

[PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[Arnd Bergmann]

On Wednesday 15 April 2015 20:30:02 Ding Tianhong wrote:
> The patches series was used to fix the issues of the hip04 driver, and added
> some optimizations according to some good suggestion. 
> 
> 

Thanks, that looks much better, except for patch 4 that I commented on.

I had at some point sent a patch that is archived at
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318120.html

I believe that one is also still needed, but I have not tested whether it
is correct. Can you have a look at the patch from back then and see if it
works, of if you find something wrong about it?

I'm sending the unmodified patch from then here again for you to apply
or comment. It will have to be rebased on top of your current changes.

	Arnd

8<----
Subject: [PATCH] net/hip04: refactor interrupt masking

The hip04 ethernet driver currently acknowledges all interrupts directly
in the interrupt handler, and leaves all interrupts except the RX data
enabled the whole time. This causes multiple problems:

- When more packets come in between the original interrupt and the
  NAPI poll function, we will get an extraneous RX interrupt as soon
  as interrupts are enabled again.

- The two error interrupts are by definition combining all errors that
  may have happened since the last time they were handled, but just
  acknowledging the irq without dealing with the cause of the condition
  makes it come back immediately. In particular, when NAPI is intentionally
  stalling the rx queue, this causes a storm of "rx dropped" messages.  

- The access to the 'reg_inten' field in hip_priv is used for serializing
  access, but is in fact racy itself.

To deal with these issues, the driver is changed to only acknowledge
the IRQ at the point when it is being handled. The RX interrupts now get
acked right before reading the number of received frames to keep spurious
interrupts to the absolute minimum without losing interrupts.

As there is no tx-complete interrupt, only an informational tx-dropped
one, we now ack that in the tx reclaim handler, hoping that clearing
the descriptors has also eliminated the error condition.

The only place that reads the reg_inten now just relies on
napi_schedule_prep() to return whether NAPI is active or not, and
the poll() function is then going to ack and reenabled the IRQ if
necessary.

Finally, when we disable interrupts, they are now all disabled
together, in order to simplify the logic and to avoid getting
rx-dropped interrupts when NAPI is in control of the rx queue.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index 525214ef5984..83773247a368 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -56,6 +56,8 @@
 #define RCV_DROP                       BIT(7)
 #define TX_DROP                                BIT(6)
 #define DEF_INT_ERR                    (RCV_NOBUF | RCV_DROP | TX_DROP)
+#define DEF_INT_RX                     (RCV_INT | RCV_NOBUF | RCV_DROP)
+#define DEF_INT_TX                     (TX_DROP)
 #define DEF_INT_MASK                   (RCV_INT | DEF_INT_ERR)
 
 /* TX descriptor config */
@@ -154,7 +156,6 @@ struct hip04_priv {
        unsigned int port;
        unsigned int speed;
        unsigned int duplex;
-       unsigned int reg_inten;
 
        struct napi_struct napi;
        struct net_device *ndev;
@@ -303,17 +304,15 @@ static void hip04_mac_enable(struct net_device *ndev)
        val |= GE_RX_PORT_EN | GE_TX_PORT_EN;
        writel_relaxed(val, priv->base + GE_PORT_EN);
 
-       /* clear rx int */
-       val = RCV_INT;
-       writel_relaxed(val, priv->base + PPE_RINT);
+       /* clear prior interrupts */
+       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
 
        /* config recv int */
        val = GE_RX_INT_THRESHOLD | GE_RX_TIMEOUT;
        writel_relaxed(val, priv->base + PPE_CFG_RX_PKT_INT);
 
        /* enable interrupt */
-       priv->reg_inten = DEF_INT_MASK;
-       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
+       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
 }
 
 static void hip04_mac_disable(struct net_device *ndev)
@@ -322,8 +321,7 @@ static void hip04_mac_disable(struct net_device *ndev)
        u32 val;
 
        /* disable int */
-       priv->reg_inten &= ~(DEF_INT_MASK);
-       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
+       writel_relaxed(0, priv->base + PPE_INTEN);
 
        /* disable tx & rx */
        val = readl_relaxed(priv->base + GE_PORT_EN);
@@ -403,6 +401,8 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
        priv->tx_tail = tx_tail;
        smp_wmb(); /* Ensure tx_tail visible to xmit */
 
+       writel_relaxed(DEF_INT_TX, priv->base + PPE_RINT);
+
 out:
        if (pkts_compl || bytes_compl)
                netdev_completed_queue(ndev, pkts_compl, bytes_compl);
@@ -458,9 +458,7 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
        if (count >= priv->tx_coalesce_frames) {
                if (napi_schedule_prep(&priv->napi)) {
                        /* disable rx interrupt and timer */
-                       priv->reg_inten &= ~(RCV_INT);
-                       writel_relaxed(DEF_INT_MASK & ~RCV_INT,
-                                      priv->base + PPE_INTEN);
+                       writel_relaxed(0, priv->base + PPE_INTEN);
                        hrtimer_cancel(&priv->tx_coalesce_timer);
                        __napi_schedule(&priv->napi);
                }
@@ -478,7 +476,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
        struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
        struct net_device *ndev = priv->ndev;
        struct net_device_stats *stats = &ndev->stats;
-       unsigned int cnt = hip04_recv_cnt(priv);
+       unsigned int cnt;
        struct rx_desc *desc;
        struct sk_buff *skb;
        unsigned char *buf;
@@ -489,6 +487,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
        u16 len;
        u32 err;
 
+       /* acknowledge interrupts and read status */
+       writel_relaxed(DEF_INT_RX, priv->base + PPE_RINT);
+       cnt = hip04_recv_cnt(priv);
+
        while (cnt && !last) {
                buf = priv->rx_buf[priv->rx_head];
                skb = build_skb(buf, priv->rx_buf_size);
@@ -539,11 +541,8 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
                        cnt = hip04_recv_cnt(priv);
        }
 
-       if (!(priv->reg_inten & RCV_INT)) {
-               /* enable rx interrupt */
-               priv->reg_inten |= RCV_INT;
-               writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
-       }
+       /* enable rx interrupt */
+       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
        napi_complete(napi);
 done:
        /* clean up tx descriptors and start a new timer if necessary */
@@ -564,23 +563,21 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
        if (!ists)
                return IRQ_NONE;
 
-       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
-
        if (unlikely(ists & DEF_INT_ERR)) {
-               if (ists & (RCV_NOBUF | RCV_DROP))
+               if (ists & (RCV_NOBUF | RCV_DROP)) {
                        stats->rx_errors++;
                        stats->rx_dropped++;
-                       netdev_err(ndev, "rx drop\n");
+                       netdev_dbg(ndev, "rx drop\n");
+               }
                if (ists & TX_DROP) {
                        stats->tx_dropped++;
-                       netdev_err(ndev, "tx drop\n");
+                       netdev_dbg(ndev, "tx drop\n");
                }
        }
 
-       if (ists & RCV_INT && napi_schedule_prep(&priv->napi)) {
-               /* disable rx interrupt */
-               priv->reg_inten &= ~(RCV_INT);
-               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
+       if (napi_schedule_prep(&priv->napi)) {
+               /* disable interrupt */
+               writel_relaxed(0, priv->base + PPE_INTEN);
                hrtimer_cancel(&priv->tx_coalesce_timer);
                __napi_schedule(&priv->napi);
        }
@@ -596,8 +593,7 @@ enum hrtimer_restart tx_done(struct hrtimer *hrtimer)
 
        if (napi_schedule_prep(&priv->napi)) {
                /* disable rx interrupt */
-               priv->reg_inten &= ~(RCV_INT);
-               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
+               writel_relaxed(0, priv->base + PPE_INTEN);
                __napi_schedule(&priv->napi);
        }
 

Re: [PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[Ding Tianhong]

On 2015/4/15 22:25, Arnd Bergmann wrote:
> On Wednesday 15 April 2015 20:30:02 Ding Tianhong wrote:
>> The patches series was used to fix the issues of the hip04 driver, and added
>> some optimizations according to some good suggestion. 
>>
>>
> 
> Thanks, that looks much better, except for patch 4 that I commented on.
> 
I will check and fix it.

> I had at some point sent a patch that is archived at
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318120.html
> 
> I believe that one is also still needed, but I have not tested whether it
> is correct. Can you have a look at the patch from back then and see if it
> works, of if you find something wrong about it?
> 
> I'm sending the unmodified patch from then here again for you to apply
> or comment. It will have to be rebased on top of your current changes.
> 
> 	Arnd
> 

Thanks for the suggestion, I notices that I miss this patch in my series from the maillist, 
I need time to check the code and try to test it, thanks for the work.:)

Ding

> 8<----
> Subject: [PATCH] net/hip04: refactor interrupt masking
> 
> The hip04 ethernet driver currently acknowledges all interrupts directly
> in the interrupt handler, and leaves all interrupts except the RX data
> enabled the whole time. This causes multiple problems:
> 
> - When more packets come in between the original interrupt and the
>   NAPI poll function, we will get an extraneous RX interrupt as soon
>   as interrupts are enabled again.
> 
> - The two error interrupts are by definition combining all errors that
>   may have happened since the last time they were handled, but just
>   acknowledging the irq without dealing with the cause of the condition
>   makes it come back immediately. In particular, when NAPI is intentionally
>   stalling the rx queue, this causes a storm of "rx dropped" messages.  
> 
> - The access to the 'reg_inten' field in hip_priv is used for serializing
>   access, but is in fact racy itself.
> 
> To deal with these issues, the driver is changed to only acknowledge
> the IRQ at the point when it is being handled. The RX interrupts now get
> acked right before reading the number of received frames to keep spurious
> interrupts to the absolute minimum without losing interrupts.
> 
> As there is no tx-complete interrupt, only an informational tx-dropped
> one, we now ack that in the tx reclaim handler, hoping that clearing
> the descriptors has also eliminated the error condition.
> 
> The only place that reads the reg_inten now just relies on
> napi_schedule_prep() to return whether NAPI is active or not, and
> the poll() function is then going to ack and reenabled the IRQ if
> necessary.
> 
> Finally, when we disable interrupts, they are now all disabled
> together, in order to simplify the logic and to avoid getting
> rx-dropped interrupts when NAPI is in control of the rx queue.
> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index 525214ef5984..83773247a368 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -56,6 +56,8 @@
>  #define RCV_DROP                       BIT(7)
>  #define TX_DROP                                BIT(6)
>  #define DEF_INT_ERR                    (RCV_NOBUF | RCV_DROP | TX_DROP)
> +#define DEF_INT_RX                     (RCV_INT | RCV_NOBUF | RCV_DROP)
> +#define DEF_INT_TX                     (TX_DROP)
>  #define DEF_INT_MASK                   (RCV_INT | DEF_INT_ERR)
>  
>  /* TX descriptor config */
> @@ -154,7 +156,6 @@ struct hip04_priv {
>         unsigned int port;
>         unsigned int speed;
>         unsigned int duplex;
> -       unsigned int reg_inten;
>  
>         struct napi_struct napi;
>         struct net_device *ndev;
> @@ -303,17 +304,15 @@ static void hip04_mac_enable(struct net_device *ndev)
>         val |= GE_RX_PORT_EN | GE_TX_PORT_EN;
>         writel_relaxed(val, priv->base + GE_PORT_EN);
>  
> -       /* clear rx int */
> -       val = RCV_INT;
> -       writel_relaxed(val, priv->base + PPE_RINT);
> +       /* clear prior interrupts */
> +       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
>  
>         /* config recv int */
>         val = GE_RX_INT_THRESHOLD | GE_RX_TIMEOUT;
>         writel_relaxed(val, priv->base + PPE_CFG_RX_PKT_INT);
>  
>         /* enable interrupt */
> -       priv->reg_inten = DEF_INT_MASK;
> -       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> +       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
>  }
>  
>  static void hip04_mac_disable(struct net_device *ndev)
> @@ -322,8 +321,7 @@ static void hip04_mac_disable(struct net_device *ndev)
>         u32 val;
>  
>         /* disable int */
> -       priv->reg_inten &= ~(DEF_INT_MASK);
> -       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> +       writel_relaxed(0, priv->base + PPE_INTEN);
>  
>         /* disable tx & rx */
>         val = readl_relaxed(priv->base + GE_PORT_EN);
> @@ -403,6 +401,8 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
>         priv->tx_tail = tx_tail;
>         smp_wmb(); /* Ensure tx_tail visible to xmit */
>  
> +       writel_relaxed(DEF_INT_TX, priv->base + PPE_RINT);
> +
>  out:
>         if (pkts_compl || bytes_compl)
>                 netdev_completed_queue(ndev, pkts_compl, bytes_compl);
> @@ -458,9 +458,7 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>         if (count >= priv->tx_coalesce_frames) {
>                 if (napi_schedule_prep(&priv->napi)) {
>                         /* disable rx interrupt and timer */
> -                       priv->reg_inten &= ~(RCV_INT);
> -                       writel_relaxed(DEF_INT_MASK & ~RCV_INT,
> -                                      priv->base + PPE_INTEN);
> +                       writel_relaxed(0, priv->base + PPE_INTEN);
>                         hrtimer_cancel(&priv->tx_coalesce_timer);
>                         __napi_schedule(&priv->napi);
>                 }
> @@ -478,7 +476,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>         struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
>         struct net_device *ndev = priv->ndev;
>         struct net_device_stats *stats = &ndev->stats;
> -       unsigned int cnt = hip04_recv_cnt(priv);
> +       unsigned int cnt;
>         struct rx_desc *desc;
>         struct sk_buff *skb;
>         unsigned char *buf;
> @@ -489,6 +487,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>         u16 len;
>         u32 err;
>  
> +       /* acknowledge interrupts and read status */
> +       writel_relaxed(DEF_INT_RX, priv->base + PPE_RINT);
> +       cnt = hip04_recv_cnt(priv);
> +
>         while (cnt && !last) {
>                 buf = priv->rx_buf[priv->rx_head];
>                 skb = build_skb(buf, priv->rx_buf_size);
> @@ -539,11 +541,8 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>                         cnt = hip04_recv_cnt(priv);
>         }
>  
> -       if (!(priv->reg_inten & RCV_INT)) {
> -               /* enable rx interrupt */
> -               priv->reg_inten |= RCV_INT;
> -               writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> -       }
> +       /* enable rx interrupt */
> +       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
>         napi_complete(napi);
>  done:
>         /* clean up tx descriptors and start a new timer if necessary */
> @@ -564,23 +563,21 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
>         if (!ists)
>                 return IRQ_NONE;
>  
> -       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
> -
>         if (unlikely(ists & DEF_INT_ERR)) {
> -               if (ists & (RCV_NOBUF | RCV_DROP))
> +               if (ists & (RCV_NOBUF | RCV_DROP)) {
>                         stats->rx_errors++;
>                         stats->rx_dropped++;
> -                       netdev_err(ndev, "rx drop\n");
> +                       netdev_dbg(ndev, "rx drop\n");
> +               }
>                 if (ists & TX_DROP) {
>                         stats->tx_dropped++;
> -                       netdev_err(ndev, "tx drop\n");
> +                       netdev_dbg(ndev, "tx drop\n");
>                 }
>         }
>  
> -       if (ists & RCV_INT && napi_schedule_prep(&priv->napi)) {
> -               /* disable rx interrupt */
> -               priv->reg_inten &= ~(RCV_INT);
> -               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
> +       if (napi_schedule_prep(&priv->napi)) {
> +               /* disable interrupt */
> +               writel_relaxed(0, priv->base + PPE_INTEN);
>                 hrtimer_cancel(&priv->tx_coalesce_timer);
>                 __napi_schedule(&priv->napi);
>         }
> @@ -596,8 +593,7 @@ enum hrtimer_restart tx_done(struct hrtimer *hrtimer)
>  
>         if (napi_schedule_prep(&priv->napi)) {
>                 /* disable rx interrupt */
> -               priv->reg_inten &= ~(RCV_INT);
> -               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
> +               writel_relaxed(0, priv->base + PPE_INTEN);
>                 __napi_schedule(&priv->napi);
>         }
>  
> 
> .
> 


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

[PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[Ding Tianhong]

On 2015/4/15 22:25, Arnd Bergmann wrote:
> On Wednesday 15 April 2015 20:30:02 Ding Tianhong wrote:
>> The patches series was used to fix the issues of the hip04 driver, and added
>> some optimizations according to some good suggestion. 
>>
>>
> 
> Thanks, that looks much better, except for patch 4 that I commented on.
> 
I will check and fix it.

> I had at some point sent a patch that is archived at
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318120.html
> 
> I believe that one is also still needed, but I have not tested whether it
> is correct. Can you have a look at the patch from back then and see if it
> works, of if you find something wrong about it?
> 
> I'm sending the unmodified patch from then here again for you to apply
> or comment. It will have to be rebased on top of your current changes.
> 
> 	Arnd
> 

Thanks for the suggestion, I notices that I miss this patch in my series from the maillist, 
I need time to check the code and try to test it, thanks for the work.:)

Ding

> 8<----
> Subject: [PATCH] net/hip04: refactor interrupt masking
> 
> The hip04 ethernet driver currently acknowledges all interrupts directly
> in the interrupt handler, and leaves all interrupts except the RX data
> enabled the whole time. This causes multiple problems:
> 
> - When more packets come in between the original interrupt and the
>   NAPI poll function, we will get an extraneous RX interrupt as soon
>   as interrupts are enabled again.
> 
> - The two error interrupts are by definition combining all errors that
>   may have happened since the last time they were handled, but just
>   acknowledging the irq without dealing with the cause of the condition
>   makes it come back immediately. In particular, when NAPI is intentionally
>   stalling the rx queue, this causes a storm of "rx dropped" messages.  
> 
> - The access to the 'reg_inten' field in hip_priv is used for serializing
>   access, but is in fact racy itself.
> 
> To deal with these issues, the driver is changed to only acknowledge
> the IRQ at the point when it is being handled. The RX interrupts now get
> acked right before reading the number of received frames to keep spurious
> interrupts to the absolute minimum without losing interrupts.
> 
> As there is no tx-complete interrupt, only an informational tx-dropped
> one, we now ack that in the tx reclaim handler, hoping that clearing
> the descriptors has also eliminated the error condition.
> 
> The only place that reads the reg_inten now just relies on
> napi_schedule_prep() to return whether NAPI is active or not, and
> the poll() function is then going to ack and reenabled the IRQ if
> necessary.
> 
> Finally, when we disable interrupts, they are now all disabled
> together, in order to simplify the logic and to avoid getting
> rx-dropped interrupts when NAPI is in control of the rx queue.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index 525214ef5984..83773247a368 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -56,6 +56,8 @@
>  #define RCV_DROP                       BIT(7)
>  #define TX_DROP                                BIT(6)
>  #define DEF_INT_ERR                    (RCV_NOBUF | RCV_DROP | TX_DROP)
> +#define DEF_INT_RX                     (RCV_INT | RCV_NOBUF | RCV_DROP)
> +#define DEF_INT_TX                     (TX_DROP)
>  #define DEF_INT_MASK                   (RCV_INT | DEF_INT_ERR)
>  
>  /* TX descriptor config */
> @@ -154,7 +156,6 @@ struct hip04_priv {
>         unsigned int port;
>         unsigned int speed;
>         unsigned int duplex;
> -       unsigned int reg_inten;
>  
>         struct napi_struct napi;
>         struct net_device *ndev;
> @@ -303,17 +304,15 @@ static void hip04_mac_enable(struct net_device *ndev)
>         val |= GE_RX_PORT_EN | GE_TX_PORT_EN;
>         writel_relaxed(val, priv->base + GE_PORT_EN);
>  
> -       /* clear rx int */
> -       val = RCV_INT;
> -       writel_relaxed(val, priv->base + PPE_RINT);
> +       /* clear prior interrupts */
> +       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
>  
>         /* config recv int */
>         val = GE_RX_INT_THRESHOLD | GE_RX_TIMEOUT;
>         writel_relaxed(val, priv->base + PPE_CFG_RX_PKT_INT);
>  
>         /* enable interrupt */
> -       priv->reg_inten = DEF_INT_MASK;
> -       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> +       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
>  }
>  
>  static void hip04_mac_disable(struct net_device *ndev)
> @@ -322,8 +321,7 @@ static void hip04_mac_disable(struct net_device *ndev)
>         u32 val;
>  
>         /* disable int */
> -       priv->reg_inten &= ~(DEF_INT_MASK);
> -       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> +       writel_relaxed(0, priv->base + PPE_INTEN);
>  
>         /* disable tx & rx */
>         val = readl_relaxed(priv->base + GE_PORT_EN);
> @@ -403,6 +401,8 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
>         priv->tx_tail = tx_tail;
>         smp_wmb(); /* Ensure tx_tail visible to xmit */
>  
> +       writel_relaxed(DEF_INT_TX, priv->base + PPE_RINT);
> +
>  out:
>         if (pkts_compl || bytes_compl)
>                 netdev_completed_queue(ndev, pkts_compl, bytes_compl);
> @@ -458,9 +458,7 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>         if (count >= priv->tx_coalesce_frames) {
>                 if (napi_schedule_prep(&priv->napi)) {
>                         /* disable rx interrupt and timer */
> -                       priv->reg_inten &= ~(RCV_INT);
> -                       writel_relaxed(DEF_INT_MASK & ~RCV_INT,
> -                                      priv->base + PPE_INTEN);
> +                       writel_relaxed(0, priv->base + PPE_INTEN);
>                         hrtimer_cancel(&priv->tx_coalesce_timer);
>                         __napi_schedule(&priv->napi);
>                 }
> @@ -478,7 +476,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>         struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
>         struct net_device *ndev = priv->ndev;
>         struct net_device_stats *stats = &ndev->stats;
> -       unsigned int cnt = hip04_recv_cnt(priv);
> +       unsigned int cnt;
>         struct rx_desc *desc;
>         struct sk_buff *skb;
>         unsigned char *buf;
> @@ -489,6 +487,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>         u16 len;
>         u32 err;
>  
> +       /* acknowledge interrupts and read status */
> +       writel_relaxed(DEF_INT_RX, priv->base + PPE_RINT);
> +       cnt = hip04_recv_cnt(priv);
> +
>         while (cnt && !last) {
>                 buf = priv->rx_buf[priv->rx_head];
>                 skb = build_skb(buf, priv->rx_buf_size);
> @@ -539,11 +541,8 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>                         cnt = hip04_recv_cnt(priv);
>         }
>  
> -       if (!(priv->reg_inten & RCV_INT)) {
> -               /* enable rx interrupt */
> -               priv->reg_inten |= RCV_INT;
> -               writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> -       }
> +       /* enable rx interrupt */
> +       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
>         napi_complete(napi);
>  done:
>         /* clean up tx descriptors and start a new timer if necessary */
> @@ -564,23 +563,21 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
>         if (!ists)
>                 return IRQ_NONE;
>  
> -       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
> -
>         if (unlikely(ists & DEF_INT_ERR)) {
> -               if (ists & (RCV_NOBUF | RCV_DROP))
> +               if (ists & (RCV_NOBUF | RCV_DROP)) {
>                         stats->rx_errors++;
>                         stats->rx_dropped++;
> -                       netdev_err(ndev, "rx drop\n");
> +                       netdev_dbg(ndev, "rx drop\n");
> +               }
>                 if (ists & TX_DROP) {
>                         stats->tx_dropped++;
> -                       netdev_err(ndev, "tx drop\n");
> +                       netdev_dbg(ndev, "tx drop\n");
>                 }
>         }
>  
> -       if (ists & RCV_INT && napi_schedule_prep(&priv->napi)) {
> -               /* disable rx interrupt */
> -               priv->reg_inten &= ~(RCV_INT);
> -               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
> +       if (napi_schedule_prep(&priv->napi)) {
> +               /* disable interrupt */
> +               writel_relaxed(0, priv->base + PPE_INTEN);
>                 hrtimer_cancel(&priv->tx_coalesce_timer);
>                 __napi_schedule(&priv->napi);
>         }
> @@ -596,8 +593,7 @@ enum hrtimer_restart tx_done(struct hrtimer *hrtimer)
>  
>         if (napi_schedule_prep(&priv->napi)) {
>                 /* disable rx interrupt */
> -               priv->reg_inten &= ~(RCV_INT);
> -               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
> +               writel_relaxed(0, priv->base + PPE_INTEN);
>                 __napi_schedule(&priv->napi);
>         }
>  
> 
> .
> 

Re: [PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[Ding Tianhong]

On 2015/4/15 22:25, Arnd Bergmann wrote:
> On Wednesday 15 April 2015 20:30:02 Ding Tianhong wrote:
>> The patches series was used to fix the issues of the hip04 driver, and added
>> some optimizations according to some good suggestion. 
>>
>>
> 
> Thanks, that looks much better, except for patch 4 that I commented on.
> 
I will check and fix it.

> I had at some point sent a patch that is archived at
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318120.html
> 
> I believe that one is also still needed, but I have not tested whether it
> is correct. Can you have a look at the patch from back then and see if it
> works, of if you find something wrong about it?
> 
> I'm sending the unmodified patch from then here again for you to apply
> or comment. It will have to be rebased on top of your current changes.
> 
> 	Arnd
> 

Thanks for the suggestion, I notices that I miss this patch in my series from the maillist, 
I need time to check the code and try to test it, thanks for the work.:)

Ding

> 8<----
> Subject: [PATCH] net/hip04: refactor interrupt masking
> 
> The hip04 ethernet driver currently acknowledges all interrupts directly
> in the interrupt handler, and leaves all interrupts except the RX data
> enabled the whole time. This causes multiple problems:
> 
> - When more packets come in between the original interrupt and the
>   NAPI poll function, we will get an extraneous RX interrupt as soon
>   as interrupts are enabled again.
> 
> - The two error interrupts are by definition combining all errors that
>   may have happened since the last time they were handled, but just
>   acknowledging the irq without dealing with the cause of the condition
>   makes it come back immediately. In particular, when NAPI is intentionally
>   stalling the rx queue, this causes a storm of "rx dropped" messages.  
> 
> - The access to the 'reg_inten' field in hip_priv is used for serializing
>   access, but is in fact racy itself.
> 
> To deal with these issues, the driver is changed to only acknowledge
> the IRQ at the point when it is being handled. The RX interrupts now get
> acked right before reading the number of received frames to keep spurious
> interrupts to the absolute minimum without losing interrupts.
> 
> As there is no tx-complete interrupt, only an informational tx-dropped
> one, we now ack that in the tx reclaim handler, hoping that clearing
> the descriptors has also eliminated the error condition.
> 
> The only place that reads the reg_inten now just relies on
> napi_schedule_prep() to return whether NAPI is active or not, and
> the poll() function is then going to ack and reenabled the IRQ if
> necessary.
> 
> Finally, when we disable interrupts, they are now all disabled
> together, in order to simplify the logic and to avoid getting
> rx-dropped interrupts when NAPI is in control of the rx queue.
> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index 525214ef5984..83773247a368 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -56,6 +56,8 @@
>  #define RCV_DROP                       BIT(7)
>  #define TX_DROP                                BIT(6)
>  #define DEF_INT_ERR                    (RCV_NOBUF | RCV_DROP | TX_DROP)
> +#define DEF_INT_RX                     (RCV_INT | RCV_NOBUF | RCV_DROP)
> +#define DEF_INT_TX                     (TX_DROP)
>  #define DEF_INT_MASK                   (RCV_INT | DEF_INT_ERR)
>  
>  /* TX descriptor config */
> @@ -154,7 +156,6 @@ struct hip04_priv {
>         unsigned int port;
>         unsigned int speed;
>         unsigned int duplex;
> -       unsigned int reg_inten;
>  
>         struct napi_struct napi;
>         struct net_device *ndev;
> @@ -303,17 +304,15 @@ static void hip04_mac_enable(struct net_device *ndev)
>         val |= GE_RX_PORT_EN | GE_TX_PORT_EN;
>         writel_relaxed(val, priv->base + GE_PORT_EN);
>  
> -       /* clear rx int */
> -       val = RCV_INT;
> -       writel_relaxed(val, priv->base + PPE_RINT);
> +       /* clear prior interrupts */
> +       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
>  
>         /* config recv int */
>         val = GE_RX_INT_THRESHOLD | GE_RX_TIMEOUT;
>         writel_relaxed(val, priv->base + PPE_CFG_RX_PKT_INT);
>  
>         /* enable interrupt */
> -       priv->reg_inten = DEF_INT_MASK;
> -       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> +       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
>  }
>  
>  static void hip04_mac_disable(struct net_device *ndev)
> @@ -322,8 +321,7 @@ static void hip04_mac_disable(struct net_device *ndev)
>         u32 val;
>  
>         /* disable int */
> -       priv->reg_inten &= ~(DEF_INT_MASK);
> -       writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> +       writel_relaxed(0, priv->base + PPE_INTEN);
>  
>         /* disable tx & rx */
>         val = readl_relaxed(priv->base + GE_PORT_EN);
> @@ -403,6 +401,8 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force)
>         priv->tx_tail = tx_tail;
>         smp_wmb(); /* Ensure tx_tail visible to xmit */
>  
> +       writel_relaxed(DEF_INT_TX, priv->base + PPE_RINT);
> +
>  out:
>         if (pkts_compl || bytes_compl)
>                 netdev_completed_queue(ndev, pkts_compl, bytes_compl);
> @@ -458,9 +458,7 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>         if (count >= priv->tx_coalesce_frames) {
>                 if (napi_schedule_prep(&priv->napi)) {
>                         /* disable rx interrupt and timer */
> -                       priv->reg_inten &= ~(RCV_INT);
> -                       writel_relaxed(DEF_INT_MASK & ~RCV_INT,
> -                                      priv->base + PPE_INTEN);
> +                       writel_relaxed(0, priv->base + PPE_INTEN);
>                         hrtimer_cancel(&priv->tx_coalesce_timer);
>                         __napi_schedule(&priv->napi);
>                 }
> @@ -478,7 +476,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>         struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
>         struct net_device *ndev = priv->ndev;
>         struct net_device_stats *stats = &ndev->stats;
> -       unsigned int cnt = hip04_recv_cnt(priv);
> +       unsigned int cnt;
>         struct rx_desc *desc;
>         struct sk_buff *skb;
>         unsigned char *buf;
> @@ -489,6 +487,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>         u16 len;
>         u32 err;
>  
> +       /* acknowledge interrupts and read status */
> +       writel_relaxed(DEF_INT_RX, priv->base + PPE_RINT);
> +       cnt = hip04_recv_cnt(priv);
> +
>         while (cnt && !last) {
>                 buf = priv->rx_buf[priv->rx_head];
>                 skb = build_skb(buf, priv->rx_buf_size);
> @@ -539,11 +541,8 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>                         cnt = hip04_recv_cnt(priv);
>         }
>  
> -       if (!(priv->reg_inten & RCV_INT)) {
> -               /* enable rx interrupt */
> -               priv->reg_inten |= RCV_INT;
> -               writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN);
> -       }
> +       /* enable rx interrupt */
> +       writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN);
>         napi_complete(napi);
>  done:
>         /* clean up tx descriptors and start a new timer if necessary */
> @@ -564,23 +563,21 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id)
>         if (!ists)
>                 return IRQ_NONE;
>  
> -       writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT);
> -
>         if (unlikely(ists & DEF_INT_ERR)) {
> -               if (ists & (RCV_NOBUF | RCV_DROP))
> +               if (ists & (RCV_NOBUF | RCV_DROP)) {
>                         stats->rx_errors++;
>                         stats->rx_dropped++;
> -                       netdev_err(ndev, "rx drop\n");
> +                       netdev_dbg(ndev, "rx drop\n");
> +               }
>                 if (ists & TX_DROP) {
>                         stats->tx_dropped++;
> -                       netdev_err(ndev, "tx drop\n");
> +                       netdev_dbg(ndev, "tx drop\n");
>                 }
>         }
>  
> -       if (ists & RCV_INT && napi_schedule_prep(&priv->napi)) {
> -               /* disable rx interrupt */
> -               priv->reg_inten &= ~(RCV_INT);
> -               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
> +       if (napi_schedule_prep(&priv->napi)) {
> +               /* disable interrupt */
> +               writel_relaxed(0, priv->base + PPE_INTEN);
>                 hrtimer_cancel(&priv->tx_coalesce_timer);
>                 __napi_schedule(&priv->napi);
>         }
> @@ -596,8 +593,7 @@ enum hrtimer_restart tx_done(struct hrtimer *hrtimer)
>  
>         if (napi_schedule_prep(&priv->napi)) {
>                 /* disable rx interrupt */
> -               priv->reg_inten &= ~(RCV_INT);
> -               writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN);
> +               writel_relaxed(0, priv->base + PPE_INTEN);
>                 __napi_schedule(&priv->napi);
>         }
>  
> 
> .
> 


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