[PATCH v4 3/7] staging: vchiq_core: Factor out bulk transfer for blocking mode

[Umang Jain]

Hi Stefan

On 07/09/24 5:19 pm, Stefan Wahren wrote:
> Hi Umang,
>
> Am 06.09.24 um 09:25 schrieb Umang Jain:
>> Factor out bulk transfer for blocking mode into a separate dedicated
>> function bulk_xfer_blocking_interruptible(). It is suffixed by
>> "_interruptible" to denote that it can be interrupted and -EAGAIN
>> can be returned. It would be up to the users of the function to retry
>> the call in those cases.
>>
>> Adjust the calls to vchiq-dev.c ioctl interface and vchiq_arm.c
>> for blocking bulk transfers.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> i applied this series on top of recent linux-next and the vchiq ping
> test on Raspberry Pi 3 B Plus (multi_v7_defconfig) crashes or hanges
> strangly.

This is really annoying. The behavior is non-deterministic

I tested this repeatedly and I see the hang on my 3rd ping test.
The first 2 ping tests completed successfully.

Here is the output for all 3 tries:

https://paste.debian.net/plain/1328739

I'll take a look and fix it in next version.

>
> I bisected the issue to this patch, but it's possible the root cause
> might be introduced before.
>
> But i'm pretty sure that the series introduced the regression.
>
> Sorry, i don't have the time analyse this issue deeper.
>
> Best regards
>> ---
>>   .../interface/vchiq_arm/vchiq_arm.c           |  5 +--
>>   .../interface/vchiq_arm/vchiq_core.c          | 42 ++++++++++++++++---
>>   .../interface/vchiq_arm/vchiq_core.h          |  5 +++
>>   .../interface/vchiq_arm/vchiq_dev.c           |  6 +++
>>   4 files changed, 49 insertions(+), 9 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 c4d97dbf6ba8..688c9b1be868 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -968,9 +968,8 @@ vchiq_blocking_bulk_transfer(struct 
>> vchiq_instance *instance, unsigned int handl
>>               return -ENOMEM;
>>       }
>>
>> -    ret = vchiq_bulk_transfer(instance, handle, data, NULL, size,
>> -                  &waiter->bulk_waiter,
>> -                  VCHIQ_BULK_MODE_BLOCKING, dir);
>> +    ret = vchiq_bulk_xfer_blocking_interruptible(instance, handle, 
>> data, NULL, size,
>> +                             &waiter->bulk_waiter, dir);
>>       if ((ret != -EAGAIN) || fatal_signal_pending(current) || 
>> !waiter->bulk_waiter.bulk) {
>>           struct vchiq_bulk *bulk = waiter->bulk_waiter.bulk;
>>
>> diff --git 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> index f36044bab194..43f951fa4b89 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -2985,6 +2985,42 @@ vchiq_remove_service(struct vchiq_instance 
>> *instance, unsigned int handle)
>>       return status;
>>   }
>>
>> +int
>> +vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance 
>> *instance, unsigned int handle,
>> +                       void *offset, void __user *uoffset, int size,
>> +                       void __user *userdata, enum vchiq_bulk_dir dir)
>> +{
>> +    struct vchiq_service *service = find_service_by_handle(instance, 
>> handle);
>> +    struct bulk_waiter *bulk_waiter;
>> +    enum vchiq_bulk_mode mode = VCHIQ_BULK_MODE_BLOCKING;
>> +    int status = -EINVAL;
>> +
>> +    if (!service)
>> +        return -EINVAL;
>> +
>> +    if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
>> +        goto error_exit;
>> +
>> +    if (!offset && !uoffset)
>> +        goto error_exit;
>> +
>> +    if (vchiq_check_service(service))
>> +        goto error_exit;
>> +
>> +    bulk_waiter = userdata;
>> +    init_completion(&bulk_waiter->event);
>> +    bulk_waiter->actual = 0;
>> +    bulk_waiter->bulk = NULL;
>> +
>> +    status = vchiq_bulk_xfer_queue_msg_interruptible(service, 
>> offset, uoffset, size,
>> +                             userdata, mode, dir);
>> +
>> +error_exit:
>> +    vchiq_service_put(service);
>> +
>> +    return status;
>> +}
>> +
>>   /*
>>    * This function may be called by kernel threads or user threads.
>>    * User threads may receive -EAGAIN to indicate that a signal has been
>> @@ -3018,12 +3054,6 @@ int vchiq_bulk_transfer(struct vchiq_instance 
>> *instance, unsigned int handle,
>>       case VCHIQ_BULK_MODE_NOCALLBACK:
>>       case VCHIQ_BULK_MODE_CALLBACK:
>>           break;
>> -    case VCHIQ_BULK_MODE_BLOCKING:
>> -        bulk_waiter = userdata;
>> -        init_completion(&bulk_waiter->event);
>> -        bulk_waiter->actual = 0;
>> -        bulk_waiter->bulk = NULL;
>> -        break;
>>       default:
>>           goto error_exit;
>>       }
>> diff --git 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> index 985d9ea3a06a..2dd89101c1c6 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> @@ -474,6 +474,11 @@ extern int
>>   vchiq_bulk_xfer_waiting_interruptible(struct vchiq_instance *instance,
>>                         unsigned int handle, struct bulk_waiter 
>> *userdata);
>>
>> +extern int
>> +vchiq_bulk_xfer_blocking_interruptible(struct vchiq_instance 
>> *instance, unsigned int handle,
>> +                       void *offset, void __user *uoffset, int size,
>> +                       void __user *userdata, enum vchiq_bulk_dir dir);
>> +
>>   extern int
>>   vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int 
>> handle, void *offset,
>>               void __user *uoffset, int size, void *userdata, enum 
>> vchiq_bulk_mode mode,
>> diff --git 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>> index 550838d2863b..830633f2326b 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>> @@ -304,6 +304,12 @@ static int vchiq_irq_queue_bulk_tx_rx(struct 
>> vchiq_instance *instance,
>>           }
>>
>>           userdata = &waiter->bulk_waiter;
>> +
>> +        status = vchiq_bulk_xfer_blocking_interruptible(instance, 
>> args->handle,
>> +                                NULL, args->data, args->size,
>> +                                userdata, dir);
>> +
>> +        goto bulk_transfer_handled;
>>       } else if (args->mode == VCHIQ_BULK_MODE_WAITING) {
>>           mutex_lock(&instance->bulk_waiter_list_mutex);
>>           list_for_each_entry(iter, &instance->bulk_waiter_list,
>