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

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] [day] [month] [year] [list]

Message-ID: <fa686aa41002091753n36c59e3brd2b2174035e4933d@mail.gmail.com>
Date:	Tue, 9 Feb 2010 18:53:01 -0700
From:	Grant Likely <grant.likely@...retlab.ca>
To:	John Linn <John.Linn@...inx.com>
Cc:	John Williams <john.williams@...alogix.com>,
	netdev@...r.kernel.org, linuxppc-dev@...abs.org, jgarzik@...ox.com,
	jwboyer@...ux.vnet.ibm.com, Sadanand Mutyala <sadanan@...inx.com>
Subject: Re: [PATCH] [V3] net: emaclite: adding MDIO and phy lib support

On Tue, Feb 9, 2010 at 4:00 PM, John Linn <John.Linn@...inx.com> wrote:
>> -----Original Message-----
>> From: John Williams [mailto:john.williams@...alogix.com]
>> Sent: Tuesday, February 09, 2010 3:30 PM
>> To: John Linn
>> Cc: netdev@...r.kernel.org; linuxppc-dev@...abs.org; jgarzik@...ox.com; grant.likely@...retlab.ca;
>> jwboyer@...ux.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.
--
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