[v5,0/7] staging: vchiq_core: Refactor vchiq_bulk_transfer() logic

Message

Umang Jain Sept. 9, 2024, 6:14 a.m. UTC

Series mostly refactors vchiq_bulk_transfer() code according
to Arnd's review comment from [1]:

> 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.

I didn't wrap vchiq_bulk_transfer(), instead created four
differnet function, one for each mode.
This will pave the way to address his second comment:

> Ideally this should be cleaned up in a way that completely
> avoids passing both user and kernel data at the same time.

which is not part of this series and will be done on top as
arguments shuffling will have to fix the sparse warnings
that exists currently.

Patch 1/7 first moves the VCHIQ_BULK_MODE_WAITING logic out
of vchiq_bulk_transfer

Patch 2/7 then moves the core logic to vchiq_bulk_transfer()
which can be shared in subsequent patches

Patch 3/7 and 4/7 refactors remaining bulk transfer modes

Patch 5/7 finally drops the vchiq_bulk_transfer() as not needed

patch 6/7 and 7/7 are drive by patches, noticed during development.

[1]: https://lore.kernel.org/linux-staging/[e-mail]/

changes in v5:
- Fix regression for VCHIQ_BULK_MODE_BLOCKING in 3/7
  (Fixes strange hang from v4 with 'vchiq_test -p')

Changes in v4:
- In 1/7 return 0 direcly instead of assigning status = 0
- In 3/7, fix null assignment of bulk waiter 

CHanges in v3:
- Directly return if service == NULL in 1/7 and 4/7
- Drop set-but-unused variable from 1/7

Changes in v2:
- Directly return if service == NULL in 3/7
- Prefix non-static functions with "vchiq_" (appropriate driver prefix)
- Use typed argument for 'userdata' in 1/7


Umang Jain (7):
  staging: vchiq: Factor out bulk transfer for VCHIQ_BULK_MODE_WAITING
  staging: vchiq_core: Simplify vchiq_bulk_transfer()
  staging: vchiq_core: Factor out bulk transfer for blocking mode
  staging: vchiq_core: Factor out bulk transfer for (no/)callback mode
  staging: vchiq_core: Drop vchiq_bulk_transfer()
  staging: vchiq_core: Remove unused function argument
  staging: vchiq_core: Pass enumerated flag instead of int

 .../interface/vchiq_arm/vchiq_arm.c           |  20 +-
 .../interface/vchiq_arm/vchiq_core.c          | 330 +++++++++++-------
 .../interface/vchiq_arm/vchiq_core.h          |  16 +-
 .../interface/vchiq_arm/vchiq_dev.c           |  15 +-
 4 files changed, 239 insertions(+), 142 deletions(-)

Comments

Stefan Wahren Sept. 9, 2024, 6:08 p.m. UTC | #1

Am 09.09.24 um 08:14 schrieb Umang Jain:
> Series mostly refactors vchiq_bulk_transfer() code according
> to Arnd's review comment from [1]:
>
>> 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.
> I didn't wrap vchiq_bulk_transfer(), instead created four
> differnet function, one for each mode.
> This will pave the way to address his second comment:
>
>> Ideally this should be cleaned up in a way that completely
>> avoids passing both user and kernel data at the same time.
> which is not part of this series and will be done on top as
> arguments shuffling will have to fix the sparse warnings
> that exists currently.
>
> Patch 1/7 first moves the VCHIQ_BULK_MODE_WAITING logic out
> of vchiq_bulk_transfer
>
> Patch 2/7 then moves the core logic to vchiq_bulk_transfer()
> which can be shared in subsequent patches
>
> Patch 3/7 and 4/7 refactors remaining bulk transfer modes
>
> Patch 5/7 finally drops the vchiq_bulk_transfer() as not needed
>
> patch 6/7 and 7/7 are drive by patches, noticed during development.
>
> [1]: https://lore.kernel.org/linux-staging/[e-mail]/
>
> changes in v5:
> - Fix regression for VCHIQ_BULK_MODE_BLOCKING in 3/7
>    (Fixes strange hang from v4 with 'vchiq_test -p')

The whole series is

Tested-by: Stefan Wahren <[e-mail]>

Umang Jain Sept. 10, 2024, 5:04 a.m. UTC | #2

On 09/09/24 11:38 pm, Stefan Wahren wrote:
> Am 09.09.24 um 08:14 schrieb Umang Jain:
>> Series mostly refactors vchiq_bulk_transfer() code according
>> to Arnd's review comment from [1]:
>>
>>> 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.
>> I didn't wrap vchiq_bulk_transfer(), instead created four
>> differnet function, one for each mode.
>> This will pave the way to address his second comment:
>>
>>> Ideally this should be cleaned up in a way that completely
>>> avoids passing both user and kernel data at the same time.
>> which is not part of this series and will be done on top as
>> arguments shuffling will have to fix the sparse warnings
>> that exists currently.
>>
>> Patch 1/7 first moves the VCHIQ_BULK_MODE_WAITING logic out
>> of vchiq_bulk_transfer
>>
>> Patch 2/7 then moves the core logic to vchiq_bulk_transfer()
>> which can be shared in subsequent patches
>>
>> Patch 3/7 and 4/7 refactors remaining bulk transfer modes
>>
>> Patch 5/7 finally drops the vchiq_bulk_transfer() as not needed
>>
>> patch 6/7 and 7/7 are drive by patches, noticed during development.
>>
>> [1]: 
>> https://lore.kernel.org/linux-staging/[e-mail]/
>>
>> changes in v5:
>> - Fix regression for VCHIQ_BULK_MODE_BLOCKING in 3/7
>>    (Fixes strange hang from v4 with 'vchiq_test -p')
>
> The whole series is
>
> Tested-by: Stefan Wahren <[e-mail]>

Thanks for testing and especially catching the strange hang!

The 2/7 review comment will be addressed in v6 (shortly)