Re: [PATCH] [V3] net: emaclite: adding MDIO and phy lib support

On Tue, Feb 9, 2010 at 4:00 PM, John Linn [off-list ref] wrote:

quoted

-----Original Message-----
From: John Williams [mailto:john.williams@petalogix.com]
Sent: Tuesday, February 09, 2010 3:30 PM
To: John Linn
Cc: netdev@vger.kernel.org; linuxppc-dev@ozlabs.org; jgarzik@pobox.com; =

grant.likely@secretlab.ca;

quoted

jwboyer@linux.vnet.ibm.com; Sadanand Mutyala
Subject: Re: [PATCH] [V3] net: emaclite: adding MDIO and phy lib support

Hi John,

Sorry If I'm painting bike-sheds here, just one tiny tweak might be in
order to standardise your mutex_unlock exit path:

quoted

+static int xemaclite_mdio_read(struct mii_bus *bus, int phy_id, int r=

eg)

quoted

quoted

+{
+ =A0 =A0 =A0 struct net_local *lp =3D bus->priv;
+ =A0 =A0 =A0 u32 ctrl_reg;
+ =A0 =A0 =A0 u32 rc;
+
+ =A0 =A0 =A0 mutex_lock(&lp->mdio_mutex);
+
+ =A0 =A0 =A0 if (xemaclite_mdio_wait(lp)) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&lp->mdio_mutex);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ETIMEDOUT;
+ =A0 =A0 =A0 }

[snip]

quoted

+ =A0 =A0 =A0 if (xemaclite_mdio_wait(lp)) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&lp->mdio_mutex);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ETIMEDOUT;
+ =A0 =A0 =A0 }

[snip]

quoted

+ =A0 =A0 =A0 dev_dbg(&lp->ndev->dev,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 "xemaclite_mdio_read(phy_id=3D%i, reg=3D=

%x) =3D=3D %x\n",

quoted

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 phy_id, reg, rc);
+
+ =A0 =A0 =A0 return rc;

Can this be better expressed like this:

my_func() {
=A0 mutex_lock()
..

=A0 if(some error) {
=A0 =A0 rc=3D-ETIMEDOUT;
=A0 =A0 goto out_unlock;
=A0 }
=A0 ...

=A0 /* success path */
=A0 rc=3D0;
..
out_unlock:
=A0 mutex_unlock()
=A0 return rc;
}


Is this style still favoured in driver exit paths?

It looks to me like the mutex is not needed in the driver mdio functions =

as there's a mutex in the mdiobus functions already.

Yes, you're correct, but you still need to protect against direct
calls to the read/write routines from within the driver.  But you can
probably use the mdio_lock mutex for this.

g.