[PATCH 0/2] staging: vc04_services: Remove completed TODO items

[PATCH 0/2] staging: vc04_services: Remove completed TODO items

[Umang Jain]

As per staging-next, there are two items which are completed.
Drop those two entries from the vc04_services TODOs.

1/2 drop the item which was addressed by vchiq-bus and struct
vchiq_device per device handling. All blockers have been resolved and
landed in staging-next as of today.

2/2 was already completed as far as I can see.
It was also confirmed by Stefan here [1].

[1]: https://lore.kernel.org/lkml/79cf6cc0-d071-f834-1db9-cb5411c1f356@i2se.com/

Umang Jain (2):
  staging: vc04_services: Drop completed TODO item
  staging: vc04_services: Drop completed TODO item

 drivers/staging/vc04_services/interface/TODO | 15 ---------------
 1 file changed, 15 deletions(-)


base-commit: 5d9f6f26ec6656b43bf90f12705dbe88ce77f8d5
-- 
2.41.0

[PATCH 1/2] staging: vc04_services: Drop completed TODO item

[Umang Jain]

Drop the TODO item which deals with non-essential global
structures and per-device structure. All this has been
handled now through vchiq-bus and struct vchiq_device.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/interface/TODO | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index 6d9d4a800aa7..9c79ed549831 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -46,14 +46,6 @@ The code follows the 80 characters limitation yet tends to go 3 or 4 levels of
 indentation deep making it very unpleasant to read. This is specially relevant
 in the character driver ioctl code and in the core thread functions.
 
-* Get rid of all non essential global structures and create a proper per
-device structure
-
-The first thing one generally sees in a probe function is a memory allocation
-for all the device specific data. This structure is then passed all over the
-driver. This is good practice since it makes the driver work regardless of the
-number of devices probed.
-
 * Clean up Sparse warnings from __user annotations. See
 vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of "&waiter->bulk_waiter"
 is never disclosed to userspace.
-- 
2.41.0

[PATCH 2/2] staging: vc04_services: Drop completed TODO item

[Umang Jain]

Memory barries and remote_event_*() are documented.
Drop the TODO item related to them.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/interface/TODO | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index 9c79ed549831..2d51f6928e0f 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -33,13 +33,6 @@ such as dev_info, dev_dbg, and friends.
 A short top-down description of this driver's architecture (function of
 kthreads, userspace, limitations) could be very helpful for reviewers.
 
-* Review and comment memory barriers
-
-There is a heavy use of memory barriers in this driver, it would be very
-beneficial to go over all of them and, if correct, comment on their merits.
-Extra points to whomever confidently reviews the remote_event_*() family of
-functions.
-
 * Reformat core code with more sane indentations
 
 The code follows the 80 characters limitation yet tends to go 3 or 4 levels of
-- 
2.41.0

Re: [PATCH 2/2] staging: vc04_services: Drop completed TODO item

[Laurent Pinchart]

On Wed, Oct 25, 2023 at 06:33:00AM -0400, Umang Jain wrote:
> Memory barries and remote_event_*() are documented.
> Drop the TODO item related to them.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  drivers/staging/vc04_services/interface/TODO | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
> index 9c79ed549831..2d51f6928e0f 100644
> --- a/drivers/staging/vc04_services/interface/TODO
> +++ b/drivers/staging/vc04_services/interface/TODO
> @@ -33,13 +33,6 @@ such as dev_info, dev_dbg, and friends.
>  A short top-down description of this driver's architecture (function of
>  kthreads, userspace, limitations) could be very helpful for reviewers.
>  
> -* Review and comment memory barriers
> -
> -There is a heavy use of memory barriers in this driver, it would be very
> -beneficial to go over all of them and, if correct, comment on their merits.
> -Extra points to whomever confidently reviews the remote_event_*() family of
> -functions.
> -

Is vchiq so timing sensitive that it can't afford spinlocks ?

>  * Reformat core code with more sane indentations
>  
>  The code follows the 80 characters limitation yet tends to go 3 or 4 levels of

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] staging: vc04_services: Drop completed TODO item

[Stefan Wahren]

Hi,

[add the Raspberry Pi guys]

Am 25.10.23 um 12:46 schrieb Laurent Pinchart:
> On Wed, Oct 25, 2023 at 06:33:00AM -0400, Umang Jain wrote:
>> Memory barries and remote_event_*() are documented.
>> Drop the TODO item related to them.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   drivers/staging/vc04_services/interface/TODO | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
>> index 9c79ed549831..2d51f6928e0f 100644
>> --- a/drivers/staging/vc04_services/interface/TODO
>> +++ b/drivers/staging/vc04_services/interface/TODO
>> @@ -33,13 +33,6 @@ such as dev_info, dev_dbg, and friends.
>>   A short top-down description of this driver's architecture (function of
>>   kthreads, userspace, limitations) could be very helpful for reviewers.
>>
>> -* Review and comment memory barriers
>> -
>> -There is a heavy use of memory barriers in this driver, it would be very
>> -beneficial to go over all of them and, if correct, comment on their merits.
>> -Extra points to whomever confidently reviews the remote_event_*() family of
>> -functions.
>> -
from my point of view this part is done.
> Is vchiq so timing sensitive that it can't afford spinlocks ?
Sorry, i cannot answer this question. Maybe Phil or Dave?
>
>>   * Reformat core code with more sane indentations
>>
>>   The code follows the 80 characters limitation yet tends to go 3 or 4 levels of

