[PATCH 1/2] firewire: core: no need to track irq flags in bm

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH 1/2] firewire: core: no need to track irq flags in bm_work
@ 2010-06-21 21:23 Stefan Richter
  2010-06-21 21:24 ` [PATCH 2/2] firewire: cdev: fix fw_cdev_event_bus_reset.bm_node_id Stefan Richter
  2010-06-22  9:36 ` [PATCH 1/2] firewire: core: no need to track irq flags in bm_work Philippe De Muyter
  0 siblings, 2 replies; 6+ messages in thread
From: Stefan Richter @ 2010-06-21 21:23 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

This is a workqueue job and always entered with IRQs enabled.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/core-card.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Index: b/drivers/firewire/core-card.c
===================================================================
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -239,7 +239,6 @@ static void fw_card_bm_work(struct work_
 	struct fw_card *card = container_of(work, struct fw_card, work.work);
 	struct fw_device *root_device, *irm_device;
 	struct fw_node *root_node;
-	unsigned long flags;
 	int root_id, new_root_id, irm_id, local_id;
 	int gap_count, generation, grace, rcode;
 	bool do_reset = false;
@@ -247,10 +246,10 @@ static void fw_card_bm_work(struct work_
 	bool root_device_is_cmc;
 	bool irm_is_1394_1995_only;
 
-	spin_lock_irqsave(&card->lock, flags);
+	spin_lock_irq(&card->lock);
 
 	if (card->local_node == NULL) {
-		spin_unlock_irqrestore(&card->lock, flags);
+		spin_unlock_irq(&card->lock);
 		goto out_put_card;
 	}
 
@@ -305,7 +304,7 @@ static void fw_card_bm_work(struct work_
 		card->bm_transaction_data[0] = cpu_to_be32(0x3f);
 		card->bm_transaction_data[1] = cpu_to_be32(local_id);
 
-		spin_unlock_irqrestore(&card->lock, flags);
+		spin_unlock_irq(&card->lock);
 
 		rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP,
 				irm_id, generation, SCODE_100,
@@ -336,7 +335,7 @@ static void fw_card_bm_work(struct work_
 			goto out;
 		}
 
-		spin_lock_irqsave(&card->lock, flags);
+		spin_lock_irq(&card->lock);
 
 		if (rcode != RCODE_COMPLETE) {
 			/*
@@ -355,7 +354,7 @@ static void fw_card_bm_work(struct work_
 		 * We weren't BM in the last generation, and the last
 		 * bus reset is less than 125ms ago.  Reschedule this job.
 		 */
-		spin_unlock_irqrestore(&card->lock, flags);
+		spin_unlock_irq(&card->lock);
 		fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8));
 		goto out;
 	}
@@ -378,7 +377,7 @@ static void fw_card_bm_work(struct work_
 		 * If we haven't probed this device yet, bail out now
 		 * and let's try again once that's done.
 		 */
-		spin_unlock_irqrestore(&card->lock, flags);
+		spin_unlock_irq(&card->lock);
 		goto out;
 	} else if (root_device_is_cmc) {
 		/*
@@ -416,7 +415,7 @@ static void fw_card_bm_work(struct work_
 	    (card->gap_count != gap_count || new_root_id != root_id))
 		do_reset = true;
 
-	spin_unlock_irqrestore(&card->lock, flags);
+	spin_unlock_irq(&card->lock);
 
 	if (do_reset) {
 		fw_notify("phy config: card %d, new root=%x, gap_count=%d\n",

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] firewire: cdev: fix fw_cdev_event_bus_reset.bm_node_id
  2010-06-21 21:23 [PATCH 1/2] firewire: core: no need to track irq flags in bm_work Stefan Richter
@ 2010-06-21 21:24 ` Stefan Richter
  2010-06-22  9:36 ` [PATCH 1/2] firewire: core: no need to track irq flags in bm_work Philippe De Muyter
  1 sibling, 0 replies; 6+ messages in thread
From: Stefan Richter @ 2010-06-21 21:24 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

Fix an obscure ABI feature that is a bit of a hassle to implement.
However, somebody put it into the ABI, so let's fill in a sensible
value there.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/core-card.c     |   12 +++++++++---
 drivers/firewire/core-cdev.c     |    2 +-
 drivers/firewire/core-topology.c |    1 +
 include/linux/firewire-cdev.h    |    5 +++++
 include/linux/firewire.h         |    1 +
 5 files changed, 17 insertions(+), 4 deletions(-)

Index: b/drivers/firewire/core-card.c
===================================================================
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -239,7 +239,7 @@ static void fw_card_bm_work(struct work_
 	struct fw_card *card = container_of(work, struct fw_card, work.work);
 	struct fw_device *root_device, *irm_device;
 	struct fw_node *root_node;
-	int root_id, new_root_id, irm_id, local_id;
+	int root_id, new_root_id, irm_id, bm_id, local_id;
 	int gap_count, generation, grace, rcode;
 	bool do_reset = false;
 	bool root_device_is_running;
@@ -315,9 +315,15 @@ static void fw_card_bm_work(struct work_
 			/* Another bus reset, BM work has been rescheduled. */
 			goto out;
 
-		if (rcode == RCODE_COMPLETE &&
-		    card->bm_transaction_data[0] != cpu_to_be32(0x3f)) {
+		bm_id = be32_to_cpu(card->bm_transaction_data[0]);
 
+		spin_lock_irq(&card->lock);
+		if (rcode == RCODE_COMPLETE && generation == card->generation)
+			card->bm_node_id =
+			    bm_id == 0x3f ? local_id : 0xffc0 | bm_id;
+		spin_unlock_irq(&card->lock);
+
+		if (rcode == RCODE_COMPLETE && bm_id != 0x3f) {
 			/* Somebody else is BM.  Only act as IRM. */
 			if (local_id == irm_id)
 				allocate_broadcast_channel(card, generation);
Index: b/drivers/firewire/core-cdev.c
===================================================================
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -319,7 +319,7 @@ static void fill_bus_reset_event(struct 
 	event->generation    = client->device->generation;
 	event->node_id       = client->device->node_id;
 	event->local_node_id = card->local_node->node_id;
-	event->bm_node_id    = 0; /* FIXME: We don't track the BM. */
+	event->bm_node_id    = card->bm_node_id;
 	event->irm_node_id   = card->irm_node->node_id;
 	event->root_node_id  = card->root_node->node_id;
 
Index: b/drivers/firewire/core-topology.c
===================================================================
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -552,6 +552,7 @@ void fw_core_handle_bus_reset(struct fw_
 	smp_wmb();
 	card->generation = generation;
 	card->reset_jiffies = jiffies;
+	card->bm_node_id  = 0xffff;
 	card->bm_abdicate = bm_abdicate;
 	fw_schedule_bm_work(card, 0);
 
Index: b/include/linux/firewire-cdev.h
===================================================================
--- a/include/linux/firewire-cdev.h
+++ b/include/linux/firewire-cdev.h
@@ -71,6 +71,10 @@ struct fw_cdev_event_common {
  * This event is sent when the bus the device belongs to goes through a bus
  * reset.  It provides information about the new bus configuration, such as
  * new node ID for this device, new root ID, and others.
+ *
+ * If @bm_node_id is 0xffff right after bus reset it can be reread by an
+ * %FW_CDEV_IOC_GET_INFO ioctl after bus manager selection was finished.
+ * Kernels with ABI version < 4 do not set @bm_node_id.
  */
 struct fw_cdev_event_bus_reset {
 	__u64 closure;
@@ -353,6 +357,7 @@ union fw_cdev_event {
  *  3  (2.6.34)  - made &fw_cdev_get_cycle_timer reliable
  *               - added %FW_CDEV_IOC_GET_CYCLE_TIMER2
  *  4  (2.6.36)  - added %FW_CDEV_EVENT_REQUEST2
+ *               - implemented &fw_cdev_event_bus_reset.bm_node_id
  */
 #define FW_CDEV_VERSION 3 /* Meaningless; don't use this macro. */
 
Index: b/include/linux/firewire.h
===================================================================
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -119,6 +119,7 @@ struct fw_card {
 	int bm_retries;
 	int bm_generation;
 	__be32 bm_transaction_data[2];
+	int bm_node_id;
 	bool bm_abdicate;
 
 	bool priority_budget_implemented;	/* controller feature */

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] firewire: core: no need to track irq flags in bm_work
  2010-06-21 21:23 [PATCH 1/2] firewire: core: no need to track irq flags in bm_work Stefan Richter
  2010-06-21 21:24 ` [PATCH 2/2] firewire: cdev: fix fw_cdev_event_bus_reset.bm_node_id Stefan Richter
@ 2010-06-22  9:36 ` Philippe De Muyter
  2010-06-22 11:43   ` Stefan Richter
  1 sibling, 1 reply; 6+ messages in thread
From: Philippe De Muyter @ 2010-06-22  9:36 UTC (permalink / raw)
  To: Stefan Richter; +Cc: linux1394-devel, linux-kernel

On Mon, Jun 21, 2010 at 11:23:52PM +0200, Stefan Richter wrote:
> This is a workqueue job and always entered with IRQs enabled.

did you mean 'disabled' ?

Philippe

> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
>  drivers/firewire/core-card.c |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> Index: b/drivers/firewire/core-card.c
> ===================================================================
> --- a/drivers/firewire/core-card.c
> +++ b/drivers/firewire/core-card.c
> @@ -239,7 +239,6 @@ static void fw_card_bm_work(struct work_
>  	struct fw_card *card = container_of(work, struct fw_card, work.work);
>  	struct fw_device *root_device, *irm_device;
>  	struct fw_node *root_node;
> -	unsigned long flags;
>  	int root_id, new_root_id, irm_id, local_id;
>  	int gap_count, generation, grace, rcode;
>  	bool do_reset = false;
> @@ -247,10 +246,10 @@ static void fw_card_bm_work(struct work_
>  	bool root_device_is_cmc;
>  	bool irm_is_1394_1995_only;
>  
> -	spin_lock_irqsave(&card->lock, flags);
> +	spin_lock_irq(&card->lock);


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] firewire: core: no need to track irq flags in bm_work
  2010-06-22  9:36 ` [PATCH 1/2] firewire: core: no need to track irq flags in bm_work Philippe De Muyter
@ 2010-06-22 11:43   ` Stefan Richter
  2010-06-22 14:26     ` Philippe De Muyter
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Richter @ 2010-06-22 11:43 UTC (permalink / raw)
  To: Philippe De Muyter; +Cc: linux1394-devel, linux-kernel

Philippe De Muyter wrote:
> On Mon, Jun 21, 2010 at 11:23:52PM +0200, Stefan Richter wrote:
>> This is a workqueue job and always entered with IRQs enabled.
> 
> did you mean 'disabled' ?

I meant enabled.

[...]
>> @@ -247,10 +246,10 @@ static void fw_card_bm_work(struct work_
>>  	bool root_device_is_cmc;
>>  	bool irm_is_1394_1995_only;
>>  
>> -	spin_lock_irqsave(&card->lock, flags);
>> +	spin_lock_irq(&card->lock);

  - spin_lock + spin_unlock don't influence whether IRQs on the current
    CPU are on or off.

  - spin_lock_irq + spin_unlock_irq always switch IRQs on the current
    CPU off and back on.  This is necessary if the lock could also be
    taken by an IRQ handler.  (Well, card->lock is actually only taken
    by process contexts and by tasklets.  Seems we could switch to
    spin_lock_bh + spin_unlock_bh for card->lock everywhere in the
    firewire stack.)

  - spin_lock_irqsave + spin_unlock_irqrestore switch IRQs on the
    current CPU off and back on only if used while IRQs are enabled;
    if used while local IRQs are already disabled they leave them
    disabled.

http://lwn.net/images/pdf/LDD3/ch05.pdf#page=14

Therefore some people prefer to use the safer spin_lock_irqsave()/
spin_unlock_irqrestore() everywhere.  However, their downsides are the
need to track IRQ state flags, and --- subjectively --- that their
appearance in the code could create an impression to a casual reader
that this code was meant to be able to run in IRQs-on context as well as
in IRQs-off context.  fw_card_bm_work() however definitely requires to
be called with IRQs on, notably to be able to wait for IEEE 1394
transactions to complete.
-- 
Stefan Richter
-=====-==-=- -==- =-==-
http://arcgraph.de/sr/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] firewire: core: no need to track irq flags in bm_work
  2010-06-22 11:43   ` Stefan Richter
@ 2010-06-22 14:26     ` Philippe De Muyter
  2010-06-22 18:39       ` Stefan Richter
  0 siblings, 1 reply; 6+ messages in thread
From: Philippe De Muyter @ 2010-06-22 14:26 UTC (permalink / raw)
  To: Stefan Richter; +Cc: linux1394-devel, linux-kernel

Hello Stephan,

On Tue, Jun 22, 2010 at 01:43:26PM +0200, Stefan Richter wrote:
> Philippe De Muyter wrote:
> > On Mon, Jun 21, 2010 at 11:23:52PM +0200, Stefan Richter wrote:
> >> This is a workqueue job and always entered with IRQs enabled.
> > 
> > did you mean 'disabled' ?
> 
> I meant enabled.
> 
> [...]
> >> @@ -247,10 +246,10 @@ static void fw_card_bm_work(struct work_
> >>  	bool root_device_is_cmc;
> >>  	bool irm_is_1394_1995_only;
> >>  
> >> -	spin_lock_irqsave(&card->lock, flags);
> >> +	spin_lock_irq(&card->lock);
> 
>   - spin_lock + spin_unlock don't influence whether IRQs on the current
>     CPU are on or off.
> 
>   - spin_lock_irq + spin_unlock_irq always switch IRQs on the current
>     CPU off and back on.  This is necessary if the lock could also be
>     taken by an IRQ handler.  (Well, card->lock is actually only taken
>     by process contexts and by tasklets.  Seems we could switch to
>     spin_lock_bh + spin_unlock_bh for card->lock everywhere in the
>     firewire stack.)
> 
>   - spin_lock_irqsave + spin_unlock_irqrestore switch IRQs on the
>     current CPU off and back on only if used while IRQs are enabled;
>     if used while local IRQs are already disabled they leave them
>     disabled.
> 
> http://lwn.net/images/pdf/LDD3/ch05.pdf#page=14
> 
> Therefore some people prefer to use the safer spin_lock_irqsave()/
> spin_unlock_irqrestore() everywhere.  However, their downsides are the
> need to track IRQ state flags, and --- subjectively --- that their
> appearance in the code could create an impression to a casual reader
> that this code was meant to be able to run in IRQs-on context as well as
> in IRQs-off context.  fw_card_bm_work() however definitely requires to
> be called with IRQs on, notably to be able to wait for IEEE 1394
> transactions to complete.

Thanks for the clear explanation, and sorry for your wasted time.

Philippe

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] firewire: core: no need to track irq flags in bm_work
  2010-06-22 14:26     ` Philippe De Muyter
@ 2010-06-22 18:39       ` Stefan Richter
  0 siblings, 0 replies; 6+ messages in thread
From: Stefan Richter @ 2010-06-22 18:39 UTC (permalink / raw)
  To: Philippe De Muyter; +Cc: linux1394-devel, linux-kernel

Philippe De Muyter wrote:
> Thanks for the clear explanation, and sorry for your wasted time.

Oh, no problem.  It helps that somebody actually looks at these patches
and asks if something looks illogical.  I for one am on record to have
posted patches that got things backwards. :-)
-- 
Stefan Richter
-=====-==-=- -==- =-==-
http://arcgraph.de/sr/

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2010-06-22 18:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-21 21:23 [PATCH 1/2] firewire: core: no need to track irq flags in bm_work Stefan Richter
2010-06-21 21:24 ` [PATCH 2/2] firewire: cdev: fix fw_cdev_event_bus_reset.bm_node_id Stefan Richter
2010-06-22  9:36 ` [PATCH 1/2] firewire: core: no need to track irq flags in bm_work Philippe De Muyter
2010-06-22 11:43   ` Stefan Richter
2010-06-22 14:26     ` Philippe De Muyter
2010-06-22 18:39       ` Stefan Richter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®