[PATCH] scsi: dc395x: Remove leftover if statement in reselect()

[PATCH] scsi: dc395x: Remove leftover if statement in reselect()

[Nathan Chancellor]

Clang warns (or errors with CONFIG_WERROR=y):

  drivers/scsi/dc395x.c:2553:6: error: variable 'id' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
   2553 |         if (!(rsel_tar_lun_id & (IDENTIFY_BASE << 8)))
        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/scsi/dc395x.c:2556:22: note: uninitialized use occurs here
   2556 |         dcb = find_dcb(acb, id, lun);
        |                             ^~
  drivers/scsi/dc395x.c:2553:2: note: remove the 'if' if its condition is always true
   2553 |         if (!(rsel_tar_lun_id & (IDENTIFY_BASE << 8)))
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   2554 |         id = rsel_tar_lun_id & 0xff;

This if statement only existed for a debugging print but it was not
removed with the debugging print in a recent cleanup, leading to id only
being initialized when the if condition is true. Remove the if
statement to ensure id is always initialized, clearing up the warning.

Fixes: 62b434b0db2c ("scsi: dc395x: Remove DEBUG conditional compilation")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/scsi/dc395x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/dc395x.c b/drivers/scsi/dc395x.c
index 95145b9d9ce3..96b335c92603 100644
--- a/drivers/scsi/dc395x.c
+++ b/drivers/scsi/dc395x.c
@@ -2550,7 +2550,6 @@ static void reselect(struct AdapterCtlBlk *acb)
 		}
 	}
 	/* Read Reselected Target Id and LUN */
-	if (!(rsel_tar_lun_id & (IDENTIFY_BASE << 8)))
 	id = rsel_tar_lun_id & 0xff;
 	lun = (rsel_tar_lun_id >> 8) & 7;
 	dcb = find_dcb(acb, id, lun);

---
base-commit: e142de4aac2aae697d7e977b01e7a889e9f454df
change-id: 20250429-scsi-dc395x-fix-uninit-var-c1cfd889a63d

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH] scsi: dc395x: Remove leftover if statement in reselect()

[Finn Thain]

On Tue, 29 Apr 2025, Nathan Chancellor wrote:

> This if statement only existed for a debugging print but it was not 
> removed with the debugging print in a recent cleanup

The patch you called "cleanup" has a "fixes" tag. Strange.

I think it's unreasonable to refer to a patch which alters object code as 
"cleanup".

I also think it's unreasonable to put a "fixes" tag on a patch that 
doesn't alter object code (for any shipping config, for any supported 
toolchain).

Those assertions could be tested automatically by CI. And maybe that could 
prevent this kind of regression.

Re: [PATCH] scsi: dc395x: Remove leftover if statement in reselect()

[Oliver Neukum]

On 30.04.25 03:36, Finn Thain wrote:
> 
> On Tue, 29 Apr 2025, Nathan Chancellor wrote:
> 
>> This if statement only existed for a debugging print but it was not
>> removed with the debugging print in a recent cleanup
> 
> The patch you called "cleanup" has a "fixes" tag. Strange.
> 
> I think it's unreasonable to refer to a patch which alters object code as
> "cleanup".

Hi,

yes, I was unsure about terminology used for code that is by default not
compiled, but would not compile if the attempt is made to compile it.

	Regards
		Oliver

Re: [PATCH] scsi: dc395x: Remove leftover if statement in reselect()

[Finn Thain]

On Wed, 30 Apr 2025, Oliver Neukum wrote:

> On 30.04.25 03:36, Finn Thain wrote:
> > 
> > On Tue, 29 Apr 2025, Nathan Chancellor wrote:
> > 
> >> This if statement only existed for a debugging print but it was not
> >> removed with the debugging print in a recent cleanup
> > 
> > The patch you called "cleanup" has a "fixes" tag. Strange.
> > 
> > I think it's unreasonable to refer to a patch which alters object code as
> > "cleanup".
> 
> Hi,
> 
> yes, I was unsure about terminology used for code that is by default not
> compiled, but would not compile if the attempt is made to compile it.
> 

Yes, I realize that you were referring to the intention as "cleanup" and 
not the actual patch that got merged.

I'm afraid my message was poorly expressed. I don't have a problem with 
your fix. I was only interested in the general case.

Re: [PATCH] scsi: dc395x: Remove leftover if statement in reselect()

[Oliver Neukum]

On 30.04.25 11:43, Finn Thain wrote:

>> yes, I was unsure about terminology used for code that is by default not
>> compiled, but would not compile if the attempt is made to compile it.
>>
> 
> Yes, I realize that you were referring to the intention as "cleanup" and
> not the actual patch that got merged.
> 
> I'm afraid my message was poorly expressed. I don't have a problem with
> your fix. I was only interested in the general case.

Well, in general I think such code is problematic. In general I think we should use dynamic debugging statements. The issue seems to be of terminology. However, we can hope that this will
go away and become moot.

	Regards
		Oliver
  

Re: [PATCH] scsi: dc395x: Remove leftover if statement in reselect()

[Finn Thain]

On Wed, 30 Apr 2025, Oliver Neukum wrote:

> On 30.04.25 11:43, Finn Thain wrote:
> 
> >> yes, I was unsure about terminology used for code that is by default not
> >> compiled, but would not compile if the attempt is made to compile it.
> >>
> > 
> > Yes, I realize that you were referring to the intention as "cleanup" and
> > not the actual patch that got merged.
> > 
> > I'm afraid my message was poorly expressed. I don't have a problem with
> > your fix. I was only interested in the general case.
> 
> Well, in general I think such code is problematic. In general I think we 
> should use dynamic debugging statements. The issue seems to be of 
> terminology. However, we can hope that this will go away and become 
> moot.
> 

I think you're saying that the general problem is the style of the 
debugging code that everyone disables. I'm afraid I don't see it that way.

The general problem here is a bad cleanup masquerading as a fix that got 
merged because of a missed opportunity for automated vetting.

Re: [PATCH] scsi: dc395x: Remove leftover if statement in reselect()

[Martin K. Petersen]

Nathan,

>   drivers/scsi/dc395x.c:2553:6: error: variable 'id' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>    2553 |         if (!(rsel_tar_lun_id & (IDENTIFY_BASE << 8)))
>         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Applied to 6.16/scsi-staging, thanks!

-- 
Martin K. Petersen

Re: [PATCH] scsi: dc395x: Remove leftover if statement in reselect()

[Martin K. Petersen]

On Tue, 29 Apr 2025 17:46:49 -0700, Nathan Chancellor wrote:

> Clang warns (or errors with CONFIG_WERROR=y):
> 
>   drivers/scsi/dc395x.c:2553:6: error: variable 'id' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>    2553 |         if (!(rsel_tar_lun_id & (IDENTIFY_BASE << 8)))
>         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   drivers/scsi/dc395x.c:2556:22: note: uninitialized use occurs here
>    2556 |         dcb = find_dcb(acb, id, lun);
>         |                             ^~
>   drivers/scsi/dc395x.c:2553:2: note: remove the 'if' if its condition is always true
>    2553 |         if (!(rsel_tar_lun_id & (IDENTIFY_BASE << 8)))
>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    2554 |         id = rsel_tar_lun_id & 0xff;
> 
> [...]

Applied to 6.16/scsi-queue, thanks!

[1/1] scsi: dc395x: Remove leftover if statement in reselect()
      https://git.kernel.org/mkp/scsi/c/bf6971a2b3ee

-- 
Martin K. Petersen	Oracle Linux Engineering