* [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 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.