Re: [PATCH] hpsa: SCSI driver for HP Smart Array controllers

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

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

You wrote:

> +static int adjust_hpsa_scsi_table(struct ctlr_info *h, int hostno,
> +	struct hpsa_scsi_dev_t sd[], int nsds)
> +{
> +	/* sd contains scsi3 addresses and devtypes, and inquiry */
> +	/* data.  This function takes what's in sd to be the current */
> +	/* reality and updates h->dev[] to reflect that reality. */
> +
> +	int i, entry, device_change, changes = 0;
> +	struct hpsa_scsi_dev_t *csd;
> +	unsigned long flags;
> +	struct scsi2map *added, *removed;
> +	int nadded, nremoved;
> +	struct Scsi_Host *sh = NULL;
> +
> +	added = kzalloc(sizeof(*added) * HPSA_MAX_SCSI_DEVS_PER_HBA,
> +		GFP_KERNEL);
> +	removed = kzalloc(sizeof(*removed) * HPSA_MAX_SCSI_DEVS_PER_HBA,
> +		GFP_KERNEL);

kcalloc()?

> +static struct CommandList_struct *cmd_alloc(struct ctlr_info *h)
> +{
> +	struct CommandList_struct *c;
> +	int i;
> +	union u64bit temp64;
> +	dma_addr_t cmd_dma_handle, err_dma_handle;
> +
> +	do {
> +		i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> +		if (i == h->nr_cmds)
> +			return NULL;
> +	} while (test_and_set_bit
> +		 (i & (BITS_PER_LONG - 1),
> +		  h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
> +	c = h->cmd_pool + i;
> +	memset(c, 0, sizeof(struct CommandList_struct));

sizeof(*c)

> +	cmd_dma_handle = h->cmd_pool_dhandle
> +	    + i * sizeof(struct CommandList_struct);
> +	c->err_info = h->errinfo_pool + i;
> +	memset(c->err_info, 0, sizeof(struct ErrorInfo_struct));

sizeof(c->err_info)

> +	err_dma_handle = h->errinfo_pool_dhandle
> +	    + i * sizeof(struct ErrorInfo_struct);
> +	h->nr_allocs++;
> +
> +	c->cmdindex = i;
> +
> +	INIT_HLIST_NODE(&c->list);
> +	c->busaddr = (__u32) cmd_dma_handle;
> +	temp64.val = (__u64) err_dma_handle;
> +	c->ErrDesc.Addr.lower = temp64.val32.lower;
> +	c->ErrDesc.Addr.upper = temp64.val32.upper;
> +	c->ErrDesc.Len = sizeof(struct ErrorInfo_struct);
> +
> +	c->ctlr = h->ctlr;
> +	return c;
> +}
> +
> +/* For operations that can wait for kmalloc to possibly sleep,
> + * this routine can be called. Lock need not be held to call
> + * cmd_special_alloc. cmd_special_free() is the complement.
> + */
> +static struct CommandList_struct *cmd_special_alloc(struct ctlr_info *h)
> +{
> +	struct CommandList_struct *c;
> +	union u64bit temp64;
> +	dma_addr_t cmd_dma_handle, err_dma_handle;
> +
> +	c = (struct CommandList_struct *) pci_alloc_consistent(h->pdev,
> +		sizeof(struct CommandList_struct), &cmd_dma_handle);

No need to cast a void pointer.

> +	if (c == NULL)
> +		return NULL;
> +	memset(c, 0, sizeof(struct CommandList_struct));

sizeof(*c)

> +	c->cmdindex = -1;
> +
> +	c->err_info = (struct ErrorInfo_struct *)
> +	    pci_alloc_consistent(h->pdev, sizeof(struct ErrorInfo_struct),
> +		    &err_dma_handle);
> +
> +	if (c->err_info == NULL) {
> +		pci_free_consistent(h->pdev,
> +			sizeof(struct CommandList_struct), c, cmd_dma_handle);
> +		return NULL;
> +	}
> +	memset(c->err_info, 0, sizeof(struct ErrorInfo_struct));

Here again.

> +	INIT_HLIST_NODE(&c->list);
> +	c->busaddr = (__u32) cmd_dma_handle;
> +	temp64.val = (__u64) err_dma_handle;
> +	c->ErrDesc.Addr.lower = temp64.val32.lower;
> +	c->ErrDesc.Addr.upper = temp64.val32.upper;
> +	c->ErrDesc.Len = sizeof(struct ErrorInfo_struct);
> +
> +	c->ctlr = h->ctlr;
> +	return c;
> +}
> +
> +
> +/* Free a command block previously allocated with cmd_alloc(). */
> +static void cmd_free(struct ctlr_info *h, struct CommandList_struct *c)
> +{
> +	int i;
> +	i = c - h->cmd_pool;
> +	clear_bit(i & (BITS_PER_LONG - 1),
> +		  h->cmd_pool_bits + (i / BITS_PER_LONG));
> +	h->nr_frees++;
> +}
> +
> +/* Free a command block previously allocated with cmd_special_alloc(). */
> +static void cmd_special_free(struct ctlr_info *h, struct
> CommandList_struct *c) +{
> +	union u64bit temp64;
> +
> +	temp64.val32.lower = c->ErrDesc.Addr.lower;
> +	temp64.val32.upper = c->ErrDesc.Addr.upper;
> +	pci_free_consistent(h->pdev, sizeof(struct ErrorInfo_struct),
> +			    c->err_info, (dma_addr_t) temp64.val);

sizeof(c->err_info)

> +	pci_free_consistent(h->pdev, sizeof(struct CommandList_struct),
> +			    c, (dma_addr_t) c->busaddr);

sizeof(*c)

I stopped looking for that here, there are maybe some more instances.

> +static int hpsa_pci_init(struct ctlr_info *c, struct pci_dev *pdev)
> +{
> +	ushort subsystem_vendor_id, subsystem_device_id, command;
> +	__u32 board_id, scratchpad = 0;
> +	__u64 cfg_offset;
> +	__u32 cfg_base_addr;
> +	__u64 cfg_base_addr_index;
> +	int i, prod_index, err;
> +
> +	subsystem_vendor_id = pdev->subsystem_vendor;
> +	subsystem_device_id = pdev->subsystem_device;
> +	board_id = (((__u32) (subsystem_device_id << 16) & 0xffff0000) |
> +		    subsystem_vendor_id);
> +
> +	for (i = 0; i < ARRAY_SIZE(products); i++)
> +		if (board_id == products[i].board_id)
> +			break;
> +
> +	prod_index = i;
> +
> +	if (prod_index == ARRAY_SIZE(products)) {
> +		prod_index--;
> +		if (subsystem_vendor_id == !PCI_VENDOR_ID_HP ||
> +				!allow_unknown_smartarray) {
> +			printk(KERN_WARNING "hpsa: Sorry, I don't "
> +				"know how to access the Smart "
> +				"Array controller %08lx\n",
> +				(unsigned long) board_id);
> +			return -ENODEV;
> +		}
> +	}
> +	/* check to see if controller has been disabled */
> +	/* BEFORE trying to enable it */
> +	(void)pci_read_config_word(pdev, PCI_COMMAND, &command);
> +	if (!(command & 0x02)) {
> +		printk(KERN_WARNING
> +		       "hpsa: controller appears to be disabled\n");
> +		return -ENODEV;
> +	}
> +
> +	err = pci_enable_device(pdev);

You may want to use pcim_enable_device() as it moves the work of freeing a 
bunch of resources (like pci_request_regions()) to devres and you don't need 
to care about this.

Eike

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