[v3,2/2] drivers/net: remove explicit include of legacy filtering
Checks
Commit Message
The header file rte_eth_ctrl.h should not be needed because
this legacy filtering API is completely replaced with the rte_flow API.
However some definitions from this file are still used by some drivers,
but such usage is already covered by an implicit include via rte_ethdev.h.
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Rosen Xu <rosen.xu@intel.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
drivers/net/dpaa2/dpaa2_ptp.c | 1 -
drivers/net/iavf/iavf_hash.c | 1 -
drivers/net/ice/ice_acl_filter.c | 1 -
drivers/net/ice/ice_hash.c | 1 -
drivers/net/ice/ice_switch_filter.c | 1 -
drivers/net/igc/igc_filter.h | 1 -
drivers/net/ipn3ke/ipn3ke_flow.c | 1 -
7 files changed, 7 deletions(-)
Comments
On 3/21/2021 9:00 AM, Thomas Monjalon wrote:
> The header file rte_eth_ctrl.h should not be needed because
> this legacy filtering API is completely replaced with the rte_flow API.
> However some definitions from this file are still used by some drivers,
> but such usage is already covered by an implicit include via rte_ethdev.h.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: Rosen Xu <rosen.xu@intel.com>
> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> drivers/net/dpaa2/dpaa2_ptp.c | 1 -
> drivers/net/iavf/iavf_hash.c | 1 -
> drivers/net/ice/ice_acl_filter.c | 1 -
> drivers/net/ice/ice_hash.c | 1 -
> drivers/net/ice/ice_switch_filter.c | 1 -
> drivers/net/igc/igc_filter.h | 1 -
> drivers/net/ipn3ke/ipn3ke_flow.c | 1 -
Although this will work, if the above drives are using the defines from the
header file, isn't it better to include it explicitly?
What is the benefit of including the header implicitly?
24/03/2021 19:08, Ferruh Yigit:
> On 3/21/2021 9:00 AM, Thomas Monjalon wrote:
> > The header file rte_eth_ctrl.h should not be needed because
> > this legacy filtering API is completely replaced with the rte_flow API.
> > However some definitions from this file are still used by some drivers,
> > but such usage is already covered by an implicit include via rte_ethdev.h.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > Acked-by: Rosen Xu <rosen.xu@intel.com>
> > Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> > ---
> > drivers/net/dpaa2/dpaa2_ptp.c | 1 -
> > drivers/net/iavf/iavf_hash.c | 1 -
> > drivers/net/ice/ice_acl_filter.c | 1 -
> > drivers/net/ice/ice_hash.c | 1 -
> > drivers/net/ice/ice_switch_filter.c | 1 -
> > drivers/net/igc/igc_filter.h | 1 -
> > drivers/net/ipn3ke/ipn3ke_flow.c | 1 -
>
> Although this will work, if the above drives are using the defines from the
> header file, isn't it better to include it explicitly?
>
> What is the benefit of including the header implicitly?
The benefit is to progressively remove rte_eth_ctrl.h.
I want it to disappear.
On 3/24/21 11:00 PM, Thomas Monjalon wrote:
> 24/03/2021 19:08, Ferruh Yigit:
>> On 3/21/2021 9:00 AM, Thomas Monjalon wrote:
>>> The header file rte_eth_ctrl.h should not be needed because
>>> this legacy filtering API is completely replaced with the rte_flow API.
>>> However some definitions from this file are still used by some drivers,
>>> but such usage is already covered by an implicit include via rte_ethdev.h.
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>> Acked-by: Rosen Xu <rosen.xu@intel.com>
>>> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>> ---
>>> drivers/net/dpaa2/dpaa2_ptp.c | 1 -
>>> drivers/net/iavf/iavf_hash.c | 1 -
>>> drivers/net/ice/ice_acl_filter.c | 1 -
>>> drivers/net/ice/ice_hash.c | 1 -
>>> drivers/net/ice/ice_switch_filter.c | 1 -
>>> drivers/net/igc/igc_filter.h | 1 -
>>> drivers/net/ipn3ke/ipn3ke_flow.c | 1 -
>>
>> Although this will work, if the above drives are using the defines from the
>> header file, isn't it better to include it explicitly?
>>
>> What is the benefit of including the header implicitly?
>
> The benefit is to progressively remove rte_eth_ctrl.h.
> I want it to disappear.
>
+1
On 3/25/2021 5:53 AM, Andrew Rybchenko wrote:
> On 3/24/21 11:00 PM, Thomas Monjalon wrote:
>> 24/03/2021 19:08, Ferruh Yigit:
>>> On 3/21/2021 9:00 AM, Thomas Monjalon wrote:
>>>> The header file rte_eth_ctrl.h should not be needed because
>>>> this legacy filtering API is completely replaced with the rte_flow API.
>>>> However some definitions from this file are still used by some drivers,
>>>> but such usage is already covered by an implicit include via rte_ethdev.h.
>>>>
>>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>>> Acked-by: Rosen Xu <rosen.xu@intel.com>
>>>> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>>> ---
>>>> drivers/net/dpaa2/dpaa2_ptp.c | 1 -
>>>> drivers/net/iavf/iavf_hash.c | 1 -
>>>> drivers/net/ice/ice_acl_filter.c | 1 -
>>>> drivers/net/ice/ice_hash.c | 1 -
>>>> drivers/net/ice/ice_switch_filter.c | 1 -
>>>> drivers/net/igc/igc_filter.h | 1 -
>>>> drivers/net/ipn3ke/ipn3ke_flow.c | 1 -
>>>
>>> Although this will work, if the above drives are using the defines from the
>>> header file, isn't it better to include it explicitly?
>>>
>>> What is the benefit of including the header implicitly?
>>
>> The benefit is to progressively remove rte_eth_ctrl.h.
>> I want it to disappear.
>>
>
> +1
>
This is just hiding its usage, the patch is not making it less used as a step
forward to remove it.
But anyway I guess it doesn't worth spending more time to discuss it ...
25/03/2021 11:00, Ferruh Yigit:
> On 3/25/2021 5:53 AM, Andrew Rybchenko wrote:
> > On 3/24/21 11:00 PM, Thomas Monjalon wrote:
> >> 24/03/2021 19:08, Ferruh Yigit:
> >>> On 3/21/2021 9:00 AM, Thomas Monjalon wrote:
> >>>> The header file rte_eth_ctrl.h should not be needed because
> >>>> this legacy filtering API is completely replaced with the rte_flow API.
> >>>> However some definitions from this file are still used by some drivers,
> >>>> but such usage is already covered by an implicit include via rte_ethdev.h.
> >>>>
> >>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >>>> Acked-by: Rosen Xu <rosen.xu@intel.com>
> >>>> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >>>> ---
> >>>> drivers/net/dpaa2/dpaa2_ptp.c | 1 -
> >>>> drivers/net/iavf/iavf_hash.c | 1 -
> >>>> drivers/net/ice/ice_acl_filter.c | 1 -
> >>>> drivers/net/ice/ice_hash.c | 1 -
> >>>> drivers/net/ice/ice_switch_filter.c | 1 -
> >>>> drivers/net/igc/igc_filter.h | 1 -
> >>>> drivers/net/ipn3ke/ipn3ke_flow.c | 1 -
> >>>
> >>> Although this will work, if the above drives are using the defines from the
> >>> header file, isn't it better to include it explicitly?
> >>>
> >>> What is the benefit of including the header implicitly?
> >>
> >> The benefit is to progressively remove rte_eth_ctrl.h.
> >> I want it to disappear.
> >>
> >
> > +1
> >
>
> This is just hiding its usage, the patch is not making it less used as a step
> forward to remove it.
Yes you're right. The only step forward is esthetic:
hiding something which should be removed.
And maybe some of these files don't need the include at all.
> But anyway I guess it doesn't worth spending more time to discuss it ...
Feel free to reject if you feel it is not a good step.
On 3/25/2021 10:20 AM, Thomas Monjalon wrote:
> 25/03/2021 11:00, Ferruh Yigit:
>> On 3/25/2021 5:53 AM, Andrew Rybchenko wrote:
>>> On 3/24/21 11:00 PM, Thomas Monjalon wrote:
>>>> 24/03/2021 19:08, Ferruh Yigit:
>>>>> On 3/21/2021 9:00 AM, Thomas Monjalon wrote:
>>>>>> The header file rte_eth_ctrl.h should not be needed because
>>>>>> this legacy filtering API is completely replaced with the rte_flow API.
>>>>>> However some definitions from this file are still used by some drivers,
>>>>>> but such usage is already covered by an implicit include via rte_ethdev.h.
>>>>>>
>>>>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>>>>> Acked-by: Rosen Xu <rosen.xu@intel.com>
>>>>>> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>>>>> ---
>>>>>> drivers/net/dpaa2/dpaa2_ptp.c | 1 -
>>>>>> drivers/net/iavf/iavf_hash.c | 1 -
>>>>>> drivers/net/ice/ice_acl_filter.c | 1 -
>>>>>> drivers/net/ice/ice_hash.c | 1 -
>>>>>> drivers/net/ice/ice_switch_filter.c | 1 -
>>>>>> drivers/net/igc/igc_filter.h | 1 -
>>>>>> drivers/net/ipn3ke/ipn3ke_flow.c | 1 -
>>>>>
>>>>> Although this will work, if the above drives are using the defines from the
>>>>> header file, isn't it better to include it explicitly?
>>>>>
>>>>> What is the benefit of including the header implicitly?
>>>>
>>>> The benefit is to progressively remove rte_eth_ctrl.h.
>>>> I want it to disappear.
>>>>
>>>
>>> +1
>>>
>>
>> This is just hiding its usage, the patch is not making it less used as a step
>> forward to remove it.
>
> Yes you're right. The only step forward is esthetic:
> hiding something which should be removed.
> And maybe some of these files don't need the include at all.
>
>> But anyway I guess it doesn't worth spending more time to discuss it ...
>
> Feel free to reject if you feel it is not a good step.
>
What do you think doing exact opposite,
remove "#include <rte_eth_ctrl.h>" from 'rte_ethdev.h',
and include 'rte_eth_ctrl.h' explicitly where ever it is required,
this can make more clear what components use the 'rte_eth_ctrl.h' and why, which
may help clearing them to remove the header. Plus it highlights if a new PMD
wants to use the header, we can catch it easier.
When I tried above approach, it highlighted that not only drivers, libraries
also using this header, 'librte_ehtdev' & 'librte_flow_classify'.
And for 'ethdev', the structs defined in the 'rte_eth_ctrl.h' are part of public
structs, so it is hard to remove them.
Some PMD specific APIs also needs 'rte_eth_ctrl.h' header, but that is hidden
because of the implicit include, but again some structs in the 'rte_eth_ctrl.h'
are part of public APIs (although they are experimental).
Also, it turned out that same required headers in the drivers are hidden because
of this implicit include in 'rte_ethdev.h', I will send a fix for it soon.
26/03/2021 16:37, Ferruh Yigit:
> On 3/25/2021 10:20 AM, Thomas Monjalon wrote:
> > 25/03/2021 11:00, Ferruh Yigit:
> >> On 3/25/2021 5:53 AM, Andrew Rybchenko wrote:
> >>> On 3/24/21 11:00 PM, Thomas Monjalon wrote:
> >>>> 24/03/2021 19:08, Ferruh Yigit:
> >>>>> On 3/21/2021 9:00 AM, Thomas Monjalon wrote:
> >>>>>> The header file rte_eth_ctrl.h should not be needed because
> >>>>>> this legacy filtering API is completely replaced with the rte_flow API.
> >>>>>> However some definitions from this file are still used by some drivers,
> >>>>>> but such usage is already covered by an implicit include via rte_ethdev.h.
> >>>>>>
> >>>>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >>>>>> Acked-by: Rosen Xu <rosen.xu@intel.com>
> >>>>>> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >>>>>> ---
> >>>>>> drivers/net/dpaa2/dpaa2_ptp.c | 1 -
> >>>>>> drivers/net/iavf/iavf_hash.c | 1 -
> >>>>>> drivers/net/ice/ice_acl_filter.c | 1 -
> >>>>>> drivers/net/ice/ice_hash.c | 1 -
> >>>>>> drivers/net/ice/ice_switch_filter.c | 1 -
> >>>>>> drivers/net/igc/igc_filter.h | 1 -
> >>>>>> drivers/net/ipn3ke/ipn3ke_flow.c | 1 -
> >>>>>
> >>>>> Although this will work, if the above drives are using the defines from the
> >>>>> header file, isn't it better to include it explicitly?
> >>>>>
> >>>>> What is the benefit of including the header implicitly?
> >>>>
> >>>> The benefit is to progressively remove rte_eth_ctrl.h.
> >>>> I want it to disappear.
> >>>>
> >>>
> >>> +1
> >>>
> >>
> >> This is just hiding its usage, the patch is not making it less used as a step
> >> forward to remove it.
> >
> > Yes you're right. The only step forward is esthetic:
> > hiding something which should be removed.
> > And maybe some of these files don't need the include at all.
> >
> >> But anyway I guess it doesn't worth spending more time to discuss it ...
> >
> > Feel free to reject if you feel it is not a good step.
> >
>
> What do you think doing exact opposite,
>
> remove "#include <rte_eth_ctrl.h>" from 'rte_ethdev.h',
> and include 'rte_eth_ctrl.h' explicitly where ever it is required,
>
> this can make more clear what components use the 'rte_eth_ctrl.h' and why, which
> may help clearing them to remove the header. Plus it highlights if a new PMD
> wants to use the header, we can catch it easier.
>
> When I tried above approach, it highlighted that not only drivers, libraries
> also using this header, 'librte_ehtdev' & 'librte_flow_classify'.
> And for 'ethdev', the structs defined in the 'rte_eth_ctrl.h' are part of public
> structs, so it is hard to remove them.
> Some PMD specific APIs also needs 'rte_eth_ctrl.h' header, but that is hidden
> because of the implicit include, but again some structs in the 'rte_eth_ctrl.h'
> are part of public APIs (although they are experimental).
>
> Also, it turned out that same required headers in the drivers are hidden because
> of this implicit include in 'rte_ethdev.h', I will send a fix for it soon.
OK thanks
@@ -12,7 +12,6 @@
#include <rte_ethdev.h>
#include <rte_log.h>
-#include <rte_eth_ctrl.h>
#include <rte_malloc.h>
#include <rte_time.h>
@@ -15,7 +15,6 @@
#include <ethdev_driver.h>
#include <rte_log.h>
#include <rte_malloc.h>
-#include <rte_eth_ctrl.h>
#include <rte_tailq.h>
#include <rte_flow_driver.h>
@@ -14,7 +14,6 @@
#include <ethdev_driver.h>
#include <rte_log.h>
#include <rte_malloc.h>
-#include <rte_eth_ctrl.h>
#include <rte_tailq.h>
#include <rte_flow_driver.h>
#include <rte_flow.h>
@@ -15,7 +15,6 @@
#include <ethdev_driver.h>
#include <rte_log.h>
#include <rte_malloc.h>
-#include <rte_eth_ctrl.h>
#include <rte_tailq.h>
#include <rte_flow_driver.h>
@@ -14,7 +14,6 @@
#include <ethdev_driver.h>
#include <rte_log.h>
#include <rte_malloc.h>
-#include <rte_eth_ctrl.h>
#include <rte_tailq.h>
#include <rte_flow_driver.h>
#include <rte_flow.h>
@@ -9,7 +9,6 @@
#include <rte_ethdev.h>
#include <rte_ethdev_core.h>
#include <ethdev_driver.h>
-#include <rte_eth_ctrl.h>
#include "igc_ethdev.h"
@@ -16,7 +16,6 @@
#include <ethdev_driver.h>
#include <rte_log.h>
#include <rte_malloc.h>
-#include <rte_eth_ctrl.h>
#include <rte_tailq.h>
#include <rte_rawdev.h>
#include <rte_rawdev_pmd.h>