Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1321726

Re: ata: BUG in ata_sff_hsm_move

Show all headers | View raw

Hello, Dmitry.

On Fri, Jan 29, 2016 at 12:59:49PM +0100, Dmitry Vyukov wrote:
> > Hmmm... the port interrupt handler checks for IDLE before calling into
> > hsm_move, so the only explanation would be that something is resetting
> > it to IDLE inbetween.  ce7514526742 ("libata: prevent HSM state change
> > race between ISR and PIO") describes and fixes the same problem.  The
> > fix seems correct and I can't find anywhere else where this can
> > happen.  :(
> >
> > Can you please post the kernel log leading to the BUG?  Also, I don't
> > think that condition needs to be BUG.  I'll change it to WARN.
> 
> Here are two logs, in both cases no kernel messages prior to the bug:
> https://gist.githubusercontent.com/dvyukov/5087d633e3620280b6c7/raw/31c9ab1ced92ac5f85cfb15eaf48ec5793c2c3a1/gistfile1.txt
> https://gist.githubusercontent.com/dvyukov/825b2e3d5fb80ae08a9a/raw/03c5a4f4c4bd9d0a304a71cda2da4c92f4b7f1ba/gistfile1.txt

lol, this is kinda embarrassing.  It looks like the poll path wasn't
doing any locking.  Can you please verify the following patch at least
doesn't crash the machine immediately and if so keep it applied to the
test kernel so that we can verify that the problem actually goes away?

Thanks.

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 608677d..6991efc 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1362,12 +1362,14 @@ static void ata_sff_pio_task(struct work_struct *work)
 	u8 status;
 	int poll_next;
 
+	spin_lock_irq(ap->lock);
+
 	BUG_ON(ap->sff_pio_task_link == NULL);
 	/* qc can be NULL if timeout occurred */
 	qc = ata_qc_from_tag(ap, link->active_tag);
 	if (!qc) {
 		ap->sff_pio_task_link = NULL;
-		return;
+		goto out_unlock;
 	}
 
 fsm_start:
@@ -1382,11 +1384,14 @@ static void ata_sff_pio_task(struct work_struct *work)
 	 */
 	status = ata_sff_busy_wait(ap, ATA_BUSY, 5);
 	if (status & ATA_BUSY) {
+		spin_unlock_irq(ap->lock);
 		ata_msleep(ap, 2);
+		spin_lock_irq(ap->lock);
+
 		status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
 		if (status & ATA_BUSY) {
 			ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
-			return;
+			goto out_unlock;
 		}
 	}
 
@@ -1403,6 +1408,8 @@ static void ata_sff_pio_task(struct work_struct *work)
 	 */
 	if (poll_next)
 		goto fsm_start;
+out_unlock:
+	spin_unlock_irq(ap->lock);
 }
 
 /**

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

ata: BUG in ata_sff_hsm_move Dmitry Vyukov <dvyukov@google.com> - 2016-01-28 12:40 +0100
  Re: ata: BUG in ata_sff_hsm_move Tejun Heo <tj@kernel.org> - 2016-01-29 13:00 +0100
    Re: ata: BUG in ata_sff_hsm_move Dmitry Vyukov <dvyukov@google.com> - 2016-01-29 13:10 +0100
      Re: ata: BUG in ata_sff_hsm_move Tejun Heo <tj@kernel.org> - 2016-01-29 13:30 +0100
        Re: ata: BUG in ata_sff_hsm_move Dmitry Vyukov <dvyukov@google.com> - 2016-01-29 14:20 +0100
          Re: ata: BUG in ata_sff_hsm_move Dmitry Vyukov <dvyukov@google.com> - 2016-01-29 14:50 +0100
            Re: ata: BUG in ata_sff_hsm_move David Milburn <dmilburn@redhat.com> - 2016-01-29 19:10 +0100
              Re: ata: BUG in ata_sff_hsm_move Tejun Heo <tj@kernel.org> - 2016-01-29 21:30 +0100
            Re: ata: BUG in ata_sff_hsm_move Tejun Heo <tj@kernel.org> - 2016-01-29 21:30 +0100
              Re: ata: BUG in ata_sff_hsm_move Dmitry Vyukov <dvyukov@google.com> - 2016-02-01 11:50 +0100
                [PATCH libata/for-4.5-fixes] libata: fix sff host state machine  locking while polling Tejun Heo <tj@kernel.org> - 2016-02-01 18:00 +0100
    [PATCH libata/for-4.5-fixes] libata-sff: use WARN instead of BUG on  illegal host state machine state Tejun Heo <tj@kernel.org> - 2016-01-29 13:30 +0100

csiph-web