2022-05-02 23:30:44

by Adrien Thierry

[permalink] [raw]
Subject: [PATCH 0/3] staging: vchiq_arm: remove some unnecessary global

This patch series aims at removing some unnecessary global structures in
order to make the driver more robust. This is part of an effort to address
TODO item "Get rid of all non essential global structures and create a
proper per device structure".

Adrien Thierry (3):
staging: vchiq_arm: add reference to vchiq device in vchiq_state
staging: vchiq_arm: get rid of global device structure
staging: vchiq_arm: get rid of global vchiq_states array

.../bcm2835-audio/bcm2835-vchiq.c | 40 +++++---
.../include/linux/raspberrypi/vchiq.h | 49 +++++-----
.../interface/vchiq_arm/vchiq_arm.c | 94 +++++++++---------
.../interface/vchiq_arm/vchiq_arm.h | 8 +-
.../interface/vchiq_arm/vchiq_core.c | 98 ++++++++++---------
.../interface/vchiq_arm/vchiq_core.h | 38 +++----
.../interface/vchiq_arm/vchiq_dev.c | 40 ++++----
.../vc04_services/vchiq-mmal/mmal-vchiq.c | 63 ++++++------
8 files changed, 218 insertions(+), 212 deletions(-)


base-commit: 5fe7856ad59afc56a6ff35d091bfaddd1d4f4bce
--
2.35.1


2022-05-03 00:54:46

by Adrien Thierry

[permalink] [raw]
Subject: [PATCH 2/3] staging: vchiq_arm: get rid of global device structure

Get rid of the global g_dev structure.

This is part of an effort to address TODO item "Get rid of all non
essential global structures and create a proper per device structure"

