netdev - Re: [PATCH] usbnet_link_change() fails to call netif_carrier

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]

Message-ID: <m3plmpf5ar.fsf@t19.piap.pl>
Date: Thu, 21 Nov 2024 07:51:24 +0100
From: Krzysztof Hałasa <khalasa@...p.pl>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev <netdev@...r.kernel.org>,  Oliver Neukum <oneukum@...e.com>,
  Andrew Lunn <andrew+netdev@...n.ch>,  "David S. Miller"
 <davem@...emloft.net>,  Eric Dumazet <edumazet@...gle.com>,  Jakub
 Kicinski <kuba@...nel.org>,  Paolo Abeni <pabeni@...hat.com>,
  linux-usb@...r.kernel.org,  linux-kernel@...r.kernel.org,  Jose Ignacio
 Tornos Martinez <jtornosm@...hat.com>,  Ming Lei <ming.lei@...hat.com>
Subject: Re: [PATCH] usbnet_link_change() fails to call netif_carrier_on()

Hi Andrew,
thanks for a looking at this.

Andrew Lunn <andrew@...n.ch> writes:

>> void usbnet_link_change(struct usbnet *dev, bool link, bool need_reset)
>> {
>>       /* update link after link is reseted */
>>       if (link && !need_reset)
>>               netif_carrier_on(dev->net);
>>       else
>>               netif_carrier_off(dev->net);
>>
>>       if (need_reset && link)
>>               usbnet_defer_kevent(dev, EVENT_LINK_RESET);
>>       else
>>               usbnet_defer_kevent(dev, EVENT_LINK_CHANGE);
>> }
>
> This device is using phylink to manage the PHY. phylink will than
> manage the carrier. It assumes it is solely responsible for the
> carrier. So i think your fix is wrong. You probably should be removing
> all code in this driver which touches the carrier.

Ok, I wasn't aware that phylink manages netdev's carrier state.

Then, is the patch wrong just because the asix driver shouldn't use the
function, or is it wrong because the function should work differently
(i.e., the semantics are different)?

Surely the function is broken, isn't it? Calling netif_carrier_off()
on link up event can't be right?

Now the ASIX driver, I'm looking at it for some time now. It consists
of two parts linked together. The ax88172a.c part doesn't use phylink,
while the main asix_devices.c does. So I'm leaving ax88172a.c alone for
now (while it could probably be better ported to the same framework,
i.e., phylink).

The main part uses usbnet.c, which does netif_carrier_{on,off}() in the
above usbnet_link_change(). I guess I can make it use directly
usbnet_defer_kevent() only so it won't be a problem.

Also, usbnet.c calls usbnet_defer_kevent() and thus netif_carrier_off()
in usbnet_probe, removing FLAG_LINK_INTR from asix_devices.c will stop
that.

The last place interacting with carrier status is asix_status(), called
about 8 times a second by usbnet.c intr_complete(). This is independent
of any MDIO traffic. Should I now remove this as well? I guess removing
asix_status would suffice.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa