| Message ID | 5c0f2f1bd1dc7bbb9500afd4273e36378e00a35d.1662606045.git.william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: bcm: random optimizations | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Sorry, but NACK. The curr_frame counter handles the sequence counter of multiplex messages. Even when this single send attempt failed the curr_frame counter has to continue. For that reason the comment about statistics *before* the curr_frame++ might be misleading. A potential improvement could be: if (!(can_send(skb, 1))) op->frames_abs++; op->currframe++; But as op->frames_abs is a functional unused(!) value for tx ops and only displayed via procfs I would NOT tag such improvement as a 'fix' which might then be queued up for stable. This could be something for the can-next tree ... Best regards, Oliver On 08.09.22 05:04, Ziyang Xuan wrote: > If can_send() fail, it should not update statistics in bcm_can_tx(). > Add the result check for can_send() in bcm_can_tx(). > > Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > --- > net/can/bcm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/can/bcm.c b/net/can/bcm.c > index e2783156bfd1..8f5d704a409f 100644 > --- a/net/can/bcm.c > +++ b/net/can/bcm.c > @@ -298,7 +298,8 @@ static void bcm_can_tx(struct bcm_op *op) > /* send with loopback */ > skb->dev = dev; > can_skb_set_owner(skb, op->sk); > - can_send(skb, 1); > + if (can_send(skb, 1)) > + goto out; > > /* update statistics */ > op->currframe++;
> Sorry, but NACK. > > The curr_frame counter handles the sequence counter of multiplex messages. > > Even when this single send attempt failed the curr_frame counter has to continue. > > For that reason the comment about statistics *before* the curr_frame++ might be misleading. > > A potential improvement could be: > > if (!(can_send(skb, 1))) > op->frames_abs++; > > op->currframe++; > > But as op->frames_abs is a functional unused(!) value for tx ops and only displayed via procfs I would NOT tag such improvement as a 'fix' which might then be queued up for stable. > I will modify and remove 'Fixes' tag in v2. Thank you for your review. > This could be something for the can-next tree ... > > Best regards, > Oliver > > > On 08.09.22 05:04, Ziyang Xuan wrote: >> If can_send() fail, it should not update statistics in bcm_can_tx(). >> Add the result check for can_send() in bcm_can_tx(). >> >> Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol") >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >> --- >> net/can/bcm.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/net/can/bcm.c b/net/can/bcm.c >> index e2783156bfd1..8f5d704a409f 100644 >> --- a/net/can/bcm.c >> +++ b/net/can/bcm.c >> @@ -298,7 +298,8 @@ static void bcm_can_tx(struct bcm_op *op) >> /* send with loopback */ >> skb->dev = dev; >> can_skb_set_owner(skb, op->sk); >> - can_send(skb, 1); >> + if (can_send(skb, 1)) >> + goto out; >> /* update statistics */ >> op->currframe++; > .
On 08.09.2022 08:47:57, Oliver Hartkopp wrote: > Sorry, but NACK. > > The curr_frame counter handles the sequence counter of multiplex messages. > > Even when this single send attempt failed the curr_frame counter has to > continue. > > For that reason the comment about statistics *before* the curr_frame++ might > be misleading. > > A potential improvement could be: > > if (!(can_send(skb, 1))) Nitpick: In the kernel we usually assign the return value to a variable first, and evaluate this variable in the if (). > op->frames_abs++; > > op->currframe++; Marc
diff --git a/net/can/bcm.c b/net/can/bcm.c index e2783156bfd1..8f5d704a409f 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -298,7 +298,8 @@ static void bcm_can_tx(struct bcm_op *op) /* send with loopback */ skb->dev = dev; can_skb_set_owner(skb, op->sk); - can_send(skb, 1); + if (can_send(skb, 1)) + goto out; /* update statistics */ op->currframe++;
If can_send() fail, it should not update statistics in bcm_can_tx(). Add the result check for can_send() in bcm_can_tx(). Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- net/can/bcm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)