Introduce the Counter character device interface

Duplicate ABIs are not valid, so let's consolidate these sysfs attributes into the main sysfs-bus-counter documentation file.

Cc Patrick Havelange <[email protected]>

Reviewed-by David Lechner <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-counter   | 76 ++++++++++++++++++-
 .../ABI/testing/sysfs-bus-counter-104-quad-8  | 61 ---------------
 .../ABI/testing/sysfs-bus-counter-ftm-quaddec | 16 ----
 MAINTAINERS                                   |  4 +-
 4 files changed, 75 insertions(+), 82 deletions(-)
 delete mode 100644 Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
 delete mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ftm-quaddec

Documentation/ABI/testing/sysfs-bus-counter +74 -2

index e9d9e50f03be..20fe5afd4f9e 100644
@@ -57,6 +57,7 @@ Description:
 What:		/sys/bus/counter/devices/counterX/countY/count_mode_available
 What:		/sys/bus/counter/devices/counterX/countY/error_noise_available
 What:		/sys/bus/counter/devices/counterX/countY/function_available
+What:		/sys/bus/counter/devices/counterX/countY/prescaler_available
 What:		/sys/bus/counter/devices/counterX/countY/signalZ_action_available
 KernelVersion:	5.2
 Contact:	[email protected]

@@ -154,6 +155,15 @@ Description:
 		Count Y. If possible, this should match the name of the
 		respective channel as it appears in the device datasheet.
 
+What:		/sys/bus/counter/devices/counterX/countY/prescaler
+KernelVersion:	5.2
+Contact:	[email protected]
+Description:
+		Configure the prescaler value associated with Count Y.
+		On the FlexTimer, the counter clock source passes through a
+		prescaler (i.e. a counter). This acts like a clock
+		divider.
+
 What:		/sys/bus/counter/devices/counterX/countY/preset
 KernelVersion:	5.2
 Contact:	[email protected]

@@ -224,11 +234,45 @@ Description:
 		Read-only attribute that indicates the total number of Signals
 		belonging to the Counter.
 
-What:		/sys/bus/counter/devices/counterX/signalY/signal
+What:		/sys/bus/counter/devices/counterX/signalY/cable_fault
+KernelVersion:	5.7
+Contact:	[email protected]
+Description:
+		Read-only attribute that indicates whether a differential
+		encoder cable fault (not connected or loose wires) is detected
+		for the respective channel of Signal Y. Valid attribute values
+		are boolean. Detection must first be enabled via the
+		corresponding cable_fault_enable attribute.
+
+What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable
+KernelVersion:	5.7
+Contact:	[email protected]
+Description:
+		Whether detection of differential encoder cable faults for the
+		respective channel of Signal Y is enabled. Valid attribute
+		values are boolean.
+
+What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler
+KernelVersion:	5.7
+Contact:	[email protected]
+Description:
+		Filter clock factor for input Signal Y. This prescaler value
+		affects the inputs of both quadrature pair signals.
+
+What:		/sys/bus/counter/devices/counterX/signalY/index_polarity
 KernelVersion:	5.2
 Contact:	[email protected]
 Description:
-		Signal data of Signal Y represented as a string.
+		Active level of index input Signal Y; irrelevant in
+		non-synchronous load mode.
+
+What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
+What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
+KernelVersion:	5.2
+Contact:	[email protected]
+Description:
+		Discrete set of available values for the respective Signal Y
+		configuration are listed in this file.
 
 What:		/sys/bus/counter/devices/counterX/signalY/name
 KernelVersion:	5.2

@@ -237,3 +281,31 @@ Description:
 		Read-only attribute that indicates the device-specific name of
 		Signal Y. If possible, this should match the name of the
 		respective signal as it appears in the device datasheet.
+
+What:		/sys/bus/counter/devices/counterX/signalY/signal
+KernelVersion:	5.2
+Contact:	[email protected]
+Description:
+		Signal data of Signal Y represented as a string.
+
+What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode
+KernelVersion:	5.2
+Contact:	[email protected]
+Description:
+		Configure the counter associated with Signal Y for
+		non-synchronous or synchronous load mode. Synchronous load mode
+		cannot be selected in non-quadrature (Pulse-Direction) clock
+		mode.
+
+		non-synchronous:
+			A logic low level is the active level at this index
+			input. The index function (as enabled via preset_enable)
+			is performed directly on the active level of the index
+			input.
+
+		synchronous:
+			Intended for interfacing with encoder Index output in
+			quadrature clock mode. The active level is configured
+			via index_polarity. The index function (as enabled via
+			preset_enable) is performed synchronously with the
+			quadrature clock on the active level of the index input.

Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 deleted +0 -61

deleted file mode 100644
index eac32180c40d..000000000000
@@ -1,61 +0,0 @@
-What:		/sys/bus/counter/devices/counterX/signalY/cable_fault
-KernelVersion:	5.7
-Contact:	[email protected]
-Description:
-		Read-only attribute that indicates whether a differential
-		encoder cable fault (not connected or loose wires) is detected
-		for the respective channel of Signal Y. Valid attribute values
-		are boolean. Detection must first be enabled via the
-		corresponding cable_fault_enable attribute.
-
-What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable
-KernelVersion:	5.7
-Contact:	[email protected]
-Description:
-		Whether detection of differential encoder cable faults for the
-		respective channel of Signal Y is enabled. Valid attribute
-		values are boolean.
-
-What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler
-KernelVersion:	5.7
-Contact:	[email protected]
-Description:
-		Filter clock factor for input Signal Y. This prescaler value
-		affects the inputs of both quadrature pair signals.
-
-What:		/sys/bus/counter/devices/counterX/signalY/index_polarity
-KernelVersion:	5.2
-Contact:	[email protected]
-Description:
-		Active level of index input Signal Y; irrelevant in
-		non-synchronous load mode.
-
-What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
-What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
-KernelVersion:	5.2
-Contact:	[email protected]
-Description:
-		Discrete set of available values for the respective Signal Y
-		configuration are listed in this file.
-
-What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode
-KernelVersion:	5.2
-Contact:	[email protected]
-Description:
-		Configure the counter associated with Signal Y for
-		non-synchronous or synchronous load mode. Synchronous load mode
-		cannot be selected in non-quadrature (Pulse-Direction) clock
-		mode.
-
-		non-synchronous:
-			A logic low level is the active level at this index
-			input. The index function (as enabled via preset_enable)
-			is performed directly on the active level of the index
-			input.
-
-		synchronous:
-			Intended for interfacing with encoder Index output in
-			quadrature clock mode. The active level is configured
-			via index_polarity. The index function (as enabled via
-			preset_enable) is performed synchronously with the
-			quadrature clock on the active level of the index input.

Documentation/ABI/testing/sysfs-bus-counter-ftm-quaddec deleted +0 -16

