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 Fri, May 26, 2023 at 02:45:46PM +0300, Kalle Valo wrote:
>
>> 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;
>> 
>
> Ah, I thought I already sorted them up but apparently not. Keep it as it is,
> I'll sort this and other structs later.

Thanks, I'll then keep it as is and send v3 soon.

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

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