On 07/04/2012 08:03 PM, Paolo Bonzini wrote:
>>> The cache_type file in sysfs lets users configure the disk cache in
>>> write-through or write-back modes. However, ata disks do not support
>>> writing to the file because they do not implement the MODE SELECT
>>> command.
>>> This patch adds a translation from MODE SELECT (for the caching page
>>> only) to the ATA SET FEATURES command.
>>> Cc: Jeff Garzik <jgarzik@xxxxxxxxx>
>>> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>> ---
>>> drivers/ata/libata-scsi.c | 147 +++++++++++++++++++++++++++++++++++++++++++--
>>> 1 files changed, 142 insertions(+), 5 deletions(-)
>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>> index 41cde45..e7702d3 100644
>>> --- a/drivers/ata/libata-scsi.c
>>> +++ b/drivers/ata/libata-scsi.c
>>> @@ -3081,6 +3081,144 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>>> }
>>>
>>> /**
>>> + * ata_mselect_caching - Simulate MODE SELECT for caching info page
>>> + * @tf: taskfile to be filled
>>> + * @buf: input buffer
>>> + * @len: number of valid bytes in the input buffer
>>> + *
>>> + * Prepare a taskfile to modify caching information for the device.
>>> + *
>>> + * LOCKING:
>>> + * None.
>>> + */
>>> +static unsigned int ata_mselect_caching(struct ata_taskfile *tf, const u8 *buf,
>>> + int len)
>>> +{
>>> + u8 wce;
>> Empty line after declaration block wouldn't hurt...
>>> + if (len == 0)
>>> + return 1;
>>> +
>>> + /*
>>> + * The first two bytes are a header, so offsets here are 2 less
>>> + * than in ata_msense_caching.
>>> + */
>>> + wce = buf[0] & (1 << 2);
>> Perhaps it's worth denying the command if the other page fields dfifer from
>> 'def_cache_mpage' (i.e. all zeros)?
> I thought about it, but it seemed a useless complication.
If you bothered to implement that, go all the way. :-)
>>> +/**
>>> + * ata_scsi_mode_select_xlat - Translate MODE SELECT 6, 10 commands
>>> + * @qc: Storage for translated ATA taskfile
>>> + *
>>> + * Converts a MODE SELECT command to an ATA SET FEATURES taskfile.
>>> + * Assume this is invoked for direct access devices (e.g. disks) only.
>>> + * There should be no block descriptor for other device types.
>>> + *
>>> + * LOCKING:
>>> + * spin_lock_irqsave(host lock)
>>> + */
>>> +static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
>>> +{
[...]
>>> + /*
>>> + * No mode subpages supported (yet) but asking for _all_
>>> + * subpages may be valid
>>> + */
>>> + if (spg && (spg != ALL_SUB_MPAGES))
>>> + goto invalid_fld;
>>> + if (pg_len > len)
>>> + goto invalid_fld;
>> It's valid to have buffer size less than data size.
> Perhaps for MODE SENSE, but not for MODE SELECT. Otherwise, where do
> you get the mode data from?
They simply don't change?
> Anyhow, checking buffer size vs. data size is done early, in
> + /* Test early for possible overrun. */
> + if (scsi_sglist(scmd)->length < len)
> + goto invalid_fld;
> not here. What this check is doing, is ensuring that the page data does
> not extend past the end of the buffer.
That's what I say is valid. You can get incomplete data, according to the
buffer size. Though maybe it was only my impression...
>>> + switch (pg) {
>>> + case CACHE_MPAGE:
>>> + if (ata_mselect_caching(tf, p, pg_len))
>>> + goto invalid_fld;
>>> + break;
>>> +
>>> + default: /* invalid page code */
>>> + goto invalid_fld;
>> Page code is not a part of CDB, hence the sense code you'll return doesn't
>> fit there.
> Yes, invalid field in parameter list probably would be better.
Or "parameter not supported" (0x26/0x01).
> Let me know if I should respin.
I'd prefer if you respin.
> Thanks for the review!
> Paolo
MBR, Sergei
--
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
