> + if (is_multi_taskfile(tf)) {
> + unsigned int multi_count = 1 << (cdb[1] >> 5);
> +
> + /* compare the passed through multi_count
> + * with the cached multi_count of libata
> + */
> + if (multi_count != dev->multi_count)
> + ata_dev_printk(dev, KERN_WARNING,
> + "invalid multi_count %u ignored\n",
> + multi_count);
> + }
What limits log spamming here ? Also shouldn't we error this
situation not proceed and hope that enough data was supplied not
to leave us stuck half way through a command having made a nasty
mess on disk ?
Alan Cox wrote:
>> + if (is_multi_taskfile(tf)) {
>> + unsigned int multi_count = 1 << (cdb[1] >> 5);
>> +
>> + /* compare the passed through multi_count
>> + * with the cached multi_count of libata
>> + */
>> + if (multi_count != dev->multi_count)
>> + ata_dev_printk(dev, KERN_WARNING,
>> + "invalid multi_count %u ignored\n",
>> + multi_count);
>> + }
>
> What limits log spamming here ?
Intelligence of the user with privs?
> Also shouldn't we error this
> situation not proceed and hope that enough data was supplied not
> to leave us stuck half way through a command having made a nasty
> mess on disk ?
Is that English? Can you be more specific and more clear?
Jeff
> >> + if (multi_count != dev->multi_count)
> >> + ata_dev_printk(dev, KERN_WARNING,
> >> + "invalid multi_count %u ignored\n",
> >> + multi_count);
> >> + }
> >
> > What limits log spamming here ?
>
> Intelligence of the user with privs?
Not ever SG_IO call requires priviledges. Not all priviledged users are
so perfect they never make a mistake.
> > Also shouldn't we error this
> > situation not proceed and hope that enough data was supplied not
> > to leave us stuck half way through a command having made a nasty
> > mess on disk ?
>
> Is that English? Can you be more specific and more clear?
Issue a multi count sized write. Acidentally collide with another program
which mucks up the multiwrite count (for once it probably won't be HAL
making a mess ;)) and send data. The state of the disk at the point a
short multiwrite terminates is undefined, it may have written some of the
sectors, it may have blanked them all (Optical media) etc. Far better
just to error the invalid command - especially as in some cases without
Mark's fixes for DRQ cleanup on error it may even hang in one direction.
Alan
Alan Cox wrote:
>>>> + if (multi_count != dev->multi_count)
>>>> + ata_dev_printk(dev, KERN_WARNING,
>>>> + "invalid multi_count %u ignored\n",
>>>> + multi_count);
>>>> + }
>>> What limits log spamming here ?
>> Intelligence of the user with privs?
>
> Not ever SG_IO call requires priviledges.
All calls relevant to this thread require CAP_SYS_RAWIO.
> Issue a multi count sized write. Acidentally collide with another program
> which mucks up the multiwrite count (for once it probably won't be HAL
> making a mess ;)) and send data. The state of the disk at the point a
> short multiwrite terminates is undefined, it may have written some of the
> sectors, it may have blanked them all (Optical media) etc. Far better
> just to error the invalid command - especially as in some cases without
> Mark's fixes for DRQ cleanup on error it may even hang in one direction.
ata_scsi_pass_thru() is not executed at ioctl submission time (block
queue submission time), but rather immediately before it is issued to
the drive. At that point you know the bus is idle, all other commands
have finished executing, and dev->multi_count is fresh not stale. The
code path goes from ata_scsi_pass_thru() to ata_qc_issue() without
releasing the spinlock, even.
But the last point is true -- we should error rather than just warn
there, AFAICS.
Jeff
> ata_scsi_pass_thru() is not executed at ioctl submission time (block
> queue submission time), but rather immediately before it is issued to
> the drive. At that point you know the bus is idle, all other commands
> have finished executing, and dev->multi_count is fresh not stale. The
> code path goes from ata_scsi_pass_thru() to ata_qc_issue() without
> releasing the spinlock, even.
Think up to user space
Poorusersapp set multicount to 4
Evilproprietarytuningdaemon set multicount to 8
Poorusersapp issue I/O
at which point an error is indeed best.
> But the last point is true -- we should error rather than just warn
> there, AFAICS.
Definitely. We've been asked "please do something stupid" and not even in
a case where the requester may know better.
Alan
Alan Cox wrote:
>>ata_scsi_pass_thru() is not executed at ioctl submission time (block
>>queue submission time), but rather immediately before it is issued to
>>the drive. At that point you know the bus is idle, all other commands
>>have finished executing, and dev->multi_count is fresh not stale. The
>>code path goes from ata_scsi_pass_thru() to ata_qc_issue() without
>>releasing the spinlock, even.
>
>
> Think up to user space
>
> Poorusersapp set multicount to 4
>
> Evilproprietarytuningdaemon set multicount to 8
>
> Poorusersapp issue I/O
>
> at which point an error is indeed best.
>
>
>>But the last point is true -- we should error rather than just warn
>>there, AFAICS.
>
>
> Definitely. We've been asked "please do something stupid" and not even in
> a case where the requester may know better.
>
It looks like the ATA passthru commands contain more information than
what libata needs to execute a command.
e.g. protocol number:
libata could possibly infer the protocol from the command opcode.
e.g. multi_count:
libata caches dev->multi_count. Passing multi_count along with
each passthru command looks useless for libata.
e.g. t_dir:
libata could possible infer the direction from the command opcode
or from the protocol number (e.g. 4: PIO_IN / 5: PIO_OUT).
Due to the redundant info, there is possiblely inconsistency between
the parameters. e.g. t_dir vs protocol. e.g. command vs protocol.
It seems the "redundant" parameters are designed to allow stateless SATL
implementation: The application/passthru command tells the stateless SATL
implementation the protocol and the multi_count, etc. Then SATL just
follows the instruction blindly, even if asked to do something stupid.
Currently libata
- uses the passthru protocol number blindly
(even if the application issues a DMA command with wrong PIO protocol.)
- checks and warns about multi_count
- ignores t_dir, byte_block and so on.
Maybe we need a strategy to deal with incorrect passed-thru commands?
say,
- check and reject if something wrong
- mimimal check and warn/ignore, if it doesn't hurt command execution
--
albert
Albert Lee wrote:
> ..
> It looks like the ATA passthru commands contain more information than
> what libata needs to execute a command.
>
> e.g. protocol number:
> libata could possibly infer the protocol from the command opcode.
>
> e.g. multi_count:
> libata caches dev->multi_count. Passing multi_count along with
> each passthru command looks useless for libata.
>
> e.g. t_dir:
> libata could possible infer the direction from the command opcode
> or from the protocol number (e.g. 4: PIO_IN / 5: PIO_OUT).
I wonder if the *intent* of the specification was that the low-level driver
should perform whatever setup is necessary to issue the command as given.
So if the command specifies a multcount of 8, then the LLD should issue the
appropriate initialization commands to use a multcount of 8, and then issue
the given R/W MULT command, and then perhaps reset the multcount back to
what it was normally (beforehand) ?
Ditto for the others.
I am *not* proposing that we actually do it this way,
but rather just suggesting a possible rationale.
What does the SAT spec say? Any further hints as to the intent?
Cheers
Albert Lee wrote:
> Alan Cox wrote:
>
>>>ata_scsi_pass_thru() is not executed at ioctl submission time (block
>>>queue submission time), but rather immediately before it is issued to
>>>the drive. At that point you know the bus is idle, all other commands
>>>have finished executing, and dev->multi_count is fresh not stale. The
>>>code path goes from ata_scsi_pass_thru() to ata_qc_issue() without
>>>releasing the spinlock, even.
>>
>>
>>Think up to user space
>>
>>Poorusersapp set multicount to 4
>>
>>Evilproprietarytuningdaemon set multicount to 8
>>
>>Poorusersapp issue I/O
>>
>>at which point an error is indeed best.
>>
>>
>>
>>>But the last point is true -- we should error rather than just warn
>>>there, AFAICS.
>>
>>
>>Definitely. We've been asked "please do something stupid" and not even in
>>a case where the requester may know better.
>>
>
>
> It looks like the ATA passthru commands contain more information than
> what libata needs to execute a command.
>
> e.g. protocol number:
> libata could possibly infer the protocol from the command opcode.
This is generally a bad practice to guess protocol based on opcode. What
if the code will have to handle a vendor unique command (or some other command
not yet known to it but known to issuer)?
> e.g. multi_count:
> libata caches dev->multi_count. Passing multi_count along with
> each passthru command looks useless for libata.
I'd agree here.
> e.g. t_dir:
> libata could possible infer the direction from the command opcode
Bad idea again.
> or from the protocol number (e.g. 4: PIO_IN / 5: PIO_OUT).
This is reasonable if DMA direction can also be inferred from the protocol
number.
> Due to the redundant info, there is possiblely inconsistency between
> the parameters. e.g. t_dir vs protocol. e.g. command vs protocol.
I think it's better to allow inconsistency then to limit functionality.
There's another option though -- let the caller specify the default protocol
for the command to be issued or override it if it *knows* what it's doing.
> It seems the "redundant" parameters are designed to allow stateless SATL
> implementation: The application/passthru command tells the stateless SATL
> implementation the protocol and the multi_count, etc. Then SATL just
> follows the instruction blindly, even if asked to do something stupid.
> Currently libata
> - uses the passthru protocol number blindly
> (even if the application issues a DMA command with wrong PIO protocol.)
> - checks and warns about multi_count
> - ignores t_dir, byte_block and so on.
> Maybe we need a strategy to deal with incorrect passed-thru commands?
> say,
> - check and reject if something wrong
> - mimimal check and warn/ignore, if it doesn't hurt command execution
- let the caller use defaults based on command code or override them.
> --
> albert
MBR, Sergei
Sergei Shtylyov wrote:
> This is generally a bad practice to guess protocol based on opcode.
> What if the code will have to handle a vendor unique command (or some
> other command not yet known to it but known to issuer)?
Agreed -- we simply cannot rely on guessing protocol from opcode, we
MUST take the protocol that is specified.
Jeff