libata: BLKFLSBUF ioctl broken

libata: BLKFLSBUF ioctl broken

[Mark Lord]

Like the subject line says.. BLKFLSBUF (and friends) are broken for SATA.
I'm getting tons of emails about this now that 2.6.9 is out.

Jeff: any chance of you fixing this soon?

Cheers
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Re: libata: BLKFLSBUF ioctl broken

[Jeff Garzik]

On Thu, Oct 21, 2004 at 05:04:14PM -0400, Mark Lord wrote:
> Like the subject line says.. BLKFLSBUF (and friends) are broken for SATA.
> I'm getting tons of emails about this now that 2.6.9 is out.
> 
> Jeff: any chance of you fixing this soon?

More details...  define broken.

libata doesn't handle BLKFLSBUF directly, and never has, 
so nothing changed in that regard for 2.6.9.

	Jeff

Re: libata: BLKFLSBUF ioctl broken

[Mark Lord]

Try issuing a BLKFLSBUF to a SATA drive.
As of 2.6.9 this apparently returns EOPNOTSUPP to userspace.

I don't have any libata controllers here to try it with.
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[PATCH] Re: libata: BLKFLSBUF ioctl broken

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

Mark Lord wrote:
> Try issuing a BLKFLSBUF to a SATA drive.
> As of 2.6.9 this apparently returns EOPNOTSUPP to userspace.

Ok, I think I see what's going on.  Can you get your users to try the 
attached patch?

Maybe libata should be returning ENOTTY, but that's _quite_ nonsensical 
considering that EOPNOTSUPP describes the error much better (and is a 
standard I-don't-support-that-operation return code in tons of other 
Linux ioctl-handling code).


> I don't have any libata controllers here to try it with.

You mean you don't have an Intel PATA controller lying around 
anywhere???  :)  To enable Intel PATA controllers, set the 
ATA_ENABLE_PATA define in include/linux/libata.h, and recompile the 
ata_piix driver:

#undef ATA_DEBUG                /* debugging output */
#undef ATA_VERBOSE_DEBUG        /* yet more debugging output */
#undef ATA_IRQ_TRAP             /* define to ack screaming irqs */
#undef ATA_NDEBUG               /* define to disable quick runtime checks */
#undef ATA_ENABLE_ATAPI         /* define to enable ATAPI support */
#undef ATA_ENABLE_PATA          /* define to enable PATA support in some
                                  * low-level drivers */
#undef ATAPI_ENABLE_DMADIR      /* enables ATAPI DMADIR bridge support */

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 476 bytes --]

===== drivers/block/ioctl.c 1.62 vs edited =====
--- 1.62/drivers/block/ioctl.c	2004-10-03 00:05:10 -04:00
+++ edited/drivers/block/ioctl.c	2004-10-22 13:06:16 -04:00
@@ -195,7 +195,8 @@
 		if (disk->fops->ioctl) {
 			ret = disk->fops->ioctl(inode, file, cmd, arg);
 			/* -EINVAL to handle old uncorrected drivers */
-			if (ret != -EINVAL && ret != -ENOTTY)
+			if (ret != -EINVAL && ret != -ENOTTY &&
+			    ret != -EOPNOTSUPP)
 				return ret;
 		}
 		fsync_bdev(bdev);

Re: [PATCH] Re: libata: BLKFLSBUF ioctl broken

[Alan Cox]

On Gwe, 2004-10-22 at 18:07, Jeff Garzik wrote:
> Ok, I think I see what's going on.  Can you get your users to try the 
> attached patch?

This patch is definitely wrong.

> Maybe libata should be returning ENOTTY, but that's _quite_ nonsensical 
> considering that EOPNOTSUPP describes the error much better (and is a 
> standard I-don't-support-that-operation return code in tons of other 
> Linux ioctl-handling code).

libata should be returning -ENOTTY "I don't know what you are talking
about", and the core block layer will then fallback and do generic
handling

-EOPNOTSUPP is saying "I do know what is going on but its broken so
fail it totally".

Re: [PATCH] Re: libata: BLKFLSBUF ioctl broken

[Jeff Garzik]

Alan Cox wrote:
> libata should be returning -ENOTTY "I don't know what you are talking
> about", and the core block layer will then fallback and do generic
> handling
> 
> -EOPNOTSUPP is saying "I do know what is going on but its broken so
> fail it totally".

Are you describing behavior mandated in some standard?

Because that differs with _tons_ of existing ioctl code, whose default 
return code is EOPNOTSUPP when no known operation is found in a C 
"switch(cmd)" statement.

grep for it...

	Jeff

Re: [PATCH] Re: libata: BLKFLSBUF ioctl broken

[Mark Lord]

The best I can find (with a quick google) is:

http://www.opengroup.org/onlinepubs/009695399/basedefs/errno.h.html

[ENOTTY]
     Inappropriate I/O control operation.
[EOPNOTSUPP]
     Operation not supported on socket.

Cheers
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Re: [PATCH] Re: libata: BLKFLSBUF ioctl broken

[Alan Cox]

On Gwe, 2004-10-22 at 18:20, Jeff Garzik wrote:
> > -EOPNOTSUPP is saying "I do know what is going on but its broken so
> > fail it totally".
> 
> Are you describing behavior mandated in some standard?

Yes. ENOTTY means "the ioctl you threw at me isn't one I understand",
always has been in Unix proper.

-EINVAL: "I recognize this ioctl but the arguments are bogus"
-ENOTTY: "I don't recognize this ioctl at all"
-ENOIOCTLCMD: Internal (never user visible) used by some driver layers 
to pass down "please do the defaults". This one might be a sane addition
to the block layer list.

> Because that differs with _tons_ of existing ioctl code, whose default 
> return code is EOPNOTSUPP when no known operation is found in a C 
> "switch(cmd)" statement.
> 
> grep for it...

We are slowly fixing the abusers of -EINVAL/-EOPNOTSUPP. Note btw that
networking stuff is in many places a little different in its returns.

Alan

Re: [PATCH] Re: libata: BLKFLSBUF ioctl broken

[Doug Maxey]

On Fri, 22 Oct 2004 19:16:59 BST, Alan Cox wrote:
>On Gwe, 2004-10-22 at 18:20, Jeff Garzik wrote:
>> > -EOPNOTSUPP is saying "I do know what is going on but its broken so
>> > fail it totally".
>> 
>> Are you describing behavior mandated in some standard?
>
>Yes. ENOTTY means "the ioctl you threw at me isn't one I understand",
>always has been in Unix proper.
>
>-EINVAL: "I recognize this ioctl but the arguments are bogus"
>-ENOTTY: "I don't recognize this ioctl at all"
>-ENOIOCTLCMD: Internal (never user visible) used by some driver layers 
>to pass down "please do the defaults". This one might be a sane addition
>to the block layer list.
>
>> Because that differs with _tons_ of existing ioctl code, whose default 
>> return code is EOPNOTSUPP when no known operation is found in a C 
>> "switch(cmd)" statement.
>> 
>> grep for it...
>
>We are slowly fixing the abusers of -EINVAL/-EOPNOTSUPP. Note btw that
>networking stuff is in many places a little different in its returns.

What about the case of "This is normally a valid command, but this
particulat device is incapable of this particular subcommand"?

Hypothetical instance: a disk drive that indicates it is not capable of 
changing it's write cache setting?


>
>Alan
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] Re: libata: BLKFLSBUF ioctl broken

[Alan Cox]

On Gwe, 2004-10-22 at 20:42, Doug Maxey wrote:
> >We are slowly fixing the abusers of -EINVAL/-EOPNOTSUPP. Note btw that
> >networking stuff is in many places a little different in its returns.
> 
> What about the case of "This is normally a valid command, but this
> particulat device is incapable of this particular subcommand"?
> 
> Hypothetical instance: a disk drive that indicates it is not capable of 
> changing it's write cache setting?

Thats less clear. Traditional Unix has tended to use EINVAL for that
too, while Linux re-uses things like EOPNOTSUPP and that may well be a
better approach - its at least fairly consistent in Linux.

Re: [PATCH] Re: libata: BLKFLSBUF ioctl broken

[Bryan Henderson]

>What about the case of "This is normally a valid command, but this
>particulat device is incapable of this particular subcommand"?
>
>Hypothetical instance: a disk drive that indicates it is not capable of 
>changing it's write cache setting?

I believe there is a fundamental difference between "A device of this kind 
can't do that" and "that doesn't make any sense for a device of this 
kind."  It's the difference between a request to boil the ocean and a 
request to boil Christmas.  ENOTTY is for the request to boil Christmas. 
If you look at the history (from when it just meant "hey, this isn't a 
terminal (tty)"), it's clear:  An ioctl to set the backspace character on 
a disk drive isn't impossible -- it's nonsensical.

--
Bryan Henderson                          IBM Almaden Research Center
San Jose CA                              Filesystems

Re: [PATCH] Re: libata: BLKFLSBUF ioctl broken

[Bryan Henderson]

>-EINVAL: "I recognize this ioctl but the arguments are bogus"
>-ENOTTY: "I don't recognize this ioctl at all"

You mean of course "this ioctl command code."

--
Bryan Henderson                          IBM Almaden Research Center
San Jose CA                              Filesystems