netdev - Re: [PATCH v2 1/1] alx: add a simple AR816x/AR817x device 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]

Message-ID: <1371910911.8771.53.camel@deadeye.wl.decadent.org.uk>
Date:	Sat, 22 Jun 2013 15:21:51 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Johannes Stezenbach <js@...21.net>
Cc:	Johannes Berg <johannes@...solutions.net>, netdev@...r.kernel.org,
	mcgrof@...not-panic.com, kvalo@...rom.com, adrian.chadd@...il.com
Subject: Re: [PATCH v2 1/1] alx: add a simple AR816x/AR817x device driver

On Sat, 2013-06-22 at 12:17 +0200, Johannes Stezenbach wrote:
> On Thu, Jun 20, 2013 at 11:40:38PM +0200, Johannes Berg wrote:
> > 
> > On Tue, 2013-06-18 at 02:18 +0100, Ben Hutchings wrote:
> 
> > > > +	/* Workaround for PCI problem when BIOS sets MMRBC incorrectly. */
> > > > +	pci_read_config_word(hw->pdev, PCI_COMMAND, &val16);
> > > > +	if (!(val16 & ALX_PCI_CMD) || (val16 & PCI_COMMAND_INTX_DISABLE)) {
> > > > +		val16 = (val16 | ALX_PCI_CMD) & ~PCI_COMMAND_INTX_DISABLE;
> > > > +		pci_write_config_word(hw->pdev, PCI_COMMAND, val16);
> > > > +	}
> > > [...]
> > > 
> > > I don't understand what this is trying to work around but it looks
> > > extremely dodgy.  The driver already appears to be doing
> > > pci_{save,restore}_state() in suspend/resume which is the right thing to
> > > do.
> > 
> > I have no idea, but ~PCI_COMMAND_INTX_DISABLE seems really just like
> > what we do with PCI_DEV_FLAGS_MSI_INTX_DISABLE_BUG... maybe this was
> > some sort of workaround for that.
> > 
> > Johannes, can you try to remove this chunk of code and see if your
> > device still works? If so I'd just kill it.
> 
> I removed it and did a full power cycle, alx still works.
> Also tried suspend-to-RAM and alx still works.  However,
> since the comment for this code is about BIOS bugs it might
> be that it is required on some other machine?  Not sure
> if it is wise to remove it...
> 
> Another thing: When testing suspend-to-RAM my machine immediately
> woke up.  ethtool said wol is enabled by default:
>         Supports Wake-on: pg
> 	Wake-on: pg
> but even after "ethtool -s eth0 wol d" it still woke up.
> I did a few experiments, it seems that once the alx driver
> is loaded, even when the interface is down, the other end
> of the link (laptop with e1000e) reports the link goes
> into 10Mbit/s speed during suspend (actually it goes
> 1G -> down -> 10M),
[...]

Yes, the driver explicitly selects a lower link speed to save power
(alx_select_powersaving_speed()).  If WAKE_PHY is enabled, I think
alx_pre_suspend() needs to wait for link-up before returning.  Not sure
though.

Ben.

-- 
Ben Hutchings
Klipstein's 4th Law of Prototyping and Production:
                                    A fail-safe circuit will destroy others.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)