Re: [PATCH] Add new driver for LSI 3ware 9750

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

[-- Attachment #1: Type: Text/Plain, Size: 11292 bytes --]

adam radford wrote:

> +static int use_msi = 0;

Static variables are automatically 0.

> +/* This function will look up an AEN severity string */
> +static char *twl_aen_severity_lookup(unsigned char severity_code)
> +{
> +	char *retval = NULL;
> +
> +	if ((severity_code < (unsigned char) TW_AEN_SEVERITY_ERROR) ||
> +	    (severity_code > (unsigned char) TW_AEN_SEVERITY_DEBUG))
> +		goto out;
> +
> +	retval = twl_aen_severity_table[severity_code];
> +out:
> +	return retval;
> +} /* End twl_aen_severity_lookup() */

I would find it more readable with direct "return NULL" and without the goto.

> +/* This function will read the aen queue from the isr */
> +static int twl_aen_read_queue(TW_Device_Extension *tw_dev, int request_id)
> +{
> +	char cdb[TW_MAX_CDB_LEN];
> +	TW_SG_Entry_ISO sglist[1];
> +	TW_Command_Full *full_command_packet;
> +	int retval = 1;
> +
> +	full_command_packet = tw_dev->command_packet_virt[request_id];
> +	memset(full_command_packet, 0, sizeof(TW_Command_Full));

sizeof(*full_command_packet)

> +	/* Initialize cdb */
> +	memset(&cdb, 0, TW_MAX_CDB_LEN);

memset(...,sizeof(cdb))

> +	cdb[0] = REQUEST_SENSE; /* opcode */
> +	cdb[4] = TW_ALLOCATION_LENGTH; /* allocation length */
> +
> +	/* Initialize sglist */
> +	memset(&sglist, 0, sizeof(TW_SG_Entry_ISO));

sizeof(sglist)

In all cases you will always get the correct size even if you change the 
variable to another type or the array to another length. This happens also in 
a bunch of other places.

> +/* This function will assign an available request id */
> +static void twl_get_request_id(TW_Device_Extension *tw_dev, int
>  *request_id) +{
> +	*request_id = tw_dev->free_queue[tw_dev->free_head];
> +	tw_dev->free_head = (tw_dev->free_head + 1) % TW_Q_LENGTH;
> +	tw_dev->state[*request_id] = TW_S_STARTED;
> +} /* End twl_get_request_id() */

Why not simply return the request id instead of being a void function and 
taking that as pointer?

> +/* This function will poll for a response */
> +static int twl_poll_response(TW_Device_Extension *tw_dev, int
> request_id, int seconds)
> +{
> +	unsigned long before;
> +	dma_addr_t mfa;
> +	u32 regh, regl;
> +	u32 response;
> +	int retval = 1;
> +	int found = 0;
> +
> +	before = jiffies;
> +
> +	while (!found) {
> +		if (sizeof(dma_addr_t) > 4) {
> +			regh = readl(TWL_HOBQPH_REG_ADDR(tw_dev));
> +			regl = readl(TWL_HOBQPL_REG_ADDR(tw_dev));
> +			mfa = ((u64)regh << 32) | regl;
> +		} else
> +			mfa = readl(TWL_HOBQPL_REG_ADDR(tw_dev));
> +
> +		response = (u32)mfa;

Reading regh is useless here. And if you need to read it for protocol reasons 
at the end it's at least useless to put it into mfa, simply reading 
TWL_HOBQPL_REG_ADDR(tw_dev) into response should be enough.

> +static int twl_initialize_device_extension(TW_Device_Extension *tw_dev)

Device extension? Don't call anything IRP ;) Bah, I was so lucky that I had 
forgotten that pain for a while ;)

> +{
> +	int i, retval = 1;
> +
> +	/* Initialize command packet buffers */
> +	if (twl_allocate_memory(tw_dev, sizeof(TW_Command_Full), 0)) {
> +		TW_PRINTK(tw_dev->host, TW_DRIVER, 0x9, "Command packet memory
> allocation failed");
> +		goto out;
> +	}

Your gotos here don't do anything useful as there is no error handling at the 
destination so you could simply do "return something" here as that would allow 
anyone to spot what's going on without scrolling. Also you should return some 
useful return code (like ENOMEM here) instead of just "1" as that could ease 
debugging.

> +/* This funciton returns unit geometry in cylinders/heads/sectors */
> +static int twl_scsi_biosparam(struct scsi_device *sdev, struct
> block_device *bdev, sector_t capacity, int geom[])
> +{
> +	int heads, sectors, cylinders;
> +	TW_Device_Extension *tw_dev;
> +
> +	tw_dev = (TW_Device_Extension *)sdev->host->hostdata;
> +
> +	if (capacity >= 0x200000) {
> +		heads = 255;
> +		sectors = 63;
> +		cylinders = sector_div(capacity, heads * sectors);
> +	} else {
> +		heads = 64;
> +		sectors = 32;
> +		cylinders = sector_div(capacity, heads * sectors);
> +	}

The calculation of cylinders is both the same, you could move it out of the if 
or just directly assign the result to geom[2].

> +/* This function will probe and initialize a card */
> +static int __devinit twl_probe(struct pci_dev *pdev, const struct
> pci_device_id *dev_id)
> +{
> +	struct Scsi_Host *host = NULL;
> +	TW_Device_Extension *tw_dev;
> +	resource_size_t mem_addr, mem_len;
> +	int retval = -ENODEV;
> +	int *ptr_phycount, phycount=0;
> +
> +	retval = pci_enable_device(pdev);
> +	if (retval) {
> +		TW_PRINTK(host, TW_DRIVER, 0x17, "Failed to enable pci device");
> +		goto out_disable_device;
> +	}

You may want to look at devres (see Documentation/driver-model/devres.txt) as 
that could make you the live a bit easier. It would care both for error 
handling here as well for device release.

> +	pci_set_master(pdev);
> +	pci_try_set_mwi(pdev);
> +
> +	if (pci_set_dma_mask(pdev, DMA_BIT_MASK(64))
> +	    || pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)))
> +		if (pci_set_dma_mask(pdev, DMA_BIT_MASK(32))
> +		    || pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32))) {
> +			TW_PRINTK(host, TW_DRIVER, 0x18, "Failed to set dma mask");
> +			retval = -ENODEV;
> +			goto out_disable_device;
> +		}
> +
> +	host = scsi_host_alloc(&driver_template, sizeof(TW_Device_Extension));
> +	if (!host) {
> +		TW_PRINTK(host, TW_DRIVER, 0x19, "Failed to allocate memory for
> device extension");
> +		retval = -ENOMEM;
> +		goto out_disable_device;
> +	}
> +	tw_dev = (TW_Device_Extension *)host->hostdata;

tw_dev = shost_priv(host);

> +	/* Save values to device extension */
> +	tw_dev->host = host;
> +	tw_dev->tw_pci_dev = pdev;
> +
> +	if (twl_initialize_device_extension(tw_dev)) {
> +		TW_PRINTK(tw_dev->host, TW_DRIVER, 0x1a, "Failed to initialize
> device extension");
> +		goto out_free_device_extension;
> +	}
> +
> +	/* Request IO regions */
> +	retval = pci_request_regions(pdev, "3w-sas");
> +	if (retval) {
> +		TW_PRINTK(tw_dev->host, TW_DRIVER, 0x1b, "Failed to get mem region");
> +		goto out_free_device_extension;
> +	}
> +
> +	/* Use region 1 */
> +	mem_addr = pci_resource_start(pdev, 1);
> +	mem_len = pci_resource_len(pdev, 1);
> +
> +	/* Save base address */
> +	tw_dev->base_addr = ioremap(mem_addr, mem_len);

tw_dev->base_addr = pci_iomap(pdev, 1, 0);

> +	if (!tw_dev->base_addr) {
> +		TW_PRINTK(tw_dev->host, TW_DRIVER, 0x1c, "Failed to ioremap");
> +		goto out_release_mem_region;
> +	}
> +
> +	/* Disable interrupts on the card */
> +	TWL_MASK_INTERRUPTS(tw_dev);
> +
> +	/* Initialize the card */
> +	if (twl_reset_sequence(tw_dev, 0)) {
> +		TW_PRINTK(tw_dev->host, TW_DRIVER, 0x1d, "Controller reset failed
> during probe");
> +		goto out_iounmap;
> +	}
> +
> +	/* Set host specific parameters */
> +	host->max_id = TW_MAX_UNITS;
> +	host->max_cmd_len = TW_MAX_CDB_LEN;
> +	host->max_lun = TW_MAX_LUNS;
> +	host->max_channel = 0;
> +
> +	/* Register the card with the kernel SCSI layer */
> +	retval = scsi_add_host(host, &pdev->dev);
> +	if (retval) {
> +		TW_PRINTK(tw_dev->host, TW_DRIVER, 0x1e, "scsi add host failed");
> +		goto out_iounmap;
> +	}
> +
> +	pci_set_drvdata(pdev, host);
> +
> +	printk(KERN_WARNING "3w-sas: scsi%d: Found an LSI 3ware %s
> Controller at 0x%llx, IRQ: %d.\n",
> +	       host->host_no,
> +	       (char *)twl_get_param(tw_dev, 1, TW_VERSION_TABLE,
> +				     TW_PARAM_MODEL, TW_PARAM_MODEL_LENGTH),
> +	       (u64)mem_addr, pdev->irq);
> +
> +	ptr_phycount = twl_get_param(tw_dev, 2, TW_PARAM_PHY_SUMMARY_TABLE,
> +				     TW_PARAM_PHYCOUNT, TW_PARAM_PHYCOUNT_LENGTH);
> +	if (ptr_phycount)
> +		phycount = le32_to_cpu(*(int *)ptr_phycount);
> +
> +	printk(KERN_WARNING "3w-sas: scsi%d: Firmware %s, BIOS %s, Phys: %d.\n",
> +	       host->host_no,
> +	       (char *)twl_get_param(tw_dev, 1, TW_VERSION_TABLE,
> +				     TW_PARAM_FWVER, TW_PARAM_FWVER_LENGTH),
> +	       (char *)twl_get_param(tw_dev, 2, TW_VERSION_TABLE,
> +				     TW_PARAM_BIOSVER, TW_PARAM_BIOSVER_LENGTH),
> +	       phycount);
> +
> +	/* Try to enable MSI */
> +	if (use_msi && !pci_enable_msi(pdev))
> +		set_bit(TW_USING_MSI, &tw_dev->flags);
> +
> +	/* Now setup the interrupt handler */
> +	retval = request_irq(pdev->irq, twl_interrupt, IRQF_SHARED, "3w-sas",
>  tw_dev); +	if (retval) {
> +		TW_PRINTK(tw_dev->host, TW_DRIVER, 0x1f, "Error requesting IRQ");
> +		goto out_remove_host;
> +	}

If you're using MSI you should not pass IRQF_SHARED here.

> +/* PCI Devices supported by this driver */
> +static struct pci_device_id twl_pci_tbl[] __devinitdata = {
> +	{ PCI_VENDOR_ID_3WARE, PCI_DEVICE_ID_3WARE_9750,
> +	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> +	{ }
> +};

PCI_VDEVICE(3WARE, PCI_DEVICE_ID_3WARE_9750)

> +MODULE_DEVICE_TABLE(pci, twl_pci_tbl);
> +
> +/* pci_driver initializer */
> +static struct pci_driver twl_driver = {
> +	.name		= "3w-sas",

You probably should put that string in some constant and use it all over the 
place.

> +/* This function is called on driver initialization */
> +static int __init twl_init(void)
> +{
> +	printk(KERN_WARNING "LSI 3ware SAS/SATA-RAID Controller device
> driver for Linux v%s.\n", TW_DRIVER_VERSION);

Ehm, no, that is no warning.

> +	return pci_register_driver(&twl_driver);
> +} /* End twl_init() */

> +/* AEN severity table */
> +static char *twl_aen_severity_table[] =
> +{
> +	"None", "ERROR", "WARNING", "INFO", "DEBUG", (char*) 0
> +};

NULL instead of (char*) 0

> +/* Register access macros */
> +#define TWL_STATUS_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_STATUS)
> +#define TWL_HOBQPL_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_HOBQPL)
> +#define TWL_HOBQPH_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_HOBQPH)
> +#define TWL_HOBDB_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_HOBDB)
> +#define TWL_HOBDBC_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_HOBDBC)
> +#define TWL_HIMASK_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_HIMASK)
> +#define TWL_HISTAT_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_HISTAT)
> +#define TWL_HIBQPH_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_HIBQPH)
> +#define TWL_HIBQPL_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_HIBQPL)
> +#define TWL_HIBDB_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_HIBDB)
> +#define TWL_SCRPD3_REG_ADDR(x) ((unsigned char __iomem *)x->base_addr
> + TWL_SCRPD3)

You can add to a void pointer, this is equivalent. So you might not need to 
cast here at all.

> +/* Macros */
> +#define TW_PRINTK(h,a,b,c) { \
> +if (h) \
> +printk(KERN_WARNING "3w-sas: scsi%d: ERROR: (0x%02X:0x%04X):
> %s.\n",h->host_no,a,b,c); \

shost_printk

> +else \
> +printk(KERN_WARNING "3w-sas: ERROR: (0x%02X:0x%04X): %s.\n",a,b,c); \

pr_warning

> +}

Also I don't see a point where you don't pass a host to that makro at all.

HTH

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]