Hi Paul,
Thanks for your work.
On 2024-06-15 11:30:38 +0100, Paul Barker wrote:
> The RX frame size limit should not be based on the current MTU setting.
> Instead it should be based on the hardware capabilities.
>
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
I agree with the change the RFLR.RFL setting should not be connected to
the MTU setting. And this likely comes from the early days of the driver
where neither Rx or Tx supported multiple descriptors for each packet.
In those days the single descriptor used for each packet was tied to the
MTU setting. So likely the fixes tag should point to a later commit?
This is a great find and shows a flaw in the driver. But limiting the
size of each descriptor used for Tx is only half the solution right? As
the driver now supports multiple descriptors for Tx (on GbEth) the
driver allows setting an MTU larger then the maximum size for single
descriptor on those devices. But the xmit function of the driver still
hardcode the maximum of 2 descriptors for each Tx packet. And it only
uses the two descriptors to align the data to hardware constrains.
Is it not incorrect for the driver to accept an MTU larger then the
maximum size of a single descriptor with the current Tx implementation?
The driver can only support larger MTU settings for Rx, but not Tx ATM.
I think the complete fix is to extend ravb_start_xmit() to fully support
split descriptors for packets larger then the maximum single descriptor
size. Not just to align the packet between a DT_FSTART and DT_FEND
descriptor when needed.
> Signed-off-by: Paul Barker <paul.barker.ct@xxxxxxxxxxxxxx>
> ---
> drivers/net/ethernet/renesas/ravb_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 02cbf850bd85..481c854cb305 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -555,8 +555,10 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
>
> static void ravb_emac_init_rcar(struct net_device *ndev)
> {
> + struct ravb_private *priv = netdev_priv(ndev);
> +
> /* Receive frame limit set register */
> - ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
> + ravb_write(ndev, priv->info->rx_max_frame_size + ETH_FCS_LEN, RFLR);
>
> /* EMAC Mode: PAUSE prohibition; Duplex; RX Checksum; TX; RX */
> ravb_write(ndev, ECMR_ZPF | ECMR_DM |
> --
> 2.39.2
>
--
Kind Regards,
Niklas Söderlund
