Message-ID: <3f72e038-016d-8b1c-a215-243199bac033@intel.com>
Date:   Mon, 29 Aug 2022 11:10:41 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Gal Pressman <gal@...dia.com>, Jakub Kicinski <kuba@...nel.org>
CC:     Saeed Mahameed <saeedm@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Simon Horman <horms@...ge.net.au>,
        Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [PATCH net-next 0/2] ice: support FEC automatic disable



On 8/29/2022 4:21 AM, Gal Pressman wrote:
> On 29/08/2022 10:11, Keller, Jacob E wrote:
>>> Regardless, changing our interface because of one device' firmware
>>> bug/behavior change doesn't make any sense. This interface affects all
>>> consumers, and is going to stick with us forever. Firmware will
>>> eventually get updated and it only affects one driver.
>> Well, the current ice behavior for every FEC mode *except* No FEC, we try modes which may be supportable even though they're outside the spec. As far as I understand, the reason we attempt these is because it allows linking in more scenarios, presumably because some combination of things is not fully spec compliant? I don't really know for sure.
>>
>> For future firmware, this would include No FEC as well. Thus, even with future firmware we'd still be trying some modes outside of the spec. I can try to get some more answers tomorrow about the reasoning and justification for this behavior.
>>
> 
> Yea, understood, but respectfully, I don't understand why we should go
> along with your requirement to support this non-spec behavior.

My understanding is that this is requested by customers for a few reasons:

1) interopability with legacy switches

2) interopability with modules which don't follow spec

3) low latency applications for which disabling FEC can improve latency
if the module is able to achieve a low enough error rate.

We have a fair number of customer requests to support these
non-compliant modules and modes, including both enabling certain FEC
modes or disabling FEC.

We already have this enabled with existing drivers. Of course, part of
that was caused by confusion due to poor naming scheme and lack of clear
communication to us about what the real behavior was. (Thanks Kuba for
pushing on that...) It probably comes across as a bit disingenuous
because we've implemented and enabled this support without being clear
about the behavior.

I haven't 100% confirmed, but I would be surprised if this only affects
ice. Its likely something that behaves similarly for other Intel products.

The ability to go outside the spec enables some of our customers and
solves real problems. The reality is that we don't always have perfect
hardware, and we want to inter-operate with the existing hardware. Some
switches were designed and built while the standards were still being
developed, and they don't 100% follow the spec because of this.

By extending the interface it becomes clear and obvious that we're going
outside the spec. If this hadn't been brought up this would have more or
less hidden behind a binary firmware blob with almost no way to notice
it, and no way to communicate that is whats happening.

I'm frustrated by the poor communication here because it was not at all
obvious to me until the last week that this is what we were doing.
However, I do see value in supporting the existing hardware available
even when its not quite spec compliant.

Thanks,
Jake

Powered by blists - more mailing lists