Received-SPF: pass (google.com: domain of linux-kernel+bounces-47220-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <5626e874-066c-4bf2-842d-a7f3387b6c1b@quicinc.com>
Date: Wed, 31 Jan 2024 13:59:20 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver
To: Esben Haabendal <esben@geanix.com>
CC: Rohan G Thomas <rohan.g.thomas@intel.com>,
        "David S . Miller"
	<davem@davemloft.net>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        "Jose
 Abreu" <joabreu@synopsys.com>,
        Eric Dumazet <edumazet@google.com>,
        "Jakub
 Kicinski" <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
        Maxime Coquelin
	<mcoquelin.stm32@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        "Krzysztof
 Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley
	<conor+dt@kernel.org>,
        Giuseppe Cavallaro <peppe.cavallaro@st.com>,
        "Serge
 Semin" <fancer.lancer@gmail.com>,
        Andrew Halaney <ahalaney@redhat.com>, <elder@linaro.org>,
        <netdev@vger.kernel.org>, <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-kernel@lists.infradead.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <quic_bhaviks@quicinc.com>,
        <kernel.upstream@quicinc.com>
References: <20230927130919.25683-1-rohan.g.thomas@intel.com>
 <20230927130919.25683-3-rohan.g.thomas@intel.com>
 <92892988-bb77-4075-812e-19f6112f436e@quicinc.com>
 <87r0i44h8v.fsf@geanix.com>
Content-Language: en-US
From: "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com>
In-Reply-To: <87r0i44h8v.fsf@geanix.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

Hi Esben,
	My responses are inline to the queries. I hope i have justified my problem statement.  

On 1/26/2024 12:43 AM, Esben Haabendal wrote:
> "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> writes:
> 
>> Qualcomm had similar discussions with respect to enabling of TBS for a
>> particular queue. We had similar discussion on these terms yesterday
>> with Redhat. Adding Andrew from Redhat here
>>
>> What we discovered as part of the discussions is listed below.
>>
>> 1. Today upstream stmmac code is designed in such a way that TBS flag
>> is put as part of queue configurations(see below snippet) and as well
>> know that stmmac queue configuration comes from the dtsi file.
>>
>> //ndo_open => stmmac_open
>> int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;(comes from tx_queues_cfg)
>>
>> /* Setup per-TXQ tbs flag before TX descriptor alloc */
>> tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
>>
>> 2. There is a no way to do this dynamically from user space because we don't have any 
>> API exposed which can do it from user space
> 
> Not now. But why not extend ethtool API to allow enabling TBS for
> supported controllers?
> 
ethtool API can be implemented but that still doesn't solve the problem of stopping the 
entire MAC block because of enhanced desc allocation. 
1. We can either allocate enhanced desc for all channels at bootup and then choose 
to switch to enable TBS mode at runtime  (Additional memory usage)
2. Live with the disruption of traffic for a brief duration of time. 
Which is not a good solution for priority and critical traffic. 
>> and also TBS rely on special descriptors aka enhanced desc this cannot
>> be done run time and stmmac has to be aware of it before we do
>> DMA/MAC/MTL start.
> 
> Isn't this somewhat similar to changing the RX/TX ring parameters, which
> I believe also is quite difficult to do at run time, and ethtool
> therefore requires the interface to be down in oroer to change them?
> 
>> To do this dynamically would only mean stopping DMA/MAC/MTL realloc
>> resources for enhanced desc and the starting MAC/DMA/MTL. This means
>> we are disrupting other traffic(By stopping MAC block).
> 
> Yes. But you would be disrupting traffic less than by requiring a
> complete reboot of the target which is needed if the devicetree must be
> changed.
> 
any DTS solution today anyway requires completely loading the boot image 
and rebooting the device, but once the device is functional,
End user can activate TBS, as he knows the exact usecase and requirements. 
I understand the solution is not scalable, but at this point we don't have a solution 
to activate TBS  at runtime. 
>> 3. I dont think there is a way we can enable this dynamically today. I
>> would like upstream community to share your thoughts as well.
> 
> Hereby done. Could we investigate the possibility of using ethtool to
> change TBS enable/disable "run-time"?
> 
We can either allocate enhanced desc for all channels at bootup
and then choose to switch to enable TBS mode at runtime
>> 4. I agree with Rohan's patch here and want upstream community to
>> accept it. This will allow use to configure the queues where TBS needs
>> to be enabled as hardcoding in the code unless upstream has better way
>> to this using userspace.
>>
>> Please let us know if you think otherwise. 
> 
> /Esben
Best Regards
ABC