1 Apr 2004 01:12
Re: [PATCH] ibmvscsi driver - sixth version
Dave Boutcher <sleddog <at> us.ibm.com>
2004-03-31 23:12:13 GMT
2004-03-31 23:12:13 GMT
On Wed, 31 Mar 2004 17:02:02 -0500, Jeff Garzik <jgarzik <at> 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?
(Continue reading)
>>
>>And the:
>>
>>
>> if (blk_pc_request(req))
>> leftover = req->data_len - bytes;
>>
>>Since end_that_request chunk won't muck with that, is that OK?
>
>
> It's a bit messy right now really, but it's up to the drivers to modify
> that one. So for partial completions, it should be updated to be
>
> leftover = req->data_len;
>
> as well.
>
Is that a typo? If drivers are to account for it, scsi should do
leftover = req->data_len - bytes;
?
-
I'll resubmit without the _irq
Actually, no, with the irq is correct. Wait_for_completion will sleep,
and sleeping with interrupts disabled is wrong.
The reason for this is that the error handler takes the host lock around
calls to the driver error handler functions. The rationale was that
"simple" drivers didn't want to bother with locking, but it's obviously
causing more problems than it solves.
> > 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.
Erm, something is very wrong in the iSeries code then. This
RSS Feed