[PATCH] st.c for GET_IDLUN 2.6.6-rc2

[PATCH] st.c for GET_IDLUN 2.6.6-rc2

[Douglas Gilbert]

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

Kai,
A little more testing of st's SG_IO ioctl turned up a
small problem.

This is the corresponding patch that was applied to the
sd driver when it received the block layer SG_IO ioctl.

For least surprise of lk 2.4 utilities that use the
SCSI_IOCTL_GET_IDLUN and SCSI_IOCTL_GET_BUS_NUMBER
ioctls (e.g. sg_map) it is better to return the correct
values rather than 0.

BTW Does cdrecord correctly distinguish between 2
(non-scsi) ATAPI cd writers on the same system in
lk 2.6 ?

Doug Gilbert

[-- Attachment #2: st266rc2.diff --]
[-- Type: text/plain, Size: 629 bytes --]

--- linux/drivers/scsi/st.c	2004-04-05 20:49:34.000000000 +1000
+++ linux/drivers/scsi/st.c266rc2id	2004-04-25 18:13:50.272071680 +1000
@@ -3402,11 +3402,17 @@
 		goto out;
 	}
 	up(&STp->lock);
-	i = scsi_cmd_ioctl(STp->disk, cmd_in, arg);
-	if (i != -ENOTTY)
-		return i;
-	else
-		return scsi_ioctl(STp->device, cmd_in, (void *) arg);
+	switch (cmd_in) {
+		case SCSI_IOCTL_GET_IDLUN:
+		case SCSI_IOCTL_GET_BUS_NUMBER:
+			break;
+		default:
+			i = scsi_cmd_ioctl(STp->disk, cmd_in, arg);
+			if (i != -ENOTTY)
+				return i;
+			break;
+	}
+	return scsi_ioctl(STp->device, cmd_in, (void *) arg);
 
  out:
 	up(&STp->lock);

Re: [PATCH] st.c for GET_IDLUN 2.6.6-rc2

[Kai Makisara]

On Sun, 25 Apr 2004, Douglas Gilbert wrote:

> Kai,
> A little more testing of st's SG_IO ioctl turned up a
> small problem.
> 
> This is the corresponding patch that was applied to the
> sd driver when it received the block layer SG_IO ioctl.
> 
> For least surprise of lk 2.4 utilities that use the
> SCSI_IOCTL_GET_IDLUN and SCSI_IOCTL_GET_BUS_NUMBER
> ioctls (e.g. sg_map) it is better to return the correct
> values rather than 0.
> 
I definitely agree that those ioctl should not return 0 for SCSI devices. 
However, I think this is a bug in drivers/block/scsi_ioctl.c. Making the 
high-level SCSI drivers parse the ioctl covers up the bug but, IMHO, it is 
not a clean solution. The following two solutions come into my mind:

1. Fixing drivers/block/scsi_ioctl.c. The simple solution would be to 
return -ENOTTY and then the call would be redirected to the SCSI ioctl 
that does the proper thing. There probably is some reason why this has not 
been done. Other fixes would require knowing if the transport type 
implements the ioctl and this complicates things.

2. Reversing the order of calls in st so that scsi_ioctl is called first 
and if it returns -ENOTTY, then scsi_cmd_ioctl is called. This would 
require changing scsi_ioctl to return -ENOTTY for commands not being 
handled but would be othewise a logical solution: first try SCSI specific 
implementation of the ioctl, if it does not exist, then try the block 
layer version (Linux block layer, not Unix block layer ;-).

-- 
Kai

Re: [PATCH] st.c for GET_IDLUN 2.6.6-rc2

[Douglas Gilbert]

