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

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

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

Stephen M. Cameron wrote:

> +static const char *raid_label[] = { "0", "4", "1(1+0)", "5", "5+1", "ADG",
> +	"UNKNOWN"
> +};
> +#define RAID_UNKNOWN (sizeof(raid_label) / sizeof(raid_label[0]) - 1)

(ARRAY_SIZE(raid_label) - 1)

> +
> +static ssize_t raid_level_show(struct device *dev,
> +	     struct device_attribute *attr, char *buf)
> +{
> +	ssize_t l = 0;
> +	int rlevel;
> +	struct ctlr_info *h;
> +	struct scsi_device *sdev;
> +	struct hpsa_scsi_dev_t *hdev;
> +	unsigned long flags;
> +
> +	sdev = to_scsi_device(dev);
> +	h = (struct ctlr_info *) sdev->host->hostdata[0];
> +	spin_lock_irqsave(&h->lock, flags);
> +	hdev = sdev->hostdata;
> +	if (!hdev) {
> +		spin_unlock_irqrestore(&h->lock, flags);
> +		return -ENODEV;
> +	}
> +
> +	/* Is this even a logical drive? */
> +	if (!is_logical_dev_addr_mode(hdev->scsi3addr)) {
> +		spin_unlock_irqrestore(&h->lock, flags);
> +		l = snprintf(buf, PAGE_SIZE, "N/A\n");
> +		return l;
> +	}
> +
> +	rlevel = hdev->raid_level;
> +	spin_unlock_irqrestore(&h->lock, flags);
> +	if (rlevel < 0 || rlevel > RAID_UNKNOWN)
> +		rlevel = RAID_UNKNOWN;

How about making raid_level an unsigned int, the check for less than zero 
could go away then.

> +/* Remove an entry from h->dev[] array. */
> +static void hpsa_scsi_remove_entry(struct ctlr_info *h, int hostno, int
>  entry, +	struct hpsa_scsi_dev_t *removed[], int *nremoved)
> +{
> +	/* assumes h->devlock is held */
> +	int i;
> +	struct hpsa_scsi_dev_t *sd;
> +
> +	if (entry < 0 || entry >= HPSA_MAX_SCSI_DEVS_PER_HBA)
> +		BUG();

BUG_ON(entry < 0 || entry >= HPSA_MAX_SCSI_DEVS_PER_HBA);

> +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 hpsa_scsi_dev_t **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()

> +	if (!added || !removed) {
> +		dev_warn(&h->pdev->dev, "out of memory in "
> +			"adjust_hpsa_scsi_table\n");
> +		goto free_and_out;
> +	}

You should better return ENOMEM instead of 0 now. On the other hand the only 
caller of this function does not check the return value anyway so this 
function could also be void.

> +static void hpsa_slave_destroy(struct scsi_device *sdev)
> +{
> +	return; /* nothing to do. */
> +}
> +
> +static void hpsa_scsi_setup(struct ctlr_info *h)
> +{
> +	h->ndevices = 0;
> +	h->scsi_host = NULL;
> +	spin_lock_init(&h->devlock);
> +	return;
> +}

Those return statements are completely superfluous.

> +static int hpsa_scsi_detect(struct ctlr_info *h)
> +{
> +	struct Scsi_Host *sh;
> +	int error;
> +
> +	sh = scsi_host_alloc(&hpsa_driver_template, sizeof(*h));
> +	if (sh == NULL)
> +		goto fail;
> +
> +	sh->io_port = 0;
> +	sh->n_io_port = 0;
> +	sh->this_id = -1;
> +	sh->max_channel = 3;
> +	sh->max_cmd_len = MAX_COMMAND_SIZE;
> +	sh->max_lun = HPSA_MAX_LUN;
> +	sh->max_id = HPSA_MAX_LUN;
> +	h->scsi_host = sh;
> +	sh->hostdata[0] = (unsigned long) h;
> +	sh->irq = h->intr[SIMPLE_MODE_INT];
> +	sh->unique_id = sh->irq;
> +	error = scsi_add_host(sh, &h->pdev->dev);
> +	if (error)
> +		goto fail_host_put;
> +	scsi_scan_host(sh);
> +	return 0;
> +
> + fail_host_put:
> +	dev_err(&h->pdev->dev, "hpsa_scsi_detect: scsi_add_host"
> +		" failed for controller %d\n", h->ctlr);
> +	scsi_host_put(sh);
> +	return -1;
> + fail:
> +	dev_err(&h->pdev->dev, "hpsa_scsi_detect: scsi_host_alloc"
> +		" failed for controller %d\n", h->ctlr);
> +	return -1;
> +}

You should forward the error code of scsi_add_host() here and return ENOMEM if 
scsi_host_alloc() failed, that would make debugging probably easier.

> +static void hpsa_unmap_one(struct pci_dev *pdev,
> +		struct CommandList *cp,
> +		size_t buflen,
> +		int data_direction)
> +{
> +	union u64bit addr64;
> +
> +	addr64.val32.lower = cp->SG[0].Addr.lower;
> +	addr64.val32.upper = cp->SG[0].Addr.upper;
> +	pci_unmap_single(pdev, (dma_addr_t) addr64.val,
> +		buflen, data_direction);
> +}
> +
> +static void hpsa_map_one(struct pci_dev *pdev,
> +		struct CommandList *cp,
> +		unsigned char *buf,
> +		size_t buflen,
> +		int data_direction)
> +{
> +	__u64 addr64;
> +
> +	if (buflen == 0 || data_direction == PCI_DMA_NONE) {
> +		cp->Header.SGList = 0;
> +		cp->Header.SGTotal = 0;
> +		return;
> +	}
> +
> +	addr64 = (__u64) pci_map_single(pdev, buf, buflen, data_direction);
> +	cp->SG[0].Addr.lower =
> +	  (__u32) (addr64 & (__u64) 0x00000000FFFFFFFF);
> +	cp->SG[0].Addr.upper =
> +	  (__u32) ((addr64 >> 32) & (__u64) 0x00000000FFFFFFFF);

You want to use upper_32_bits() and lower_32_bits() from linux/kernel.h, I'm 
sure. ;)

> +static int hpsa_scsi_do_inquiry(struct ctlr_info *h, unsigned char
>  *scsi3addr, +			unsigned char page, unsigned char *buf,
> +			unsigned char bufsize)
> +{
> +	int rc;
> +	struct CommandList *c;
> +	struct ErrorInfo *ei;
> +
> +	c = cmd_special_alloc(h);
> +
> +	if (c == NULL) {			/* trouble... */
> +		dev_warn(&h->pdev->dev, "cmd_special_alloc returned NULL!\n");
> +		return -1;
> +	}

-ENOMEM

> +	rc = fill_cmd(c, HPSA_INQUIRY, h, buf, bufsize, page, scsi3addr,
> +		TYPE_CMD);
> +	if (rc == 0) {
> +		hpsa_scsi_do_simple_cmd_core(h, c);
> +		hpsa_unmap_one(h->pdev, c, bufsize, PCI_DMA_FROMDEVICE);
> +
> +		ei = c->err_info;
> +		if (ei->CommandStatus != 0 &&
> +		    ei->CommandStatus != CMD_DATA_UNDERRUN) {
> +			hpsa_scsi_interpret_error(c);
> +			rc = -1;

-EINVAL or whatever

> +		}
> +	}
> +	cmd_special_free(h, c);
> +	return rc;
> +}
> +
> +static int hpsa_send_reset(struct ctlr_info *h, unsigned char *scsi3addr)
> +{
> +	int rc;
> +	struct CommandList *c;
> +	struct ErrorInfo *ei;
> +
> +	c = cmd_special_alloc(h);
> +
> +	if (c == NULL) {			/* trouble... */
> +		dev_warn(&h->pdev->dev, "cmd_special_alloc returned NULL!\n");
> +		return -1;
> +	}

-ENOMEM

> +	rc = fill_cmd(c, HPSA_DEVICE_RESET_MSG, h, NULL, 0, 0, scsi3addr,
> +		TYPE_MSG);
> +	if (rc != 0)
> +		goto out;
> +
> +	hpsa_scsi_do_simple_cmd_core(h, c);
> +	/* no unmap needed here because no data xfer. */
> +
> +	ei = c->err_info;
> +	if (ei->CommandStatus != 0) {
> +		hpsa_scsi_interpret_error(c);
> +		rc = -1;
> +	}

-EINVAL

More of those follows.


> +static int hpsa_scsi_do_report_luns(struct ctlr_info *h, int logical,
> +		struct ReportLUNdata *buf, int bufsize,
> +		int extended_response)
> +{
> +	int rc;
> +	struct CommandList *c;
> +	unsigned char scsi3addr[8];
> +	struct ErrorInfo *ei;
> +
> +	c = cmd_special_alloc(h);
> +	if (c == NULL) {			/* trouble... */
> +		dev_err(&h->pdev->dev, "cmd_special_alloc returned NULL!\n");
> +		return -1;
> +	}
> +
> +	memset(&scsi3addr[0], 0, 8); /* address the controller */

memset(scsi3addr, 0, sizeof(scsi3addr));

> +static inline int hpsa_scsi_do_report_phys_luns(struct ctlr_info *h,
> +		struct ReportLUNdata *buf,
> +		int bufsize, int extended_response)
> +{
> +	return hpsa_scsi_do_report_luns(h, 0, buf, bufsize, extended_response);
> +}
> +
> +static inline int hpsa_scsi_do_report_log_luns(struct ctlr_info *h,
> +		struct ReportLUNdata *buf, int bufsize)
> +{
> +	return hpsa_scsi_do_report_luns(h, 1, buf, bufsize, 0);
> +}

Given that these two functions are both absolutely trivial and only called 
from one place each they should be open coded there.

> +static int hpsa_update_device_info(struct ctlr_info *h,
> +	unsigned char scsi3addr[], struct hpsa_scsi_dev_t *this_device)
> +{
> +#define OBDR_TAPE_INQ_SIZE 49
> +	unsigned char *inq_buff = NULL;

No need to initialize as you overwrite that anyway.

> +	inq_buff = kmalloc(OBDR_TAPE_INQ_SIZE, GFP_KERNEL);
> +	if (!inq_buff)
> +		goto bail_out;
> +
> +	memset(inq_buff, 0, OBDR_TAPE_INQ_SIZE);

kzalloc()

> +	/* Do an inquiry to the device to see what it is. */
> +	if (hpsa_scsi_do_inquiry(h, scsi3addr, 0, inq_buff,
> +		(unsigned char) OBDR_TAPE_INQ_SIZE) != 0) {
> +		/* Inquiry failed (msg printed already) */
> +		dev_err(&h->pdev->dev,
> +			"hpsa_update_device_info: inquiry failed\n");
> +		goto bail_out;
> +	}
> +
> +	/* As a side effect, record the firmware version number
> +	 * if we happen to be talking to the RAID controller.
> +	 */
> +	if (is_hba_lunid(scsi3addr))
> +		memcpy(h->firm_ver, &inq_buff[32], 4);

I've not looked into the details but you might need le32_to_cpu() or something 
here.

> +static int is_msa2xxx(struct ctlr_info *h, struct hpsa_scsi_dev_t *device)
> +{
> +	int i;
> +
> +	for (i = 0; msa2xxx_model[i]; i++)
> +		if (strncmp(device->model, msa2xxx_model[i],
> +			strlen(msa2xxx_model[i])) == 0)
> +			return 1;
> +	return 0;
> +}

This could probably be bool instead of int.

> +static void figure_bus_target_lun(struct ctlr_info *h,
> +	__u8 *lunaddrbytes, int *bus, int *target, int *lun,
> +	struct hpsa_scsi_dev_t *device)
> +{
> +

Extra newline.

> +	__u32 lunid;
> +
> +	if (is_logical_dev_addr_mode(lunaddrbytes)) {
> +		/* logical device */
> +		memcpy(&lunid, lunaddrbytes, sizeof(lunid));
> +		lunid = le32_to_cpu(lunid);

Why not "lunid = le32_to_cpu(*((__le32 *)lunaddrbytes))"

> +static int hpsa_gather_lun_info(struct ctlr_info *h,
> +	int reportlunsize,
> +	struct ReportLUNdata *physdev, __u32 *nphysicals,
> +	struct ReportLUNdata *logdev, __u32 *nlogicals)
> +{
> +	if (hpsa_scsi_do_report_phys_luns(h, physdev, reportlunsize, 0)) {
> +		dev_err(&h->pdev->dev, "report physical LUNs failed.\n");
> +		return -1;
> +	}
> +	memcpy(nphysicals, &physdev->LUNListLength[0], sizeof(*nphysicals));
> +	*nphysicals = be32_to_cpu(*nphysicals) / 8;

*nphysicals = be32_to_cpu(*((__be32 *)physdev->LUNListLength)) / 8;

There are also some more of these will I will not mark all by themself.

> +#ifdef DEBUG
> +	dev_info(&h->pdev->dev, "number of physical luns is %d\n", *nphysicals);
> +#endif

dev_dbg(...), some more times, too.

> +static void hpsa_update_scsi_devices(struct ctlr_info *h, int hostno)
> +{
[...]
> +out:
> +	kfree(tmpdevice);
> +	for (i = 0; i < ndev_allocated; i++)
> +		kfree(currentsd[i]);
> +	kfree(currentsd);
> +	kfree(inq_buff);
> +	kfree(physdev_list);
> +	kfree(logdev_list);
> +	return;
> +}

Superfluous return.

> +/* hpsa_scatter_gather takes a struct scsi_cmnd, (cmd), and does the pci
> + * dma mapping  and fills in the scatter gather entries of the
> + * hpsa command, cp.
> + */
> +static int hpsa_scatter_gather(struct pci_dev *pdev,
> +		struct CommandList *cp,
> +		struct scsi_cmnd *cmd)
> +{
> +	unsigned int len;
> +	struct scatterlist *sg;
> +	__u64 addr64;
> +	int use_sg, i;
> +
> +	BUG_ON(scsi_sg_count(cmd) > MAXSGENTRIES);
> +
> +	use_sg = scsi_dma_map(cmd);
> +	if (use_sg < 0)
> +		return use_sg;
> +
> +	if (!use_sg)
> +		goto sglist_finished;
> +
> +	scsi_for_each_sg(cmd, sg, use_sg, i) {
> +		addr64 = (__u64) sg_dma_address(sg);
> +		len  = sg_dma_len(sg);
> +		cp->SG[i].Addr.lower =
> +			(__u32) (addr64 & (__u64) 0x00000000FFFFFFFF);
> +		cp->SG[i].Addr.upper =
> +			(__u32) ((addr64 >> 32) & (__u64) 0x00000000FFFFFFFF);

upper_32_bits/lower_32_bits

> +static int wait_for_device_to_become_ready(struct ctlr_info *h,
> +	unsigned char lunaddr[])
> +{
> +	int rc;
> +	int count = 0;
> +	int waittime = HZ;
> +	struct CommandList *c;
> +
> +	c = cmd_special_alloc(h);
> +	if (!c) {
> +		dev_warn(&h->pdev->dev, "out of memory in "
> +			"wait_for_device_to_become_ready.\n");
> +		return IO_ERROR;
> +	}
> +
> +	/* Send test unit ready until device ready, or give up. */
> +	while (count < HPSA_TUR_RETRY_LIMIT) {
> +
> +		/* Wait for a bit.  do this first, because if we send
> +		 * the TUR right away, the reset will just abort it.
> +		 */
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +		schedule_timeout(waittime);
> +		count++;
> +
> +		/* Increase wait time with each try, up to a point. */
> +		if (waittime < (HZ * HPSA_MAX_WAIT_INTERVAL_SECS))
> +			waittime = waittime * 2;
> +
> +		/* Send the Test Unit Ready */
> +		rc = fill_cmd(c, TEST_UNIT_READY, h, NULL, 0, 0,
> +			lunaddr, TYPE_CMD);
> +		if (rc != 0) {
> +			/* We don't expect to get in here */
> +			dev_warn(&h->pdev->dev, "fill_cmd failed at %s:%d\n",
> +				__FILE__, __LINE__);
> +			break;
> +		}

Maybe __func__ is more informative.

> +#ifdef CONFIG_COMPAT
> +
> +static int do_ioctl(struct scsi_device *dev, int cmd, void *arg)
> +{
> +	int ret;
> +
> +	lock_kernel();
> +	ret = hpsa_ioctl(dev, cmd, arg);
> +	unlock_kernel();
> +	return ret;
> +}

Are you sure you really need the BKL? I don't think new code that relies on 
that is a good idea anyway.


> +static int hpsa_ioctl32_passthru(struct scsi_device *dev, int cmd, void
>  *arg) +{
> +	IOCTL32_Command_struct __user *arg32 =
> +	    (IOCTL32_Command_struct __user *) arg;
> +	IOCTL_Command_struct arg64;
> +	IOCTL_Command_struct __user *p = compat_alloc_user_space(sizeof(arg64));
> +	int err;
> +	u32 cp;
> +
> +	err = 0;
> +	err |= copy_from_user(&arg64.LUN_info, &arg32->LUN_info,
> +			   sizeof(arg64.LUN_info));
> +	err |= copy_from_user(&arg64.Request, &arg32->Request,
> +			   sizeof(arg64.Request));
> +	err |= copy_from_user(&arg64.error_info, &arg32->error_info,
> +			   sizeof(arg64.error_info));
> +	err |= get_user(arg64.buf_size, &arg32->buf_size);
> +	err |= get_user(cp, &arg32->buf);
> +	arg64.buf = compat_ptr(cp);
> +	err |= copy_to_user(p, &arg64, sizeof(arg64));
> +
> +	if (err)
> +		return -EFAULT;
> +
> +	err = do_ioctl(dev, CCISS_PASSTHRU, (void *)p);
> +	if (err)
> +		return err;
> +	err |= copy_in_user(&arg32->error_info, &p->error_info,
> +			 sizeof(arg32->error_info));
> +	if (err)
> +		return -EFAULT;
> +	return err;
> +}

return 0;

> +static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp)
> +{
> +	BIG_IOCTL_Command_struct *ioc;
> +	struct CommandList *c;
> +	unsigned char **buff = NULL;
> +	int *buff_size = NULL;
> +	union u64bit temp64;
> +	BYTE sg_used = 0;
> +	int status = 0;
> +	int i;
> +	DECLARE_COMPLETION_ONSTACK(wait);
> +	__u32 left;
> +	__u32 sz;
> +	BYTE __user *data_ptr;
> +
> +	if (!argp)
> +		return -EINVAL;
> +	if (!capable(CAP_SYS_RAWIO))
> +		return -EPERM;
> +	ioc = (BIG_IOCTL_Command_struct *)
> +	    kmalloc(sizeof(*ioc), GFP_KERNEL);
> +	if (!ioc) {
> +		status = -ENOMEM;
> +		goto cleanup1;
> +	}
> +	if (copy_from_user(ioc, argp, sizeof(*ioc))) {
> +		status = -EFAULT;
> +		goto cleanup1;
> +	}
> +	if ((ioc->buf_size < 1) &&
> +	    (ioc->Request.Type.Direction != XFER_NONE)) {
> +		status = -EINVAL;
> +		goto cleanup1;
> +	}
> +	/* Check kmalloc limits  using all SGs */
> +	if (ioc->malloc_size > MAX_KMALLOC_SIZE) {
> +		status = -EINVAL;
> +		goto cleanup1;
> +	}
> +	if (ioc->buf_size > ioc->malloc_size * MAXSGENTRIES) {
> +		status = -EINVAL;
> +		goto cleanup1;
> +	}
> +	buff = kzalloc(MAXSGENTRIES * sizeof(char *), GFP_KERNEL);

kcalloc()?


> +/*
> + * Map (physical) PCI mem into (virtual) kernel space
> + */
> +static void __iomem *remap_pci_mem(ulong base, ulong size)
> +{
> +	ulong page_base = ((ulong) base) & PAGE_MASK;
> +	ulong page_offs = ((ulong) base) - page_base;
> +	void __iomem *page_remapped = ioremap(page_base, page_offs + size);
> +
> +	return page_remapped ? (page_remapped + page_offs) : NULL;
> +}

You should simply use pci_iomap() and remove this one entirely.


> +static int hpsa_pci_init(struct ctlr_info *h, 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 ||
> +				!hpsa_allow_any) {
> +			dev_warn(&pdev->dev, "unrecognized board ID:"
> +				" 0x%08lx, ignoring.\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)) {
> +		dev_warn(&pdev->dev, "controller appears to be disabled\n");
> +		return -ENODEV;
> +	}
> +
> +	err = pci_enable_device(pdev);
> +	if (err) {
> +		dev_warn(&pdev->dev, "unable to enable PCI device\n");
> +		return err;
> +	}

I usually suggest at this place to take a look at devres 
(Documentation/driver-model/devres.txt) as that would make some resource 
tracking much easier.

> +err_out_free_res:
> +	/*
> +	 * Deliberately omit pci_disable_device(): it does something nasty to
> +	 * Smart Array controllers that pci_enable_device does not undo
> +	 */
> +	pci_release_regions(pdev);
> +	return err;
> +}

That sounds interesting. Maybe the PCI folks would like to hear about that.

> +static unsigned long SA5_completed(struct ctlr_info *h)
> +{
> +	unsigned long register_value
> +		= readl(h->vaddr + SA5_REPLY_PORT_OFFSET);
> +
> +	if (register_value != FIFO_EMPTY)
> +		h->commands_outstanding--;
> +
> +#ifdef HPSA_DEBUG
> +	if (register_value != FIFO_EMPTY)
> +		printk(KERN_INFO "hpsa:  Read %lx back from board\n",
> +			register_value);
> +	else
> +		printk(KERN_INFO "hpsa:  FIFO Empty read\n");
> +#endif
> +
> +	return register_value;
> +}

I don't think all those stuff should be implemented in a header file.

> +struct vals32 {
> +	__u32   lower;
> +	__u32   upper;
> +};
> +
> +union u64bit {
> +	struct vals32 val32;
> +	__u64 val;
> +};

There are helpers to access both 32 bit halves of a 64 bit value and other 
nice things. Do you really need those struct things?


> +/* The size of this structure needs to be divisible by 8
> + * od on all architectures, because the controller uses 2
      ^^
??

Greetings,

Eike

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