On Tue, Jul 30, 2024, at 20:05, Stefan Wahren wrote:
> Hi Umang,
>
> Am 30.07.24 um 19:08 schrieb Umang Jain:
>> In vchiq_dev.c, there are two places where the __user bulk_userdata
>> pointer to set to a kernel-space pointer which then gives relevant
>> Sparse warnings as below:
>>
>> vchiq_dev.c:328:26: warning: incorrect type in assignment (different address spaces)
>> vchiq_dev.c:328:26: expected void *[assigned] userdata
>> vchiq_dev.c:328:26: got void [noderef] __user *userdata
>> vchiq_dev.c:543:47: warning: incorrect type in assignment (different address spaces)
>> vchiq_dev.c:543:47: expected void [noderef] __user *[addressable] [assigned] bulk_userdata
>> vchiq_dev.c:543:47: got void *bulk_userdata
>>
>> This is solved by adding additional functional argument to track the
>> userspace bulk_userdata separately and passing it accordingly to
>> completion handlers.
> IMO this patch fixes the issue for spare, but don't address the
> confusing member naming for humans. It's not clear that "userdata" is a
> kernel pointer and "uuserdata" is a pointer to userspace. It would be
> nice to avoid the word "user" for kernel pointer in this case.
Right, also you need to provide a much better explanation about
how the code is meant to work, and what this opaque pointer
is meant to do.
Ideally this should be cleaned up in a way that completely
avoids passing both user and kernel data at the same time.
A small step would be to separate out the "struct bulk_waiter
*bulk_waiter" argument and make that typesafe.
You can also wrap vchiq_bulk_transfer() in order to have
four separate functions based on the different 'mode'
values and have them only take the arguments they actually
need.
Arnd
