Messages in this thread | | | Date | Fri, 27 Feb 2004 12:04:51 +1100 | From | Stephen Rothwell <> | Subject | Re: [PATCH] iSeries virtual disk |
| |
Hi Christoph,
On Thu, 26 Feb 2004 10:51:56 +0100 Christoph Hellwig <hch@lst.de> wrote: > > On Thu, Feb 26, 2004 at 05:23:25PM +1100, Stephen Rothwell wrote: > > Unfortunately, things have moved on in the last couple of weeks and to > > fix everyone;s abjections, I need to include in this patch a ppc64 specific > > version of the dma_mapping routines. They are pretty straight forward. > > While thes dma mapping changes look good they really don't belong into a > patch for a new driver. Can you split them out into a separate patch?
Yeah, I don't know what I was thinking :-(
> > Disks are now called /dev/iseries/vd<x><n> (without devfs) or > > /dev/viod/disc<n>/part<n> (with devfs). Up to 7 partitions are supported > > on each of up to 32 disks. > > Hmm, different names for devfs vs non-devfs seems silly. But I think we > can't easily get rid of the disc<foo>/part sillyness. Can you at least > make both use the same prefix?
I was trying to maintain compatability with a previously published (out of tree) version of the driver, but I think I have broken that anyway, so I will change ths as well.
> > config VIOCD > > tristate "iSeries Virtual I/O CD support" > > help > > What happened to this driver, btw?
Coming soon to a mailing list near you :-) (I decided to separate them)
> So vio on iseries claims to be pci? That's a little silly if you ask > me..
It is an expedience and changes are already in train to change this.
> > +#include <linux/major.h> > > +#include <linux/fs.h> > > +#include <asm/uaccess.h> > > please put all asm/* headers after linux/*
done.
> > +#include <linux/hdreg.h> > > probably not needed anymore without ide emulation.
it is, actually. for HDIO_GETGEO ioctl stuff.
> > +#include <linux/seq_file.h> > > you don't use this anymore, do you?
Thanks, missed removing it with the rest of the proc stuff.
> > +static int viodasd_max_disk; > > the code surrounding this doesn't look right yet. You first initialize > it to the maximum value and then reset it in a magic even handler? > I think that logic needs some clarification.
The "magic event handler" is synchronous with the probe_disk routine. I agree it is a bit confusing, but, at least I have the comment there about the side effects of the probe_disk routine. Changed slightly.
> > +#define DEVICE_NO(cell) ((struct viodasd_device *)(cell) - &viodasd_devices[0]) > > Aniother idea: It might be better to just put a disk_no member into > struct viodasd_device - then you can allocate the struct viodasd_device > dynamically one by one and easily support lots of disks without overhead > for lowend configurations (remember we have a 32bit dev_t now)
Can I leave this for now?
> > +extern struct device *iSeries_vio_dev; > > shouldn't this be in some header?
Yes, it should, but can I leave this to the future as well, please?
> > +static void viodasd_init_disk(struct viodasd_device *d); > > What's preventing this one to be implemented above it's usage? or even > better merging it into it's only caller.
Nothing, now.
> > +static void viodasd_end_request(struct request *req, int uptodate, > > + int num_sectors) > > +{ > > + if (end_that_request_first(req, uptodate, num_sectors)) > > + return; > > + add_disk_randomness(req->rq_disk); > > + end_that_request_last(req); > > +} > > indentation looks messed up here.
Yeah, spaces v. tabs.
> > +static void viodasd_init_disk(struct viodasd_device *d) > > +{ > > + struct gendisk *g; > > + struct request_queue *q; > > + int disk_no = DEVICE_NO(d); > > + > > + if (d->disk) > > + return; > > as there's only one caller it shouldn't happen, should it?
Correct.
> > + g->private_data = (void *)d; > > no need to cast to void * here - this is implicit in C
OK.
> > + for (i = 0; i < MAX_DISKNO; i++) { > > + d = &viodasd_devices[i]; > > + if (d->disk) { > > How can d->disk ever be NULLL here?
Because it was never initialised ... we only create the gendisks for disks that exist.
> > + if (d->disk->queue) > > + blk_cleanup_queue(d->disk->queue); > > + del_gendisk(d->disk); > > the queue cleanups needs to be after put_gendisk. Also where did you > see d->disk->queue as NULL?
OK, fixed.
New patch following soon ... -- Cheers, Stephen Rothwell sfr@canb.auug.org.au http://www.canb.auug.org.au/~sfr/ [unhandled content-type:application/pgp-signature] | |