Re: [PATCH V5 06/14] powerpc/vas: Setup thread IRQ handler per VAS instance

[Michael Neuling <mikey@neuling.org>]

On Sun, 2020-02-09 at 21:17 -0800, Haren Myneni wrote:
> On Fri, 2020-02-07 at 16:57 +1100, Michael Neuling wrote:
> > >  /*
> > > + * Process CRBs that we receive on the fault window.
> > > + */
> > > +irqreturn_t vas_fault_handler(int irq, void *data)
> > > +{
> > > +	struct vas_instance *vinst = data;
> > > +	struct coprocessor_request_block buf, *crb;
> > > +	struct vas_window *window;
> > > +	void *fifo;
> > > +
> > > +	/*
> > > +	 * VAS can interrupt with multiple page faults. So process all
> > > +	 * valid CRBs within fault FIFO until reaches invalid CRB.
> > > +	 * NX updates nx_fault_stamp in CRB and pastes in fault FIFO.
> > > +	 * kernel retrives send window from parition send window ID
> > > +	 * (pswid) in nx_fault_stamp. So pswid should be non-zero and
> > > +	 * use this to check whether CRB is valid.
> > > +	 * After reading CRB entry, it is reset with 0's in fault FIFO.
> > > +	 *
> > > +	 * In case kernel receives another interrupt with different page
> > > +	 * fault and CRBs are processed by the previous handling, will be
> > > +	 * returned from this function when it sees invalid CRB (means 0's).
> > > +	 */
> > > +	do {
> > > +		mutex_lock(&vinst->mutex);
> > 
> > This isn't going to work.
> > 
> > From Documentation/locking/mutex-design.rst
> > 
> >     - Mutexes may not be used in hardware or software interrupt
> >       contexts such as tasklets and timers.
> 
> Initially used kernel thread per VAS instance and later using IRQ
> thread. 
> 
> vas_fault_handler() is IRQ thread function, not IRQ handler. I thought
> we can use mutex_lock() in thread function.

Sorry, I missed it was a threaded IRQ handler, so I think is ok to use a
mutex_lock() in there.

You should run with CONFIG DEBUG_MUTEXES and CONFIG_LOCKDEP enabled to give you
some more confidence.

It would be good to document how this mutex is used and document the start of
the function so it doesn't get changed later to a non-threaded handler. 

Mikey