linux-kernel - Re: [PATCH 6/6] coresight: allow to build as modules

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4986a636-d5a5-387c-abf1-d68afe063f06@arm.com>
Date:   Fri, 25 May 2018 18:12:30 +0100
From:   Suzuki K Poulose <Suzuki.Poulose@....com>
To:     Kim Phillips <kim.phillips@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Howells <dhowells@...hat.com>,
        Eric Auger <eric.auger@...hat.com>,
        Eric Biederman <ebiederm@...ssion.com>,
        Gargi Sharma <gs051095@...il.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Kirill Tkhai <ktkhai@...tuozzo.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Pavel Tatashin <pasha.tatashin@...cle.com>,
        Rik van Riel <riel@...hat.com>,
        Robin Murphy <robin.murphy@....com>,
        Russell King <linux@...linux.org.uk>,
        Thierry Reding <treding@...dia.com>,
        Todd Kjos <tkjos@...gle.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] coresight: allow to build as modules

On 18/05/18 02:20, Kim Phillips wrote:
> Allow to build coresight as modules.  This greatly enhances developer
> efficiency by allowing the development to take place exclusively on the
> target, and without needing to reboot in between changes.
> 
> - Kconfig bools become tristates, to allow =m
> 
> - use -objs to denote merge object directives in Makefile, adds a
>    coresight-core nomenclature for the base module.
> 
> - Export core functions so as to be able to be used by
>    non-core modules.
> 
> - add a coresight_exit() that unregisters the coresight bus, add
>    remove fns for most others.
> 
> - fix up modules with ID tables for autoloading on boot
> 
> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
> Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> Cc: Randy Dunlap <rdunlap@...radead.org>
> Signed-off-by: Kim Phillips <kim.phillips@....com>

Kim,

I see that you have addressed my comments on a previous version
of this series posted in April. But I don't see the version number
increased for this new version. Please add versioning to make it
easier to make it more obvious.

Also, generally it is a good idea to keep the people who reviewed
and commented on your previous versions in the newer versions.

Some comments below :

> diff --git a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
> index fc742215ab05..bc42b8022556 100644
> --- a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
> @@ -37,7 +37,12 @@ struct replicator_state {
>   static int replicator_enable(struct coresight_device *csdev, int inport,
>   			      int outport)
>   {
> -	struct replicator_state *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct device *parent_dev = csdev->dev.parent;
> +	struct replicator_state *drvdata = dev_get_drvdata(parent_dev);
> +	struct module *module = parent_dev->driver->owner;
> +
> +	if (!try_module_get(module))
> +		return -ENODEV;
>   
>   	CS_UNLOCK(drvdata->base);

What is the guarantee that the "csdev" is still available when we reach
here ?

A module could be unloaded "after the component was added to the path"
(via coresight_build_path) and before we invoke the "enable" on each
component in the path ?

Also, it  is tedious to do module_get in "enable" and module_put in the
disable call backs for each component.

Instead, if we do a module_get() in build_path and module_put() in
release path, we could solve all these problems and keep it the module
refcount in a central place.

> +MODULE_DEVICE_TABLE(amba, replicator_ids);
> +
>   static struct amba_driver replicator_driver = {
>   	.drv = {
>   		.name	= "coresight-dynamic-replicator",
> @@ -207,9 +226,10 @@ static struct amba_driver replicator_driver = {
>   		.suppress_bind_attrs = true,
>   	},
>   	.probe		= replicator_probe,
> +	.remove		= replicator_remove,
>   	.id_table	= replicator_ids,
>   };

Do we have the owner field set here for this driver ? I see that you
added it for some components and not others. e.g, you have added it for
etm4x, while not for replicator and others.


> +MODULE_DEVICE_TABLE(amba, etm4_ids);
> +
>   static struct amba_driver etm4x_driver = {
>   	.drv = {
>   		.name   = "coresight-etm4x",
> +		.owner  = THIS_MODULE,
>   		.suppress_bind_attrs = true,
>   	},
>   	.probe		= etm4_probe,
> +	.remove		= etm4_remove,
>   	.id_table	= etm4_ids,
>   };
> -builtin_amba_driver(etm4x_driver);
> +module_amba_driver(etm4x_driver);


Suzuki