Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 28 Nov 2020, Ahmed S. Darwish wrote:

On Sat, Nov 28, 2020 at 08:48:00AM +1100, Finn Thain wrote:

On Sat, 28 Nov 2020, Finn Thain wrote:


diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index d654a6cc4162..739def70cffb 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -223,7 +223,10 @@ static int NCR5380_poll_politely2(struct NCR5380_hostdata *hostdata,
 		cpu_relax();
 	} while (n--);

-	if (irqs_disabled() || in_interrupt())
+	/* We can't sleep when local irqs are disabled and callers ensure
+	 * that local irqs are disabled whenever we can't sleep.
+	 */
+	if (irqs_disabled())
 		return -ETIMEDOUT;

 	/* Repeatedly sleep for 1 ms until deadline */


Michael, Andreas, would you please confirm that this is workable on Atari?
The driver could sleep when IPL == 2 because arch_irqs_disabled_flags()
would return false (on Atari). I'm wondering whether that would deadlock.

Please re-check the commit log:

  "Linus clearly requested that code which changes behaviour depending
   on context should either be separated, or the context be explicitly
   conveyed in an argument passed by the caller."


Yes, I knew about the discussion around the issues with preempt_count() 
and CONFIG_PREEMPT. And I don't have any problem with removing 
in_interrupt(), as you can see from my patch.

So, sorry, drivers shouldn't do context-dependent dances anymore.


I don't know what is meant by 'context-dependent'. I suspect that it's 
left ill-defined because there are many cases where global state is 
needed, such as those mentioned in the thread you cited, like the 
memalloc_no*() calls. See also, in_compat_syscall().

For more context (no pun intended), please check the thread mentioned in
the cover letter, and also below message:

  https://lkml.kernel.org/r/CAKMK7uHAk9-Vy2cof0ws=DrcD52GHiCDiyHbjLd19CgpBU2rKQ@xxxxxxxxxxxxxx


Are you also planning to remove spin_lock_irqsave/restore() and replace 
these with spin_lock_irq/unlock_irq()? And if not, why do you object to 
irqs_disabled()?

Please also compare your patch and mine with regard to stack usage, 
readability and code size. Also consider that adding a new argument to all 
those functions creates new opportunities for mistakes in new callers.

Kind regards,

--
Ahmed S. Darwish
Linutronix GmbH




[Index of Archives]     [Video for Linux]     [Yosemite News]     [Linux S/390]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux