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 B87D3CD4857 for ; Wed, 4 Sep 2024 15:44:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=M6e2YvfuCivW4PaDMInxNwbpYH1/P7j1Q84PnG6YUUM=; b=Of9DXfRDC3GVKpmiNiGCQFPFa4 QVN7w6+yvJFNEL+8Q/5qWL8wgdmgAC8S6U8x4+s7ObCZrvbgyrZkvNEsBu+z18vb2/4O/ITgFAD03 Rpd8uvF4jwgPjitPLsDJITKvTFjvi7vhjrt71yxYkxJ5YNmgcJgn3eBcDFybb9bzt4W2LOXaR+vm2 EEwk40AeOMpogpTGWT+a8/PHJh0zTXAinouzYt/qJkDTegmZVzvSvqlvepe5z7AGqXeaw46tRuG37 Gs2KvVE8nErao/Y6lezv+jV7rwX72GDnbn7vpq+3S0xYzcB+eesexlrW9FzstPvuMmnPV5VxONXYZ gHOuR7tQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1slsBC-000000054Nx-1R3A; Wed, 04 Sep 2024 15:44:22 +0000 Received: from mail-ej1-x633.google.com ([2a00:1450:4864:20::633]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sls9Q-000000053sq-0KJT for linux-arm-kernel@lists.infradead.org; Wed, 04 Sep 2024 15:42:33 +0000 Received: by mail-ej1-x633.google.com with SMTP id a640c23a62f3a-a8713b00219so42763866b.0 for ; Wed, 04 Sep 2024 08:42:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1725464550; x=1726069350; darn=lists.infradead.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=M6e2YvfuCivW4PaDMInxNwbpYH1/P7j1Q84PnG6YUUM=; b=Y8tb2p3zjmQ63883urNWIaX4jKPZlcGPylpkBCoQIns9B02EW+MW8dPDNE3Ha5it2X TD0BsBJrhtmf5KrtddBdiqt8gk9iVIGDgQ1v/p92wTs7Xpps05Zoj7oGAqOEbNpmu/C9 CQmtRjqdczMLW4zE9LvsCwlqIIkQSzYjfs4JO/8O3R6MTb5MTLsoxqgUcnAYQvbnQ2wb XwKZcvAsHjtKEzfI5z+MLyPxSg7PCiN8kcg+yrvU4pzWFo2Hk9NWMQU5CpGYNmKAkR1L 4PkaID9u7YIKJsLGk01gypYmYrWVTgpniFd0dEGIs/JdqIu1aRNsQzDwkGmjjsRWHzhX zung== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725464550; x=1726069350; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=M6e2YvfuCivW4PaDMInxNwbpYH1/P7j1Q84PnG6YUUM=; b=Zs/3CHBHx5KTwtsp7gBqW7rH1g+V1n5CKncabJtnAcbiaZyru+k9MtKXLdTPqyJp2k Kl3+WQ/ihjDe0X/mtReKsmtjHPe38mnnS1+VxoeNMqyYDnQ7nRQz7W3qsKnjFSvr4Ddh yxzoQkXprPV121bYOtowSdiB0uB1X3xHi1Cvzmc5FA2WfY4hFjqrIhhJR7aPQUq96l0c J/oKiPLS+5ZGOKaE7tmXUZEouR4a305tKZ1SqrIIasQMBdmj6UGPk3TI9FFmFpu+bRJM 8IzWXS4LxxwBZzNNEYHXNRwV6AGYf2UV/G0mMblx69uGlt9r/BtNolFi6XJHhcIR/tod W2FA== X-Forwarded-Encrypted: i=1; AJvYcCU7gXlyeY+YEXONGdlqsvjfVPIJEuL9db4mEUW8FxnlNbsnPB+UCvoTsRwJswTUUNLmuaf9Gdku+RlTEExTn/Wz@lists.infradead.org X-Gm-Message-State: AOJu0YwcZF5qef90iviPFsOu3cRPOjr7ek7ENLViU7BBx3EZQlNM6OH3 dT48U2qFrPmPinqcGYmeeV8F2Pw10RDRMuiLvmMhLVHJ6flB81BJ X-Google-Smtp-Source: AGHT+IHMic4XBuw2foanj88RblIDJ3ytZUAtd4gGVvzDtDWkXLLbXdQlWIpM8/kI4V1eZSw3Ww7rJQ== X-Received: by 2002:a17:907:7b94:b0:a7a:87b3:722f with SMTP id a640c23a62f3a-a89a34c8c79mr775057766b.3.1725464549704; Wed, 04 Sep 2024 08:42:29 -0700 (PDT) Received: from skbuf ([188.25.134.29]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a8a62038d59sm7125066b.52.2024.09.04.08.42.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Sep 2024 08:42:29 -0700 (PDT) Date: Wed, 4 Sep 2024 18:42:26 +0300 From: Vladimir Oltean To: Furong Xu <0x1207@gmail.com> Cc: Alexander Lobakin , Serge Semin , "David S. Miller" , Alexandre Torgue , Jose Abreu , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Maxime Coquelin , Joao Pinto , netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, rmk+kernel@armlinux.org.uk, linux@armlinux.org.uk, xfr@outlook.com Subject: Re: [PATCH net-next v7 5/7] net: stmmac: support fp parameter of tc-mqprio Message-ID: <20240904154226.ztksb6sv4mjccb5l@skbuf> References: <28f580d1c1e3cfdb0803207a5e05d42c4f9dd529.1725441317.git.0x1207@gmail.com> <28f580d1c1e3cfdb0803207a5e05d42c4f9dd529.1725441317.git.0x1207@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <28f580d1c1e3cfdb0803207a5e05d42c4f9dd529.1725441317.git.0x1207@gmail.com> <28f580d1c1e3cfdb0803207a5e05d42c4f9dd529.1725441317.git.0x1207@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240904_084232_179143_33E4B566 X-CRM114-Status: GOOD ( 19.55 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Sep 04, 2024 at 05:21:20PM +0800, Furong Xu wrote: > +static void stmmac_reset_tc_mqprio(struct net_device *ndev, > + struct netlink_ext_ack *extack) > +{ > + struct stmmac_priv *priv = netdev_priv(ndev); > + > + netdev_reset_tc(ndev); > + netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use); > + stmmac_fpe_map_preemption_class(priv, ndev, extack, 0); > +} > + > +static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv, > + struct tc_mqprio_qopt_offload *mqprio) > +{ > + struct netlink_ext_ack *extack = mqprio->extack; > + struct tc_mqprio_qopt *qopt = &mqprio->qopt; > + u32 offset, count, num_stack_tx_queues = 0; > + struct net_device *ndev = priv->dev; > + u32 num_tc = qopt->num_tc; > + int err; > + > + if (!num_tc) { > + stmmac_reset_tc_mqprio(ndev, extack); > + return 0; > + } > + > + err = netdev_set_num_tc(ndev, num_tc); > + if (err) > + return err; > + > + for (u32 tc = 0; tc < num_tc; tc++) { > + offset = qopt->offset[tc]; > + count = qopt->count[tc]; > + num_stack_tx_queues += count; > + > + err = netdev_set_tc_queue(ndev, tc, count, offset); > + if (err) > + goto err_reset_tc; > + } > + > + err = netif_set_real_num_tx_queues(ndev, num_stack_tx_queues); > + if (err) > + goto err_reset_tc; > + > + err = stmmac_fpe_map_preemption_class(priv, ndev, extack, > + mqprio->preemptible_tcs); > + if (err) > + goto err_reset_tc; I appreciate the improvement with the separate tc_ops, but I'm still not in love with this. This stmmac_hw entry (copied with line numbers because it lacks a name by which I can easily reference it): 159 » }, { 160 » » .gmac = false, 161 » » .gmac4 = true, 162 » » .xgmac = false, 163 » » .min_id = 0, 164 » » .regs = { 165 » » » .ptp_off = PTP_GMAC4_OFFSET, 166 » » » .mmc_off = MMC_GMAC4_OFFSET, 167 » » » .est_off = EST_GMAC4_OFFSET, 168 » » }, 169 » » .desc = &dwmac4_desc_ops, 170 » » .dma = &dwmac4_dma_ops, 171 » » .mac = &dwmac4_ops, 172 » » .hwtimestamp = &stmmac_ptp, 173 » » .mode = NULL, 174 » » .tc = &dwmac510_tc_ops, 175 » » .mmc = &dwmac_mmc_ops, 176 » » .est = &dwmac510_est_ops, 177 » » .setup = dwmac4_setup, 178 » » .quirks = stmmac_dwmac4_quirks, 179 » }, { has .mac = &dwmac4_ops, so it does not implement .fpe_map_preemption_class(). But it also has .tc = &dwmac510_tc_ops, so tc_setup_dwmac510_mqprio() will get called. Thus, I suppose that the stmmac_fpe_map_preemption_class() -> stmmac_do_void_callback() mechanism will return -EINVAL for dwmac4_ops, and this will make mqprio offload fail, for the sole reason that FPE is not supported, _EVEN IF_ FPE was never requested (mqprio->preemptible_tcs is 0), and the offload could have otherwise been applied just fine. Not to mention my previous complaint still applies, that the test for the presence of stmmac_ops :: fpe_map_preemption_class() is unnaturally late relative to the flow of the tc_setup_dwmac510_mqprio() function. Thus, I really recommend you to replace the stmmac_do_void_callback() anti-pattern with something like: // early if (mqprio->preemptible_tcs && !priv->hw->ops->fpe_map_preemption_class) { NL_SET_ERR_MSG_MOD(mqprio->extack, "Cannot map preemptible TCs to TXQs"); return -EOPNOTSUPP; } // late if (priv->hw->ops->fpe_map_preemption_class) { err = priv->hw->ops->fpe_map_preemption_class(priv->dev, mqprio->preemptible_tcs, extack); if (err) goto err_reset_tc; } WARNING: code not tested. The idea is that the early check makes sure that only dwmac410_ops and dwmac510_ops permit mqprio->preemptible_tcs to go to a non-zero value (which can later be reset to zero if desired, in the late code path). For dwmac4_ops, mqprio->preemptible_tcs = 0 is the only supported value (for which nothing needs to be done), and the late code path is bypassed, due to the "if" condition returning false. Either organize like this, or if you really, really, really insist to use the stmmac_do_callback() anti-pattern in new code, at least don't share the setup_mqprio() code path between MACs that implement fpe_map_preemption_class() and MACs that don't.