linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8] counter: Remove struct counter_device::priv
@ 2021-12-21 10:45 Uwe Kleine-König
  2021-12-21 10:45 ` [PATCH 5/8] counter: microchip-tcp-capture: Use container_of instead of " Uwe Kleine-König
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Uwe Kleine-König @ 2021-12-21 10:45 UTC (permalink / raw)
  To: Alexandre Torgue, David Lechner, Fabrice Gasnier, Jarkko Nikula,
	Kamel Bouhara, Maxime Coquelin, Oleksij Rempel,
	Patrick Havelange, Syed Nayyar Waris, William Breathitt Gray
  Cc: linux-arm-kernel, linux-iio, linux-kernel, linux-stm32

Hello,

similar to patch
https://lore.kernel.org/r/4bde7cbd9e43a5909208102094444219d3154466.1640072891.git.vilhelm.gray@gmail.com
the usage of struct counter_device::priv can be replaced by
container_of which improves type safety and code size.

This series depends on above patch, converts the remaining drivers and
finally drops struct counter_device::priv.

This series was compile tested using ARCH=arm allmodconfig with the
following change:

 config 104_QUAD_8
        tristate "ACCES 104-QUAD-8 driver"
-       depends on PC104 && X86
+       depends on PC104 && X86 || COMPILE_TEST
        select ISA_BUS_API

in drivers/counter/Kconfig.

Best regards
Uwe

Uwe Kleine-König (8):
  counter: 104-quad-8: Use container_of instead of struct
    counter_device::priv
  counter: ftm-quaddec: Use container_of instead of struct
    counter_device::priv
  counter: intel-qeb: Use container_of instead of struct
    counter_device::priv
  counter: interrupt-cnt: Use container_of instead of struct
    counter_device::priv
  counter: microchip-tcp-capture: Use container_of instead of struct
    counter_device::priv
  counter: stm32-lptimer-cnt: Use container_of instead of struct
    counter_device::priv
  counter: stm32-timer-cnt: Use container_of instead of struct
    counter_device::priv
  counter: Remove unused member from struct counter_device

 drivers/counter/104-quad-8.c            | 64 +++++++++++++------------
 drivers/counter/ftm-quaddec.c           | 14 ++++--
 drivers/counter/intel-qep.c             | 24 ++++++----
 drivers/counter/interrupt-cnt.c         | 16 ++++---
 drivers/counter/microchip-tcb-capture.c | 18 ++++---
 drivers/counter/stm32-lptimer-cnt.c     | 24 ++++++----
 drivers/counter/stm32-timer-cnt.c       | 24 ++++++----
 drivers/counter/ti-eqep.c               |  1 -
 include/linux/counter.h                 |  3 --
 9 files changed, 106 insertions(+), 82 deletions(-)


base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
prerequisite-patch-id: 9459ad8bc78190558df9123f8bebe28ca1c396ea
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH 5/8] counter: microchip-tcp-capture: Use container_of instead of struct counter_device::priv
  2021-12-21 10:45 [PATCH 0/8] counter: Remove struct counter_device::priv Uwe Kleine-König
@ 2021-12-21 10:45 ` Uwe Kleine-König
  2021-12-21 10:45 ` [PATCH 6/8] counter: stm32-lptimer-cnt: " Uwe Kleine-König
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Uwe Kleine-König @ 2021-12-21 10:45 UTC (permalink / raw)
  To: Kamel Bouhara, William Breathitt Gray
  Cc: linux-arm-kernel, linux-iio, linux-kernel

Using counter->priv is a memory read and so more expensive than
container_of which is only an addition.

So container_of is expected to be a tad faster, it's type-safe, and
produces smaller code (ARCH=arm allmodconfig):

	add/remove: 0/0 grow/shrink: 1/6 up/down: 12/-68 (-56)
	Function                                     old     new   delta
	mchp_tc_count_function_write                1016    1028     +12
	mchp_tc_count_action_write                   204     196      -8
	mchp_tc_probe                               1376    1364     -12
	mchp_tc_count_signal_read                    360     348     -12
	mchp_tc_count_read                           264     252     -12
	mchp_tc_count_function_read                  108      96     -12
	mchp_tc_count_action_read                    392     380     -12
	Total: Before=5920, After=5864, chg -0.95%

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/counter/microchip-tcb-capture.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index 0ab1b2716784..031e79f5f06a 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -32,6 +32,11 @@ struct mchp_tc_data {
 	bool trig_inverted;
 };
 
+static inline struct mchp_tc_data *mchp_tc_from_counter(struct counter_device *counter)
+{
+	return container_of(counter, struct mchp_tc_data, counter);
+}
+
 static const enum counter_function mchp_tc_count_functions[] = {
 	COUNTER_FUNCTION_INCREASE,
 	COUNTER_FUNCTION_QUADRATURE_X4,
@@ -72,7 +77,7 @@ static int mchp_tc_count_function_read(struct counter_device *counter,
 				       struct counter_count *count,
 				       enum counter_function *function)
 {
-	struct mchp_tc_data *const priv = counter->priv;
+	struct mchp_tc_data *const priv = mchp_tc_from_counter(counter);
 
 	if (priv->qdec_mode)
 		*function = COUNTER_FUNCTION_QUADRATURE_X4;
@@ -86,7 +91,7 @@ static int mchp_tc_count_function_write(struct counter_device *counter,
 					struct counter_count *count,
 					enum counter_function function)
 {
-	struct mchp_tc_data *const priv = counter->priv;
+	struct mchp_tc_data *const priv = mchp_tc_from_counter(counter);
 	u32 bmr, cmr;
 
 	regmap_read(priv->regmap, ATMEL_TC_BMR, &bmr);
@@ -148,7 +153,7 @@ static int mchp_tc_count_signal_read(struct counter_device *counter,
 				     struct counter_signal *signal,
 				     enum counter_signal_level *lvl)
 {
-	struct mchp_tc_data *const priv = counter->priv;
+	struct mchp_tc_data *const priv = mchp_tc_from_counter(counter);
 	bool sigstatus;
 	u32 sr;
 
@@ -169,7 +174,7 @@ static int mchp_tc_count_action_read(struct counter_device *counter,
 				     struct counter_synapse *synapse,
 				     enum counter_synapse_action *action)
 {
-	struct mchp_tc_data *const priv = counter->priv;
+	struct mchp_tc_data *const priv = mchp_tc_from_counter(counter);
 	u32 cmr;
 
 	regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr);
@@ -197,7 +202,7 @@ static int mchp_tc_count_action_write(struct counter_device *counter,
 				      struct counter_synapse *synapse,
 				      enum counter_synapse_action action)
 {
-	struct mchp_tc_data *const priv = counter->priv;
+	struct mchp_tc_data *const priv = mchp_tc_from_counter(counter);
 	u32 edge = ATMEL_TC_ETRGEDG_NONE;
 
 	/* QDEC mode is rising edge only */
@@ -230,7 +235,7 @@ static int mchp_tc_count_action_write(struct counter_device *counter,
 static int mchp_tc_count_read(struct counter_device *counter,
 			      struct counter_count *count, u64 *val)
 {
-	struct mchp_tc_data *const priv = counter->priv;
+	struct mchp_tc_data *const priv = mchp_tc_from_counter(counter);
 	u32 cnt;
 
 	regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CV), &cnt);
@@ -369,7 +374,6 @@ static int mchp_tc_probe(struct platform_device *pdev)
 	priv->counter.counts = mchp_tc_counts;
 	priv->counter.num_signals = ARRAY_SIZE(mchp_tc_count_signals);
 	priv->counter.signals = mchp_tc_count_signals;
-	priv->counter.priv = priv;
 
 	return devm_counter_register(&pdev->dev, &priv->counter);
 }
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 6/8] counter: stm32-lptimer-cnt: Use container_of instead of struct counter_device::priv
  2021-12-21 10:45 [PATCH 0/8] counter: Remove struct counter_device::priv Uwe Kleine-König
  2021-12-21 10:45 ` [PATCH 5/8] counter: microchip-tcp-capture: Use container_of instead of " Uwe Kleine-König
