netdev - Re: [PATCH 1/2 net-next] phy: fix the use of PHY_IGNORE

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] [day] [month] [year] [list]

Message-Id: <20130516.150546.1380610594436313255.davem@davemloft.net>
Date:	Thu, 16 May 2013 15:05:46 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	f.fainelli@...il.com
Cc:	netdev@...r.kernel.org, jogo@...nwrt.org,
	thomas.petazzoni@...e-electrons.com, afleming@...escale.com
Subject: Re: [PATCH 1/2 net-next] phy: fix the use of PHY_IGNORE_INTERRUPT

From: "Florian Fainelli" <f.fainelli@...il.com>
Date: Thu, 16 May 2013 18:04:55 +0000

> When a PHY device is registered with the special IRQ value
> PHY_IGNORE_INTERRUPT (-2) it will not properly be handled by the PHY
> library:
> 
> - it continues to poll its register, while we do not want this
>   because such PHY link events or register changes are serviced by an
>   Ethernet MAC interrupt line
> - it will still try to configure PHY interrupts at the PHY level, such
>   interrupts do not exist at the PHY but at the MAC level
> - the state machine only handles PHY_POLL, but should also handle
>   PHY_IGNORE_INTERRUPT similarly
> 
> This patch updates the PHY state machine and initialization paths to
> account for the specific PHY_IGNORE_INTERRUPT. Based on an earlier patch
> by Thomas Petazzoni, and reworked to add the missing bits.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> ---
>  drivers/net/phy/phy.c        | 6 +++---
>  drivers/net/phy/phy_device.c | 5 +++--
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index c14f147..2de4290 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -682,7 +682,7 @@ void phy_stop(struct phy_device *phydev)
>  	if (PHY_HALTED == phydev->state)
>  		goto out_unlock;
>  
> -	if (phydev->irq != PHY_POLL) {
> +	if (phydev->irq >= 0) {

This test, as well as the other two you changed in this file, have
a meaning.  But negative vs. non-negative does not convey what that
meaning actually is.

Add a helper function that says what this is testing, and I would add
a comment to that function referencing all of the special PHY_* values
that are relevant to the test.

> +	/* Disable the interrupt if the PHY doesn't support it
> +	 * but the interrupt is still a valid one */

Multi-line comments in the networking should be formatted:

	/* Like
	 * this.
	 */

Please fix this stuff up and resubmit the entire series, thanks.
--
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