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

From: Michael Schmitz <schmitzmic@gmail.com>
To: Finn Thain <fthain@telegraphics.com.au>
Cc: 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>,
	"Ahmed S . Darwish" <a.darwish@linutronix.de>
Subject: Re: [PATCH 12/14] scsi: NCR5380: Remove in_interrupt().
Date: Mon, 30 Nov 2020 15:42:07 +1300	[thread overview]
Message-ID: <81f7776f-2faa-da8b-9941-6490fe606370@gmail.com> (raw)
In-Reply-To: <alpine.LNX.2.23.453.2011301115360.6@nippy.intranet>

Hi Finn,

Am 30.11.2020 um 13:15 schrieb Finn Thain:
> On Sun, 29 Nov 2020, Michael Schmitz wrote:
>
>> Am 28.11.20 um 10:48 schrieb Finn Thain:
>>> 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.
>>
>> Pretty sure this would deadlock when in interrupt context here.
>
> When in interrupt context, irqs_disabled() is true due to
> spinlock_irqsave/restore() in NCR5380_intr().

OK.

> My question was really about what would happen if we sleep with IPL == 2.

All relevant system interrupts are at higher priority (5 or 6). Both 
timer and SCSI / DMA completion interrupt in particular are at IPL 6 and 
won't be blocked when sleeping with IPL == 2. That's what I meant by 
'IPL 2 is perfectly OK' below.

>> Otherwise, IPL 2 is perfectly OK (which is why
>> arch_irqs_disabled_flags() returns false in that case).
>>
>> If you want to be 100% certain, I can give this one a spin.
>>
>
> Please only test it if you think it will work.

With your explanation above, I'm now quite certain your patch will work. 
I've not seen deadlocks in softirq context since you rewrote the driver 
so it will no more sleep waiting for the ST-DMA lock.

Cheers,

	Michael