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 2020/07/05 17:59:49 UTC
[GitHub] [incubator-nuttx] lukegluke opened a new pull request #1371: drivers/can: correct checking sem is locked
lukegluke opened a new pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371
Object filled by nxsem_get_value can be zero or a negative number
fix for 190782c2959de2b28867022941e52f429d5d07fb
issue #1354
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653927706
As for me, I mean this particular case in can driver.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke commented on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke commented on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653927706
> As I said in my response, it depends on what you are trying to determine. There is no one single answer.
Yes, I understand, sorry for put it wrong. I mean this particular case in can driver.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653921030
> could you please explain, why in some other places `sval < 0` is used?
Semaphore counts can be interrupreted this way:
- nsem > 0 : There are nsem counts remaining on the semaphore; no tasks are waiting for a count
- nsem == 0: There are no remaining counts on the semaphore but no task are waiting for a count
- nsem < 0: There are not remaining counts on the semaphore and -nsem tasks are waiting for a count
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke commented on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke commented on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653920251
Alan, could you please explain, why in other places `sval < 0` is used?
https://github.com/apache/incubator-nuttx/blob/57bc329aace6d816b9a51a368665ea4c7116a9c6/sched/pthread/pthread_condbroadcast.c#L99
https://github.com/apache/incubator-nuttx/blob/57bc329aace6d816b9a51a368665ea4c7116a9c6/drivers/pipes/pipe_common.c#L223
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653921030
> could you please explain, why in some other places `sval < 0` is used?
Semaphore counts can be interpreted this way:
- nsem > 0 : There are nsem counts remaining on the semaphore; no tasks are waiting for a count
- nsem == 0: There are no remaining counts on the semaphore but no task is waiting for a count
- nsem < 0: There are no remaining counts on the semaphore and -nsem tasks are waiting for a count
So nsem < 0 is a test to see if there are any tasks waiting for a semaphore count. nsem < = 0 tests if there are any counts remaining of the semaphore.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653922957
Yes, it could return 0 or negative. But in all the tests the sval is compared to < 0.
/****************************************************************************
* Name: nxsem_get_value
*
* Description:
* This function updates the location referenced by 'sval' argument to
* have the value of the semaphore referenced by 'sem' without effecting
* the state of the semaphore. The updated value represents the actual
* semaphore value that occurred at some unspecified time during the call,
* but may not reflect the actual value of the semaphore when it is
* returned to the calling task.
*
* If 'sem' is locked, the value return by nxsem_get_value() will either be
* zero or a negative number whose absolute value represents the number
* of tasks waiting for the semaphore.
*
* Input Parameters:
* sem - Semaphore descriptor
* sval - Buffer by which the value is returned
*
* Returned Value:
* This is an internal OS interface and should not be used by applications.
* It follows the NuttX internal error return policy: Zero (OK) is
* returned on success. A negated errno value is returned on failure.
*
****************************************************************************/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653927179
Thanks for info, but I more interested should we nxsem_post when nsem == 0?
As I see, in similar situations in other drivers `sval <= 0` is used widely. So it seems to be correctly to apply this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653927862
>
>
> Thanks for info, but I more interested should we nxsem_post when nsem == 0?
> As I see, in similar situations in other drivers `sval <= 0` is used widely. So it seems to be correctly to apply this PR.
Depends on what you are trying do. Is this logic checking to see if there is a task waiting on the semaphore? If so, then the correct test is nsem < 0 because that means that there are tasks waiting for the semaphore. I haven't look at the code, but from the snippet, it looks like nsem < 0. That says, "Is a task waiting for a semaphore notfication? nsem < 0? Yes, then a task is waiting, post the semaphore to wake up one task."
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653920251
@acassis Alan, could you please explain, why in other places `sval < 0` is used?
https://github.com/apache/incubator-nuttx/blob/57bc329aace6d816b9a51a368665ea4c7116a9c6/sched/pthread/pthread_condbroadcast.c#L99
https://github.com/apache/incubator-nuttx/blob/57bc329aace6d816b9a51a368665ea4c7116a9c6/drivers/pipes/pipe_common.c#L223
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke commented on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke commented on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653930666
Oh, initial value - that what I missed, it explains my concerns, thanks!
> The waiting semaphore has an initial count of zero, right?
No, it is 1.
https://github.com/apache/incubator-nuttx/blob/57bc329aace6d816b9a51a368665ea4c7116a9c6/drivers/can/can.c#L391
Same as in several "similar situations in other drivers" I've checked.
But in these cases I mentioned https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653920251 it is zero.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653921030
> could you please explain, why in some other places `sval < 0` is used?
Semaphore counts can be interpreted this way:
- nsem > 0 : There are nsem counts remaining on the semaphore; no tasks are waiting for a count
- nsem == 0: There are no remaining counts on the semaphore but no task is waiting for a count
- nsem < 0: There are no remaining counts on the semaphore and -nsem tasks are waiting for a count
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653920251
@acassis Alan, could you please explain, why in some other places `sval < 0` is used?
https://github.com/apache/incubator-nuttx/blob/57bc329aace6d816b9a51a368665ea4c7116a9c6/sched/pthread/pthread_condbroadcast.c#L99
https://github.com/apache/incubator-nuttx/blob/57bc329aace6d816b9a51a368665ea4c7116a9c6/drivers/pipes/pipe_common.c#L223
https://github.com/apache/incubator-nuttx/blob/c6c0214f9a0e29fe563060069979d0cebf474d95/drivers/video/video.c#L459-L460
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke commented on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke commented on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-654087600
> No, it is 1.
So this PR should be applied.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653920251
@acassis Alan, could you please explain, why in some other places `sval < 0` is used?
https://github.com/apache/incubator-nuttx/blob/57bc329aace6d816b9a51a368665ea4c7116a9c6/sched/pthread/pthread_condbroadcast.c#L99
https://github.com/apache/incubator-nuttx/blob/57bc329aace6d816b9a51a368665ea4c7116a9c6/drivers/pipes/pipe_common.c#L223
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-654087600
> No, it is 1.
So this PR should be applied.
Inside my project, I've tested this bug fix with `sval <= 0` from beginning, do not fall into any issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653927528
> Yes, it could return 0 or negative. But in all the tests the sval is compared to < 0.
As I said in my response, it depends on what you are trying to determine. There is no one single answer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] lukegluke commented on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
lukegluke commented on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653927179
Thanks for info, but I more interested in should we nxsem_post when nsem == 0?
As I see, in similar situations in other drivers `sval <= 0` is used widely. So it seems to be correctly to apply this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653921030
> could you please explain, why in some other places `sval < 0` is used?
Semaphore counts can be interpreted this way:
- nsem > 0 : There are nsem counts remaining on the semaphore; no tasks are waiting for a count
- nsem == 0: There are no remaining counts on the semaphore but no task are waiting for a count
- nsem < 0: There are not remaining counts on the semaphore and -nsem tasks are waiting for a count
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653927862
>
>
> Thanks for info, but I more interested should we nxsem_post when nsem == 0?
> As I see, in similar situations in other drivers `sval <= 0` is used widely. So it seems to be correctly to apply this PR.
Depends on what you are trying do. Is this logic checking to see if there is a task waiting on the semaphore? If so, then the correct test is nsem < 0 because that means that there are tasks waiting for the semaphore. I haven't look at the code, but from the snippet, it looks like nsem < 0. That says, "Is a task waiting for a semaphore notfication? nsem < 0? Yes, then a task is waiting, post the semaphore to wake up one task."
The waiting semaphore has an initial count of zero, right? So when a task waits, it goes to minus one. So posting the semaphore wakes it up.
If yo post the semaphore when nsem <= 0, then the the next time, the task would fail to block and there would be a bug. Is that correct?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1371: drivers/can: correct checking sem is locked
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1371:
URL: https://github.com/apache/incubator-nuttx/pull/1371#issuecomment-653921030
> could you please explain, why in some other places `sval < 0` is used?
Semaphore counts can be interpreted this way:
- nsem > 0 : There are nsem counts remaining on the semaphore; no tasks are waiting for a count
- nsem == 0: There are no remaining counts on the semaphore but no task is waiting for a count
- nsem < 0: There are no remaining counts on the semaphore and -nsem tasks are waiting for a count
So nsem < 0 is a test to see if there are any tasks blocked waiting for a semaphore count. nsem < = 0 tests if there are any counts remaining of the semaphore. nsem <= 0 is also a test to see IF the task would block if it tried to take a semaphore count.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org