[PATCH 00/16] sleep_on removal, second try

[PATCH 00/16] sleep_on removal, second try

[Arnd Bergmann]

It's been a while since the first submission of these patches,
but a lot of them have made it into linux-next already, so here
is the stuff that is not merged yet, hopefully addressing all
the comments.

Geert and Michael: the I was expecting the ataflop and atari_scsi
patches to be merged already, based on earlier discussion.
Can you apply them to the linux-m68k tree, or do you prefer
them to go through the scsi and block maintainers?

Jens: I did not get any comments for the DAC960 and swim3 patches,
I assume they are good to go in. Please merge.

Hans and Mauro: As I commented on the old thread, I thought the
four media patches were on their way. I have addressed the one
comment that I missed earlier now, and used Hans' version for
the two patches he changed. Please merge or let me know the status
if you have already put them in some tree, but not yet into linux-next

Greg or Andrew: The parport subsystem is orphaned unfortunately,
can one of you pick up that patch?

Davem: The two ATM patches got acks, but I did not hear back from
Karsten regarding the ISDN patches. Can you pick up all six, or
should we wait for comments about the ISDN patches?

	Arnd

Cc: Andrew Morton <akpm@osdl.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Karsten Keil <isdn@linux-pingi.de>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: Michael Schmitz <schmitz@biophys.uni-duesseldorf.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-atm-general@lists.sourceforge.net
Cc: linux-media@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
Cc: netdev@vger.kernel.org

Arnd Bergmann (16):
  ataflop: fix sleep_on races
  scsi: atari_scsi: fix sleep_on race
  DAC960: remove sleep_on usage
  swim3: fix interruptible_sleep_on race
  [media] omap_vout: avoid sleep_on race
  [media] usbvision: drop unused define USBVISION_SAY_AND_WAIT
  [media] radio-cadet: avoid interruptible_sleep_on race
  [media] arv: fix sleep_on race
  parport: fix interruptible_sleep_on race
  atm: nicstar: remove interruptible_sleep_on_timeout
  atm: firestream: fix interruptible_sleep_on race
  isdn: pcbit: fix interruptible_sleep_on race
  isdn: hisax/elsa: fix sleep_on race in elsa FSM
  isdn: divert, hysdn: fix interruptible_sleep_on race
  isdn: fix multiple sleep_on races
  sched: remove sleep_on() and friends

 Documentation/DocBook/kernel-hacking.tmpl    | 10 ------
 drivers/atm/firestream.c                     |  4 +--
 drivers/atm/nicstar.c                        | 13 ++++----
 drivers/block/DAC960.c                       | 34 ++++++++++----------
 drivers/block/ataflop.c                      | 16 +++++-----
 drivers/block/swim3.c                        | 18 ++++++-----
 drivers/isdn/divert/divert_procfs.c          |  7 +++--
 drivers/isdn/hisax/elsa.c                    |  9 ++++--
 drivers/isdn/hisax/elsa_ser.c                |  3 +-
 drivers/isdn/hysdn/hysdn_proclog.c           |  7 +++--
 drivers/isdn/i4l/isdn_common.c               | 13 +++++---
 drivers/isdn/pcbit/drv.c                     |  6 ++--
 drivers/media/platform/arv.c                 |  6 ++--
 drivers/media/platform/omap/omap_vout_vrfb.c |  3 +-
 drivers/media/radio/radio-cadet.c            | 46 ++++++++++++++++------------
 drivers/media/usb/usbvision/usbvision.h      |  8 -----
 drivers/parport/share.c                      |  3 +-
 drivers/scsi/atari_scsi.c                    | 12 ++++++--
 include/linux/wait.h                         | 11 -------
 kernel/sched/core.c                          | 46 ----------------------------
 20 files changed, 113 insertions(+), 162 deletions(-)

-- 
1.8.3.2

[PATCH 02/16] scsi: atari_scsi: fix sleep_on race

[Arnd Bergmann]

sleep_on is known broken and going away. The atari_scsi driver is one of
two remaining users in the falcon_get_lock() function, which is a rather
crazy piece of code. This does not attempt to fix the driver's locking
scheme in general, but at least prevents falcon_get_lock from going to
sleep when no other thread holds the same lock or tries to get it,
and we no longer schedule with irqs disabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/atari_scsi.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a3e6c8a..b33ce34 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -90,6 +90,7 @@
 #include <linux/init.h>
 #include <linux/nvram.h>
 #include <linux/bitops.h>
