All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stefan Richter <stefanr@s5r6.in-berlin.de>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org, linux1394-devel@lists.sourceforge.net
Subject: [git pull] FireWire regression fix
Date: Wed, 4 May 2011 13:45:20 +0200	[thread overview]
Message-ID: <20110504134520.0a0aa35e@stein> (raw)

Linus, please pull from the fixes branch at

    git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git fixes

to receive the following update for the IEEE 1394 (FireWire) subsystem.
Thanks.

B.J. Buchalter (1):
      firewire: Fix for broken configrom updates in quick succession

 drivers/firewire/ohci.c |   39 +++++++++++++++++++++++++--------------
 1 files changed, 25 insertions(+), 14 deletions(-)


commit 2e053a27d9d5ad5e0831e002cbf8043836fb2060
Author: B.J. Buchalter <bj@mhlabs.com>
Date:   Mon May 2 13:33:42 2011 -0400

    firewire: Fix for broken configrom updates in quick succession
    
    Current implementation of ohci_set_config_rom() uses a deferred
    bus reset via fw_schedule_bus_reset(). If clients add multiple
    unit descriptors to the config_rom in quick succession, the
    deferred bus reset may not have fired before succeeding update
    requests have come in. This can lead to an incorrect partial
    update of the config_rom for both addition and removal of
    config_rom descriptors, as the ohci_set_config_rom() routine
    will return -EBUSY if a previous pending update has not been
    completed yet; the requested update just gets dropped on the floor.
    
    This patch recognizes that the "in-flight" update can be modified
    until it has been processed by the bus-reset, and the locking
    in the bus_reset_tasklet ensures that the update is done atomically
    with respect to modifications made by ohci_set_config_rom(). The
    -EBUSY error case is simply removed.
    
    [Stefan R:  The bug always existed at least theoretically.  But it
    became easy to trigger since 2.6.36 commit 02d37bed188c "firewire: core:
    integrate software-forced bus resets with bus management" which
    introduced long mandatory delays between janitorial bus resets.]
    
    Signed-off-by: Benjamin Buchalter <bj@mhlabs.com>
    Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> (trivial style changes)
    Cc: <stable@kernel.org> # 2.6.36.y and newer

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index f903d7b6..23d1468 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -2199,7 +2199,6 @@ static int ohci_set_config_rom(struct fw_card *card,
 {
 	struct fw_ohci *ohci;
 	unsigned long flags;
-	int ret = -EBUSY;
 	__be32 *next_config_rom;
 	dma_addr_t uninitialized_var(next_config_rom_bus);
 
@@ -2240,22 +2239,37 @@ static int ohci_set_config_rom(struct fw_card *card,
 
 	spin_lock_irqsave(&ohci->lock, flags);
 
+	/*
+	 * If there is not an already pending config_rom update,
+	 * push our new allocation into the ohci->next_config_rom
+	 * and then mark the local variable as null so that we
+	 * won't deallocate the new buffer.
+	 *
+	 * OTOH, if there is a pending config_rom update, just
+	 * use that buffer with the new config_rom data, and
+	 * let this routine free the unused DMA allocation.
+	 */
+
 	if (ohci->next_config_rom == NULL) {
 		ohci->next_config_rom = next_config_rom;
 		ohci->next_config_rom_bus = next_config_rom_bus;
+		next_config_rom = NULL;
+	}
 
-		copy_config_rom(ohci->next_config_rom, config_rom, length);
+	copy_config_rom(ohci->next_config_rom, config_rom, length);
 
-		ohci->next_header = config_rom[0];
-		ohci->next_config_rom[0] = 0;
+	ohci->next_header = config_rom[0];
+	ohci->next_config_rom[0] = 0;
 
-		reg_write(ohci, OHCI1394_ConfigROMmap,
-			  ohci->next_config_rom_bus);
-		ret = 0;
-	}
+	reg_write(ohci, OHCI1394_ConfigROMmap, ohci->next_config_rom_bus);
 
 	spin_unlock_irqrestore(&ohci->lock, flags);
 
+	/* If we didn't use the DMA allocation, delete it. */
+	if (next_config_rom != NULL)
+		dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
+				  next_config_rom, next_config_rom_bus);
+
 	/*
 	 * Now initiate a bus reset to have the changes take
 	 * effect. We clean up the old config rom memory and DMA
@@ -2263,13 +2277,10 @@ static int ohci_set_config_rom(struct fw_card *card,
 	 * controller could need to access it before the bus reset
 	 * takes effect.
 	 */
-	if (ret == 0)
-		fw_schedule_bus_reset(&ohci->card, true, true);
-	else
-		dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
-				  next_config_rom, next_config_rom_bus);
 
-	return ret;
+	fw_schedule_bus_reset(&ohci->card, true, true);
+
+	return 0;
 }
 
 static void ohci_send_request(struct fw_card *card, struct fw_packet *packet)

-- 
Stefan Richter
-=====-==-== -=-= --=--
http://arcgraph.de/sr/

             reply	other threads:[~2011-05-04 11:45 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-04 11:45 Stefan Richter [this message]
  -- strict thread matches above, loose matches on Subject: below --
2014-07-26  8:48 [git pull] FireWire regression fix Stefan Richter
2013-07-29 20:33 Stefan Richter
2008-08-06 18:47 Stefan Richter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110504134520.0a0aa35e@stein \
    --to=stefanr@s5r6.in-berlin.de \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux1394-devel@lists.sourceforge.net \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.