2002-01-01 22:16:46

by Linus Torvalds

[permalink] [raw]
Subject: NFS "dev_t" issues..


I made a pre6, which contains a new-and-anal "kdev_t".

The format of the thing is the same as it used to be, ie 16 bits of
information, but I made it a structure so that you _couldn't_ mix up
"dev_t" and "kdev_t", or use the "kdev_t" as a number (so when kdev_t
expands to 12+20 bits later in 2.5.x you shouldn't get surprises)

I fixed up the stuff I use and which showed up in compiles (on a source
level, it's so far totally untested), but I'd really like people to check
out their own subsystems. _Especially_ NFS and NFSD, which had several
cases of mixing the two dev_t's around, and which also used them as
numbers. Trond, Neil?

Because the types aren't at all compatible any more, the macros that are
used for user-level "dev_t" are no longer working for a kdev_t. So we have

dev_t kdev_t

MKDEV(major,minor) mk_kdev(major, minor)
MAJOR(dev) major(dev)
MINOR(dev) minor(dev)
dev == dev2 kdev_same(dev, dev2)
!dev kdev_none(dev)

and _most_ of the time the fixes are trivial - just translate as above. It
only gets interesting when you have code that looks at the value or starts
mixing the two and compares a "dev_t" against a "kdev_t", which can be
quite interesting.

The knfsd file handle thing is also an issue - Neil, please check out that
what I did looks sane, and would be on-the-wire-compatible with the old
behaviour, even when we expand kdev_t to 12+20 bits, ok?

(Marcelo, for easier backporting of drivers to 2.4.x, we'll probably want
to eventually add

#define mk_kdev(a,b) MKDEV(a,b)
#define major(d) MAJOR(d)
...

to the 2.4.x <linux/kdev_t.h> so that you can move drivers back and
forth).

Apart from some knfsd issues, most of the kdev_t users were proper. The
strict type-checking found one bug in the SCSI layer (which I knew about,
and was one of the impetuses for doing it in the first place), and found a
lot of small "works-but-will-break-with-a-bigger-kdev_t" issues).

Linus


2002-01-01 22:38:38

by Jeff Garzik

[permalink] [raw]
Subject: [PATCH] Re: NFS "dev_t" issues..

Index: drivers/block/rd.c
===================================================================
RCS file: /cvsroot/gkernel/linux_2_5/drivers/block/rd.c,v
retrieving revision 1.7
diff -u -r1.7 rd.c
--- drivers/block/rd.c 2001/12/20 18:55:32 1.7
+++ drivers/block/rd.c 2002/01/01 22:35:11
@@ -246,7 +246,7 @@
unsigned long offset, len;
int rw = sbh->bi_rw;

- minor = MINOR(sbh->bi_dev);
+ minor = minor(sbh->bi_dev);

if (minor >= NUM_RAMDISKS)
goto fail;
@@ -280,10 +280,10 @@
int error = -EINVAL;
unsigned int minor;

- if (!inode || !inode->i_rdev)
+ if (!inode || kdev_none(inode->i_rdev))
goto out;

- minor = MINOR(inode->i_rdev);
+ minor = minor(inode->i_rdev);

switch (cmd) {
case BLKFLSBUF:
@@ -407,7 +407,7 @@
rd_bdev[i] = NULL;
if (bdev)
blkdev_put(bdev, BDEV_FILE);
- destroy_buffers(MKDEV(MAJOR_NR, i));
+ destroy_buffers(mk_kdev(MAJOR_NR, i));
}

devfs_unregister (devfs_handle);
@@ -449,7 +449,7 @@
&rd_bd_op, NULL);

for (i = 0; i < NUM_RAMDISKS; i++)
- register_disk(NULL, MKDEV(MAJOR_NR,i), 1, &rd_bd_op, rd_size<<1);
+ register_disk(NULL, mk_kdev(MAJOR_NR,i), 1, &rd_bd_op, rd_size<<1);

#ifdef CONFIG_BLK_DEV_INITRD
/* We ought to separate initrd operations here */
Index: drivers/block/loop.c
===================================================================
RCS file: /cvsroot/gkernel/linux_2_5/drivers/block/loop.c,v
retrieving revision 1.7
diff -u -r1.7 loop.c
--- drivers/block/loop.c 2001/12/30 22:22:58 1.7
+++ drivers/block/loop.c 2002/01/01 22:35:21
@@ -155,8 +155,8 @@
{
if (S_ISREG(lo_dentry->d_inode->i_mode))
return (lo_dentry->d_inode->i_size - lo->lo_offset) >> BLOCK_SIZE_BITS;
- if (blk_size[MAJOR(lodev)])
- return blk_size[MAJOR(lodev)][MINOR(lodev)] -
+ if (blk_size[major(lodev)])
+ return blk_size[major(lodev)][minor(lodev)] -
(lo->lo_offset >> BLOCK_SIZE_BITS);
return MAX_DISK_SIZE;
}
@@ -379,7 +379,7 @@
*/
static int loop_end_io_transfer(struct bio *bio, int nr_sectors)
{
- struct loop_device *lo = &loop_dev[MINOR(bio->bi_dev)];
+ struct loop_device *lo = &loop_dev[minor(bio->bi_dev)];
int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);

