I wonder how reliable these numbers are. We did similar testing a while back
because we thought kmalloc-96 caches had weird cache behavior but finally
figured out the anomaly was explained by the order of the tests run, not cache
size.

mostly just nits

* Alok Kataria (akataria@vmware.com) wrote:
+#include +#include +#include
shouldn't be needed

+#include
shouldn't be needed

+#include +#include +#include +
+#include +#include +#include +#include +
+#include "vmw_pvscsi.h"
+
+#define PVSCSI_LINUX_DRIVER_DESC "VMware PVSCSI driver"
+
+MODULE_DESCRIPTION(PVSCSI_LINUX_DRIVER_DESC);
+MODULE_AUTHOR("VMware, Inc.");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(PVSCSI_DRIVER_VERSION_STRING);
+
+#define PVSCSI_DEFAULT_NUM_PAGES_PER_RING 8
+#define PVSCSI_DEFAULT_NUM_PAGES_MSG_RING 1
+#define PVSCSI_DEFAULT_QUEUE_DEPTH 64
+#define SGL_SIZE PAGE_SIZE
+
+#define pvscsi_dev(adapter) (&(adapter->dev->dev))

easy to make it static inline and get some type checking for free

+
+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_first_entry(&adapter->cmd_pool, struct pvscsi_ctx, list);
+ ctx->cmd = cmd;
+ list_del(&ctx->list);
+
+ return ctx;
+}
+
+static void pvscsi_release_context(struct pvscsi_adapter *adapter,
+ struct pvscsi_ctx *ctx)
+{
+ ctx->cmd = NULL;
+ list_add(&ctx->list, &adapter->cmd_pool);
+}

These list manipulations are protected by hw_lock? Looks like all cases
are covered.

