netdev - Re: [PATCH net-next 19/19] net: usb: aqc111: Add support for wake on LAN by MAGIC packet

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: <20181006174921.GG6990@lunn.ch>
Date:   Sat, 6 Oct 2018 19:49:21 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Igor Russkikh <Igor.Russkikh@...antia.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com>
Subject: Re: [PATCH net-next 19/19] net: usb: aqc111: Add support for wake on
 LAN by MAGIC packet

> +		if (aqc111_data->dpa) {
> +			aqc111_set_phy_speed(dev, AUTONEG_DISABLE, SPEED_100);

I don't think that works. You should leave AUTONEG on, but only
advertise SPEED_100 and trigger auto-neg. If you force it to 100,
there is no guarantee the peer will figure out what the new link speed
is. I've often seen failed auto-net result in 10/Half. So you will
loose the link, making WoL pointless.

> +static int aqc111_resume(struct usb_interface *intf)
> +{
> +	struct usbnet *dev = usb_get_intfdata(intf);
> +	struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0];
> +	u8 reg8;
> +	u16 reg16;
> +
> +	netif_carrier_off(dev->net);
> +
> +	/* Power up ethernet PHY */
> +	aqc111_data->phy_ops.phy_power = 1;
> +	aqc111_data->phy_ops.low_power = 0;
> +	aqc111_data->phy_ops.wol = 0;
> +	if (aqc111_data->dpa) {
> +		aqc111_read_cmd_nopm(dev, AQ_PHY_POWER, 0, 0, 1, &reg8);
> +		if (reg8 == 0x00) {
> +			reg8 = 0x02;
> +			aqc111_write_cmd_nopm(dev, AQ_PHY_POWER, 0, 0,
> +					      1, &reg8);
> +			msleep(200);
> +		}
> +
> +		aq_mdio_read_cmd(dev, AQ_GLB_STD_CTRL_REG, AQ_PHY_GLOBAL_ADDR,
> +				 2, &reg16);
> +		if (reg16 & AQ_PHY_LOW_POWER_MODE) {
> +			reg16 &= ~AQ_PHY_LOW_POWER_MODE;
> +			aq_mdio_write_cmd(dev, AQ_GLB_STD_CTRL_REG,
> +					  AQ_PHY_GLOBAL_ADDR, 2, &reg16);
> +		}
> +	}
> +
> +	reg8 = 0xFF;
> +	aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_BM_INT_MASK,
> +			      1, 1, &reg8);
> +	/* Configure RX control register => start operation */
> +	reg16 = aqc111_data->rxctl;
> +	reg16 &= ~SFR_RX_CTL_START;
> +	aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_RX_CTL, 2, 2, &reg16);
> +
> +	reg16 |= SFR_RX_CTL_START;
> +	aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_RX_CTL, 2, 2, &reg16);
> +
> +	aqc111_set_phy_speed(dev, aqc111_data->autoneg,
> +			     aqc111_data->advertised_speed);
> +

Should that be conditional on aqc111_data->dpa?

> +struct aqc111_wol_cfg {
> +	u8 hw_addr[6];
> +	u8 flags;
> +	u8 rsvd[283];
> +};

Do you really need these 283 bytes??

> +
> +#define WOL_CFG_SIZE sizeof(struct aqc111_wol_cfg)
> +
>  struct aqc111_data {
>  	u16 rxctl;
>  	u8 rx_checksum;
> @@ -228,6 +238,7 @@ struct aqc111_data {
>  	} fw_ver;
>  	u8 dpa; /*direct PHY access*/
>  	struct aqc111_phy_options phy_ops;
> +	struct aqc111_wol_cfg wol_cfg;

Those 283 bytes make this whole structure bigger...

      Andrew