Re: [PATCH v2 1/3] mhi: allow MHI client drivers to provide the firmware via a pointer

[Kalle Valo <kvalo@kernel.org>]

Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> writes:

> On Wed, Mar 08, 2023 at 05:25:20PM +0200, Kalle Valo wrote:
>> From: Kalle Valo <quic_kvalo@quicinc.com>
>> 
>
> Subject prefix should be: "bus: mhi: host: ..."

Will do.

>> @@ -392,9 +390,10 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>>  	const struct firmware *firmware = NULL;
>>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>  	const char *fw_name;
>> +	const u8 *fw_data;
>>  	void *buf;
>>  	dma_addr_t dma_addr;
>> -	size_t size;
>> +	size_t size, fw_sz;
>>  	int i, ret;
>>  
>>  	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
>> @@ -424,6 +423,14 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>>  	fw_name = (mhi_cntrl->ee == MHI_EE_EDL) ?
>>  		mhi_cntrl->edl_image : mhi_cntrl->fw_image;
>>  
>
> Can you please add a comment here?

Ok.

>
>> +	if (!fw_name && mhi_cntrl->fbc_download &&
>> +	    mhi_cntrl->fw_data && mhi_cntrl->fw_sz) {
>> +		size = mhi_cntrl->sbl_size;
>
> Don't you need to validate sbl_size?

Good point, I'll add that.

>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -299,6 +299,10 @@ struct mhi_controller_config {
>>   * @iova_start: IOMMU starting address for data (required)
>>   * @iova_stop: IOMMU stop address for data (required)
>>   * @fw_image: Firmware image name for normal booting (optional)
>> + * @fw_data: Firmware image data content for normal booting, used only
>> + *           if fw_image is NULL (optional)
>> + * @fw_sz: Firmware image data size for normal booting, used only if fw_image
>> + *         is NULL and fbc_download is true (optional)
>>   * @edl_image: Firmware image name for emergency download mode (optional)
>>   * @rddm_size: RAM dump size that host should allocate for debugging purpose
>>   * @sbl_size: SBL image size downloaded through BHIe (optional)
>> @@ -384,6 +388,8 @@ struct mhi_controller {
>>  	dma_addr_t iova_start;
>>  	dma_addr_t iova_stop;
>>  	const char *fw_image;
>> +	const u8 *fw_data;
>> +	size_t fw_sz;
>
> Even though these members are not creating holes now, shuffling the datatypes
> will create holes in the future. So I always prefer to keep the struct members
> sorted in the below order:
>
> pointer
> struct/union
> u64
> u32
> u16
> u8
> bool

I'm not sure what are suggesting here as struct mhi_controller is not
using that style. Are you saying that fw_sz should be after reg_len? So
something like this:

	const u8 *fw_data;
	const char *edl_image;
	size_t rddm_size;
	size_t sbl_size;
	size_t seg_len;
	size_t reg_len;
	size_t fw_sz;

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches