Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210814091750.73931-1-mailhol.vincent@wanadoo.fr>
 <20210814091750.73931-6-mailhol.vincent@wanadoo.fr> <20210814111243.biquurwkyzmhmsad@pengutronix.de>
In-Reply-To: <20210814111243.biquurwkyzmhmsad@pengutronix.de>
From:   Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Date:   Sun, 15 Aug 2021 12:38:00 +0900
Message-ID: <CAMZ6RqK9pBP3VH7s78G90KXWe6nnm_bs+xbT+v-N3uBS3VSW6g@mail.gmail.com>
Subject: Re: [PATCH v4 5/7] can: netlink: add interface for CAN-FD Transmitter
 Delay Compensation (TDC)
To:     Marc Kleine-Budde <mkl@pengutronix.de>
Cc:     linux-can <linux-can@vger.kernel.org>,
        =?UTF-8?Q?Stefan_M=C3=A4tje?= <Stefan.Maetje@esd.eu>,
        netdev <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat. 14 Aug 2021 at 20:12, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 14.08.2021 18:17:48, Vincent Mailhol wrote:
> [...]
>
> >  static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> >                         struct nlattr *data[],
> >                         struct netlink_ext_ack *extack)
> >  {
> >       struct can_priv *priv = netdev_priv(dev);
> > +     u32 tdc_mask = 0;
> >       int err;
> >
> >       /* We need synchronization with dev->stop() */
> > @@ -107,6 +179,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> >               struct can_ctrlmode *cm;
> >               u32 ctrlstatic;
> >               u32 maskedflags;
> > +             u32 tdc_flags;
> >
> >               /* Do not allow changing controller mode while running */
> >               if (dev->flags & IFF_UP)
> > @@ -138,7 +211,18 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> >                       dev->mtu = CAN_MTU;
> >                       memset(&priv->data_bittiming, 0,
> >                              sizeof(priv->data_bittiming));
> > +                     memset(&priv->tdc, 0, sizeof(priv->tdc));
> > +                     priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
> >               }
> > +
> > +             tdc_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;
> > +             /* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
> > +             if (tdc_flags == CAN_CTRLMODE_TDC_MASK)
> > +                     return -EOPNOTSUPP;
> > +             /* If one of CAN_CTRLMODE_TDC_* is set then TDC must be set */
> > +             if (tdc_flags && !data[IFLA_CAN_TDC])
> > +                     return -EOPNOTSUPP;
>
> These don't need information form the can_priv, right? So these checks
> can be moved to can_validate()?

ACK. Actually, this comment applies not only to can_changelink()
but also to can_tdc_changelink().  I just sent a v5 where I made
sure to move all the checks that do not rely on can_priv to
can_validate().


Yours sincerely,
Vincent