* [PATCH v2 2/2] libata-scsi: do not respond with "invalid field" for FORMAT UNIT [not found] <20160705064556.909-1-me> @ 2016-07-05 6:45 ` tom.ty89 2016-07-05 11:08 ` Sergei Shtylyov 0 siblings, 1 reply; 6+ messages in thread From: tom.ty89 @ 2016-07-05 6:45 UTC (permalink / raw) To: tj, martin.petersen, sergei.shtylyov; +Cc: linux-ide, linux-scsi, Tom Yan 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" */ 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); -- 2.9.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] libata-scsi: do not respond with "invalid field" for FORMAT UNIT 2016-07-05 6:45 ` [PATCH v2 2/2] libata-scsi: do not respond with "invalid field" for FORMAT UNIT tom.ty89 @ 2016-07-05 11:08 ` Sergei Shtylyov 2016-07-05 19:13 ` Tom Yan 0 siblings, 1 reply; 6+ messages in thread From: Sergei Shtylyov @ 2016-07-05 11:08 UTC (permalink / raw) To: tom.ty89, tj, martin.petersen; +Cc: linux-ide, linux-scsi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] libata-scsi: do not respond with "invalid field" for FORMAT UNIT 2016-07-05 11:08 ` Sergei Shtylyov @ 2016-07-05 19:13 ` Tom Yan 2016-07-05 22:39 ` Tejun Heo 0 siblings, 1 reply; 6+ messages in thread From: Tom Yan @ 2016-07-05 19:13 UTC (permalink / raw) To: Sergei Shtylyov; +Cc: tj, martin.petersen, linux-ide, linux-scsi 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] libata-scsi: do not respond with "invalid field" for FORMAT UNIT 2016-07-05 19:13 ` Tom Yan @ 2016-07-05 22:39 ` Tejun Heo 2016-07-06 6:40 ` Tom Yan 0 siblings, 1 reply; 6+ messages in thread From: Tejun Heo @ 2016-07-05 22:39 UTC (permalink / raw) To: Tom Yan; +Cc: Sergei Shtylyov, martin.petersen, linux-ide, linux-scsi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] libata-scsi: do not respond with "invalid field" for FORMAT UNIT 2016-07-05 22:39 ` Tejun Heo @ 2016-07-06 6:40 ` Tom Yan 2016-07-06 13:39 ` Tejun Heo 0 siblings, 1 reply; 6+ messages in thread From: Tom Yan @ 2016-07-06 6:40 UTC (permalink / raw) To: Tejun Heo; +Cc: Sergei Shtylyov, martin.petersen, linux-ide, linux-scsi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] libata-scsi: do not respond with "invalid field" for FORMAT UNIT 2016-07-06 6:40 ` Tom Yan @ 2016-07-06 13:39 ` Tejun Heo 0 siblings, 0 replies; 6+ messages in thread From: Tejun Heo @ 2016-07-06 13:39 UTC (permalink / raw) To: Tom Yan; +Cc: Sergei Shtylyov, martin.petersen, linux-ide, linux-scsi 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. -- tejun ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-06 13:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20160705064556.909-1-me>
2016-07-05 6:45 ` [PATCH v2 2/2] libata-scsi: do not respond with "invalid field" for FORMAT UNIT tom.ty89
2016-07-05 11:08 ` Sergei Shtylyov
2016-07-05 19:13 ` Tom Yan
2016-07-05 22:39 ` Tejun Heo
2016-07-06 6:40 ` Tom Yan
2016-07-06 13:39 ` Tejun Heo
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.