Re: ip link valid options checking

["Stefan Mätje" <Stefan.Maetje@esd.eu>]

Am Samstag, den 10.07.2021, 12:19 +0200 schrieb Oliver Hartkopp:
> Hi Stefan,
> 
> On 09.07.21 17:07, Stefan Mätje wrote:
> > Am Freitag, den 09.07.2021, 14:00 +0200 schrieb Vincent MAILHOL:
> > 
> > Unfortunately the netlink kernel interface gives access only to the
> > IFLA_CAN_CTRLMODE data which boils down to an access of the netdev_priv(dev)-
> > > priv->ctrlmode flags set in the driver (see also in a Linux tree under
> > 
> > drivers/net/can/dev/netlink.c). But the "ctrlmode" flags represent only the
> > current setup. So you can see if CAN-FD mode is currently enabled.
> > 
> > But I think the thread opener wants to know in advance if the kernel gives us
> > the information what modes a certain driver supports. That is encoded in
> > netdev_priv(dev)->priv->ctrlmode_supported and netdev_priv(dev)->priv-
> > > ctrlmode_static. But for these flags there is no netlink interface to
> > 
> > interrogate that settings at the moment.
> > 
> > So you can't see in advance if a CAN driver would support for instance triple-
> > sampling or the CAN_CTRLMODE_CC_LEN8_DLC mode. To know it you must try to set
> > such option atm. I think.
> 
> Yes, but the settings of priv->ctrlmode_static may lead into problems 
> too as there might be a fixed setting that can not be altered.
> 
> As you already pointed out the netlink interface only provides the 
> current setting of priv->ctrlmode.
> 
> Btw. the struct can_ctrlmode has two u32 elements, so we could use the 
> mask element to pass the priv->ctrlmode_supported value to the user space:
> 
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index e38c2566aff4..91c6ae06a576 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -259,11 +259,12 @@ static size_t can_get_size(const struct net_device 
> *dev)
>   }
> 
>   static int can_fill_info(struct sk_buff *skb, const struct net_device 
> *dev)
>   {
>          struct can_priv *priv = netdev_priv(dev);
> -       struct can_ctrlmode cm = {.flags = priv->ctrlmode};
> +       struct can_ctrlmode cm = {.flags = priv->ctrlmode,
> +               .mask = priv->ctrlmode_supported};
>          struct can_berr_counter bec = { };
>          enum can_state state = priv->state;
> 
>          if (priv->do_get_state)
>                  priv->do_get_state(dev, &state);
> 
> Additionally we could also make the mask element in struct can_ctrlmode 
> a union with a 'supported' element ...
> 
> But this is, what would be needed here, right?

It looks like a step in the right direction. But I want to add two points to 
the discussion.

1) I think the flags member of struct can_ctrlmode should contain the 
ctrlmode_static flags in any case, because the ctrlmode_static flags also describe
a part of the current setting. I. e. the changed lines in your patch should read then:

+       struct can_ctrlmode cm = {.flags = priv->ctrlmode | priv->ctrlmode_static,
+               .mask = priv->ctrlmode_supported};

I believe this did not show up as an error till now because ctrlmode_static
is not used by any driver atm.


Then an application can possibly do something like this to get the needed 
information (pseudo code):

app_decode_ctrlmode (struct can_ctrlmode *cm) {
	u32 app_ctrlmode,  app_ctrlmode_supported, app_ctrlmode_static;

	app_ctrlmode = cm->flags;			/* current setting */
	app_ctrlmode_supported = cm->mask; 		/* changeable settings */
	app_ctrlmode_static = cm->flags & ~cm->mask;	/* fixed settings */
}


2) I don't know how an application like the "ip" tool could discriminate whether 
it talks to a kernel with the new interface (mask element contains 
ctrlmode_supported) or to a kernel with the old interface. This is because on 
an old kernel the mask element contains uninitialized data from the kernel 
stack as the mask element is not set till now.

I don't see how this decision could be made by the "ip" tool.

Best regards,
    Stefan