[PATCH] Provide refrigerator support for irda

[PATCH] Provide refrigerator support for irda

[Neil Brown]

Hi Jean.
 2.5.73/MAINTAINERS: IRDA SUBSYSTEM
doesn't have an "M:" field, so I'm guessing that the P: field is close
enough.

Without this patch, kIrDAd prevents my notebook from entering suspend
mode.

NeilBrown


 ----------- Diffstat output ------------
 ./drivers/net/irda/sir_kthread.c |    3 +++
 1 files changed, 3 insertions(+)

diff ./drivers/net/irda/sir_kthread.c~current~ ./drivers/net/irda/sir_kthread.c
--- ./drivers/net/irda/sir_kthread.c~current~	2003-06-25 11:50:36.000000000 +1000
+++ ./drivers/net/irda/sir_kthread.c	2003-06-25 11:51:02.000000000 +1000
@@ -166,6 +166,9 @@ static int irda_thread(void *startup)
 			set_task_state(current, TASK_RUNNING);
 		remove_wait_queue(&irda_rq_queue.kick, &wait);
 
+		if (current->flags & PF_FREEZE)
+			refrigerator(PF_IOTHREAD);
+
 		run_irda_queue();
 	}
 

Re: [PATCH] Provide refrigerator support for irda

[Jean Tourrilhes]

On Wed, Jun 25, 2003 at 11:56:53AM +1000, Neil Brown wrote:
> 
> Hi Jean.
>  2.5.73/MAINTAINERS: IRDA SUBSYSTEM
> doesn't have an "M:" field, so I'm guessing that the P: field is close
> enough.
> 
> Without this patch, kIrDAd prevents my notebook from entering suspend
> mode.
> 
> NeilBrown

	Wow ! I knew about microwave for 802.11b, but not about
refrigerator for IrDA.
	The man for sir_kthread is Martin Diehl. He is much more
intimate with kernel tasks than me, and he has other sir_dev updates
in the pipeline.
	Martin ?

	Thanks !

	Jean

>  ----------- Diffstat output ------------
>  ./drivers/net/irda/sir_kthread.c |    3 +++
>  1 files changed, 3 insertions(+)
> 
> diff ./drivers/net/irda/sir_kthread.c~current~ ./drivers/net/irda/sir_kthread.c
> --- ./drivers/net/irda/sir_kthread.c~current~	2003-06-25 11:50:36.000000000 +1000
> +++ ./drivers/net/irda/sir_kthread.c	2003-06-25 11:51:02.000000000 +1000
> @@ -166,6 +166,9 @@ static int irda_thread(void *startup)
>  			set_task_state(current, TASK_RUNNING);
>  		remove_wait_queue(&irda_rq_queue.kick, &wait);
>  
> +		if (current->flags & PF_FREEZE)
> +			refrigerator(PF_IOTHREAD);
> +
>  		run_irda_queue();
>  	}
>  

Re: [PATCH] Provide refrigerator support for irda

[Martin Diehl]

On Tue, 24 Jun 2003, Jean Tourrilhes wrote:

> On Wed, Jun 25, 2003 at 11:56:53AM +1000, Neil Brown wrote:
> > 
> > Without this patch, kIrDAd prevents my notebook from entering suspend
> > mode.

Are you talking about normal apm or acpi suspend or this swsusp thing?

> 	Wow ! I knew about microwave for 802.11b, but not about
> refrigerator for IrDA.

Inclined to say "me too".

> 	The man for sir_kthread is Martin Diehl. He is much more
> intimate with kernel tasks than me, and he has other sir_dev updates
> in the pipeline.

Admittedly I didn't care about swsusp so far. Given the big fat warning on 
top of Documentation/swsusp.txt I would not even try and i personally 
don't see what I would miss without swsusp.

But of course, if we can make all parties happy, why not.

> > +		if (current->flags & PF_FREEZE)
> > +			refrigerator(PF_IOTHREAD);
> > +

I've tried to find some documentation about this - without success. So I 
have no idea why this might be needed and what it will do. Grepping for 
other kernel threads it looks like most of them use the same two lines.
OTOH the irda thread is very similar to keventd's workqueue which do not
use this. Not sure about the reason.

Well, I'm thinking about making the irda thread using workqueue anyway, in 
which case the issue would either disappear or get shifted to 
kernel/workqueue.c.

For the meantime, I think we should apply this if somebody would explain 
what's going on here.

Martin

Re: [PATCH] Provide refrigerator support for irda

[Pavel Machek]

Hi!

> > > Without this patch, kIrDAd prevents my notebook from entering suspend
> > > mode.
> 
> Are you talking about normal apm or acpi suspend or this swsusp
> thing?

ACPI and swsusp uses same mechanism. apm is different.

