From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757724AbcAJXo7 (ORCPT ); Sun, 10 Jan 2016 18:44:59 -0500 Received: from lxorguk.ukuu.org.uk ([81.2.110.251]:40084 "EHLO lxorguk.ukuu.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751868AbcAJXo4 (ORCPT ); Sun, 10 Jan 2016 18:44:56 -0500 Date: Sun, 10 Jan 2016 23:44:39 +0000 From: One Thousand Gnomes To: Peter Hurley Cc: "linux-serial@vger.kernel.org" , Linux kernel , Greg KH , Jiri Slaby Subject: Re: RFC: out-of-tree tty driver breakage (changing ASYNC_ bits) Message-ID: <20160110234439.4191f3f4@lxorguk.ukuu.org.uk> In-Reply-To: <5692D054.7050203@hurleysoftware.com> References: <5692D054.7050203@hurleysoftware.com> Organization: Intel Corporation X-Mailer: Claws Mail 3.12.0 (GTK+ 2.24.29; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 10 Jan 2016 13:42:44 -0800 Peter Hurley wrote: > The tty/serial core uses 5 bits in the tty_port.flags field to > manage state. They are: > > ASYNCB_INITIALIZED > ASYNCB_SUSPENDED > ASYNCB_NORMAL_ACTIVE > - and - > ASYNCB_CTS_FLOW > ASYNCB_CHECK_CD > > Unfortunately, updates to this field (tty_port.flags) are often > non-atomic. Additionally, the field is visible to/modifiable by > userspace (the first 3 bits above are not modifiable by userspace > though). Lastly, the multi-bit ASYNC_SPD_ bitfield is in this > tty_port.flags field as well. ASYNC_SPD ought to just get retired, it's been obsolete and warning people since forever 8) Two comments: 1. Making something unsigned long doesn't magically make it atomic. You either use atomic_foo() or you use set_bit() and friends or the compiler sneaks up on you and does evil things. It might make it "a bit more atomic" but it doesn't make it correct. The compiler is free to do stupid things like turn x |= 1 into store 1 to memory or memory with reg (holding old x) gcc won't afaik ever do that on any platform we support, but it's not against the rules if its ever optimal ! 2. On a lot of architectures it's going to be easier to just use set_bit() and friends I suspect than take the cache hit of 5 unsigned longs. At the very least re-order the struct to keep the hot stuff together. The compiler will also play other games with your intentions. It'll defer or even eliminate invisible writes that don't get protected by memory barriers or forced by say function calls. Alan