st.c for GET_IDLUN 2.6.6-rc2

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

@@ -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);

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

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).

This solution looks cleaner.

Doug Gilbert

....

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

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

On Wed, 28 Apr 2004, Douglas Gilbert wrote:

...

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