The hrtimer "thrtimer" is not used for TX. But this timer is initialized
regardless.
Remove the hrtimer_init() for the unused hrtimer and change bcm_remove_op()
to make sure hrtimer_cancel() is not called with the uninitialized hrtimer.
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Jakub Kicinski <kuba@kernel.org>
---
net/can/bcm.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 217049fa496e..792528f7bce2 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -780,10 +780,11 @@ static void bcm_free_op_rcu(struct rcu_head *rcu_head)
kfree(op);
}
-static void bcm_remove_op(struct bcm_op *op)
+static void bcm_remove_op(struct bcm_op *op, bool is_tx)
{
hrtimer_cancel(&op->timer);
- hrtimer_cancel(&op->thrtimer);
+ if (!is_tx)
+ hrtimer_cancel(&op->thrtimer);
call_rcu(&op->rcu, bcm_free_op_rcu);
}
@@ -844,7 +845,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
bcm_rx_handler, op);
list_del(&op->list);
- bcm_remove_op(op);
+ bcm_remove_op(op, false);
return 1; /* done */
}
}
@@ -864,7 +865,7 @@ static int bcm_delete_tx_op(struct list_head *ops, struct bcm_msg_head *mh,
if ((op->can_id == mh->can_id) && (op->ifindex == ifindex) &&
(op->flags & CAN_FD_FRAME) == (mh->flags & CAN_FD_FRAME)) {
list_del(&op->list);
- bcm_remove_op(op);
+ bcm_remove_op(op, true);
return 1; /* done */
}
}
@@ -1015,10 +1016,6 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
HRTIMER_MODE_REL_SOFT);
op->timer.function = bcm_tx_timeout_handler;
- /* currently unused in tx_ops */
- hrtimer_init(&op->thrtimer, CLOCK_MONOTONIC,
- HRTIMER_MODE_REL_SOFT);
-
/* add this bcm_op to the list of the tx_ops */
list_add(&op->list, &bo->tx_ops);
@@ -1277,7 +1274,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
if (err) {
/* this bcm rx op is broken -> remove it */
list_del(&op->list);
- bcm_remove_op(op);
+ bcm_remove_op(op, false);
return err;
}
}
@@ -1581,7 +1578,7 @@ static int bcm_release(struct socket *sock)
#endif /* CONFIG_PROC_FS */
list_for_each_entry_safe(op, next, &bo->tx_ops, list)
- bcm_remove_op(op);
+ bcm_remove_op(op, true);
list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
/*
@@ -1613,7 +1610,7 @@ static int bcm_release(struct socket *sock)
synchronize_rcu();
list_for_each_entry_safe(op, next, &bo->rx_ops, list)
- bcm_remove_op(op);
+ bcm_remove_op(op, false);
/* remove device reference */
if (bo->bound) {
--
2.39.5
On 28.10.24 08:29, Nam Cao wrote:
> The hrtimer "thrtimer" is not used for TX. But this timer is initialized
> regardless.
>
> Remove the hrtimer_init() for the unused hrtimer and change bcm_remove_op()
> to make sure hrtimer_cancel() is not called with the uninitialized hrtimer.
NAK.
There are several other occurrences of thrtimer that are not covered by
RX/TX distinction, where the second timer is canceled.
This one-time init and cancel of an unused hrtimer costs nearly nothing
and is not even in any hot path.
So this incomplete patch only adds complexity and potential error cases
in some 20 y/o code for nothing.
Therefore I would like to skip it.
Thanks,
Oliver
>
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> ---
> net/can/bcm.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 217049fa496e..792528f7bce2 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -780,10 +780,11 @@ static void bcm_free_op_rcu(struct rcu_head *rcu_head)
> kfree(op);
> }
>
> -static void bcm_remove_op(struct bcm_op *op)
> +static void bcm_remove_op(struct bcm_op *op, bool is_tx)
> {
> hrtimer_cancel(&op->timer);
> - hrtimer_cancel(&op->thrtimer);
> + if (!is_tx)
> + hrtimer_cancel(&op->thrtimer);
>
> call_rcu(&op->rcu, bcm_free_op_rcu);
> }
> @@ -844,7 +845,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
> bcm_rx_handler, op);
>
> list_del(&op->list);
> - bcm_remove_op(op);
> + bcm_remove_op(op, false);
> return 1; /* done */
> }
> }
> @@ -864,7 +865,7 @@ static int bcm_delete_tx_op(struct list_head *ops, struct bcm_msg_head *mh,
> if ((op->can_id == mh->can_id) && (op->ifindex == ifindex) &&
> (op->flags & CAN_FD_FRAME) == (mh->flags & CAN_FD_FRAME)) {
> list_del(&op->list);
> - bcm_remove_op(op);
> + bcm_remove_op(op, true);
> return 1; /* done */
> }
> }
> @@ -1015,10 +1016,6 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> HRTIMER_MODE_REL_SOFT);
> op->timer.function = bcm_tx_timeout_handler;
>
> - /* currently unused in tx_ops */
> - hrtimer_init(&op->thrtimer, CLOCK_MONOTONIC,
> - HRTIMER_MODE_REL_SOFT);
> -
> /* add this bcm_op to the list of the tx_ops */
> list_add(&op->list, &bo->tx_ops);
>
> @@ -1277,7 +1274,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> if (err) {
> /* this bcm rx op is broken -> remove it */
> list_del(&op->list);
> - bcm_remove_op(op);
> + bcm_remove_op(op, false);
> return err;
> }
> }
> @@ -1581,7 +1578,7 @@ static int bcm_release(struct socket *sock)
> #endif /* CONFIG_PROC_FS */
>
> list_for_each_entry_safe(op, next, &bo->tx_ops, list)
> - bcm_remove_op(op);
> + bcm_remove_op(op, true);
>
> list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
> /*
> @@ -1613,7 +1610,7 @@ static int bcm_release(struct socket *sock)
> synchronize_rcu();
>
> list_for_each_entry_safe(op, next, &bo->rx_ops, list)
> - bcm_remove_op(op);
> + bcm_remove_op(op, false);
>
> /* remove device reference */
> if (bo->bound) {
On Wed, Oct 30, 2024 at 11:49:49AM +0100, Oliver Hartkopp wrote: > On 28.10.24 08:29, Nam Cao wrote: > > The hrtimer "thrtimer" is not used for TX. But this timer is initialized > > regardless. > > > > Remove the hrtimer_init() for the unused hrtimer and change bcm_remove_op() > > to make sure hrtimer_cancel() is not called with the uninitialized hrtimer. > > NAK. > > There are several other occurrences of thrtimer that are not covered by > RX/TX distinction, where the second timer is canceled. > > This one-time init and cancel of an unused hrtimer costs nearly nothing and > is not even in any hot path. > > So this incomplete patch only adds complexity and potential error cases in > some 20 y/o code for nothing. The "real" motivation is preparing to use hrtimer_setup() instead of hrtimer_init() [1] and deleting hrtimer_init() [2]. The new function mandates a callback function, and since the TX thrtimer doesn't have a callback function, hrtimer_setup() cannot be used. Your concerns are also valid. So I can drop this patch, and use a dummy function to make hrtimer_setup() happy, like how it's done for the rt2x00 driver [3]. It will make the driver a bit ugly, but it's obvious that it won't cause any regression. Best regards, Nam [1] https://lore.kernel.org/lkml/e4ce3a3a28625d54ef93e47bfb02f7ffb741758a.1729865232.git.namcao@linutronix.de/ [2] https://lore.kernel.org/lkml/7bde2762d82d30dab184c7a747e76afc41208da0.1729865740.git.namcao@linutronix.de/ [3] https://lore.kernel.org/lkml/49f2bce487f56eb2a3ff572ea6d7de0a43560c0f.1729865232.git.namcao@linutronix.de/
On 30.10.24 13:15, Nam Cao wrote: > On Wed, Oct 30, 2024 at 11:49:49AM +0100, Oliver Hartkopp wrote: >> On 28.10.24 08:29, Nam Cao wrote: >>> The hrtimer "thrtimer" is not used for TX. But this timer is initialized >>> regardless. >>> >>> Remove the hrtimer_init() for the unused hrtimer and change bcm_remove_op() >>> to make sure hrtimer_cancel() is not called with the uninitialized hrtimer. >> >> NAK. >> >> There are several other occurrences of thrtimer that are not covered by >> RX/TX distinction, where the second timer is canceled. >> >> This one-time init and cancel of an unused hrtimer costs nearly nothing and >> is not even in any hot path. >> >> So this incomplete patch only adds complexity and potential error cases in >> some 20 y/o code for nothing. > > The "real" motivation is preparing to use hrtimer_setup() instead of > hrtimer_init() [1] and deleting hrtimer_init() [2]. The new function > mandates a callback function, and since the TX thrtimer doesn't have a > callback function, hrtimer_setup() cannot be used. > > Your concerns are also valid. So I can drop this patch, and use a dummy > function to make hrtimer_setup() happy, like how it's done for the rt2x00 > driver [3]. It will make the driver a bit ugly, but it's obvious that it > won't cause any regression. Many thanks for your efforts! That's a good approach! Best regards, Oliver > > Best regards, > Nam > > [1] https://lore.kernel.org/lkml/e4ce3a3a28625d54ef93e47bfb02f7ffb741758a.1729865232.git.namcao@linutronix.de/ > [2] https://lore.kernel.org/lkml/7bde2762d82d30dab184c7a747e76afc41208da0.1729865740.git.namcao@linutronix.de/ > [3] https://lore.kernel.org/lkml/49f2bce487f56eb2a3ff572ea6d7de0a43560c0f.1729865232.git.namcao@linutronix.de/
Just an additional idea:
Would it probably make sense to create a generic dummy function for all
those cases like in the referenced rt2x00 driver?
E.g. as (inline?) function in hrtimer.h
enum hrtimer_restart hrtimer_nop_callback(struct hrtimer *timer)
{
return HRTIMER_NORESTART;
}
??
Best regards,
Oliver
On 30.10.24 15:51, Oliver Hartkopp wrote:
>
>
> On 30.10.24 13:15, Nam Cao wrote:
>> On Wed, Oct 30, 2024 at 11:49:49AM +0100, Oliver Hartkopp wrote:
>>> On 28.10.24 08:29, Nam Cao wrote:
>>>> The hrtimer "thrtimer" is not used for TX. But this timer is
>>>> initialized
>>>> regardless.
>>>>
>>>> Remove the hrtimer_init() for the unused hrtimer and change
>>>> bcm_remove_op()
>>>> to make sure hrtimer_cancel() is not called with the uninitialized
>>>> hrtimer.
>>>
>>> NAK.
>>>
>>> There are several other occurrences of thrtimer that are not covered by
>>> RX/TX distinction, where the second timer is canceled.
>>>
>>> This one-time init and cancel of an unused hrtimer costs nearly
>>> nothing and
>>> is not even in any hot path.
>>>
>>> So this incomplete patch only adds complexity and potential error
>>> cases in
>>> some 20 y/o code for nothing.
>>
>> The "real" motivation is preparing to use hrtimer_setup() instead of
>> hrtimer_init() [1] and deleting hrtimer_init() [2]. The new function
>> mandates a callback function, and since the TX thrtimer doesn't have a
>> callback function, hrtimer_setup() cannot be used.
>>
>> Your concerns are also valid. So I can drop this patch, and use a dummy
>> function to make hrtimer_setup() happy, like how it's done for the rt2x00
>> driver [3]. It will make the driver a bit ugly, but it's obvious that it
>> won't cause any regression.
>
> Many thanks for your efforts!
> That's a good approach!
>
> Best regards,
> Oliver
>
>>
>> Best regards,
>> Nam
>>
>> [1] https://lore.kernel.org/lkml/
>> e4ce3a3a28625d54ef93e47bfb02f7ffb741758a.1729865232.git.namcao@linutronix.de/
>> [2] https://lore.kernel.org/
>> lkml/7bde2762d82d30dab184c7a747e76afc41208da0.1729865740.git.namcao@linutronix.de/
>> [3] https://lore.kernel.org/
>> lkml/49f2bce487f56eb2a3ff572ea6d7de0a43560c0f.1729865232.git.namcao@linutronix.de/
>
On Wed, Oct 30 2024 at 16:01, Oliver Hartkopp wrote:
> Just an additional idea:
>
> Would it probably make sense to create a generic dummy function for all
> those cases like in the referenced rt2x00 driver?
>
> E.g. as (inline?) function in hrtimer.h
>
> enum hrtimer_restart hrtimer_nop_callback(struct hrtimer *timer)
> {
> return HRTIMER_NORESTART;
> }
Yes.
© 2016 - 2024 Red Hat, Inc.