Re: [PATCH 6/7] net/qede: fix maximum VF count to 0

From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Harish Patil <harish.patil@qlogic.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: Dept-Eng DPDK Dev <Dept-EngDPDKDev@qlogic.com>,
	Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH 6/7] net/qede: fix maximum VF count to 0
Date: Mon, 12 Dec 2016 18:13:48 +0000	[thread overview]
Message-ID: <eabebd19-fd7e-4c1e-e921-13c809f79f6e@intel.com> (raw)
In-Reply-To: <D47420A6.C0178%Harish.Patil@cavium.com>

On 12/12/2016 5:47 PM, Harish Patil wrote:
> 
>> On 12/3/2016 2:43 AM, Harish Patil wrote:
>>> Set max_vfs to 0 since it is relevant only to SR-IOV PF
>>> which is not supported yet.
>>>
>>> Fixes: 2ea6f76a ("qede: add core driver")
>>>
>>> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
>>
>> Can you please update patch title to indicate what has been fixed
>> instead of what has been done in the patch.
> 
> Can you please clarify? The change in the patch is to set max_vfs=0.
> So that’s why the title - "fix maximum VF count to 0"

Let me try, if I can :)

I can see what patch does, and I got the reasoning behind.

Previously driver was reporting as it was supporting SR-IOV with DPDK
PF, right? But it was wrong and you are fixing it.

How you fix is by setting max_vfs=0, but that is the technical detail of
the fix.

What you are trying to fix is not setting a variable to a specific
value, what you are trying to fix is SR-IOV support.

I hope I can make it more clear.

> 
>>
>>
>> btw, while checking feature list, I have seen qede_vf supports SR-IOV,
>> is that correct?
> 
> Yes. The supported combination for SR-IOV is VF driver (qede PMD) with PF
> driver (qede linux driver).

So you are using SR-IOV feature set in VF driver, as meaning VF driver
support exists. I don't know what SR-IOV feature mean for VF drivers.
Some other VF drivers not has this feature flag set.

CC'ed Thomas for help, if this is the intention of the feature flag, it
is OK.

> We don’t have support for SR-IOV PF driver yet. When SR-IOV PF driver is
> supported then max_vfs shall return the actual max VFs, but for now it
> should return max_vfs=0. Hope it is clear.
> 
>>
>>> ---
>>>  drivers/net/qede/qede_ethdev.c | 5 +----
>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/qede/qede_ethdev.c
>>> b/drivers/net/qede/qede_ethdev.c
>>> index ee8fb43..10abb8b 100644
>>> --- a/drivers/net/qede/qede_ethdev.c
>>> +++ b/drivers/net/qede/qede_ethdev.c
>>> @@ -976,10 +976,7 @@ static int qede_dev_configure(struct rte_eth_dev
>>> *eth_dev)
>>>  	dev_info->max_rx_queues = (uint16_t)QEDE_MAX_RSS_CNT(qdev);
>>>  	dev_info->max_tx_queues = dev_info->max_rx_queues;
>>>  	dev_info->max_mac_addrs = qdev->dev_info.num_mac_addrs;
>>> -	if (IS_VF(edev))
>>> -		dev_info->max_vfs = 0;
>>> -	else
>>> -		dev_info->max_vfs = (uint16_t)NUM_OF_VFS(&qdev->edev);
>>> +	dev_info->max_vfs = 0;
>>>  	dev_info->reta_size = ECORE_RSS_IND_TABLE_SIZE;
>>>  	dev_info->hash_key_size = ECORE_RSS_KEY_SIZE * sizeof(uint32_t);
>>>  	dev_info->flow_type_rss_offloads = (uint64_t)QEDE_RSS_OFFLOAD_ALL;
>>>
>>
>>
> 
> 