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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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.

- Mani

> -- 
> https://patchwork.kernel.org/project/linux-wireless/list/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

-- 
மணிவண்ணன் சதாசிவம்