Re: libata passthru: support PIO multi commands

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux