| Message ID | 1436188508-1539-6-git-send-email-sagig@mellanox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/06/2015 06:15 AM, Sagi Grimberg wrote: > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 0181f8b..79bb8d1 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -2625,6 +2625,7 @@ struct sense_info { > u8 asc; > u8 ascq; > bool add_sector_info; > + int desc_format; > }; Something minor: has it been considered to use the data type "bool" instead of "int" for desc_format ? Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/6/2015 6:28 PM, Bart Van Assche wrote: > On 07/06/2015 06:15 AM, Sagi Grimberg wrote: >> diff --git a/drivers/target/target_core_transport.c >> b/drivers/target/target_core_transport.c >> index 0181f8b..79bb8d1 100644 >> --- a/drivers/target/target_core_transport.c >> +++ b/drivers/target/target_core_transport.c >> @@ -2625,6 +2625,7 @@ struct sense_info { >> u8 asc; >> u8 ascq; >> bool add_sector_info; >> + int desc_format; >> }; > > Something minor: has it been considered to use the data type "bool" > instead of "int" for desc_format ? I've considered that, but since scsi_build_sense_buffer() desc argument is an int, I figured it would be better than passing desc_format ? 1 : 0 But I can change it if you prefer. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/06/2015 09:14 AM, Sagi Grimberg wrote: > On 7/6/2015 6:28 PM, Bart Van Assche wrote: >> On 07/06/2015 06:15 AM, Sagi Grimberg wrote: >>> diff --git a/drivers/target/target_core_transport.c >>> b/drivers/target/target_core_transport.c >>> index 0181f8b..79bb8d1 100644 >>> --- a/drivers/target/target_core_transport.c >>> +++ b/drivers/target/target_core_transport.c >>> @@ -2625,6 +2625,7 @@ struct sense_info { >>> u8 asc; >>> u8 ascq; >>> bool add_sector_info; >>> + int desc_format; >>> }; >> >> Something minor: has it been considered to use the data type "bool" >> instead of "int" for desc_format ? > > I've considered that, but since scsi_build_sense_buffer() desc argument > is an int, I figured it would be better than passing desc_format ? 1 : 0 > > But I can change it if you prefer. Hello Sagi, The C language supports implicit conversion from bool to int so I think "? 1 : 0" is not necessary to convert a bool into an int. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 06, 2015 at 04:15:08PM +0300, Sagi Grimberg wrote: > PI errors should be reported in a descriptor format sense data. > Fix that by adding a desc_format flag to struct sense_info and pass > it to scsi_build_sense_buffer() to do the right thing. Do we really need the additional flag? We only need the descriptor sense format because we add the sector information. So just checking for that should be enough, especially when paired with a comment explaining that logic in the source file. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/8/2015 1:19 PM, Christoph Hellwig wrote: > On Mon, Jul 06, 2015 at 04:15:08PM +0300, Sagi Grimberg wrote: >> PI errors should be reported in a descriptor format sense data. >> Fix that by adding a desc_format flag to struct sense_info and pass >> it to scsi_build_sense_buffer() to do the right thing. > > Do we really need the additional flag? We only need the descriptor > sense format because we add the sector information. So just checking > for that should be enough, especially when paired with a comment > explaining that logic in the source file. We don't have any other information today, but sector is not the only information that is requires a descriptor format, so maybe it will be a bit awkward to condition the descriptor format on the sector info? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" 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 08, 2015 at 01:36:04PM +0300, Sagi Grimberg wrote: > We don't have any other information today, but sector is not the only > information that is requires a descriptor format, so maybe it will be a > bit awkward to condition the descriptor format on the sector info? The only reason why you'd want to support descriptor type sense data is because you need to add a second descriptor. If we have another case that needs descriptor sense data it'll also need to add that additional descriptor. So we'll need a conditional for it in the sense data generation anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/08/2015 12:49 PM, Christoph Hellwig wrote: > On Wed, Jul 08, 2015 at 01:36:04PM +0300, Sagi Grimberg wrote: >> We don't have any other information today, but sector is not the only >> information that is requires a descriptor format, so maybe it will be a >> bit awkward to condition the descriptor format on the sector info? > > The only reason why you'd want to support descriptor type sense data is > because you need to add a second descriptor. If we have another case > that needs descriptor sense data it'll also need to add that additional > descriptor. So we'll need a conditional for it in the sense data > generation anyway. > Actually it's controlled by the D_SENSE bit in the Control mode page (that's bit[2] of byte 2 in the control mode page). Which is currently set to '0', ie we will be returning fixed sense information. _If_ we were to report descriptor sense we will need to change that, too. And it's actually not true that you'd need descriptor sense to encode the sector information; it'll be stored in the 'information' section (byte 3-6) for fixed format sense. Cheers, Hannes
On 7/8/2015 1:59 PM, Hannes Reinecke wrote: > On 07/08/2015 12:49 PM, Christoph Hellwig wrote: >> On Wed, Jul 08, 2015 at 01:36:04PM +0300, Sagi Grimberg wrote: >>> We don't have any other information today, but sector is not the only >>> information that is requires a descriptor format, so maybe it will be a >>> bit awkward to condition the descriptor format on the sector info? >> >> The only reason why you'd want to support descriptor type sense data is >> because you need to add a second descriptor. If we have another case >> that needs descriptor sense data it'll also need to add that additional >> descriptor. So we'll need a conditional for it in the sense data >> generation anyway. >> > Actually it's controlled by the D_SENSE bit in the Control mode page > (that's bit[2] of byte 2 in the control mode page). > Which is currently set to '0', ie we will be returning fixed sense > information. > _If_ we were to report descriptor sense we will need to change that, > too. I missed that bit. > > And it's actually not true that you'd need descriptor sense to > encode the sector information; it'll be stored in the 'information' > section (byte 3-6) for fixed format sense. But when I return the sector info in a fixed size format, the initiator is not able to decode the faulty sector: kernel: DIFv1 Type 1 reference failed on sector: 15 tag: 0xfffffff0 sector MSB: 0x0000000f kernel: sd 10:0:1:0: [sdc] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE kernel: sd 10:0:1:0: [sdc] tag#0 Sense Key : Aborted Command [current] kernel: sd 10:0:1:0: [sdc] tag#0 Add. Sense: No additional sense information kernel: sd 10:0:1:0: [sdc] tag#0 CDB: Read(10) 28 20 00 00 00 00 00 00 10 00 kernel: blk_update_request: I/O error, dev sdc, sector 0 Is that a bug? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" 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 08, 2015 at 12:59:18PM +0200, Hannes Reinecke wrote: > Actually it's controlled by the D_SENSE bit in the Control mode page > (that's bit[2] of byte 2 in the control mode page). > Which is currently set to '0', ie we will be returning fixed sense > information. > _If_ we were to report descriptor sense we will need to change that, > too. Just looked over SPC, and indeed D_SENSE is a strict either fixed or descriptor, not a may return descriptor data. So this patch actually is wrong as we never must return different sense data types based on the sense code. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/8/2015 2:44 PM, Christoph Hellwig wrote: > On Wed, Jul 08, 2015 at 12:59:18PM +0200, Hannes Reinecke wrote: >> Actually it's controlled by the D_SENSE bit in the Control mode page >> (that's bit[2] of byte 2 in the control mode page). >> Which is currently set to '0', ie we will be returning fixed sense >> information. >> _If_ we were to report descriptor sense we will need to change that, >> too. > > Just looked over SPC, and indeed D_SENSE is a strict either fixed > or descriptor, not a may return descriptor data. > > So this patch actually is wrong as we never must return different sense > data types based on the sense code. > I'll send out v4 without this patch altogether. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 0181f8b..79bb8d1 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -2625,6 +2625,7 @@ struct sense_info { u8 asc; u8 ascq; bool add_sector_info; + int desc_format; }; static const struct sense_info sense_info_table[] = { @@ -2708,18 +2709,21 @@ static const struct sense_info sense_info_table[] = { .asc = 0x10, .ascq = 0x01, /* LOGICAL BLOCK GUARD CHECK FAILED */ .add_sector_info = true, + .desc_format = 1, }, [TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED] = { .key = ILLEGAL_REQUEST, .asc = 0x10, .ascq = 0x02, /* LOGICAL BLOCK APPLICATION TAG CHECK FAILED */ .add_sector_info = true, + .desc_format = 1, }, [TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED] = { .key = ILLEGAL_REQUEST, .asc = 0x10, .ascq = 0x03, /* LOGICAL BLOCK REFERENCE TAG CHECK FAILED */ .add_sector_info = true, + .desc_format = 1, }, [TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE] = { /* @@ -2758,7 +2762,7 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason) ascq = si->ascq; } - scsi_build_sense_buffer(0, buffer, si->key, asc, ascq); + scsi_build_sense_buffer(si->desc_format, buffer, si->key, asc, ascq); if (si->add_sector_info) scsi_set_sense_information(buffer, cmd->bad_sector); }
PI errors should be reported in a descriptor format sense data. Fix that by adding a desc_format flag to struct sense_info and pass it to scsi_build_sense_buffer() to do the right thing. Signed-off-by: Sagi Grimberg <sagig@mellanox.com> --- drivers/target/target_core_transport.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)