On Fri, Dec 21, 2018 at 01:59:20PM +0000, Ferruh Yigit wrote: > On 12/21/2018 1:16 PM, Gaëtan Rivet wrote: > > Hi Ferruh, Andrew, > > > > On Fri, Dec 21, 2018 at 03:52:07PM +0300, Andrew Rybchenko wrote: > >> Hi Ferruh, > >> > >> On 12/21/18 3:43 PM, Ferruh Yigit wrote: > >>> On 12/21/2018 12:28 PM, Andrew Rybchenko wrote: > >>>> On 12/21/18 3:12 PM, Ferruh Yigit wrote: > >>>>> On 10/12/2018 12:36 PM, Andrew Rybchenko wrote: > >>>>>> From: Ivan Malov <ivan.ma...@oktetlabs.ru> > >>>>>> > >>>>>> This capability is reported when supported by the current emitting > >>>>>> sub-device. Failsafe PMD itself does not excercise fast free logic. > >>>>> I think overlay device capability reporting already discussed a few > >>>>> times, the > >>>>> question is if an overlay devices should claim a feature when it > >>>>> depends on > >>>>> underlay devices? > >>>> The capability may be reported by the failsafe since it is transparent > >>>> from > >>>> fast free logic point of view. > >>> Why it is transparent? If one of the underlying device doesn't > >>> support/claim > >>> this feature, application can't use this feature with failsafe, isn't it? > > > > If a VF is forbidden by its PF from adding MAC addresses, the driver > > should still advertize support for "Unicast MAC filter" right? > > > > This is the same here. Fail-safe needs to forward configurations items > > to its sub-device for a feature to work. Sometimes, the hardware won't > > be sufficient. But the fail-safe itself still has the parts needed (even > > if it is only a flag to add to a feature list). > > I see your point, also I think it may be misleading for an overlay device to > announce a feature which is completely depends underlay devices. Anyway this > may > be a nuance. > > I am OK with the change after Andrew's explanation, and as far as I understand > you are OK too, may I add your explicit ack to the patch? >
Yes the patch is ok for me, thank you Ferruh. > > > > It is necessary, at the fail-safe level, to be able to describe the > > current feature set. This is what the feature list is for. There are > > important caveats to consider however, which is inherent to using the > > fail-safe. > > > > It does not mean those features should be removed from the fail-safe > > feature list. > > > >> > >> tx_offload_capa in failsafe is a mask to apply on sub-device capabilities. > >> So, if the capability is not supported by any sub-device it will not be > >> reported. > >> As well if there is the capability bit in the mask, it will not be reported > >> regardless > >> sub-devices capabilities. The description for the patch above tries to > >> explain it - > >> it looks like not that successful. > >> > >>>>> Given that no ack/review given to the patch, I am updating it as > >>>>> rejected. > >>>> Is it a new policy? I thought that it was vice versa before. > >>> Hi Andrew, > >>> > >>> Yes policy is other-way around indeed, when there is no comment at all > >>> default > >>> behavior is accept, but please take above paragraph as my comment to the > >>> patch. > >> > >> Got it. > >> > >>> And I was thinking it is a little controversial and there is no support > >>> to have > >>> it, so lets don't get it. What do you think? > >> > >> I see you motivation. > >> > >>>>>> Signed-off-by: Ivan Malov <ivan.ma...@oktetlabs.ru> > >>>>>> Signed-off-by: Andrew Rybchenko <arybche...@solarflare.com> > >>>>>> --- > >>>>>> doc/guides/nics/features/failsafe.ini | 1 + > >>>>>> drivers/net/failsafe/failsafe_ops.c | 1 + > >>>>>> 2 files changed, 2 insertions(+) > >>>>>> > >>>>>> diff --git a/doc/guides/nics/features/failsafe.ini > >>>>>> b/doc/guides/nics/features/failsafe.ini > >>>>>> index e3c4c08f2..b6f3dcee6 100644 > >>>>>> --- a/doc/guides/nics/features/failsafe.ini > >>>>>> +++ b/doc/guides/nics/features/failsafe.ini > >>>>>> @@ -7,6 +7,7 @@ > >>>>>> Link status = Y > >>>>>> Link status event = Y > >>>>>> Rx interrupt = Y > >>>>>> +Fast mbuf free = Y > >>>>>> Queue start/stop = Y > >>>>>> Runtime Rx queue setup = Y > >>>>>> Runtime Tx queue setup = Y > >>>>>> diff --git a/drivers/net/failsafe/failsafe_ops.c > >>>>>> b/drivers/net/failsafe/failsafe_ops.c > >>>>>> index 7f8bcd4c6..e3add404b 100644 > >>>>>> --- a/drivers/net/failsafe/failsafe_ops.c > >>>>>> +++ b/drivers/net/failsafe/failsafe_ops.c > >>>>>> @@ -78,6 +78,7 @@ static struct rte_eth_dev_info default_infos = { > >>>>>> DEV_RX_OFFLOAD_SECURITY, > >>>>>> .tx_offload_capa = > >>>>>> DEV_TX_OFFLOAD_MULTI_SEGS | > >>>>>> + DEV_TX_OFFLOAD_MBUF_FAST_FREE | > >>>>>> DEV_TX_OFFLOAD_IPV4_CKSUM | > >>>>>> DEV_TX_OFFLOAD_UDP_CKSUM | > >>>>>> DEV_TX_OFFLOAD_TCP_CKSUM | > >>>>>> > >> > > > -- Gaëtan Rivet 6WIND