lkml.org 
[lkml]   [2009]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] SCSI driver for VMware's virtual HBA.
    From
    Date
    Hi Eike,

    Thanks a lot for taking a look. Please find my replies inline.

    On Thu, 2009-08-27 at 23:03 -0700, Rolf Eike Beer wrote:
    > Alok Kataria wrote:
    > > Greetings to all,
    > >
    > > Please find below a patch which adds support for the VMware
    > > paravirtualized SCSI device (PVSCSI).
    >
    > > +static const struct pci_device_id pvscsi_pci_tbl[] = {
    > > + { PCI_DEVICE(PCI_VENDOR_ID_VMWARE, PCI_DEVICE_ID_VMWARE_PVSCSI) },
    > > + { 0 }
    > > +};
    >
    > This can be written shorter as PCI_VDEVICE(VMWARE, 0x07C0). Putting the device
    > id into the header doesn't get any benefit I see, it just makes harder to
    > collect the pieces together.
    Done, I have kept the device id too, for readability.

    > It's used only here AFAICT so just write it down
    > here and be done with it. The vendor id might be better places in
    > include/linux/pci_ids.h.
    >
    Yes will move it to the header file, right now the VMware vendor id is
    also being used by the x86 paravirt-vmi code, will delay that cleanup
    till the patch is in mainline, to avoid putting dependency on multiple
    trees.

    > > +static struct pvscsi_ctx *
    > > +pvscsi_acquire_context(struct pvscsi_adapter *adapter, struct scsi_cmnd
    > > *cmd) +{
    > > + struct pvscsi_ctx *ctx;
    > > +
    > > + if (list_empty(&adapter->cmd_pool))
    > > + return NULL;
    > > +
    > > + ctx = list_entry(adapter->cmd_pool.next, struct pvscsi_ctx, list);
    >
    > list_first_entry(&adapter->cmd_pool, struct pvscsi_ctx, list);
    Done.
    >
    > > +/*
    > > + * Map a pvscsi_ctx struct to a context ID field value; we map to a simple
    > > + * non-zero integer.
    > > + */
    > > +static u64 pvscsi_map_context(const struct pvscsi_adapter *adapter,
    > > + const struct pvscsi_ctx *ctx)
    > > +{
    > > + return ctx - adapter->cmd_map + 1;
    > > +}
    >
    > Is this guaranteed to always be !=0? Maybe add a BUG_ON or WARN_ON here? And
    > if it is guaranteed please add a short comment to say /how/ this works, as
    > just from the first look this is at least suspicious.

    ctx always points to an entry in the adapter->cmd_map array. So the
    return value can never be less than 1. Will add a comment.
    >
    > > +static void pvscsi_write_cmd_desc(const struct pvscsi_adapter *adapter,
    > > + u32 cmd, const void *desc, size_t len)
    > > +{
    > > + const u32 *ptr = desc;
    > > + unsigned i;
    > > +
    > > + len /= sizeof(u32);
    >
    > How about sizeof(*ptr)? This would just remove the "magic" knowledge about the
    > size.
    >
    Yep, done.
    > > +static int pvscsi_setup_msg_workqueue(struct pvscsi_adapter *adapter)
    > > +{
    > > + char name[32];
    > > +
    > > + if (!pvscsi_use_msg)
    > > + return 0;
    > > +
    > > + pvscsi_reg_write(adapter, PVSCSI_REG_OFFSET_COMMAND,
    > > + PVSCSI_CMD_SETUP_MSG_RING);
    > > +
    > > + if (pvscsi_reg_read(adapter, PVSCSI_REG_OFFSET_COMMAND_STATUS) == -1)
    > > + return 0;
    > > +
    > > + snprintf(name, sizeof name, "pvscsi_wq_%u", adapter->host->host_no);
    >
    > sizeof(name)
    Yep, have put parenthesis for all sizeof usage's.

    >
    > > + adapter->workqueue = create_singlethread_workqueue(name);
    > > + if (!adapter->workqueue) {
    > > + printk(KERN_ERR "pvscsi: failed to create work queue\n");
    > > + return 0;
    > > + }
    > > + INIT_WORK(&adapter->work, pvscsi_msg_workqueue_handler);
    > > +
    > > + return 1;
    > > +}
    > > +
    > > +static irqreturn_t pvscsi_isr(int irq, void *devp)
    > > +{
    > > + struct pvscsi_adapter *adapter = devp;
    > > + int handled;
    > > +
    > > + if (adapter->use_msi || adapter->use_msix)
    > > + handled = true;
    > > + else {
    > > + u32 val = pvscsi_read_intr_status(adapter);
    > > + handled = (val & PVSCSI_INTR_ALL_SUPPORTED) != 0;
    > > + if (handled)
    > > + pvscsi_write_intr_status(devp, val);
    > > + }
    > > +
    > > + if (handled) {
    > > + unsigned long flags;
    > > +
    > > + spin_lock_irqsave(&adapter->hw_lock, flags);
    > > +
    > > + pvscsi_process_completion_ring(adapter);
    > > + if (adapter->use_msg && pvscsi_msg_pending(adapter))
    > > + queue_work(adapter->workqueue, &adapter->work);
    > > +
    > > + spin_unlock_irqrestore(&adapter->hw_lock, flags);
    > > + }
    > > +
    > > + return IRQ_RETVAL(handled);
    > > +}
    > > +
    > > +static void pvscsi_free_sgls(const struct pvscsi_adapter *adapter)
    > > +{
    > > + struct pvscsi_ctx *ctx = adapter->cmd_map;
    > > + unsigned i;
    > > +
    > > + for (i = 0; i < adapter->req_depth; ++i, ++ctx)
    > > + pci_free_consistent(adapter->dev, PAGE_SIZE, ctx->sgl,
    > > + ctx->sglPA);
    > > +}
    > > +
    > > +static int pvscsi_setup_msix(const struct pvscsi_adapter *adapter, int
    > > *irq) +{
    > > +#ifdef CONFIG_PCI_MSI
    > > + struct msix_entry entry = { 0, PVSCSI_VECTOR_COMPLETION };
    > > + int ret;
    > > +
    > > + ret = pci_enable_msix(adapter->dev, &entry, 1);
    > > + if (ret)
    > > + return ret;
    > > +
    > > + *irq = entry.vector;
    > > +
    > > + return 0;
    > > +#else
    > > + return -1;
    > > +#endif
    > > +}
    >
    > You don't need those #ifdef's here. If CONFIG_PCI_MSI is not defined
    > pci_enable_msix() is a static inline that always returns -1 (see
    > include/linux/pci.h).

    True, removed.
    >
    > > +static void pvscsi_shutdown_intr(struct pvscsi_adapter *adapter)
    > > +{
    > > + if (adapter->irq) {
    > > + free_irq(adapter->irq, adapter);
    > > + adapter->irq = 0;
    > > + }
    > > +#ifdef CONFIG_PCI_MSI
    > > + if (adapter->use_msi) {
    > > + pci_disable_msi(adapter->dev);
    > > + adapter->use_msi = 0;
    > > + }
    > > +
    > > + if (adapter->use_msix) {
    > > + pci_disable_msix(adapter->dev);
    > > + adapter->use_msix = 0;
    > > + }
    > > +#endif
    > > +}
    >
    > Those can go away then too, I think.
    Yep.
    >
    > > +static int __devinit pvscsi_probe(struct pci_dev *pdev,
    > > + const struct pci_device_id *id)
    > > +{
    > > + struct pvscsi_adapter *adapter;
    > > + struct Scsi_Host *host;
    > > + unsigned long base, i;
    > > + int error;
    > > +
    > > + error = -ENODEV;
    > > +
    > > + if (pci_enable_device(pdev))
    > > + return error;
    >
    > As always I suggest having a look on devres (see Documentation/driver-
    > model/devres.txt) which could simplify your error handling here and your
    > release function a lot. You only need to make sure it doesn't hurt if all the
    > PCI resources are freed after the scsi ones as you would end up cleaning the
    > scsi ones by hand and afterwards devres would throw all it handles (which will
    > probably be most of your PCI stuff) away itself.
    >
    I took a quick look, but would prefer not to change this right now. Will
    do this as a incremental change later. Hope that is fine.

    > > + if (pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) == 0 &&
    > > + pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)) == 0) {
    > > + printk(KERN_INFO "pvscsi: using 64bit dma\n");
    > > + } else if (pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) == 0 &&
    > > + pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) == 0) {
    > > + printk(KERN_INFO "pvscsi: using 32bit dma\n");
    > > + } else {
    > > + printk(KERN_ERR "pvscsi: failed to set DMA mask\n");
    > > + goto out_disable_device;
    > > + }
    > > +
    > > + pvscsi_template.can_queue =
    > > + min(PVSCSI_MAX_NUM_PAGES_REQ_RING, pvscsi_ring_pages) *
    > > + PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE;
    > > + pvscsi_template.cmd_per_lun =
    > > + min(pvscsi_template.can_queue, pvscsi_cmd_per_lun);
    > > + host = scsi_host_alloc(&pvscsi_template, sizeof(struct pvscsi_adapter));
    > > + if (!host) {
    > > + printk(KERN_ERR "pvscsi: failed to allocate host\n");
    > > + goto out_disable_device;
    > > + }
    > > +
    > > + adapter = shost_priv(host);
    > > + memset(adapter, 0, sizeof *adapter);
    >
    > sizeof(*adapter)
    >
    > > + adapter->dev = pdev;
    > > + adapter->host = host;
    > > +
    > > + spin_lock_init(&adapter->hw_lock);
    > > +
    > > + host->max_channel = 0;
    > > + host->max_id = 16;
    > > + host->max_lun = 1;
    > > +
    > > + pci_read_config_byte(pdev, PCI_CLASS_REVISION, &adapter->rev);
    >
    > That's in pdev->revision anyway, isn't it?

    Yep, though, its needed in pvscsi_info, so will keep this in adapter
    too.

    >
    > > + if (pci_request_regions(pdev, "pvscsi")) {
    > > + printk(KERN_ERR "pvscsi: pci memory selection failed\n");
    > > + goto out_free_host;
    > > + }
    > > +
    > > + base = 0;
    > > + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
    > > + if ((pci_resource_flags(pdev, i) & PCI_BASE_ADDRESS_SPACE_IO))
    > > + continue;
    > > +
    > > + if (pci_resource_len(pdev, i) <
    > > + PVSCSI_MEM_SPACE_NUM_PAGES * PAGE_SIZE)
    > > + continue;
    > > +
    > > + base = pci_resource_start(pdev, i);
    > > + break;
    > > + }
    > > +
    > > + if (base == 0) {
    > > + printk(KERN_ERR "pvscsi: adapter has no suitable MMIO region\n");
    > > + goto out_release_resources;
    > > + }
    > > +
    > > + adapter->mmioBase = ioremap(base, PVSCSI_MEM_SPACE_SIZE);
    >
    > You are mapping this here with a (probably) different size than the one you
    > checked above.
    Actually no, PVSCSI_MEM_SPACE_SIZE is defined as
    PVSCSI_MEM_SPACE_NUM_PAGES * PAGE_SIZE.

    > Also pci_iomap() could simplify that as you don't have to get
    > the base address and only need to tell it the bar number.

    Yeah done.
    >
    > > + if (!adapter->mmioBase) {
    > > + printk(KERN_ERR "pvscsi: can't ioremap 0x%lx\n", base);
    > > + goto out_release_resources;
    > > + }
    > > +
    > > + pci_set_master(pdev);
    > > + pci_set_drvdata(pdev, host);
    > > +
    > > + ll_adapter_reset(adapter);
    > > +
    > > + adapter->use_msg = pvscsi_setup_msg_workqueue(adapter);
    > > +
    > > + error = pvscsi_allocate_rings(adapter);
    > > + if (error) {
    > > + printk(KERN_ERR "pvscsi: unable to allocate ring memory\n");
    > > + goto out_release_resources;
    > > + }
    > > +
    > > + /*
    > > + * From this point on we should reset the adapter if anything goes
    > > + * wrong.
    > > + */
    > > + pvscsi_setup_all_rings(adapter);
    > > +
    > > + adapter->cmd_map = kmalloc(adapter->req_depth *
    > > + sizeof(struct pvscsi_ctx), GFP_KERNEL);
    >
    > kcalloc(), this will do overflow checking and also clear the memory for you.
    Done.
    >
    >
    > > + printk(KERN_INFO "VMware PVSCSI rev %d on bus:%u slot:%u func:%u host
    > > #%u\n", + adapter->rev, pdev->bus->number, PCI_SLOT(pdev->devfn),
    > > + PCI_FUNC(pdev->devfn), host->host_no);
    >
    > dev_info(&pdev->dev, ..), this should also give you the PCI
    > domain/bus/slot/function information for free.
    >
    Ok.
    > > +static int __init pvscsi_init(void)
    > > +{
    > > + printk(KERN_DEBUG "%s - version %s\n",
    > > + PVSCSI_LINUX_DRIVER_DESC, PVSCSI_DRIVER_VERSION_STRING);
    >
    > pr_debug()
    Ok.

    Will send the V2 patch with all changes.
    Thanks again for your comments.

    Alok
    >
    > HTH & HAND
    >
    > Eike



    \
     
     \ /
      Last update: 2009-08-31 19:29    [W:0.046 / U:59.512 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site