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

From: Finn Thain <fthain@telegraphics.com.au>
To: "Ahmed S. Darwish" <a.darwish@linutronix.de>
Cc: Michael Schmitz <schmitzmic@gmail.com>,
	Andreas Schwab <schwab@linux-m68k.org>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	linux-scsi@vger.kernel.org,
	GR-QLogic-Storage-Upstream@marvell.com,
	Hannes Reinecke <hare@kernel.org>,
	Jack Wang <jinpu.wang@cloud.ionos.com>,
	John Garry <john.garry@huawei.com>,
	linux-m68k@lists.linux-m68k.org,
	Manish Rangankar <mrangankar@marvell.com>,
	MPT-FusionLinux.pdl@broadcom.com,
	Nilesh Javali <njavali@marvell.com>,
	Sathya Prakash <sathya.prakash@broadcom.com>,
	Sreekanth Reddy <sreekanth.reddy@broadcom.com>,
	Suganath Prabu Subramani  <suganath-prabu.subramani@broadcom.com>,
	Vikram Auradkar <auradkar@google.com>,
	Xiang Chen <chenxiang66@hisilicon.com>,
	Xiaofei Tan <tanxiaofei@huawei.com>,
	"James E . J . Bottomley" <jejb@linux.ibm.com>,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().
Date: Mon, 30 Nov 2020 11:21:01 +1100 (AEDT)	[thread overview]
Message-ID: <alpine.LNX.2.23.453.2011301118520.6@nippy.intranet> (raw)
In-Reply-To: <X8H8M2AM0hfMX7aj@lx-t490>

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@mail.gmail.com
> 

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
> 