On Wed, Dec 16, 2009 at 08:25:03AM +0100, Wolfgang Grandegger wrote: > Marc Kleine-Budde wrote: > > Oliver Hartkopp wrote: > >> Kurt Van Dijck wrote: > >>> On Mon, Dec 14, 2009 at 08:57:37PM +0100, Oliver Hartkopp wrote: > >>>> Kurt Van Dijck wrote: > >>>>>>>> http://lxr.linux.no/#linux+v2.6.32/net/can/af_can.c#L218 > >>>>>>> Wolfgang, > >>>>>>> > >>>>>>> To address your concern, would a construction like this suit, when > >>>>>>> fitted in Oliver's proposal? > >>>>>>> > >>>>>>> inline int no_can_skb((struct sk_buff *skb) > >>>>>>> { > >>>>>>> struct can_frame *cf = (struct can_frame *)skb->data > >>>>>>> > >>>>>>> if ((skb->len != sizeof(*cf)) || ((cf->can_dlc > 8)) { > >>>>>>> if (skb->sk && !sock_flag(skb->sk, SOCK_DEAD)) { > >>>>>>> skb->sk->sk_err = EINVAL; > >>>>>>> skb->sk->sk_error_report(skb->sk); /* can this block?*/ > >>>>>>> } > >>>>>>> > >>>>>>> WARN_ONCE(1, "non conform skbuf: ..."); > >>>>>>> "Dropped non conform skbuf: len %d, can_dlc > >>>>>>> %d\n", > >>>>>>> skb->len, cf->can_dlc); > >>>>>>> return 1; > >>>>>>> } > >>>>>>> return 0; > >>>>>>> } > >>>>>> My primary concern is about using *WARN_ONCE*. The BUG, WARN, functions > >>>>>> and friends indicate to the user that there is a problem with the > >>>>>> kernel, e.g. a bug and therefore I prefer a simple dev_err(). Also the > >>>>> I see. > >>>>> I agree that a dev_err seems more appropriate here. > >>>>> Since this is TX path, kernel message flood is with respect to local > >>>>> activity? > >>>> Indeed i would like to have a PRINTK_ONCE() or DEV_ERR_ONCE() :-) > >>>> > >>>> > >>>>>> word "skbuf" does say little to the normal Linux users. I find > >>>>>> s/skbuf/packet/ more intuitive. Of course, if there is a better way to > >>>>> ack. > >>>>>> inform the user we should use it. Unfortunately, I can't tell if your > >>>>>> approach will work. > >>>>> I went into the code, in net/core/sock.c:sock_def_error_report. > >>>>> that looks like atomic code to me, so the above sk_error_report() stuff > >>>>> should work. > >>>>> no_can_skb() would not even need a dev_err in that case. > >>>> Using sk_error_report() and leaving out dev_err() is an interesting idea! > >>>> > >>>> Is that a usual way to provide this kind of error notification, e.g. for > >>>> broken PF_PACKET packets? > >>> I'm not familiar with PF_PACKET sockets. > >>> > >>> I encountered this sk_error_report() during my current development. > >>> It appears to be _the_ method of saying > >>> "something got in error", which must first be set by ->sk_err. > >> I wonder if socket layer error reporting is the right thing on netdriver > >> level. > > > >> ~/net-2.6/drivers/net$ find . -name \*.c | xargs grep sk_error > > > >> returns nothing - and i'm sure accessing skb->sk->sk_error_report(skb->sk) > >> on > >> driver level will bounce quite hard on netdev-ML ;-) > > > > well....ask them. I mean state our problem, give our sk_err solution and > > ask if we can make it better. > > Good idea. We could combine the question with the patch. Either it goes > in or they will advice us to use something else for error reporting to > the user. Ack.
Kurt _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
