netdev - 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, 19 Jun 2024 12:07:42 +0000
From: Omer Shpigelman <oshpigelman@...ana.ai>
To: Stephen Hemminger <stephen@...workplumber.org>
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/15/24 03:10, Stephen Hemminger wrote:
> [You don't often get email from stephen@...workplumber.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Thu, 13 Jun 2024 11:22:02 +0300
> Omer Shpigelman <oshpigelman@...ana.ai> wrote:
> 
>> +static int hbl_en_ports_reopen(struct hbl_aux_dev *aux_dev)
>> +{
>> +     struct hbl_en_device *hdev = aux_dev->priv;
>> +     struct hbl_en_port *port;
>> +     int rc = 0, i;
>> +
>> +     for (i = 0; i < hdev->max_num_of_ports; i++) {
>> +             if (!(hdev->ports_mask & BIT(i)))
>> +                     continue;
>> +
>> +             port = &hdev->ports[i];
>> +
>> +             /* It could be that the port was shutdown by 'ip link set down' and there is no need
>> +              * in reopening it.
>> +              * Since we mark the ports as in reset even if they are disabled, we clear the flag
>> +              * here anyway.
>> +              * See hbl_en_ports_stop_prepare() for more info.
>> +              */
>> +             if (!netif_running(port->ndev)) {
>> +                     atomic_set(&port->in_reset, 0);
>> +                     continue;
>> +             }
>> +
> 
> Rather than duplicating network device state in your own flags, it would be better to use
> existing infrastructure. Read Documentation/networking/operstates.rst
> 
> Then you could also get rid of the kludge timer stuff in hbl_en_close().
> 

I think that additional explanation is needed here.
In addition to netdev flows, we also support an internal reset flow
(that's what the in_reset boolean indicates).
Our NIC driver is an extension of the compute driver (they share the same
HW) and a reset flow might be needed due to some compute operation which
is entirely unrelated to the NIC driver. But we must not access the HW
while this reset flow is executed.
Note that this internal reset flow originates from the compute driver and
hence we might have parallel netdev operations that should be blocked
meanwhile.
The internal reset flow has 2 phases - teardown and re-init. This reopen
function is called during the re-init phase to restore the NIC ports, but
only if they were actually opened prior to the reset flow.
Regarding hbl_en_close() - during the port close we need to write to the
HW so due to the explanation above, also there we should wait for an
existing internal reset flow to finish first.
Let me know if that's clear enough and addresses your concerns.