I agree with Steffan's concerns below. Please fix or explain.
On 03/13/2013 05:29 AM, Steffen Maier wrote:
> On 03/12/2013 05:39 AM, jayamohank@xxxxxxxxx wrote:
>> From: "Jayamohan.Kallickal" <jayamohan.kallickal@xxxxxxxxxx>
>>
>> This patch fixes the display of proper FW Version from the driver.
>>
>> Signed-off-by: John Soni Jose <sony.john-n@xxxxxxxxxx>
>> Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal@xxxxxxxxxx>
>> ---
>> drivers/scsi/be2iscsi/be_main.c | 2 ++
>> drivers/scsi/be2iscsi/be_main.h | 2 ++
>> drivers/scsi/be2iscsi/be_mgmt.c | 21 +++++++++++++++++++++
>> drivers/scsi/be2iscsi/be_mgmt.h | 32 ++++++++++++++++++--------------
>> 4 files changed, 43 insertions(+), 14 deletions(-)
>
>> diff --git a/drivers/scsi/be2iscsi/be_mgmt.h
>> b/drivers/scsi/be2iscsi/be_mgmt.h
>> index 2e4968a..0a406a4 100644
>> --- a/drivers/scsi/be2iscsi/be_mgmt.h
>> +++ b/drivers/scsi/be2iscsi/be_mgmt.h
>> @@ -156,25 +156,25 @@ union invalidate_commands_params {
>> } __packed;
>>
>> struct mgmt_hba_attributes {
>> - u8 flashrom_version_string[32];
>> - u8 manufacturer_name[32];
>> + u8 flashrom_version_string[BEISCSI_VER_STRLEN];
>> + u8 manufacturer_name[BEISCSI_VER_STRLEN];
>> u32 supported_modes;
>> u8 seeprom_version_lo;
>> u8 seeprom_version_hi;
>> u8 rsvd0[2];
>> - u32 fw_cmd_data_struct_version;
>> + u32 ioctl_data_struct_version;
>> u32 ep_fw_data_struct_version;
>> - u32 future_reserved[12];
>> + u8 ncsi_version_string[12];
>
> Hm, this seems to replace 12*u32 by 12*u8, i.e. the subsequent fields
> would now have a reduced offset. Is this intentional? I would have
> expected to just use some part of future_reserved for newly defined
> fields and have the remainder of future_reserved still part of the
> structure to not shift the offset of subsequent fields; just like you
> did below in struct mgmt_hba_attributes.
>
>> u32 default_extended_timeout;
>> - u8 controller_model_number[32];
>> + u8 controller_model_number[BEISCSI_VER_STRLEN];
>> u8 controller_description[64];
>> - u8 controller_serial_number[32];
>> - u8 ip_version_string[32];
>> - u8 firmware_version_string[32];
>> - u8 bios_version_string[32];
>> - u8 redboot_version_string[32];
>> - u8 driver_version_string[32];
>> - u8 fw_on_flash_version_string[32];
>> + u8 controller_serial_number[BEISCSI_VER_STRLEN];
>> + u8 ip_version_string[BEISCSI_VER_STRLEN];
>> + u8 firmware_version_string[BEISCSI_VER_STRLEN];
>> + u8 bios_version_string[BEISCSI_VER_STRLEN];
>> + u8 redboot_version_string[BEISCSI_VER_STRLEN];
>> + u8 driver_version_string[BEISCSI_VER_STRLEN];
>> + u8 fw_on_flash_version_string[BEISCSI_VER_STRLEN];
>> u32 functionalities_supported;
>> u16 max_cdblength;
>> u8 asic_revision;
>> @@ -190,7 +190,8 @@ struct mgmt_hba_attributes {
>> u32 firmware_post_status;
>> u32 hba_mtu[8];
>> u8 iscsi_features;
>> - u8 future_u8[3];
>> + u8 asic_generation;
>> + u8 future_u8[2];
>> u32 future_u32[3];
>> } __packed;
>
>> @@ -207,7 +208,7 @@ struct mgmt_controller_attributes {
>> u64 unique_identifier;
>> u8 netfilters;
>> u8 rsvd0[3];
>> - u8 future_u32[4];
>> + u32 future_u32[4];
>> } __packed;
>
> While I suppose this doesn't break existing functionality, is it
> intentional to actually increase the size of the trailing reserved
> fields and thus the size of the entire struct mgmt_controller_attributes?
>
> Steffen
>
> Linux on System z Development
>
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
