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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 22.02.2012 16:19, Stephane Grosjean wrote:

> 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


Hm - i didn't test that so far.

But nice it works :-)

> 
> - 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


Doesn't fit to your described setup ... anyway ...

> 
> (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 ;-)


i can imagine that :-)

Pls try the following:

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

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

PC2:
cangen -g0 -i can0
cangen -g0 -i can1
candump any

and then pull the usb-pro from PC1 ...

That's really unfriendly 8-)

> 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.


Please check this use-case above. And when it works with your fixed driver you
can probalby check the 'old' v5 version to see if you can follow my bug report
on your setup.

Best regards,
Oliver