Re: [PATCH v2 2/3] staging: vchiq: revert "switch to wait_for_completion_killable"
On Mon, May 06, 2019 at 04:40:29PM +0200, Nicolas Saenz Julienne wrote:
> The killable version of wait_for_completion() is meant to be used on
> situations where it should not fail at all costs, but still have the
> convenience of being able to kill it if really necessary. VCHIQ doesn't
> fit this criteria, as it's mainly used as an interface to V4L2 and ALSA
> devices.
>
> Fixes: a772f116702e ("staging: vchiq: switch to wait_for_completion_killable")
> Signed-off-by: Nicolas Saenz Julienne
>
> This reverts commit a772f116702e3f0afdd7e6acadc1b8fb3b20b9ff.
> ---
Git just sets you up for failure with its revert. That code was from
when git was really new and now everyone gets annoyed when they see a
raw git hash without a human readable subject. Just say at the start of
the commit message:
This reverts commit a772f116702e ("staging: vchiq: switch to
wait_for_completion_killable").
The killable version of wait_for_completion() is meant to be used on
situations where it should not fail at all costs, but still have the
convenience of being able to kill it if really necessary. VCHIQ doesn't
fit this criteria, as it's mainly used as an interface to V4L2 and ALSA
devices.
Fixes: a772f116702e ("staging: vchiq: switch to wait_for_completion_killable")
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/3] staging: vchiq: revert "switch to wait_for_completion_killable"
Hi Nicolas,
Am 06.05.19 um 17:59 schrieb Nicolas Saenz Julienne:
> Hi Dan, thanks for reviewing.
>
> On Mon, 2019-05-06 at 18:20 +0300, Dan Carpenter wrote:
>> On Mon, May 06, 2019 at 04:40:29PM +0200, Nicolas Saenz Julienne wrote:
>>> @@ -1740,7 +1740,8 @@ parse_rx_slots(struct vchiq_state *state)
>>> >bulk_rx : >bulk_tx;
>>>
>>> DEBUG_TRACE(PARSE_LINE);
>>> - if (mutex_lock_killable(>bulk_mutex)) {
>>> + if (mutex_lock_killable(
>>> + >bulk_mutex) != 0) {
>> This series does't add != 0 consistently... Personally, I would prefer
>> we just leave it out. I use != 0 for two things. 1) When I'm talking
>> about the number zero.
>>
>> if (len == 0) {
>>
>> Or with strcmp():
>>
>> if (strcmp(a, b) == 0) { // a equals b
>> if (strcmp(a, b) < 0) { // a less than b.
>>
>> But here zero means no errors, so I would just leave it out...
> I agree, I'll fix it.
i also agree with Dan, but this specific patch should revert the changes
of a772f116702e3f0afdd7e6acadc1b8fb3b20b9ff . So mentioned style issue
should be fixed in a separate patch.
Regards Stefan
>
> Regards,
> Nicolas
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/3] staging: vchiq: revert "switch to wait_for_completion_killable"
Hi Dan, thanks for reviewing.
On Mon, 2019-05-06 at 18:20 +0300, Dan Carpenter wrote:
> On Mon, May 06, 2019 at 04:40:29PM +0200, Nicolas Saenz Julienne wrote:
> > @@ -1740,7 +1740,8 @@ parse_rx_slots(struct vchiq_state *state)
> > >bulk_rx : >bulk_tx;
> >
> > DEBUG_TRACE(PARSE_LINE);
> > - if (mutex_lock_killable(>bulk_mutex)) {
> > + if (mutex_lock_killable(
> > + >bulk_mutex) != 0) {
>
> This series does't add != 0 consistently... Personally, I would prefer
> we just leave it out. I use != 0 for two things. 1) When I'm talking
> about the number zero.
>
> if (len == 0) {
>
> Or with strcmp():
>
> if (strcmp(a, b) == 0) { // a equals b
> if (strcmp(a, b) < 0) { // a less than b.
>
> But here zero means no errors, so I would just leave it out...
I agree, I'll fix it.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/3] staging: vchiq: revert "switch to wait_for_completion_killable"
On Mon, May 06, 2019 at 04:40:29PM +0200, Nicolas Saenz Julienne wrote:
> @@ -1740,7 +1740,8 @@ parse_rx_slots(struct vchiq_state *state)
> >bulk_rx : >bulk_tx;
>
> DEBUG_TRACE(PARSE_LINE);
> - if (mutex_lock_killable(>bulk_mutex)) {
> + if (mutex_lock_killable(
> + >bulk_mutex) != 0) {
This series does't add != 0 consistently... Personally, I would prefer
we just leave it out. I use != 0 for two things. 1) When I'm talking
about the number zero.
if (len == 0) {
Or with strcmp():
if (strcmp(a, b) == 0) { // a equals b
if (strcmp(a, b) < 0) { // a less than b.
But here zero means no errors, so I would just leave it out...
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/3] staging: vchiq: revert "switch to wait_for_completion_killable"
The killable version of wait_for_completion() is meant to be used on
situations where it should not fail at all costs, but still have the
convenience of being able to kill it if really necessary. VCHIQ doesn't
fit this criteria, as it's mainly used as an interface to V4L2 and ALSA
devices.
Fixes: a772f116702e ("staging: vchiq: switch to wait_for_completion_killable")
Signed-off-by: Nicolas Saenz Julienne
This reverts commit a772f116702e3f0afdd7e6acadc1b8fb3b20b9ff.
---
.../interface/vchiq_arm/vchiq_arm.c | 21 ++-
.../interface/vchiq_arm/vchiq_core.c | 21 ++-
.../interface/vchiq_arm/vchiq_util.c | 6 +++---
3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 064d0db4c51e..ccfb8218b83c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -560,7 +560,8 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T
reason,
vchiq_log_trace(vchiq_arm_log_level,
"%s - completion queue full", __func__);
DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
- if (wait_for_completion_killable( >remove_event)) {
+ if (wait_for_completion_interruptible(
+ >remove_event)) {
vchiq_log_info(vchiq_arm_log_level,
"service_callback interrupted");
return VCHIQ_RETRY;
@@ -671,7 +672,7 @@ service_callback(VCHIQ_REASON_T reason, struct vchiq_header
*header,
}
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
- if (wait_for_completion_killable(
+ if (wait_for_completion_interruptible(
_service->remove_event)
!= 0) {
vchiq_log_info(vchiq_arm_log_level,
@@ -1006,7 +1007,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
has been closed until the client library calls the
CLOSE_DELIVERED ioctl, signalling close_event. */
if (user_service->close_pending &&
- wait_for_completion_killable(
+ wait_for_completion_interruptible(
_service->close_event))
status = VCHIQ_RETRY;
break;
@@ -1182,7 +1183,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
DEBUG_TRACE(AWAIT_COMPLETION_LINE);
mutex_unlock(>completion_mutex);
- rc = wait_for_completion_killable(
+ rc = wait_for_completion_interruptible(
>insert_event);
mutex_lock(>completion_mutex);
if (rc != 0) {
@@ -1352,7 +1353,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
do {
spin_unlock(_queue_spinlock);
DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
- if (wait_for_completion_killable(
+ if (wait_for_completion_interruptible(
_service->insert_event)) {
vchiq_log_info(vchiq_arm_log_level,
"DEQUEUE_MESSAGE interrupted");
@@ -2360,7 +2361,7 @@ vchiq_keepalive_thread_func(void *v)
while (1) {
long rc = 0, uc = 0;
- if (wait_for_completion_killable(_state->ka_evt)
+ if (wait_for_completion_interruptible(_state->ka_evt)
!= 0) {
vchiq_log_error(vchiq_susp_log_level,
"%s interrupted", __func__);
@@ -2611,7 +2612,7 @@ block_resume(struct vchiq_arm_state *arm_state)
write_unlock_bh(_state->susp_res_lock);
vchiq_log_info(vchiq_susp_log_level, "%s wait for previously "
"blocked clients", __func__);
- if (wait_for_completion_killable_timeout(
+ if (wait_for_completion_interruptible_timeout(
_state->blocked_blocker, timeout_val)
<= 0) {
vchiq_log_error(vchiq_susp_log_level, "%s wait for "
@@ -2637,7 +2638,7 @@ block_resume(struct vchiq_arm_state *arm_state)
write_unlock_bh(_state->susp_res_lock);
vchiq_log_info(vchiq_susp_log_level, "%s wait for resume",
__func__);
-
