Message ID | 1456779187-32336-1-git-send-email-bill.fischofer@linaro.org |
---|---|
State | Superseded |
Headers | show |
+cc Nicolas and Barry for their input. In debugging why the TM Cunit tests weren't working, I discovered that one of the issues was that they call odp_pktio_link_status() to verify that the link is "up" and fail if it is not. When running with a loopback interface these tests always fail because we omitted having a link_status handler for loopback devices so these calls always return -1. Since loopback interfaces are purely internal (start and stop for them are no-ops), it seems reasonable that they should always report link status up. Is there a reason why this isn't a good idea? Thanks. On Mon, Feb 29, 2016 at 2:53 PM, Bill Fischofer <bill.fischofer@linaro.org> wrote: > ODP loopback interfaces should always be considered up, so add a > handler for odp_pktio_link_status() for this device type that always > returns 1 to indicate the link is up. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/pktio/loop.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/platform/linux-generic/pktio/loop.c > b/platform/linux-generic/pktio/loop.c > index dbb0e7b..0ea6d0e 100644 > --- a/platform/linux-generic/pktio/loop.c > +++ b/platform/linux-generic/pktio/loop.c > @@ -134,6 +134,12 @@ static int loopback_mac_addr_get(pktio_entry_t > *pktio_entry ODP_UNUSED, > return ETH_ALEN; > } > > +static int loopback_link_status(pktio_entry_t *pktio_entry ODP_UNUSED) > +{ > + /* loopback interfaces are always up */ > + return 1; > +} > + > static int loopback_promisc_mode_set(pktio_entry_t *pktio_entry, > odp_bool_t enable) > { > @@ -176,6 +182,7 @@ const pktio_if_ops_t loopback_pktio_ops = { > .promisc_mode_set = loopback_promisc_mode_set, > .promisc_mode_get = loopback_promisc_mode_get, > .mac_get = loopback_mac_addr_get, > + .link_status = loopback_link_status, > .capability = NULL, > .input_queues_config = NULL, > .output_queues_config = NULL, > -- > 2.5.0 > >
Bill, Idea if ops is not defined that some common value has to be used. Instead of fixing loop it's needed to fix common code to return 1 if link_status is not implemented. int odp_pktio_link_status(odp_pktio_t id) { pktio_entry_t *entry; int ret = -1; <--- has to be 1 ..... if (entry->s.ops->link_status) ret = entry->s.ops->link_status(entry); unlock_entry(entry); return ret; } Maxim. On 02/29/16 23:53, Bill Fischofer wrote: > ODP loopback interfaces should always be considered up, so add a > handler for odp_pktio_link_status() for this device type that always > returns 1 to indicate the link is up. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/pktio/loop.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/platform/linux-generic/pktio/loop.c b/platform/linux-generic/pktio/loop.c > index dbb0e7b..0ea6d0e 100644 > --- a/platform/linux-generic/pktio/loop.c > +++ b/platform/linux-generic/pktio/loop.c > @@ -134,6 +134,12 @@ static int loopback_mac_addr_get(pktio_entry_t *pktio_entry ODP_UNUSED, > return ETH_ALEN; > } > > +static int loopback_link_status(pktio_entry_t *pktio_entry ODP_UNUSED) > +{ > + /* loopback interfaces are always up */ > + return 1; > +} > + > static int loopback_promisc_mode_set(pktio_entry_t *pktio_entry, > odp_bool_t enable) > { > @@ -176,6 +182,7 @@ const pktio_if_ops_t loopback_pktio_ops = { > .promisc_mode_set = loopback_promisc_mode_set, > .promisc_mode_get = loopback_promisc_mode_get, > .mac_get = loopback_mac_addr_get, > + .link_status = loopback_link_status, > .capability = NULL, > .input_queues_config = NULL, > .output_queues_config = NULL,
If it's not defined it's better to return -1. However the question is shouldn't a loop interface have its own definition for this function? I know link_status was added after the original restructure so perhaps this was just an omission? On Tue, Mar 1, 2016 at 8:53 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Bill, > > Idea if ops is not defined that some common value has to be used. > Instead of fixing loop it's needed to fix common code to return 1 if > link_status is not implemented. > > int odp_pktio_link_status(odp_pktio_t id) > { > pktio_entry_t *entry; > int ret = -1; <--- has to be 1 > ..... > if (entry->s.ops->link_status) > ret = entry->s.ops->link_status(entry); > unlock_entry(entry); > > return ret; > } > > Maxim. > > > > On 02/29/16 23:53, Bill Fischofer wrote: > >> ODP loopback interfaces should always be considered up, so add a >> handler for odp_pktio_link_status() for this device type that always >> returns 1 to indicate the link is up. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> platform/linux-generic/pktio/loop.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/platform/linux-generic/pktio/loop.c >> b/platform/linux-generic/pktio/loop.c >> index dbb0e7b..0ea6d0e 100644 >> --- a/platform/linux-generic/pktio/loop.c >> +++ b/platform/linux-generic/pktio/loop.c >> @@ -134,6 +134,12 @@ static int loopback_mac_addr_get(pktio_entry_t >> *pktio_entry ODP_UNUSED, >> return ETH_ALEN; >> } >> +static int loopback_link_status(pktio_entry_t *pktio_entry ODP_UNUSED) >> +{ >> + /* loopback interfaces are always up */ >> + return 1; >> +} >> + >> static int loopback_promisc_mode_set(pktio_entry_t *pktio_entry, >> odp_bool_t enable) >> { >> @@ -176,6 +182,7 @@ const pktio_if_ops_t loopback_pktio_ops = { >> .promisc_mode_set = loopback_promisc_mode_set, >> .promisc_mode_get = loopback_promisc_mode_get, >> .mac_get = loopback_mac_addr_get, >> + .link_status = loopback_link_status, >> .capability = NULL, >> .input_queues_config = NULL, >> .output_queues_config = NULL, >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 03/01/16 17:56, Bill Fischofer wrote: > If it's not defined it's better to return -1. However the question is > shouldn't a loop interface have its own definition for this function? > I know link_status was added after the original restructure so perhaps > this was just an omission? > Really, sorry I already forgot what this function does: /** * Determine pktio link is up or down for a packet IO interface. * * @param pktio Packet IO handle. * * @retval 1 link is up * @retval 0 link is down * @retval <0 on failure */ int odp_pktio_link_status(odp_pktio_t pktio); In linux-generic loop is definitely always up. But on other platforms I have no idea. Maxim. > On Tue, Mar 1, 2016 at 8:53 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > Bill, > > Idea if ops is not defined that some common value has to be used. > Instead of fixing loop it's needed to fix common code to return 1 > if link_status is not implemented. > > int odp_pktio_link_status(odp_pktio_t id) > { > pktio_entry_t *entry; > int ret = -1; <--- has to be 1 > ..... > if (entry->s.ops->link_status) > ret = entry->s.ops->link_status(entry); > unlock_entry(entry); > > return ret; > } > > Maxim. > > > > On 02/29/16 23:53, Bill Fischofer wrote: > > ODP loopback interfaces should always be considered up, so add a > handler for odp_pktio_link_status() for this device type that > always > returns 1 to indicate the link is up. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> > --- > platform/linux-generic/pktio/loop.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/platform/linux-generic/pktio/loop.c > b/platform/linux-generic/pktio/loop.c > index dbb0e7b..0ea6d0e 100644 > --- a/platform/linux-generic/pktio/loop.c > +++ b/platform/linux-generic/pktio/loop.c > @@ -134,6 +134,12 @@ static int > loopback_mac_addr_get(pktio_entry_t *pktio_entry ODP_UNUSED, > return ETH_ALEN; > } > +static int loopback_link_status(pktio_entry_t *pktio_entry > ODP_UNUSED) > +{ > + /* loopback interfaces are always up */ > + return 1; > +} > + > static int loopback_promisc_mode_set(pktio_entry_t *pktio_entry, > odp_bool_t enable) > { > @@ -176,6 +182,7 @@ const pktio_if_ops_t loopback_pktio_ops = { > .promisc_mode_set = loopback_promisc_mode_set, > .promisc_mode_get = loopback_promisc_mode_get, > .mac_get = loopback_mac_addr_get, > + .link_status = loopback_link_status, > .capability = NULL, > .input_queues_config = NULL, > .output_queues_config = NULL, > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
On 29 February 2016 at 15:53, Bill Fischofer <bill.fischofer@linaro.org> wrote: > ODP loopback interfaces should always be considered up, so add a > handler for odp_pktio_link_status() for this device type that always > returns 1 to indicate the link is up. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > Reviewed-and-tested-by: Mike Holmes <mike.holmes@linaro.org> > --- > platform/linux-generic/pktio/loop.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/platform/linux-generic/pktio/loop.c > b/platform/linux-generic/pktio/loop.c > index dbb0e7b..0ea6d0e 100644 > --- a/platform/linux-generic/pktio/loop.c > +++ b/platform/linux-generic/pktio/loop.c > @@ -134,6 +134,12 @@ static int loopback_mac_addr_get(pktio_entry_t > *pktio_entry ODP_UNUSED, > return ETH_ALEN; > } > > +static int loopback_link_status(pktio_entry_t *pktio_entry ODP_UNUSED) > +{ > + /* loopback interfaces are always up */ > + return 1; > +} > + > static int loopback_promisc_mode_set(pktio_entry_t *pktio_entry, > odp_bool_t enable) > { > @@ -176,6 +182,7 @@ const pktio_if_ops_t loopback_pktio_ops = { > .promisc_mode_set = loopback_promisc_mode_set, > .promisc_mode_get = loopback_promisc_mode_get, > .mac_get = loopback_mac_addr_get, > + .link_status = loopback_link_status, > .capability = NULL, > .input_queues_config = NULL, > .output_queues_config = NULL, > -- > 2.5.0 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp > -- Mike Holmes Technical Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collborative, the rest follows"
I am seeing a problem now, I ran twice, not sure what is different, I did a git clean -xdf before this run PATCH_DIR=~/incoming/bill ./apply-and-build.sh make check-TESTS make[4]: Entering directory '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' make[5]: Entering directory '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' PASS: pktio/pktio_run SKIP: pktio/pktio_run_tap PASS: ../../../test/validation/atomic/atomic_main PASS: ../../../test/validation/barrier/barrier_main PASS: ../../../test/validation/buffer/buffer_main PASS: ../../../test/validation/classification/classification_main PASS: ../../../test/validation/config/config_main PASS: ../../../test/validation/cpumask/cpumask_main PASS: ../../../test/validation/crypto/crypto_main PASS: ../../../test/validation/errno/errno_main PASS: ../../../test/validation/hash/hash_main PASS: ../../../test/validation/init/init_main_ok PASS: ../../../test/validation/init/init_main_abort PASS: ../../../test/validation/init/init_main_log PASS: ../../../test/validation/lock/lock_main PASS: ../../../test/validation/packet/packet_main PASS: ../../../test/validation/pool/pool_main PASS: ../../../test/validation/queue/queue_main PASS: ../../../test/validation/random/random_main PASS: ../../../test/validation/scheduler/scheduler_main PASS: ../../../test/validation/std_clib/std_clib_main PASS: ../../../test/validation/thread/thread_main PASS: ../../../test/validation/time/time_main PASS: ../../../test/validation/timer/timer_main make[5]: *** No rule to make target '../../../test/validation/traffic_mngr/traffic_mngr_main', needed by '../../../test/validation/traffic_mngr/traffic_mngr_main.log'. Stop. make[5]: Leaving directory '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' Makefile:911: recipe for target 'check-TESTS' failed make[4]: *** [check-TESTS] Error 2 make[4]: Leaving directory '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' Makefile:1198: recipe for target 'check-am' failed make[3]: *** [check-am] Error 2 make[3]: Leaving directory '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' Makefile:698: recipe for target 'check-recursive' failed make[2]: *** [check-recursive] Error 1 make[2]: Leaving directory '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' Makefile:456: recipe for target 'check-recursive' failed make[1]: *** [check-recursive] Error 1 make[1]: Leaving directory '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub' Makefile:662: recipe for target 'distcheck' failed make: *** [distcheck] Error 1 On 1 March 2016 at 12:56, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 29 February 2016 at 15:53, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> ODP loopback interfaces should always be considered up, so add a >> handler for odp_pktio_link_status() for this device type that always >> returns 1 to indicate the link is up. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> > > Reviewed-and-tested-by: Mike Holmes <mike.holmes@linaro.org> > > >> --- >> platform/linux-generic/pktio/loop.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/platform/linux-generic/pktio/loop.c >> b/platform/linux-generic/pktio/loop.c >> index dbb0e7b..0ea6d0e 100644 >> --- a/platform/linux-generic/pktio/loop.c >> +++ b/platform/linux-generic/pktio/loop.c >> @@ -134,6 +134,12 @@ static int loopback_mac_addr_get(pktio_entry_t >> *pktio_entry ODP_UNUSED, >> return ETH_ALEN; >> } >> >> +static int loopback_link_status(pktio_entry_t *pktio_entry ODP_UNUSED) >> +{ >> + /* loopback interfaces are always up */ >> + return 1; >> +} >> + >> static int loopback_promisc_mode_set(pktio_entry_t *pktio_entry, >> odp_bool_t enable) >> { >> @@ -176,6 +182,7 @@ const pktio_if_ops_t loopback_pktio_ops = { >> .promisc_mode_set = loopback_promisc_mode_set, >> .promisc_mode_get = loopback_promisc_mode_get, >> .mac_get = loopback_mac_addr_get, >> + .link_status = loopback_link_status, >> .capability = NULL, >> .input_queues_config = NULL, >> .output_queues_config = NULL, >> -- >> 2.5.0 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collborative, the rest follows" > > > -- Mike Holmes Technical Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collborative, the rest follows"
Parts 2 and 3 need to be reversed since the required build will fail without the fixes. v2 submitted to correct this. (I also added back Mike's signed-off-by to Part 3 since he found that omission first). apply-and-build runs clean for me with that change. On Tue, Mar 1, 2016 at 1:08 PM, Mike Holmes <mike.holmes@linaro.org> wrote: > I am seeing a problem now, I ran twice, not sure what is different, I did > a git clean -xdf before this run > > PATCH_DIR=~/incoming/bill ./apply-and-build.sh > > make check-TESTS > make[4]: Entering directory > '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' > make[5]: Entering directory > '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' > PASS: pktio/pktio_run > SKIP: pktio/pktio_run_tap > PASS: ../../../test/validation/atomic/atomic_main > PASS: ../../../test/validation/barrier/barrier_main > PASS: ../../../test/validation/buffer/buffer_main > PASS: ../../../test/validation/classification/classification_main > PASS: ../../../test/validation/config/config_main > PASS: ../../../test/validation/cpumask/cpumask_main > PASS: ../../../test/validation/crypto/crypto_main > PASS: ../../../test/validation/errno/errno_main > PASS: ../../../test/validation/hash/hash_main > PASS: ../../../test/validation/init/init_main_ok > PASS: ../../../test/validation/init/init_main_abort > PASS: ../../../test/validation/init/init_main_log > PASS: ../../../test/validation/lock/lock_main > PASS: ../../../test/validation/packet/packet_main > PASS: ../../../test/validation/pool/pool_main > PASS: ../../../test/validation/queue/queue_main > PASS: ../../../test/validation/random/random_main > PASS: ../../../test/validation/scheduler/scheduler_main > PASS: ../../../test/validation/std_clib/std_clib_main > PASS: ../../../test/validation/thread/thread_main > PASS: ../../../test/validation/time/time_main > PASS: ../../../test/validation/timer/timer_main > make[5]: *** No rule to make target > '../../../test/validation/traffic_mngr/traffic_mngr_main', needed by > '../../../test/validation/traffic_mngr/traffic_mngr_main.log'. Stop. > make[5]: Leaving directory > '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' > Makefile:911: recipe for target 'check-TESTS' failed > make[4]: *** [check-TESTS] Error 2 > make[4]: Leaving directory > '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' > Makefile:1198: recipe for target 'check-am' failed > make[3]: *** [check-am] Error 2 > make[3]: Leaving directory > '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' > Makefile:698: recipe for target 'check-recursive' failed > make[2]: *** [check-recursive] Error 1 > make[2]: Leaving directory > '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub/platform/linux-generic/test' > Makefile:456: recipe for target 'check-recursive' failed > make[1]: *** [check-recursive] Error 1 > make[1]: Leaving directory > '/home/mike/git/check-odp/build/odp-apply/opendataplane-1.7.0.0.git22.gbf3c73b/_build/sub' > Makefile:662: recipe for target 'distcheck' failed > make: *** [distcheck] Error 1 > > > On 1 March 2016 at 12:56, Mike Holmes <mike.holmes@linaro.org> wrote: > >> >> >> On 29 February 2016 at 15:53, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >>> ODP loopback interfaces should always be considered up, so add a >>> handler for odp_pktio_link_status() for this device type that always >>> returns 1 to indicate the link is up. >>> >>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>> >> >> Reviewed-and-tested-by: Mike Holmes <mike.holmes@linaro.org> >> >> >>> --- >>> platform/linux-generic/pktio/loop.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/platform/linux-generic/pktio/loop.c >>> b/platform/linux-generic/pktio/loop.c >>> index dbb0e7b..0ea6d0e 100644 >>> --- a/platform/linux-generic/pktio/loop.c >>> +++ b/platform/linux-generic/pktio/loop.c >>> @@ -134,6 +134,12 @@ static int loopback_mac_addr_get(pktio_entry_t >>> *pktio_entry ODP_UNUSED, >>> return ETH_ALEN; >>> } >>> >>> +static int loopback_link_status(pktio_entry_t *pktio_entry ODP_UNUSED) >>> +{ >>> + /* loopback interfaces are always up */ >>> + return 1; >>> +} >>> + >>> static int loopback_promisc_mode_set(pktio_entry_t *pktio_entry, >>> odp_bool_t enable) >>> { >>> @@ -176,6 +182,7 @@ const pktio_if_ops_t loopback_pktio_ops = { >>> .promisc_mode_set = loopback_promisc_mode_set, >>> .promisc_mode_get = loopback_promisc_mode_get, >>> .mac_get = loopback_mac_addr_get, >>> + .link_status = loopback_link_status, >>> .capability = NULL, >>> .input_queues_config = NULL, >>> .output_queues_config = NULL, >>> -- >>> 2.5.0 >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs >> "Work should be fun and collborative, the rest follows" >> >> >> > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collborative, the rest follows" > > >
diff --git a/platform/linux-generic/pktio/loop.c b/platform/linux-generic/pktio/loop.c index dbb0e7b..0ea6d0e 100644 --- a/platform/linux-generic/pktio/loop.c +++ b/platform/linux-generic/pktio/loop.c @@ -134,6 +134,12 @@ static int loopback_mac_addr_get(pktio_entry_t *pktio_entry ODP_UNUSED, return ETH_ALEN; } +static int loopback_link_status(pktio_entry_t *pktio_entry ODP_UNUSED) +{ + /* loopback interfaces are always up */ + return 1; +} + static int loopback_promisc_mode_set(pktio_entry_t *pktio_entry, odp_bool_t enable) { @@ -176,6 +182,7 @@ const pktio_if_ops_t loopback_pktio_ops = { .promisc_mode_set = loopback_promisc_mode_set, .promisc_mode_get = loopback_promisc_mode_get, .mac_get = loopback_mac_addr_get, + .link_status = loopback_link_status, .capability = NULL, .input_queues_config = NULL, .output_queues_config = NULL,
ODP loopback interfaces should always be considered up, so add a handler for odp_pktio_link_status() for this device type that always returns 1 to indicate the link is up. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/pktio/loop.c | 7 +++++++ 1 file changed, 7 insertions(+)