On Mon, Aug 25, 2025 at 03:49:17PM +0530, Kishore Batta wrote:
Just noticed that your recipients list doesn't match get_maintainers.
Please adopt b4 and let it choose recipients for you.
And same subject prefix issues as all the patches.
> Support the registration of image tables in the Sahara driver. Each
> Qualcomm device can define its own image table, and client drivers can
> register their image tables with the Sahara protocol. The Sahara protocol
> driver now exposes the necessary APIs to facilitate image table
> registration and de-registration. These image tables are used by Sahara
> to transfer images from the host filesystem to the device.
>
> Signed-off-by: Kishore Batta <kishore.batta@xxxxxxxxxxxxxxxx>
> ---
> drivers/accel/qaic/Makefile | 3 +-
> drivers/accel/qaic/sahara_image_table.c | 173 ++++++++++++++++++++
> drivers/accel/qaic/sahara_image_table_ops.h | 102 ++++++++++++
> 3 files changed, 277 insertions(+), 1 deletion(-)
> create mode 100644 drivers/accel/qaic/sahara_image_table.c
> create mode 100644 drivers/accel/qaic/sahara_image_table_ops.h
>
> diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
> index 1106b876f737..586a6877e568 100644
> --- a/drivers/accel/qaic/Makefile
> +++ b/drivers/accel/qaic/Makefile
> @@ -12,6 +12,7 @@ qaic-y := \
> qaic_drv.o \
> qaic_ras.o \
> qaic_timesync.o \
> - sahara.o
> + sahara.o \
> + sahara_image_table.o
>
> qaic-$(CONFIG_DEBUG_FS) += qaic_debugfs.o
> diff --git a/drivers/accel/qaic/sahara_image_table.c b/drivers/accel/qaic/sahara_image_table.c
> new file mode 100644
> index 000000000000..dd0793a33727
> --- /dev/null
> +++ b/drivers/accel/qaic/sahara_image_table.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +
> +#include "sahara_image_table_ops.h"
> +
> +struct sahara_image_table_context {
> + struct list_head provider_list;
> + /* Protects access to provider_list and related operations */
> + struct mutex provider_mutex;
> +};
Drop this struct and turn the two members global variables.
> +
> +static struct sahara_image_table_context sahara_img_ctx = {
> + .provider_list = LIST_HEAD_INIT(sahara_img_ctx.provider_list),
> + .provider_mutex = __MUTEX_INITIALIZER(sahara_img_ctx.provider_mutex),
> +};
> +
> +/**
> + * sahara_register_image_table_provider - Register an image table provider.
https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
says you should put () after the function name.
> + * @provider: Pointer to the sahara_image_table_provider structure to be
> + * registered.
> + *
> + * This function validates the provided sahara_image_table_provider structure
> + * and adds it to the global list of image table providers.
What is the key thing this function does? It validates the
image_table_provider! And then second to that it might add it to some
list...
> The list is
> + * protected by a mutex to ensure thread-safe operations.
https://docs.kernel.org/doc-guide/kernel-doc.html#function-context
> + *
> + * Return: 0 on success, -EINVAL if the provider or its required fields are
> + * invalid.
> + */
> +int sahara_register_image_table_provider(struct sahara_image_table_provider
> + *provider)
> +{
> + /* Validate the provider and its required fields */
> + if (!provider || !provider->image_table || !provider->dev_name)
> + return -EINVAL;
> +
> + /* Acquire the mutex before modifying the list */
If that isn't obvious from the line mutex_lock(something) consider
giving the lock a better name.
It's obvious what this sequence does
lock()
modify(list)
unlock()
Document things that aren't obvious.
> + mutex_lock(&sahara_img_ctx.provider_mutex);
> +
> + /* Add the provider to the list */
> + list_add(&provider->list, &sahara_img_ctx.provider_list);
> +
> + /* Release the mutex after modification */
> + mutex_unlock(&sahara_img_ctx.provider_mutex);
> +
> + return 0;
> +}
> +
> +/**
> + * sahara_get_image_table - Get the image table for a given device name
> + * @dev_name: The name of the device for which the image table is requested.
> + *
> + * This function iterates through the list of registered image table providers
> + * and returns the image table for the provider matching the given device name.
> + *
> + * Return: A pointer to the image table if found, or NULL if no matching
> + * provider is found.
> + */
> +const char * const *sahara_get_image_table(const char *dev_name)
> +{
> + struct sahara_image_table_provider *provider;
> +
> + /* Validate the device name */
> + if (!dev_name) {
> + pr_debug("sahara: Invalid argument %s\n", dev_name);
> + return NULL;
> + }
This is overly defensive. You're writing the only code that should ever
call this function, just make sure you don't explicitly pass NULL when
you do...
> +
> + /* Iterate through the list to find the matching provider */
> + list_for_each_entry(provider, &sahara_img_ctx.provider_list, list) {
> + if (strcmp(provider->dev_name, dev_name) == 0)
> + return provider->image_table;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * sahara_get_image_table_size - Get the size of the image table for a given
> + * device name.
> + * @dev_name: The name of the device for which the image table size is requested
> + *
> + * This function iterates through the list of registered image table providers
> + * and returns the size of the image table for the provider matching the given
> + * device name.
> + *
> + * Return: The size of the image table if found, or 0 if no matching provider
> + * is found or if the device name is invalid.
> + */
> +size_t sahara_get_image_table_size(const char *dev_name)
You don't need two identical functions for getting the table and its
size, just add a "size_t *size" parameter to sahara_get_image_table()
and return both values in one - saves you 29 lines of ~copy-pasta.
> +{
> + struct sahara_image_table_provider *provider;
> +
> + /* Validate the dev name */
> + if (!dev_name) {
> + pr_debug("sahara: Invalid argument %s\n", dev_name);
> + return 0;
> + }
> +
> + /* Iterate through the list to find the matching provider */
> + list_for_each_entry(provider, &sahara_img_ctx.provider_list, list) {
> + if (strcmp(provider->dev_name, dev_name) == 0)
> + return provider->image_table_size;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * sahara_unregister_image_table_provider - Unregister an image table provider.
> + * @provider: Pointer to the sahara_image_table_provider structure to be
> + * unregistered
> + *
> + * This function validates the provided sahara_image_table_provider structure
> + * and removes it from the global list of image table providers. The list is
> + * protected by a mutex to ensure thread-safe operations.
> + *
> + * Return: 0 on success, -EINVAL if the provider is invalid.
> + */
> +int sahara_unregister_image_table_provider(struct sahara_image_table_provider
> + *provider)
unregister functions typically return void, because there isn't much
useful one can do if it fails.
> +{
> + /* Validate the provider */
> + if (!provider)
> + return -EINVAL;
This doesn't really check that the point is valid, just that it's not
NULL. And per the intended usage, that can never happen. So I'd suggest
dropping this check.
> +
> + /* Acquire the mutex before modifying the list */
> + mutex_lock(&sahara_img_ctx.provider_mutex);
> +
> + /* Remove the provider from the list */
> + list_del(&provider->list);
> +
> + /* Release the mutex after modification */
> + mutex_unlock(&sahara_img_ctx.provider_mutex);
> +
> + return 0;
> +}
> +
> +/**
> + * sahara_get_fw_folder_name - Retrieve the firmware folder name for a given
> + * device
> + * @dev_name: Name of the device for which the firmware folder name is requested
> + *
> + * This function searches through the list of Sahara image table providers to
> + * find the provider matching the given device name. If a matching provider is
> + * found, the firmware folder name associated with that provider is returned.
> + * If the device name is invalid or no matching provider is found, the function
> + * returns NULL.
> + *
> + * Return: Firmware folder name if found, otherwise NULL.
> + */
> +char *sahara_get_fw_folder_name(const char *dev_name)
> +{
> + struct sahara_image_table_provider *provider;
> +
> + /* Validate the device name */
> + if (!dev_name) {
> + pr_debug("sahara: Invalid argument %s\n", dev_name);
> + return NULL;
> + }
> +
> + /* Iterate through the list to find the matching provider */
> + list_for_each_entry(provider, &sahara_img_ctx.provider_list, list) {
> + if (strcmp(provider->dev_name, dev_name) == 0)
> + return provider->fw_folder_name;
> + }
> +
> + return NULL;
> +}
> diff --git a/drivers/accel/qaic/sahara_image_table_ops.h b/drivers/accel/qaic/sahara_image_table_ops.h
> new file mode 100644
> index 000000000000..f8496bd1aa35
> --- /dev/null
> +++ b/drivers/accel/qaic/sahara_image_table_ops.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
> +
> +#ifndef __SAHARA_IMAGE_TABLE_OPS_H__
> +#define __SAHARA_IMAGE_TABLE_OPS_H__
> +
> +#include <linux/list.h>
> +
> +/**
> + * struct sahara_image_table_provider - Structure representing an image table
> + * provider.
> + * @image_table: Pointer to the image table
> + * @image_table_size: Size of the image table
> + * @dev_name: Device name to identify the provider
> + * @fw_folder_name: Name of the folder where the image binaries exist.
> + * @list: List head for linking providers in a list
> + *
> + * This structure is used to represent an image table provider in the Sahara
> + * driver. It contains a pointer to the image table, the size of the image
> + * table, the device name for identifying the provider, and a list head for
> + * linking providers in a linked list.
> + */
> +struct sahara_image_table_provider {
> + const char * const *image_table;
> + size_t image_table_size;
> + const char *dev_name;
> + char *fw_folder_name;
> + struct list_head list;
> +};
> +
> +/**
> + * sahara_register_image_table_provider - Register an image table provider.
You already provide kernel-doc in the implementation, no need to
duplicate it also in the header file.
Regards,
Bjorn
> + * @provider: Pointer to the sahara_image_table_provider structure to be
> + * registered.
> + *
> + * This function validates the provided sahara_image_table_provider structure
> + * and adds it to the global list of image table providers. The list is
> + * protected by a mutex to ensure thread-safe operations.
> + *
> + * Return: 0 on success, -EINVAL if the provider or its required fields are
> + * invalid.
> + */
> +int sahara_register_image_table_provider(struct sahara_image_table_provider
> + *provider);
> +
> +/**
> + * sahara_get_image_table - Get the image table for a given device name
> + * @dev_name: The name of the device for which the image table is requested.
> + *
> + * This function iterates through the list of registered image table providers
> + * and returns the image table for the provider matching the given device name.
> + *
> + * Return: A pointer to the image table if found, or NULL if no matching
> + * provider is found.
> + */
> +const char * const *sahara_get_image_table(const char *dev_name);
> +
> +/**
> + * sahara_get_image_table_size - Get the size of the image table for a given
> + * device name.
> + * @dev_name: The name of the device for which the image table size is requested
> + *
> + * This function iterates through the list of registered image table providers
> + * and returns the size of the image table for the provider matching the given
> + * device name.
> + *
> + * Return: The size of the image table if found, or 0 if no matching provider
> + * is found or if the device name is invalid.
> + */
> +size_t sahara_get_image_table_size(const char *dev_name);
> +
> +/**
> + * sahara_unregister_image_table_provider - Unregister an image table provider.
> + * @provider: Pointer to the sahara_image_table_provider structure to be
> + * unregistered
> + *
> + * This function validates the provided sahara_image_table_provider structure
> + * and removes it from the global list of image table providers. The list is
> + * protected by a mutex to ensure thread-safe operations.
> + *
> + * Return: 0 on success, -EINVAL if the provider is invalid.
> + */
> +int sahara_unregister_image_table_provider(struct sahara_image_table_provider
> + *provider);
> +
> +/**
> + * sahara_get_fw_folder_name - Retrieve the firmware folder name for a given
> + * device
> + * @dev_name: Name of the device for which the firmware folder name is requested
> + *
> + * This function searches through the list of Sahara image table providers to
> + * find the provider matching the given device name. If a matching provider is
> + * found, the firmware folder name associated with that provider is returned.
> + * If the device name is invalid or no matching provider is found, the function
> + * returns NULL.
> + *
> + * Return: Firmware folder name if found, otherwise NULL.
> + */
> +char *sahara_get_fw_folder_name(const char *dev_name);
> +
> +#endif // __SAHARA_IMAGE_TABLE_OPS_H__
> --
> 2.34.1
>
