netdev - RE: [PATCH net-next 05/10] lan78xx: Disable USB3 link power state transitions

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: <001b01d798c6$5b4d7b30$11e87190$@pebblebay.com>
Date:   Tue, 24 Aug 2021 09:59:12 +0100
From:   "John Efstathiades" <john.efstathiades@...blebay.com>
To:     "'Jakub Kicinski'" <kuba@...nel.org>, <linux-usb@...r.kernel.org>
Cc:     <UNGLinuxDriver@...rochip.com>, <woojung.huh@...rochip.com>,
        <davem@...emloft.net>, <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next 05/10] lan78xx: Disable USB3 link power state transitions


> > +/* Enabling link power state transitions will reduce power consumption
> > + * when the link is idle. However, this can cause problems with some
> > + * USB3 hubs resulting in erratic packet flow.
> > + */
> > +static bool enable_link_power_states;
> 
> How is the user supposed to control this? Are similar issues not
> addressed at the USB layer? There used to be a "no autosuspend"
> flag that all netdev drivers set..

The change is specific to U1 and U2 transitions initiated by the device
itself and does not affect ability of the device to respond to
host-initiated U1 and U2 transitions.

There is no user access to this. The driver would have to be recompiled to
change the default. 

> 
> Was linux-usb consulted? Adding the list to Cc.
> 
No, they weren't, but the change was discussed with the driver maintainer at
Microchip.

> >  		/* reset MAC */
> >  		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
> >  		if (unlikely(ret < 0))
> > -			return -EIO;
> > +			return ret;
> >  		buf |= MAC_CR_RST_;
> >  		ret = lan78xx_write_reg(dev, MAC_CR, buf);
> >  		if (unlikely(ret < 0))
> > -			return -EIO;
> > +			return ret;
> 
> Please split the ret code changes to a separate, earlier patch.

There are ret code changes in later patches in this set. As a general, rule
should ret code changes be put into their own patch?