if (!uptodate || bio_rw(bio) == WRITE) {
@@ -429,10 +429,10 @@
unsigned long IV;
int rw = bio_rw(rbh);

- if (MINOR(rbh->bi_dev) >= max_loop)
+ if (minor(rbh->bi_dev) >= max_loop)
goto out;

- lo = &loop_dev[MINOR(rbh->bi_dev)];
+ lo = &loop_dev[minor(rbh->bi_dev)];
spin_lock_irq(&lo->lo_lock);
if (lo->lo_state != Lo_bound)
goto inactive;
@@ -615,7 +615,7 @@

if (S_ISBLK(inode->i_mode)) {
lo_device = inode->i_rdev;
- if (lo_device == dev) {
+ if (kdev_same(lo_device, dev)) {
error = -EBUSY;
goto out;
}
@@ -725,7 +725,7 @@
loop_release_xfer(lo);
lo->transfer = NULL;
lo->ioctl = NULL;
- lo->lo_device = 0;
+ lo->lo_device = NODEV;
lo->lo_encrypt_type = 0;
lo->lo_offset = 0;
lo->lo_encrypt_key_size = 0;
@@ -818,12 +818,12 @@

if (!inode)
return -EINVAL;
- if (MAJOR(inode->i_rdev) != MAJOR_NR) {
+ if (major(inode->i_rdev) != MAJOR_NR) {
printk(KERN_WARNING "lo_ioctl: pseudo-major != %d\n",
MAJOR_NR);
return -ENODEV;
}
- dev = MINOR(inode->i_rdev);
+ dev = minor(inode->i_rdev);
if (dev >= max_loop)
return -ENODEV;
lo = &loop_dev[dev];
@@ -873,11 +873,11 @@

if (!inode)
return -EINVAL;
- if (MAJOR(inode->i_rdev) != MAJOR_NR) {
+ if (major(inode->i_rdev) != MAJOR_NR) {
printk(KERN_WARNING "lo_open: pseudo-major != %d\n", MAJOR_NR);
return -ENODEV;
}
- dev = MINOR(inode->i_rdev);
+ dev = minor(inode->i_rdev);
if (dev >= max_loop)
return -ENODEV;

@@ -900,12 +900,12 @@

if (!inode)
return 0;
- if (MAJOR(inode->i_rdev) != MAJOR_NR) {
+ if (major(inode->i_rdev) != MAJOR_NR) {
printk(KERN_WARNING "lo_release: pseudo-major != %d\n",
MAJOR_NR);
return 0;
}
- dev = MINOR(inode->i_rdev);
+ dev = minor(inode->i_rdev);
if (dev >= max_loop)
return 0;

@@ -1016,7 +1016,7 @@
blk_size[MAJOR_NR] = loop_sizes;
blksize_size[MAJOR_NR] = loop_blksizes;
for (i = 0; i < max_loop; i++)
- register_disk(NULL, MKDEV(MAJOR_NR, i), 1, &lo_fops, 0);
+ register_disk(NULL, mk_kdev(MAJOR_NR, i), 1, &lo_fops, 0);

printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
return 0;


Attachments:
block.patch (4.23 kB)

2002-01-01 22:42:48

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] Re: NFS "dev_t" issues..


On Tue, 1 Jan 2002, Jeff Garzik wrote:
>
> And, assignments "kdev_t foo = 0" become "kdev_t foo = NODEV".

Indeed.

> At least I hope so ;-) The below patch fixes up rd.c and loop.c.

Looks good, applied,

Linus

2002-01-01 22:57:49

by Alexander Viro

[permalink] [raw]
Subject: Re: NFS "dev_t" issues..



On Tue, 1 Jan 2002, Linus Torvalds wrote:

> Apart from some knfsd issues, most of the kdev_t users were proper. The
> strict type-checking found one bug in the SCSI layer (which I knew about,
> and was one of the impetuses for doing it in the first place), and found a
> lot of small "works-but-will-break-with-a-bigger-kdev_t" issues).

Sigh... Most of the ->i_dev instances are crap and ought to be replaced
with ->i_sb. At the very least, let's

--- C2-pre6/fs/namei.c Tue Jan 1 17:49:13 2002
+++ /tmp/namei.c Tue Jan 1 17:54:08 2002
@@ -1589,7 +1589,7 @@
goto exit_lock;

error = -EXDEV;
- if (!kdev_same(dir->i_dev, inode->i_dev))
+ if (dir->i_sb != inode->i_sb)
goto exit_lock;

/*
@@ -1707,7 +1707,7 @@
if (error)
return error;

- if (!kdev_same(new_dir->i_dev, old_dir->i_dev))
+ if (new_dir->i_sb != old_dir->i_sb)
return -EXDEV;

if (!new_dentry->d_inode)
@@ -1787,7 +1787,7 @@
if (error)
return error;

- if (!kdev_same(new_dir->i_dev, old_dir->i_dev))
+ if (new_dir->i_sb != old_dir->i_sb)
return -EXDEV;

if (!new_dentry->d_inode)

2002-01-01 23:04:49

by Jeff Garzik

[permalink] [raw]
Subject: [PATCH] Re: NFS "dev_t" issues..

Index: include/linux/kdev_t.h
===================================================================
RCS file: /cvsroot/gkernel/linux_2_5/include/linux/kdev_t.h,v
retrieving revision 1.2
diff -u -r1.2 kdev_t.h
--- include/linux/kdev_t.h 2002/01/01 22:41:11 1.2
+++ include/linux/kdev_t.h 2002/01/01 23:02:10
@@ -86,7 +86,7 @@
* internal equality comparisons and for things
* like NFS filehandle conversion.
*/
-static inline unsigned int kdev_val(kdev_t dev)
+static inline u32 kdev_val(kdev_t dev)
{
return dev.value;
}


Attachments:
kdev.patch (528.00 B)

2002-01-01 23:27:41

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] Re: NFS "dev_t" issues..


On Tue, 1 Jan 2002, Jeff Garzik wrote:
>
> What do you think about the attached simple patch, making the cookie
> size more explicit?

Well, I suspect that we actually should also make the format explicit, and
basically use the same translation that I did for the NFS filehandle. That
way it's still just a cookie, but it's a cookie with (a) explicit size and
(b) meaning that won't change over different kernel revisions.

Hmm?

Linus

2002-01-01 23:28:31

by Linus Torvalds

[permalink] [raw]
Subject: Re: NFS "dev_t" issues..


On Tue, 1 Jan 2002, Alexander Viro wrote:
>
> Sigh... Most of the ->i_dev instances are crap and ought to be replaced
> with ->i_sb. At the very least, let's

Looks good, and yes, it makes a lot more sense from an EXDEV standpoint to
check the superblock, as "i_dev" really has no good meaning for many
filesystems.

Linus

2002-01-01 23:50:21

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] Re: NFS "dev_t" issues..

Linus Torvalds wrote:
>
> On Tue, 1 Jan 2002, Jeff Garzik wrote:
> >
> > What do you think about the attached simple patch, making the cookie
> > size more explicit?
>
> Well, I suspect that we actually should also make the format explicit, and
> basically use the same translation that I did for the NFS filehandle. That
> way it's still just a cookie, but it's a cookie with (a) explicit size and
> (b) meaning that won't change over different kernel revisions.

true, each filesystem needs to figure out how to make sure their on-disk
format doesn't change across kernel revisions... Storing the raw i_rdev
onto disk definitely silly but it appears to be an issue some
filesystems will have to deal with. I'm leaving reiserfs alone so they
can make a policy decision...

--
Jeff Garzik | Only so many songs can be sung
Building 1024 | with two lips, two lungs, and one tongue.
MandrakeSoft | - nomeansno

2002-01-02 05:47:07

by Greg KH

[permalink] [raw]
Subject: Re: NFS "dev_t" issues..

On Tue, Jan 01, 2002 at 02:15:58PM -0800, Linus Torvalds wrote:
>
> I made a pre6, which contains a new-and-anal "kdev_t".

Here's a patch to fix the usb code drivers.

thanks,

greg k-h


diff -Nru a/drivers/usb/acm.c b/drivers/usb/acm.c
--- a/drivers/usb/acm.c Tue Jan 1 21:33:37 2002
+++ b/drivers/usb/acm.c Tue Jan 1 21:33:37 2002
@@ -297,7 +297,7 @@

static int acm_tty_open(struct tty_struct *tty, struct file *filp)
{
- struct acm *acm = acm_table[MINOR(tty->device)];
+ struct acm *acm = acm_table[minor(tty->device)];

if (!acm || !acm->dev) return -EINVAL;

diff -Nru a/drivers/usb/bluetooth.c b/drivers/usb/bluetooth.c
--- a/drivers/usb/bluetooth.c Tue Jan 1 21:33:37 2002
+++ b/drivers/usb/bluetooth.c Tue Jan 1 21:33:37 2002
@@ -360,7 +360,7 @@
tty->driver_data = NULL;

/* get the bluetooth object associated with this tty pointer */
- bluetooth = get_bluetooth_by_minor (MINOR(tty->device));
+ bluetooth = get_bluetooth_by_minor (minor(tty->device));

if (bluetooth_paranoia_check (bluetooth, __FUNCTION__)) {
return -ENODEV;
diff -Nru a/drivers/usb/dabusb.c b/drivers/usb/dabusb.c
--- a/drivers/usb/dabusb.c Tue Jan 1 21:33:37 2002
+++ b/drivers/usb/dabusb.c Tue Jan 1 21:33:37 2002
@@ -579,7 +579,7 @@

static int dabusb_open (struct inode *inode, struct file *file)
{
- int devnum = MINOR (inode->i_rdev);
+ int devnum = minor (inode->i_rdev);
pdabusb_t s;

if (devnum < DABUSB_MINOR || devnum >= (DABUSB_MINOR + NRDABUSB))
diff -Nru a/drivers/usb/dc2xx.c b/drivers/usb/dc2xx.c
--- a/drivers/usb/dc2xx.c Tue Jan 1 21:33:37 2002
+++ b/drivers/usb/dc2xx.c Tue Jan 1 21:33:37 2002
@@ -298,7 +298,7 @@
int value = 0;

down (&state_table_mutex);
- subminor = MINOR (inode->i_rdev) - USB_CAMERA_MINOR_BASE;
+ subminor = minor (inode->i_rdev) - USB_CAMERA_MINOR_BASE;
if (subminor < 0 || subminor >= MAX_CAMERAS
|| !(camera = minor_data [subminor])) {
up (&state_table_mutex);
diff -Nru a/drivers/usb/hiddev.c b/drivers/usb/hiddev.c
--- a/drivers/usb/hiddev.c Tue Jan 1 21:33:37 2002
+++ b/drivers/usb/hiddev.c Tue Jan 1 21:33:37 2002
@@ -218,7 +218,7 @@
static int hiddev_open(struct inode * inode, struct file * file) {
struct hiddev_list *list;

- int i = MINOR(inode->i_rdev) - HIDDEV_MINOR_BASE;
+ int i = minor(inode->i_rdev) - HIDDEV_MINOR_BASE;

if (i >= HIDDEV_MINORS || !hiddev_table[i])
return -ENODEV;
diff -Nru a/drivers/usb/printer.c b/drivers/usb/printer.c
--- a/drivers/usb/printer.c Tue Jan 1 21:33:37 2002
+++ b/drivers/usb/printer.c Tue Jan 1 21:33:37 2002
@@ -201,7 +201,7 @@

static int usblp_open(struct inode *inode, struct file *file)
{
- int minor = MINOR(inode->i_rdev) - USBLP_MINOR_BASE;
+ int minor = minor(inode->i_rdev) - USBLP_MINOR_BASE;
struct usblp *usblp;
int retval;

diff -Nru a/drivers/usb/scanner.c b/drivers/usb/scanner.c
--- a/drivers/usb/scanner.c Tue Jan 1 21:33:37 2002
+++ b/drivers/usb/scanner.c Tue Jan 1 21:33:37 2002
@@ -365,7 +365,7 @@
struct scn_usb_data *scn;
struct usb_device *dev;

- kdev_t scn_minor;
+ int scn_minor;

int err=0;

@@ -432,7 +432,7 @@
{
struct scn_usb_data *scn;

- kdev_t scn_minor;
+ int scn_minor;

scn_minor = USB_SCN_MINOR (inode);

@@ -469,7 +469,7 @@
ssize_t bytes_written = 0; /* Overall count of bytes written */
ssize_t ret = 0;

- kdev_t scn_minor;
+ int scn_minor;

int this_write; /* Number of bytes to write */
int partial; /* Number of bytes successfully written */
@@ -556,8 +556,7 @@
ssize_t bytes_read; /* Overall count of bytes_read */
ssize_t ret;

- kdev_t scn_minor;
-
+ int scn_minor;
int partial; /* Number of bytes successfully read */
int this_read; /* Max number of bytes to read */
int result;
@@ -671,7 +670,7 @@
{
struct usb_device *dev;

- kdev_t scn_minor;
+ int scn_minor;

scn_minor = USB_SCN_MINOR(inode);

@@ -810,8 +809,7 @@

int ep_cnt;
int ix;
-
- kdev_t scn_minor;
+ int scn_minor;

char valid_device = 0;
char have_bulk_in, have_bulk_out, have_intr;
diff -Nru a/drivers/usb/scanner.h b/drivers/usb/scanner.h
--- a/drivers/usb/scanner.h Tue Jan 1 21:33:37 2002
+++ b/drivers/usb/scanner.h Tue Jan 1 21:33:37 2002
@@ -203,7 +203,7 @@
#define IS_EP_BULK_OUT(ep) (IS_EP_BULK(ep) && ((ep).bEndpointAddress & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT)
#define IS_EP_INTR(ep) ((ep).bmAttributes == USB_ENDPOINT_XFER_INT ? 1 : 0)

-#define USB_SCN_MINOR(X) MINOR((X)->i_rdev) - SCN_BASE_MNR
+#define USB_SCN_MINOR(X) minor((X)->i_rdev) - SCN_BASE_MNR

#ifdef DEBUG
#define SCN_DEBUG(X) X
@@ -243,7 +243,7 @@
devfs_handle_t devfs; /* devfs device */
struct urb scn_irq;
unsigned int ifnum; /* Interface number of the USB device */
- kdev_t scn_minor; /* Scanner minor - used in disconnect() */
+ int scn_minor; /* Scanner minor - used in disconnect() */
unsigned char button; /* Front panel buffer */
char isopen; /* Not zero if the device is open */
char present; /* Not zero if device is present */
diff -Nru a/drivers/usb/serial/usbserial.c b/drivers/usb/serial/usbserial.c
--- a/drivers/usb/serial/usbserial.c Tue Jan 1 21:33:37 2002
+++ b/drivers/usb/serial/usbserial.c Tue Jan 1 21:33:37 2002
@@ -514,14 +514,14 @@
tty->driver_data = NULL;

/* get the serial object associated with this tty pointer */
- serial = get_serial_by_minor (MINOR(tty->device));
+ serial = get_serial_by_minor (minor(tty->device));

if (serial_paranoia_check (serial, __FUNCTION__)) {
return -ENODEV;
}

/* set up our port structure making the tty driver remember our port object, and us it */
- portNumber = MINOR(tty->device) - serial->minor;
+ portNumber = minor(tty->device) - serial->minor;
port = &serial->port[portNumber];
tty->driver_data = port;
port->tty = tty;

2002-01-02 16:16:32

by Martin Dalecki

[permalink] [raw]
Subject: [PATCH] Make 2.5.2-pre6 usable

The attached patch is making the mentioned kernel usable by propagating the
kdev_t handling to much more dirvers.

Of esp. interrest is the fact that there where some places, where NODEV
was used
instead of ENODEV:

linux-new/drivers/net/defxx.c
linux-new/fs/dquot.c
linux-new/drivers/media/video/cpia.c





Attachments:
usable-2.5.2-pre6.gz (11.94 kB)

2002-01-02 19:35:20

by Andries E. Brouwer

[permalink] [raw]
Subject: Re: [PATCH] Re: NFS "dev_t" issues..

I see lots of people sending patches for kdev_t.
In order to possibly avoid duplication of work,
I put my patch at ftp.kernel.org:
2.5.2pre6-kdev_t-diff (415841 bytes)

It has kminor and kmajor, but if that is not desired
a very simple edit on the patch will turn them into
minor and major.

(It is incomplete, but a good start.)

Andries

2002-01-03 01:28:34

by Andries E. Brouwer

[permalink] [raw]
Subject: Re: [PATCH] Re: NFS "dev_t" issues..

From [email protected] Thu Jan 3 00:22:23 2002

[email protected] wrote:
>
> I see lots of people sending patches for kdev_t.
> In order to possibly avoid duplication of work,
> I put my patch at ftp.kernel.org:
> 2.5.2pre6-kdev_t-diff (415841 bytes)
>
> It has kminor and kmajor, but if that is not desired
> a very simple edit on the patch will turn them into
> minor and major.
>
> (It is incomplete, but a good start.)

It doesn't build for me in make_rdonly() in ext3 with debug
configured in:

Yes. Still w.i.p. but a better version is now
2.5.2pre6-kdev_t-diff-v3 (443024 bytes).

Andries

2002-01-03 15:22:23

by Martin Dalecki

[permalink] [raw]
Subject: [PATCH] usabe2-2.5.2-pre6

The attached patch is making the SCSI "mid-layer" vomit bag usable again.
I have fixed some very ugly offenders as well in due course:

1. Moved the "informative" scsi_device_types[MAX_SCSI_DEVICE_CODE];
2. Unneccessary/inappriopriate locking in ppa.c
3. "Write only" field in Scsi_CD struct:
unsigned readcd_cdda:1; /* reading audio data using READ_CD */

It get's only set but is never used and never needed.

After having a look at this SCSI code I really got the opinnion that it
was written
buy someone who didn't know what functions and local variables are good for.
Everything there seems to be done in a maximally terse style...

Anyway.


Attachments:
usable2-2.5.2-pre6.gz (6.62 kB)

2002-01-03 15:22:22

by Alessandro Suardi

[permalink] [raw]
Subject: Re: [PATCH] Re: NFS "dev_t" issues..

[email protected] wrote:
>
> From [email protected] Thu Jan 3 00:22:23 2002
>
> It doesn't build for me in make_rdonly() in ext3 with debug
> configured in:
>
> Yes. Still w.i.p. but a better version is now
> 2.5.2pre6-kdev_t-diff-v3 (443024 bytes).

Compiles and boots fine with my config - laptop including ext2,
ext3, fat, vfat, reiserfs, iso9660, tmpfs, ramfs, IrDA, PPP,
Xircom Cardbus (old driver), parport, floppy, ramdisk, loop,
IDE, ipv4, nfs/nfsd, samba, maestro3, usb UHCI and more. Not
everything tested and one possibly related warning:

gcc -D__KERNEL__ -I/usr/src/linux-2.5.2-pre6/include -Wall -Wstrict-prototypes -Wno-trigraphs -O2 -fomit-frame-pointer -fno-strict-aliasing -fno-common -pipe -mpreferred-stack-boundary=2 -march=i686
-DMODULE -c -o write.o write.c
write.c: In function `nfs_commit_done':
write.c:1204: warning: unsigned int format, kdev_t arg (arg 2)

Quite good so far :) thanks a lot,

--alessandro

"this machine will, will not communicate
these thoughts and the strain I am under
be a world child, form a circle before we all go under"
(Radiohead, "Street Spirit [fade out]")

2002-01-07 16:51:21

by Trond Myklebust

[permalink] [raw]
Subject: NFS "dev_t" issues..

>>>>> " " == Linus Torvalds <[email protected]> writes:

> I made a pre6, which contains a new-and-anal "kdev_t".

<snip>

> I fixed up the stuff I use and which showed up in compiles (on
> a source level, it's so far totally untested), but I'd really
> like people to check out their own subsystems. _Especially_ NFS
> and NFSD, which had several cases of mixing the two dev_t's
> around, and which also used them as numbers. Trond, Neil?

Hi Linus & Marcelo,

Sorry I'm a bit late in replying. AFAICS as of 2.5.2-pre9, all is
more or less well, however when reviewing that code, I noticed what is
probably a bug:

Given that (for character devices) the value of inode->i_cdev in
2.[45].x depends on the i_rdev, it would appear to be a bug for us to
be able to change inode->i_rdev *after* we've called
init_special_inode().
For this reason, I'd advocate removing the lines in
nfs_refresh_inode() that reset the inode->i_rdev (as per the patch
below) and instead rely on the ordinary stale inode checks to tell us
if/when the inode->i_rdev has changed.

It's hardly a new bug. It's been around ever since we added
init_special_inode(), so it's clearly not one that bites us every day
of the year. Even so, the same patch should probably be applied to
2.4.x.

Cheers,
Trond


diff -u --recursive --new-file linux-2.5.2-pre9/fs/nfs/inode.c linux-2.5.2-fix/fs/nfs/inode.c
--- linux-2.5.2-pre9/fs/nfs/inode.c Mon Jan 7 16:57:18 2002
+++ linux-2.5.2-fix/fs/nfs/inode.c Mon Jan 7 17:08:42 2002
@@ -1107,9 +1107,6 @@
inode->i_blocks = fattr->du.nfs2.blocks;
inode->i_blksize = fattr->du.nfs2.blocksize;
}
- inode->i_rdev = NODEV;
- if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode))
- inode->i_rdev = to_kdev_t(fattr->rdev);

/* Update attrtimeo value */
if (!invalid && time_after(jiffies, NFS_ATTRTIMEO_UPDATE(inode)+NFS_ATTRTIMEO(inode))) {

2002-01-08 09:41:26

by Martin Dalecki

[permalink] [raw]
Subject: PATCH 2.5.2-pre9 scsi cleanup

diff -ur linux-old/drivers/scsi/dpt_i2o.c linux/drivers/scsi/dpt_i2o.c
--- linux-old/drivers/scsi/dpt_i2o.c Sat May 4 09:11:43 2002
+++ linux/drivers/scsi/dpt_i2o.c Sat May 4 06:54:58 2002
@@ -1551,7 +1551,7 @@

static int adpt_open(struct inode *inode, struct file *file)
{
- int minor;
+ unsigned int minor;
adpt_hba* pHba;

//TODO check for root access
@@ -1584,7 +1584,7 @@

static int adpt_close(struct inode *inode, struct file *file)
{
- int minor;
+ unsigned int minor;
adpt_hba* pHba;

minor = minor(inode->i_rdev);
@@ -1878,7 +1878,7 @@
static int adpt_ioctl(struct inode *inode, struct file *file, uint cmd,
ulong arg)
{
- int minor;
+ unsigned int minor;
int error = 0;
adpt_hba* pHba;
ulong flags;
diff -ur linux-old/drivers/scsi/fcal.c linux/drivers/scsi/fcal.c
--- linux-old/drivers/scsi/fcal.c Fri Feb 9 20:30:23 2001
+++ linux/drivers/scsi/fcal.c Sat May 4 07:47:15 2002
@@ -249,8 +249,6 @@
if (scd->host->host_no == hostno && scd->id == target) {
SPRINTF (" [AL-PA: %02x, Id: %02d, Port WWN: %08x%08x, Node WWN: %08x%08x] ",
alpa, target, u1[0], u1[1], u2[0], u2[1]);
- SPRINTF ("%s ", (scd->type < MAX_SCSI_DEVICE_CODE) ?
- scsi_device_types[(short) scd->type] : "Unknown device");

for (j = 0; (j < 8) && (scd->vendor[j] >= 0x20); j++)
SPRINTF ("%c", scd->vendor[j]);
diff -ur linux-old/drivers/scsi/g_NCR5380.c linux/drivers/scsi/g_NCR5380.c
--- linux-old/drivers/scsi/g_NCR5380.c Sun Sep 30 21:26:07 2001
+++ linux/drivers/scsi/g_NCR5380.c Sat May 4 07:48:21 2002
@@ -789,7 +789,6 @@
struct NCR5380_hostdata *hostdata;
#ifdef NCR5380_STATS
Scsi_Device *dev;
- extern const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE];
#endif
save_flags(flags);
cli();
@@ -833,7 +832,7 @@
long tr = hostdata->time_read[dev->id] / HZ;
long tw = hostdata->time_write[dev->id] / HZ;

- PRINTP(" T:%d %s " ANDP dev->id ANDP (dev->type < MAX_SCSI_DEVICE_CODE) ? scsi_device_types[(int)dev->type] : "Unknown");
+ PRINTP(" T:%d " ANDP dev->id );
for (i=0; i<8; i++)
if (dev->vendor[i] >= 0x20)
*(buffer+(len++)) = dev->vendor[i];
diff -ur linux-old/drivers/scsi/ppa.c linux/drivers/scsi/ppa.c
--- linux-old/drivers/scsi/ppa.c Sun Sep 30 21:26:07 2001
+++ linux/drivers/scsi/ppa.c Sat May 4 07:45:50 2002
@@ -115,11 +115,6 @@
int i, nhosts, try_again;
struct parport *pb;

- /*
- * unlock to allow the lowlevel parport driver to probe
- * the irqs
- */
- spin_unlock_irq(&io_request_lock);
pb = parport_enumerate();

printk("ppa: Version %s\n", PPA_VERSION);
@@ -128,7 +123,6 @@

if (!pb) {
printk("ppa: parport reports no devices.\n");
- spin_lock_irq(&io_request_lock);
return 0;
}
retry_entry:
@@ -154,7 +148,6 @@
"pardevice is owning the port for too longtime!\n",
i);
parport_unregister_device(ppa_hosts[i].dev);
- spin_lock_irq(&io_request_lock);
return 0;
}
}
@@ -223,15 +216,12 @@
printk(" supported by the imm (ZIP Plus) driver. If the\n");
printk(" cable is marked with \"AutoDetect\", this is what has\n");
printk(" happened.\n");
- spin_lock_irq(&io_request_lock);
return 0;
}
try_again = 1;
goto retry_entry;
- } else {
- spin_lock_irq(&io_request_lock);
+ } else
return 1; /* return number of hosts detected */
- }
}

/* This is to give the ppa driver a way to modify the timings (and other
@@ -847,9 +837,9 @@

tmp->cur_cmd = 0;

- spin_lock_irqsave(&io_request_lock, flags);
+ spin_lock_irqsave(&cmd->host->host_lock, flags);
cmd->scsi_done(cmd);
- spin_unlock_irqrestore(&io_request_lock, flags);
+ spin_unlock_irqrestore(&cmd->host->host_lock, flags);
return;
}

diff -ur linux-old/drivers/scsi/scsi.c linux/drivers/scsi/scsi.c
--- linux-old/drivers/scsi/scsi.c Sat May 4 09:11:44 2002
+++ linux/drivers/scsi/scsi.c Sat May 4 07:49:19 2002
@@ -139,25 +139,7 @@
*/
unsigned int scsi_logging_level;

-const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE] =
-{
- "Direct-Access ",
- "Sequential-Access",
- "Printer ",
- "Processor ",
- "WORM ",
- "CD-ROM ",
- "Scanner ",
- "Optical Device ",
- "Medium Changer ",
- "Communications ",
- "Unknown ",
- "Unknown ",
- "Unknown ",
- "Enclosure ",
-};
-
-/*
+/*
* Function prototypes.
*/
extern void scsi_times_out(Scsi_Cmnd * SCpnt);
diff -ur linux-old/drivers/scsi/scsi.h linux/drivers/scsi/scsi.h
--- linux-old/drivers/scsi/scsi.h Sat May 4 09:11:44 2002
+++ linux/drivers/scsi/scsi.h Sat May 4 07:49:38 2002
@@ -89,9 +89,6 @@
#define FALSE 0
#endif

-#define MAX_SCSI_DEVICE_CODE 14
-extern const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE];
-
#ifdef DEBUG
#define SCSI_TIMEOUT (5*HZ)
#else
diff -ur linux-old/drivers/scsi/scsi_proc.c linux/drivers/scsi/scsi_proc.c
--- linux-old/drivers/scsi/scsi_proc.c Thu Jun 28 02:10:55 2001
+++ linux/drivers/scsi/scsi_proc.c Sat May 4 07:51:31 2002
@@ -260,7 +260,6 @@
{

int x, y = *size;
- extern const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE];

y = sprintf(buffer + len,
"Host: scsi%d Channel: %02d Id: %02d Lun: %02d\n Vendor: ",
@@ -285,13 +284,8 @@
else
y += sprintf(buffer + len + y, " ");
}
- y += sprintf(buffer + len + y, "\n");
-
- y += sprintf(buffer + len + y, " Type: %s ",
- scd->type < MAX_SCSI_DEVICE_CODE ?
- scsi_device_types[(int) scd->type] : "Unknown ");
- y += sprintf(buffer + len + y, " ANSI"
- " SCSI revision: %02x", (scd->scsi_level - 1) ? scd->scsi_level - 1 : 1);
+ y += sprintf(buffer + len + y, "\n ANSI SCSI revision: %02x",
+ (scd->scsi_level - 1) ? scd->scsi_level - 1 : 1);
if (scd->scsi_level == 2)
y += sprintf(buffer + len + y, " CCS\n");
else
diff -ur linux-old/drivers/scsi/scsi_scan.c linux/drivers/scsi/scsi_scan.c
--- linux-old/drivers/scsi/scsi_scan.c Sat May 4 09:11:44 2002
+++ linux/drivers/scsi/scsi_scan.c Sat May 4 07:52:25 2002
@@ -231,13 +231,7 @@
printk(" ");
}

- printk("\n");
-
- i = data[0] & 0x1f;
-
- printk(" Type: %s ",
- i < MAX_SCSI_DEVICE_CODE ? scsi_device_types[i] : "Unknown ");
- printk(" ANSI SCSI revision: %02x", data[2] & 0x07);
+ printk("\n ANSI SCSI revision: %02x", data[2] & 0x07);
if ((data[2] & 0x07) == 1 && (data[3] & 0x0f) == 1)
printk(" CCS\n");
else
diff -ur linux-old/drivers/scsi/scsi_syms.c linux/drivers/scsi/scsi_syms.c
--- linux-old/drivers/scsi/scsi_syms.c Sat May 4 09:11:44 2002
+++ linux/drivers/scsi/scsi_syms.c Sat May 4 07:52:50 2002
@@ -87,7 +87,6 @@
EXPORT_SYMBOL(scsi_hostlist);
EXPORT_SYMBOL(scsi_hosts);
EXPORT_SYMBOL(scsi_devicelist);
-EXPORT_SYMBOL(scsi_device_types);

/*
* Externalize timers so that HBAs can safely start/restart commands.
diff -ur linux-old/drivers/scsi/sg.c linux/drivers/scsi/sg.c
--- linux-old/drivers/scsi/sg.c Sat May 4 09:11:44 2002
+++ linux/drivers/scsi/sg.c Sat May 4 06:56:26 2002
@@ -254,7 +254,7 @@

static int sg_open(struct inode * inode, struct file * filp)
{
- int dev = minor(inode->i_rdev);
+ unsigned int dev = minor(inode->i_rdev);
int flags = filp->f_flags;
Sg_device * sdp;
Sg_fd * sfp;
@@ -1035,7 +1035,7 @@
static void sg_cmd_done_bh(Scsi_Cmnd * SCpnt)
{
Scsi_Request * SRpnt = SCpnt->sc_request;
- int dev = minor(SRpnt->sr_request.rq_dev);
+ unsigned int dev = minor(SRpnt->sr_request.rq_dev);
Sg_device * sdp = NULL;
Sg_fd * sfp;
Sg_request * srp = NULL;
@@ -2687,7 +2687,8 @@
Sg_fd * fp;
Sg_request * srp;
struct scsi_device * scsidp;
- int dev, k, m, blen, usg;
+ unsigned int dev;
+ int k, m, blen, usg;

scsidp = sdp->device;
if (NULL == scsidp) {
diff -ur linux-old/drivers/scsi/sr.c linux/drivers/scsi/sr.c
--- linux-old/drivers/scsi/sr.c Sat May 4 09:11:44 2002
+++ linux/drivers/scsi/sr.c Sat May 4 07:58:13 2002
@@ -99,11 +99,13 @@

static void sr_release(struct cdrom_device_info *cdi)
{
- if (scsi_CDs[minor(cdi->dev)].device->sector_size > 2048)
- sr_set_blocklength(minor(cdi->dev), 2048);
- scsi_CDs[minor(cdi->dev)].device->access_count--;
- if (scsi_CDs[minor(cdi->dev)].device->host->hostt->module)
- __MOD_DEC_USE_COUNT(scsi_CDs[minor(cdi->dev)].device->host->hostt->module);
+ unsigned int minor = minor(cdi->dev);
+
+ if (scsi_CDs[minor].device->sector_size > 2048)
+ sr_set_blocklength(minor, 2048);
+ scsi_CDs[minor].device->access_count--;
+ if (scsi_CDs[minor].device->host->hostt->module)
+ __MOD_DEC_USE_COUNT(scsi_CDs[minor].device->host->hostt->module);
if (sr_template.module)
__MOD_DEC_USE_COUNT(sr_template.module);
}
@@ -145,12 +147,14 @@
int sr_media_change(struct cdrom_device_info *cdi, int slot)
{
int retval;
+ unsigned int minor;

if (CDSL_CURRENT != slot) {
/* no changer support */
return -EINVAL;
}
- retval = scsi_ioctl(scsi_CDs[minor(cdi->dev)].device,
+ minor = minor(cdi->dev);
+ retval = scsi_ioctl(scsi_CDs[minor].device,
SCSI_IOCTL_TEST_UNIT_READY, 0);

if (retval) {
@@ -159,13 +163,13 @@
* and we will figure it out later once the drive is
* available again. */

- scsi_CDs[minor(cdi->dev)].device->changed = 1;
+ scsi_CDs[minor].device->changed = 1;
return 1; /* This will force a flush, if called from
* check_disk_change */
};

- retval = scsi_CDs[minor(cdi->dev)].device->changed;
- scsi_CDs[minor(cdi->dev)].device->changed = 0;
+ retval = scsi_CDs[minor].device->changed;
+ scsi_CDs[minor].device->changed = 0;
/* If the disk changed, the capacity will now be different,
* so we force a re-read of this information */
if (retval) {
@@ -179,9 +183,9 @@
* be trying to use something that is too small if the disc
* has changed.
*/
- scsi_CDs[minor(cdi->dev)].needs_sector_size = 1;
+ scsi_CDs[minor].needs_sector_size = 1;

- scsi_CDs[minor(cdi->dev)].device->sector_size = 2048;
+ scsi_CDs[minor].device->sector_size = 2048;
}
return retval;
}
@@ -250,18 +254,21 @@

static request_queue_t *sr_find_queue(kdev_t dev)
{
+ unsigned int minor = minor(dev);
/*
* No such device
*/
- if (minor(dev) >= sr_template.dev_max || !scsi_CDs[minor(dev)].device)
+ if (minor >= sr_template.dev_max || !scsi_CDs[minor].device)
return NULL;

- return &scsi_CDs[minor(dev)].device->request_queue;
+ return &scsi_CDs[minor].device->request_queue;
}

static int sr_init_command(Scsi_Cmnd * SCpnt)
{
- int dev, devm, block=0, this_count, s_size;
+ int dev;
+ unsigned int devm;
+ int block=0, this_count, s_size;

devm = minor(SCpnt->request.rq_dev);
dev = DEVICE_NR(SCpnt->request.rq_dev);
@@ -397,22 +404,22 @@

static int sr_open(struct cdrom_device_info *cdi, int purpose)
{
+ unsigned int minor = minor(cdi->dev);
check_disk_change(cdi->dev);

- if (minor(cdi->dev) >= sr_template.dev_max
- || !scsi_CDs[minor(cdi->dev)].device) {
+ if (minor >= sr_template.dev_max || !scsi_CDs[minor].device) {
return -ENXIO; /* No such device */
}
/*
* If the device is in error recovery, wait until it is done.
* If the device is offline, then disallow any access to it.
*/
- if (!scsi_block_when_processing_errors(scsi_CDs[minor(cdi->dev)].device)) {
+ if (!scsi_block_when_processing_errors(scsi_CDs[minor].device)) {
return -ENXIO;
}
- scsi_CDs[minor(cdi->dev)].device->access_count++;
- if (scsi_CDs[minor(cdi->dev)].device->host->hostt->module)
- __MOD_INC_USE_COUNT(scsi_CDs[minor(cdi->dev)].device->host->hostt->module);
+ scsi_CDs[minor].device->access_count++;
+ if (scsi_CDs[minor].device->host->hostt->module)
+ __MOD_INC_USE_COUNT(scsi_CDs[minor].device->host->hostt->module);
if (sr_template.module)
__MOD_INC_USE_COUNT(sr_template.module);

@@ -421,8 +428,8 @@
* this is the case, and try again.
*/

- if (scsi_CDs[minor(cdi->dev)].needs_sector_size)
- get_sectorsize(minor(cdi->dev));
+ if (scsi_CDs[minor].needs_sector_size)
+ get_sectorsize(minor);

return 0;
}
@@ -616,7 +623,6 @@
n = buffer[3] + 4;
scsi_CDs[i].cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
scsi_CDs[i].readcd_known = 1;
- scsi_CDs[i].readcd_cdda = buffer[n + 5] & 0x01;
/* print some capability bits */
printk("sr%i: scsi3-mmc drive: %dx/%dx %s%s%s%s%s%s\n", i,
((buffer[n + 14] << 8) + buffer[n + 15]) / 176,
@@ -671,13 +677,14 @@
*/
static int sr_packet(struct cdrom_device_info *cdi, struct cdrom_generic_command *cgc)
{
- Scsi_Device *device = scsi_CDs[minor(cdi->dev)].device;
+ unsigned int minor = minor(cdi->dev);
+ Scsi_Device *device = scsi_CDs[minor].device;

/* set the LUN */
if (device->scsi_level <= SCSI_2)
cgc->cmd[1] |= device->lun << 5;

- cgc->stat = sr_do_ioctl(minor(cdi->dev), cgc->cmd, cgc->buffer, cgc->buflen, cgc->quiet, cgc->data_direction, cgc->sense);
+ cgc->stat = sr_do_ioctl(minor, cgc->cmd, cgc->buffer, cgc->buflen, cgc->quiet, cgc->data_direction, cgc->sense);

return cgc->stat;
}
@@ -761,7 +768,6 @@
scsi_CDs[i].device->ten = 1;
scsi_CDs[i].device->remap = 1;
scsi_CDs[i].readcd_known = 0;
- scsi_CDs[i].readcd_cdda = 0;
sr_sizes[i] = scsi_CDs[i].capacity >> (BLOCK_SIZE_BITS - 9);

scsi_CDs[i].cdi.ops = &sr_dops;
diff -ur linux-old/drivers/scsi/sr.h linux/drivers/scsi/sr.h
--- linux-old/drivers/scsi/sr.h Fri Jul 20 06:18:31 2001
+++ linux/drivers/scsi/sr.h Sat May 4 07:58:37 2002
@@ -30,7 +30,6 @@
unsigned use:1; /* is this device still supportable */
unsigned xa_flag:1; /* CD has XA sectors ? */
unsigned readcd_known:1; /* drive supports READ_CD (0xbe) */
- unsigned readcd_cdda:1; /* reading audio data using READ_CD */
struct cdrom_device_info cdi;
} Scsi_CD;

diff -ur linux-old/drivers/scsi/sr_ioctl.c linux/drivers/scsi/sr_ioctl.c
--- linux-old/drivers/scsi/sr_ioctl.c Sat May 4 09:11:44 2002
+++ linux/drivers/scsi/sr_ioctl.c Sat May 4 06:59:15 2002
@@ -333,7 +333,8 @@
int sr_audio_ioctl(struct cdrom_device_info *cdi, unsigned int cmd, void *arg)
{
u_char sr_cmd[10];
- int result, target = minor(cdi->dev);
+ int result;
+ unsigned int target = minor(cdi->dev);
unsigned char buffer[32];

memset(sr_cmd, 0, sizeof(sr_cmd));
@@ -539,7 +540,7 @@
int sr_dev_ioctl(struct cdrom_device_info *cdi,
unsigned int cmd, unsigned long arg)
{
- int target;
+ unsigned int target;

target = minor(cdi->dev);

diff -ur linux-old/drivers/scsi/sr_vendor.c linux/drivers/scsi/sr_vendor.c
--- linux-old/drivers/scsi/sr_vendor.c Sat May 4 09:11:44 2002
+++ linux/drivers/scsi/sr_vendor.c Sat May 4 06:59:49 2002
@@ -156,7 +156,8 @@
unsigned long sector;
unsigned char *buffer; /* the buffer for the ioctl */
unsigned char cmd[MAX_COMMAND_SIZE]; /* the scsi-command */
- int rc, no_multi, minor;
+ int rc, no_multi;
+ unsigned int minor;

minor = minor(cdi->dev);
if (scsi_CDs[minor].cdi.mask & CDC_MULTI_SESSION)
diff -ur linux-old/drivers/scsi/wd7000.c linux/drivers/scsi/wd7000.c
--- linux-old/drivers/scsi/wd7000.c Sat May 4 09:11:44 2002
+++ linux/drivers/scsi/wd7000.c Sat May 4 07:59:58 2002
@@ -1469,8 +1469,6 @@
if (scd->host->host_no == hostno) {
SPRINTF (" [Channel: %02d, Id: %02d, Lun: %02d] ",
scd->channel, scd->id, scd->lun);
- SPRINTF ("%s ", (scd->type < MAX_SCSI_DEVICE_CODE) ?
- scsi_device_types[(short) scd->type] : "Unknown device");

for (i = 0; (i < 8) && (scd->vendor[i] >= 0x20); i++)
SPRINTF ("%c", scd->vendor[i]);
diff -ur linux-old/init/do_mounts.c linux/init/do_mounts.c
--- linux-old/init/do_mounts.c Sat May 4 09:11:56 2002
+++ linux/init/do_mounts.c Sat May 4 05:20:30 2002
@@ -364,6 +364,7 @@
return sys_symlink(path + n + 5, name);
}

+#ifdef CONFIG_BLK_DEV_RAM
static void __init change_floppy(char *fmt, ...)
{
struct termios termios;
@@ -392,8 +393,6 @@
}
}

-#ifdef CONFIG_BLK_DEV_RAM
-
int __initdata rd_prompt = 1; /* 1 = prompt for RAM disk, 0 = don't prompt */

static int __init prompt_ramdisk(char *str)
@@ -843,6 +842,7 @@
}

#ifdef BUILD_CRAMDISK
+# ifdef CONFIG_BLK_DEV_RAM

/*
* gzip declarations
@@ -986,5 +986,5 @@
kfree(window);
return result;
}
-
+# endif
#endif /* BUILD_CRAMDISK */


Attachments:
cleanup-scsi-2.5.2-pre9.patch (15.93 kB)

2002-01-08 16:15:53

by Cameron, Steve

[permalink] [raw]
Subject: Re: PATCH 2.5.2-pre9 scsi cleanup

Martin Dalecki ([email protected]) wrote:

[...]
> --- linux-old/drivers/scsi/scsi.c Sat May 4 09:11:44 2002
> +++ linux/drivers/scsi/scsi.c Sat May 4 07:49:19 2002
> @@ -139,25 +139,7 @@
> */
> unsigned int scsi_logging_level;
>
> -const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE] =
> -{
> - "Direct-Access ",
> - "Sequential-Access",
> - "Printer ",
> - "Processor ",
> - "WORM ",
> - "CD-ROM ",
> - "Scanner ",
[...etc...]

Hmmm, I was using that.... (In, for example,
the cciss patch here: http://www.geocities.com/smcameron
It's not any big deal, though.)

-- steve

2002-01-08 16:54:56

by Martin Dalecki

[permalink] [raw]
Subject: Re: PATCH 2.5.2-pre9 scsi cleanup

Cameron, Steve wrote:

>Martin Dalecki ([email protected]) wrote:
>
>[...]
>
>>--- linux-old/drivers/scsi/scsi.c Sat May 4 09:11:44 2002
>>+++ linux/drivers/scsi/scsi.c Sat May 4 07:49:19 2002
>>@@ -139,25 +139,7 @@
>> */
>> unsigned int scsi_logging_level;
>>
>>-const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE] =
>>-{
>>- "Direct-Access ",
>>- "Sequential-Access",
>>- "Printer ",
>>- "Processor ",
>>- "WORM ",
>>- "CD-ROM ",
>>- "Scanner ",
>>
>[...etc...]
>
>Hmmm, I was using that.... (In, for example,
>the cciss patch here: http://www.geocities.com/smcameron
>It's not any big deal, though.)
>
Precisely this "not any big deal" is the point: It was the wrong
approach to a
trivial problem ;-).

2002-01-08 17:19:57

by Cameron, Steve

[permalink] [raw]
Subject: RE: PATCH 2.5.2-pre9 scsi cleanup

Martin Dalecki [mailto:[email protected]] wrote,
regarding removal of scsi_device_types[] from drivers/scsi/scsi.c

> Cameron, Steve wrote:
[...]
> >Hmmm, I was using that.... (In, for example,
> >the cciss patch here: http://www.geocities.com/smcameron
> >It's not any big deal, though.)
> >
> Precisely this "not any big deal" is the point: It was the wrong
> approach to a trivial problem ;-).

So what's the right approach? I can invent my own easily enough,
but each driver doing its own thing doesn't seem right. I assumed
that it was in scsi.c foi common usage, so each driver that wanted
to say, use these device type strings in diagnostic messages or
some such wouldn't have to reinvent this wheel, and so all the
drivers would consistently use the same names. Will it be
replaced with something else?

Just want to know so I don't waste (even more :-) time
doing something dumb.

Thanks,

-- steve

2002-01-08 18:54:45

by Martin Dalecki

[permalink] [raw]
Subject: Re: PATCH 2.5.2-pre9 scsi cleanup

Cameron, Steve wrote:

>Martin Dalecki [mailto:[email protected]] wrote,
>regarding removal of scsi_device_types[] from drivers/scsi/scsi.c
>
>>Cameron, Steve wrote:
>>
>[...]
>
>>>Hmmm, I was using that.... (In, for example,
>>>the cciss patch here: http://www.geocities.com/smcameron
>>>It's not any big deal, though.)
>>>
>>Precisely this "not any big deal" is the point: It was the wrong
>>approach to a trivial problem ;-).
>>
>
>So what's the right approach? I can invent my own easily enough,
>but each driver doing its own thing doesn't seem right. I assumed
>that it was in scsi.c foi common usage, so each driver that wanted
>to say, use these device type strings in diagnostic messages or
>some such wouldn't have to reinvent this wheel, and so all the
>drivers would consistently use the same names. Will it be
>replaced with something else?
>
>Just want to know so I don't waste (even more :-) time
>doing something dumb.
>

Please just case in the ->type enum. And if you wan't to provide special
messages, well
then please do it yourself, the removal showed, that nearly no one
driver used the generic
stuff, so it wasn't trully generic at all. (It should be handled by some
userlevel stuff anyway...)


2002-01-09 04:05:56

by Masanori Goto

[permalink] [raw]
Subject: Re: PATCH 2.5.2-pre9 scsi cleanup

At Tue, 08 Jan 2002 10:29:48 +0100,
Martin Dalecki <[email protected]> wrote:
> The attached patch does the following.
>
> 1. Clean up some ifdef confusion in do_mount
>
> 2. Clean up the scsi code to make ppa.c work.
>
> 3. Clean up some unneccessary unneeded globals from scsi code.

I don't understand why below removal are needed.
Do you think to replace with function like scsi_device_types()?

> -const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE] =
> -{
> - "Direct-Access ",
> - "Sequential-Access",
> - "Printer ",
> - "Processor ",
> - "WORM ",
> - "CD-ROM ",
> - "Scanner ",
> - "Optical Device ",
> - "Medium Changer ",
> - "Communications ",
> - "Unknown ",
> - "Unknown ",
> - "Unknown ",
> - "Enclosure ",
> -};
> -
> -/*

-- gotom

2002-01-09 05:03:41

by Douglas Gilbert

[permalink] [raw]
Subject: Re: PATCH 2.5.2-pre9 scsi cleanup

Martin Dalecki <[email protected]>

> The attached patch does the following.
>
> 1. Clean up some ifdef confusion in do_mount
>
> 2. Clean up the scsi code to make ppa.c work.
>
> 3. Clean up some unneccessary unneeded globals from scsi code.
>
> 4. Make a bit more sure, that the minor() and friends end up in
> unsigned int's.

<snip/>

Martin,
Please don't post a omnibus SCSI subsystem patch like this.

Most of the code you are changing is actively maintained.
For example:
- scsi mid-level + sr [Jens Axboe]
- ppa [Tim Waugh]
- sg [me]

Some of us have grown attached to the way 'cat /proc/scsi/scsi'
looks. More importantly, many distributions have scripts that
parse it. GOTO Masanori's <[email protected]> suggestion of
of an exported scsi_device_types() seems a better option.
Also there are 32 SCSI device types (0 to 31) and perhaps
the "unknown"s and those that are missing could be replaced
by scsi_dev_<n> .

Also there is an appropriate newsgroup for this sort of
proposal and it is called: [email protected]

Doug Gilbert

2002-01-09 09:39:51

by Martin Dalecki

[permalink] [raw]
Subject: Re: PATCH 2.5.2-pre9 scsi cleanup

Douglas Gilbert wrote:

>Martin Dalecki <[email protected]>
>
>>The attached patch does the following.
>>
>>1. Clean up some ifdef confusion in do_mount
>>
>>2. Clean up the scsi code to make ppa.c work.
>>
>>3. Clean up some unneccessary unneeded globals from scsi code.
>>
>>4. Make a bit more sure, that the minor() and friends end up in
>>unsigned int's.
>>
>
><snip/>
>
>Martin,
>Please don't post a omnibus SCSI subsystem patch like this.
>
>Most of the code you are changing is actively maintained.
>For example:
> - scsi mid-level + sr [Jens Axboe]
> - ppa [Tim Waugh]
> - sg [me]
>
>Some of us have grown attached to the way 'cat /proc/scsi/scsi'
>
There where just two drivers for obsolete hardware which actually used this.