Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> writes:
> 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.
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