@ 2021-12-21 10:45 ` Uwe Kleine-König
  2021-12-21 10:45 ` [PATCH 7/8] counter: stm32-timer-cnt: " Uwe Kleine-König
  2021-12-21 11:12 ` [PATCH 0/8] counter: Remove " Lars-Peter Clausen
  3 siblings, 0 replies; 10+ messages in thread
From: Uwe Kleine-König @ 2021-12-21 10:45 UTC (permalink / raw)
  To: Fabrice Gasnier, William Breathitt Gray, Maxime Coquelin,
	Alexandre Torgue
  Cc: linux-iio, linux-stm32, linux-arm-kernel, linux-kernel

Using counter->priv is a memory read and so more expensive than
container_of which is only an addition. (In this case even a noop
because the offset is 0.)

So container_of is expected to be a tad faster, it's type-safe, and
produces smaller code (ARCH=arm allmodconfig):

	add/remove: 0/0 grow/shrink: 0/10 up/down: 0/-140 (-140)
	Function                                     old     new   delta
	stm32_lptim_cnt_read                         272     260     -12
	stm32_lptim_cnt_probe                        528     516     -12
	stm32_lptim_cnt_function_write               420     408     -12
	stm32_lptim_cnt_function_read                184     172     -12
	stm32_lptim_cnt_enable_write                 436     424     -12
	stm32_lptim_cnt_enable_read                  312     300     -12
	stm32_lptim_cnt_ceiling_write                368     356     -12
	stm32_lptim_cnt_ceiling_read                  84      72     -12
	stm32_lptim_cnt_action_read                  388     376     -12
	stm32_lptim_cnt_action_write                 576     544     -32
	Total: Before=6458, After=6318, chg -2.17%

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/counter/stm32-lptimer-cnt.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c
index 5168833b1fdf..c6eb3071571f 100644
--- a/drivers/counter/stm32-lptimer-cnt.c
+++ b/drivers/counter/stm32-lptimer-cnt.c
@@ -30,6 +30,11 @@ struct stm32_lptim_cnt {
 	bool enabled;
 };
 
+static inline struct stm32_lptim_cnt *stm32_lptim_from_counter(struct counter_device *counter)
+{
+	return container_of(counter, struct stm32_lptim_cnt, counter);
+}
+
 static int stm32_lptim_is_enabled(struct stm32_lptim_cnt *priv)
 {
 	u32 val;
@@ -141,7 +146,7 @@ static const enum counter_synapse_action stm32_lptim_cnt_synapse_actions[] = {
 static int stm32_lptim_cnt_read(struct counter_device *counter,
 				struct counter_count *count, u64 *val)
 {
-	struct stm32_lptim_cnt *const priv = counter->priv;
+	struct stm32_lptim_cnt *const priv = stm32_lptim_from_counter(counter);
 	u32 cnt;
 	int ret;
 
@@ -158,7 +163,7 @@ static int stm32_lptim_cnt_function_read(struct counter_device *counter,
 					 struct counter_count *count,
 					 enum counter_function *function)
 {
-	struct stm32_lptim_cnt *const priv = counter->priv;
+	struct stm32_lptim_cnt *const priv = stm32_lptim_from_counter(counter);
 
 	if (!priv->quadrature_mode) {
 		*function = COUNTER_FUNCTION_INCREASE;
@@ -177,7 +182,7 @@ static int stm32_lptim_cnt_function_write(struct counter_device *counter,
 					  struct counter_count *count,
 					  enum counter_function function)
 {
-	struct stm32_lptim_cnt *const priv = counter->priv;
+	struct stm32_lptim_cnt *const priv = stm32_lptim_from_counter(counter);
 
 	if (stm32_lptim_is_enabled(priv))
 		return -EBUSY;
@@ -200,7 +205,7 @@ static int stm32_lptim_cnt_enable_read(struct counter_device *counter,
 				       struct counter_count *count,
 				       u8 *enable)
 {
-	struct stm32_lptim_cnt *const priv = counter->priv;
+	struct stm32_lptim_cnt *const priv = stm32_lptim_from_counter(counter);
 	int ret;
 
 	ret = stm32_lptim_is_enabled(priv);
@@ -216,7 +221,7 @@ static int stm32_lptim_cnt_enable_write(struct counter_device *counter,
 					struct counter_count *count,
 					u8 enable)
 {
-	struct stm32_lptim_cnt *const priv = counter->priv;
+	struct stm32_lptim_cnt *const priv = stm32_lptim_from_counter(counter);
 	int ret;
 
 	/* Check nobody uses the timer, or already disabled/enabled */
@@ -241,7 +246,7 @@ static int stm32_lptim_cnt_ceiling_read(struct counter_device *counter,
 					struct counter_count *count,
 					u64 *ceiling)
 {
-	struct stm32_lptim_cnt *const priv = counter->priv;
+	struct stm32_lptim_cnt *const priv = stm32_lptim_from_counter(counter);
 
 	*ceiling = priv->ceiling;
 
@@ -252,7 +257,7 @@ static int stm32_lptim_cnt_ceiling_write(struct counter_device *counter,
 					 struct counter_count *count,
 					 u64 ceiling)
 {
-	struct stm32_lptim_cnt *const priv = counter->priv;
+	struct stm32_lptim_cnt *const priv = stm32_lptim_from_counter(counter);
 
 	if (stm32_lptim_is_enabled(priv))
 		return -EBUSY;
@@ -277,7 +282,7 @@ static int stm32_lptim_cnt_action_read(struct counter_device *counter,
 				       struct counter_synapse *synapse,
 				       enum counter_synapse_action *action)
 {
-	struct stm32_lptim_cnt *const priv = counter->priv;
+	struct stm32_lptim_cnt *const priv = stm32_lptim_from_counter(counter);
 	enum counter_function function;
 	int err;
 
@@ -321,7 +326,7 @@ static int stm32_lptim_cnt_action_write(struct counter_device *counter,
 					struct counter_synapse *synapse,
 					enum counter_synapse_action action)
 {
-	struct stm32_lptim_cnt *const priv = counter->priv;
+	struct stm32_lptim_cnt *const priv = stm32_lptim_from_counter(counter);
 	enum counter_function function;
 	int err;
 
@@ -438,7 +443,6 @@ static int stm32_lptim_cnt_probe(struct platform_device *pdev)
 	}
 	priv->counter.num_counts = 1;
 	priv->counter.signals = stm32_lptim_cnt_signals;
-	priv->counter.priv = priv;
 
 	platform_set_drvdata(pdev, priv);
 
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 7/8] counter: stm32-timer-cnt: Use container_of instead of struct counter_device::priv
  2021-12-21 10:45 [PATCH 0/8] counter: Remove struct counter_device::priv Uwe Kleine-König
  2021-12-21 10:45 ` [PATCH 5/8] counter: microchip-tcp-capture: Use container_of instead of " Uwe Kleine-König
  2021-12-21 10:45 ` [PATCH 6/8] counter: stm32-lptimer-cnt: " Uwe Kleine-König
@ 2021-12-21 10:45 ` Uwe Kleine-König
  2021-12-21 11:12 ` [PATCH 0/8] counter: Remove " Lars-Peter Clausen
  3 siblings, 0 replies; 10+ messages in thread
From: Uwe Kleine-König @ 2021-12-21 10:45 UTC (permalink / raw)
  To: Fabrice Gasnier, William Breathitt Gray, Maxime Coquelin,
	Alexandre Torgue
  Cc: linux-iio, linux-stm32, linux-arm-kernel, linux-kernel

Using counter->priv is a memory read and so more expensive than
container_of which is only an addition. (In this case even a noop
because the offset is 0.)

So container_of is expected to be a tad faster, it's type-safe, and
produces smaller code (ARCH=arm allmodconfig):

	add/remove: 0/0 grow/shrink: 0/11 up/down: 0/-132 (-132)
	Function                                     old     new   delta
	stm32_timer_cnt_probe                        436     424     -12
	stm32_count_write                            312     300     -12
	stm32_count_read                             236     224     -12
	stm32_count_function_write                   492     480     -12
	stm32_count_function_read                    396     384     -12
	stm32_count_enable_write                     488     476     -12
	stm32_count_enable_read                      236     224     -12
	stm32_count_direction_read                   240     228     -12
	stm32_count_ceiling_write                    200     188     -12
	stm32_count_ceiling_read                     236     224     -12
	stm32_action_read                            492     480     -12
	Total: Before=5504, After=5372, chg -2.40%

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/counter/stm32-timer-cnt.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 0546e932db0c..ac0bea6ce690 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -37,6 +37,11 @@ struct stm32_timer_cnt {
 	struct stm32_timer_regs bak;
 };
 
+static inline struct stm32_timer_cnt *stm32_count_from_counter(struct counter_device *counter)
+{
+	return container_of(counter, struct stm32_timer_cnt, counter);
+}
+
 static const enum counter_function stm32_count_functions[] = {
 	COUNTER_FUNCTION_INCREASE,
 	COUNTER_FUNCTION_QUADRATURE_X2_A,
@@ -47,7 +52,7 @@ static const enum counter_function stm32_count_functions[] = {
 static int stm32_count_read(struct counter_device *counter,
 			    struct counter_count *count, u64 *val)
 {
-	struct stm32_timer_cnt *const priv = counter->priv;
+	struct stm32_timer_cnt *const priv = stm32_count_from_counter(counter);
 	u32 cnt;
 
 	regmap_read(priv->regmap, TIM_CNT, &cnt);
@@ -59,7 +64,7 @@ static int stm32_count_read(struct counter_device *counter,
 static int stm32_count_write(struct counter_device *counter,
 			     struct counter_count *count, const u64 val)
 {
-	struct stm32_timer_cnt *const priv = counter->priv;
+	struct stm32_timer_cnt *const priv = stm32_count_from_counter(counter);
 	u32 ceiling;
 
 	regmap_read(priv->regmap, TIM_ARR, &ceiling);
@@ -73,7 +78,7 @@ static int stm32_count_function_read(struct counter_device *counter,
 				     struct counter_count *count,
 				     enum counter_function *function)
 {
-	struct stm32_timer_cnt *const priv = counter->priv;
+	struct stm32_timer_cnt *const priv = stm32_count_from_counter(counter);
 	u32 smcr;
 
 	regmap_read(priv->regmap, TIM_SMCR, &smcr);
@@ -100,7 +105,7 @@ static int stm32_count_function_write(struct counter_device *counter,
 				      struct counter_count *count,
 				      enum counter_function function)
 {
-	struct stm32_timer_cnt *const priv = counter->priv;
+	struct stm32_timer_cnt *const priv = stm32_count_from_counter(counter);
 	u32 cr1, sms;
 
 	switch (function) {
@@ -140,7 +145,7 @@ static int stm32_count_direction_read(struct counter_device *counter,
 				      struct counter_count *count,
 				      enum counter_count_direction *direction)
 {
-	struct stm32_timer_cnt *const priv = counter->priv;
+	struct stm32_timer_cnt *const priv = stm32_count_from_counter(counter);
 	u32 cr1;
 
 	regmap_read(priv->regmap, TIM_CR1, &cr1);
@@ -153,7 +158,7 @@ static int stm32_count_direction_read(struct counter_device *counter,
 static int stm32_count_ceiling_read(struct counter_device *counter,
 				    struct counter_count *count, u64 *ceiling)
 {
-	struct stm32_timer_cnt *const priv = counter->priv;
+	struct stm32_timer_cnt *const priv = stm32_count_from_counter(counter);
 	u32 arr;
 
 	regmap_read(priv->regmap, TIM_ARR, &arr);
@@ -166,7 +171,7 @@ static int stm32_count_ceiling_read(struct counter_device *counter,
 static int stm32_count_ceiling_write(struct counter_device *counter,
 				     struct counter_count *count, u64 ceiling)
 {
-	struct stm32_timer_cnt *const priv = counter->priv;
+	struct stm32_timer_cnt *const priv = stm32_count_from_counter(counter);
 
 	if (ceiling > priv->max_arr)
 		return -ERANGE;
@@ -181,7 +186,7 @@ static int stm32_count_ceiling_write(struct counter_device *counter,
 static int stm32_count_enable_read(struct counter_device *counter,
 				   struct counter_count *count, u8 *enable)
 {
-	struct stm32_timer_cnt *const priv = counter->priv;
+	struct stm32_timer_cnt *const priv = stm32_count_from_counter(counter);
 	u32 cr1;
 
 	regmap_read(priv->regmap, TIM_CR1, &cr1);
@@ -194,7 +199,7 @@ static int stm32_count_enable_read(struct counter_device *counter,
 static int stm32_count_enable_write(struct counter_device *counter,
 				    struct counter_count *count, u8 enable)
 {
-	struct stm32_timer_cnt *const priv = counter->priv;
+	struct stm32_timer_cnt *const priv = stm32_count_from_counter(counter);
 	u32 cr1;
 
 	if (enable) {
@@ -336,7 +341,6 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
 	priv->counter.num_counts = 1;
 	priv->counter.signals = stm32_signals;
 	priv->counter.num_signals = ARRAY_SIZE(stm32_signals);
-	priv->counter.priv = priv;
 
 	platform_set_drvdata(pdev, priv);
 
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 0/8] counter: Remove struct counter_device::priv
  2021-12-21 10:45 [PATCH 0/8] counter: Remove struct counter_device::priv Uwe Kleine-König
                   ` (2 preceding siblings ...)
  2021-12-21 10:45 ` [PATCH 7/8] counter: stm32-timer-cnt: " Uwe Kleine-König
@ 2021-12-21 11:12 ` Lars-Peter Clausen
  2021-12-21 11:35   ` Uwe Kleine-König
  3 siblings, 1 reply; 10+ messages in thread
From: Lars-Peter Clausen @ 2021-12-21 11:12 UTC (permalink / raw)
  To: Uwe Kleine-König, Alexandre Torgue, David Lechner,
	Fabrice Gasnier, Jarkko Nikula, Kamel Bouhara, Maxime Coquelin,
	Oleksij Rempel, Patrick Havelange, Syed Nayyar Waris,
	William Breathitt Gray
  Cc: linux-arm-kernel, linux-iio, linux-kernel, linux-stm32

On 12/21/21 11:45 AM, Uwe Kleine-König wrote:
> Hello,
>
> similar to patch
> https://lore.kernel.org/r/4bde7cbd9e43a5909208102094444219d3154466.1640072891.git.vilhelm.gray@gmail.com
> the usage of struct counter_device::priv can be replaced by
> container_of which improves type safety and code size.
>
> This series depends on above patch, converts the remaining drivers and
> finally drops struct counter_device::priv.

Not sure if this is such a good idea. struct counter_device should not 
be embedded in the drivers state struct in the first place.

struct counter_device contains a struct device, which is a reference 
counted object. But by embedding it in the driver state struct the life 
time of both the struct counter_device and and struct device are bound 
to the life time of the driver state struct.

Which means the struct device memory can get freed before the last 
reference is dropped, which leads to a use-after-free and undefined 
behavior.

The framework should be changed to rather then embedding the struct 
counter_device in the state struct to just have a pointer to it. With 
the struct counter_device having its own allocation that will be freed 
when the last reference to the struct device is dropped.

- Lars


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 0/8] counter: Remove struct counter_device::priv
  2021-12-21 11:12 ` [PATCH 0/8] counter: Remove " Lars-Peter Clausen
