Following the implementation of "igc: Add TransmissionOverrun counter" patch, when a taprio command is triggered by user, igc processes two commands: TAPRIO_CMD_REPLACE followed by TAPRIO_CMD_STATS. However, both commands unconditionally pass through igc_tsn_offload_apply() which evaluates and triggers reset adapter. The double reset causes issues in the calculation of adapter->qbv_count in igc.
TAPRIO_CMD_REPLACE command is expected to reset the adapter since it activates qbv. It's unexpected for TAPRIO_CMD_STATS to do the same because it doesn't configure any driver-specific TSN settings. So, the evaluation in igc_tsn_offload_apply() isn't needed for TAPRIO_CMD_STATS.
To address this, commands parsing are relocated to igc_tsn_enable_qbv_scheduling(). Commands that don't require an adapter reset will exit after processing, thus avoiding igc_tsn_offload_apply().
Fixes: d3750076d464 ("igc: Add TransmissionOverrun counter") Signed-off-by: Faizal Rahim faizal.abdul.rahim@linux.intel.com --- drivers/net/ethernet/intel/igc/igc_main.c | 33 ++++++++++++----------- 1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 87b655b839c1..33069880c86c 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6310,21 +6310,6 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, size_t n; int i;
- switch (qopt->cmd) { - case TAPRIO_CMD_REPLACE: - break; - case TAPRIO_CMD_DESTROY: - return igc_tsn_clear_schedule(adapter); - case TAPRIO_CMD_STATS: - igc_taprio_stats(adapter->netdev, &qopt->stats); - return 0; - case TAPRIO_CMD_QUEUE_STATS: - igc_taprio_queue_stats(adapter->netdev, &qopt->queue_stats); - return 0; - default: - return -EOPNOTSUPP; - } - if (qopt->base_time < 0) return -ERANGE;
@@ -6433,7 +6418,23 @@ static int igc_tsn_enable_qbv_scheduling(struct igc_adapter *adapter, if (hw->mac.type != igc_i225) return -EOPNOTSUPP;
- err = igc_save_qbv_schedule(adapter, qopt); + switch (qopt->cmd) { + case TAPRIO_CMD_REPLACE: + err = igc_save_qbv_schedule(adapter, qopt); + break; + case TAPRIO_CMD_DESTROY: + err = igc_tsn_clear_schedule(adapter); + break; + case TAPRIO_CMD_STATS: + igc_taprio_stats(adapter->netdev, &qopt->stats); + return 0; + case TAPRIO_CMD_QUEUE_STATS: + igc_taprio_queue_stats(adapter->netdev, &qopt->queue_stats); + return 0; + default: + return -EOPNOTSUPP; + } + if (err) return err;
-- 2.25.1
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH net 1/1] igc: Fix double reset adapter triggered from a single taprio cmd Link: https://lore.kernel.org/stable/20240625082656.2702440-1-faizal.abdul.rahim%4...
Added Vladimir and Husaini in CC.
On 25/6/2024 4:26 pm, Faizal Rahim wrote:
Following the implementation of "igc: Add TransmissionOverrun counter" patch, when a taprio command is triggered by user, igc processes two commands: TAPRIO_CMD_REPLACE followed by TAPRIO_CMD_STATS. However, both commands unconditionally pass through igc_tsn_offload_apply() which evaluates and triggers reset adapter. The double reset causes issues in the calculation of adapter->qbv_count in igc.
TAPRIO_CMD_REPLACE command is expected to reset the adapter since it activates qbv. It's unexpected for TAPRIO_CMD_STATS to do the same because it doesn't configure any driver-specific TSN settings. So, the evaluation in igc_tsn_offload_apply() isn't needed for TAPRIO_CMD_STATS.
To address this, commands parsing are relocated to igc_tsn_enable_qbv_scheduling(). Commands that don't require an adapter reset will exit after processing, thus avoiding igc_tsn_offload_apply().
Fixes: d3750076d464 ("igc: Add TransmissionOverrun counter") Signed-off-by: Faizal Rahim faizal.abdul.rahim@linux.intel.com
drivers/net/ethernet/intel/igc/igc_main.c | 33 ++++++++++++----------- 1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 87b655b839c1..33069880c86c 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6310,21 +6310,6 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, size_t n; int i;
- switch (qopt->cmd) {
- case TAPRIO_CMD_REPLACE:
break;- case TAPRIO_CMD_DESTROY:
return igc_tsn_clear_schedule(adapter);- case TAPRIO_CMD_STATS:
igc_taprio_stats(adapter->netdev, &qopt->stats);return 0;- case TAPRIO_CMD_QUEUE_STATS:
igc_taprio_queue_stats(adapter->netdev, &qopt->queue_stats);return 0;- default:
return -EOPNOTSUPP;- }
- if (qopt->base_time < 0) return -ERANGE;
@@ -6433,7 +6418,23 @@ static int igc_tsn_enable_qbv_scheduling(struct igc_adapter *adapter, if (hw->mac.type != igc_i225) return -EOPNOTSUPP;
- err = igc_save_qbv_schedule(adapter, qopt);
- switch (qopt->cmd) {
- case TAPRIO_CMD_REPLACE:
err = igc_save_qbv_schedule(adapter, qopt);break;- case TAPRIO_CMD_DESTROY:
err = igc_tsn_clear_schedule(adapter);break;- case TAPRIO_CMD_STATS:
igc_taprio_stats(adapter->netdev, &qopt->stats);return 0;- case TAPRIO_CMD_QUEUE_STATS:
igc_taprio_queue_stats(adapter->netdev, &qopt->queue_stats);return 0;- default:
return -EOPNOTSUPP;- }
- if (err) return err;
-- 2.25.1
On Wed, Jun 26, 2024 at 09:19:12AM +0800, Abdul Rahim, Faizal wrote:
Added Vladimir and Husaini in CC.
On 25/6/2024 4:26 pm, Faizal Rahim wrote:
Following the implementation of "igc: Add TransmissionOverrun counter" patch, when a taprio command is triggered by user, igc processes two commands: TAPRIO_CMD_REPLACE followed by TAPRIO_CMD_STATS. However, both commands unconditionally pass through igc_tsn_offload_apply() which evaluates and triggers reset adapter. The double reset causes issues in the calculation of adapter->qbv_count in igc.
TAPRIO_CMD_REPLACE command is expected to reset the adapter since it activates qbv. It's unexpected for TAPRIO_CMD_STATS to do the same because it doesn't configure any driver-specific TSN settings. So, the evaluation in igc_tsn_offload_apply() isn't needed for TAPRIO_CMD_STATS.
To address this, commands parsing are relocated to igc_tsn_enable_qbv_scheduling(). Commands that don't require an adapter reset will exit after processing, thus avoiding igc_tsn_offload_apply().
Fixes: d3750076d464 ("igc: Add TransmissionOverrun counter") Signed-off-by: Faizal Rahim faizal.abdul.rahim@linux.intel.com
Thank you for the patch. The code organization is much more logical this way.
Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com
Faizal Rahim faizal.abdul.rahim@linux.intel.com writes:
Following the implementation of "igc: Add TransmissionOverrun counter" patch, when a taprio command is triggered by user, igc processes two commands: TAPRIO_CMD_REPLACE followed by TAPRIO_CMD_STATS. However, both commands unconditionally pass through igc_tsn_offload_apply() which evaluates and triggers reset adapter. The double reset causes issues in the calculation of adapter->qbv_count in igc.
TAPRIO_CMD_REPLACE command is expected to reset the adapter since it activates qbv. It's unexpected for TAPRIO_CMD_STATS to do the same because it doesn't configure any driver-specific TSN settings. So, the evaluation in igc_tsn_offload_apply() isn't needed for TAPRIO_CMD_STATS.
To address this, commands parsing are relocated to igc_tsn_enable_qbv_scheduling(). Commands that don't require an adapter reset will exit after processing, thus avoiding igc_tsn_offload_apply().
Fixes: d3750076d464 ("igc: Add TransmissionOverrun counter") Signed-off-by: Faizal Rahim faizal.abdul.rahim@linux.intel.com
drivers/net/ethernet/intel/igc/igc_main.c | 33 ++++++++++++----------- 1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 87b655b839c1..33069880c86c 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6310,21 +6310,6 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, size_t n; int i;
- switch (qopt->cmd) {
- case TAPRIO_CMD_REPLACE:
break;- case TAPRIO_CMD_DESTROY:
return igc_tsn_clear_schedule(adapter);- case TAPRIO_CMD_STATS:
igc_taprio_stats(adapter->netdev, &qopt->stats);return 0;- case TAPRIO_CMD_QUEUE_STATS:
igc_taprio_queue_stats(adapter->netdev, &qopt->queue_stats);return 0;- default:
return -EOPNOTSUPP;- }
- if (qopt->base_time < 0) return -ERANGE;
@@ -6433,7 +6418,23 @@ static int igc_tsn_enable_qbv_scheduling(struct igc_adapter *adapter, if (hw->mac.type != igc_i225) return -EOPNOTSUPP;
- err = igc_save_qbv_schedule(adapter, qopt);
- switch (qopt->cmd) {
- case TAPRIO_CMD_REPLACE:
err = igc_save_qbv_schedule(adapter, qopt);break;- case TAPRIO_CMD_DESTROY:
err = igc_tsn_clear_schedule(adapter);break;- case TAPRIO_CMD_STATS:
igc_taprio_stats(adapter->netdev, &qopt->stats);return 0;- case TAPRIO_CMD_QUEUE_STATS:
igc_taprio_queue_stats(adapter->netdev, &qopt->queue_stats);return 0;- default:
return -EOPNOTSUPP;- }
Yeah, moving the command parsing to be done earlier sounds like the right fix:
Acked-by: Vinicius Costa Gomes vinicius.gomes@intel.com
if (err) return err;
-- 2.25.1
linux-stable-mirror@lists.linaro.org
-
Abdul Rahim, Faizal -
Faizal Rahim
-
kernel test robot -
Vinicius Costa Gomes -
Vladimir Oltean