You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by Carlos Sanchez <ca...@geotab.com.INVALID> on 2025/04/11 10:16:18 UTC
socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up
Hi devs,
Short history: in all the existing socketcan drivers in Nuttx (imxrt,
s32k3xx, s32k1xx, stm32h7 and kinetis), the driver-specific
implementation of ioctl(...SIOCSCANBITRATE...) calls ifup at the end.
Is there a rationale / spec for this?
Long history:
In Linux, CAN bitrate is set with netlink, not ioctl, and you need to
set a bitrate before bringing the interface up (make sense). In Nuttx,
you can begin the interface up (it uses a default bitrate) and then
you can change it. My guess is calling ifup at the end is precisely to
avoid requiring another ifdown/ifup cycle from app code.
SocketCAN is a Linux thing, so we should try to mimic the behaviour.
Implementing netlink would be overkill (and moreover, it is common in
Linux to use libsocketcan to abstract the netlink internals anyways)
but at least we should have similar semantics. In our use case, we
have a portable Linux/Nuttx app which uses socketcan, we have a thin
compatibility layer for Linux/Nuttx differences (so bitrate setting
uses icotl on Nuttx and libsocketcan in Linux) but it is difficult to
make such a thin layer accommodate this difference (it would need to
store the setting, and only call the ioctl just before bringing the
interface up... essentially duplicating the driver functionality).
Couldn't we (Nuttx):
1) Make SIOCSCANBITRATE just set the baudrate (and avoid changing
interface state)? It could even fail if the interface is already up
(AFAIK this is what netlink would do).
2) Provide a slightly more detailed documentation of Nuttx-specific
SocketCAN ioctls (like, specifying whether the interface state is
changed under the hood)
I volunteer to make the change, but this might affect application
space so I'd like to know what others' thoughts are.
BR
Carlos
Re: [EXT] socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up
Posted by Carlos Sanchez <ca...@geotab.com.INVALID>.
> > While testing this, I think I have discovered a small mistake on my
> > previous, Nuttx-side PR, which slipped by me and by revision: "ret"
> > might be used uninitialized now because I removed the assignment on
> > stm32_fdcan_sock:1976
> Yes, please make a PR to fix that!! When it's ready, post about it in this
> thread so we can get it reviewed and merged quickly.
https://github.com/apache/nuttx/pull/16224
I will separate the (unrelated) bitrate discussion to a new thread.
Thanks!
Carlos
Re: [EXT] socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Apr 15, 2025 at 1:25 PM Carlos Sanchez
<ca...@geotab.com.invalid> wrote:
> Related PR on the apps side, to make slcan work after the "bitrate no
> longer brings interface up" change:
> https://github.com/apache/nuttx-apps/pull/3059
>
> While testing this, I think I have discovered a small mistake on my
> previous, Nuttx-side PR, which slipped by me and by revision: "ret"
> might be used uninitialized now because I removed the assignment on
> stm32_fdcan_sock:1976
>
>
> https://github.com/apache/nuttx/pull/16199/files#diff-8efec4e19b7401adcc3961c078b98ab945ebcada8635105fb591d16cbd6e5210L1976
>
> Should I make a mini PR for this?
Hi,
Good catch!
Yes, please make a PR to fix that!! When it's ready, post about it in this
thread so we can get it reviewed and merged quickly.
Thanks,
Nathan
Re: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up)
Posted by Carlos Sanchez <ca...@geotab.com.INVALID>.
I have created the PRs:
nuttx: https://github.com/apache/nuttx/pull/16225
nuttx-apps: https://github.com/apache/nuttx-apps/pull/3061
BR
Carlos
On Wed, Apr 16, 2025 at 10:07 AM raiden00pl <ra...@gmail.com> wrote:
> +1 for compatibility with Linux.
>
> śr., 16 kwi 2025 o 10:04 Xiang Xiao <xi...@gmail.com>
> napisał(a):
>
> > On Wed, Apr 16, 2025 at 3:53 PM Peter van der Perk <
> > peter.vanderperk@nxp.com>
> > wrote:
> >
> > > can_ioctl_data_s is in a 32-bit union.
> > >
> > >
> >
> https://github.com/apache/nuttx/blob/40c6af6dec0d769ac57f69e89709f9d6310ee0c6/include/net/if.h#L299
> > > Making it 64-bit would break that union.
> > >
> > >
> > but lifr_ifru doesn't require its member size to be less than 64-bit, why
> > not align can_ioctl_data_s with Linux definition.
> >
> > Hence back then it was chosen to use kHz instead.
> > >
> > > Yours sincerely,
> > >
> > > Peter van der Perk
> > >
> > > -----Original Message-----
> > > From: Carlos Sanchez <ca...@geotab.com.INVALID>
> > > Sent: Wednesday, April 16, 2025 9:37 AM
> > > To: dev@nuttx.apache.org
> > > Subject: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...)
> > > brings the interface up)
> > >
> > >
> > > > > I was going to propose another global CAN change, removing the 1000
> > > > > factor from bitrate thus making bitrate calls use units in Hz
> > > > > instead of kHz, so we can discuss that and (if agreed) I make the
> > > > > change on the same PR to avoid creating so a tiny one.
> > >
> > > > What's the unit used on Linux? we need to align the interface with
> > Linux.
> > >
> > > Units in linux are Hz, according to
> > >
> > >
> >
> https://docs.kernel.org/networking/can.html#the-can-network-device-driver-interface
> > >
> > > One problem with using kHz is Single Wire Can (SAE J2411) uses bitrates
> > of
> > > 33333 bps (83333 bps in high speed mode) so representing this in kbps
> > > causes a timing error which can be significant in some cases.
> > >
> > > The change would require modifying arbi_bitrate and data_bitrate fields
> > on
> > > struct can_ioctl_data_s from uint16_t to uint32_t, otherwise usual
> > bitrates
> > > cannot be represented.
> > >
> > > Carlos
> > >
> >
>
--
Carlos Sanchez (he, him, his)
Geotab
Senior Team Lead, Embedded Development | Europe
Visit
www.geotab.com
Twitter <https://twitter.com/geotab> | Facebook
<https://www.facebook.com/Geotab> | YouTube
<https://www.youtube.com/user/MyGeotab> | LinkedIn
<https://www.linkedin.com/company/geotab/>
Re: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up)
Posted by raiden00pl <ra...@gmail.com>.
+1 for compatibility with Linux.
śr., 16 kwi 2025 o 10:04 Xiang Xiao <xi...@gmail.com> napisał(a):
> On Wed, Apr 16, 2025 at 3:53 PM Peter van der Perk <
> peter.vanderperk@nxp.com>
> wrote:
>
> > can_ioctl_data_s is in a 32-bit union.
> >
> >
> https://github.com/apache/nuttx/blob/40c6af6dec0d769ac57f69e89709f9d6310ee0c6/include/net/if.h#L299
> > Making it 64-bit would break that union.
> >
> >
> but lifr_ifru doesn't require its member size to be less than 64-bit, why
> not align can_ioctl_data_s with Linux definition.
>
> Hence back then it was chosen to use kHz instead.
> >
> > Yours sincerely,
> >
> > Peter van der Perk
> >
> > -----Original Message-----
> > From: Carlos Sanchez <ca...@geotab.com.INVALID>
> > Sent: Wednesday, April 16, 2025 9:37 AM
> > To: dev@nuttx.apache.org
> > Subject: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...)
> > brings the interface up)
> >
> >
> > > > I was going to propose another global CAN change, removing the 1000
> > > > factor from bitrate thus making bitrate calls use units in Hz
> > > > instead of kHz, so we can discuss that and (if agreed) I make the
> > > > change on the same PR to avoid creating so a tiny one.
> >
> > > What's the unit used on Linux? we need to align the interface with
> Linux.
> >
> > Units in linux are Hz, according to
> >
> >
> https://docs.kernel.org/networking/can.html#the-can-network-device-driver-interface
> >
> > One problem with using kHz is Single Wire Can (SAE J2411) uses bitrates
> of
> > 33333 bps (83333 bps in high speed mode) so representing this in kbps
> > causes a timing error which can be significant in some cases.
> >
> > The change would require modifying arbi_bitrate and data_bitrate fields
> on
> > struct can_ioctl_data_s from uint16_t to uint32_t, otherwise usual
> bitrates
> > cannot be represented.
> >
> > Carlos
> >
>
Re: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up)
Posted by Xiang Xiao <xi...@gmail.com>.
On Wed, Apr 16, 2025 at 3:53 PM Peter van der Perk <pe...@nxp.com>
wrote:
> can_ioctl_data_s is in a 32-bit union.
>
> https://github.com/apache/nuttx/blob/40c6af6dec0d769ac57f69e89709f9d6310ee0c6/include/net/if.h#L299
> Making it 64-bit would break that union.
>
>
but lifr_ifru doesn't require its member size to be less than 64-bit, why
not align can_ioctl_data_s with Linux definition.
Hence back then it was chosen to use kHz instead.
>
> Yours sincerely,
>
> Peter van der Perk
>
> -----Original Message-----
> From: Carlos Sanchez <ca...@geotab.com.INVALID>
> Sent: Wednesday, April 16, 2025 9:37 AM
> To: dev@nuttx.apache.org
> Subject: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...)
> brings the interface up)
>
>
> > > I was going to propose another global CAN change, removing the 1000
> > > factor from bitrate thus making bitrate calls use units in Hz
> > > instead of kHz, so we can discuss that and (if agreed) I make the
> > > change on the same PR to avoid creating so a tiny one.
>
> > What's the unit used on Linux? we need to align the interface with Linux.
>
> Units in linux are Hz, according to
>
> https://docs.kernel.org/networking/can.html#the-can-network-device-driver-interface
>
> One problem with using kHz is Single Wire Can (SAE J2411) uses bitrates of
> 33333 bps (83333 bps in high speed mode) so representing this in kbps
> causes a timing error which can be significant in some cases.
>
> The change would require modifying arbi_bitrate and data_bitrate fields on
> struct can_ioctl_data_s from uint16_t to uint32_t, otherwise usual bitrates
> cannot be represented.
>
> Carlos
>
Re: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up)
Posted by Carlos Sanchez <ca...@geotab.com.INVALID>.
Hi Peter,
> can_ioctl_data_s is in a 32-bit union.
> https://github.com/apache/nuttx/blob/40c6af6dec0d769ac57f69e89709f9d6310ee0c6/include/net/if.h#L299
> Making it 64-bit would break that union.
Maybe I am missing something. AFAIU can_ioctl_data_s is already 64-bit
(it includes 4 uint16_t members). Other members of the ifr_ifru union
are 32-bit (ifru_flags, for example) and some others are 64-bit (most
ioctl structures), some depend on the arch (ifru_data, which is a
pointer) and in fact struct sockaddr is even bigger (sa_data itself is
14-byte already). So I do not see a problem in increasing
can_ioctl_data_s to 16+32+16+32 = 96 bits.
BR
Carlos
RE: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up)
Posted by Peter van der Perk <pe...@nxp.com>.
can_ioctl_data_s is in a 32-bit union.
https://github.com/apache/nuttx/blob/40c6af6dec0d769ac57f69e89709f9d6310ee0c6/include/net/if.h#L299
Making it 64-bit would break that union.
Hence back then it was chosen to use kHz instead.
Yours sincerely,
Peter van der Perk
-----Original Message-----
From: Carlos Sanchez <ca...@geotab.com.INVALID>
Sent: Wednesday, April 16, 2025 9:37 AM
To: dev@nuttx.apache.org
Subject: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up)
> > I was going to propose another global CAN change, removing the 1000
> > factor from bitrate thus making bitrate calls use units in Hz
> > instead of kHz, so we can discuss that and (if agreed) I make the
> > change on the same PR to avoid creating so a tiny one.
> What's the unit used on Linux? we need to align the interface with Linux.
Units in linux are Hz, according to
https://docs.kernel.org/networking/can.html#the-can-network-device-driver-interface
One problem with using kHz is Single Wire Can (SAE J2411) uses bitrates of
33333 bps (83333 bps in high speed mode) so representing this in kbps causes a timing error which can be significant in some cases.
The change would require modifying arbi_bitrate and data_bitrate fields on struct can_ioctl_data_s from uint16_t to uint32_t, otherwise usual bitrates cannot be represented.
Carlos
Re: CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up)
Posted by Xiang Xiao <xi...@gmail.com>.
On Wed, Apr 16, 2025 at 3:37 PM Carlos Sanchez
<ca...@geotab.com.invalid> wrote:
> > > I was going to propose another global CAN change, removing the 1000
> > > factor from bitrate thus making bitrate calls use units in Hz instead
> > > of kHz, so we can discuss that and (if agreed) I make the change on
> > > the same PR to avoid creating so a tiny one.
>
> > What's the unit used on Linux? we need to align the interface with Linux.
>
> Units in linux are Hz, according to
>
> https://docs.kernel.org/networking/can.html#the-can-network-device-driver-interface
>
>
If so, the change is good and necessary, it's always good to keep our
public interface definition same as other popular OS.
CAN ioctl units (WAS: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up)
Posted by Carlos Sanchez <ca...@geotab.com.INVALID>.
> > I was going to propose another global CAN change, removing the 1000
> > factor from bitrate thus making bitrate calls use units in Hz instead
> > of kHz, so we can discuss that and (if agreed) I make the change on
> > the same PR to avoid creating so a tiny one.
> What's the unit used on Linux? we need to align the interface with Linux.
Units in linux are Hz, according to
https://docs.kernel.org/networking/can.html#the-can-network-device-driver-interface
One problem with using kHz is Single Wire Can (SAE J2411) uses bitrates of
33333 bps (83333 bps in high speed mode) so representing this in kbps
causes a timing error which can be significant in some cases.
The change would require modifying arbi_bitrate and data_bitrate fields on
struct can_ioctl_data_s from uint16_t to uint32_t, otherwise usual bitrates
cannot be represented.
Carlos
Re: [EXT] socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up
Posted by Xiang Xiao <xi...@gmail.com>.
On Wed, Apr 16, 2025 at 1:25 AM Carlos Sanchez
<ca...@geotab.com.invalid> wrote:
> Related PR on the apps side, to make slcan work after the "bitrate no
> longer brings interface up" change:
> https://github.com/apache/nuttx-apps/pull/3059
>
> While testing this, I think I have discovered a small mistake on my
> previous, Nuttx-side PR, which slipped by me and by revision: "ret"
> might be used uninitialized now because I removed the assignment on
> stm32_fdcan_sock:1976
>
>
> https://github.com/apache/nuttx/pull/16199/files#diff-8efec4e19b7401adcc3961c078b98ab945ebcada8635105fb591d16cbd6e5210L1976
>
> Should I make a mini PR for this?
>
> I was going to propose another global CAN change, removing the 1000
> factor from bitrate thus making bitrate calls use units in Hz instead
> of kHz, so we can discuss that and (if agreed) I make the change on
> the same PR to avoid creating so a tiny one.
>
What's the unit used on Linux? we need to align the interface with Linux.
>
> Thoughts?
>
> Carlos
>
Re: [EXT] socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up
Posted by Carlos Sanchez <ca...@geotab.com.INVALID>.
Related PR on the apps side, to make slcan work after the "bitrate no
longer brings interface up" change:
https://github.com/apache/nuttx-apps/pull/3059
While testing this, I think I have discovered a small mistake on my
previous, Nuttx-side PR, which slipped by me and by revision: "ret"
might be used uninitialized now because I removed the assignment on
stm32_fdcan_sock:1976
https://github.com/apache/nuttx/pull/16199/files#diff-8efec4e19b7401adcc3961c078b98ab945ebcada8635105fb591d16cbd6e5210L1976
Should I make a mini PR for this?
I was going to propose another global CAN change, removing the 1000
factor from bitrate thus making bitrate calls use units in Hz instead
of kHz, so we can discuss that and (if agreed) I make the change on
the same PR to avoid creating so a tiny one.
Thoughts?
Carlos
Re: [EXT] socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up
Posted by Carlos Sanchez <ca...@geotab.com.INVALID>.
Hi Peter, that is the same flow I was thinking of.
I will create a PR affecting all the existing drivers.
Thanks,
Carlos
On Fri, Apr 11, 2025 at 1:42 PM Peter van der Perk
<pe...@nxp.com> wrote:
>
> Hi Carlos,
>
> I think your concern is valid and that the current implementation of SIOCSCANBITRATE is a bugged.
>
> I think the solution would be to only allow SIOCSCANBITRATE when the interface is down.
>
> My proposal would be that SIOCSCANBITRATE checks if the interface is already up and if that's the case it should return -EBUSY.
> Then the automatic ifup action that after SIOCSCANBITRATE that currently happens should be removed.
>
> The correct procedure to change bitrate would be then
> 1. ifdown the interface is it's up
> 2. Call SIOCSCANBITRATE
> 3. ifup the interface.
>
> Yours sincerely,
>
> Peter van der Perk
>
> -----Original Message-----
> From: Carlos Sanchez <ca...@geotab.com.INVALID>
> Sent: Friday, April 11, 2025 12:16 PM
> To: dev@nuttx.apache.org
> Subject: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up
>
>
> Hi devs,
>
> Short history: in all the existing socketcan drivers in Nuttx (imxrt, s32k3xx, s32k1xx, stm32h7 and kinetis), the driver-specific implementation of ioctl(...SIOCSCANBITRATE...) calls ifup at the end.
> Is there a rationale / spec for this?
>
> Long history:
>
> In Linux, CAN bitrate is set with netlink, not ioctl, and you need to set a bitrate before bringing the interface up (make sense). In Nuttx, you can begin the interface up (it uses a default bitrate) and then you can change it. My guess is calling ifup at the end is precisely to avoid requiring another ifdown/ifup cycle from app code.
>
> SocketCAN is a Linux thing, so we should try to mimic the behaviour.
> Implementing netlink would be overkill (and moreover, it is common in Linux to use libsocketcan to abstract the netlink internals anyways) but at least we should have similar semantics. In our use case, we have a portable Linux/Nuttx app which uses socketcan, we have a thin compatibility layer for Linux/Nuttx differences (so bitrate setting uses icotl on Nuttx and libsocketcan in Linux) but it is difficult to make such a thin layer accommodate this difference (it would need to store the setting, and only call the ioctl just before bringing the interface up... essentially duplicating the driver functionality).
>
> Couldn't we (Nuttx):
> 1) Make SIOCSCANBITRATE just set the baudrate (and avoid changing interface state)? It could even fail if the interface is already up (AFAIK this is what netlink would do).
> 2) Provide a slightly more detailed documentation of Nuttx-specific SocketCAN ioctls (like, specifying whether the interface state is changed under the hood)
>
> I volunteer to make the change, but this might affect application space so I'd like to know what others' thoughts are.
>
> BR
>
> Carlos
--
Carlos Sanchez (he, him, his)
Geotab
Senior Team Lead, Embedded Development | Europe
Visit
www.geotab.com
Twitter | Facebook | YouTube | LinkedIn
RE: [EXT] socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up
Posted by Peter van der Perk <pe...@nxp.com>.
Hi Carlos,
I think your concern is valid and that the current implementation of SIOCSCANBITRATE is a bugged.
I think the solution would be to only allow SIOCSCANBITRATE when the interface is down.
My proposal would be that SIOCSCANBITRATE checks if the interface is already up and if that's the case it should return -EBUSY.
Then the automatic ifup action that after SIOCSCANBITRATE that currently happens should be removed.
The correct procedure to change bitrate would be then
1. ifdown the interface is it's up
2. Call SIOCSCANBITRATE
3. ifup the interface.
Yours sincerely,
Peter van der Perk
-----Original Message-----
From: Carlos Sanchez <ca...@geotab.com.INVALID>
Sent: Friday, April 11, 2025 12:16 PM
To: dev@nuttx.apache.org
Subject: socketcan ioctl(...SIOCSCANBITRATE...) brings the interface up
Hi devs,
Short history: in all the existing socketcan drivers in Nuttx (imxrt, s32k3xx, s32k1xx, stm32h7 and kinetis), the driver-specific implementation of ioctl(...SIOCSCANBITRATE...) calls ifup at the end.
Is there a rationale / spec for this?
Long history:
In Linux, CAN bitrate is set with netlink, not ioctl, and you need to set a bitrate before bringing the interface up (make sense). In Nuttx, you can begin the interface up (it uses a default bitrate) and then you can change it. My guess is calling ifup at the end is precisely to avoid requiring another ifdown/ifup cycle from app code.
SocketCAN is a Linux thing, so we should try to mimic the behaviour.
Implementing netlink would be overkill (and moreover, it is common in Linux to use libsocketcan to abstract the netlink internals anyways) but at least we should have similar semantics. In our use case, we have a portable Linux/Nuttx app which uses socketcan, we have a thin compatibility layer for Linux/Nuttx differences (so bitrate setting uses icotl on Nuttx and libsocketcan in Linux) but it is difficult to make such a thin layer accommodate this difference (it would need to store the setting, and only call the ioctl just before bringing the interface up... essentially duplicating the driver functionality).
Couldn't we (Nuttx):
1) Make SIOCSCANBITRATE just set the baudrate (and avoid changing interface state)? It could even fail if the interface is already up (AFAIK this is what netlink would do).
2) Provide a slightly more detailed documentation of Nuttx-specific SocketCAN ioctls (like, specifying whether the interface state is changed under the hood)
I volunteer to make the change, but this might affect application space so I'd like to know what others' thoughts are.
BR
Carlos