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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |