>-----Original Message----- >From: De Marchi, Lucas >Sent: Friday, June 14, 2019 2:37 PM >To: Srivatsa, Anusha <[email protected]> >Cc: [email protected]; Vivi, Rodrigo <[email protected]> >Subject: Re: [PATCH 9/9] drm/i915/dmc: protect against loading wrong firmware > >On Fri, Jun 14, 2019 at 12:44:50PM -0700, Anusha Srivatsa wrote: >> >> >>>-----Original Message----- >>>From: De Marchi, Lucas >>>Sent: Friday, June 7, 2019 2:13 AM >>>To: [email protected] >>>Cc: Vivi, Rodrigo <[email protected]>; Srivatsa, Anusha >>><[email protected]>; De Marchi, Lucas >>><[email protected]> >>>Subject: [PATCH 9/9] drm/i915/dmc: protect against loading wrong >>>firmware >>> >>>In intel_package_header version 2 there's a new field in the fw_info >>>table that must be 0, otherwise it's not the correct DMC firmware. Add >>>a check for version 2 or later. >>> >>>Signed-off-by: Lucas De Marchi <[email protected]> >>>--- >>> drivers/gpu/drm/i915/intel_csr.c | 14 +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >>> >>>diff --git a/drivers/gpu/drm/i915/intel_csr.c >>>b/drivers/gpu/drm/i915/intel_csr.c >>>index 7608e4e2950d..864531aae1a5 100644 >>>--- a/drivers/gpu/drm/i915/intel_csr.c >>>+++ b/drivers/gpu/drm/i915/intel_csr.c >>>@@ -120,7 +120,10 @@ struct intel_css_header { } __packed; >>> >>> struct intel_fw_info { >>>- u16 reserved1; >>>+ u8 reserved1; >>>+ >>>+ /* reserved on package_header version 1, must be 0 on version 2 */ >>>+ u8 dmc_id; >>> >>> /* Stepping (A, B, C, ..., *). * is a wildcard */ >>> char stepping; >>>@@ -325,12 +328,16 @@ void intel_csr_load_program(struct >>>drm_i915_private >>>*dev_priv) >>> */ >>> static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, >>> unsigned int num_entries, >>>- const struct stepping_info *si) >>>+ const struct stepping_info *si, >>>+ u8 package_ver) >>> { >>> u32 dmc_offset = CSR_DEFAULT_FW_OFFSET; >>> unsigned int i; >>> >>> for (i = 0; i < num_entries; i++) { >>>+ if (package_ver > 1 && fw_info[i].dmc_id != 0) >>>+ continue; >> >>Wont we need a message here? "Wrong DMC version, not loading v_x.0x" or >something?.." > >I don't think so. It's normal to have other blobs inside the firmware that we >don't >care about. At most I could put a debug, but I don't think we really care. > >As long as we find one we should be fine. And if we don't, then we will >complain >loudly later in this function with a DRM_ERROR(). >dmc_offset remaining as CSR_DEFAULT_FW_OFFSET has a double meaning here: >either we didn't find any entry or we found one that had the offset set to this >value.
Ok. Makes sense. Reviewed-by: Anusha Srivatsa <[email protected]> >Lucas De Marchi > >> >>Anusha >>> if (fw_info[i].substepping == '*' && >>> si->stepping == fw_info[i].stepping) { >>> dmc_offset = fw_info[i].offset; >>>@@ -508,7 +515,8 @@ parse_csr_fw_package(struct intel_csr *csr, >>> >>> fw_info = (const struct intel_fw_info *) >>> ((u8 *)package_header + sizeof(*package_header)); >>>- dmc_offset = find_dmc_fw_offset(fw_info, num_entries, si); >>>+ dmc_offset = find_dmc_fw_offset(fw_info, num_entries, si, >>>+ package_header->header_ver); >>> if (dmc_offset == CSR_DEFAULT_FW_OFFSET) { >>> DRM_ERROR("DMC firmware not supported for %c stepping\n", >>> si->stepping); >>>-- >>>2.21.0 >> _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
