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

[Grant Likely]

On Tue, Feb 9, 2010 at 4:00 PM, John Linn <John.Linn at xilinx.com> wrote:
>> -----Original Message-----
>> From: John Williams [mailto:john.williams at petalogix.com]
>> Sent: Tuesday, February 09, 2010 3:30 PM
>> To: John Linn
>> Cc: netdev at vger.kernel.org; linuxppc-dev at ozlabs.org; jgarzik at pobox.com; grant.likely at secretlab.ca;
>> jwboyer at 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:
>>
>> > +static int xemaclite_mdio_read(struct mii_bus *bus, int phy_id, int reg)
>> > +{
>> > +       struct net_local *lp = bus->priv;
>> > +       u32 ctrl_reg;
>> > +       u32 rc;
>> > +
>> > +       mutex_lock(&lp->mdio_mutex);
>> > +
>> > +       if (xemaclite_mdio_wait(lp)) {
>> > +               mutex_unlock(&lp->mdio_mutex);
>> > +               return -ETIMEDOUT;
>> > +       }
>>
>> [snip]
>>
>>
>> > +       if (xemaclite_mdio_wait(lp)) {
>> > +               mutex_unlock(&lp->mdio_mutex);
>> > +               return -ETIMEDOUT;
>> > +       }
>>
>> [snip]
>>
>>
>> > +       dev_dbg(&lp->ndev->dev,
>> > +               "xemaclite_mdio_read(phy_id=%i, reg=%x) == %x\n",
>> > +               phy_id, reg, rc);
>> > +
>> > +       return rc;
>>
>> Can this be better expressed like this:
>>
>> my_func() {
>>   mutex_lock()
>> ..
>>
>>   if(some error) {
>>     rc=-ETIMEDOUT;
>>     goto out_unlock;
>>   }
>>   ...
>>
>>   /* success path */
>>   rc=0;
>> ..
>> out_unlock:
>>   mutex_unlock()
>>   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.