deleted file mode 100644
index 7d2e7b363467..000000000000
@@ -1,16 +0,0 @@
-What:		/sys/bus/counter/devices/counterX/countY/prescaler_available
-KernelVersion:	5.2
-Contact:	[email protected]
-Description:
-		Discrete set of available values for the respective Count Y
-		configuration are listed in this file. Values are delimited by
-		newline characters.
-
-What:		/sys/bus/counter/devices/counterX/countY/prescaler
-KernelVersion:	5.2
-Contact:	[email protected]
-Description:
-		Configure the prescaler value associated with Count Y.
-		On the FlexTimer, the counter clock source passes through a
-		prescaler (i.e. a counter). This acts like a clock
-		divider.

MAINTAINERS +1 -3

index e679d422b472..8fa9202610c5 100644
@@ -299,7 +299,6 @@ M:	William Breathitt Gray <[email protected]>
 M:	Syed Nayyar Waris <[email protected]>
 L:	[email protected]
 S:	Maintained
-F:	Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
 F:	drivers/counter/104-quad-8.c
 
 ACCES PCI-IDIO-16 GPIO DRIVER

@@ -4675,7 +4674,7 @@ COUNTER SUBSYSTEM
 M:	William Breathitt Gray <[email protected]>
 L:	[email protected]
 S:	Maintained
-F:	Documentation/ABI/testing/sysfs-bus-counter*
+F:	Documentation/ABI/testing/sysfs-bus-counter
 F:	Documentation/driver-api/generic-counter.rst
 F:	drivers/counter/
 F:	include/linux/counter.h

@@ -7087,7 +7086,6 @@ FLEXTIMER FTM-QUADDEC DRIVER
 M:	Patrick Havelange <[email protected]>
 L:	[email protected]
 S:	Maintained
-F:	Documentation/ABI/testing/sysfs-bus-counter-ftm-quaddec
 F:	Documentation/devicetree/bindings/counter/ftm-quaddec.txt
 F:	drivers/counter/ftm-quaddec.c
 

"Miscellaneous" is the correct spelling.

Reviewed-by David Lechner <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 Documentation/driver-api/generic-counter.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/driver-api/generic-counter.rst +1 -1

index b02c52cd69d6..64fe7db080e5 100644
@@ -307,7 +307,7 @@ Determining the type of extension to create is a matter of scope.
 
 * Device extensions are attributes that expose information/control
   non-specific to a particular Count or Signal. This is where you would
-  put your global features or other miscellanous functionality.
+  put your global features or other miscellaneous functionality.
 
   For example, if your device has an overtemp sensor, you can report the
   chip overheated via a device extension called "error_overtemp":

It is obvious that devm_counter_register() is used to register a Counter device, so a comment stating such is pointless here.

Acked-by Syed Nayyar Waris <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/104-quad-8.c | 1 -
 1 file changed, 1 deletion(-)

drivers/counter/104-quad-8.c +0 -1

index 9691f8612be8..4bb9abffae48 100644
@@ -1082,7 +1082,6 @@ static int quad8_probe(struct device *dev, unsigned int id)
 	/* Enable all counters */
 	outb(QUAD8_CHAN_OP_ENABLE_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
 
-	/* Register Counter device */
 	return devm_counter_register(dev, &priv->counter);
 }
 

The 104-QUAD-8 only has two count modes where a ceiling value makes
sense: Range Limit and Modulo-N. Outside of these two modes, setting a ceiling value is an invalid operation -- so let's report it as such by returning -EINVAL.

Fixes fc069262261c ("counter: 104-quad-8: Add lock guards - generic interface")

Acked-by Syed Nayyar Waris <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

drivers/counter/104-quad-8.c +2 -2

 		quad8_preset_register_set(priv, count->id, ceiling);
-		break;
+		return len;
 	}
 
 	mutex_unlock(&priv->lock);
 
