Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752634AbdHJOiN (ORCPT ); Thu, 10 Aug 2017 10:38:13 -0400 Received: from hs01.dk-develop.de ([213.136.71.231]:59053 "EHLO hs01.dk-develop.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751439AbdHJOiM (ORCPT ); Thu, 10 Aug 2017 10:38:12 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 10 Aug 2017 16:38:10 +0200 From: Danilo Krummrich To: Linus Walleij Cc: linux-kernel@vger.kernel.org, Linux Input , Dmitry Torokhov , devicetree@vger.kernel.org Subject: Re: [PATCH] serio: PS2 gpio bit banging driver for the serio bus In-Reply-To: <20170807182207.348762301bf3d7f8509b1bf7@dk-develop.de> References: <20170731222452.22887-1-danilokrummrich@dk-develop.de> <20170807182207.348762301bf3d7f8509b1bf7@dk-develop.de> Message-ID: <8e5e73575b3a70e0e60931698687471d@dk-develop.de> User-Agent: Roundcube Webmail/1.3.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1371 Lines: 42 Hi Linus, On 2017-08-07 18:22, Danilo Krummrich wrote: >> > +static int ps2_gpio_write(struct serio *serio, unsigned char val) >> > +{ >> > + struct ps2_gpio_data *drvdata = serio->port_data; >> > + >> > + drvdata->mode = PS2_MODE_TX; >> > + drvdata->tx_byte = val; >> > + /* Make sure ISR running on other CPU notice changes. */ >> > + barrier(); >> >> This seems overengineered, is this really needed? >> >> If we have races like this, the error is likely elsewhere, and should >> be >> fixed in the GPIO driver MMIO access or so. >> > Yes, seems it can be removed. I didn't saw any explicit barriers in the > GPIO > driver (I'm testing on bcm2835), but it seems MMIO operations on SMP > archs > does contain barriers. Not sure if all do. If some do not this barrier > might > be needed to ensure ISR on other CPU notice the correct mode and byte > to send. > I couldn't find any guarantee that the mode and tx_byte change is implicitly covered by a barrier in this case. E.g. the bcm2835 driver does not make sure stores are completed before the particular interrupt is enabled, except by the fact that writel on ARM contains a wmb(). But this is nothing to rely on. (Please tell me if I miss something.) Therefore I would like to keep this barrier and replace it with smp_wmb() if you are fine with that. Regards, Danilo