Date: Fri, 25 Aug 2023 14:52:57 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: "Radu Pirea (NXP OSS)" <radu-nicolae.pirea@....nxp.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, richardcochran@...il.com,
	sebastian.tobuschat@....com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC net-next v2 3/5] net: phy: nxp-c45-tja11xx add MACsec
 support

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

Another nit: for consistency, it would be nice to stick to either
"tx_sa" or "txsa" (same for rxsa and rxsc) in function names.

2023-08-24, 12:16:13 +0300, Radu Pirea (NXP OSS) wrote:
> +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)?

I see that no caller of nxp_c45_macsec_read actually checks the return
value, so maybe those errors don't matter.


[...]
> +void nxp_c45_macsec_config_init(struct phy_device *phydev)
> +{
> +	if (!phydev->macsec_ops)
> +		return;
> +
> +	phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PORT_FUNC_ENABLES,
> +			 MACSEC_EN | ADAPTER_EN);

The calls to phy_set_bits_mmd() in nxp_c45_config_intr() have error
handling. Does this need error handling as well?

[...]
> +static bool nxp_c45_port_valid(struct nxp_c45_secy *phy_secy, u16 port)
> +{
> +	if (phy_secy->secy->tx_sc.end_station &&
> +	    __be16_to_cpu((__force __be16)port) != 1)
> +		return false;
> +
> +	return true;
> +}
> +
> +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);
u16 port = (u16)sci;

And then drop the __be16_to_cpu conversion from nxp_c45_port_valid

> +
> +	if (phy_secy->point_to_point && phy_secy->secy_id != 0)
> +		return false;
> +
> +	return nxp_c45_port_valid(phy_secy, port);
> +}
> +
> +static bool nxp_c45_secy_cfg_valid(struct nxp_c45_secy *phy_secy, bool can_ptp)
> +{
> +	u16 port =  (__force u64)phy_secy->secy->sci >> (ETH_ALEN * 8);

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

> +	if (phy_secy->secy->tx_sc.scb)
> +		return false;

[...]
> +static int nxp_c45_update_tx_sc_secy_cfg(struct phy_device *phydev,
> +					 struct nxp_c45_secy *phy_secy)
> +{
[...]
> +	phydev_dbg(phydev, "scb %s\n",
> +		   phy_secy->secy->tx_sc.scb ? "on" : "off");
> +	if (phy_secy->secy->tx_sc.scb)
> +		cfg |= MACSEC_TXSC_CFG_SCI;
> +	else
> +		cfg &= ~MACSEC_TXSC_CFG_SCI;

Should that be called MACSEC_TXSC_CFG_SCB? I had to check that it
wasn't using the wrong constant, using "SCI" for "SCB" (when SCI is
already a well-defined thing in macsec) confused me.

> +
> +	nxp_c45_macsec_write(phydev, MACSEC_TXSC_CFG, cfg);
> +
> +	return 0;
> +}
> +
[...]
> +static int nxp_c45_set_rxsa_key(struct macsec_context *ctx, bool key_a)
> +{
> +	u32 *salt = (u32 *)ctx->sa.rx_sa->key.salt.bytes;
> +	const struct nxp_c45_macsec_sa_regs *sa_regs;
> +	u32 ssci = (__force u32)ctx->sa.rx_sa->ssci;
> +	u32 key_size = ctx->secy->key_len / 4;
> +	u32 salt_size = MACSEC_SALT_LEN / 4;
> +	u32 *key = (u32 *)ctx->sa.key;
> +	u32 reg;
> +	int i;
> +
> +	sa_regs = nxp_c45_get_macsec_sa_regs(key_a);
> +
> +	for (i = 0; i < key_size; i++) {
> +		reg = sa_regs->rxsa_ka + i * 4;
> +		nxp_c45_macsec_write(ctx->phydev, reg,
> +				     (__force u32)cpu_to_be32(key[i]));
> +	}
> +
> +	if (ctx->secy->xpn) {
> +		for (i = 0; i < salt_size; i++) {
> +			reg = sa_regs->rxsa_salt + (2 - i) * 4;
> +			nxp_c45_macsec_write(ctx->phydev, reg,
> +					     (__force u32)cpu_to_be32(salt[i]));
> +		}
> +		nxp_c45_macsec_write(ctx->phydev, sa_regs->rxsa_ssci,
> +				     (__force u32)cpu_to_be32(ssci));
> +	}

This looks basically identical to nxp_c45_txsa_set_key except for the
registers it writes to. It could be turned into 2 or 3 small helpers
(one for the key, then salt and ssci).

> +
> +	nxp_c45_set_rxsa_key_cfg(ctx, key_a, false);
> +
> +	return 0;
> +}

