From: Mike Christie <michaelc@cs.wisc.edu>
To: Steffen Maier <maier@linux.vnet.ibm.com>
Cc: jayamohank@gmail.com, james.bottomley@hansenpartnership.com,
linux-scsi@vger.kernel.org,
"Jayamohan.Kallickal" <jayamohan.kallickal@emulex.com>,
John Soni Jose <sony.john-n@emulex.com>
Subject: Re: [PATCH 08/15] be2iscsi: Fix displaying the FW Version from driver.
Date: Sat, 23 Mar 2013 01:00:47 -0500 [thread overview]
Message-ID: <514D450F.9000801@cs.wisc.edu> (raw)
In-Reply-To: <51405526.1040203@linux.vnet.ibm.com>
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@gmail.com wrote:
>> From: "Jayamohan.Kallickal" <jayamohan.kallickal@emulex.com>
>>
>> This patch fixes the display of proper FW Version from the driver.
>>
>> Signed-off-by: John Soni Jose <sony.john-n@emulex.com>
>> Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal@emulex.com>
>> ---
>> 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@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2013-03-23 6:00 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-12 4:39 [PATCH 01/15] be2iscsi: Fix lack of uninitialize pattern to FW jayamohank
2013-03-12 4:39 ` [PATCH 02/15] be2iscsi: Fix returning Failure when MBX fails with Insufficient buffer error jayamohank
2013-03-12 4:39 ` [PATCH 03/15] be2iscsi: Fix MBX Command issues jayamohank
2013-03-12 4:39 ` [PATCH 04/15] be2iscsi: Fix MSIx support in SKH-R to 32 jayamohank
2013-03-12 4:39 ` [PATCH 05/15] be2iscsi: Fix freeing CXN specific driver resources jayamohank
2013-03-23 5:56 ` Mike Christie
2013-03-12 4:39 ` [PATCH 06/15] be2iscsi: Fix MACRO for checking the adapter type jayamohank
2013-03-12 4:39 ` [PATCH 07/15] be2iscsi: Fix support for DEFQ extension jayamohank
2013-03-12 4:39 ` [PATCH 08/15] be2iscsi: Fix displaying the FW Version from driver jayamohank
2013-03-13 10:29 ` Steffen Maier
2013-03-23 6:00 ` Mike Christie [this message]
2013-03-27 20:19 ` Kallickal, Jayamohan
2013-03-12 4:39 ` [PATCH 09/15] be2iscsi: Fix displaying the Active Session Count " jayamohank
2013-03-12 4:39 ` [PATCH 10/15] be2iscsi: Fix the Port Link Status issue jayamohank
2013-03-12 4:39 ` [PATCH 11/15] be2iscsi: Fix the NOP-In handling code path jayamohank
2013-03-12 4:39 ` [PATCH 12/15] be2iscsi: Fix dynamic CID allocation Mechanism in driver jayamohank
2013-03-12 4:39 ` [PATCH 13/15] be2iscsi: Fix checking Adapter state while establishing CXN jayamohank
2013-03-12 4:39 ` [PATCH 14/15] be2iscsi: Fix the copyright information jayamohank
2013-03-12 4:39 ` [PATCH 15/15] be2iscsi: Bump the driver version jayamohank
2013-03-12 4:39 ` be2iscsi: set of patches for be2iscsi jayamohank
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=514D450F.9000801@cs.wisc.edu \
--to=michaelc@cs.wisc.edu \
--cc=james.bottomley@hansenpartnership.com \
--cc=jayamohan.kallickal@emulex.com \
--cc=jayamohank@gmail.com \
--cc=linux-scsi@vger.kernel.org \
--cc=maier@linux.vnet.ibm.com \
--cc=sony.john-n@emulex.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.