Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754482AbdFNHUv (ORCPT ); Wed, 14 Jun 2017 03:20:51 -0400 Received: from mail-oi0-f52.google.com ([209.85.218.52]:33496 "EHLO mail-oi0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751971AbdFNHUt (ORCPT ); Wed, 14 Jun 2017 03:20:49 -0400 MIME-Version: 1.0 In-Reply-To: References: <1497345926-3262-1-git-send-email-binoy.jayan@linaro.org> <20170613095618.GB29589@mail.corp.redhat.com> From: Arnd Bergmann Date: Wed, 14 Jun 2017 09:20:48 +0200 X-Google-Sender-Auth: teAlKLE1guhRiW9f5GfdGKt5ERc Message-ID: Subject: Re: [PATCH v2] HID: Replace semaphore driver_lock with mutex To: Binoy Jayan Cc: David Herrmann , Benjamin Tissoires , "open list:HID CORE LAYER" , Linux Kernel Mailing List , Rajendra , Mark Brown , Jiri Kosina , David Herrmann , Andrew de los Reyes 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: 1827 Lines: 37 On Wed, Jun 14, 2017 at 7:22 AM, Binoy Jayan wrote: > Hi, > > On 14 June 2017 at 01:55, Arnd Bergmann wrote: > >>> The mutex code clearly states mutex_trylock() must not be used in >>> interrupt context (see kernel/locking/mutex.c), hence we used a >>> semaphore here. Unless the mutex code is changed to allow this, we >>> cannot switch away from semaphores. >> >> Right, that makes a lot of sense. I don't think changing the mutex >> code is an option here, but I wonder if we can replace the semaphore >> with something simpler anyway. >> >> From what I can tell, it currently does two things: >> >> 1. it acts as a simple flag to prevent hid_input_report from derefencing >> the hid->driver pointer during initialization and exit. I think this could >> be done equally well using a simple atomic set_bit()/test_bit() or similar. >> >> 2. it prevents the hid->driver pointer from becoming invalid while an >> asynchronous hid_input_report() is in progress. This actually seems to >> be a reference counting problem rather than a locking problem. >> I don't immediately see how to better address it, or how exactly this >> could go wrong in practice, but I would naively expect that either >> hdev->driver->remove() needs to wait for the last user of hdev->driver >> to complete, or we need kref_get/kref_put in hid_input_report() >> to trigger the actual release function. > > Thank you everyone for the comments. I'll resend the patch with Benjamin's > comments incorporated and address the changes in the second semaphore later. I hope that David or someone else can provide some more feedback on my interpretation above first so we can decide how this should be handled. Right now, I wouldn't know how to address point 2 above. Arnd