| Message ID | 20200123042345.23886-7-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | qla2xxx patches for kernel v5.6 | expand |
On Wed, Jan 22, 2020 at 08:23:45PM -0800, Bart Van Assche wrote: > Make sure that sparse doesn't complain about the statements that load or > store the port attribute max_frame_size member. The port attribute data > structures represent FC protocol data and hence use big endian format. > This patch only changes the meaning of two ql_dbg() statements. > > Cc: Quinn Tran <qutran@marvell.com> > Cc: Martin Wilck <mwilck@suse.com> > Cc: Daniel Wagner <dwagner@suse.de> > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Daniel Wagner <dwagner@suse.de?
On Wed, Jan 22, 2020 at 08:23:45PM -0800, Bart Van Assche wrote: > Make sure that sparse doesn't complain about the statements that load or > store the port attribute max_frame_size member. The port attribute data > structures represent FC protocol data and hence use big endian format. > This patch only changes the meaning of two ql_dbg() statements. > > Cc: Quinn Tran <qutran@marvell.com> > Cc: Martin Wilck <mwilck@suse.com> > Cc: Daniel Wagner <dwagner@suse.de> > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/qla2xxx/qla_def.h | 4 ++-- > drivers/scsi/qla2xxx/qla_gs.c | 14 ++++++-------- > 2 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h > index 968f19995063..5c6bae116b58 100644 > --- a/drivers/scsi/qla2xxx/qla_def.h > +++ b/drivers/scsi/qla2xxx/qla_def.h > @@ -2741,7 +2741,7 @@ struct ct_fdmiv2_port_attr { > uint8_t fc4_types[32]; > uint32_t sup_speed; > uint32_t cur_speed; > - uint32_t max_frame_size; > + __be32 max_frame_size; > uint8_t os_dev_name[32]; > uint8_t host_name[256]; > uint8_t node_name[WWN_SIZE]; > @@ -2772,7 +2772,7 @@ struct ct_fdmi_port_attr { > uint8_t fc4_types[32]; > uint32_t sup_speed; > uint32_t cur_speed; > - uint32_t max_frame_size; > + __be32 max_frame_size; > uint8_t os_dev_name[32]; > uint8_t host_name[256]; > } a; > diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c > index aaa4a5bbf2ff..594b366db0ef 100644 > --- a/drivers/scsi/qla2xxx/qla_gs.c > +++ b/drivers/scsi/qla2xxx/qla_gs.c > @@ -1848,14 +1848,13 @@ qla2x00_fdmi_rpa(scsi_qla_host_t *vha) > eiter = entries + size; > eiter->type = cpu_to_be16(FDMI_PORT_MAX_FRAME_SIZE); > eiter->len = cpu_to_be16(4 + 4); > - eiter->a.max_frame_size = IS_FWI2_CAPABLE(ha) ? > + eiter->a.max_frame_size = cpu_to_be32(IS_FWI2_CAPABLE(ha) ? > le16_to_cpu(icb24->frame_payload_size) : > - le16_to_cpu(ha->init_cb->frame_payload_size); > - eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size); > + le16_to_cpu(ha->init_cb->frame_payload_size)); > size += 4 + 4; > > ql_dbg(ql_dbg_disc, vha, 0x203c, > - "Max_Frame_Size=%x.\n", eiter->a.max_frame_size); > + "Max_Frame_Size=%x.\n", be32_to_cpu(eiter->a.max_frame_size)); > > /* OS device name. */ > eiter = entries + size; > @@ -2419,14 +2418,13 @@ qla2x00_fdmiv2_rpa(scsi_qla_host_t *vha) > eiter = entries + size; > eiter->type = cpu_to_be16(FDMI_PORT_MAX_FRAME_SIZE); > eiter->len = cpu_to_be16(4 + 4); > - eiter->a.max_frame_size = IS_FWI2_CAPABLE(ha) ? > + eiter->a.max_frame_size = cpu_to_be32(IS_FWI2_CAPABLE(ha) ? > le16_to_cpu(icb24->frame_payload_size) : > - le16_to_cpu(ha->init_cb->frame_payload_size); > - eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size); > + le16_to_cpu(ha->init_cb->frame_payload_size)); > size += 4 + 4; > > ql_dbg(ql_dbg_disc, vha, 0x20bc, > - "Max_Frame_Size = %x.\n", eiter->a.max_frame_size); > + "Max_Frame_Size = %x.\n", be32_to_cpu(eiter->a.max_frame_size)); > > /* OS device name. */ > eiter = entries + size; Hi Bart, I'm not an expert of FDMI feature but it seems to introduce an inconsistency with regards to structure definition. IMO, All multi-byte binary fields and bitmasks in RPA request should be made __be32 rather than only one. Probably that should go into one patch series where all fields in the structure are fixed patch-by-patch or one patch that fixes all fields at once (the latter is harder to review though). According to Table 402 – Port Attribute Values in FC-GS-7 and RPA request attributes in the structure above, that includes: Supported Speed Current Port Speed Maximum Frame Size Port Type Supported Classes of Service Port State Number of Discovered Ports Port Identifier But the patch itself looks good, Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> Regards, Roman
On 2/25/20 10:43 AM, Roman Bolshakov wrote: > I'm not an expert of FDMI feature but it seems to introduce an > inconsistency with regards to structure definition. IMO, All multi-byte > binary fields and bitmasks in RPA request should be made __be32 rather > than only one. Probably that should go into one patch series where all > fields in the structure are fixed patch-by-patch or one patch that fixes > all fields at once (the latter is harder to review though). Hi Roman, I agree with what you wrote. I have a (huge) patch pending that fixes all endianness issues in the qla2xxx driver. I plan to post that patch after agreement has been achieved about this patch series. Thanks, Bart.
diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h index 968f19995063..5c6bae116b58 100644 --- a/drivers/scsi/qla2xxx/qla_def.h +++ b/drivers/scsi/qla2xxx/qla_def.h @@ -2741,7 +2741,7 @@ struct ct_fdmiv2_port_attr { uint8_t fc4_types[32]; uint32_t sup_speed; uint32_t cur_speed; - uint32_t max_frame_size; + __be32 max_frame_size; uint8_t os_dev_name[32]; uint8_t host_name[256]; uint8_t node_name[WWN_SIZE]; @@ -2772,7 +2772,7 @@ struct ct_fdmi_port_attr { uint8_t fc4_types[32]; uint32_t sup_speed; uint32_t cur_speed; - uint32_t max_frame_size; + __be32 max_frame_size; uint8_t os_dev_name[32]; uint8_t host_name[256]; } a; diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c index aaa4a5bbf2ff..594b366db0ef 100644 --- a/drivers/scsi/qla2xxx/qla_gs.c +++ b/drivers/scsi/qla2xxx/qla_gs.c @@ -1848,14 +1848,13 @@ qla2x00_fdmi_rpa(scsi_qla_host_t *vha) eiter = entries + size; eiter->type = cpu_to_be16(FDMI_PORT_MAX_FRAME_SIZE); eiter->len = cpu_to_be16(4 + 4); - eiter->a.max_frame_size = IS_FWI2_CAPABLE(ha) ? + eiter->a.max_frame_size = cpu_to_be32(IS_FWI2_CAPABLE(ha) ? le16_to_cpu(icb24->frame_payload_size) : - le16_to_cpu(ha->init_cb->frame_payload_size); - eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size); + le16_to_cpu(ha->init_cb->frame_payload_size)); size += 4 + 4; ql_dbg(ql_dbg_disc, vha, 0x203c, - "Max_Frame_Size=%x.\n", eiter->a.max_frame_size); + "Max_Frame_Size=%x.\n", be32_to_cpu(eiter->a.max_frame_size)); /* OS device name. */ eiter = entries + size; @@ -2419,14 +2418,13 @@ qla2x00_fdmiv2_rpa(scsi_qla_host_t *vha) eiter = entries + size; eiter->type = cpu_to_be16(FDMI_PORT_MAX_FRAME_SIZE); eiter->len = cpu_to_be16(4 + 4); - eiter->a.max_frame_size = IS_FWI2_CAPABLE(ha) ? + eiter->a.max_frame_size = cpu_to_be32(IS_FWI2_CAPABLE(ha) ? le16_to_cpu(icb24->frame_payload_size) : - le16_to_cpu(ha->init_cb->frame_payload_size); - eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size); + le16_to_cpu(ha->init_cb->frame_payload_size)); size += 4 + 4; ql_dbg(ql_dbg_disc, vha, 0x20bc, - "Max_Frame_Size = %x.\n", eiter->a.max_frame_size); + "Max_Frame_Size = %x.\n", be32_to_cpu(eiter->a.max_frame_size)); /* OS device name. */ eiter = entries + size;
Make sure that sparse doesn't complain about the statements that load or store the port attribute max_frame_size member. The port attribute data structures represent FC protocol data and hence use big endian format. This patch only changes the meaning of two ql_dbg() statements. Cc: Quinn Tran <qutran@marvell.com> Cc: Martin Wilck <mwilck@suse.com> Cc: Daniel Wagner <dwagner@suse.de> Cc: Roman Bolshakov <r.bolshakov@yadro.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/qla2xxx/qla_def.h | 4 ++-- drivers/scsi/qla2xxx/qla_gs.c | 14 ++++++-------- 2 files changed, 8 insertions(+), 10 deletions(-)