Re: [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 #include #include +#include #include #include @@ -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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race
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 > #include > #include > +#include > > #include > #include > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race
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 Cc: Michael Schmitz Cc: Geert Uytterhoeven Cc: James E.J. Bottomley 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 #include #include +#include #include #include @@ -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 #include #include +#include #include #include @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/16] scsi: atari_scsi: fix sleep_on race
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 Cc: Michael Schmitz Cc: Geert Uytterhoeven Cc: James E.J. Bottomley 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 #include #include +#include #include #include @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html