Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <4f09985e-396b-42e9-b7a0-5f9db58e6131@kernel.org>
Date:   Tue, 21 Nov 2023 13:11:00 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 net-next 2/7] net: ethernet: am65-cpsw: cleanup TAPRIO
 handling
Content-Language: en-US
From:   Roger Quadros <rogerq@kernel.org>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, s-vadapalli@ti.com, r-gunasekaran@ti.com,
        vigneshr@ti.com, srk@ti.com, horms@kernel.org, p-varis@ti.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20231120140147.78726-1-rogerq@kernel.org>
 <20231120140147.78726-1-rogerq@kernel.org>
 <20231120140147.78726-3-rogerq@kernel.org>
 <20231120140147.78726-3-rogerq@kernel.org>
 <20231120225648.pgvzd2jejg5jll2t@skbuf>
 <af23bd5d-2daf-487b-858c-9e3ad684864d@kernel.org>
In-Reply-To: <af23bd5d-2daf-487b-858c-9e3ad684864d@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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!
> 

-- 
cheers,
-roger