[BK PATCH] 2.6.x libata bug fix

[BK PATCH] 2.6.x libata bug fix

[Jeff Garzik]

When I added ioctl handling, I returned EOPNOTSUPP for the "I didn't
recognize that ioctl you wanted to execute" return code.

The block layer will execute certain ioctl operations IFF the low-level
driver returns ENOTTY, signalling to the block layer that that driver
does not support the specified ioctl.

The net effect of libata returning EOPNOTSUPP was such that it
__broke__ BLSFLSBUF ioctl, simply by implementing ->ioctl.

So, we return the proper return code, and ioctls we don't handle
start to work again.  Overall, though, this is a fragile way to do
things in the block layer, IMHO.

Please do a

	bk pull bk://gkernel.bkbits.net/libata-2.6

This will update the following files:

 drivers/scsi/libata-scsi.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

through these ChangeSets:

<jgarzik@pobox.com> (04/10/26 1.2068)
   [libata] return ENOTTY rather than EOPNOTSUPP for unknown-ioctl

diff -Nru a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
--- a/drivers/scsi/libata-scsi.c	2004-10-26 11:56:47 -04:00
+++ b/drivers/scsi/libata-scsi.c	2004-10-26 11:56:47 -04:00
@@ -97,7 +97,7 @@
 		return 0;
 
 	default:
-		rc = -EOPNOTSUPP;
+		rc = -ENOTTY;
 		break;
 	}
 

Re: [BK PATCH] 2.6.x libata bug fix

[Jens Axboe]

On Tue, Oct 26 2004, Jeff Garzik wrote:
> 
> When I added ioctl handling, I returned EOPNOTSUPP for the "I didn't
> recognize that ioctl you wanted to execute" return code.
> 
> The block layer will execute certain ioctl operations IFF the low-level
> driver returns ENOTTY, signalling to the block layer that that driver
> does not support the specified ioctl.
> 
> The net effect of libata returning EOPNOTSUPP was such that it
> __broke__ BLSFLSBUF ioctl, simply by implementing ->ioctl.
> 
> So, we return the proper return code, and ioctls we don't handle
> start to work again.  Overall, though, this is a fragile way to do
> things in the block layer, IMHO.

Well, it's pretty much the universally accepted way of signalling this
information, I'm not sure I agree. The crappy part is that EINVAL is so
wide spread as well.

And EOPNOTSUPP just shows you are a networking whore as well :-)

-- 
Jens Axboe

Re: [BK PATCH] 2.6.x libata bug fix

[Jeff Garzik]

Jens Axboe wrote:
> On Tue, Oct 26 2004, Jeff Garzik wrote:
>>So, we return the proper return code, and ioctls we don't handle
>>start to work again.  Overall, though, this is a fragile way to do
>>things in the block layer, IMHO.
> 
> 
> Well, it's pretty much the universally accepted way of signalling this
> information, I'm not sure I agree. The crappy part is that EINVAL is so
> wide spread as well.


My point is that most Linux APIs don't apply default behavior by means 
of a magic return code.

Each block/scsi/etc. driver should provide their own ioctl handler as 
the highest-level callback.  Then, each individual driver decides its 
fallback strategy for unknown ioctls -- in most cases, by calling 
"libata_ioctl" or "scsi_ioctl" or "block_ioctl".

In this manner, ioctl handling cascades naturally up through the layers, 
without magic return codes.

The current "top-down" ioctl handling implementation leads to the 
current situation:  a bunch of ->ioctl() callsites scattered through the 
generic block ioctl handling code.

	Jeff

Re: [BK PATCH] 2.6.x libata bug fix

[Jens Axboe]

On Tue, Oct 26 2004, Jeff Garzik wrote:
> Jens Axboe wrote:
> >On Tue, Oct 26 2004, Jeff Garzik wrote:
> >>So, we return the proper return code, and ioctls we don't handle
> >>start to work again.  Overall, though, this is a fragile way to do
> >>things in the block layer, IMHO.
> >
> >
> >Well, it's pretty much the universally accepted way of signalling this
> >information, I'm not sure I agree. The crappy part is that EINVAL is so
> >wide spread as well.
> 
> 
> My point is that most Linux APIs don't apply default behavior by means 
> of a magic return code.
> 
> Each block/scsi/etc. driver should provide their own ioctl handler as 
> the highest-level callback.  Then, each individual driver decides its 
> fallback strategy for unknown ioctls -- in most cases, by calling 
> "libata_ioctl" or "scsi_ioctl" or "block_ioctl".
> 
> In this manner, ioctl handling cascades naturally up through the layers, 
> without magic return codes.

But it doesn't quite work so well if you are a device/driver that needs
to call several of these functions - you get the same problem, you need
to know when it was accepted or when to pass it on. Or you need to know
every single ioctl. Maybe you can kill it in the driver, but you need it
at least some place in the stack.

> The current "top-down" ioctl handling implementation leads to the 
> current situation:  a bunch of ->ioctl() callsites scattered through the 
> generic block ioctl handling code.

It is a bit messy in places, I agree. But I don't think this has
anything to do with it using -ENOTTY as a special magic "I dunno what
you are talking about", rather it just needs a little cleanup action.

Honestly, I don't think -ENOTTY is so nasty. It's been described a
number of times in various places, it should be common knowledge for
anyone doing a linux/unix driver.

-- 
Jens Axboe

[BK PATCH] 2.6.x libata bug fix

[Jeff Garzik]

Sigh...

Please do a

	bk pull bk://gkernel.bkbits.net/libata-2.6

This will update the following files:

 drivers/scsi/libata-scsi.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

through these ChangeSets:

<jgarzik@pobox.com> (04/10/26 1.2069)
   [libata] use kunmap_atomic() correctly

diff -Nru a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
--- a/drivers/scsi/libata-scsi.c	2004-10-26 19:59:30 -04:00
+++ b/drivers/scsi/libata-scsi.c	2004-10-26 19:59:30 -04:00
@@ -742,13 +742,13 @@
  *	spin_lock_irqsave(host_set lock)
  */
 
-static inline void ata_scsi_rbuf_put(struct scsi_cmnd *cmd)
+static inline void ata_scsi_rbuf_put(struct scsi_cmnd *cmd, u8 *buf)
 {
 	if (cmd->use_sg) {
 		struct scatterlist *sg;
 
 		sg = (struct scatterlist *) cmd->request_buffer;
-		kunmap_atomic(sg->page, KM_USER0);
+		kunmap_atomic(buf - sg->offset, KM_USER0);
 	}
 }
 
@@ -778,7 +778,7 @@
 	buflen = ata_scsi_rbuf_get(cmd, &rbuf);
 	memset(rbuf, 0, buflen);
 	rc = actor(args, rbuf, buflen);
-	ata_scsi_rbuf_put(cmd);
+	ata_scsi_rbuf_put(cmd, rbuf);
 
 	if (rc)
 		ata_bad_cdb(cmd, args->done);
@@ -1264,7 +1264,7 @@
 			buflen = ata_scsi_rbuf_get(cmd, &buf);
 			buf[2] = 0x5;
 			buf[3] = (buf[3] & 0xf0) | 2;
-			ata_scsi_rbuf_put(cmd);
+			ata_scsi_rbuf_put(cmd, buf);
 		}
 		cmd->result = SAM_STAT_GOOD;
 	}