On Thu, Sep 10, 2009 at 04:07:00PM +0200, Wolfgang Grandegger wrote: > Date: Thu, 10 Sep 2009 16:07:00 +0200 > Subject: Re: [PATCH v3] ems_usb: CAN/USB driver for EMS CPC-USB/ARM7 CAN > interfaces > From: Wolfgang Grandegger <[email protected]> > To: Kurt Van Dijck <[email protected]> > CC: SocketCAN Core Mailing List <[email protected]>, > Oliver Hartkopp <[email protected]> > > 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. I was in the process of preparing a patch with alternate approach. > > > 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. ??? echo_skb is a cache for the skb's queued in hardware. The more you can queue in hardware, the bigger the echo_skb cache, not? So, it's hardware dependent. The ems_usb driver proved that 10 is a viable number for that driver. SJA1000 _can_ only use 1. why allocate 10 for sja1000? That's what I found hard to accept.
Kurt > > Wolfgang. > _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
