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

[Oliver Hartkopp]

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) {