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