On Mon, Aug 25, 2025 at 03:49:25PM +0530, Kishore Batta wrote:
> Load the saved DDR training data during subsequent bootups. The image ID 34
> is for DDR training data image. During subsequent bootups, the Sahara
> driver needs to load the training data file associated with the serial
> number instead of the default file present in the image table. If the
> serial number-specific file is not present in the firmware directory,
> it indicates the first bootup of the device, and training data gets
> generated.
As with your other commit messages, you start by stating what the patch
does and they you add layer after layer with explanation on top of that.
Rewrite this one as well to clearly describe what problem the patch is
trying to solve, why the firmware loader is invoked here and what
expectations this has on user space.
>
> Signed-off-by: Kishore Batta <kishore.batta@xxxxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/sahara/sahara.c | 71 +++++++++++++++++++++++++++-----
> 1 file changed, 60 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
> index 3887cdcfe256..28e52a9974a1 100644
> --- a/drivers/soc/qcom/sahara/sahara.c
> +++ b/drivers/soc/qcom/sahara/sahara.c
> @@ -68,6 +68,7 @@
> #define SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST 0x8
> #define SAHARA_EXEC_CMD_GET_TRAINING_DATA 0x9
> #define SAHARA_NUM_CMD_BUF SAHARA_NUM_TX_BUF
> +#define SAHARA_DDR_TRAINING_IMG_ID 34
>
> struct sahara_dev_trng_data {
> void *trng_data;
> @@ -213,6 +214,8 @@ struct sahara_context {
> enum sahara_mode current_mode;
> char *cmd_buff[SAHARA_NUM_CMD_BUF];
> u64 bytes_copied;
> + u32 serial_num;
> + char *fw_folder_name;
Both of these are assigned and used solely within that one block below,
and they are 12 bytes. Can they not be put on the stack?
> };
>
> struct sahara_dev_driver_data {
> @@ -270,6 +273,7 @@ static ssize_t ddr_training_read(struct file *filp, struct kobject *kobj,
> static int sahara_find_image(struct sahara_context *context, u32 image_id)
> {
> int ret;
> + char *fw_path;
Reverse xmas-tree please.
>
> if (image_id == context->active_image_id)
> return 0;
> @@ -286,19 +290,64 @@ static int sahara_find_image(struct sahara_context *context, u32 image_id)
> }
>
> /*
> - * This image might be optional. The device may continue without it.
> - * Only the device knows. Suppress error messages that could suggest an
> - * a problem when we were actually able to continue.
> + * Image ID 34 corresponds to DDR training data. On subsequent
> + * bootups, the sahara driver may need to load the training data file
> + * associated with device's serial number instead of the default file
> + * listed in the image table.
> + *
> + * If the serial number specific file is not found in the firmware
> + * directory, it likely indicates the device is booting for the first
> + * time, and new training data will be generated.
> */
This comment only relates to the first block below. How about moving it
into the block to make that clear - and to keep symmetry with the other
comment that you moved into the its block?
> - ret = firmware_request_nowarn(&context->firmware,
> - context->image_table[image_id],
> - &context->mhi_dev->dev);
> - if (ret) {
> - dev_dbg(&context->mhi_dev->dev, "request for image id %d / file %s failed %d\n",
> - image_id, context->image_table[image_id], ret);
> - return ret;
> - }
> + if (image_id == SAHARA_DDR_TRAINING_IMG_ID) {
> + context->serial_num = context->mhi_dev->mhi_cntrl->serial_number;
> + context->fw_folder_name =
> + sahara_get_fw_folder_name(context->mhi_dev->mhi_cntrl->name);
Use a local variable for "name" to shorted the line, such that you don't
need to wrap it weirdly.
> + if (!context->fw_folder_name)
> + return -EINVAL;
> +
> + fw_path = devm_kasprintf(&context->mhi_dev->dev, GFP_KERNEL,
fw_path is used for 3 lines, but you're keeping this allocation for an
undefined amount of time.
Regards,
Bjorn
> + "qcom/%s/mdmddr_0x%x.mbn",
> + context->fw_folder_name,
> + context->serial_num);
> +
> + if (!fw_path)
> + return -ENOMEM;
> +
> + ret = firmware_request_nowarn(&context->firmware,
> + fw_path,
> + &context->mhi_dev->dev);
> +
> + /*
> + * If there is failure to load serial number specific file, load
> + * the default file from image table
> + */
> + if (ret) {
> + ret = firmware_request_nowarn(&context->firmware,
> + context->image_table[image_id],
> + &context->mhi_dev->dev);
> + if (ret) {
> + dev_dbg(&context->mhi_dev->dev, "request for image id %d / file %s failed %d\n",
> + image_id, context->image_table[image_id], ret);
> + }
> + return ret;
> + }
> + } else {
> + /*
> + * This image might be optional. The device may continue without it.
> + * Only the device knows. Suppress error messages that could suggest an
> + * a problem when we were actually able to continue.
> + */
> + ret = firmware_request_nowarn(&context->firmware,
> + context->image_table[image_id],
> + &context->mhi_dev->dev);
> + if (ret) {
> + dev_dbg(&context->mhi_dev->dev, "request for image id %d / file %s failed %d\n",
> + image_id, context->image_table[image_id], ret);
>
> + return ret;
> + }
> + }
> context->active_image_id = image_id;
>
> return 0;
> --
> 2.34.1
>
