Re: [PATCH v3 00/23] counter: cleanups and device lifetime fixes

[Jarkko Nikula <jarkko.nikula@linux.intel.com>]

Hi

On 1/6/22 17:13, Uwe Kleine-König wrote:
> On Wed, Jan 05, 2022 at 09:26:58PM +0900, William Breathitt Gray wrote:
>> On Wed, Dec 29, 2021 at 04:44:18PM +0100, Uwe Kleine-König wrote:
>>>   - I think intel-qep.c makes the counter unfunctional in
>>>     intel_qep_remove before the counter is unregistered.
>>
>> Hello Uwe,
>>
>> Would you elaborate some more on this? I think intel_qep_remove() is
>> only called after the counter is unregistered because the struct
>> counter_device parent is set to &pci->dev in intel_qep_probe(). Am I
>> misunderstanding the removal path?
> 
> If the counter device is unbound (e.g. via sysfs), the following calls
> are made:
> 
> 	intel_qep_remove() (stopping the hardware?)
> 	devm_counter_release (devm callback of devm_counter_register or ..._add)
> 	then the release callbacks of the earlier devm functions
> 
> My concern is, that in the timeslot between intel_qep_remove() and
> devm_counter_release() the device looks like a functional device and
> might be queried/reconfigured/... while the hardware is already dead.
> 
> It's probably not a big issue (unless for example reading the counter
> this race window makes the hardware hang?), but it's at least ugly.
> Maybe the worst effect is that a counter value is missed (which is OK at
> unregister time). Still it would be nicer to first take down the counter
> device and only then stop the hardware.
> 
In HW point of view it should be safe. We do disable the HW in 
intel_qep_remove() but that doesn't render the HW unusable and registers 
are accessible.

Perhaps that line can go since I think it was put there just to stop the 
HW just in case after remove.

Jarkko