Re: [PATCH] ibmvscsi driver - sixth version

Dave Boutcher
2004-03-31T23:12:37Z

On Wed, 31 Mar 2004 17:02:02 -0500, Jeff Garzik <jgarzik@pobox.com> wrote:
> Comments:
>
> 1) Would be nice to eliminate module options for commands-per-lun,
> max-requests etc. and actually have the driver figure out the real
> needs. One or two options could fall into the "performance tuning"
> category, and stay, but the others are really dependent on the
> underlying configuration and underlying limits.
Hmmm...well, I could collapse the two together, since commands_per_lun
is not limited by anything specific for this adapter. I would prefer
to leave them broken out to handle users who have extreme requirements.

> 2) why is one-descriptor a special case in map_sg_data()? You proceed
> to do the same thing in a loop, right below that... AFAICS you can just
> use the loop, and clamp the number of scatterlist elements to one.
The SRP spec has two different buffer formats: SRP_DIRECT_BUFFER for a
single buffer, and SRP_INDIRECT_BUFFER for lists of buffers, and the
layout of the buffers in the request is different for those two cases.

> 3) in the following code...
>
> + if ((evt_struct->crq.format == VIOSRP_SRP_FORMAT) &&
> + (atomic_dec_if_positive(&hostdata->request_limit) < 0)) {
> + printk("ibmvscsi: Warning, request_limit exceeded\n");
> + unmap_cmd_data(&evt_struct->evt->srp.cmd, hostdata->dev);
> + ibmvscsi_free_event_struct(&hostdata->pool, evt_struct);
> + return SCSI_MLQUEUE_HOST_BUSY;
> + }
>
> is the code prepared to deal with hostdata->request_limit being negative?
Yes....that is one of the things that would drive you down that path.
There are a couple of places in the code where request_limit is explicitly
set to -1 to force all requests to be rejected.

> 4) style: don't bother with unneeded brackets...
>
> + if (!evt_struct) {
> + return SCSI_MLQUEUE_HOST_BUSY;
> + }
> +
Fair enough.

>
> 5) two issues with your abort handler...
>
> + spin_unlock_irq(hostdata->host->host_lock);
> + wait_for_completion(&evt->comp);
> + spin_lock_irq(hostdata->host->host_lock);
>
> 5a) are there recursion or deadlock issues, when you release the lock
> like this?
>
> 5b) IMO unconditional spin_{un}lock_irq() is unwise... the
> ->eh_abort_handler() saved the flags, so you might be restoring things
> to an incorrect state.
>
> Since you're already inside spin_lock_irqsave(), and #5a is not an
> issue, just use spin_unlock() and spin_lock().
OK, two issues. There are a bunch of SCSI LLDDs that use this same
logic in abort and reset handlers to wait for adapter events to
complete, so I think the logic is OK. The issue of spin_lock_irq
vs spin_lock is a good one...and points out that there are a bunch
of LLDDs that are broke :-) I'll resubmit without the _irq

>
> 6) ditto ibmvscsi_eh_device_reset_handler

> 7) style: the names of many of the structures defined by this driver are
> IN ALL CAPS AND SHOUTING :) Structs should not be in all caps.
Ya, copied from less sane operating systems. I'll fix.

> 8) purge_requests() locking looks bogus. It is safe to complete a SCSI
> command inside the host lock.
I'll fix.

> 9) ibmvscsi_do_host_config() continues, after a DMA mapping error
Oops.

> 10) what the heck is this??? do some people not know they are running
> Linux???
> +static ssize_t show_host_os_type(struct class_device *class_dev, char
> *buf)
This whole driver has to do with adapter sharing....and this answers
the question with whom you are sharing it. Which may in fact NOT
be Linux.

> 11) I'm not sure ".emulated = 1" is correct, since you are really
> talking SCSI. But this is arguable, as is the value of 'emulated' flag
> itself...
Agreed...I'll walk either side of the street on that one.

> 12) in ibmvscsi_probe(), you want to use TASK_UNINTERRUPTIBLE here:
>
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(5);
>
> 13) in the code pasted in #12, you should pass a value calculated using
> the 'HZ' constant.
Hmmm...above code copied from the qlogic driver...and it looked reasonable
to me, but I'll tweak it.

> 14) why are you faking a PCI bus? The following is very, very wrong:
>
> +static struct pci_dev iseries_vscsi_dev = {
> + .dev.bus = &pci_bus_type,
> + .dev.bus_id = "vscsi",
> + .dev.release = noop_release
>
> Did I mention "very" wrong? :)
Because for iseries it is implemented in the pci code. While it may
look wrong, it is actually correct. Check out
arch/ppc64/kernel/iSeries_iommu.c and arch/ppc64/kernel/dma.c.
This device has to have dev->bus == &pci_bus_type otherwise the
dma_mapping_foo functions won't work correctly.

Thanks for the comments!!!!!!!!!!!!!!

Dave B