Re: [PATCH net-next v1] octeon_ep: get max rx packet length from firmware

[Jesse Brandeburg]

On 11/21/2023 11:12 AM, Shinas Rasheed wrote:
> Fill max rx packet length value from firmware.

Hi Shinas, thanks for the patch.

Please provide why, and make sure you're talking to the linux kernel
developer audience who don't know anything about your hardware. We're
interested to know why this patch is useful to the kernel and why it
might need to be applied.


> 
> Signed-off-by: Shinas Rasheed <srasheed@xxxxxxxxxxx>
> ---
>  .../marvell/octeon_ep/octep_ctrl_net.c         | 18 ++++++++++++++++++
>  .../marvell/octeon_ep/octep_ctrl_net.h         |  9 +++++++++
>  .../ethernet/marvell/octeon_ep/octep_main.c    | 10 +++++++++-
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c
> index 6dd3d03c1c0f..c9fcebb9bd9b 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c
> @@ -198,6 +198,24 @@ int octep_ctrl_net_set_mac_addr(struct octep_device *oct, int vfid, u8 *addr,
>  	return octep_send_mbox_req(oct, &d, wait_for_response);
>  }
>  
> +int octep_ctrl_net_get_mtu(struct octep_device *oct, int vfid)
> +{
> +	struct octep_ctrl_net_wait_data d = {0};

I think preferred style is now d = { }; since that doesn't mess up if
the first member of the struct is an enum.

> +	struct octep_ctrl_net_h2f_req *req;
> +	int err;
> +
> +	req = &d.data.req;
> +	init_send_req(&d.msg, req, mtu_sz, vfid);
> +	req->hdr.s.cmd = OCTEP_CTRL_NET_H2F_CMD_MTU;
> +	req->mtu.cmd = OCTEP_CTRL_NET_CMD_GET;
> +
> +	err = octep_send_mbox_req(oct, &d, true);
> +	if (err < 0)
> +		return err;
> +
> +	return d.data.resp.mtu.val;
> +}
> +
>  int octep_ctrl_net_set_mtu(struct octep_device *oct, int vfid, int mtu,
>  			   bool wait_for_response)
>  {
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.h b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.h
> index 4bb97ad1f1c6..46ddaa5c64d1 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.h
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.h
> @@ -282,6 +282,15 @@ int octep_ctrl_net_get_mac_addr(struct octep_device *oct, int vfid, u8 *addr);
>  int octep_ctrl_net_set_mac_addr(struct octep_device *oct, int vfid, u8 *addr,
>  				bool wait_for_response);
>  
> +/** Get max MTU from firmware.
> + *
> + * @param oct: non-null pointer to struct octep_device.
> + * @param vfid: Index of virtual function.
> + *
> + * return value: mtu on success, -errno on failure.
> + */

The above block is definitely not correctly formatted kdoc (if that's
what you wanted), and you can probably get feedback about it from
scripts/kernel-doc -v
drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.h

I see you have some correctly formatted doc in octep_tx.c


> +int octep_ctrl_net_get_mtu(struct octep_device *oct, int vfid);
> +
>  /** Set mtu in firmware.
>   *
>   * @param oct: non-null pointer to struct octep_device.
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 3cee69b3ac38..f9c539178114 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -1276,6 +1276,7 @@ static int octep_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct octep_device *octep_dev = NULL;
>  	struct net_device *netdev;
> +	int max_rx_pktlen;
>  	int err;
>  
>  	err = pci_enable_device(pdev);
> @@ -1346,8 +1347,15 @@ static int octep_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	netdev->hw_features = NETIF_F_SG;
>  	netdev->features |= netdev->hw_features;
> +
> +	max_rx_pktlen = octep_ctrl_net_get_mtu(octep_dev, OCTEP_CTRL_NET_INVALID_VFID);
> +	if (max_rx_pktlen < 0) {
> +		dev_err(&octep_dev->pdev->dev,
> +			"Failed to get max receive packet size; err = %d\n", max_rx_pktlen);
> +		goto register_dev_err;
> +	}
>  	netdev->min_mtu = OCTEP_MIN_MTU;
> -	netdev->max_mtu = OCTEP_MAX_MTU;
> +	netdev->max_mtu = max_rx_pktlen - (ETH_HLEN + ETH_FCS_LEN);
>  	netdev->mtu = OCTEP_DEFAULT_MTU;

Not part of this patch, but was there a point to setting the mtu here
without telling the netdev? most of the time it seems sufficient to just
set max and min since the kernel default is already 1500 (which your
internal define also duplicates)

Mostly the code seems fine.