Message-ID: <26384052-86fa-dc29-51d8-f154a0a71561@intel.com>
Date:   Tue, 30 Aug 2022 13:09:20 -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 11:10 AM, Jacob Keller wrote:
> 
> 
> 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

I'm trying to figure out what my next steps are here.

Jakub, from earlier discussion it sounded like you are ok with accepting
patch to include "No FEC" into our auto override behavior, with no uAPI
changes. Is that still ok given the recent dicussion regarding going
beyond the spec?

I'm also happy to rename the flag in ice so that its not misnamed and
clearly indicates its behavior.

Gal seems against extending uAPI to indicate or support "ignore spec".
To be properly correct that would mean changing ice to stop setting the
AUTO_FEC flag. As explained above, I believe this will lead to breakage
in situations where we used to link and function properly.

I have no way to verify whether other vendors actually follow this or
not, as it essentially requires checking with modules that wouldn't link
otherwise and likely requires a lot of trial and error.

Powered by blists - more mailing lists