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

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

[Shinas Rasheed]

Fill max rx packet length value from firmware.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
 .../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};
+	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.
+ */
+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;
 
 	err = octep_ctrl_net_get_mac_addr(octep_dev, OCTEP_CTRL_NET_INVALID_VFID,
-- 
2.25.1

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@marvell.com>
> ---
>  .../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.

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

[Shinas Rasheed]

> -----Original Message-----
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Sent: Wednesday, November 22, 2023 2:34 AM
> To: Shinas Rasheed <srasheed@marvell.com>; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: Haseeb Gani <hgani@marvell.com>; Vimlesh Kumar
> <vimleshk@marvell.com>; egallen@redhat.com; mschmidt@redhat.com;
> pabeni@redhat.com; horms@kernel.org; kuba@kernel.org;
> davem@davemloft.net; wizhao@redhat.com; konguyen@redhat.com;
> Veerasenareddy Burru <vburru@marvell.com>; Sathesh B Edara
> <sedara@marvell.com>; Eric Dumazet <edumazet@google.com>
> Subject: [EXT] Re: [PATCH net-next v1] octeon_ep: get max rx packet length
> from firmware
> 
> External Email
> 
> ----------------------------------------------------------------------
> 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.

Sure will update that in the changelog in the next version.
 
> > +/** 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

I'll see to it to properly format it.

> > 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)

I suppose that piece of code is redundant, but serves to atleast say that the default expected is 1500 for the device.

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

[Suman Ghosh]

>Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
>---
> .../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};
>+	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.
>+ */
>+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;
>+	}
[Suman] Do we need to check if max_rx_pktlen <= OCTEP_MAX_MTU as well? If not, then this macro is not required further after the change?

> 	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;
>
> 	err = octep_ctrl_net_get_mac_addr(octep_dev,
>OCTEP_CTRL_NET_INVALID_VFID,
>--
>2.25.1
>

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

[Shinas Rasheed]

Hi Suman

> -----Original Message-----
> From: Suman Ghosh <sumang@marvell.com>
> Sent: Wednesday, November 22, 2023 10:13 AM
> To: Shinas Rasheed <srasheed@marvell.com>; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: Haseeb Gani <hgani@marvell.com>; Vimlesh Kumar
> <vimleshk@marvell.com>; egallen@redhat.com; mschmidt@redhat.com;
> pabeni@redhat.com; horms@kernel.org; kuba@kernel.org;
> davem@davemloft.net; wizhao@redhat.com; konguyen@redhat.com;
> Shinas Rasheed <srasheed@marvell.com>; Veerasenareddy Burru
> <vburru@marvell.com>; Sathesh B Edara <sedara@marvell.com>; Eric
> Dumazet <edumazet@google.com>
> Subject: RE: [EXT] [PATCH net-next v1] octeon_ep: get max rx packet length
> from firmware
> >@@ -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;
> >+	}
> [Suman] Do we need to check if max_rx_pktlen <= OCTEP_MAX_MTU as
> well? If not, then this macro is not required further after the change?

I suppose we should check this.