From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752035AbbLUVm2 (ORCPT ); Mon, 21 Dec 2015 16:42:28 -0500 Received: from smtp687.redcondor.net ([208.80.206.87]:58376 "EHLO smtp687.redcondor.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751434AbbLUVm0 (ORCPT ); Mon, 21 Dec 2015 16:42:26 -0500 X-RC-FROM: Subject: Re: [PATCH] ata: sata_dwc_460ex: remove incorrect locking To: Andy Shevchenko , Mans Rullgard , Tejun Heo , linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org References: <1450721870-23266-1-git-send-email-mans@mansr.com> <1450730477.30729.274.camel@linux.intel.com> From: Julian Margetson Message-ID: <5678722B.8050600@candw.ms> Date: Mon, 21 Dec 2015 17:42:03 -0400 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1450730477.30729.274.camel@linux.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-DLP-ENABLED: 137.118.22.64/27 X-MAG-OUTBOUND: greymail.redcondor.net@137.118.22.64/27 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/21/2015 4:41 PM, Andy Shevchenko wrote: > On Mon, 2015-12-21 at 18:17 +0000, Mans Rullgard wrote: >> This lock is already taken in ata_scsi_queuecmd() a few levels up the >> call stack so attempting to take it here is an error. Moreover, it >> is >> pointless in the first place since it only protects a single, atomic >> assignment. >> >> Enabling lock debugging gives the following output: >> > +Julian. > > I think Julian may give his Tested-by: tag. > >> ============================================= >> [ INFO: possible recursive locking detected ] >> 4.4.0-rc5+ #189 Not tainted >> --------------------------------------------- >> kworker/u2:3/37 is trying to acquire lock: >> (&(&host->lock)->rlock){-.-...}, at: [<90283294>] >> sata_dwc_exec_command_by_tag.constprop.14+0x44/0x8c >> >> but task is already holding lock: >> (&(&host->lock)->rlock){-.-...}, at: [<902761ac>] >> ata_scsi_queuecmd+0x2c/0x330 >> >> other info that might help us debug this: >> Possible unsafe locking scenario: >> >> CPU0 >> ---- >> lock(&(&host->lock)->rlock); >> lock(&(&host->lock)->rlock); >> >> *** DEADLOCK *** >> May be due to missing lock nesting notation >> >> 4 locks held by kworker/u2:3/37: >> #0: ("events_unbound"){.+.+.+}, at: [<9003a0a4>] >> process_one_work+0x12c/0x430 >> #1: ((&entry->work)){+.+.+.}, at: [<9003a0a4>] >> process_one_work+0x12c/0x430 >> #2: (&bdev->bd_mutex){+.+.+.}, at: [<9011fd54>] >> __blkdev_get+0x50/0x380 >> #3: (&(&host->lock)->rlock){-.-...}, at: [<902761ac>] >> ata_scsi_queuecmd+0x2c/0x330 >> >> stack backtrace: >> CPU: 0 PID: 37 Comm: kworker/u2:3 Not tainted 4.4.0-rc5+ #189 >> Workqueue: events_unbound async_run_entry_fn >> Stack : 90b38e30 00000021 00000003 9b2a6040 00000000 9005f3f0 >> 904fc8dc 00000025 >> 906b96e4 00000000 90528648 9b3336c4 904fc8dc 9009bf18 >> 00000002 00000004 >> 00000000 00000000 9b3336c4 9b3336e4 904fc8dc 9003d074 >> 00000000 90500000 >> 9005e738 00000000 00000000 00000000 00000000 00000000 >> 00000000 00000000 >> 6e657665 755f7374 756f626e 0000646e 00000000 00000000 >> 9b00ca00 9b025000 >> ... >> Call Trace: >> [<90009d6c>] show_stack+0x88/0xa4 >> [<90057744>] __lock_acquire+0x1ce8/0x2154 >> [<900583e4>] lock_acquire+0x64/0x8c >> [<9045ff10>] _raw_spin_lock_irqsave+0x54/0x78 >> [<90283294>] sata_dwc_exec_command_by_tag.constprop.14+0x44/0x8c >> [<90283484>] sata_dwc_qc_issue+0x1a8/0x24c >> [<9026b39c>] ata_qc_issue+0x1f0/0x410 >> [<90273c6c>] ata_scsi_translate+0xb4/0x200 >> [<90276234>] ata_scsi_queuecmd+0xb4/0x330 >> [<9025800c>] scsi_dispatch_cmd+0xd0/0x128 >> [<90259934>] scsi_request_fn+0x58c/0x638 >> [<901a3e50>] __blk_run_queue+0x40/0x5c >> [<901a83d4>] blk_queue_bio+0x27c/0x28c >> [<901a5914>] generic_make_request+0xf0/0x188 >> [<901a5a54>] submit_bio+0xa8/0x194 >> [<9011adcc>] submit_bh_wbc.isra.23+0x15c/0x17c >> [<9011c908>] block_read_full_page+0x3e4/0x428 >> [<9009e2e0>] do_read_cache_page+0xac/0x210 >> [<9009fd90>] read_cache_page+0x18/0x24 >> [<901bbd18>] read_dev_sector+0x38/0xb0 >> [<901bd174>] msdos_partition+0xb4/0x5c0 >> [<901bcb8c>] check_partition+0x140/0x274 >> [<901bba60>] rescan_partitions+0xa0/0x2b0 >> [<9011ff68>] __blkdev_get+0x264/0x380 >> [<901201ac>] blkdev_get+0x128/0x36c >> [<901b9378>] add_disk+0x3c0/0x4bc >> [<90268268>] sd_probe_async+0x100/0x224 >> [<90043a44>] async_run_entry_fn+0x50/0x124 >> [<9003a11c>] process_one_work+0x1a4/0x430 >> [<9003a4f4>] worker_thread+0x14c/0x4fc >> [<900408f4>] kthread+0xd0/0xe8 >> [<90004338>] ret_from_kernel_thread+0x14/0x1c >> >> Fixes: 62936009f35a ("[libata] Add 460EX on-chip SATA driver, >> sata_dwc_460ex") >> Signed-off-by: Mans Rullgard >> --- >> drivers/ata/sata_dwc_460ex.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/ata/sata_dwc_460ex.c >> b/drivers/ata/sata_dwc_460ex.c >> index 902034991517..7a7faca0ddcd 100644 >> --- a/drivers/ata/sata_dwc_460ex.c >> +++ b/drivers/ata/sata_dwc_460ex.c >> @@ -924,15 +924,13 @@ static void sata_dwc_exec_command_by_tag(struct >> ata_port *ap, >> struct ata_taskfile *tf, >> u8 tag, u32 cmd_issued) >> { >> - unsigned long flags; >> struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap); >> >> dev_dbg(ap->dev, "%s cmd(0x%02x): %s tag=%d\n", __func__, >> tf->command, >> ata_get_cmd_descript(tf->command), tag); >> >> - spin_lock_irqsave(&ap->host->lock, flags); >> hsdevp->cmd_issued[tag] = cmd_issued; >> - spin_unlock_irqrestore(&ap->host->lock, flags); >> + >> /* >> * Clear SError before executing a new command. >> * sata_dwc_scr_write and read can not be used here. >> Clearing the PM Tested-by: Julian Margetson