snip
+/*
+ * Allocate scatter gather lists.
+ *
+ * These are statically allocated. Trying to be clever was not worth it.
+ *
+ * Dynamic allocation can fail, and we can't go deeep into the memory
+ * allocator, since we're a SCSI driver, and trying too hard to allocate
+ * memory might generate disk I/O. We also don't want to fail disk I/O
+ * in that case because we can't get an allocation - the I/O could be
+ * trying to swap out data to free memory. Since that is pathological,
+ * just use a statically allocated scatter list.
+ *
+ */
+static int __devinit pvscsi_allocate_sg(struct pvscsi_adapter *adapter)
+{
+ struct pvscsi_ctx *ctx;
+ int i;
+
+ ctx = adapter->cmd_map;
+ BUILD_BUG_ON(sizeof(struct pvscsi_sg_list) > SGL_SIZE);
+
+ for (i = 0; i < adapter->req_depth; ++i, ++ctx) {
+ ctx->sgl = kmalloc(SGL_SIZE, GFP_KERNEL);
+ ctx->sglPA = 0;
+ BUG_ON(!IS_ALIGNED(((unsigned long)ctx->sgl), PAGE_SIZE));

Why not simply allocate a page? Seems different allocator or debugging
options could trigger this.

+
+static int __devinit pvscsi_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ struct pvscsi_adapter *adapter;
+ struct Scsi_Host *host;
+ unsigned int i;
+ int error;
+
+ error = -ENODEV;
+
+ if (pci_enable_device(pdev))
+ return error;

looks mmio only, pci_enable_device_mem()

+
+ 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 "vmw_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 "vmw_pvscsi: using 32bit dma\n");
+ } else {
+ printk(KERN_ERR "vmw_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);

When/how are these tunables used? Are they still useful?

+ host = scsi_host_alloc(&pvscsi_template, sizeof(struct pvscsi_adapter));
+ if (!host) {
+ printk(KERN_ERR "vmw_pvscsi: failed to allocate host\n");
+ goto out_disable_device;
+ }
+
+ adapter = shost_priv(host);
+ memset(adapter, 0, 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;
+ host->max_cmd_len = 16;
+
+ adapter->rev = pdev->revision;
+
+ if (pci_request_regions(pdev, "vmw_pvscsi")) {
+ printk(KERN_ERR "vmw_pvscsi: pci memory selection failed\n");
+ goto out_free_host;
+ }
+
+ 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_SIZE)
+ continue;
+
+ break;
+ }
+
+ if (i == DEVICE_COUNT_RESOURCE) {
+ printk(KERN_ERR
+ "vmw_pvscsi: adapter has no suitable MMIO region\n");
+ goto out_release_resources;
+ }

Could simplify and just do pci_request_selected_regions.

+ adapter->mmioBase = pci_iomap(pdev, i, PVSCSI_MEM_SPACE_SIZE);
+
+ if (!adapter->mmioBase) {
+ printk(KERN_ERR
+ "vmw_pvscsi: can't iomap for BAR %d memsize %lu\n",
+ i, PVSCSI_MEM_SPACE_SIZE);
+ 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 "vmw_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 = kcalloc(adapter->req_depth,
+ sizeof(struct pvscsi_ctx), GFP_KERNEL);
+ if (!adapter->cmd_map) {
+ printk(KERN_ERR "vmw_pvscsi: failed to allocate memory.\n");
+ error = -ENOMEM;
+ goto out_reset_adapter;
+ }
+
+ INIT_LIST_HEAD(&adapter->cmd_pool);
+ for (i = 0; i < adapter->req_depth; i++) {
+ struct pvscsi_ctx *ctx = adapter->cmd_map + i;
+ list_add(&ctx->list, &adapter->cmd_pool);
+ }
+
+ error = pvscsi_allocate_sg(adapter);
+ if (error) {
+ printk(KERN_ERR "vmw_pvscsi: unable to allocate s/g table\n");
+ goto out_reset_adapter;
+ }
+
+ if (!pvscsi_disable_msix &&
+ pvscsi_setup_msix(adapter, &adapter->irq) == 0) {
+ printk(KERN_INFO "vmw_pvscsi: using MSI-X\n");
+ adapter->use_msix = 1;
+ } else if (!pvscsi_disable_msi && pci_enable_msi(pdev) == 0) {
+ printk(KERN_INFO "vmw_pvscsi: using MSI\n");
+ adapter->use_msi = 1;
+ adapter->irq = pdev->irq;
+ } else {
+ printk(KERN_INFO "vmw_pvscsi: using INTx\n");
+ adapter->irq = pdev->irq;
+ }
+
+ error = request_irq(adapter->irq, pvscsi_isr, IRQF_SHARED,
+ "vmw_pvscsi", adapter);

Typically IRQF_SHARED w/ INTx, not MSI and MSI-X.

drivers/gpu/vga/vgaarb.c
drivers/hwmon/lis3lv02d.c
fs/cachefiles/daemon.c
fs/fuse/cuse.c
net/rfkill/core.c

drivers/staging/android/binder.c
drivers/staging/dream/qdsp5/audio_aac.c
drivers/staging/dream/qdsp5/audio_evrc.c
drivers/staging/dream/qdsp5/audio_in.c
drivers/staging/dream/qdsp5/audio_out.c
drivers/staging/dream/qdsp5/snd.c
drivers/staging/dream/smd/smd_qmi.c
drivers/staging/panel/panel.c

These are new drivers that were merged after the pushdown
and should be looked at, though I don't expect that any of
them to need it because none of them use the locked ioctl
method or any other form of BKL.

On Tuesday 13 October 2009, Jonathan Corbet wrote:

(I'm less worried about the uniqueness side, BTW; it just means drivers
need to create meaningful names for their GPIOs.)

I don't see how they *can* though ... unless they're dynamically
allocated using a scheme like "combine with ". Consider two PCI cards with two different GPIOs
for their "camera_active" LED... "camera_active" is meaningful,
but unusable because it's not unique.

On Wed, Oct 14, 2009 at 04:52:40PM +0200, Bartlomiej Zolnierkiewicz wrote:
On Wednesday 14 October 2009 16:09:24 John W. Linville wrote:
On Tue, Oct 13, 2009 at 11:57:57PM +0200, Bartlomiej Zolnierkiewicz wrote:

Several months later (after all current users are happy with the improved
drivers) the old drivers will be removed from staging..

I _really_ hope this isn't the standard. By that measure, nothing
will ever leave staging as _someone_ will always have some use case
that makes the other driver better for them.

Doesn't seem to be the case with all other drivers except wireless ones
and the only real reason why wireless ones are so special is because of:

"But then there's the users, who bitch about everything and simply do
not understand what it is at stake.

And all this is becoming way too messy for us to handle.

Personally all I see around me is people bitching and complaining, and
no code being ported."

attitude which pretty much explains the problem..

Please adjust the process to fix the problem yourself or if you do not
want to do it just make the room for people who do.

Alright Bartlomiej, you've had your say. Perhaps you should go about
your business now.

John

P.S. For the record, the "But then there's the users..." quote is
from someone else...

If I need to live with a display glitch every 5-10 minutes or so to
get better power savings, I'll take it....