Re: [PATCH net-next v3] net: marvell: prestera: add firmware v4.0 support

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Oct 20, 2021 at 12:32:28PM +0300, Volodymyr Mytnyk wrote:
> From: Volodymyr Mytnyk <vmytnyk@marvell.com>
> 
> Add firmware (FW) version 4.0 support for Marvell Prestera
> driver.
> 
> Major changes have been made to new v4.0 FW ABI to add support
> of new features, introduce the stability of the FW ABI and ensure
> better forward compatibility for the future driver vesrions.
> 
> Current v4.0 FW feature set support does not expect any changes
> to ABI, as it was defined and tested through long period of time.
> The ABI may be extended in case of new features, but it will not
> break the backward compatibility.
> 
> ABI major changes done in v4.0:
> - L1 ABI, where MAC and PHY API configuration are split.
> - ACL has been split to low-level TCAM and Counters ABI
>   to provide more HW ACL capabilities for future driver
>   versions.
> 
> To support backward support, the addition compatibility layer is
> required in the driver which will have two different codebase under
> "if FW-VER elif FW-VER else" conditions that will be removed
> in the future anyway, So, the idea was to break backward support
> and focus on more stable FW instead of supporting old version
> with very minimal and limited set of features/capabilities.
 
> +/* TODO: add another parameters here: modes, etc... */
> +struct prestera_port_phy_config {
> +	bool admin;
> +	u32 mode;
> +	u8 mdix;
> +};

> @@ -242,10 +246,44 @@ union prestera_msg_port_param {
>  	u8  duplex;
>  	u8  fec;
>  	u8  fc;
> -	struct prestera_msg_port_mdix_param mdix;
> -	struct prestera_msg_port_autoneg_param autoneg;
> +
> +	union {
> +		struct {
> +			/* TODO: merge it with "mode" */

> +		struct {
> +			/* TODO: merge it with "mode" */
> +			u8 admin:1;
> +			u8 adv_enable;
> +			u64 modes;
> +			/* TODO: merge it with modes */
> +			u32 mode;
> +			u8 mdix;
> +		} phy;

You claim this is stable, yet there are four TODOs. Please could you
convince us you can actually do these TODO without breaking the
ABI. Can you add more members to the end of these structures, and the
firmware/driver can know they are there? Since these are often unions,
you might not be able to tell from the length of the message
exchanged.

As Jakub pointed out, your structures have horrible alignment. Have
you run this on both 32 and 64 bit systems? It would be good to add

BUILD_BUG_ON(sizeof(*foo) != 42)

for all the structures that get passed to/from the firmware.

    Andrew