All of lore.kernel.org
 help / color / mirror / Atom feed
From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: "linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"haowenchao@huawei.com" <haowenchao@huawei.com>
Cc: "liuzhiqiang26@huawei.com" <liuzhiqiang26@huawei.com>
Subject: Re: [PATCH demo] ata_scsi_queuecmd: Make input parameters check more clearly
Date: Wed, 5 Jan 2022 06:04:19 +0000	[thread overview]
Message-ID: <c40f2d6e11cfd428fc59e89b8d7b536d5808af2f.camel@wdc.com> (raw)
In-Reply-To: <20220105021704.1679067-1-haowenchao@huawei.com>

Resending my reply as the server mx5.huawei.com bounces back to me all my
emails from @opensource.wdc.com... (error 554, but SPF record is correct !)


On Tue, 2022-01-04 at 21:17 -0500, Wenchao Hao wrote:
> This is just a clean code. Since each branch of "if" state would check
> scmd->cmd_len, so move the check of scmd->cmd_len out of "if" state to
> simplify the logic of input parameters check.
> 
> The patch do not change origin function logic.
> 
> Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
> ---
>  drivers/ata/libata-scsi.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 313e9475507b..1c653b5327db 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4020,19 +4020,18 @@ void ata_scsi_dump_cdb(struct ata_port *ap, struct scsi_cmnd *cmd)
>  int __ata_scsi_queuecmd(struct scsi_cmnd *scmd, struct ata_device *dev)
>  {
>  	u8 scsi_op = scmd->cmnd[0];
> -	ata_xlat_func_t xlat_func;
> +	ata_xlat_func_t xlat_func = NULL;

Not needed. xlat_func is always set for the non-error cases.

>  	int rc = 0;
>  
> +	if (unlikely(!scmd->cmd_len))
> +		goto bad_cdb_len;
> +
>  	if (dev->class == ATA_DEV_ATA || dev->class == ATA_DEV_ZAC) {
> -		if (unlikely(!scmd->cmd_len || scmd->cmd_len > dev->cdb_len))
> +		if (unlikely(scmd->cmd_len > dev->cdb_len))
>  			goto bad_cdb_len;
>  
>  		xlat_func = ata_get_xlat_func(dev, scsi_op);
>  	} else {
> -		if (unlikely(!scmd->cmd_len))
> -			goto bad_cdb_len;
> -
> -		xlat_func = NULL;
>  		if (likely((scsi_op != ATA_16) || !atapi_passthru16)) {
>  			/* relay SCSI command to ATAPI device */
>  			int len = COMMAND_SIZE(scsi_op);

I would go further and cleanup the if else { if else } sequence too.
Something like this:

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a16ef0030667..ed8be585a98f 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3958,42 +3958,39 @@ int __ata_scsi_queuecmd(struct scsi_cmnd *scmd,
struct ata_device *dev)
 {
 	u8 scsi_op = scmd->cmnd[0];
 	ata_xlat_func_t xlat_func;
-	int rc = 0;
+
+	if (unlikely(!scmd->cmd_len))
+		goto bad_cdb_len;

 	if (dev->class == ATA_DEV_ATA || dev->class == ATA_DEV_ZAC) {
-		if (unlikely(!scmd->cmd_len || scmd->cmd_len > dev->cdb_len))
+		if (unlikely(scmd->cmd_len > dev->cdb_len))
 			goto bad_cdb_len;

 		xlat_func = ata_get_xlat_func(dev, scsi_op);
-	} else {
-		if (unlikely(!scmd->cmd_len))
-			goto bad_cdb_len;
+	} else if (likely((scsi_op != ATA_16) || !atapi_passthru16)) {
+		/* relay SCSI command to ATAPI device */
+		int len = COMMAND_SIZE(scsi_op);

-		xlat_func = NULL;
-		if (likely((scsi_op != ATA_16) || !atapi_passthru16)) {
-			/* relay SCSI command to ATAPI device */
-			int len = COMMAND_SIZE(scsi_op);
-			if (unlikely(len > scmd->cmd_len ||
-				     len > dev->cdb_len ||
-				     scmd->cmd_len > ATAPI_CDB_LEN))
-				goto bad_cdb_len;
+		if (unlikely(len > scmd->cmd_len ||
+			     len > dev->cdb_len ||
+			     scmd->cmd_len > ATAPI_CDB_LEN))
+			goto bad_cdb_len;

-			xlat_func = atapi_xlat;
-		} else {
-			/* ATA_16 passthru, treat as an ATA command */
-			if (unlikely(scmd->cmd_len > 16))
-				goto bad_cdb_len;
+		xlat_func = atapi_xlat;
+	} else {
+		/* ATA_16 passthru, treat as an ATA command */
+		if (unlikely(scmd->cmd_len > 16))
+			goto bad_cdb_len;

-			xlat_func = ata_get_xlat_func(dev, scsi_op);
-		}
+		xlat_func = ata_get_xlat_func(dev, scsi_op);
 	}

 	if (xlat_func)
-		rc = ata_scsi_translate(dev, scmd, xlat_func);
-	else
-		ata_scsi_simulate(dev, scmd);
+		return ata_scsi_translate(dev, scmd, xlat_func);

-	return rc;
+	ata_scsi_simulate(dev, scmd);
+
+	return 0;

  bad_cdb_len:
 	scmd->result = DID_ERROR << 16;

Also please change the patch title prefix to: "ata: libata-scsi: ...."

-- 
Damien Le Moal
Western Digital Research


      parent reply	other threads:[~2022-01-05  6:04 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-05  2:17 [PATCH demo] ata_scsi_queuecmd: Make input parameters check more clearly Wenchao Hao
2022-01-05  5:59 ` Damien Le Moal
2022-01-05  6:04 ` Damien Le Moal [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c40f2d6e11cfd428fc59e89b8d7b536d5808af2f.camel@wdc.com \
    --to=damien.lemoal@wdc.com \
    --cc=haowenchao@huawei.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=liuzhiqiang26@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.