netdev - Re: [PATCH] alx: add a simple AR816x/AR817x device 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:	Thu, 13 Jun 2013 23:52:22 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Francois Romieu <romieu@...zoreil.com>
Cc:	netdev@...r.kernel.org, mcgrof@...not-panic.com, kvalo@...rom.com,
	adrian.chadd@...il.com
Subject: Re: [PATCH] alx: add a simple AR816x/AR817x device driver

Thanks again :-)

> > +int alx_read_phy_reg(struct alx_hw *hw, u16 reg, u16 *phy_data)
> > +{
> > +	int err;
> > +
> > +	spin_lock(&hw->mdio_lock);
> > +	err = __alx_read_phy_reg(hw, reg, phy_data);
> > +	spin_unlock(&hw->mdio_lock);
> 
> Isn't it possible to remove the phy ops from any irq / napi / tasklet
> context ?

I don't think they're actually called there.

> If you only need it in user / workqueue context you'll be able to trade
> the spinlock for a mutex (and push it in the common core methods ?).

Yes, I suppose I could, but is it worth it? It's held only for a very
short amount of time to get the indirect register access correct. I
don't really see any reason to prefer a mutex here?

Not sure what you mean by "push it in the common core methods"? I
actually suspect that this lock can't ever be contended because the
callers hold the RTNL anyway, but I don't really want to rely on just
that.

> > +		for (i = 0; i < ALX_SLD_MAX_TO; i++) {
> > +			mdelay(1);
> > +			val = alx_read_mem32(hw, ALX_SLD);
> > +			if ((val & ALX_SLD_START) == 0)
> > +				break;
> > +		}
> 
> You may add an helper for the loops above.
> 
> > +		if (i == ALX_SLD_MAX_TO)
> > +			return -EIO;
> > +		loaded_intn = true;
> > +		goto read_mcadr;
> 
> (I don't dislike it but it verges on goto fetishismi :o) )

It is pretty ugly ... I've now rewritten it using two helper functions.

> > +static irqreturn_t alx_intr_handle(struct alx_priv *alx, u32 intr)
> > +{
> > +	struct alx_hw *hw = &alx->hw;
> > +	bool write_int_mask = false;
> > +
> > +	spin_lock(&alx->irq_lock);
> 
> Do yourself a favor: avoid any work in the irq handler.
> 
> Forget the lock. Mask irqs, insert mmiowb and memory barrier, then
> schedule napi if there is any event.
> 
> In the napi handler, enable napi polling then irq.

Hmm, yeah, I'll have to think about that. I don't really care about the
performance all that much ... just want the device to work :-)

Would I really want to rely on NAPI for error interrupts and the like
though? I thought NAPI could potentially be deferred due to budget etc.

johannes

--
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