Re: [PATCH 08/15] be2iscsi: Fix displaying the FW Version from driver.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux