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

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

[Uwe Kleine-König]

Hello,

this is v2 of this series, it's goal is to fix struct device lifetime issues as
pointed out in patch #13. The patches up to patch #12 are only prepatory and
cleanup patches. Patch #13 provides the needed functions to fix the issues in
all drivers (patches #15 to #22). The last patch removes the then unused API
calls.

The changes compared to v1 is only build fixes that I missed to include in v1,
they were only in my working copy. Additionally I changed:

diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index cdc6004a7e77..3f7dc5718423 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c
@@ -27,7 +27,7 @@ static DEFINE_IDA(counter_ida);
 
 struct counter_device_allochelper {
 	struct counter_device counter;
-	unsigned long privdata[0];
+	unsigned long privdata[];
 };
 
 static void counter_device_release(struct device *dev)

The stm32-timer-cnt driver was used to test
this series, the other drivers are only compile tested.


To complete the information from the v1 thread: There are a few more
issues I found while working on this patch set:

 - 104_QUAD_8 depends on X86, but compiles fine on ARCH=arm. Maybe
   adding support for COMPILE_TEST would be a good idea.

 - 104-quad-8.c uses devm_request_irq() and (now) devm_counter_add(). On
   unbind an irq might be pending which results in quad8_irq_handler()
   calling counter_push_event() for a counter that is already
   unregistered. (The issue exists also without my changes.)

 - I think intel-qep.c makes the counter unfunctional in
   intel_qep_remove before the counter is unregistered.

 - I wonder why counter is a bus and not a class device type. There is
   no driver that would ever bind a counter device, is there? So
   /sys/bus/counter/driver is always empty.

Do whatever you want with this list, I won't address these in the near
future.

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               | 168 +++++++++++++------
 drivers/counter/ftm-quaddec.c                |  37 ++--
 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                    |  47 +++---
 include/linux/counter.h                      |  15 +-
 11 files changed, 348 insertions(+), 249 deletions(-)


base-commit: a7904a538933c525096ca2ccde1e60d0ee62c08e
-- 
2.33.0

[PATCH v2 14/23] counter: Update documentation for new counter registration functions

[Uwe Kleine-König]

In order to replace the counter registration API also update the
documentation to the new way.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 Documentation/driver-api/generic-counter.rst | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
index 1b487a331467..991b180c7b47 100644
--- a/Documentation/driver-api/generic-counter.rst
+++ b/Documentation/driver-api/generic-counter.rst
@@ -262,11 +262,11 @@ order to communicate with the device: to read and write various Signals
 and Counts, and to set and get the "action mode" and "function mode" for
 various Synapses and Counts respectively.
 
-A defined counter_device structure may be registered to the system by
-passing it to the counter_register function, and unregistered by passing
-it to the counter_unregister function. Similarly, the
-devm_counter_register function may be used if device memory-managed
-registration is desired.
+A counter_device structure is supposed to be allocated using counter_alloc()
+and may be registered to the system by passing it to the counter_add()
+function, and unregistered by passing it to the counter_unregister function.
+There are device managed variants of these functions: devm_counter_alloc() and
+devm_counter_add().
 
 The struct counter_comp structure is used to define counter extensions
 for Signals, Synapses, and Counts.
-- 
2.33.0

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

[Lars-Peter Clausen]

On 12/27/21 10:45 AM, Uwe Kleine-König wrote:
> [...]
>
>   - I wonder why counter is a bus and not a class device type. There is
>     no driver that would ever bind a counter device, is there? So
>     /sys/bus/counter/driver is always empty.
>
There used to be a time when GKH said that we do not want new driver 
classes. And all new subsystems should use bus since bus is a superset 
of class. This restriction has been eased since then.

But it was around when the IIO subsystem was merged and since the 
counter subsystem originated from the IIO subsystem I assume it just 
copied this.

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

[Jonathan Cameron]

On Mon, 27 Dec 2021 13:25:25 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 12/27/21 10:45 AM, Uwe Kleine-König wrote:
> > [...]
> >
> >   - I wonder why counter is a bus and not a class device type. There is
> >     no driver that would ever bind a counter device, is there? So
> >     /sys/bus/counter/driver is always empty.
> >  
> There used to be a time when GKH said that we do not want new driver 
> classes. And all new subsystems should use bus since bus is a superset 
> of class. This restriction has been eased since then.
> 
> But it was around when the IIO subsystem was merged and since the 
> counter subsystem originated from the IIO subsystem I assume it just 
> copied this.
> 

Yup. Discussion about this back then with one view being there
should never have been class in the first place.

https://lore.kernel.org/lkml/4B571DA4.6070603@cam.ac.uk/

For anyone who loves the history of these things...

FWIW I think Greg suggested IIO should be a bus because we were hanging
a bunch of different types of device off a class and it was getting messy.
Kay then gave some history on class vs bus and suggested no new
subsystem should use class.

Ah well, opinions change over time!

Also interesting to see we were discussing a bridge to input all that
time ago and it's still not gone beyond various prototypes (with
exception of touch screens).

