lkml.org 
[lkml]   [2014]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race
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



\
 
 \ /
  Last update: 2014-03-01 02:01    [W:0.080 / U:50.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site