Re: [PATCH] can: ems_usb: Fix possible tx overflow

[David Miller]

From: Marc Kleine-Budde 
Date: Mon, 22 Feb 2016 13:18:38 +0100

> David, I can send a patch to clean up the coding style. Do you want it
> for net or via net-next?

net-next is fine.

Re: [PATCH] can: ems_usb: Fix possible tx overflow

[Marc Kleine-Budde]

On 02/22/2016 01:11 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 2/21/2016 6:27 PM, Marc Kleine-Budde wrote:
> 
>> From: Gerhard Uttenthaler 
>>
>> This patch fixes the problem that more CAN messages could be sent to the
>> interface as could be send on the CAN bus. This was more likely for slow baud
>> rates. The sleeping _start_xmit was woken up in the _write_bulk_callback. 
>> Under
>> heavy TX load this produced another bulk transfer without checking the
>> free_slots variable and hence caused the overflow in the interface.
>>
>> Signed-off-by: Gerhard Uttenthaler 
>> Cc: linux-stable 
>> Signed-off-by: Marc Kleine-Budde 
>> ---
>>   drivers/net/can/usb/ems_usb.c | 14 ++
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
>> index fc5b75675cd8..eb7192fab593 100644
>> --- a/drivers/net/can/usb/ems_usb.c
>> +++ b/drivers/net/can/usb/ems_usb.c
>> @@ -117,6 +117,9 @@ MODULE_LICENSE("GPL v2");
>>*/
>>   #define EMS_USB_ARM7_CLOCK 800
>>
>> +#define CPC_TX_QUEUE_TRIGGER_LOW25
>> +#define CPC_TX_QUEUE_TRIGGER_HIGH   35
>> +
>>   /*
>>* CAN-Message representation in a CPC_MSG. Message object type is
>>* CPC_MSG_TYPE_CAN_FRAME or CPC_MSG_TYPE_RTR_FRAME or
>> @@ -278,6 +281,11 @@ static void ems_usb_read_interrupt_callback(struct urb 
>> *urb)
>>  switch (urb->status) {
>>  case 0:
>>  dev->free_slots = dev->intr_in_buffer[1];
>> +if(dev->free_slots > CPC_TX_QUEUE_TRIGGER_HIGH){
>> +if (netif_queue_stopped(netdev)){
>> +netif_wake_queue(netdev);
>> +}
>> +}
> 
> Hm, did anyone run scripts/checkpatch.pl on this patch?

Obviously not. :(

David, I can send a patch to clean up the coding style. Do you want it
for net or via net-next?

Marc
-- 
Pengutronix e.K.  | Marc Kleine-Budde   |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund  | Fax:   +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] can: ems_usb: Fix possible tx overflow

[Sergei Shtylyov]

Hello.

On 2/21/2016 6:27 PM, Marc Kleine-Budde wrote:


From: Gerhard Uttenthaler 

This patch fixes the problem that more CAN messages could be sent to the
interface as could be send on the CAN bus. This was more likely for slow baud
rates. The sleeping _start_xmit was woken up in the _write_bulk_callback. Under
heavy TX load this produced another bulk transfer without checking the
free_slots variable and hence caused the overflow in the interface.

Signed-off-by: Gerhard Uttenthaler 
Cc: linux-stable 
Signed-off-by: Marc Kleine-Budde 
---
  drivers/net/can/usb/ems_usb.c | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index fc5b75675cd8..eb7192fab593 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -117,6 +117,9 @@ MODULE_LICENSE("GPL v2");
   */
  #define EMS_USB_ARM7_CLOCK 800

+#define CPC_TX_QUEUE_TRIGGER_LOW   25
+#define CPC_TX_QUEUE_TRIGGER_HIGH  35
+
  /*
   * CAN-Message representation in a CPC_MSG. Message object type is
   * CPC_MSG_TYPE_CAN_FRAME or CPC_MSG_TYPE_RTR_FRAME or
@@ -278,6 +281,11 @@ static void ems_usb_read_interrupt_callback(struct urb 
*urb)
switch (urb->status) {
case 0:
dev->free_slots = dev->intr_in_buffer[1];
+   if(dev->free_slots > CPC_TX_QUEUE_TRIGGER_HIGH){
+   if (netif_queue_stopped(netdev)){
+   netif_wake_queue(netdev);
+   }
+   }


   Hm, did anyone run scripts/checkpatch.pl on this patch?

[...]

MBR, Sergei

[PATCH] can: ems_usb: Fix possible tx overflow

[Marc Kleine-Budde]

From: Gerhard Uttenthaler 

This patch fixes the problem that more CAN messages could be sent to the
interface as could be send on the CAN bus. This was more likely for slow baud
rates. The sleeping _start_xmit was woken up in the _write_bulk_callback. Under
heavy TX load this produced another bulk transfer without checking the
free_slots variable and hence caused the overflow in the interface.

Signed-off-by: Gerhard Uttenthaler 
Cc: linux-stable 
Signed-off-by: Marc Kleine-Budde 
---
 drivers/net/can/usb/ems_usb.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index fc5b75675cd8..eb7192fab593 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -117,6 +117,9 @@ MODULE_LICENSE("GPL v2");
  */
 #define EMS_USB_ARM7_CLOCK 800
 
+#define CPC_TX_QUEUE_TRIGGER_LOW   25
+#define CPC_TX_QUEUE_TRIGGER_HIGH  35
+
 /*
  * CAN-Message representation in a CPC_MSG. Message object type is
  * CPC_MSG_TYPE_CAN_FRAME or CPC_MSG_TYPE_RTR_FRAME or
@@ -278,6 +281,11 @@ static void ems_usb_read_interrupt_callback(struct urb 
*urb)
switch (urb->status) {
case 0:
dev->free_slots = dev->intr_in_buffer[1];
+   if(dev->free_slots > CPC_TX_QUEUE_TRIGGER_HIGH){
+   if (netif_queue_stopped(netdev)){
+   netif_wake_queue(netdev);
+   }
+   }
break;
 
case -ECONNRESET: /* unlink */
@@ -526,8 +534,6 @@ static void ems_usb_write_bulk_callback(struct urb *urb)
/* Release context */
context->echo_index = MAX_TX_URBS;
 
-   if (netif_queue_stopped(netdev))
-   netif_wake_queue(netdev);
 }
 
 /*
@@ -587,7 +593,7 @@ static int ems_usb_start(struct ems_usb *dev)
int err, i;
 
dev->intr_in_buffer[0] = 0;
-   dev->free_slots = 15; /* initial size */
+   dev->free_slots = 50; /* initial size */
 
for (i = 0; i < MAX_RX_URBS; i++) {
struct urb *urb = NULL;
@@ -835,7 +841,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, 
struct net_device *ne
 
/* Slow down tx path */
if (atomic_read(>active_tx_urbs) >= MAX_TX_URBS ||
-   dev->free_slots < 5) {
+   dev->free_slots < CPC_TX_QUEUE_TRIGGER_LOW) {
netif_stop_queue(netdev);
}
}
-- 
2.7.0