Re: [PATCH 1/2] staging: vc04_services: Drop completed TODO item

[Stefan Wahren]

Hi Umang,

Am 25.10.23 um 12:32 schrieb Umang Jain:
> Drop the TODO item which deals with non-essential global
> structures and per-device structure. All this has been
> handled now through vchiq-bus and struct vchiq_device.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   drivers/staging/vc04_services/interface/TODO | 8 --------
>   1 file changed, 8 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
> index 6d9d4a800aa7..9c79ed549831 100644
> --- a/drivers/staging/vc04_services/interface/TODO
> +++ b/drivers/staging/vc04_services/interface/TODO
> @@ -46,14 +46,6 @@ The code follows the 80 characters limitation yet tends to go 3 or 4 levels of
>   indentation deep making it very unpleasant to read. This is specially relevant
>   in the character driver ioctl code and in the core thread functions.
>
> -* Get rid of all non essential global structures and create a proper per
> -device structure
at first thank for all the great work. Currently i don't have much time
to give a feedback for everything :-(

Tbh this TODO is not finished yet. There are still global structures.
Please look at variables starting with g_* in vchiq_arm.c and
vchiq_connected.c

Unfortunately i cannot tell which of them are easy to move into the
device structure.
> -
> -The first thing one generally sees in a probe function is a memory allocation
> -for all the device specific data. This structure is then passed all over the
> -driver. This is good practice since it makes the driver work regardless of the
> -number of devices probed.
> -
>   * Clean up Sparse warnings from __user annotations. See
>   vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of "&waiter->bulk_waiter"
>   is never disclosed to userspace.

Re: [PATCH 1/2] staging: vc04_services: Drop completed TODO item

[Stefan Wahren]

Hi Umang,

i hope it's okay to add the most of the recipients again.

Am 26.10.23 um 14:42 schrieb Umang Jain:
> Hi Stefan
>
> On 10/25/23 10:16 PM, Stefan Wahren wrote:
>> Hi Umang,
>>
>> Am 25.10.23 um 12:32 schrieb Umang Jain:
>>> Drop the TODO item which deals with non-essential global
>>> structures and per-device structure. All this has been
>>> handled now through vchiq-bus and struct vchiq_device.
>>>
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> ---
>>>   drivers/staging/vc04_services/interface/TODO | 8 --------
>>>   1 file changed, 8 deletions(-)
>>>
>>> diff --git a/drivers/staging/vc04_services/interface/TODO
>>> b/drivers/staging/vc04_services/interface/TODO
>>> index 6d9d4a800aa7..9c79ed549831 100644
>>> --- a/drivers/staging/vc04_services/interface/TODO
>>> +++ b/drivers/staging/vc04_services/interface/TODO
>>> @@ -46,14 +46,6 @@ The code follows the 80 characters limitation yet
>>> tends to go 3 or 4 levels of
>>>   indentation deep making it very unpleasant to read. This is
>>> specially relevant
>>>   in the character driver ioctl code and in the core thread functions.
>>>
>>> -* Get rid of all non essential global structures and create a
>>> proper per
>>> -device structure
>> at first thank for all the great work. Currently i don't have much time
>> to give a feedback for everything :-(
>
> No worries. As long as things are progressing, I don't mind.
>>
>> Tbh this TODO is not finished yet. There are still global structures.
>> Please look at variables starting with g_* in vchiq_arm.c and
>> vchiq_connected.c
>>
>
> Ah just saw them now. Oh god, these are a pain.
>> Unfortunately i cannot tell which of them are easy to move into the
>> device structure.
>
> Indeed. I don't have much time left this week, so probably I'll start
> developing something over next week. :-/
>
> Also do you know where I can find some overview documentation of the
> driver itself? I don't think I understand the details of "how this
> driver is supposed to function" in the first place.
unfortunately this is the reason for point 4 in the TODO list.

In the past i started a request in the Raspberry Pi github [1], but i
didn't get a fully satisfying response. Maybe this kind of documentation
is under NDA.

I vaguely remember of a little bit documentation on Github, but it
wasn't specific to the Linux driver.

[1] - https://github.com/raspberrypi/linux/issues/2730