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/08/24 23:57:58 UTC
[GitHub] [incubator-nuttx] fjpanag opened a new pull request, #6918: Added lock in ifr ioctl calls.
fjpanag opened a new pull request, #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918
## Summary
Network device ioctl calls were not protected against concurrent access.
If two different threads try such a call simultaneously, the operation was corrupted.
In my case, this was especially true for MIIM operations on the Ethernet PHY. But I think that I can see other calls that may fail similarly.
This PR locks the network when such an ioctl operation is performed, so they are thread-safe now.
## Impact
Network device ioctl calls are now thread-safe.
No functional change is expected by this PR.
## Testing
I am using a custom target for testing, based on the STM32F427 MCU.
I can confirm that the network operates normally after the change.
By stepping through with my debugger, I can see that the lock is working correctly and no side-effects were observed.
However, please do review this commit carefully!
Please comment if you know that this change may/may not cause a dead-lock or any other problem.
--
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 commented on pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918#issuecomment-1227173853
@pkarashchenko this patch will hit dead lock with bcm43xx driver. @fjpanag I remember you also use bcm43xx driver? Do you try ifdown wlan0 after connecting ap?
--
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] fjpanag commented on pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918#issuecomment-1227298748
@xiaoxiang781216 no, I only use the STM32F4 and LPC1769 Ethernet interfaces.
But for the moment I can only test on an STM32F4 board.
--
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 commented on pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918#issuecomment-1230059564
> @xiaoxiang781216 Have you been able to identify the cause of the dead-lock?
>
> Is it device-specific, or is there any actual problem with this patch?
Yes, the ifdown thread wait the work thread to finish the shutdown, but the work thread try to hold the net lock, and then the dead lock happen. The fix is switch to net_lockedwait.
--
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 pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918#issuecomment-1227175017
@xiaoxiang781216 should we revert for now?
--
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 commented on pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918#issuecomment-1227182317
> @xiaoxiang781216 should we revert for now? Sorry, I saw approve from you and misinterpreted the comment
We are identifying the root cause, so let's hold the revert until we confirm the root cause.
--
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 merged pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918
--
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] fjpanag commented on pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918#issuecomment-1230011516
@xiaoxiang781216 Have you been able to identify the cause of the dead-lock?
Is it device-specific, or is there any actual problem with this patch?
--
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] fjpanag commented on pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918#issuecomment-1227024379
> @zhhyu7 find that this patch introduce a dead lock with our internal test.
@xiaoxiang781216 @zhhyu7 Can you please share the test routine or describe the dead-lock condition?
--
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 commented on pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918#issuecomment-1226687472
yes, we need more test to see whether the dead lock will happen or not.
--
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 commented on pull request #6918: Added lock in ifr ioctl calls.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6918:
URL: https://github.com/apache/incubator-nuttx/pull/6918#issuecomment-1227008681
@zhhyu7 find that this patch introduce a dead lock with our internal 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