Kurt Van Dijck wrote:
> On Thu, Sep 10, 2009 at 02:29:37PM +0200, Wolfgang Grandegger wrote:
> [...]
>>>>> Index: include/linux/can/dev.h
>>>>> ===================================================================
>>>>> --- include/linux/can/dev.h (Revision 1048)
>>>>> +++ include/linux/can/dev.h (Arbeitskopie)
>>>>> @@ -32,7 +32,7 @@
>>>>> /*
>>>>> * CAN common private data
>>>>> */
>>>>> -#define CAN_ECHO_SKB_MAX 4
>>>>> +#define CAN_ECHO_SKB_MAX 10
>>>>>
>>>>> struct can_priv {
>>>>> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23)
>>>>> Index: include/socketcan/can/dev.h
>>>>> ===================================================================
>>>>> --- include/socketcan/can/dev.h (Revision 1048)
>>>>> +++ include/socketcan/can/dev.h (Arbeitskopie)
>>>>> @@ -32,7 +32,7 @@
>>>>> /*
>>>>> * CAN common private data
>>>>> */
>>>>> -#define CAN_ECHO_SKB_MAX 4
>>>>> +#define CAN_ECHO_SKB_MAX 10
>>>>>
>>>>> struct can_priv {
>>>>> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23)
>>>> For the moment I want to keep the old settings. We need to discuss how
>>>> to handle this in a generic way. Either by increasing it to 8, let's
>>>> say, or by providing a kernel config. Other opinions?
>>> with regard to CAN_ECHO_SKB_MAX, I had the same problem in the softing
>>> driver.
>>> would some interface like this:
>> [...]
>>> +struct net_device *alloc_candev(int sizeof_priv, int echo_skb_max);
>> This is a good solution if we want the driver to set the number of echo
>> skb's. I would actually put the echo_skbs at the end of the
>> netdev_priv(). Would it also be useful that the user decides via kernel
>> configuration option?
> 1) 'behind netdev_priv'
> so a layout in memory:
> [[can_priv]driver_priv][echo_skb[]]
Yep.
> this is possible, not breaking things. But still, you'd need a pointer
> in from within can_priv, as the exact size of the priv is lost after the
> constructor. so, it justs saves the heap overhead.
Of course. The address of the echo skbs should be *close* to the other
addresses used by the CAN network device.
> It must then be allocated during alloc_candev(), and requires an extra
> argument. I'm not sure how easily such change gets accepted in mainline.
I don't see a real problem here. Other opinions? I'm going to prepare a
patch now.
> 2) 'kernel config option'
> I assume that every device will have the same echo_skb_max then?
>
> Before choosing a value, a user needs to be informed very well, but I
> can imagine that a user does not even understand the consequences, ie. I
> have difficulties understanding the relation between echo_skb_max & URB
> traffic in a driver _I didn't write_. How would I choose a good value?
>
> IMO, such things are very driver specific, and thus better put const in
> the driver. But this requires a variable echo_skb_max from can_priv
> scope. That why I put both echo_skb & echo_skb_max as members.
> This addressess the problematic choice of a static echo_skb_max that
> would otherwise be left to the end user.
>
> Please mind this does not address how to malloc such things (ie. after
> netdev_priv, or seperate block in heap).
The user may want to restrict the number of messages queued in hardware
for the sake of latency. But as TX messages are queued in software as
well and as there is not yet a facility to handle prioritized messages
it's currently not really a strong argument.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
