linux-kernel - Re: [PATCH] advansys: Remove redundant assignment to err and n_q

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <516aeebe-f93-52e3-c554-bcb725bcf4dd@nippy.intranet>
Date:   Sat, 1 May 2021 10:27:16 +1000 (AEST)
From:   Finn Thain <fthain@...egraphics.com.au>
To:     Jiapeng Chong <jiapeng.chong@...ux.alibaba.com>
cc:     willy@...radead.org, hare@...e.com, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, linux-scsi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] advansys: Remove redundant assignment to err and
 n_q_required

On Fri, 30 Apr 2021, Jiapeng Chong wrote:

> Variable err and n_q_required is set to '-ENOMEM' and '1', but they are
> either overwritten or unused later on, so these are redundant assignments
> that can be removed.
> 
> Clean up the following clang-analyzer warning:
> 
> drivers/scsi/advansys.c:11235:2: warning: Value stored to 'err' is never
> read [clang-analyzer-deadcode.DeadStores].
> 
> drivers/scsi/advansys.c:8091:2: warning: Value stored to 'n_q_required'
> is never read [clang-analyzer-deadcode.DeadStores].
> 
> drivers/scsi/advansys.c:11484:2: warning: Value stored to 'err' is never
> read [clang-analyzer-deadcode.DeadStores].
> 
> Reported-by: Abaci Robot <abaci@...ux.alibaba.com>
> Signed-off-by: Jiapeng Chong <jiapeng.chong@...ux.alibaba.com>
> ---
>  drivers/scsi/advansys.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> index 800052f..f9969d4 100644
> --- a/drivers/scsi/advansys.c
> +++ b/drivers/scsi/advansys.c
> @@ -8088,7 +8088,6 @@ static int AscExeScsiQueue(ASC_DVC_VAR *asc_dvc, ASC_SCSI_Q *scsiq)
>  	sta = 0;
>  	target_ix = scsiq->q2.target_ix;
>  	tid_no = ASC_TIX_TO_TID(target_ix);
> -	n_q_required = 1;
>  	if (scsiq->cdbptr[0] == REQUEST_SENSE) {
>  		if ((asc_dvc->init_sdtr & scsiq->q1.target_id) != 0) {
>  			asc_dvc->sdtr_done &= ~scsiq->q1.target_id;
> @@ -11232,7 +11231,6 @@ static int advansys_vlb_probe(struct device *dev, unsigned int id)
>  	if (AscGetChipVersion(iop_base, ASC_IS_VL) > ASC_CHIP_MAX_VER_VL)
>  		goto release_region;
>  
> -	err = -ENOMEM;
>  	shost = scsi_host_alloc(&advansys_template, sizeof(*board));
>  	if (!shost)
>  		goto release_region;

No, the correct way to resolve that particular clang warning is,

  free_host:
         scsi_host_put(shost);
  release_region:
         release_region(iop_base, ASC_IOADR_GAP);
-        return -ENODEV;
+        return err;
 }

> @@ -11457,7 +11455,6 @@ static int advansys_pci_probe(struct pci_dev *pdev,
>  
>  	ioport = pci_resource_start(pdev, 0);
>  
> -	err = -ENOMEM;
>  	shost = scsi_host_alloc(&advansys_template, sizeof(*board));
>  	if (!shost)
>  		goto release_region;
> 

No, that would be a behavioural change for the error path.

Moreover, the clang warning you claimed for line 11484 appears to be bogus 
with regard to mainline code. You may want to check your checker.