-----Original Message----- From: Martin Wilck [mailto:mwilck@xxxxxxxx] Subject: Re: [PATCH V3 08/25] smartpqi: add support for long firmware version > @@ -1405,7 +1405,7 @@ enum pqi_ctrl_mode { struct > bmic_identify_controller { > u8 configured_logical_drive_count; > __le32 configuration_signature; > - u8 firmware_version[4]; > + u8 firmware_version_short[4]; > u8 reserved[145]; > __le16 extended_logical_unit_count; > u8 reserved1[34]; > @@ -1413,11 +1413,17 @@ struct bmic_identify_controller { > u8 reserved2[8]; > u8 vendor_id[8]; > u8 product_id[16]; > - u8 reserved3[68]; > + u8 reserved3[62]; > + __le32 extra_controller_flags; > + u8 reserved4[2]; > u8 controller_mode; > - u8 reserved4[32]; > + u8 spare_part_number[32]; > + u8 firmware_version_long[32]; > }; > > --- a/drivers/scsi/smartpqi/smartpqi_init.c > +++ b/drivers/scsi/smartpqi/smartpqi_init.c > pqi_get_ctrl_product_details(struct pqi_ctrl_info *ctrl_info) > if (rc) > goto out; > > + if (get_unaligned_le32(&identify->extra_controller_flags) & > + BMIC_IDENTIFY_EXTRA_FLAGS_LONG_FW_VERSION_SUPPORTED) > { > + memcpy(ctrl_info->firmware_version, > + identify->firmware_version_long, > + sizeof(identify->firmware_version_long)); > + } else { > + memcpy(ctrl_info->firmware_version, > + identify->firmware_version_short, > + sizeof(identify->firmware_version_short)); > + ctrl_info->firmware_version > + [sizeof(identify->firmware_version_short)] = > '\0'; > + snprintf(ctrl_info->firmware_version + > + strlen(ctrl_info->firmware_version), > + sizeof(ctrl_info->firmware_version), This looks wrong. I suppose a real overflow can't happen, but shouldn't it rather be written like this? snprintf(ctrl_info->firmware_version + sizeof(identify->firmware_version_short), sizeof(ctrl_info->firmware_version) - sizeof(identify->firmware_version_short), "-u", ...) > + "-%u", > + get_unaligned_le16(&identify- > > firmware_build_number)); Don: Agreed. Updated. Thanks for your revew.
