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: <20211026121651.1814251-1-mailhol.vincent@wanadoo.fr> <20211029124411.lkmngckiiwotste7@pengutronix.de>
In-Reply-To: <20211029124411.lkmngckiiwotste7@pengutronix.de>
From:   Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Date:   Sat, 30 Oct 2021 01:14:46 +0900
Message-ID: <CAMZ6Rq+RH3C3C5=qzZ_CRRF3bnW+oDB7_P_OW3UEp9Ty2GNfbQ@mail.gmail.com>
Subject: Re: [PATCH v4] can: netlink: report the CAN controller mode supported flags
To:     Marc Kleine-Budde <mkl@pengutronix.de>
Cc:     linux-can <linux-can@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri. 29 Oct 2021 at 21:44, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 26.10.2021 21:16:51, Vincent Mailhol wrote:
> > This patch introduces a method for the user to check both the
> > supported and the static capabilities. The proposed method reuses the
> > existing struct can_ctrlmode and thus do not need a new IFLA_CAN_*
> > entry.
> >
> > Currently, the CAN netlink interface provides no easy ways to check
> > the capabilities of a given controller. The only method from the
> > command line is to try each CAN_CTRLMODE_* individually to check
> > whether the netlink interface returns an -EOPNOTSUPP error or not
> > (alternatively, one may find it easier to directly check the source
> > code of the driver instead...)
> >
> > It appears that can_ctrlmode::mask is only used in one direction: from
> > the userland to the kernel. So we can just reuse this field in the
> > other direction (from the kernel to userland). But, because the
> > semantic is different, we use a union to give this field a proper
> > name: "supported".
> >
> > The union is tagged as packed to prevent any ABI from adding
> > padding. In fact, any padding added after the union would change the
> > offset of can_ctrlmode::flags within the structure and thus break the
> > UAPI backward compatibility. References:
> >
> >   - ISO/IEC 9899-1999, section 6.7.2.1 "Structure and union
> >     specifiers", clause 15: "There may be unnamed padding at the end
> >     of a structure or union."
> >
> >   - The -mstructure-size-boundary=64 ARM option in GCC:
> >     https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html
> >
> >   - A similar issue which occurred on struct can_frame:
> >     https://lore.kernel.org/linux-can/212c8bc3-89f9-9c33-ed1b-b50ac04e7532@hartkopp.net
> >
> > Below table explains how the two fields can_ctrlmode::supported and
> > can_ctrlmode::flags, when masked with any of the CAN_CTRLMODE_* bit
> > flags, allow us to identify both the supported and the static
> > capabilities:
> >
> >  supported &  flags &         Controller capabilities
> >  CAN_CTRLMODE_*       CAN_CTRLMODE_*
> >  -----------------------------------------------------------------------
> >  false                false           Feature not supported (always disabled)
> >  false                true            Static feature (always enabled)
> >  true         false           Feature supported but disabled
> >  true         true            Feature supported and enabled
>
> What about forwards and backwards compatibility?

Backward compatibility (new kernel, old iproute2) should be OK: the
kernel will report the value but it will not be consumed.

> Using the new ip (or any other user space app) on an old kernel, it
> looks like enabled features are static features. For example the ip
> output on a mcp251xfd with enabled CAN-FD, which is _not_ static.
>
> |         "linkinfo": {
> |             "info_kind": "can",
> |             "info_data": {
> |                 "ctrlmode": [ "FD" ],
> |                 "ctrlmode_static": [ "FD" ],
> |                 "state": "ERROR-ACTIVE",
> |                 "berr_counter": {
> |                     "tx": 0,
> |                     "rx": 0
> |                 },

I missed that, nice catch!

> Is it worth and add a new IFLA_CAN_CTRLMODE_EXT that doesn't pass a
> struct, but is a NLA_NESTED type?

Adding a new nested entry only for one u32 seemed overkill to
me. This is why I tried to do the change as tiny as possible.

I would like to use this IFLA_CAN_CTRLMODE_EXT as a last
resort. I gave it a second thought and I have another idea: we
could keep the exact same kernel code and just have the userland
to discard the can_ctrlmode::supported if it is zero. The caveat
would be that it will be impossible to report the static features
of a controller which do have ctrlmode_static features but no
ctrlmode_supported features. Other use cases would be
supported. As a matter of fact, the two drivers which rely on the
static features (m_can and rcar_canfd) do also have supported
modes. So discarding can_ctrlmode::supported when it is zero would
introduce no issue for all existing drivers. The only remaining
risk is for the yet to be introduced drivers.

So if we are ready to accept the limitation that we would not be
able to report the static features of such hypothetical drivers,
then we can keep the current patch (maybe add a comment) and
introduce an if switch in iproute2 to discard zero value.

The probability for such a driver to ever exist is already low. I
think that this limitation is acceptable.

What do you think?