Handle offloading commands using switch-case in
am65_cpsw_setup_taprio().
Move checks to am65_cpsw_taprio_replace().
Use NL_SET_ERR_MSG_MOD for error messages.
Change error message from "Failed to set cycle time extension"
to "cycle time extension not supported"
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
drivers/net/ethernet/ti/am65-cpsw-qos.c | 151 +++++++++++-------------
1 file changed, 71 insertions(+), 80 deletions(-)
Changelog:
v8: don't initialize ret = 0, tact = TACT_PROG
v7: don't use "\n" in NL_SET_ERR_MSG_MOD()
v6: initial commit
diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.c b/drivers/net/ethernet/ti/am65-cpsw-qos.c
index 4bc611cc4aad..2c97fa05a852 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-qos.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-qos.c
@@ -428,7 +428,7 @@ static void am65_cpsw_stop_est(struct net_device *ndev)
am65_cpsw_timer_stop(ndev);
}
-static void am65_cpsw_purge_est(struct net_device *ndev)
+static void am65_cpsw_taprio_destroy(struct net_device *ndev)
{
struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
@@ -441,29 +441,66 @@ static void am65_cpsw_purge_est(struct net_device *ndev)
port->qos.est_admin = NULL;
}
-static int am65_cpsw_configure_taprio(struct net_device *ndev,
- struct am65_cpsw_est *est_new)
+static void am65_cpsw_cp_taprio(struct tc_taprio_qopt_offload *from,
+ struct tc_taprio_qopt_offload *to)
+{
+ int i;
+
+ *to = *from;
+ for (i = 0; i < from->num_entries; i++)
+ to->entries[i] = from->entries[i];
+}
+
+static int am65_cpsw_taprio_replace(struct net_device *ndev,
+ struct tc_taprio_qopt_offload *taprio)
{
struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
+ struct netlink_ext_ack *extack = taprio->mqprio.extack;
+ struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
struct am65_cpts *cpts = common->cpts;
- int ret = 0, tact = TACT_PROG;
+ struct am65_cpsw_est *est_new;
+ int ret, tact;
- am65_cpsw_est_update_state(ndev);
+ if (!netif_running(ndev)) {
+ NL_SET_ERR_MSG_MOD(extack, "interface is down, link speed unknown");
+ return -ENETDOWN;
+ }
- if (est_new->taprio.cmd == TAPRIO_CMD_DESTROY) {
- am65_cpsw_stop_est(ndev);
- return ret;
+ if (common->pf_p0_rx_ptype_rrobin) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "p0-rx-ptype-rrobin flag conflicts with taprio qdisc");
+ return -EINVAL;
+ }
+
+ if (port->qos.link_speed == SPEED_UNKNOWN)
+ return -ENOLINK;
+
+ if (taprio->cycle_time_extension) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "cycle time extension not supported");
+ return -EOPNOTSUPP;
}
+ est_new = devm_kzalloc(&ndev->dev,
+ struct_size(est_new, taprio.entries, taprio->num_entries),
+ GFP_KERNEL);
+ if (!est_new)
+ return -ENOMEM;
+
+ am65_cpsw_cp_taprio(taprio, &est_new->taprio);
+
+ am65_cpsw_est_update_state(ndev);
+
ret = am65_cpsw_est_check_scheds(ndev, est_new);
if (ret < 0)
- return ret;
+ goto fail;
tact = am65_cpsw_timer_act(ndev, est_new);
if (tact == TACT_NEED_STOP) {
- dev_err(&ndev->dev,
- "Can't toggle estf timer, stop taprio first");
- return -EINVAL;
+ NL_SET_ERR_MSG_MOD(extack,
+ "Can't toggle estf timer, stop taprio first");
+ ret = -EINVAL;
+ goto fail;
}
if (tact == TACT_PROG)
@@ -476,62 +513,24 @@ static int am65_cpsw_configure_taprio(struct net_device *ndev,
am65_cpsw_est_set_sched_list(ndev, est_new);
am65_cpsw_port_est_assign_buf_num(ndev, est_new->buf);
- am65_cpsw_est_set(ndev, est_new->taprio.cmd == TAPRIO_CMD_REPLACE);
+ am65_cpsw_est_set(ndev, 1);
if (tact == TACT_PROG) {
ret = am65_cpsw_timer_set(ndev, est_new);
if (ret) {
- dev_err(&ndev->dev, "Failed to set cycle time");
- return ret;
+ NL_SET_ERR_MSG_MOD(extack,
+ "Failed to set cycle time");
+ goto fail;
}
}
- return ret;
-}
-
-static void am65_cpsw_cp_taprio(struct tc_taprio_qopt_offload *from,
- struct tc_taprio_qopt_offload *to)
-{
- int i;
-
- *to = *from;
- for (i = 0; i < from->num_entries; i++)
- to->entries[i] = from->entries[i];
-}
-
-static int am65_cpsw_set_taprio(struct net_device *ndev, void *type_data)
-{
- struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
- struct tc_taprio_qopt_offload *taprio = type_data;
- struct am65_cpsw_est *est_new;
- int ret = 0;
-
- if (taprio->cycle_time_extension) {
- dev_err(&ndev->dev, "Failed to set cycle time extension");
- return -EOPNOTSUPP;
- }
-
- est_new = devm_kzalloc(&ndev->dev,
- struct_size(est_new, taprio.entries, taprio->num_entries),
- GFP_KERNEL);
- if (!est_new)
- return -ENOMEM;
-
- am65_cpsw_cp_taprio(taprio, &est_new->taprio);
- ret = am65_cpsw_configure_taprio(ndev, est_new);
- if (!ret) {
- if (taprio->cmd == TAPRIO_CMD_REPLACE) {
- devm_kfree(&ndev->dev, port->qos.est_admin);
+ devm_kfree(&ndev->dev, port->qos.est_admin);
+ port->qos.est_admin = est_new;
- port->qos.est_admin = est_new;
- } else {
- devm_kfree(&ndev->dev, est_new);
- am65_cpsw_purge_est(ndev);
- }
- } else {
- devm_kfree(&ndev->dev, est_new);
- }
+ return 0;
+fail:
+ devm_kfree(&ndev->dev, est_new);
return ret;
}
@@ -558,34 +557,26 @@ static void am65_cpsw_est_link_up(struct net_device *ndev, int link_speed)
return;
purge_est:
- am65_cpsw_purge_est(ndev);
+ am65_cpsw_taprio_destroy(ndev);
}
static int am65_cpsw_setup_taprio(struct net_device *ndev, void *type_data)
{
- struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
struct tc_taprio_qopt_offload *taprio = type_data;
- struct am65_cpsw_common *common = port->common;
-
- if (taprio->cmd != TAPRIO_CMD_REPLACE &&
- taprio->cmd != TAPRIO_CMD_DESTROY)
- return -EOPNOTSUPP;
-
- if (!netif_running(ndev)) {
- dev_err(&ndev->dev, "interface is down, link speed unknown\n");
- return -ENETDOWN;
- }
-
- if (common->pf_p0_rx_ptype_rrobin) {
- dev_err(&ndev->dev,
- "p0-rx-ptype-rrobin flag conflicts with taprio qdisc\n");
- return -EINVAL;
+ int err = 0;
+
+ switch (taprio->cmd) {
+ case TAPRIO_CMD_REPLACE:
+ err = am65_cpsw_taprio_replace(ndev, taprio);
+ break;
+ case TAPRIO_CMD_DESTROY:
+ am65_cpsw_taprio_destroy(ndev);
+ break;
+ default:
+ err = -EOPNOTSUPP;
}
- if (port->qos.link_speed == SPEED_UNKNOWN)
- return -ENOLINK;
-
- return am65_cpsw_set_taprio(ndev, type_data);
+ return err;
}
static int am65_cpsw_tc_query_caps(struct net_device *ndev, void *type_data)
--
2.34.1On Wed, Dec 13, 2023 at 01:07:15PM +0200, Roger Quadros wrote:
> +static int am65_cpsw_taprio_replace(struct net_device *ndev,
> + struct tc_taprio_qopt_offload *taprio)
> {
> struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
> + struct netlink_ext_ack *extack = taprio->mqprio.extack;
> + struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
> struct am65_cpts *cpts = common->cpts;
> - int ret = 0, tact = TACT_PROG;
> + struct am65_cpsw_est *est_new;
> + int ret, tact;
>
> - am65_cpsw_est_update_state(ndev);
> + if (!netif_running(ndev)) {
> + NL_SET_ERR_MSG_MOD(extack, "interface is down, link speed unknown");
> + return -ENETDOWN;
> + }
I haven't used the runtime PM API that this driver uses. I don't know
much about how it works. What are the rules here? By checking for
netif_running(), are you intending to rely on the pm_runtime_resume_and_get()
call from ndo_open(), which is released with pm_runtime_put() at
ndo_stop() time?
I see some inconsistencies I don't quite understand.
am65_cpsw_nuss_ndo_slave_add_vid() checks for netif_running() then calls
pm_runtime_resume_and_get() anyway.
am65_cpsw_setup_mqprio() allows changing the offload even when the link
is down (which is more user-friendly anyway) and performs the pm_runtime_get_sync()
call itself.
> -}
On 14/12/2023 13:23, Vladimir Oltean wrote:
> On Wed, Dec 13, 2023 at 01:07:15PM +0200, Roger Quadros wrote:
>> +static int am65_cpsw_taprio_replace(struct net_device *ndev,
>> + struct tc_taprio_qopt_offload *taprio)
>> {
>> struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
>> + struct netlink_ext_ack *extack = taprio->mqprio.extack;
>> + struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
>> struct am65_cpts *cpts = common->cpts;
>> - int ret = 0, tact = TACT_PROG;
>> + struct am65_cpsw_est *est_new;
>> + int ret, tact;
>>
>> - am65_cpsw_est_update_state(ndev);
>> + if (!netif_running(ndev)) {
>> + NL_SET_ERR_MSG_MOD(extack, "interface is down, link speed unknown");
>> + return -ENETDOWN;
>> + }
>
> I haven't used the runtime PM API that this driver uses. I don't know
> much about how it works. What are the rules here? By checking for
The only rule is that if network interface is down, the device might be
runtime_suspended so we need to get it back to runtime_active before any
device access.
> netif_running(), are you intending to rely on the pm_runtime_resume_and_get()
> call from ndo_open(), which is released with pm_runtime_put() at
> ndo_stop() time?
Actually, this code is already present upstream. I'm only moving it around
in this patch.
Based on the error message and looking at am65_cpsw_est_check_scheds() and
am65_cpsw_est_set_sched_list() which is called later in am65_cpsw_taprio_replace(),
both of which eventually call am65_est_cmd_ns_to_cnt() which expects valid link_speed,
my understanding is that the author intended to have a valid link_speed before
proceeding further.
Although it seems netif_running() check isn't enough to have valid link_speed
as the link could still be down even if the netif is brought up.
Another gap is that in am65_cpsw_est_link_up(), if link was down for more than 1 second
it just abruptly calls am65_cpsw_taprio_destroy().
So I think we need to do the following to improve taprio support in this driver:
1) accept taprio schedule irrespective of netif/link_speed status
2) call pm_runtime_get()/put() before any device access regardless of netif/link_speed state
3) on link_up when if have valid link_speed and taprio_schedule, apply it.
4) on link_down, destroy the taprio schedule form the controller.
But my concern is, this is a decent amount of work and I don't want to delay this series.
My original subject of this patch series was mpqrio/frame-preemption/coalescing. ;)
Can we please defer taprio enhancement to a separate series? Thanks!
>
> I see some inconsistencies I don't quite understand.
>
> am65_cpsw_nuss_ndo_slave_add_vid() checks for netif_running() then calls
> pm_runtime_resume_and_get() anyway.
>
> am65_cpsw_setup_mqprio() allows changing the offload even when the link
> is down (which is more user-friendly anyway) and performs the pm_runtime_get_sync()
> call itself.
>
>> -}
--
cheers,
-roger
On Thu, Dec 14, 2023 at 03:36:57PM +0200, Roger Quadros wrote: > Actually, this code is already present upstream. I'm only moving it around > in this patch. > > Based on the error message and looking at am65_cpsw_est_check_scheds() and > am65_cpsw_est_set_sched_list() which is called later in am65_cpsw_taprio_replace(), > both of which eventually call am65_est_cmd_ns_to_cnt() which expects valid link_speed, > my understanding is that the author intended to have a valid link_speed before > proceeding further. > > Although it seems netif_running() check isn't enough to have valid link_speed > as the link could still be down even if the netif is brought up. > > Another gap is that in am65_cpsw_est_link_up(), if link was down for more than 1 second > it just abruptly calls am65_cpsw_taprio_destroy(). > > So I think we need to do the following to improve taprio support in this driver: > 1) accept taprio schedule irrespective of netif/link_speed status > 2) call pm_runtime_get()/put() before any device access regardless of netif/link_speed state > 3) on link_up when if have valid link_speed and taprio_schedule, apply it. > 4) on link_down, destroy the taprio schedule form the controller. > > But my concern is, this is a decent amount of work and I don't want to delay this series. > My original subject of this patch series was mpqrio/frame-preemption/coalescing. ;) > > Can we please defer taprio enhancement to a separate series? Thanks! Ok, sounds fair to have some further taprio clean-up scheduled for later. I would also add taprio_offload_get() to the list of improvements that could be made.
On 14/12/2023 15:41, Vladimir Oltean wrote: > On Thu, Dec 14, 2023 at 03:36:57PM +0200, Roger Quadros wrote: >> Actually, this code is already present upstream. I'm only moving it around >> in this patch. >> >> Based on the error message and looking at am65_cpsw_est_check_scheds() and >> am65_cpsw_est_set_sched_list() which is called later in am65_cpsw_taprio_replace(), >> both of which eventually call am65_est_cmd_ns_to_cnt() which expects valid link_speed, >> my understanding is that the author intended to have a valid link_speed before >> proceeding further. >> >> Although it seems netif_running() check isn't enough to have valid link_speed >> as the link could still be down even if the netif is brought up. >> >> Another gap is that in am65_cpsw_est_link_up(), if link was down for more than 1 second >> it just abruptly calls am65_cpsw_taprio_destroy(). >> >> So I think we need to do the following to improve taprio support in this driver: >> 1) accept taprio schedule irrespective of netif/link_speed status >> 2) call pm_runtime_get()/put() before any device access regardless of netif/link_speed state >> 3) on link_up when if have valid link_speed and taprio_schedule, apply it. >> 4) on link_down, destroy the taprio schedule form the controller. >> >> But my concern is, this is a decent amount of work and I don't want to delay this series. >> My original subject of this patch series was mpqrio/frame-preemption/coalescing. ;) >> >> Can we please defer taprio enhancement to a separate series? Thanks! > > Ok, sounds fair to have some further taprio clean-up scheduled for later. > I would also add taprio_offload_get() to the list of improvements that > could be made. Noted. Thanks! -- cheers, -roger
© 2016 - 2025 Red Hat, Inc.