From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arjan van de Ven Subject: Re: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv Date: Tue, 20 May 2008 21:43:11 -0700 Message-ID: <20080520214311.2d2aa8d3@infradead.org> References: <48338624.3000304@shaw.ca> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: Received: from pentafluge.infradead.org ([213.146.154.40]:37206 "EHLO pentafluge.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750821AbYEUEn2 (ORCPT ); Wed, 21 May 2008 00:43:28 -0400 In-Reply-To: <48338624.3000304@shaw.ca> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Robert Hancock Cc: linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org On Tue, 20 May 2008 20:17:08 -0600 Robert Hancock wrote: > Arjan van de Ven wrote: > > From: Arjan van de Ven > > Subject: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv > > > > blk_queue_bounce_limit() is a sleeping function, so reorganize > > the code a little to not call it while holding a spinlock. > > > > Signed-off-by: Arjan van de Ven > > --- > > drivers/ata/sata_nv.c | 8 +++++--- > > 1 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c > > index 858f706..a7bc56d 100644 > > --- a/drivers/ata/sata_nv.c > > +++ b/drivers/ata/sata_nv.c > > @@ -671,6 +671,11 @@ static int nv_adma_slave_config(struct > > scsi_device *sdev) port1 = ap->host->ports[1]->private_data; > > sdev0 = ap->host->ports[0]->link.device[0].sdev; > > sdev1 = ap->host->ports[1]->link.device[0].sdev; > > + > > + blk_queue_segment_boundary(sdev->request_queue, > > segment_boundary); > > + blk_queue_max_hw_segments(sdev->request_queue, > > sg_tablesize); > > + spin_unlock_irqrestore(ap->lock, flags); > > + > > if ((port0->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) || > > (port1->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)) { > > /** We have to set the DMA mask to 32-bit if > > either port is in @@ -701,14 +706,11 @@ static int > > nv_adma_slave_config(struct scsi_device *sdev) pp->adma_dma_mask); > > } > > > > - blk_queue_segment_boundary(sdev->request_queue, > > segment_boundary); > > - blk_queue_max_hw_segments(sdev->request_queue, > > sg_tablesize); ata_port_printk(ap, KERN_INFO, > > "DMA mask 0x%llX, segment boundary 0x%lX, hw segs > > %hu\n", (unsigned long long)*ap->host->dev->dma_mask, > > segment_boundary, sg_tablesize); > > > > - spin_unlock_irqrestore(ap->lock, flags); > > > > return rc; > > } > > I'm not certain this is safe to do it quite this way. It would be > better to keep that spinlock held so that no operations could be in > progress on either port while these operations are happening. blk_bounce_limit can sleep. that's just a fact of life ;( > > It would be better to fix the regression from > 419c434c35614609fd0c79d335c134bf4b88b30b in block/blk_settings.c that > resulted in the blk_queue_bounce_limit allocation wrongly allocating > emergency ISA pages in the first place as a 32-bit DMA mask does not > need them. the condition under which it sleeps might be slightly buggy on your exact x86 machine... but that doesn't mean that that is guaranteed to be so forever going forward.... it's still a sleeping function.