Re: [PATCH 0/5] sg_ring for scsi

[FUJITA Tomonori]

On Wed, 26 Dec 2007 11:27:40 +1100
Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:

> On Friday 21 December 2007 15:37:46 FUJITA Tomonori wrote:
> > Some scsi drivers like ips access to sglist in a tricky way.
> 
> Indeed.  I fail to see how this code works, in fact:
> 
> drivers/scsi/ips.c:ips_queue() line 1101
> 
> 	if (ips_is_passthru(SC)) {
> 
> 		ips_copp_wait_item_t *scratch;
> 
> 		/* A Reset IOCTL is only sent by the boot CD in extreme cases.           */
> 		/* There can never be any system activity ( network or disk ), but check */
> 		/* anyway just as a good practice.                                       */
> 		pt = (ips_passthru_t *) scsi_sglist(SC);
> 		if ((pt->CoppCP.cmd.reset.op_code == IPS_CMD_RESET_CHANNEL) &&
> 		    (pt->CoppCP.cmd.reset.adapter_flag == 1)) {
> 
> Casting the scsi_sglist(SC) (which ips_is_passthrough treated as a
> scatterlist) to an ips_passthrough_t seems completely bogus.  It looks like
> it wants to access the region mapped by the scatterlist.

Yeah, it seems to be broken.

> There are many signs through the code that it needs a great deal of
> work: what is the purpose of sg_break? Why does the code check if kmap_atomic
> fails?

sg_break is a workaround for the hardware restrictions, I think.


> ===
> Convert the ips SCSI driver to sg_ring.
> 
> Slightly non-trivial conversion, will need testing.

As I said, I don't think that converting SCSI drivers to sg_ring (with
lots of non-trivial work) provides any benefits. Surely, sg_ring
enables us to modify sg lists but SCSI drivers don't need the
feature. What SCSI drivers needs is just a efficient way to get the
next sg entry (they use 'sg++' in the past and sg_next now).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/