Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Florian Fainelli <f.fainelli@gmail.com>]

On 04/12/2017 07:58 AM, Andrew Lunn wrote:
> On Wed, Apr 12, 2017 at 01:07:02PM +0100, Joao Pinto wrote:
>> Hi Andrew,
>>
>> Às 12:56 PM de 4/12/2017, Andrew Lunn escreveu:
>>> On Wed, Apr 12, 2017 at 10:26:20AM +0100, Joao Pinto wrote:
>>>> When the Drop Transmit Status bit is set, the Tx packet status
>>>> received from the MAC is dropped in the MTL. When this bit is reset,
>>>> the Tx packet status received from the MAC is forwarded to the
>>>> application. This feature will cause a performance improvement.
>>>>
>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>>>> ---
>>>> changes v1->v2:
>>>> - removed mask from dwmac4_enable_tx_drop()
>>>>
>>>>  Documentation/devicetree/bindings/net/stmmac.txt      |  1 +
>>>>  drivers/net/ethernet/stmicro/stmmac/common.h          |  2 ++
>>>>  drivers/net/ethernet/stmicro/stmmac/dwmac4.h          |  1 +
>>>>  drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c     | 12 ++++++++++++
>>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     |  3 +++
>>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c |  2 ++
>>>>  include/linux/stmmac.h                                |  1 +
>>>>  7 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
>>>> index f652b0c..dbcb2cc 100644
>>>> --- a/Documentation/devicetree/bindings/net/stmmac.txt
>>>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
>>>> @@ -60,6 +60,7 @@ Optional properties:
>>>>  		 and MAC2MAC connection.
>>>>  - snps,tso: this enables the TSO feature otherwise it will be managed by
>>>>  		 MAC HW capability register. Only for GMAC4 and newer.
>>>> +- snps,drop-tx-status: this enables drop tx status
>>>
>>> Hi Joao
>>>
>>> Was the conclusion from testing that this cannot be turned on by
>>> default?
>>
>> This feature is great for applications that need good performance, but has a
>> drawback since it has an impact in timestamp feature in Tx. There are some
>> operations in PTP where the timestamp is given to the host through the TX status
>> in the descriptor, so this will have an impact.
>>
>> There's a way of solving this of course by making the driver checking the
>> timestamp in the MAC_Tx_Timestamp_Status_XXX registers, but I can only look into
>> that feature later in the future.
> 
> So the numbers you show, even if they are not on real hardware that
> anybody uses, do look good.
> 
> But i don't like this DT property, it sounds like it can potential do
> bad things to PTP, and you say it can be done without the DT or PTP
> problems. So I would personally NACK this patch, and ask you to
> re-submit when you have solved these issues.

Is not it possible to simply figure out whether this can safely be
enabled at run time or not? If nobody uses PTP, then turn that on as
soon as you get someone using PTP, turn this back off?

The problem is the commit message is extremely scarce about what this
implies, and quite frankly, I don't understand what "application" means
here, whether this means:

- application in the sense of the HW IP, which is then, the OS'
networking stack presumably?

- application in the sense of the OS, which is sort of conflated with
the socket user?

Worst case, can't you create a new netdev feature (e.g: similar to
NETIF_F_RXFCS) that allows dynamically turning this on based on user
configuration? Not suggesting this is the right approach here, but it
should be thought of. Maybe an ethtool private flag is more appropriate?

One perspective you need to look at is the following:

- as an IP vendor, you would like to exercise every little configuration
knob that the IP can be synthesized with, and that's fine and fun to do
as long as you are in your controlled environment, this may result in
custom DT properties, but those may not be candidate for the upstream
kernel (just downstream)

- customers want something that works out of the box, don't require
reading a DT binding (in fact, not reading at all is best) and just get
the promised performance, and the upstream kernel is the best place to
get this done, but it needs to be painless

Finally, and completely tangential to this patch, pretty much *every*
SoC that has a STMMAC is reasonably cheap to buy, so if Synopsys does
not, you could, with your own personal money acquire a full set of
Allwinner, Meson, Sunxi and what not platforms and put these in a test
labs, heck, I'm sure someone could offer these. Most maintainers just
collect HW as well as tribal knowledge about what typically breaks on
this or that platform, and then run their tests on said platforms.

Hope this helps!
-- 
Florian