Changeset View
Changeset View
Standalone View
Standalone View
sys/dev/mlx5/mlx5_en/mlx5_en_main.c
| Show First 20 Lines • Show All 3,428 Lines • ▼ Show 20 Lines | |||||
| mlx5e_ioctl(struct ifnet *ifp, u_long command, caddr_t data) | mlx5e_ioctl(struct ifnet *ifp, u_long command, caddr_t data) | ||||
| { | { | ||||
| struct mlx5e_priv *priv; | struct mlx5e_priv *priv; | ||||
| struct ifreq *ifr; | struct ifreq *ifr; | ||||
| struct ifdownreason *ifdr; | struct ifdownreason *ifdr; | ||||
| struct ifi2creq i2c; | struct ifi2creq i2c; | ||||
| struct ifrsskey *ifrk; | struct ifrsskey *ifrk; | ||||
| struct ifrsshash *ifrh; | struct ifrsshash *ifrh; | ||||
| struct siocsifcapnv_driver_data *drv_ioctl_data, drv_ioctl_data_d; | |||||
hselasky: I think it would be clever to zero-init drv_ioctl_data_d:
```
struct siocsifcapnv_driver_data… | |||||
Done Inline ActionsIt is not needed, in the sense that driver code which fills the structure, uses it as well, but this is not a critical path. kib: It is not needed, in the sense that driver code which fills the structure, uses it as well, but… | |||||
| int error = 0; | int error = 0; | ||||
| int mask = 0; | int mask = 0; | ||||
| int size_read = 0; | int size_read = 0; | ||||
| int module_status; | int module_status; | ||||
| int module_num; | int module_num; | ||||
| int max_mtu; | int max_mtu; | ||||
| uint8_t read_addr; | uint8_t read_addr; | ||||
| ▲ Show 20 Lines • Show All 62 Lines • ▼ Show 20 Lines | case SIOCDELMULTI: | ||||
| mlx5e_set_rx_mode(ifp); | mlx5e_set_rx_mode(ifp); | ||||
| break; | break; | ||||
| case SIOCSIFMEDIA: | case SIOCSIFMEDIA: | ||||
| case SIOCGIFMEDIA: | case SIOCGIFMEDIA: | ||||
| case SIOCGIFXMEDIA: | case SIOCGIFXMEDIA: | ||||
| ifr = (struct ifreq *)data; | ifr = (struct ifreq *)data; | ||||
| error = ifmedia_ioctl(ifp, ifr, &priv->media, command); | error = ifmedia_ioctl(ifp, ifr, &priv->media, command); | ||||
| break; | break; | ||||
| case SIOCGIFCAPNV: | |||||
| error = 0; | |||||
| break; | |||||
| case SIOCSIFCAP: | case SIOCSIFCAP: | ||||
| ifr = (struct ifreq *)data; | ifr = (struct ifreq *)data; | ||||
| drv_ioctl_data = &drv_ioctl_data_d; | |||||
| drv_ioctl_data->reqcap = ifr->ifr_reqcap; | |||||
| PRIV_LOCK(priv); | PRIV_LOCK(priv); | ||||
| mask = ifr->ifr_reqcap ^ ifp->if_capenable; | drv_ioctl_data->reqcap2 = ifp->if_capabilities2; | ||||
| drv_ioctl_data->nvcap = NULL; | |||||
| goto siocsifcap_driver; | |||||
| case SIOCSIFCAPNV_DRIVER: | |||||
| drv_ioctl_data = (struct siocsifcapnv_driver_data *)data; | |||||
| PRIV_LOCK(priv); | |||||
| siocsifcap_driver: | |||||
| mask = drv_ioctl_data->reqcap ^ ifp->if_capenable; | |||||
Done Inline ActionsDo we need to update "ifp->if_capenable2" here? hselasky: Do we need to update "ifp->if_capenable2" here? | |||||
Done Inline ActionsYes, we do. But this should be done on the cap-by-cap basis, same as it is done for if_capenable. We need to use e.g. mask2 or reuse mask and do similar manipulations. This is why I did not touched this aspect. Or, do you mean something different? kib: Yes, we do. But this should be done on the cap-by-cap basis, same as it is done for… | |||||
| if (mask & IFCAP_TXCSUM) { | if (mask & IFCAP_TXCSUM) { | ||||
| ifp->if_capenable ^= IFCAP_TXCSUM; | ifp->if_capenable ^= IFCAP_TXCSUM; | ||||
| ifp->if_hwassist ^= (CSUM_TCP | CSUM_UDP | CSUM_IP); | ifp->if_hwassist ^= (CSUM_TCP | CSUM_UDP | CSUM_IP); | ||||
| if (IFCAP_TSO4 & ifp->if_capenable && | if (IFCAP_TSO4 & ifp->if_capenable && | ||||
| !(IFCAP_TXCSUM & ifp->if_capenable)) { | !(IFCAP_TXCSUM & ifp->if_capenable)) { | ||||
| mask &= ~IFCAP_TSO4; | mask &= ~IFCAP_TSO4; | ||||
| ifp->if_capenable &= ~IFCAP_TSO4; | ifp->if_capenable &= ~IFCAP_TSO4; | ||||
| ifp->if_hwassist &= ~CSUM_IP_TSO; | ifp->if_hwassist &= ~CSUM_IP_TSO; | ||||
| mlx5_en_err(ifp, | mlx5_en_err(ifp, | ||||
| "tso4 disabled due to -txcsum.\n"); | "tso4 disabled due to -txcsum.\n"); | ||||
Done Inline ActionsIn netpfil/pf/pf_nv.c there are a couple of 'pf_nv_XXX_opt()' functions, which deal with fetching values from an nvlist that may or may not be present. If the name isn't present in the nvlist they return a configurable default value. Perhaps something similar would be useful here. There's at least some usage examples of that in netpfil/pf/pf_syncookies.c. kp: In netpfil/pf/pf_nv.c there are a couple of 'pf_nv_XXX_opt()' functions, which deal with… | |||||
Done Inline ActionsThey might be useful for something more advanced/different. There the info passed is both the presence of the option, and its value. In other words, if the bool name is present, it means that userspace asked driver to toggle; it the name is not present, driver should keep the setting as is. So it is actually quite naturally structures around nvlist_exists/nvlist_get, and no artificial things like "default or current value" are needed. I tried to explain it in the man page description. kib: They might be useful for something more advanced/different. There the info passed is both the… | |||||
Done Inline ActionsYeah, the primary reason for the _opt() variants in the pf code is to deal with new features. That is, when we need an extra field for something we can't count on userspace always supplying it (rephrased, it's not an error for old userspace to not know about new features). This situation is slightly different, and your implementation makes sense here. kp: Yeah, the primary reason for the _opt() variants in the pf code is to deal with new features. | |||||
| } | } | ||||
| } | } | ||||
| if (mask & IFCAP_TXCSUM_IPV6) { | if (mask & IFCAP_TXCSUM_IPV6) { | ||||
| ifp->if_capenable ^= IFCAP_TXCSUM_IPV6; | ifp->if_capenable ^= IFCAP_TXCSUM_IPV6; | ||||
| ifp->if_hwassist ^= (CSUM_UDP_IPV6 | CSUM_TCP_IPV6); | ifp->if_hwassist ^= (CSUM_UDP_IPV6 | CSUM_TCP_IPV6); | ||||
| if (IFCAP_TSO6 & ifp->if_capenable && | if (IFCAP_TSO6 & ifp->if_capenable && | ||||
| !(IFCAP_TXCSUM_IPV6 & ifp->if_capenable)) { | !(IFCAP_TXCSUM_IPV6 & ifp->if_capenable)) { | ||||
| ▲ Show 20 Lines • Show All 974 Lines • ▼ Show 20 Lines | mlx5e_create_ifp(struct mlx5_core_dev *mdev) | ||||
| ifp->if_ioctl = mlx5e_ioctl; | ifp->if_ioctl = mlx5e_ioctl; | ||||
| ifp->if_transmit = mlx5e_xmit; | ifp->if_transmit = mlx5e_xmit; | ||||
| ifp->if_qflush = if_qflush; | ifp->if_qflush = if_qflush; | ||||
| ifp->if_get_counter = mlx5e_get_counter; | ifp->if_get_counter = mlx5e_get_counter; | ||||
| ifp->if_snd.ifq_maxlen = ifqmaxlen; | ifp->if_snd.ifq_maxlen = ifqmaxlen; | ||||
| /* | /* | ||||
| * Set driver features | * Set driver features | ||||
| */ | */ | ||||
| ifp->if_capabilities |= IFCAP_NV; | |||||
| ifp->if_capabilities |= IFCAP_HWCSUM | IFCAP_HWCSUM_IPV6; | ifp->if_capabilities |= IFCAP_HWCSUM | IFCAP_HWCSUM_IPV6; | ||||
| ifp->if_capabilities |= IFCAP_VLAN_MTU | IFCAP_VLAN_HWTAGGING; | ifp->if_capabilities |= IFCAP_VLAN_MTU | IFCAP_VLAN_HWTAGGING; | ||||
| ifp->if_capabilities |= IFCAP_VLAN_HWCSUM | IFCAP_VLAN_HWFILTER; | ifp->if_capabilities |= IFCAP_VLAN_HWCSUM | IFCAP_VLAN_HWFILTER; | ||||
| ifp->if_capabilities |= IFCAP_LINKSTATE | IFCAP_JUMBO_MTU; | ifp->if_capabilities |= IFCAP_LINKSTATE | IFCAP_JUMBO_MTU; | ||||
| ifp->if_capabilities |= IFCAP_LRO; | ifp->if_capabilities |= IFCAP_LRO; | ||||
| ifp->if_capabilities |= IFCAP_TSO | IFCAP_VLAN_HWTSO; | ifp->if_capabilities |= IFCAP_TSO | IFCAP_VLAN_HWTSO; | ||||
| ifp->if_capabilities |= IFCAP_HWSTATS | IFCAP_HWRXTSTMP; | ifp->if_capabilities |= IFCAP_HWSTATS | IFCAP_HWRXTSTMP; | ||||
| ifp->if_capabilities |= IFCAP_MEXTPG; | ifp->if_capabilities |= IFCAP_MEXTPG; | ||||
| ifp->if_capabilities |= IFCAP_TXTLS4 | IFCAP_TXTLS6; | ifp->if_capabilities |= IFCAP_TXTLS4 | IFCAP_TXTLS6; | ||||
| #ifdef RATELIMIT | #ifdef RATELIMIT | ||||
| ifp->if_capabilities |= IFCAP_TXRTLMT | IFCAP_TXTLS_RTLMT; | ifp->if_capabilities |= IFCAP_TXRTLMT | IFCAP_TXTLS_RTLMT; | ||||
| #endif | #endif | ||||
| ifp->if_capabilities |= IFCAP_VXLAN_HWCSUM | IFCAP_VXLAN_HWTSO; | ifp->if_capabilities |= IFCAP_VXLAN_HWCSUM | IFCAP_VXLAN_HWTSO; | ||||
| ifp->if_capabilities2 |= IFCAP2_RXTLS4 | IFCAP2_RXTLS6; | |||||
| ifp->if_snd_tag_alloc = mlx5e_snd_tag_alloc; | ifp->if_snd_tag_alloc = mlx5e_snd_tag_alloc; | ||||
| #ifdef RATELIMIT | #ifdef RATELIMIT | ||||
| ifp->if_ratelimit_query = mlx5e_ratelimit_query; | ifp->if_ratelimit_query = mlx5e_ratelimit_query; | ||||
| #endif | #endif | ||||
| /* set TSO limits so that we don't have to drop TX packets */ | /* set TSO limits so that we don't have to drop TX packets */ | ||||
| ifp->if_hw_tsomax = MLX5E_MAX_TX_PAYLOAD_SIZE - (ETHER_HDR_LEN + ETHER_VLAN_ENCAP_LEN); | ifp->if_hw_tsomax = MLX5E_MAX_TX_PAYLOAD_SIZE - (ETHER_HDR_LEN + ETHER_VLAN_ENCAP_LEN); | ||||
| ifp->if_hw_tsomaxsegcount = MLX5E_MAX_TX_MBUF_FRAGS - 1 /* hdr */; | ifp->if_hw_tsomaxsegcount = MLX5E_MAX_TX_MBUF_FRAGS - 1 /* hdr */; | ||||
| ifp->if_hw_tsomaxsegsize = MLX5E_MAX_TX_MBUF_SIZE; | ifp->if_hw_tsomaxsegsize = MLX5E_MAX_TX_MBUF_SIZE; | ||||
| ifp->if_capenable = ifp->if_capabilities; | ifp->if_capenable = ifp->if_capabilities; | ||||
| ifp->if_capenable2 = ifp->if_capabilities2; | |||||
Done Inline ActionsShould add here: ifp->if_capenable2 = ifp->if_capabilities2; hselasky: Should add here:
```
ifp->if_capenable2 = ifp->if_capabilities2;
```
| |||||
| ifp->if_hwassist = 0; | ifp->if_hwassist = 0; | ||||
| if (ifp->if_capenable & IFCAP_TSO) | if (ifp->if_capenable & IFCAP_TSO) | ||||
| ifp->if_hwassist |= CSUM_TSO; | ifp->if_hwassist |= CSUM_TSO; | ||||
Done Inline ActionsDoes this not need to be behind '#ifdef RATELIMIT` any more? kp: Does this not need to be behind '#ifdef RATELIMIT` any more? | |||||
Done Inline ActionsI wanted to keep the structure layout constant (mlx5e_priv). Ok, the code is ifdef-ed, but the bits are kept in the structure still. kib: I wanted to keep the structure layout constant (mlx5e_priv). Ok, the code is ifdef-ed, but the… | |||||
| if (ifp->if_capenable & IFCAP_TXCSUM) | if (ifp->if_capenable & IFCAP_TXCSUM) | ||||
| ifp->if_hwassist |= (CSUM_TCP | CSUM_UDP | CSUM_IP); | ifp->if_hwassist |= (CSUM_TCP | CSUM_UDP | CSUM_IP); | ||||
| if (ifp->if_capenable & IFCAP_TXCSUM_IPV6) | if (ifp->if_capenable & IFCAP_TXCSUM_IPV6) | ||||
| ifp->if_hwassist |= (CSUM_UDP_IPV6 | CSUM_TCP_IPV6); | ifp->if_hwassist |= (CSUM_UDP_IPV6 | CSUM_TCP_IPV6); | ||||
| if (ifp->if_capabilities & IFCAP_VXLAN_HWCSUM) | if (ifp->if_capabilities & IFCAP_VXLAN_HWCSUM) | ||||
| ifp->if_hwassist |= CSUM_INNER_IP6_UDP | CSUM_INNER_IP6_TCP | | ifp->if_hwassist |= CSUM_INNER_IP6_UDP | CSUM_INNER_IP6_TCP | | ||||
| CSUM_INNER_IP | CSUM_INNER_IP_UDP | CSUM_INNER_IP_TCP | | CSUM_INNER_IP | CSUM_INNER_IP_UDP | CSUM_INNER_IP_TCP | | ||||
| CSUM_ENCAP_VXLAN; | CSUM_ENCAP_VXLAN; | ||||
| ▲ Show 20 Lines • Show All 483 Lines • Show Last 20 Lines | |||||
I think it would be clever to zero-init drv_ioctl_data_d:
struct siocsifcapnv_driver_data *drv_ioctl_data: struct siocsifcapnv_driver_data drv_ioctl_data_d = {};