On Fri, May 26, 2023 at 02:45:46PM +0300, Kalle Valo wrote:
> Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> writes:
>
> > On Wed, Mar 08, 2023 at 05:25:20PM +0200, Kalle Valo wrote:
> >> From: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
> >>
> >
> > 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
--
மணிவண்ணன் சதாசிவம்