@ 2021-12-21 11:35   ` Uwe Kleine-König
  2021-12-21 12:04     ` Lars-Peter Clausen
  0 siblings, 1 reply; 10+ messages in thread
From: Uwe Kleine-König @ 2021-12-21 11:35 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Alexandre Torgue, David Lechner, Fabrice Gasnier, Jarkko Nikula,
	Kamel Bouhara, Maxime Coquelin, Oleksij Rempel,
	Patrick Havelange, Syed Nayyar Waris, William Breathitt Gray,
	linux-arm-kernel, linux-iio, linux-kernel, linux-stm32


[-- Attachment #1.1: Type: text/plain, Size: 2012 bytes --]

Hello Lars,

On Tue, Dec 21, 2021 at 12:12:12PM +0100, Lars-Peter Clausen wrote:
> On 12/21/21 11:45 AM, Uwe Kleine-König wrote:
> > similar to patch
> > https://lore.kernel.org/r/4bde7cbd9e43a5909208102094444219d3154466.1640072891.git.vilhelm.gray@gmail.com
> > the usage of struct counter_device::priv can be replaced by
> > container_of which improves type safety and code size.
> > 
> > This series depends on above patch, converts the remaining drivers and
> > finally drops struct counter_device::priv.
> 
> Not sure if this is such a good idea. struct counter_device should not be
> embedded in the drivers state struct in the first place.

Just to mention it: My patch series didn't change this, this was already
broken before.

> struct counter_device contains a struct device, which is a reference counted
> object. But by embedding it in the driver state struct the life time of both
> the struct counter_device and and struct device are bound to the life time
> of the driver state struct.
> 
> Which means the struct device memory can get freed before the last reference
> is dropped, which leads to a use-after-free and undefined behavior.

Well, the driver struct is allocated using devm_kzalloc for all drivers.
So I think it's not *very* urgent to fix. Still you're right, this
should be addressed.
 
> The framework should be changed to rather then embedding the struct
> counter_device in the state struct to just have a pointer to it. With the
> struct counter_device having its own allocation that will be freed when the
> last reference to the struct device is dropped.

My favourite would be to implement a counter_device_alloc /
counter_device_add approach, similar to what spi_alloc_controller and
alloc_etherdev do. The downside is that this isn't typesafe either :-\

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 0/8] counter: Remove struct counter_device::priv
  2021-12-21 11:35   ` Uwe Kleine-König
@ 2021-12-21 12:04     ` Lars-Peter Clausen
  2021-12-21 13:22       ` Uwe Kleine-König
  2021-12-22 17:51       ` Uwe Kleine-König
  0 siblings, 2 replies; 10+ messages in thread
