From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0F6FBC433EF for ; Thu, 23 Jun 2022 19:38:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=n4iVpTNj7eLxa1+aMDxvyPgcZRHETkHM/gSlzis6d9Y=; b=RBa1VepI7KM0HD eqllKT+9lJ3qFKFsOxznj5zcC4v2OYVYg63H89w2CG0ztv0mwprygppgh04wFRF3qTfW+YLaMGCNW ocRDe3o1GmsVU2fjJHLFHOAjOR7WahMk8BzMBrZKzXWHEYrYp9Hz+m5ij+UH3zxpOF6kUNmkX5bHX RbARGXzhlWkMldzQRgx/ymX8B0EJORaomb6p1BpKDMOV22uPFWQFkjvqppKsNcCd4E2U8XJusqsVX MP9lz9L7f1ZOIkN1tU7UQGsJDpwq8690c9kQ2MmYGoLQj5wqiNLiSNAYxY7JDEaJJSHNqtkEU0bsC COo1O17fBy7PHapHz3dQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1o4Sdl-00GWsx-RR; Thu, 23 Jun 2022 19:37:22 +0000 Received: from mail-pg1-x533.google.com ([2607:f8b0:4864:20::533]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1o4Sb4-00GVPx-9R for linux-arm-kernel@lists.infradead.org; Thu, 23 Jun 2022 19:34:36 +0000 Received: by mail-pg1-x533.google.com with SMTP id 23so359711pgc.8 for ; Thu, 23 Jun 2022 12:34:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=SS2A7OJp1TI5gGfSVSLBw1YITG3GzljMQ/pt3L4SXf8=; b=iUjGkkNlozL+a26G8myNEVJ7Sl9IPL6k/s9f9cnVeox6CMWAEXpuY6OJYWElHiV38K ORCfxAZFi+rWGyGjM3gCG2JpPZUJ8g4981VMvGpwNuKc57a5OG8vIl2iCkgNGL7BGGo8 LqoQLHP8KUuB6+Wq7WYx3QGyy3ReGbcnYoKqqvEsSPAEnSlVvk/FSWrqo8A4epkTqyI1 3Dc8ou9I8Sz1Uw9KdEKbecoJgnqxfJae57W1mDF67QZxwmv76PUHTC1h7sFUauQJEP/6 8DHBe+zZ20eKSi2JZCqhhcI3IHxSt4+upSZQvlxc+Uwpi+Luwr6hNmez89R1EiaN8+Tg N5EQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=SS2A7OJp1TI5gGfSVSLBw1YITG3GzljMQ/pt3L4SXf8=; b=mtj2RGj1+SGruzM158gI93Gi4hElvoV0vncq88O7rXZ22UqEw7ehNJj+idNEGxtaS8 lXfKB7p0TPW0ijYW6Z2SbLVsRaTHJdaLodjxUg8R7Ittpb1yd1roCDND/ouudRLsbFj0 J73B3joYX1zylQt8oFas0lM0fGUXdcAkemUFSnY4ayVp8pAik2aI6DEKMpGr3hxAsWNs RL9Rm2dF4NwSVyMrF8OAYLUki0xM6HMqKajNo4su7zK+ER5r8CdXi7XJLkJIVbdj9XAb OLEA4d7eX/+hQV5KGAi1eDNG0W3+1EH5qbO1rA/igjY91X2dWzuOfo2pzeyMaOMVAWFK 7KrQ== X-Gm-Message-State: AJIora/R3amfIe1xh/33r6rVLVQCEhXdaYr32SfyGFj3JsGNiAWCrjsf NPZUnymswKoDpUJtxfRaeJAT2A== X-Google-Smtp-Source: AGRyM1vv2ybrAiP9gjCiUc75kwwSWGUR/XxImcPmZMnkC1aEf5irYGp3NaID3KykWgmfr6Zj49FznA== X-Received: by 2002:a05:6a00:2484:b0:525:45bd:945a with SMTP id c4-20020a056a00248400b0052545bd945amr10088760pfv.51.1656012872040; Thu, 23 Jun 2022 12:34:32 -0700 (PDT) Received: from p14s (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id s42-20020a056a0017aa00b0052553215444sm16967pfg.101.2022.06.23.12.34.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Jun 2022 12:34:30 -0700 (PDT) Date: Thu, 23 Jun 2022 13:34:28 -0600 From: Mathieu Poirier To: Mike Leach Cc: linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org, suzuki.poulose@arm.com, leo.yan@linaro.org Subject: Re: [PATCH v4 1/2] coresight: configfs: Fix unload of configurations on module exit Message-ID: <20220623193428.GA1678885@p14s> References: <20220622101621.4312-1-mike.leach@linaro.org> <20220622101621.4312-2-mike.leach@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220622101621.4312-2-mike.leach@linaro.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220623_123434_380057_08533C68 X-CRM114-Status: GOOD ( 45.99 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Jun 22, 2022 at 11:16:20AM +0100, Mike Leach wrote: > Any loaded configurations must be correctly unloaded on coresight module > exit, or issues can arise with nested locking in the configfs directory > code if built with CONFIG_LOCKDEP. > > Prior to this patch, the preloaded configuration configfs directory entries > were being unloaded by the recursive code in > configfs_unregister_subsystem(). > > However, when built with CONFIG_LOCKDEP, this caused a nested lock warning, > which was not mitigated by the LOCKDEP dependent code in fs/configfs/dir.c > designed to prevent this, due to the different directory levels for the > root of the directory being removed. > > As the preloaded (and all other) configurations are registered after > configfs_register_subsystem(), we now explicitly unload them before the > call to configfs_unregister_subsystem(). > > The new routine cscfg_unload_cfgs_on_exit() iterates through the load > owner list to unload any remaining configurations that were not unloaded > by the user before the module exits. This covers both the > CSCFG_OWNER_PRELOAD and CSCFG_OWNER_MODULE owner types, and will be > extended to cover future load owner types for CoreSight configurations. > > Applies to coresight/next The above sentence will end up in the kernel log forever, which is not what we want. > > Fixes: eb2ec49606c2 ("coresight: syscfg: Update load API for config loadable modules") > Reported-by: Suzuki Poulose > Signed-off-by: Mike Leach > --- > .../hwtracing/coresight/coresight-syscfg.c | 106 ++++++++++++++++-- > 1 file changed, 95 insertions(+), 11 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > index 11850fd8c3b5..9cd7d3c91d8e 100644 > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > @@ -414,6 +414,27 @@ static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, vo > } > } > > +/* > + * Unregister all configuration and features from configfs owned by load_owner. > + * Although this is called without the list mutex being held, it is in the > + * context of an unload operation which are strictly serialised, > + * so the lists cannot change during this call. > + */ > +static void cscfg_fs_unregister_cfgs_feats(void *load_owner) > +{ > + struct cscfg_config_desc *config_desc; > + struct cscfg_feature_desc *feat_desc; > + > + list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) { > + if (config_desc->load_owner == load_owner) > + cscfg_configfs_del_config(config_desc); > + } > + list_for_each_entry(feat_desc, &cscfg_mgr->feat_desc_list, item) { > + if (feat_desc->load_owner == load_owner) > + cscfg_configfs_del_feature(feat_desc); > + } > +} > + > /* > * removal is relatively easy - just remove from all lists, anything that > * matches the owner. Memory for the descriptors will be managed by the owner, > @@ -1022,10 +1043,13 @@ struct device *cscfg_device(void) > /* Must have a release function or the kernel will complain on module unload */ > static void cscfg_dev_release(struct device *dev) > { > + mutex_lock(&cscfg_mutex); > kfree(cscfg_mgr); > cscfg_mgr = NULL; > + mutex_unlock(&cscfg_mutex); > } > > + > /* a device is needed to "own" some kernel elements such as sysfs entries. */ > static int cscfg_create_device(void) > { > @@ -1042,6 +1066,13 @@ static int cscfg_create_device(void) > if (!cscfg_mgr) > goto create_dev_exit_unlock; > > + /* initialise the cscfg_mgr structure */ > + INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list); > + INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list); > + INIT_LIST_HEAD(&cscfg_mgr->config_desc_list); > + INIT_LIST_HEAD(&cscfg_mgr->load_order_list); > + atomic_set(&cscfg_mgr->sys_active_cnt, 0); > + > /* setup the device */ > dev = cscfg_device(); > dev->release = cscfg_dev_release; > @@ -1056,17 +1087,74 @@ static int cscfg_create_device(void) > return err; > } > > -static void cscfg_clear_device(void) > +/* > + * Loading and unloading is generally on user discretion. > + * If exiting due to coresight module unload, we need to unload any configurations that remain, > + * before we unregister the configfs intrastructure. > + * > + * Do this by walking the load_owner list and taking appropriate action, depending on the load > + * owner type. > + */ > +static void cscfg_unload_cfgs_on_exit(void) > { > - struct cscfg_config_desc *cfg_desc; > + struct cscfg_load_owner_info *owner_info = NULL; > > + /* > + * grab the mutex - even though we are exiting, some configfs files > + * may still be live till we dump them, so ensure list data is > + * protected from a race condition. > + */ > mutex_lock(&cscfg_mutex); > - list_for_each_entry(cfg_desc, &cscfg_mgr->config_desc_list, item) { > - etm_perf_del_symlink_cscfg(cfg_desc); > + while (!list_empty(&cscfg_mgr->load_order_list)) { > + > + /* remove in reverse order of loading */ > + owner_info = list_last_entry(&cscfg_mgr->load_order_list, > + struct cscfg_load_owner_info, item); > + > + /* action according to type */ > + switch (owner_info->type) { > + case CSCFG_OWNER_PRELOAD: > + /* > + * preloaded descriptors are statically allocated in > + * this module - just need to unload dynamic items from > + * csdev lists, and remove from configfs directories. > + */ > + pr_info("cscfg: unloading preloaded configurations\n"); > + break; > + > + case CSCFG_OWNER_MODULE: > + /* > + * this is an error - the loadable module must have been unloaded prior > + * to the coresight module unload. Therefore that module has not > + * correctly unloaded configs in its own exit code. > + * Nothing to do other than emit an error string as the static descriptor > + * references we need to unload will have disappeared with the module. > + */ > + pr_err("cscfg: ERROR - a loadable module failed to " > + "unload configs on exit\n"); This gives me a checkpatch warning. Please correct and resend a new revision. Thanks, Mathieu > + goto list_remove; > + } > + > + /* remove from configfs - outside the scope of the list mutex */ > + mutex_unlock(&cscfg_mutex); > + cscfg_fs_unregister_cfgs_feats(owner_info); > + mutex_lock(&cscfg_mutex); > + > + /* Next unload from csdev lists. */ > + cscfg_unload_owned_cfgs_feats(owner_info); > + > +list_remove: > + /* remove from load order list */ > + list_del(&owner_info->item); > } > + mutex_unlock(&cscfg_mutex); > +} > + > +static void cscfg_clear_device(void) > +{ > + cscfg_unload_cfgs_on_exit(); > cscfg_configfs_release(cscfg_mgr); > device_unregister(cscfg_device()); > - mutex_unlock(&cscfg_mutex); > } > > /* Initialise system config management API device */ > @@ -1074,20 +1162,16 @@ int __init cscfg_init(void) > { > int err = 0; > > + /* create the device and init cscfg_mgr */ > err = cscfg_create_device(); > if (err) > return err; > > + /* initialise configfs subsystem */ > err = cscfg_configfs_init(cscfg_mgr); > if (err) > goto exit_err; > > - INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list); > - INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list); > - INIT_LIST_HEAD(&cscfg_mgr->config_desc_list); > - INIT_LIST_HEAD(&cscfg_mgr->load_order_list); > - atomic_set(&cscfg_mgr->sys_active_cnt, 0); > - > /* preload built-in configurations */ > err = cscfg_preload(THIS_MODULE); > if (err) > -- > 2.17.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel