On Mon, Aug 25, 2025 at 03:49:21PM +0530, Kishore Batta wrote: No "drivers: " prefix in subject please. > Add the Sahara training data structure and populate the DDR training data > sysfs node. Start with a problem description. > During device boot, the device performs DDR training and sends > the training data back to the host once complete. "The device"? All devices, some devices? > The host exposes this > data to userspace via the sysfs interface. I've read three sentences, you've given me breadcrumbs of what's going on...but you're forcing me to guess how these three things fit together and what you're trying to achieve... > The "ddr_training_data" sysfs > node will be present in the MHI controller node (e.g., mhi0, mhi1) along > with other existing sysfs nodes at /sys/bus/mhi/devices/mhi*/. > > When the host reboots, a userspace service is triggered via an udev rule to > read the training data from the sysfs entry. This describes one possible way to do that, but it's not the job of the kernel to dictate how this is implemented. You should describe the expected work to be performed by userspace, and you may suggest how that could be implemented. > The service then copies the > valid training data to the image firmware filesystem. This sentence doesn't make sense to a general Linux user, because there's no separate "image firmware filesystem". > This change includes > the structures added for DDR training data and embeds them in the > sahara_dev_driver_data structure. It also populates the sysfs attributes > for DDR training data. This half of the paragraph isn't directly related to the implementation of the user space service, so better split it out in a paragraph of its own. > > Userspace service - https://github.com/qualcomm/csm-utils > > Signed-off-by: Kishore Batta <kishore.batta@xxxxxxxxxxxxxxxx> > --- > drivers/soc/qcom/sahara/sahara.c | 109 ++++++++++++++++++++++++++++++- > 1 file changed, 108 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c > index b07f6bd0e19f..df103473af3a 100644 > --- a/drivers/soc/qcom/sahara/sahara.c > +++ b/drivers/soc/qcom/sahara/sahara.c > @@ -60,6 +60,12 @@ > #define SAHARA_MEM_DEBUG64_LENGTH 0x18 > #define SAHARA_MEM_READ64_LENGTH 0x18 > > +struct sahara_dev_trng_data { trng - "True Random Number Generator"? > + void *trng_data; > + u64 trng_data_sz; > + bool receiving_trng_data; > +}; > + > struct sahara_packet { > __le32 cmd; > __le32 length; > @@ -177,6 +183,58 @@ struct sahara_context { > bool is_mem_dump_mode; > }; > > +struct sahara_dev_driver_data { > + struct bin_attribute ddr_attr; > + struct sahara_dev_trng_data *sdev; "sdev" as an abbreviation for "sahara device training data"? I would have guessed it related tot he "sahara device driver data"... Why do you have two separate structs for these? > + struct sahara_context *context; > +}; > + > +/* Exposes DDR training data via sysfs binary attribute for user-space access */ > +static ssize_t ddr_training_read(struct file *filp, struct kobject *kobj, > + const struct bin_attribute *attr, char *buf, > + loff_t offset, size_t count) > +{ > + struct sahara_dev_driver_data *sahara_data; > + struct sahara_dev_trng_data *sdev; > + size_t data_size; > + > + sahara_data = container_of(attr, struct sahara_dev_driver_data, ddr_attr); > + > + if (!sahara_data) > + return -EIO; > + > + sdev = sahara_data->sdev; > + > + if (!sdev || !sdev->trng_data) This isn't assigned anywhere... > + return -EIO; > + > + data_size = attr->size; > + > + if (data_size == 0) > + return 0; > + > + if (offset >= data_size) > + return -EINVAL; > + > + if (count > data_size - offset) > + count = data_size - offset; > + > + /* Copy the training data into the buffer */ > + memcpy(buf, (sdev->trng_data + offset), count); > + > + /* Free memory after last read */ > + if (offset + count >= data_size) { > + kfree(sdev->trng_data); > + sdev->trng_data = NULL; > + kfree(sdev); Allowing the data to be read only one time and then failing subsequent reads is going to be confusing to people. Imagine debugging this and depending on how fast you can hexdump the attribute you either break the userspace thing or you are unable to catch the data. > + sdev = NULL; > + kfree(sahara_data); But you did device_create_bin_file() on &sahara_data->ddr_attr...what happens now? > + sahara_data = NULL; > + } > + > + return count; > +} > + > static int sahara_find_image(struct sahara_context *context, u32 image_id) > { > int ret; > @@ -703,11 +761,42 @@ static void sahara_dump_processing(struct work_struct *work) > sahara_send_reset(context); > } > > +static int sahara_dev_populate_attribute(struct sahara_dev_driver_data *sahara_data) > +{ > + int ret; > + struct sahara_context *context = sahara_data->context; > + > + /* DDR training data attribute */ > + sahara_data->ddr_attr.attr.name = "ddr_training_data"; > + sahara_data->ddr_attr.attr.mode = 0444; > + sahara_data->ddr_attr.read = ddr_training_read; > + > + /* Size is populated during device bootup */ Where? In some other patch? > + sahara_data->ddr_attr.size = 0; > + sahara_data->ddr_attr.write = NULL; > + > + /* > + * Remove any existing sysfs binary attribute to avoid stale entries > + * during warm boot or reinitialization. This ensures clean state before > + * re-creating the attribute. But why do you need to recreate it? What is the life cycle of this file and how does it conflict with the life cycle of the sahara MHI device? > + */ > + device_remove_bin_file(&context->mhi_dev->mhi_cntrl->mhi_dev->dev, > + &sahara_data->ddr_attr); > + > + /* Create the binary attribute */ > + ret = device_create_bin_file(&context->mhi_dev->mhi_cntrl->mhi_dev->dev, > + &sahara_data->ddr_attr); > + > + return ret; > +} > + > static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id) > { > struct sahara_context *context; > int ret; > int i; > + struct sahara_dev_driver_data *sahara_data; > + struct sahara_dev_trng_data *sdev; This had a nice reverse xmas tree feel to it...perhaps you can keep that? > > context = devm_kzalloc(&mhi_dev->dev, sizeof(*context), GFP_KERNEL); > if (!context) > @@ -717,6 +806,17 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_ > if (!context->rx) > return -ENOMEM; > > + sahara_data = kzalloc(sizeof(*sahara_data), GFP_KERNEL); For the case where userspace doesn't read the DDR training data (e.g. because the particular device doesn't implement/need that), where is this freed? > + if (!sahara_data) > + return -ENOMEM; > + > + sdev = kzalloc(sizeof(*sdev), GFP_KERNEL); > + if (!sdev) > + return -ENOMEM; > + > + sahara_data->context = context; > + sahara_data->sdev = sdev; > + > /* > * AIC100 defines SAHARA_TRANSFER_MAX_SIZE as the largest value it > * will request for READ_DATA. This is larger than > @@ -749,7 +849,14 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_ > return -EINVAL; > > context->active_image_id = SAHARA_IMAGE_ID_NONE; > - dev_set_drvdata(&mhi_dev->dev, context); > + dev_set_drvdata(&mhi_dev->dev, sahara_data); sahara_mhi_remove and sahara_mhi_dl_xfer_cb still assumes that drvdata is of type struct sahara_context. Regards, Bjorn > + > + ret = sahara_dev_populate_attribute(sahara_data); > + if (ret) { > + dev_err(&context->mhi_dev->dev, > + "Failed to populate bin attribute: %d\n", ret); > + return ret; > + } > > ret = mhi_prepare_for_transfer(mhi_dev); > if (ret) > -- > 2.34.1 >
