gcc-6 found a couple of bugs in scsi drivers that are not
yet fixed in linux-next.
In all three cases, the code is indented in a rather misleading
way, but actually correct. I think it would be good to have
this fixed in 4.6, but no backports are needed even though
the problems have all been around for a while.
gcc-6 complains about the indentation of the lpfc_destroy_vport_work_array()
call in lpfc_online(), which clearly doesn't look right:
drivers/scsi/lpfc/lpfc_init.c: In function 'lpfc_online':
drivers/scsi/lpfc/lpfc_init.c:2880:3: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
lpfc_destroy_vport_work_array(phba, vports);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/lpfc/lpfc_init.c:2863:2: note: ...this 'if' clause, but it is not
if (vports != NULL)
^~
Looking at the patch that introduced this code, it's clear that the
behavior is correct and the indentation is wrong.
This fixes the indentation and adds curly braces around the previous
if() block for clarity, as that is most likely what caused the code
to be misindented in the first place.
Signed-off-by: Arnd Bergmann <[email protected]>
Fixes: 549e55cd2a1b ("[SCSI] lpfc 8.2.2 : Fix locking around HBA's port_list")
---
drivers/scsi/lpfc/lpfc_init.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index a544366a367e..f57d02c3b6cf 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -2860,7 +2860,7 @@ lpfc_online(struct lpfc_hba *phba)
}
vports = lpfc_create_vport_work_array(phba);
- if (vports != NULL)
+ if (vports != NULL) {
for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++) {
struct Scsi_Host *shost;
shost = lpfc_shost_from_vport(vports[i]);
@@ -2877,7 +2877,8 @@ lpfc_online(struct lpfc_hba *phba)
}
spin_unlock_irq(shost->host_lock);
}
- lpfc_destroy_vport_work_array(phba, vports);
+ }
+ lpfc_destroy_vport_work_array(phba, vports);
lpfc_unblock_mgmt_io(phba);
return 0;
--
2.7.0
gcc-6 warns about obviously wrong indentation for newly added
code in aac_slave_configure():
drivers/scsi/aacraid/linit.c: In function 'aac_slave_configure':
drivers/scsi/aacraid/linit.c:458:3: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
sdev->tagged_supported = 1;
^~~~
drivers/scsi/aacraid/linit.c:455:4: note: ...this 'else' clause, but it is not
gcc is correct, and evidently this was meant to be within the curly
braces that should have been there to start with. This patch adds
them, which avoids the warning and makes it clear what was intended
here.
Nothing changes in behavior because in the 'if' block, the
sdev->tagged_supported flag is known to be set already.
Signed-off-by: Arnd Bergmann <[email protected]>
Fixes: 6bf3b630d0a7 ("aacraid: SCSI blk tag support")
---
drivers/scsi/aacraid/linit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 21a67ed047e8..ff6caab8cc8b 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -452,10 +452,11 @@ static int aac_slave_configure(struct scsi_device *sdev)
else if (depth < 2)
depth = 2;
scsi_change_queue_depth(sdev, depth);
- } else
+ } else {
scsi_change_queue_depth(sdev, 1);
sdev->tagged_supported = 1;
+ }
return 0;
}
--
2.7.0
gcc-6 found a dubious indentation in the megasas_mgmt_fw_ioctl
function:
drivers/scsi/megaraid/megaraid_sas_base.c: In function 'megasas_mgmt_fw_ioctl':
drivers/scsi/megaraid/megaraid_sas_base.c:6658:4: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
kbuff_arr[i] = NULL;
^~~~~~~~~
drivers/scsi/megaraid/megaraid_sas_base.c:6653:3: note: ...this 'if' clause, but it is not
if (kbuff_arr[i])
^~
The code is actually correct, as there is no downside in clearing a NULL
pointer again.
This clarifies the code and avoids the warning by adding extra curly
braces.
Signed-off-by: Arnd Bergmann <[email protected]>
Fixes: 90dc9d98f01b ("megaraid_sas : MFI MPT linked list corruption fix")
---
drivers/scsi/megaraid/megaraid_sas_base.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 5c08568ccfbf..2627200d4f82 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -6650,12 +6650,13 @@ out:
}
for (i = 0; i < ioc->sge_count; i++) {
- if (kbuff_arr[i])
+ if (kbuff_arr[i]) {
dma_free_coherent(&instance->pdev->dev,
le32_to_cpu(kern_sge32[i].length),
kbuff_arr[i],
le32_to_cpu(kern_sge32[i].phys_addr));
kbuff_arr[i] = NULL;
+ }
}
megasas_return_cmd(instance, cmd);
--
2.7.0
On 03/14/2016 03:29 PM, Arnd Bergmann wrote:
> gcc-6 complains about the indentation of the lpfc_destroy_vport_work_array()
> call in lpfc_online(), which clearly doesn't look right:
>
> drivers/scsi/lpfc/lpfc_init.c: In function 'lpfc_online':
> drivers/scsi/lpfc/lpfc_init.c:2880:3: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
> lpfc_destroy_vport_work_array(phba, vports);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/lpfc/lpfc_init.c:2863:2: note: ...this 'if' clause, but it is not
> if (vports != NULL)
> ^~
>
> Looking at the patch that introduced this code, it's clear that the
> behavior is correct and the indentation is wrong.
>
> This fixes the indentation and adds curly braces around the previous
> if() block for clarity, as that is most likely what caused the code
> to be misindented in the first place.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Fixes: 549e55cd2a1b ("[SCSI] lpfc 8.2.2 : Fix locking around HBA's port_list")
> ---
> drivers/scsi/lpfc/lpfc_init.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index a544366a367e..f57d02c3b6cf 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -2860,7 +2860,7 @@ lpfc_online(struct lpfc_hba *phba)
> }
>
> vports = lpfc_create_vport_work_array(phba);
> - if (vports != NULL)
> + if (vports != NULL) {
> for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++) {
> struct Scsi_Host *shost;
> shost = lpfc_shost_from_vport(vports[i]);
> @@ -2877,7 +2877,8 @@ lpfc_online(struct lpfc_hba *phba)
> }
> spin_unlock_irq(shost->host_lock);
> }
> - lpfc_destroy_vport_work_array(phba, vports);
> + }
> + lpfc_destroy_vport_work_array(phba, vports);
>
> lpfc_unblock_mgmt_io(phba);
> return 0;
>
Nope.
vports is only valid from within the indentation block, so it should
be moved into it.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)
On 03/14/2016 03:29 PM, Arnd Bergmann wrote:
> gcc-6 found a dubious indentation in the megasas_mgmt_fw_ioctl
> function:
>
> drivers/scsi/megaraid/megaraid_sas_base.c: In function 'megasas_mgmt_fw_ioctl':
> drivers/scsi/megaraid/megaraid_sas_base.c:6658:4: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
> kbuff_arr[i] = NULL;
> ^~~~~~~~~
> drivers/scsi/megaraid/megaraid_sas_base.c:6653:3: note: ...this 'if' clause, but it is not
> if (kbuff_arr[i])
> ^~
>
> The code is actually correct, as there is no downside in clearing a NULL
> pointer again.
>
> This clarifies the code and avoids the warning by adding extra curly
> braces.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Fixes: 90dc9d98f01b ("megaraid_sas : MFI MPT linked list corruption fix")
> ---
> drivers/scsi/megaraid/megaraid_sas_base.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 5c08568ccfbf..2627200d4f82 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -6650,12 +6650,13 @@ out:
> }
>
> for (i = 0; i < ioc->sge_count; i++) {
> - if (kbuff_arr[i])
> + if (kbuff_arr[i]) {
> dma_free_coherent(&instance->pdev->dev,
> le32_to_cpu(kern_sge32[i].length),
> kbuff_arr[i],
> le32_to_cpu(kern_sge32[i].phys_addr));
> kbuff_arr[i] = NULL;
> + }
> }
>
> megasas_return_cmd(instance, cmd);
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)
On Monday 14 March 2016 16:19:58 Hannes Reinecke wrote:
> > vports = lpfc_create_vport_work_array(phba);
> > - if (vports != NULL)
> > + if (vports != NULL) {
> > for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++) {
> > struct Scsi_Host *shost;
> > shost = lpfc_shost_from_vport(vports[i]);
> > @@ -2877,7 +2877,8 @@ lpfc_online(struct lpfc_hba *phba)
> > }
> > spin_unlock_irq(shost->host_lock);
> > }
> > - lpfc_destroy_vport_work_array(phba, vports);
> > + }
> > + lpfc_destroy_vport_work_array(phba, vports);
> >
> > lpfc_unblock_mgmt_io(phba);
> > return 0;
> >
> Nope.
>
> vports is only valid from within the indentation block, so it should
> be moved into it.
>
>
Well, every other user of the function also looks like
vports = lpfc_create_vport_work_array(phba);
if (vports != NULL) {
do_something(vports);
}
lpfc_destroy_vport_work_array(phba, vports);
and lpfc_destroy_vport_work_array() does nothing if its argument is NULL.
I still think my patch is the correct fix for the warning.
Arnd
On 03/14/2016 04:25 PM, Arnd Bergmann wrote:
> On Monday 14 March 2016 16:19:58 Hannes Reinecke wrote:
>>> vports = lpfc_create_vport_work_array(phba);
>>> - if (vports != NULL)
>>> + if (vports != NULL) {
>>> for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++) {
>>> struct Scsi_Host *shost;
>>> shost = lpfc_shost_from_vport(vports[i]);
>>> @@ -2877,7 +2877,8 @@ lpfc_online(struct lpfc_hba *phba)
>>> }
>>> spin_unlock_irq(shost->host_lock);
>>> }
>>> - lpfc_destroy_vport_work_array(phba, vports);
>>> + }
>>> + lpfc_destroy_vport_work_array(phba, vports);
>>>
>>> lpfc_unblock_mgmt_io(phba);
>>> return 0;
>>>
>> Nope.
>>
>> vports is only valid from within the indentation block, so it should
>> be moved into it.
>>
>>
>
> Well, every other user of the function also looks like
>
> vports = lpfc_create_vport_work_array(phba);
> if (vports != NULL) {
> do_something(vports);
> }
> lpfc_destroy_vport_work_array(phba, vports);
>
> and lpfc_destroy_vport_work_array() does nothing if its argument is NULL.
>
> I still think my patch is the correct fix for the warning.
>
Okay, good point.
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)
On Mon, 2016-03-14 at 16:26 +0100, Hannes Reinecke wrote:
> On 03/14/2016 04:25 PM, Arnd Bergmann wrote:
> > On Monday 14 March 2016 16:19:58 Hannes Reinecke wrote:
> >>> vports = lpfc_create_vport_work_array(phba);
> >>> - if (vports != NULL)
> >>> + if (vports != NULL) {
> >>> for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++) {
> >>> struct Scsi_Host *shost;
> >>> shost = lpfc_shost_from_vport(vports[i]);
> >>> @@ -2877,7 +2877,8 @@ lpfc_online(struct lpfc_hba *phba)
> >>> }
> >>> spin_unlock_irq(shost->host_lock);
> >>> }
> >>> - lpfc_destroy_vport_work_array(phba, vports);
> >>> + }
> >>> + lpfc_destroy_vport_work_array(phba, vports);
> >>>
> >>> lpfc_unblock_mgmt_io(phba);
> >>> return 0;
> >>>
> >> Nope.
> >>
> >> vports is only valid from within the indentation block, so it should
> >> be moved into it.
> >>
> >>
> >
> > Well, every other user of the function also looks like
> >
> > vports = lpfc_create_vport_work_array(phba);
> > if (vports != NULL) {
> > do_something(vports);
> > }
> > lpfc_destroy_vport_work_array(phba, vports);
Actually the lpfc code is inconsistent about whether the _destroy call
is within the (vports != NULL) test or not, but as you say below it
doesn't matter.
Reviewed-by: Ewan D. Milne <[email protected]>
> >
> > and lpfc_destroy_vport_work_array() does nothing if its argument is NULL.
> >
> > I still think my patch is the correct fix for the warning.
> >
> Okay, good point.
>
> Reviewed-by: Hannes Reinecke <[email protected]>
>
> Cheers,
>
> Hannes
Arnd Bergmann wrote:
> gcc-6 complains about the indentation of the lpfc_destroy_vport_work_array()
> call in lpfc_online(), which clearly doesn't look right:
>
> drivers/scsi/lpfc/lpfc_init.c: In function 'lpfc_online':
> drivers/scsi/lpfc/lpfc_init.c:2880:3: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
> lpfc_destroy_vport_work_array(phba, vports);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/lpfc/lpfc_init.c:2863:2: note: ...this 'if' clause, but it is not
> if (vports != NULL)
> ^~
>
> Looking at the patch that introduced this code, it's clear that the
> behavior is correct and the indentation is wrong.
>
> This fixes the indentation and adds curly braces around the previous
> if() block for clarity, as that is most likely what caused the code
> to be misindented in the first place.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Fixes: 549e55cd2a1b ("[SCSI] lpfc 8.2.2 : Fix locking around HBA's port_list")
> ---
> drivers/scsi/lpfc/lpfc_init.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Sebastian Herbszt <[email protected]>
> -----Original Message-----
> From: Arnd Bergmann [mailto:[email protected]]
> Sent: Monday, March 14, 2016 8:00 PM
> To: [email protected]; [email protected];
> Kashyap Desai; Sumit Saxena; Uday Lingala; James E.J. Bottomley
> Cc: [email protected]; [email protected]; Arnd
Bergmann;
> Tomas Henzl; Bjorn Helgaas; [email protected]
> Subject: [PATCH 3/3] megaraid_sas: add missing curly braces in ioctl
handler
>
> gcc-6 found a dubious indentation in the megasas_mgmt_fw_ioctl
> function:
>
> drivers/scsi/megaraid/megaraid_sas_base.c: In function
> 'megasas_mgmt_fw_ioctl':
> drivers/scsi/megaraid/megaraid_sas_base.c:6658:4: warning: statement is
> indented as if it were guarded by... [-Wmisleading-indentation]
> kbuff_arr[i] = NULL;
> ^~~~~~~~~
> drivers/scsi/megaraid/megaraid_sas_base.c:6653:3: note: ...this 'if'
clause, but
> it is not
> if (kbuff_arr[i])
> ^~
>
> The code is actually correct, as there is no downside in clearing a NULL
pointer
> again.
>
> This clarifies the code and avoids the warning by adding extra curly
braces.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Fixes: 90dc9d98f01b ("megaraid_sas : MFI MPT linked list corruption
fix")
> ---
> drivers/scsi/megaraid/megaraid_sas_base.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 5c08568ccfbf..2627200d4f82 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -6650,12 +6650,13 @@ out:
> }
>
> for (i = 0; i < ioc->sge_count; i++) {
> - if (kbuff_arr[i])
> + if (kbuff_arr[i]) {
> dma_free_coherent(&instance->pdev->dev,
>
le32_to_cpu(kern_sge32[i].length),
> kbuff_arr[i],
>
> le32_to_cpu(kern_sge32[i].phys_addr));
> kbuff_arr[i] = NULL;
> + }
> }
>
> megasas_return_cmd(instance, cmd);
Acked-by: Sumit Saxena <[email protected]>
> --
> 2.7.0
>>>>> "Arnd" == Arnd Bergmann <[email protected]> writes:
Raghava,
Please review.
Arnd> gcc-6 warns about obviously wrong indentation for newly added code
Arnd> in aac_slave_configure():
https://patchwork.kernel.org/patch/8579681/
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "Arnd" == Arnd Bergmann <[email protected]> writes:
Arnd> gcc-6 complains about the indentation of the
Arnd> lpfc_destroy_vport_work_array() call in lpfc_online(), which
Arnd> clearly doesn't look right:
Applied to 4.6/scsi-fixes.
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "Arnd" == Arnd Bergmann <[email protected]> writes:
Arnd> gcc-6 found a dubious indentation in the megasas_mgmt_fw_ioctl
Arnd> function:
Applied to 4.6/scsi-fixes.
--
Martin K. Petersen Oracle Linux Engineering
> -----Original Message-----
> From: Arnd Bergmann [mailto:[email protected]]
> Sent: Monday, March 14, 2016 7:30 AM
> To: [email protected];
> [email protected]; Adaptec OEM Raid Solutions;
> James E.J. Bottomley
> Cc: [email protected]; [email protected]; Arnd
> Bergmann; Johannes Thumshirn; Tomas Henzl; Mahesh Rajashekhara;
> Raghava Aditya Renukunta; Fengguang Wu
> Subject: [PATCH 1/3] aacraid: add missing curly braces
>
> gcc-6 warns about obviously wrong indentation for newly added
> code in aac_slave_configure():
>
> drivers/scsi/aacraid/linit.c: In function 'aac_slave_configure':
> drivers/scsi/aacraid/linit.c:458:3: warning: statement is indented as if it were
> guarded by... [-Wmisleading-indentation]
> sdev->tagged_supported = 1;
> ^~~~
> drivers/scsi/aacraid/linit.c:455:4: note: ...this 'else' clause, but it is not
>
> gcc is correct, and evidently this was meant to be within the curly
> braces that should have been there to start with. This patch adds
> them, which avoids the warning and makes it clear what was intended
> here.
>
> Nothing changes in behavior because in the 'if' block, the
> sdev->tagged_supported flag is known to be set already.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Fixes: 6bf3b630d0a7 ("aacraid: SCSI blk tag support")
> ---
> drivers/scsi/aacraid/linit.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index 21a67ed047e8..ff6caab8cc8b 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -452,10 +452,11 @@ static int aac_slave_configure(struct scsi_device
> *sdev)
> else if (depth < 2)
> depth = 2;
> scsi_change_queue_depth(sdev, depth);
> - } else
> + } else {
> scsi_change_queue_depth(sdev, 1);
> sdev->tagged_supported = 1;
> + }
>
> return 0;
> }
> --
> 2.7.0
Reviewed-by: Raghava Aditya Renukunta < [email protected] >
Regards,
Raghava Aditya
>>>>> "Arnd" == Arnd Bergmann <[email protected]> writes:
Arnd> gcc-6 warns about obviously wrong indentation for newly added code
Arnd> in aac_slave_configure():
Applied to 4.6/scsi-fixes.
--
Martin K. Petersen Oracle Linux Engineering