netdev - Re: [PATCH v2 1/5] ethernet: add sun8i-emac 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:	Sat, 30 Jul 2016 09:34:12 +0200
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	Chen-Yu Tsai <wens@...e.org>
Cc:	LABBE Corentin <clabbe.montjoie@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Russell King <linux@...linux.org.uk>,
	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	devicetree <devicetree@...r.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-sunxi <linux-sunxi@...glegroups.com>
Subject: Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

On Sat, Jul 30, 2016 at 09:30:01AM +0800, Chen-Yu Tsai wrote:
> >> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev)
> >> > > +{
> >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> >> > > + u32 reg = 0;
> >> > > +
> >> > > + if (priv->variant == H3_EMAC)
> >> > > +         reg = H3_EPHY_DEFAULT_VALUE;
> >> >
> >> > Why do you need that?
> >> >
> >> For resetting the syscon to the factory default.
> >
> > Yes, but does it matter? Does it have any side effect? Is that
> > register shared with another device?
> >
> > Otherwise, either it won't be used anymore, and you don't care, or you
> > will reload the driver later, and the driver should work whatever
> > state is programmed in there. In both cases, you don't need to reset
> > that value.
> 
> The "default" setting also disables and powers down the internal PHY.
> I think that's a good thing? The naming could be better.

Ah, it might, and that would obviously be the right thing to do. Using
a define for those enable and power down bits would be better though.

> >> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id)
> >> > > +{
> >> > > + struct net_device *ndev = dev_id;
> >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> >> > > + u32 v, u;
> >> > > +
> >> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA);
> >> > > +
> >> > > + /* When this bit is asserted, a frame transmission is completed. */
> >> > > + if (v & BIT(0)) {
> >> > > +         priv->estats.tx_int++;
> >> > > +         writel(0, priv->base + SUN8I_EMAC_INT_EN);
> >> > > +         napi_schedule(&priv->napi);
> >> > > + }
> >> > > +
> >> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */
> >> > > + if (v & BIT(1))
> >> > > +         priv->estats.tx_dma_stop++;
> >> > > +
> >> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor
> >> > > +  * and TX DMA FSM is suspended.
> >> > > + */
> >> > > + if (v & BIT(2))
> >> > > +         priv->estats.tx_dma_ua++;
> >> > > +
> >> > > + if (v & BIT(3))
> >> > > +         netif_dbg(priv, intr, ndev, "Unhandled interrupt TX TIMEOUT\n");
> >> >
> >> > Why do you enable that interrupt if you can't handle it?
> >>
> >> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT)
> >
> > So the bits 9 and 2, respectively, in the interrupt enable register
> > are useless?
> 
> Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it just
> the interrupt state showing an event? IIRC some other hardware blocks have this
> behavior, such as the timer.

That's quite easy to implement, you can do a bitwise and on the status
and enable registers.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)