On 8/12/2018 9:46 AM, Shahaf Shuler wrote:
> Sunday, August 12, 2018 10:53 AM, Andrew Rybchenko:
>> Subject: Re: [PATCH] net/ixgbe: remove hardcoded CRC STRIP config from
>> ixgbe
>>
>> On 12.08.2018 09:28, Shahaf Shuler wrote:
>>> Thursday, August 9, 2018 11:32 AM, Ferruh Yigit:
>>>> Subject: Re: [PATCH] net/ixgbe: remove hardcoded CRC STRIP config
>>>> from ixgbe
>>>>
>>>> On 7/24/2018 3:36 AM, Wei Zhao wrote:
>>>>> There is CRC related ifdefs for ixgbe:
>>>>> CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
>>>>> It is used in VF drivers ixgbevf_dev_configure() functions.
>>>>> VF cannot change the CRC strip behavior and based on what PF
>>>>> configured it needs to response proper to user
>>>>> ixgbevf_dev_configure() request. Right now what PF set is defined by
>>>>> above config options but this method is too static.
>>>>>
>>>>> Signed-off-by: Wei Zhao <wei.zh...@intel.com>
>>>>> Signed-off-by: Wenzhuo Lu <wenzhuo...@intel.com>
>>>> <...>
>>>>> @@ -334,6 +334,7 @@ struct rte_eth_rxmode {
>>>>> * structure are allowed to be set.
>>>>> */
>>>>> uint64_t offloads;
>>>>> + uint64_t offloads_disable;
>>>> Do we need a disable flag in ethdev, an offload not enabled is
>>>> disabled by default isn't it. This conflicts with offloads flag and makes
>>>> is
>> confusing.
>>> +1.
>>>
>>> **all** the offloads are disabled by default.
>>>
>>>> For igb/e1000 VF case, VF driver can't change what PF set and VF
>>>> driver can't learn the PF setting dynamically, so this information
>>>> needs to be passed to VF driver by application/user.
>>>> Currently this information passed by compile time config option, my
>>>> suggestion was using devargs.
>>>>
>>>> In your implementation testpmd parameter added to get this
>>>> information and pass to driver, but this means all applications needs
>>>> to do this, instead adding this support to driver looks better to me.
>>
>> I think we should add fixed offloads to dev_info. I.e. if offlload is
>> supported,
>> it could be marked as fixed (i.e. always enabled).
>> If offload is not supported, it is always disabled (and cannot/should not be
>> marked as fixed).
>> May be the right name for it is not "fixed", but "always_enabled".
>
> I think it will over complicate applications. Those limitation should be
> expressed as part of the "limitation" section of the corresponding PMD guide.
>
> PMDs with such limitation can also put some warning message to notify or even
> fail the device configuration if the needed permanent offload is not set by
> the application.
Some PMDs already have these warning messages, for the case device supports an
offload, so it is in advertised capabilities, but doesn't support disabling it.
"Can't disable"/"always on" information from PMD is missing now, it would be
nice to get it from PMD but I agree that it will complicate things.
And this won't help the ixgbe VF case anyway, for that case if offload can be
enabled/disable in VF depends on PF configuration, so it is not a fixed
information for VF that you can put into driver code.
>
>> Also it should be persistent. It should not be allowed in above ixgbe case to
>> change the offload state on PF if there are other users (drivers attached).
>> Otherwise, we need mechanism to notify apps about these changes -
>> overcomplicated.
