On Fri, Aug 25, 2023 at 02:52:57PM +0200, Sabrina Dubroca wrote:

[Some of the questions I'm asking are probably dumb since I don't know
anything about phy drivers. Sorry if that's the case.]

General code organization nit: I think it would be easier to review
the code if helpers functions were grouped by the type of object they
work on. All the RXSA-related functions together, all the TXSA
functions together, same for RXSC and then TXSC/SecY. Right now I see
some RXSA functions in a group of TXSA functions, another in the
middle of a group of RXSC functions. It makes navigating through the
code a bit less convenient.

For networking, and Linux in general, forward declarations are not
liked. Functions should appear before they are used. That places a bit
of restrictions on ordering, but in general you can still group code
in meaningful ways.

2023-08-24, 12:16:13 +0300, Radu Pirea (NXP OSS) wrote:

quoted

+static int nxp_c45_macsec_write(struct phy_device *phydev, u16 reg, u32 val)
+{
+	WARN_ON_ONCE(reg % 4);
+
+	reg = reg / 2;
+	phy_write_mmd(phydev, MDIO_MMD_VEND2,
+		      VEND1_MACSEC_BASE + reg, val);
+	phy_write_mmd(phydev, MDIO_MMD_VEND2,
+		      VEND1_MACSEC_BASE + reg + 1, val >> 16);

Can these calls fail? ie, do you need to handle errors like in
nxp_c45_macsec_read (and then in callers of nxp_c45_macsec_write)?

Access to PHY devices can fail, but if it does, such failures are
generally fatal and there is no real recovery, also the next read/
write is also likely to fail. So we do recommend checking return codes
and just return the error up the stack. That failure might get trapped
up the stack, and turned into a phy_error() call which will disable
the PHY.

quoted

+static bool nxp_c45_rx_sc_valid(struct nxp_c45_secy *phy_secy,
+				struct macsec_rx_sc *rx_sc)
+{
+	u16 port =  (__force u64)rx_sc->sci >> (ETH_ALEN * 8);

u64 sci = be64_to_cpu((__force __be64)rx_sc->sci);

why is the __force needed? What happens with a normal cast?

    Andrew