+#include <linux/wait.h>
 
 #include <asm/setup.h>
 #include <asm/atarihw.h>
@@ -549,8 +550,10 @@ static void falcon_get_lock(void)
 
 	local_irq_save(flags);
 
-	while (!in_irq() && falcon_got_lock && stdma_others_waiting())
-		sleep_on(&falcon_fairness_wait);
+	wait_event_cmd(falcon_fairness_wait,
+		       !in_irq() && falcon_got_lock && stdma_others_waiting(),
+		       local_irq_restore(flags),
+		       local_irq_save(flags));
 
 	while (!falcon_got_lock) {
 		if (in_irq())
@@ -562,7 +565,10 @@ static void falcon_get_lock(void)
 			falcon_trying_lock = 0;
 			wake_up(&falcon_try_wait);
 		} else {
-			sleep_on(&falcon_try_wait);
+			wait_event_cmd(falcon_try_wait,
+				       falcon_got_lock && !falcon_trying_lock,
+				       local_irq_restore(flags),
+				       local_irq_save(flags));
 		}
 	}
 
-- 
1.8.3.2

Re: [PATCH 00/16] sleep_on removal, second try

[Jens Axboe]

On Wed, Feb 26 2014, Arnd Bergmann wrote:
> It's been a while since the first submission of these patches,
> but a lot of them have made it into linux-next already, so here
> is the stuff that is not merged yet, hopefully addressing all
> the comments.
> 
> Geert and Michael: the I was expecting the ataflop and atari_scsi
> patches to be merged already, based on earlier discussion.
> Can you apply them to the linux-m68k tree, or do you prefer
> them to go through the scsi and block maintainers?
> 
> Jens: I did not get any comments for the DAC960 and swim3 patches,
> I assume they are good to go in. Please merge.

Picked up 1, 3, 4 of the patches. Thanks Arnd.

-- 
Jens Axboe

Re: [PATCH 00/16] sleep_on removal, second try

[Michael Schmitz]

Arnd,


> It's been a while since the first submission of these patches,
> but a lot of them have made it into linux-next already, so here
> is the stuff that is not merged yet, hopefully addressing all
> the comments.
>
> Geert and Michael: the I was expecting the ataflop and atari_scsi
> patches to be merged already, based on earlier discussion.
> Can you apply them to the linux-m68k tree, or do you prefer
> them to go through the scsi and block maintainers?

Not sure what we decided to do - I'd prefer to double-check the latest 
ones first, but I'd be OK with these to go via m68k.

Maybe Geert waits for acks from linux-scsi and linux-block? (The rest 
of my patches to Atari SCSI still awaits comment there.)

Geert?

Regards,

	Michael