> > 	Wow ! I knew about microwave for 802.11b, but not about
> > refrigerator for IrDA.
> 
> Inclined to say "me too".

:-).

> > 	The man for sir_kthread is Martin Diehl. He is much more
> > intimate with kernel tasks than me, and he has other sir_dev updates
> > in the pipeline.
> 
> Admittedly I didn't care about swsusp so far. Given the big fat warning on 
> top of Documentation/swsusp.txt I would not even try and i personally 
> don't see what I would miss without swsusp.

That's okay... Just support your DMA-ing devices are supported.

> But of course, if we can make all parties happy, why not.
> 
> > > +		if (current->flags & PF_FREEZE)
> > > +			refrigerator(PF_IOTHREAD);
> > > +
> 
> I've tried to find some documentation about this - without success. So I 
> have no idea why this might be needed and what it will do. Grepping for 
> other kernel threads it looks like most of them use the same two lines.
> OTOH the irda thread is very similar to keventd's workqueue which do not
> use this. Not sure about the reason.

> Well, I'm thinking about making the irda thread using workqueue anyway, in 
> which case the issue would either disappear or get shifted to 
> kernel/workqueue.c.

> For the meantime, I think we should apply this if somebody would explain 
> what's going on here.

We need kernel thread to sleep at defined place. Process running
userspace can be stopped any time, but processes running in kernel can
be only stopped at defined places, to avoid unexpected surprises.

If it is possible to sleep at place "a", and no locks needed for
suspend are held at "a", inserting 

	if (current->flags & PF_FREEZE)
		refrigerator(PF_IOTHREAD);

is the right thing to do.
								Pavel

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [PATCH] Provide refrigerator support for irda

[Martin Diehl]

On Wed, 25 Jun 2003, Pavel Machek wrote:

> > Admittedly I didn't care about swsusp so far. Given the big fat warning on 
> > top of Documentation/swsusp.txt I would not even try and i personally 
> > don't see what I would miss without swsusp.
> 
> That's okay... Just support your DMA-ing devices are supported.

No matter whether it would be ISA-DMA or PCI busmastering I guess?

Not sure whether we can claim most irda drivers ready for swsusp then.
vlsi_ir should be fine and for irda-usb I assume it would be up to the
hcd - but don't know about the others...

> > For the meantime, I think we should apply this if somebody would explain 
> > what's going on here.
> 
> We need kernel thread to sleep at defined place. Process running
> userspace can be stopped any time, but processes running in kernel can
> be only stopped at defined places, to avoid unexpected surprises.

Ok, thanks for explaining!

> If it is possible to sleep at place "a", and no locks needed for
> suspend are held at "a", inserting 
> 
> 	if (current->flags & PF_FREEZE)
> 		refrigerator(PF_IOTHREAD);
> 
> is the right thing to do.

Ok, so the proposed patch is doing the right thing and should be applied.

Pavel, two more questions:

* For a kernel compiled with CONFIG_SWSUSP=n, may I assume PF_FREEZE would 
  never be set or at least refrigerator would be noop?

* Isn't there a race on SMP (and probably UP with CONFIG_PREEMPT) when 
  PF_FREEZE gets modified by another cpu right after the test?

Thanks
Martin

Re: [PATCH] Provide refrigerator support for irda

[Pavel Machek]

Hi!

> > > Admittedly I didn't care about swsusp so far. Given the big fat warning on 
> > > top of Documentation/swsusp.txt I would not even try and i personally 
> > > don't see what I would miss without swsusp.
> > 
> > That's okay... Just support your DMA-ing devices are supported.
> 
> No matter whether it would be ISA-DMA or PCI busmastering I guess?

Any DMA is a problem.

> Not sure whether we can claim most irda drivers ready for swsusp then.
> vlsi_ir should be fine and for irda-usb I assume it would be up to the
> hcd - but don't know about the others...

Well, at least SIR should work as it works over normal serial, IIRC.

> > If it is possible to sleep at place "a", and no locks needed for
> > suspend are held at "a", inserting 
> > 
> > 	if (current->flags & PF_FREEZE)
> > 		refrigerator(PF_IOTHREAD);
> > 
> > is the right thing to do.
> 
> Ok, so the proposed patch is doing the right thing and should be applied.
> 
> Pavel, two more questions:
> 
> * For a kernel compiled with CONFIG_SWSUSP=n, may I assume PF_FREEZE would 
>   never be set or at least refrigerator would be noop?

PF_FREEZE is 0 so if() never succeeds.

> * Isn't there a race on SMP (and probably UP with CONFIG_PREEMPT) when 
>   PF_FREEZE gets modified by another cpu right after the test?

Only task itself ever clears its PF_FREEZE. That means there should be
no problem.
									Pavel

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]