Kai Makisara wrote:
> On Sun, 25 Apr 2004, Douglas Gilbert wrote:
> 
> 
>>Kai,
>>A little more testing of st's SG_IO ioctl turned up a
>>small problem.
>>
>>This is the corresponding patch that was applied to the
>>sd driver when it received the block layer SG_IO ioctl.
>>
>>For least surprise of lk 2.4 utilities that use the
>>SCSI_IOCTL_GET_IDLUN and SCSI_IOCTL_GET_BUS_NUMBER
>>ioctls (e.g. sg_map) it is better to return the correct
>>values rather than 0.
>>
> 
> I definitely agree that those ioctl should not return 0 for SCSI devices. 
> However, I think this is a bug in drivers/block/scsi_ioctl.c. Making the 
> high-level SCSI drivers parse the ioctl covers up the bug but, IMHO, it is 
> not a clean solution. The following two solutions come into my mind:
> 
> 1. Fixing drivers/block/scsi_ioctl.c. The simple solution would be to 
> return -ENOTTY and then the call would be redirected to the SCSI ioctl 
> that does the proper thing. There probably is some reason why this has not 
> been done. Other fixes would require knowing if the transport type 
> implements the ioctl and this complicates things.

One (!@#$%) reason for putting SG_IO ioctl and friends in the
block layer was to trick cdrecord that it was talking to the
sg device :-) Older versions of cdrecord won't take kindly to
receiving -ENOTTY from the SCSI_IOCTL_GET_IDLUN ioctl.
However always returning 0 may trick cdrecord if there are
2 or more cd/dvd writers (hence my question at the end of my
last post on this thread).

> 2. Reversing the order of calls in st so that scsi_ioctl is called first 
> and if it returns -ENOTTY, then scsi_cmd_ioctl is called. This would 
> require changing scsi_ioctl to return -ENOTTY for commands not being 
> handled but would be othewise a logical solution: first try SCSI specific 
> implementation of the ioctl, if it does not exist, then try the block 
> layer version (Linux block layer, not Unix block layer ;-).

This solution looks cleaner.

Doug Gilbert

Re: [PATCH] st.c for GET_IDLUN 2.6.6-rc2

[Kai Makisara]

On Mon, 26 Apr 2004, Douglas Gilbert wrote:

> Kai Makisara wrote:
....
> > 2. Reversing the order of calls in st so that scsi_ioctl is called first 
> > and if it returns -ENOTTY, then scsi_cmd_ioctl is called. This would 
> > require changing scsi_ioctl to return -ENOTTY for commands not being 
> > handled but would be othewise a logical solution: first try SCSI specific 
> > implementation of the ioctl, if it does not exist, then try the block 
> > layer version (Linux block layer, not Unix block layer ;-).
> 
> This solution looks cleaner.
> 
I looked at the necessary changes a little more carefully and I think
your solution is better from the practical point of view. The problem in  
my solution is to make sure that scsi_ioctl() returns -ENOTTY in case it 
does not implement the ioctl. This is easy except for the default case:

                if (sdev->host->hostt->ioctl)
                        return sdev->host->hostt->ioctl(sdev, cmd, arg);

where any kind of code can be found ;-) It is safer to leave this as the
last resort.

-- 
Kai

Re: [PATCH] st.c for GET_IDLUN 2.6.6-rc2

[Douglas Gilbert]

Kai Makisara wrote:
> On Mon, 26 Apr 2004, Douglas Gilbert wrote:
> 
> 
>>Kai Makisara wrote:
> 
> ....
> 
>>>2. Reversing the order of calls in st so that scsi_ioctl is called first 
>>>and if it returns -ENOTTY, then scsi_cmd_ioctl is called. This would 
>>>require changing scsi_ioctl to return -ENOTTY for commands not being 
>>>handled but would be othewise a logical solution: first try SCSI specific 
>>>implementation of the ioctl, if it does not exist, then try the block 
>>>layer version (Linux block layer, not Unix block layer ;-).
>>
>>This solution looks cleaner.
>>
> 
> I looked at the necessary changes a little more carefully and I think
> your solution is better from the practical point of view. The problem in  
> my solution is to make sure that scsi_ioctl() returns -ENOTTY in case it 
> does not implement the ioctl. This is easy except for the default case:
> 
>                 if (sdev->host->hostt->ioctl)
>                         return sdev->host->hostt->ioctl(sdev, cmd, arg);
> 
> where any kind of code can be found ;-) It is safer to leave this as the
> last resort.

