| Message ID | 20231218232639.33327-9-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | qca_spi: collection of improvements | expand |
On 12/18/2023 3:26 PM, Stefan Wahren wrote: > There are two points with the calculation of RX buffer size which are > not optimal: > 1. mtu is a mutual parameter, but actually we need the maximum possible > MTU. So better use the define directly. > 2. This magic number 4 represent the hardware generated frame length > which is specific to SPI. We better replace this with the suitable > define. > > There is no functional change. > This is a bit confusing why you said no functional change. At first my thought was that dev->mtu could easily be smaller than the maximum MTU. (thats why its configurable after all?). But actually the diff context cuts off a few lines that are extremely relevant: > struct qcaspi *qca = netdev_priv(dev); > > dev->mtu = QCAFRM_MAX_MTU; We assign dev->mtu here to QCAFRM_MAX_MTU > dev->type = ARPHRD_ETHER; > qca->clkspeed = qcaspi_clkspeed; > qca->burst_len = qcaspi_burst_len; > qca->spi_thread = NULL; > qca->buffer_size = (dev->mtu + VLAN_ETH_HLEN + QCAFRM_HEADER_LEN + > QCAFRM_FOOTER_LEN + 4) * 4; And then use it here. > > memset(&qca->stats, 0, sizeof(struct qcaspi_stats)); > Makes sense to use the constant directly. It might have helped to have a bit more of this context in the commit message, but its still a good cleanup. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > drivers/net/ethernet/qualcomm/qca_spi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c > index 80182f8a4a50..6d2859aad921 100644 > --- a/drivers/net/ethernet/qualcomm/qca_spi.c > +++ b/drivers/net/ethernet/qualcomm/qca_spi.c > @@ -813,8 +813,8 @@ qcaspi_netdev_init(struct net_device *dev) > qca->clkspeed = qcaspi_clkspeed; > qca->burst_len = qcaspi_burst_len; > qca->spi_thread = NULL; > - qca->buffer_size = (dev->mtu + VLAN_ETH_HLEN + QCAFRM_HEADER_LEN + > - QCAFRM_FOOTER_LEN + 4) * QCASPI_RX_MAX_FRAMES; > + qca->buffer_size = (QCAFRM_MAX_MTU + VLAN_ETH_HLEN + QCAFRM_HEADER_LEN + > + QCAFRM_FOOTER_LEN + QCASPI_HW_PKT_LEN) * QCASPI_RX_MAX_FRAMES; > > memset(&qca->stats, 0, sizeof(struct qcaspi_stats)); > > -- > 2.34.1 > >
diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c index 80182f8a4a50..6d2859aad921 100644 --- a/drivers/net/ethernet/qualcomm/qca_spi.c +++ b/drivers/net/ethernet/qualcomm/qca_spi.c @@ -813,8 +813,8 @@ qcaspi_netdev_init(struct net_device *dev) qca->clkspeed = qcaspi_clkspeed; qca->burst_len = qcaspi_burst_len; qca->spi_thread = NULL; - qca->buffer_size = (dev->mtu + VLAN_ETH_HLEN + QCAFRM_HEADER_LEN + - QCAFRM_FOOTER_LEN + 4) * QCASPI_RX_MAX_FRAMES; + qca->buffer_size = (QCAFRM_MAX_MTU + VLAN_ETH_HLEN + QCAFRM_HEADER_LEN + + QCAFRM_FOOTER_LEN + QCASPI_HW_PKT_LEN) * QCASPI_RX_MAX_FRAMES; memset(&qca->stats, 0, sizeof(struct qcaspi_stats));
There are two points with the calculation of RX buffer size which are not optimal: 1. mtu is a mutual parameter, but actually we need the maximum possible MTU. So better use the define directly. 2. This magic number 4 represent the hardware generated frame length which is specific to SPI. We better replace this with the suitable define. There is no functional change. Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- drivers/net/ethernet/qualcomm/qca_spi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.34.1