Messages in this thread | | | Date | Sun, 4 Jul 2004 14:05:44 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH] CDRW packet writing support for 2.6.7-bk13 |
| |
> + * - Generic interface for UDF to submit large packets for variable length > + * packet writing
Huh, what's bad about bios?
> +#include <linux/buffer_head.h>
Where do you need buffer_head.h?
> +#define SCSI_IOCTL_SEND_COMMAND 1
Please include scsi_ioctl.h instead of duplicating it.
> +static struct gendisk *disks[MAX_WRITERS];
Please add a pointer to the gendisk to struct pktcdvd_device instead of a global array
> + > +static struct pktcdvd_device *pkt_find_dev(request_queue_t *q) > +{ > + int i; > + > + for (i = 0; i < MAX_WRITERS; i++) > + if (pkt_devs[i].bdev && bdev_get_queue(pkt_devs[i].bdev) == q) > + return &pkt_devs[i]; > + > + return NULL; > +}
Please just store the pktcdvd_device * in q->queuedata.
> + sprintf(current->comm, pd->name);
not needed, saemonize does it for you.
> +static int pkt_get_minor(struct pktcdvd_device *pd) > +{ > + int minor; > + for (minor = 0; minor < MAX_WRITERS; minor++) > + if (pd == &pkt_devs[minor]) > + break; > + BUG_ON(minor == MAX_WRITERS); > + return minor; > +}
Shouldn't be needed at all. Use an idr allocator to get a free minor in the setup, the actual I/O code shouldn't care about minors at all (if you follow my suggestions in the begining of this mail that should be taken care of) and the actual code should never
> + pd->cdrw.elv_merge_fn = q->elevator.elevator_merge_fn; > + pd->cdrw.elv_completed_req_fn = q->elevator.elevator_completed_req_fn; > + pd->cdrw.merge_requests_fn = q->merge_requests_fn; > + q->elevator.elevator_merge_fn = pkt_lowlevel_elv_merge_fn; > + q->elevator.elevator_completed_req_fn = pkt_lowlevel_elv_completed_req_fn;
This looks really really fishy. Playing with other drivers' elevator settings can't be safe. I'd say wait for the runtime selectable I/O scheduler that's planned for a while and add a special packetwriting scheduler that you switch to.
> +/* > + * called when the device is closed. makes sure that the device flushes > + * the internal cache before we close. > + */ > +static void pkt_release_dev(struct pktcdvd_device *pd, int flush) > +{ > + struct block_device *bdev; > + > + atomic_dec(&pd->refcnt); > + if (atomic_read(&pd->refcnt) > 0) > + return; > + > + bdev = bdget(pd->pkt_dev); > + if (bdev) {
You reallu should keep a reference to the underlying bdev as long as you use it, aka bdev_get + blkdev_get in ->open, blkdev_put in ->release
> + fsync_bdev(bdev);
fs/block_dev.c already does a sync_blockdev() on last close, that should be enough.
> +static int pkt_proc_device(struct pktcdvd_device *pd, char *buf) > +{
seq_file interface please, or even better a one value per file sysfs interface.
> + pd->cdrw.pid = kernel_thread(kcdrwd, pd, CLONE_FS | CLONE_FILES | CLONE_SIGHAND);
please use the kernel/ktread.c infastructure.
> +static int pkt_setup_dev(struct pktcdvd_device *pd, unsigned int arg) > +{ > + struct inode *inode; > + struct file *file; > + int ret; > + > + if ((file = fget(arg)) == NULL) { > + printk("pktcdvd: bad file descriptor passed\n"); > + return -EBADF; > + } > + > + ret = -EINVAL; > + if ((inode = file->f_dentry->d_inode) == NULL) { > + printk("pktcdvd: huh? file descriptor contains no inode?\n"); > + goto out; > + }
If fget is successfull file->f_dentry->d_inode can't be NULL.
> + case BLKROSET: > + if (capable(CAP_SYS_ADMIN)) > + clear_bit(PACKET_WRITABLE, &pd->flags); > + case BLKROGET: > + case BLKSSZGET: > + case BLKFLSBUF: > + if (!pd->bdev) > + return -ENXIO; > + return -EINVAL; /* Handled by blkdev layer */ > +
These aren't handled by drivers anyore in 2.6
> + for (i = 0; i < MAX_WRITERS; i++) { > + disks[i] = alloc_disk(1); > + if (!disks[i]) > + goto out_mem2; > + } > + > + for (i = 0; i < MAX_WRITERS; i++) { > + struct pktcdvd_device *pd = &pkt_devs[i]; > + struct gendisk *disk = disks[i]; > + disk->major = PACKET_MAJOR; > + disk->first_minor = i; > + disk->fops = &pktcdvd_ops; > + disk->flags = GENHD_FL_REMOVABLE; > + sprintf(disk->disk_name, "pktcdvd%d", i); > + sprintf(disk->devfs_name, "pktcdvd/%d", i); > + disk->private_data = pd; > + disk->queue = blk_alloc_queue(GFP_KERNEL); > + if (!disk->queue) > + goto out_mem3; > + add_disk(disk); > + }
Please allocate all these on demand only when you actually attach a device.
All in all I really wonder whether a separate driver is really that a good fit for the functionality or whether it should be more integrated with the block layer, ala drivers/block/scsi_ioctl.c - 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/
| |