Re: Potential regression/bug in net/mlx5 driver

[Saeed Mahameed <saeedm@nvidia.com>]

On 13 Apr 15:51, Jakub Kicinski wrote:
>On Thu, 13 Apr 2023 15:34:21 -0700 Saeed Mahameed wrote:
>> >On a closer read I don't like what this patch is doing at all.
>> >I'm not sure we have precedent for "management connection" functions.
>> >This requires a larger discussion. And after looking up the patch set
>>
>> But this management connection function has the same architecture as other
>> "Normal" mlx5 functions, from the driver pov. The same way mlx5
>> doesn't care if the underlaying function is CX4/5/6 we don't care if it was
>> a "management function".
>
>Yes, and that's why every single IPU implementation thinks that it's
>a great idea. Because it's easy to implement. But what is it for
>architecturally? Running what is effectively FW commands over TCP?

Where did you get this idea from? maybe we got the name wrong, 
"management PF" is simply a minimalistic netdev PF to have eth connection
with the on board BMC .. 

I agree that the name "management PF" sounds scary, but it is not a control
function as you think, not at all. As the original commit message states:
"loopback PF designed for communication with BMC".

>
>> We are currently working on enabling a subset of netdev functionality using
>> the same mlx5 constructs and current mlx5e code to load up a mlx5e netdev
>> on it..
>>
>> >it went in, it seems to have been one of the hastily merged ones.
>> >I'm sending a revert.
>>
>> But let's discuss what's wrong with it, and what are your thoughts ?
>> the fact that it breaks a 6 years OLD FW, doesn't make it so horrible.
>
>Right, the breakage is a separate topic.
>
>You say 6 years old but the part is EOL, right? The part is old and
>stable, AFAIU the breakage stems from development work for parts which
>are 3 or so generations newer.
>

Officially we test only 3 GA FWs back. The fact that mlx5 is a generic CX
driver makes it really hard to test all the possible combinations, so we
need to be strict with how back we want to officially support and test old
generations.

>The question is who's supposed to be paying the price of mlx5 being
>used for old and new parts? What is fair to expect from the user
>when the FW Paul has presumably works just fine for him?
>
Upgrade FW when possible, it is always easier than upgrading the kernel.
Anyways this was a very rare FW/Arch bug, We should've exposed an
explicit cap for this new type of PF when we had the chance, now it's too
late since a proper fix will require FW and Driver upgrades and breaking
the current solution we have over other OSes as well.

Yes I can craft an if condition to explicitly check for chip id and FW
version for this corner case, which has no precedence in mlx5, but I prefer
to ask to upgrade FW first, and if that's an acceptable solution, I would
like to keep the mlx5 clean and device agnostic as much as possible.

>> The patchset is a bug fix where previous mlx5 load on such function failed
>> with some nasty kernel log messages, so the patchset only provides a fix to
>> make mlx5 load on such function go smooth and avoid loading any interface
>> on that function until we provide the patches for that which is a WIP right
>> now.
>
>Ah, that's probably why I wasn't screaming at it when it was
>posted. I must have understood it then. The commit title is quite
>confusing by iteself - "_Enable_ management PF initialization".
>

Yes the naming is misleading, this not what the name suggests, just a minimal
PF ethernet channel to the BMC, no body is planning to run "raw FW commands
over TCP", you don't need "special PF" to do this :) .. 
In fact any vendor could already be doing this on any normal
PF, so I think you are basing your argument on an irrelevant claim.

>Why is it hard to exclude anything older than CX6 from this condition?
>That part I'm still not understanding.. can you add more color?

CX arch and mlx5 are forward compatible, we try to keep mlx5 device
agnostic and use the CX well-defined feature discovery protocols to boot
the correct set of features.