Re: [PATCH 0/3 v5] can/usb: Add PEAK-System PCAN USB adapters driver

[Stephane Grosjean <s.grosjean@peak-system.com>]

Le 22/02/2012 08:30, Oliver Hartkopp a écrit :
> On 22.02.2012 08:05, Oliver Hartkopp wrote:
>
>
> Just printing this status and continue with business as usual (netif_wake_queue!) is probably not the right approach 8-)
>
> Please check if it makes sense to check (urb->status != 0) earlier in this callback function.
>

Hi Oliver,

I took time to have a look to how things are done in other (net+usb) 
drivers and it's not easy to conclude to anything: some are testing that 
status (but with some other errno values) while others are always waking 
up the netdev queues... So, I tried to do a "mixed-of", cleaned up the 
code and tested it in (something like) your conditions, that is:

PC1:
can0 -> usb-pro ch1 -+
can1 -> usb-pro ch2  |
can2 -> usb ---------+
                      | 1Mpbs
PC2:                 |
can0 -> pcmcia ch1 --+

PC1:
cangen -g0 -i can0
cangen -g0 -i can2
candump any

I did the two following tests:

- removing the peak_usb driver while sending/reading data:

[ 4354.534576] peak_usb 1-1:1.0: can0: attached to PCAN-USB Pro channel 
0 (device 1898240)
[ 4354.535574] peak_usb 1-1:1.0: can1: attached to PCAN-USB Pro channel 
1 (device 2)
[ 4360.388020] usb 4-1: new full speed USB device number 7 using uhci_hcd
[ 4360.588035] peak_usb 4-1:1.0: PEAK-System PCAN-USB adapter hwrev 28 
serial FFFFFFFF (1 channel)
[ 4360.592034] peak_usb 4-1:1.0: can2: attached to PCAN-USB channel 0 
(device 255)
[ 4363.753037] peak_usb 1-1:1.0: can0: setting ccbt=0x00140006
[ 4363.770637] peak_usb 1-1:1.0: can1: setting ccbt=0x00140006
[ 4363.787775] peak_usb 4-1:1.0: can2: setting BTR0=0x00 BTR1=0x14
[ 4385.878968] usbcore: deregistering interface driver peak_usb
[ 4385.920049] peak_usb 4-1:1.0: can2 removed
[ 4385.948036] peak_usb 1-1:1.0: can1 removed
[ 4385.964042] peak_usb 1-1:1.0: can0 removed
[ 4385.964076] peak_usb: PCAN-USB interfaces driver unloaded

=> ok

- unplug the usb then usb-pro adapters while sending/reading data:

[ 5043.008127] peak_usb 4-1:1.0: PEAK-System PCAN-USB adapter hwrev 28 
serial FFFFFFFF (1 channel)
[ 5043.012133] peak_usb 4-1:1.0: can0: attached to PCAN-USB channel 0 
(device 255)
[ 5048.029973] peak_usb 1-1:1.0: PEAK-System PCAN-USB Pro hwrev 0 serial 
FFFFFFFF.00000001 (2 channels)
[ 5048.030596] peak_usb 1-1:1.0: can1: attached to PCAN-USB Pro channel 
0 (device 1898240)
[ 5048.031717] peak_usb 1-1:1.0: can2: attached to PCAN-USB Pro channel 
1 (device 2)
[ 5053.795570] peak_usb 4-1:1.0: can0: setting BTR0=0x00 BTR1=0x14
[ 5053.825418] peak_usb 1-1:1.0: can1: setting ccbt=0x00140006
[ 5053.844272] peak_usb 1-1:1.0: can2: setting ccbt=0x00140006
[ 5081.440039] usb 4-1: USB disconnect, device number 8
[ 5081.468028] peak_usb 4-1:1.0: can0 removed
[ 5096.856274] usb 1-1: USB disconnect, device number 16
[ 5096.876038] peak_usb 1-1:1.0: can2 removed
[ 5096.896051] peak_usb 1-1:1.0: can1 removed

(then rmmod):

[ 6489.510730] usbcore: deregistering interface driver peak_usb
[ 6489.510756] peak_usb: PCAN-USB interfaces driver unloaded

=> Ok

(I definitively prefer my logs to yours ;-)

In my opinion, the Kernel hangs you got don't come from the 
netif_wake_queue() wrong usage, but these tests lead to run some failure 
cases in the driver with potential bad memory usage... So I did a pass 
on (I hope) all of these cases too. Anyway, this should be better now.

So, if you don't have any other comments or any other (not very 
friendly) tests, I'm ready to send a new version of the peak_usb.

Once again, thanks for your time.

Stéphane



--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com