Re: [PATCH v3 19/20] coresight: add try_get_module() in coresight_grab_device()

From: tingwei@codeaurora.org
To: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: tsoni@codeaurora.org, saiprakash.ranjan@codeaurora.org,
	kim.phillips@arm.com, mathieu.poirier@linaro.org,
	alexander.shishkin@linux.intel.com, gregkh@linuxfoundation.org,
	coresight@lists.linaro.org, rdunlap@infradead.org,
	ykaukab@suse.de, linux@armlinux.org.uk, jinlmao@codeaurora.org,
	leo.yan@linaro.org,
	linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 19/20] coresight: add try_get_module() in coresight_grab_device()
Date: Thu, 23 Jul 2020 08:18:44 +0800	[thread overview]
Message-ID: <97a0a247c2c0833ec08c18e01a7d3c01@codeaurora.org> (raw)
In-Reply-To: <cce31f20-24fc-9e92-8054-bc1791647512@arm.com>

On 2020-07-22 19:26, Suzuki K Poulose wrote:
> On 07/22/2020 11:48 AM, Greg KH wrote:
>> On Wed, Jul 22, 2020 at 11:49:48AM +0100, Suzuki K Poulose wrote:
>>> Hi Tingwei,
>>> 
>>> On 07/17/2020 06:45 AM, Tingwei Zhang wrote:
>>>> When coresight device is in an active session, driver module of
>>>> that device should not be removed. Use try_get_module() in
>>>> coresight_grab_device() to prevent module to be unloaded.
>>>> 
>>> 
>>> Is this really sufficient ? AFAIU, a device could be removed, but the
>>> module may still be alive due to the refcount on the module. This
>>> could imply that we have stale pointers in the _path_, which could
>>> lead to corruption elsewhere. Should we do a get/put_device() instead 
>>> ?
>> 
>> Remember there are two separate things here, code and data.  There are
>> two different reference counts for them, do not confuse the two.
>> 
>> get/put is needed when you have a reference to the data, module stuff 
>> is
>> when you are calling into code.
> 
> Exactly. In this case, we have reference to the data specific to the
> device in a data structure specific to one session, which doesn't have
> any link back from the device to release it. Thus we need get/put here
> to make sure that data doesn't get released under our feet.
> 

Agree with you, Suzuki and Greg.  Device refcount should be used to
protect the device data and module refcount should be used to protect
module code.

This series is trying to add support to load/unload coresight module.
When there's active session ongoing, coresight framework could call
operation of client driver like sink->enable().  If module is removed
at that time, it will be an issue. Add refcount to module here protect
this kind of situation.

For device count, coresight framework currently doesn't hold device
refcount.  I think that's because coresight client drivers doesn't
support dynamic remove the device. As a consequence of module unload,
devices which are using that module will be removed. However, module
framework will first check the module refcount before calls module exit
which leads to device remove. It will return without doing anything if
refcount of module is hold by coresight framework.

If we want to support dynamic remove device from unbind interface in
coresight driver, we should definitely add device refcount there.
This is out of this series' scope.

Thanks, Tingwei

>> 
>> But note that you do not always need to grab a reference count to the
>> module, as long as the module can properly tear the data down when it 
>> is
>> asked to be removed.  Look at networking drivers as a great example of
>> that.
> 
> Thanks
> 
> Suzuki
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel