[PATCH net-next v2 0/9] net: macsec: initial support for hardware offloading

[PATCH net-next v2 0/9] net: macsec: initial support for hardware offloading

[Antoine Tenart]

Hello,

This series intends to add support for offloading MACsec transformations
in hardware enabled devices. The series is divided in two parts: the
first 6 patches add the infrastructure support to offload a MACsec
configuration to hardware drivers; and the last 3 patches introduce the
MACsec offloading support in the Microsemi Ocelot networking PHY, making
it the first driver to support the MACsec hardware offloading feature.

The series can also be found at:
https://github.com/atenart/linux/tree/net-next/macsec

MACsec hardware offloading infrastructure
-----------------------------------------

Linux has a software implementation of the MACsec standard and so far no
hardware offloading feature was developed and submitted. Some hardware
engines can perform MACsec operations, such as the Intel ixgbe NIC and
the Microsemi Ocelot PHY (the one we use in this series). This means the
MACsec offloading infrastructure should support networking PHY and
Ethernet drivers. A preliminary email[1] was sent about this.

The main idea here is to re-use the logic and data structures of the
software MACsec implementation. This allows not to duplicate definitions
and structure storing the same kind of information. It also allows to
use a unified genlink interface for both MACsec implementations (so that
the same userspace tool, `ip macsec`, is used with the same arguments).
The MACsec offloading support cannot be disabled if an interface
supports it at the moment, but this could be implemented later on if
this is a need (we could think of something like
`ip macsec set macsec0 offloading off`).

Because we do reuse the software implementation logic and because the
choice was made to expose the exact same interface to the user, a
virtual interface is created exactly as if the MACsec software
implementation was used. This was a big question when doing this work,
and another approach would have been to register the genl helpers for
all MACsec implementations and to have the software one a provider (such
as the h/w offloading device drivers are). This would mean there would
be no way to switch between implementations in the future at runtime.
I'm open to discuss this point as I think this is really important and
I'm not sure what is the best solution here.

The MACsec configuration is passed to device drivers supporting it
through MACsec ops which are called (indirectly) from the MACsec
genl helpers. This function calls the MACsec ops of PHY and Ethernet
drivers in two steps: a preparation one, and a commit one. The first
step is allowed to fail and should be used to check if a provided
configuration is compatible with the features provided by a MACsec
engine, while the second step is not allowed to fail and should only be
used to enable a given MACsec configuration. Two extra calls are made:
when a virtual MACsec interface is created and when it is deleted, so
that the hardware driver can stay in sync.

The Rx and TX handlers are modified to take in account the special case
were the MACsec transformation happens in the hardware, whether in a PHY
or in a MAC, as the packets seen by the networking stack on both the
physical and MACsec virtual interface are exactly the same. This leads
to some limitations: the hardware and software implementations can't be
used on the same physical interface, as the policies would be impossible
to fulfill (such as strict validation of the frames). Also only a single
virtual MACsec interface can be attached to a physical port supporting
hardware offloading as it would be impossible to guess onto which
interface a given packet should go (for ingress traffic).

Another limitation as of now is that the counters and statistics are not
reported back from the hardware to the software MACsec implementation.
This isn't an issue when using offloaded MACsec transformations, but it
should be added in the future so that the MACsec state can be reported
to the user (which would also improve the debug).

[1] https://www.spinics.net/lists/netdev/msg513047.html

Microsemi Ocelot PHY MACsec support
-----------------------------------

In order to add support for the MACsec offloading feature in the
Microsemi Ocelot driver, the __phy_read_page and __phy_write_page
helpers had to be exported. This is because the initialization of the
PHY is done while holding the MDIO bus lock, and we need to change the
page to configure the MACsec block.

The support itself is then added in two patches. The first one adds
support for configuring the MACsec block within the PHY, so that it is
up, running and available for future configuration, but is not doing any
modification on the traffic passing through the PHY. The second patch
implements the phy_device MACsec ops in the Microsemi Ocelot PHY driver,
and introduce helpers to configure MACsec transformations and flows to
match specific packets.

Thanks!
Antoine

Since v1:
  - Reworked the MACsec offloading API, moving from a single helper
    called for all MACsec configuration operations, to a per-operation
    function that is provided by the underlying hardware drivers.
  - Those functions now contain a verb to describe the configuration
    action they're offloading.
  - Improved the error handling in the MACsec genl helpers to revert
    the configuration to its previous state when the offloading call
    failed.
  - Reworked the file inclusions.

Antoine Tenart (9):
  net: introduce the MACSEC netdev feature
  net: macsec: move some definitions in a dedicated header
  net: macsec: introduce the macsec_context structure
  net: introduce MACsec ops and add a reference in net_device
  net: phy: add MACsec ops in phy_device
  net: macsec: hardware offloading infrastructure
  net: phy: export __phy_read_page/__phy_write_page
  net: phy: mscc: macsec initialization
  net: phy: mscc: macsec support

 drivers/net/macsec.c             |  542 ++++++++++------
 drivers/net/phy/Kconfig          |    2 +
 drivers/net/phy/mscc.c           | 1024 ++++++++++++++++++++++++++++++
 drivers/net/phy/mscc_fc_buffer.h |   64 ++
 drivers/net/phy/mscc_mac.h       |  159 +++++
 drivers/net/phy/mscc_macsec.h    |  258 ++++++++
 drivers/net/phy/phy-core.c       |    6 +-
 include/linux/netdev_features.h  |    3 +
 include/linux/netdevice.h        |   31 +
 include/linux/phy.h              |   13 +
 include/net/macsec.h             |  203 ++++++
 include/uapi/linux/if_macsec.h   |    3 +-
 net/core/ethtool.c               |    1 +
 13 files changed, 2125 insertions(+), 184 deletions(-)
 create mode 100644 drivers/net/phy/mscc_fc_buffer.h
 create mode 100644 drivers/net/phy/mscc_mac.h
 create mode 100644 drivers/net/phy/mscc_macsec.h
 create mode 100644 include/net/macsec.h

-- 
2.21.0

[PATCH net-next v2 1/9] net: introduce the MACSEC netdev feature

[Antoine Tenart]

This patch introduce a new netdev feature, which will be used by drivers
to state they can perform MACsec transformations in hardware.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 include/linux/netdev_features.h | 3 +++
 net/core/ethtool.c              | 1 +
 2 files changed, 4 insertions(+)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 4b19c544c59a..020f8542b92f 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -81,6 +81,8 @@ enum {
 	NETIF_F_GRO_HW_BIT,		/* Hardware Generic receive offload */
 	NETIF_F_HW_TLS_RECORD_BIT,	/* Offload TLS record */
 
+	NETIF_F_HW_MACSEC_BIT,		/* Offload MACsec operations */
+
 	/*
 	 * Add your fresh new feature above and remember to update
 	 * netdev_features_strings[] in net/core/ethtool.c and maybe
@@ -150,6 +152,7 @@ enum {
 #define NETIF_F_GSO_UDP_L4	__NETIF_F(GSO_UDP_L4)
 #define NETIF_F_HW_TLS_TX	__NETIF_F(HW_TLS_TX)
 #define NETIF_F_HW_TLS_RX	__NETIF_F(HW_TLS_RX)
+#define NETIF_F_HW_MACSEC	__NETIF_F(HW_MACSEC)
 
 /* Finds the next feature with the highest number of the range of start till 0.
  */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 6288e69e94fc..4a410e0ff179 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -111,6 +111,7 @@ static const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN]
 	[NETIF_F_HW_TLS_RECORD_BIT] =	"tls-hw-record",
 	[NETIF_F_HW_TLS_TX_BIT] =	 "tls-hw-tx-offload",
 	[NETIF_F_HW_TLS_RX_BIT] =	 "tls-hw-rx-offload",
+	[NETIF_F_HW_MACSEC_BIT] =	 "macsec-hw-offload",
 };
 
 static const char
-- 
2.21.0

[PATCH net-next v2 2/9] net: macsec: move some definitions in a dedicated header

[Antoine Tenart]

This patch moves some structure, type and identifier definitions into a
MACsec specific header. This patch does not modify how the MACsec code
is running and only move things around. This is a preparation for the
future MACsec hardware offloading support, which will re-use those
definitions outside macsec.c.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/macsec.c           | 163 ------------------------------
 include/net/macsec.h           | 179 +++++++++++++++++++++++++++++++++
 include/uapi/linux/if_macsec.h |   3 +-
 3 files changed, 180 insertions(+), 165 deletions(-)
 create mode 100644 include/net/macsec.h

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 8f46aa1ddec0..3815cb6e9bf2 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -19,8 +19,6 @@
 
 #include <uapi/linux/if_macsec.h>
 
-typedef u64 __bitwise sci_t;
-
 #define MACSEC_SCI_LEN 8
 
 /* SecTAG length = macsec_eth_header without the optional SCI */