From: Lars-Peter Clausen @ 2021-12-21 12:04 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Alexandre Torgue, David Lechner, Fabrice Gasnier, Jarkko Nikula,
	Kamel Bouhara, Maxime Coquelin, Oleksij Rempel,
	Patrick Havelange, Syed Nayyar Waris, William Breathitt Gray,
	linux-arm-kernel, linux-iio, linux-kernel, linux-stm32

On 12/21/21 12:35 PM, Uwe Kleine-König wrote:
> Hello Lars,
>
> On Tue, Dec 21, 2021 at 12:12:12PM +0100, Lars-Peter Clausen wrote:
>> On 12/21/21 11:45 AM, Uwe Kleine-König wrote:
>>> similar to patch
>>> https://lore.kernel.org/r/4bde7cbd9e43a5909208102094444219d3154466.1640072891.git.vilhelm.gray@gmail.com
>>> the usage of struct counter_device::priv can be replaced by
>>> container_of which improves type safety and code size.
>>>
>>> This series depends on above patch, converts the remaining drivers and
>>> finally drops struct counter_device::priv.
>> Not sure if this is such a good idea. struct counter_device should not be
>> embedded in the drivers state struct in the first place.
> Just to mention it: My patch series didn't change this, this was already
> broken before.
I know, but this series has to be reverted when the framework is fixed.
>
>> struct counter_device contains a struct device, which is a reference counted
>> object. But by embedding it in the driver state struct the life time of both
>> the struct counter_device and and struct device are bound to the life time
>> of the driver state struct.
>>
>> Which means the struct device memory can get freed before the last reference
>> is dropped, which leads to a use-after-free and undefined behavior.
> Well, the driver struct is allocated using devm_kzalloc for all drivers.

