You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/06/25 05:54:50 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request, #6519: netdev/carrier: check the IF stautus before carrier
anchao opened a new pull request, #6519:
URL: https://github.com/apache/incubator-nuttx/pull/6519
## Summary
netdev/carrier: check the IF stautus before carrier
## Impact
N/A
## Testing
netdev_carrier_on/off test
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6519: netdev/carrier: check the IF stautus before carrier
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6519:
URL: https://github.com/apache/incubator-nuttx/pull/6519#discussion_r906655111
##########
net/netdev/netdev_carrier.c:
##########
@@ -62,8 +62,12 @@ int netdev_carrier_on(FAR struct net_driver_s *dev)
{
if (dev)
{
- dev->d_flags |= IFF_RUNNING;
- netlink_device_notify(dev);
+ if (!IFF_IS_RUNNING(dev->d_flags))
Review Comment:
So we expect `netdev_carrier_on` to be called multiple times in a row? I mean that I do not see any harm in this code, but do not see explicit benefits.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a diff in pull request #6519: netdev/carrier: check the IF stautus before carrier
Posted by GitBox <gi...@apache.org>.
anchao commented on code in PR #6519:
URL: https://github.com/apache/incubator-nuttx/pull/6519#discussion_r906665668
##########
net/netdev/netdev_carrier.c:
##########
@@ -89,13 +93,16 @@ int netdev_carrier_off(FAR struct net_driver_s *dev)
{
if (dev)
{
- dev->d_flags &= ~IFF_RUNNING;
- netlink_device_notify(dev);
+ if (IFF_IS_RUNNING(dev->d_flags))
Review Comment:
Done
##########
net/netdev/netdev_carrier.c:
##########
@@ -62,8 +62,12 @@ int netdev_carrier_on(FAR struct net_driver_s *dev)
{
if (dev)
{
- dev->d_flags |= IFF_RUNNING;
- netlink_device_notify(dev);
+ if (!IFF_IS_RUNNING(dev->d_flags))
Review Comment:
Done
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6519: netdev/carrier: check the IF stautus before carrier
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6519:
URL: https://github.com/apache/incubator-nuttx/pull/6519#discussion_r906665068
##########
net/netdev/netdev_carrier.c:
##########
@@ -62,8 +62,12 @@ int netdev_carrier_on(FAR struct net_driver_s *dev)
{
if (dev)
{
- dev->d_flags |= IFF_RUNNING;
- netlink_device_notify(dev);
+ if (!IFF_IS_RUNNING(dev->d_flags))
Review Comment:
Let's merge two lines into one
```
if (dev && !IFF_IS_RUNNING(dev->d_flags))
```
##########
net/netdev/netdev_carrier.c:
##########
@@ -89,13 +93,16 @@ int netdev_carrier_off(FAR struct net_driver_s *dev)
{
if (dev)
{
- dev->d_flags &= ~IFF_RUNNING;
- netlink_device_notify(dev);
+ if (IFF_IS_RUNNING(dev->d_flags))
Review Comment:
let's merge 2 lines
```
if (dev && IFF_IS_RUNNING(dev->d_flags))
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a diff in pull request #6519: netdev/carrier: check the IF stautus before carrier
Posted by GitBox <gi...@apache.org>.
anchao commented on code in PR #6519:
URL: https://github.com/apache/incubator-nuttx/pull/6519#discussion_r906664524
##########
net/netdev/netdev_carrier.c:
##########
@@ -62,8 +62,12 @@ int netdev_carrier_on(FAR struct net_driver_s *dev)
{
if (dev)
{
- dev->d_flags |= IFF_RUNNING;
- netlink_device_notify(dev);
+ if (!IFF_IS_RUNNING(dev->d_flags))
Review Comment:
Yes, this issue can also be solved on the driver side, but I think if which can be fix inside netdev_carrier_*, it will also bring benefits to other drivers, because it is unnecessary to report more of the same events to netlink
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6519: netdev/carrier: check the IF stautus before carrier
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6519:
URL: https://github.com/apache/incubator-nuttx/pull/6519#discussion_r906664910
##########
net/netdev/netdev_carrier.c:
##########
@@ -62,8 +62,12 @@ int netdev_carrier_on(FAR struct net_driver_s *dev)
{
if (dev)
{
- dev->d_flags |= IFF_RUNNING;
- netlink_device_notify(dev);
+ if (!IFF_IS_RUNNING(dev->d_flags))
Review Comment:
Yes and no :) I mean if 2 out of 100 drivers can call `netdev_carrier_on` in a row, then we add check (extra code) for 98 cases that do not need it. Now if driver call it once then event is reported once to netlink, so no issue.
Anyway this will lead to a code duplication in 2 (or more) cases.
I do not have anything against this change, so we can keep it.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #6519: netdev/carrier: check the IF stautus before carrier
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6519:
URL: https://github.com/apache/incubator-nuttx/pull/6519
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org