-	return len;
+	return -EINVAL;
 }
 
 static ssize_t quad8_count_preset_enable_read(struct counter_device *counter,

Quoted 5 lines · On Wed, 9 Jun 2021 10:31:07 +0900 William Breathitt Gray…

The 104-QUAD-8 only has two count modes where a ceiling value makes
sense: Range Limit and Modulo-N. Outside of these two modes, setting a ceiling value is an invalid operation -- so let's report it as such by returning -EINVAL.

Quoted 3 lines

Fixes fc069262261c ("counter: 104-quad-8: Add lock guards - generic interface")

Acked-by Syed Nayyar Waris <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

We seem to have some inconsistency in the driver about whether we hold the priv->lock when calling quad8_preset_register_set()

Can we look to make that consistent?

drivers/counter/104-quad-8.c static ssize_t quad8_count_ceiling_write(struct counter_device *counter, · line 717

Quoted 13 lines

---
 drivers/counter/104-quad-8.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/counter/104-quad-8.c +1 -0

index 4bb9abffae48..233a3acc1377 100644
@@ -714,13 +714,14 @@ static ssize_t quad8_count_ceiling_write(struct counter_device *counter,
 	switch (priv->count_mode[count->id]) {
 	case 1:
 	case 3:
+		mutex_unlock(&priv->lock);

Probably swap this line and the next one...]

Quoted 12 lines

drivers/counter/104-quad-8.c +2 -2

 		quad8_preset_register_set(priv, count->id, ceiling);
-		break;
+		return len;
 	}
 
 	mutex_unlock(&priv->lock);
 
-	return len;
+	return -EINVAL;
 }
 
 static ssize_t quad8_count_preset_enable_read(struct counter_device *counter,

When the kernel is running in secure boot mode, we lock down the kernel to prevent userspace from modifying the running kernel image. Whilst this includes prohibiting access to things like /dev/mem, it must also prevent access by means of configuring driver modules in such a way as to cause a device to access or modify the kernel image.

To this end, annotate module_param* statements that refer to hardware configuration and indicate for future reference what type of parameter they specify. The parameter parser in the core sees this information and can skip such parameters with an error message if the kernel is locked down. The module initialisation then runs as normal, but just sees whatever the default values for those parameters is.

Note that we do still need to do the module initialisation because some drivers have viable defaults set in case parameters aren't specified and some drivers support automatic configuration (e.g. PNP or PCI) in addition to manually coded parameters.

This patch annotates the 104-QUAD-8 driver.

Acked-by Syed Nayyar Waris <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/104-quad-8.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/104-quad-8.c +1 -1

index 233a3acc1377..0fd61cc82d30 100644
@@ -21,7 +21,7 @@
 
 static unsigned int base[max_num_isa_dev(QUAD8_EXTENT)];
 static unsigned int num_quad8;
-module_param_array(base, uint, &num_quad8, 0);
+module_param_hw_array(base, uint, ioport, &num_quad8, 0);
 MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
 
 #define QUAD8_NUM_COUNTERS 8

Add some safety by qualifying the quad8_preset_register_set() function parameters as const.

Acked-by Syed Nayyar Waris <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/104-quad-8.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/counter/104-quad-8.c +2 -2

index 0fd61cc82d30..51fba8cf9c2a 100644
@@ -632,8 +632,8 @@ static ssize_t quad8_count_preset_read(struct counter_device *counter,
 	return sprintf(buf, "%u\n", priv->preset[count->id]);
 }
 
-static void quad8_preset_register_set(struct quad8 *priv, int id,
-				      unsigned int preset)
+static void quad8_preset_register_set(struct quad8 *const priv, const int id,
+				      const unsigned int preset)
 {
 	const unsigned int base_offset = priv->base + 2 * id;
 	int i;

The struct counter_count functions_list member expects a const enum counter_count_function array. This patch adds the const qualifier to the quad8_count_functions_list to match functions_list.

Acked-by Syed Nayyar Waris <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/104-quad-8.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/104-quad-8.c +1 -1

index 51fba8cf9c2a..ae89ad7a91c6 100644
@@ -193,7 +193,7 @@ enum quad8_count_function {
 	QUAD8_COUNT_FUNCTION_QUADRATURE_X4
 };
 
-static enum counter_count_function quad8_count_functions_list[] = {
+static const enum counter_count_function quad8_count_functions_list[] = {
 	[QUAD8_COUNT_FUNCTION_PULSE_DIRECTION] = COUNTER_COUNT_FUNCTION_PULSE_DIRECTION,
 	[QUAD8_COUNT_FUNCTION_QUADRATURE_X1] = COUNTER_COUNT_FUNCTION_QUADRATURE_X1_A,
 	[QUAD8_COUNT_FUNCTION_QUADRATURE_X2] = COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A,

The struct counter_count functions_list member expects a const enum counter_count_function array. This patch adds the const qualifier to the interrupt__cnt_functions to match functions_list.

Cc Oleksij Rempel <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/interrupt-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/interrupt-cnt.c +1 -1

index a99ee7996977..827d785e19b4 100644
@@ -112,7 +112,7 @@ static int interrupt_cnt_write(struct counter_device *counter,
 	return 0;
 }
 
-static enum counter_count_function interrupt_cnt_functions[] = {
+static const enum counter_count_function interrupt_cnt_functions[] = {
 	COUNTER_COUNT_FUNCTION_INCREASE,
 };
 

The struct counter_count functions_list member expects a const enum counter_count_function array. This patch adds the const qualifier to the mchp_tc_count_functions to match functions_list.

Cc Kamel Bouhara <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/microchip-tcb-capture.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/microchip-tcb-capture.c +1 -1

index 710acc0a3704..cabcfebfc799 100644
@@ -37,7 +37,7 @@ enum mchp_tc_count_function {
 	MCHP_TC_FUNCTION_QUADRATURE,
 };
 
-static enum counter_count_function mchp_tc_count_functions[] = {
+static const enum counter_count_function mchp_tc_count_functions[] = {
 	[MCHP_TC_FUNCTION_INCREASE] = COUNTER_COUNT_FUNCTION_INCREASE,
 	[MCHP_TC_FUNCTION_QUADRATURE] = COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
 };

The struct counter_count functions_list member expects a const enum counter_count_function array. This patch adds the const qualifier to the stm32_lptim_cnt_functions to match functions_list.

Reviewed-by Fabrice Gasnier <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/stm32-lptimer-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/stm32-lptimer-cnt.c +1 -1

index 937439635d53..0f7d3f1ec1b6 100644
@@ -134,7 +134,7 @@ enum stm32_lptim_cnt_function {
 	STM32_LPTIM_ENCODER_BOTH_EDGE,
 };
 
-static enum counter_count_function stm32_lptim_cnt_functions[] = {
+static const enum counter_count_function stm32_lptim_cnt_functions[] = {
 	[STM32_LPTIM_COUNTER_INCREASE] = COUNTER_COUNT_FUNCTION_INCREASE,
 	[STM32_LPTIM_ENCODER_BOTH_EDGE] = COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
 };

The struct counter_count functions_list member expects a const enum counter_count_function array. This patch adds the const qualifier to the stm32_count_functions to match functions_list.

Cc Benjamin Gaignard <[email protected]>

Reviewed-by Fabrice Gasnier <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/stm32-timer-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/stm32-timer-cnt.c +1 -1

index 75bc401fdd18..0c18573a7837 100644
@@ -50,7 +50,7 @@ enum stm32_count_function {
 	STM32_COUNT_ENCODER_MODE_3,
 };
 
-static enum counter_count_function stm32_count_functions[] = {
+static const enum counter_count_function stm32_count_functions[] = {
 	[STM32_COUNT_SLAVE_MODE_DISABLED] = COUNTER_COUNT_FUNCTION_INCREASE,
 	[STM32_COUNT_ENCODER_MODE_1] = COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A,
 	[STM32_COUNT_ENCODER_MODE_2] = COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B,

The struct counter_synapse actions_list member expects a const enum counter_synapse_action array. This patch adds the const qualifier to the quad8_index_actions_list and quad8_synapse_actions_list to match actions_list.

Acked-by Syed Nayyar Waris <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/104-quad-8.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/counter/104-quad-8.c +2 -2

index ae89ad7a91c6..09d779544969 100644
@@ -305,12 +305,12 @@ enum quad8_synapse_action {
 	QUAD8_SYNAPSE_ACTION_BOTH_EDGES
 };
 
-static enum counter_synapse_action quad8_index_actions_list[] = {
+static const enum counter_synapse_action quad8_index_actions_list[] = {
 	[QUAD8_SYNAPSE_ACTION_NONE] = COUNTER_SYNAPSE_ACTION_NONE,
 	[QUAD8_SYNAPSE_ACTION_RISING_EDGE] = COUNTER_SYNAPSE_ACTION_RISING_EDGE
 };
 
-static enum counter_synapse_action quad8_synapse_actions_list[] = {
+static const enum counter_synapse_action quad8_synapse_actions_list[] = {
 	[QUAD8_SYNAPSE_ACTION_NONE] = COUNTER_SYNAPSE_ACTION_NONE,
 	[QUAD8_SYNAPSE_ACTION_RISING_EDGE] = COUNTER_SYNAPSE_ACTION_RISING_EDGE,
 	[QUAD8_SYNAPSE_ACTION_FALLING_EDGE] = COUNTER_SYNAPSE_ACTION_FALLING_EDGE,

The struct counter_synapse actions_list member expects a const enum counter_synapse_action array. This patch adds the const qualifier to the ftm_quaddec_synapse_actions to match actions_list.

Cc Patrick Havelange <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/ftm-quaddec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/ftm-quaddec.c +1 -1

index c2b3fdfd8b77..9371532406ca 100644
@@ -162,7 +162,7 @@ enum ftm_quaddec_synapse_action {
 	FTM_QUADDEC_SYNAPSE_ACTION_BOTH_EDGES,
 };
 
-static enum counter_synapse_action ftm_quaddec_synapse_actions[] = {
+static const enum counter_synapse_action ftm_quaddec_synapse_actions[] = {
 	[FTM_QUADDEC_SYNAPSE_ACTION_BOTH_EDGES] =
 	COUNTER_SYNAPSE_ACTION_BOTH_EDGES
 };

The struct counter_synapse actions_list member expects a const enum counter_synapse_action array. This patch adds the const qualifier to the interrupt_cnt_synapse_actionss to match actions_list.

Cc Oleksij Rempel <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/interrupt-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/interrupt-cnt.c +0 -0

index 827d785e19b4..0e07607f2cd3 100644

drivers/counter/interrupt-cnt.c +0 -0

 	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
 };
 

Quoted 6 lines · On Wed, 9 Jun 2021 10:31:17 +0900 William Breathitt Gray…

The struct counter_synapse actions_list member expects a const enum counter_synapse_action array. This patch adds the const qualifier to the interrupt_cnt_synapse_actionss to match actions_list.

Cc Oleksij Rempel <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

Naming looks unusual...

Quoted 12 lines

---
 drivers/counter/interrupt-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/interrupt-cnt.c +0 -0

index 827d785e19b4..0e07607f2cd3 100644
@@ -77,7 +77,7 @@ static const struct counter_count_ext interrupt_cnt_ext[] = {
 	},
 };
 

drivers/counter/interrupt-cnt.c static const struct counter_count_ext interrupt_cnt_ext[] = { · line 80

Quoted 2 lines

drivers/counter/interrupt-cnt.c +1 -1

-static enum counter_synapse_action interrupt_cnt_synapse_actionss[] = {
+static const enum counter_synapse_action interrupt_cnt_synapse_actionss[] = {

actions?

Perhaps good to fix that whilst we are here.

Quoted 3 lines

drivers/counter/interrupt-cnt.c +0 -0

 	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
 };
 

The struct counter_synapse actions_list member expects a const enum counter_synapse_action array. This patch adds the const qualifier to the mchp_tc_synapse_actions to match actions_list.

Cc Kamel Bouhara <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/microchip-tcb-capture.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/microchip-tcb-capture.c +1 -1

index cabcfebfc799..51b8af80f98b 100644
@@ -49,7 +49,7 @@ enum mchp_tc_synapse_action {
 	MCHP_TC_SYNAPSE_ACTION_BOTH_EDGE
 };
 
-static enum counter_synapse_action mchp_tc_synapse_actions[] = {
+static const enum counter_synapse_action mchp_tc_synapse_actions[] = {
 	[MCHP_TC_SYNAPSE_ACTION_NONE] = COUNTER_SYNAPSE_ACTION_NONE,
 	[MCHP_TC_SYNAPSE_ACTION_RISING_EDGE] = COUNTER_SYNAPSE_ACTION_RISING_EDGE,
 	[MCHP_TC_SYNAPSE_ACTION_FALLING_EDGE] = COUNTER_SYNAPSE_ACTION_FALLING_EDGE,

The struct counter_synapse actions_list member expects a const enum counter_synapse_action array. This patch adds the const qualifier to the stm32_lptim_cnt_synapse_actions to match actions_list.

Reviewed-by Fabrice Gasnier <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/stm32-lptimer-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/stm32-lptimer-cnt.c +1 -1

index 0f7d3f1ec1b6..c19d998df5ba 100644
@@ -146,7 +146,7 @@ enum stm32_lptim_synapse_action {
 	STM32_LPTIM_SYNAPSE_ACTION_NONE,
 };
 
-static enum counter_synapse_action stm32_lptim_cnt_synapse_actions[] = {
+static const enum counter_synapse_action stm32_lptim_cnt_synapse_actions[] = {
 	/* Index must match with stm32_lptim_cnt_polarity[] (priv->polarity) */
 	[STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE] = COUNTER_SYNAPSE_ACTION_RISING_EDGE,
 	[STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE] = COUNTER_SYNAPSE_ACTION_FALLING_EDGE,

The struct counter_synapse actions_list member expects a const enum counter_synapse_action array. This patch adds the const qualifier to the stm32_synapse_actions to match actions_list.

Cc Benjamin Gaignard <[email protected]>

Reviewed-by Fabrice Gasnier <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/stm32-timer-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/stm32-timer-cnt.c +1 -1

index 0c18573a7837..603b30ada839 100644
@@ -267,7 +267,7 @@ enum stm32_synapse_action {
 	STM32_SYNAPSE_ACTION_BOTH_EDGES
 };
 
-static enum counter_synapse_action stm32_synapse_actions[] = {
+static const enum counter_synapse_action stm32_synapse_actions[] = {
 	[STM32_SYNAPSE_ACTION_NONE] = COUNTER_SYNAPSE_ACTION_NONE,
 	[STM32_SYNAPSE_ACTION_BOTH_EDGES] = COUNTER_SYNAPSE_ACTION_BOTH_EDGES
 };

Only a select set of modes (function, action, etc.) are valid for a given device configuration. This patch ensures that invalid modes result in a return -EINVAL. Such a situation should never occur in reality, but it's good to define a default switch cases for the sake of making the intent of the code clear.

Cc Kamel Bouhara <[email protected]>

Cc Maxime Coquelin <[email protected]>

Cc Alexandre Torgue <[email protected]>

Cc David Lechner <[email protected]>

Acked-by Syed Nayyar Waris <[email protected]>

Reviewed-by Fabrice Gasnier <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/104-quad-8.c            | 20 +++++++----
 drivers/counter/microchip-tcb-capture.c |  6 ++++
 drivers/counter/stm32-lptimer-cnt.c     | 10 +++---
 drivers/counter/ti-eqep.c               | 45 +++++++++++--------------
 4 files changed, 46 insertions(+), 35 deletions(-)

drivers/counter/104-quad-8.c +14 -6

index 09d779544969..b7d6c1c43655 100644
@@ -273,6 +273,10 @@ static int quad8_function_set(struct counter_device *counter,
 			*scale = 2;
 			mode_cfg |= QUAD8_CMR_QUADRATURE_X4;
 			break;
+		default:
+			/* should never reach this path */
+			mutex_unlock(&priv->lock);
+			return -EINVAL;
 		}
 	}
 

@@ -349,7 +353,7 @@ static int quad8_action_get(struct counter_device *counter,
 	case QUAD8_COUNT_FUNCTION_PULSE_DIRECTION:
 		if (synapse->signal->id == signal_a_id)
 			*action = QUAD8_SYNAPSE_ACTION_RISING_EDGE;
-		break;
+		return 0;
 	case QUAD8_COUNT_FUNCTION_QUADRATURE_X1:
 		if (synapse->signal->id == signal_a_id) {
 			quad8_direction_get(counter, count, &direction);

@@ -359,17 +363,18 @@ static int quad8_action_get(struct counter_device *counter,
 			else
 				*action = QUAD8_SYNAPSE_ACTION_FALLING_EDGE;
 		}
-		break;
+		return 0;
 	case QUAD8_COUNT_FUNCTION_QUADRATURE_X2:
 		if (synapse->signal->id == signal_a_id)
 			*action = QUAD8_SYNAPSE_ACTION_BOTH_EDGES;
-		break;
+		return 0;
 	case QUAD8_COUNT_FUNCTION_QUADRATURE_X4:
 		*action = QUAD8_SYNAPSE_ACTION_BOTH_EDGES;
-		break;
+		return 0;
+	default:
+		/* should never reach this path */
+		return -EINVAL;
 	}
-
-	return 0;
 }
 
 static const struct counter_ops quad8_ops = {

@@ -529,6 +534,9 @@ static int quad8_count_mode_set(struct counter_device *counter,
 	case COUNTER_COUNT_MODE_MODULO_N:
 		cnt_mode = 3;
 		break;
+	default:
+		/* should never reach this path */
+		return -EINVAL;
 	}
 
 	mutex_lock(&priv->lock);

drivers/counter/microchip-tcb-capture.c +6 -0

index 51b8af80f98b..0c9a61962911 100644
@@ -133,6 +133,9 @@ static int mchp_tc_count_function_set(struct counter_device *counter,
 		bmr |= ATMEL_TC_QDEN | ATMEL_TC_POSEN;
 		cmr |= ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_ABETRG | ATMEL_TC_XC0;
 		break;
+	default:
+		/* should never reach this path */
+		return -EINVAL;
 	}
 
 	regmap_write(priv->regmap, ATMEL_TC_BMR, bmr);

@@ -226,6 +229,9 @@ static int mchp_tc_count_action_set(struct counter_device *counter,
 	case MCHP_TC_SYNAPSE_ACTION_BOTH_EDGE:
 		edge = ATMEL_TC_ETRGEDG_BOTH;
 		break;
+	default:
+		/* should never reach this path */
+		return -EINVAL;
 	}
 
 	return regmap_write_bits(priv->regmap,

drivers/counter/stm32-lptimer-cnt.c +0 -0

index c19d998df5ba..78f383b77bd2 100644

drivers/counter/stm32-lptimer-cnt.c +2 -2

+		/* should never reach this path */
+		return -EINVAL;
 	}
-
-	return -EINVAL;
 }
 
 static int stm32_lptim_cnt_action_set(struct counter_device *counter,

drivers/counter/ti-eqep.c +0 -0

index 65df9ef5b5bc..878725c2f010 100644

drivers/counter/ti-eqep.c +4 -11

-		case TI_EQEP_SIGNAL_QEPA:
-			*action = TI_EQEP_SYNAPSE_ACTION_RISING_EDGE;
-			break;
-		default:
+		if (synapse->signal->id == TI_EQEP_SIGNAL_QEPB)
 			*action = TI_EQEP_SYNAPSE_ACTION_NONE;
-			break;
-		}
-		break;
+		else
+			*action = TI_EQEP_SYNAPSE_ACTION_RISING_EDGE;
+		return 0;
 	case TI_EQEP_COUNT_FUNC_UP_COUNT:
 	case TI_EQEP_COUNT_FUNC_DOWN_COUNT:
 		/* In up/down-count modes only QEPA is counted and QEPB is not
 		 * used.
 		 */
-		switch (synapse->signal->id) {
-		case TI_EQEP_SIGNAL_QEPA:
-			err = regmap_read(priv->regmap16, QDECCTL, &qdecctl);
-			if (err)

drivers/counter/ti-eqep.c +14 -4

-			break;
+			return 0;
 		}
-		break;
-	}
 
-	return 0;
+		err = regmap_read(priv->regmap16, QDECCTL, &qdecctl);
+		if (err)
+			return err;
+
+		if (qdecctl & QDECCTL_XCR)
+			*action = TI_EQEP_SYNAPSE_ACTION_BOTH_EDGES;
+		else
+			*action = TI_EQEP_SYNAPSE_ACTION_RISING_EDGE;
+		return 0;
+	default:
+		/* should never reach this path */
+		return -EINVAL;
+	}
 }
 
 static const struct counter_ops ti_eqep_counter_ops = {

drivers/counter/stm32-lptimer-cnt.c static int stm32_lptim_cnt_function_set(struct counter_device *counter, · line 211

Quoted 11 lines · On Wed, 9 Jun 2021 10:31:21 +0900 William Breathitt Gray…

drivers/counter/stm32-lptimer-cnt.c +3 -0

index c19d998df5ba..78f383b77bd2 100644
@@ -206,9 +206,10 @@ static int stm32_lptim_cnt_function_set(struct counter_device *counter,
 		priv->quadrature_mode = 1;
 		priv->polarity = STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES;
 		return 0;
+	default:
+		/* should never reach this path */
+		return -EINVAL;

In this particular case we are already exhaustive. So we should have neither default nor ideally the return below.

If we have a local variable of the relevant enum type, then I think the compiler should be able to tell this is exhaustive and usefully it will then issue a warning should the enum gain more entries in future.

drivers/counter/stm32-lptimer-cnt.c static int stm32_lptim_cnt_action_get(struct counter_device *counter, · line 330

Quoted 12 lines

drivers/counter/stm32-lptimer-cnt.c +1 -2

 	}
-
-	return -EINVAL;
 }
 
 static ssize_t stm32_lptim_cnt_enable_read(struct counter_device *counter,

@@ -326,9 +327,10 @@ static int stm32_lptim_cnt_action_get(struct counter_device *counter,
 	case STM32_LPTIM_ENCODER_BOTH_EDGE:
 		*action = priv->polarity;
 		return 0;
+	default:

Same in this path.

Quoted 13 lines

drivers/counter/stm32-lptimer-cnt.c +2 -2

+		/* should never reach this path */
+		return -EINVAL;
 	}
-
-	return -EINVAL;
 }
 
 static int stm32_lptim_cnt_action_set(struct counter_device *counter,

drivers/counter/ti-eqep.c +0 -0

index 65df9ef5b5bc..878725c2f010 100644

drivers/counter/ti-eqep.c static int ti_eqep_action_get(struct counter_device *counter, · line 165

Quoted 10 lines

drivers/counter/ti-eqep.c +1 -2

@@ -157,44 +157,39 @@ static int ti_eqep_action_get(struct counter_device *counter,
 		 * QEPA and QEPB trigger QCLK.
 		 */
 		*action = TI_EQEP_SYNAPSE_ACTION_BOTH_EDGES;
-		break;
+		return 0;
 	case TI_EQEP_COUNT_FUNC_DIR_COUNT:
 		/* In direction-count mode only rising edge of QEPA is counted
 		 * and QEPB gives direction.
 		 */
-		switch (synapse->signal->id) {

I'd rather see this as

case TI_EQEP_SIGNAL_QEPA:
caes TI_EQEP_SIGNAL_QEPB:

To make it clear what the two cases are. Then we don't need the default assuming the type is right so the compiler should be able to see that we have been exhaustive.

Quoted 22 lines

drivers/counter/ti-eqep.c +4 -11

-		case TI_EQEP_SIGNAL_QEPA:
-			*action = TI_EQEP_SYNAPSE_ACTION_RISING_EDGE;
-			break;
-		default:
+		if (synapse->signal->id == TI_EQEP_SIGNAL_QEPB)
 			*action = TI_EQEP_SYNAPSE_ACTION_NONE;
-			break;
-		}
-		break;
+		else
+			*action = TI_EQEP_SYNAPSE_ACTION_RISING_EDGE;
+		return 0;
 	case TI_EQEP_COUNT_FUNC_UP_COUNT:
 	case TI_EQEP_COUNT_FUNC_DOWN_COUNT:
 		/* In up/down-count modes only QEPA is counted and QEPB is not
 		 * used.
 		 */
-		switch (synapse->signal->id) {
-		case TI_EQEP_SIGNAL_QEPA:
-			err = regmap_read(priv->regmap16, QDECCTL, &qdecctl);
-			if (err)

drivers/counter/ti-eqep.c static int ti_eqep_action_get(struct counter_device *counter, · line 176

Quoted 10 lines

drivers/counter/ti-eqep.c +1 -8

-				return err;
-
-			if (qdecctl & QDECCTL_XCR)
-				*action = TI_EQEP_SYNAPSE_ACTION_BOTH_EDGES;
-			else
-				*action = TI_EQEP_SYNAPSE_ACTION_RISING_EDGE;
-			break;
-		default:
+		if (synapse->signal->id == TI_EQEP_SIGNAL_QEPB) {
 			*action = TI_EQEP_SYNAPSE_ACTION_NONE;

Same as above

Quoted 23 lines

drivers/counter/ti-eqep.c +14 -4

-			break;
+			return 0;
 		}
-		break;
-	}
 
-	return 0;
+		err = regmap_read(priv->regmap16, QDECCTL, &qdecctl);
+		if (err)
+			return err;
+
+		if (qdecctl & QDECCTL_XCR)
+			*action = TI_EQEP_SYNAPSE_ACTION_BOTH_EDGES;
+		else
+			*action = TI_EQEP_SYNAPSE_ACTION_RISING_EDGE;
+		return 0;
+	default:
+		/* should never reach this path */
+		return -EINVAL;
+	}
 }
 
 static const struct counter_ops ti_eqep_counter_ops = {

This is a reimplementation of the Generic Counter driver interface. There are no modifications to the Counter subsystem userspace interface, so existing userspace applications should continue to run seamlessly.

The purpose of this patch is to internalize the sysfs interface code among the various counter drivers into a shared module. Counter drivers pass and take data natively (i.e. u8, u64, etc.) and the shared counter module handles the translation between the sysfs interface and the device drivers. This guarantees a standard userspace interface for all counter drivers, and helps generalize the Generic Counter driver ABI in order to support the Generic Counter chrdev interface (introduced in a subsequent patch) without significant changes to the existing counter drivers.

Note, Counter device registration is the same as before: drivers populate a struct counter_device with components and callbacks, then pass the structure to the devm_counter_register function. However, what's different now is how the Counter subsystem code handles this registration internally.

Whereas before callbacks would interact directly with sysfs data, this interaction is now abstracted and instead callbacks interact with native C data types. The counter_comp structure forms the basis for Counter extensions.

The counter-sysfs.c file contains the code to parse through the counter_device structure and register the requested components and extensions. Attributes are created and populated based on type, with respective translation functions to handle the mapping between sysfs and the counter driver callbacks.

The translation performed for each attribute is straightforward: the attribute type and data is parsed from the counter_attribute structure, the respective counter driver read/write callback is called, and sysfs I/O is handled before or after the driver read/write function is called.

Cc Syed Nayyar Waris <[email protected]>

Cc Jarkko Nikula <[email protected]>

Cc Patrick Havelange <[email protected]>

Cc Kamel Bouhara <[email protected]>

Cc Fabrice Gasnier <[email protected]>

Cc Maxime Coquelin <[email protected]>

Cc Alexandre Torgue <[email protected]>

Cc Dan Carpenter <[email protected]>

Reviewed-by David Lechner <[email protected]>

Tested-by David Lechner <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 MAINTAINERS                             |    1 -
 drivers/counter/104-quad-8.c            |  449 +++----
 drivers/counter/Makefile                |    1 +
 drivers/counter/counter-core.c          |  155 +++
 drivers/counter/counter-sysfs.c         |  846 +++++++++++++
 drivers/counter/counter-sysfs.h         |   13 +
 drivers/counter/counter.c               | 1496 -----------------------
 drivers/counter/ftm-quaddec.c           |   56 +-
 drivers/counter/intel-qep.c             |  144 +--
 drivers/counter/interrupt-cnt.c         |   62 +-
 drivers/counter/microchip-tcb-capture.c |   93 +-
 drivers/counter/stm32-lptimer-cnt.c     |  162 ++-
 drivers/counter/stm32-timer-cnt.c       |  147 +--
 drivers/counter/ti-eqep.c               |  180 +--
 include/linux/counter.h                 |  629 +++++-----
 include/linux/counter_enum.h            |   45 -
 16 files changed, 1918 insertions(+), 2561 deletions(-)
 create mode 100644 drivers/counter/counter-core.c
 create mode 100644 drivers/counter/counter-sysfs.c
 create mode 100644 drivers/counter/counter-sysfs.h
 delete mode 100644 drivers/counter/counter.c
 delete mode 100644 include/linux/counter_enum.h

drivers/counter/counter-core.c new file +18 -0

new file mode 100644
index 000000000000..e7dd6ea01c8a
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic Counter interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#include <linux/counter.h>
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/gfp.h>
+#include <linux/idr.h>
+#include <linux/init.h>
+#include <linux/module.h>
+
+#include "counter-sysfs.h"
+
+/* Provides a unique ID for each counter device */
+static DEFINE_IDA(counter_ida);
+

drivers/counter/counter-core.c new file +35 -0

+int counter_register(struct counter_device *const counter)
+{
+	struct device *const dev = &counter->dev;
+	int id;
+	int err;
+
+	/* Acquire unique ID */
+	id = ida_alloc(&counter_ida, GFP_KERNEL);
+	if (id < 0)
+		return id;
+
+	/* Configure device structure for Counter */
+	dev->id = id;
+	dev->type = &counter_device_type;
+	dev->bus = &counter_bus_type;
+	if (counter->parent) {
+		dev->parent = counter->parent;
+		dev->of_node = counter->parent->of_node;
+	}
+	device_initialize(dev);
+	dev_set_drvdata(dev, counter);
+
+	/* Add Counter sysfs attributes */
+	err = counter_sysfs_add(counter);
+	if (err < 0)
+		goto err_free_id;
+
+	/* Add device to system */
+	err = device_add(dev);
+	if (err < 0)
+		goto err_free_id;
+
+	return 0;
+
+err_free_id:

+ */

drivers/counter/counter-core.c new file +25 -0

+void counter_unregister(struct counter_device *const counter)
+{
+	if (!counter)
+		return;
+
+	device_unregister(&counter->dev);
+}
+EXPORT_SYMBOL_GPL(counter_unregister);
+
+static void devm_counter_release(struct device *dev, void *res)
+{
+	counter_unregister(*(struct counter_device **)res);
+}
+
+/**
+ * devm_counter_register - Resource-managed counter_register
+ * @dev:	device to allocate counter_device for
+ * @counter:	pointer to Counter to register
+ *
+ * Managed counter_register. The Counter registered with this function is
+ * automatically unregistered on driver detach. This function calls
+ * counter_register internally. Refer to that function for more information.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.

drivers/counter/counter-core.c new file +30 -0

+
+	err = counter_register(counter);
+	if (err < 0) {
+		devres_free(ptr);
+		return err;
+	}
+
+	*ptr = counter;
+	devres_add(dev, ptr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_counter_register);
+
+static int __init counter_init(void)
+{
+	return bus_register(&counter_bus_type);
+}
+
+static void __exit counter_exit(void)
+{
+	bus_unregister(&counter_bus_type);
+}
+
+subsys_initcall(counter_init);
+module_exit(counter_exit);
+
+MODULE_AUTHOR("William Breathitt Gray <[email protected]>");
+MODULE_DESCRIPTION("Generic Counter interface");
+MODULE_LICENSE("GPL v2");

drivers/counter/counter-sysfs.c new file +17 -0

new file mode 100644
index 000000000000..07588130600a
@@ -0,0 +1,846 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic Counter sysfs interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#include <linux/counter.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gfp.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+
+#include "counter-sysfs.h"
+

The Counter subsystem architecture and driver implementations have changed in order to handle Counter sysfs interactions in a more consistent way. This patch updates the Generic Counter interface header file comments to reflect the changes.

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/counter/counter-core.c |  3 ++
 include/linux/counter.h        | 72 +++++++++++++++-------------------
 2 files changed, 35 insertions(+), 40 deletions(-)

drivers/counter/counter-core.c +3 -0

index e7dd6ea01c8a..407661c6feb0 100644
@@ -41,6 +41,9 @@ static struct bus_type counter_bus_type = {
  * This function registers a Counter to the system. A sysfs "counter" directory
  * will be created and populated with sysfs attributes correlating with the
  * Counter Signals, Synapses, and Counts respectively.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
  */
 int counter_register(struct counter_device *const counter)
 {

include/linux/counter.h +0 -0

index 567471818ec3..e7fd6d81a929 100644
@@ -188,12 +188,10 @@ struct counter_comp {
 
 /**
  * struct counter_signal - Counter Signal node

include/linux/counter.h +28 -34

  */
 struct counter_signal {
 	int id;

@@ -207,7 +205,7 @@ struct counter_signal {
  * struct counter_synapse - Counter Synapse node
  * @actions_list:	array of available action modes
  * @num_actions:	number of action modes specified in @actions_list
- * @signal:		pointer to associated signal
+ * @signal:		pointer to the associated Signal
  */
 struct counter_synapse {
 	const enum counter_synapse_action *actions_list;

@@ -218,17 +216,14 @@ struct counter_synapse {
 
 /**
  * struct counter_count - Counter Count node
- * @id:			unique ID used to identify Count
- * @name:		device-specific Count name; ideally, this should match
- *			the name as it appears in the datasheet documentation
- * @function:		index of current function mode
- * @functions_list:	array available function modes
+ * @id:			unique ID used to identify the Count
+ * @name:		device-specific Count name
+ * @functions_list:	array of available function modes
  * @num_functions:	number of function modes specified in @functions_list
- * @synapses:		array of synapses for initialization
- * @num_synapses:	number of synapses specified in @synapses
- * @ext:		optional array of Counter Count extensions
- * @num_ext:		number of Counter Count extensions specified in @ext
- * @priv:		optional private data supplied by driver
+ * @synapses:		array of Synapses for initialization
+ * @num_synapses:	number of Synapses specified in @synapses
+ * @ext:		optional array of Count extensions
+ * @num_ext:		number of Count extensions specified in @ext
  */
 struct counter_count {
 	int id;

@@ -246,29 +241,26 @@ struct counter_count {
 
 /**
  * struct counter_ops - Callbacks from driver
- * @signal_read:	optional read callback for Signal attribute. The read
- *			level of the respective Signal should be passed back via
- *			the level parameter.
- * @count_read:		optional read callback for Count attribute. The read
- *			value of the respective Count should be passed back via
- *			the val parameter.
- * @count_write:	optional write callback for Count attribute. The write
- *			value for the respective Count is passed in via the val
+ * @signal_read:	read callback for Signals. The read level of the
+ *			respective Signal should be passed back via the level
+ *			parameter.
+ * @count_read:		read callback for Counts. The read value of the
+ *			respective Count should be passed back via the value
  *			parameter.
- * @function_get:	function to get the current count function mode. Returns
- *			0 on success and negative error code on error. The index
- *			of the respective Count's returned function mode should
- *			be passed back via the function parameter.
- * @function_set:	function to set the count function mode. function is the
- *			index of the requested function mode from the respective
- *			Count's functions_list array.
- * @action_get:		function to get the current action mode. Returns 0 on
- *			success and negative error code on error. The index of
- *			the respective Synapse's returned action mode should be
- *			passed back via the action parameter.
- * @action_set:		function to set the action mode. action is the index of
- *			the requested action mode from the respective Synapse's
- *			actions_list array.
+ * @count_write:	write callback for Counts. The write value for the
+ *			respective Count is passed in via the value parameter.
+ * @function_read:	read callback the Count function modes. The read
+ *			function mode of the respective Count should be passed
+ *			back via the function parameter.
+ * @function_write:	write callback for Count function modes. The function
+ *			mode to write for the respective Count is passed in via
+ *			the function parameter.
+ * @action_read:	read callback the Synapse action modes. The read action
+ *			mode of the respective Synapse should be passed back via
+ *			the action parameter.
+ * @action_write:	write callback for Synapse action modes. The action mode
+ *			to write for the respective Synapse is passed in via the
+ *			action parameter.
  */
 struct counter_ops {
 	int (*signal_read)(struct counter_device *counter,

@@ -296,7 +288,7 @@ struct counter_ops {
 
 /**
  * struct counter_device - Counter data structure
- * @name:		name of the device as it appears in the datasheet
+ * @name:		name of the device
  * @parent:		optional parent device providing the counters
  * @ops:		callbacks from driver
  * @signals:		array of Signals

drivers/counter/counter-chrdev.c new file +59 -0

+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->next_events_list, l)
+		if (event_node->event == watch->event &&
+		    event_node->channel == watch->channel)
+			break;
+
+	/* If event is not already in the list */
+	if (&event_node->l == &counter->next_events_list) {
+		/* Allocate new event node */
+		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
+		if (!event_node)
+			return -ENOMEM;
+
+		/* Configure event node and add to the list */
+		event_node->event = watch->event;
+		event_node->channel = watch->channel;
+		INIT_LIST_HEAD(&event_node->comp_list);
+		list_add(&event_node->l, &counter->next_events_list);
+	}
+
+	/* Check if component watch has already been set before */
+	list_for_each_entry(comp_node, &event_node->comp_list, l)
+		if (comp_node->parent == cfg->parent &&
+		    comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
+			return -EINVAL;
+
+	/* Allocate component node */
+	comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
+	if (!comp_node) {
+		/* Free event node if no one else is watching */
+		if (list_empty(&event_node->comp_list)) {
+			list_del(&event_node->l);
+			kfree(event_node);
+		}
+		return -ENOMEM;
+	}
+	*comp_node = *cfg;
+
+	/* Add component node to event node */
+	list_add_tail(&comp_node->l, &event_node->comp_list);
+
+	return 0;
+}
+
+static int counter_disable_events(struct counter_device *const counter)
+{
+	unsigned long flags;
+	int err = 0;
+
+	spin_lock_irqsave(&counter->events_list_lock, flags);
+
+	counter_events_list_free(&counter->events_list);
+
+	if (counter->ops->events_configure)
+		err = counter->ops->events_configure(counter);
+
+	spin_unlock_irqrestore(&counter->events_list_lock, flags);
+
+	counter_events_list_free(&counter->next_events_list);

drivers/counter/counter-chrdev.c line 104

Quoted 7 lines · William Breathitt Gray

drivers/counter/counter-chrdev.c new file +7 -0

+static int counter_set_event_node(struct counter_device *const counter,
+				  struct counter_watch *const watch,
+				  const struct counter_comp_node *const cfg)
+{
+	struct counter_event_node *event_node;
+	struct counter_comp_node *comp_node;
+

The caller should be holding the counter->events_list_lock lock but it's not.

Quoted 64 lines

drivers/counter/counter-chrdev.c new file +64 -0

+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->next_events_list, l)
+		if (event_node->event == watch->event &&
+		    event_node->channel == watch->channel)
+			break;
+
+	/* If event is not already in the list */
+	if (&event_node->l == &counter->next_events_list) {
+		/* Allocate new event node */
+		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
+		if (!event_node)
+			return -ENOMEM;
+
+		/* Configure event node and add to the list */
+		event_node->event = watch->event;
+		event_node->channel = watch->channel;
+		INIT_LIST_HEAD(&event_node->comp_list);
+		list_add(&event_node->l, &counter->next_events_list);
+	}
+
+	/* Check if component watch has already been set before */
+	list_for_each_entry(comp_node, &event_node->comp_list, l)
+		if (comp_node->parent == cfg->parent &&
+		    comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
+			return -EINVAL;
+
+	/* Allocate component node */
+	comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
+	if (!comp_node) {
+		/* Free event node if no one else is watching */
+		if (list_empty(&event_node->comp_list)) {
+			list_del(&event_node->l);
+			kfree(event_node);
+		}
+		return -ENOMEM;
+	}
+	*comp_node = *cfg;
+
+	/* Add component node to event node */
+	list_add_tail(&comp_node->l, &event_node->comp_list);
+
+	return 0;
+}
+
+static int counter_disable_events(struct counter_device *const counter)
+{
+	unsigned long flags;
+	int err = 0;
+
+	spin_lock_irqsave(&counter->events_list_lock, flags);
+
+	counter_events_list_free(&counter->events_list);
+
+	if (counter->ops->events_configure)
+		err = counter->ops->events_configure(counter);
+
+	spin_unlock_irqrestore(&counter->events_list_lock, flags);
+
+	counter_events_list_free(&counter->next_events_list);
+
+	return err;
+}
+
+static int counter_add_watch(struct counter_device *const counter,

drivers/counter/counter-chrdev.c line 173

Quoted 5 lines

drivers/counter/counter-chrdev.c new file +5 -0

+			     const unsigned long arg)
+{
+	void __user *const uwatch = (void __user *)arg;
+	struct counter_watch watch;
+	struct counter_comp_node comp_node = {0};

Always use = {};. It's the new hotness, and it avoids a Sparse warning for using 0 instead of NULL. #IDidNotTest #CouldBeWrongAboutSparse

Quoted 11 lines · Dan Carpenter

drivers/counter/counter-chrdev.c line 104

Quoted 7 lines · William Breathitt Gray

drivers/counter/counter-chrdev.c new file +7 -0

+static int counter_set_event_node(struct counter_device *const counter,
+				  struct counter_watch *const watch,
+				  const struct counter_comp_node *const cfg)
+{
+	struct counter_event_node *event_node;
+	struct counter_comp_node *comp_node;
+

The caller should be holding the counter->events_list_lock lock but it's not.

Hi Dan,

The counter_set_event_node() function doesn't access or modify counter->events_list (it works on counter->next_events_list) so holding the counter->events_list_lock here isn't necessary.

Quoted 10 lines

drivers/counter/counter-chrdev.c line 114

Quoted 10 lines

drivers/counter/counter-chrdev.c new file +10 -0

+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->next_events_list, l)
+		if (event_node->event == watch->event &&
+		    event_node->channel == watch->channel)
+			break;
+
+	/* If event is not already in the list */
+	if (&event_node->l == &counter->next_events_list) {
+		/* Allocate new event node */
+		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);

Quoted 10 lines

Quoted 10 lines

drivers/counter/counter-chrdev.c new file +10 -0

+		if (!event_node)
+			return -ENOMEM;
+
+		/* Configure event node and add to the list */
+		event_node->event = watch->event;
+		event_node->channel = watch->channel;
+		INIT_LIST_HEAD(&event_node->comp_list);
+		list_add(&event_node->l, &counter->next_events_list);
+	}
+

Quoted 8 lines

drivers/counter/counter-chrdev.c line 132

Quoted 8 lines

drivers/counter/counter-chrdev.c new file +8 -0

+	/* Check if component watch has already been set before */
+	list_for_each_entry(comp_node, &event_node->comp_list, l)
+		if (comp_node->parent == cfg->parent &&
+		    comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
+			return -EINVAL;
+
+	/* Allocate component node */
+	comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);

Quoted 7 lines

Quoted 7 lines

drivers/counter/counter-chrdev.c new file +7 -0

+	if (!comp_node) {
+		/* Free event node if no one else is watching */
+		if (list_empty(&event_node->comp_list)) {
+			list_del(&event_node->l);
+			kfree(event_node);
+		}
+		return -ENOMEM;

Quoted 8 lines

drivers/counter/counter-chrdev.c line 147

Quoted 8 lines

drivers/counter/counter-chrdev.c new file +8 -0

+	}
+	*comp_node = *cfg;
+
+	/* Add component node to event node */
+	list_add_tail(&comp_node->l, &event_node->comp_list);
+
+	return 0;
+}

Quoted 13 lines · William Breathitt Gray

Quoted 11 lines · Dan Carpenter

drivers/counter/counter-chrdev.c line 104

Quoted 7 lines · William Breathitt Gray

drivers/counter/counter-chrdev.c new file +7 -0

+static int counter_set_event_node(struct counter_device *const counter,
+				  struct counter_watch *const watch,
+				  const struct counter_comp_node *const cfg)
+{
+	struct counter_event_node *event_node;
+	struct counter_comp_node *comp_node;
+

The caller should be holding the counter->events_list_lock lock but it's not.

Quoted 6 lines

Hi Dan,

The counter_set_event_node() function doesn't access or modify counter->events_list (it works on counter->next_events_list) so holding the counter->events_list_lock here isn't necessary.

There needs to be some sort of locking or this function can race with itself. (Two threads add the same event at exactly the same time). It looks like it can also race with counter_disable_events() leading to a use after free.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/counter/counter-chrdev.c new file +5 -0

+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->next_events_list, l)
+		if (event_node->event == watch->event &&
+		    event_node->channel == watch->channel)
+			break;

Quoted 5 lines

Quoted 5 lines

drivers/counter/counter-chrdev.c line 114

Quoted 5 lines

drivers/counter/counter-chrdev.c new file +5 -0

+
+	/* If event is not already in the list */
+	if (&event_node->l == &counter->next_events_list) {
+		/* Allocate new event node */
+		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);

Btw, say we decided that we can add/remove events locklessly, then these GFP_ATOMICs can be changed to GFP_KERNEL.