linux-kernel - Re: [PATCH v4 10/14] net: phy: adin: implement PHY subsystem software reset

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: <20190812141954.GP14290@lunn.ch>
Date:   Mon, 12 Aug 2019 16:19:54 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Alexandru Ardelean <alexandru.ardelean@...log.com>
Cc:     netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, davem@...emloft.net,
        robh+dt@...nel.org, mark.rutland@....com, f.fainelli@...il.com,
        hkallweit1@...il.com
Subject: Re: [PATCH v4 10/14] net: phy: adin: implement PHY subsystem
 software reset

> +static int adin_reset(struct phy_device *phydev)
> +{
> +	/* If there is a reset GPIO just exit */
> +	if (!IS_ERR_OR_NULL(phydev->mdio.reset_gpio))
> +		return 0;

I'm not so happy with this.

First off, there are two possible GPIO configurations. The GPIO can be
applied to all PHYs on the MDIO bus. That GPIO is used when the bus is
probed. There can also be a per PHY GPIO, which is what you are
looking at here.

The idea of putting the GPIO handling in the core is that PHYs don't
need to worry about it. How much of a difference does it make if the
PHY is both reset via GPIO and then again in software? How slow is the
software reset? Maybe just unconditionally do the reset, if it is not
too slow.

> +
> +	/* Reset PHY core regs & subsystem regs */
> +	return adin_subsytem_soft_reset(phydev);
> +}
> +
> +static int adin_probe(struct phy_device *phydev)
> +{
> +	return adin_reset(phydev);
> +}

Why did you decide to do this as part of probe, and not use the
.soft_reset member of phy_driver?

> +
>  static struct phy_driver adin_driver[] = {
>  	{
>  		PHY_ID_MATCH_MODEL(PHY_ID_ADIN1200),
>  		.name		= "ADIN1200",
>  		.config_init	= adin_config_init,
> +		.probe		= adin_probe,
>  		.config_aneg	= adin_config_aneg,
>  		.read_status	= adin_read_status,
>  		.ack_interrupt	= adin_phy_ack_intr,
> @@ -461,6 +503,7 @@ static struct phy_driver adin_driver[] = {
>  		PHY_ID_MATCH_MODEL(PHY_ID_ADIN1300),
>  		.name		= "ADIN1300",
>  		.config_init	= adin_config_init,
> +		.probe		= adin_probe,
>  		.config_aneg	= adin_config_aneg,
>  		.read_status	= adin_read_status,
>  		.ack_interrupt	= adin_phy_ack_intr,

Thanks
	Andrew