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]

Date:	Sat, 22 Jun 2013 12:17:58 +0200
From:	Johannes Stezenbach <js@...21.net>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	Ben Hutchings <ben@...adent.org.uk>, 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 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), I have to unplug the cable to
prevent the wakeup.  Even after "ifconfig eth down"
and "rmmod alx" the link is still up.

  # ethtool -s eth0 wol d
  # ifconfig eth0 down
  # ethtool eth0
  ...
         Wake-on: d
         Link detected: no

But the link LEDs are still on and the other side says
"Link detected: yes" and it can't get some sleep.

I think hw->sleep_ctrl should be initialized to 0 in alx_init_sw(),
but I cannot find why the link stays up after "ifconfig eth0 down".


Thanks,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html