Received-SPF: pass (google.com: domain of linux-kernel+bounces-47775-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Esben Haabendal <esben@geanix.com>
To: "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.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>
Subject: Re: [PATCH net-next 2/2] net: stmmac: TBS support for platform driver
In-Reply-To: <5626e874-066c-4bf2-842d-a7f3387b6c1b@quicinc.com> (Abhishek
	Chauhan's message of "Wed, 31 Jan 2024 13:59:20 -0800")
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>
	<5626e874-066c-4bf2-842d-a7f3387b6c1b@quicinc.com>
Date: Thu, 01 Feb 2024 09:26:06 +0100
Message-ID: <87a5okvbdt.fsf@geanix.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

"Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com> writes:
> 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.

I am not sure what you exact point is here.

If you look at the implementation of ethtool API for changing ring
parameters, you have stmmac_set_ringparam() which calls
stmmac_reinit_ringparam(), which again calls stmmac_release() if the
interface is up (stopping the entire MAC), and then stmmac_open() which
reinitializes everything.

The same pattern could be applied to changes to enable enhanced
descriptor allocation.

I don't see why that will not be acceptable. Why would anyone have to do
that while critical traffic is flowing? In your system you should be
able to know which queues needs enhanced descriptors before starting
communication.

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

A good default would IMHO be to enable enhanced descriptors for all but
TX queue 0. This will allow use of TBS without needing to change
anything. If the rather minimal extra memory usage is disturbing anyone,
then they can tune it at boot time before bringing the interface up.

> 2. Live with the disruption of traffic for a brief duration of time.
> Which is not a good solution for priority and critical traffic.

As mentioned above, I don't see why anyone would need to modify the
descriptor allocation while critical traffic is flowing.

If you are able put this information in your device tree, you definitely
will be able to put it in an boot script in some form.

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

Exactly. We are discussing a solution to activate enhanced descriptors
at "runtime". But I propose to do it in a similar way as changing ring
parameters, so it is in runtime seen from a CPU perspective, but the
interface will be shortly brought down when changing it.

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

I think we should do something like this:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3b12ec8f618e

for all glue drivers, so a sane default is established that allows using
TBS from boot up.

But in addition to that, I think it would make sense to create an
ethtool API to change it from that default. And it will bring down the
interface while applying the change.

/Esben