* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).