Re: [PR] net/netdev: Change SIOCSCANBITRATE to require interface down. [nuttx]
xiaoxiang781216 merged PR #16199: URL: https://github.com/apache/nuttx/pull/16199 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] net/netdev: Change SIOCSCANBITRATE to require interface down. [nuttx]
raiden00pl commented on code in PR #16199:
URL: https://github.com/apache/nuttx/pull/16199#discussion_r2041855826
##
net/netdev/netdev_ioctl.c:
##
@@ -1176,8 +1176,17 @@ static int netdev_ifr_ioctl(FAR struct socket *psock,
int cmd,
#endif
#if defined(CONFIG_NETDEV_IOCTL) && defined(CONFIG_NETDEV_CAN_BITRATE_IOCTL)
- case SIOCGCANBITRATE: /* Get bitrate from a CAN controller */
case SIOCSCANBITRATE: /* Set bitrate of a CAN controller */
+if (dev->d_flags & IFF_UP)
+ {
+/* Cannot set bitrate if the interface is up.
+ If down, fall-through to common code in SIOCGCANBITRATE. */
Review Comment:
```suggestion
/* Cannot set bitrate if the interface is up.
* If down, fall-through to common code in SIOCGCANBITRATE.
*/
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] net/netdev: Change SIOCSCANBITRATE to require interface down. [nuttx]
nuttxpr commented on PR #16199: URL: https://github.com/apache/nuttx/pull/16199#issuecomment-2801144751 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although some sections could be expanded for better clarity. **Strengths:** * **Clear problem description:** The summary clearly explains the difference between Linux SocketCAN and NuttX's current behavior regarding bitrate setting. * **Rationale for the change:** The rationale for mimicking Linux behavior is well-justified. * **Impact description:** The impact on users (requiring `ifdown`/`ifup` cycle) and drivers is stated. * **Testing mention:** Testing is mentioned, including the platform used. * **Cross-referencing:** Mention of a related PR in `apps/canutils/slcan` demonstrates consideration for the wider impact. **Areas for improvement:** * **Conciseness in Summary:** While the summary is clear, it could be slightly more concise by focusing directly on *what* the change does (preventing bitrate changes on an up interface) before explaining *why*. * **Missing details in Impact:** * The impact on the build process, hardware, documentation, security, and compatibility are all marked as "NO" without explanation. Even if there is truly no impact, explicitly stating "NO - No changes to the build process" adds clarity. This prevents reviewers from wondering if these aspects were simply overlooked. * **Insufficient Testing Information:** While testing is mentioned, it's lacking detail. The provided information doesn't show *how* the change was validated. The "testing logs before/after" sections are empty. Include specific commands used and the expected/observed output. For example: * **Before:** `ifconfig can0 up; ip link set can0 type can bitrate 50` (expected: bitrate change succeeds) * **After:** `ifconfig can0 up; ip link set can0 type can bitrate 50` (expected: operation fails with an error message) **Recommendation:** While the PR seems generally well-formed, adding the missing details in the Impact and Testing sections will significantly strengthen it and make the review process smoother. Provide concrete examples of commands used for testing and the expected outcomes, showcasing how the change functions and demonstrating its effectiveness. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
[PR] net/netdev: Change SIOCSCANBITRATE to require interface down. [nuttx]
csanchezdll opened a new pull request, #16199: URL: https://github.com/apache/nuttx/pull/16199 ## Summary 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 ## Impact The change makes changing bitrate on an up interface fails. This is done on the portable, architecture-independent section of ioctl handling, so it affects all the drivers and makes the semantics clear. Al existing SocketCAN drivers have also been updated. Now the process for setting bitrate on an open interface is: 1. ifdown the interface is it's up 2. Call SIOCSCANBITRATE 3. ifup the interface. This requires an update in apps/canutils/slcan. PR following. ## Testing The change was tested on a custom platform using s32k148 uC, confirming changing the bitrate no longer brings the interface up. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
