| Message ID | 20231213110721.69154-6-rogerq@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: am65-cpsw: Add mqprio, frame pre-emption & coalescing | expand |
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 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. > >> -}
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!
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)
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