| Message ID | 20200610024215.27997-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] qla2xxx: Fix the ARM build | expand |
On Tue, Jun 09, 2020 at 07:42:15PM -0700, Bart Van Assche wrote: > This patch fixes the build errors reported by the kernel test robot on June > 7th. Does this perhaps mean that so far nobody has tried to use the qla2xxx > driver on an ARM system? For the kernel test robot output, see also > https://lore.kernel.org/lkml/202006070558.Cy93XggE%25lkp@intel.com/. > > Cc: Nilesh Javali <njavali@marvell.com> > Cc: Quinn Tran <qutran@marvell.com> > Cc: Himanshu Madhani <himanshu.madhani@oracle.com> > Cc: Hannes Reinecke <hare@suse.de> > 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h > index 42dbf90d4651..de9c1604c575 100644 > --- a/drivers/scsi/qla2xxx/qla_def.h > +++ b/drivers/scsi/qla2xxx/qla_def.h > @@ -46,7 +46,7 @@ typedef struct { > uint8_t al_pa; > uint8_t area; > uint8_t domain; > -} le_id_t; > +} __packed le_id_t; > > #include "qla_bsg.h" > #include "qla_dsd.h" Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> BTW, it might worth to mention the bot found the issue: Reported-by: kernel test robot <lkp@intel.com> Thanks, Roman
Hi Bart, > diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h > index 42dbf90d4651..de9c1604c575 100644 > --- a/drivers/scsi/qla2xxx/qla_def.h > +++ b/drivers/scsi/qla2xxx/qla_def.h > @@ -46,7 +46,7 @@ typedef struct { > uint8_t al_pa; > uint8_t area; > uint8_t domain; > -} le_id_t; > +} __packed le_id_t; Now I am totally confused. le_id_t (and why does be_id_t not need it?) are not used inside either of the reported data structure (cmd_entry_t, ms_iocb_entry_t, request_t, struct ctio_crc2_to_fw, struct ctio7_to_24xx, struct ctio_to_2xxx) which the bot reports. I must oversee something. Besides this, my small test report 3 bytes size for le_id_t on all architectures too. I'll try to see what the compiler generates from the kernel source (takes a while because I am running it natively...)
On Wed, Jun 10, 2020 at 01:27:45PM +0200, Daniel Wagner wrote: > Hi Bart, > > > diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h > > index 42dbf90d4651..de9c1604c575 100644 > > --- a/drivers/scsi/qla2xxx/qla_def.h > > +++ b/drivers/scsi/qla2xxx/qla_def.h > > @@ -46,7 +46,7 @@ typedef struct { > > uint8_t al_pa; > > uint8_t area; > > uint8_t domain; > > -} le_id_t; > > +} __packed le_id_t; > > Now I am totally confused. le_id_t (and why does be_id_t not need it?) are > not used inside either of the reported data structure (cmd_entry_t, > ms_iocb_entry_t, request_t, struct ctio_crc2_to_fw, struct ctio7_to_24xx, > struct ctio_to_2xxx) which the bot reports. I must oversee something. > I also had the thought that both fields should be packed for sake of consistency because there is fcp_hdr with be_id_t sid/did and fcp_hdr_le with le_id_t sid/did. You also seem to be correct, about your concerns. I overlooked that only ctio_crc2_to_fw and ctio7_to_24xx have le_id_t initiator_id field. Roman
On 2020-06-10 05:11, Roman Bolshakov wrote: > On Wed, Jun 10, 2020 at 01:27:45PM +0200, Daniel Wagner wrote: >>> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h >>> index 42dbf90d4651..de9c1604c575 100644 >>> --- a/drivers/scsi/qla2xxx/qla_def.h >>> +++ b/drivers/scsi/qla2xxx/qla_def.h >>> @@ -46,7 +46,7 @@ typedef struct { >>> uint8_t al_pa; >>> uint8_t area; >>> uint8_t domain; >>> -} le_id_t; >>> +} __packed le_id_t; >> >> Now I am totally confused. le_id_t (and why does be_id_t not need it?) are >> not used inside either of the reported data structure (cmd_entry_t, >> ms_iocb_entry_t, request_t, struct ctio_crc2_to_fw, struct ctio7_to_24xx, >> struct ctio_to_2xxx) which the bot reports. I must oversee something. > > I also had the thought that both fields should be packed for sake of > consistency because there is fcp_hdr with be_id_t sid/did and > fcp_hdr_le with le_id_t sid/did. You also seem to be correct, about your > concerns. I overlooked that only ctio_crc2_to_fw and ctio7_to_24xx have > le_id_t initiator_id field. It's only now that I noticed that the build failures are reported by sparse (C=1). So the build failures may indicate a bug in sparse. Bart.
diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h index 42dbf90d4651..de9c1604c575 100644 --- a/drivers/scsi/qla2xxx/qla_def.h +++ b/drivers/scsi/qla2xxx/qla_def.h @@ -46,7 +46,7 @@ typedef struct { uint8_t al_pa; uint8_t area; uint8_t domain; -} le_id_t; +} __packed le_id_t; #include "qla_bsg.h" #include "qla_dsd.h"
This patch fixes the build errors reported by the kernel test robot on June 7th. Does this perhaps mean that so far nobody has tried to use the qla2xxx driver on an ARM system? For the kernel test robot output, see also https://lore.kernel.org/lkml/202006070558.Cy93XggE%25lkp@intel.com/. Cc: Nilesh Javali <njavali@marvell.com> Cc: Quinn Tran <qutran@marvell.com> Cc: Himanshu Madhani <himanshu.madhani@oracle.com> Cc: Hannes Reinecke <hare@suse.de> 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)