Re: [PATCH 0/5] sg_ring for scsi

FUJITA Tomonori
2007-12-27T02:11:20Z

On Wed, 26 Dec 2007 11:27:40 +1100
Rusty Russell <rusty@rustcorp.com.au> 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).