linux-kernel - Re: [RFC PATCH 1/2] can: dummy_can: add CAN termination support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251225072857.53085-1-rakuram.e96@gmail.com>
Date: Thu, 25 Dec 2025 12:58:54 +0530
From: Rakuram Eswaran <rakuram.e96@...il.com>
To: mailhol@...nel.org
Cc: linux-can@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	mkl@...gutronix.de,
	rakuram.e96@...il.com,
	socketcan@...tkopp.net
Subject: Re: [RFC PATCH 1/2] can: dummy_can: add CAN termination support

Hi Vincent, 

Thank you for the review and the detailed feedback.

On Wed, 24 Dec 2025 at 03:03, Vincent Mailhol <mailhol@...nel.org> wrote:
>
> Hi Rakuram,
>
> Thanks for the patch. My comments are only on the cosmetic aspect.
>
> Le 27/11/2025 à 20:18, Rakuram Eswaran a écrit :
> > Add support for configuring bus termination in the dummy_can driver.
> > This allows users to emulate a properly terminated CAN bus when
> > setting up virtual test environments.
> >
> > Signed-off-by: Rakuram Eswaran <rakuram.e96@...il.com>
> > ---
> > Tested the termination setting using below iproute commands:
> >
> >   ip link set can0 type can termination 120
> >   ip link set can0 type can termination off
>
> When you test, do not forget to also try incorrect values ;)
>
>   ip link set can0 type can termination 100

Noted. I did test with invalid numeric values and observed that
they were ignored.

>   ip link set can0 type can termination potato

Good point — I had not explicitly tested non-numeric inputs.
I will verify this case as well.

>
> (I think that the code is correct, just see this as a generic
> comment).
>
> >  drivers/net/can/dummy_can.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/net/can/dummy_can.c b/drivers/net/can/dummy_can.c
> > index 41953655e3d3..2949173547e6 100644
> > --- a/drivers/net/can/dummy_can.c
> > +++ b/drivers/net/can/dummy_can.c
> > @@ -23,6 +23,21 @@ struct dummy_can {
> >
> >  static struct dummy_can *dummy_can;
> >
> > +static const u16 dummy_can_termination_const[] = {
> > +     CAN_TERMINATION_DISABLED,       /* 0 = off */
> > +     120,                            /* 120 Ohms */
> > +};
> > +
> > +static int dummy_can_set_termination(struct net_device *dev, u16 term)
> > +{
> > +     struct dummy_can *priv = netdev_priv(dev);
> > +
> > +     netdev_dbg(dev, "set termination to %u Ohms\n", term);
> > +     priv->can.termination = term;
> > +
> > +     return 0;
> > +}
>
> The driver has a kind of structure:
>
>   - first the const bittiming struct declarations
>   - then the dummy_can_print_*() functions
>   - finally the actual code
>
> Try to preserve this structure when adding your changes.

Acknowledged. I will reorder the termination-related additions to match
the existing structure in the next revision.

>
> >  static const struct can_bittiming_const dummy_can_bittiming_const = {
> >       .name = "dummy_can CC",
> >       .tseg1_min = 2,
> > @@ -250,6 +265,12 @@ static int __init dummy_can_init(void)
> >       priv->can.xl.data_bittiming_const = &dummy_can_xl_databittiming_const;
> >       priv->can.xl.tdc_const = &dummy_can_xl_tdc_const;
> >       priv->can.xl.pwm_const = &dummy_can_pwm_const;
> > +    
> > +     /* Advertise software termination support */
>
> This comment doesn't add much value. You may omit it.
>

Ack. Will remove it.

> > +     priv->can.termination_const = dummy_can_termination_const;
> > +     priv->can.termination_const_cnt = ARRAY_SIZE(dummy_can_termination_const);
> > +     priv->can.do_set_termination = dummy_can_set_termination;
>
> Here also try to maintain so kind of order: your declaration of
> dummy_can_termination_const is before the other const struct
> declarations, but the priv->can assignment is done after the other
> assignments. Not a big deal but it is nicer to keep the declaration
> and the assignments in the same order.
>

Ack. I will align the declaration and the corresponding assignments to
keep the ordering consistent.

I will address these points and send an updated version. Is it okay to send
the next version without the RFC tag?

Best Regards,
Rakuram