| Message ID | 20230619100858.116286-2-florian.kauer@linutronix.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igc: Fix corner cases for TSN offload | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/build_clang | success | Errors and warnings before: 10 this patch: 10 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 6/19/2023 13:08, Florian Kauer wrote: > 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. > > 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> > --- > drivers/net/ethernet/intel/igc/igc.h | 2 +- > drivers/net/ethernet/intel/igc/igc_main.c | 2 +- > drivers/net/ethernet/intel/igc/igc_tsn.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> The rename should reduce this confusion. Would the wording “Reduce this confusion by renaming a variable at three places” be more appropriate for a subsequent change description? See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94 > Since it is a pure > rename, it has no impact on functionality. > > Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv") How does such information fit together? Regards, Markus
Hi Markus, On 02.07.23 18:55, Markus Elfring wrote: >> The rename should reduce this confusion. > > Would the wording “Reduce this confusion by renaming a variable at three places” > be more appropriate for a subsequent change description? > > See also: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94 Thanks for pointing that out (also in your other mail for this series). I will be more careful regarding the use of imperative mood. > >> Since it is a pure >> rename, it has no impact on functionality. >> >> Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv") > > How does such information fit together? The referenced commit introduced an issue into the kernel by introducing a variable that does not exactly describe its actual purpose. It is not only a cosmetic change, but in my view this confusion was related to other issues (see the other patches). So, it seemed to be worth fixing alongside with the other fixes, even if it does not directly impact functionality if it is applied or not (until someone else comes along, also gets confused and introduces another bug...). Thanks, Florian
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index 34aebf00a512..4c7a1e92ce4c 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -185,7 +185,7 @@ struct igc_adapter { ktime_t base_time; ktime_t cycle_time; - bool qbv_enable; + bool taprio_offload_enable; u32 qbv_config_change_errors; /* OS defined structs */ diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index fa764190f270..dda057a3b5e3 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6075,7 +6075,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, size_t n; int i; - adapter->qbv_enable = qopt->enable; + adapter->taprio_offload_enable = qopt->enable; if (!qopt->enable) return igc_tsn_clear_schedule(adapter); diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index 94a2b0dfb54d..c6636a7264d5 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))
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. 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> --- drivers/net/ethernet/intel/igc/igc.h | 2 +- drivers/net/ethernet/intel/igc/igc_main.c | 2 +- drivers/net/ethernet/intel/igc/igc_tsn.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)