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;
Date:   Fri, 29 Oct 2021 14:44:11 +0200
From:   Marc Kleine-Budde <mkl@pengutronix.de>
To:     Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Cc:     linux-can@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] can: netlink: report the CAN controller mode
 supported flags
Message-ID: <20211029124411.lkmngckiiwotste7@pengutronix.de>
References: <20211026121651.1814251-1-mailhol.vincent@wanadoo.fr>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="eyd3iugdqq4tttdt"
Content-Disposition: inline
In-Reply-To: <20211026121651.1814251-1-mailhol.vincent@wanadoo.fr>
Precedence: bulk


--eyd3iugdqq4tttdt
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline

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?

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
|                 },

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

Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

--eyd3iugdqq4tttdt
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEK3kIWJt9yTYMP3ehqclaivrt76kFAmF77JgACgkQqclaivrt
76luHgf/SLb+/I09MNvU0um+puegckHKw9U+uOUHe2Tx+m2KCXD56Bya1sKOlnrm
P7jOp2pdhbHr99n1wVeAeV9u9OGJC5sc+HxQrjIr0C1Rgufvft5ML1DhSWyhxAZ5
w6m4jEG/U/MdRqlhSR/4MYGwWNKhLtwNxQ+e99W2n9V35jPcDV4TNfVNFbCLFr4t
YrRP3DKhyxEejk8dX7Db4jW3AJBaYXtKCAqEOCgLmjr8JZ3HIgpS8rQtSVQ3uPq6
fOAbB0PABWSnQJBwGAPhr12fheli1lFE9yIbyt6n+hpUUYNPJUK1bJwrxNQB3fPY
QSdVGfa30LGZzKFnXvWUFs0D7oxmYw==
=VoXU
-----END PGP SIGNATURE-----

--eyd3iugdqq4tttdt--