Re: [PATCH 5/6] firewire: SBP-2 highlevel driver

Stefan Richter
2007-05-02T21:53:57Z

Christoph Hellwig wrote:
>> + /* Convert the scatterlist to an sbp2 page table. If any
>> + * scatterlist entries are too big for sbp2 we split the as we go. */
>
> Please set the max_sectors value in your host template so that the
> block layer doesn't build sg entries too big for you.

Hmm, what about this:

James Bottomley wrote on 2007-01-15:
| Actually, there's one unfortunate case where Linux won't respect this:
| an IOMMU that can do virtual merging. This parameter is a block queue
| parameter, so block will happily make sure the request segments obey it.
| However, when you get to dma_map_rq() it doesn't see the segment limits,
| so, if the iommu merges, you can end up with SG elements the other side
| that violate this. I've been meaning to do something about this for
| ages (IDE is the other subsystem that has an absolute requirement for a
| fixed maximum segment size) but never found an excuse to fix it.

http://marc.info/?l=linux-scsi&m=116889641203397

>> +static int add_scsi_devices(struct fw_unit *unit)
>> +{
>> + struct sbp2_device *sd = unit->device.driver_data;
>> + int retval, lun;
>> +
>> + if (sd->scsi_host != NULL)
>> + return 0;
>> +
>> + sd->scsi_host = scsi_host_alloc(&scsi_driver_template,
>> + sizeof(unsigned long));
>> + if (sd->scsi_host == NULL) {
>> + fw_error("failed to register scsi host\n");
>> + return -1;
>> + }
>> +
>> + sd->scsi_host->hostdata[0] = (unsigned long)unit;
>
> Please take a look ar ther other scsi drivers how this is supposed
> to be used.

Do you mean the one Scsi_Host per LU? If it is that, then it was just
taken over from drivers/ieee1394/sbp2.c. Sbp2 is doing this still today
mostly for historical reasons; I just didn't find the time yet to try to
get to a leaner scheme.

Or do you mean something else?

>> + retval = scsi_add_host(sd->scsi_host, &unit->device);
>> + if (retval < 0) {
>> + fw_error("failed to add scsi host\n");
>> + scsi_host_put(sd->scsi_host);
>> + sd->scsi_host = NULL;
>> + return retval;
>> + }
>> +
>> + /* FIXME: Loop over luns here. */
>> + lun = 0;
>> + retval = scsi_add_device(sd->scsi_host, 0, 0, lun);
>> + if (retval < 0) {
>> + fw_error("failed to add scsi device\n");
>> + scsi_remove_host(sd->scsi_host);
>> + scsi_host_put(sd->scsi_host);
>> + sd->scsi_host = NULL;
>> + return retval;
>> + }
>> +
>> + return 0;
>> +}
>
> Do we really need another scanning algorithm?

Yes.

> Can't you use scsi_scan_target instead and let the core scsi code
> handle the scanning?

No. The discovery of LUs of SBP-2 targets happens on the IEEE 1212
level of things. The initiator has to parse the configuration ROM of
the target FireWire node; the ROM has entries for each LU. (After that,
SBP-2 login protocol commences for each LU, and only after that can SCSI
requests be issued. There is nothing SCSIish going on before that.)

What's missing as a /* FIXME */ here is actually implemented in the
mainline sbp2.c and needs to be brought over here; converted to the new
FireWire core APIs.

>> +
>> +static void remove_scsi_devices(struct fw_unit *unit)
>> +{
>> + struct sbp2_device *sd = unit->device.driver_data;
>> +
>> + if (sd->scsi_host != NULL) {
>> + scsi_remove_host(sd->scsi_host);
>> + scsi_host_put(sd->scsi_host);
>> + }
>> + sd->scsi_host = NULL;
>> +}
>
> This function seems rather oddly named. And the checking and
> setting of scsi_host looks like you have some lifetime rule
> problems.
>

The NULL probably has to do with the ability to call remove_scsi_devices
in different paths. (These paths are not concurrent.)
--
Stefan Richter
-=====-=-=== -=-= ---=-