Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
IronPort-SDR: g+cZ83vC91+YKJWssm8bH7LR6uLiFuGWAp2aoHbeScXsnt5G1C9ZFVaJVZ7gKm50RdZOn9vjb4
 vGdxubSTZlyyUoGVNdpp7Ssp9sa1ZqRxIhnLyhJRKjY17B8oymRZKsjGQb+VlPwfG9VVGz3CQy
 7gZaQ7fReM3kbaC/sg9pA9FLwNB+87wruTe3AUlz99JM/kLt/nwy7mmyw5y1KekI8nrmooRs+R
 PSUhs0kSwuyGfcK6MYMltRXCrzgrk/yebcinr7SBnHsRb0i/m9OmshPif4iiZ4ZffyP9GUDwcM
 QLTzwvgkGqYBcEaQbPde6Y3+
IronPort-SDR: DS58eNbnI5wBJDjFA1VusNiloulCLGMvtG2l0gTcWif4nkVeOi4t5wlFpz0lZe2sWzozKXcDvC
 9YDPtsUVu2Bit1k+A3SZPafjNgcXh/83nFR0f/aalvakfnsverGWb00BChvnZRMneA1xDfcumo
 au9nPrtP/3zMSUsnRuMVo4cK3DEgjRerAJZ3PHOG5x3mE7yz+eRHeydNOjrW9bzeOymEdqHlZR
 h3WYH3fOE2yAyX494sUyg7h7BRXtVhrz09LyfW4fklmQdGkcUUoCIb8y7YW/jQDyBIFezTto2n
 C0s=
WDCIronportException: Internal
Message-ID: <281475cc-a159-8e25-f5e7-cde343bc50c4@opensource.wdc.com>
Date:   Mon, 7 Feb 2022 12:25:04 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH -next] scsi: pm8001: clean up some inconsistent indenting
Content-Language: en-US
To:     Yang Li <yang.lee@linux.alibaba.com>, jejb@linux.ibm.com
Cc:     martin.petersen@oracle.com, jinpu.wang@cloud.ionos.com,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        Abaci Robot <abaci@linux.alibaba.com>
References: <20220206125548.110945-1-yang.lee@linux.alibaba.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <20220206125548.110945-1-yang.lee@linux.alibaba.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/6/22 21:55, Yang Li wrote:
> Eliminate the follow smatch warning:
> drivers/scsi/pm8001/pm8001_ctl.c:760 pm8001_update_flash() warn:
> inconsistent indenting
> 
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
> ---
>  drivers/scsi/pm8001/pm8001_ctl.c | 65 +++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
> index 41a63c9b719b..213ebf39261f 100644
> --- a/drivers/scsi/pm8001/pm8001_ctl.c
> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
> @@ -755,36 +755,41 @@ static int pm8001_update_flash(struct pm8001_hba_info *pm8001_ha)
>  			fwControl->retcode = 0;/* OUT */
>  			fwControl->offset = loopNumber * IOCTL_BUF_SIZE;/*OUT */
>  
> -		/* for the last chunk of data in case file size is not even with
> -		4k, load only the rest*/
> -		if (((loopcount-loopNumber) == 1) &&
> -			((partitionSize + HEADER_LEN) % IOCTL_BUF_SIZE)) {
> -			fwControl->len =
> -				(partitionSize + HEADER_LEN) % IOCTL_BUF_SIZE;
> -			memcpy((u8 *)fwControl->buffer,
> -				(u8 *)pm8001_ha->fw_image->data + sizeRead,
> -				(partitionSize + HEADER_LEN) % IOCTL_BUF_SIZE);
> -			sizeRead +=
> -				(partitionSize + HEADER_LEN) % IOCTL_BUF_SIZE;
> -		} else {
> -			memcpy((u8 *)fwControl->buffer,
> -				(u8 *)pm8001_ha->fw_image->data + sizeRead,
> -				IOCTL_BUF_SIZE);
> -			sizeRead += IOCTL_BUF_SIZE;
> -		}
> -
> -		pm8001_ha->nvmd_completion = &completion;
> -		ret = PM8001_CHIP_DISP->fw_flash_update_req(pm8001_ha, payload);
> -		if (ret) {
> -			pm8001_ha->fw_status = FAIL_OUT_MEMORY;
> -			goto out;
> -		}
> -		wait_for_completion(&completion);
> -		if (fwControl->retcode > FLASH_UPDATE_IN_PROGRESS) {
> -			pm8001_ha->fw_status = fwControl->retcode;
> -			ret = -EFAULT;
> -			goto out;
> -		}
> +			/* for the last chunk of data in case file
> +			 * size is not even with 4k, load only the rest
> +			 */

Please start multi-line comments with a line that has only "/*". It
would be good to add a blnak line before that too, for readability.

> +			if (((loopcount-loopNumber) == 1) &&
> +				((partitionSize + HEADER_LEN) % IOCTL_BUF_SIZE)) {

While at it, align the conditions together to make this code more
readable. As is, it is really unpleasant to read :)

Also, "(partitionSize + HEADER_LEN) % IOCTL_BUF_SIZE" is calculated way
too many times. Using a local variable for the value would be nice.

> +				fwControl->len =
> +					(partitionSize + HEADER_LEN) %
> +					IOCTL_BUF_SIZE;
> +				memcpy((u8 *)fwControl->buffer,
> +					(u8 *)pm8001_ha->fw_image->data + sizeRead,
> +					(partitionSize + HEADER_LEN) %
> +					IOCTL_BUF_SIZE);
> +				sizeRead +=
> +					(partitionSize + HEADER_LEN) %
> +					IOCTL_BUF_SIZE;
> +			} else {
> +				memcpy((u8 *)fwControl->buffer,
> +					(u8 *)pm8001_ha->fw_image->data + sizeRead,
> +					IOCTL_BUF_SIZE);
> +				sizeRead += IOCTL_BUF_SIZE;
> +			}
> +
> +			pm8001_ha->nvmd_completion = &completion;
> +			ret = PM8001_CHIP_DISP->fw_flash_update_req(pm8001_ha,
> +				payload);
> +			if (ret) {
> +				pm8001_ha->fw_status = FAIL_OUT_MEMORY;
> +				goto out;
> +			}
> +			wait_for_completion(&completion);
> +			if (fwControl->retcode > FLASH_UPDATE_IN_PROGRESS) {
> +				pm8001_ha->fw_status = fwControl->retcode;
> +				ret = -EFAULT;
> +				goto out;
> +			}
>  		}
>  	}
>  out:


-- 
Damien Le Moal
Western Digital Research