Messages in this thread | | | Date | Thu, 7 Oct 2004 22:15:37 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3 |
| |
> So, skipping the EXPORTs for now, how do you guys > feel about the driver ?
- please kill the #ifndef SERVICE_ACTION_IN section - please use <scsi/scsi.h> sense data ifdefs instead of adding your own - please kill your scsi_to_pci_dma_dirusage, it's been obsoleted for a reason - please remove DRPINTK in favour of pr_debug from <linux/kernel.h> - please removæ the global host list, not just the exports for it - dito for qs_notify - code like + if (dev->raid) { + memset(dev->raid, 0, sizeof(struct qs_raid)); + kfree(dev->raid); + } is totally bogus, you defeat the slab poisoning with that - qs_alloc duplicates kcalloc - the !dev case in qs_scsi_queuecomman can't happen - no need for the case in sc->host_scribble = (void *)(dev->uhba); - never mess with eh_timeout from inside a driver - please don't implemente the HDIO_ ioctls, Jeff said this can be done via SG_IO - if ->info return a static string you can just store it into ->name - please don't discard the exact error returned from pci_enable_device(pdev) - never call scsi_set_device() in a new driver, scsi_add_host does that for you - never use scsi_assign_lock in a new driver, the midlayer already has a per-host lock for you - please use the kernel/kthread.c interface for your kernel thread - never cast your ioregs before iounmap. - in fact it looks like you want to run sparse over the driver and fix up everything it found - at least __iomem annotations are missing - please don't use set_fs(KERNEL_DS), split up the underlying code for user and kernel buffers
This was just a quick 5 minute review, I'll give it a deeper look once I get a little bit more time. - 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/
| |