Signed-off-by: Adrien Thierry <[email protected]>
---
.../bcm2835-audio/bcm2835-vchiq.c | 7 ++-
.../include/linux/raspberrypi/vchiq.h | 12 ++--
.../interface/vchiq_arm/vchiq_arm.c | 63 +++++++++----------
.../interface/vchiq_arm/vchiq_core.c | 14 ++---
.../interface/vchiq_arm/vchiq_core.h | 10 +--
.../interface/vchiq_arm/vchiq_dev.c | 2 +-
.../vc04_services/vchiq-mmal/mmal-vchiq.c | 2 +-
7 files changed, 53 insertions(+), 57 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index e429b33b4d39..701abe430877 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -322,6 +322,8 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
unsigned int size, void *src)
{
struct bcm2835_audio_instance *instance = alsa_stream->instance;
+ struct bcm2835_vchi_ctx *vchi_ctx = alsa_stream->chip->vchi_ctx;
+ struct vchiq_instance *vchiq_instance = vchi_ctx->instance;
struct vc_audio_msg m = {
.type = VC_AUDIO_MSG_TYPE_WRITE,
.write.count = size,
@@ -343,9 +345,8 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
count = size;
if (!instance->max_packet) {
/* Send the message to the videocore */
- status = vchiq_bulk_transmit(instance->service_handle, src,
- count, NULL,
- VCHIQ_BULK_MODE_BLOCKING);
+ status = vchiq_bulk_transmit(vchiq_instance, instance->service_handle, src, count,
+ NULL, VCHIQ_BULK_MODE_BLOCKING);
} else {
while (count > 0) {
int bytes = min(instance->max_packet, count);
diff --git a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
index c93f2f3e87bb..715f02e7f1e1 100644
--- a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
+++ b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
@@ -96,12 +96,12 @@ extern void vchiq_release_message(unsigned int service,
struct vchiq_header *header);
extern int vchiq_queue_kernel_message(unsigned int handle, void *data,
unsigned int size);
-extern enum vchiq_status vchiq_bulk_transmit(unsigned int service,
- const void *data, unsigned int size, void *userdata,
- enum vchiq_bulk_mode mode);
-extern enum vchiq_status vchiq_bulk_receive(unsigned int service,
- void *data, unsigned int size, void *userdata,
- enum vchiq_bulk_mode mode);
+extern enum vchiq_status vchiq_bulk_transmit(struct vchiq_instance *instance, unsigned int service,
+ const void *data, unsigned int size, void *userdata,
+ enum vchiq_bulk_mode mode);
+extern enum vchiq_status vchiq_bulk_receive(struct vchiq_instance *instance, unsigned int service,
+ void *data, unsigned int size, void *userdata,
+ enum vchiq_bulk_mode mode);
extern void *vchiq_get_service_userdata(unsigned int service);
extern enum vchiq_status vchiq_get_peer_version(unsigned int handle,
short *peer_version);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index e6e0737c85fc..3b447c635c3f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -148,12 +148,11 @@ static unsigned int g_fragments_size;
static char *g_fragments_base;
static char *g_free_fragments;
static struct semaphore g_free_fragments_sema;
-static struct device *g_dev;

static DEFINE_SEMAPHORE(g_free_fragments_mutex);

static enum vchiq_status
-vchiq_blocking_bulk_transfer(unsigned int handle, void *data,
+vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
unsigned int size, enum vchiq_bulk_dir dir);

static irqreturn_t
@@ -175,17 +174,17 @@ vchiq_doorbell_irq(int irq, void *dev_id)
}

static void
-cleanup_pagelistinfo(struct vchiq_pagelist_info *pagelistinfo)
+cleanup_pagelistinfo(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo)
{
if (pagelistinfo->scatterlist_mapped) {
- dma_unmap_sg(g_dev, pagelistinfo->scatterlist,
+ dma_unmap_sg(instance->state->dev, pagelistinfo->scatterlist,
pagelistinfo->num_pages, pagelistinfo->dma_dir);
}

if (pagelistinfo->pages_need_release)
unpin_user_pages(pagelistinfo->pages, pagelistinfo->num_pages);

- dma_free_coherent(g_dev, pagelistinfo->pagelist_buffer_size,
+ dma_free_coherent(instance->state->dev, pagelistinfo->pagelist_buffer_size,
pagelistinfo->pagelist, pagelistinfo->dma_addr);
}

@@ -212,7 +211,7 @@ is_adjacent_block(u32 *addrs, u32 addr, unsigned int k)
*/

static struct vchiq_pagelist_info *
-create_pagelist(char *buf, char __user *ubuf,
+create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
size_t count, unsigned short type)
{
struct pagelist *pagelist;
@@ -250,7 +249,7 @@ create_pagelist(char *buf, char __user *ubuf,
/* Allocate enough storage to hold the page pointers and the page
* list
*/
- pagelist = dma_alloc_coherent(g_dev, pagelist_size, &dma_addr,
+ pagelist = dma_alloc_coherent(instance->state->dev, pagelist_size, &dma_addr,
GFP_KERNEL);

vchiq_log_trace(vchiq_arm_log_level, "%s - %pK", __func__, pagelist);
@@ -292,7 +291,7 @@ create_pagelist(char *buf, char __user *ubuf,
size_t bytes = PAGE_SIZE - off;

if (!pg) {
- cleanup_pagelistinfo(pagelistinfo);
+ cleanup_pagelistinfo(instance, pagelistinfo);
return NULL;
}

@@ -315,7 +314,7 @@ create_pagelist(char *buf, char __user *ubuf,
/* This is probably due to the process being killed */
if (actual_pages > 0)
unpin_user_pages(pages, actual_pages);
- cleanup_pagelistinfo(pagelistinfo);
+ cleanup_pagelistinfo(instance, pagelistinfo);
return NULL;
}
/* release user pages */
@@ -338,13 +337,13 @@ create_pagelist(char *buf, char __user *ubuf,
count -= len;
}

- dma_buffers = dma_map_sg(g_dev,
+ dma_buffers = dma_map_sg(instance->state->dev,
scatterlist,
num_pages,
pagelistinfo->dma_dir);

if (dma_buffers == 0) {
- cleanup_pagelistinfo(pagelistinfo);
+ cleanup_pagelistinfo(instance, pagelistinfo);
return NULL;
}

@@ -378,7 +377,7 @@ create_pagelist(char *buf, char __user *ubuf,
char *fragments;

if (down_interruptible(&g_free_fragments_sema)) {
- cleanup_pagelistinfo(pagelistinfo);
+ cleanup_pagelistinfo(instance, pagelistinfo);
return NULL;
}

@@ -397,7 +396,7 @@ create_pagelist(char *buf, char __user *ubuf,
}

static void
-free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
+free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo,
int actual)
{
struct pagelist *pagelist = pagelistinfo->pagelist;
@@ -411,7 +410,7 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
* NOTE: dma_unmap_sg must be called before the
* cpu can touch any of the data/pages.
*/
- dma_unmap_sg(g_dev, pagelistinfo->scatterlist,
+ dma_unmap_sg(instance->state->dev, pagelistinfo->scatterlist,
pagelistinfo->num_pages, pagelistinfo->dma_dir);
pagelistinfo->scatterlist_mapped = 0;

@@ -460,7 +459,7 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
set_page_dirty(pages[i]);
}

- cleanup_pagelistinfo(pagelistinfo);
+ cleanup_pagelistinfo(instance, pagelistinfo);
}

int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
@@ -547,7 +546,6 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
return err ? : -ENXIO;
}

- g_dev = dev;
vchiq_log_info(vchiq_arm_log_level, "vchiq_init - done (slots %pK, phys %pad)",
vchiq_slot_zero, &slot_phys);

@@ -615,12 +613,12 @@ remote_event_signal(struct remote_event *event)
}

int
-vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
+vchiq_prepare_bulk_data(struct vchiq_instance *instance, struct vchiq_bulk *bulk, void *offset,
void __user *uoffset, int size, int dir)
{
struct vchiq_pagelist_info *pagelistinfo;

- pagelistinfo = create_pagelist(offset, uoffset, size,
+ pagelistinfo = create_pagelist(instance, offset, uoffset, size,
(dir == VCHIQ_BULK_RECEIVE)
? PAGELIST_READ
: PAGELIST_WRITE);
@@ -640,10 +638,10 @@ vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
}

void
-vchiq_complete_bulk(struct vchiq_bulk *bulk)
+vchiq_complete_bulk(struct vchiq_instance *instance, struct vchiq_bulk *bulk)
{
if (bulk && bulk->remote_data && bulk->actual)
- free_pagelist((struct vchiq_pagelist_info *)bulk->remote_data,
+ free_pagelist(instance, (struct vchiq_pagelist_info *)bulk->remote_data,
bulk->actual);
}

@@ -834,8 +832,8 @@ vchiq_open_service(struct vchiq_instance *instance,
EXPORT_SYMBOL(vchiq_open_service);

enum vchiq_status
-vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
- void *userdata, enum vchiq_bulk_mode mode)
+vchiq_bulk_transmit(struct vchiq_instance *instance, unsigned int handle, const void *data,
+ unsigned int size, void *userdata, enum vchiq_bulk_mode mode)
{
enum vchiq_status status;

@@ -843,13 +841,13 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
switch (mode) {
case VCHIQ_BULK_MODE_NOCALLBACK:
case VCHIQ_BULK_MODE_CALLBACK:
- status = vchiq_bulk_transfer(handle,
+ status = vchiq_bulk_transfer(instance, handle,
(void *)data, NULL,
size, userdata, mode,
VCHIQ_BULK_TRANSMIT);
break;
case VCHIQ_BULK_MODE_BLOCKING:
- status = vchiq_blocking_bulk_transfer(handle, (void *)data, size,
+ status = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
VCHIQ_BULK_TRANSMIT);
break;
default:
@@ -871,8 +869,8 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
}
EXPORT_SYMBOL(vchiq_bulk_transmit);

-enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
- unsigned int size, void *userdata,
+enum vchiq_status vchiq_bulk_receive(struct vchiq_instance *instance, unsigned int handle,
+ void *data, unsigned int size, void *userdata,
enum vchiq_bulk_mode mode)
{
enum vchiq_status status;
@@ -881,12 +879,12 @@ enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
switch (mode) {
case VCHIQ_BULK_MODE_NOCALLBACK:
case VCHIQ_BULK_MODE_CALLBACK:
- status = vchiq_bulk_transfer(handle, data, NULL,
+ status = vchiq_bulk_transfer(instance, handle, data, NULL,
size, userdata,
mode, VCHIQ_BULK_RECEIVE);
break;
case VCHIQ_BULK_MODE_BLOCKING:
- status = vchiq_blocking_bulk_transfer(handle, (void *)data, size,
+ status = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
VCHIQ_BULK_RECEIVE);
break;
default:
@@ -909,10 +907,9 @@ enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
EXPORT_SYMBOL(vchiq_bulk_receive);

static enum vchiq_status
-vchiq_blocking_bulk_transfer(unsigned int handle, void *data, unsigned int size,
- enum vchiq_bulk_dir dir)
+vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
+ unsigned int size, enum vchiq_bulk_dir dir)
{
- struct vchiq_instance *instance;
struct vchiq_service *service;
enum vchiq_status status;
struct bulk_waiter_node *waiter = NULL, *iter;
@@ -921,8 +918,6 @@ vchiq_blocking_bulk_transfer(unsigned int handle, void *data, unsigned int size,
if (!service)
return VCHIQ_ERROR;

- instance = service->instance;
-
vchiq_service_put(service);

mutex_lock(&instance->bulk_waiter_list_mutex);
@@ -959,7 +954,7 @@ vchiq_blocking_bulk_transfer(unsigned int handle, void *data, unsigned int size,
}
}

- status = vchiq_bulk_transfer(handle, data, NULL, size,
+ status = vchiq_bulk_transfer(instance, handle, data, NULL, size,
&waiter->bulk_waiter,
VCHIQ_BULK_MODE_BLOCKING, dir);
if ((status != VCHIQ_RETRY) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 0d5c39d7c6e2..04eec18835da 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1441,7 +1441,7 @@ abort_outstanding_bulks(struct vchiq_service *service,
}

if (queue->process != queue->local_insert) {
- vchiq_complete_bulk(bulk);
+ vchiq_complete_bulk(service->instance, bulk);

vchiq_log_info(SRVTRACE_LEVEL(service),
"%s %c%c%c%c d:%d ABORTED - tx len:%d, rx len:%d",
@@ -1769,7 +1769,7 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)

DEBUG_TRACE(PARSE_LINE);
WARN_ON(queue->process == queue->local_insert);
- vchiq_complete_bulk(bulk);
+ vchiq_complete_bulk(service->instance, bulk);
queue->process++;
mutex_unlock(&service->bulk_mutex);
DEBUG_TRACE(PARSE_LINE);
@@ -2998,9 +2998,9 @@ vchiq_remove_service(unsigned int handle)
* When called in blocking mode, the userdata field points to a bulk_waiter
* structure.
*/
-enum vchiq_status vchiq_bulk_transfer(unsigned int handle, void *offset, void __user *uoffset,
- int size, void *userdata, enum vchiq_bulk_mode mode,
- enum vchiq_bulk_dir dir)
+enum vchiq_status vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
+ void *offset, void __user *uoffset, int size, void *userdata,
+ enum vchiq_bulk_mode mode, enum vchiq_bulk_dir dir)
{
struct vchiq_service *service = find_service_by_handle(handle);
struct vchiq_bulk_queue *queue;
@@ -3077,7 +3077,7 @@ enum vchiq_status vchiq_bulk_transfer(unsigned int handle, void *offset, void __
bulk->size = size;
bulk->actual = VCHIQ_BULK_ACTUAL_ABORTED;

- if (vchiq_prepare_bulk_data(bulk, offset, uoffset, size, dir))
+ if (vchiq_prepare_bulk_data(instance, bulk, offset, uoffset, size, dir))
goto unlock_error_exit;

wmb();
@@ -3141,7 +3141,7 @@ enum vchiq_status vchiq_bulk_transfer(unsigned int handle, void *offset, void __
unlock_both_error_exit:
mutex_unlock(&state->slot_mutex);
cancel_bulk_error_exit:
- vchiq_complete_bulk(bulk);
+ vchiq_complete_bulk(service->instance, bulk);
unlock_error_exit:
mutex_unlock(&service->bulk_mutex);

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 352017ff5309..2ce54ce9f02a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -489,8 +489,8 @@ extern void
remote_event_pollall(struct vchiq_state *state);

extern enum vchiq_status
-vchiq_bulk_transfer(unsigned int handle, void *offset, void __user *uoffset,
- int size, void *userdata, enum vchiq_bulk_mode mode,
+vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *offset,
+ void __user *uoffset, int size, void *userdata, enum vchiq_bulk_mode mode,
enum vchiq_bulk_dir dir);

extern int
@@ -556,10 +556,10 @@ vchiq_queue_message(unsigned int handle,
void *context,
size_t size);

-int vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset, void __user *uoffset,
- int size, int dir);
+int vchiq_prepare_bulk_data(struct vchiq_instance *instance, struct vchiq_bulk *bulk, void *offset,
+ void __user *uoffset, int size, int dir);

-void vchiq_complete_bulk(struct vchiq_bulk *bulk);
+void vchiq_complete_bulk(struct vchiq_instance *instance, struct vchiq_bulk *bulk);

void remote_event_signal(struct remote_event *event);

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 66bbfec332ba..077e3fcbd651 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -330,7 +330,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
userdata = args->userdata;
}

- status = vchiq_bulk_transfer(args->handle, NULL, args->data, args->size,
+ status = vchiq_bulk_transfer(instance, args->handle, NULL, args->data, args->size,
userdata, args->mode, dir);

if (!waiter) {
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
index 249dd3e30c2f..88813af8de49 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
@@ -293,7 +293,7 @@ static void buffer_to_host_work_cb(struct work_struct *work)
len = 8;
/* queue the bulk submission */
vchiq_use_service(instance->service_handle);
- ret = vchiq_bulk_receive(instance->service_handle,
+ ret = vchiq_bulk_receive(instance->vchiq_instance, instance->service_handle,
msg_context->u.bulk.buffer->buffer,
/* Actual receive needs to be a multiple
* of 4 bytes
--
2.35.1

2022-05-03 01:22:07

by Adrien Thierry

[permalink] [raw]
Subject: [PATCH 1/3] staging: vchiq_arm: add reference to vchiq device in vchiq_state

Add a reference to the vchiq device in the vchiq_state structure. This
allows the device structure to be passed around, which will be useful in
order to get rid of the global g_dev structure.

Signed-off-by: Adrien Thierry <[email protected]>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 4 +++-
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 3 ++-
3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 0596ac61e286..e6e0737c85fc 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -519,7 +519,7 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);

- err = vchiq_init_state(state, vchiq_slot_zero);
+ err = vchiq_init_state(state, vchiq_slot_zero, dev);
if (err)
return err;

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 8f99272dbd6f..0d5c39d7c6e2 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2142,7 +2142,7 @@ vchiq_init_slots(void *mem_base, int mem_size)
}

int
-vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
+vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero, struct device *dev)
{
struct vchiq_shared_state *local;
struct vchiq_shared_state *remote;
@@ -2169,6 +2169,8 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)

memset(state, 0, sizeof(struct vchiq_state));

+ state->dev = dev;
+
/*
* initialize shared state pointers
*/
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 82b7bd7b54b2..352017ff5309 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -315,6 +315,7 @@ struct vchiq_slot_zero {
};

struct vchiq_state {
+ struct device *dev;
int id;
int initialised;
enum vchiq_connstate conn_state;
@@ -458,7 +459,7 @@ extern struct vchiq_slot_zero *
vchiq_init_slots(void *mem_base, int mem_size);

extern int
-vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero);
+vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero, struct device *dev);

extern enum vchiq_status
vchiq_connect_internal(struct vchiq_state *state, struct vchiq_instance *instance);
--
2.35.1

2022-05-04 17:22:57

by Stefan Wahren

[permalink] [raw]
Subject: Re: [PATCH 2/3] staging: vchiq_arm: get rid of global device structure

Hi Adrien,

Am 02.05.22 um 20:30 schrieb Adrien Thierry:
> Get rid of the global g_dev structure.

i understand the motivation, but could you please explain more in detail
why you decided to add vchiq_instance instead of device reference? I
think vchiq_instance is a more internal structure which should be
avoided in kernel consumers like bcm2835-audio or mmal.

Best regards

>
> This is part of an effort to address TODO item "Get rid of all non
> essential global structures and create a proper per device structure"
>
> Signed-off-by: Adrien Thierry <[email protected]>
> ---
> .../bcm2835-audio/bcm2835-vchiq.c | 7 ++-
> .../include/linux/raspberrypi/vchiq.h | 12 ++--
> .../interface/vchiq_arm/vchiq_arm.c | 63 +++++++++----------
> .../interface/vchiq_arm/vchiq_core.c | 14 ++---
> .../interface/vchiq_arm/vchiq_core.h | 10 +--
> .../interface/vchiq_arm/vchiq_dev.c | 2 +-
> .../vc04_services/vchiq-mmal/mmal-vchiq.c | 2 +-
> 7 files changed, 53 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> index e429b33b4d39..701abe430877 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
> @@ -322,6 +322,8 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
> unsigned int size, void *src)
> {
> struct bcm2835_audio_instance *instance = alsa_stream->instance;
> + struct bcm2835_vchi_ctx *vchi_ctx = alsa_stream->chip->vchi_ctx;
> + struct vchiq_instance *vchiq_instance = vchi_ctx->instance;
> struct vc_audio_msg m = {
> .type = VC_AUDIO_MSG_TYPE_WRITE,
> .write.count = size,
> @@ -343,9 +345,8 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
> count = size;
> if (!instance->max_packet) {
> /* Send the message to the videocore */
> - status = vchiq_bulk_transmit(instance->service_handle, src,
> - count, NULL,
> - VCHIQ_BULK_MODE_BLOCKING);
> + status = vchiq_bulk_transmit(vchiq_instance, instance->service_handle, src, count,
> + NULL, VCHIQ_BULK_MODE_BLOCKING);
> } else {
> while (count > 0) {
> int bytes = min(instance->max_packet, count);
> diff --git a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
> index c93f2f3e87bb..715f02e7f1e1 100644
> --- a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
> +++ b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
> @@ -96,12 +96,12 @@ extern void vchiq_release_message(unsigned int service,
> struct vchiq_header *header);
> extern int vchiq_queue_kernel_message(unsigned int handle, void *data,
> unsigned int size);
> -extern enum vchiq_status vchiq_bulk_transmit(unsigned int service,
> - const void *data, unsigned int size, void *userdata,
> - enum vchiq_bulk_mode mode);
> -extern enum vchiq_status vchiq_bulk_receive(unsigned int service,
> - void *data, unsigned int size, void *userdata,
> - enum vchiq_bulk_mode mode);
> +extern enum vchiq_status vchiq_bulk_transmit(struct vchiq_instance *instance, unsigned int service,
> + const void *data, unsigned int size, void *userdata,
> + enum vchiq_bulk_mode mode);
> +extern enum vchiq_status vchiq_bulk_receive(struct vchiq_instance *instance, unsigned int service,
> + void *data, unsigned int size, void *userdata,
> + enum vchiq_bulk_mode mode);
> extern void *vchiq_get_service_userdata(unsigned int service);
> extern enum vchiq_status vchiq_get_peer_version(unsigned int handle,
> short *peer_version);
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index e6e0737c85fc..3b447c635c3f 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -148,12 +148,11 @@ static unsigned int g_fragments_size;
> static char *g_fragments_base;
> static char *g_free_fragments;
> static struct semaphore g_free_fragments_sema;
> -static struct device *g_dev;
>
> static DEFINE_SEMAPHORE(g_free_fragments_mutex);
>
> static enum vchiq_status
> -vchiq_blocking_bulk_transfer(unsigned int handle, void *data,
> +vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
> unsigned int size, enum vchiq_bulk_dir dir);
>
> static irqreturn_t
> @@ -175,17 +174,17 @@ vchiq_doorbell_irq(int irq, void *dev_id)
> }
>
> static void
> -cleanup_pagelistinfo(struct vchiq_pagelist_info *pagelistinfo)
> +cleanup_pagelistinfo(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo)
> {
> if (pagelistinfo->scatterlist_mapped) {
> - dma_unmap_sg(g_dev, pagelistinfo->scatterlist,
> + dma_unmap_sg(instance->state->dev, pagelistinfo->scatterlist,
> pagelistinfo->num_pages, pagelistinfo->dma_dir);
> }
>
> if (pagelistinfo->pages_need_release)
> unpin_user_pages(pagelistinfo->pages, pagelistinfo->num_pages);
>
> - dma_free_coherent(g_dev, pagelistinfo->pagelist_buffer_size,
> + dma_free_coherent(instance->state->dev, pagelistinfo->pagelist_buffer_size,
> pagelistinfo->pagelist, pagelistinfo->dma_addr);
> }
>
> @@ -212,7 +211,7 @@ is_adjacent_block(u32 *addrs, u32 addr, unsigned int k)
> */
>
> static struct vchiq_pagelist_info *
> -create_pagelist(char *buf, char __user *ubuf,
> +create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
> size_t count, unsigned short type)
> {
> struct pagelist *pagelist;
> @@ -250,7 +249,7 @@ create_pagelist(char *buf, char __user *ubuf,
> /* Allocate enough storage to hold the page pointers and the page
> * list
> */
> - pagelist = dma_alloc_coherent(g_dev, pagelist_size, &dma_addr,
> + pagelist = dma_alloc_coherent(instance->state->dev, pagelist_size, &dma_addr,
> GFP_KERNEL);
>
> vchiq_log_trace(vchiq_arm_log_level, "%s - %pK", __func__, pagelist);
> @@ -292,7 +291,7 @@ create_pagelist(char *buf, char __user *ubuf,
> size_t bytes = PAGE_SIZE - off;
>
> if (!pg) {
> - cleanup_pagelistinfo(pagelistinfo);
> + cleanup_pagelistinfo(instance, pagelistinfo);
> return NULL;
> }
>
> @@ -315,7 +314,7 @@ create_pagelist(char *buf, char __user *ubuf,
> /* This is probably due to the process being killed */
> if (actual_pages > 0)
> unpin_user_pages(pages, actual_pages);
> - cleanup_pagelistinfo(pagelistinfo);
> + cleanup_pagelistinfo(instance, pagelistinfo);
> return NULL;
> }
> /* release user pages */
> @@ -338,13 +337,13 @@ create_pagelist(char *buf, char __user *ubuf,
> count -= len;
> }
>
> - dma_buffers = dma_map_sg(g_dev,
> + dma_buffers = dma_map_sg(instance->state->dev,
> scatterlist,
> num_pages,
> pagelistinfo->dma_dir);
>
> if (dma_buffers == 0) {
> - cleanup_pagelistinfo(pagelistinfo);
> + cleanup_pagelistinfo(instance, pagelistinfo);
> return NULL;
> }
>
> @@ -378,7 +377,7 @@ create_pagelist(char *buf, char __user *ubuf,
> char *fragments;
>
> if (down_interruptible(&g_free_fragments_sema)) {
> - cleanup_pagelistinfo(pagelistinfo);
> + cleanup_pagelistinfo(instance, pagelistinfo);
> return NULL;
> }
>
> @@ -397,7 +396,7 @@ create_pagelist(char *buf, char __user *ubuf,
> }
>
> static void
> -free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
> +free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagelistinfo,
> int actual)
> {
> struct pagelist *pagelist = pagelistinfo->pagelist;
> @@ -411,7 +410,7 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
> * NOTE: dma_unmap_sg must be called before the
> * cpu can touch any of the data/pages.
> */
> - dma_unmap_sg(g_dev, pagelistinfo->scatterlist,
> + dma_unmap_sg(instance->state->dev, pagelistinfo->scatterlist,
> pagelistinfo->num_pages, pagelistinfo->dma_dir);
> pagelistinfo->scatterlist_mapped = 0;
>
> @@ -460,7 +459,7 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
> set_page_dirty(pages[i]);
> }
>
> - cleanup_pagelistinfo(pagelistinfo);
> + cleanup_pagelistinfo(instance, pagelistinfo);
> }
>
> int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
> @@ -547,7 +546,6 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
> return err ? : -ENXIO;
> }
>
> - g_dev = dev;
> vchiq_log_info(vchiq_arm_log_level, "vchiq_init - done (slots %pK, phys %pad)",
> vchiq_slot_zero, &slot_phys);
>
> @@ -615,12 +613,12 @@ remote_event_signal(struct remote_event *event)
> }
>
> int
> -vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
> +vchiq_prepare_bulk_data(struct vchiq_instance *instance, struct vchiq_bulk *bulk, void *offset,
> void __user *uoffset, int size, int dir)
> {
> struct vchiq_pagelist_info *pagelistinfo;
>
> - pagelistinfo = create_pagelist(offset, uoffset, size,
> + pagelistinfo = create_pagelist(instance, offset, uoffset, size,
> (dir == VCHIQ_BULK_RECEIVE)
> ? PAGELIST_READ
> : PAGELIST_WRITE);
> @@ -640,10 +638,10 @@ vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
> }
>
> void
> -vchiq_complete_bulk(struct vchiq_bulk *bulk)
> +vchiq_complete_bulk(struct vchiq_instance *instance, struct vchiq_bulk *bulk)
> {
> if (bulk && bulk->remote_data && bulk->actual)
> - free_pagelist((struct vchiq_pagelist_info *)bulk->remote_data,
> + free_pagelist(instance, (struct vchiq_pagelist_info *)bulk->remote_data,
> bulk->actual);
> }
>
> @@ -834,8 +832,8 @@ vchiq_open_service(struct vchiq_instance *instance,
> EXPORT_SYMBOL(vchiq_open_service);
>
> enum vchiq_status
> -vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
> - void *userdata, enum vchiq_bulk_mode mode)
> +vchiq_bulk_transmit(struct vchiq_instance *instance, unsigned int handle, const void *data,
> + unsigned int size, void *userdata, enum vchiq_bulk_mode mode)
> {
> enum vchiq_status status;
>
> @@ -843,13 +841,13 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
> switch (mode) {
> case VCHIQ_BULK_MODE_NOCALLBACK:
> case VCHIQ_BULK_MODE_CALLBACK:
> - status = vchiq_bulk_transfer(handle,
> + status = vchiq_bulk_transfer(instance, handle,
> (void *)data, NULL,
> size, userdata, mode,
> VCHIQ_BULK_TRANSMIT);
> break;
> case VCHIQ_BULK_MODE_BLOCKING:
> - status = vchiq_blocking_bulk_transfer(handle, (void *)data, size,
> + status = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
> VCHIQ_BULK_TRANSMIT);
> break;
> default:
> @@ -871,8 +869,8 @@ vchiq_bulk_transmit(unsigned int handle, const void *data, unsigned int size,
> }
> EXPORT_SYMBOL(vchiq_bulk_transmit);
>
> -enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
> - unsigned int size, void *userdata,
> +enum vchiq_status vchiq_bulk_receive(struct vchiq_instance *instance, unsigned int handle,
> + void *data, unsigned int size, void *userdata,
> enum vchiq_bulk_mode mode)
> {
> enum vchiq_status status;
> @@ -881,12 +879,12 @@ enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
> switch (mode) {
> case VCHIQ_BULK_MODE_NOCALLBACK:
> case VCHIQ_BULK_MODE_CALLBACK:
> - status = vchiq_bulk_transfer(handle, data, NULL,
> + status = vchiq_bulk_transfer(instance, handle, data, NULL,
> size, userdata,
> mode, VCHIQ_BULK_RECEIVE);
> break;
> case VCHIQ_BULK_MODE_BLOCKING:
> - status = vchiq_blocking_bulk_transfer(handle, (void *)data, size,
> + status = vchiq_blocking_bulk_transfer(instance, handle, (void *)data, size,
> VCHIQ_BULK_RECEIVE);
> break;
> default:
> @@ -909,10 +907,9 @@ enum vchiq_status vchiq_bulk_receive(unsigned int handle, void *data,
> EXPORT_SYMBOL(vchiq_bulk_receive);
>
> static enum vchiq_status
> -vchiq_blocking_bulk_transfer(unsigned int handle, void *data, unsigned int size,
> - enum vchiq_bulk_dir dir)
> +vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
> + unsigned int size, enum vchiq_bulk_dir dir)
> {
> - struct vchiq_instance *instance;
> struct vchiq_service *service;
> enum vchiq_status status;
> struct bulk_waiter_node *waiter = NULL, *iter;
> @@ -921,8 +918,6 @@ vchiq_blocking_bulk_transfer(unsigned int handle, void *data, unsigned int size,
> if (!service)
> return VCHIQ_ERROR;
>
> - instance = service->instance;
> -
> vchiq_service_put(service);
>
> mutex_lock(&instance->bulk_waiter_list_mutex);
> @@ -959,7 +954,7 @@ vchiq_blocking_bulk_transfer(unsigned int handle, void *data, unsigned int size,
> }
> }
>
> - status = vchiq_bulk_transfer(handle, data, NULL, size,
> + status = vchiq_bulk_transfer(instance, handle, data, NULL, size,
> &waiter->bulk_waiter,
> VCHIQ_BULK_MODE_BLOCKING, dir);
> if ((status != VCHIQ_RETRY) || fatal_signal_pending(current) || !waiter->bulk_waiter.bulk) {
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> index 0d5c39d7c6e2..04eec18835da 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -1441,7 +1441,7 @@ abort_outstanding_bulks(struct vchiq_service *service,
> }
>
> if (queue->process != queue->local_insert) {
> - vchiq_complete_bulk(bulk);
> + vchiq_complete_bulk(service->instance, bulk);
>
> vchiq_log_info(SRVTRACE_LEVEL(service),
> "%s %c%c%c%c d:%d ABORTED - tx len:%d, rx len:%d",
> @@ -1769,7 +1769,7 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
>
> DEBUG_TRACE(PARSE_LINE);
> WARN_ON(queue->process == queue->local_insert);
> - vchiq_complete_bulk(bulk);
> + vchiq_complete_bulk(service->instance, bulk);
> queue->process++;
> mutex_unlock(&service->bulk_mutex);
> DEBUG_TRACE(PARSE_LINE);
> @@ -2998,9 +2998,9 @@ vchiq_remove_service(unsigned int handle)
> * When called in blocking mode, the userdata field points to a bulk_waiter
> * structure.
> */
> -enum vchiq_status vchiq_bulk_transfer(unsigned int handle, void *offset, void __user *uoffset,
> - int size, void *userdata, enum vchiq_bulk_mode mode,
> - enum vchiq_bulk_dir dir)
> +enum vchiq_status vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
> + void *offset, void __user *uoffset, int size, void *userdata,
> + enum vchiq_bulk_mode mode, enum vchiq_bulk_dir dir)
> {
> struct vchiq_service *service = find_service_by_handle(handle);
> struct vchiq_bulk_queue *queue;
> @@ -3077,7 +3077,7 @@ enum vchiq_status vchiq_bulk_transfer(unsigned int handle, void *offset, void __
> bulk->size = size;
> bulk->actual = VCHIQ_BULK_ACTUAL_ABORTED;
>
> - if (vchiq_prepare_bulk_data(bulk, offset, uoffset, size, dir))
> + if (vchiq_prepare_bulk_data(instance, bulk, offset, uoffset, size, dir))
> goto unlock_error_exit;
>
> wmb();
> @@ -3141,7 +3141,7 @@ enum vchiq_status vchiq_bulk_transfer(unsigned int handle, void *offset, void __
> unlock_both_error_exit:
> mutex_unlock(&state->slot_mutex);
> cancel_bulk_error_exit:
> - vchiq_complete_bulk(bulk);
> + vchiq_complete_bulk(service->instance, bulk);
> unlock_error_exit:
> mutex_unlock(&service->bulk_mutex);
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> index 352017ff5309..2ce54ce9f02a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -489,8 +489,8 @@ extern void
> remote_event_pollall(struct vchiq_state *state);
>
> extern enum vchiq_status
> -vchiq_bulk_transfer(unsigned int handle, void *offset, void __user *uoffset,
> - int size, void *userdata, enum vchiq_bulk_mode mode,
> +vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *offset,
> + void __user *uoffset, int size, void *userdata, enum vchiq_bulk_mode mode,
> enum vchiq_bulk_dir dir);
>
> extern int
> @@ -556,10 +556,10 @@ vchiq_queue_message(unsigned int handle,
> void *context,
> size_t size);
>
> -int vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset, void __user *uoffset,
> - int size, int dir);
> +int vchiq_prepare_bulk_data(struct vchiq_instance *instance, struct vchiq_bulk *bulk, void *offset,
> + void __user *uoffset, int size, int dir);
>
> -void vchiq_complete_bulk(struct vchiq_bulk *bulk);
> +void vchiq_complete_bulk(struct vchiq_instance *instance, struct vchiq_bulk *bulk);
>
> void remote_event_signal(struct remote_event *event);
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> index 66bbfec332ba..077e3fcbd651 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> @@ -330,7 +330,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
> userdata = args->userdata;
> }
>
> - status = vchiq_bulk_transfer(args->handle, NULL, args->data, args->size,
> + status = vchiq_bulk_transfer(instance, args->handle, NULL, args->data, args->size,
> userdata, args->mode, dir);
>
> if (!waiter) {
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> index 249dd3e30c2f..88813af8de49 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> @@ -293,7 +293,7 @@ static void buffer_to_host_work_cb(struct work_struct *work)
> len = 8;
> /* queue the bulk submission */
> vchiq_use_service(instance->service_handle);
> - ret = vchiq_bulk_receive(instance->service_handle,
> + ret = vchiq_bulk_receive(instance->vchiq_instance, instance->service_handle,
> msg_context->u.bulk.buffer->buffer,
> /* Actual receive needs to be a multiple
> * of 4 bytes

2022-05-05 08:23:03

by Stefan Wahren

[permalink] [raw]
Subject: Re: [PATCH 1/3] staging: vchiq_arm: add reference to vchiq device in vchiq_state

Hi Adrien,

Am 02.05.22 um 20:30 schrieb Adrien Thierry:
> Add a reference to the vchiq device in the vchiq_state structure. This
> allows the device structure to be passed around, which will be useful in
> order to get rid of the global g_dev structure.

the patch looks good to me. It would be nice to mention that with adding
the device reference it would possible to introduce common kernel logging.

Best regards

>
> Signed-off-by: Adrien Thierry <[email protected]>
> ---
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
> .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 4 +++-
> .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 3 ++-
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 0596ac61e286..e6e0737c85fc 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -519,7 +519,7 @@ int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
> *(char **)&g_fragments_base[i * g_fragments_size] = NULL;
> sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
>
> - err = vchiq_init_state(state, vchiq_slot_zero);
> + err = vchiq_init_state(state, vchiq_slot_zero, dev);
> if (err)
> return err;
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> index 8f99272dbd6f..0d5c39d7c6e2 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -2142,7 +2142,7 @@ vchiq_init_slots(void *mem_base, int mem_size)
> }
>
> int
> -vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
> +vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero, struct device *dev)
> {
> struct vchiq_shared_state *local;
> struct vchiq_shared_state *remote;
> @@ -2169,6 +2169,8 @@ vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero)
>
> memset(state, 0, sizeof(struct vchiq_state));
>
> + state->dev = dev;
> +
> /*
> * initialize shared state pointers
> */
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> index 82b7bd7b54b2..352017ff5309 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -315,6 +315,7 @@ struct vchiq_slot_zero {
> };
>
> struct vchiq_state {
> + struct device *dev;
> int id;
> int initialised;
> enum vchiq_connstate conn_state;
> @@ -458,7 +459,7 @@ extern struct vchiq_slot_zero *
> vchiq_init_slots(void *mem_base, int mem_size);
>
> extern int
> -vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero);
> +vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero, struct device *dev);
>
> extern enum vchiq_status
> vchiq_connect_internal(struct vchiq_state *state, struct vchiq_instance *instance);

2022-05-06 07:12:23

by Adrien Thierry

[permalink] [raw]
Subject: Re: [PATCH 2/3] staging: vchiq_arm: get rid of global device structure

Hi Stefan,

Thanks for your feedback.

> i understand the motivation, but could you please explain more in detail
> why you decided to add vchiq_instance instead of device reference? I
> think vchiq_instance is a more internal structure which should be
> avoided in kernel consumers like bcm2835-audio or mmal.

I used the vchiq_instance instead of the device reference because in order
to get rid of the vchiq_states array (patch 3/3 [1]), I needed another way
to access the vchiq_state in the 'handle_to_service' function. So I passed
the vchiq_instance to it (I could also have passed the state directly
instead of the instance), and this propagated in the caller chain all the
way up to 'vchiq_bulk_transmit' and friends which are used in the
bcm2835-audio consumer. Please let me know if you see a better way of
doing this :)

Thanks,

Adrien

[1] https://lore.kernel.org/all/[email protected]/


2022-05-06 08:11:47

by Adrien Thierry

[permalink] [raw]
Subject: Re: [PATCH 1/3] staging: vchiq_arm: add reference to vchiq device in vchiq_state

Hi Stefan,

> the patch looks good to me. It would be nice to mention that with adding
> the device reference it would possible to introduce common kernel logging.

Sure, I can mention this in the next version.

Thanks,

Adrien


2022-05-09 11:44:39

by Stefan Wahren

[permalink] [raw]
Subject: Re: [PATCH 2/3] staging: vchiq_arm: get rid of global device structure

Hi Adrien,

Am 05.05.22 um 20:11 schrieb Adrien Thierry:
> Hi Stefan,
>
> Thanks for your feedback.
>
>> i understand the motivation, but could you please explain more in detail
>> why you decided to add vchiq_instance instead of device reference? I
>> think vchiq_instance is a more internal structure which should be
>> avoided in kernel consumers like bcm2835-audio or mmal.
> I used the vchiq_instance instead of the device reference because in order
> to get rid of the vchiq_states array (patch 3/3 [1]), I needed another way
> to access the vchiq_state in the 'handle_to_service' function. So I passed
> the vchiq_instance to it (I could also have passed the state directly
> instead of the instance), and this propagated in the caller chain all the
> way up to 'vchiq_bulk_transmit' and friends which are used in the
> bcm2835-audio consumer.

Okay, in this case please add this explanation to the commit message.

Best regards

> Please let me know if you see a better way of
> doing this :)
>
> Thanks,
>
> Adrien
>
> [1] https://lore.kernel.org/all/[email protected]/
>