[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices
Jan Kiszka wrote:
Wolfgang Grandegger wrote:
Jan Kiszka wrote:
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. :)
Puh, still plenty of issues. I wanted to make RTCAN public a.s.a.p.
because a few people already ask for it.
Most of my points are either small changes or just notes for future
cleanups, i.e. no show stoppers. The name, that one is critical.
See below.
- 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...
For the time being I fixed it to 29.
Ok.
- 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.
OK, I'm going to implement a general "debug" option selectable via
kernel parameter and proc filesystem during run time some time later.
Fine with me, but I would skip the work for a /proc switch.
There will be no separate switch for the PROC output. I was thinking
about one global debug option.
- 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_ALIGN32
#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.
I removed alignment because I think it's better to have all data close
together. What's the benefit of such an alignment. Note that the priv
structures are normally small.
This needs a closer look indeed. We adopted what Linux does without
checking if it makes sense for RTnet. The CAN devices' private data
structures are different from RT-NICs, so a different alignment may make
sense as well. Let's keep it as it is and push it on the to-do list.
IMHO, the alignment makes sense, if the private structure is used in a
different context. This is not true for RTCAN. I will put it to the TODO
list.
- rtcan_dev_unregister() looks racy: down(&rtcan_devices_nrt_lock) comes
after the device state check. Is this ok?
That's copied from rtdev.c. Indeed, it should be:
RTCAN_ASSERT(atomic_read(&dev->refcount) == 0,
printk("RTCAN: dev reference counter < 0!\n"););
rtdm_lock_put_irqrestore(&rtcan_devices_rt_lock, context);
up(&rtcan_devices_nrt_lock);
Argh, that locking is indeed like RTnet. And it makes me sick though I'm
likely responsible for most of this mess, thus also for this confusion
here. Please revert it for now until I sorted things in RTnet. The
locking must be simplified - and documented...
OK.
In any case, we may only suffer from theoretical races between
rtcanconfig and rmmod here. Uncritical.
Dito.
- 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.
I don't like the __u8 either. What do you mean with the last sentence?
What should be consistent?
The types for can_dlc in both of our own structures. If can_frame uses
unsigned int, rtcan_rb_frame should do so as well.
Ah, of course, will fix that.
- Well known issue: the RTCAN name. This should definitely get resolved
before we merge. Any feedback already?
I contacted the author. If I will not get an answer soon, I tend
changing the global name to RT-Socket-CAN (rtsocketcan).
I would really hate to have a drivers/rtsocketcan or a
rtdm/rtsocketcan.h. The short name is so much nicer.
He did not say, that we cannot use the name RTCAN but he prefers that we
use a different name to avoid confusion. Therefore I suggest to use the
offical name "RT-Socket-CAN" for the project, but leave almost all
internal rtcan prefixes as they are apart f
[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices
On Sat, 2006-08-05 at 18:44 +0200, Jan Kiszka wrote: > > > > Thanks a lot for your time. I'm going to provide a revised patch a.s.a.p. > > Again, the naming should be resolved, but then I think we could merge. > Philippe, ok for you as well? > Yes. > > > > A few other issues: > > > > Till now I do not use rt_owner. Is it necessary? > > That's for locking the driver module against removal. I guess this is > not needed for RTCAN if you can unregister a device at any time, maybe > just by spinning on the reference counter release. > > Jan > -- Philippe. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices
Wolfgang Grandegger wrote: - Well known issue: the RTCAN name. This should definitely get resolved before we merge. Any feedback already? >>> I contacted the author. If I will not get an answer soon, I tend >>> changing the global name to RT-Socket-CAN (rtsocketcan). >> >> I would really hate to have a drivers/rtsocketcan or a >> rtdm/rtsocketcan.h. The short name is so much nicer. > > He did not say, that we cannot use the name RTCAN but he prefers that we > use a different name to avoid confusion. Therefore I suggest to use the > offical name "RT-Socket-CAN" for the project, but leave almost all > internal rtcan prefixes as they are apart from: > > drivers/rtsocketcan > rtdm/rtsocketcan.h > > Note that the API does use the Linux naming in most cases (with the > prefix can). > > Another possibility would be to use rtscan as short form for rtsocketcan > as prefix. > > What do you think? Well, it's just a name. Never underestimate naming. Ok, I have this proposal now: o drivers/can/ - That's consistent with the existing subdir naming anyway. o rtdm/rtcan.h - The "rtdm/" prefix clearly defines the context: It's THE standard real-time CAN profile for RTDM. o All references to "RTCAN" in comments, READMEs, headers, etc. must be changed to RT-Socket-CAN. So it should be clear that this has nothing to do with the existing "rtcan" project. o Variable, type, and function names remain as they are. Jan PS: Another point for the long-term to-do list :-> : The nested locking and the global scope of certain locks. It's safe, it's harmless for current primary target platforms (UP), but it's not really beautiful when considering SMP setups. A bit tricky, for sure. signature.asc Description: OpenPGP digital signature ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices
Wolfgang Grandegger wrote:
> Jan Kiszka wrote:
>> 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. :)
>
> Puh, still plenty of issues. I wanted to make RTCAN public a.s.a.p.
> because a few people already ask for it.
Most of my points are either small changes or just notes for future
cleanups, i.e. no show stoppers. The name, that one is critical.
>
>> - 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...
>
> For the time being I fixed it to 29.
Ok.
>
>> - 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.
>
> OK, I'm going to implement a general "debug" option selectable via
> kernel parameter and proc filesystem during run time some time later.
Fine with me, but I would skip the work for a /proc 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_ALIGN32
>> #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.
>
> I removed alignment because I think it's better to have all data close
> together. What's the benefit of such an alignment. Note that the priv
> structures are normally small.
This needs a closer look indeed. We adopted what Linux does without
checking if it makes sense for RTnet. The CAN devices' private data
structures are different from RT-NICs, so a different alignment may make
sense as well. Let's keep it as it is and push it on the to-do list.
>
>> - rtcan_dev_unregister() looks racy: down(&rtcan_devices_nrt_lock) comes
>> after the device state check. Is this ok?
>
> That's copied from rtdev.c. Indeed, it should be:
>
> RTCAN_ASSERT(atomic_read(&dev->refcount) == 0,
> printk("RTCAN: dev reference counter < 0!\n"););
> rtdm_lock_put_irqrestore(&rtcan_devices_rt_lock, context);
> up(&rtcan_devices_nrt_lock);
Argh, that locking is indeed like RTnet. And it makes me sick though I'm
likely responsible for most of this mess, thus also for this confusion
here. Please revert it for now until I sorted things in RTnet. The
locking must be simplified - and documented...
In any case, we may only suffer from theoretical races between
rtcanconfig and rmmod here. Uncritical.
>> - 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.
>
> I don't like the __u8 either. What do you mean with the last sentence?
> What should be consistent?
The types for can_dlc in both of our own structures. If can_frame uses
unsigned int, rtcan_rb_frame should do so as well.
>>
>> - Well known issue: the RTCAN name. This should definitely get resolved
>> before we merge. Any feedback already?
>
> I contacted the author. If I will not get an answer soon, I tend
> changing the global name to RT-Socket-CAN (rtsocketcan).
I would really hate to have a drivers/rtsocketcan or a
rtdm/rtsocketcan.h. The short name is so much nicer.
>
>> - Low prio (as long as my own code doesn't follow this ;)): Linux coding
>> style.
>
> Thanks ;-).
>
>> 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.
>
> Thanks a lot for your time. I'm going to provide a revised patch a.s.a.p.
Again, the namin
Re: [Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices
On Sat, 2006-08-05 at 15:45 +0200, Wolfgang Grandegger wrote: > > - 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. > > Fine for me. Other comments? > It's ok with me too. > And how can I generate Doxygen documentation to check the integration of > the RTCAN device profile description? > Passing --enable-dox-doc to configure will cause Doxygen to be run on the sources as part of the build process. Doxygen.in should be updated so that new documented contents appear in the INPUT stanza. -- Philippe. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
[Xenomai-core] Re: [Xenomai-help] [ANNOUNCE] RTDM driver for CAN devices
Jan Kiszka wrote:
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. :)
Puh, still plenty of issues. I wanted to make RTCAN public a.s.a.p.
because a few people already ask for it.
- 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...
For the time being I fixed it to 29.
- 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.
OK, I'm going to implement a general "debug" option selectable via
kernel parameter and proc filesystem during run time some time later.
- 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.
There are a few open issues with the MSCAN hardware, especially entering
sleep mode (it does not work as expected/documented). I will cleanup anyhow.
- Can rtcan_mscan_exit() be tagged with __exit?
Of course, fixed.
- 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.
It's only useful for development and debugging. I will enable it with
the debug option mentioned above sometime later.
- 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_ALIGN32
#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.
I removed alignment because I think it's better to have all data close
together. What's the benefit of such an alignment. Note that the priv
structures are normally small.
- rtcan_dev_unregister() looks racy: down(&rtcan_devices_nrt_lock) comes
after the device state check. Is this ok?
That's copied from rtdev.c. Indeed, it should be:
RTCAN_ASSERT(atomic_read(&dev->refcount) == 0,
printk("RTCAN: dev reference counter < 0!\n"););
rtdm_lock_put_irqrestore(&rtcan_devices_rt_lock, context);
up(&rtcan_devices_nrt_lock);
- 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.
Fine for me. Other comments?
- 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.
These variables might be necessary for some other CAN controllers to
calculate the bit-timing parameters. Anyhow, I will remove them as long
as they are not needed.
- 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.
OK.
- [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.
Fine for me. Let me know when it will be available.
- 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.
OK, I will leave it for the moment.
- rtcan_dev_get_state_name(): Mm, having some array lookup for the name
