| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 2 Oct 2009 22:23:22 +0200
From: Rolf Eike Beer <eike-kernel@...tec.de>
To: "Stephen M. Cameron" <scameron@...rdog.cce.hp.com>
Cc: akpm@...ux-foundation.org, James.Bottomley@...senpartnership.com,
axboe@...nel.dk, linux-kernel@...r.kernel.org,
linux-scsi@...r.kernel.org, mikem@...rdog.cce.hp.com
Subject: Re: [PATCH] Add hpsa driver for HP Smart Array controllers.
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
Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)
Powered by blists - more mailing lists