> Jens: I did not get any comments for the DAC960 and swim3 patches,
> I assume they are good to go in. Please merge.
>
> Hans and Mauro: As I commented on the old thread, I thought the
> four media patches were on their way. I have addressed the one
> comment that I missed earlier now, and used Hans' version for
> the two patches he changed. Please merge or let me know the status
> if you have already put them in some tree, but not yet into linux-next
>
> Greg or Andrew: The parport subsystem is orphaned unfortunately,
> can one of you pick up that patch?
>
> Davem: The two ATM patches got acks, but I did not hear back from
> Karsten regarding the ISDN patches. Can you pick up all six, or
> should we wait for comments about the ISDN patches?
>
> 	Arnd
>
> Cc: Andrew Morton <akpm@osdl.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Karsten Keil <isdn@linux-pingi.de>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: Michael Schmitz <schmitz@biophys.uni-duesseldorf.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-atm-general@lists.sourceforge.net
> Cc: linux-media@vger.kernel.org
> Cc: linux-scsi@vger.kernel.org
> Cc: netdev@vger.kernel.org
>
> Arnd Bergmann (16):
>   ataflop: fix sleep_on races
>   scsi: atari_scsi: fix sleep_on race
>   DAC960: remove sleep_on usage
>   swim3: fix interruptible_sleep_on race
>   [media] omap_vout: avoid sleep_on race
>   [media] usbvision: drop unused define USBVISION_SAY_AND_WAIT
>   [media] radio-cadet: avoid interruptible_sleep_on race
>   [media] arv: fix sleep_on race
>   parport: fix interruptible_sleep_on race
>   atm: nicstar: remove interruptible_sleep_on_timeout
>   atm: firestream: fix interruptible_sleep_on race
>   isdn: pcbit: fix interruptible_sleep_on race
>   isdn: hisax/elsa: fix sleep_on race in elsa FSM
>   isdn: divert, hysdn: fix interruptible_sleep_on race
>   isdn: fix multiple sleep_on races
>   sched: remove sleep_on() and friends
>
>  Documentation/DocBook/kernel-hacking.tmpl    | 10 ------
>  drivers/atm/firestream.c                     |  4 +--
>  drivers/atm/nicstar.c                        | 13 ++++----
>  drivers/block/DAC960.c                       | 34 ++++++++++----------
>  drivers/block/ataflop.c                      | 16 +++++-----
>  drivers/block/swim3.c                        | 18 ++++++-----
>  drivers/isdn/divert/divert_procfs.c          |  7 +++--
>  drivers/isdn/hisax/elsa.c                    |  9 ++++--
>  drivers/isdn/hisax/elsa_ser.c                |  3 +-
>  drivers/isdn/hysdn/hysdn_proclog.c           |  7 +++--
>  drivers/isdn/i4l/isdn_common.c               | 13 +++++---
>  drivers/isdn/pcbit/drv.c                     |  6 ++--
>  drivers/media/platform/arv.c                 |  6 ++--
>  drivers/media/platform/omap/omap_vout_vrfb.c |  3 +-
>  drivers/media/radio/radio-cadet.c            | 46 
> ++++++++++++++++------------
>  drivers/media/usb/usbvision/usbvision.h      |  8 -----
>  drivers/parport/share.c                      |  3 +-
>  drivers/scsi/atari_scsi.c                    | 12 ++++++--
>  include/linux/wait.h                         | 11 -------
>  kernel/sched/core.c                          | 46 
> ----------------------------
>  20 files changed, 113 insertions(+), 162 deletions(-)
>
> -- 
> 1.8.3.2

Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Arnd Bergmann wrote:
> sleep_on is known broken and going away. The atari_scsi driver is one of
> two remaining users in the falcon_get_lock() function, which is a rather
> crazy piece of code. This does not attempt to fix the driver's locking
> scheme in general, but at least prevents falcon_get_lock from going to
> sleep when no other thread holds the same lock or tries to get it,
> and we no longer schedule with irqs disabled.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Michael Schmitz <schmitzmic@gmail.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/atari_scsi.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
> index a3e6c8a..b33ce34 100644
> --- a/drivers/scsi/atari_scsi.c
> +++ b/drivers/scsi/atari_scsi.c
> @@ -90,6 +90,7 @@
>  #include <linux/init.h>
>  #include <linux/nvram.h>
>  #include <linux/bitops.h>
> +#include <linux/wait.h>
>  
>  #include <asm/setup.h>
>  #include <asm/atarihw.h>
> @@ -549,8 +550,10 @@ static void falcon_get_lock(void)
>  
>  	local_irq_save(flags);
>  
> -	while (!in_irq() && falcon_got_lock && stdma_others_waiting())
> -		sleep_on(&falcon_fairness_wait);
> +	wait_event_cmd(falcon_fairness_wait,
> +		       !in_irq() && falcon_got_lock && stdma_others_waiting(),
> +		       local_irq_restore(flags),
> +		       local_irq_save(flags));
>  
>  	while (!falcon_got_lock) {
>  		if (in_irq())
> @@ -562,7 +565,10 @@ static void falcon_get_lock(void)
>  			falcon_trying_lock = 0;
>  			wake_up(&falcon_try_wait);
>  		} else {
> -			sleep_on(&falcon_try_wait);
> +			wait_event_cmd(falcon_try_wait,
> +				       falcon_got_lock && !falcon_trying_lock,
> +				       local_irq_restore(flags),
> +				       local_irq_save(flags));
>  		}
>  	}
>  
>   
Nack - the completion condition in the first hunk has its logic 
reversed. Try this instead (while() loops while condition true, do {} 
until () loops while condition false, no?)

I'm 99% confident I had tested your current version of the patch before 
and found it still attempts to schedule while in interrupt. I can retest 
if you prefer, but that'll have to wait a few days.

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a3e6c8a..cc1b013 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -90,6 +90,7 @@
 #include <linux/init.h>
 #include <linux/nvram.h>
 #include <linux/bitops.h>
+#include <linux/wait.h>
 
 #include <asm/setup.h>
 #include <asm/atarihw.h>
@@ -549,8 +550,10 @@ static void falcon_get_lock(void)
 
        local_irq_save(flags);
 
