Implement mdo_insert_tx_tag to insert the TLV header in the ethernet
frame.
If extscs parameter is set to 1, then the TLV header will contain the
TX SC that will be used to encrypt the frame, otherwise the TX SC will
be selected using the MAC source address.
Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
---
drivers/net/phy/nxp-c45-tja11xx-macsec.c | 66 ++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/drivers/net/phy/nxp-c45-tja11xx-macsec.c b/drivers/net/phy/nxp-c45-tja11xx-macsec.c
index edfdd2540d39..f06505c04b13 100644
--- a/drivers/net/phy/nxp-c45-tja11xx-macsec.c
+++ b/drivers/net/phy/nxp-c45-tja11xx-macsec.c
@@ -11,6 +11,7 @@
#include <linux/module.h>
#include <linux/phy.h>
#include <linux/processor.h>
+#include <net/dst_metadata.h>
#include <net/macsec.h>
#include "nxp-c45-tja11xx.h"
@@ -23,6 +24,7 @@
#define VEND1_MACSEC_BASE 0x9000
#define MACSEC_CFG 0x0000
+#define MACSEC_CFG_EXTSCS BIT(26)
#define MACSEC_CFG_BYPASS BIT(1)
#define MACSEC_CFG_S0I BIT(0)
@@ -121,6 +123,8 @@
#define ADPTR_CNTRL 0x0F00
#define ADPTR_CNTRL_CONFIG_EN BIT(14)
#define ADPTR_CNTRL_ADPTR_EN BIT(12)
+#define ADPTR_TX_TAG_CNTRL 0x0F0C
+#define ADPTR_TX_TAG_CNTRL_ENA BIT(31)
#define TX_SC_FLT_BASE 0x800
#define TX_SC_FLT_SIZE 0x10
@@ -167,6 +171,18 @@
#define MACSEC_INPBTS 0x0638
#define MACSEC_IPSNFS 0x063C
+#define TJA11XX_TLV_TX_NEEDED_HEADROOM (32)
+#define TJA11XX_TLV_NEEDED_TAILROOM (0)
+
+#define MACSEC_TLV_CP BIT(0)
+#define MACSEC_TLV_SC_ID_OFF (2)
+
+#define ETH_P_TJA11XX_TLV (0x4e58)
+
+bool extscs;
+module_param(extscs, bool, 0);
+MODULE_PARM_DESC(extscs, "Select the TX SC using TLV header information.");
+
struct nxp_c45_rx_sc {
struct macsec_rx_sc *rx_sc;
struct macsec_rx_sa *rx_sa_a;
@@ -315,6 +331,8 @@ void nxp_c45_macsec_config_init(struct phy_device *phydev)
nxp_c45_macsec_write(phydev, ADPTR_CNTRL, ADPTR_CNTRL_CONFIG_EN |
ADPTR_CNTRL_ADPTR_EN);
+ nxp_c45_macsec_write(phydev, ADPTR_TX_TAG_CNTRL,
+ ADPTR_TX_TAG_CNTRL_ENA);
nxp_c45_macsec_write(phydev, ADPTR_CNTRL, ADPTR_CNTRL_ADPTR_EN);
nxp_c45_macsec_write(phydev, MACSEC_TPNET, PN_WRAP_THRESHOLD);
@@ -324,6 +342,9 @@ void nxp_c45_macsec_config_init(struct phy_device *phydev)
nxp_c45_macsec_write(phydev, MACSEC_UPFR0M1, MACSEC_OVP);
nxp_c45_macsec_write(phydev, MACSEC_UPFR0M2, ETYPE_MASK);
nxp_c45_macsec_write(phydev, MACSEC_UPFR0R, MACSEC_UPFR_EN);
+
+ if (extscs)
+ nxp_c45_macsec_write(phydev, MACSEC_CFG, MACSEC_CFG_EXTSCS);
}
static void nxp_c45_macsec_cfg_ptp(struct phy_device *phydev, bool enable)
@@ -1741,6 +1762,48 @@ static int nxp_c45_mdo_get_rx_sa_stats(struct macsec_context *ctx)
return 0;
}
+struct tja11xx_tlv_header {
+ struct ethhdr eth;
+ u8 subtype;
+ u8 len;
+ u8 payload[28];
+};
+
+static int nxp_c45_mdo_insert_tx_tag(struct phy_device *phydev,
+ struct sk_buff *skb)
+{
+ struct nxp_c45_phy *priv = phydev->priv;
+ struct tja11xx_tlv_header *tlv;
+ struct nxp_c45_secy *phy_secy;
+ struct metadata_dst *md_dst;
+ struct ethhdr *eth;
+ sci_t sci;
+
+ eth = eth_hdr(skb);
+ tlv = skb_push(skb, TJA11XX_TLV_TX_NEEDED_HEADROOM);
+ memmove(tlv, eth, sizeof(*eth));
+ skb_reset_mac_header(skb);
+ tlv->eth.h_proto = htons(ETH_P_TJA11XX_TLV);
+ tlv->subtype = 1;
+ tlv->len = sizeof(tlv->payload);
+ memset(tlv->payload, 0, sizeof(tlv->payload));
+
+ if (!extscs)
+ return 0;
+
+ /* md_dst should be always set if MACsec is offloaded. */
+ md_dst = skb_metadata_dst(skb);
+ sci = md_dst->u.macsec_info.sci;
+ phy_secy = nxp_c45_find_secy(&priv->macsec->secy_list, sci);
+ if (IS_ERR(phy_secy))
+ return PTR_ERR(phy_secy);
+
+ tlv->payload[3] = phy_secy->secy_id << MACSEC_TLV_SC_ID_OFF |
+ MACSEC_TLV_CP;
+
+ return 0;
+}
+
static const struct macsec_ops nxp_c45_macsec_ops = {
.mdo_dev_open = nxp_c45_mdo_dev_open,
.mdo_dev_stop = nxp_c45_mdo_dev_stop,
@@ -1761,6 +1824,9 @@ static const struct macsec_ops nxp_c45_macsec_ops = {
.mdo_get_tx_sa_stats = nxp_c45_mdo_get_tx_sa_stats,
.mdo_get_rx_sc_stats = nxp_c45_mdo_get_rx_sc_stats,
.mdo_get_rx_sa_stats = nxp_c45_mdo_get_rx_sa_stats,
+ .mdo_insert_tx_tag = nxp_c45_mdo_insert_tx_tag,
+ .needed_headroom = TJA11XX_TLV_TX_NEEDED_HEADROOM,
+ .needed_tailroom = TJA11XX_TLV_NEEDED_TAILROOM,
};
int nxp_c45_macsec_probe(struct phy_device *phydev)
--
2.34.12023-08-24, 12:16:15 +0300, Radu Pirea (NXP OSS) wrote: > Implement mdo_insert_tx_tag to insert the TLV header in the ethernet > frame. > > If extscs parameter is set to 1, then the TLV header will contain the > TX SC that will be used to encrypt the frame, otherwise the TX SC will > be selected using the MAC source address. In which case would a user choose not to use the SCI? Using the MAC address is probably fine in basic setups, but having to fiddle with a module parameter (so unloading and reloading the module, which means losing network connectivity) to make things work when the setup evolves is really not convenient. Is there a drawback to always using the SCI? -- Sabrina
On 28.08.2023 13:17, Sabrina Dubroca wrote: > 2023-08-24, 12:16:15 +0300, Radu Pirea (NXP OSS) wrote: >> Implement mdo_insert_tx_tag to insert the TLV header in the ethernet >> frame. >> >> If extscs parameter is set to 1, then the TLV header will contain the >> TX SC that will be used to encrypt the frame, otherwise the TX SC will >> be selected using the MAC source address. > > In which case would a user choose not to use the SCI? Using the MAC > address is probably fine in basic setups, but having to fiddle with a > module parameter (so unloading and reloading the module, which means > losing network connectivity) to make things work when the setup > evolves is really not convenient. > > Is there a drawback to always using the SCI? > I see your concern. If the PHY driver is reloaded, then the offloaded MACsec configuration will vanish from the hardware. Actually, just a call to phy_disconnect is enough to break an offloaded MACsec iface and can be achieved by: ip link set eth0 down && ip link set eth0 up The only drawback is related to the PTP frames encryption. Due to hardware limitations, PHY timestamping + MACsec will not work if the custom header is inserted. The only way to get this work is by using the MAC SA selection and running PTP on the real netdev. -- Radu P.
2023-08-28, 16:46:02 +0300, Radu Pirea (OSS) wrote:
>
>
> On 28.08.2023 13:17, Sabrina Dubroca wrote:
> > 2023-08-24, 12:16:15 +0300, Radu Pirea (NXP OSS) wrote:
> > > Implement mdo_insert_tx_tag to insert the TLV header in the ethernet
> > > frame.
> > >
> > > If extscs parameter is set to 1, then the TLV header will contain the
> > > TX SC that will be used to encrypt the frame, otherwise the TX SC will
> > > be selected using the MAC source address.
> >
> > In which case would a user choose not to use the SCI? Using the MAC
> > address is probably fine in basic setups, but having to fiddle with a
> > module parameter (so unloading and reloading the module, which means
> > losing network connectivity) to make things work when the setup
> > evolves is really not convenient.
> >
> > Is there a drawback to always using the SCI?
> >
>
> I see your concern. If the PHY driver is reloaded, then the offloaded MACsec
> configuration will vanish from the hardware. Actually, just a call to
> phy_disconnect is enough to break an offloaded MACsec iface and can be
> achieved by:
> ip link set eth0 down && ip link set eth0 up
And it's not restored when the link goes back up? That's inconvenient :/
Do we end up with inconsistent state? ie driver and core believe
everything is still offloaded, but HW lost all state? do we leak
some resources allocated by the driver?
We could add a flush/restore in macsec_notify when the lower device
goes down/up, maybe limited to devices that request this (I don't know
if all devices would need it, or maybe all devices offloading to the
PHY but not to the MAC).
And what happens in this case?
ip link add link eth0 type macsec offload phy
ip link set eth0 down
ip macsec add macsec0 rx sci ...
ip macsec add macsec0 tx sa 0 ...
# etc
ip link set eth0 up
Will offload work with the current code?
> The only drawback is related to the PTP frames encryption. Due to hardware
> limitations, PHY timestamping + MACsec will not work if the custom header is
> inserted. The only way to get this work is by using the MAC SA selection and
> running PTP on the real netdev.
Could you add some documentation explaining that? Users need this
information to make the right choice for their use case. Maybe
directly in the description of the module parameter, something like:
"Select the TX SC using TLV header information. PTP frames encryption
cannot work when this feature is enabled."
If it's in the module parameter I guess it can't be too
verbose. Otherwise I don't know where else to put it.
And the parameter's name and/or description should probably include
macsec/MACsec if it's visible at the level of the whole module (ie if
macsec support isn't a separate module), just to give context at to
what the TXSC is (and what the encryption for the PTP frames refers
to).
--
Sabrina
On Wed, 2023-08-30 at 13:35 +0200, Sabrina Dubroca wrote:
...
> And it's not restored when the link goes back up? That's inconvenient
> :/
> Do we end up with inconsistent state? ie driver and core believe
> everything is still offloaded, but HW lost all state? do we leak
> some resources allocated by the driver?
Yes. We end up with inconsistent state. The HW will lost all state when
the phy is reseted. No resource is leaked, everything is there, but the
configuration needs to be reapplied.
>
> We could add a flush/restore in macsec_notify when the lower device
> goes down/up, maybe limited to devices that request this (I don't
> know
> if all devices would need it, or maybe all devices offloading to the
> PHY but not to the MAC).
Agreed.
We can do a flush very simple, but to restore the configuration maybe
we should to save the key in the macsec_key structure. I am not sure if
the key can be extracted from crypto_aead structure.
>
> And what happens in this case?
> ip link add link eth0 type macsec offload phy
> ip link set eth0 down
> ip macsec add macsec0 rx sci ...
> ip macsec add macsec0 tx sa 0 ...
> # etc
> ip link set eth0 up
>
> Will offload work with the current code?
(the interface was up before)
[root@alarm ~]# ip link add link end0 macsec0 type macsec encrypt on
offload phy
[root@alarm ~]# ip link set end0 down
[root@alarm ~]# ip macsec add macsec0 rx port 1 address
00:01:be:be:ef:33
RTNETLINK answers: Operation not supported
But let's consider the next case:
ip link add link eth0 type macsec offload phy
ip link set eth0 down
ip link set eth0 up
ip macsec add macsec0 rx sci ...
ip macsec add macsec0 tx sa 0 ...
# etc
In this case, any HW configuration written by .mdo_add_secy will be
lost.
>
> > The only drawback is related to the PTP frames encryption. Due to
> > hardware
> > limitations, PHY timestamping + MACsec will not work if the custom
> > header is
> > inserted. The only way to get this work is by using the MAC SA
> > selection and
> > running PTP on the real netdev.
>
> Could you add some documentation explaining that? Users need this
> information to make the right choice for their use case. Maybe
> directly in the description of the module parameter, something like:
> "Select the TX SC using TLV header information. PTP frames encryption
> cannot work when this feature is enabled."
>
> If it's in the module parameter I guess it can't be too
> verbose. Otherwise I don't know where else to put it.
>
> And the parameter's name and/or description should probably include
> macsec/MACsec if it's visible at the level of the whole module (ie if
> macsec support isn't a separate module), just to give context at to
> what the TXSC is (and what the encryption for the PTP frames refers
> to).
I will improve the comment and change the name. Thank you.
2023-09-01, 09:09:06 +0000, Radu Pirea wrote: > On Wed, 2023-08-30 at 13:35 +0200, Sabrina Dubroca wrote: > ... > > > And it's not restored when the link goes back up? That's inconvenient > > :/ > > Do we end up with inconsistent state? ie driver and core believe > > everything is still offloaded, but HW lost all state? do we leak > > some resources allocated by the driver? > > Yes. We end up with inconsistent state. The HW will lost all state when > the phy is reseted. No resource is leaked, everything is there, but the > configuration needs to be reapplied. > > > > > We could add a flush/restore in macsec_notify when the lower device > > goes down/up, maybe limited to devices that request this (I don't > > know > > if all devices would need it, or maybe all devices offloading to the > > PHY but not to the MAC). > > Agreed. > We can do a flush very simple, but to restore the configuration maybe > we should to save the key in the macsec_key structure. I am not sure if > the key can be extracted from crypto_aead structure. Either that or in the driver. I have a small preference for driver, because then cases that don't need this restore won't have to keep the key in memory, reducing the likelihood of accidentally sharing it. OTOH, if we centralize that code, it's easier to make sure everything is cleared from kernel memory when we delete the SA. > > And what happens in this case? > > ip link add link eth0 type macsec offload phy > > ip link set eth0 down > > ip macsec add macsec0 rx sci ... > > ip macsec add macsec0 tx sa 0 ... > > # etc > > ip link set eth0 up > > > > Will offload work with the current code? > > (the interface was up before) > [root@alarm ~]# ip link add link end0 macsec0 type macsec encrypt on > offload phy > [root@alarm ~]# ip link set end0 down > [root@alarm ~]# ip macsec add macsec0 rx port 1 address > 00:01:be:be:ef:33 > RTNETLINK answers: Operation not supported Where does that EOPNOTSUPP come from? nxp_c45_mdo_add_rxsc from this version of the code can't return that, and macsec_add_rxsc also shouldn't at this point. Ideally all implementations (HW or SW) should behave the same, but at least that saves us from having to restore state in the HW, if we couldn't create it at all. > But let's consider the next case: > ip link add link eth0 type macsec offload phy > ip link set eth0 down > ip link set eth0 up > ip macsec add macsec0 rx sci ... > ip macsec add macsec0 tx sa 0 ... > # etc > > In this case, any HW configuration written by .mdo_add_secy will be > lost. So we need a way to restore the config in HW, whether that's done entirely in the driver or initiated by macsec itself. Antoine, is any of this relevant to the mscc driver? -- Sabrina
On 01.09.2023 13:07, Sabrina Dubroca wrote: ... > >>> And what happens in this case? >>> ip link add link eth0 type macsec offload phy >>> ip link set eth0 down >>> ip macsec add macsec0 rx sci ... >>> ip macsec add macsec0 tx sa 0 ... >>> # etc >>> ip link set eth0 up >>> >>> Will offload work with the current code? >> >> (the interface was up before) >> [root@alarm ~]# ip link add link end0 macsec0 type macsec encrypt on >> offload phy >> [root@alarm ~]# ip link set end0 down >> [root@alarm ~]# ip macsec add macsec0 rx port 1 address >> 00:01:be:be:ef:33 >> RTNETLINK answers: Operation not supported > > Where does that EOPNOTSUPP come from? nxp_c45_mdo_add_rxsc from this > version of the code can't return that, and macsec_add_rxsc also > shouldn't at this point. This is the source of -EOPNOTSUPP https://elixir.bootlin.com/linux/latest/source/drivers/net/macsec.c#L1928 -- Radu P.
2023-09-01, 14:58:12 +0300, Radu Pirea (OSS) wrote: > On 01.09.2023 13:07, Sabrina Dubroca wrote: > > > (the interface was up before) > > > [root@alarm ~]# ip link add link end0 macsec0 type macsec encrypt on > > > offload phy > > > [root@alarm ~]# ip link set end0 down > > > [root@alarm ~]# ip macsec add macsec0 rx port 1 address > > > 00:01:be:be:ef:33 > > > RTNETLINK answers: Operation not supported > > > > Where does that EOPNOTSUPP come from? nxp_c45_mdo_add_rxsc from this > > version of the code can't return that, and macsec_add_rxsc also > > shouldn't at this point. > > This is the source of -EOPNOTSUPP > https://elixir.bootlin.com/linux/latest/source/drivers/net/macsec.c#L1928 Could you check which part of macsec_get_ops is failing? Since macsec_newlink with "offload phy" worked, macsec_check_offload shouldn't fail, so why does macsec_get_ops return NULL? real_dev->phydev was NULL'ed? -- Sabrina
On 01.09.2023 16:57, Sabrina Dubroca wrote: > 2023-09-01, 14:58:12 +0300, Radu Pirea (OSS) wrote: >> On 01.09.2023 13:07, Sabrina Dubroca wrote: >>>> (the interface was up before) >>>> [root@alarm ~]# ip link add link end0 macsec0 type macsec encrypt on >>>> offload phy >>>> [root@alarm ~]# ip link set end0 down >>>> [root@alarm ~]# ip macsec add macsec0 rx port 1 address >>>> 00:01:be:be:ef:33 >>>> RTNETLINK answers: Operation not supported >>> >>> Where does that EOPNOTSUPP come from? nxp_c45_mdo_add_rxsc from this >>> version of the code can't return that, and macsec_add_rxsc also >>> shouldn't at this point. >> >> This is the source of -EOPNOTSUPP >> https://elixir.bootlin.com/linux/latest/source/drivers/net/macsec.c#L1928 > > Could you check which part of macsec_get_ops is failing? Since > macsec_newlink with "offload phy" worked, macsec_check_offload > shouldn't fail, so why does macsec_get_ops return NULL? > real_dev->phydev was NULL'ed? This check logical and returns false: https://elixir.bootlin.com/linux/latest/source/drivers/net/macsec.c#L343 real_dev->phydev was nulled. The call stack is next: fec_enet_close -> phy_disconnect -> phy_detach -> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L1815 -- Radu P.
2023-09-01, 17:22:49 +0300, Radu Pirea (OSS) wrote: > > > On 01.09.2023 16:57, Sabrina Dubroca wrote: > > 2023-09-01, 14:58:12 +0300, Radu Pirea (OSS) wrote: > > > On 01.09.2023 13:07, Sabrina Dubroca wrote: > > > > > (the interface was up before) > > > > > [root@alarm ~]# ip link add link end0 macsec0 type macsec encrypt on > > > > > offload phy > > > > > [root@alarm ~]# ip link set end0 down > > > > > [root@alarm ~]# ip macsec add macsec0 rx port 1 address > > > > > 00:01:be:be:ef:33 > > > > > RTNETLINK answers: Operation not supported > > > > > > > > Where does that EOPNOTSUPP come from? nxp_c45_mdo_add_rxsc from this > > > > version of the code can't return that, and macsec_add_rxsc also > > > > shouldn't at this point. > > > > > > This is the source of -EOPNOTSUPP > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/macsec.c#L1928 > > > > Could you check which part of macsec_get_ops is failing? Since > > macsec_newlink with "offload phy" worked, macsec_check_offload > > shouldn't fail, so why does macsec_get_ops return NULL? > > real_dev->phydev was NULL'ed? > > This check logical and returns false: > https://elixir.bootlin.com/linux/latest/source/drivers/net/macsec.c#L343 > > real_dev->phydev was nulled. > The call stack is next: > fec_enet_close -> phy_disconnect -> phy_detach -> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L1815 Ok, thanks for looking this up. So we can't have a consistent behavior between SW and PHY modes unfortunately. -- Sabrina
On Fri, Sep 01, 2023 at 12:07:32PM +0200, Sabrina Dubroca wrote: > 2023-09-01, 09:09:06 +0000, Radu Pirea wrote: > > On Wed, 2023-08-30 at 13:35 +0200, Sabrina Dubroca wrote: > > ... > > > > > And it's not restored when the link goes back up? That's inconvenient > > > :/ > > > Do we end up with inconsistent state? ie driver and core believe > > > everything is still offloaded, but HW lost all state? do we leak > > > some resources allocated by the driver? > > > > Yes. We end up with inconsistent state. The HW will lost all state when > > the phy is reseted. No resource is leaked, everything is there, but the > > configuration needs to be reapplied. > > > > > > > > We could add a flush/restore in macsec_notify when the lower device > > > goes down/up, maybe limited to devices that request this (I don't > > > know > > > if all devices would need it, or maybe all devices offloading to the > > > PHY but not to the MAC). > > > > Agreed. > > We can do a flush very simple, but to restore the configuration maybe > > we should to save the key in the macsec_key structure. I am not sure if > > the key can be extracted from crypto_aead structure. > > Either that or in the driver. I have a small preference for driver, > because then cases that don't need this restore won't have to keep the > key in memory, reducing the likelihood of accidentally sharing it. > OTOH, if we centralize that code, it's easier to make sure everything > is cleared from kernel memory when we delete the SA. Maybe consider about doing it as a library function, so drivers that need this don't have to reimplement the functionality in randomly buggy ways? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
2023-09-01, 11:32:19 +0100, Russell King (Oracle) wrote: > On Fri, Sep 01, 2023 at 12:07:32PM +0200, Sabrina Dubroca wrote: > > 2023-09-01, 09:09:06 +0000, Radu Pirea wrote: > > > We can do a flush very simple, but to restore the configuration maybe > > > we should to save the key in the macsec_key structure. I am not sure if > > > the key can be extracted from crypto_aead structure. > > > > Either that or in the driver. I have a small preference for driver, > > because then cases that don't need this restore won't have to keep the > > key in memory, reducing the likelihood of accidentally sharing it. > > OTOH, if we centralize that code, it's easier to make sure everything > > is cleared from kernel memory when we delete the SA. > > Maybe consider about doing it as a library function, so drivers that > need this don't have to reimplement the functionality in randomly > buggy ways? But then the driver would depend on the macsec module, right? It's not a large module, but that seems a bit undesirable. I think I'd rather add the key to macsec_key, and only copy it there in case we're offloading (we currently don't allow enabling offloading after installing some SAs/keys so that would be fine). Maybe add a driver flag to request keeping the keys in memory (I don't know if all drivers will require that -- seems like all PHY drivers would, but what about the MAC ones?). -- Sabrina
On Fri, Sep 01, 2023 at 09:09:06AM +0000, Radu Pirea wrote: > On Wed, 2023-08-30 at 13:35 +0200, Sabrina Dubroca wrote: > ... > > > And it's not restored when the link goes back up? That's inconvenient > > :/ > > Do we end up with inconsistent state? ie driver and core believe > > everything is still offloaded, but HW lost all state? do we leak > > some resources allocated by the driver? > > Yes. We end up with inconsistent state. The HW will lost all state when > the phy is reseted. No resource is leaked, everything is there, but the > configuration needs to be reapplied. If it's happening because the PHY is being re-attached from the network driver, then wouldn't it be a good idea to synchronise the hardware state with the software configuration in the ->config_init function? Presumably the hardware state is also lost when resuming from suspend as well? If so, that'll also fix that issue as well. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 01.09.2023 12:27, Russell King (Oracle) wrote: > On Fri, Sep 01, 2023 at 09:09:06AM +0000, Radu Pirea wrote: >> On Wed, 2023-08-30 at 13:35 +0200, Sabrina Dubroca wrote: >> ... >> >>> And it's not restored when the link goes back up? That's inconvenient >>> :/ >>> Do we end up with inconsistent state? ie driver and core believe >>> everything is still offloaded, but HW lost all state? do we leak >>> some resources allocated by the driver? >> >> Yes. We end up with inconsistent state. The HW will lost all state when >> the phy is reseted. No resource is leaked, everything is there, but the >> configuration needs to be reapplied. > > If it's happening because the PHY is being re-attached from the network > driver, then wouldn't it be a good idea to synchronise the hardware > state with the software configuration in the ->config_init function? .config_init might be an option, but keeping the keys in the driver might not be a good idea. > > Presumably the hardware state is also lost when resuming from suspend > as well? If so, that'll also fix that issue as well. soft_reset is called when resuming from suspend, so, in this case, the MACsec configuration will be lost. -- Radu P.
On Fri, Sep 01, 2023 at 02:31:32PM +0300, Radu Pirea (OSS) wrote: > On 01.09.2023 12:27, Russell King (Oracle) wrote: > > On Fri, Sep 01, 2023 at 09:09:06AM +0000, Radu Pirea wrote: > > > On Wed, 2023-08-30 at 13:35 +0200, Sabrina Dubroca wrote: > > > ... > > > > > > > And it's not restored when the link goes back up? That's inconvenient > > > > :/ > > > > Do we end up with inconsistent state? ie driver and core believe > > > > everything is still offloaded, but HW lost all state? do we leak > > > > some resources allocated by the driver? > > > > > > Yes. We end up with inconsistent state. The HW will lost all state when > > > the phy is reseted. No resource is leaked, everything is there, but the > > > configuration needs to be reapplied. > > > > If it's happening because the PHY is being re-attached from the network > > driver, then wouldn't it be a good idea to synchronise the hardware > state with the software configuration in the ->config_init function? > > .config_init might be an option, but keeping the keys in the driver might > not be a good idea. > > > > > Presumably the hardware state is also lost when resuming from suspend > > as well? If so, that'll also fix that issue as well. > soft_reset is called when resuming from suspend, so, in this case, the > MACsec configuration will be lost. Depending on what loses power at suspend time, it could be that the PHY is powered down, and thus would lose all configuration. This is something that the MACSEC core _has_ to expect may happen, and there has to be some way to restore the configuration, including the keys! One can't "write configuration to hardware and then forget" when the hardware may lose state, no matter what the configuration is. Take for example hibernation... where the system may be effectively powered off - maybe even if it's a piece of mains powered equipment, it may be unplugged from the mains. When the system resumes, shouldn't the configuration be completely restored, keys and all, so that it continues to function as it was before hibernation? The only possible alternative would be to have some kind of way for the driver to tell the core that state was lost, so the core can invalidate that state and inform userspace of that event, so userspace gets the opportunity itself to restore the lost state. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Aug 24, 2023 at 12:16:15PM +0300, Radu Pirea (NXP OSS) wrote: > Implement mdo_insert_tx_tag to insert the TLV header in the ethernet > frame. > > If extscs parameter is set to 1, then the TLV header will contain the > TX SC that will be used to encrypt the frame, otherwise the TX SC will > be selected using the MAC source address. > > Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com> > --- > drivers/net/phy/nxp-c45-tja11xx-macsec.c | 66 ++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/drivers/net/phy/nxp-c45-tja11xx-macsec.c b/drivers/net/phy/nxp-c45-tja11xx-macsec.c ... > @@ -167,6 +171,18 @@ > #define MACSEC_INPBTS 0x0638 > #define MACSEC_IPSNFS 0x063C > > +#define TJA11XX_TLV_TX_NEEDED_HEADROOM (32) > +#define TJA11XX_TLV_NEEDED_TAILROOM (0) > + > +#define MACSEC_TLV_CP BIT(0) > +#define MACSEC_TLV_SC_ID_OFF (2) > + > +#define ETH_P_TJA11XX_TLV (0x4e58) > + > +bool extscs; Hi Radu, Sparse suggests that extscs should be static. > +module_param(extscs, bool, 0); > +MODULE_PARM_DESC(extscs, "Select the TX SC using TLV header information."); ...
© 2016 - 2025 Red Hat, Inc.