2006-03-06 11:03:26

by Bodo Eggert

[permalink] [raw]
Subject: RFC: Move SG_GET_SCSI_ID from sg to scsi

I suggest moving the SG_GET_SCSI_ID ioctl from sg to scsi, since it's
generally usefull and the alternative function SCSI_IOCTL_GET_IDLUN
is limited in range (in-kernel data types may be larger) and
functionality (type, ...).

However, I have some questions about that ioctl:

- There is the concept of 8-Byte-LUNs: Are they mapped to integer LUNs?
Should the extra space in the struct sg_scsi_id be used for that?
Or should I abandon the idea and create a new IOCTL instead?

- The original IOCTL will check for sdp->detached. If the moved-to-scsi
ioctl is called, the check will be done before chaining from sg, but what
will I need to check if it's called on a non-sg device?

- Are there any (planned) changes that will conflict with this patch?


Proof-of-concept-hack (unclean):

diff -pru -x dontdiff 2.6.15.ori/drivers/scsi/scsi_ioctl.c 2.6.15/drivers/scsi/scsi_ioctl.c
--- 2.6.15.ori/drivers/scsi/scsi_ioctl.c 2006-02-08 15:10:48.000000000 +0100
+++ 2.6.15/drivers/scsi/scsi_ioctl.c 2006-03-04 21:24:13.000000000 +0100
@@ -434,6 +434,31 @@ int scsi_ioctl(struct scsi_device *sdev,
START_STOP_TIMEOUT, NORMAL_RETRIES);
case SCSI_IOCTL_GET_PCI:
return scsi_ioctl_get_pci(sdev, arg);
+ case SG_GET_SCSI_ID:
+ if (!access_ok(VERIFY_WRITE, arg, sizeof (sg_scsi_id_t)))
+ return -EFAULT;
+ else {
+ sg_scsi_id_t __user *sg_idp = arg;
+
+// if (sdp->detached)
+// return -ENODEV;
+// is the above covered by !sdev ath the
+// beginning of this function?
+ __put_user((int) sdev->host->host_no,
+ &sg_idp->host_no);
+ __put_user((int) sdev->channel,
+ &sg_idp->channel);
+ __put_user((int) sdev->id, &sg_idp->scsi_id);
+ __put_user((int) sdev->lun, &sg_idp->lun);
+ __put_user((int) sdev->type, &sg_idp->scsi_type);
+ __put_user((short) sdev->host->cmd_per_lun,
+ &sg_idp->h_cmd_per_lun);
+ __put_user((short) sdev->queue_depth,
+ &sg_idp->d_queue_depth);
+ __put_user(0, &sg_idp->unused[0]);
+ __put_user(0, &sg_idp->unused[1]);
+ return 0;
+ }
default:
if (sdev->host->hostt->ioctl)
return sdev->host->hostt->ioctl(sdev, cmd, arg);
diff -pru -x dontdiff 2.6.15.ori/drivers/scsi/sg.c 2.6.15/drivers/scsi/sg.c
--- 2.6.15.ori/drivers/scsi/sg.c 2006-02-08 15:10:48.000000000 +0100
+++ 2.6.15/drivers/scsi/sg.c 2006-03-04 22:15:15.000000000 +0100
@@ -871,29 +871,6 @@ sg_ioctl(struct inode *inode, struct fil
return 0;
case SG_GET_LOW_DMA:
return put_user((int) sfp->low_dma, ip);
- case SG_GET_SCSI_ID:
- if (!access_ok(VERIFY_WRITE, p, sizeof (sg_scsi_id_t)))
- return -EFAULT;
- else {
- sg_scsi_id_t __user *sg_idp = p;
-
- if (sdp->detached)
- return -ENODEV;
- __put_user((int) sdp->device->host->host_no,
- &sg_idp->host_no);
- __put_user((int) sdp->device->channel,
- &sg_idp->channel);
- __put_user((int) sdp->device->id, &sg_idp->scsi_id);
- __put_user((int) sdp->device->lun, &sg_idp->lun);
- __put_user((int) sdp->device->type, &sg_idp->scsi_type);
- __put_user((short) sdp->device->host->cmd_per_lun,
- &sg_idp->h_cmd_per_lun);
- __put_user((short) sdp->device->queue_depth,
- &sg_idp->d_queue_depth);
- __put_user(0, &sg_idp->unused[0]);
- __put_user(0, &sg_idp->unused[1]);
- return 0;
- }
case SG_SET_FORCE_PACK_ID:
result = get_user(val, ip);
if (result)
@@ -1072,6 +1049,7 @@ sg_ioctl(struct inode *inode, struct fil
case SCSI_IOCTL_GET_BUS_NUMBER:
case SCSI_IOCTL_PROBE_HOST:
case SG_GET_TRANSFORM:
+ case SG_GET_SCSI_ID:
if (sdp->detached)
return -ENODEV;
return scsi_ioctl(sdp->device, cmd_in, p);
diff -pru -x dontdiff 2.6.15.ori/include/scsi/scsi.h 2.6.15/include/scsi/scsi.h
--- 2.6.15.ori/include/scsi/scsi.h 2006-02-08 15:11:08.000000000 +0100
+++ 2.6.15/include/scsi/scsi.h 2006-03-04 11:11:31.000000000 +0100
@@ -245,6 +245,21 @@ struct scsi_lun {
__u8 scsi_lun[8];
};

+// copyed to scsi.h, so don't typedef it twice:
+#ifndef sg_scsi_id_t
+typedef struct sg_scsi_id { /* used by SG_GET_SCSI_ID ioctl() */
+ int host_no; /* as in "scsi<n>" where 'n' is one of 0, 1, 2 etc */
+ int channel;
+ int scsi_id; /* scsi id of target device */
+ int lun;
+ int scsi_type; /* TYPE_... defined in scsi/scsi.h */
+ short h_cmd_per_lun;/* host (adapter) maximum commands per lun */
+ short d_queue_depth;/* device (or adapter) maximum queue length */
+ int unused[2]; /* probably find a good use, set 0 for now */
+} sg_scsi_id_t; /* 32 bytes long on i386 */
+#define sg_scsi_id_t sg_scsi_id_t
+#endif
+
/*
* MESSAGE CODES
*/
diff -pru -x dontdiff 2.6.15.ori/include/scsi/sg.h 2.6.15/include/scsi/sg.h
--- 2.6.15.ori/include/scsi/sg.h 2005-06-17 21:48:29.000000000 +0200
+++ 2.6.15/include/scsi/sg.h 2006-03-04 11:11:30.000000000 +0100
@@ -156,6 +156,8 @@ typedef struct sg_io_hdr
#define SG_INFO_MIXED_IO 0x4 /* part direct, part indirect IO */


+// copyed to scsi.h, so don't typedef it twice:
+#ifndef sg_scsi_id_t
typedef struct sg_scsi_id { /* used by SG_GET_SCSI_ID ioctl() */
int host_no; /* as in "scsi<n>" where 'n' is one of 0, 1, 2 etc */
int channel;
@@ -166,6 +168,8 @@ typedef struct sg_scsi_id { /* used by S
short d_queue_depth;/* device (or adapter) maximum queue length */
int unused[2]; /* probably find a good use, set 0 for now */
} sg_scsi_id_t; /* 32 bytes long on i386 */
+#define sg_scsi_id_t sg_scsi_id_t
+#endif

typedef struct sg_req_info { /* used by SG_GET_REQUEST_TABLE ioctl() */
char req_state; /* 0 -> not used, 1 -> written, 2 -> ready to read */
@@ -196,8 +200,11 @@ typedef struct sg_req_info { /* used by
#define SG_GET_RESERVED_SIZE 0x2272 /* actual size of reserved buffer */

/* The following ioctl has a 'sg_scsi_id_t *' object as its 3rd argument. */
+#ifndef SG_GET_SCSI_ID
#define SG_GET_SCSI_ID 0x2276 /* Yields fd's bus, chan, dev, lun + type */
-/* SCSI id information can also be obtained from SCSI_IOCTL_GET_IDLUN */
+#endif
+/* SCSI id information can also be obtained from SCSI_IOCTL_GET_IDLUN, *
+ * but that is limited in range (/usurally/ no problem ...) */

/* Override host setting and always DMA using low memory ( <16MB on i386) */
#define SG_SET_FORCE_LOW_DMA 0x2279 /* 0-> use adapter setting, 1-> force */


2006-03-06 19:33:51

by Douglas Gilbert

[permalink] [raw]
Subject: Re: RFC: Move SG_GET_SCSI_ID from sg to scsi

Bodo Eggert wrote:
> I suggest moving the SG_GET_SCSI_ID ioctl from sg to scsi, since it's
> generally usefull and the alternative function SCSI_IOCTL_GET_IDLUN
> is limited in range (in-kernel data types may be larger) and
> functionality (type, ...).

Bodo,
ioctls, especially new ones, have become very unpopular
in the linux kernel. Whoever implemented the SG_IO ioctl
in the block layer moved just enough other sg ioctls to
fool cdrecord that it was talking to the sg driver. The
SG_GET_SCSI_ID ioctl didn't make the cut, probably because
cdrecord didn't use it.

Mind you, I think it is correct to (try and) move
SG_GET_SCSI_ID to the SCSI subsystem rather than
the block layer. Otherwise we would not be able
to use your proposed ioctl on non-block, sg visible
only devices (e.g. a SCSI enclosure (SES protocol)).

> However, I have some questions about that ioctl:
>
> - There is the concept of 8-Byte-LUNs: Are they mapped to integer LUNs?
> Should the extra space in the struct sg_scsi_id be used for that?
> Or should I abandon the idea and create a new IOCTL instead?

Yes, the SG_GET_SCSI_ID ioctl should allow for 8 byte
LUNs and that is a flaw in the current design. It is
no excuse that the rest of the SCSI subsystem has a
similar shortcoming.

In linux there is also a move away from the host_number,
channel_number, target_identifier and LUN tuple used
traditionally by many Unix SCSI subsystems (most do not
have the second component: channel_number). At least the
LUN is not controversial (as long as it is 8 byte!). The
target_identifier is actually transport dependent (but
could just be a simple enumeration). The host_number is
typically an enumeration over PCI addresses but some
other type of computer buses (e.g. microchannel) could be
involved.

> - The original IOCTL will check for sdp->detached. If the moved-to-scsi
> ioctl is called, the check will be done before chaining from sg, but what
> will I need to check if it's called on a non-sg device?

That should not be needed as the open file descriptor
to the SCSI device should be sufficient to keep the
relevant objects alive even if the device was just hot
unplugged.

> - Are there any (planned) changes that will conflict with this patch?

There is this thing called sysfs which is advertised
as an ioctl replacement. However if a routine is given
an open device node and you want to find its identity
an ioctl does have some advantages over a procfs followed
by a sysfs trawl. Just like ioctl related structures,
sysfs also changes, frustrating applications built on
it. Sysfs might even change more often than a well designed
ioctl since sysfs is often tightly bound to the driver
software architecture which may change without impacting
interfaces.

Lets see if this post gets a response.

Doug Gilbert


2006-03-07 15:00:58

by Stefan Richter

[permalink] [raw]
Subject: Re: RFC: Move SG_GET_SCSI_ID from sg to scsi

Douglas Gilbert wrote:
> In linux there is also a move away from the host_number,
> channel_number, target_identifier and LUN tuple used
> traditionally by many Unix SCSI subsystems (most do not
> have the second component: channel_number). At least the
> LUN is not controversial (as long as it is 8 byte!). The
> target_identifier is actually transport dependent (but
> could just be a simple enumeration). The host_number is
> typically an enumeration over PCI addresses but some
> other type of computer buses (e.g. microchannel) could be
> involved.

For some transports, not only the channel but also the Scsi_Host is
meaningless. Such transports deal only with targets and logical units.
This includes all multi-protocol + multi-bus or network infrastructures
such as iSCSI, USB, IEEE 1394.
--
Stefan Richter
-=====-=-==- --== --===
http://arcgraph.de/sr/

2006-03-07 18:48:50

by Bill Davidsen

[permalink] [raw]
Subject: Re: RFC: Move SG_GET_SCSI_ID from sg to scsi

Douglas Gilbert wrote:
> Bodo Eggert wrote:
>> I suggest moving the SG_GET_SCSI_ID ioctl from sg to scsi, since it's
>> generally usefull and the alternative function SCSI_IOCTL_GET_IDLUN
>> is limited in range (in-kernel data types may be larger) and
>> functionality (type, ...).
>
> Bodo,
> ioctls, especially new ones, have become very unpopular
> in the linux kernel. Whoever implemented the SG_IO ioctl
> in the block layer moved just enough other sg ioctls to
> fool cdrecord that it was talking to the sg driver. The
> SG_GET_SCSI_ID ioctl didn't make the cut, probably because
> cdrecord didn't use it.
>
> Mind you, I think it is correct to (try and) move
> SG_GET_SCSI_ID to the SCSI subsystem rather than
> the block layer. Otherwise we would not be able
> to use your proposed ioctl on non-block, sg visible
> only devices (e.g. a SCSI enclosure (SES protocol)).
>
>> However, I have some questions about that ioctl:
>>
>> - There is the concept of 8-Byte-LUNs: Are they mapped to integer LUNs?
>> Should the extra space in the struct sg_scsi_id be used for that?
>> Or should I abandon the idea and create a new IOCTL instead?
>
> Yes, the SG_GET_SCSI_ID ioctl should allow for 8 byte
> LUNs and that is a flaw in the current design. It is
> no excuse that the rest of the SCSI subsystem has a
> similar shortcoming.
>
> In linux there is also a move away from the host_number,
> channel_number, target_identifier and LUN tuple used
> traditionally by many Unix SCSI subsystems (most do not
> have the second component: channel_number). At least the
> LUN is not controversial (as long as it is 8 byte!). The
> target_identifier is actually transport dependent (but
> could just be a simple enumeration). The host_number is
> typically an enumeration over PCI addresses but some
> other type of computer buses (e.g. microchannel) could be
> involved.

In real SCSI systems that usually maps to card, bus/cable, ID and LUN
and allows some correspondence between physical and logical space. Handy
to trouble shoot, if you see a lot of errors on the same cable yo have a
clue that it may be common cause.
>
>> - The original IOCTL will check for sdp->detached. If the moved-to-scsi
>> ioctl is called, the check will be done before chaining from sg, but what
>> will I need to check if it's called on a non-sg device?
>
> That should not be needed as the open file descriptor
> to the SCSI device should be sufficient to keep the
> relevant objects alive even if the device was just hot
> unplugged.
>
>> - Are there any (planned) changes that will conflict with this patch?
>
> There is this thing called sysfs which is advertised
> as an ioctl replacement. However if a routine is given
> an open device node and you want to find its identity
> an ioctl does have some advantages over a procfs followed
> by a sysfs trawl. Just like ioctl related structures,
> sysfs also changes, frustrating applications built on
> it. Sysfs might even change more often than a well designed
> ioctl since sysfs is often tightly bound to the driver
> software architecture which may change without impacting
> interfaces.
>
> Lets see if this post gets a response.

Good to see you still around and posting about the relevant technical
details.

--
-bill davidsen ([email protected])
"The secret to procrastination is to put things off until the
last possible moment - but no longer" -me

2006-03-27 22:06:15

by Douglas Gilbert

[permalink] [raw]
Subject: Re: RFC: Move SG_GET_SCSI_ID from sg to scsi

Stefan Richter wrote:
> Douglas Gilbert wrote:
>
>> In linux there is also a move away from the host_number,
>> channel_number, target_identifier and LUN tuple used
>> traditionally by many Unix SCSI subsystems (most do not
>> have the second component: channel_number). At least the
>> LUN is not controversial (as long as it is 8 byte!). The
>> target_identifier is actually transport dependent (but
>> could just be a simple enumeration). The host_number is
>> typically an enumeration over PCI addresses but some
>> other type of computer buses (e.g. microchannel) could be
>> involved.
>
>
> For some transports, not only the channel but also the Scsi_Host is
> meaningless. Such transports deal only with targets and logical units.
> This includes all multi-protocol + multi-bus or network infrastructures
> such as iSCSI, USB, IEEE 1394.

Stefan,
I have been reviewing this thread and had one point
to add here.

The identity of the initiator port is important, at
least to a SCSI target that can implement (PERSISTENT)
RESERVE on behalf of one of its logical units.
So you may need to keep the equivalent of Scsi_Host:this_id
somewhere.

That is another shortcoming of the <hctl> tuple: the
initiator port isn't there.

Doug Gilbert

2006-03-27 22:22:25

by James Bottomley

[permalink] [raw]
Subject: Re: RFC: Move SG_GET_SCSI_ID from sg to scsi

On Mon, 2006-03-27 at 17:05 -0500, Douglas Gilbert wrote:
> I have been reviewing this thread and had one point
> to add here.
>
> The identity of the initiator port is important, at
> least to a SCSI target that can implement (PERSISTENT)
> RESERVE on behalf of one of its logical units.
> So you may need to keep the equivalent of Scsi_Host:this_id
> somewhere.

Since Scsi_Host:this_id is an integer, it's impossible that a
TransportID (which is what you use to identify the port) would ever fit
into it, since these are defined to be at least 24 bytes long.

> That is another shortcoming of the <hctl> tuple: the
> initiator port isn't there.

Actually, it may not be in HCIL, but it is in the transport classes
(although the only implementor is FC at this time, I think). And that's
where it should be since format and contents of the transport ID are
transport specific.

James