Re: [PATCH v3] net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

[Andrew Lunn]

>  #define MII_M1011_IMASK			0x12
>  #define MII_M1011_IMASK_INIT		0x6400
>  #define MII_M1011_IMASK_CLEAR		0x0000
> +/* Bits updated by config_intr. The WoL interrupt enable is owned by
> + * set_wol, so the config_intr call from phy_init_hw() on resume does not
> + * silently disarm Wake-on-LAN.
> + */
> +#define MII_M1011_IMASK_CONFIG_MASK	(U16_MAX & ~MII_88E1318S_PHY_CSIER_WOL_EIE)

I still don't like this. The name MII_88E1318S_PHY_CSIER_ suggests
this belongs to the MII_88E1318S_PHY_CSIER register.

> +		err = phy_modify(phydev, MII_M1011_IMASK,
> +				 MII_M1011_IMASK_CONFIG_MASK,
> +				 MII_M1011_IMASK_INIT);
>  	} else {
> -		err = phy_write(phydev, MII_M1011_IMASK,
> -				MII_M1011_IMASK_CLEAR);
> +		err = phy_modify(phydev, MII_M1011_IMASK,
> +				 MII_M1011_IMASK_CONFIG_MASK,
> +				 MII_M1011_IMASK_CLEAR);

But here you apply it to the MII_M1011_IMASK register.

> +	if (!(wol->wolopts & (WAKE_MAGIC | WAKE_PHY))) {
> +		err = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
> +		if (err < 0)
> +			goto error;
> +
> +		/* Disable the WOL interrupt, config_intr leaves it alone */
> +		err = __phy_clear_bits(phydev, MII_M1011_IMASK,
> +				       MII_88E1318S_PHY_CSIER_WOL_EIE);

And here it much more obviously looks wrong. These prefixes are there
to catch dumb typos, and somebody is going to look at this, and think
it is a dumb typo and report it.

Please fix the naming.


    Andrew

---
pw-bot: cr