@@ -58,8 +56,6 @@ struct macsec_eth_header {
 #define GCM_AES_IV_LEN 12
 #define DEFAULT_ICV_LEN 16
 
-#define MACSEC_NUM_AN 4 /* 2 bits for the association number */
-
 #define for_each_rxsc(secy, sc)				\
 	for (sc = rcu_dereference_bh(secy->rx_sc);	\
 	     sc;					\
@@ -77,49 +73,6 @@ struct gcm_iv {
 	__be32 pn;
 };
 
-/**
- * struct macsec_key - SA key
- * @id: user-provided key identifier
- * @tfm: crypto struct, key storage
- */
-struct macsec_key {
-	u8 id[MACSEC_KEYID_LEN];
-	struct crypto_aead *tfm;
-};
-
-struct macsec_rx_sc_stats {
-	__u64 InOctetsValidated;
-	__u64 InOctetsDecrypted;
-	__u64 InPktsUnchecked;
-	__u64 InPktsDelayed;
-	__u64 InPktsOK;
-	__u64 InPktsInvalid;
-	__u64 InPktsLate;
-	__u64 InPktsNotValid;
-	__u64 InPktsNotUsingSA;
-	__u64 InPktsUnusedSA;
-};
-
-struct macsec_rx_sa_stats {
-	__u32 InPktsOK;
-	__u32 InPktsInvalid;
-	__u32 InPktsNotValid;
-	__u32 InPktsNotUsingSA;
-	__u32 InPktsUnusedSA;
-};
-
-struct macsec_tx_sa_stats {
-	__u32 OutPktsProtected;
-	__u32 OutPktsEncrypted;
-};
-
-struct macsec_tx_sc_stats {
-	__u64 OutPktsProtected;
-	__u64 OutPktsEncrypted;
-	__u64 OutOctetsProtected;
-	__u64 OutOctetsEncrypted;
-};
-
 struct macsec_dev_stats {
 	__u64 OutPktsUntagged;
 	__u64 InPktsUntagged;
@@ -131,124 +84,8 @@ struct macsec_dev_stats {
 	__u64 InPktsOverrun;
 };
 
-/**
- * struct macsec_rx_sa - receive secure association
- * @active:
- * @next_pn: packet number expected for the next packet
- * @lock: protects next_pn manipulations
- * @key: key structure
- * @stats: per-SA stats
- */
-struct macsec_rx_sa {
-	struct macsec_key key;
-	spinlock_t lock;
-	u32 next_pn;
-	refcount_t refcnt;
-	bool active;
-	struct macsec_rx_sa_stats __percpu *stats;
-	struct macsec_rx_sc *sc;
-	struct rcu_head rcu;
-};
-
-struct pcpu_rx_sc_stats {
-	struct macsec_rx_sc_stats stats;
-	struct u64_stats_sync syncp;
-};
-
-/**
- * struct macsec_rx_sc - receive secure channel
- * @sci: secure channel identifier for this SC
- * @active: channel is active
- * @sa: array of secure associations
- * @stats: per-SC stats
- */
-struct macsec_rx_sc {
-	struct macsec_rx_sc __rcu *next;
-	sci_t sci;
-	bool active;
-	struct macsec_rx_sa __rcu *sa[MACSEC_NUM_AN];
-	struct pcpu_rx_sc_stats __percpu *stats;
-	refcount_t refcnt;
-	struct rcu_head rcu_head;
-};
-
-/**
- * struct macsec_tx_sa - transmit secure association
- * @active:
- * @next_pn: packet number to use for the next packet
- * @lock: protects next_pn manipulations
- * @key: key structure
- * @stats: per-SA stats
- */
-struct macsec_tx_sa {
-	struct macsec_key key;
-	spinlock_t lock;
-	u32 next_pn;
-	refcount_t refcnt;
-	bool active;
-	struct macsec_tx_sa_stats __percpu *stats;
-	struct rcu_head rcu;
-};
-
-struct pcpu_tx_sc_stats {
-	struct macsec_tx_sc_stats stats;
-	struct u64_stats_sync syncp;
-};
-
-/**
- * struct macsec_tx_sc - transmit secure channel
- * @active:
- * @encoding_sa: association number of the SA currently in use
- * @encrypt: encrypt packets on transmit, or authenticate only
- * @send_sci: always include the SCI in the SecTAG
- * @end_station:
- * @scb: single copy broadcast flag
- * @sa: array of secure associations
- * @stats: stats for this TXSC
- */
-struct macsec_tx_sc {
-	bool active;
-	u8 encoding_sa;
-	bool encrypt;
-	bool send_sci;
-	bool end_station;
-	bool scb;
-	struct macsec_tx_sa __rcu *sa[MACSEC_NUM_AN];
-	struct pcpu_tx_sc_stats __percpu *stats;
-};
-
 #define MACSEC_VALIDATE_DEFAULT MACSEC_VALIDATE_STRICT
 
-/**
- * struct macsec_secy - MACsec Security Entity
- * @netdev: netdevice for this SecY
- * @n_rx_sc: number of receive secure channels configured on this SecY
- * @sci: secure channel identifier used for tx
- * @key_len: length of keys used by the cipher suite
- * @icv_len: length of ICV used by the cipher suite
- * @validate_frames: validation mode
- * @operational: MAC_Operational flag
- * @protect_frames: enable protection for this SecY
- * @replay_protect: enable packet number checks on receive
- * @replay_window: size of the replay window
- * @tx_sc: transmit secure channel
- * @rx_sc: linked list of receive secure channels
- */
-struct macsec_secy {
-	struct net_device *netdev;
-	unsigned int n_rx_sc;
-	sci_t sci;
-	u16 key_len;
-	u16 icv_len;
-	enum macsec_validation_type validate_frames;
-	bool operational;
-	bool protect_frames;
-	bool replay_protect;
-	u32 replay_window;
-	struct macsec_tx_sc tx_sc;
-	struct macsec_rx_sc __rcu *rx_sc;
-};
-
 struct pcpu_secy_stats {
 	struct macsec_dev_stats stats;
 	struct u64_stats_sync syncp;
diff --git a/include/net/macsec.h b/include/net/macsec.h
new file mode 100644
index 000000000000..5db18a272ffd
--- /dev/null
+++ b/include/net/macsec.h
@@ -0,0 +1,179 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * MACsec netdev header, used for h/w accelerated implementations.
+ *
+ * Copyright (c) 2015 Sabrina Dubroca <sd@queasysnail.net>
+ */
+#ifndef _NET_MACSEC_H_
+#define _NET_MACSEC_H_
+
+#include <linux/u64_stats_sync.h>
+#include <uapi/linux/if_link.h>
+#include <uapi/linux/if_macsec.h>
+
+typedef u64 __bitwise sci_t;
+
+#define MACSEC_NUM_AN 4 /* 2 bits for the association number */
+#define MACSEC_KEYID_LEN 16
+
+/**
+ * struct macsec_key - SA key
+ * @id: user-provided key identifier
+ * @tfm: crypto struct, key storage
+ */
+struct macsec_key {
+	u8 id[MACSEC_KEYID_LEN];
+	struct crypto_aead *tfm;
+};
+
+struct macsec_rx_sc_stats {
+	__u64 InOctetsValidated;
+	__u64 InOctetsDecrypted;
+	__u64 InPktsUnchecked;
+	__u64 InPktsDelayed;
+	__u64 InPktsOK;
+	__u64 InPktsInvalid;
+	__u64 InPktsLate;
+	__u64 InPktsNotValid;
+	__u64 InPktsNotUsingSA;
+	__u64 InPktsUnusedSA;
+};
+
+struct macsec_rx_sa_stats {
+	__u32 InPktsOK;
+	__u32 InPktsInvalid;
+	__u32 InPktsNotValid;
+	__u32 InPktsNotUsingSA;
+	__u32 InPktsUnusedSA;
+};
+
+struct macsec_tx_sa_stats {
+	__u32 OutPktsProtected;
+	__u32 OutPktsEncrypted;
+};
+
+struct macsec_tx_sc_stats {
+	__u64 OutPktsProtected;
+	__u64 OutPktsEncrypted;
+	__u64 OutOctetsProtected;
+	__u64 OutOctetsEncrypted;
+};
+
+/**
+ * struct macsec_rx_sa - receive secure association
+ * @active:
+ * @next_pn: packet number expected for the next packet
+ * @lock: protects next_pn manipulations
+ * @key: key structure
+ * @stats: per-SA stats
+ */
+struct macsec_rx_sa {
+	struct macsec_key key;
+	spinlock_t lock;
+	u32 next_pn;
+	refcount_t refcnt;
+	bool active;
+	struct macsec_rx_sa_stats __percpu *stats;
+	struct macsec_rx_sc *sc;
+	struct rcu_head rcu;
+};
+
+struct pcpu_rx_sc_stats {
+	struct macsec_rx_sc_stats stats;
+	struct u64_stats_sync syncp;
+};
+
+struct pcpu_tx_sc_stats {
+	struct macsec_tx_sc_stats stats;
+	struct u64_stats_sync syncp;
+};
+
+/**
+ * struct macsec_rx_sc - receive secure channel
+ * @sci: secure channel identifier for this SC
+ * @active: channel is active
+ * @sa: array of secure associations
+ * @stats: per-SC stats
+ */
+struct macsec_rx_sc {
+	struct macsec_rx_sc __rcu *next;
+	sci_t sci;
+	bool active;
+	struct macsec_rx_sa __rcu *sa[MACSEC_NUM_AN];
+	struct pcpu_rx_sc_stats __percpu *stats;
+	refcount_t refcnt;
+	struct rcu_head rcu_head;
+};
+
+/**
+ * struct macsec_tx_sa - transmit secure association
+ * @active:
+ * @next_pn: packet number to use for the next packet
+ * @lock: protects next_pn manipulations
+ * @key: key structure
+ * @stats: per-SA stats
+ */
+struct macsec_tx_sa {
+	struct macsec_key key;
+	spinlock_t lock;
+	u32 next_pn;
+	refcount_t refcnt;
+	bool active;
+	bool offloaded;
+	struct macsec_tx_sa_stats __percpu *stats;
+	struct rcu_head rcu;
+};
+
+/**
+ * struct macsec_tx_sc - transmit secure channel
+ * @active:
+ * @encoding_sa: association number of the SA currently in use
+ * @encrypt: encrypt packets on transmit, or authenticate only
+ * @send_sci: always include the SCI in the SecTAG
+ * @end_station:
+ * @scb: single copy broadcast flag
+ * @sa: array of secure associations
+ * @stats: stats for this TXSC
+ */
+struct macsec_tx_sc {
+	bool active;
+	u8 encoding_sa;
+	bool encrypt;
+	bool send_sci;
+	bool end_station;
+	bool scb;
+	struct macsec_tx_sa __rcu *sa[MACSEC_NUM_AN];
+	struct pcpu_tx_sc_stats __percpu *stats;
+};
+
+/**
+ * struct macsec_secy - MACsec Security Entity
+ * @netdev: netdevice for this SecY
+ * @n_rx_sc: number of receive secure channels configured on this SecY
+ * @sci: secure channel identifier used for tx
+ * @key_len: length of keys used by the cipher suite
+ * @icv_len: length of ICV used by the cipher suite
+ * @validate_frames: validation mode
+ * @operational: MAC_Operational flag
+ * @protect_frames: enable protection for this SecY
+ * @replay_protect: enable packet number checks on receive
+ * @replay_window: size of the replay window
+ * @tx_sc: transmit secure channel
+ * @rx_sc: linked list of receive secure channels
+ */
+struct macsec_secy {
+	struct net_device *netdev;
+	unsigned int n_rx_sc;
+	sci_t sci;
+	u16 key_len;
+	u16 icv_len;
+	enum macsec_validation_type validate_frames;
+	bool operational;
+	bool protect_frames;
+	bool replay_protect;
+	u32 replay_window;
+	struct macsec_tx_sc tx_sc;
+	struct macsec_rx_sc __rcu *rx_sc;
+};
+
+#endif /* _NET_MACSEC_H_ */
diff --git a/include/uapi/linux/if_macsec.h b/include/uapi/linux/if_macsec.h
index 98e4d5d7c45c..573208cac210 100644
--- a/include/uapi/linux/if_macsec.h
+++ b/include/uapi/linux/if_macsec.h
@@ -14,14 +14,13 @@
 #define _UAPI_MACSEC_H
 
 #include <linux/types.h>
+#include <net/macsec.h>
 
 #define MACSEC_GENL_NAME "macsec"
 #define MACSEC_GENL_VERSION 1
 
 #define MACSEC_MAX_KEY_LEN 128
 
-#define MACSEC_KEYID_LEN 16
-
 /* cipher IDs as per IEEE802.1AEbn-2011 */
 #define MACSEC_CIPHER_ID_GCM_AES_128 0x0080C20001000001ULL
 #define MACSEC_CIPHER_ID_GCM_AES_256 0x0080C20001000002ULL
-- 
2.21.0

Re: [PATCH net-next v2 2/9] net: macsec: move some definitions in a dedicated header

[Igor Russkikh]

Hi Antoine,

Overall good looking patchset, great!

> +/**
> + * struct macsec_tx_sa - transmit secure association
> + * @active:
> + * @next_pn: packet number to use for the next packet
> + * @lock: protects next_pn manipulations
> + * @key: key structure
> + * @stats: per-SA stats
> + */
> +struct macsec_tx_sa {
> +	struct macsec_key key;
> +	spinlock_t lock;
> +	u32 next_pn;
> +	refcount_t refcnt;
> +	bool active;
> +	bool offloaded;

I don't see this `offloaded` field being used anywhere. Is this needed?

Regards,
  Igor

Re: [PATCH net-next v2 2/9] net: macsec: move some definitions in a dedicated header

[Antoine Tenart]

Hi Igor,

On Sat, Aug 10, 2019 at 12:19:36PM +0000, Igor Russkikh wrote:
> > +/**
> > + * struct macsec_tx_sa - transmit secure association
> > + * @active:
> > + * @next_pn: packet number to use for the next packet
> > + * @lock: protects next_pn manipulations
> > + * @key: key structure
> > + * @stats: per-SA stats
> > + */
> > +struct macsec_tx_sa {
> > +	struct macsec_key key;
> > +	spinlock_t lock;
> > +	u32 next_pn;
> > +	refcount_t refcnt;
> > +	bool active;
> > +	bool offloaded;
> 
> I don't see this `offloaded` field being used anywhere. Is this needed?

You're right it's not and was only used in previous versions of this
patchset. I'll remove it.

Thanks for spotting this!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next v2 3/9] net: macsec: introduce the macsec_context structure

[Antoine Tenart]

This patch introduces the macsec_context structure. It will be used
in the kernel to exchange information between the common MACsec
implementation (macsec.c) and the MACsec hardware offloading
implementations. This structure contains pointers to MACsec specific
structures which contain the actual MACsec configuration, and to the
underlying device (netdev or phydev).

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 include/net/macsec.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/net/macsec.h b/include/net/macsec.h
index 5db18a272ffd..1c82026dc17e 100644
--- a/include/net/macsec.h
+++ b/include/net/macsec.h
@@ -176,4 +176,28 @@ struct macsec_secy {
 	struct macsec_rx_sc __rcu *rx_sc;
 };
 
+/**
+ * struct macsec_context - MACsec context for hardware offloading
+ */
+struct macsec_context {
+	union {
+		struct net_device *netdev;
+		struct phy_device *phydev;
+	};
+
+	const struct macsec_secy *secy;
+	const struct macsec_rx_sc *rx_sc;
+	struct {
+		unsigned char assoc_num;
+		u8 key[MACSEC_KEYID_LEN];
+		union {
+			const struct macsec_rx_sa *rx_sa;
+			const struct macsec_tx_sa *tx_sa;
+		};
+	} sa;
+
+	u8 prepare:1;
+	u8 is_phy:1;
+};
+
 #endif /* _NET_MACSEC_H_ */
-- 
2.21.0

[PATCH net-next v2 4/9] net: introduce MACsec ops and add a reference in net_device

[Antoine Tenart]

This patch introduces MACsec ops for drivers to support offloading
MACsec operations. A reference to those ops is added in net_device.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 include/linux/netdevice.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 88292953aa6f..59ff123d62e3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -53,6 +53,7 @@ struct netpoll_info;
 struct device;
 struct phy_device;
 struct dsa_port;
+struct macsec_context;
 
 struct sfp_bus;
 /* 802.11 specific */
@@ -910,6 +911,29 @@ struct xfrmdev_ops {
 };
 #endif
 
+#if defined(CONFIG_MACSEC)
+struct macsec_ops {
+	/* Device wide */
+	int (*mdo_dev_open)(struct macsec_context *ctx);
+	int (*mdo_dev_stop)(struct macsec_context *ctx);
+	/* SecY */
+	int (*mdo_add_secy)(struct macsec_context *ctx);
+	int (*mdo_upd_secy)(struct macsec_context *ctx);
+	int (*mdo_del_secy)(struct macsec_context *ctx);
+	/* Security channels */
+	int (*mdo_add_rxsc)(struct macsec_context *ctx);
+	int (*mdo_upd_rxsc)(struct macsec_context *ctx);
+	int (*mdo_del_rxsc)(struct macsec_context *ctx);
+	/* Security associations */
+	int (*mdo_add_rxsa)(struct macsec_context *ctx);
+	int (*mdo_upd_rxsa)(struct macsec_context *ctx);
+	int (*mdo_del_rxsa)(struct macsec_context *ctx);
+	int (*mdo_add_txsa)(struct macsec_context *ctx);
+	int (*mdo_upd_txsa)(struct macsec_context *ctx);
+	int (*mdo_del_txsa)(struct macsec_context *ctx);
+};
+#endif
+
 struct dev_ifalias {
 	struct rcu_head rcuhead;
 	char ifalias[];
@@ -1755,6 +1779,8 @@ enum netdev_priv_flags {
  *
  *	@wol_enabled:	Wake-on-LAN is enabled
  *
+ *	@macsec_ops:    MACsec offloading ops
+ *
  *	FIXME: cleanup struct net_device such that network protocol info
  *	moves out.
  */
@@ -2036,6 +2062,11 @@ struct net_device {
 	struct lock_class_key	*qdisc_running_key;
 	bool			proto_down;
 	unsigned		wol_enabled:1;
+
+#if IS_ENABLED(CONFIG_MACSEC)
+	/* MACsec management functions */
+	const struct macsec_ops *macsec_ops;
+#endif
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
-- 
2.21.0

Re: [PATCH net-next v2 4/9] net: introduce MACsec ops and add a reference in net_device

[Jakub Kicinski]

On Thu,  8 Aug 2019 16:05:55 +0200, Antoine Tenart wrote:
> This patch introduces MACsec ops for drivers to support offloading
> MACsec operations. A reference to those ops is added in net_device.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  include/linux/netdevice.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 88292953aa6f..59ff123d62e3 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -53,6 +53,7 @@ struct netpoll_info;
>  struct device;
>  struct phy_device;
>  struct dsa_port;
> +struct macsec_context;
>  
>  struct sfp_bus;
>  /* 802.11 specific */
> @@ -910,6 +911,29 @@ struct xfrmdev_ops {
>  };
>  #endif
>  
> +#if defined(CONFIG_MACSEC)
> +struct macsec_ops {

I think it'd be cleaner to have macsec_ops declared in macsec.h
and forward declare macsec_ops rather than macsec_context.

> +	/* Device wide */
> +	int (*mdo_dev_open)(struct macsec_context *ctx);
> +	int (*mdo_dev_stop)(struct macsec_context *ctx);
> +	/* SecY */
> +	int (*mdo_add_secy)(struct macsec_context *ctx);
> +	int (*mdo_upd_secy)(struct macsec_context *ctx);
> +	int (*mdo_del_secy)(struct macsec_context *ctx);
> +	/* Security channels */
> +	int (*mdo_add_rxsc)(struct macsec_context *ctx);
> +	int (*mdo_upd_rxsc)(struct macsec_context *ctx);
> +	int (*mdo_del_rxsc)(struct macsec_context *ctx);
> +	/* Security associations */
> +	int (*mdo_add_rxsa)(struct macsec_context *ctx);
> +	int (*mdo_upd_rxsa)(struct macsec_context *ctx);
> +	int (*mdo_del_rxsa)(struct macsec_context *ctx);
> +	int (*mdo_add_txsa)(struct macsec_context *ctx);
> +	int (*mdo_upd_txsa)(struct macsec_context *ctx);
> +	int (*mdo_del_txsa)(struct macsec_context *ctx);
> +};
> +#endif
> +
>  struct dev_ifalias {
>  	struct rcu_head rcuhead;
>  	char ifalias[];
> @@ -1755,6 +1779,8 @@ enum netdev_priv_flags {
>   *
>   *	@wol_enabled:	Wake-on-LAN is enabled
>   *
> + *	@macsec_ops:    MACsec offloading ops
> + *
>   *	FIXME: cleanup struct net_device such that network protocol info
>   *	moves out.
>   */
> @@ -2036,6 +2062,11 @@ struct net_device {
>  	struct lock_class_key	*qdisc_running_key;
>  	bool			proto_down;
>  	unsigned		wol_enabled:1;
> +
> +#if IS_ENABLED(CONFIG_MACSEC)
> +	/* MACsec management functions */
> +	const struct macsec_ops *macsec_ops;
> +#endif
>  };
>  #define to_net_dev(d) container_of(d, struct net_device, dev)
>  

Re: [PATCH net-next v2 4/9] net: introduce MACsec ops and add a reference in net_device

[Antoine Tenart]

Hi Jakub,

On Fri, Aug 09, 2019 at 01:35:09PM -0700, Jakub Kicinski wrote:
> On Thu,  8 Aug 2019 16:05:55 +0200, Antoine Tenart wrote:
> >  
> > +#if defined(CONFIG_MACSEC)
> > +struct macsec_ops {
> 
> I think it'd be cleaner to have macsec_ops declared in macsec.h
> and forward declare macsec_ops rather than macsec_context.

That makes sense, I'll move this declaration in macsec.h

> > +	/* Device wide */
> > +	int (*mdo_dev_open)(struct macsec_context *ctx);
> > +	int (*mdo_dev_stop)(struct macsec_context *ctx);
> > +	/* SecY */
> > +	int (*mdo_add_secy)(struct macsec_context *ctx);
> > +	int (*mdo_upd_secy)(struct macsec_context *ctx);
> > +	int (*mdo_del_secy)(struct macsec_context *ctx);
> > +	/* Security channels */
> > +	int (*mdo_add_rxsc)(struct macsec_context *ctx);
> > +	int (*mdo_upd_rxsc)(struct macsec_context *ctx);
> > +	int (*mdo_del_rxsc)(struct macsec_context *ctx);
> > +	/* Security associations */
> > +	int (*mdo_add_rxsa)(struct macsec_context *ctx);
> > +	int (*mdo_upd_rxsa)(struct macsec_context *ctx);
> > +	int (*mdo_del_rxsa)(struct macsec_context *ctx);
> > +	int (*mdo_add_txsa)(struct macsec_context *ctx);
> > +	int (*mdo_upd_txsa)(struct macsec_context *ctx);
> > +	int (*mdo_del_txsa)(struct macsec_context *ctx);
> > +};
> > +#endif

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next v2 5/9] net: phy: add MACsec ops in phy_device

[Antoine Tenart]

This patch adds a reference to MACsec ops in the phy_device, to allow
PHYs to support offloading MACsec operations. The phydev lock will be
held while calling those helpers.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 include/linux/phy.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 462b90b73f93..6947a19587e4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -22,6 +22,10 @@
 #include <linux/workqueue.h>
 #include <linux/mod_devicetable.h>
 
+#ifdef CONFIG_MACSEC
+#include <net/macsec.h>
+#endif
+
 #include <linux/atomic.h>
 
 #define PHY_DEFAULT_FEATURES	(SUPPORTED_Autoneg | \
@@ -345,6 +349,7 @@ struct phy_c45_device_ids {
  * attached_dev: The attached enet driver's device instance ptr
  * adjust_link: Callback for the enet controller to respond to
  * changes in the link state.
+ * macsec_ops: MACsec offloading ops.
  *
  * speed, duplex, pause, supported, advertising, lp_advertising,
  * and autoneg are used like in mii_if_info
@@ -438,6 +443,11 @@ struct phy_device {
 
 	void (*phy_link_change)(struct phy_device *, bool up, bool do_carrier);
 	void (*adjust_link)(struct net_device *dev);
+
+#if defined(CONFIG_MACSEC)
+	/* MACsec management functions */
+	const struct macsec_ops *macsec_ops;
+#endif
 };
 #define to_phy_device(d) container_of(to_mdio_device(d), \
 				      struct phy_device, mdio)
-- 
2.21.0

Re: [PATCH net-next v2 5/9] net: phy: add MACsec ops in phy_device

[Florian Fainelli]

On 8/8/19 7:05 AM, Antoine Tenart wrote:
> This patch adds a reference to MACsec ops in the phy_device, to allow
> PHYs to support offloading MACsec operations. The phydev lock will be
> held while calling those helpers.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  include/linux/phy.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 462b90b73f93..6947a19587e4 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -22,6 +22,10 @@
>  #include <linux/workqueue.h>
>  #include <linux/mod_devicetable.h>
>  
> +#ifdef CONFIG_MACSEC
> +#include <net/macsec.h>
> +#endif

#if IS_ENABLED(CONFIG_MACSEC)

> +
>  #include <linux/atomic.h>
>  
>  #define PHY_DEFAULT_FEATURES	(SUPPORTED_Autoneg | \
> @@ -345,6 +349,7 @@ struct phy_c45_device_ids {
>   * attached_dev: The attached enet driver's device instance ptr
>   * adjust_link: Callback for the enet controller to respond to
>   * changes in the link state.
> + * macsec_ops: MACsec offloading ops.
>   *
>   * speed, duplex, pause, supported, advertising, lp_advertising,
>   * and autoneg are used like in mii_if_info
> @@ -438,6 +443,11 @@ struct phy_device {
>  
>  	void (*phy_link_change)(struct phy_device *, bool up, bool do_carrier);
>  	void (*adjust_link)(struct net_device *dev);
> +
> +#if defined(CONFIG_MACSEC)
> +	/* MACsec management functions */
> +	const struct macsec_ops *macsec_ops;
> +#endif

#if IS_ENABLED(CONFIG_MACSEC)

likewise.
-- 
Florian

Re: [PATCH net-next v2 5/9] net: phy: add MACsec ops in phy_device

[Antoine Tenart]

Hi Florian,

On Wed, Aug 14, 2019 at 04:15:03PM -0700, Florian Fainelli wrote:
> On 8/8/19 7:05 AM, Antoine Tenart wrote:
> > This patch adds a reference to MACsec ops in the phy_device, to allow
> > PHYs to support offloading MACsec operations. The phydev lock will be
> > held while calling those helpers.
> > 
> > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > ---
> >  include/linux/phy.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 462b90b73f93..6947a19587e4 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -22,6 +22,10 @@
> >  #include <linux/workqueue.h>
> >  #include <linux/mod_devicetable.h>
> >  
> > +#ifdef CONFIG_MACSEC
> > +#include <net/macsec.h>
> > +#endif
> 
> #if IS_ENABLED(CONFIG_MACSEC)
> 
> > +
> >  #include <linux/atomic.h>
> >  
> >  #define PHY_DEFAULT_FEATURES	(SUPPORTED_Autoneg | \
> > @@ -345,6 +349,7 @@ struct phy_c45_device_ids {
> >   * attached_dev: The attached enet driver's device instance ptr
> >   * adjust_link: Callback for the enet controller to respond to
> >   * changes in the link state.
> > + * macsec_ops: MACsec offloading ops.
> >   *
> >   * speed, duplex, pause, supported, advertising, lp_advertising,
> >   * and autoneg are used like in mii_if_info
> > @@ -438,6 +443,11 @@ struct phy_device {
> >  
> >  	void (*phy_link_change)(struct phy_device *, bool up, bool do_carrier);
> >  	void (*adjust_link)(struct net_device *dev);
> > +
> > +#if defined(CONFIG_MACSEC)
> > +	/* MACsec management functions */
> > +	const struct macsec_ops *macsec_ops;
> > +#endif
> 
> #if IS_ENABLED(CONFIG_MACSEC)
> 
> likewise.

I'll fix it.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Antoine Tenart]

This patch introduces the MACsec hardware offloading infrastructure.

The main idea here is to re-use the logic and data structures of the
software MACsec implementation. This allows not to duplicate definitions
and structure storing the same kind of information. It also allows to
use a unified genlink interface for both MACsec implementations (so that
the same userspace tool, `ip macsec`, is used with the same arguments).
The MACsec offloading support cannot be disabled if an interface
supports it at the moment.

The MACsec configuration is passed to device drivers supporting it
through macsec_hw_offload() which is called from the MACsec genl
helpers. This function calls the macsec ops of PHY and Ethernet
drivers in two steps: a preparation one, and a commit one. The first
step is allowed to fail and should be used to check if a provided
configuration is compatible with the features provided by a MACsec
engine, while the second step is not allowed to fail and should only be
used to enable a given MACsec configuration. Two extra calls are made:
when a virtual MACsec interface is created and when it is deleted, so
that the hardware driver can stay in sync.

The Rx and TX handlers are modified to take in account the special case
were the MACsec transformation happens in the hardware, whether in a PHY
or in a MAC, as the packets seen by the networking stack on both the
physical and MACsec virtual interface are exactly the same. This leads
to some limitations: the hardware and software implementations can't be
used on the same physical interface, as the policies would be impossible
to fulfill (such as strict validation of the frames). Also only a single
virtual MACsec interface can be attached to a physical port supporting
hardware offloading as it would be impossible to guess onto which
interface a given packet should go (for ingress traffic).

Another limitation as of now is that the counters and statistics are not
reported back from the hardware to the software MACsec implementation.
This isn't an issue when using offloaded MACsec transformations, but it
should be added in the future so that the MACsec state can be reported
to the user (which would also improve the debug).

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/macsec.c | 379 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 362 insertions(+), 17 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 3815cb6e9bf2..74f0e06a9fc2 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -11,11 +11,13 @@
 #include <linux/module.h>
 #include <crypto/aead.h>
 #include <linux/etherdevice.h>
+#include <linux/netdevice.h>
 #include <linux/rtnetlink.h>
 #include <linux/refcount.h>
 #include <net/genetlink.h>
 #include <net/sock.h>
 #include <net/gro_cells.h>
+#include <linux/phy.h>
 
 #include <uapi/linux/if_macsec.h>
 
@@ -318,6 +320,44 @@ static void macsec_set_shortlen(struct macsec_eth_header *h, size_t data_len)
 		h->short_length = data_len;
 }
 
+/* Checks if underlying layers implement MACsec offloading functions
+ * and returns a pointer to the MACsec ops struct if any (also updates
+ * the MACsec context device reference if provided).
+ */
+static const struct macsec_ops *macsec_get_ops(struct macsec_dev *dev,
+					       struct macsec_context *ctx)
+{
+	struct phy_device *phydev;
+
+	if (!dev || !dev->real_dev)
+		return NULL;
+
+	/* Check if the PHY device provides MACsec ops */
+	phydev = dev->real_dev->phydev;
+	if (phydev && phydev->macsec_ops) {
+		if (ctx) {
+			memset(ctx, 0, sizeof(*ctx));
+			ctx->phydev = phydev;
+			ctx->is_phy = 1;
+		}
+
+		return phydev->macsec_ops;
+	}
+
+	/* Check if the net device provides MACsec ops */
+	if (dev->real_dev->features & NETIF_F_HW_MACSEC &&
+	    dev->real_dev->macsec_ops) {
+		if (ctx) {
+			memset(ctx, 0, sizeof(*ctx));
+			ctx->netdev = dev->real_dev;
+		}
+
+		return dev->real_dev->macsec_ops;
+	}
+
+	return NULL;
+}
+
 /* validate MACsec packet according to IEEE 802.1AE-2006 9.12 */
 static bool macsec_validate_skb(struct sk_buff *skb, u16 icv_len)
 {
@@ -867,8 +907,10 @@ static struct macsec_rx_sc *find_rx_sc_rtnl(struct macsec_secy *secy, sci_t sci)
 	return NULL;
 }
 
-static void handle_not_macsec(struct sk_buff *skb)
+static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 {
+	/* Deliver to the uncontrolled port by default */
+	enum rx_handler_result ret = RX_HANDLER_PASS;
 	struct macsec_rxh_data *rxd;
 	struct macsec_dev *macsec;
 
@@ -883,7 +925,8 @@ static void handle_not_macsec(struct sk_buff *skb)
 		struct sk_buff *nskb;
 		struct pcpu_secy_stats *secy_stats = this_cpu_ptr(macsec->stats);
 
-		if (macsec->secy.validate_frames == MACSEC_VALIDATE_STRICT) {
+		if (!macsec_get_ops(macsec, NULL) &&
+		    macsec->secy.validate_frames == MACSEC_VALIDATE_STRICT) {
 			u64_stats_update_begin(&secy_stats->syncp);
 			secy_stats->stats.InPktsNoTag++;
 			u64_stats_update_end(&secy_stats->syncp);
@@ -902,9 +945,17 @@ static void handle_not_macsec(struct sk_buff *skb)
 			secy_stats->stats.InPktsUntagged++;
 			u64_stats_update_end(&secy_stats->syncp);
 		}
+
+		if (netif_running(macsec->secy.netdev) &&
+		    macsec_get_ops(macsec, NULL)) {
+			ret = RX_HANDLER_EXACT;
+			goto out;
+		}
 	}
 
+out:
 	rcu_read_unlock();
+	return ret;
 }
 
 static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
@@ -929,12 +980,8 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
 		goto drop_direct;
 
 	hdr = macsec_ethhdr(skb);
-	if (hdr->eth.h_proto != htons(ETH_P_MACSEC)) {
-		handle_not_macsec(skb);
-
-		/* and deliver to the uncontrolled port */
-		return RX_HANDLER_PASS;
-	}
+	if (hdr->eth.h_proto != htons(ETH_P_MACSEC))
+		return handle_not_macsec(skb);
 
 	skb = skb_unshare(skb, GFP_ATOMIC);
 	*pskb = skb;
@@ -1439,6 +1486,40 @@ static const struct nla_policy macsec_genl_sa_policy[NUM_MACSEC_SA_ATTR] = {
 				 .len = MACSEC_MAX_KEY_LEN, },
 };
 
+/* Offloads an operation to a device driver */
+static int macsec_offload(int (* const func)(struct macsec_context *),
+			  struct macsec_context *ctx)
+{
+	int ret;
+
+	if (unlikely(!func))
+		return 0;
+
+	if (ctx->is_phy)
+		mutex_lock(&ctx->phydev->lock);
+
+	/* Phase I: prepare. The drive should fail here if there are going to be
+	 * issues in the commit phase.
+	 */
+	ctx->prepare = true;
+	ret = (*func)(ctx);
+	if (ret)
+		goto phy_unlock;
+
+	/* Phase II: commit. This step cannot fail. */
+	ctx->prepare = false;
+	ret = (*func)(ctx);
+	/* This should never happen: commit is not allowed to fail */
+	if (unlikely(ret))
+		WARN(1, "MACsec offloading commit failed (%d)\n", ret);
+
+phy_unlock:
+	if (ctx->is_phy)
+		mutex_unlock(&ctx->phydev->lock);
+
+	return ret;
+}
+
 static int parse_sa_config(struct nlattr **attrs, struct nlattr **tb_sa)
 {
 	if (!attrs[MACSEC_ATTR_SA_CONFIG])
@@ -1490,11 +1571,14 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info)
 	struct net_device *dev;
 	struct nlattr **attrs = info->attrs;
 	struct macsec_secy *secy;
-	struct macsec_rx_sc *rx_sc;
+	struct macsec_rx_sc *rx_sc, *prev_sc;
 	struct macsec_rx_sa *rx_sa;
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
 	unsigned char assoc_num;
 	struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1];
 	struct nlattr *tb_sa[MACSEC_SA_ATTR_MAX + 1];
+	bool was_active;
 	int err;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
@@ -1551,11 +1635,32 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info)
 		spin_unlock_bh(&rx_sa->lock);
 	}
 
+	was_active = rx_sa->active;
 	if (tb_sa[MACSEC_SA_ATTR_ACTIVE])
 		rx_sa->active = !!nla_get_u8(tb_sa[MACSEC_SA_ATTR_ACTIVE]);
 
-	nla_memcpy(rx_sa->key.id, tb_sa[MACSEC_SA_ATTR_KEYID], MACSEC_KEYID_LEN);
+	prev_sc = rx_sa->sc;
 	rx_sa->sc = rx_sc;
+
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.sa.assoc_num = assoc_num;
+		ctx.sa.rx_sa = rx_sa;
+		memcpy(ctx.sa.key, nla_data(tb_sa[MACSEC_SA_ATTR_KEY]),
+		       MACSEC_KEYID_LEN);
+
+		err = macsec_offload(ops->mdo_add_rxsa, &ctx);
+		if (err) {
+			rx_sa->active = was_active;
+			rx_sa->sc = prev_sc;
+			kfree(rx_sa);
+			rtnl_unlock();
+			return err;
+		}
+	}
+
+	nla_memcpy(rx_sa->key.id, tb_sa[MACSEC_SA_ATTR_KEYID], MACSEC_KEYID_LEN);
 	rcu_assign_pointer(rx_sc->sa[assoc_num], rx_sa);
 
 	rtnl_unlock();
@@ -1583,6 +1688,10 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
 	struct nlattr **attrs = info->attrs;
 	struct macsec_rx_sc *rx_sc;
 	struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1];
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
+	bool was_active;
+	int ret;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
 		return -EINVAL;
@@ -1608,9 +1717,22 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
 		return PTR_ERR(rx_sc);
 	}
 
+	was_active = rx_sc->active;
 	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE])
 		rx_sc->active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);
 
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.rx_sc = rx_sc;
+
+		ret = macsec_offload(ops->mdo_add_rxsc, &ctx);
+		if (ret) {
+			rx_sc->active = was_active;
+			rtnl_unlock();
+			return ret;
+		}
+	}
+
 	rtnl_unlock();
 
 	return 0;
@@ -1648,8 +1770,12 @@ static int macsec_add_txsa(struct sk_buff *skb, struct genl_info *info)
 	struct macsec_secy *secy;
 	struct macsec_tx_sc *tx_sc;
 	struct macsec_tx_sa *tx_sa;
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
 	unsigned char assoc_num;
 	struct nlattr *tb_sa[MACSEC_SA_ATTR_MAX + 1];
+	bool was_operational, was_active;
+	u32 prev_pn;
 	int err;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
@@ -1700,18 +1826,42 @@ static int macsec_add_txsa(struct sk_buff *skb, struct genl_info *info)
 		return err;
 	}
 
-	nla_memcpy(tx_sa->key.id, tb_sa[MACSEC_SA_ATTR_KEYID], MACSEC_KEYID_LEN);
-
 	spin_lock_bh(&tx_sa->lock);
+	prev_pn = tx_sa->next_pn;
 	tx_sa->next_pn = nla_get_u32(tb_sa[MACSEC_SA_ATTR_PN]);
 	spin_unlock_bh(&tx_sa->lock);
 
+	was_active = tx_sa->active;
 	if (tb_sa[MACSEC_SA_ATTR_ACTIVE])
 		tx_sa->active = !!nla_get_u8(tb_sa[MACSEC_SA_ATTR_ACTIVE]);
 
+	was_operational = secy->operational;
 	if (assoc_num == tx_sc->encoding_sa && tx_sa->active)
 		secy->operational = true;
 
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.sa.assoc_num = assoc_num;
+		ctx.sa.tx_sa = tx_sa;
+		memcpy(ctx.sa.key, nla_data(tb_sa[MACSEC_SA_ATTR_KEY]),
+		       MACSEC_KEYID_LEN);
+
+		err = macsec_offload(ops->mdo_add_txsa, &ctx);
+		if (err) {
+			spin_lock_bh(&tx_sa->lock);
+			tx_sa->next_pn = prev_pn;
+			spin_unlock_bh(&tx_sa->lock);
+
+			tx_sa->active = was_active;
+			secy->operational = was_operational;
+			kfree(tx_sa);
+			rtnl_unlock();
+			return err;
+		}
+	}
+
+	nla_memcpy(tx_sa->key.id, tb_sa[MACSEC_SA_ATTR_KEYID], MACSEC_KEYID_LEN);
 	rcu_assign_pointer(tx_sc->sa[assoc_num], tx_sa);
 
 	rtnl_unlock();
@@ -1726,9 +1876,12 @@ static int macsec_del_rxsa(struct sk_buff *skb, struct genl_info *info)
 	struct macsec_secy *secy;
 	struct macsec_rx_sc *rx_sc;
 	struct macsec_rx_sa *rx_sa;
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
 	u8 assoc_num;
 	struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1];
 	struct nlattr *tb_sa[MACSEC_SA_ATTR_MAX + 1];
+	int ret;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
 		return -EINVAL;
@@ -1752,6 +1905,19 @@ static int macsec_del_rxsa(struct sk_buff *skb, struct genl_info *info)
 		return -EBUSY;
 	}
 
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.sa.assoc_num = assoc_num;
+		ctx.sa.rx_sa = rx_sa;
+
+		ret = macsec_offload(ops->mdo_del_rxsa, &ctx);
+		if (ret) {
+			rtnl_unlock();
+			return ret;
+		}
+	}
+
 	RCU_INIT_POINTER(rx_sc->sa[assoc_num], NULL);
 	clear_rx_sa(rx_sa);
 
@@ -1766,8 +1932,11 @@ static int macsec_del_rxsc(struct sk_buff *skb, struct genl_info *info)
 	struct net_device *dev;
 	struct macsec_secy *secy;
 	struct macsec_rx_sc *rx_sc;
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
 	sci_t sci;
 	struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1];
+	int ret;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
 		return -EINVAL;
@@ -1794,6 +1963,17 @@ static int macsec_del_rxsc(struct sk_buff *skb, struct genl_info *info)
 		return -ENODEV;
 	}
 
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.rx_sc = rx_sc;
+		ret = macsec_offload(ops->mdo_del_rxsc, &ctx);
+		if (ret) {
+			rtnl_unlock();
+			return ret;
+		}
+	}
+
 	free_rx_sc(rx_sc);
 	rtnl_unlock();
 
@@ -1807,8 +1987,11 @@ static int macsec_del_txsa(struct sk_buff *skb, struct genl_info *info)
 	struct macsec_secy *secy;
 	struct macsec_tx_sc *tx_sc;
 	struct macsec_tx_sa *tx_sa;
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
 	u8 assoc_num;
 	struct nlattr *tb_sa[MACSEC_SA_ATTR_MAX + 1];
+	int ret;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
 		return -EINVAL;
@@ -1829,6 +2012,19 @@ static int macsec_del_txsa(struct sk_buff *skb, struct genl_info *info)
 		return -EBUSY;
 	}
 
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.sa.assoc_num = assoc_num;
+		ctx.sa.tx_sa = tx_sa;
+
+		ret = macsec_offload(ops->mdo_del_txsa, &ctx);
+		if (ret) {
+			rtnl_unlock();
+			return ret;
+		}
+	}
+
 	RCU_INIT_POINTER(tx_sc->sa[assoc_num], NULL);
 	clear_tx_sa(tx_sa);
 
@@ -1865,8 +2061,13 @@ static int macsec_upd_txsa(struct sk_buff *skb, struct genl_info *info)
 	struct macsec_secy *secy;
 	struct macsec_tx_sc *tx_sc;
 	struct macsec_tx_sa *tx_sa;
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
 	u8 assoc_num;
 	struct nlattr *tb_sa[MACSEC_SA_ATTR_MAX + 1];
+	bool was_operational, was_active;
+	u32 prev_pn = 0;
+	int ret = 0;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
 		return -EINVAL;
@@ -1887,19 +2088,41 @@ static int macsec_upd_txsa(struct sk_buff *skb, struct genl_info *info)
 
 	if (tb_sa[MACSEC_SA_ATTR_PN]) {
 		spin_lock_bh(&tx_sa->lock);
+		prev_pn = tx_sa->next_pn;
 		tx_sa->next_pn = nla_get_u32(tb_sa[MACSEC_SA_ATTR_PN]);
 		spin_unlock_bh(&tx_sa->lock);
 	}
 
+	was_active = tx_sa->active;
 	if (tb_sa[MACSEC_SA_ATTR_ACTIVE])
 		tx_sa->active = nla_get_u8(tb_sa[MACSEC_SA_ATTR_ACTIVE]);
 
+	was_operational = secy->operational;
 	if (assoc_num == tx_sc->encoding_sa)
 		secy->operational = tx_sa->active;
 
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.sa.assoc_num = assoc_num;
+		ctx.sa.tx_sa = tx_sa;
+
+		ret = macsec_offload(ops->mdo_upd_txsa, &ctx);
+		if (ret) {
+			if (tb_sa[MACSEC_SA_ATTR_PN]) {
+				spin_lock_bh(&tx_sa->lock);
+				tx_sa->next_pn = prev_pn;
+				spin_unlock_bh(&tx_sa->lock);
+			}
+
+			tx_sa->active = was_active;
+			secy->operational = was_operational;
+		}
+	}
+
 	rtnl_unlock();
 
