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/03/11 15:54:24 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request #5718: drivers/can: optimize can driver reader side
pkarashchenko opened a new pull request #5718:
URL: https://github.com/apache/incubator-nuttx/pull/5718
## Summary
This PR solves two issues:
1. If CAN driver is open two times one time with `O_RDONLY` flag and second time with `O_WRONLY` flag, then receiver side is constantly reporting `CAN_ERROR_INTERNAL` error with `CAN_ERROR5_RXOVERFLOW` error code.
2. The `CAN_ERROR_INTERNAL` error with `CAN_ERROR5_RXOVERFLOW` error code is reported globally across all the opened driver instances. This lead to a situation when CAN driver was open for read and then file descriptor was duplicated due to new task spawn. The new task is actually not a CAN reader, but if the file descriptor is not closed, then the RX overflow error is reported continuously to the actual CAN reader.
## Impact
CAN driver users
## Testing
In progress. Testing with a custom SAME70 based 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] pkarashchenko commented on pull request #5718: drivers/can: optimize can driver reader side
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5718:
URL: https://github.com/apache/incubator-nuttx/pull/5718#issuecomment-1065266369
> I don't have hardware to test with, but the code changes look reasonable to me.
I'm in the middle of the testing. Will need at least a day to verify on HW.
--
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 #5718: drivers/can: optimize can driver reader side
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5718:
URL: https://github.com/apache/incubator-nuttx/pull/5718
--
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 change in pull request #5718: drivers/can: optimize can driver reader side
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5718:
URL: https://github.com/apache/incubator-nuttx/pull/5718#discussion_r824989145
##########
File path: drivers/can/can.c
##########
@@ -407,46 +406,48 @@ static int can_open(FAR struct file *filep)
* for this device, then perform hardware initialization.
*/
- if (list_is_empty(&dev->cd_readers))
+ caninfo("ocount: %u\n", dev->cd_crefs);
+
+ if (dev->cd_crefs >= 255)
{
- caninfo("ocount: %d\n", 0);
+ /* Limit to no more than 255 opens */
+ ret = -EMFILE;
+ goto errout;
+ }
+ else
+ {
flags = enter_critical_section();
- ret = dev_setup(dev);
- if (ret >= 0)
+
+ if (dev->cd_crefs == 0)
{
- /* Mark the FIFOs empty */
+ ret = dev_setup(dev);
+ if (ret == OK)
+ {
+ /* Mark the FIFOs empty */
- dev->cd_xmit.tx_head = 0;
- dev->cd_xmit.tx_queue = 0;
- dev->cd_xmit.tx_tail = 0;
+ dev->cd_xmit.tx_head = 0;
+ dev->cd_xmit.tx_queue = 0;
+ dev->cd_xmit.tx_tail = 0;
- /* Finally, Enable the CAN RX interrupt */
+ /* Finally, Enable the CAN RX interrupt */
- dev_rxint(dev, true);
+ dev_rxint(dev, true);
+ }
}
- list_add_head(&dev->cd_readers,
- (FAR struct list_node *)init_can_reader(filep));
-
- leave_critical_section(flags);
- }
- else
- {
- tmp = list_length(&dev->cd_readers);
- caninfo("ocount: %d\n", tmp);
-
- if (tmp >= 255)
+ if (ret == OK)
Review comment:
yes. this is expected. The reason for this check is not to increment reference counter if `dev_setup()` fails during the first init. I want to avoid a situation when failure is returned, but reference counter is incremented.
--
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] hartmannathan commented on a change in pull request #5718: drivers/can: optimize can driver reader side
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #5718:
URL: https://github.com/apache/incubator-nuttx/pull/5718#discussion_r824860390
##########
File path: drivers/can/can.c
##########
@@ -407,46 +406,48 @@ static int can_open(FAR struct file *filep)
* for this device, then perform hardware initialization.
*/
- if (list_is_empty(&dev->cd_readers))
+ caninfo("ocount: %u\n", dev->cd_crefs);
+
+ if (dev->cd_crefs >= 255)
{
- caninfo("ocount: %d\n", 0);
+ /* Limit to no more than 255 opens */
+ ret = -EMFILE;
+ goto errout;
+ }
+ else
+ {
flags = enter_critical_section();
- ret = dev_setup(dev);
- if (ret >= 0)
+
+ if (dev->cd_crefs == 0)
{
- /* Mark the FIFOs empty */
+ ret = dev_setup(dev);
+ if (ret == OK)
+ {
+ /* Mark the FIFOs empty */
- dev->cd_xmit.tx_head = 0;
- dev->cd_xmit.tx_queue = 0;
- dev->cd_xmit.tx_tail = 0;
+ dev->cd_xmit.tx_head = 0;
+ dev->cd_xmit.tx_queue = 0;
+ dev->cd_xmit.tx_tail = 0;
- /* Finally, Enable the CAN RX interrupt */
+ /* Finally, Enable the CAN RX interrupt */
- dev_rxint(dev, true);
+ dev_rxint(dev, true);
+ }
}
- list_add_head(&dev->cd_readers,
- (FAR struct list_node *)init_can_reader(filep));
-
- leave_critical_section(flags);
- }
- else
- {
- tmp = list_length(&dev->cd_readers);
- caninfo("ocount: %d\n", tmp);
-
- if (tmp >= 255)
+ if (ret == OK)
Review comment:
This ret may be the one from either can_takesem() or dev_setup(). Not saying that's a problem, just saying.
##########
File path: drivers/can/can.c
##########
@@ -407,46 +406,48 @@ static int can_open(FAR struct file *filep)
* for this device, then perform hardware initialization.
*/
- if (list_is_empty(&dev->cd_readers))
+ caninfo("ocount: %u\n", dev->cd_crefs);
+
+ if (dev->cd_crefs >= 255)
{
- caninfo("ocount: %d\n", 0);
+ /* Limit to no more than 255 opens */
Review comment:
LGTM; we were missing this before and could rollover if too many opens.
--
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] hartmannathan commented on pull request #5718: drivers/can: optimize can driver reader side
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #5718:
URL: https://github.com/apache/incubator-nuttx/pull/5718#issuecomment-1065293983
> I'm in the middle of the testing. Will need at least a day to verify on HW.
Ok no problem, no rush. Maybe it is a good idea to mark as draft so someone doesn't merge it prematurely?
--
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 #5718: drivers/can: optimize can driver reader side
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5718:
URL: https://github.com/apache/incubator-nuttx/pull/5718#issuecomment-1068977693
Changes have been verified
--
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 change in pull request #5718: drivers/can: optimize can driver reader side
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5718:
URL: https://github.com/apache/incubator-nuttx/pull/5718#discussion_r824870134
##########
File path: drivers/can/can.c
##########
@@ -407,46 +406,48 @@ static int can_open(FAR struct file *filep)
* for this device, then perform hardware initialization.
*/
- if (list_is_empty(&dev->cd_readers))
+ caninfo("ocount: %u\n", dev->cd_crefs);
+
+ if (dev->cd_crefs >= 255)
{
- caninfo("ocount: %d\n", 0);
+ /* Limit to no more than 255 opens */
Review comment:
It was tracked before my changes, but was relying on the readers list. Now since the readers list is not updated in case of the write only we can't use the readers list any more
--
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] hartmannathan commented on a change in pull request #5718: drivers/can: optimize can driver reader side
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #5718:
URL: https://github.com/apache/incubator-nuttx/pull/5718#discussion_r824893325
##########
File path: drivers/can/can.c
##########
@@ -407,46 +406,48 @@ static int can_open(FAR struct file *filep)
* for this device, then perform hardware initialization.
*/
- if (list_is_empty(&dev->cd_readers))
+ caninfo("ocount: %u\n", dev->cd_crefs);
+
+ if (dev->cd_crefs >= 255)
{
- caninfo("ocount: %d\n", 0);
+ /* Limit to no more than 255 opens */
Review comment:
Ah yes I see that now. Still, this change is an improvement because it is becoming more consistent with the way other drivers are written.
--
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