Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757079AbYHKTwp (ORCPT ); Mon, 11 Aug 2008 15:52:45 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753265AbYHKTwh (ORCPT ); Mon, 11 Aug 2008 15:52:37 -0400 Received: from smtp-out.google.com ([216.239.33.17]:41064 "EHLO smtp-out.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751630AbYHKTwh (ORCPT ); Mon, 11 Aug 2008 15:52:37 -0400 DomainKey-Signature: a=rsa-sha1; s=beta; d=google.com; c=nofws; q=dns; h=message-id:date:from:to:subject:cc:in-reply-to: mime-version:content-type:content-transfer-encoding: content-disposition:references; b=Q7xdW/FT2Sh3SNSXMpGeHwNuYtxIu2Zu+w+stT9j/mDUaThfU6Ln5aTe10r1cgqih FCATe7xfY5n+CEgM40xPA== Message-ID: Date: Mon, 11 Aug 2008 12:52:19 -0700 From: "Grant Grundler" To: "Stefan Richter" Subject: Re: [PATCH] firewire: fw-sbp2: enforce s/g segment size limit Cc: linux1394-devel@lists.sourceforge.net, "FUJITA Tomonori" , linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <489CB8D8.60908@s5r6.in-berlin.de> <20080809063159F.fujita.tomonori@lab.ntt.co.jp> <489CC17F.7000208@s5r6.in-berlin.de> <20080809071744I.fujita.tomonori@lab.ntt.co.jp> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7192 Lines: 179 On Sat, Aug 9, 2008 at 11:21 AM, Stefan Richter wrote: > 1. We don't need to round the SBP-2 segment size limit down to a > multiple of 4 kB (0xffff -> 0xf000). It is only necessary to > ensure quadlet alignment (0xffff -> 0xfffc). > > 2. Use dma_set_max_seg_size() to tell the DMA mapping infrastructure > and the block IO layer about the restriction. This way we can > remove the size checks and segment splitting in the queuecommand > path. > > This assumes that no other code in the firewire stack uses > dma_map_sg() with conflicting requirements. It furthermore assumes > that the controller device's platform actually allows us to set the > segment size to our liking. Assert the latter with a BUG_ON(). > > 3. Also use blk_queue_max_segment_size() to tell the block IO layer > about it. It cannot know it because our scsi_add_host() does not > point to the FireWire controller's device. > > Signed-off-by: Stefan Richter > --- > drivers/firewire/fw-sbp2.c | 68 +++++++++++++++++-------------------- > 1 file changed, 33 insertions(+), 35 deletions(-) > > Index: linux-2.6.27-rc2/drivers/firewire/fw-sbp2.c > =================================================================== > --- linux-2.6.27-rc2.orig/drivers/firewire/fw-sbp2.c > +++ linux-2.6.27-rc2/drivers/firewire/fw-sbp2.c > @@ -29,6 +29,7 @@ > */ > > #include > +#include > #include > #include > #include > @@ -181,10 +182,16 @@ struct sbp2_target { > #define SBP2_MAX_LOGIN_ORB_TIMEOUT 40000U /* Timeout in ms */ > #define SBP2_ORB_TIMEOUT 2000U /* Timeout in ms */ > #define SBP2_ORB_NULL 0x80000000 > -#define SBP2_MAX_SG_ELEMENT_LENGTH 0xf000 > #define SBP2_RETRY_LIMIT 0xf /* 15 retries */ > #define SBP2_CYCLE_LIMIT (0xc8 << 12) /* 200 125us cycles */ > > +/* > + * The default maximum s/g segment size of a FireWire controller is > + * usually 0x10000, but SBP-2 only allows 0xffff. Since buffers have to > + * be quadlet-aligned, we set the length limit to 0xffff & ~3. > + */ > +#define SBP2_MAX_SEG_SIZE 0xfffc > + > /* Unit directory keys */ > #define SBP2_CSR_UNIT_CHARACTERISTICS 0x3a > #define SBP2_CSR_FIRMWARE_REVISION 0x3c > @@ -1099,6 +1106,10 @@ static int sbp2_probe(struct device *dev > struct Scsi_Host *shost; > u32 model, firmware_revision; > > + if (dma_get_max_seg_size(device->card->device) > SBP2_MAX_SEG_SIZE) > + BUG_ON(dma_set_max_seg_size(device->card->device, > + SBP2_MAX_SEG_SIZE)); > + > shost = scsi_host_alloc(&scsi_driver_template, sizeof(*tgt)); > if (shost == NULL) > return -ENOMEM; > @@ -1347,14 +1358,12 @@ static int > sbp2_map_scatterlist(struct sbp2_command_orb *orb, struct fw_device *device, > struct sbp2_logical_unit *lu) > { > - struct scatterlist *sg; > - int sg_len, l, i, j, count; > - dma_addr_t sg_addr; > - > - sg = scsi_sglist(orb->cmd); > - count = dma_map_sg(device->card->device, sg, scsi_sg_count(orb->cmd), > - orb->cmd->sc_data_direction); > - if (count == 0) > + struct scatterlist *sg = scsi_sglist(orb->cmd); > + int i, j, len; > + > + i = dma_map_sg(device->card->device, sg, scsi_sg_count(orb->cmd), > + orb->cmd->sc_data_direction); > + if (i == 0) > goto fail; > > /* > @@ -1364,7 +1373,7 @@ sbp2_map_scatterlist(struct sbp2_command > * as the second generation iPod which doesn't support page > * tables. > */ > - if (count == 1 && sg_dma_len(sg) < SBP2_MAX_SG_ELEMENT_LENGTH) { > + if (i == 1) { > orb->request.data_descriptor.high = > cpu_to_be32(lu->tgt->address_high); > orb->request.data_descriptor.low = > @@ -1374,29 +1383,15 @@ sbp2_map_scatterlist(struct sbp2_command > return 0; > } > > - /* > - * Convert the scatterlist to an sbp2 page table. If any > - * scatterlist entries are too big for sbp2, we split them as we > - * go. Even if we ask the block I/O layer to not give us sg > - * elements larger than 65535 bytes, some IOMMUs may merge sg elements > - * during DMA mapping, and Linux currently doesn't prevent this. > - */ > - for (i = 0, j = 0; i < count; i++, sg = sg_next(sg)) { > - sg_len = sg_dma_len(sg); > - sg_addr = sg_dma_address(sg); > - while (sg_len) { > - /* FIXME: This won't get us out of the pinch. */ > - if (unlikely(j >= ARRAY_SIZE(orb->page_table))) { > - fw_error("page table overflow\n"); > - goto fail_page_table; > - } > - l = min(sg_len, SBP2_MAX_SG_ELEMENT_LENGTH); > - orb->page_table[j].low = cpu_to_be32(sg_addr); > - orb->page_table[j].high = cpu_to_be32(l << 16); I didn't check the rest of the driver - but it would be good if it explicitly called dma_set_mask() or pci_dma_set_mask() with a 32-bit mask value. Most drivers assume 32-bit and that's why I point this out. The rest looks ok at first glance. thanks, grant > - sg_addr += l; > - sg_len -= l; > - j++; > - } > + j = 0; > + for_each_sg(sg, sg, scsi_sg_count(orb->cmd), i) { > + len = sg_dma_len(sg); > + if (len == 0) > + continue; > + > + orb->page_table[j].high = cpu_to_be32(len << 16); > + orb->page_table[j].low = cpu_to_be32(sg_dma_address(sg)); > + j++; > } > > orb->page_table_bus = > @@ -1420,8 +1415,9 @@ sbp2_map_scatterlist(struct sbp2_command > return 0; > > fail_page_table: > - dma_unmap_sg(device->card->device, sg, scsi_sg_count(orb->cmd), > - orb->cmd->sc_data_direction); > + orb->page_table_bus = 0; > + dma_unmap_sg(device->card->device, scsi_sglist(orb->cmd), > + scsi_sg_count(orb->cmd), orb->cmd->sc_data_direction); > fail: > return -ENOMEM; > } > @@ -1542,6 +1538,8 @@ static int sbp2_scsi_slave_configure(str > if (lu->tgt->workarounds & SBP2_WORKAROUND_128K_MAX_TRANS) > blk_queue_max_sectors(sdev->request_queue, 128 * 1024 / 512); > > + blk_queue_max_segment_size(sdev->request_queue, SBP2_MAX_SEG_SIZE); > + > return 0; > } > > > -- > Stefan Richter > -=====-==--- =--- -=--= > http://arcgraph.de/sr/ > > -- 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/