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;
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