-       while (!in_irq() && falcon_got_lock && stdma_others_waiting())
-               sleep_on(&falcon_fairness_wait);
+       wait_event_cmd(falcon_fairness_wait,
+               in_irq() || !falcon_got_lock || !stdma_others_waiting(),
+               local_irq_restore(flags),
+               local_irq_save(flags));
 
        while (!falcon_got_lock) {
                if (in_irq())
@@ -562,7 +565,10 @@ static void falcon_get_lock(void)
                        falcon_trying_lock = 0;
                        wake_up(&falcon_try_wait);
                } else {
-                       sleep_on(&falcon_try_wait);
+                       wait_event_cmd(falcon_try_wait,
+                               falcon_got_lock && !falcon_trying_lock,
+                               local_irq_restore(flags),
+                               local_irq_save(flags));
                }
        }
 

Cheers,

    Michael

Re: [PATCH 00/16] sleep_on removal, second try

[Geert Uytterhoeven]

Hi Michael, Arnd,

On Thu, Feb 27, 2014 at 7:37 AM, Michael Schmitz
<schmitz@biophys.uni-duesseldorf.de> wrote:
>> It's been a while since the first submission of these patches,
>> but a lot of them have made it into linux-next already, so here
>> is the stuff that is not merged yet, hopefully addressing all
>> the comments.
>>
>> Geert and Michael: the I was expecting the ataflop and atari_scsi
>> patches to be merged already, based on earlier discussion.
>> Can you apply them to the linux-m68k tree, or do you prefer
>> them to go through the scsi and block maintainers?
>
> Not sure what we decided to do - I'd prefer to double-check the latest ones
> first, but I'd be OK with these to go via m68k.
>
> Maybe Geert waits for acks from linux-scsi and linux-block? (The rest of my
> patches to Atari SCSI still awaits comment there.)

I was waiting for a final confirmation. I was under the impression some rework
was needed, and seeing Michael's NAK confirms that.

I'd be glad to take them through the m68k tree (for 3.15), once they have
received testing and Michael's ACK. Or the block resp. SCSI maintainers can
take them if they prefer, which apparently already happened for 01/16.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race

[Arnd Bergmann]

On Thursday 27 February 2014, Michael Schmitz wrote:
> Arnd Bergmann wrote:
> >   
> Nack - the completion condition in the first hunk has its logic 
> reversed. Try this instead (while() loops while condition true, do {} 
> until () loops while condition false, no?)

Sorry about messing it up again. I though I had fixed it up the
way you commented when you said it worked.
 
> I'm 99% confident I had tested your current version of the patch before 
> and found it still attempts to schedule while in interrupt. I can retest 
> if you prefer, but that'll have to wait a few days.

I definitely trust you to have the right version, since you did the
testing.

> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
> index a3e6c8a..cc1b013 100644
> --- a/drivers/scsi/atari_scsi.c
> +++ b/drivers/scsi/atari_scsi.c
> @@ -90,6 +90,7 @@
>  #include <linux/init.h>
>  #include <linux/nvram.h>
>  #include <linux/bitops.h>
> +#include <linux/wait.h>
>  
>  #include <asm/setup.h>
>  #include <asm/atarihw.h>
> @@ -549,8 +550,10 @@ static void falcon_get_lock(void)
>  
>         local_irq_save(flags);
>  
> -       while (!in_irq() && falcon_got_lock && stdma_others_waiting())
> -               sleep_on(&falcon_fairness_wait);
> +       wait_event_cmd(falcon_fairness_wait,
> +               in_irq() || !falcon_got_lock || !stdma_others_waiting(),
> +               local_irq_restore(flags),
> +               local_irq_save(flags));
>  
>         while (!falcon_got_lock) {
>                 if (in_irq())

Yes, by inspection your version looks correct and mine looks wrong.
I had figured this out before, just sent the wrong version.

> @@ -562,7 +565,10 @@ static void falcon_get_lock(void)
>                         falcon_trying_lock = 0;
>                         wake_up(&falcon_try_wait);
>                 } else {
> -                       sleep_on(&falcon_try_wait);
> +                       wait_event_cmd(falcon_try_wait,
> +                               falcon_got_lock && !falcon_trying_lock,
> +                               local_irq_restore(flags),
> +                               local_irq_save(flags));
>                 }

I did correct this part compared to my first patch, but forgot
to change the other hunk.

Can you send your version of the patch to Geert for inclusion?
That way I don't have the danger of missing another negation.
This code is clearly too weird to rely on inspection alone and
we know that your version was working when you last tested it.

	Arnd

