| 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
| ||
|
Message-id: <1232024974.4105.251.camel@dhollis-lnx>
Date: Thu, 15 Jan 2009 08:09:34 -0500
From: David Hollis <dhollis@...ehollis.com>
To: Ben Hutchings <bhutchings@...arflare.com>
Cc: jeff@...zik.org, David Miller <davem@...emloft.net>,
netdev@...r.kernel.org
Subject: Re: [PATCH - REPOST] asix.c - Add support for AX88772A devices -
On Wed, 2009-01-14 at 16:53 +0000, Ben Hutchings wrote:
> > #define AX88772_IPG0_DEFAULT 0x15
> > -#define AX88772_IPG1_DEFAULT 0x0c
> > -#define AX88772_IPG2_DEFAULT 0x12
> > +#define AX88772_IPG1_DEFAULT 0x16
> > +#define AX88772_IPG2_DEFAULT 0x1A
>
> Is this change correct for the plain AX88772 as well?
>
I had to ask that as I well. I have tested on a standard AX88772 and
they do work. I definitely didn't want those devices breaking since I
use them quite regularly as well. Apparently these values help in
certain half-duplex situations.
> > /* GPIO 0 .. 2 toggles */
> > #define AX_GPIO_GPO0EN 0x01 /* GPIO0 Output enable */
> > #define AX_GPIO_GPO_0 0x02 /* GPIO0 Output value */
> > @@ -891,7 +898,11 @@ static int ax88772_link_reset(struct usb
> > if (ecmd.duplex != DUPLEX_FULL)
> > mode &= ~AX_MEDIUM_FD;
> >
> > - devdbg(dev, "ax88772_link_reset() speed: %d duplex: %d setting mode to 0x%04x", ecmd.speed, ecmd.duplex, mode);
> > + devdbg(dev,
> > + "ax88772_link_reset() speed: %d duplex: %d setting mode to 0x%04x",
> > + ecmd.speed,
> > + ecmd.duplex,
> > + mode);
>
> This is just a formatting change so it belongs in a separate patch. And
> if you're going to change it, the format string should be indented
> consistently with the other arguments.
>
Good catch. I missed that line.
> > asix_write_medium_mode(dev, mode);
> >
> > @@ -1018,6 +1029,121 @@ out:
> > return ret;
> > }
> >
> > +static int ax88772a_bind(struct usbnet *dev, struct usb_interface *intf)
> > +{
> > + int ret;
> > + u16 rx_ctl;
> > + struct asix_data *data = (struct asix_data *)&dev->data;
> > + u8 buf[ETH_ALEN];
> > + u32 phyid;
> > +
> > + data->eeprom_len = AX88772_EEPROM_LEN;
> > +
> > + usbnet_get_endpoints(dev,intf);
> > +
> > + /* Power up the embedded PHY */
> > + if ((ret = asix_sw_reset(dev, AX_SWRESET_IPRL)) < 0)
> > + goto out;
>
> Please separate the assignment and the test. I know the existing code
> combines them and you don't need to fix that, but new code should not do
> this.
>
I can work on doing that. I'd really prefer that the whole file be one
way or the other, just so it's all consistent.
> > + /* Select the embedded PHY as the active PHY */
> > + if ((ret = asix_write_cmd(dev, AX_CMD_SW_PHY_SELECT,
> > + AX_PHYSELECT_SSEN | AX_PHYSELECT_PSEL,
> > + 0, 0, NULL)) < 0) {
> > + dbg("Select PHY #1 failed: %d", ret);
> > + goto out;
> > + }
> > +
> > + /* Reload the EEPROM and configure the GPIO pins */
> > + if ((ret = asix_write_gpio(dev,
> > + AX_GPIO_RSE | AX_GPIO_GPO_2 | AX_GPIO_GPO2EN, 5)) < 0)
> > + goto out;
>
> "goto out;" should be indented.
>
Another good catch. Thanks.
> > + /* Set embedded PHY in power down state */
> > + if ((ret = asix_sw_reset(dev, AX_SWRESET_IPRL | AX_SWRESET_IPPD)) < 0)
> > + goto out;
> > +
> > + msleep(10);
> > +
> > + /* Set embedded PHY in power up state */
> > + if ((ret = asix_sw_reset(dev, AX_SWRESET_IPRL)) < 0)
> > + goto out;
> > +
> > + msleep(60);
> > +
> > + /* Set embedded PHY in reset state */
> > + if ((ret = asix_sw_reset(dev, AX_SWRESET_CLEAR)) < 0)
> > + goto out;
> > +
> > + /* Set embedded PHY in operating state */
> > + if ((ret = asix_sw_reset(dev, AX_SWRESET_IPRL)) < 0)
> > + goto out;
> > +
> > + /* Stop RX operation */
> > + if ((ret = asix_write_rx_ctl(dev, 0)) < 0)
> > + goto out;
> > +
> > + /* Get the MAC address */
> > + if ((ret = asix_read_cmd(dev, AX_CMD_READ_NODE_ID,
> > + 0, 0, ETH_ALEN, buf)) < 0) {
> > + dbg("Failed to read MAC address: %d", ret);
> > + goto out;
> > + }
> > + memcpy(dev->net->dev_addr, buf, ETH_ALEN);
> > +
> > + /* Initialize MII structure */
> > + dev->mii.dev = dev->net;
> > + dev->mii.mdio_read = asix_mdio_read;
> > + dev->mii.mdio_write = asix_mdio_write;
> > + dev->mii.phy_id_mask = 0x1f;
> > + dev->mii.reg_num_mask = 0x1f;
> > + dev->net->do_ioctl = asix_ioctl;
> > + dev->mii.phy_id = asix_get_phy_addr(dev);
> > +
> > + phyid = asix_get_phyid(dev);
> > + dbg("PHYID=0x%08x", phyid);
> > +
> > + dev->net->set_multicast_list = asix_set_multicast;
> > + dev->net->ethtool_ops = &ax88772_ethtool_ops;
> > +
> > + asix_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR, BMCR_RESET);
> > + asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE,
> > + ADVERTISE_ALL | ADVERTISE_CSMA);
> > + mii_nway_restart(&dev->mii);
> > +
> > + if ((ret = asix_write_medium_mode(dev, AX88772_MEDIUM_DEFAULT)) < 0)
> > + goto out;
> > +
> > + if ((ret = asix_write_cmd(dev, AX_CMD_WRITE_IPG0,
> > + AX88772_IPG0_DEFAULT | AX88772_IPG1_DEFAULT,
> > + AX88772_IPG2_DEFAULT, 0, NULL)) < 0) {
> > + dbg("Write IPG,IPG1,IPG2 failed: %d", ret);
> > + goto out;
> > + }
> > +
> > + msleep(1);
> > +
> > + /* Set RX_CTL to default values with 2k buffer, and enable cactus */
> > + if ((ret = asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL)) < 0)
> > + goto out;
> > +
> > + rx_ctl = asix_read_rx_ctl(dev);
> > + dbg("RX_CTL is 0x%04x after all initializations", rx_ctl);
> > +
> > + rx_ctl = asix_read_medium_status(dev);
> > + dbg("Medium Status is 0x%04x after all initializations", rx_ctl);
> > +
> > + /* Asix framing packs multiple eth frames into a 2K usb bulk transfer */
> > + if (dev->driver_info->flags & FLAG_FRAMING_AX) {
> > + /* hard_mtu is still the default - the device does not support
> > + jumbo eth frames */
> > + dev->rx_urb_size = 2048;
> > + }
> > + return 0;
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > static struct ethtool_ops ax88178_ethtool_ops = {
> > .get_drvinfo = asix_get_drvinfo,
> > .get_link = asix_get_link,
> > @@ -1339,6 +1465,17 @@ static const struct driver_info ax88772_
> > .tx_fixup = asix_tx_fixup,
> > };
> >
> > +static const struct driver_info ax88772a_info = {
> > + .description = "ASIX AX88772A USB 2.0 Ethernet",
> > + .bind = ax88772a_bind,
> > + .status = asix_status,
> > + .link_reset = ax88772_link_reset,
> > + .reset = ax88772_link_reset,
> > + .flags = FLAG_ETHER | FLAG_FRAMING_AX,
> > + .rx_fixup = asix_rx_fixup,
> > + .tx_fixup = asix_tx_fixup,
> > +};
> > +
> > static const struct driver_info ax88178_info = {
> > .description = "ASIX AX88178 USB 2.0 Ethernet",
> > .bind = ax88178_bind,
> > @@ -1451,6 +1588,10 @@ static const struct usb_device_id produc
> > // Cables-to-Go USB Ethernet Adapter
> > USB_DEVICE(0x0b95, 0x772a),
> > .driver_info = (unsigned long) &ax88772_info,
> > +}, {
> > + // ASIX AX88772A
> > + USB_DEVICE(0x0b95, 0x772a),
> > + .driver_info = (unsigned long) &ax88772a_info,
>
> This is matching the same id as the entry above, so how will it ever be
> used? Do the bind operations distinguish them somehow?
>
Ugh! I didn't notice that either. Unfortunately, I don't have this new
device to be able to test with so I have to rely on the vendors
contribution. I would presume that the Cables-to-Go device needs to be
using ax88772a_info and in the current state, it isn't going to. I'll
check with the contributor for an assist on that.
> Please also run checkpatch.pl and fix the errors it finds.
>
Will do, thanks.
> Ben.
>
> > },
> > { }, // END
> > };
>
--
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
Powered by blists - more mailing lists