| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <592fff842e9ae2d9178cef052151fca7a0b9a8c7.camel@duagon.com>
Date: Tue, 2 Jan 2024 15:27:26 +0000
From: Sanjuán García, Jorge
<Jorge.SanjuanGarcia@...gon.com>
To: "s-vadapalli@...com" <s-vadapalli@...com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"edumazet@...gle.com" <edumazet@...gle.com>, "davem@...emloft.net"
<davem@...emloft.net>, "kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>
Subject: Re: [PATCH 3/3] net: ethernet: ti: am65-cpsw: Add device tree
property to set max MTU
On Tue, 2024-01-02 at 16:03 +0530, Siddharth Vadapalli wrote:
> [No suele recibir correo electrónico de s-vadapalli@...com. Descubra
> por qué esto es importante en
> https://aka.ms/LearnAboutSenderIdentification ]
>
> Hello,
Hello,
First of all thanks for the quick review. Some comments bellow:
>
> On 02-01-2024 13:49, Sanjuán García, Jorge wrote:
> > The switch supports ethernet frame sizes between 64 and 2024 bytes
> > (including VLAN) as stated in the technical reference manual.
>
> Could you please share the source for the "2024 bytes" mentioned
> above?
> In J7200 SoC's TRM, I see support for up to 9604 bytes (including
> VLAN)
> in the "CPSW_PN_RX_MAXLEN_REG_k" register description for CPSW5G
> instance of CPSW.
>
The 2024 bytes as max value I got it from the AM6442 TRF which is the
SoC I have been working on. At least for port 0, the register
CPSW_P0_RX_MAXLEN_REG is documented as: "The maximum value is
9604 (including VLAN) when fifo_blk_size = 4. When fifo_blk_size = 1
the maximum value is 2024 (including VLAN)". It is not clear to me how
the fifo_blk_size should work from the reference manual so I kept it
safe to those 2024 bytes. Please let me know if something else should
be considered.
> >
> > This patch adds a new devicetree property so the switch ports can
> > be configured with an MTU higher than the standar 1500 bytes,
> > making
>
> nitpick: standar/standard.
Oops.
>
> > the max frame length configured on the registers and the max_mtu
> > advertised on the network device consistent.
> >
> > Signed-off-by: Jorge Sanjuan Garcia
> > <jorge.sanjuangarcia@...gon.com>
> > ---
>
> For patches which add new features, please use the subject prefix
> [PATCH net-next].
>
> > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 18 ++++++++++++++----
> > drivers/net/ethernet/ti/am65-cpsw-nuss.h | 1 +
> > 2 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > index a920146d7a60..6a5c8b6e03f4 100644
> > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > @@ -56,7 +56,7 @@
> > #define AM65_CPSW_MAX_PORTS 8
> >
> > #define AM65_CPSW_MIN_PACKET_SIZE VLAN_ETH_ZLEN
> > -#define AM65_CPSW_MAX_PACKET_SIZE (VLAN_ETH_FRAME_LEN +
> > ETH_FCS_LEN)
> > +#define AM65_CPSW_MAX_PACKET_SIZE 2024
> >
> > #define AM65_CPSW_REG_CTL 0x004
> > #define AM65_CPSW_REG_STAT_PORT_EN 0x014
> > @@ -2198,8 +2198,7 @@ am65_cpsw_nuss_init_port_ndev(struct
> > am65_cpsw_common
> > *common, u32 port_idx)
> > eth_hw_addr_set(port->ndev, port->slave.mac_addr);
> >
> > port->ndev->min_mtu = AM65_CPSW_MIN_PACKET_SIZE;
> > - port->ndev->max_mtu = common->rx_packet_max -
> > - (VLAN_ETH_HLEN + ETH_FCS_LEN);
> > + port->ndev->max_mtu = common->max_mtu;
>
> This seems to be modifying what was added in just the previous patch.
> Isn't it better to merge these changes into a single patch?
Yeah. I was not sure about whether it would be best to split the new
struct member and the device tree parsing into two patches. I'll merge
patches 2/3 and 3/3 of this series as one patch with the updates.
>
> > port->ndev->hw_features = NETIF_F_SG |
> > NETIF_F_RXCSUM |
> > NETIF_F_HW_CSUM |
> > @@ -2927,8 +2926,19 @@ static int am65_cpsw_nuss_probe(struct
> > platform_device *pdev)
> > if (common->port_num < 1 || common->port_num >
> > AM65_CPSW_MAX_PORTS)
> > return -ENOENT;
> >
> > + common->max_mtu = VLAN_ETH_DATA_LEN;
> > + of_property_read_u32(dev->of_node, "max-frame-size",
> > &common->max_mtu);
>
> The device-tree property "max-frame-size" is a port-specific
> property.
> Therefore, it is wrong to expect the property to be present at the
> CPSW
> node level instead of being present within each port in the
> "ethernet-ports" node. This section should be moved into the
> function:
> am65_cpsw_nuss_init_slave_ports()
> which parses the device-tree nodes for each port. The "max-frame-
> size"
> property can be stored there on a per-port basis within a newly added
> member in "struct am65_cpsw_port" as mentioned in my previous mail
> for
> patch 2/3.
>
That makes sense. I agree this should be a per slave port property.
I'll start putting together a version 2 doing it that way. I need to
think about what we should do with port 0's max frame size.
> > +
> > + common->rx_packet_max = common->max_mtu + VLAN_ETH_HLEN +
> > ETH_FCS_LEN;
> > + if (common->rx_packet_max > AM65_CPSW_MAX_PACKET_SIZE) {
> > + common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
> > + common->max_mtu = AM65_CPSW_MAX_PACKET_SIZE -
> > + (VLAN_ETH_HLEN + ETH_FCS_LEN);
> > + }
> > +
> > + dev_info(common->dev, "Max RX packet size set to %d\n",
> > common->rx_packet_max);
> > +
> > common->rx_flow_id_base = -1;
> > - common->rx_packet_max = AM65_CPSW_MAX_PACKET_SIZE;
> > init_completion(&common->tdown_complete);
> > common->tx_ch_num = AM65_CPSW_DEFAULT_TX_CHNS;
> > common->pf_p0_rx_ptype_rrobin = false;
> > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> > b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> > index 141160223d73..3bb0ff94a46a 100644
> > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
> > @@ -130,6 +130,7 @@ struct am65_cpsw_common {
> > u32 tx_ch_rate_msk;
> > u32 rx_flow_id_base;
> >
> > + int max_mtu;
> > int rx_packet_max;
> >
> > struct am65_cpsw_tx_chn tx_chns[AM65_CPSW_MAX_TX_QUEUES];
>
> ...
>
> --
> Regards,
> Siddharth.
Regards,
Jorge
Powered by blists - more mailing lists