Kai,
Here is another take on this matter. A summary of the ioctls
defined in block/scsi_ioctl.c follows with my comments:

ioctls defined in
block/scsi_ioctl.c          Comment
==================          =======
SCSI_IOCTL_SEND_COMMAND     redirects to scsi mid level
SCSI_IOCTL_GET_IDLUN        yields 0 (wrong, trips up cdrecord??)
SCSI_IOCTL_GET_BUS_NUMBER   yields 0 (wrong)
SG_EMULATED_HOST            irrelevant to st (obsolete in sg)
SG_GET_RESERVED_SIZE        irrelevant to st (to trick cdrecord)
SG_GET_TIMEOUT              irrelevant to st (to trick cdrecord)
SG_GET_VERSION_NUM          irrelevant to st (to trick cdrecord)
SG_IO                       <<< only ioctl useful to st (+ osst) >>
SG_SET_RESERVED_SIZE        irrelevant to st (to trick cdrecord)
SG_SET_TIMEOUT              irrelevant to st (to trick cdrecord)
CDROM_SEND_PACKET           irrelevant
CDROMCLOSETRAY              irrelevant
CDROMEJECT                  irrelevant

So if st just redirects only the SG_IO ioctl to scsi_cmd_ioctl()
and lets the rest go to the scsi mid level (from where they can
dribble down to the LLD) that solves the problem.

Doug Gilbert

Re: [PATCH] st.c for GET_IDLUN 2.6.6-rc2

[Kai Makisara]

On Wed, 28 Apr 2004, Douglas Gilbert wrote:

...
> Kai,
> Here is another take on this matter. A summary of the ioctls
> defined in block/scsi_ioctl.c follows with my comments:
> 
> ioctls defined in
> block/scsi_ioctl.c          Comment
> ==================          =======
> SCSI_IOCTL_SEND_COMMAND     redirects to scsi mid level
> SCSI_IOCTL_GET_IDLUN        yields 0 (wrong, trips up cdrecord??)
> SCSI_IOCTL_GET_BUS_NUMBER   yields 0 (wrong)
> SG_EMULATED_HOST            irrelevant to st (obsolete in sg)
> SG_GET_RESERVED_SIZE        irrelevant to st (to trick cdrecord)
> SG_GET_TIMEOUT              irrelevant to st (to trick cdrecord)
> SG_GET_VERSION_NUM          irrelevant to st (to trick cdrecord)
> SG_IO                       <<< only ioctl useful to st (+ osst) >>
> SG_SET_RESERVED_SIZE        irrelevant to st (to trick cdrecord)
> SG_SET_TIMEOUT              irrelevant to st (to trick cdrecord)
> CDROM_SEND_PACKET           irrelevant
> CDROMCLOSETRAY              irrelevant
> CDROMEJECT                  irrelevant
> 
> So if st just redirects only the SG_IO ioctl to scsi_cmd_ioctl()
> and lets the rest go to the scsi mid level (from where they can
> dribble down to the LLD) that solves the problem.
> 
If I read drivers/block/scsi_ioctl.c correctly, it contains a 
re-implementation of SCSI_IOCTL_SEND_COMMAND and it is then not redirected 
to mid level (again, I don't know why this has been done ;-). It is 
probably best that all SCSI drivers use the same implementation (and the 
other one is removed ...).

Your analysis is correct, and currently the only really useful thing 
implemented by scsi_cmd_ioctl() is SG_IO. However, I am an optimist and 
see a possibility that some other useful operations may be implemented in 
scsi_cmd_ioctl(). They may not be re-implementations of sg operations but 
something else useful for the users of the block layer. If only the buggy  
SCSI_IOCTL_GET_IDLUN and SCSI_IOCTL_GET_BUS_NUMBER are filtered in the 
high-level drivers, the new functions will be directly available.

-- 
Kai