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

On Wed, Oct 20, 2021 at 12:32:28PM +0300, Volodymyr Mytnyk wrote:

quoted

From: Volodymyr Mytnyk <redacted>

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.

quoted

+/* TODO: add another parameters here: modes, etc... */
+struct prestera_port_phy_config {
+     bool admin;
+     u32 mode;
+     u8 mdix;
+};

quoted

@@ -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" */

quoted

+             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.

Those TODOs are not valid anymore, will just remove the comments. No merge
is needed :) Adding new members to the end will be fine for the firmware
& driver.

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

Most of the testing is done on 64 bit system. On 32 should be fine also, as
the structure is packed & aligned. Anyway, I think to add build checking is
good idea to make sure the size is fine on both systems. Thanks.

  Volodymyr