Re: [PATCH] iSeries virtual disk
From: Christoph Hellwig (hch_at_lst.de)
Date: 02/26/04
- Previous message: Nathan Scott: "Re: another hard disk broken or xfs problems?"
- In reply to: Stephen Rothwell: "Re: [PATCH] iSeries virtual disk"
- Next in thread: Stephen Rothwell: "Re: [PATCH] iSeries virtual disk"
- Reply: Stephen Rothwell: "Re: [PATCH] iSeries virtual disk"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: Thu, 26 Feb 2004 10:51:56 +0100 To: Stephen Rothwell <sfr@canb.auug.org.au>
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?
> 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?
> config VIOCD
> tristate "iSeries Virtual I/O CD support"
> help
What happened to this driver, btw?
> +void dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
> + enum dma_data_direction direction)
> +{
> + if (dev->bus == &pci_bus_type)
> + pci_unmap_single(to_pci_dev(dev), dma_addr, size, (int)direction);
>
>
> +#ifdef CONFIG_PPC_PSERIES
> + else if (dev->bus == &vio_bus_type)
> + vio_unmap_single(to_vio_dev(dev), dma_addr, size, (int)direction);
> +#endif
So vio on iseries claims to be pci? That's a little silly if you ask
me..
> +#include <linux/major.h>
> +#include <linux/fs.h>
> +#include <asm/uaccess.h>
please put all asm/* headers after linux/*
> +#include <linux/hdreg.h>
probably not needed anymore without ide emulation.
> +#include <linux/seq_file.h>
you don't use this anymore, do you?
> +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.
> +#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)
> +extern struct device *iSeries_vio_dev;
shouldn't this be in some header?
> +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.
> +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.
> +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?
> + g->private_data = (void *)d;
no need to cast to void * here - this is implicit in C
> + for (i = 0; i < MAX_DISKNO; i++) {
> + d = &viodasd_devices[i];
> + if (d->disk) {
How can d->disk ever be NULLL here?
>
> + 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?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- Previous message: Nathan Scott: "Re: another hard disk broken or xfs problems?"
- In reply to: Stephen Rothwell: "Re: [PATCH] iSeries virtual disk"
- Next in thread: Stephen Rothwell: "Re: [PATCH] iSeries virtual disk"
- Reply: Stephen Rothwell: "Re: [PATCH] iSeries virtual disk"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
|
