Borislav Petkov wrote:
>> + /*
>> + * Sense is always read into drive->sense_data.
>> + * Copy back if the failed request has its
>> + * sense pointer set.
>> + */
>> + memcpy(failed->sense, sense, 18);
>
> shouldn't this line be:
>
> memcpy(failed->sense, sense, rq->sense);
>
> ?
>
> According to SFF8020i, Section 10.8.20 REQUEST SENSE Command, the sense
> length can be > 18:
It doesn't really matter in this patch. This patch shouldn't change
what number of bytes are considered by ide-cd for sense data. Before
the patch it was 18, so it should remain 18 after the patch.
> "ATAPI CD-ROM Drives shall be capable of returning at least 18 bytes of
> data in response to a REQUEST SENSE command. (...)
>
> Host Computers can determine how much sense data has been returned by
> examining the allocation length parameter in the Command Packet and the
> additional sense length in the sense data. ATAPI CD-ROM Drives shall
> not adjust the additional sense length to reflect truncation if the
> allocation length is less than the sense data available."
>
> So, a possible scenario might be:
>
> We issue a REQUEST_SENSE to a device and it returns more than 18 bytes
> of sense data which should normally fit in the request_sense buffer but
> we only copyback the first 18 bytes. Now, the error handling doesn't
> touch any members behind the 18th byte (sense->asb) but we still subtly
> carry stale data with us resulting in future bugs if one decides to
> touch the additional sense bytes (->asb).
>
> However, is there any reason for copying back the sense bytes at all?
> In other words, does the block layer code need sense data when ending a
> request or is it used only by LLDDs for error handling? Because if I'm
> not missing something, we could just as well use the drive->sense_data
> in the failed->sense case and thus alleviate the need for the copy.
> ide_cd_complete_failed_rq is called on the IRQ path so drive->sense_data
> has to be still valid for the current request is a sense request, no?
>
> As a result and if I'm reading the code correctly, we could simply do
> (pasting the whole function instead of a diff for more clarity) :
>
> static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
> {
> /*
> * For REQ_TYPE_SENSE, "rq->special" points to the original
> * failed request
> */
> struct request *failed = (struct request *)rq->special;
> struct request_sense *sense = &drive->sense_data;
>
> if (failed) {
> if (failed->sense)
> /* Sense is always read into drive->sense_data. */
> failed->sense_len = rq->sense_len;
>
> cdrom_analyze_sense_data(drive, failed, sense);
>
> if (ide_end_rq(drive, failed, -EIO, blk_rq_bytes(failed)))
> BUG();
> } else
> cdrom_analyze_sense_data(drive, NULL, sense);
> }
So, please feel free to submit a separate patch for this. :-)
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
