RE: Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI drivers

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Finn Thain [mailto:fthain@xxxxxxxxxxxxxxxxxxx]
> Sent: Friday, February 12, 2021 1:09 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>
> Cc: tanxiaofei <tanxiaofei@xxxxxxxxxx>; jejb@xxxxxxxxxxxxx;
> martin.petersen@xxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linuxarm@xxxxxxxxxxxxx;
> linux-m68k@xxxxxxxxxxxxxxx
> Subject: RE: Re: [PATCH for-next 00/32] spin lock usage optimization for SCSI
> drivers
> 
> On Fri, 12 Feb 2021, Song Bao Hua (Barry Song) wrote:
> 
> >
> > > -----Original Message-----
> > > From: Finn Thain [mailto:fthain@xxxxxxxxxxxxxxxxxxx]
> > > Sent: Friday, February 12, 2021 12:57 PM
> > > To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>
> > > Cc: tanxiaofei <tanxiaofei@xxxxxxxxxx>; jejb@xxxxxxxxxxxxx;
> > > martin.petersen@xxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; linuxarm@xxxxxxxxxxxxx;
> > > linux-m68k@xxxxxxxxxxxxxxx
> > > Subject: RE: Re: [PATCH for-next 00/32] spin lock usage optimization for
> SCSI
> > > drivers
> > >
> > >
> > > On Thu, 11 Feb 2021, Song Bao Hua (Barry Song) wrote:
> > >
> > > >
> > > > Actually in m68k, I also saw its IRQ entry disabled interrupts by
> > > > ' move	#0x2700,%sr		/* disable intrs */'
> > > >
> > > > arch/m68k/include/asm/entry.h:
> > > >
> > > > .macro SAVE_ALL_SYS
> > > > 	move	#0x2700,%sr		/* disable intrs */
> > > > 	btst	#5,%sp@(2)		/* from user? */
> > > > 	bnes	6f			/* no, skip */
> > > > 	movel	%sp,sw_usp		/* save user sp */
> > > > ...
> > > >
> > > > .macro SAVE_ALL_INT
> > > > 	SAVE_ALL_SYS
> > > > 	moveq	#-1,%d0			/* not system call entry */
> > > > 	movel	%d0,%sp@(PT_OFF_ORIG_D0)
> > > > .endm
> > > >
> > > > arch/m68k/kernel/entry.S:
> > > >
> > > > /* This is the main interrupt handler for autovector interrupts */
> > > >
> > > > ENTRY(auto_inthandler)
> > > > 	SAVE_ALL_INT
> > > > 	GET_CURRENT(%d0)
> > > > 					|  put exception # in d0
> > > > 	bfextu	%sp@(PT_OFF_FORMATVEC){#4,#10},%d0
> > > > 	subw	#VEC_SPUR,%d0
> > > >
> > > > 	movel	%sp,%sp@-
> > > > 	movel	%d0,%sp@-		|  put vector # on stack
> > > > auto_irqhandler_fixup = . + 2
> > > > 	jsr	do_IRQ			|  process the IRQ
> > > > 	addql	#8,%sp			|  pop parameters off stack
> > > > 	jra	ret_from_exception
> > > >
> > > > So my question is that " move	#0x2700,%sr" is actually disabling
> > > > all interrupts? And is m68k actually running irq handlers
> > > > with interrupts disabled?
> > > >
> > >
> > > When sonic_interrupt() executes, the IPL is 2 or 3 (since either IRQ may
> > > be involved). That is, SR & 0x700 is 0x200 or 0x300. The level 3 interrupt
> > > may interrupt execution of the level 2 handler so an irq lock is used to
> > > avoid re-entrance.
> > >
> > > This patch,
> > >
> > > diff --git a/drivers/net/ethernet/natsemi/sonic.c
> > > b/drivers/net/ethernet/natsemi/sonic.c
> > > index d17d1b4f2585..041354647bad 100644
> > > --- a/drivers/net/ethernet/natsemi/sonic.c
> > > +++ b/drivers/net/ethernet/natsemi/sonic.c
> > > @@ -355,6 +355,8 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
> > >          */
> > >         spin_lock_irqsave(&lp->lock, flags);
> > >
> > > +       printk_once(KERN_INFO "%s: %08lx\n", __func__, flags);
> > > +
> > >         status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT;
> > >         if (!status) {
> > >                 spin_unlock_irqrestore(&lp->lock, flags);
> > >
> > > produces this output,
> > >
> > > [    3.800000] sonic_interrupt: 00002300
> >
> > I actually hope you can directly read the register rather than reading
> > a flag which might be a software one not from register.
> >
> 
> Again, the implementation of arch_local_irq_save() may be found in
> arch/m68k/include/asm/irqflags.h

Yes. I have read it. Anyway, I started a discussion in genirq
with you cc-ed:
https://lore.kernel.org/lkml/c46ddb954cfe45d9849c911271d7ec23@xxxxxxxxxxxxx/

And thanks very much for all your efforts to help me understand
M68k. Let's get this clarified thoroughly in genirq level.

In arm, we also have some special high-priority interrupts
which are not NMI but able to preempt normal IRQ. They are
managed by arch-extended APIs rather than common APIs.

Neither arch_irqs_disabled() nor local_irq_disable() API can
access this kind of interrupts. They are using things specific
to ARM like:
local_fiq_disable()
local_fiq_enable()
set_fiq_handler()
disable_fiq()
enable_fiq()
...

so fiq doesn't bother us anyhow in genirq.

> 
> > >
> > > I ran that code in QEMU, but experience shows that Apple hardware works
> > > exactly the same. Please do confirm this for yourself, if you still think
> > > the code and comments in sonic_interrupt are wrong.
> > >
> > > > Best Regards
> > > > Barry
> > > >
> >

Thanks
Barry