linux-kernel - Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet 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:   Tue, 12 Sep 2017 18:24:01 +0900
From:   Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>,
        Jassi Brar <jaswinder.singh@...aro.org>
Subject: Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver

Hi Andrew,

On Mon, 11 Sep 2017 14:00:09 +0200 Andrew Lunn <andrew@...n.ch> wrote:

> > > > +static irqreturn_t ave_interrupt(int irq, void *netdev)
> > > > +{
> > > > +	struct net_device *ndev = (struct net_device *)netdev;
> > > > +	struct ave_private *priv = netdev_priv(ndev);
> > > > +	u32 gimr_val, gisr_val;
> > > > +
> > > > +	gimr_val = ave_irq_disable_all(ndev);
> > > > +
> > > > +	/* get interrupt status */
> > > > +	gisr_val = ave_r32(ndev, AVE_GISR);
> > > > +
> > > > +	/* PHY */
> > > > +	if (gisr_val & AVE_GI_PHY) {
> > > > +		ave_w32(ndev, AVE_GISR, AVE_GI_PHY);
> > > > +		if (priv->internal_phy_interrupt)
> > > > +			phy_mac_interrupt(ndev->phydev, ndev->phydev->link);
> > > 
> > > Humm. I don't think this is correct. You are supposed to give it the
> > > new link state, not the old.
> > > 
> > > What does a PHY interrupt mean here? 
> > 
> > In the general case, I think PHY events like changing link state are transmitted
> > to CPU as interrupt via interrupt controller, then PHY driver itself can handle
> > the interrupt.
> > 
> > And in this case, PHY events are transmitted to MAC as one of its interrupt factor,
> > then I thought that MAC driver had to tell the events to PHY.
> 
> Could this be in-band SGMI signalling from the PHY to the MAC? Does
> the documentation give examples of when this interrupt will happen?
> 
>     Andrew

Unfortunately this MAC doesn't support SGMII.
And there aren't any examples of when this interrupt will happen.
This interrupt happens after ave_open() is called and link is established.

However, I found that auto negotiation didn't start when this interrupt wasn't handled.

Although ave_init() calls phy_start_aneg(), it doesn't make sense
because phy driver doesn't start yet.

And according to Florian's comment in ave_init(),

> +	phy_start_interrupts(phydev);
> +
> +	phy_start_aneg(phydev);
>
> No, no, phy_start() would take care of all of that correctly for you,
> you don't have have to do it just there because ave_open() eventually
> calls phy_start() for you, so just drop these two calls.

When moving phy_start_aneg() to ave_open(), the handler doesn't need to call
phy_mac_interrupt() and I can remove it from the handler.

---
Best Regards,
Kunihiko Hayashi