-	return 0;
+	return ret;
 }
 
 static int macsec_upd_rxsa(struct sk_buff *skb, struct genl_info *info)
@@ -1909,9 +2132,14 @@ static int macsec_upd_rxsa(struct sk_buff *skb, struct genl_info *info)
 	struct macsec_secy *secy;
 	struct macsec_rx_sc *rx_sc;
 	struct macsec_rx_sa *rx_sa;
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
 	u8 assoc_num;
 	struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1];
 	struct nlattr *tb_sa[MACSEC_SA_ATTR_MAX + 1];
+	bool was_active;
+	u32 prev_pn = 0;
+	int ret = 0;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
 		return -EINVAL;
@@ -1935,15 +2163,35 @@ static int macsec_upd_rxsa(struct sk_buff *skb, struct genl_info *info)
 
 	if (tb_sa[MACSEC_SA_ATTR_PN]) {
 		spin_lock_bh(&rx_sa->lock);
+		prev_pn = rx_sa->next_pn;
 		rx_sa->next_pn = nla_get_u32(tb_sa[MACSEC_SA_ATTR_PN]);
 		spin_unlock_bh(&rx_sa->lock);
 	}
 
+	was_active = rx_sa->active;
 	if (tb_sa[MACSEC_SA_ATTR_ACTIVE])
 		rx_sa->active = nla_get_u8(tb_sa[MACSEC_SA_ATTR_ACTIVE]);
 
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.sa.assoc_num = assoc_num;
+		ctx.sa.rx_sa = rx_sa;
+
+		ret = macsec_offload(ops->mdo_upd_rxsa, &ctx);
+		if (ret) {
+			if (tb_sa[MACSEC_SA_ATTR_PN]) {
+				spin_lock_bh(&rx_sa->lock);
+				rx_sa->next_pn = prev_pn;
+				spin_unlock_bh(&rx_sa->lock);
+			}
+
+			rx_sa->active = was_active;
+		}
+	}
+
 	rtnl_unlock();
-	return 0;
+	return ret;
 }
 
 static int macsec_upd_rxsc(struct sk_buff *skb, struct genl_info *info)
@@ -1953,6 +2201,11 @@ static int macsec_upd_rxsc(struct sk_buff *skb, struct genl_info *info)
 	struct macsec_secy *secy;
 	struct macsec_rx_sc *rx_sc;
 	struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1];
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
+	unsigned int prev_n_rx_sc;
+	bool was_active;
+	int ret;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
 		return -EINVAL;
@@ -1970,6 +2223,8 @@ static int macsec_upd_rxsc(struct sk_buff *skb, struct genl_info *info)
 		return PTR_ERR(rx_sc);
 	}
 
+	was_active = rx_sc->active;
+	prev_n_rx_sc = secy->n_rx_sc;
 	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]) {
 		bool new = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);
 
@@ -1979,6 +2234,19 @@ static int macsec_upd_rxsc(struct sk_buff *skb, struct genl_info *info)
 		rx_sc->active = new;
 	}
 
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.rx_sc = rx_sc;
+
+		ret = macsec_offload(ops->mdo_upd_rxsc, &ctx);
+		if (ret) {
+			secy->n_rx_sc = prev_n_rx_sc;
+			rx_sc->active = was_active;
+			rtnl_unlock();
+			return 0;
+		}
+	}
+
 	rtnl_unlock();
 
 	return 0;
@@ -2546,11 +2814,15 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 {
 	struct macsec_dev *macsec = netdev_priv(dev);
 	struct macsec_secy *secy = &macsec->secy;
+	struct macsec_tx_sc *tx_sc = &secy->tx_sc;
 	struct pcpu_secy_stats *secy_stats;
+	struct macsec_tx_sa *tx_sa;
 	int ret, len;
 
+	tx_sa = macsec_txsa_get(tx_sc->sa[tx_sc->encoding_sa]);
+
 	/* 10.5 */
-	if (!secy->protect_frames) {
+	if (!secy->protect_frames || macsec_get_ops(netdev_priv(dev), NULL)) {
 		secy_stats = this_cpu_ptr(macsec->stats);
 		u64_stats_update_begin(&secy_stats->syncp);
 		secy_stats->stats.OutPktsUntagged++;
@@ -2645,6 +2917,8 @@ static int macsec_dev_open(struct net_device *dev)
 {
 	struct macsec_dev *macsec = macsec_priv(dev);
 	struct net_device *real_dev = macsec->real_dev;
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
 	int err;
 
 	err = dev_uc_add(real_dev, dev->dev_addr);
@@ -2663,6 +2937,14 @@ static int macsec_dev_open(struct net_device *dev)
 			goto clear_allmulti;
 	}
 
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		err = macsec_offload(ops->mdo_dev_open, &ctx);
+		if (err)
+			goto clear_allmulti;
+	}
+
 	if (netif_carrier_ok(real_dev))
 		netif_carrier_on(dev);
 
@@ -2680,9 +2962,16 @@ static int macsec_dev_stop(struct net_device *dev)
 {
 	struct macsec_dev *macsec = macsec_priv(dev);
 	struct net_device *real_dev = macsec->real_dev;
+	const struct macsec_ops *ops;
+	struct macsec_context ctx;
 
 	netif_carrier_off(dev);
 
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops)
+		macsec_offload(ops->mdo_dev_stop, &ctx);
+
 	dev_mc_unsync(real_dev, dev);
 	dev_uc_unsync(real_dev, dev);
 
@@ -2922,6 +3211,11 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
 			     struct nlattr *data[],
 			     struct netlink_ext_ack *extack)
 {
+	struct macsec_dev *macsec = macsec_priv(dev);
+	struct macsec_context ctx;
+	const struct macsec_ops *ops;
+	int ret;
+
 	if (!data)
 		return 0;
 
@@ -2931,7 +3225,18 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
 	    data[IFLA_MACSEC_PORT])
 		return -EINVAL;
 
-	return macsec_changelink_common(dev, data);
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.secy = &macsec->secy;
+		return macsec_offload(ops->mdo_upd_secy, &ctx);
+	}
+
+	ret = macsec_changelink_common(dev, data);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 static void macsec_del_dev(struct macsec_dev *macsec)
@@ -2973,6 +3278,15 @@ static void macsec_dellink(struct net_device *dev, struct list_head *head)
 	struct macsec_dev *macsec = macsec_priv(dev);
 	struct net_device *real_dev = macsec->real_dev;
 	struct macsec_rxh_data *rxd = macsec_data_rtnl(real_dev);
+	struct macsec_context ctx;
+	const struct macsec_ops *ops;
+
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.secy = &macsec->secy;
+		macsec_offload(ops->mdo_del_secy, &ctx);
+	}
 
 	macsec_common_dellink(dev, head);
 
@@ -3069,7 +3383,10 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
 			  struct netlink_ext_ack *extack)
 {
 	struct macsec_dev *macsec = macsec_priv(dev);
-	struct net_device *real_dev;
+	struct net_device *real_dev, *loop_dev;
+	struct macsec_context ctx;
+	const struct macsec_ops *ops;
+	struct net *loop_net;
 	int err;
 	sci_t sci;
 	u8 icv_len = DEFAULT_ICV_LEN;
@@ -3081,6 +3398,25 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
 	if (!real_dev)
 		return -ENODEV;
 
+	for_each_net(loop_net) {
+		for_each_netdev(loop_net, loop_dev) {
+			struct macsec_dev *priv;
+
+			if (!netif_is_macsec(loop_dev))
+				continue;
+
+			priv = macsec_priv(loop_dev);
+
+			/* A limitation of the MACsec h/w offloading is only a
+			 * single MACsec interface can be created for a given
+			 * real interface.
+			 */
+			if (macsec_get_ops(netdev_priv(dev), NULL) &&
+			    priv->real_dev == real_dev)
+				return -EBUSY;
+		}
+	}
+
 	dev->priv_flags |= IFF_MACSEC;
 
 	macsec->real_dev = real_dev;
@@ -3134,6 +3470,15 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
 			goto del_dev;
 	}
 
+	/* If h/w offloading is available, propagate to the device */
+	ops = macsec_get_ops(netdev_priv(dev), &ctx);
+	if (ops) {
+		ctx.secy = &macsec->secy;
+		err = macsec_offload(ops->mdo_add_secy, &ctx);
+		if (err)
+			goto del_dev;
+	}
+
 	err = register_macsec_dev(real_dev, dev);
 	if (err < 0)
 		goto del_dev;
-- 
2.21.0

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Igor Russkikh]

On 08.08.2019 17:05, Antoine Tenart wrote:

> The Rx and TX handlers are modified to take in account the special case
> were the MACsec transformation happens in the hardware, whether in a PHY
> or in a MAC, as the packets seen by the networking stack on both the

Don't you think we may eventually may need xmit / handle_frame ops to be
a part of macsec_ops?

That way software macsec could be extract to just another type of offload.
The drawback of current code is it doesn't show explicitly the path of
offloaded packets. It is hidden in `handle_not_macsec` and in
`macsec_start_xmit` branch. This makes incorrect counters to tick (see my below
comment)

Another thing is that both xmit / macsec_handle_frame can't now be customized
by device driver. But this may be required.
We for example have usecases and HW features to allow specific flows to bypass
macsec encryption. This is normally used for macsec key control protocols,
identified by ethertype. Your phy is also capable on that as I see.


> @@ -2546,11 +2814,15 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
>  {
>  	struct macsec_dev *macsec = netdev_priv(dev);
>  	struct macsec_secy *secy = &macsec->secy;
> +	struct macsec_tx_sc *tx_sc = &secy->tx_sc;
>  	struct pcpu_secy_stats *secy_stats;
> +	struct macsec_tx_sa *tx_sa;
>  	int ret, len;
>  
> +	tx_sa = macsec_txsa_get(tx_sc->sa[tx_sc->encoding_sa]);

Declared, but not used?

>  	/* 10.5 */
> -	if (!secy->protect_frames) {
> +	if (!secy->protect_frames || macsec_get_ops(netdev_priv(dev), NULL)) {
>  		secy_stats = this_cpu_ptr(macsec->stats);
>  		u64_stats_update_begin(&secy_stats->syncp);
>  		secy_stats->stats.OutPktsUntagged++;

Here you use same `if` for sw and hw flows, this making `OutPktsUntagged`
counter invalid.

>  	struct macsec_dev *macsec = macsec_priv(dev);
> -	struct net_device *real_dev;
> +	struct net_device *real_dev, *loop_dev;
> +	struct macsec_context ctx;
> +	const struct macsec_ops *ops;
> +	struct net *loop_net;

Reverse Christmas tree is normally a formatting requirement where possible.

> +	for_each_net(loop_net) {
> +		for_each_netdev(loop_net, loop_dev) {
> +			struct macsec_dev *priv;
> +
> +			if (!netif_is_macsec(loop_dev))
> +				continue;
> +
> +			priv = macsec_priv(loop_dev);
> +
> +			/* A limitation of the MACsec h/w offloading is only a
> +			 * single MACsec interface can be created for a given
> +			 * real interface.
> +			 */
> +			if (macsec_get_ops(netdev_priv(dev), NULL) &&
> +			    priv->real_dev == real_dev)
> +				return -EBUSY;
> +		}
> +	}
> +

There is no need to do this search loop if `macsec_get_ops(..) == NULL` ?
So you can extract this check before `for_each_net` for SW macsec...

Regards,
  Igor

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Antoine Tenart]

Hi Igor,

On Sat, Aug 10, 2019 at 01:20:32PM +0000, Igor Russkikh wrote:
> On 08.08.2019 17:05, Antoine Tenart wrote:
> 
> > The Rx and TX handlers are modified to take in account the special case
> > were the MACsec transformation happens in the hardware, whether in a PHY
> > or in a MAC, as the packets seen by the networking stack on both the
> 
> Don't you think we may eventually may need xmit / handle_frame ops to be
> a part of macsec_ops?
> 
> That way software macsec could be extract to just another type of offload.
> The drawback of current code is it doesn't show explicitly the path of
> offloaded packets. It is hidden in `handle_not_macsec` and in
> `macsec_start_xmit` branch. This makes incorrect counters to tick (see my below
> comment)
> 
> Another thing is that both xmit / macsec_handle_frame can't now be customized
> by device driver. But this may be required.
> We for example have usecases and HW features to allow specific flows to bypass
> macsec encryption. This is normally used for macsec key control protocols,
> identified by ethertype. Your phy is also capable on that as I see.

I think this question is linked to the use of a MACsec virtual interface
when using h/w offloading. The starting point for me was that I wanted
to reuse the data structures and the API exposed to the userspace by the
s/w implementation of MACsec. I then had two choices: keeping the exact
same interface for the user (having a virtual MACsec interface), or
registering the MACsec genl ops onto the real net devices (and making
the s/w implementation a virtual net dev and a provider of the MACsec
"offloading" ops).

The advantages of the first option were that nearly all the logic of the
s/w implementation could be kept and especially that it would be
transparent for the user to use both implementations of MACsec. But this
raised an issue as I had to modify the xmit / handle_frame ops to let
all the traffic pass. This is because we have no way of knowing if a
frame was handled by the MACsec h/w or not in ingress. So the virtual
interface here only serve as the entrypoint for the API...

The second option would have the advantage to better represent the actual
flow, but the way of configuring MACsec would be a bit different for the
user, whether he wants to use s/w or h/w MACsec. If we were to do this I
think we could extract the genl functions from the MACsec s/w
implementation, and let it implement the MACsec ops (exactly as the
offloading drivers).

I'm open to discussing this :)

As for the need for xmit / handle_frame ops (for a MAC w/ MACsec
offloading), I'd say the xmit / handle_frame ops of the real net device
driver could be used as the one of the MACsec virtual interface do not
do much (regardless of the implementation choice discussed above).

> > @@ -2546,11 +2814,15 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
> >  {
> >  	struct macsec_dev *macsec = netdev_priv(dev);
> >  	struct macsec_secy *secy = &macsec->secy;
> > +	struct macsec_tx_sc *tx_sc = &secy->tx_sc;
> >  	struct pcpu_secy_stats *secy_stats;
> > +	struct macsec_tx_sa *tx_sa;
> >  	int ret, len;
> >  
> > +	tx_sa = macsec_txsa_get(tx_sc->sa[tx_sc->encoding_sa]);
> 
> Declared, but not used?

I'll remove it then.

> >  	/* 10.5 */
> > -	if (!secy->protect_frames) {
> > +	if (!secy->protect_frames || macsec_get_ops(netdev_priv(dev), NULL)) {
> >  		secy_stats = this_cpu_ptr(macsec->stats);
> >  		u64_stats_update_begin(&secy_stats->syncp);
> >  		secy_stats->stats.OutPktsUntagged++;
> 
> Here you use same `if` for sw and hw flows, this making `OutPktsUntagged`
> counter invalid.

Right, I'll try to fix that.

> >  	struct macsec_dev *macsec = macsec_priv(dev);
> > -	struct net_device *real_dev;
> > +	struct net_device *real_dev, *loop_dev;
> > +	struct macsec_context ctx;
> > +	const struct macsec_ops *ops;
> > +	struct net *loop_net;
> 
> Reverse Christmas tree is normally a formatting requirement where possible.

Sure.

> > +	for_each_net(loop_net) {
> > +		for_each_netdev(loop_net, loop_dev) {
> > +			struct macsec_dev *priv;
> > +
> > +			if (!netif_is_macsec(loop_dev))
> > +				continue;
> > +
> > +			priv = macsec_priv(loop_dev);
> > +
> > +			/* A limitation of the MACsec h/w offloading is only a
> > +			 * single MACsec interface can be created for a given
> > +			 * real interface.
> > +			 */
> > +			if (macsec_get_ops(netdev_priv(dev), NULL) &&
> > +			    priv->real_dev == real_dev)
> > +				return -EBUSY;
> > +		}
> > +	}
> > +
> 
> There is no need to do this search loop if `macsec_get_ops(..) == NULL` ?
> So you can extract this check before `for_each_net` for SW macsec...

Right, I'll fix it!

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Andrew Lunn]

On Tue, Aug 13, 2019 at 10:58:17AM +0200, Antoine Tenart wrote:
> I think this question is linked to the use of a MACsec virtual interface
> when using h/w offloading. The starting point for me was that I wanted
> to reuse the data structures and the API exposed to the userspace by the
> s/w implementation of MACsec. I then had two choices: keeping the exact
> same interface for the user (having a virtual MACsec interface), or
> registering the MACsec genl ops onto the real net devices (and making
> the s/w implementation a virtual net dev and a provider of the MACsec
> "offloading" ops).
> 
> The advantages of the first option were that nearly all the logic of the
> s/w implementation could be kept and especially that it would be
> transparent for the user to use both implementations of MACsec.

Hi Antoine

We have always talked about offloading operations to the hardware,
accelerating what the linux stack can do by making use of hardware
accelerators. The basic user API should not change because of
acceleration. Those are the general guidelines.

It would however be interesting to get comments from those who did the
software implementation and what they think of this architecture. I've
no personal experience with MACSec, so it is hard for me to say if the
current architecture makes sense when using accelerators.

	Andrew

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Igor Russkikh]

On 13.08.2019 16:17, Andrew Lunn wrote:
> On Tue, Aug 13, 2019 at 10:58:17AM +0200, Antoine Tenart wrote:
>> I think this question is linked to the use of a MACsec virtual interface
>> when using h/w offloading. The starting point for me was that I wanted
>> to reuse the data structures and the API exposed to the userspace by the
>> s/w implementation of MACsec. I then had two choices: keeping the exact
>> same interface for the user (having a virtual MACsec interface), or
>> registering the MACsec genl ops onto the real net devices (and making
>> the s/w implementation a virtual net dev and a provider of the MACsec
>> "offloading" ops).
>>
>> The advantages of the first option were that nearly all the logic of the
>> s/w implementation could be kept and especially that it would be
>> transparent for the user to use both implementations of MACsec.
> 
> Hi Antoine
> 
> We have always talked about offloading operations to the hardware,
> accelerating what the linux stack can do by making use of hardware
> accelerators. The basic user API should not change because of
> acceleration. Those are the general guidelines.
> 
> It would however be interesting to get comments from those who did the
> software implementation and what they think of this architecture. I've
> no personal experience with MACSec, so it is hard for me to say if the
> current architecture makes sense when using accelerators.

In terms of overall concepts, I'd add the following:

1) With current implementation it's impossible to install SW macsec engine onto
the device which supports HW offload. That could be a strong limitation in
cases when user sees HW macsec offload is broken or work differently, and he/she
wants to replace it with SW one.
MACSec is a complex feature, and it may happen something is missing in HW.
Trivial example is 256bit encryption, which is not always a musthave in HW
implementations.

2) I think, Antoine, its not totally true that otherwise the user macsec API
will be broken/changed. netlink api is the same, the only thing we may want to
add is an optional parameter to force selection of SW macsec engine.

I'm also eager to hear from sw macsec users/devs on whats better here.


Regards,
  Igor

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Andrew Lunn]

> 1) With current implementation it's impossible to install SW macsec engine onto
> the device which supports HW offload. That could be a strong limitation in
> cases when user sees HW macsec offload is broken or work differently, and he/she
> wants to replace it with SW one.
> MACSec is a complex feature, and it may happen something is missing in HW.
> Trivial example is 256bit encryption, which is not always a musthave in HW
> implementations.

Ideally, we want the driver to return EOPNOTSUPP if it does not
support something and the software implement should be used.

If the offload is broken, we want a bug report! And if it works
differently, it suggests there is also a bug we need to fix, or the
standard is ambiguous.

It would also be nice to add extra information to the netlink API to
indicate if HW or SW is being used. In other places where we offload
to accelerators we have such additional information.

   Andrew

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Antoine Tenart]

Hi Andrew,

On Tue, Aug 13, 2019 at 06:28:23PM +0200, Andrew Lunn wrote:
> 
> It would also be nice to add extra information to the netlink API to
> indicate if HW or SW is being used. In other places where we offload
> to accelerators we have such additional information.

Yes, that would be very nice to have.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Florian Fainelli]

On 8/13/19 9:28 AM, Andrew Lunn wrote:
>> 1) With current implementation it's impossible to install SW macsec engine onto
>> the device which supports HW offload. That could be a strong limitation in
>> cases when user sees HW macsec offload is broken or work differently, and he/she
>> wants to replace it with SW one.
>> MACSec is a complex feature, and it may happen something is missing in HW.
>> Trivial example is 256bit encryption, which is not always a musthave in HW
>> implementations.
> 
> Ideally, we want the driver to return EOPNOTSUPP if it does not
> support something and the software implement should be used.
> 
> If the offload is broken, we want a bug report! And if it works
> differently, it suggests there is also a bug we need to fix, or the
> standard is ambiguous.
> 
> It would also be nice to add extra information to the netlink API to
> indicate if HW or SW is being used. In other places where we offload
> to accelerators we have such additional information.

Igor's point is entirely valid in that you should allow both offload to
HW for what is possible and offload to a software implementation for
what is not supported in HW.

Let's an example that is hopefully more familiar to the people in this
thread. Consider a NIC that can do single VLAN tag offload on xmit (or
receive, does not matter), and you find yourself using a double VLAN tag
configuration. You would create a first VLAN stacked network device
which is fully offloaded onto the underlying NIC, and a second VLAN
stacked network device on top of the first once which is not offloaded.

The way I would imagine a MACsec offload is kind of similar here, in
that it would be a macsec virtual network device on top of an underlying
physical device. If no offload is possible, the virtual network device's
xmit/receive path is used. If the NIC driver can offload it, it does
not. How it does it, whether at the MAC/DMA level, or at the PHY level
can be a check added at the appropriate level.
-- 
Florian

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Sabrina Dubroca]

2019-08-13, 18:28:23 +0200, Andrew Lunn wrote:
> > 1) With current implementation it's impossible to install SW macsec engine onto
> > the device which supports HW offload. That could be a strong limitation in
> > cases when user sees HW macsec offload is broken or work differently, and he/she
> > wants to replace it with SW one.
> > MACSec is a complex feature, and it may happen something is missing in HW.
> > Trivial example is 256bit encryption, which is not always a musthave in HW
> > implementations.
> 
> Ideally, we want the driver to return EOPNOTSUPP if it does not
> support something and the software implement should be used.
> 
> If the offload is broken, we want a bug report! And if it works
> differently, it suggests there is also a bug we need to fix, or the
> standard is ambiguous.

Yes. But in the meantime, we want the user to be able to disable the
offload. It's helpful for debugging purposes, and it can provide some
level of functionality until the bug is fixed or non-buggy hardware
becomes available.

> It would also be nice to add extra information to the netlink API to
> indicate if HW or SW is being used. In other places where we offload
> to accelerators we have such additional information.

+1

-- 
Sabrina

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Antoine Tenart]

Hi Andrew,

On Tue, Aug 13, 2019 at 06:28:23PM +0200, Andrew Lunn wrote:
> > 1) With current implementation it's impossible to install SW macsec engine onto
> > the device which supports HW offload. That could be a strong limitation in
> > cases when user sees HW macsec offload is broken or work differently, and he/she
> > wants to replace it with SW one.
> > MACSec is a complex feature, and it may happen something is missing in HW.
> > Trivial example is 256bit encryption, which is not always a musthave in HW
> > implementations.
> 
> It would also be nice to add extra information to the netlink API to
> indicate if HW or SW is being used. In other places where we offload
> to accelerators we have such additional information.

Agreed, in addition to being able to enable/disable the offloading we
should have a way to know if a MACsec interface is being offloaded or
not.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Antoine Tenart]

Hi Igor,

On Tue, Aug 13, 2019 at 04:18:40PM +0000, Igor Russkikh wrote:
> On 13.08.2019 16:17, Andrew Lunn wrote:
> > On Tue, Aug 13, 2019 at 10:58:17AM +0200, Antoine Tenart wrote:
> >> I think this question is linked to the use of a MACsec virtual interface
> >> when using h/w offloading. The starting point for me was that I wanted
> >> to reuse the data structures and the API exposed to the userspace by the
> >> s/w implementation of MACsec. I then had two choices: keeping the exact
> >> same interface for the user (having a virtual MACsec interface), or
> >> registering the MACsec genl ops onto the real net devices (and making
> >> the s/w implementation a virtual net dev and a provider of the MACsec
> >> "offloading" ops).
> >>
> >> The advantages of the first option were that nearly all the logic of the
> >> s/w implementation could be kept and especially that it would be
> >> transparent for the user to use both implementations of MACsec.
> > 
> > We have always talked about offloading operations to the hardware,
> > accelerating what the linux stack can do by making use of hardware
> > accelerators. The basic user API should not change because of
> > acceleration. Those are the general guidelines.
> > 
> > It would however be interesting to get comments from those who did the
> > software implementation and what they think of this architecture. I've
> > no personal experience with MACSec, so it is hard for me to say if the
> > current architecture makes sense when using accelerators.
> 
> In terms of overall concepts, I'd add the following:
> 
> 1) With current implementation it's impossible to install SW macsec engine onto
> the device which supports HW offload. That could be a strong limitation in
> cases when user sees HW macsec offload is broken or work differently, and he/she
> wants to replace it with SW one.
> MACSec is a complex feature, and it may happen something is missing in HW.
> Trivial example is 256bit encryption, which is not always a musthave in HW
> implementations.

Agreed. I'm not sure it would be possible to have both used at the same
time but there should be a way to switch between the two
implementations. That is not supported for now, but I think that would
be a good thing, and can probably come later on.

> 2) I think, Antoine, its not totally true that otherwise the user macsec API
> will be broken/changed. netlink api is the same, the only thing we may want to
> add is an optional parameter to force selection of SW macsec engine.

I meant that we can either have a virtual net device representing the
MACsec feature and being the iface used to configure it, or we could
have it only when s/w MACsec is used. That to me is part of the "API",
or at least part of what's exposed to the user.

> I'm also eager to hear from sw macsec users/devs on whats better here.

I'd like more comments as well :)

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Sabrina Dubroca]

