lkml.org 
[lkml]   [2021]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Subject[PATCH 0/1] vchiq: Replacing global structs with per device
Date
Hello!

Sorry for being so late on this patch, I recently found some time to
make progress on this hence sending this here for review and suggestions

This patch is the first draft to address the following task in the vchiq TODO
(staging/vc04_services/interface/TODO):

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

The patch is NOT complete yet but I thought it is a good time for a review so
that I can confirm if I'm approaching this correctly and not missing anything.

To start with, I have focused on making the state (g_state) as per-device instead of
global as it is one of the most frequently used global varibale in the driver
code right now.

To achive this, I have modified the code to define a new data structure
"vchiq_device" which is initialised/allocated during probing and holds the state
as well as some other things like the miscdevice object that is needed to create
the misc device driver for vchiq. Embedding the miscdevice structure in it
helps us retrieve vchiq_device struct when the misc_device open() is called and
then use it to retrieve the state.

For all the ioctl and miscdevice fops, the filp->private_data stores the
vchiq_instance struct which embeds the vchiq state in it. I have modified the
code to fetch the state from here instead of using the global state or the
vchiq_get_state() function.

This patch has been tested using vchiq_test utility and seems to be working
functional so far.

------- Changes ---------

* A short summary of the changes:

* Define per device structure vchiq_device
* Use instance->state (per device state) instead of using the global state
in the code
* Split vchiq_get_state() into vchiq_get_state() and vchiq_validate_state().
The validation function is used to validate the per device state.
* Modify vchiq_dump_platform_instances(...) to pass in an extra vchiq_state
argument.

------- Some questions -------

**HOWEVER**, the patch is still not complete as there are some areas that still
need some work to ensure our driver is able to support multiple devices. I will
be listing them out below, would love to have some suggestions around it.

1. There is one specific function where I have not been able to replace the use
of global state, ie vchiq_initialise() function in vchiq_arm.c. The root
issue here is that vchiq_initialise is called from either the IOCTL
functions of the cdev or from other kernel modules directly. (Example, in
bcm2835_new_vchi_ctx() in bcm2835-audio)

When called from ioctl we have a reference to our per device state and we
can pass it in, but this is not possible when calling it from a different
kernel module. This forces us to use a global state in the driver.

I'm not sure how we can approach this in such a way that our driver is able
to support multiple devices. I would love to have some suggestions and
throughts around this.

2. In vchiq_deregister_chrdev() in vchiq_dev.c, I need the reference to the
miscdevice to call deregister on it. To get this reference, I use a global
variable but again, this wont work when we are trying to support multiple
devices. In this case, how do we handle registering and deregistering
multiple cdevs which might be created when we try to support multiple
VideoCore VPUs in the same driver.

--------------------------

Please let me know if you have any other suggestions or questions around the
patch and we can discuss the further.

Thank you in advance!

Regards,
Ojaswin

Ojaswin Mujoo (1):
staging: vchiq: Replace global state with per device state

.../interface/vchiq_arm/vchiq_arm.c | 100 ++++++++++++++----
.../interface/vchiq_arm/vchiq_arm.h | 12 ++-
.../interface/vchiq_arm/vchiq_core.c | 2 +-
.../interface/vchiq_arm/vchiq_core.h | 3 +-
.../interface/vchiq_arm/vchiq_dev.c | 64 +++++++----
5 files changed, 138 insertions(+), 43 deletions(-)

--
2.25.1

\
 
 \ /
  Last update: 2021-11-01 12:11    [W:0.103 / U:0.432 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site