| Message ID | 20230710163503.2821068-2-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8046063df887bee35c002224267ba46f41be7cf6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igc: Fix corner cases for TSN offload | expand |
On Mon, Jul 10, 2023 at 09:34:58AM -0700, Tony Nguyen wrote: > From: Florian Kauer <florian.kauer@linutronix.de> > > In the current implementation the flags adapter->qbv_enable > and IGC_FLAG_TSN_QBV_ENABLED have a similar name, but do not > have the same meaning. The first one is used only to indicate > taprio offload (i.e. when igc_save_qbv_schedule was called), > while the second one corresponds to the Qbv mode of the hardware. > However, the second one is also used to support the TX launchtime > feature, i.e. ETF qdisc offload. This leads to situations where > adapter->qbv_enable is false, but the flag IGC_FLAG_TSN_QBV_ENABLED > is set. This is prone to confusion. > > The rename should reduce this confusion. Since it is a pure > rename, it has no impact on functionality. And shouldn't be sent to net, but to net-next. Thanks > > Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv") > Signed-off-by: Florian Kauer <florian.kauer@linutronix.de> > Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> > Tested-by: Naama Meir <naamax.meir@linux.intel.com> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > --- > drivers/net/ethernet/intel/igc/igc.h | 2 +- > drivers/net/ethernet/intel/igc/igc_main.c | 6 +++--- > drivers/net/ethernet/intel/igc/igc_tsn.c | 2 +- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index 639a50c02537..9db384f66a8e 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -191,7 +191,7 @@ struct igc_adapter { > int tc_setup_type; > ktime_t base_time; > ktime_t cycle_time; > - bool qbv_enable; > + bool taprio_offload_enable; > u32 qbv_config_change_errors; > bool qbv_transition; > unsigned int qbv_count; > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 281a0e35b9d1..fae534ef1c4f 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -6126,16 +6126,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, > > switch (qopt->cmd) { > case TAPRIO_CMD_REPLACE: > - adapter->qbv_enable = true; > + adapter->taprio_offload_enable = true; > break; > case TAPRIO_CMD_DESTROY: > - adapter->qbv_enable = false; > + adapter->taprio_offload_enable = false; > break; > default: > return -EOPNOTSUPP; > } > > - if (!adapter->qbv_enable) > + if (!adapter->taprio_offload_enable) > return igc_tsn_clear_schedule(adapter); > > if (qopt->base_time < 0) > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c > index 3cdb0c988728..b76ebfc10b1d 100644 > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > @@ -37,7 +37,7 @@ static unsigned int igc_tsn_new_flags(struct igc_adapter *adapter) > { > unsigned int new_flags = adapter->flags & ~IGC_FLAG_TSN_ANY_ENABLED; > > - if (adapter->qbv_enable) > + if (adapter->taprio_offload_enable) > new_flags |= IGC_FLAG_TSN_QBV_ENABLED; > > if (is_any_launchtime(adapter)) > -- > 2.38.1 > >
Hi Leon, On 11.07.23 09:01, Leon Romanovsky wrote: > On Mon, Jul 10, 2023 at 09:34:58AM -0700, Tony Nguyen wrote: >> From: Florian Kauer <florian.kauer@linutronix.de> >> >> In the current implementation the flags adapter->qbv_enable >> and IGC_FLAG_TSN_QBV_ENABLED have a similar name, but do not >> have the same meaning. The first one is used only to indicate >> taprio offload (i.e. when igc_save_qbv_schedule was called), >> while the second one corresponds to the Qbv mode of the hardware. >> However, the second one is also used to support the TX launchtime >> feature, i.e. ETF qdisc offload. This leads to situations where >> adapter->qbv_enable is false, but the flag IGC_FLAG_TSN_QBV_ENABLED >> is set. This is prone to confusion. >> >> The rename should reduce this confusion. Since it is a pure >> rename, it has no impact on functionality. > > And shouldn't be sent to net, but to net-next.> > Thanks In principle I fully agree that sole renames are not intended for net. But in this case the rename is tightly coupled with the other patches of the series, not only due to overlapping code changes, but in particular because the naming might very likely be one root cause of the regressions. I probably should have just squashed it with the second patch, but my initial idea was to keep them separate for clarity. Also see: https://lore.kernel.org/netdev/SJ1PR11MB6180B5C87699252B91FB7EE1B858A@SJ1PR11MB6180.namprd11.prod.outlook.com/ https://lore.kernel.org/netdev/0c02e976-0da6-8ed8-4546-4df7af4ebed5@linutronix.de/ Thanks, Florian >> >> Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv") >> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de> >> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> >> Tested-by: Naama Meir <naamax.meir@linux.intel.com> >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> >> --- >> drivers/net/ethernet/intel/igc/igc.h | 2 +- >> drivers/net/ethernet/intel/igc/igc_main.c | 6 +++--- >> drivers/net/ethernet/intel/igc/igc_tsn.c | 2 +- >> 3 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h >> index 639a50c02537..9db384f66a8e 100644 >> --- a/drivers/net/ethernet/intel/igc/igc.h >> +++ b/drivers/net/ethernet/intel/igc/igc.h >> @@ -191,7 +191,7 @@ struct igc_adapter { >> int tc_setup_type; >> ktime_t base_time; >> ktime_t cycle_time; >> - bool qbv_enable; >> + bool taprio_offload_enable; >> u32 qbv_config_change_errors; >> bool qbv_transition; >> unsigned int qbv_count; >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index 281a0e35b9d1..fae534ef1c4f 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -6126,16 +6126,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, >> >> switch (qopt->cmd) { >> case TAPRIO_CMD_REPLACE: >> - adapter->qbv_enable = true; >> + adapter->taprio_offload_enable = true; >> break; >> case TAPRIO_CMD_DESTROY: >> - adapter->qbv_enable = false; >> + adapter->taprio_offload_enable = false; >> break; >> default: >> return -EOPNOTSUPP; >> } >> >> - if (!adapter->qbv_enable) >> + if (!adapter->taprio_offload_enable) >> return igc_tsn_clear_schedule(adapter); >> >> if (qopt->base_time < 0) >> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c >> index 3cdb0c988728..b76ebfc10b1d 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c >> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c >> @@ -37,7 +37,7 @@ static unsigned int igc_tsn_new_flags(struct igc_adapter *adapter) >> { >> unsigned int new_flags = adapter->flags & ~IGC_FLAG_TSN_ANY_ENABLED; >> >> - if (adapter->qbv_enable) >> + if (adapter->taprio_offload_enable) >> new_flags |= IGC_FLAG_TSN_QBV_ENABLED; >> >> if (is_any_launchtime(adapter)) >> -- >> 2.38.1 >> >>
On Tue, Jul 11, 2023 at 09:18:31AM +0200, Florian Kauer wrote: > Hi Leon, > > On 11.07.23 09:01, Leon Romanovsky wrote: > > On Mon, Jul 10, 2023 at 09:34:58AM -0700, Tony Nguyen wrote: > >> From: Florian Kauer <florian.kauer@linutronix.de> > >> > >> In the current implementation the flags adapter->qbv_enable > >> and IGC_FLAG_TSN_QBV_ENABLED have a similar name, but do not > >> have the same meaning. The first one is used only to indicate > >> taprio offload (i.e. when igc_save_qbv_schedule was called), > >> while the second one corresponds to the Qbv mode of the hardware. > >> However, the second one is also used to support the TX launchtime > >> feature, i.e. ETF qdisc offload. This leads to situations where > >> adapter->qbv_enable is false, but the flag IGC_FLAG_TSN_QBV_ENABLED > >> is set. This is prone to confusion. > >> > >> The rename should reduce this confusion. Since it is a pure > >> rename, it has no impact on functionality. > > > > And shouldn't be sent to net, but to net-next.> > > Thanks > > In principle I fully agree that sole renames are not intended for net. > But in this case the rename is tightly coupled with the other patches > of the series, not only due to overlapping code changes, but in particular > because the naming might very likely be one root cause of the regressions. I understand the intention, but your second patch showed that rename was premature. Thanks
On 11.07.23 09:32, Leon Romanovsky wrote: > On Tue, Jul 11, 2023 at 09:18:31AM +0200, Florian Kauer wrote: >> Hi Leon, >> >> On 11.07.23 09:01, Leon Romanovsky wrote: >>> On Mon, Jul 10, 2023 at 09:34:58AM -0700, Tony Nguyen wrote: >>>> From: Florian Kauer <florian.kauer@linutronix.de> >>>> >>>> In the current implementation the flags adapter->qbv_enable >>>> and IGC_FLAG_TSN_QBV_ENABLED have a similar name, but do not >>>> have the same meaning. The first one is used only to indicate >>>> taprio offload (i.e. when igc_save_qbv_schedule was called), >>>> while the second one corresponds to the Qbv mode of the hardware. >>>> However, the second one is also used to support the TX launchtime >>>> feature, i.e. ETF qdisc offload. This leads to situations where >>>> adapter->qbv_enable is false, but the flag IGC_FLAG_TSN_QBV_ENABLED >>>> is set. This is prone to confusion. >>>> >>>> The rename should reduce this confusion. Since it is a pure >>>> rename, it has no impact on functionality. >>> >>> And shouldn't be sent to net, but to net-next.> >>> Thanks >> >> In principle I fully agree that sole renames are not intended for net. >> But in this case the rename is tightly coupled with the other patches >> of the series, not only due to overlapping code changes, but in particular >> because the naming might very likely be one root cause of the regressions. > > I understand the intention, but your second patch showed that rename was > premature. > > Thanks The second patch does not touch the rename in igc.h and igc_tsn.c... (and the latter is from the context probably the most relevant one) But I see what you mean. I am fine with both squashing and keeping it separate, but I have no idea how the preferred process is since this is already so far through the pipeline... Thanks, Florian
On Tue, 11 Jul 2023 09:51:34 +0200 Florian Kauer wrote: > > I understand the intention, but your second patch showed that rename was > > premature. I think it's fine. It's a rename, it can't regress anything. And the separate commit message clearly describing reasoning is good to have. > The second patch does not touch the rename in igc.h and igc_tsn.c... > (and the latter is from the context probably the most relevant one) > But I see what you mean. I am fine with both squashing and keeping it separate, > but I have no idea how the preferred process is since this > is already so far through the pipeline... "This is so far through the pipeline" is an argument which may elicit a very negative reaction upstream :)
Hi Jakub, On 12.07.23 02:58, Jakub Kicinski wrote: > On Tue, 11 Jul 2023 09:51:34 +0200 Florian Kauer wrote: >>> I understand the intention, but your second patch showed that rename was >>> premature. > > I think it's fine. It's a rename, it can't regress anything. > And the separate commit message clearly describing reasoning > is good to have. > >> The second patch does not touch the rename in igc.h and igc_tsn.c... >> (and the latter is from the context probably the most relevant one) >> But I see what you mean. I am fine with both squashing and keeping it separate, >> but I have no idea how the preferred process is since this >> is already so far through the pipeline... > > "This is so far through the pipeline" is an argument which may elicit > a very negative reaction upstream :) Sorry, I didn't mean to use that as an argument to push it through. It is just that since this is my first patch series (except a small contribution some years ago), I just honestly do not know what the usual practice in such a situation would be. E.g. if I would just send a new version and if yes, which tags I would keep since it would just be a squash. Especially, if it needs to be retested. Or if Tony would directly squash it on his tree, or... I personally would have no problem with doing extra work if it improves the series, I just do not want to provoke unnecessary work or confusion for others by doing it in an unusual way. Greetings, Florian
On 7/11/2023 11:53 PM, Florian Kauer wrote: > Hi Jakub, > > On 12.07.23 02:58, Jakub Kicinski wrote: >> On Tue, 11 Jul 2023 09:51:34 +0200 Florian Kauer wrote: >>>> I understand the intention, but your second patch showed that rename was >>>> premature. >> >> I think it's fine. It's a rename, it can't regress anything. >> And the separate commit message clearly describing reasoning >> is good to have. >> >>> The second patch does not touch the rename in igc.h and igc_tsn.c... >>> (and the latter is from the context probably the most relevant one) >>> But I see what you mean. I am fine with both squashing and keeping it separate, >>> but I have no idea how the preferred process is since this >>> is already so far through the pipeline... >> >> "This is so far through the pipeline" is an argument which may elicit >> a very negative reaction upstream :) > > Sorry, I didn't mean to use that as an argument to push it through. > It is just that since this is my first patch series (except a small > contribution some years ago), I just honestly do not know what the > usual practice in such a situation would be. > > E.g. if I would just send a new version and if yes, which tags I would > keep since it would just be a squash. Especially, if it needs to be > retested. Or if Tony would directly squash it on his tree, or... Hi Florian, I would expect it as a new version of your submission [1]. If anything needs to be done/changed on the back end for an updated PR to netdev, I'd take care of that. It does look like this was pulled though so any changes will need to be follow-on patches. Thanks, Tony > I personally would have no problem with doing extra work if it improves > the series, I just do not want to provoke unnecessary work or confusion > for others by doing it in an unusual way. > > Greetings, > Florian [1] https://lore.kernel.org/netdev/20230619100858.116286-1-florian.kauer@linutronix.de/
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index 639a50c02537..9db384f66a8e 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -191,7 +191,7 @@ struct igc_adapter { int tc_setup_type; ktime_t base_time; ktime_t cycle_time; - bool qbv_enable; + bool taprio_offload_enable; u32 qbv_config_change_errors; bool qbv_transition; unsigned int qbv_count; diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 281a0e35b9d1..fae534ef1c4f 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6126,16 +6126,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, switch (qopt->cmd) { case TAPRIO_CMD_REPLACE: - adapter->qbv_enable = true; + adapter->taprio_offload_enable = true; break; case TAPRIO_CMD_DESTROY: - adapter->qbv_enable = false; + adapter->taprio_offload_enable = false; break; default: return -EOPNOTSUPP; } - if (!adapter->qbv_enable) + if (!adapter->taprio_offload_enable) return igc_tsn_clear_schedule(adapter); if (qopt->base_time < 0) diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index 3cdb0c988728..b76ebfc10b1d 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.c +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c @@ -37,7 +37,7 @@ static unsigned int igc_tsn_new_flags(struct igc_adapter *adapter) { unsigned int new_flags = adapter->flags & ~IGC_FLAG_TSN_ANY_ENABLED; - if (adapter->qbv_enable) + if (adapter->taprio_offload_enable) new_flags |= IGC_FLAG_TSN_QBV_ENABLED; if (is_any_launchtime(adapter))