netdev - Re: [PATCH 4/4] asix: Add a new driver for the AX88172A

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:	Mon, 9 Jul 2012 12:22:11 +0200
From:	Christian Riesch <christian.riesch@...cron.at>
To:	Grant Grundler <grundler@...omium.org>
Cc:	netdev@...r.kernel.org, Oliver Neukum <oneukum@...e.de>,
	Eric Dumazet <edumazet@...gle.com>,
	Allan Chou <allan@...x.com.tw>,
	Mark Lord <kernel@...savvy.com>,
	Ming Lei <tom.leiming@...il.com>,
	Michael Riesch <michael@...sch.at>
Subject: Re: [PATCH 4/4] asix: Add a new driver for the AX88172A

Grant,

On Fri, Jul 6, 2012 at 11:20 PM, Grant Grundler <grundler@...omium.org> wrote:
> On Fri, Jul 6, 2012 at 4:33 AM, Christian Riesch
> <christian.riesch@...cron.at> wrote:
>> The Asix AX88172A is a USB 2.0 Ethernet interface that supports both an
>> internal PHY as well as an external PHY (connected via MII).
>>
>> This patch adds a driver for the AX88172A and provides support for
>> both modes and supports phylib.
>
> Christian,
> In general this looks fine to me...but I wouldn't know about "bus
> identifier life times" (Ben Hutchings comment).
>
> My nit pick is the declaration and of use_embdphy. An alternative
> coding _suggestion_ below.  I'm not substantially altering the
> functionality.
>
> thanks,
> grant

[...]

>> +
>> +struct ax88172a_private {
>> +       int use_embdphy;
>
> Can you move the "int" to the end of the struct?
> It's cleaner to have fields "natively align". ie pointers should start
> at 8 byte alignments when compiled for 64-bit.
>
>> +       struct mii_bus *mdio;
>> +       struct phy_device *phydev;
>> +       char phy_name[20];
>> +       u16 phy_addr;
>> +       u16 oldmode;
>> +};
>> +

[...]

>> +       /* are we using the internal or the external phy? */
>> +       ret = asix_read_cmd(dev, AX_CMD_SW_PHY_STATUS, 0, 0, 1, buf);
>> +       if (ret < 0) {
>> +               dbg("Failed to read software interface selection register: %d",
>> +                   ret);
>> +               goto free;
>> +       }
>> +       dbg("AX_CMD_SW_PHY_STATUS = 0x%02x\n", buf[0]);
>> +       switch ((buf[0] & 0x0c) >> 2) {
>> +       case 0:
>> +               dbg("use internal phy\n");
>> +               priv->use_embdphy = 1;
>> +               break;
>> +       case 1:
>> +               dbg("use external phy\n");
>> +               priv->use_embdphy = 0;
>> +               break;
>> +       default:
>> +               dbg("Interface mode not supported by driver\n");
>> +               goto free;
>> +       }
>
> This switch statement inverts the existing logic. Much simpler code would be:
>     /* buf[0] & 0xc describes phy interface mode */
>     if (buf[0] &  8) {
>          dbg("Interface mode not supported by driver\n");
>          goto free;
>     }
>     priv->use_extphy = (buf[0] & 4) >> 2;
>

Thank your for your comments! I'll change that in the next version!
Regards, Christian
--
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