devm_kzalloc() doesn't make a difference. The managed memory is freed 
when the parent device is unbound/removed. There may very well be 
reference to the counter_device at this point.

> So I think it's not *very* urgent to fix. Still you're right, this
> should be addressed.

Yes and no, this can trivially be used for privilege escalation, but 
then again on systems with a counter_device probably everything runs as 
root anyway.

>   
>> The framework should be changed to rather then embedding the struct
>> counter_device in the state struct to just have a pointer to it. With the
>> struct counter_device having its own allocation that will be freed when the
>> last reference to the struct device is dropped.
> My favourite would be to implement a counter_device_alloc /
> counter_device_add approach, similar to what spi_alloc_controller and
> alloc_etherdev do. The downside is that this isn't typesafe either :-\



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 0/8] counter: Remove struct counter_device::priv
  2021-12-21 12:04     ` Lars-Peter Clausen
@ 2021-12-21 13:22       ` Uwe Kleine-König
  2021-12-21 14:12         ` Lars-Peter Clausen
  2021-12-22 17:51       ` Uwe Kleine-König
  1 sibling, 1 reply; 10+ messages in thread
From: Uwe Kleine-König @ 2021-12-21 13:22 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Alexandre Torgue, David Lechner, Fabrice Gasnier, Jarkko Nikula,
	Kamel Bouhara, Maxime Coquelin, Oleksij Rempel,
	Patrick Havelange, Syed Nayyar Waris, William Breathitt Gray,
	linux-arm-kernel, linux-iio, linux-kernel, linux-stm32


