Commit 17e415cf authored by David S. Miller's avatar David S. Miller

Merge branch '40GbE' of git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue

Tony Nguyen says:

====================
40GbE Intel Wired LAN Driver Updates 2022-04-12

This series contains updates to i40e and ice drivers.

Joe Damato adds TSO support for MPLS packets on i40e and ice drivers. He
also adds tracking and reporting of tx_stopped statistic for i40e.

Nabil S. Alramli adds reporting of tx_restart to ethtool for i40e.

Mateusz adds new device id support for i40e.
====================
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents 8f1c3850 a941d5ee
...@@ -852,6 +852,7 @@ struct i40e_vsi { ...@@ -852,6 +852,7 @@ struct i40e_vsi {
u64 tx_busy; u64 tx_busy;
u64 tx_linearize; u64 tx_linearize;
u64 tx_force_wb; u64 tx_force_wb;
u64 tx_stopped;
u64 rx_buf_failed; u64 rx_buf_failed;
u64 rx_page_failed; u64 rx_page_failed;
u64 rx_page_reuse; u64 rx_page_reuse;
......
...@@ -47,6 +47,7 @@ i40e_status i40e_set_mac_type(struct i40e_hw *hw) ...@@ -47,6 +47,7 @@ i40e_status i40e_set_mac_type(struct i40e_hw *hw)
case I40E_DEV_ID_1G_BASE_T_X722: case I40E_DEV_ID_1G_BASE_T_X722:
case I40E_DEV_ID_10G_BASE_T_X722: case I40E_DEV_ID_10G_BASE_T_X722:
case I40E_DEV_ID_SFP_I_X722: case I40E_DEV_ID_SFP_I_X722:
case I40E_DEV_ID_SFP_X722_A:
hw->mac.type = I40E_MAC_X722; hw->mac.type = I40E_MAC_X722;
break; break;
default: default:
......
...@@ -309,10 +309,11 @@ static void i40e_dbg_dump_vsi_seid(struct i40e_pf *pf, int seid) ...@@ -309,10 +309,11 @@ static void i40e_dbg_dump_vsi_seid(struct i40e_pf *pf, int seid)
tx_ring->stats.bytes, tx_ring->stats.bytes,
tx_ring->tx_stats.restart_queue); tx_ring->tx_stats.restart_queue);
dev_info(&pf->pdev->dev, dev_info(&pf->pdev->dev,
" tx_rings[%i]: tx_stats: tx_busy = %lld, tx_done_old = %lld\n", " tx_rings[%i]: tx_stats: tx_busy = %lld, tx_done_old = %lld, tx_stopped = %lld\n",
i, i,
tx_ring->tx_stats.tx_busy, tx_ring->tx_stats.tx_busy,
tx_ring->tx_stats.tx_done_old); tx_ring->tx_stats.tx_done_old,
tx_ring->tx_stats.tx_stopped);
dev_info(&pf->pdev->dev, dev_info(&pf->pdev->dev,
" tx_rings[%i]: size = %i\n", " tx_rings[%i]: size = %i\n",
i, tx_ring->size); i, tx_ring->size);
......
...@@ -33,6 +33,7 @@ ...@@ -33,6 +33,7 @@
#define I40E_DEV_ID_1G_BASE_T_X722 0x37D1 #define I40E_DEV_ID_1G_BASE_T_X722 0x37D1
#define I40E_DEV_ID_10G_BASE_T_X722 0x37D2 #define I40E_DEV_ID_10G_BASE_T_X722 0x37D2
#define I40E_DEV_ID_SFP_I_X722 0x37D3 #define I40E_DEV_ID_SFP_I_X722 0x37D3
#define I40E_DEV_ID_SFP_X722_A 0x0DDA
#endif /* _I40E_DEVIDS_H_ */ #endif /* _I40E_DEVIDS_H_ */
...@@ -293,12 +293,14 @@ static const struct i40e_stats i40e_gstrings_misc_stats[] = { ...@@ -293,12 +293,14 @@ static const struct i40e_stats i40e_gstrings_misc_stats[] = {
I40E_VSI_STAT("tx_linearize", tx_linearize), I40E_VSI_STAT("tx_linearize", tx_linearize),
I40E_VSI_STAT("tx_force_wb", tx_force_wb), I40E_VSI_STAT("tx_force_wb", tx_force_wb),
I40E_VSI_STAT("tx_busy", tx_busy), I40E_VSI_STAT("tx_busy", tx_busy),
I40E_VSI_STAT("tx_stopped", tx_stopped),
I40E_VSI_STAT("rx_alloc_fail", rx_buf_failed), I40E_VSI_STAT("rx_alloc_fail", rx_buf_failed),
I40E_VSI_STAT("rx_pg_alloc_fail", rx_page_failed), I40E_VSI_STAT("rx_pg_alloc_fail", rx_page_failed),
I40E_VSI_STAT("rx_cache_reuse", rx_page_reuse), I40E_VSI_STAT("rx_cache_reuse", rx_page_reuse),
I40E_VSI_STAT("rx_cache_alloc", rx_page_alloc), I40E_VSI_STAT("rx_cache_alloc", rx_page_alloc),
I40E_VSI_STAT("rx_cache_waive", rx_page_waive), I40E_VSI_STAT("rx_cache_waive", rx_page_waive),
I40E_VSI_STAT("rx_cache_busy", rx_page_busy), I40E_VSI_STAT("rx_cache_busy", rx_page_busy),
I40E_VSI_STAT("tx_restart", tx_restart),
}; };
/* These PF_STATs might look like duplicates of some NETDEV_STATs, /* These PF_STATs might look like duplicates of some NETDEV_STATs,
......
...@@ -77,6 +77,7 @@ static const struct pci_device_id i40e_pci_tbl[] = { ...@@ -77,6 +77,7 @@ static const struct pci_device_id i40e_pci_tbl[] = {
{PCI_VDEVICE(INTEL, I40E_DEV_ID_1G_BASE_T_X722), 0}, {PCI_VDEVICE(INTEL, I40E_DEV_ID_1G_BASE_T_X722), 0},
{PCI_VDEVICE(INTEL, I40E_DEV_ID_10G_BASE_T_X722), 0}, {PCI_VDEVICE(INTEL, I40E_DEV_ID_10G_BASE_T_X722), 0},
{PCI_VDEVICE(INTEL, I40E_DEV_ID_SFP_I_X722), 0}, {PCI_VDEVICE(INTEL, I40E_DEV_ID_SFP_I_X722), 0},
{PCI_VDEVICE(INTEL, I40E_DEV_ID_SFP_X722_A), 0},
{PCI_VDEVICE(INTEL, I40E_DEV_ID_20G_KR2), 0}, {PCI_VDEVICE(INTEL, I40E_DEV_ID_20G_KR2), 0},
{PCI_VDEVICE(INTEL, I40E_DEV_ID_20G_KR2_A), 0}, {PCI_VDEVICE(INTEL, I40E_DEV_ID_20G_KR2_A), 0},
{PCI_VDEVICE(INTEL, I40E_DEV_ID_X710_N3000), 0}, {PCI_VDEVICE(INTEL, I40E_DEV_ID_X710_N3000), 0},
...@@ -785,6 +786,7 @@ static void i40e_update_vsi_stats(struct i40e_vsi *vsi) ...@@ -785,6 +786,7 @@ static void i40e_update_vsi_stats(struct i40e_vsi *vsi)
unsigned int start; unsigned int start;
u64 tx_linearize; u64 tx_linearize;
u64 tx_force_wb; u64 tx_force_wb;
u64 tx_stopped;
u64 rx_p, rx_b; u64 rx_p, rx_b;
u64 tx_p, tx_b; u64 tx_p, tx_b;
u16 q; u16 q;
...@@ -804,6 +806,7 @@ static void i40e_update_vsi_stats(struct i40e_vsi *vsi) ...@@ -804,6 +806,7 @@ static void i40e_update_vsi_stats(struct i40e_vsi *vsi)
rx_b = rx_p = 0; rx_b = rx_p = 0;
tx_b = tx_p = 0; tx_b = tx_p = 0;
tx_restart = tx_busy = tx_linearize = tx_force_wb = 0; tx_restart = tx_busy = tx_linearize = tx_force_wb = 0;
tx_stopped = 0;
rx_page = 0; rx_page = 0;
rx_buf = 0; rx_buf = 0;
rx_reuse = 0; rx_reuse = 0;
...@@ -828,6 +831,7 @@ static void i40e_update_vsi_stats(struct i40e_vsi *vsi) ...@@ -828,6 +831,7 @@ static void i40e_update_vsi_stats(struct i40e_vsi *vsi)
tx_busy += p->tx_stats.tx_busy; tx_busy += p->tx_stats.tx_busy;
tx_linearize += p->tx_stats.tx_linearize; tx_linearize += p->tx_stats.tx_linearize;
tx_force_wb += p->tx_stats.tx_force_wb; tx_force_wb += p->tx_stats.tx_force_wb;
tx_stopped += p->tx_stats.tx_stopped;
/* locate Rx ring */ /* locate Rx ring */
p = READ_ONCE(vsi->rx_rings[q]); p = READ_ONCE(vsi->rx_rings[q]);
...@@ -872,6 +876,7 @@ static void i40e_update_vsi_stats(struct i40e_vsi *vsi) ...@@ -872,6 +876,7 @@ static void i40e_update_vsi_stats(struct i40e_vsi *vsi)
vsi->tx_busy = tx_busy; vsi->tx_busy = tx_busy;
vsi->tx_linearize = tx_linearize; vsi->tx_linearize = tx_linearize;
vsi->tx_force_wb = tx_force_wb; vsi->tx_force_wb = tx_force_wb;
vsi->tx_stopped = tx_stopped;
vsi->rx_page_failed = rx_page; vsi->rx_page_failed = rx_page;
vsi->rx_buf_failed = rx_buf; vsi->rx_buf_failed = rx_buf;
vsi->rx_page_reuse = rx_reuse; vsi->rx_page_reuse = rx_reuse;
...@@ -13436,8 +13441,7 @@ static int i40e_config_netdev(struct i40e_vsi *vsi) ...@@ -13436,8 +13441,7 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
np->vsi = vsi; np->vsi = vsi;
hw_enc_features = NETIF_F_SG | hw_enc_features = NETIF_F_SG |
NETIF_F_IP_CSUM | NETIF_F_HW_CSUM |
NETIF_F_IPV6_CSUM |
NETIF_F_HIGHDMA | NETIF_F_HIGHDMA |
NETIF_F_SOFT_FEATURES | NETIF_F_SOFT_FEATURES |
NETIF_F_TSO | NETIF_F_TSO |
...@@ -13468,6 +13472,23 @@ static int i40e_config_netdev(struct i40e_vsi *vsi) ...@@ -13468,6 +13472,23 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
/* record features VLANs can make use of */ /* record features VLANs can make use of */
netdev->vlan_features |= hw_enc_features | NETIF_F_TSO_MANGLEID; netdev->vlan_features |= hw_enc_features | NETIF_F_TSO_MANGLEID;
#define I40E_GSO_PARTIAL_FEATURES (NETIF_F_GSO_GRE | \
NETIF_F_GSO_GRE_CSUM | \
NETIF_F_GSO_IPXIP4 | \
NETIF_F_GSO_IPXIP6 | \
NETIF_F_GSO_UDP_TUNNEL | \
NETIF_F_GSO_UDP_TUNNEL_CSUM)
netdev->gso_partial_features = I40E_GSO_PARTIAL_FEATURES;
netdev->features |= NETIF_F_GSO_PARTIAL |
I40E_GSO_PARTIAL_FEATURES;
netdev->mpls_features |= NETIF_F_SG;
netdev->mpls_features |= NETIF_F_HW_CSUM;
netdev->mpls_features |= NETIF_F_TSO;
netdev->mpls_features |= NETIF_F_TSO6;
netdev->mpls_features |= I40E_GSO_PARTIAL_FEATURES;
/* enable macvlan offloads */ /* enable macvlan offloads */
netdev->hw_features |= NETIF_F_HW_L2FW_DOFFLOAD; netdev->hw_features |= NETIF_F_HW_L2FW_DOFFLOAD;
......
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
#include <linux/prefetch.h> #include <linux/prefetch.h>
#include <linux/bpf_trace.h> #include <linux/bpf_trace.h>
#include <net/mpls.h>
#include <net/xdp.h> #include <net/xdp.h>
#include "i40e.h" #include "i40e.h"
#include "i40e_trace.h" #include "i40e_trace.h"
...@@ -3015,6 +3016,7 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len, ...@@ -3015,6 +3016,7 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len,
{ {
struct sk_buff *skb = first->skb; struct sk_buff *skb = first->skb;
u64 cd_cmd, cd_tso_len, cd_mss; u64 cd_cmd, cd_tso_len, cd_mss;
__be16 protocol;
union { union {
struct iphdr *v4; struct iphdr *v4;
struct ipv6hdr *v6; struct ipv6hdr *v6;
...@@ -3026,7 +3028,7 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len, ...@@ -3026,7 +3028,7 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len,
unsigned char *hdr; unsigned char *hdr;
} l4; } l4;
u32 paylen, l4_offset; u32 paylen, l4_offset;
u16 gso_segs, gso_size; u16 gso_size;
int err; int err;
if (skb->ip_summed != CHECKSUM_PARTIAL) if (skb->ip_summed != CHECKSUM_PARTIAL)
...@@ -3039,15 +3041,23 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len, ...@@ -3039,15 +3041,23 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len,
if (err < 0) if (err < 0)
return err; return err;
ip.hdr = skb_network_header(skb); protocol = vlan_get_protocol(skb);
l4.hdr = skb_transport_header(skb);
if (eth_p_mpls(protocol))
ip.hdr = skb_inner_network_header(skb);
else
ip.hdr = skb_network_header(skb);
l4.hdr = skb_checksum_start(skb);
/* initialize outer IP header fields */ /* initialize outer IP header fields */
if (ip.v4->version == 4) { if (ip.v4->version == 4) {
ip.v4->tot_len = 0; ip.v4->tot_len = 0;
ip.v4->check = 0; ip.v4->check = 0;
first->tx_flags |= I40E_TX_FLAGS_TSO;
} else { } else {
ip.v6->payload_len = 0; ip.v6->payload_len = 0;
first->tx_flags |= I40E_TX_FLAGS_TSO;
} }
if (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE | if (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE |
...@@ -3100,10 +3110,9 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len, ...@@ -3100,10 +3110,9 @@ static int i40e_tso(struct i40e_tx_buffer *first, u8 *hdr_len,
/* pull values out of skb_shinfo */ /* pull values out of skb_shinfo */
gso_size = skb_shinfo(skb)->gso_size; gso_size = skb_shinfo(skb)->gso_size;
gso_segs = skb_shinfo(skb)->gso_segs;
/* update GSO size and bytecount with header size */ /* update GSO size and bytecount with header size */
first->gso_segs = gso_segs; first->gso_segs = skb_shinfo(skb)->gso_segs;
first->bytecount += (first->gso_segs - 1) * *hdr_len; first->bytecount += (first->gso_segs - 1) * *hdr_len;
/* find the field values */ /* find the field values */
...@@ -3187,13 +3196,27 @@ static int i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags, ...@@ -3187,13 +3196,27 @@ static int i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
unsigned char *exthdr; unsigned char *exthdr;
u32 offset, cmd = 0; u32 offset, cmd = 0;
__be16 frag_off; __be16 frag_off;
__be16 protocol;
u8 l4_proto = 0; u8 l4_proto = 0;
if (skb->ip_summed != CHECKSUM_PARTIAL) if (skb->ip_summed != CHECKSUM_PARTIAL)
return 0; return 0;
ip.hdr = skb_network_header(skb); protocol = vlan_get_protocol(skb);
l4.hdr = skb_transport_header(skb);
if (eth_p_mpls(protocol))
ip.hdr = skb_inner_network_header(skb);
else
ip.hdr = skb_network_header(skb);
l4.hdr = skb_checksum_start(skb);
/* set the tx_flags to indicate the IP protocol type. this is
* required so that checksum header computation below is accurate.
*/
if (ip.v4->version == 4)
*tx_flags |= I40E_TX_FLAGS_IPV4;
else
*tx_flags |= I40E_TX_FLAGS_IPV6;
/* compute outer L2 header size */ /* compute outer L2 header size */
offset = ((ip.hdr - skb->data) / 2) << I40E_TX_DESC_LENGTH_MACLEN_SHIFT; offset = ((ip.hdr - skb->data) / 2) << I40E_TX_DESC_LENGTH_MACLEN_SHIFT;
...@@ -3373,6 +3396,8 @@ int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size) ...@@ -3373,6 +3396,8 @@ int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size)
/* Memory barrier before checking head and tail */ /* Memory barrier before checking head and tail */
smp_mb(); smp_mb();
++tx_ring->tx_stats.tx_stopped;
/* Check again in a case another CPU has just made room available. */ /* Check again in a case another CPU has just made room available. */
if (likely(I40E_DESC_UNUSED(tx_ring) < size)) if (likely(I40E_DESC_UNUSED(tx_ring) < size))
return -EBUSY; return -EBUSY;
...@@ -3749,7 +3774,6 @@ static netdev_tx_t i40e_xmit_frame_ring(struct sk_buff *skb, ...@@ -3749,7 +3774,6 @@ static netdev_tx_t i40e_xmit_frame_ring(struct sk_buff *skb,
struct i40e_tx_buffer *first; struct i40e_tx_buffer *first;
u32 td_offset = 0; u32 td_offset = 0;
u32 tx_flags = 0; u32 tx_flags = 0;
__be16 protocol;
u32 td_cmd = 0; u32 td_cmd = 0;
u8 hdr_len = 0; u8 hdr_len = 0;
int tso, count; int tso, count;
...@@ -3791,15 +3815,6 @@ static netdev_tx_t i40e_xmit_frame_ring(struct sk_buff *skb, ...@@ -3791,15 +3815,6 @@ static netdev_tx_t i40e_xmit_frame_ring(struct sk_buff *skb,
if (i40e_tx_prepare_vlan_flags(skb, tx_ring, &tx_flags)) if (i40e_tx_prepare_vlan_flags(skb, tx_ring, &tx_flags))
goto out_drop; goto out_drop;
/* obtain protocol of skb */
protocol = vlan_get_protocol(skb);
/* setup IPv4/IPv6 offloads */
if (protocol == htons(ETH_P_IP))
tx_flags |= I40E_TX_FLAGS_IPV4;
else if (protocol == htons(ETH_P_IPV6))
tx_flags |= I40E_TX_FLAGS_IPV6;
tso = i40e_tso(first, &hdr_len, &cd_type_cmd_tso_mss); tso = i40e_tso(first, &hdr_len, &cd_type_cmd_tso_mss);
if (tso < 0) if (tso < 0)
......
...@@ -290,6 +290,7 @@ struct i40e_tx_queue_stats { ...@@ -290,6 +290,7 @@ struct i40e_tx_queue_stats {
u64 tx_done_old; u64 tx_done_old;
u64 tx_linearize; u64 tx_linearize;
u64 tx_force_wb; u64 tx_force_wb;
u64 tx_stopped;
int prev_pkt_ctr; int prev_pkt_ctr;
}; };
......
...@@ -3329,7 +3329,9 @@ static void ice_set_netdev_features(struct net_device *netdev) ...@@ -3329,7 +3329,9 @@ static void ice_set_netdev_features(struct net_device *netdev)
vlano_features | tso_features; vlano_features | tso_features;
/* add support for HW_CSUM on packets with MPLS header */ /* add support for HW_CSUM on packets with MPLS header */
netdev->mpls_features = NETIF_F_HW_CSUM; netdev->mpls_features = NETIF_F_HW_CSUM |
NETIF_F_TSO |
NETIF_F_TSO6;
/* enable features */ /* enable features */
netdev->features |= netdev->hw_features; netdev->features |= netdev->hw_features;
......
...@@ -8,6 +8,7 @@ ...@@ -8,6 +8,7 @@
#include <linux/prefetch.h> #include <linux/prefetch.h>
#include <linux/bpf_trace.h> #include <linux/bpf_trace.h>
#include <net/dsfield.h> #include <net/dsfield.h>
#include <net/mpls.h>
#include <net/xdp.h> #include <net/xdp.h>
#include "ice_txrx_lib.h" #include "ice_txrx_lib.h"
#include "ice_lib.h" #include "ice_lib.h"
...@@ -1748,18 +1749,24 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off) ...@@ -1748,18 +1749,24 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
if (skb->ip_summed != CHECKSUM_PARTIAL) if (skb->ip_summed != CHECKSUM_PARTIAL)
return 0; return 0;
ip.hdr = skb_network_header(skb); protocol = vlan_get_protocol(skb);
l4.hdr = skb_transport_header(skb);
if (eth_p_mpls(protocol))
ip.hdr = skb_inner_network_header(skb);
else
ip.hdr = skb_network_header(skb);
l4.hdr = skb_checksum_start(skb);
/* compute outer L2 header size */ /* compute outer L2 header size */
l2_len = ip.hdr - skb->data; l2_len = ip.hdr - skb->data;
offset = (l2_len / 2) << ICE_TX_DESC_LEN_MACLEN_S; offset = (l2_len / 2) << ICE_TX_DESC_LEN_MACLEN_S;
protocol = vlan_get_protocol(skb); /* set the tx_flags to indicate the IP protocol type. this is
* required so that checksum header computation below is accurate.
if (protocol == htons(ETH_P_IP)) */
if (ip.v4->version == 4)
first->tx_flags |= ICE_TX_FLAGS_IPV4; first->tx_flags |= ICE_TX_FLAGS_IPV4;
else if (protocol == htons(ETH_P_IPV6)) else if (ip.v6->version == 6)
first->tx_flags |= ICE_TX_FLAGS_IPV6; first->tx_flags |= ICE_TX_FLAGS_IPV6;
if (skb->encapsulation) { if (skb->encapsulation) {
...@@ -1957,6 +1964,7 @@ int ice_tso(struct ice_tx_buf *first, struct ice_tx_offload_params *off) ...@@ -1957,6 +1964,7 @@ int ice_tso(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
unsigned char *hdr; unsigned char *hdr;
} l4; } l4;
u64 cd_mss, cd_tso_len; u64 cd_mss, cd_tso_len;
__be16 protocol;
u32 paylen; u32 paylen;
u8 l4_start; u8 l4_start;
int err; int err;
...@@ -1972,8 +1980,13 @@ int ice_tso(struct ice_tx_buf *first, struct ice_tx_offload_params *off) ...@@ -1972,8 +1980,13 @@ int ice_tso(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
return err; return err;
/* cppcheck-suppress unreadVariable */ /* cppcheck-suppress unreadVariable */
ip.hdr = skb_network_header(skb); protocol = vlan_get_protocol(skb);
l4.hdr = skb_transport_header(skb);
if (eth_p_mpls(protocol))
ip.hdr = skb_inner_network_header(skb);
else
ip.hdr = skb_network_header(skb);
l4.hdr = skb_checksum_start(skb);
/* initialize outer IP header fields */ /* initialize outer IP header fields */
if (ip.v4->version == 4) { if (ip.v4->version == 4) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment