Re: [PATCH net-next 2/3] devlink: Consider other controller while building phys_port_name

From: Jiri Pirko <jiri@resnulli.us>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Parav Pandit <parav@nvidia.com>,
	Parav Pandit <parav@mellanox.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"roid@mellanox.com" <roid@mellanox.com>,
	"saeedm@mellanox.com" <saeedm@mellanox.com>,
	Jiri Pirko <jiri@nvidia.com>
Subject: Re: [PATCH net-next 2/3] devlink: Consider other controller while building phys_port_name
Date: Fri, 4 Sep 2020 10:43:21 +0200	[thread overview]
Message-ID: <20200904084321.GG2997@nanopsycho.orion> (raw)
In-Reply-To: <20200903123123.7e6025ec@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

Thu, Sep 03, 2020 at 09:31:23PM CEST, kuba@kernel.org wrote:
>On Thu, 3 Sep 2020 07:54:39 +0200 Jiri Pirko wrote:
>> Wed, Sep 02, 2020 at 05:23:58PM CEST, kuba@kernel.org wrote:
>> >On Wed, 2 Sep 2020 10:00:11 +0200 Jiri Pirko wrote:  
>> >>>> I didn't quite get the fact that you want to not show controller ID on the local
>> >>>> port, initially.    
>> >>> Mainly to not_break current users.    
>> >> 
>> >> You don't have to take it to the name, unless "external" flag is set.
>> >> 
>> >> But I don't really see the point of showing !external, cause such
>> >> controller number would be always 0. Jakub, why do you think it is
>> >> needed?  
>> >
>> >It may seem reasonable for a smartNIC where there are only two
>> >controllers, and all you really need is that external flag. 
>> >
>> >In a general case when users are trying to figure out the topology
>> >not knowing which controller they are sitting at looks like a serious
>> >limitation.  
>> 
>> I think we misunderstood each other. I never proposed just "external"
>> flag.
>
>Sorry, I was just saying that assuming a single host SmartNIC the
>controller ID is not necessary at all. You never suggested that, I did. 
>Looks like I just confused everyone with that comment :(
>
>Different controller ID for different PFs but the same PCIe link would
>be very wrong. So please clarify - if I have a 2 port smartNIC, with on
>PCIe link to the host, and the embedded controller - what would I see?

Parav?

>
>> What I propose is either:
>> 1) ecnum attribute absent for local
>>    ecnum attribute absent set to 0 for external controller X
>>    ecnum attribute absent set to 1 for external controller Y
>>    ...
>> 
>> or:
>> 2) ecnum attribute absent for local, external flag set to false
>>    ecnum attribute absent set to 0 for external controller X, external flag set to true
>>    ecnum attribute absent set to 1 for external controller Y, external flag set to true
>
>I'm saying that I do want to see the the controller ID for all ports.
>
>So:
>
>3) local:   { "controller ID": x }
>   remote1: { "controller ID": y, "external": true }
>   remote1: { "controller ID": z, "external": true }
>
>We don't have to put the controller ID in the name for local ports, but
>the attribute should be reported. AFAIU name was your main concern, no?

Okay. Sounds fine. Let's put the controller number there for all ports.
ctrlnum X external true
ctrlnum Y external false

if (!external)
	ignore the ctrlnum when generating the name

>
>> >Example - multi-host system and you want to know which controller you
>> >are to run power cycle from the BMC side.
>> >
>> >We won't be able to change that because it'd change the names for you.  