diff mbox series

[v6,net-next,2/7] net: ethernet: am65-cpsw: cleanup TAPRIO handling

Message ID 20231120140147.78726-3-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

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/codegen success Generated files up to date
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -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 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
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 No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: line length of 86 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Roger Quadros Nov. 20, 2023, 2:01 p.m. UTC 
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 | 149 +++++++++++-------------
 1 file changed, 70 insertions(+), 79 deletions(-)

Changelog:

v6: initial commit

Comments

Vladimir Oltean Nov. 20, 2023, 10:56 p.m. UTC | #1 
On Mon, Nov 20, 2023 at 04:01:42PM +0200, Roger Quadros wrote:
> -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];
> +}

I think I mentioned this before: have you looked at taprio_offload_get()
and taprio_offload_put()?

> +
> +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;
>  
> -	am65_cpsw_est_update_state(ndev);
> +	if (!netif_running(ndev)) {
> +		NL_SET_ERR_MSG_MOD(extack, "interface is down, link speed unknown\n");

The extack message doesn't need a \n.

> +		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\n");

Also here.

> +		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");

Here it's ok.

> +		return -EOPNOTSUPP;
>  	}
Roger Quadros Nov. 21, 2023, 9:23 a.m. UTC | #2 
On 21/11/2023 00:56, Vladimir Oltean wrote:
> On Mon, Nov 20, 2023 at 04:01:42PM +0200, Roger Quadros wrote:
>> -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];
>> +}
> 
> I think I mentioned this before: have you looked at taprio_offload_get()
> and taprio_offload_put()?

I'm sorry that I missed this. I'll take a look.

> 
>> +
>> +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;
>>  
>> -	am65_cpsw_est_update_state(ndev);
>> +	if (!netif_running(ndev)) {
>> +		NL_SET_ERR_MSG_MOD(extack, "interface is down, link speed unknown\n");
> 
> The extack message doesn't need a \n.

OK.

> 
>> +		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\n");
> 
> Also here.
> 
>> +		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");
> 
> Here it's ok.
> 
>> +		return -EOPNOTSUPP;
>>  	}

Thanks for the detailed review!
Roger Quadros Nov. 21, 2023, 11:11 a.m. UTC | #3 
On 21/11/2023 11:23, Roger Quadros wrote:
> 
> 
> On 21/11/2023 00:56, Vladimir Oltean wrote:
>> On Mon, Nov 20, 2023 at 04:01:42PM +0200, Roger Quadros wrote:
>>> -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];
>>> +}
>>
>> I think I mentioned this before: have you looked at taprio_offload_get()
>> and taprio_offload_put()?
> 
> I'm sorry that I missed this. I'll take a look.

Now I recollect. You mentioned this in a different series review
https://lore.kernel.org/all/20231011102536.r65xyzmh5kap2cf2@skbuf/

Since this patch is more trivial cleanups I will do the
taprio_offload_get/free() change to a separate patch.
> 
>>
>>> +
>>> +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;
>>>  
>>> -	am65_cpsw_est_update_state(ndev);
>>> +	if (!netif_running(ndev)) {
>>> +		NL_SET_ERR_MSG_MOD(extack, "interface is down, link speed unknown\n");
>>
>> The extack message doesn't need a \n.
> 
> OK.
> 
>>
>>> +		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\n");
>>
>> Also here.
>>
>>> +		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");
>>
>> Here it's ok.
>>
>>> +		return -EOPNOTSUPP;
>>>  	}
> 
> Thanks for the detailed review!
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.c b/drivers/net/ethernet/ti/am65-cpsw-qos.c
index 4bc611cc4aad..3539fb31541b 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;
 
-	am65_cpsw_est_update_state(ndev);
+	if (!netif_running(ndev)) {
+		NL_SET_ERR_MSG_MOD(extack, "interface is down, link speed unknown\n");
+		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\n");
+		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)