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

[Birger Koblitz <mail@birger-koblitz.de>]

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