linux-kernel - Re: [PATCH 09/15] net: hbl_en: add habanalabs Ethernet driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 26 Jun 2024 10:13:34 +0000
From: Omer Shpigelman <oshpigelman@...ana.ai>
To: Andrew Lunn <andrew@...n.ch>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "ogabbay@...nel.org" <ogabbay@...nel.org>,
        Zvika Yehudai <zyehudai@...ana.ai>
Subject: Re: [PATCH 09/15] net: hbl_en: add habanalabs Ethernet driver

On 6/23/24 17:46, Andrew Lunn wrote:
>>> But what about when the system is under memory pressure? You say it
>>> allocates memory. What happens if those allocations fail. Does
>>> changing the MTU take me from a working system to a dead system? It is
>>> good practice to not kill a working system under situations like
>>> memory pressure. You try to first allocate the memory you need to
>>> handle the new MTU, and only if successful do you free existing memory
>>> you no longer need. That means if you cannot allocate the needed
>>> memory, you still have the old memory, you can keep the old MTU and
>>> return -ENOMEM, and the system keeps running.
>>>
>>
>> That's a good optimization for these kind of on-the-fly configurations but
>> as you wrote before, changing an MTU value is not a hot path so out of
>> cost-benefit considerations we didn't find it mandatory to optimize this
>> flow.
> 
> I would not call this an optimization. And it is not just about
> changing the MTU. ethtool set_ringparam() is also likely to run into
> this problem, and any other configuration which requires reallocating
> the rings.
> 
> This is something else which comes up every few months on the list,
> and drivers writers who monitor the list will write their drivers that
> why, not 'optimise' it later.
> 

Actually I was wrong, we don't allocate memory in this port reset flow, we
only reset the rings. But I get your point, it makes sense.

>> I get your point but still it will be good if it would be documented
>> somewhere IMHO.
> 
> Kernel documentation is poor, agreed. But kernel policy is also
> somewhat fluid, best practices change, and any developers can
> influence that policy, different subsystems can and do have
> contradictory policy, etc. The mailing list is the best place to learn
> and to take part in this community. You need to be on the list for
> other reasons as well.
> 

Ok, got it.

>> I'm familiar with this logic but I don't understand your point. The point
>> you are making is that setting this Autoneg bit in lp_advertising is
>> pointless? I see other vendors setting it too in case that autoneg was
>> completed.
>> Is that redundant also in their case? because it looks to me that in this
>> case we followed the same logic and conventions other vendors followed.
> 
> Please show us the output from ethtool. Does it look like the example
> i showed? I must admit, i'm more from the embedded world and don't
> have access to high speed interfaces. But the basic concept of
> auto-neg should not change that much.
> 
> 	 Andrew

Here is the output:
$ ethtool eth0
Settings for eth0:
	Supported ports: [ FIBRE	 Backplane ]
	Supported link modes:   100000baseKR4/Full
	                        100000baseSR4/Full
	                        100000baseCR4/Full
	                        100000baseLR4_ER4/Full
	Supported pause frame use: Symmetric
	Supports auto-negotiation: Yes
	Supported FEC modes: Not reported
	Advertised link modes:  100000baseKR4/Full
	                        100000baseSR4/Full
	                        100000baseCR4/Full
	                        100000baseLR4_ER4/Full
	Advertised pause frame use: Symmetric
	Advertised auto-negotiation: Yes
	Advertised FEC modes: Not reported
	Link partner advertised link modes:  Not reported
	Link partner advertised pause frame use: No
	Link partner advertised auto-negotiation: Yes
	Link partner advertised FEC modes: Not reported
	Speed: 100000Mb/s
	Duplex: Full
	Auto-negotiation: on

There are few points to mention:
1. We don't allow to modify the advertised link modes so by definition the
   advertised ones are a copy of the supported ones.
2. Reading the peer advertised link modes is not supported so we don't
   report them (similarly to some other vendors).
3. Our speed is fixed and also cannot be changed so we don't mask
   lp_advertising with advertising to pick the highest speed. We aim for a
   specific speed and hence it's binary - or we'll have a link with that
   specific speed or we won't have a link at all.
4. If we support autoneg and it was completed, we can conclude that also
   our peer supports autoneg and hence we report that.