Re: PATCH: PMC-Sierra MaxRAID driver to support 6Gb/s SAS RAID controller

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

[-- Attachment #1: Type: text/plain, Size: 5172 bytes --]

Anil Ravindranath wrote:
> Hi,
>
> This patch adds a driver to support PMC-Sierra 6Gb/s SAS RAID controller.
> This patch is created against scsi-misc-2.6.git.

> +module_param_named(debug, pmcraid_debug, uint, (S_IRUGO | S_IWUSR));
> +MODULE_PARM_DESC(debug,
> +		 "Enable driver verbose message logging. Set 1  to enable."
> +		 "(default: 0)");

I would say that "1" would enable this is rather obvious. And since this is 
only 0 or 1 better use bool instead of uint.


> +static int pmcraid_slave_alloc(struct scsi_device *scsi_dev)
> +{
> +	struct pmcraid_resource_entry *temp, *res = NULL;
> +	struct pmcraid_instance *pinstance;
> +	u8 target, bus, lun;
> +	unsigned long lock_flags;
> +	int rc = -ENXIO;
> +
> +	pinstance = (struct pmcraid_instance *)scsi_dev->host->hostdata;

hostdata is void* AFAIR so there is no need to cast. It's C, there is no need 
to cast to or from void* anywhere if the other thing is a pointer.

> +static int pmcraid_slave_configure(struct scsi_device *sdev)
> +{
> +	struct pmcraid_resource_entry *res = NULL;
> +
> +	res = sdev->hostdata;

No need to initialize res to NULL.

> +static u32 pmcraid_read_interrupts(struct pmcraid_instance *pinstance)
> +{
> +
> +	return ioread32(pinstance->int_regs.ioa_host_interrupt_reg);
> +}

Extra newline.

> +/**
> + * pmcraid_disable_interrupts - Masks and clears all specified interrupts
> + *
> + * @pinstance: pointer to per adapter instance structure
> + * @intr: interrupts to disable
> + *
> + * Return Value
> + *	 None
> + */

Well, that's rather obvious if it's a void function, isn't it? ;)


> +static int __devinit pmcraid_probe(
> +	struct pci_dev *pdev,
> +	const struct pci_device_id *dev_id
> +)
> +{
> +	struct pmcraid_instance *pinstance;
> +	struct Scsi_Host *host;
> +	void __iomem *mapped_pci_addr;
> +	int rc = PCIBIOS_SUCCESSFUL;
> +
> +	if (pmcraid_adapter_count >= PMCRAID_MAX_ADAPTERS) {
> +		pmcraid_err
> +			("maximum number(%d) of supported adapters reached\n",
> +			 pmcraid_adapter_count);
> +		return -ENOMEM;
> +	}
> +
> +	pmcraid_adapter_count++;

This counter may need a lock, else you might get into trouble.

> +	rc = pci_enable_device(pdev);

Well, I ask it every time, but nevertheless: why not use devres for your 
driver? It will make error handling in _probe as well as the _release stuff 
much easier.

> +	if (rc) {
> +		dev_err(&pdev->dev, "Cannot enable adapter\n");
> +		pmcraid_adapter_count--;
> +		goto out;
> +	}

Just "return rc" here. This goto into the middle of the function just for a 
return is IMHO hard to read.

> +	dev_info(&pdev->dev,
> +		"Found IOA(%x:%x) on PCI bus %d slot %d with IRQ: %d\n",
> +		 pdev->vendor, pdev->device, pdev->bus->number,
> +		 PCI_SLOT(pdev->bus->number), pdev->irq);

The IRQ and stuff is printed by request_irq() or such anyway. If you have 
enabled MSI the IRQ is a different one anyway.

> +	/* zero out entire instance structure */
> +	pinstance = (struct pmcraid_instance *)host->hostdata;
> +	memset(pinstance, 0, sizeof(struct pmcraid_instance));

memset(pinstance, 0, sizeof(*pinstance));

> +	/* Schedule worker thread to handle CCN and take care of adding and
> +	 * removing devices to OS
> +	 */
> +	schedule_work(&pinstance->worker_q);
> +
> +out:
> +	return rc;
> +
> +out_remove_host:
> +	scsi_remove_host(host);
> +
> +out_release_bufs:
> +	pmcraid_release_buffers(pinstance);
> +
> +out_unregister_isr:
> +	pmcraid_kill_tasklets(pinstance);
> +	pmcraid_unregister_interrupt_handler(pinstance);
> +
> +out_scsi_host_put:
> +	scsi_host_put(host);
> +
> +cleanup_nomem:
> +	iounmap(mapped_pci_addr);
> +
> +out_release_regions:
> +	pci_release_regions(pdev);
> +
> +out_disable_device:
> +	pmcraid_adapter_count--;
> +	pci_set_drvdata(pdev, NULL);
> +	pci_disable_device(pdev);
> +	rc = -ENODEV;
> +	goto out;
> +}

Just "return rc" here. And maybe it's a good idea to just keep the previous rc 
so you will see it was ENOMEM or whatever.

> +/* PMC PCI vendor ID and device ID values */
> +#define PCI_VENDOR_ID_PMC			0x11F8
> +#define PCI_DEVICE_ID_PMC_MAXRAID		0x5220
> +#define PCI_DEVICE_ID_PMC_0x8010		0x8010

You could just use the 0x8010 directly at the only place this constant is 
referenced.

> +#define IOARCB_LENGTH_CODE(n)         (((n)-3)/8 + (((n)-3)%8 > 0))

Oh, wow. I needed to throw this into a for loop to understand what you are 
doing here. Try DIV_ROUND_CLOSEST(n, 8) from include/kernel.h which does right 
the same.

> +/* macros to help in debugging */
> +#define pmcraid_err(...)  \
> +	printk(KERN_ERR "MaxRAID: "__VA_ARGS__)
> +
> +#define pmcraid_info(...) \
> +	if (pmcraid_debug) \
> +		printk(KERN_INFO "MaxRAID: "__VA_ARGS__)

Those should probably be rewritten to use pr_err() and pr_debug(). Or better 
use dev_dbg() and dev_info() directly.

> +/*
> + * pmcraid_adapter_id - structure defining the adapter id used by LLD
> + */
> +union pmcraid_adapter_id {
> +	struct {
> +		u32 slot_no:8;

Why not use u8?

> +		u32 bus_number:24;
> +	} y;

Greetings,

Eike

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