2019-08-13, 16:18:40 +0000, Igor Russkikh wrote:
> On 13.08.2019 16:17, Andrew Lunn wrote:
> > On Tue, Aug 13, 2019 at 10:58:17AM +0200, Antoine Tenart wrote:
> >> I think this question is linked to the use of a MACsec virtual interface
> >> when using h/w offloading. The starting point for me was that I wanted
> >> to reuse the data structures and the API exposed to the userspace by the
> >> s/w implementation of MACsec. I then had two choices: keeping the exact
> >> same interface for the user (having a virtual MACsec interface), or
> >> registering the MACsec genl ops onto the real net devices (and making
> >> the s/w implementation a virtual net dev and a provider of the MACsec
> >> "offloading" ops).
> >>
> >> The advantages of the first option were that nearly all the logic of the
> >> s/w implementation could be kept and especially that it would be
> >> transparent for the user to use both implementations of MACsec.
> > 
> > Hi Antoine
> > 
> > We have always talked about offloading operations to the hardware,
> > accelerating what the linux stack can do by making use of hardware
> > accelerators. The basic user API should not change because of
> > acceleration. Those are the general guidelines.
> > 
> > It would however be interesting to get comments from those who did the
> > software implementation and what they think of this architecture. I've
> > no personal experience with MACSec, so it is hard for me to say if the
> > current architecture makes sense when using accelerators.
> 
> In terms of overall concepts, I'd add the following:
> 
> 1) With current implementation it's impossible to install SW macsec engine onto
> the device which supports HW offload.

You mean how it's implemented in this patchset?

> That could be a strong limitation in
> cases when user sees HW macsec offload is broken or work differently, and he/she
> wants to replace it with SW one.

Agreed, I think an offload that cannot be disabled is quite problematic.

> MACSec is a complex feature, and it may happen something is missing in HW.
> Trivial example is 256bit encryption, which is not always a musthave in HW
> implementations.

+1

> 2) I think, Antoine, its not totally true that otherwise the user macsec API
> will be broken/changed. netlink api is the same, the only thing we may want to
> add is an optional parameter to force selection of SW macsec engine.

Yes, I think we need an offload on/off parameter (and IMO it should
probably be off by default). Then, if offloading is requested but
cannot be satisfied (unsupported key length, too many SAs, etc), or if
incompatible settings are requested (mixing offloaded and
non-offloaded SCs on a device that cannot do it), return an error.

If we also export that offload parameter during netlink dumps, we can
inspect the state of the system, which helps for debugging.

> I'm also eager to hear from sw macsec users/devs on whats better here.

I don't do much development on MACsec these days, and I don't
personally use it outside of testing and development.

-- 
Sabrina

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Antoine Tenart]

Hi Sabrina,

On Fri, Aug 16, 2019 at 03:29:59PM +0200, Sabrina Dubroca wrote:
> 2019-08-13, 16:18:40 +0000, Igor Russkikh wrote:
> > On 13.08.2019 16:17, Andrew Lunn wrote:
> 
> > That could be a strong limitation in
> > cases when user sees HW macsec offload is broken or work differently, and he/she
> > wants to replace it with SW one.
> 
> Agreed, I think an offload that cannot be disabled is quite problematic.
> 
> > MACSec is a complex feature, and it may happen something is missing in HW.
> > Trivial example is 256bit encryption, which is not always a musthave in HW
> > implementations.
> 
> +1
> 
> > 2) I think, Antoine, its not totally true that otherwise the user macsec API
> > will be broken/changed. netlink api is the same, the only thing we may want to
> > add is an optional parameter to force selection of SW macsec engine.
> 
> Yes, I think we need an offload on/off parameter (and IMO it should
> probably be off by default). Then, if offloading is requested but
> cannot be satisfied (unsupported key length, too many SAs, etc), or if
> incompatible settings are requested (mixing offloaded and
> non-offloaded SCs on a device that cannot do it), return an error.
> 
> If we also export that offload parameter during netlink dumps, we can
> inspect the state of the system, which helps for debugging.

So it seems the ability to enable or disable the offloading on a given
interface is the main missing feature. I'll add that, however I'll
probably (at least at first):

- Have the interface to be fully offloaded or fully handled in s/w (with
  errors being thrown if a given configuration isn't supported). Having
  both at the same time on a given interface would be tricky because of
  the MACsec validation parameter.

- Won't allow to enable/disable the offloading of there are rules in
  place, as we're not sure the same rules would be accepted by the other
  implementation.

I'm not sure if we should allow to mix the implementations on a given
physical interface (by having two MACsec interfaces attached) as the
validation would be impossible to do (we would have no idea if a
packet was correctly handled by the offloading part or just not being
a MACsec packet in the first place, in Rx).

I agree the offloading should be disabled by default, and only enabled
by an user explicitly.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Sabrina Dubroca]

2019-08-20, 12:01:40 +0200, Antoine Tenart wrote:
> So it seems the ability to enable or disable the offloading on a given
> interface is the main missing feature. I'll add that, however I'll
> probably (at least at first):
> 
> - Have the interface to be fully offloaded or fully handled in s/w (with
>   errors being thrown if a given configuration isn't supported). Having
>   both at the same time on a given interface would be tricky because of
>   the MACsec validation parameter.
> 
> - Won't allow to enable/disable the offloading of there are rules in
>   place, as we're not sure the same rules would be accepted by the other
>   implementation.

That's probably quite problematic actually, because to do that you
need to be able to resync the state between software and hardware,
particularly packet numbers. So, yeah, we're better off having it
completely blocked until we have a working implementation (if that
ever happens).

However, that means in case the user wants to set up something that's
not offloadable, they'll have to:
 - configure the offloaded version until EOPNOTSUPP
 - tear everything down
 - restart from scratch without offloading

That's inconvenient.

Talking about packet numbers, can you describe how PN exhaustion is
handled?  I couldn't find much about packet numbers at all in the
driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
the same SA).  At some point userspace needs to know that we're
getting close to 2^32 and that it's time to re-key.  Since the whole
TX path of the software implementation is bypassed, it looks like the
PN (as far as drivers/net/macsec.c is concerned) never increases, so
userspace can't know when to negotiate a new SA.

> I'm not sure if we should allow to mix the implementations on a given
> physical interface (by having two MACsec interfaces attached) as the
> validation would be impossible to do (we would have no idea if a
> packet was correctly handled by the offloading part or just not being
> a MACsec packet in the first place, in Rx).

That's something that really bothers me with this proposal. Quoting
from the commit message:

> the packets seen by the networking stack on both the physical and
> MACsec virtual interface are exactly the same

If the HW/driver is expected to strip the sectag, I don't see how we
could ever have multiple secy's at the same time and demultiplex
properly between them. That's part of the reason why I chose to have
virtual interfaces: without them, picking the right secy on TX gets
weird.

AFAICT, it means that any filters (tc, tcpdump) need to be different
between offloaded and non-offloaded cases.

And it's going to be confusing to the administrator when they look at
tcpdumps expecting to see MACsec frames.

I don't see how this implementation handles non-macsec traffic (on TX,
that would be packets sent directly through the real interface, for
example by wpa_supplicant - on RX, incoming MKA traffic for
wpa_supplicant). Unless I missed something, incoming MKA traffic will
end up on the macsec interface as well as the lower interface (not
entirely critical, as long as wpa_supplicant can grab it on the lower
device, but not consistent with the software implementation). How does
the driver distinguish traffic that should pass through unmodified
from traffic that the HW needs to encapsulate and encrypt?

If you look at IPsec offloading, the networking stack builds up the
ESP header, and passes the unencrypted data down to the driver. I'm
wondering if the same would be possible with MACsec offloading: the
macsec virtual interface adds the header (and maybe a dummy ICV), and
then the HW does the encryption. In case of HW that needs to add the
sectag itself, the driver would first strip the headers that the stack
created. On receive, the driver would recreate a sectag and the macsec
interface would just skip all verification (decrypt, PN).

-- 
Sabrina

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Andrew Lunn]

> If you look at IPsec offloading, the networking stack builds up the
> ESP header, and passes the unencrypted data down to the driver. I'm
> wondering if the same would be possible with MACsec offloading: the
> macsec virtual interface adds the header (and maybe a dummy ICV), and
> then the HW does the encryption. In case of HW that needs to add the
> sectag itself, the driver would first strip the headers that the stack
> created. On receive, the driver would recreate a sectag and the macsec
> interface would just skip all verification (decrypt, PN).

Hi Sabrina

I assume the software implementation cannot make use of TSO or GSO,
letting the hardware segment a big buffer up into Ethernet frames?
When using hardware MACSEC, is it possible to enable these? By the
time the frames have reach the PHY GSO has been done. So it sees a
stream of frames it needs to encode/decode.

But if you are suggesting the extra headers are added by the virtual
interface, i don't think GSO can be used? My guess would be, we get a
performance boost from using hardware MAC sec, but there will also be
a performance boost if GSO can be enabled when it was disabled before.

      Andrew

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Igor Russkikh]

> 
> Talking about packet numbers, can you describe how PN exhaustion is
> handled?  I couldn't find much about packet numbers at all in the
> driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
> the same SA).  At some point userspace needs to know that we're
> getting close to 2^32 and that it's time to re-key.  Since the whole
> TX path of the software implementation is bypassed, it looks like the
> PN (as far as drivers/net/macsec.c is concerned) never increases, so
> userspace can't know when to negotiate a new SA.

I think there should be driver specific implementation of this functionality.
As an example, our macsec HW issues an interrupt towards the host to indicate
PN threshold has reached and it's time for userspace to change the keys.

In contrast, current SW macsec implementation just stops this SA/secy.

> I don't see how this implementation handles non-macsec traffic (on TX,
> that would be packets sent directly through the real interface, for
> example by wpa_supplicant - on RX, incoming MKA traffic for
> wpa_supplicant). Unless I missed something, incoming MKA traffic will
> end up on the macsec interface as well as the lower interface (not
> entirely critical, as long as wpa_supplicant can grab it on the lower
> device, but not consistent with the software implementation). How does
> the driver distinguish traffic that should pass through unmodified
> from traffic that the HW needs to encapsulate and encrypt?

I can comment on our HW engine - where it has special bypass rules
for configured ethertypes. This way macsec engine skips encryption on TX and
passes in RX unencrypted for the selected control packets.

But thats true, realdev driver is hard to distinguish encrypted/unencrypted
packets. In case realdev should make a decision where to put RX packet,
it only may do some heuristic (since after macsec decription all the
macsec related info is dropped. Thats true at least for our HW implementation).

> If you look at IPsec offloading, the networking stack builds up the
> ESP header, and passes the unencrypted data down to the driver. I'm
> wondering if the same would be possible with MACsec offloading: the
> macsec virtual interface adds the header (and maybe a dummy ICV), and
> then the HW does the encryption. In case of HW that needs to add the
> sectag itself, the driver would first strip the headers that the stack
> created. On receive, the driver would recreate a sectag and the macsec
> interface would just skip all verification (decrypt, PN).

I don't think this way is good, as driver have to do per packet header mangling.
That'll harm linerate performance heavily.

Regards,
   Igor

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[allan.nielsen]

The 08/21/2019 09:20, Igor Russkikh wrote:
> > Talking about packet numbers, can you describe how PN exhaustion is
> > handled?  I couldn't find much about packet numbers at all in the
> > driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
> > the same SA).  At some point userspace needs to know that we're
> > getting close to 2^32 and that it's time to re-key.  Since the whole
> > TX path of the software implementation is bypassed, it looks like the
> > PN (as far as drivers/net/macsec.c is concerned) never increases, so
> > userspace can't know when to negotiate a new SA.
> 
> I think there should be driver specific implementation of this functionality.
> As an example, our macsec HW issues an interrupt towards the host to indicate
> PN threshold has reached and it's time for userspace to change the keys.
> 
> In contrast, current SW macsec implementation just stops this SA/secy.
> 
> > I don't see how this implementation handles non-macsec traffic (on TX,
> > that would be packets sent directly through the real interface, for
> > example by wpa_supplicant - on RX, incoming MKA traffic for
> > wpa_supplicant). Unless I missed something, incoming MKA traffic will
> > end up on the macsec interface as well as the lower interface (not
> > entirely critical, as long as wpa_supplicant can grab it on the lower
> > device, but not consistent with the software implementation). How does
> > the driver distinguish traffic that should pass through unmodified
> > from traffic that the HW needs to encapsulate and encrypt?
> 
> I can comment on our HW engine - where it has special bypass rules
> for configured ethertypes. This way macsec engine skips encryption on TX and
> passes in RX unencrypted for the selected control packets.
In our case it is a TCAM which can look at various fields (including ethertype),
but is sounds like we have a vary similar design.

> But thats true, realdev driver is hard to distinguish encrypted/unencrypted
> packets. In case realdev should make a decision where to put RX packet,
> it only may do some heuristic (since after macsec decription all the
> macsec related info is dropped. Thats true at least for our HW implementation).
Same for ours.

> > If you look at IPsec offloading, the networking stack builds up the
> > ESP header, and passes the unencrypted data down to the driver. I'm
> > wondering if the same would be possible with MACsec offloading: the
> > macsec virtual interface adds the header (and maybe a dummy ICV), and
> > then the HW does the encryption. In case of HW that needs to add the
> > sectag itself, the driver would first strip the headers that the stack
> > created. On receive, the driver would recreate a sectag and the macsec
> > interface would just skip all verification (decrypt, PN).
> 
> I don't think this way is good, as driver have to do per packet header mangling.
> That'll harm linerate performance heavily.
Agree, and it will also prevent MACsec offload in offloaded bridge devices.

/Allan

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[allan.nielsen]

Hi,

I can add some information to the HW Antoine is working on, general design of it
and the thoughts behind it. See below.

The 08/20/2019 16:41, Sabrina Dubroca wrote:
> 2019-08-20, 12:01:40 +0200, Antoine Tenart wrote:
> > So it seems the ability to enable or disable the offloading on a given
> > interface is the main missing feature. I'll add that, however I'll
> > probably (at least at first):
> > 
> > - Have the interface to be fully offloaded or fully handled in s/w (with
> >   errors being thrown if a given configuration isn't supported). Having
> >   both at the same time on a given interface would be tricky because of
> >   the MACsec validation parameter.
> > 
> > - Won't allow to enable/disable the offloading of there are rules in
> >   place, as we're not sure the same rules would be accepted by the other
> >   implementation.
> 
> That's probably quite problematic actually, because to do that you
> need to be able to resync the state between software and hardware,
> particularly packet numbers. So, yeah, we're better off having it
> completely blocked until we have a working implementation (if that
> ever happens).
> 
> However, that means in case the user wants to set up something that's
> not offloadable, they'll have to:
>  - configure the offloaded version until EOPNOTSUPP
>  - tear everything down
>  - restart from scratch without offloading
> 
> That's inconvenient.
> 
> Talking about packet numbers, can you describe how PN exhaustion is
> handled?  I couldn't find much about packet numbers at all in the
> driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
> the same SA).
New SA's are suppose to be installed ahead of time. The HW will automatic move
to the next SA and reset the PN.

> At some point userspace needs to know that we're
> getting close to 2^32 and that it's time to re-key.  Since the whole
> TX path of the software implementation is bypassed, it looks like the
> PN (as far as drivers/net/macsec.c is concerned) never increases, so
> userspace can't know when to negotiate a new SA.
> 
> > I'm not sure if we should allow to mix the implementations on a given
> > physical interface (by having two MACsec interfaces attached) as the
> > validation would be impossible to do (we would have no idea if a
> > packet was correctly handled by the offloading part or just not being
> > a MACsec packet in the first place, in Rx).
> 
> That's something that really bothers me with this proposal. Quoting
> from the commit message:
> 
> > the packets seen by the networking stack on both the physical and
> > MACsec virtual interface are exactly the same
> 
> If the HW/driver is expected to strip the sectag, I don't see how we
> could ever have multiple secy's at the same time and demultiplex
> properly between them. That's part of the reason why I chose to have
> virtual interfaces: without them, picking the right secy on TX gets
> weird.

The HW does frame clasification, and use the claisfication to associate frames
to a given secy.

We we in SW have eth0, with 2 vlan-sub interfaces, and enable macsec on those,
then we have:

eth0
eth0.10
eth0.10.macsec
eth0.20
eth0.20.macsec

In this case the HW needs to be configured to match vlan 10 to one secy, and
vlan 20 to an other one.

This is nor supported in the current patch, but is something we can add later.
We just wanted to get the basic functionallity done right before moving on to
this.

But in the current design, there is nothing that prevent us from adding this.

If anyone is interested in the details of this then it is described in section
3.6.3 in http://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10455.pdf

It is possible to construct an encapsulation that the HW can not classify
correctly. If that is the case, then we should reject the HW offload, and use
the SW.

But it is a good point, which I think is missing. We should properly reject
MACsec HW offload (with this driver, in this state) on virtual interfaces, if
the encapsulation can not be handled (could start by reject all virtual
interfaces)

> AFAICT, it means that any filters (tc, tcpdump) need to be different
> between offloaded and non-offloaded cases.
It will see the result of the offloaded operation. But I guess that this is no
different from when 'tc' operations are offloaded to HW.

> How does the driver distinguish traffic that should pass through unmodified
> from traffic that the HW needs to encapsulate and encrypt?
It relay on frame classification (I think Antoine is missing a flow
configuration to bypass all MKA traffic).

> If you look at IPsec offloading, the networking stack builds up the
> ESP header, and passes the unencrypted data down to the driver. I'm
> wondering if the same would be possible with MACsec offloading: the
> macsec virtual interface adds the header (and maybe a dummy ICV), and
> then the HW does the encryption. In case of HW that needs to add the
> sectag itself, the driver would first strip the headers that the stack
> created. On receive, the driver would recreate a sectag and the macsec
> interface would just skip all verification (decrypt, PN).
I do not think this is possible with this HW, nor do I think this is desirable.

One of the big differences between MACsec and IPsec, is the fact that it is a L2
protocol, designed to be running on switches (this is how MACsec claims to limit
key-distributions issues, as all frames are decrypted when entering a switch,
and encrypted with a new key when leaving).

If a SW stack needs to add a tag to the frame, then we cannot offload traffic
being bridged in HW, and never goes to the CPU.

/Allan

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Antoine Tenart]

Hi Sabrina,

On Tue, Aug 20, 2019 at 04:41:19PM +0200, Sabrina Dubroca wrote:
> 2019-08-20, 12:01:40 +0200, Antoine Tenart wrote:
> > So it seems the ability to enable or disable the offloading on a given
> > interface is the main missing feature. I'll add that, however I'll
> > probably (at least at first):
> > 
> > - Have the interface to be fully offloaded or fully handled in s/w (with
> >   errors being thrown if a given configuration isn't supported). Having
> >   both at the same time on a given interface would be tricky because of
> >   the MACsec validation parameter.
> > 
> > - Won't allow to enable/disable the offloading of there are rules in
> >   place, as we're not sure the same rules would be accepted by the other
> >   implementation.
> 
> That's probably quite problematic actually, because to do that you
> need to be able to resync the state between software and hardware,
> particularly packet numbers. So, yeah, we're better off having it
> completely blocked until we have a working implementation (if that
> ever happens).
> 
> However, that means in case the user wants to set up something that's
> not offloadable, they'll have to:
>  - configure the offloaded version until EOPNOTSUPP
>  - tear everything down
>  - restart from scratch without offloading
> 
> That's inconvenient.

That's right, the user might have to replay the whole configuration if
on rule failed to match the h/w requirements. It's inconvenient, but I
think it's better to be safe until we have (if that happens) a working
implementation of synchronizing the rules' state.

> Talking about packet numbers, can you describe how PN exhaustion is
> handled?  I couldn't find much about packet numbers at all in the
> driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
> the same SA).  At some point userspace needs to know that we're
> getting close to 2^32 and that it's time to re-key.  Since the whole
> TX path of the software implementation is bypassed, it looks like the
> PN (as far as drivers/net/macsec.c is concerned) never increases, so
> userspace can't know when to negotiate a new SA.

That's a very good point. It actually was on my todo list for the next
version (I wanted to discuss the other points first). We would also
need to sync the stats at some point.

> > I'm not sure if we should allow to mix the implementations on a given
> > physical interface (by having two MACsec interfaces attached) as the
> > validation would be impossible to do (we would have no idea if a
> > packet was correctly handled by the offloading part or just not being
> > a MACsec packet in the first place, in Rx).
> 
> That's something that really bothers me with this proposal. Quoting
> from the commit message:
> 
> > the packets seen by the networking stack on both the physical and
> > MACsec virtual interface are exactly the same

That bothers me as well.

> If the HW/driver is expected to strip the sectag, I don't see how we
> could ever have multiple secy's at the same time and demultiplex
> properly between them. That's part of the reason why I chose to have
> virtual interfaces: without them, picking the right secy on TX gets
> weird.
> 
> AFAICT, it means that any filters (tc, tcpdump) need to be different
> between offloaded and non-offloaded cases.
> 
> And it's going to be confusing to the administrator when they look at
> tcpdumps expecting to see MACsec frames.

Right. I did not see how *not* to strip the sectag in the h/w back then,
I'll have another look because that would improve things a lot.

@all: do other MACsec offloading hardware allow not to stip the sectag?

> I don't see how this implementation handles non-macsec traffic (on TX,
> that would be packets sent directly through the real interface, for
> example by wpa_supplicant - on RX, incoming MKA traffic for
> wpa_supplicant). Unless I missed something, incoming MKA traffic will
> end up on the macsec interface as well as the lower interface (not
> entirely critical, as long as wpa_supplicant can grab it on the lower
> device, but not consistent with the software implementation).

That's right, as we have no way to tell if an Rx packet was MACsec or
non-MACsec traffic, both will end up on both interfaces. Some h/w may be
able to insert a custom header (or may allow not to strip the sectag),
but I did not find anything related to this in mine (I'll double check).

> How does the driver distinguish traffic that should pass through
> unmodified from traffic that the HW needs to encapsulate and encrypt?

At least in PHYs, packets go in a classification unit (that can match on
multiple parts of the packet, given the hardware capabilities, eg. the
MAC addresses). The result of the match is an action, which can be
"bypass the MACsec block" or "go through it (which links the packet to a
given configuration)". This is done in Rx and in Tx, and this is how the
h/w block will know what to encapsulate and encrypt.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Igor Russkikh]

> Right. I did not see how *not* to strip the sectag in the h/w back then,
> I'll have another look because that would improve things a lot.
> 
> @all: do other MACsec offloading hardware allow not to stip the sectag?

I've just checked this, and see an action option in our HW classifier to keep
macsec header with optional error information added. But we've never
experimented configuring this honestly, I don't think we should rely in general
design that such a feature is widely available in HW.

Regards,
  Igor

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Sabrina Dubroca]

2019-08-13, 10:58:17 +0200, Antoine Tenart wrote:
> Hi Igor,
> 
> On Sat, Aug 10, 2019 at 01:20:32PM +0000, Igor Russkikh wrote:
> > On 08.08.2019 17:05, Antoine Tenart wrote:
> > 
> > > The Rx and TX handlers are modified to take in account the special case
> > > were the MACsec transformation happens in the hardware, whether in a PHY
> > > or in a MAC, as the packets seen by the networking stack on both the
> > 
> > Don't you think we may eventually may need xmit / handle_frame ops to be
> > a part of macsec_ops?
> > 
> > That way software macsec could be extract to just another type of offload.
> > The drawback of current code is it doesn't show explicitly the path of
> > offloaded packets. It is hidden in `handle_not_macsec` and in
> > `macsec_start_xmit` branch. This makes incorrect counters to tick (see my below
> > comment)
> > 
> > Another thing is that both xmit / macsec_handle_frame can't now be customized
> > by device driver. But this may be required.
> > We for example have usecases and HW features to allow specific flows to bypass
> > macsec encryption. This is normally used for macsec key control protocols,
> > identified by ethertype. Your phy is also capable on that as I see.
> 
> I think this question is linked to the use of a MACsec virtual interface
> when using h/w offloading. The starting point for me was that I wanted
> to reuse the data structures and the API exposed to the userspace by the
> s/w implementation of MACsec. I then had two choices: keeping the exact
> same interface for the user (having a virtual MACsec interface), or

Unless it's really infeasible, yes, that's how things should be done IMO.

> registering the MACsec genl ops onto the real net devices (and making
> the s/w implementation a virtual net dev and a provider of the MACsec
> "offloading" ops).

Please, no :( Let's keep it as close as possible to the software
implementation, unless there's a really good reason not to. It's not
just "ip macsec" btw, wpa_supplicant can also configure MACsec and
would also need some logic to pick the device on which to do the genl
operations in that case.

> The advantages of the first option were that nearly all the logic of the
> s/w implementation could be kept and especially that it would be
> transparent for the user to use both implementations of MACsec. But this
> raised an issue as I had to modify the xmit / handle_frame ops to let
> all the traffic pass. This is because we have no way of knowing if a
> frame was handled by the MACsec h/w or not in ingress. So the virtual
> interface here only serve as the entrypoint for the API...

It's also the interface on which you'll run DHCP or install IP addresses.

> The second option would have the advantage to better represent the actual
> flow, but the way of configuring MACsec would be a bit different for the
> user, whether he wants to use s/w or h/w MACsec. If we were to do this I
> think we could extract the genl functions from the MACsec s/w
> implementation, and let it implement the MACsec ops (exactly as the
> offloading drivers).
> 
> I'm open to discussing this :)
> 
> As for the need for xmit / handle_frame ops (for a MAC w/ MACsec
> offloading), I'd say the xmit / handle_frame ops of the real net device
> driver could be used as the one of the MACsec virtual interface do not
> do much (regardless of the implementation choice discussed above).

There's no "handle_frame" op on a real device. macsec_handle_frame is
an rx_handler specificity that grabs packets from a real device and
sends them into a virtual device stacked on top of it. A real device
just hands packets over to the stack via NAPI.


> > > @@ -2546,11 +2814,15 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
> > >  {
> > >  	struct macsec_dev *macsec = netdev_priv(dev);
> > >  	struct macsec_secy *secy = &macsec->secy;
> > > +	struct macsec_tx_sc *tx_sc = &secy->tx_sc;
> > >  	struct pcpu_secy_stats *secy_stats;
> > > +	struct macsec_tx_sa *tx_sa;
> > >  	int ret, len;
> > >  
> > > +	tx_sa = macsec_txsa_get(tx_sc->sa[tx_sc->encoding_sa]);
> > 
> > Declared, but not used?
> 
> I'll remove it then.

That's also a refcount leak, so, yes, please get rid of it.

[I'll answer the rest of the patch separately]

-- 
Sabrina

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Antoine Tenart]

Hi Sabrina,

On Fri, Aug 16, 2019 at 03:25:00PM +0200, Sabrina Dubroca wrote:
> 2019-08-13, 10:58:17 +0200, Antoine Tenart wrote:
> > 
> > As for the need for xmit / handle_frame ops (for a MAC w/ MACsec
> > offloading), I'd say the xmit / handle_frame ops of the real net device
> > driver could be used as the one of the MACsec virtual interface do not
> > do much (regardless of the implementation choice discussed above).
> 
> There's no "handle_frame" op on a real device. macsec_handle_frame is
> an rx_handler specificity that grabs packets from a real device and
> sends them into a virtual device stacked on top of it. A real device
> just hands packets over to the stack via NAPI.

Right, so if there is a need for a custom implementation of xmit /
hamdle_frame we could let the offloading driver provide it. I'll
probably let the first MAC implementation add it, as we agreed not to
add an API with no user (and mine is done in the PHY).

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Andrew Lunn]

On Thu, Aug 08, 2019 at 04:05:57PM +0200, Antoine Tenart wrote:
> This patch introduces the MACsec hardware offloading infrastructure.
> 
> The main idea here is to re-use the logic and data structures of the
> software MACsec implementation. This allows not to duplicate definitions
> and structure storing the same kind of information. It also allows to
> use a unified genlink interface for both MACsec implementations (so that
> the same userspace tool, `ip macsec`, is used with the same arguments).
> The MACsec offloading support cannot be disabled if an interface
> supports it at the moment.
> 
> The MACsec configuration is passed to device drivers supporting it
> through macsec_hw_offload() which is called from the MACsec genl
> helpers. This function calls the macsec ops of PHY and Ethernet
> drivers in two steps

Hi Antoine, Igor

It is great that you are thinking how a MAC driver would make use of
this. But on the flip side, we don't usual add an API unless there is
a user. And as far as i see, you only add a PHY level implementation,
not a MAC level.

Igor, what is your interest here? I know the Aquantia PHY can do
MACsec, but i guess you are more interested in the atlantic and AQC111
MAC drivers which hide the PHY behind firmware rather than make use of
the Linux aquantia PHY driver. Are you likely to be contributing a MAC
driver level implementation of MACsec soon?

Thanks
       Andrew

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Antoine Tenart]

