diff mbox series

[v3,2/2] drivers/net: remove explicit include of legacy filtering

Message ID 20210321090002.595744-3-thomas@monjalon.net (mailing list archive)
State Rejected, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: remove some use of legacy filtering |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/travis-robot success travis build: passed
ci/github-robot success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Thomas Monjalon March 21, 2021, 9 a.m. UTC 
  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

Ferruh Yigit March 24, 2021, 6:08 p.m. UTC | #1 
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?
Thomas Monjalon March 24, 2021, 8 p.m. UTC | #2 
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.
Andrew Rybchenko March 25, 2021, 5:53 a.m. UTC | #3 
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
Ferruh Yigit March 25, 2021, 10 a.m. UTC | #4 
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 ...
Thomas Monjalon March 25, 2021, 10:20 a.m. UTC | #5 
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.
Ferruh Yigit March 26, 2021, 3:37 p.m. UTC | #6 
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.
Thomas Monjalon March 26, 2021, 9:45 p.m. UTC | #7 
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
diff mbox series

Patch

diff --git a/drivers/net/dpaa2/dpaa2_ptp.c b/drivers/net/dpaa2/dpaa2_ptp.c
index 899dd5d442..6290a559d0 100644
--- a/drivers/net/dpaa2/dpaa2_ptp.c
+++ b/drivers/net/dpaa2/dpaa2_ptp.c
@@ -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>
 
diff --git a/drivers/net/iavf/iavf_hash.c b/drivers/net/iavf/iavf_hash.c
index d8d22f8009..82f017db3d 100644
--- a/drivers/net/iavf/iavf_hash.c
+++ b/drivers/net/iavf/iavf_hash.c
@@ -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>
 
diff --git a/drivers/net/ice/ice_acl_filter.c b/drivers/net/ice/ice_acl_filter.c
index 9e06e8a3de..2203720d15 100644
--- a/drivers/net/ice/ice_acl_filter.c
+++ b/drivers/net/ice/ice_acl_filter.c
@@ -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>
diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
index 58a0c18d09..c12c2c0dbc 100644
--- a/drivers/net/ice/ice_hash.c
+++ b/drivers/net/ice/ice_hash.c
@@ -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>
 
diff --git a/drivers/net/ice/ice_switch_filter.c b/drivers/net/ice/ice_switch_filter.c
index ada3ecf60b..fa034c0e0c 100644
--- a/drivers/net/ice/ice_switch_filter.c
+++ b/drivers/net/ice/ice_switch_filter.c
@@ -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>
diff --git a/drivers/net/igc/igc_filter.h b/drivers/net/igc/igc_filter.h
index 781d270def..fb51f7066c 100644
--- a/drivers/net/igc/igc_filter.h
+++ b/drivers/net/igc/igc_filter.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"
 
diff --git a/drivers/net/ipn3ke/ipn3ke_flow.c b/drivers/net/ipn3ke/ipn3ke_flow.c
index c702e19ea5..452fb447ef 100644
--- a/drivers/net/ipn3ke/ipn3ke_flow.c
+++ b/drivers/net/ipn3ke/ipn3ke_flow.c
@@ -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>