On Thu, Sep 10, 2009 at 01:29:34PM +0200, Wolfgang Grandegger wrote: > Date: Thu, 10 Sep 2009 13:29:34 +0200 > Subject: Re: [PATCH v3] ems_usb: CAN/USB driver for EMS CPC-USB/ARM7 CAN > interfaces > From: Wolfgang Grandegger <[email protected]> > To: Sebastian Haas <[email protected]> > Cc: SocketCAN Core Mailing List <[email protected]>, > Oliver Hartkopp <[email protected]> > > Hi Sebastian, > > Fedora 11 provides Thunderbird 3.0b2, which is obviously buggy :-(. I > now installed 2.0 and the problems with the DOS endings and wired space > substitutions are gone. Sorry for the inconvenience it caused. > > Sebastian Haas wrote: > > This patch adds support for the CAN/USB interface CPC-USB/ARM7 from EMS > > Dr. Thomas Wuensche. > > > > Signed-off-by: Sebastian Haas <[email protected]> > > > > Makefile | 1 > > drivers/net/can/Kconfig | 7 > > drivers/net/can/Makefile | 1 > > drivers/net/can/ems_usb.c | 1140 > > ++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/can/dev.h | 2 > > include/socketcan/can/dev.h | 2 > > 6 files changed, 1151 insertions(+), 2 deletions(-) > > > > 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: Index: include/linux/can/dev.h =================================================================== --- include/linux/can/dev.h (revision 1053) +++ include/linux/can/dev.h (working copy) @@ -50,7 +50,8 @@ int restart_ms; struct timer_list restart_timer; - struct sk_buff *echo_skb[CAN_ECHO_SKB_MAX]; + struct sk_buff **echo_skb; + size_t echo_skb_num; int (*do_set_bittiming)(struct net_device *dev); int (*do_set_mode)(struct net_device *dev, enum can_mode mode); @@ -74,6 +75,7 @@ struct net_device *alloc_candev(int sizeof_priv); void free_candev(struct net_device *dev); +int candev_set_max_echo_skb(struct net_device *dev, size_t n); int open_candev(struct net_device *dev); void close_candev(struct net_device *dev);
solve any problem. The netto memory usage for SJA1000 chips would be 1 pointer _less_ as you'd allocate an array of 1 (thereby ignoring the heap overhead). For other chips, like this ems, the extra memory usage (1 size_t, 1 ptr) is minimal with regard to the allocated buffer of 16, or 24 entries. I did not implement this modification yet. It's rather an idea. I would, per default, allocate a 1 element array for echo_skb. directly after alloc_candev, a candev_set_max_echo_skb can be done. To prevent locking, problems, we could also do: @@ -72,7 +73,7 @@ struct net_device_stats *can_get_stats(struct net_device *dev); #endif -struct net_device *alloc_candev(int sizeof_priv); +struct net_device *alloc_candev(int sizeof_priv, int echo_skb_max); void free_candev(struct net_device *dev); int open_candev(struct net_device *dev); which will generate compiler errrors for existing code. It's easier, but touches existing kernel stuff. > > I'm still not happy with a few coding issues, e.g.: > > > + struct ems_usb *dev = usb_get_intfdata(intf); > > > + struct ems_usb *dev = netdev_priv(netdev); > > > + struct ems_usb *priv = netdev_priv(dev); > > > +static int ems_usb_set_mode(struct net_device *dev, ... > > > +static int ems_usb_start_xmit(struct sk_buff *skb, struct net_device > *netdev) > > The inconsistant use of dev, priv and netdev is confusing when you read > the code. Also the error messages could still be improved. Anyhow, I'm > going to apply the patch to the SVN trunk as is now. Feel free to > improve the code quality when you have time. > > BTW: does hot plugging work properly? > > Thanks for your contribution. > > Wolfgang. > > _______________________________________________ > Socketcan-core mailing list > [email protected] > https://lists.berlios.de/mailman/listinfo/socketcan-core _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
