Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933308AbXJQLi3 (ORCPT ); Wed, 17 Oct 2007 07:38:29 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1760034AbXJQLiS (ORCPT ); Wed, 17 Oct 2007 07:38:18 -0400 Received: from tama5.ecl.ntt.co.jp ([129.60.39.102]:48379 "EHLO tama5.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754511AbXJQLiR (ORCPT ); Wed, 17 Oct 2007 07:38:17 -0400 To: jens.axboe@oracle.com Cc: davem@davemloft.net, fujita.tomonori@lab.ntt.co.jp, linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, tomof@acm.org Subject: Re: [PATCH] SPARC64: fix iommu sg chaining From: FUJITA Tomonori In-Reply-To: <20071017110142.GU5043@kernel.dk> References: <20071017.035441.74746708.davem@davemloft.net> <20071017105840.GT5043@kernel.dk> <20071017110142.GU5043@kernel.dk> Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-Id: <20071017203758E.fujita.tomonori@lab.ntt.co.jp> Date: Wed, 17 Oct 2007 20:37:58 +0900 X-Dispatcher: imput version 20040704(IM147) Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3811 Lines: 108 On Wed, 17 Oct 2007 13:01:42 +0200 Jens Axboe wrote: > On Wed, Oct 17 2007, Jens Axboe wrote: > > On Wed, Oct 17 2007, David Miller wrote: > > > From: Jens Axboe > > > Date: Wed, 17 Oct 2007 11:16:29 +0200 > > > > > > > On Wed, Oct 17 2007, David Miller wrote: > > > > > From: Jens Axboe > > > > > Date: Wed, 17 Oct 2007 10:45:28 +0200 > > > > > > > > > > > Righto, it's invalid to call sg_next() on the last entry! > > > > > > > > > > Unfortunately, that's what the sparc64 code wanted to do, this > > > > > transformation in the sparc64 sg chaining patch is not equilavent: > > > > > > > > > > - struct scatterlist *sg_end = sg + nelems; > > > > > + struct scatterlist *sg_end = sg_last(sg, nelems); > > > > > ... > > > > > - while (sg < sg_end && > > > > > + while (sg != sg_end && > > > > > > > > Auch indeed. That'd probably be better as a > > > > > > > > do { > > > > ... > > > > } while (sg != sg_end); > > > > > > Ok, next bug, introduced by this change: > > > > > > commit f565913ef8a8d0cfa46a1faaf8340cc357a46f3a > > > Author: Jens Axboe > > > Date: Fri Sep 21 10:44:19 2007 +0200 > > > > > > block: convert to using sg helpers > > > > > > Convert the main rq mapper (blk_rq_map_sg()) to the sg helper setup. > > > > > > Signed-off-by: Jens Axboe > > > > > > Specifically this part: > > > > > > new_segment: > > > - memset(&sg[nsegs],0,sizeof(struct scatterlist)); > > > - sg[nsegs].page = bvec->bv_page; > > > - sg[nsegs].length = nbytes; > > > - sg[nsegs].offset = bvec->bv_offset; > > > + sg = next_sg; > > > + next_sg = sg_next(sg); > > > > > > + sg->page = bvec->bv_page; > > > + sg->length = nbytes; > > > + sg->offset = bvec->bv_offset; > > > > > > You can't remove that memset(), it's there for a reason. The IOMMU > > > layers depended upon the code zero'ing out the whole scatterlist > > > struct, there might be more to it than page, length and offset :-) > > > > I realize that, and I was pretty worried about this specific change. But > > there's only been one piece of fallout because if it until now - well > > two, with the sparc64 stuff. > > > > The problem is that you cannot zero the entire sg entry, because then > > you'd potentially overwrite the chain pointer. > > > > I'd propose just adding a > > > > sg_dma_address(sg) = 0; > > sg_dma_len(sg) = 0; > > > > there for now, or provide an arch_clear_sg_entry() helper if we need > > more killed. > > Actually, just clearing AFTER sg_next() would be fine, since we know > that is not a link entry. Duh... > > diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c > index 9eabac9..1014d34 100644 > --- a/block/ll_rw_blk.c > +++ b/block/ll_rw_blk.c > @@ -1352,6 +1352,7 @@ new_segment: > sg = next_sg; > next_sg = sg_next(sg); > > + memset(sg, 0, sizeof(*sg)); > sg->page = bvec->bv_page; > sg->length = nbytes; > sg->offset = bvec->bv_offset; > > -- So now how about removing zero'ing out sglist in scsi-ml? diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index aac8a02..0c86be7 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -764,8 +764,6 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask) if (unlikely(!sgl)) goto enomem; - memset(sgl, 0, sizeof(*sgl) * sgp->size); - /* * first loop through, set initial index and return value */ - 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/