Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751533AbdFIBeC (ORCPT ); Thu, 8 Jun 2017 21:34:02 -0400 Received: from mail.kernel.org ([198.145.29.99]:46492 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751442AbdFIBeA (ORCPT ); Thu, 8 Jun 2017 21:34:00 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0E8AF23A0E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mcgrof@kernel.org MIME-Version: 1.0 In-Reply-To: References: <20170526194640.GS8951@wotan.suse.de> <20170526215518.GB40877@dtor-ws> <20170605202410.GQ8951@wotan.suse.de> <1496760796.5682.48.camel@linux.intel.com> <20170606164734.GB27288@wotan.suse.de> <20170606221151.ygoxqkwhhjsqw632@thunk.org> <20170607002237.GJ27288@wotan.suse.de> <20170607062515.GA23434@dtor-ws> From: "Luis R. Rodriguez" Date: Thu, 8 Jun 2017 18:33:36 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback To: Andy Lutomirski Cc: Dmitry Torokhov , "Theodore Ts'o" , Alan Cox , Linux FS Devel , Stephen Boyd , "Li, Yi" , Peter Zijlstra , Jonathan Corbet , "Eric W. Biederman" , "Michael Kerrisk (man-pages)" , Greg KH , "Fuzzey, Martin" , Linux API , Daniel Wagner , David Woodhouse , jewalt@lgsinnovations.com, rafal@milecki.pl, Arend Van Spriel , "Rafael J. Wysocki" , atull@opensource.altera.com, Moritz Fischer , Petr Mladek , Johannes Berg , Emmanuel Grumbach , Luca Coelho , Kalle Valo , Linus Torvalds , Kees Cook , AKASHI Takahiro , David Howells , Peter Jones , Hans de G oede , "linux-kernel@vger.kernel.org" , Julia Lawall , "Luis R. Rodriguez" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2836 Lines: 58 On Thu, Jun 8, 2017 at 6:14 PM, Andy Lutomirski wrote: > On Tue, Jun 6, 2017 at 11:25 PM, Dmitry Torokhov > wrote: >> On Tue, Jun 06, 2017 at 09:56:47PM -0700, Andy Lutomirski wrote: >>> On Tue, Jun 6, 2017 at 5:22 PM, Luis R. Rodriguez wrote: >>> > On Tue, Jun 06, 2017 at 06:11:51PM -0400, Theodore Ts'o wrote: >>> >> On Tue, Jun 06, 2017 at 06:47:34PM +0200, Luis R. Rodriguez wrote: >>> >> > On Tue, Jun 06, 2017 at 03:53:16PM +0100, Alan Cox wrote: >>> > >>> > We rely on swait, and swait right now only uses -ERESTARTSYS. Are >>> > you saying we could mask out -ERESTARTSYS and map it to -ERESTARTNOINTR >>> > or -ERESTARTNOHAND if we see fit for some future functionality / need ? >>> >>> I think that has essentially nothing to do with swait. User code does >>> some syscall. That syscall triggers a firmware load. The caller gets >>> a signal. If you're going to let firmware load get interrupted, you >>> need to consider what the syscall is. >> >> I think it is way too complicated and I do not think driver writers will >> stand a chance of implementing this correctly, given that often firmware >> load might be triggered indirectly and by multitude of syscalls. >> > > That's what I meant, but I said it unclearly. I meant that, if we're > going to start allowing interruption, we would need to audit all the > callers. Ugh. There are actually two audits worth evaluating if what we've concluded is fair game: a) firmware sync calls on interruptible paths b) use of swait / old interruptible waits on sysfs paths As for a) we drove tons of code away from using sync, request firmware, and on async firmware the return value is lost, we only can currently know if a failure of some sort occurred. If that push to async failed we also scared away tons of drivers to use request_firmware() calls on init and later incorrectly on probe due to serialized init + probe delay on boot. From what I recall my last Coccinelle evil-monster-hunt, I did indeed find quite a bit of drivers still still relying on sync firmware which ultimately revealed use on a probe path. The signal however was only effective as of commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion is interrupted") merged as of v4.0. Creating awareness of the issue seems fair, but I don't think its worth a huge fly swatter. I have no information to contribute for b) other than I was reluctant to even consider it. > I suppose we could have request_firmware_interruptable(), but that > seems like it's barely worth it. I don't think that's worth it, given the signal was effective only as of v4.0, we already had a big push away from sync requests, and also had the "don't use request firmware" on init scare which also propagated to probe later. Luis