Re: [PATCH v1 07/12] drivers: soc: qcom: Add sysfs support for DDR training data in Sahara.

[Kishore Batta <kishore.batta@oss.qualcomm.com>]

On 8/26/2025 4:07 AM, Bjorn Andersson wrote:
> 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.
>
ACK. Rewritten the commit message in v2.
>> Userspace service - https://github.com/qualcomm/csm-utils
>>
>> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
>> ---
>>   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"?
Sorry for the confusion. I was referring to sahara device training data. 
In v2, I have used a different structure and naming conventions are 
properly used.
>> +	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?
ACK. In v2, i have removed them.
>> +	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...
ACK. In v2, i have rearranged the patches such that the changes are clean.
>> +		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.
ACK.
>> +		sdev = NULL;
>> +		kfree(sahara_data);
> But you did device_create_bin_file() on &sahara_data->ddr_attr...what
> happens now?
ACK. Handled properly in v2.
>> +		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?
Its during runtime of the device. Handled properly in v2 patchset so 
that the assignments are in single 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?
ACK. Fixed properly in v2 by using controller scoped lifetime and device 
managed resources.
>> +	 */
>> +	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?
ACK. Fixed in v2.
>
>>   
>>   	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?
ACK. Memory allocations and de-allocations are now properly handled in v2.
>> +	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
ACK. Fixed in v2.
>> +
>> +	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
>>