| Message ID | 577b57b3.5b4c620a.37648.6c91@mx.google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 7/5/2016 9:45 AM, tom.ty89@gmail.com wrote: > From: Tom Yan <tom.ty89@gmail.com> > > It does not make sense and is confusing to respond with "Invalid > field in CDB" while we have no support at all implemented for > FORMAT UNIT. It is decent to let it go to the default, which > will respond with "Invalid command operation code" instead. > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 029e738..ac5676e 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -307,7 +307,7 @@ static void ata_scsi_set_invalid_field(struct ata_device *dev, > struct scsi_cmnd *cmd, u16 field, u8 bit) > { > ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x24, 0x0); > - /* "Invalid field in cbd" */ > + /* "Invalid field in CDB" */ Don't do 2 things in one patch please> This change wasn't even documented in the patch description. [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I just thought that such a minor change in a comment can fit in the same patch where the issue was first noticed. Anyway, will split them if I am going to send a v3 set. On 5 July 2016 at 19:08, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > On 7/5/2016 9:45 AM, tom.ty89@gmail.com wrote: > >> From: Tom Yan <tom.ty89@gmail.com> >> >> It does not make sense and is confusing to respond with "Invalid >> field in CDB" while we have no support at all implemented for >> FORMAT UNIT. It is decent to let it go to the default, which >> will respond with "Invalid command operation code" instead. >> >> Signed-off-by: Tom Yan <tom.ty89@gmail.com> >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index 029e738..ac5676e 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -307,7 +307,7 @@ static void ata_scsi_set_invalid_field(struct >> ata_device *dev, >> struct scsi_cmnd *cmd, u16 field, >> u8 bit) >> { >> ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x24, 0x0); >> - /* "Invalid field in cbd" */ >> + /* "Invalid field in CDB" */ > > > Don't do 2 things in one patch please> This change wasn't even documented > in the patch description. > > [...] > > MBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Jul 06, 2016 at 03:13:31AM +0800, Tom Yan wrote: > I just thought that such a minor change in a comment can fit in the > same patch where the issue was first noticed. Anyway, will split them > if I am going to send a v3 set. > > On 5 July 2016 at 19:08, Sergei Shtylyov > <sergei.shtylyov@cogentembedded.com> wrote: > > On 7/5/2016 9:45 AM, tom.ty89@gmail.com wrote: > > > >> From: Tom Yan <tom.ty89@gmail.com> > >> > >> It does not make sense and is confusing to respond with "Invalid > >> field in CDB" while we have no support at all implemented for > >> FORMAT UNIT. It is decent to let it go to the default, which > >> will respond with "Invalid command operation code" instead. > >> > >> Signed-off-by: Tom Yan <tom.ty89@gmail.com> > >> > >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > >> index 029e738..ac5676e 100644 > >> --- a/drivers/ata/libata-scsi.c > >> +++ b/drivers/ata/libata-scsi.c > >> @@ -307,7 +307,7 @@ static void ata_scsi_set_invalid_field(struct > >> ata_device *dev, > >> struct scsi_cmnd *cmd, u16 field, > >> u8 bit) > >> { > >> ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x24, 0x0); > >> - /* "Invalid field in cbd" */ > >> + /* "Invalid field in CDB" */ > > > > > > Don't do 2 things in one patch please> This change wasn't even documented > > in the patch description. So, for trivial cleanups, it's fine to do it along with other changes when the cleanups and the said changes are related and close by; however, the description should be able to encompass all changes. It doesn't have to be super detailed. Something like "While at it, do some trivial / typo / whatever cleanups" works fine. Here, the cleanup isn't that close. I'd just drop that chunk. It really doesn't matter whether cdb is in caps or not. Thanks.
Um it's not mainly about in caps or not, but more about wrongly spelled as cbd. On 5 July 2016 at 22:39, Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Wed, Jul 06, 2016 at 03:13:31AM +0800, Tom Yan wrote: >> I just thought that such a minor change in a comment can fit in the >> same patch where the issue was first noticed. Anyway, will split them >> if I am going to send a v3 set. >> >> On 5 July 2016 at 19:08, Sergei Shtylyov >> <sergei.shtylyov@cogentembedded.com> wrote: >> > On 7/5/2016 9:45 AM, tom.ty89@gmail.com wrote: >> > >> >> From: Tom Yan <tom.ty89@gmail.com> >> >> >> >> It does not make sense and is confusing to respond with "Invalid >> >> field in CDB" while we have no support at all implemented for >> >> FORMAT UNIT. It is decent to let it go to the default, which >> >> will respond with "Invalid command operation code" instead. >> >> >> >> Signed-off-by: Tom Yan <tom.ty89@gmail.com> >> >> >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> >> index 029e738..ac5676e 100644 >> >> --- a/drivers/ata/libata-scsi.c >> >> +++ b/drivers/ata/libata-scsi.c >> >> @@ -307,7 +307,7 @@ static void ata_scsi_set_invalid_field(struct >> >> ata_device *dev, >> >> struct scsi_cmnd *cmd, u16 field, >> >> u8 bit) >> >> { >> >> ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x24, 0x0); >> >> - /* "Invalid field in cbd" */ >> >> + /* "Invalid field in CDB" */ >> > >> > >> > Don't do 2 things in one patch please> This change wasn't even documented >> > in the patch description. > > So, for trivial cleanups, it's fine to do it along with other changes > when the cleanups and the said changes are related and close by; > however, the description should be able to encompass all changes. It > doesn't have to be super detailed. Something like "While at it, do > some trivial / typo / whatever cleanups" works fine. > > Here, the cleanup isn't that close. I'd just drop that chunk. It > really doesn't matter whether cdb is in caps or not. > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 06, 2016 at 06:40:32AM +0000, Tom Yan wrote:
> Um it's not mainly about in caps or not, but more about wrongly spelled as cbd.
Heh, right, so please just note it in the description.
Thanks.
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 029e738..ac5676e 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -307,7 +307,7 @@ static void ata_scsi_set_invalid_field(struct ata_device *dev, struct scsi_cmnd *cmd, u16 field, u8 bit) { ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x24, 0x0); - /* "Invalid field in cbd" */ + /* "Invalid field in CDB" */ scsi_set_sense_field_pointer(cmd->sense_buffer, SCSI_SENSE_BUFFERSIZE, field, bit, 1); } @@ -4045,11 +4045,6 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd) args.done = cmd->scsi_done; switch(scsicmd[0]) { - /* TODO: worth improving? */ - case FORMAT_UNIT: - ata_scsi_invalid_field(dev, cmd, 0); - break; - case INQUIRY: if (scsicmd[1] & 2) /* is CmdDt set? */ ata_scsi_invalid_field(dev, cmd, 1);