diff mbox

[dpdk-dev,v3] virtio:add mtu set in virtio

Message ID 20160907032156.34492-1-sodey@sonusnet.com (mailing list archive)
State Superseded, archived
Delegated to: Yuanhan Liu
Headers

Commit Message

souvikdey33 Sept. 7, 2016, 3:21 a.m. UTC 
  Signed-off-by: Souvik Dey <sodey@sonusnet.com>
Fixes: 1fb8e8896ca8 ("Signed-off-by: Souvik Dey <sodey@sonusnet.com>")
Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>

Virtio interfaces should also support setting of mtu, as in case of cloud
it is expected to have the consistent mtu across the infrastructure that
the dhcp server sends and not hardcoded to 1500(default).
---
Corrected few style errors as reported by sys-stv.

 drivers/net/virtio/virtio_ethdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Yuanhan Liu Sept. 7, 2016, 3:37 a.m. UTC | #1 
Firstly, thanks for the patch!

And I got few more style issues for you :) The first goes to the subject
(commit summary):

- the prefix is "net/virtio", but not "virtio"

- a space is needed after ':'

On Tue, Sep 06, 2016 at 11:21:56PM -0400, souvikdey33 wrote:
> Signed-off-by: Souvik Dey <sodey@sonusnet.com>

SoB should go the end of the commit log.

> Fixes: 1fb8e8896ca8 ("Signed-off-by: Souvik Dey <sodey@sonusnet.com>")

The fixline is needed for bug fixing patch only. Besides that, the
commit has to be an commit has been applied before.

> Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>

I don't see such Reviewed-by from Stephen. I think you should not add it,
unless someone has given you that, explicitly.

> Virtio interfaces should also support setting of mtu, as in case of cloud
> it is expected to have the consistent mtu across the infrastructure that
> the dhcp server sends and not hardcoded to 1500(default).
> ---
> Corrected few style errors as reported by sys-stv.

It's better to keep old changes, such as:

v3: correct few style errors ...

v2: ....

FYI, you might want to read others patch to get more used to the right
way of making a patch.

> 
>  drivers/net/virtio/virtio_ethdev.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 07d6449..da16ad4 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -92,6 +92,7 @@ static void virtio_mac_addr_add(struct rte_eth_dev *dev,
>  static void virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
>  static void virtio_mac_addr_set(struct rte_eth_dev *dev,
>  				struct ether_addr *mac_addr);
> +static int  virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);

I think it's not necessary if you defined the function before the usage.

	--yliu
Yuanhan Liu Sept. 7, 2016, 3:42 a.m. UTC | #2 
On Tue, Sep 06, 2016 at 11:21:56PM -0400, souvikdey33 wrote:
> +static int
> +virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
> +{
> +	struct virtio_hw *hw = dev->data->dev_private;
> +	if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) {
> +		PMD_INIT_LOG(ERR, "Mtu should be between 64 and 9728\n");

I forgot to mention in last email, that you should not use the number (64
and 9728) directly, use the MACRO instead.

	--yliu
souvikdey33 Sept. 7, 2016, 3:45 a.m. UTC | #3 
Hi Liu,


The first version of the patch was reviewed by Stephen and after he agreed I have incorporated all those changes in v2 and v3 had only the style error fixes. That is why I have put the line Reviewed by .
Still I might have some errors in filing the patch as I am new to this. Do you recommend to submit an updated version or this is fine ? 

--
Regards,
Souvik
-----Original Message-----
From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] 
Sent: Tuesday, September 6, 2016 11:38 PM
To: Dey, Souvik <sodey@sonusnet.com>
Cc: dev@dpdk.org; stephen@networkplumber.org
Subject: Re: [dpdk-dev] [PATCH v3]virtio:add mtu set in virtio

Firstly, thanks for the patch!

And I got few more style issues for you :) The first goes to the subject (commit summary):

- the prefix is "net/virtio", but not "virtio"

- a space is needed after ':'

On Tue, Sep 06, 2016 at 11:21:56PM -0400, souvikdey33 wrote:
> Signed-off-by: Souvik Dey <sodey@sonusnet.com>

SoB should go the end of the commit log.

> Fixes: 1fb8e8896ca8 ("Signed-off-by: Souvik Dey <sodey@sonusnet.com>")

The fixline is needed for bug fixing patch only. Besides that, the commit has to be an commit has been applied before.

> Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>

I don't see such Reviewed-by from Stephen. I think you should not add it, unless someone has given you that, explicitly.

> Virtio interfaces should also support setting of mtu, as in case of 
> cloud it is expected to have the consistent mtu across the 
> infrastructure that the dhcp server sends and not hardcoded to 1500(default).
> ---
> Corrected few style errors as reported by sys-stv.

It's better to keep old changes, such as:

v3: correct few style errors ...

v2: ....

FYI, you might want to read others patch to get more used to the right way of making a patch.

> 
>  drivers/net/virtio/virtio_ethdev.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c 
> b/drivers/net/virtio/virtio_ethdev.c
> index 07d6449..da16ad4 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -92,6 +92,7 @@ static void virtio_mac_addr_add(struct rte_eth_dev 
> *dev,  static void virtio_mac_addr_remove(struct rte_eth_dev *dev, 
> uint32_t index);  static void virtio_mac_addr_set(struct rte_eth_dev *dev,
>  				struct ether_addr *mac_addr);
> +static int  virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);

I think it's not necessary if you defined the function before the usage.

	--yliu