[...]
> +static int nxp_c45_mdo_add_secy(struct macsec_context *ctx)
> +{
> +	struct phy_device *phydev = ctx->phydev;
> +	struct nxp_c45_phy *priv = phydev->priv;
> +	u64 sci = (__force u64)ctx->secy->sci;
> +	struct nxp_c45_secy *phy_secy;
> +	bool can_ptp;
> +	int idx;
> +	u32 reg;
> +
> +	phydev_dbg(ctx->phydev, "add secy SCI %llu\n", ctx->secy->sci);

nit: %016llx feels more natural for SCIs since they can be broken down
into address+port.

And since it's stored in network byte order, you'll want to convert it
via be64_to_cpu before you print it out.

I'd suggest doing that directly into the sci variable:

    u64 sci = be64_to_cpu((__force __be64)ctx->secy->sci);

and then adapt the uses of sci further down.

Feel free to move the sci_to_cpu function from
drivers/net/netdevsim/macsec.c to include/net/macsec.h and reuse it.


[...]
> +static int nxp_c45_mdo_upd_secy(struct macsec_context *ctx)
> +{
[...]
> +	if (phy_secy->enabled_an != ctx->secy->tx_sc.encoding_sa) {
> +		old_tx_sa = phy_secy->tx_sa[phy_secy->enabled_an];
> +		phy_secy->enabled_an = ctx->secy->tx_sc.encoding_sa;
> +		new_tx_sa = phy_secy->tx_sa[phy_secy->enabled_an];
> +		if (!new_tx_sa) {
> +			nxp_c45_tx_sa_disable(phydev, phy_secy);
> +			goto disable_old_tx_sa;
> +		}
> +
> +		if (!new_tx_sa->tx_sa->active) {
> +			nxp_c45_tx_sa_disable(phydev, phy_secy);
> +			goto disable_old_tx_sa;
> +		}

You can combine those two conditions into

		if (!new_tx_sa || !new_tx_sa->tx_sa->active) {
			nxp_c45_tx_sa_disable(phydev, phy_secy);
			goto disable_old_tx_sa;
		}

> +
> +		new_tx_sa->is_key_a = phy_secy->tx_sa_key_a;
> +		phy_secy->tx_sa_key_a = phy_secy->tx_sa_key_a;

Is this missing a ! on the right side?

Maybe worth creating a "next_sa_key_id" helper (or something like
that) that returns the current value and updates tx_sa_key_a, since
you use this pattern a few times.


[...]
> +static int nxp_c45_mdo_add_rxsc(struct macsec_context *ctx)
> +{
> +	struct phy_device *phydev = ctx->phydev;
> +	struct nxp_c45_phy *priv = phydev->priv;
> +	struct nxp_c45_secy *phy_secy;
> +	struct nxp_c45_rx_sc *rx_sc;
> +
> +	phydev_dbg(phydev, "add RX SC %s\n",
> +		   ctx->rx_sc->active ? "enabled" : "disabled");

If the HW/driver supports multiple TXSC/RXSC on the same device, it
would probably be helpful to add their SCIs to this debug message (and
the update/delete ones, also for the mdo_*_rxsa and mdo_*_txsa
functions).

[...]
> +static int nxp_c45_mdo_add_rxsa(struct macsec_context *ctx)
> +{
[...]
> +	if (!rx_sc->rx_sa_b) {
> +		phydev_dbg(phydev, "add RX SA B %u %s\n",
> +			   an, rx_sa->active ? "enabled" : "disabled");
> +		nxp_c45_set_rxsa_key(ctx, false);
> +		rx_sc->rx_sa_b = rx_sa;
> +		return 0;
> +	}
> +
> +	return -ENOMEM;

maybe -ENOSPC would fit better?

> +}
> +

[...]
> +static int nxp_c45_mdo_add_txsa(struct macsec_context *ctx)
> +{
[...]
> +	if (phy_secy->tx_sa[sa])
> +		return -EBUSY;
> +
> +	tx_sa = kzalloc(sizeof(*tx_sa), GFP_KERNEL);

missing NULL check

[...]
> +static int nxp_c45_mdo_del_txsa(struct macsec_context *ctx)
> +{
[...]
> +
> +	phy_secy->tx_sa[sa] = NULL;
> +	kfree(tx_sa);

tx_sa contains the key, so this needs to be kfree_sensitive, or add a
memzero_explicit(tx_sa->key) before freeing. Or if possible, don't
copy the key at all into tx_sa.

similar changes in the mscc driver:
1b16b3fdf675 ("net: phy: mscc: macsec: clear encryption keys when freeing a flow")
0dc33c65835d ("net: phy: mscc: macsec: do not copy encryption keys")


[...]
> diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
> index 7ab080ff02df..5bf7caa4e63d 100644
> --- a/drivers/net/phy/nxp-c45-tja11xx.c
> +++ b/drivers/net/phy/nxp-c45-tja11xx.c
[...]
> @@ -1218,12 +1201,25 @@ static int nxp_c45_start_op(struct phy_device *phydev)
>  
>  static int nxp_c45_config_intr(struct phy_device *phydev)
>  {
> -	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> +	int ret;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
> +				       VEND1_PORT_FUNC_IRQ_EN, MACSEC_IRQS);
> +		if (ret)
> +			return ret;
> +
>  		return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1,
>  					VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);

Maybe a dumb question: should we be clearing the MACSEC_IRQS bits when
this 2nd call to phy_set_bits_mmd fails? (and same below, reset when
the 2nd clear fails)


> -	else
> -		return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> -					  VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
> +	}
> +
> +	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> +				 VEND1_PORT_FUNC_IRQ_EN, MACSEC_IRQS);
> +	if (ret)
> +		return ret;
> +
> +	return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> +				  VEND1_PHY_IRQ_EN, PHY_IRQ_LINK_EVENT);
>  }

[...]
> @@ -1666,6 +1666,20 @@ static int nxp_c45_probe(struct phy_device *phydev)
>  	}
>  
>  no_ptp_support:
> +	macsec_ability = !!(phy_abilities & MACSEC_ABILITY);
> +	if (!macsec_ability) {
> +		phydev_info(phydev, "the phy does not support MACsec\n");
> +		goto no_macsec_support;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_MACSEC)) {
> +		ret = nxp_c45_macsec_probe(phydev);

I don't know how this probing is handled so maybe another dumb
question: if that fails, are we going to leak resources allocated
earlier?  (devm_kzalloc for example)

> +		phydev_dbg(phydev, "MACsec support enabled.");
> +	} else {
> +		phydev_dbg(phydev, "MACsec support not enabled even if the phy supports it");
> +	}
> +
> +no_macsec_support:
>  
>  	return ret;
>  }

-- 
Sabrina

Powered by blists - more mailing lists