Re: [PATCH 00/16] sleep_on removal, second try

[Karsten Keil]

Am 26.02.2014 12:01, schrieb Arnd Bergmann:
> It's been a while since the first submission of these patches,
> but a lot of them have made it into linux-next already, so here
> is the stuff that is not merged yet, hopefully addressing all
> the comments.
> 
> Geert and Michael: the I was expecting the ataflop and atari_scsi
> patches to be merged already, based on earlier discussion.
> Can you apply them to the linux-m68k tree, or do you prefer
> them to go through the scsi and block maintainers?
> 
> Jens: I did not get any comments for the DAC960 and swim3 patches,
> I assume they are good to go in. Please merge.
> 
> Hans and Mauro: As I commented on the old thread, I thought the
> four media patches were on their way. I have addressed the one
> comment that I missed earlier now, and used Hans' version for
> the two patches he changed. Please merge or let me know the status
> if you have already put them in some tree, but not yet into linux-next
> 
> Greg or Andrew: The parport subsystem is orphaned unfortunately,
> can one of you pick up that patch?
> 
> Davem: The two ATM patches got acks, but I did not hear back from
> Karsten regarding the ISDN patches. Can you pick up all six, or
> should we wait for comments about the ISDN patches?
>


Ack on the ISDN stuff (12,13,14,15)

Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Hello Arnd,
> On Thursday 27 February 2014, Michael Schmitz wrote:
>   
>> Arnd Bergmann wrote:
>>     
>>>   
>>>       
>> Nack - the completion condition in the first hunk has its logic 
>> reversed. Try this instead (while() loops while condition true, do {} 
>> until () loops while condition false, no?)
>>     
>
> Sorry about messing it up again. I though I had fixed it up the
> way you commented when you said it worked.
>  
>   
>> I'm 99% confident I had tested your current version of the patch before 
>> and found it still attempts to schedule while in interrupt. I can retest 
>> if you prefer, but that'll have to wait a few days.
>>     
>
> I definitely trust you to have the right version, since you did the
> testing.
>   

I'm glad I double checked, since there's one other error left in my 
correction to your patch below:

The in_irq() condition is not sufficient, we need in_interrupt() there. 
This has somehow slipped into a related patch sent to linux-scsi, so 
I'll have to refactor the lot. Bugger.

I'll resend the correct version via Geert.

>   
>> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
>> index a3e6c8a..cc1b013 100644
>> --- a/drivers/scsi/atari_scsi.c
>> +++ b/drivers/scsi/atari_scsi.c
>> @@ -90,6 +90,7 @@
>>  #include <linux/init.h>
>>  #include <linux/nvram.h>
>>  #include <linux/bitops.h>
>> +#include <linux/wait.h>
>>  
>>  #include <asm/setup.h>
>>  #include <asm/atarihw.h>
>> @@ -549,8 +550,10 @@ static void falcon_get_lock(void)
>>  
>>         local_irq_save(flags);
>>  
>> -       while (!in_irq() && falcon_got_lock && stdma_others_waiting())
>> -               sleep_on(&falcon_fairness_wait);
>> +       wait_event_cmd(falcon_fairness_wait,
>> +               in_irq() || !falcon_got_lock || !stdma_others_waiting(),
>> +               local_irq_restore(flags),
>> +               local_irq_save(flags));
>>  
>>         while (!falcon_got_lock) {
>>                 if (in_irq())
>>     
>
> Yes, by inspection your version looks correct and mine looks wrong.
> I had figured this out before, just sent the wrong version.
>   

These things happen if you bother fixing other people's weird code :-)
And as I mentioned above, I missed another detail myself

>   
>> @@ -562,7 +565,10 @@ static void falcon_get_lock(void)
>>                         falcon_trying_lock = 0;
>>                         wake_up(&falcon_try_wait);
>>                 } else {
>> -                       sleep_on(&falcon_try_wait);
>> +                       wait_event_cmd(falcon_try_wait,
>> +                               falcon_got_lock && !falcon_trying_lock,
>> +                               local_irq_restore(flags),
>> +                               local_irq_save(flags));
>>                 }
>>     
>
> I did correct this part compared to my first patch, but forgot
> to change the other hunk.
>
> Can you send your version of the patch to Geert for inclusion?
> That way I don't have the danger of missing another negation.
> This code is clearly too weird to rely on inspection alone and
> we know that your version was working when you last tested it.
>   

Will do - I'll CC: you in so you can ACK the patch if Geert needs 
convincing.

Cheers,

    Michael