Re: [PATCH net-next v11 06/15] ax88179_178a: Add HW support for AX179A-based chips

[Birger Koblitz]

Hi Nicolai,

thanks so much for reviewing this patch-series!
On 9/14/26 16:12, Nicolai Buchwitz wrote:
> >  static const struct usb_device_id products[] = {
> >  {
> > +    /* ASIX AX88179A USB 3.2 1000Mbit Ethernet */
> > +    USB_DEVICE_VER(0x0b95, 0x1790, 0, 0x0200),
>
> USB_DEVICE_VER(0x0b95, 0x1790, 0x0200, 0x0200),
>
> Otherwise it would select the wrong callback the existing AX88179 (0x0100)?
Oops, that is a good catch! I took this from manufacturer code and 
probably that range includes also prototypes. The older devices are
handled there with a different driver, however, but indeed we need to be 
sure the 0x0100 versions are not caught. Will fix in the v12.

> Same as with AX88179 above.
Indeed. Will fix.


> > +static const struct net_device_ops ax88179a_netdev_ops = {
> > +    .ndo_open        = usbnet_open,
> > +    .ndo_stop        = usbnet_stop,
> > +    .ndo_start_xmit        = usbnet_start_xmit,
> > +    .ndo_tx_timeout        = usbnet_tx_timeout,
> > +    .ndo_get_stats64    = dev_get_tstats64,
> > +    .ndo_change_mtu        = ax88179_change_mtu,
> > +    .ndo_set_mac_address    = ax88179_set_mac_addr,
> > +    .ndo_validate_addr    = eth_validate_addr,
> > +    .ndo_eth_ioctl        = usbnet_mii_ioctl,
>
> dev->mii.mdio_read is never initialized for ax88179a, so dev- 
>  >mii.mdio_read()
> through usbnet_mii_ioctl() is a NULL function pointer.
>
> Something like:
>
>    static int ax88179a_mii_ioctl(...)
>    {
>        struct ax88179_data *data = netdev2data(net);
>        return phylink_mii_ioctl(data->phylink, ifr, cmd);
>    }
I will remove the entire
+    .ndo_eth_ioctl        = usbnet_mii_ioctl,
line. It should not be necessary.

> > +
> > +        if (pkt_desc & AX179A_RX_PD_VLAN) {
> > +            vlan_tag = pkt_desc >> AX179A_RX_PD_VLAN_SHIFT;
> > +            __vlan_hwaccel_put_tag(ax_skb, htons(ETH_P_8021Q),
> > +                           vlan_tag & VLAN_VID_MASK);
>
> Drop the mask and pass the complete vlan_tag, so priority is preserved?
Indeed. Will fix.

> > +    if (!dev->can_dma_sg && (dev->net->features & NETIF_F_SG) && 
> > skb_linearize(skb))
> > +        return NULL;
>
> Call dev_kfree_skb_any() before returning, so the skb can't leak when 
> skb_linearize() fails.
Will fix.

Birger