Hi Andrew,

On Sat, Aug 10, 2019 at 06:34:23PM +0200, Andrew Lunn wrote:
> On Thu, Aug 08, 2019 at 04:05:57PM +0200, Antoine Tenart wrote:
> > This patch introduces the MACsec hardware offloading infrastructure.
> > 
> > The main idea here is to re-use the logic and data structures of the
> > software MACsec implementation. This allows not to duplicate definitions
> > and structure storing the same kind of information. It also allows to
> > use a unified genlink interface for both MACsec implementations (so that
> > the same userspace tool, `ip macsec`, is used with the same arguments).
> > The MACsec offloading support cannot be disabled if an interface
> > supports it at the moment.
> > 
> > The MACsec configuration is passed to device drivers supporting it
> > through macsec_hw_offload() which is called from the MACsec genl
> > helpers. This function calls the macsec ops of PHY and Ethernet
> > drivers in two steps
> 
> It is great that you are thinking how a MAC driver would make use of
> this. But on the flip side, we don't usual add an API unless there is
> a user. And as far as i see, you only add a PHY level implementation,
> not a MAC level.

That's right, and the only modification here is a simple patch adding
the MACsec ops within struct net_device. I can remove it as we do not
have providers as of now and it can be added easily later on.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Igor Russkikh]

On 10.08.2019 19:34, Andrew Lunn wrote:
> On Thu, Aug 08, 2019 at 04:05:57PM +0200, Antoine Tenart wrote:

>> The MACsec configuration is passed to device drivers supporting it
>> through macsec_hw_offload() which is called from the MACsec genl
>> helpers. This function calls the macsec ops of PHY and Ethernet
>> drivers in two steps
> 
> Hi Antoine, Igor
> 
> It is great that you are thinking how a MAC driver would make use of
> this. But on the flip side, we don't usual add an API unless there is
> a user. And as far as i see, you only add a PHY level implementation,
> not a MAC level.
> 
> Igor, what is your interest here? I know the Aquantia PHY can do
> MACsec, but i guess you are more interested in the atlantic and AQC111
> MAC drivers which hide the PHY behind firmware rather than make use of
> the Linux aquantia PHY driver. Are you likely to be contributing a MAC
> driver level implementation of MACsec soon?

Hi Andrew,

Yes, we are interested in MAC level MACSec offload implementation.
Although in our solution macsec engine itself is in Phy, we do
actively use firmware support in areas of configuration, interrupt management.

So from SW perspective that'll be MAC driver level macsec.

Regards,
  Igor

[PATCH net-next v2 7/9] net: phy: export __phy_read_page/__phy_write_page

[Antoine Tenart]

This patch exports the __phy_read_page and __phy_write_page helpers, to
allow reading and setting the current page when a function already holds
the MDIO lock.

This is something the Microsemi PHY driver does during its
initialization because parts of its registers and engines are shared
between ports. With the upcoming MACsec offloading support in this PHY,
we'll need to configure the MACsec engine and to do so changing pages is
required.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/phy-core.c | 6 ++++--
 include/linux/phy.h        | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 16667fbac8bf..09621193ba2c 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -648,15 +648,17 @@ int phy_modify_mmd(struct phy_device *phydev, int devad, u32 regnum,
 }
 EXPORT_SYMBOL_GPL(phy_modify_mmd);
 
-static int __phy_read_page(struct phy_device *phydev)
+int __phy_read_page(struct phy_device *phydev)
 {
 	return phydev->drv->read_page(phydev);
 }
+EXPORT_SYMBOL_GPL(__phy_read_page);
 
-static int __phy_write_page(struct phy_device *phydev, int page)
+int __phy_write_page(struct phy_device *phydev, int page)
 {
 	return phydev->drv->write_page(phydev, page);
 }
+EXPORT_SYMBOL_GPL(__phy_write_page);
 
 /**
  * phy_save_page() - take the bus lock and save the current page
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6947a19587e4..a64b99158b38 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -792,6 +792,9 @@ int __phy_modify_changed(struct phy_device *phydev, u32 regnum, u16 mask,
 			 u16 set);
 int phy_modify_changed(struct phy_device *phydev, u32 regnum, u16 mask,
 		       u16 set);
+int __phy_read_page(struct phy_device *phydev);
+int __phy_write_page(struct phy_device *phydev, int page);
+
 int __phy_modify(struct phy_device *phydev, u32 regnum, u16 mask, u16 set);
 int phy_modify(struct phy_device *phydev, u32 regnum, u16 mask, u16 set);
 
-- 
2.21.0

[PATCH net-next v2 8/9] net: phy: mscc: macsec initialization

[Antoine Tenart]

This patch adds support for initializing the MACsec engine found within
the Microsemi Ocelot PHY. The engine is initialized in a passthrough
mode and does not modify any incoming or outgoing packet. But thanks to
this it now can be configured to perform MACsec transformations on
packets, which will be supported by a future patch.

The MACsec read and write functions are wrapped into two versions: one
called during the init phase, and the other one later on. This is
because the init functions in the Microsemi Ocelot PHY driver are called
while the MDIO bus lock is taken.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/mscc.c           | 403 +++++++++++++++++++++++++++++++
 drivers/net/phy/mscc_fc_buffer.h |  64 +++++
 drivers/net/phy/mscc_mac.h       | 159 ++++++++++++
 drivers/net/phy/mscc_macsec.h    | 256 ++++++++++++++++++++
 4 files changed, 882 insertions(+)
 create mode 100644 drivers/net/phy/mscc_fc_buffer.h
 create mode 100644 drivers/net/phy/mscc_mac.h
 create mode 100644 drivers/net/phy/mscc_macsec.h

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 645d354ffb48..603a3dbd83e9 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -18,6 +18,10 @@
 #include <linux/netdevice.h>
 #include <dt-bindings/net/mscc-phy-vsc8531.h>
 
+#include "mscc_macsec.h"
+#include "mscc_mac.h"
+#include "mscc_fc_buffer.h"
+
 enum rgmii_rx_clock_delay {
 	RGMII_RX_CLK_DELAY_0_2_NS = 0,
 	RGMII_RX_CLK_DELAY_0_8_NS = 1,
@@ -121,6 +125,26 @@ enum rgmii_rx_clock_delay {
 #define PHY_S6G_PLL_FSM_CTRL_DATA_POS	  8
 #define PHY_S6G_PLL_FSM_ENA_POS		  7
 
+#define MSCC_EXT_PAGE_MACSEC_17		  17
+#define MSCC_EXT_PAGE_MACSEC_18		  18
+
+#define MSCC_EXT_PAGE_MACSEC_19		  19
+#define MSCC_PHY_MACSEC_19_REG_ADDR(x)	  (x)
+#define MSCC_PHY_MACSEC_19_TARGET(x)	  ((x) << 12)
+#define MSCC_PHY_MACSEC_19_READ		  BIT(14)
+#define MSCC_PHY_MACSEC_19_CMD		  BIT(15)
+
+#define MSCC_EXT_PAGE_MACSEC_20		  20
+#define MSCC_PHY_MACSEC_20_TARGET(x)	  (x)
+enum macsec_bank {
+	FC_BUFFER   = 0x04,
+	HOST_MAC    = 0x05,
+	LINE_MAC    = 0x06,
+	IP_1588     = 0x0e,
+	MACSEC_INGR = 0x38,
+	MACSEC_EGR  = 0x3c,
+};
+
 #define MSCC_EXT_PAGE_ACCESS		  31
 #define MSCC_PHY_PAGE_STANDARD		  0x0000 /* Standard registers */
 #define MSCC_PHY_PAGE_EXTENDED		  0x0001 /* Extended registers */
@@ -128,6 +152,7 @@ enum rgmii_rx_clock_delay {
 #define MSCC_PHY_PAGE_EXTENDED_3	  0x0003 /* Extended reg - page 3 */
 #define MSCC_PHY_PAGE_EXTENDED_4	  0x0004 /* Extended reg - page 4 */
 #define MSCC_PHY_PAGE_CSR_CNTL		  MSCC_PHY_PAGE_EXTENDED_4
+#define MSCC_PHY_PAGE_MACSEC		  MSCC_PHY_PAGE_EXTENDED_4
 /* Extended reg - GPIO; this is a bank of registers that are shared for all PHYs
  * in the same package.
  */
@@ -1576,6 +1601,379 @@ static int vsc8584_config_pre_init(struct phy_device *phydev)
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_MACSEC)
+static u32 __vsc8584_macsec_phy_read(struct phy_device *phydev,
+				     enum macsec_bank bank, u32 reg, bool init)
+{
+	u32 val, val_l = 0, val_h = 0;
+	unsigned long deadline;
+	int rc;
+
+	if (!init) {
+		rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
+		if (rc < 0)
+			goto failed;
+	} else {
+		__phy_write_page(phydev, MSCC_PHY_PAGE_MACSEC);
+	}
+
+	__phy_write(phydev, MSCC_EXT_PAGE_MACSEC_20,
+		    MSCC_PHY_MACSEC_20_TARGET(bank >> 2));
+
+	if (bank >> 2 == 0x1)
+		/* non-MACsec access */
+		bank &= 0x3;
+	else
+		bank = 0;
+
+	__phy_write(phydev, MSCC_EXT_PAGE_MACSEC_19,
+		    MSCC_PHY_MACSEC_19_CMD | MSCC_PHY_MACSEC_19_READ |
+		    MSCC_PHY_MACSEC_19_REG_ADDR(reg) |
+		    MSCC_PHY_MACSEC_19_TARGET(bank));
+
+	deadline = jiffies + msecs_to_jiffies(PROC_CMD_NCOMPLETED_TIMEOUT_MS);
+	do {
+		val = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_19);
+	} while (time_before(jiffies, deadline) && !(val & MSCC_PHY_MACSEC_19_CMD));
+
+	val_l = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_17);
+	val_h = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_18);
+
+	if (!init) {
+failed:
+		phy_restore_page(phydev, rc, rc);
+	} else {
+		__phy_write_page(phydev, MSCC_PHY_PAGE_STANDARD);
+	}
+
+	return (val_h << 16) | val_l;
+}
+
+static u32 vsc8584_macsec_init_phy_read(struct phy_device *phydev,
+					enum macsec_bank bank, u32 reg)
+{
+	return __vsc8584_macsec_phy_read(phydev, bank, reg, true);
+}
+
+static u32 vsc8584_macsec_phy_read(struct phy_device *phydev,
+				   enum macsec_bank bank, u32 reg)
+{
+	return __vsc8584_macsec_phy_read(phydev, bank, reg, false);
+}
+
+static void __vsc8584_macsec_phy_write(struct phy_device *phydev,
+				       enum macsec_bank bank, u32 reg, u32 val,
+				       bool init)
+{
+	unsigned long deadline;
+	int rc;
+
+	if (!init) {
+		rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
+		if (rc < 0)
+			goto failed;
+	} else {
+		__phy_write_page(phydev, MSCC_PHY_PAGE_MACSEC);
+	}
+
+	__phy_write(phydev, MSCC_EXT_PAGE_MACSEC_20,
+		    MSCC_PHY_MACSEC_20_TARGET(bank >> 2));
+
+	if ((bank >> 2 == 0x1) || (bank >> 2 == 0x3))
+		bank &= 0x3;
+	else
+		/* MACsec access */
+		bank = 0;
+
+	__phy_write(phydev, MSCC_EXT_PAGE_MACSEC_17, (u16)val);
+	__phy_write(phydev, MSCC_EXT_PAGE_MACSEC_18, (u16)(val >> 16));
+
+	__phy_write(phydev, MSCC_EXT_PAGE_MACSEC_19,
+		    MSCC_PHY_MACSEC_19_CMD | MSCC_PHY_MACSEC_19_REG_ADDR(reg) |
+		    MSCC_PHY_MACSEC_19_TARGET(bank));
+
+	deadline = jiffies + msecs_to_jiffies(PROC_CMD_NCOMPLETED_TIMEOUT_MS);
+	do {
+		val = __phy_read(phydev, MSCC_EXT_PAGE_MACSEC_19);
+	} while (time_before(jiffies, deadline) && !(val & MSCC_PHY_MACSEC_19_CMD));
+
+	if (!init) {
+failed:
+		phy_restore_page(phydev, rc, rc);
+	} else {
+		__phy_write_page(phydev, MSCC_PHY_PAGE_STANDARD);
+	}
+}
+
+static void vsc8584_macsec_init_phy_write(struct phy_device *phydev,
+					  enum macsec_bank bank, u32 reg,
+					  u32 val)
+{
+	return __vsc8584_macsec_phy_write(phydev, bank, reg, val, true);
+}
+
+static void vsc8584_macsec_phy_write(struct phy_device *phydev,
+				     enum macsec_bank bank, u32 reg, u32 val)
+{
+	return __vsc8584_macsec_phy_write(phydev, bank, reg, val, false);
+}
+
+static void vsc8584_macsec_classification(struct phy_device *phydev,
+					  enum macsec_bank bank)
+{
+	/* enable VLAN tag parsing */
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_SAM_CP_TAG,
+				      MSCC_MS_SAM_CP_TAG_PARSE_STAG |
+				      MSCC_MS_SAM_CP_TAG_PARSE_QTAG |
+				      MSCC_MS_SAM_CP_TAG_PARSE_QINQ);
+}
+
+static void vsc8584_macsec_flow_default_action(struct phy_device *phydev,
+					       enum macsec_bank bank)
+{
+	u32 port = (bank == MACSEC_INGR) ?
+		   MSCC_MS_PORT_CONTROLLED : MSCC_MS_PORT_COMMON;
+
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_SAM_NM_FLOW_NCP,
+				      /* MACsec untagged */
+				      MSCC_MS_SAM_NM_FLOW_NCP_UNTAGGED_DROP_ACTION(MSCC_MS_ACTION_DROP) |
+				      MSCC_MS_SAM_NM_FLOW_NCP_UNTAGGED_DEST_PORT(port) |
+				      /* MACsec tagged */
+				      MSCC_MS_SAM_NM_FLOW_NCP_TAGGED_DROP_ACTION(MSCC_MS_ACTION_DROP) |
+				      MSCC_MS_SAM_NM_FLOW_NCP_TAGGED_DEST_PORT(port) |
+				      /* Bad tag */
+				      MSCC_MS_SAM_NM_FLOW_NCP_BADTAG_DROP_ACTION(MSCC_MS_ACTION_DROP) |
+				      MSCC_MS_SAM_NM_FLOW_NCP_BADTAG_DEST_PORT(port) |
+				      /* Kay tag */
+				      MSCC_MS_SAM_NM_FLOW_NCP_KAY_DROP_ACTION(MSCC_MS_ACTION_DROP) |
+				      MSCC_MS_SAM_NM_FLOW_NCP_KAY_DEST_PORT(port));
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_SAM_NM_FLOW_CP,
+				      /* MACsec untagged */
+				      MSCC_MS_SAM_NM_FLOW_CP_UNTAGGED_DROP_ACTION(MSCC_MS_ACTION_DROP) |
+				      MSCC_MS_SAM_NM_FLOW_CP_UNTAGGED_DEST_PORT(port) |
+				      /* MACsec tagged */
+				      MSCC_MS_SAM_NM_FLOW_CP_TAGGED_DROP_ACTION(MSCC_MS_ACTION_DROP) |
+				      MSCC_MS_SAM_NM_FLOW_CP_TAGGED_DEST_PORT(port) |
+				      /* Bad tag */
+				      MSCC_MS_SAM_NM_FLOW_CP_BADTAG_DROP_ACTION(MSCC_MS_ACTION_DROP) |
+				      MSCC_MS_SAM_NM_FLOW_CP_BADTAG_DEST_PORT(port) |
+				      /* Kay tag */
+				      MSCC_MS_SAM_NM_FLOW_CP_KAY_DROP_ACTION(MSCC_MS_ACTION_DROP) |
+				      MSCC_MS_SAM_NM_FLOW_CP_KAY_DEST_PORT(port));
+}
+
+static void vsc8584_macsec_integrity_checks(struct phy_device *phydev,
+					    enum macsec_bank bank)
+{
+	u32 val;
+
+	if (bank != MACSEC_INGR)
+		return;
+
+	/* Set default rules to pass unmatched frames */
+	val = vsc8584_macsec_init_phy_read(phydev, bank,
+					   MSCC_MS_PARAMS2_IG_CC_CONTROL);
+	val |= MSCC_MS_PARAMS2_IG_CC_CONTROL_NON_MATCH_CTRL_ACT |
+	       MSCC_MS_PARAMS2_IG_CC_CONTROL_NON_MATCH_ACT;
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_PARAMS2_IG_CC_CONTROL,
+				      val);
+
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_PARAMS2_IG_CP_TAG,
+				      MSCC_MS_PARAMS2_IG_CP_TAG_PARSE_STAG |
+				      MSCC_MS_PARAMS2_IG_CP_TAG_PARSE_QTAG |
+				      MSCC_MS_PARAMS2_IG_CP_TAG_PARSE_QINQ);
+}
+
+static void vsc8584_macsec_block_init(struct phy_device *phydev,
+				      enum macsec_bank bank)
+{
+	u32 val;
+	int i;
+
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_ENA_CFG,
+				      MSCC_MS_ENA_CFG_SW_RST |
+				      MSCC_MS_ENA_CFG_MACSEC_BYPASS_ENA);
+
+	/* Set the MACsec block out of s/w reset and enable clocks */
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_ENA_CFG,
+				      MSCC_MS_ENA_CFG_CLK_ENA);
+
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_STATUS_CONTEXT_CTRL,
+				      bank == MACSEC_INGR ? 0xe5880214 : 0xe5880218);
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_MISC_CONTROL,
+				      MSCC_MS_MISC_CONTROL_MC_LATENCY_FIX(bank == MACSEC_INGR ? 57 : 40) |
+				      MSCC_MS_MISC_CONTROL_XFORM_REC_SIZE(bank == MACSEC_INGR ? 1 : 2));
+
+	/* Clear the counters */
+	val = vsc8584_macsec_init_phy_read(phydev, bank, MSCC_MS_COUNT_CONTROL);
+	val |= MSCC_MS_COUNT_CONTROL_AUTO_CNTR_RESET;
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_COUNT_CONTROL, val);
+
+	/* Enable octet increment mode */
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_PP_CTRL,
+				      MSCC_MS_PP_CTRL_MACSEC_OCTET_INCR_MODE);
+
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_BLOCK_CTX_UPDATE, 0x3);
+
+	val = vsc8584_macsec_init_phy_read(phydev, bank, MSCC_MS_COUNT_CONTROL);
+	val |= MSCC_MS_COUNT_CONTROL_RESET_ALL;
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_COUNT_CONTROL, val);
+
+	/* Set the MTU */
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_NON_VLAN_MTU_CHECK,
+				      MSCC_MS_NON_VLAN_MTU_CHECK_NV_MTU_COMPARE(32761) |
+				      MSCC_MS_NON_VLAN_MTU_CHECK_NV_MTU_COMP_DROP);
+
+	for (i = 0; i < 8; i++)
+		vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_VLAN_MTU_CHECK(i),
+					      MSCC_MS_VLAN_MTU_CHECK_MTU_COMPARE(32761) |
+					      MSCC_MS_VLAN_MTU_CHECK_MTU_COMP_DROP);
+
+	if (bank == MACSEC_EGR) {
+		val = vsc8584_macsec_init_phy_read(phydev, bank, MSCC_MS_INTR_CTRL_STATUS);
+		val &= ~MSCC_MS_INTR_CTRL_STATUS_INTR_ENABLE_M;
+		vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_INTR_CTRL_STATUS, val);
+
+		vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_FC_CFG,
+					      MSCC_MS_FC_CFG_FCBUF_ENA |
+					      MSCC_MS_FC_CFG_LOW_THRESH(0x1) |
+					      MSCC_MS_FC_CFG_HIGH_THRESH(0x4) |
+					      MSCC_MS_FC_CFG_LOW_BYTES_VAL(0x4) |
+					      MSCC_MS_FC_CFG_HIGH_BYTES_VAL(0x6));
+	}
+
+	vsc8584_macsec_classification(phydev, bank);
+	vsc8584_macsec_flow_default_action(phydev, bank);
+	vsc8584_macsec_integrity_checks(phydev, bank);
+
+	/* Enable the MACsec block */
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MS_ENA_CFG,
+				      MSCC_MS_ENA_CFG_CLK_ENA |
+				      MSCC_MS_ENA_CFG_MACSEC_ENA |
+				      MSCC_MS_ENA_CFG_MACSEC_SPEED_MODE(0x5));
+}
+
+static void vsc8584_macsec_mac_init(struct phy_device *phydev,
+				    enum macsec_bank bank)
+{
+	u32 val;
+	int i;
+
+	/* Clear host & line stats */
+	for (i = 0; i < 36; i++)
+		vsc8584_macsec_init_phy_write(phydev, bank, 0x1c + i, 0);
+
+	val = vsc8584_macsec_init_phy_read(phydev, bank,
+					   MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL);
+	val &= ~MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_PAUSE_MODE_M;
+	val |= MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_PAUSE_MODE(2) |
+	       MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_PAUSE_VALUE(0xffff);
+	vsc8584_macsec_init_phy_write(phydev, bank,
+				      MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL, val);
+
+	val = vsc8584_macsec_init_phy_read(phydev, bank,
+					   MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_2);
+	val |= 0xffff;
+	vsc8584_macsec_init_phy_write(phydev, bank,
+				      MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_2, val);
+
+	val = vsc8584_macsec_init_phy_read(phydev, bank,
+					   MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL);
+	if (bank == HOST_MAC)
+		val |= MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PAUSE_TIMER_ENA |
+		       MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PAUSE_FRAME_DROP_ENA;
+	else
+		val |= MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PAUSE_REACT_ENA |
+		       MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PAUSE_FRAME_DROP_ENA |
+		       MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PAUSE_MODE |
+		       MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_EARLY_PAUSE_DETECT_ENA;
+	vsc8584_macsec_init_phy_write(phydev, bank,
+				      MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL, val);
+
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MAC_CFG_PKTINF_CFG,
+				      MSCC_MAC_CFG_PKTINF_CFG_STRIP_FCS_ENA |
+				      MSCC_MAC_CFG_PKTINF_CFG_INSERT_FCS_ENA |
+				      MSCC_MAC_CFG_PKTINF_CFG_LPI_RELAY_ENA |
+				      MSCC_MAC_CFG_PKTINF_CFG_STRIP_PREAMBLE_ENA |
+				      MSCC_MAC_CFG_PKTINF_CFG_INSERT_PREAMBLE_ENA |
+				      (bank == HOST_MAC ?
+				       MSCC_MAC_CFG_PKTINF_CFG_ENABLE_TX_PADDING : 0));
+
+	val = vsc8584_macsec_init_phy_read(phydev, bank, MSCC_MAC_CFG_MODE_CFG);
+	val &= ~MSCC_MAC_CFG_MODE_CFG_DISABLE_DIC;
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MAC_CFG_MODE_CFG, val);
+
+	val = vsc8584_macsec_init_phy_read(phydev, bank, MSCC_MAC_CFG_MAXLEN_CFG);
+	val &= ~MSCC_MAC_CFG_MAXLEN_CFG_MAX_LEN_M;
+	val |= MSCC_MAC_CFG_MAXLEN_CFG_MAX_LEN(10240);
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MAC_CFG_MAXLEN_CFG, val);
+
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MAC_CFG_ADV_CHK_CFG,
+				      MSCC_MAC_CFG_ADV_CHK_CFG_SFD_CHK_ENA |
+				      MSCC_MAC_CFG_ADV_CHK_CFG_PRM_CHK_ENA |
+				      MSCC_MAC_CFG_ADV_CHK_CFG_OOR_ERR_ENA |
+				      MSCC_MAC_CFG_ADV_CHK_CFG_INR_ERR_ENA);
+
+	val = vsc8584_macsec_init_phy_read(phydev, bank, MSCC_MAC_CFG_LFS_CFG);
+	val &= ~MSCC_MAC_CFG_LFS_CFG_LFS_MODE_ENA;
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MAC_CFG_LFS_CFG, val);
+
+	vsc8584_macsec_init_phy_write(phydev, bank, MSCC_MAC_CFG_ENA_CFG,
+				      MSCC_MAC_CFG_ENA_CFG_RX_CLK_ENA |
+				      MSCC_MAC_CFG_ENA_CFG_TX_CLK_ENA |
+				      MSCC_MAC_CFG_ENA_CFG_RX_ENA |
+				      MSCC_MAC_CFG_ENA_CFG_TX_ENA);
+}
+
+/* Must be called with mdio_lock taken */
+static int vsc8584_macsec_init(struct phy_device *phydev)
+{
+	u32 val;
+
+	vsc8584_macsec_block_init(phydev, MACSEC_INGR);
+	vsc8584_macsec_block_init(phydev, MACSEC_EGR);
+	vsc8584_macsec_mac_init(phydev, HOST_MAC);
+	vsc8584_macsec_mac_init(phydev, LINE_MAC);
+
+	vsc8584_macsec_init_phy_write(phydev, FC_BUFFER,
+				      MSCC_FCBUF_FC_READ_THRESH_CFG,
+				      MSCC_FCBUF_FC_READ_THRESH_CFG_TX_THRESH(4) |
+				      MSCC_FCBUF_FC_READ_THRESH_CFG_RX_THRESH(5));
+
+	val = vsc8584_macsec_init_phy_read(phydev, FC_BUFFER, MSCC_FCBUF_MODE_CFG);
+	val |= MSCC_FCBUF_MODE_CFG_PAUSE_GEN_ENA |
+	       MSCC_FCBUF_MODE_CFG_RX_PPM_RATE_ADAPT_ENA |
+	       MSCC_FCBUF_MODE_CFG_TX_PPM_RATE_ADAPT_ENA;
+	vsc8584_macsec_init_phy_write(phydev, FC_BUFFER, MSCC_FCBUF_MODE_CFG, val);
+
+	vsc8584_macsec_init_phy_write(phydev, FC_BUFFER, MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG,
+				      MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG_TX_THRESH(8) |
+				      MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG_TX_OFFSET(9));
+
+	val = vsc8584_macsec_init_phy_read(phydev, FC_BUFFER,
+					   MSCC_FCBUF_TX_DATA_QUEUE_CFG);
+	val &= ~(MSCC_FCBUF_TX_DATA_QUEUE_CFG_START_M |
+		 MSCC_FCBUF_TX_DATA_QUEUE_CFG_END_M);
+	val |= MSCC_FCBUF_TX_DATA_QUEUE_CFG_START(0) |
+		MSCC_FCBUF_TX_DATA_QUEUE_CFG_END(5119);
+	vsc8584_macsec_init_phy_write(phydev, FC_BUFFER,
+				      MSCC_FCBUF_TX_DATA_QUEUE_CFG, val);
+
+	val = vsc8584_macsec_init_phy_read(phydev, FC_BUFFER, MSCC_FCBUF_ENA_CFG);
+	val |= MSCC_FCBUF_ENA_CFG_TX_ENA | MSCC_FCBUF_ENA_CFG_RX_ENA;
+	vsc8584_macsec_init_phy_write(phydev, FC_BUFFER, MSCC_FCBUF_ENA_CFG, val);
+
+	val = vsc8584_macsec_init_phy_read(phydev, IP_1588,
+					   MSCC_PROC_0_IP_1588_TOP_CFG_STAT_MODE_CTL);
+	val &= ~MSCC_PROC_0_IP_1588_TOP_CFG_STAT_MODE_CTL_PROTOCOL_MODE_M;
+	val |= MSCC_PROC_0_IP_1588_TOP_CFG_STAT_MODE_CTL_PROTOCOL_MODE(4);
+	vsc8584_macsec_init_phy_write(phydev, IP_1588,
+				      MSCC_PROC_0_IP_1588_TOP_CFG_STAT_MODE_CTL, val);
+
+	return 0;
+}
+#endif /* CONFIG_MACSEC */
+
 /* Check if one PHY has already done the init of the parts common to all PHYs
  * in the Quad PHY package.
  */
