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

[Christoph Hellwig <hch@infradead.org>]

On Wed, May 02, 2007 at 11:53:30PM +0200, Stefan Richter wrote:
> > 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

Hmm, okay.  Probably wants a comment in there explaining it, and we
should poke jejb to fix it for real :)

> >> +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.

No, sorry.  I should have written a better explanation.  scsi_host_alloc
is designed to allocate space for your private data aswell.  So you
should call it early on an allocate the sbp2_device as part of the Scsi_Host
instead of just stuffing in a pointer.

> > 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.

Okay, so sbp2 decided to be non-standard here, what a pity.  It's probably
better to use scsi_scan_target with a specific lun, though as scsi_add_device
is a rather awkward API.

> > 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.)

Needs documentation at least.  And at least my preference would be
to have a deleted flag instead of the null setting because the latter
can easily paper over bugs.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/