[-- Attachment #1.1: Type: text/plain, Size: 1333 bytes --]

On Tue, Dec 21, 2021 at 01:04:50PM +0100, Lars-Peter Clausen wrote:
> On 12/21/21 12:35 PM, Uwe Kleine-König wrote:
> > On Tue, Dec 21, 2021 at 12:12:12PM +0100, Lars-Peter Clausen wrote:
> > > On 12/21/21 11:45 AM, Uwe Kleine-König wrote:
> > > > similar to patch
> > > > https://lore.kernel.org/r/4bde7cbd9e43a5909208102094444219d3154466.1640072891.git.vilhelm.gray@gmail.com
> > > > the usage of struct counter_device::priv can be replaced by
> > > > container_of which improves type safety and code size.
> > > > 
> > > > This series depends on above patch, converts the remaining drivers and
> > > > finally drops struct counter_device::priv.
> > > Not sure if this is such a good idea. struct counter_device should not be
> > > embedded in the drivers state struct in the first place.
> > Just to mention it: My patch series didn't change this, this was already
> > broken before.
> 
> I know, but this series has to be reverted when the framework is fixed.

All drivers have to be touched. With my patch series you have to modify
one function in each driver, without my patch you have touch nearly
every function.

*shrug*

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 0/8] counter: Remove struct counter_device::priv
  2021-12-21 13:22       ` Uwe Kleine-König
@ 2021-12-21 14:12         ` Lars-Peter Clausen
  0 siblings, 0 replies; 10+ messages in thread