@@ -1705,6 +2103,11 @@ static int vsc8584_config_init(struct phy_device *phydev)
 	if (ret)
 		goto err;
 
+	/* MACsec */
+	ret = vsc8584_macsec_init(phydev);
+	if (ret)
+		goto err;
+
 	mutex_unlock(&phydev->mdio.bus->mdio_lock);
 
 	phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD);
diff --git a/drivers/net/phy/mscc_fc_buffer.h b/drivers/net/phy/mscc_fc_buffer.h
new file mode 100644
index 000000000000..7e9c0e877895
--- /dev/null
+++ b/drivers/net/phy/mscc_fc_buffer.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Microsemi Ocelot Switch driver
+ *
+ * Copyright (C) 2019 Microsemi Corporation
+ */
+
+#ifndef _MSCC_OCELOT_FC_BUFFER_H_
+#define _MSCC_OCELOT_FC_BUFFER_H_
+
+#define MSCC_FCBUF_ENA_CFG					0x00
+#define MSCC_FCBUF_MODE_CFG					0x01
+#define MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG			0x02
+#define MSCC_FCBUF_TX_CTRL_QUEUE_CFG				0x03
+#define MSCC_FCBUF_TX_DATA_QUEUE_CFG				0x04
+#define MSCC_FCBUF_RX_DATA_QUEUE_CFG				0x05
+#define MSCC_FCBUF_TX_BUFF_XON_XOFF_THRESH_CFG			0x06
+#define MSCC_FCBUF_FC_READ_THRESH_CFG				0x07
+#define MSCC_FCBUF_TX_FRM_GAP_COMP				0x08
+
+#define MSCC_FCBUF_ENA_CFG_TX_ENA				BIT(0)
+#define MSCC_FCBUF_ENA_CFG_RX_ENA				BIT(4)
+
+#define MSCC_FCBUF_MODE_CFG_DROP_BEHAVIOUR			BIT(4)
+#define MSCC_FCBUF_MODE_CFG_PAUSE_REACT_ENA			BIT(8)
+#define MSCC_FCBUF_MODE_CFG_RX_PPM_RATE_ADAPT_ENA		BIT(12)
+#define MSCC_FCBUF_MODE_CFG_TX_PPM_RATE_ADAPT_ENA		BIT(16)
+#define MSCC_FCBUF_MODE_CFG_TX_CTRL_QUEUE_ENA			BIT(20)
+#define MSCC_FCBUF_MODE_CFG_PAUSE_GEN_ENA			BIT(24)
+#define MSCC_FCBUF_MODE_CFG_INCLUDE_PAUSE_RCVD_IN_PAUSE_GEN	BIT(28)
+
+#define MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG_TX_THRESH(x)	(x)
+#define MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG_TX_THRESH_M	GENMASK(15, 0)
+#define MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG_TX_OFFSET(x)	((x) << 16)
+#define MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG_TX_OFFSET_M	GENMASK(19, 16)
+#define MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG_RX_THRESH(x)	((x) << 20)
+#define MSCC_FCBUF_PPM_RATE_ADAPT_THRESH_CFG_RX_THRESH_M	GENMASK(31, 20)
+
+#define MSCC_FCBUF_TX_CTRL_QUEUE_CFG_START(x)			(x)
+#define MSCC_FCBUF_TX_CTRL_QUEUE_CFG_START_M			GENMASK(15, 0)
+#define MSCC_FCBUF_TX_CTRL_QUEUE_CFG_END(x)			((x) << 16)
+#define MSCC_FCBUF_TX_CTRL_QUEUE_CFG_END_M			GENMASK(31, 16)
+
+#define MSCC_FCBUF_TX_DATA_QUEUE_CFG_START(x)			(x)
+#define MSCC_FCBUF_TX_DATA_QUEUE_CFG_START_M			GENMASK(15, 0)
+#define MSCC_FCBUF_TX_DATA_QUEUE_CFG_END(x)			((x) << 16)
+#define MSCC_FCBUF_TX_DATA_QUEUE_CFG_END_M			GENMASK(31, 16)
+
+#define MSCC_FCBUF_RX_DATA_QUEUE_CFG_START(x)			(x)
+#define MSCC_FCBUF_RX_DATA_QUEUE_CFG_START_M			GENMASK(15, 0)
+#define MSCC_FCBUF_RX_DATA_QUEUE_CFG_END(x)			((x) << 16)
+#define MSCC_FCBUF_RX_DATA_QUEUE_CFG_END_M			GENMASK(31, 16)
+
+#define MSCC_FCBUF_TX_BUFF_XON_XOFF_THRESH_CFG_XOFF_THRESH(x)	(x)
+#define MSCC_FCBUF_TX_BUFF_XON_XOFF_THRESH_CFG_XOFF_THRESH_M	GENMASK(15, 0)
+#define MSCC_FCBUF_TX_BUFF_XON_XOFF_THRESH_CFG_XON_THRESH(x)	((x) << 16)
+#define MSCC_FCBUF_TX_BUFF_XON_XOFF_THRESH_CFG_XON_THRESH_M	GENMASK(31, 16)
+
+#define MSCC_FCBUF_FC_READ_THRESH_CFG_TX_THRESH(x)		(x)
+#define MSCC_FCBUF_FC_READ_THRESH_CFG_TX_THRESH_M		GENMASK(15, 0)
+#define MSCC_FCBUF_FC_READ_THRESH_CFG_RX_THRESH(x)		((x) << 16)
+#define MSCC_FCBUF_FC_READ_THRESH_CFG_RX_THRESH_M		GENMASK(31, 16)
+
+#endif
diff --git a/drivers/net/phy/mscc_mac.h b/drivers/net/phy/mscc_mac.h
new file mode 100644
index 000000000000..9420ee5175a6
--- /dev/null
+++ b/drivers/net/phy/mscc_mac.h
@@ -0,0 +1,159 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Microsemi Ocelot Switch driver
+ *
+ * Copyright (c) 2017 Microsemi Corporation
+ */
+
+#ifndef _MSCC_OCELOT_LINE_MAC_H_
+#define _MSCC_OCELOT_LINE_MAC_H_
+
+#define MSCC_MAC_CFG_ENA_CFG					0x00
+#define MSCC_MAC_CFG_MODE_CFG					0x01
+#define MSCC_MAC_CFG_MAXLEN_CFG					0x02
+#define MSCC_MAC_CFG_NUM_TAGS_CFG				0x03
+#define MSCC_MAC_CFG_TAGS_CFG					0x04
+#define MSCC_MAC_CFG_ADV_CHK_CFG				0x07
+#define MSCC_MAC_CFG_LFS_CFG					0x08
+#define MSCC_MAC_CFG_LB_CFG					0x09
+#define MSCC_MAC_CFG_PKTINF_CFG					0x0a
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL			0x0b
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_2			0x0c
+#define MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL			0x0d
+#define MSCC_MAC_PAUSE_CFG_STATE				0x0e
+#define MSCC_MAC_PAUSE_CFG_MAC_ADDRESS_LSB			0x0f
+#define MSCC_MAC_PAUSE_CFG_MAC_ADDRESS_MSB			0x10
+#define MSCC_MAC_STATUS_RX_LANE_STICKY_0			0x11
+#define MSCC_MAC_STATUS_RX_LANE_STICKY_1			0x12
+#define MSCC_MAC_STATUS_TX_MONITOR_STICKY			0x13
+#define MSCC_MAC_STATUS_TX_MONITOR_STICKY_MASK			0x14
+#define MSCC_MAC_STATUS_STICKY					0x15
+#define MSCC_MAC_STATUS_STICKY_MASK				0x16
+#define MSCC_MAC_STATS_32BIT_RX_HIH_CKSM_ERR_CNT		0x17
+#define MSCC_MAC_STATS_32BIT_RX_XGMII_PROT_ERR_CNT		0x18
+#define MSCC_MAC_STATS_32BIT_RX_SYMBOL_ERR_CNT			0x19
+#define MSCC_MAC_STATS_32BIT_RX_PAUSE_CNT			0x1a
+#define MSCC_MAC_STATS_32BIT_RX_UNSUP_OPCODE_CNT		0x1b
+#define MSCC_MAC_STATS_32BIT_RX_UC_CNT				0x1c
+#define MSCC_MAC_STATS_32BIT_RX_MC_CNT				0x1d
+#define MSCC_MAC_STATS_32BIT_RX_BC_CNT				0x1e
+#define MSCC_MAC_STATS_32BIT_RX_CRC_ERR_CNT			0x1f
+#define MSCC_MAC_STATS_32BIT_RX_UNDERSIZE_CNT			0x20
+#define MSCC_MAC_STATS_32BIT_RX_FRAGMENTS_CNT			0x21
+#define MSCC_MAC_STATS_32BIT_RX_IN_RANGE_LEN_ERR_CNT		0x22
+#define MSCC_MAC_STATS_32BIT_RX_OUT_OF_RANGE_LEN_ERR_CNT	0x23
+#define MSCC_MAC_STATS_32BIT_RX_OVERSIZE_CNT			0x24
+#define MSCC_MAC_STATS_32BIT_RX_JABBERS_CNT			0x25
+#define MSCC_MAC_STATS_32BIT_RX_SIZE64_CNT			0x26
+#define MSCC_MAC_STATS_32BIT_RX_SIZE65TO127_CNT			0x27
+#define MSCC_MAC_STATS_32BIT_RX_SIZE128TO255_CNT		0x28
+#define MSCC_MAC_STATS_32BIT_RX_SIZE256TO511_CNT		0x29
+#define MSCC_MAC_STATS_32BIT_RX_SIZE512TO1023_CNT		0x2a
+#define MSCC_MAC_STATS_32BIT_RX_SIZE1024TO1518_CNT		0x2b
+#define MSCC_MAC_STATS_32BIT_RX_SIZE1519TOMAX_CNT		0x2c
+#define MSCC_MAC_STATS_32BIT_RX_IPG_SHRINK_CNT			0x2d
+#define MSCC_MAC_STATS_32BIT_TX_PAUSE_CNT			0x2e
+#define MSCC_MAC_STATS_32BIT_TX_UC_CNT				0x2f
+#define MSCC_MAC_STATS_32BIT_TX_MC_CNT				0x30
+#define MSCC_MAC_STATS_32BIT_TX_BC_CNT				0x31
+#define MSCC_MAC_STATS_32BIT_TX_SIZE64_CNT			0x32
+#define MSCC_MAC_STATS_32BIT_TX_SIZE65TO127_CNT			0x33
+#define MSCC_MAC_STATS_32BIT_TX_SIZE128TO255_CNT		0x34
+#define MSCC_MAC_STATS_32BIT_TX_SIZE256TO511_CNT		0x35
+#define MSCC_MAC_STATS_32BIT_TX_SIZE512TO1023_CNT		0x36
+#define MSCC_MAC_STATS_32BIT_TX_SIZE1024TO1518_CNT		0x37
+#define MSCC_MAC_STATS_32BIT_TX_SIZE1519TOMAX_CNT		0x38
+#define MSCC_MAC_STATS_40BIT_RX_BAD_BYTES_CNT			0x39
+#define MSCC_MAC_STATS_40BIT_RX_BAD_BYTES_MSB_CNT		0x3a
+#define MSCC_MAC_STATS_40BIT_RX_OK_BYTES_CNT			0x3b
+#define MSCC_MAC_STATS_40BIT_RX_OK_BYTES_MSB_CNT		0x3c
+#define MSCC_MAC_STATS_40BIT_RX_IN_BYTES_CNT			0x3d
+#define MSCC_MAC_STATS_40BIT_RX_IN_BYTES_MSB_CNT		0x3e
+#define MSCC_MAC_STATS_40BIT_TX_OK_BYTES_CNT			0x3f
+#define MSCC_MAC_STATS_40BIT_TX_OK_BYTES_MSB_CNT		0x40
+#define MSCC_MAC_STATS_40BIT_TX_OUT_BYTES_CNT			0x41
+#define MSCC_MAC_STATS_40BIT_TX_OUT_BYTES_MSB_CNT		0x42
+
+#define MSCC_MAC_CFG_ENA_CFG_RX_CLK_ENA				BIT(0)
+#define MSCC_MAC_CFG_ENA_CFG_TX_CLK_ENA				BIT(4)
+#define MSCC_MAC_CFG_ENA_CFG_RX_SW_RST				BIT(8)
+#define MSCC_MAC_CFG_ENA_CFG_TX_SW_RST				BIT(12)
+#define MSCC_MAC_CFG_ENA_CFG_RX_ENA				BIT(16)
+#define MSCC_MAC_CFG_ENA_CFG_TX_ENA				BIT(20)
+
+#define MSCC_MAC_CFG_MODE_CFG_FORCE_CW_UPDATE_INTERVAL(x)	((x) << 20)
+#define MSCC_MAC_CFG_MODE_CFG_FORCE_CW_UPDATE_INTERVAL_M	GENMASK(29, 20)
+#define MSCC_MAC_CFG_MODE_CFG_FORCE_CW_UPDATE			BIT(16)
+#define MSCC_MAC_CFG_MODE_CFG_TUNNEL_PAUSE_FRAMES		BIT(14)
+#define MSCC_MAC_CFG_MODE_CFG_MAC_PREAMBLE_CFG(x)		((x) << 10)
+#define MSCC_MAC_CFG_MODE_CFG_MAC_PREAMBLE_CFG_M		GENMASK(12, 10)
+#define MSCC_MAC_CFG_MODE_CFG_MAC_IPG_CFG			BIT(6)
+#define MSCC_MAC_CFG_MODE_CFG_XGMII_GEN_MODE_ENA		BIT(4)
+#define MSCC_MAC_CFG_MODE_CFG_HIH_CRC_CHECK			BIT(2)
+#define MSCC_MAC_CFG_MODE_CFG_UNDERSIZED_FRAME_DROP_DIS		BIT(1)
+#define MSCC_MAC_CFG_MODE_CFG_DISABLE_DIC			BIT(0)
+
+#define MSCC_MAC_CFG_MAXLEN_CFG_MAX_LEN_TAG_CHK			BIT(16)
+#define MSCC_MAC_CFG_MAXLEN_CFG_MAX_LEN(x)			(x)
+#define MSCC_MAC_CFG_MAXLEN_CFG_MAX_LEN_M			GENMASK(15, 0)
+
+#define MSCC_MAC_CFG_TAGS_CFG_RSZ				0x4
+#define MSCC_MAC_CFG_TAGS_CFG_TAG_ID(x)				((x) << 16)
+#define MSCC_MAC_CFG_TAGS_CFG_TAG_ID_M				GENMASK(31, 16)
+#define MSCC_MAC_CFG_TAGS_CFG_TAG_ENA				BIT(4)
+
+#define MSCC_MAC_CFG_ADV_CHK_CFG_EXT_EOP_CHK_ENA		BIT(24)
+#define MSCC_MAC_CFG_ADV_CHK_CFG_EXT_SOP_CHK_ENA		BIT(20)
+#define MSCC_MAC_CFG_ADV_CHK_CFG_SFD_CHK_ENA			BIT(16)
+#define MSCC_MAC_CFG_ADV_CHK_CFG_PRM_SHK_CHK_DIS		BIT(12)
+#define MSCC_MAC_CFG_ADV_CHK_CFG_PRM_CHK_ENA			BIT(8)
+#define MSCC_MAC_CFG_ADV_CHK_CFG_OOR_ERR_ENA			BIT(4)
+#define MSCC_MAC_CFG_ADV_CHK_CFG_INR_ERR_ENA			BIT(0)
+
+#define MSCC_MAC_CFG_LFS_CFG_LFS_INH_TX				BIT(8)
+#define MSCC_MAC_CFG_LFS_CFG_LFS_DIS_TX				BIT(4)
+#define MSCC_MAC_CFG_LFS_CFG_LFS_UNIDIR_ENA			BIT(3)
+#define MSCC_MAC_CFG_LFS_CFG_USE_LEADING_EDGE_DETECT		BIT(2)
+#define MSCC_MAC_CFG_LFS_CFG_SPURIOUS_Q_DIS			BIT(1)
+#define MSCC_MAC_CFG_LFS_CFG_LFS_MODE_ENA			BIT(0)
+
+#define MSCC_MAC_CFG_LB_CFG_XGMII_HOST_LB_ENA			BIT(4)
+#define MSCC_MAC_CFG_LB_CFG_XGMII_PHY_LB_ENA			BIT(0)
+
+#define MSCC_MAC_CFG_PKTINF_CFG_STRIP_FCS_ENA			BIT(0)
+#define MSCC_MAC_CFG_PKTINF_CFG_INSERT_FCS_ENA			BIT(4)
+#define MSCC_MAC_CFG_PKTINF_CFG_STRIP_PREAMBLE_ENA		BIT(8)
+#define MSCC_MAC_CFG_PKTINF_CFG_INSERT_PREAMBLE_ENA		BIT(12)
+#define MSCC_MAC_CFG_PKTINF_CFG_LPI_RELAY_ENA			BIT(16)
+#define MSCC_MAC_CFG_PKTINF_CFG_LF_RELAY_ENA			BIT(20)
+#define MSCC_MAC_CFG_PKTINF_CFG_RF_RELAY_ENA			BIT(24)
+#define MSCC_MAC_CFG_PKTINF_CFG_ENABLE_TX_PADDING		BIT(25)
+#define MSCC_MAC_CFG_PKTINF_CFG_ENABLE_RX_PADDING		BIT(26)
+#define MSCC_MAC_CFG_PKTINF_CFG_ENABLE_4BYTE_PREAMBLE		BIT(27)
+#define MSCC_MAC_CFG_PKTINF_CFG_MACSEC_BYPASS_NUM_PTP_STALL_CLKS(x)	((x) << 28)
+#define MSCC_MAC_CFG_PKTINF_CFG_MACSEC_BYPASS_NUM_PTP_STALL_CLKS_M	GENMASK(30, 28)
+
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_PAUSE_VALUE(x)		((x) << 16)
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_PAUSE_VALUE_M		GENMASK(31, 16)
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_WAIT_FOR_LPI_LOW	BIT(12)
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_USE_PAUSE_STALL_ENA	BIT(8)
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_PAUSE_REPL_MODE	BIT(4)
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_PAUSE_FRC_FRAME	BIT(2)
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_PAUSE_MODE(x)		(x)
+#define MSCC_MAC_PAUSE_CFG_TX_FRAME_CTRL_PAUSE_MODE_M		GENMASK(1, 0)
+
+#define MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_EARLY_PAUSE_DETECT_ENA	BIT(16)
+#define MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PRE_CRC_MODE		BIT(20)
+#define MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PAUSE_TIMER_ENA	BIT(12)
+#define MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PAUSE_REACT_ENA	BIT(8)
+#define MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PAUSE_FRAME_DROP_ENA	BIT(4)
+#define MSCC_MAC_PAUSE_CFG_RX_FRAME_CTRL_PAUSE_MODE		BIT(0)
+
+#define MSCC_MAC_PAUSE_CFG_STATE_PAUSE_STATE			BIT(0)
+#define MSCC_MAC_PAUSE_CFG_STATE_MAC_TX_PAUSE_GEN		BIT(4)
+
+#define MSCC_PROC_0_IP_1588_TOP_CFG_STAT_MODE_CTL			0x2
+#define MSCC_PROC_0_IP_1588_TOP_CFG_STAT_MODE_CTL_PROTOCOL_MODE(x)	(x)
+#define MSCC_PROC_0_IP_1588_TOP_CFG_STAT_MODE_CTL_PROTOCOL_MODE_M	GENMASK(2, 0)
+
+#endif /* _MSCC_OCELOT_LINE_MAC_H_ */
diff --git a/drivers/net/phy/mscc_macsec.h b/drivers/net/phy/mscc_macsec.h
new file mode 100644
index 000000000000..52902669e8ca
--- /dev/null
+++ b/drivers/net/phy/mscc_macsec.h
@@ -0,0 +1,256 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Microsemi Ocelot Switch driver
+ *
+ * Copyright (c) 2018 Microsemi Corporation
+ */
+
+#ifndef _MSCC_OCELOT_MACSEC_H_
+#define _MSCC_OCELOT_MACSEC_H_
+
+#define CONTROL_TYPE_EGRESS		0x6
+#define CONTROL_TYPE_INGRESS		0xf
+#define CONTROL_IV0			BIT(5)
+#define CONTROL_IV1			BIT(6)
+#define CONTROL_IV2			BIT(7)
+#define CONTROL_UPDATE_SEQ		BIT(13)
+#define CONTROL_IV_IN_SEQ		BIT(14)
+#define CONTROL_ENCRYPT_AUTH		BIT(15)
+#define CONTROL_KEY_IN_CTX		BIT(16)
+#define CONTROL_CRYPTO_ALG(x)		((x) << 17)
+#define     CTRYPTO_ALG_AES_CTR_128	0x5
+#define     CTRYPTO_ALG_AES_CTR_192	0x6
+#define     CTRYPTO_ALG_AES_CTR_256	0x7
+#define CONTROL_DIGEST_TYPE(x)		((x) << 21)
+#define CONTROL_AUTH_ALG(x)		((x) << 23)
+#define     AUTH_ALG_AES_GHAS		0x4
+#define CONTROL_AN(x)			((x) << 26)
+#define CONTROL_SEQ_TYPE(x)		((x) << 28)
+#define CONTROL_SEQ_MASK		BIT(30)
+#define CONTROL_CONTEXT_ID		BIT(31)
+
+enum mscc_macsec_destination_ports {
+	MSCC_MS_PORT_COMMON		= 0,
+	MSCC_MS_PORT_RSVD		= 1,
+	MSCC_MS_PORT_CONTROLLED		= 2,
+	MSCC_MS_PORT_UNCONTROLLED	= 3,
+};
+
+enum mscc_macsec_drop_actions {
+	MSCC_MS_ACTION_BYPASS_CRC	= 0,
+	MSCC_MS_ACTION_BYPASS_BAD	= 1,
+	MSCC_MS_ACTION_DROP		= 2,
+	MSCC_MS_ACTION_BYPASS		= 3,
+};
+
+enum mscc_macsec_flow_types {
+	MSCC_MS_FLOW_BYPASS		= 0,
+	MSCC_MS_FLOW_DROP		= 1,
+	MSCC_MS_FLOW_INGRESS		= 2,
+	MSCC_MS_FLOW_EGRESS		= 3,
+};
+
+enum mscc_macsec_validate_levels {
+	MSCC_MS_VALIDATE_DISABLED	= 0,
+	MSCC_MS_VALIDATE_CHECK		= 1,
+	MSCC_MS_VALIDATE_STRICT		= 2,
+};
+
+#define MSCC_MS_XFORM_REC(x, y)		(((x) << 5) + (y))
+#define MSCC_MS_ENA_CFG			0x800
+#define MSCC_MS_FC_CFG			0x804
+#define MSCC_MS_SAM_MISC_MATCH(x)	(0x1004 + ((x) << 4))
+#define MSCC_MS_SAM_MATCH_SCI_LO(x)	(0x1005 + ((x) << 4))
+#define MSCC_MS_SAM_MATCH_SCI_HI(x)	(0x1006 + ((x) << 4))
+#define MSCC_MS_SAM_MASK(x)		(0x1007 + ((x) << 4))
+#define MSCC_MS_SAM_ENTRY_SET1		0x1808
+#define MSCC_MS_SAM_ENTRY_CLEAR1	0x180c
+#define MSCC_MS_SAM_FLOW_CTRL(x)	(0x1c00 + (x))
+#define MSCC_MS_SAM_CP_TAG		0x1e40
+#define MSCC_MS_SAM_NM_FLOW_NCP		0x1e51
+#define MSCC_MS_SAM_NM_FLOW_CP		0x1e52
+#define MSCC_MS_MISC_CONTROL		0x1e5f
+#define MSCC_MS_COUNT_CONTROL		0x3204
+#define MSCC_MS_PARAMS2_IG_CC_CONTROL	0x3a10
+#define MSCC_MS_PARAMS2_IG_CP_TAG	0x3a14
+#define MSCC_MS_VLAN_MTU_CHECK(x)	(0x3c40 + (x))
+#define MSCC_MS_NON_VLAN_MTU_CHECK	0x3c48
+#define MSCC_MS_PP_CTRL			0x3c4b
+#define MSCC_MS_STATUS_CONTEXT_CTRL	0x3d02
+#define MSCC_MS_INTR_CTRL_STATUS	0x3d04
+#define MSCC_MS_BLOCK_CTX_UPDATE	0x3d0c
+
+/* MACSEC_ENA_CFG */
+#define MSCC_MS_ENA_CFG_CLK_ENA				BIT(0)
+#define MSCC_MS_ENA_CFG_SW_RST				BIT(1)
+#define MSCC_MS_ENA_CFG_MACSEC_BYPASS_ENA		BIT(8)
+#define MSCC_MS_ENA_CFG_MACSEC_ENA			BIT(9)
+#define MSCC_MS_ENA_CFG_MACSEC_SPEED_MODE(x)		((x) << 10)
+#define MSCC_MS_ENA_CFG_MACSEC_SPEED_MODE_M		GENMASK(12, 10)
+
+/* MACSEC_FC_CFG */
+#define MSCC_MS_FC_CFG_FCBUF_ENA			BIT(0)
+#define MSCC_MS_FC_CFG_USE_PKT_EXPANSION_INDICATION	BIT(1)
+#define MSCC_MS_FC_CFG_LOW_THRESH(x)			((x) << 4)
+#define MSCC_MS_FC_CFG_LOW_THRESH_M			GENMASK(7, 4)
+#define MSCC_MS_FC_CFG_HIGH_THRESH(x)			((x) << 8)
+#define MSCC_MS_FC_CFG_HIGH_THRESH_M			GENMASK(11, 8)
+#define MSCC_MS_FC_CFG_LOW_BYTES_VAL(x)			((x) << 12)
+#define MSCC_MS_FC_CFG_LOW_BYTES_VAL_M			GENMASK(14, 12)
+#define MSCC_MS_FC_CFG_HIGH_BYTES_VAL(x)		((x) << 16)
+#define MSCC_MS_FC_CFG_HIGH_BYTES_VAL_M			GENMASK(18, 16)
+
+/* MACSEC_SAM_MISC_MATCH */
+#define MSCC_MS_SAM_MISC_MATCH_VLAN_VALID		BIT(0)
+#define MSCC_MS_SAM_MISC_MATCH_QINQ_FOUND		BIT(1)
+#define MSCC_MS_SAM_MISC_MATCH_STAG_VALID		BIT(2)
+#define MSCC_MS_SAM_MISC_MATCH_QTAG_VALID		BIT(3)
+#define MSCC_MS_SAM_MISC_MATCH_VLAN_UP(x)		((x) << 4)
+#define MSCC_MS_SAM_MISC_MATCH_VLAN_UP_M		GENMASK(6, 4)
+#define MSCC_MS_SAM_MISC_MATCH_CONTROL_PACKET		BIT(7)
+#define MSCC_MS_SAM_MISC_MATCH_UNTAGGED			BIT(8)
+#define MSCC_MS_SAM_MISC_MATCH_TAGGED			BIT(9)
+#define MSCC_MS_SAM_MISC_MATCH_BAD_TAG			BIT(10)
+#define MSCC_MS_SAM_MISC_MATCH_KAY_TAG			BIT(11)
+#define MSCC_MS_SAM_MISC_MATCH_SOURCE_PORT(x)		((x) << 12)
+#define MSCC_MS_SAM_MISC_MATCH_SOURCE_PORT_M		GENMASK(13, 12)
+#define MSCC_MS_SAM_MISC_MATCH_MATCH_PRIORITY(x)	((x) << 16)
+#define MSCC_MS_SAM_MISC_MATCH_MATCH_PRIORITY_M		GENMASK(19, 16)
+#define MSCC_MS_SAM_MISC_MATCH_AN(x)			((x) << 24)
+#define MSCC_MS_SAM_MISC_MATCH_TCI(x)			((x) << 26)
+
+/* MACSEC_SAM_MASK */
+#define MSCC_MS_SAM_MASK_MAC_SA_MASK(x)			(x)
+#define MSCC_MS_SAM_MASK_MAC_SA_MASK_M			GENMASK(5, 0)
+#define MSCC_MS_SAM_MASK_MAC_DA_MASK(x)			((x) << 6)
+#define MSCC_MS_SAM_MASK_MAC_DA_MASK_M			GENMASK(11, 6)
+#define MSCC_MS_SAM_MASK_MAC_ETYPE_MASK			BIT(12)
+#define MSCC_MS_SAM_MASK_VLAN_VLD_MASK			BIT(13)
+#define MSCC_MS_SAM_MASK_QINQ_FOUND_MASK		BIT(14)
+#define MSCC_MS_SAM_MASK_STAG_VLD_MASK			BIT(15)
+#define MSCC_MS_SAM_MASK_QTAG_VLD_MASK			BIT(16)
+#define MSCC_MS_SAM_MASK_VLAN_UP_MASK			BIT(17)
+#define MSCC_MS_SAM_MASK_VLAN_ID_MASK			BIT(18)
+#define MSCC_MS_SAM_MASK_SOURCE_PORT_MASK		BIT(19)
+#define MSCC_MS_SAM_MASK_CTL_PACKET_MASK		BIT(20)
+#define MSCC_MS_SAM_MASK_VLAN_UP_INNER_MASK		BIT(21)
+#define MSCC_MS_SAM_MASK_VLAN_ID_INNER_MASK		BIT(22)
+#define MSCC_MS_SAM_MASK_SCI_MASK			BIT(23)
+#define MSCC_MS_SAM_MASK_AN_MASK(x)			((x) << 24)
+#define MSCC_MS_SAM_MASK_TCI_MASK(x)			((x) << 26)
+
+/* MACSEC_SAM_FLOW_CTRL_EGR */
+#define MSCC_MS_SAM_FLOW_CTRL_FLOW_TYPE(x)		(x)
+#define MSCC_MS_SAM_FLOW_CTRL_FLOW_TYPE_M		GENMASK(1, 0)
+#define MSCC_MS_SAM_FLOW_CTRL_DEST_PORT(x)		((x) << 2)
+#define MSCC_MS_SAM_FLOW_CTRL_DEST_PORT_M		GENMASK(3, 2)
+#define MSCC_MS_SAM_FLOW_CTRL_RESV_4			BIT(4)
+#define MSCC_MS_SAM_FLOW_CTRL_FLOW_CRYPT_AUTH		BIT(5)
+#define MSCC_MS_SAM_FLOW_CTRL_DROP_ACTION(x)		((x) << 6)
+#define MSCC_MS_SAM_FLOW_CTRL_DROP_ACTION_M		GENMASK(7, 6)
+#define MSCC_MS_SAM_FLOW_CTRL_RESV_15_TO_8(x)		((x) << 8)
+#define MSCC_MS_SAM_FLOW_CTRL_RESV_15_TO_8_M		GENMASK(15, 8)
+#define MSCC_MS_SAM_FLOW_CTRL_PROTECT_FRAME		BIT(16)
+#define MSCC_MS_SAM_FLOW_CTRL_REPLAY_PROTECT		BIT(16)
+#define MSCC_MS_SAM_FLOW_CTRL_SA_IN_USE			BIT(17)
+#define MSCC_MS_SAM_FLOW_CTRL_INCLUDE_SCI		BIT(18)
+#define MSCC_MS_SAM_FLOW_CTRL_USE_ES			BIT(19)
+#define MSCC_MS_SAM_FLOW_CTRL_USE_SCB			BIT(20)
+#define MSCC_MS_SAM_FLOW_CTRL_VALIDATE_FRAMES(x)	((x) << 19)
+#define MSCC_MS_SAM_FLOW_CTRL_TAG_BYPASS_SIZE(x)	((x) << 21)
+#define MSCC_MS_SAM_FLOW_CTRL_TAG_BYPASS_SIZE_M		GENMASK(22, 21)
+#define MSCC_MS_SAM_FLOW_CTRL_RESV_23			BIT(23)
+#define MSCC_MS_SAM_FLOW_CTRL_CONFIDENTIALITY_OFFSET(x)	((x) << 24)
+#define MSCC_MS_SAM_FLOW_CTRL_CONFIDENTIALITY_OFFSET_M	GENMASK(30, 24)
+#define MSCC_MS_SAM_FLOW_CTRL_CONF_PROTECT		BIT(31)
+
+/* MACSEC_SAM_CP_TAG */
+#define MSCC_MS_SAM_CP_TAG_MAP_TBL(x)			(x)
+#define MSCC_MS_SAM_CP_TAG_MAP_TBL_M			GENMASK(23, 0)
+#define MSCC_MS_SAM_CP_TAG_DEF_UP(x)			((x) << 24)
+#define MSCC_MS_SAM_CP_TAG_DEF_UP_M			GENMASK(26, 24)
+#define MSCC_MS_SAM_CP_TAG_STAG_UP_EN			BIT(27)
+#define MSCC_MS_SAM_CP_TAG_QTAG_UP_EN			BIT(28)
+#define MSCC_MS_SAM_CP_TAG_PARSE_QINQ			BIT(29)
+#define MSCC_MS_SAM_CP_TAG_PARSE_STAG			BIT(30)
+#define MSCC_MS_SAM_CP_TAG_PARSE_QTAG			BIT(31)
+
+/* MACSEC_SAM_NM_FLOW_NCP */
+#define MSCC_MS_SAM_NM_FLOW_NCP_UNTAGGED_FLOW_TYPE(x)	(x)
+#define MSCC_MS_SAM_NM_FLOW_NCP_UNTAGGED_DEST_PORT(x)	((x) << 2)
+#define MSCC_MS_SAM_NM_FLOW_NCP_UNTAGGED_DROP_ACTION(x)	((x) << 6)
+#define MSCC_MS_SAM_NM_FLOW_NCP_TAGGED_FLOW_TYPE(x)	((x) << 8)
+#define MSCC_MS_SAM_NM_FLOW_NCP_TAGGED_DEST_PORT(x)	((x) << 10)
+#define MSCC_MS_SAM_NM_FLOW_NCP_TAGGED_DROP_ACTION(x)	((x) << 14)
+#define MSCC_MS_SAM_NM_FLOW_NCP_BADTAG_FLOW_TYPE(x)	((x) << 16)
+#define MSCC_MS_SAM_NM_FLOW_NCP_BADTAG_DEST_PORT(x)	((x) << 18)
+#define MSCC_MS_SAM_NM_FLOW_NCP_BADTAG_DROP_ACTION(x)	((x) << 22)
+#define MSCC_MS_SAM_NM_FLOW_NCP_KAY_FLOW_TYPE(x)	((x) << 24)
+#define MSCC_MS_SAM_NM_FLOW_NCP_KAY_DEST_PORT(x)	((x) << 26)
+#define MSCC_MS_SAM_NM_FLOW_NCP_KAY_DROP_ACTION(x)	((x) << 30)
+
+/* MACSEC_SAM_NM_FLOW_CP */
+#define MSCC_MS_SAM_NM_FLOW_CP_UNTAGGED_FLOW_TYPE(x)	(x)
+#define MSCC_MS_SAM_NM_FLOW_CP_UNTAGGED_DEST_PORT(x)	((x) << 2)
+#define MSCC_MS_SAM_NM_FLOW_CP_UNTAGGED_DROP_ACTION(x)	((x) << 6)
+#define MSCC_MS_SAM_NM_FLOW_CP_TAGGED_FLOW_TYPE(x)	((x) << 8)
+#define MSCC_MS_SAM_NM_FLOW_CP_TAGGED_DEST_PORT(x)	((x) << 10)
+#define MSCC_MS_SAM_NM_FLOW_CP_TAGGED_DROP_ACTION(x)	((x) << 14)
+#define MSCC_MS_SAM_NM_FLOW_CP_BADTAG_FLOW_TYPE(x)	((x) << 16)
+#define MSCC_MS_SAM_NM_FLOW_CP_BADTAG_DEST_PORT(x)	((x) << 18)
+#define MSCC_MS_SAM_NM_FLOW_CP_BADTAG_DROP_ACTION(x)	((x) << 22)
+#define MSCC_MS_SAM_NM_FLOW_CP_KAY_FLOW_TYPE(x)		((x) << 24)
+#define MSCC_MS_SAM_NM_FLOW_CP_KAY_DEST_PORT(x)		((x) << 26)
+#define MSCC_MS_SAM_NM_FLOW_CP_KAY_DROP_ACTION(x)	((x) << 30)
+
+/* MACSEC_MISC_CONTROL */
+#define MSCC_MS_MISC_CONTROL_MC_LATENCY_FIX(x)		(x)
+#define MSCC_MS_MISC_CONTROL_MC_LATENCY_FIX_M		GENMASK(5, 0)
+#define MSCC_MS_MISC_CONTROL_STATIC_BYPASS		BIT(8)
+#define MSCC_MS_MISC_CONTROL_NM_MACSEC_EN		BIT(9)
+#define MSCC_MS_MISC_CONTROL_VALIDATE_FRAMES(x)		((x) << 10)
+#define MSCC_MS_MISC_CONTROL_VALIDATE_FRAMES_M		GENMASK(11, 10)
+#define MSCC_MS_MISC_CONTROL_XFORM_REC_SIZE(x)		((x) << 24)
+#define MSCC_MS_MISC_CONTROL_XFORM_REC_SIZE_M		GENMASK(25, 24)
+
+/* MACSEC_COUNT_CONTROL */
+#define MSCC_MS_COUNT_CONTROL_RESET_ALL			BIT(0)
+#define MSCC_MS_COUNT_CONTROL_DEBUG_ACCESS		BIT(1)
+#define MSCC_MS_COUNT_CONTROL_SATURATE_CNTRS		BIT(2)
+#define MSCC_MS_COUNT_CONTROL_AUTO_CNTR_RESET		BIT(3)
+
+/* MACSEC_PARAMS2_IG_CC_CONTROL */
+#define MSCC_MS_PARAMS2_IG_CC_CONTROL_NON_MATCH_CTRL_ACT	BIT(14)
+#define MSCC_MS_PARAMS2_IG_CC_CONTROL_NON_MATCH_ACT	BIT(15)
+
+/* MACSEC_PARAMS2_IG_CP_TAG */
+#define MSCC_MS_PARAMS2_IG_CP_TAG_MAP_TBL(x)		(x)
+#define MSCC_MS_PARAMS2_IG_CP_TAG_MAP_TBL_M		GENMASK(23, 0)
+#define MSCC_MS_PARAMS2_IG_CP_TAG_DEF_UP(x)		((x) << 24)
+#define MSCC_MS_PARAMS2_IG_CP_TAG_DEF_UP_M		GENMASK(26, 24)
+#define MSCC_MS_PARAMS2_IG_CP_TAG_STAG_UP_EN		BIT(27)
+#define MSCC_MS_PARAMS2_IG_CP_TAG_QTAG_UP_EN		BIT(28)
+#define MSCC_MS_PARAMS2_IG_CP_TAG_PARSE_QINQ		BIT(29)
+#define MSCC_MS_PARAMS2_IG_CP_TAG_PARSE_STAG		BIT(30)
+#define MSCC_MS_PARAMS2_IG_CP_TAG_PARSE_QTAG		BIT(31)
+
+/* MACSEC_VLAN_MTU_CHECK */
+#define MSCC_MS_VLAN_MTU_CHECK_MTU_COMPARE(x)		(x)
+#define MSCC_MS_VLAN_MTU_CHECK_MTU_COMPARE_M		GENMASK(14, 0)
+#define MSCC_MS_VLAN_MTU_CHECK_MTU_COMP_DROP		BIT(15)
+
+/* MACSEC_NON_VLAN_MTU_CHECK */
+#define MSCC_MS_NON_VLAN_MTU_CHECK_NV_MTU_COMPARE(x)	(x)
+#define MSCC_MS_NON_VLAN_MTU_CHECK_NV_MTU_COMPARE_M	GENMASK(14, 0)
+#define MSCC_MS_NON_VLAN_MTU_CHECK_NV_MTU_COMP_DROP	BIT(15)
+
+/* MACSEC_PP_CTRL */
+#define MSCC_MS_PP_CTRL_MACSEC_OCTET_INCR_MODE		BIT(0)
+
+/* MACSEC_INTR_CTRL_STATUS */
+#define MSCC_MS_INTR_CTRL_STATUS_INTR_CLR_STATUS(x)	(x)
+#define MSCC_MS_INTR_CTRL_STATUS_INTR_CLR_STATUS_M	GENMASK(15, 0)
+#define MSCC_MS_INTR_CTRL_STATUS_INTR_ENABLE(x)		((x) << 16)
+#define MSCC_MS_INTR_CTRL_STATUS_INTR_ENABLE_M		GENMASK(31, 16)
+
+#endif
-- 
2.21.0

