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

2014-02-28 Thread 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-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

2014-02-27 Thread 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
--
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

2014-02-26 Thread 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 a...@arndb.de
Cc: Michael Schmitz schmitz...@gmail.com
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: James E.J. Bottomley jbottom...@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

--
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

2014-02-26 Thread 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 a...@arndb.de
Cc: Michael Schmitz schmitz...@gmail.com
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: James E.J. Bottomley jbottom...@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

--
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