souvikdey33 Sept. 7, 2016, 3:47 a.m. UTC | #4 
Ok will change it. Do I need to submit a new v4 for that ? can I put your name also in the reviewed by list?

-----Original Message-----
From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] 
Sent: Tuesday, September 6, 2016 11:43 PM
To: Dey, Souvik <sodey@sonusnet.com>
Cc: dev@dpdk.org; stephen@networkplumber.org
Subject: Re: [dpdk-dev] [PATCH v3]virtio:add mtu set in virtio

On Tue, Sep 06, 2016 at 11:21:56PM -0400, souvikdey33 wrote:
> +static int
> +virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> +	struct virtio_hw *hw = dev->data->dev_private;
> +	if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) {
> +		PMD_INIT_LOG(ERR, "Mtu should be between 64 and 9728\n");

I forgot to mention in last email, that you should not use the number (64 and 9728) directly, use the MACRO instead.

	--yliu
Yuanhan Liu Sept. 7, 2016, 3:53 a.m. UTC | #5 
On Wed, Sep 07, 2016 at 03:47:27AM +0000, Dey, Souvik wrote:
> Ok will change it. Do I need to submit a new v4 for that ?

Yes.

> can I put your name also in the reviewed by list?

Nope, you should not add that. I just offered some comments. And yes, I
reviewed your patch, but that doesn't mean you could add my Reviewed-by.

You can only add the Reviewed-by tag only when the reviewer gave it to
you, explicitly, like following:

    Reviewed-by: Some One <some@one.com>

	--yliu

> 
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] 
> Sent: Tuesday, September 6, 2016 11:43 PM
> To: Dey, Souvik <sodey@sonusnet.com>
> Cc: dev@dpdk.org; stephen@networkplumber.org
> Subject: Re: [dpdk-dev] [PATCH v3]virtio:add mtu set in virtio
> 
> On Tue, Sep 06, 2016 at 11:21:56PM -0400, souvikdey33 wrote:
> > +static int
> > +virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> > +	struct virtio_hw *hw = dev->data->dev_private;
> > +	if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) {
> > +		PMD_INIT_LOG(ERR, "Mtu should be between 64 and 9728\n");
> 
> I forgot to mention in last email, that you should not use the number (64 and 9728) directly, use the MACRO instead.
> 
> 	--yliu
souvikdey33 Sept. 7, 2016, 4:04 a.m. UTC | #6 
Ok thanks understood. I will submit v4 for this.

-----Original Message-----
From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] 
Sent: Tuesday, September 6, 2016 11:54 PM
To: Dey, Souvik <sodey@sonusnet.com>
Cc: dev@dpdk.org; stephen@networkplumber.org
Subject: Re: [dpdk-dev] [PATCH v3]virtio:add mtu set in virtio

On Wed, Sep 07, 2016 at 03:47:27AM +0000, Dey, Souvik wrote:
> Ok will change it. Do I need to submit a new v4 for that ?

Yes.

> can I put your name also in the reviewed by list?

Nope, you should not add that. I just offered some comments. And yes, I reviewed your patch, but that doesn't mean you could add my Reviewed-by.

You can only add the Reviewed-by tag only when the reviewer gave it to you, explicitly, like following:

    Reviewed-by: Some One <some@one.com>

	--yliu

> 
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Tuesday, September 6, 2016 11:43 PM
> To: Dey, Souvik <sodey@sonusnet.com>
> Cc: dev@dpdk.org; stephen@networkplumber.org
> Subject: Re: [dpdk-dev] [PATCH v3]virtio:add mtu set in virtio
> 
> On Tue, Sep 06, 2016 at 11:21:56PM -0400, souvikdey33 wrote:
> > +static int
> > +virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> > +	struct virtio_hw *hw = dev->data->dev_private;
> > +	if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) {
> > +		PMD_INIT_LOG(ERR, "Mtu should be between 64 and 9728\n");
> 
> I forgot to mention in last email, that you should not use the number (64 and 9728) directly, use the MACRO instead.
> 
> 	--yliu
diff mbox

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 07d6449..da16ad4 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -92,6 +92,7 @@  static void virtio_mac_addr_add(struct rte_eth_dev *dev,
 static void virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
 static void virtio_mac_addr_set(struct rte_eth_dev *dev,
 				struct ether_addr *mac_addr);
+static int  virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
 
 static int virtio_dev_queue_stats_mapping_set(
 	__rte_unused struct rte_eth_dev *eth_dev,
@@ -652,6 +653,16 @@  virtio_dev_allmulticast_disable(struct rte_eth_dev *dev)
 		PMD_INIT_LOG(ERR, "Failed to disable allmulticast");
 }
 
+static int
+virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
+{
+	struct virtio_hw *hw = dev->data->dev_private;
+	if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) {
+		PMD_INIT_LOG(ERR, "Mtu should be between 64 and 9728\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
 /*
  * dev_ops for virtio, bare necessities for basic operation
  */
@@ -664,6 +675,7 @@  static const struct eth_dev_ops virtio_eth_dev_ops = {
 	.promiscuous_disable     = virtio_dev_promiscuous_disable,
 	.allmulticast_enable     = virtio_dev_allmulticast_enable,
 	.allmulticast_disable    = virtio_dev_allmulticast_disable,
+	.mtu_set                 = virtio_mtu_set,
 
 	.dev_infos_get           = virtio_dev_info_get,
 	.stats_get               = virtio_dev_stats_get,