Jonathan

Re: [PATCH v2 14/23] counter: Update documentation for new counter registration functions

[Jonathan Cameron]

On Mon, 27 Dec 2021 10:45:17 +0100
Uwe Kleine-König         <u.kleine-koenig@pengutronix.de> wrote:

> In order to replace the counter registration API also update the
> documentation to the new way.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Fine either way, but a suggestion below.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  Documentation/driver-api/generic-counter.rst | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> index 1b487a331467..991b180c7b47 100644
> --- a/Documentation/driver-api/generic-counter.rst
> +++ b/Documentation/driver-api/generic-counter.rst
> @@ -262,11 +262,11 @@ order to communicate with the device: to read and write various Signals
>  and Counts, and to set and get the "action mode" and "function mode" for
>  various Synapses and Counts respectively.
>  
> -A defined counter_device structure may be registered to the system by
> -passing it to the counter_register function, and unregistered by passing
> -it to the counter_unregister function. Similarly, the
> -devm_counter_register function may be used if device memory-managed
> -registration is desired.
> +A counter_device structure is supposed to be allocated using counter_alloc()
> +and may be registered to the system by passing it to the counter_add()
> +function, and unregistered by passing it to the counter_unregister function.

I'd avoid the supposed to and the odd vague use of structure in the origin
text and just go with

A struct counter_device is allocated using counter_alloc()...


> +There are device managed variants of these functions: devm_counter_alloc() and
> +devm_counter_add().
>  
>  The struct counter_comp structure is used to define counter extensions
>  for Signals, Synapses, and Counts.

Re: [PATCH v2 14/23] counter: Update documentation for new counter registration functions

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

On Tue, Dec 28, 2021 at 06:12:22PM +0000, Jonathan Cameron wrote:
> On Mon, 27 Dec 2021 10:45:17 +0100
> Uwe Kleine-König         <u.kleine-koenig@pengutronix.de> wrote:
> 
> > In order to replace the counter registration API also update the
> > documentation to the new way.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Fine either way, but a suggestion below.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > ---
> >  Documentation/driver-api/generic-counter.rst | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> > index 1b487a331467..991b180c7b47 100644
> > --- a/Documentation/driver-api/generic-counter.rst
> > +++ b/Documentation/driver-api/generic-counter.rst
> > @@ -262,11 +262,11 @@ order to communicate with the device: to read and write various Signals
> >  and Counts, and to set and get the "action mode" and "function mode" for
> >  various Synapses and Counts respectively.
> >  
> > -A defined counter_device structure may be registered to the system by
> > -passing it to the counter_register function, and unregistered by passing
> > -it to the counter_unregister function. Similarly, the
> > -devm_counter_register function may be used if device memory-managed
> > -registration is desired.
> > +A counter_device structure is supposed to be allocated using counter_alloc()
> > +and may be registered to the system by passing it to the counter_add()
> > +function, and unregistered by passing it to the counter_unregister function.
> 
> I'd avoid the supposed to and the odd vague use of structure in the origin
> text and just go with
> 
> A struct counter_device is allocated using counter_alloc()...

I like this simpler wording as well.

Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>

> 
> 
> > +There are device managed variants of these functions: devm_counter_alloc() and
> > +devm_counter_add().
> >  
> >  The struct counter_comp structure is used to define counter extensions
> >  for Signals, Synapses, and Counts.
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

On Tue, Dec 28, 2021 at 05:35:58PM +0000, Jonathan Cameron wrote:
> On Mon, 27 Dec 2021 13:25:25 +0100
> Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
> > On 12/27/21 10:45 AM, Uwe Kleine-König wrote:
> > > [...]
> > >
> > >   - I wonder why counter is a bus and not a class device type. There is
> > >     no driver that would ever bind a counter device, is there? So
> > >     /sys/bus/counter/driver is always empty.
> > >  
> > There used to be a time when GKH said that we do not want new driver 
> > classes. And all new subsystems should use bus since bus is a superset 
> > of class. This restriction has been eased since then.
> > 
> > But it was around when the IIO subsystem was merged and since the 
> > counter subsystem originated from the IIO subsystem I assume it just 
> > copied this.
> > 
> 
> Yup. Discussion about this back then with one view being there
> should never have been class in the first place.
> 
> https://lore.kernel.org/lkml/4B571DA4.6070603@cam.ac.uk/
> 
> For anyone who loves the history of these things...
> 
> FWIW I think Greg suggested IIO should be a bus because we were hanging
> a bunch of different types of device off a class and it was getting messy.
> Kay then gave some history on class vs bus and suggested no new
> subsystem should use class.
> 
> Ah well, opinions change over time!
> 
> Also interesting to see we were discussing a bridge to input all that
> time ago and it's still not gone beyond various prototypes (with
> exception of touch screens).
> 
> Jonathan

Yes this is the reason: Counter subsystem just followed the structure of
the IIO subsystem originally which is how it ended up as a bus; changing
it to a class now would break userspace expectations so that is why it
remains a bus still.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]