[PATCH 0/1] vchiq: Replacing global structs with per device

* [PATCH 0/1] vchiq: Replacing global structs with per device
@ 2021-11-01 11:09 Ojaswin Mujoo
  2021-11-01 11:09 ` [PATCH 1/1] staging: vchiq: Replace global state with per device state Ojaswin Mujoo
  0 siblings, 1 reply; 7+ messages in thread
From: Ojaswin Mujoo @ 2021-11-01 11:09 UTC (permalink / raw)
  To: nsaenz, gregkh, stefan.wahren
  Cc: dan.carpenter, phil, linux-arm-kernel, linux-staging, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread