login
Header Space

 
 

Re: [PATCH 2/7] CAN: Add PF_CAN core module

Score:
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
To: Urs Thuermann <urs@...>
Cc: <netdev@...>, David Miller <davem@...>, Thomas Gleixner <tglx@...>, Oliver Hartkopp <oliver@...>, Oliver Hartkopp <oliver.hartkopp@...>, Urs Thuermann <urs.thuermann@...>
Date: Tuesday, September 18, 2007 - 9:31 am

Urs Thuermann wrote:


Looks pretty good, please see below for a few comments (mostly minor
nitpicking, a few things that look like real bugs). Nothing that
couldn't be fixed after merging though.



Is this file used only from within the kernel? If so you could use
the nicer-to-look-at u8/u16/u32 types instead of the double underscored
ones.



The callers of the unregister function don't check the return code,
and they can't handle errors anyways since they use it in the
module unload path, so making it void seems more appropriate
(and maybe a WARN_ON for the "not-found" case).



Same here, none of the callers check the return value and since
they're all declared as void they can't propagate any errors back.



It seems most of the things declared in that file are only used within
af_can.c. Might be easier to read the code if you'd just move it over.



This seems to be only used in af_can.c, so it could be static.
__read_mostly also seems to be approriate. There are a few
more that look like they could be __read_mostly below, but
I'll skip these since you probably know better than me.



Same here (static).



Both of these printks seem to be user-triggerable, so they should
be rate-limited (or maybe get removed completely/changed to DBG).



What prevents the module from getting unloaded again (and using
a stale pointer)?



Shouldn't this be done before attempting the module load?



EPERM doesn't seem like the best fit, but I don't have a better
suggestion myself at the moment.



So the intention here is to send the packet to the non-loopback device
and manually loop it, which means sending it twice?



On the receive path you use RCU, so this should be
hlist_for_each_entry_rcu(), no? The bottem half disabling during
addition/removal also seems unnecessary, but I might be missing
something.



netdevice registration should never happen from interrupt handlers.


__read_mostly (for those below as well)?


round_jiffies?




If I'm not mistaken this comment is outdated and should refer to
can_stat_update().



round_jiffies?



Would be nicer to use seq_file (for all the proc stuff).



-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[PATCH 2/7] CAN: Add PF_CAN core module, Urs Thuermann, (Mon Sep 17, 6:03 am)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Patrick McHardy, (Tue Sep 18, 9:31 am)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Urs Thuermann, (Tue Sep 18, 5:20 pm)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Patrick McHardy, (Wed Sep 19, 4:27 am)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Urs Thuermann, (Thu Sep 20, 4:53 am)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Patrick McHardy, (Thu Sep 20, 6:33 am)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Urs Thuermann, (Thu Sep 20, 7:30 am)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Patrick McHardy, (Thu Sep 20, 7:43 am)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Urs Thuermann, (Tue Sep 18, 10:54 am)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Patrick McHardy, (Tue Sep 18, 11:07 am)
Re: [PATCH 2/7] CAN: Add PF_CAN core module, Paul E. McKenney, (Mon Sep 17, 11:50 am)
speck-geostationary