Message ID | 20211229154441.38045-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
---|---|
Headers | show |
Series | counter: cleanups and device lifetime fixes | expand |
Hello, On Wed, Dec 29, 2021 at 04:44:18PM +0100, Uwe Kleine-König wrote: > this is v3 of my series to fix device lifetime issues in the counter > framework. This hopefully addresses all things pointed out for v2. > > Note this depends on 60f07e74f86b (which is in next) now. Full diffstat > below. > > Things that could be further improved: > > [...] > > Uwe Kleine-König (23): > counter: Use container_of instead of drvdata to track counter_device > counter: ftm-quaddec: Drop unused platform_set_drvdata() > counter: microchip-tcb-capture: Drop unused platform_set_drvdata() > counter: Provide a wrapper to access device private data > counter: 104-quad-8: Convert to counter_priv() wrapper > counter: interrupt-cnt: Convert to counter_priv() wrapper > counter: microchip-tcb-capture: Convert to counter_priv() wrapper > counter: intel-qep: Convert to counter_priv() wrapper > counter: ftm-quaddec: Convert to counter_priv() wrapper > counter: ti-eqep: Convert to counter_priv() wrapper > counter: stm32-lptimer-cnt: Convert to counter_priv() wrapper > counter: stm32-timer-cnt: Convert to counter_priv() wrapper > counter: Provide alternative counter registration functions > counter: Update documentation for new counter registration functions > counter: 104-quad-8: Convert to new counter registration > counter: interrupt-cnt: Convert to new counter registration > counter: intel-qep: Convert to new counter registration > counter: ftm-quaddec: Convert to new counter registration > counter: microchip-tcb-capture: Convert to new counter registration > counter: stm32-timer-cnt: Convert to new counter registration > counter: stm32-lptimer-cnt: Convert to new counter registration > counter: ti-eqep: Convert to new counter registration > counter: remove old and now unused registration API > > Documentation/driver-api/generic-counter.rst | 10 +- > drivers/counter/104-quad-8.c | 93 +++++----- > drivers/counter/counter-core.c | 186 ++++++++++++++----- > drivers/counter/ftm-quaddec.c | 36 ++-- > drivers/counter/intel-qep.c | 46 ++--- > drivers/counter/interrupt-cnt.c | 38 ++-- > drivers/counter/microchip-tcb-capture.c | 44 ++--- > drivers/counter/stm32-lptimer-cnt.c | 51 ++--- > drivers/counter/stm32-timer-cnt.c | 48 ++--- > drivers/counter/ti-eqep.c | 31 ++-- > include/linux/counter.h | 15 +- > 11 files changed, 356 insertions(+), 242 deletions(-) > > Range-diff against v2: > [...] > > base-commit: a7904a538933c525096ca2ccde1e60d0ee62c08e > prerequisite-patch-id: 9459ad8bc78190558df9123f8bebe28ca1c396ea All patches have a blessing by at least one of William and Jonathan. The prerequisite commit (60f07e74f86b) is in Greg's char-misc-next branch. Assuming noone still finds a problem in this series that requires me to respin I wonder who will pick it up? Greg? Given that it fixes a possible use-after-free in all counter drivers, I'd like to see it hit before v5.17-rc1. For 5.16 it's probably too late. Best regards Uwe
On Thu, Dec 30, 2021 at 09:53:51AM +0100, Uwe Kleine-König wrote: > Hello, > > On Wed, Dec 29, 2021 at 04:44:18PM +0100, Uwe Kleine-König wrote: > > this is v3 of my series to fix device lifetime issues in the counter > > framework. This hopefully addresses all things pointed out for v2. > > > > Note this depends on 60f07e74f86b (which is in next) now. Full diffstat > > below. > > > > Things that could be further improved: > > > > [...] > > > > Uwe Kleine-König (23): > > counter: Use container_of instead of drvdata to track counter_device > > counter: ftm-quaddec: Drop unused platform_set_drvdata() > > counter: microchip-tcb-capture: Drop unused platform_set_drvdata() > > counter: Provide a wrapper to access device private data > > counter: 104-quad-8: Convert to counter_priv() wrapper > > counter: interrupt-cnt: Convert to counter_priv() wrapper > > counter: microchip-tcb-capture: Convert to counter_priv() wrapper > > counter: intel-qep: Convert to counter_priv() wrapper > > counter: ftm-quaddec: Convert to counter_priv() wrapper > > counter: ti-eqep: Convert to counter_priv() wrapper > > counter: stm32-lptimer-cnt: Convert to counter_priv() wrapper > > counter: stm32-timer-cnt: Convert to counter_priv() wrapper > > counter: Provide alternative counter registration functions > > counter: Update documentation for new counter registration functions > > counter: 104-quad-8: Convert to new counter registration > > counter: interrupt-cnt: Convert to new counter registration > > counter: intel-qep: Convert to new counter registration > > counter: ftm-quaddec: Convert to new counter registration > > counter: microchip-tcb-capture: Convert to new counter registration > > counter: stm32-timer-cnt: Convert to new counter registration > > counter: stm32-lptimer-cnt: Convert to new counter registration > > counter: ti-eqep: Convert to new counter registration > > counter: remove old and now unused registration API > > > > Documentation/driver-api/generic-counter.rst | 10 +- > > drivers/counter/104-quad-8.c | 93 +++++----- > > drivers/counter/counter-core.c | 186 ++++++++++++++----- > > drivers/counter/ftm-quaddec.c | 36 ++-- > > drivers/counter/intel-qep.c | 46 ++--- > > drivers/counter/interrupt-cnt.c | 38 ++-- > > drivers/counter/microchip-tcb-capture.c | 44 ++--- > > drivers/counter/stm32-lptimer-cnt.c | 51 ++--- > > drivers/counter/stm32-timer-cnt.c | 48 ++--- > > drivers/counter/ti-eqep.c | 31 ++-- > > include/linux/counter.h | 15 +- > > 11 files changed, 356 insertions(+), 242 deletions(-) > > > > Range-diff against v2: > > [...] > > > > base-commit: a7904a538933c525096ca2ccde1e60d0ee62c08e > > prerequisite-patch-id: 9459ad8bc78190558df9123f8bebe28ca1c396ea > > All patches have a blessing by at least one of William and Jonathan. > The prerequisite commit (60f07e74f86b) is in Greg's char-misc-next branch. > > Assuming noone still finds a problem in this series that requires me to > respin I wonder who will pick it up? Greg? > > Given that it fixes a possible use-after-free in all counter drivers, > I'd like to see it hit before v5.17-rc1. For 5.16 it's probably too > late. Of course it is too later for 5.16, sorry. I'll queue this up to my tree now, for 5.17-rc1, thanks. greg k-h
On Thu, 30 Dec 2021 09:53:51 +0100 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello, > > On Wed, Dec 29, 2021 at 04:44:18PM +0100, Uwe Kleine-König wrote: > > this is v3 of my series to fix device lifetime issues in the counter > > framework. This hopefully addresses all things pointed out for v2. > > > > Note this depends on 60f07e74f86b (which is in next) now. Full diffstat > > below. > > > > Things that could be further improved: > > > > [...] > > > > Uwe Kleine-König (23): > > counter: Use container_of instead of drvdata to track counter_device > > counter: ftm-quaddec: Drop unused platform_set_drvdata() > > counter: microchip-tcb-capture: Drop unused platform_set_drvdata() > > counter: Provide a wrapper to access device private data > > counter: 104-quad-8: Convert to counter_priv() wrapper > > counter: interrupt-cnt: Convert to counter_priv() wrapper > > counter: microchip-tcb-capture: Convert to counter_priv() wrapper > > counter: intel-qep: Convert to counter_priv() wrapper > > counter: ftm-quaddec: Convert to counter_priv() wrapper > > counter: ti-eqep: Convert to counter_priv() wrapper > > counter: stm32-lptimer-cnt: Convert to counter_priv() wrapper > > counter: stm32-timer-cnt: Convert to counter_priv() wrapper > > counter: Provide alternative counter registration functions > > counter: Update documentation for new counter registration functions > > counter: 104-quad-8: Convert to new counter registration > > counter: interrupt-cnt: Convert to new counter registration > > counter: intel-qep: Convert to new counter registration > > counter: ftm-quaddec: Convert to new counter registration > > counter: microchip-tcb-capture: Convert to new counter registration > > counter: stm32-timer-cnt: Convert to new counter registration > > counter: stm32-lptimer-cnt: Convert to new counter registration > > counter: ti-eqep: Convert to new counter registration > > counter: remove old and now unused registration API > > > > Documentation/driver-api/generic-counter.rst | 10 +- > > drivers/counter/104-quad-8.c | 93 +++++----- > > drivers/counter/counter-core.c | 186 ++++++++++++++----- > > drivers/counter/ftm-quaddec.c | 36 ++-- > > drivers/counter/intel-qep.c | 46 ++--- > > drivers/counter/interrupt-cnt.c | 38 ++-- > > drivers/counter/microchip-tcb-capture.c | 44 ++--- > > drivers/counter/stm32-lptimer-cnt.c | 51 ++--- > > drivers/counter/stm32-timer-cnt.c | 48 ++--- > > drivers/counter/ti-eqep.c | 31 ++-- > > include/linux/counter.h | 15 +- > > 11 files changed, 356 insertions(+), 242 deletions(-) > > > > Range-diff against v2: > > [...] > > > > base-commit: a7904a538933c525096ca2ccde1e60d0ee62c08e > > prerequisite-patch-id: 9459ad8bc78190558df9123f8bebe28ca1c396ea > > All patches have a blessing by at least one of William and Jonathan. For future reference (may be fine this time) William has final say on counter stuff as the maintainer so treat my input as just another set of eyes. Anyhow, plenty of time for any necessary fixes during the RCs so shouldn't be a problem. Jonathan > The prerequisite commit (60f07e74f86b) is in Greg's char-misc-next branch. > > Assuming noone still finds a problem in this series that requires me to > respin I wonder who will pick it up? Greg? > > Given that it fixes a possible use-after-free in all counter drivers, > I'd like to see it hit before v5.17-rc1. For 5.16 it's probably too > late. > > Best regards > Uwe >
On Thu, Dec 30, 2021 at 02:58:26PM +0000, Jonathan Cameron wrote: > On Thu, 30 Dec 2021 09:53:51 +0100 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > Hello, > > > > On Wed, Dec 29, 2021 at 04:44:18PM +0100, Uwe Kleine-König wrote: > > > this is v3 of my series to fix device lifetime issues in the counter > > > framework. This hopefully addresses all things pointed out for v2. > > > > > > Note this depends on 60f07e74f86b (which is in next) now. Full diffstat > > > below. > > > > > > Things that could be further improved: > > > > > > [...] > > > > > > Uwe Kleine-König (23): > > > counter: Use container_of instead of drvdata to track counter_device > > > counter: ftm-quaddec: Drop unused platform_set_drvdata() > > > counter: microchip-tcb-capture: Drop unused platform_set_drvdata() > > > counter: Provide a wrapper to access device private data > > > counter: 104-quad-8: Convert to counter_priv() wrapper > > > counter: interrupt-cnt: Convert to counter_priv() wrapper > > > counter: microchip-tcb-capture: Convert to counter_priv() wrapper > > > counter: intel-qep: Convert to counter_priv() wrapper > > > counter: ftm-quaddec: Convert to counter_priv() wrapper > > > counter: ti-eqep: Convert to counter_priv() wrapper > > > counter: stm32-lptimer-cnt: Convert to counter_priv() wrapper > > > counter: stm32-timer-cnt: Convert to counter_priv() wrapper > > > counter: Provide alternative counter registration functions > > > counter: Update documentation for new counter registration functions > > > counter: 104-quad-8: Convert to new counter registration > > > counter: interrupt-cnt: Convert to new counter registration > > > counter: intel-qep: Convert to new counter registration > > > counter: ftm-quaddec: Convert to new counter registration > > > counter: microchip-tcb-capture: Convert to new counter registration > > > counter: stm32-timer-cnt: Convert to new counter registration > > > counter: stm32-lptimer-cnt: Convert to new counter registration > > > counter: ti-eqep: Convert to new counter registration > > > counter: remove old and now unused registration API > > > > > > Documentation/driver-api/generic-counter.rst | 10 +- > > > drivers/counter/104-quad-8.c | 93 +++++----- > > > drivers/counter/counter-core.c | 186 ++++++++++++++----- > > > drivers/counter/ftm-quaddec.c | 36 ++-- > > > drivers/counter/intel-qep.c | 46 ++--- > > > drivers/counter/interrupt-cnt.c | 38 ++-- > > > drivers/counter/microchip-tcb-capture.c | 44 ++--- > > > drivers/counter/stm32-lptimer-cnt.c | 51 ++--- > > > drivers/counter/stm32-timer-cnt.c | 48 ++--- > > > drivers/counter/ti-eqep.c | 31 ++-- > > > include/linux/counter.h | 15 +- > > > 11 files changed, 356 insertions(+), 242 deletions(-) > > > > > > Range-diff against v2: > > > [...] > > > > > > base-commit: a7904a538933c525096ca2ccde1e60d0ee62c08e > > > prerequisite-patch-id: 9459ad8bc78190558df9123f8bebe28ca1c396ea > > > > All patches have a blessing by at least one of William and Jonathan. > > For future reference (may be fine this time) William has final say on counter > stuff as the maintainer so treat my input as just another set of eyes. Yeah, right. William only didn't ack patch 13 but wrote in reply it in v2: I agree with the approach taken in this patch, and I don't have much to add after the suggestions Lars-Peter and Jonathan have already given. So assuming those are addressed in the next version I expect to Ack this patch as well. So I assume it's just that William didn't have the time yet to look into v3 (or v4 that I just sent out) yet. Best regards and thanks to all who gave feedback to improve this patch set, Uwe
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? Thanks, William Breathitt Gray
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
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