Re: [PATCH] ibmvscsi driver - sixth version

[Dave Boutcher <sleddog@us.ibm.com>]

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