Re: [PATCH 05/21] can: bcm: Don't initialized an unused hrtimer

From: Oliver Hartkopp
Date: Wed Oct 30 2024 - 06:50:14 EST




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@xxxxxxxxxxxxx>
---
Cc: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
---
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) {