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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/