Hi Wolfgang,

Wolfgang Grandegger wrote:
> Hello,
> 
> at "ftp://ftp.denx.de/pub/xenomai/rtcan"; you can find a first version of
> RTCAN, an Open Source hard real-time protocol stack for CAN devices
> based on BSD sockets. It is based on the SJA1000 socket-based CAN driver
> for RTDM by Sebastian Smolorz (see CREDITS file).

I started a review of the patch, and here are some first results. A
deeper look into critical paths is pending. But given that your
driver(s) already proved to work fine for us here and for your own
scenarios, only very sneaking issues can persist - if at all. :)

- AF_CAN is still set to 30 which meanwhile became TIPC. The Socket-CAN
project moved to 29 now. However, this could become a nice race until
Socket-CAN is upstream and reserved a final ID. How to cope with this?
We just had some fun here after moving our AF_TIMS from 0 to 27,
breaking many installation on our robots due to the ABI change. Should
we better move the ID beyond AF_MAX for now? Also a question for the
socketcan-core list...

- Out-commented debug messages: If they can be useful for driver
development / hardware testing, I would say make them selectable via
some CONFIG-switch. The rest should be removed.

- rtcan_mscan_mode_stop() contains some #if 1-#else-#endif block. What
are the alternatives here? Is it an open issue or just a pending cleanup?

- rtcan_mscan_init_one() contains a #ifdef FIXME. What needs to be
fixed? Comments for non-obvious pending issues would be helpful.

- Can rtcan_mscan_exit() be tagged with __exit?

- Is rtcan_mscan_proc_regs() a kind of debug option or useful for normal
operation as well? If it's the former, we may bundle it with the debug
print CONFIG-switch.

- Private data alignment in rtcan_dev_alloc(): you tapped on something
that looks ugly in RTnet as well. We copied this from Linux which still
does magic 32-byte alignment (encapsulated by NETDEV_ALIGN today). But
it also uses a smarter priv lookup now:

#define NETDEV_ALIGN            32
#define NETDEV_ALIGN_CONST      (NETDEV_ALIGN - 1)

static inline void *netdev_priv(struct net_device *dev)
{
        return (char *)dev + ((sizeof(struct net_device)
                                        + NETDEV_ALIGN_CONST)
                                & ~NETDEV_ALIGN_CONST);
}

Maybe something that should be adopted as well (for both stacks).
Nothing critical though.

- rtcan_dev_unregister() looks racy: down(&rtcan_devices_nrt_lock) comes
after the device state check. Is this ok?

- EXPORT_SYMBOL vs. EXPORT_SYMBOL_GPL: For me this makes no practical
difference anymore (kernel modules, specifically drivers must be GPL, no
matter what kind of symbols they use). Anyway, shouldn't we better go
the kernel path and introduce new APIs under _GPL? Just takes a wrapper
for 2.4 I guess.

- struct rtcan_device: what the heck are max_brp and max_sjw
(comments...)? This isn't something generic, right? That's why "FIXME" I
guess.

- LIST_POISON1 and some other stuff in rtcan_internal.h should
definitely be moved to the nucleus's wrapper. Nothing critical, just a
to-do. Likely the whole header will be obsolete in the end.

- [This is a note for me] RTCAN_ASSERT: Looks like we should introduce
RTDM_ASSERT() for this, maybe accepting some driver subsystem ID to
control this separately from the rest of Xenomai.

- RTCAN_PROC_... requires some thoughts: Generalise it? Some users
(haven't checked RTCAN for this, but RTnet has some) should better be
converted to seq_operations. And the rest could benefit from a generic
interface here. Uncritical for now.

- rtcan_dev_get_state_name(): Mm, having some array lookup for the name
here may shorten things.

- rtcan_read_proc_info: Should be done as in rtcan_mscan_proc_regs. Even
better: By increasing the buffer block in RTCAN_PROC_PRINT_VARS() you
could combine several PROC_PRINTs to a single one.

- rtcan_raw_remove_filter(): "if (sock->flistlen < 0)", a real check or
rather an internal ASSERT?

- struct rtcan_rb_frame differs from struct can_frame in the type of
can_dlc. The VW guys find __u8 for this nicer than a natural unsigned
int, but I stopped arguing on this. If we follow or not, it should be
consistent.

- rtcan_socket_context(): container_of?

- [Another note for me] module_param_array vs. MODULE_PARM-array: there
is some not that bad wrapper in RTnet now. I should push it to Xenomai
to simplify such blocks.

- Well known issue: the RTCAN name. This should definitely get resolved
before we merge. Any feedback already?

- Low prio (as long as my own code doesn't follow this ;)): Linux coding
style.


No guarantee that I found every critical point (most of the stuff above
is nitpicking anyway). I will try to repeat this more in details when
time permits. Hope my comments help nevertheless.

Jan

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to