From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753487AbcICO1A (ORCPT ); Sat, 3 Sep 2016 10:27:00 -0400 Received: from netrider.rowland.org ([192.131.102.5]:48201 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752851AbcICO07 (ORCPT ); Sat, 3 Sep 2016 10:26:59 -0400 Date: Sat, 3 Sep 2016 10:26:58 -0400 (EDT) From: Alan Stern X-X-Sender: stern@netrider.rowland.org To: Peter Zijlstra cc: "Paul E. McKenney" , Ingo Molnar , Felipe Balbi , USB list , Kernel development list Subject: Re: Memory barrier needed with wake_up_process()? In-Reply-To: <20160903123133.GD2794@worktop> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 3 Sep 2016, Peter Zijlstra wrote: > On Fri, Sep 02, 2016 at 04:29:19PM -0400, Alan Stern wrote: > > I'm afraid so. The code doesn't use wait_event(), in part because > > there's no wait_queue (since only one task is involved). > > You can use wait_queue fine with just one task, and it would clean up > the code tremendously. > > You can replace things like the earlier mentioned: > > while (bh->state != BUF_STATE_EMPTY) { > rc = sleep_thread(common, false); > if (rc) > return rc; > } > > with: > > rc = wait_event_interruptible(&common->wq, bh->state == BUF_STATE_EMPTY); > if (rc) > return rc; If someone wants to devote time and effort to cleaning up the driver, that would be a good start. > > But maybe there's another barrier which needs to be fixed. Felipe, can > > you check to see if received_cbw() is getting called in > > get_next_command(), and if so, what value it returns? Or is the > > preceding sleep_thread() the one that never wakes up? > > > > It could be that the smp_wmb() in wakeup_thread() needs to be smp_mb(). > > The reason being that get_next_command() runs outside the protection of > > the spinlock. > > Being somewhat confused by the code, I fail to follow that argument. > wakeup_thread() is always called under that spinlock(), but since the > critical section is 2 stores, I fail to see how a smp_mb() can make any > difference over the smp_wmb() already there. But sleep_thread() and the code that follows it are _not_ called under the spinlock. And the following code examines values that were written by DMA, not by the CPU calling wakeup_thread(). (Although that CPU _is_ the one that receives the DMA-completion notice.) In other words, we have: CPU 0 CPU 1 ----- ----- Start DMA Handle DMA-complete irq Sleep until bh->state Set bh->state smp_wmb() Wake up CPU 0 smp_rmb() Compute rc based on contents of the DMA buffer This was written many years ago, at a time when I did not fully understand all the details of memory ordering. Do you agree that both of those barriers should really be smp_mb()? That's what Felipe has been testing. Alan Stern