netdev - Re: [net-next PATCH] octeontx2-pf: Change receive buffer size using ethtool

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220112110314.358d5295@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Wed, 12 Jan 2022 11:03:14 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Subbaraya Sundeep <sbhatta@...vell.com>
Cc:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <hkelam@...vell.com>, <gakula@...vell.com>, <sgoutham@...vell.com>
Subject: Re: [net-next PATCH] octeontx2-pf: Change receive buffer size using
 ethtool

On Wed, 12 Jan 2022 22:32:55 +0530 Subbaraya Sundeep wrote:
> ethtool rx-buf-len is for setting receive buffer size,
> support setting it via ethtool -G parameter and getting
> it via ethtool -g parameter.

I don't see a check against current MTU, in case MTU is larger than 
the buffer length the device will scatter?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> index 61e5281..6d11cb2 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> @@ -177,6 +177,7 @@ struct otx2_hw {
>  	u16			pool_cnt;
>  	u16			rqpool_cnt;
>  	u16			sqpool_cnt;
> +	u16			rbuf_len;
>  
>  	/* NPA */
>  	u32			stack_pg_ptrs;  /* No of ptrs per stack page */
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> index d85db90..a100296 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> @@ -371,6 +371,7 @@ static void otx2_get_ringparam(struct net_device *netdev,
>  	ring->rx_pending = qs->rqe_cnt ? qs->rqe_cnt : Q_COUNT(Q_SIZE_256);
>  	ring->tx_max_pending = Q_COUNT(Q_SIZE_MAX);
>  	ring->tx_pending = qs->sqe_cnt ? qs->sqe_cnt : Q_COUNT(Q_SIZE_4K);
> +	kernel_ring->rx_buf_len = pfvf->hw.rbuf_len;
>  }
>  
>  static int otx2_set_ringparam(struct net_device *netdev,
> @@ -379,6 +380,7 @@ static int otx2_set_ringparam(struct net_device *netdev,
>  			      struct netlink_ext_ack *extack)
>  {
>  	struct otx2_nic *pfvf = netdev_priv(netdev);
> +	u32 rx_buf_len = kernel_ring->rx_buf_len;
>  	bool if_up = netif_running(netdev);
>  	struct otx2_qset *qs = &pfvf->qset;
>  	u32 rx_count, tx_count;
> @@ -386,6 +388,15 @@ static int otx2_set_ringparam(struct net_device *netdev,
>  	if (ring->rx_mini_pending || ring->rx_jumbo_pending)
>  		return -EINVAL;
>  
> +	/* Hardware supports max size of 32k for a receive buffer
> +	 * and 1536 is typical ethernet frame size.
> +	 */
> +	if (rx_buf_len && (rx_buf_len < 1536 || rx_buf_len > 32768)) {
> +		netdev_err(netdev,
> +			   "Receive buffer range is 1536 - 32768");
> +		return -EINVAL;
> +	}
> +
>  	/* Permitted lengths are 16 64 256 1K 4K 16K 64K 256K 1M  */
>  	rx_count = ring->rx_pending;
>  	/* On some silicon variants a skid or reserved CQEs are
> @@ -403,7 +414,7 @@ static int otx2_set_ringparam(struct net_device *netdev,
>  			   Q_COUNT(Q_SIZE_4K), Q_COUNT(Q_SIZE_MAX));
>  	tx_count = Q_COUNT(Q_SIZE(tx_count, 3));
>  
> -	if (tx_count == qs->sqe_cnt && rx_count == qs->rqe_cnt)
> +	if (tx_count == qs->sqe_cnt && rx_count == qs->rqe_cnt && !rx_buf_len)

Should we use rx_buf_len = 0 as a way for users to reset the rxbuf len
to the default value? I think that would be handy.

>  	if (if_up)
> @@ -413,6 +424,10 @@ static int otx2_set_ringparam(struct net_device *netdev,
>  	qs->sqe_cnt = tx_count;
>  	qs->rqe_cnt = rx_count;
>  
> +	if (rx_buf_len)
> +		pfvf->hw.rbuf_len = ALIGN(rx_buf_len, OTX2_ALIGN) +
> +				    OTX2_HEAD_ROOM;
> +
>  	if (if_up)
>  		return netdev->netdev_ops->ndo_open(netdev);
>  
> @@ -1207,6 +1222,7 @@ static int otx2_set_link_ksettings(struct net_device *netdev,
>  static const struct ethtool_ops otx2_ethtool_ops = {
>  	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>  				     ETHTOOL_COALESCE_MAX_FRAMES,
> +	.supported_ring_params  = ETHTOOL_RING_USE_RX_BUF_LEN,
>  	.get_link		= otx2_get_link,
>  	.get_drvinfo		= otx2_get_drvinfo,
>  	.get_strings		= otx2_get_strings,
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> index 6080ebd..37afffa 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> @@ -66,6 +66,8 @@ static int otx2_change_mtu(struct net_device *netdev, int new_mtu)
>  		    netdev->mtu, new_mtu);
>  	netdev->mtu = new_mtu;
>  
> +	pf->hw.rbuf_len = 0;

Why reset the buf len on mtu change?

>  	if (if_up)
>  		err = otx2_open(netdev);
>  
> @@ -1306,6 +1308,9 @@ static int otx2_get_rbuf_size(struct otx2_nic *pf, int mtu)
>  	int total_size;
>  	int rbuf_size;
>  
> +	if (pf->hw.rbuf_len)
> +		return pf->hw.rbuf_len;
> +
>  	/* The data transferred by NIX to memory consists of actual packet
>  	 * plus additional data which has timestamp and/or EDSA/HIGIG2
>  	 * headers if interface is configured in corresponding modes.