Re: [patch] add scsi changer driver
On Mon, 2005-03-07 at 09:21 +0100, Gerd Knorr wrote: > Probably historical reasons, I havn't tracked the scsi layer changes for > quite some time, so this might simply be a 2.6 cleanup I've missed > because of that. Will check ... OK, Thanks. > > ch_ioctl() (and the compat): since this is a new driver, can't this all > > be done via sysfs? That way, the user would be able to manipulate it > > from the command line, and we'd no longer need any of the 32->64 compat > > glue. > > Well, it isn't new, it already exists for many years, just not living in > mainline (which I finally want to change now ...). OK, so could you look at doing the sysfs conversions? These are required before the driver goes in. I can help you when I get back from holiday (in about a week's time). James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] add scsi changer driver
On Sun, Mar 06, 2005 at 07:55:25PM +0200, James Bottomley wrote: > Looking through this, the only things I really noticed that need work > are: > > ch_do_scsi(): It looks like this has an effective reimplementation of > scsi_wait_req. We're trying to deprecate the usage of scsi_do_req so we > can make it private eventually. What's the reason you can't use > scsi_wait_req? Probably historical reasons, I havn't tracked the scsi layer changes for quite some time, so this might simply be a 2.6 cleanup I've missed because of that. Will check ... > ch_ioctl() (and the compat): since this is a new driver, can't this all > be done via sysfs? That way, the user would be able to manipulate it > from the command line, and we'd no longer need any of the 32->64 compat > glue. Well, it isn't new, it already exists for many years, just not living in mainline (which I finally want to change now ...). Gerd -- #define printk(args...) fprintf(stderr, ## args) - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] add scsi changer driver
Looking through this, the only things I really noticed that need work are: ch_do_scsi(): It looks like this has an effective reimplementation of scsi_wait_req. We're trying to deprecate the usage of scsi_do_req so we can make it private eventually. What's the reason you can't use scsi_wait_req? ch_ioctl() (and the compat): since this is a new driver, can't this all be done via sysfs? That way, the user would be able to manipulate it from the command line, and we'd no longer need any of the 32->64 compat glue. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] add scsi changer driver
> > +#include
> not needed.
> > +#include
> I doubt you'll need this one.
Dropped.
> > +#define MAJOR_NR SCSI_CHANGER_MAJOR
> please kill this one
Done.
> > +#include "scsi.h"
> never use this header but always the
Done.
> > +MODULE_SUPPORTED_DEVICE("sch");
> no needed thsese days
Dropped.
> > +static int ioctl32_register(void)
> > +{
>
> please implement ->compat_ioctl instead.
Done.
> > + sr = scsi_allocate_request(ch->device, GFP_ATOMIC);
>
> wouldn't a GFP_KERNEL do just fine?
Yes (there was another one of this ...).
Fixed.
> > + if (NULL == sr)
> > + return -ENOMEM;
>
> normal kernel style would be
>
> if (!s)
> return -ENOMEM;
Well, I prefeare the first as it's more readable IMHO.
> > + list_for_each(item,&ch_devlist) {
> > + tmp = list_entry(item, scsi_changer, list);
>
> list_for_each_entry
Oh, a new fancy toy I havn't notices yet ;)
When this was added btw.?
New version of the patch is below.
Gerd
==[ cut here ]==
Subject: [patch] scsi changer
This patch adds a device driver for scsi media changer devices.
Signed-off-by: Gerd Knorr <[EMAIL PROTECTED]>
---
Documentation/scsi-changer.txt | 180 +
drivers/scsi/Kconfig | 18
drivers/scsi/Makefile |1
drivers/scsi/ch.c | 1020 +
include/linux/chio.h | 168 +
include/linux/major.h |1
include/scsi/scsi.h|3
7 files changed, 1391 insertions(+)
Index: linux-2.6.11-rc4/Documentation/scsi-changer.txt
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux-2.6.11-rc4/Documentation/scsi-changer.txt 2005-02-16
15:04:37.0 +0100
@@ -0,0 +1,180 @@
+
+README for the SCSI media changer driver
+
+
+This is a driver for SCSI Medium Changer devices, which are listed
+with "Type: Medium Changer" in /proc/scsi/scsi.
+
+This is for *real* Jukeboxes. It is *not* supported to work with
+common small CD-ROM changers, neither one-lun-per-slot SCSI changers
+nor IDE drives.
+
+Userland tools available from here:
+ http://linux.bytesex.org/misc/changer.html
+
+
+General Information
+---
+
+First some words about how changers work: A changer has 2 (possibly
+more) SCSI ID's. One for the changer device which controls the robot,
+and one for the device which actually reads and writes the data. The
+later may be anything, a MOD, a CD-ROM, a tape or whatever. For the
+changer device this is a "don't care", he *only* shuffles around the
+media, nothing else.
+
+
+The SCSI changer model is complex, compared to - for example - IDE-CD
+changers. But it allows to handle nearly all possible cases. It knows
+4 different types of changer elements:
+
+ media transport - this one shuffles around the media, i.e. the
+transport arm. Also known as "picker".
+ storage - a slot which can hold a media.
+ import/export - the same as above, but is accessable from outside,
+i.e. there the operator (you !) can use this to
+fill in and remove media from the changer.
+ Sometimes named "mailslot".
+ data transfer - this is the device which reads/writes, i.e. the
+ CD-ROM / Tape / whatever drive.
+
+None of these is limited to one: A huge Jukebox could have slots for
+123 CD-ROM's, 5 CD-ROM readers (and therefore 6 SCSI ID's: the changer
+and each CD-ROM) and 2 transport arms. No problem to handle.
+
+
+How it is implemented
+-
+
+I implemented the driver as character device driver with a NetBSD-like
+ioctl interface. Just grabbed NetBSD's header file and one of the
+other linux SCSI device drivers as starting point. The interface
+should be source code compatible with NetBSD. So if there is any
+software (anybody knows ???) which supports a BSDish changer driver,
+it should work with this driver too.
+
+Over time a few more ioctls where added, volume tag support for example
+wasn't covered by the NetBSD ioctl API.
+
+
+Current State
+-
+
+Support for more than one transport arm is not implemented yet (and
+nobody asked for it so far...).
+
+I test and use the driver myself with a 35 slot cdrom jukebox from
+Grundig. I got some reports telling it works ok with tape autoloaders
+(Exabyte, HP and DEC). Some People use this driver with amanda. It
+works fine with small (11 slots) and a huge (4 MOs, 88 slots)
+magneto-optical Jukebox. Probably with lots of other changers too, most
+(but not all :-) people mail me only if it does *not* work...
+
+I don't have any device lists, neither black-list nor white-list. Thus
+it is quite useless to ask me whenever a specific device is supported or
+not. In theory every changer device which supports the SCSI-2 media
+cha
Re: [patch] add scsi changer driver
[this should go to linux-scsi]
> +#include
not needed.
> +#include
I doubt you'll need this one.
> +#include
> +
> +#include /* here are all the ioctls */
should always go before
> +#define MAJOR_NR SCSI_CHANGER_MAJOR
please kill this one
> +#include "scsi.h"
never use this header but always the
> +MODULE_SUPPORTED_DEVICE("sch");
no needed thsese days
> +static int dt_id[CH_DT_MAX] = { [ 0 ... (CH_DT_MAX-1) ] = -1 };
> +static int dt_lun[CH_DT_MAX];
> +module_param_array(dt_id, int, NULL, 0444);
> +module_param_array(dt_lun, int, NULL, 0444);
> +
> +/* tell the driver about vendor-specific slots */
> +static int vendor_firsts[CH_TYPES-4];
> +static int vendor_counts[CH_TYPES-4];
> +module_param_array(vendor_firsts, int, NULL, 0444);
> +module_param_array(vendor_counts, int, NULL, 0444);
> +
> +static char *vendor_labels[CH_TYPES-4] = {
> + "v0", "v1", "v2", "v3"
> +};
> +// module_param_string_array(vendor_labels, NULL, 0444);
> +
> +#define dprintk(fmt, arg...)if (debug) \
> +printk(KERN_DEBUG "%s: " fmt, ch->name, ##arg)
> +#define vprintk(fmt, arg...)if (verbose) \
> +printk(KERN_INFO "%s: " fmt, ch->name, ##arg)
> +
> +/* --- */
> +static int ioctl32_register(void)
> +{
> + unsigned int i;
> + int err;
> +
> + for (i = 0; i < ARRAY_SIZE(ioctl32_cmds); i++) {
> + err = register_ioctl32_conversion(ioctl32_cmds[i].cmd,NULL);
> + if (err >= 0)
> + ioctl32_cmds[i].reg++;
> + }
> + return 0;
> +}
please implement ->compat_ioctl instead.
> + int errno, retries = 0, timeout;
> + DECLARE_COMPLETION(wait);
> + Scsi_Request *sr;
> +
> + sr = scsi_allocate_request(ch->device, GFP_ATOMIC);
wouldn't a GFP_KERNEL do just fine?
> + if (NULL == sr)
> + return -ENOMEM;
normal kernel style would be
if (!s)
return -ENOMEM;
> + list_for_each(item,&ch_devlist) {
> + tmp = list_entry(item, scsi_changer, list);
list_for_each_entry
> + list_for_each(item,&ch_devlist) {
> + tmp = list_entry(item, scsi_changer, list);
dito
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