Re: [PATCH net-next v2 8/9] net: phy: mscc: macsec initialization

[Andrew Lunn]

> The MACsec read and write functions are wrapped into two versions: one
> called during the init phase, and the other one later on. This is
> because the init functions in the Microsemi Ocelot PHY driver are called
> while the MDIO bus lock is taken.

Hi Antoine

It is nice you have wrapped it all up, but it is still messy. Sometime
in the future, we should maybe take another look at adding the concept
of initialisation of a package, before the initialization of the PHYs
in the package.

> +static u32 __vsc8584_macsec_phy_read(struct phy_device *phydev,
> +				     enum macsec_bank bank, u32 reg, bool init)
> +{
> +	u32 val, val_l = 0, val_h = 0;
> +	unsigned long deadline;
> +	int rc;
> +
> +	if (!init) {
> +		rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
> +		if (rc < 0)
> +			goto failed;
> +	} else {
> +		__phy_write_page(phydev, MSCC_PHY_PAGE_MACSEC);
> +	}

...

> +	if (!init) {
> +failed:
> +		phy_restore_page(phydev, rc, rc);
> +	} else {
> +		__phy_write_page(phydev, MSCC_PHY_PAGE_STANDARD);
> +	}

Having the failed label inside the if is correct, but i think it is
potentially dangerous for future modifications to this function. I
would move the label before the if. I doubt it makes any difference to
the generated code, but it might prevent future bugs.

    Andrew

Re: [PATCH net-next v2 8/9] net: phy: mscc: macsec initialization

[Antoine Tenart]

Hi Andrew,

On Sat, Aug 10, 2019 at 06:53:17PM +0200, Andrew Lunn wrote:
> > The MACsec read and write functions are wrapped into two versions: one
> > called during the init phase, and the other one later on. This is
> > because the init functions in the Microsemi Ocelot PHY driver are called
> > while the MDIO bus lock is taken.
> 
> It is nice you have wrapped it all up, but it is still messy. Sometime
> in the future, we should maybe take another look at adding the concept
> of initialisation of a package, before the initialization of the PHYs
> in the package.

I agree, it's still a hack to have those read/write functions acting
differently based on an 'init' flag.

> > +static u32 __vsc8584_macsec_phy_read(struct phy_device *phydev,
> > +				     enum macsec_bank bank, u32 reg, bool init)
> > +{
> > +	u32 val, val_l = 0, val_h = 0;
> > +	unsigned long deadline;
> > +	int rc;
> > +
> > +	if (!init) {
> > +		rc = phy_select_page(phydev, MSCC_PHY_PAGE_MACSEC);
> > +		if (rc < 0)
> > +			goto failed;
> > +	} else {
> > +		__phy_write_page(phydev, MSCC_PHY_PAGE_MACSEC);
> > +	}
> 
> ...
> 
> > +	if (!init) {
> > +failed:
> > +		phy_restore_page(phydev, rc, rc);
> > +	} else {
> > +		__phy_write_page(phydev, MSCC_PHY_PAGE_STANDARD);
> > +	}
> 
> Having the failed label inside the if is correct, but i think it is
> potentially dangerous for future modifications to this function. I
> would move the label before the if. I doubt it makes any difference to
> the generated code, but it might prevent future bugs.

Right, having readable code is always better. I'll fix that.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next v2 9/9] net: phy: mscc: macsec support

[Antoine Tenart]

This patch adds MACsec support to the Microsemi Ocelot PHY, to configure
flows and transformations so that matched packets can be processed by
the MACsec engine, either at egress, or at ingress. This addition allows
a user to create an hardware accelerated virtual MACsec interface on a
port using a Microsemi Ocelot PHY.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/Kconfig       |   2 +
 drivers/net/phy/mscc.c        | 621 ++++++++++++++++++++++++++++++++++
 drivers/net/phy/mscc_macsec.h |   2 +
 3 files changed, 625 insertions(+)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 48ca213c0ada..296b29b20565 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -421,6 +421,8 @@ config MICROCHIP_T1_PHY
 
 config MICROSEMI_PHY
 	tristate "Microsemi PHYs"
+	select CRYPTO_AES
+	select CRYPTO_ECB
 	---help---
 	  Currently supports VSC8514, VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
 
diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 603a3dbd83e9..19ae897b5268 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -18,6 +18,9 @@
 #include <linux/netdevice.h>
 #include <dt-bindings/net/mscc-phy-vsc8531.h>
 
+#include <linux/scatterlist.h>
+#include <crypto/skcipher.h>
+
 #include "mscc_macsec.h"
 #include "mscc_mac.h"
 #include "mscc_fc_buffer.h"
@@ -428,6 +431,31 @@ static const struct vsc85xx_hw_stat vsc8584_hw_stats[] = {
 	},
 };
 
+#if IS_ENABLED(CONFIG_MACSEC)
+struct macsec_flow {
+	struct list_head list;
+	enum macsec_bank bank;
+	u32 index;
+	unsigned char assoc_num;
+
+	u8 key[MACSEC_KEYID_LEN];
+
+	union {
+		const struct macsec_rx_sa *rx_sa;
+		const struct macsec_tx_sa *tx_sa;
+	};
+
+	/* Matching */
+	bool tagged;
+	bool untagged;
+	bool control;
+	/* Action */
+	bool bypass;
+	bool drop;
+
+};
+#endif
+
 struct vsc8531_private {
 	int rate_magic;
 	u16 supp_led_modes;
@@ -441,6 +469,19 @@ struct vsc8531_private {
 	 * package.
 	 */
 	unsigned int base_addr;
+
+#if IS_ENABLED(CONFIG_MACSEC)
+	/* MACsec fields:
+	 * - One SecY per device (enforced at the s/w implementation level)
+	 * - macsec_flows: list of h/w flows
+	 * - ingr_flows: bitmap of ingress flows
+	 * - egr_flows: bitmap of egress flows
+	 */
+	const struct macsec_secy *secy;
+	struct list_head macsec_flows;
+	unsigned long ingr_flows;
+	unsigned long egr_flows;
+#endif
 };
 
 #ifdef CONFIG_OF_MDIO
@@ -1972,6 +2013,579 @@ static int vsc8584_macsec_init(struct phy_device *phydev)
 
 	return 0;
 }
+
+static void vsc8584_macsec_flow(struct phy_device *phydev,
+				struct macsec_flow *flow)
+{
+	struct vsc8531_private *priv = phydev->priv;
+	enum macsec_bank bank = flow->bank;
+	u32 val, match = 0, mask = 0, action = 0, idx = flow->index;
+
+	if (flow->control) {
+		match |= MSCC_MS_SAM_MISC_MATCH_CONTROL_PACKET;
+		mask |= MSCC_MS_SAM_MASK_CTL_PACKET_MASK;
+	}
+	if (flow->tagged)
+		match |= MSCC_MS_SAM_MISC_MATCH_TAGGED;
+	if (flow->untagged)
+		match |= MSCC_MS_SAM_MISC_MATCH_UNTAGGED;
+
+	if (bank == MACSEC_INGR) {
+		match |= MSCC_MS_SAM_MISC_MATCH_AN(flow->index);
+		mask |= MSCC_MS_SAM_MASK_AN_MASK(0x3);
+	}
+
+	/* If an SCI is present, the SC bit must be set */
+	if (bank == MACSEC_INGR && flow->rx_sa->sc->sci) {
+		match |= MSCC_MS_SAM_MISC_MATCH_TCI(BIT(3));
+		mask |= MSCC_MS_SAM_MASK_TCI_MASK(BIT(3)) |
+			MSCC_MS_SAM_MASK_SCI_MASK;
+
+		vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_SAM_MATCH_SCI_LO(idx),
+					 lower_32_bits(flow->rx_sa->sc->sci));
+		vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_SAM_MATCH_SCI_HI(idx),
+					 upper_32_bits(flow->rx_sa->sc->sci));
+	}
+
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_SAM_MISC_MATCH(idx), match);
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_SAM_MASK(idx), mask);
+
+	/* Action for matching packets */
+	if (flow->bypass)
+		action = MSCC_MS_FLOW_BYPASS;
+	else if (flow->drop)
+		action = MSCC_MS_FLOW_DROP;
+	else
+		action = (bank == MACSEC_INGR) ?
+			 MSCC_MS_FLOW_INGRESS : MSCC_MS_FLOW_EGRESS;
+
+	val = MSCC_MS_SAM_FLOW_CTRL_FLOW_TYPE(action) |
+	      MSCC_MS_SAM_FLOW_CTRL_DROP_ACTION(MSCC_MS_ACTION_DROP) |
+	      MSCC_MS_SAM_FLOW_CTRL_DEST_PORT(MSCC_MS_PORT_CONTROLLED);
+
+	if (bank == MACSEC_INGR) {
+		if (priv->secy->replay_protect)
+			val |= MSCC_MS_SAM_FLOW_CTRL_REPLAY_PROTECT;
+		if (priv->secy->validate_frames == MACSEC_VALIDATE_STRICT)
+			val |= MSCC_MS_SAM_FLOW_CTRL_VALIDATE_FRAMES(MSCC_MS_VALIDATE_STRICT);
+		else if (priv->secy->validate_frames == MACSEC_VALIDATE_CHECK)
+			val |= MSCC_MS_SAM_FLOW_CTRL_VALIDATE_FRAMES(MSCC_MS_VALIDATE_CHECK);
+	} else if (bank == MACSEC_EGR) {
+		if (priv->secy->protect_frames)
+			val |= MSCC_MS_SAM_FLOW_CTRL_PROTECT_FRAME;
+		if (priv->secy->tx_sc.encrypt)
+			val |= MSCC_MS_SAM_FLOW_CTRL_CONF_PROTECT;
+		if (priv->secy->tx_sc.send_sci)
+			val |= MSCC_MS_SAM_FLOW_CTRL_INCLUDE_SCI;
+	}
+
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_SAM_FLOW_CTRL(idx), val);
+}
+
+static struct macsec_flow *vsc8584_macsec_find_flow(struct macsec_context *ctx,
+						    enum macsec_bank bank)
+{
+	struct vsc8531_private *priv = ctx->phydev->priv;
+	struct macsec_flow *pos, *tmp;
+	sci_t flow_sci, sci = bank == MACSEC_INGR ?
+			      ctx->sa.rx_sa->sc->sci : priv->secy->sci;
+
+	list_for_each_entry_safe(pos, tmp, &priv->macsec_flows, list) {
+		flow_sci = pos->bank == MACSEC_INGR ?
+			   pos->rx_sa->sc->sci : priv->secy->sci;
+		if (pos->assoc_num == ctx->sa.assoc_num && flow_sci == sci &&
+		    pos->bank == bank)
+			return pos;
+	}
+
+	return ERR_PTR(-ENOENT);
+}
+
+static void vsc8584_macsec_flow_enable(struct phy_device *phydev,
+				       struct macsec_flow *flow)
+{
+	enum macsec_bank bank = flow->bank;
+	u32 val, idx = flow->index;
+	bool active = (flow->bank == MACSEC_INGR) ?
+		      flow->rx_sa->active : flow->tx_sa->active;
+
+	if (!active)
+		return;
+
+	/* Enable */
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_SAM_ENTRY_SET1, BIT(idx));
+
+	/* Set in-use */
+	val = vsc8584_macsec_phy_read(phydev, bank, MSCC_MS_SAM_FLOW_CTRL(idx));
+	val |= MSCC_MS_SAM_FLOW_CTRL_SA_IN_USE;
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_SAM_FLOW_CTRL(idx), val);
+}
+
+static void vsc8584_macsec_flow_disable(struct phy_device *phydev,
+					struct macsec_flow *flow)
+{
+	enum macsec_bank bank = flow->bank;
+	u32 val, idx = flow->index;
+
+	/* Disable */
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_SAM_ENTRY_CLEAR1, BIT(idx));
+
+	/* Clear in-use */
+	val = vsc8584_macsec_phy_read(phydev, bank, MSCC_MS_SAM_FLOW_CTRL(idx));
+	val &= ~MSCC_MS_SAM_FLOW_CTRL_SA_IN_USE;
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_SAM_FLOW_CTRL(idx), val);
+}
+
+static u32 vsc8584_macsec_flow_context_id(struct macsec_flow *flow)
+{
+	if (flow->bank == MACSEC_INGR)
+		return flow->index + MSCC_MS_MAX_FLOWS;
+
+	return flow->index;
+}
+
+/* Derive the AES key to get a key for the hash autentication */
+static int vsc8584_macsec_derive_key(const u8 key[MACSEC_KEYID_LEN],
+				     u16 key_len, u8 hkey[16])
+{
+	struct crypto_skcipher *tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
+	struct skcipher_request *req = NULL;
+	struct scatterlist src, dst;
+	DECLARE_CRYPTO_WAIT(wait);
+	u32 input[4] = {0};
+	int ret;
+
+	if (IS_ERR(tfm))
+		return PTR_ERR(tfm);
+
+	req = skcipher_request_alloc(tfm, GFP_KERNEL);
+	if (!req) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
+				      CRYPTO_TFM_REQ_MAY_SLEEP, crypto_req_done,
+				      &wait);
+	ret = crypto_skcipher_setkey(tfm, key, key_len);
+	if (ret < 0)
+		goto out;
+
+	sg_init_one(&src, input, 16);
+	sg_init_one(&dst, hkey, 16);
+	skcipher_request_set_crypt(req, &src, &dst, 16, NULL);
+
+	ret = crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
+
+out:
+	skcipher_request_free(req);
+	crypto_free_skcipher(tfm);
+	return ret;
+}
+
+static int vsc8584_macsec_transformation(struct phy_device *phydev,
+					 struct macsec_flow *flow)
+{
+	u32 rec = 0, control = 0, index = flow->index;
+	struct vsc8531_private *priv = phydev->priv;
+	enum macsec_bank bank = flow->bank;
+	u8 hkey[16];
+	int i, ret;
+	sci_t sci;
+
+	ret = vsc8584_macsec_derive_key(flow->key, priv->secy->key_len, hkey);
+	if (ret)
+		return ret;
+
+	switch (priv->secy->key_len) {
+	case 16:
+		control |= CONTROL_CRYPTO_ALG(CTRYPTO_ALG_AES_CTR_128);
+		break;
+	case 32:
+		control |= CONTROL_CRYPTO_ALG(CTRYPTO_ALG_AES_CTR_256);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	control |= (bank == MACSEC_EGR) ?
+		   (CONTROL_TYPE_EGRESS | CONTROL_AN(priv->secy->tx_sc.encoding_sa)) :
+		   (CONTROL_TYPE_INGRESS | CONTROL_SEQ_MASK);
+
+	control |= CONTROL_UPDATE_SEQ | CONTROL_ENCRYPT_AUTH | CONTROL_KEY_IN_CTX |
+		   CONTROL_IV0 | CONTROL_IV1 | CONTROL_IV_IN_SEQ |
+		   CONTROL_DIGEST_TYPE(0x2) | CONTROL_SEQ_TYPE(0x1) |
+		   CONTROL_AUTH_ALG(AUTH_ALG_AES_GHAS) | CONTROL_CONTEXT_ID;
+
+	/* Set the control word */
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_XFORM_REC(index, rec++),
+				 control);
+
+	/* Set the context ID. Must be unique. */
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_XFORM_REC(index, rec++),
+				 vsc8584_macsec_flow_context_id(flow));
+
+	/* Set the encryption/decryption key */
+	for (i = 0; i < priv->secy->key_len / sizeof(u32); i++)
+		vsc8584_macsec_phy_write(phydev, bank,
+					 MSCC_MS_XFORM_REC(index, rec++),
+					 ((u32 *)flow->key)[i]);
+
+	/* Set the authentication key */
+	for (i = 0; i < 4; i++)
+		vsc8584_macsec_phy_write(phydev, bank,
+					 MSCC_MS_XFORM_REC(index, rec++),
+					 ((u32 *)hkey)[i]);
+
+	/* Initial sequence number */
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_XFORM_REC(index, rec++),
+				 bank == MACSEC_INGR ?
+				 flow->rx_sa->next_pn : flow->tx_sa->next_pn);
+
+	if (bank == MACSEC_INGR)
+		/* Set the mask (replay window size) */
+		vsc8584_macsec_phy_write(phydev, bank,
+					 MSCC_MS_XFORM_REC(index, rec++),
+					 priv->secy->replay_window);
+
+	/* Set the input vectors */
+	sci = bank == MACSEC_INGR ? flow->rx_sa->sc->sci : priv->secy->sci;
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_XFORM_REC(index, rec++),
+				 lower_32_bits(sci));
+	vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_XFORM_REC(index, rec++),
+				 upper_32_bits(sci));
+
+	while (rec < 20)
+		vsc8584_macsec_phy_write(phydev, bank, MSCC_MS_XFORM_REC(index, rec++),
+					 0);
+	return 0;
+}
+
+static struct macsec_flow *vsc8584_macsec_alloc_flow(struct vsc8531_private *priv,
+						     enum macsec_bank bank)
+{
+	struct macsec_flow *flow;
+	unsigned long *bitmap;
+	int index;
+
+	bitmap = bank == MACSEC_INGR ? &priv->ingr_flows : &priv->egr_flows;
+	index = find_first_zero_bit(bitmap, MSCC_MS_MAX_FLOWS);
+
+	if (index == MSCC_MS_MAX_FLOWS)
+		return ERR_PTR(-ENOMEM);
+
+	flow = kzalloc(sizeof(*flow), GFP_KERNEL);
+	if (!flow)
+		return ERR_PTR(-ENOMEM);
+
+	set_bit(index, bitmap);
+	flow->index = index;
+	flow->bank = bank;
+
+	list_add_tail(&flow->list, &priv->macsec_flows);
+	return flow;
+}
+
+static void vsc8584_macsec_free_flow(struct vsc8531_private *priv,
+				     struct macsec_flow *flow)
+{
+	unsigned long *bitmap = flow->bank == MACSEC_INGR ?
+				&priv->ingr_flows : &priv->egr_flows;
+
+	list_del(&flow->list);
+	clear_bit(flow->index, bitmap);
+	kfree(flow);
+}
+
+static int vsc8584_macsec_add_flow(struct phy_device *phydev,
+				   struct macsec_flow *flow, bool update)
+{
+	int ret;
+
+	vsc8584_macsec_flow(phydev, flow);
+
+	if (update)
+		return 0;
+
+	ret = vsc8584_macsec_transformation(phydev, flow);
+	if (ret) {
+		vsc8584_macsec_free_flow(phydev->priv, flow);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void vsc8584_macsec_del_flow(struct phy_device *phydev,
+				    struct macsec_flow *flow)
+{
+	vsc8584_macsec_flow_disable(phydev, flow);
+	vsc8584_macsec_free_flow(phydev->priv, flow);
+}
+
+static int __vsc8584_macsec_add_rxsa(struct macsec_context *ctx,
+				     struct macsec_flow *flow, bool update)
+{
+	struct phy_device *phydev = ctx->phydev;
+	struct vsc8531_private *priv = phydev->priv;
+
+	if (!flow) {
+		flow = vsc8584_macsec_alloc_flow(priv, MACSEC_INGR);
+		if (IS_ERR(flow))
+			return PTR_ERR(flow);
+
+		memcpy(flow->key, ctx->sa.key, priv->secy->key_len);
+	}
+
+	flow->assoc_num = ctx->sa.assoc_num;
+	flow->rx_sa = ctx->sa.rx_sa;
+
+	/* Always match tagged packets on ingress */
+	flow->tagged = true;
+
+	if (priv->secy->validate_frames != MACSEC_VALIDATE_DISABLED)
+		flow->untagged = true;
+
+	return vsc8584_macsec_add_flow(phydev, flow, update);
+}
+
+static int __vsc8584_macsec_add_txsa(struct macsec_context *ctx,
+				     struct macsec_flow *flow, bool update)
+{
+	struct phy_device *phydev = ctx->phydev;
+	struct vsc8531_private *priv = phydev->priv;
+
+	if (!flow) {
+		flow = vsc8584_macsec_alloc_flow(priv, MACSEC_EGR);
+		if (IS_ERR(flow))
+			return PTR_ERR(flow);
+
+		memcpy(flow->key, ctx->sa.key, priv->secy->key_len);
+	}
+
+	flow->assoc_num = ctx->sa.assoc_num;
+	flow->tx_sa = ctx->sa.tx_sa;
+
+	/* Always match untagged packets on egress */
+	flow->untagged = true;
+
+	return vsc8584_macsec_add_flow(phydev, flow, update);
+}
+
+static int vsc8584_macsec_dev_open(struct macsec_context *ctx)
+{
+	struct vsc8531_private *priv = ctx->phydev->priv;
+	struct macsec_flow *flow, *tmp;
+
+	/* No operation to perform before the commit step */
+	if (ctx->prepare)
+		return 0;
+
+	list_for_each_entry_safe(flow, tmp, &priv->macsec_flows, list)
+		vsc8584_macsec_flow_enable(ctx->phydev, flow);
+
+	return 0;
+}
+
+static int vsc8584_macsec_dev_stop(struct macsec_context *ctx)
+{
+	struct vsc8531_private *priv = ctx->phydev->priv;
+	struct macsec_flow *flow, *tmp;
+
+	/* No operation to perform before the commit step */
+	if (ctx->prepare)
+		return 0;
+
+	list_for_each_entry_safe(flow, tmp, &priv->macsec_flows, list)
+		vsc8584_macsec_flow_disable(ctx->phydev, flow);
+
+	return 0;
+}
+
+static int vsc8584_macsec_add_secy(struct macsec_context *ctx)
+{
+	struct vsc8531_private *priv = ctx->phydev->priv;
+
+	if (ctx->prepare) {
+		if (priv->secy)
+			return -EEXIST;
+
+		return 0;
+	}
+
+	priv->secy = ctx->secy;
+	return 0;
+}
+
+static int vsc8584_macsec_del_secy(struct macsec_context *ctx)
+{
+	struct vsc8531_private *priv = ctx->phydev->priv;
+	struct macsec_flow *flow, *tmp;
+
+	/* No operation to perform before the commit step */
+	if (ctx->prepare)
+		return 0;
+
+	list_for_each_entry_safe(flow, tmp, &priv->macsec_flows, list)
+		vsc8584_macsec_del_flow(ctx->phydev, flow);
+
+	priv->secy = NULL;
+	return 0;
+}
+
+static int vsc8584_macsec_upd_secy(struct macsec_context *ctx)
+{
+	/* No operation to perform before the commit step */
+	if (ctx->prepare)
+		return 0;
+
+	vsc8584_macsec_del_secy(ctx);
+	return vsc8584_macsec_add_secy(ctx);
+}
+
+static int vsc8584_macsec_add_rxsc(struct macsec_context *ctx)
+{
+	/* Nothing to do */
+	return 0;
+}
+
+static int vsc8584_macsec_upd_rxsc(struct macsec_context *ctx)
+{
+	return -EOPNOTSUPP;
+}
+
+static int vsc8584_macsec_del_rxsc(struct macsec_context *ctx)
+{
+	struct vsc8531_private *priv = ctx->phydev->priv;
+	struct macsec_flow *flow, *tmp;
+
+	/* No operation to perform before the commit step */
+	if (ctx->prepare)
+		return 0;
+
+	list_for_each_entry_safe(flow, tmp, &priv->macsec_flows, list) {
+		if (flow->bank == MACSEC_INGR &&
+		    flow->rx_sa->sc->sci == ctx->rx_sc->sci)
+			vsc8584_macsec_del_flow(ctx->phydev, flow);
+	}
+
+	return 0;
+}
+
+static int vsc8584_macsec_add_rxsa(struct macsec_context *ctx)
+{
+	struct macsec_flow *flow = NULL;
+
+	if (ctx->prepare)
+		return __vsc8584_macsec_add_rxsa(ctx, flow, false);
+
+	flow = vsc8584_macsec_find_flow(ctx, MACSEC_INGR);
+	if (IS_ERR(flow))
+		return PTR_ERR(flow);
+
+	vsc8584_macsec_flow_enable(ctx->phydev, flow);
+	return 0;
+}
+
+static int vsc8584_macsec_upd_rxsa(struct macsec_context *ctx)
+{
+	struct macsec_flow *flow;
+
+	flow = vsc8584_macsec_find_flow(ctx, MACSEC_INGR);
+	if (IS_ERR(flow))
+		return PTR_ERR(flow);
+
+	if (ctx->prepare) {
+		/* Make sure the flow is disabled before updating it */
+		vsc8584_macsec_flow_disable(ctx->phydev, flow);
+
+		return __vsc8584_macsec_add_rxsa(ctx, flow, true);
+	}
+
+	vsc8584_macsec_flow_enable(ctx->phydev, flow);
+	return 0;
+}
+
+static int vsc8584_macsec_del_rxsa(struct macsec_context *ctx)
+{
+	struct macsec_flow *flow;
+
+	flow = vsc8584_macsec_find_flow(ctx, MACSEC_INGR);
+
+	if (IS_ERR(flow))
+		return PTR_ERR(flow);
+	if (ctx->prepare)
+		return 0;
+
+	vsc8584_macsec_del_flow(ctx->phydev, flow);
+	return 0;
+}
+
+static int vsc8584_macsec_add_txsa(struct macsec_context *ctx)
+{
+	struct macsec_flow *flow = NULL;
+
+	if (ctx->prepare)
+		return __vsc8584_macsec_add_txsa(ctx, flow, false);
+
+	flow = vsc8584_macsec_find_flow(ctx, MACSEC_EGR);
+	if (IS_ERR(flow))
+		return PTR_ERR(flow);
+
+	vsc8584_macsec_flow_enable(ctx->phydev, flow);
+	return 0;
+}
+
+static int vsc8584_macsec_upd_txsa(struct macsec_context *ctx)
+{
+	struct macsec_flow *flow;
+
+	flow = vsc8584_macsec_find_flow(ctx, MACSEC_EGR);
+	if (IS_ERR(flow))
+		return PTR_ERR(flow);
+
+	if (ctx->prepare) {
+		/* Make sure the flow is disabled before updating it */
+		vsc8584_macsec_flow_disable(ctx->phydev, flow);
+
+		return __vsc8584_macsec_add_txsa(ctx, flow, true);
+	}
+
+	vsc8584_macsec_flow_enable(ctx->phydev, flow);
+	return 0;
+}
+
+static int vsc8584_macsec_del_txsa(struct macsec_context *ctx)
+{
+	struct macsec_flow *flow;
+
+	flow = vsc8584_macsec_find_flow(ctx, MACSEC_EGR);
+
+	if (IS_ERR(flow))
+		return PTR_ERR(flow);
+	if (ctx->prepare)
+		return 0;
+
+	vsc8584_macsec_del_flow(ctx->phydev, flow);
+	return 0;
+}
+
+static struct macsec_ops vsc8584_macsec_ops = {
+	.mdo_dev_open = vsc8584_macsec_dev_open,
+	.mdo_dev_stop = vsc8584_macsec_dev_stop,
+	.mdo_add_secy = vsc8584_macsec_add_secy,
+	.mdo_upd_secy = vsc8584_macsec_upd_secy,
+	.mdo_del_secy = vsc8584_macsec_del_secy,
+	.mdo_add_rxsc = vsc8584_macsec_add_rxsc,
+	.mdo_upd_rxsc = vsc8584_macsec_upd_rxsc,
+	.mdo_del_rxsc = vsc8584_macsec_del_rxsc,
+	.mdo_add_rxsa = vsc8584_macsec_add_rxsa,
+	.mdo_upd_rxsa = vsc8584_macsec_upd_rxsa,
+	.mdo_del_rxsa = vsc8584_macsec_del_rxsa,
+	.mdo_add_txsa = vsc8584_macsec_add_txsa,
+	.mdo_upd_txsa = vsc8584_macsec_upd_txsa,
+	.mdo_del_txsa = vsc8584_macsec_del_txsa,
+};
 #endif /* CONFIG_MACSEC */
 
 /* Check if one PHY has already done the init of the parts common to all PHYs
@@ -2103,10 +2717,17 @@ static int vsc8584_config_init(struct phy_device *phydev)
 	if (ret)
 		goto err;
 
+#if IS_ENABLED(CONFIG_MACSEC)
 	/* MACsec */
+	INIT_LIST_HEAD(&vsc8531->macsec_flows);
+	vsc8531->secy = NULL;
+
+	phydev->macsec_ops = &vsc8584_macsec_ops;
+
 	ret = vsc8584_macsec_init(phydev);
 	if (ret)
 		goto err;
+#endif
 
 	mutex_unlock(&phydev->mdio.bus->mdio_lock);
 
diff --git a/drivers/net/phy/mscc_macsec.h b/drivers/net/phy/mscc_macsec.h
index 52902669e8ca..cf12d7967133 100644
--- a/drivers/net/phy/mscc_macsec.h
+++ b/drivers/net/phy/mscc_macsec.h
@@ -8,6 +8,8 @@
 #ifndef _MSCC_OCELOT_MACSEC_H_
 #define _MSCC_OCELOT_MACSEC_H_
 
+#define MSCC_MS_MAX_FLOWS		16
+
 #define CONTROL_TYPE_EGRESS		0x6
 #define CONTROL_TYPE_INGRESS		0xf
 #define CONTROL_IV0			BIT(5)
-- 
2.21.0

Re: [PATCH net-next v2 0/9] net: macsec: initial support for hardware offloading

[Allan W. Nielsen]

Hi Antoine,

I have done a first read through of your patch and it looks good to me.

The only thing which confused me is all the references to Ocelot.

As far as I can see, this is a driver for the vsc8584 PHY in the Viper family.
The Ocelot confusion is properly because you are developing it on an Ocelot
board. But this is actually a modded board, the official PCB 120 and PCB123 has
a different pin compatible PHY without MACsec.

FYI: In the Viper family we have VSC8575, VSC8582, VSC8584, VSC8562 and VSC8564.

VSC8575, does not have MACsec, but all other does, and they are binary
compatible (it is the same die instantiated 2 or 4 times, with or without
MACsec/SyncE).

I beleive it is only the commit comments which needs to be addressed.

/Allan



The 08/08/2019 16:05, Antoine Tenart wrote:
> External E-Mail
> 
> 
> Hello,
> 
> This series intends to add support for offloading MACsec transformations
> in hardware enabled devices. The series is divided in two parts: the
> first 6 patches add the infrastructure support to offload a MACsec
> configuration to hardware drivers; and the last 3 patches introduce the
> MACsec offloading support in the Microsemi Ocelot networking PHY, making
> it the first driver to support the MACsec hardware offloading feature.
> 
> The series can also be found at:
> https://github.com/atenart/linux/tree/net-next/macsec
> 
> MACsec hardware offloading infrastructure
> -----------------------------------------
> 
> Linux has a software implementation of the MACsec standard and so far no
> hardware offloading feature was developed and submitted. Some hardware
> engines can perform MACsec operations, such as the Intel ixgbe NIC and
> the Microsemi Ocelot PHY (the one we use in this series). This means the
> MACsec offloading infrastructure should support networking PHY and
> Ethernet drivers. A preliminary email[1] was sent about this.
> 
> The main idea here is to re-use the logic and data structures of the
> software MACsec implementation. This allows not to duplicate definitions
> and structure storing the same kind of information. It also allows to
> use a unified genlink interface for both MACsec implementations (so that
> the same userspace tool, `ip macsec`, is used with the same arguments).
> The MACsec offloading support cannot be disabled if an interface
> supports it at the moment, but this could be implemented later on if
> this is a need (we could think of something like
> `ip macsec set macsec0 offloading off`).
> 
> Because we do reuse the software implementation logic and because the
> choice was made to expose the exact same interface to the user, a
> virtual interface is created exactly as if the MACsec software
> implementation was used. This was a big question when doing this work,
> and another approach would have been to register the genl helpers for
> all MACsec implementations and to have the software one a provider (such
> as the h/w offloading device drivers are). This would mean there would
> be no way to switch between implementations in the future at runtime.
> I'm open to discuss this point as I think this is really important and
> I'm not sure what is the best solution here.
> 
> The MACsec configuration is passed to device drivers supporting it
> through MACsec ops which are called (indirectly) from the MACsec
> genl helpers. This function calls the MACsec ops of PHY and Ethernet
> drivers in two steps: a preparation one, and a commit one. The first
> step is allowed to fail and should be used to check if a provided
> configuration is compatible with the features provided by a MACsec
> engine, while the second step is not allowed to fail and should only be
> used to enable a given MACsec configuration. Two extra calls are made:
> when a virtual MACsec interface is created and when it is deleted, so
> that the hardware driver can stay in sync.
> 
> The Rx and TX handlers are modified to take in account the special case
> were the MACsec transformation happens in the hardware, whether in a PHY
> or in a MAC, as the packets seen by the networking stack on both the
> physical and MACsec virtual interface are exactly the same. This leads
> to some limitations: the hardware and software implementations can't be
> used on the same physical interface, as the policies would be impossible
> to fulfill (such as strict validation of the frames). Also only a single
> virtual MACsec interface can be attached to a physical port supporting
> hardware offloading as it would be impossible to guess onto which
> interface a given packet should go (for ingress traffic).
> 
> Another limitation as of now is that the counters and statistics are not
> reported back from the hardware to the software MACsec implementation.
> This isn't an issue when using offloaded MACsec transformations, but it
> should be added in the future so that the MACsec state can be reported
> to the user (which would also improve the debug).
> 
> [1] https://www.spinics.net/lists/netdev/msg513047.html
> 
> Microsemi Ocelot PHY MACsec support
> -----------------------------------
> 
> In order to add support for the MACsec offloading feature in the
> Microsemi Ocelot driver, the __phy_read_page and __phy_write_page
> helpers had to be exported. This is because the initialization of the
> PHY is done while holding the MDIO bus lock, and we need to change the
> page to configure the MACsec block.
> 
> The support itself is then added in two patches. The first one adds
> support for configuring the MACsec block within the PHY, so that it is
> up, running and available for future configuration, but is not doing any
> modification on the traffic passing through the PHY. The second patch
> implements the phy_device MACsec ops in the Microsemi Ocelot PHY driver,
> and introduce helpers to configure MACsec transformations and flows to
> match specific packets.
> 
> Thanks!
> Antoine
> 
> Since v1:
>   - Reworked the MACsec offloading API, moving from a single helper
>     called for all MACsec configuration operations, to a per-operation
>     function that is provided by the underlying hardware drivers.
>   - Those functions now contain a verb to describe the configuration
>     action they're offloading.
>   - Improved the error handling in the MACsec genl helpers to revert
>     the configuration to its previous state when the offloading call
>     failed.
>   - Reworked the file inclusions.
> 
> Antoine Tenart (9):
>   net: introduce the MACSEC netdev feature
>   net: macsec: move some definitions in a dedicated header
>   net: macsec: introduce the macsec_context structure
>   net: introduce MACsec ops and add a reference in net_device
>   net: phy: add MACsec ops in phy_device
>   net: macsec: hardware offloading infrastructure
>   net: phy: export __phy_read_page/__phy_write_page
>   net: phy: mscc: macsec initialization
>   net: phy: mscc: macsec support
> 
>  drivers/net/macsec.c             |  542 ++++++++++------
>  drivers/net/phy/Kconfig          |    2 +
>  drivers/net/phy/mscc.c           | 1024 ++++++++++++++++++++++++++++++
>  drivers/net/phy/mscc_fc_buffer.h |   64 ++
>  drivers/net/phy/mscc_mac.h       |  159 +++++
>  drivers/net/phy/mscc_macsec.h    |  258 ++++++++
>  drivers/net/phy/phy-core.c       |    6 +-
>  include/linux/netdev_features.h  |    3 +
>  include/linux/netdevice.h        |   31 +
>  include/linux/phy.h              |   13 +
>  include/net/macsec.h             |  203 ++++++
>  include/uapi/linux/if_macsec.h   |    3 +-
>  net/core/ethtool.c               |    1 +
>  13 files changed, 2125 insertions(+), 184 deletions(-)
>  create mode 100644 drivers/net/phy/mscc_fc_buffer.h
>  create mode 100644 drivers/net/phy/mscc_mac.h
>  create mode 100644 drivers/net/phy/mscc_macsec.h
>  create mode 100644 include/net/macsec.h
> 
> -- 
> 2.21.0
> 
> 

-- 
/Allan

Re: [PATCH net-next v2 0/9] net: macsec: initial support for hardware offloading

[Antoine Tenart]

Hi Allan,

On Fri, Aug 09, 2019 at 01:23:47PM +0200, Allan W. Nielsen wrote:
> 
> I have done a first read through of your patch and it looks good to me.
> 
> The only thing which confused me is all the references to Ocelot.
> 
> As far as I can see, this is a driver for the vsc8584 PHY in the Viper family.
> The Ocelot confusion is properly because you are developing it on an Ocelot
> board. But this is actually a modded board, the official PCB 120 and PCB123 has
> a different pin compatible PHY without MACsec.
> 
> FYI: In the Viper family we have VSC8575, VSC8582, VSC8584, VSC8562 and VSC8564.
> 
> VSC8575, does not have MACsec, but all other does, and they are binary
> compatible (it is the same die instantiated 2 or 4 times, with or without
> MACsec/SyncE).
> 
> I beleive it is only the commit comments which needs to be addressed.

That's right, I mixed up Ocelot and the actual PHY names. I'll look for
Ocelot references in the patches and I'll fix it in v3.

Thanks for spotting this,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com