Re: [patch V2 07/15] powerpc/ps3: Convert half completion to rcuwait

From: Peter Zijlstra <peterz@infradead.org>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Ingo Molnar <mingo@kernel.org>, Will Deacon <will@kernel.org>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Joel Fernandes <joel@joelfernandes.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Arnd Bergmann <arnd@arndb.de>,
	linuxppc-dev@lists.ozlabs.org,
	Logan Gunthorpe <logang@deltatee.com>,
	Kurt Schwemmer <kurt.schwemmer@microsemi.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, Felipe Balbi <balbi@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org, Kalle Valo <kvalo@codeaurora.org>,
	"David S. Miller" <davem@davemloft.net>,
	linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
	Oleg Nesterov <oleg@redhat.com>,
	Davidlohr Bueso <dave@stgolabs.net>
Subject: Re: [patch V2 07/15] powerpc/ps3: Convert half completion to rcuwait
Date: Thu, 19 Mar 2020 10:18:50 +0100	[thread overview]
Message-ID: <20200319091850.GM20730@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20200319090024.wbrywc77tff3ro7i@linutronix.de>

On Thu, Mar 19, 2020 at 10:00:24AM +0100, Sebastian Andrzej Siewior wrote:
> On 2020-03-18 21:43:09 [+0100], Thomas Gleixner wrote:
> > --- a/arch/powerpc/platforms/ps3/device-init.c
> > +++ b/arch/powerpc/platforms/ps3/device-init.c
> > @@ -725,12 +728,12 @@ static int ps3_notification_read_write(s
> >  	unsigned long flags;
> >  	int res;
> >  
> > -	init_completion(&dev->done);
> >  	spin_lock_irqsave(&dev->lock, flags);
> >  	res = write ? lv1_storage_write(dev->sbd.dev_id, 0, 0, 1, 0, lpar,
> >  					&dev->tag)
> >  		    : lv1_storage_read(dev->sbd.dev_id, 0, 0, 1, 0, lpar,
> >  				       &dev->tag);
> > +	dev->done = false;
> >  	spin_unlock_irqrestore(&dev->lock, flags);
> >  	if (res) {
> >  		pr_err("%s:%u: %s failed %d\n", __func__, __LINE__, op, res);
> > @@ -738,14 +741,10 @@ static int ps3_notification_read_write(s
> >  	}
> >  	pr_debug("%s:%u: notification %s issued\n", __func__, __LINE__, op);
> >  
> > -	res = wait_event_interruptible(dev->done.wait,
> > -				       dev->done.done || kthread_should_stop());
> > +	rcuwait_wait_event(&dev->wait, dev->done || kthread_should_stop(), TASK_IDLE);
> > +
> …
> 
> Not sure it matters but this struct `dev' is allocated on stack. Should
> the interrupt fire *before* rcuwait_wait_event() set wait.task to NULL
> then it is of random value on the first invocation of rcuwait_wake_up().
> ->
> 
> diff --git a/arch/powerpc/platforms/ps3/device-init.c b/arch/powerpc/platforms/ps3/device-init.c
> index 197347c3c0b24..e87360a0fb40d 100644
> --- a/arch/powerpc/platforms/ps3/device-init.c
> +++ b/arch/powerpc/platforms/ps3/device-init.c
> @@ -809,6 +809,7 @@ static int ps3_probe_thread(void *data)
>  	}
>  
>  	spin_lock_init(&dev.lock);
> +	rcuwait_init(&dev.wait);
>  
>  	res = request_irq(irq, ps3_notification_interrupt, 0,
>  			  "ps3_notification", &dev);
> 

Very good, sorry for that.