From: Lars-Peter Clausen @ 2021-12-21 14:12 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Alexandre Torgue, David Lechner, Fabrice Gasnier, Jarkko Nikula,
	Kamel Bouhara, Maxime Coquelin, Oleksij Rempel,
	Patrick Havelange, Syed Nayyar Waris, William Breathitt Gray,
	linux-arm-kernel, linux-iio, linux-kernel, linux-stm32

On 12/21/21 2:22 PM, Uwe Kleine-König wrote:
> On Tue, Dec 21, 2021 at 01:04:50PM +0100, Lars-Peter Clausen wrote:
>> On 12/21/21 12:35 PM, Uwe Kleine-König wrote:
>>> On Tue, Dec 21, 2021 at 12:12:12PM +0100, Lars-Peter Clausen wrote:
>>>> On 12/21/21 11:45 AM, Uwe Kleine-König wrote:
>>>>> similar to patch
>>>>> https://lore.kernel.org/r/4bde7cbd9e43a5909208102094444219d3154466.1640072891.git.vilhelm.gray@gmail.com
>>>>> the usage of struct counter_device::priv can be replaced by
>>>>> container_of which improves type safety and code size.
>>>>>
>>>>> This series depends on above patch, converts the remaining drivers and
>>>>> finally drops struct counter_device::priv.
>>>> Not sure if this is such a good idea. struct counter_device should not be
>>>> embedded in the drivers state struct in the first place.
>>> Just to mention it: My patch series didn't change this, this was already
>>> broken before.
>> I know, but this series has to be reverted when the framework is fixed.
> All drivers have to be touched. With my patch series you have to modify
> one function in each driver, without my patch you have touch nearly
> every function.
>
I'm not so sure. I don't see how you have to modify every function. 
You'd keep using priv to get a pointer to the state struct.

