diff mbox

[dpdk-dev,v2,36/46] net/liquidio: add API to set MTU

Message ID 1488454371-3342-37-git-send-email-shijith.thotton@caviumnetworks.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Shijith Thotton March 2, 2017, 11:32 a.m. UTC 
  Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
Signed-off-by: Venkat Koppula <venkat.koppula@caviumnetworks.com>
Signed-off-by: Srisivasubramanian S <ssrinivasan@caviumnetworks.com>
Signed-off-by: Mallesham Jatharakonda <mjatharakonda@oneconvergence.com>
---
 drivers/net/liquidio/lio_ethdev.c | 47 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

Comments

Ferruh Yigit March 21, 2017, 12:24 p.m. UTC | #1 
On 3/2/2017 11:32 AM, Shijith Thotton wrote:
> Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
> Signed-off-by: Venkat Koppula <venkat.koppula@caviumnetworks.com>
> Signed-off-by: Srisivasubramanian S <ssrinivasan@caviumnetworks.com>
> Signed-off-by: Mallesham Jatharakonda <mjatharakonda@oneconvergence.com>

<...>

>  
>  static int
> +lio_dev_change_vf_mtu(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
> +{
> +	struct lio_device *lio_dev = LIO_DEV(eth_dev);
> +
> +	PMD_INIT_FUNC_TRACE();
> +
> +	if (!lio_dev->intf_open) {
> +		lio_dev_err(lio_dev, "Port %d down, can't change MTU\n",
> +			    lio_dev->port_id);
> +		return -EINVAL;
> +	}
> +
> +	/* Limit the MTU to make sure the ethernet packets are between
> +	 * ETHER_MIN_MTU bytes and PF's MTU
> +	 */
> +	if ((new_mtu < ETHER_MIN_MTU) ||
> +			(new_mtu > lio_dev->linfo.link.s.mtu)) {
> +		lio_dev_err(lio_dev, "Invalid MTU: %d\n", new_mtu);
> +		lio_dev_err(lio_dev, "Valid range %d and %d\n",
> +			    ETHER_MIN_MTU, lio_dev->linfo.link.s.mtu);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

Is this really sets the MTU?
"new_mtu" seems not used, except limit check, an lio_send_ctrl_pkt()
required perhaps?
Shijith Thotton March 21, 2017, 12:53 p.m. UTC | #2 
On Tue, Mar 21, 2017 at 12:24:49PM +0000, Ferruh Yigit wrote:
> On 3/2/2017 11:32 AM, Shijith Thotton wrote:
> > Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
> > Signed-off-by: Venkat Koppula <venkat.koppula@caviumnetworks.com>
> > Signed-off-by: Srisivasubramanian S <ssrinivasan@caviumnetworks.com>
> > Signed-off-by: Mallesham Jatharakonda <mjatharakonda@oneconvergence.com>
> 
> <...>
> 
> >  
> >  static int
> > +lio_dev_change_vf_mtu(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
> > +{
> > +	struct lio_device *lio_dev = LIO_DEV(eth_dev);
> > +
> > +	PMD_INIT_FUNC_TRACE();
> > +
> > +	if (!lio_dev->intf_open) {
> > +		lio_dev_err(lio_dev, "Port %d down, can't change MTU\n",
> > +			    lio_dev->port_id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Limit the MTU to make sure the ethernet packets are between
> > +	 * ETHER_MIN_MTU bytes and PF's MTU
> > +	 */
> > +	if ((new_mtu < ETHER_MIN_MTU) ||
> > +			(new_mtu > lio_dev->linfo.link.s.mtu)) {
> > +		lio_dev_err(lio_dev, "Invalid MTU: %d\n", new_mtu);
> > +		lio_dev_err(lio_dev, "Valid range %d and %d\n",
> > +			    ETHER_MIN_MTU, lio_dev->linfo.link.s.mtu);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Is this really sets the MTU?
> "new_mtu" seems not used, except limit check, an lio_send_ctrl_pkt()
> required perhaps?

It won't set MTU for hardware and is possible only by PF. So
lio_send_ctrl_pkt is not required. VF MTU is limited by PF MTU and is
mentioned under limitations in driver documentation. Here we are
allowing upper layer to set MTU up to the value configured by PF.

Shijith
Ferruh Yigit March 21, 2017, 1:01 p.m. UTC | #3 
On 3/21/2017 12:53 PM, Shijith Thotton wrote:
> On Tue, Mar 21, 2017 at 12:24:49PM +0000, Ferruh Yigit wrote:
>> On 3/2/2017 11:32 AM, Shijith Thotton wrote:
>>> Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>> Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
>>> Signed-off-by: Venkat Koppula <venkat.koppula@caviumnetworks.com>
>>> Signed-off-by: Srisivasubramanian S <ssrinivasan@caviumnetworks.com>
>>> Signed-off-by: Mallesham Jatharakonda <mjatharakonda@oneconvergence.com>
>>
>> <...>
>>
>>>  
>>>  static int
>>> +lio_dev_change_vf_mtu(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
>>> +{
>>> +	struct lio_device *lio_dev = LIO_DEV(eth_dev);
>>> +
>>> +	PMD_INIT_FUNC_TRACE();
>>> +
>>> +	if (!lio_dev->intf_open) {
>>> +		lio_dev_err(lio_dev, "Port %d down, can't change MTU\n",
>>> +			    lio_dev->port_id);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	/* Limit the MTU to make sure the ethernet packets are between
>>> +	 * ETHER_MIN_MTU bytes and PF's MTU
>>> +	 */
>>> +	if ((new_mtu < ETHER_MIN_MTU) ||
>>> +			(new_mtu > lio_dev->linfo.link.s.mtu)) {
>>> +		lio_dev_err(lio_dev, "Invalid MTU: %d\n", new_mtu);
>>> +		lio_dev_err(lio_dev, "Valid range %d and %d\n",
>>> +			    ETHER_MIN_MTU, lio_dev->linfo.link.s.mtu);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>
>> Is this really sets the MTU?
>> "new_mtu" seems not used, except limit check, an lio_send_ctrl_pkt()
>> required perhaps?
> 
> It won't set MTU for hardware and is possible only by PF. So
> lio_send_ctrl_pkt is not required. VF MTU is limited by PF MTU and is
> mentioned under limitations in driver documentation. Here we are
> allowing upper layer to set MTU up to the value configured by PF.

I see, but lio_dev_change_vf_mtu() does not set anything at all. If it
is not modifying anything at all, why you claim "MTU update" supported?

And following logic seems wrong for this case:

	...
	if (lio_dev->linfo.link.s.mtu != mtu) {
		ret = lio_dev_change_vf_mtu(eth_dev, mtu);
	...

Should this functions set lio_dev->linfo.link.s.mtu at least, perhaps?

> 
> Shijith
>
Shijith Thotton March 21, 2017, 1:56 p.m. UTC | #4 
On Tue, Mar 21, 2017 at 01:01:58PM +0000, Ferruh Yigit wrote:
> On 3/21/2017 12:53 PM, Shijith Thotton wrote:
> > On Tue, Mar 21, 2017 at 12:24:49PM +0000, Ferruh Yigit wrote:
> >> On 3/2/2017 11:32 AM, Shijith Thotton wrote:
> >>> Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
> >>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>> Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
> >>> Signed-off-by: Venkat Koppula <venkat.koppula@caviumnetworks.com>
> >>> Signed-off-by: Srisivasubramanian S <ssrinivasan@caviumnetworks.com>
> >>> Signed-off-by: Mallesham Jatharakonda <mjatharakonda@oneconvergence.com>
> >>
> >> <...>
> >>
> >>>  
> >>>  static int
> >>> +lio_dev_change_vf_mtu(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
> >>> +{
> >>> +	struct lio_device *lio_dev = LIO_DEV(eth_dev);
> >>> +
> >>> +	PMD_INIT_FUNC_TRACE();
> >>> +
> >>> +	if (!lio_dev->intf_open) {
> >>> +		lio_dev_err(lio_dev, "Port %d down, can't change MTU\n",
> >>> +			    lio_dev->port_id);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	/* Limit the MTU to make sure the ethernet packets are between
> >>> +	 * ETHER_MIN_MTU bytes and PF's MTU
> >>> +	 */
> >>> +	if ((new_mtu < ETHER_MIN_MTU) ||
> >>> +			(new_mtu > lio_dev->linfo.link.s.mtu)) {
> >>> +		lio_dev_err(lio_dev, "Invalid MTU: %d\n", new_mtu);
> >>> +		lio_dev_err(lio_dev, "Valid range %d and %d\n",
> >>> +			    ETHER_MIN_MTU, lio_dev->linfo.link.s.mtu);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>
> >> Is this really sets the MTU?
> >> "new_mtu" seems not used, except limit check, an lio_send_ctrl_pkt()
> >> required perhaps?
> > 
> > It won't set MTU for hardware and is possible only by PF. So
> > lio_send_ctrl_pkt is not required. VF MTU is limited by PF MTU and is
> > mentioned under limitations in driver documentation. Here we are
> > allowing upper layer to set MTU up to the value configured by PF.
> 
> I see, but lio_dev_change_vf_mtu() does not set anything at all. If it
> is not modifying anything at all, why you claim "MTU update" supported?

We allow update for the upper layer till the value supported by PF even
though there are no real MTU update happening at hardware level.
Thought it is ok to have it mentioned under limitation.
Two options are:
1. mark support as partial.
2. remove this patch and support.

Please suggest which one is better.

> 
> And following logic seems wrong for this case:
> 
> 	...
> 	if (lio_dev->linfo.link.s.mtu != mtu) {
> 		ret = lio_dev_change_vf_mtu(eth_dev, mtu);
> 	...
> 
> Should this functions set lio_dev->linfo.link.s.mtu at least, perhaps?

lio_dev->linfo.link.s.mtu represents the MTU supported by the device and
it gets updated if there is a change by PF.

> 
<...>
Ferruh Yigit March 21, 2017, 2:09 p.m. UTC | #5 
On 3/21/2017 1:56 PM, Shijith Thotton wrote:
> On Tue, Mar 21, 2017 at 01:01:58PM +0000, Ferruh Yigit wrote:
>> On 3/21/2017 12:53 PM, Shijith Thotton wrote:
>>> On Tue, Mar 21, 2017 at 12:24:49PM +0000, Ferruh Yigit wrote:
>>>> On 3/2/2017 11:32 AM, Shijith Thotton wrote:
>>>>> Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
>>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>> Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
>>>>> Signed-off-by: Venkat Koppula <venkat.koppula@caviumnetworks.com>
>>>>> Signed-off-by: Srisivasubramanian S <ssrinivasan@caviumnetworks.com>
>>>>> Signed-off-by: Mallesham Jatharakonda <mjatharakonda@oneconvergence.com>
>>>>
>>>> <...>
>>>>
>>>>>  
>>>>>  static int
>>>>> +lio_dev_change_vf_mtu(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
>>>>> +{
>>>>> +	struct lio_device *lio_dev = LIO_DEV(eth_dev);
>>>>> +
>>>>> +	PMD_INIT_FUNC_TRACE();
>>>>> +
>>>>> +	if (!lio_dev->intf_open) {
>>>>> +		lio_dev_err(lio_dev, "Port %d down, can't change MTU\n",
>>>>> +			    lio_dev->port_id);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	/* Limit the MTU to make sure the ethernet packets are between
>>>>> +	 * ETHER_MIN_MTU bytes and PF's MTU
>>>>> +	 */
>>>>> +	if ((new_mtu < ETHER_MIN_MTU) ||
>>>>> +			(new_mtu > lio_dev->linfo.link.s.mtu)) {
>>>>> +		lio_dev_err(lio_dev, "Invalid MTU: %d\n", new_mtu);
>>>>> +		lio_dev_err(lio_dev, "Valid range %d and %d\n",
>>>>> +			    ETHER_MIN_MTU, lio_dev->linfo.link.s.mtu);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>
>>>> Is this really sets the MTU?
>>>> "new_mtu" seems not used, except limit check, an lio_send_ctrl_pkt()
>>>> required perhaps?
>>>
>>> It won't set MTU for hardware and is possible only by PF. So
>>> lio_send_ctrl_pkt is not required. VF MTU is limited by PF MTU and is
>>> mentioned under limitations in driver documentation. Here we are
>>> allowing upper layer to set MTU up to the value configured by PF.
>>
>> I see, but lio_dev_change_vf_mtu() does not set anything at all. If it
>> is not modifying anything at all, why you claim "MTU update" supported?
> 
> We allow update for the upper layer till the value supported by PF even
> though there are no real MTU update happening at hardware level.
> Thought it is ok to have it mentioned under limitation.
> Two options are:
> 1. mark support as partial.
> 2. remove this patch and support.
> 
> Please suggest which one is better.

If I get it right, it is not possible to set VF MTU, so I would suggest
removing MTU update support.

But you may want to keep the patch for MTU validation, and change
function name according.

> 
>>
>> And following logic seems wrong for this case:
>>
>> 	...
>> 	if (lio_dev->linfo.link.s.mtu != mtu) {
>> 		ret = lio_dev_change_vf_mtu(eth_dev, mtu);
>> 	...
>>
>> Should this functions set lio_dev->linfo.link.s.mtu at least, perhaps?
> 
> lio_dev->linfo.link.s.mtu represents the MTU supported by the device and
> it gets updated if there is a change by PF.

So, "lio_dev->linfo.link.s.mtu" is device MTU set by PF, "mtu" is VF
eth_dev configured value.
If they are not same, lio_dev_change_vf_mtu() does check if "mtu" is in
valid range and return success or failure, right?
So, this is just configuration validation, nothing changed/set here.

> 
>>
> <...>
>
Shijith Thotton March 23, 2017, 5:02 a.m. UTC | #6 
On Tue, Mar 21, 2017 at 02:09:44PM +0000, Ferruh Yigit wrote:
> On 3/21/2017 1:56 PM, Shijith Thotton wrote:
> > On Tue, Mar 21, 2017 at 01:01:58PM +0000, Ferruh Yigit wrote:
> >> On 3/21/2017 12:53 PM, Shijith Thotton wrote:
> >>> On Tue, Mar 21, 2017 at 12:24:49PM +0000, Ferruh Yigit wrote:
> >>>> On 3/2/2017 11:32 AM, Shijith Thotton wrote:
> >>>>> Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
> >>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>>>> Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
> >>>>> Signed-off-by: Venkat Koppula <venkat.koppula@caviumnetworks.com>
> >>>>> Signed-off-by: Srisivasubramanian S <ssrinivasan@caviumnetworks.com>
> >>>>> Signed-off-by: Mallesham Jatharakonda <mjatharakonda@oneconvergence.com>
> >>>>
> >>>> <...>
> >>>>
> >>>>>  
> >>>>>  static int
> >>>>> +lio_dev_change_vf_mtu(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
> >>>>> +{
> >>>>> +	struct lio_device *lio_dev = LIO_DEV(eth_dev);
> >>>>> +
> >>>>> +	PMD_INIT_FUNC_TRACE();
> >>>>> +
> >>>>> +	if (!lio_dev->intf_open) {
> >>>>> +		lio_dev_err(lio_dev, "Port %d down, can't change MTU\n",
> >>>>> +			    lio_dev->port_id);
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>> +
> >>>>> +	/* Limit the MTU to make sure the ethernet packets are between
> >>>>> +	 * ETHER_MIN_MTU bytes and PF's MTU
> >>>>> +	 */
> >>>>> +	if ((new_mtu < ETHER_MIN_MTU) ||
> >>>>> +			(new_mtu > lio_dev->linfo.link.s.mtu)) {
> >>>>> +		lio_dev_err(lio_dev, "Invalid MTU: %d\n", new_mtu);
> >>>>> +		lio_dev_err(lio_dev, "Valid range %d and %d\n",
> >>>>> +			    ETHER_MIN_MTU, lio_dev->linfo.link.s.mtu);
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>>
> >>>> Is this really sets the MTU?
> >>>> "new_mtu" seems not used, except limit check, an lio_send_ctrl_pkt()
> >>>> required perhaps?
> >>>
> >>> It won't set MTU for hardware and is possible only by PF. So
> >>> lio_send_ctrl_pkt is not required. VF MTU is limited by PF MTU and is
> >>> mentioned under limitations in driver documentation. Here we are
> >>> allowing upper layer to set MTU up to the value configured by PF.
> >>
> >> I see, but lio_dev_change_vf_mtu() does not set anything at all. If it
> >> is not modifying anything at all, why you claim "MTU update" supported?
> > 
> > We allow update for the upper layer till the value supported by PF even
> > though there are no real MTU update happening at hardware level.
> > Thought it is ok to have it mentioned under limitation.
> > Two options are:
> > 1. mark support as partial.
> > 2. remove this patch and support.
> > 
> > Please suggest which one is better.
> 
> If I get it right, it is not possible to set VF MTU, so I would suggest
> removing MTU update support.

OK.

> 
> But you may want to keep the patch for MTU validation, and change
> function name according.

Will change set to validate.

> 
> > 
> >>
> >> And following logic seems wrong for this case:
> >>
> >> 	...
> >> 	if (lio_dev->linfo.link.s.mtu != mtu) {
> >> 		ret = lio_dev_change_vf_mtu(eth_dev, mtu);
> >> 	...
> >>
> >> Should this functions set lio_dev->linfo.link.s.mtu at least, perhaps?
> > 
> > lio_dev->linfo.link.s.mtu represents the MTU supported by the device and
> > it gets updated if there is a change by PF.
> 
> So, "lio_dev->linfo.link.s.mtu" is device MTU set by PF, "mtu" is VF
> eth_dev configured value.
> If they are not same, lio_dev_change_vf_mtu() does check if "mtu" is in
> valid range and return success or failure, right?
> So, this is just configuration validation, nothing changed/set here.
> 
> > 
> >>
> > <...>
> > 
>
diff mbox

Patch

diff --git a/drivers/net/liquidio/lio_ethdev.c b/drivers/net/liquidio/lio_ethdev.c
index e2040b9..167d41e 100644
--- a/drivers/net/liquidio/lio_ethdev.c
+++ b/drivers/net/liquidio/lio_ethdev.c
@@ -152,6 +152,33 @@ 
 }
 
 static int
+lio_dev_change_vf_mtu(struct rte_eth_dev *eth_dev, uint16_t new_mtu)
+{
+	struct lio_device *lio_dev = LIO_DEV(eth_dev);
+
+	PMD_INIT_FUNC_TRACE();
+
+	if (!lio_dev->intf_open) {
+		lio_dev_err(lio_dev, "Port %d down, can't change MTU\n",
+			    lio_dev->port_id);
+		return -EINVAL;
+	}
+
+	/* Limit the MTU to make sure the ethernet packets are between
+	 * ETHER_MIN_MTU bytes and PF's MTU
+	 */
+	if ((new_mtu < ETHER_MIN_MTU) ||
+			(new_mtu > lio_dev->linfo.link.s.mtu)) {
+		lio_dev_err(lio_dev, "Invalid MTU: %d\n", new_mtu);
+		lio_dev_err(lio_dev, "Valid range %d and %d\n",
+			    ETHER_MIN_MTU, lio_dev->linfo.link.s.mtu);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int
 lio_dev_rss_reta_update(struct rte_eth_dev *eth_dev,
 			struct rte_eth_rss_reta_entry64 *reta_conf,
 			uint16_t reta_size)
@@ -824,7 +851,9 @@ 
 static int
 lio_dev_start(struct rte_eth_dev *eth_dev)
 {
+	uint16_t mtu = eth_dev->data->dev_conf.rxmode.max_rx_pkt_len;
 	struct lio_device *lio_dev = LIO_DEV(eth_dev);
+	uint16_t timeout = LIO_MAX_CMD_TIMEOUT;
 	int ret = 0;
 
 	lio_dev_info(lio_dev, "Starting port %d\n", eth_dev->data->port_id);
@@ -852,8 +881,25 @@ 
 		goto dev_lsc_handle_error;
 	}
 
+	while ((lio_dev->linfo.link.link_status64 == 0) && (--timeout))
+		rte_delay_ms(1);
+
+	if (lio_dev->linfo.link.link_status64 == 0) {
+		ret = -1;
+		goto dev_mtu_updation_error;
+	}
+
+	if (lio_dev->linfo.link.s.mtu != mtu) {
+		ret = lio_dev_change_vf_mtu(eth_dev, mtu);
+		if (ret)
+			goto dev_mtu_updation_error;
+	}
+
 	return 0;
 
+dev_mtu_updation_error:
+	rte_eal_alarm_cancel(lio_sync_link_state_check, eth_dev);
+
 dev_lsc_handle_error:
 	lio_dev->intf_open = 0;
 	lio_send_rx_ctrl_cmd(eth_dev, 0);
@@ -1034,6 +1080,7 @@  static int lio_dev_configure(struct rte_eth_dev *eth_dev)
 	.dev_start		= lio_dev_start,
 	.link_update		= lio_dev_link_update,
 	.dev_infos_get		= lio_dev_info_get,
+	.mtu_set		= lio_dev_change_vf_mtu,
 	.rx_queue_setup		= lio_dev_rx_queue_setup,
 	.rx_queue_release	= lio_dev_rx_queue_release,
 	.tx_queue_setup		= lio_dev_tx_queue_setup,