From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.codeaurora.org by pdx-caf-mail.web.codeaurora.org (Dovecot) with LMTP id QTvCBD/8GFtoDgAAmS7hNA ; Thu, 07 Jun 2018 09:34:55 +0000 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 03812605A2; Thu, 7 Jun 2018 09:34:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on pdx-caf-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI autolearn=ham autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by smtp.codeaurora.org (Postfix) with ESMTP id 48A4B601C3; Thu, 7 Jun 2018 09:34:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 48A4B601C3 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753511AbeFGJew (ORCPT + 25 others); Thu, 7 Jun 2018 05:34:52 -0400 Received: from foss.arm.com ([217.140.101.70]:49250 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753069AbeFGJev (ORCPT ); Thu, 7 Jun 2018 05:34:51 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 865D915AB; Thu, 7 Jun 2018 02:34:50 -0700 (PDT) Received: from [10.37.9.91] (unknown [10.37.9.91]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 448F53F5A0; Thu, 7 Jun 2018 02:34:45 -0700 (PDT) Subject: Re: [PATCH v4 05/14] coresight: get/put module in coresight_build/release_path From: Suzuki K Poulose To: Greg Kroah-Hartman Cc: Kim Phillips , Mathieu Poirier , Leo Yan , Alexander Shishkin , Alex Williamson , Andrew Morton , David Howells , Eric Auger , Eric Biederman , Gargi Sharma , Geert Uytterhoeven , Kefeng Wang , Kirill Tkhai , Mike Rapoport , Oleg Nesterov , Pavel Tatashin , Rik van Riel , Robin Murphy , Russell King , Thierry Reding , Todd Kjos , Randy Dunlap , linux-arm-kernel , Linux Kernel Mailing List References: <20180605210710.22227-1-kim.phillips@arm.com> <20180605210710.22227-6-kim.phillips@arm.com> <20180606082422.GB19727@kroah.com> <20180606155501.704583e1412996a1a2c6fa61@arm.com> <20180607083401.GE16651@kroah.com> <3219276b-2703-bc30-92e1-bae80cdc5901@arm.com> <20180607091353.GA20438@kroah.com> <2f8d233e-8847-ce3d-3a5b-06b175e3944b@arm.com> Message-ID: <30d4d1cc-47d8-b42d-af5d-b1f6301d1bd9@arm.com> Date: Thu, 7 Jun 2018 10:34:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <2f8d233e-8847-ce3d-3a5b-06b175e3944b@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/07/2018 10:32 AM, Suzuki K Poulose wrote: > On 06/07/2018 10:13 AM, Greg Kroah-Hartman wrote: >> On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote: >>> Hi Greg, >>> >>> On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote: >>>> On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote: >>>>> On Wed, 6 Jun 2018 10:46:36 +0100 >>>>> Suzuki K Poulose wrote: >>>>> >>>>>> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote: >>>>>>> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote: >>>>>>>> Increment the refcnt for driver modules in current use by calling >>>>>>>> module_get in coresight_build_path and module_put in release_path. >>>>>>>> >>>>>>>> This prevents driver modules from being unloaded when they are >>>>>>>> in use, >>>>>>>> either in sysfs or perf mode. >>>>>>> >>>>>>> Why does it matter?  Shouldn't you be allowed to remove any >>>>>>> module at >>>>>>> any point in time, much like a networking driver? >>> >>> The user doesn't have an explicit refcount on the individual components >>> in a trace session. So, when a trace session is in progress, it is as >>> good as having a "file" open on each component that is part of the >>> active trace session. So, we don't want the driver to be removed when >>> the component is being used in the trace collection. >> >> Why not?  What's wrong with that happening and then the trace collection >> starts failing with -ENODEV or something? Forgot to add, this will indeed hit -ENODEV, if the device driver was removed, as we fail to build the trace path before the session. > > May be I am missing something here. Can we allow the driver to be > removed when one of its device is "turned ON" and we need the same > driver to "turn it OFF" when the session ends ? To make a better > comparison : > > Can we unload a usb_mass_storage module when a USB disk(which uses the > module driver) is mounted and is being used ? I believe, the module > will eventually get unloaded when we unmount the disk, if someone did > a unload. > > We have a similar situation here. The only difference is the driver is > referenced only when one of its device is in a trace session. > >> >> Remember, removing a kernel module is something that only happens very >> rarely, and is an explicit choice by someone with root permissions.  If >> you want to remove that module, it should be able to go, as you know >> what you are doing at that point in time. > > Right, but when a device is "in use" can we do that ? I thought the user > will get a module is in use or busy, error. > > >> >> Don't try to "protect the user from themselves" here, they want to shoot >> their foot, make it hurt if they are aiming it there :) >> > > The module_get/put added here are only triggered when we start a trace > session, where we build a path for the current session from the > configured "source" to the configured "sink" and the path is destroyed > at the end of the trace session. i.e, the path is not a permanent thing. > It is constructed per session. So it is perfectly possible to remove a > device in between trace sessions. > >>> This will be >>> released as soon as the session is ended. It is just like a PMU driver >>> where the module refcount is held to ensure the module stays until the >>> session is over. In this case, we have multiple components, each with >>> its own driver invisible to the PMU driver. Hence the coresight driver >>> must hold the reference. >> >> Again, please think this through and don't add extra complexity to the >> normal path, and get it right if you do it (the existing patch is not >> right as I pointed out.)  Personally, I feel the code should just be >> able to be unloaded whenever they want, user beware... > > Sure, will explore more to refine the code. Thanks for the trigger. > > Cheers > Suzuki Suzuki From mboxrd@z Thu Jan 1 00:00:00 1970 From: suzuki.poulose@arm.com (Suzuki K Poulose) Date: Thu, 7 Jun 2018 10:34:55 +0100 Subject: [PATCH v4 05/14] coresight: get/put module in coresight_build/release_path In-Reply-To: <2f8d233e-8847-ce3d-3a5b-06b175e3944b@arm.com> References: <20180605210710.22227-1-kim.phillips@arm.com> <20180605210710.22227-6-kim.phillips@arm.com> <20180606082422.GB19727@kroah.com> <20180606155501.704583e1412996a1a2c6fa61@arm.com> <20180607083401.GE16651@kroah.com> <3219276b-2703-bc30-92e1-bae80cdc5901@arm.com> <20180607091353.GA20438@kroah.com> <2f8d233e-8847-ce3d-3a5b-06b175e3944b@arm.com> Message-ID: <30d4d1cc-47d8-b42d-af5d-b1f6301d1bd9@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 06/07/2018 10:32 AM, Suzuki K Poulose wrote: > On 06/07/2018 10:13 AM, Greg Kroah-Hartman wrote: >> On Thu, Jun 07, 2018 at 10:04:33AM +0100, Suzuki K Poulose wrote: >>> Hi Greg, >>> >>> On 06/07/2018 09:34 AM, Greg Kroah-Hartman wrote: >>>> On Wed, Jun 06, 2018 at 03:55:01PM -0500, Kim Phillips wrote: >>>>> On Wed, 6 Jun 2018 10:46:36 +0100 >>>>> Suzuki K Poulose wrote: >>>>> >>>>>> On 06/06/2018 09:24 AM, Greg Kroah-Hartman wrote: >>>>>>> On Tue, Jun 05, 2018 at 04:07:01PM -0500, Kim Phillips wrote: >>>>>>>> Increment the refcnt for driver modules in current use by calling >>>>>>>> module_get in coresight_build_path and module_put in release_path. >>>>>>>> >>>>>>>> This prevents driver modules from being unloaded when they are >>>>>>>> in use, >>>>>>>> either in sysfs or perf mode. >>>>>>> >>>>>>> Why does it matter?? Shouldn't you be allowed to remove any >>>>>>> module at >>>>>>> any point in time, much like a networking driver? >>> >>> The user doesn't have an explicit refcount on the individual components >>> in a trace session. So, when a trace session is in progress, it is as >>> good as having a "file" open on each component that is part of the >>> active trace session. So, we don't want the driver to be removed when >>> the component is being used in the trace collection. >> >> Why not?? What's wrong with that happening and then the trace collection >> starts failing with -ENODEV or something? Forgot to add, this will indeed hit -ENODEV, if the device driver was removed, as we fail to build the trace path before the session. > > May be I am missing something here. Can we allow the driver to be > removed when one of its device is "turned ON" and we need the same > driver to "turn it OFF" when the session ends ? To make a better > comparison : > > Can we unload a usb_mass_storage module when a USB disk(which uses the > module driver) is mounted and is being used ? I believe, the module > will eventually get unloaded when we unmount the disk, if someone did > a unload. > > We have a similar situation here. The only difference is the driver is > referenced only when one of its device is in a trace session. > >> >> Remember, removing a kernel module is something that only happens very >> rarely, and is an explicit choice by someone with root permissions.? If >> you want to remove that module, it should be able to go, as you know >> what you are doing at that point in time. > > Right, but when a device is "in use" can we do that ? I thought the user > will get a module is in use or busy, error. > > >> >> Don't try to "protect the user from themselves" here, they want to shoot >> their foot, make it hurt if they are aiming it there :) >> > > The module_get/put added here are only triggered when we start a trace > session, where we build a path for the current session from the > configured "source" to the configured "sink" and the path is destroyed > at the end of the trace session. i.e, the path is not a permanent thing. > It is constructed per session. So it is perfectly possible to remove a > device in between trace sessions. > >>> This will be >>> released as soon as the session is ended. It is just like a PMU driver >>> where the module refcount is held to ensure the module stays until the >>> session is over. In this case, we have multiple components, each with >>> its own driver invisible to the PMU driver. Hence the coresight driver >>> must hold the reference. >> >> Again, please think this through and don't add extra complexity to the >> normal path, and get it right if you do it (the existing patch is not >> right as I pointed out.)? Personally, I feel the code should just be >> able to be unloaded whenever they want, user beware... > > Sure, will explore more to refine the code. Thanks for the trigger. > > Cheers > Suzuki Suzuki