| Message ID | 20220105202747.1963919-1-haowenchao@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ata: libata-scsi: Make __ata_scsi_queuecmd() parameters check more clearly | expand |
On Wed, 2022-01-05 at 15:27 -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 input parameters check. > > And remove redundant init of xlat_func at hand > > The patch do not change origin function logic. > > Signed-off-by: Wenchao Hao <haowenchao@huawei.com> > --- > drivers/ata/libata-scsi.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 313e9475507b..ab8a2833dfec 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -4023,16 +4023,15 @@ int __ata_scsi_queuecmd(struct scsi_cmnd *scmd, struct ata_device *dev) > 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; > - > - xlat_func = NULL; > if (likely((scsi_op != ATA_16) || !atapi_passthru16)) { > /* relay SCSI command to ATAPI device */ > int len = COMMAND_SIZE(scsi_op); Did you miss my reply ? This change is OK, but while at it, let's cleanup this function further. I suggested something like this, which includes your changes. 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 (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; Do you see any problem with this change ?
On 2022/1/5 15:39, Damien Le Moal wrote: > On Wed, 2022-01-05 at 15:27 -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 input parameters check. >> >> And remove redundant init of xlat_func at hand >> >> The patch do not change origin function logic. >> >> Signed-off-by: Wenchao Hao <haowenchao@huawei.com> >> --- >> drivers/ata/libata-scsi.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index 313e9475507b..ab8a2833dfec 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -4023,16 +4023,15 @@ int __ata_scsi_queuecmd(struct scsi_cmnd *scmd, struct ata_device *dev) >> 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; >> - >> - xlat_func = NULL; >> if (likely((scsi_op != ATA_16) || !atapi_passthru16)) { >> /* relay SCSI command to ATAPI device */ >> int len = COMMAND_SIZE(scsi_op); > > Did you miss my reply ? > This change is OK, but while at it, let's cleanup this function further. > I suggested something like this, which includes your changes. > Maybe I misunderstood your previous reply. I think you ask me to change prefix. > 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 (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; > > Do you see any problem with this change ? > This change looks good to me. Should I include this change in next patch? Or you would do this by youself?
On Wed, 2022-01-05 at 16:48 +0800, Wenchao Hao wrote: > On 2022/1/5 15:39, Damien Le Moal wrote: > > On Wed, 2022-01-05 at 15:27 -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 input parameters check. > > > > > > And remove redundant init of xlat_func at hand > > > > > > The patch do not change origin function logic. > > > > > > Signed-off-by: Wenchao Hao <haowenchao@huawei.com> > > > --- > > > drivers/ata/libata-scsi.c | 9 ++++----- > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > > index 313e9475507b..ab8a2833dfec 100644 > > > --- a/drivers/ata/libata-scsi.c > > > +++ b/drivers/ata/libata-scsi.c > > > @@ -4023,16 +4023,15 @@ int __ata_scsi_queuecmd(struct scsi_cmnd *scmd, struct ata_device *dev) > > > 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; > > > - > > > - xlat_func = NULL; > > > if (likely((scsi_op != ATA_16) || !atapi_passthru16)) { > > > /* relay SCSI command to ATAPI device */ > > > int len = COMMAND_SIZE(scsi_op); > > > > Did you miss my reply ? > > This change is OK, but while at it, let's cleanup this function further. > > I suggested something like this, which includes your changes. > > > > Maybe I misunderstood your previous reply. I think you ask me to change > prefix. Yes, I asked that too. It is OK now. > > > 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 (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; > > > > Do you see any problem with this change ? > > > > This change looks good to me. Should I include this change in next > patch? Or you would do this by youself? Just include the change in your v3, that's fine. But test please. I did test what I proposed and it worked fine but I prefer more tests. And given that the patch does more that just changing the command checks, maybe change the patch title to something like: ata: libata-scsi: simplify __ata_scsi_queuecmd()
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 313e9475507b..ab8a2833dfec 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -4023,16 +4023,15 @@ int __ata_scsi_queuecmd(struct scsi_cmnd *scmd, struct ata_device *dev) 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; - - xlat_func = NULL; if (likely((scsi_op != ATA_16) || !atapi_passthru16)) { /* relay SCSI command to ATAPI device */ int len = COMMAND_SIZE(scsi_op);
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 input parameters check. And remove redundant init of xlat_func at hand The patch do not change origin function logic. Signed-off-by: Wenchao Hao <haowenchao@huawei.com> --- drivers/ata/libata-scsi.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)