Prev: [RFC] [PATCH] BMP085 update
Next: RTC/max6900: use rtc_valid_tm to check returning tm for max6900
From: Philippe De Muyter on 22 Jun 2010 05:40
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(a)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);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Philippe De Muyter on 22 Jun 2010 10:30
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [RFC] [PATCH] BMP085 update
Next: RTC/max6900: use rtc_valid_tm to check returning tm for max6900