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

[Michael Schmitz <schmitzmic@gmail.com>]

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