Re: [PATCH 12/15] ide-cd: convert to using generic sense request

[Tejun Heo <tj@kernel.org>]

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