That said having a centralized function in each driver to get the state 
struct from the counter device doesn't hurt either.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 0/8] counter: Remove struct counter_device::priv
  2021-12-21 12:04     ` Lars-Peter Clausen
  2021-12-21 13:22       ` Uwe Kleine-König
@ 2021-12-22 17:51       ` Uwe Kleine-König
  1 sibling, 0 replies; 10+ messages in thread
From: Uwe Kleine-König @ 2021-12-22 17:51 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Alexandre Torgue, David Lechner, Fabrice Gasnier, Jarkko Nikula,
	Kamel Bouhara, Maxime Coquelin, Oleksij Rempel,
	Patrick Havelange, Syed Nayyar Waris, William Breathitt Gray,
	linux-arm-kernel, linux-iio, linux-kernel, linux-stm32


[-- Attachment #1.1: Type: text/plain, Size: 2985 bytes --]

Hello,

On Tue, Dec 21, 2021 at 01:04:50PM +0100, Lars-Peter Clausen wrote:
> On 12/21/21 12:35 PM, Uwe Kleine-König wrote:
> > On Tue, Dec 21, 2021 at 12:12:12PM +0100, Lars-Peter Clausen wrote:
> > > On 12/21/21 11:45 AM, Uwe Kleine-König wrote:
> > > > similar to patch
> > > > https://lore.kernel.org/r/4bde7cbd9e43a5909208102094444219d3154466.1640072891.git.vilhelm.gray@gmail.com
> > > > the usage of struct counter_device::priv can be replaced by
> > > > container_of which improves type safety and code size.
> > > > 
> > > > This series depends on above patch, converts the remaining drivers and
> > > > finally drops struct counter_device::priv.
> > > Not sure if this is such a good idea. struct counter_device should not be
> > > embedded in the drivers state struct in the first place.
> > Just to mention it: My patch series didn't change this, this was already
> > broken before.
> I know, but this series has to be reverted when the framework is fixed.
> > 
> > > struct counter_device contains a struct device, which is a reference counted
> > > object. But by embedding it in the driver state struct the life time of both
> > > the struct counter_device and and struct device are bound to the life time
> > > of the driver state struct.
> > > 
> > > Which means the struct device memory can get freed before the last reference
> > > is dropped, which leads to a use-after-free and undefined behavior.
> > Well, the driver struct is allocated using devm_kzalloc for all drivers.
> 
> devm_kzalloc() doesn't make a difference. The managed memory is freed when
> the parent device is unbound/removed. There may very well be reference to
> the counter_device at this point.
> 
> > So I think it's not *very* urgent to fix. Still you're right, this
> > should be addressed.
> 
> Yes and no, this can trivially be used for privilege escalation, but then
> again on systems with a counter_device probably everything runs as root
> anyway.

I could provoke an oops with the following shell command:


	{ sleep 5; echo bang; } > /dev/counter0 & sleep 1; echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind

I have a protype here to split counter_register() into counter_alloc() +
counter_add(), but I didn't convert a driver to it yet. If you want to
take a look, it's currently available from

	https://git.pengutronix.de/git/ukl/linux counter-dev-livetime

(or if you prefer a webif:

	https://git.pengutronix.de/cgit/ukl/linux/log/?h=counter-dev-livetime

). I planned to just invest a two or so hours to fix this. But the plan
failed (one reason is that v5.16-rc6 failed to boot on the stm32mp1 I
work on and I bisected that first.)

Maybe I find some time between the years to get this forward a bit.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-12-22 17:53 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-21 10:45 [PATCH 0/8] counter: Remove struct counter_device::priv Uwe Kleine-König
2021-12-21 10:45 ` [PATCH 5/8] counter: microchip-tcp-capture: Use container_of instead of " Uwe Kleine-König
2021-12-21 10:45 ` [PATCH 6/8] counter: stm32-lptimer-cnt: " Uwe Kleine-König
2021-12-21 10:45 ` [PATCH 7/8] counter: stm32-timer-cnt: " Uwe Kleine-König
2021-12-21 11:12 ` [PATCH 0/8] counter: Remove " Lars-Peter Clausen
2021-12-21 11:35   ` Uwe Kleine-König
2021-12-21 12:04     ` Lars-Peter Clausen
2021-12-21 13:22       ` Uwe Kleine-König
2021-12-21 14:12         ` Lars-Peter Clausen
2021-12-22 17:51       ` Uwe Kleine-König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).