From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2433FC4345F for ; Thu, 25 Apr 2024 11:57:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:References:CC:To:Subject: From:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=O1L8FgAE6PKrrDQJlWy0W+mdVM/2M4V3wxze//r9b4M=; b=w2BaJOlwffAnjX pzGzQeu1jpA2IostQHZPd4RPggeFh6AvgDULp9rkY9RTGFvKZzY7PjzUmmRKU9QfoqmCT2DMe/Zmy bfiZlwiIwyamN1BWSvnFtLGfVtSEDuP6ynmrorR5cI3Digy1GT1D5PvleERTMBsH56aBtthLI+vf4 s4dQNN4AUwh+akdJIGTfoAxw9dwkoHHgGuMw6D2BUzWxFDHYO8epUxhAqAW6CHScNQ83sWvOfZdWt iOShKcE112EIfmCi+W9zgShPGK/G5ix7FlEfS3XnpW2Fg1agYnedq6tShHzst2kCJNgUxS9LxTdJm n9fdqtsLCNFXUOSoz2cw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rzxj8-0000000867N-3v1L; Thu, 25 Apr 2024 11:57:22 +0000 Received: from fllv0015.ext.ti.com ([198.47.19.141]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rzxj6-0000000866R-0pmC for linux-arm-kernel@lists.infradead.org; Thu, 25 Apr 2024 11:57:21 +0000 Received: from lelv0265.itg.ti.com ([10.180.67.224]) by fllv0015.ext.ti.com (8.15.2/8.15.2) with ESMTP id 43PBv9BN013919; Thu, 25 Apr 2024 06:57:09 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1714046229; bh=XGs43/u3uem7yqAA7OT6OEKLkrxr4H8yBUmDsRcElWI=; h=Date:From:Subject:To:CC:References:In-Reply-To; b=dng4M3qQpKNEVq8Hm+zrJciDoVBMaV3lyh3OtM9/5Hallu0VxqCqtIYh1ldiB67pF oH4tVZKifqrq7dqySNqQm/HQMQNF8bb2R+sQpDT2B/jXDiH5webJ+69km4hZEiFocq bjfk9wW9VodNydnUoCpwmM8FActCpxQBgqH6k7YI= Received: from DLEE105.ent.ti.com (dlee105.ent.ti.com [157.170.170.35]) by lelv0265.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 43PBv9IF001210 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 25 Apr 2024 06:57:09 -0500 Received: from DLEE100.ent.ti.com (157.170.170.30) by DLEE105.ent.ti.com (157.170.170.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Thu, 25 Apr 2024 06:57:09 -0500 Received: from lelvsmtp6.itg.ti.com (10.180.75.249) by DLEE100.ent.ti.com (157.170.170.30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Thu, 25 Apr 2024 06:57:08 -0500 Received: from [10.24.69.25] (danish-tpc.dhcp.ti.com [10.24.69.25]) by lelvsmtp6.itg.ti.com (8.15.2/8.15.2) with ESMTP id 43PBv3nQ038298; Thu, 25 Apr 2024 06:57:04 -0500 Message-ID: Date: Thu, 25 Apr 2024 17:27:03 +0530 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: MD Danish Anwar Subject: Re: [PATCH net-next v4] net: ti: icssg_prueth: add TAPRIO offload support To: Vladimir Oltean CC: Andrew Lunn , Roger Quadros , Vignesh Raghavendra , Richard Cochran , Paolo Abeni , Jakub Kicinski , Eric Dumazet , "David S. Miller" , Simon Horman , , , , , , Roger Quadros , Vinicius Costa Gomes References: <20231006102028.3831341-1-danishanwar@ti.com> <20231011102536.r65xyzmh5kap2cf2@skbuf> <20231006102028.3831341-1-danishanwar@ti.com> <20231011102536.r65xyzmh5kap2cf2@skbuf> <20240118012714.gzgmfwzb6tuuyofs@skbuf> Content-Language: en-US In-Reply-To: <20240118012714.gzgmfwzb6tuuyofs@skbuf> X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240425_045720_362244_FE040D4B X-CRM114-Status: GOOD ( 38.82 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 18/01/24 6:57 am, Vladimir Oltean wrote: > On Mon, Jan 15, 2024 at 12:24:12PM +0530, MD Danish Anwar wrote: >>> I believe the intention is for this code to be run before any taprio >>> offload is added, correct? But it is possible for the user to add an >> >> Yes, the intention here is to run this code before any taprio offload is >> added. > > Then it is misplaced? > Where should I move it then? Perhaps to end of prueth_probe()? If this is moved to prueth_probe() then it will mean it's always called. If user adds an offloaded Qdisc even while the netdev has not yet been brought up, it will not result into any error >>> offloaded Qdisc even while the netdev has not yet been brought up. >>> Is that case handled correctly, or will it simply result in NULL pointer >>> dereferences (tas->config_list)? >>> >> >> In that case, it will eventually result in NULL pointer dereference as >> tas->config_list will be pointing to NULL. To handle this correctly we >> can add the below check in emac_taprio_replace(). >> >> if (!ndev_running(ndev)) { >> netdev_err(ndev, "Device is not running"); >> return -EINVAL; >> } > > What is the reason for which the device has to be running, other than > your placement of icssg_qos_tas_init()? > Yes, I only suggested this check for that. If icssg_qos_tas_init() is handled correctly and called before user adds qdisc, this cheeck will no longer be needed. >>>> + >>>> + cycle_time = admin_list->cycle_time - 4; /* -4ns to compensate for IEP wraparound time */ >>> >>> Details? Doesn't this make the phase alignment of the schedule diverge >>> from what the user expects? >> >> 4ns is needed to compensate for IEP wraparound time. IEP is the clock >> used by ICSSG driver. IEP tick is 4ns and we adjust this 4ns whenever >> calculating cycle_time. You may refer to [1] for details on IEP driver. > > What is understood by "IEP wraparound time"? Its time wraps around what? IEP clock runs at 250 MHz, 1 tick of IEP clock = NSEC_PER_SEC / iep->refclk_freq i.e. 1000000000 / 250000000 = 4ns. Thus 1 tick of IEP clock is 4ns. > It wraps around exactly once every taprio cycle of each port and that's > why the cycle-time is compensated, or how does that work? > Yes, it wraps around exactly once every taprio cycle and to compensate for that we adjust 4ns. Instead of hardcoding I will use a varaible here. It is a hardware errata but it is not public yet. >>> >>>> + base_time = admin_list->base_time; >>>> + cur_time = prueth_iep_gettime(emac, &sts); >>>> + >>>> + if (base_time > cur_time) >>>> + change_cycle_count = DIV_ROUND_UP_ULL(base_time - cur_time, cycle_time); >>>> + else >>>> + change_cycle_count = 1; >>> >>> I see that the base_time is only used to calculate the number of cycles >>> relative to cur_time. Taprio users want to specify a basetime value >>> which indicates the phase alignment of the schedule. This is important >>> when the device is synchronized over PTP with other switches in the >>> network. Can you explain how is the basetime taken into consideration in >>> your implementation? >>> >> >> In this implementation base_time is used only to obtain the >> change_cycle_count and to write it to TAS_CONFIG_CHANGE_CYCLE_COUNT. In >> this implementation base_time is not used for anything else. > > So there is zero granularity in the base-time beyond the number of cycles? > That is very bad, because it means the hardware cannot be used in a > practical TSN network where schedules are offset in phase to each other. > It needs to be able to be told when the schedule begins, with precision. > Not just how many cycles from now (what does 'now' even mean?). > Currently base_time is only used for calculating number of cycles. >>> Better to say what's the hardware maximum, than to report back num_entries >>> as being not supported. >>> >> >> Sure, I'll change it to below, >> >> if (taprio->num_entries > TAS_MAX_CMD_LISTS) { >> NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "num_entries %ld is more than maximum supported entries %ld in taprio config\n", >> taprio->num_entries, TAS_MAX_CMD_LISTS); >> return -EINVAL; >> } > > Keep in mind that NETLINK_MAX_FMTMSG_LEN is only 80 characters. Also, \n > is not needed in netlink extack messages. And indentation also looks off. > Sure. >>>> + >>>> + emac_cp_taprio(taprio, est_new); >>>> + emac->qos.tas.taprio_admin = est_new; >>>> + ret = tas_update_oper_list(emac); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = tas_set_state(emac, TAS_STATE_ENABLE); >>>> + if (ret) >>>> + devm_kfree(&ndev->dev, est_new); >>>> + >>>> + return ret; >>>> +} >> >> Below is how the code will look like. >> >> emac->qos.tas.taprio_admin = taprio_offload_get(taprio); > > emac->qos.tas.taprio_admin can also hold an old offload, which is leaked > here when assigning the new one ("tc qdisc replace dev eth0 root taprio"). > >> ret = tas_update_oper_list(emac); >> if (ret) >> return ret; >> >> ret = tas_set_state(emac, TAS_STATE_ENABLE); >> if (ret) { >> emac->qos.tas.taprio_admin = NULL; >> taprio_offload_free(taprio); >> } >> >> return ret; >> >> Please let me know if all of these changes looks ok, I'll resend the >> patch once you confirm. Thanks for reviewing. > > Hard to say from this snippet. taprio_offload_free() will be needed from > emac_taprio_destroy() as well. Sure. I will do that too. I will be sharing a next revision soon. Thanks for reviewing. -- Thanks and Regards, Danish _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel