Commit 0574f2ed authored by David S. Miller's avatar David S. Miller

Merge tag 'wireless-drivers-for-davem-2019-08-06' of...

Merge tag 'wireless-drivers-for-davem-2019-08-06' of git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers

Kalle Valo says:

====================
wireless-drivers fixes for 5.3

Second set of fixes for 5.3. Lots of iwlwifi fixes have accumulated
which consists most of patches in this pull request. Only most notable
iwlwifi fixes are listed below.

mwifiex

* fix a regression related to WPA1 networks since v5.3-rc1

iwlwifi

* fix use-after-free issues

* fix DMA mapping API usage errors

* fix frame drop occurring due to reorder buffer handling in
  RSS in certain conditions

* fix rate scale locking issues

* disable TX A-MSDU on older NICs as it causes problems and was
  never supposed to be supported

* new PCI IDs

* GEO_TX_POWER_LIMIT API issue that many people were hitting
====================
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents d0d006a4 1f660725
......@@ -776,7 +776,6 @@ struct iwl_rss_config_cmd {
u8 indirection_table[IWL_RSS_INDIRECTION_TABLE_SIZE];
} __packed; /* RSS_CONFIG_CMD_API_S_VER_1 */
#define IWL_MULTI_QUEUE_SYNC_MSG_MAX_SIZE 128
#define IWL_MULTI_QUEUE_SYNC_SENDER_POS 0
#define IWL_MULTI_QUEUE_SYNC_SENDER_MSK 0xf
......@@ -812,10 +811,12 @@ struct iwl_rxq_sync_notification {
*
* @IWL_MVM_RXQ_EMPTY: empty sync notification
* @IWL_MVM_RXQ_NOTIF_DEL_BA: notify RSS queues of delBA
* @IWL_MVM_RXQ_NSSN_SYNC: notify all the RSS queues with the new NSSN
*/
enum iwl_mvm_rxq_notif_type {
IWL_MVM_RXQ_EMPTY,
IWL_MVM_RXQ_NOTIF_DEL_BA,
IWL_MVM_RXQ_NSSN_SYNC,
};
/**
......
......@@ -2438,17 +2438,19 @@ static void iwl_fw_dbg_info_apply(struct iwl_fw_runtime *fwrt,
{
u32 img_name_len = le32_to_cpu(dbg_info->img_name_len);
u32 dbg_cfg_name_len = le32_to_cpu(dbg_info->dbg_cfg_name_len);
const char err_str[] =
"WRT: ext=%d. Invalid %s name length %d, expected %d\n";
if (img_name_len != IWL_FW_INI_MAX_IMG_NAME_LEN) {
IWL_WARN(fwrt, err_str, ext, "image", img_name_len,
IWL_WARN(fwrt,
"WRT: ext=%d. Invalid image name length %d, expected %d\n",
ext, img_name_len,
IWL_FW_INI_MAX_IMG_NAME_LEN);
return;
}
if (dbg_cfg_name_len != IWL_FW_INI_MAX_DBG_CFG_NAME_LEN) {
IWL_WARN(fwrt, err_str, ext, "debug cfg", dbg_cfg_name_len,
IWL_WARN(fwrt,
"WRT: ext=%d. Invalid debug cfg name length %d, expected %d\n",
ext, dbg_cfg_name_len,
IWL_FW_INI_MAX_DBG_CFG_NAME_LEN);
return;
}
......@@ -2775,8 +2777,6 @@ static void _iwl_fw_dbg_apply_point(struct iwl_fw_runtime *fwrt,
struct iwl_ucode_tlv *tlv = iter;
void *ini_tlv = (void *)tlv->data;
u32 type = le32_to_cpu(tlv->type);
const char invalid_ap_str[] =
"WRT: ext=%d. Invalid apply point %d for %s\n";
switch (type) {
case IWL_UCODE_TLV_TYPE_DEBUG_INFO:
......@@ -2786,8 +2786,9 @@ static void _iwl_fw_dbg_apply_point(struct iwl_fw_runtime *fwrt,
struct iwl_fw_ini_allocation_data *buf_alloc = ini_tlv;
if (pnt != IWL_FW_INI_APPLY_EARLY) {
IWL_ERR(fwrt, invalid_ap_str, ext, pnt,
"buffer allocation");
IWL_ERR(fwrt,
"WRT: ext=%d. Invalid apply point %d for buffer allocation\n",
ext, pnt);
goto next;
}
......@@ -2797,8 +2798,9 @@ static void _iwl_fw_dbg_apply_point(struct iwl_fw_runtime *fwrt,
}
case IWL_UCODE_TLV_TYPE_HCMD:
if (pnt < IWL_FW_INI_APPLY_AFTER_ALIVE) {
IWL_ERR(fwrt, invalid_ap_str, ext, pnt,
"host command");
IWL_ERR(fwrt,
"WRT: ext=%d. Invalid apply point %d for host command\n",
ext, pnt);
goto next;
}
iwl_fw_dbg_send_hcmd(fwrt, tlv, ext);
......
......@@ -1640,6 +1640,8 @@ struct iwl_drv *iwl_drv_start(struct iwl_trans *trans)
init_completion(&drv->request_firmware_complete);
INIT_LIST_HEAD(&drv->list);
iwl_load_fw_dbg_tlv(drv->trans->dev, drv->trans);
#ifdef CONFIG_IWLWIFI_DEBUGFS
/* Create the device debugfs entries. */
drv->dbgfs_drv = debugfs_create_dir(dev_name(trans->dev),
......@@ -1660,8 +1662,8 @@ struct iwl_drv *iwl_drv_start(struct iwl_trans *trans)
err_fw:
#ifdef CONFIG_IWLWIFI_DEBUGFS
debugfs_remove_recursive(drv->dbgfs_drv);
iwl_fw_dbg_free(drv->trans);
#endif
iwl_fw_dbg_free(drv->trans);
kfree(drv);
err:
return ERR_PTR(ret);
......
......@@ -755,7 +755,7 @@ static int iwl_mvm_sar_get_ewrd_table(struct iwl_mvm *mvm)
for (i = 0; i < n_profiles; i++) {
/* the tables start at element 3 */
static int pos = 3;
int pos = 3;
/* The EWRD profiles officially go from 2 to 4, but we
* save them in sar_profiles[1-3] (because we don't
......@@ -880,6 +880,22 @@ int iwl_mvm_sar_select_profile(struct iwl_mvm *mvm, int prof_a, int prof_b)
return iwl_mvm_send_cmd_pdu(mvm, REDUCE_TX_POWER_CMD, 0, len, &cmd);
}
static bool iwl_mvm_sar_geo_support(struct iwl_mvm *mvm)
{
/*
* The GEO_TX_POWER_LIMIT command is not supported on earlier
* firmware versions. Unfortunately, we don't have a TLV API
* flag to rely on, so rely on the major version which is in
* the first byte of ucode_ver. This was implemented
* initially on version 38 and then backported to 36, 29 and
* 17.
*/
return IWL_UCODE_SERIAL(mvm->fw->ucode_ver) >= 38 ||
IWL_UCODE_SERIAL(mvm->fw->ucode_ver) == 36 ||
IWL_UCODE_SERIAL(mvm->fw->ucode_ver) == 29 ||
IWL_UCODE_SERIAL(mvm->fw->ucode_ver) == 17;
}
int iwl_mvm_get_sar_geo_profile(struct iwl_mvm *mvm)
{
struct iwl_geo_tx_power_profiles_resp *resp;
......@@ -909,6 +925,9 @@ int iwl_mvm_get_sar_geo_profile(struct iwl_mvm *mvm)
.data = { data },
};
if (!iwl_mvm_sar_geo_support(mvm))
return -EOPNOTSUPP;
ret = iwl_mvm_send_cmd(mvm, &cmd);
if (ret) {
IWL_ERR(mvm, "Failed to get geographic profile info %d\n", ret);
......@@ -934,13 +953,7 @@ static int iwl_mvm_sar_geo_init(struct iwl_mvm *mvm)
int ret, i, j;
u16 cmd_wide_id = WIDE_ID(PHY_OPS_GROUP, GEO_TX_POWER_LIMIT);
/*
* This command is not supported on earlier firmware versions.
* Unfortunately, we don't have a TLV API flag to rely on, so
* rely on the major version which is in the first byte of
* ucode_ver.
*/
if (IWL_UCODE_SERIAL(mvm->fw->ucode_ver) < 41)
if (!iwl_mvm_sar_geo_support(mvm))
return 0;
ret = iwl_mvm_sar_get_wgds_table(mvm);
......
......@@ -207,11 +207,11 @@ static const struct cfg80211_pmsr_capabilities iwl_mvm_pmsr_capa = {
},
};
static int iwl_mvm_mac_set_key(struct ieee80211_hw *hw,
enum set_key_cmd cmd,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
struct ieee80211_key_conf *key);
static int __iwl_mvm_mac_set_key(struct ieee80211_hw *hw,
enum set_key_cmd cmd,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
struct ieee80211_key_conf *key);
void iwl_mvm_ref(struct iwl_mvm *mvm, enum iwl_mvm_ref_type ref_type)
{
......@@ -474,7 +474,19 @@ int iwl_mvm_mac_setup_register(struct iwl_mvm *mvm)
ieee80211_hw_set(hw, SUPPORTS_VHT_EXT_NSS_BW);
ieee80211_hw_set(hw, BUFF_MMPDU_TXQ);
ieee80211_hw_set(hw, STA_MMPDU_TXQ);
ieee80211_hw_set(hw, TX_AMSDU);
/*
* On older devices, enabling TX A-MSDU occasionally leads to
* something getting messed up, the command read from the FIFO
* gets out of sync and isn't a TX command, so that we have an
* assert EDC.
*
* It's not clear where the bug is, but since we didn't used to
* support A-MSDU until moving the mac80211 iTXQs, just leave it
* for older devices. We also don't see this issue on any newer
* devices.
*/
if (mvm->cfg->device_family >= IWL_DEVICE_FAMILY_9000)
ieee80211_hw_set(hw, TX_AMSDU);
ieee80211_hw_set(hw, TX_FRAG_LIST);
if (iwl_mvm_has_tlc_offload(mvm)) {
......@@ -2726,7 +2738,7 @@ static int iwl_mvm_start_ap_ibss(struct ieee80211_hw *hw,
mvmvif->ap_early_keys[i] = NULL;
ret = iwl_mvm_mac_set_key(hw, SET_KEY, vif, NULL, key);
ret = __iwl_mvm_mac_set_key(hw, SET_KEY, vif, NULL, key);
if (ret)
goto out_quota_failed;
}
......@@ -3494,11 +3506,11 @@ static int iwl_mvm_mac_sched_scan_stop(struct ieee80211_hw *hw,
return ret;
}
static int iwl_mvm_mac_set_key(struct ieee80211_hw *hw,
enum set_key_cmd cmd,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
struct ieee80211_key_conf *key)
static int __iwl_mvm_mac_set_key(struct ieee80211_hw *hw,
enum set_key_cmd cmd,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
struct ieee80211_key_conf *key)
{
struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
struct iwl_mvm *mvm = IWL_MAC80211_GET_MVM(hw);
......@@ -3553,8 +3565,6 @@ static int iwl_mvm_mac_set_key(struct ieee80211_hw *hw,
return -EOPNOTSUPP;
}
mutex_lock(&mvm->mutex);
switch (cmd) {
case SET_KEY:
if ((vif->type == NL80211_IFTYPE_ADHOC ||
......@@ -3700,7 +3710,22 @@ static int iwl_mvm_mac_set_key(struct ieee80211_hw *hw,
ret = -EINVAL;
}
return ret;
}
static int iwl_mvm_mac_set_key(struct ieee80211_hw *hw,
enum set_key_cmd cmd,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
struct ieee80211_key_conf *key)
{
struct iwl_mvm *mvm = IWL_MAC80211_GET_MVM(hw);
int ret;
mutex_lock(&mvm->mutex);
ret = __iwl_mvm_mac_set_key(hw, cmd, vif, sta, key);
mutex_unlock(&mvm->mutex);
return ret;
}
......@@ -5041,7 +5066,6 @@ void iwl_mvm_sync_rx_queues_internal(struct iwl_mvm *mvm,
u32 qmask = BIT(mvm->trans->num_rx_queues) - 1;
int ret;
lockdep_assert_held(&mvm->mutex);
if (!iwl_mvm_has_new_rx_api(mvm))
return;
......@@ -5052,13 +5076,15 @@ void iwl_mvm_sync_rx_queues_internal(struct iwl_mvm *mvm,
atomic_set(&mvm->queue_sync_counter,
mvm->trans->num_rx_queues);
ret = iwl_mvm_notify_rx_queue(mvm, qmask, (u8 *)notif, size);
ret = iwl_mvm_notify_rx_queue(mvm, qmask, (u8 *)notif,
size, !notif->sync);
if (ret) {
IWL_ERR(mvm, "Failed to trigger RX queues sync (%d)\n", ret);
goto out;
}
if (notif->sync) {
lockdep_assert_held(&mvm->mutex);
ret = wait_event_timeout(mvm->rx_sync_waitq,
atomic_read(&mvm->queue_sync_counter) == 0 ||
iwl_mvm_is_radio_killed(mvm),
......
......@@ -1664,9 +1664,9 @@ void iwl_mvm_rx_monitor_no_data(struct iwl_mvm *mvm, struct napi_struct *napi,
void iwl_mvm_rx_frame_release(struct iwl_mvm *mvm, struct napi_struct *napi,
struct iwl_rx_cmd_buffer *rxb, int queue);
int iwl_mvm_notify_rx_queue(struct iwl_mvm *mvm, u32 rxq_mask,
const u8 *data, u32 count);
void iwl_mvm_rx_queue_notif(struct iwl_mvm *mvm, struct iwl_rx_cmd_buffer *rxb,
int queue);
const u8 *data, u32 count, bool async);
void iwl_mvm_rx_queue_notif(struct iwl_mvm *mvm, struct napi_struct *napi,
struct iwl_rx_cmd_buffer *rxb, int queue);
void iwl_mvm_rx_tx_cmd(struct iwl_mvm *mvm, struct iwl_rx_cmd_buffer *rxb);
void iwl_mvm_mfu_assert_dump_notif(struct iwl_mvm *mvm,
struct iwl_rx_cmd_buffer *rxb);
......@@ -1813,7 +1813,7 @@ iwl_mvm_vif_dbgfs_clean(struct iwl_mvm *mvm, struct ieee80211_vif *vif)
#endif /* CONFIG_IWLWIFI_DEBUGFS */
/* rate scaling */
int iwl_mvm_send_lq_cmd(struct iwl_mvm *mvm, struct iwl_lq_cmd *lq, bool sync);
int iwl_mvm_send_lq_cmd(struct iwl_mvm *mvm, struct iwl_lq_cmd *lq);
void iwl_mvm_update_frame_stats(struct iwl_mvm *mvm, u32 rate, bool agg);
int rs_pretty_print_rate(char *buf, int bufsz, const u32 rate);
void rs_update_last_rssi(struct iwl_mvm *mvm,
......
......@@ -620,7 +620,7 @@ void iwl_mvm_rx_chub_update_mcc(struct iwl_mvm *mvm,
enum iwl_mcc_source src;
char mcc[3];
struct ieee80211_regdomain *regd;
u32 wgds_tbl_idx;
int wgds_tbl_idx;
lockdep_assert_held(&mvm->mutex);
......
......@@ -1088,7 +1088,7 @@ static void iwl_mvm_rx_mq(struct iwl_op_mode *op_mode,
iwl_mvm_rx_mpdu_mq(mvm, napi, rxb, 0);
else if (unlikely(cmd == WIDE_ID(DATA_PATH_GROUP,
RX_QUEUES_NOTIFICATION)))
iwl_mvm_rx_queue_notif(mvm, rxb, 0);
iwl_mvm_rx_queue_notif(mvm, napi, rxb, 0);
else if (cmd == WIDE_ID(LEGACY_GROUP, FRAME_RELEASE))
iwl_mvm_rx_frame_release(mvm, napi, rxb, 0);
else if (cmd == WIDE_ID(DATA_PATH_GROUP, RX_NO_DATA_NOTIF))
......@@ -1812,7 +1812,7 @@ static void iwl_mvm_rx_mq_rss(struct iwl_op_mode *op_mode,
iwl_mvm_rx_frame_release(mvm, napi, rxb, queue);
else if (unlikely(cmd == WIDE_ID(DATA_PATH_GROUP,
RX_QUEUES_NOTIFICATION)))
iwl_mvm_rx_queue_notif(mvm, rxb, queue);
iwl_mvm_rx_queue_notif(mvm, napi, rxb, queue);
else if (likely(cmd == WIDE_ID(LEGACY_GROUP, REPLY_RX_MPDU_CMD)))
iwl_mvm_rx_mpdu_mq(mvm, napi, rxb, queue);
}
......
This diff is collapsed.
......@@ -4,7 +4,7 @@
* Copyright(c) 2003 - 2014 Intel Corporation. All rights reserved.
* Copyright(c) 2015 Intel Mobile Communications GmbH
* Copyright(c) 2017 Intel Deutschland GmbH
* Copyright(c) 2018 Intel Corporation
* Copyright(c) 2018 - 2019 Intel Corporation
*
* Contact Information:
* Intel Linux Wireless <linuxwifi@intel.com>
......@@ -390,6 +390,7 @@ struct iwl_lq_sta {
s8 last_rssi;
struct rs_rate_stats tx_stats[RS_COLUMN_COUNT][IWL_RATE_COUNT];
struct iwl_mvm *drv;
spinlock_t lock; /* for races in reinit/update table */
} pers;
};
......
......@@ -1684,6 +1684,8 @@ int iwl_mvm_add_sta(struct iwl_mvm *mvm,
*/
if (iwl_mvm_has_tlc_offload(mvm))
iwl_mvm_rs_add_sta(mvm, mvm_sta);
else
spin_lock_init(&mvm_sta->lq_sta.rs_drv.pers.lock);
iwl_mvm_toggle_tx_ant(mvm, &mvm_sta->tx_ant);
......@@ -2421,7 +2423,7 @@ int iwl_mvm_rm_mcast_sta(struct iwl_mvm *mvm, struct ieee80211_vif *vif)
static void iwl_mvm_sync_rxq_del_ba(struct iwl_mvm *mvm, u8 baid)
{
struct iwl_mvm_delba_notif notif = {
struct iwl_mvm_rss_sync_notif notif = {
.metadata.type = IWL_MVM_RXQ_NOTIF_DEL_BA,
.metadata.sync = 1,
.delba.baid = baid,
......@@ -2972,7 +2974,7 @@ int iwl_mvm_sta_tx_agg_oper(struct iwl_mvm *mvm, struct ieee80211_vif *vif,
IWL_DEBUG_HT(mvm, "Tx aggregation enabled on ra = %pM tid = %d\n",
sta->addr, tid);
return iwl_mvm_send_lq_cmd(mvm, &mvmsta->lq_sta.rs_drv.lq, false);
return iwl_mvm_send_lq_cmd(mvm, &mvmsta->lq_sta.rs_drv.lq);
}
static void iwl_mvm_unreserve_agg_queue(struct iwl_mvm *mvm,
......
......@@ -343,9 +343,17 @@ struct iwl_mvm_delba_data {
u32 baid;
} __packed;
struct iwl_mvm_delba_notif {
struct iwl_mvm_nssn_sync_data {
u32 baid;
u32 nssn;
} __packed;
struct iwl_mvm_rss_sync_notif {
struct iwl_mvm_internal_rxq_notif metadata;
struct iwl_mvm_delba_data delba;
union {
struct iwl_mvm_delba_data delba;
struct iwl_mvm_nssn_sync_data nssn_sync;
};
} __packed;
/**
......
......@@ -831,6 +831,7 @@ iwl_mvm_tx_tso_segment(struct sk_buff *skb, unsigned int num_subframes,
unsigned int tcp_payload_len;
unsigned int mss = skb_shinfo(skb)->gso_size;
bool ipv4 = (skb->protocol == htons(ETH_P_IP));
bool qos = ieee80211_is_data_qos(hdr->frame_control);
u16 ip_base_id = ipv4 ? ntohs(ip_hdr(skb)->id) : 0;
skb_shinfo(skb)->gso_size = num_subframes * mss;
......@@ -864,7 +865,7 @@ iwl_mvm_tx_tso_segment(struct sk_buff *skb, unsigned int num_subframes,
if (tcp_payload_len > mss) {
skb_shinfo(tmp)->gso_size = mss;
} else {
if (ieee80211_is_data_qos(hdr->frame_control)) {
if (qos) {
u8 *qc;
if (ipv4)
......
......@@ -653,12 +653,12 @@ int iwl_mvm_reconfig_scd(struct iwl_mvm *mvm, int queue, int fifo, int sta_id,
* this case to clear the state indicating that station creation is in
* progress.
*/
int iwl_mvm_send_lq_cmd(struct iwl_mvm *mvm, struct iwl_lq_cmd *lq, bool sync)
int iwl_mvm_send_lq_cmd(struct iwl_mvm *mvm, struct iwl_lq_cmd *lq)
{
struct iwl_host_cmd cmd = {
.id = LQ_CMD,
.len = { sizeof(struct iwl_lq_cmd), },
.flags = sync ? 0 : CMD_ASYNC,
.flags = CMD_ASYNC,
.data = { lq, },
};
......
......@@ -604,10 +604,13 @@ static const struct pci_device_id iwl_hw_card_ids[] = {
{IWL_PCI_DEVICE(0x2526, 0x40A4, iwl9460_2ac_cfg)},
{IWL_PCI_DEVICE(0x2526, 0x4234, iwl9560_2ac_cfg_soc)},
{IWL_PCI_DEVICE(0x2526, 0x42A4, iwl9462_2ac_cfg_soc)},
{IWL_PCI_DEVICE(0x2526, 0x6010, iwl9260_2ac_160_cfg)},
{IWL_PCI_DEVICE(0x2526, 0x6014, iwl9260_2ac_160_cfg)},
{IWL_PCI_DEVICE(0x2526, 0x8014, iwl9260_2ac_160_cfg)},
{IWL_PCI_DEVICE(0x2526, 0x8010, iwl9260_2ac_160_cfg)},
{IWL_PCI_DEVICE(0x2526, 0xA014, iwl9260_2ac_160_cfg)},
{IWL_PCI_DEVICE(0x2526, 0xE010, iwl9260_2ac_160_cfg)},
{IWL_PCI_DEVICE(0x2526, 0xE014, iwl9260_2ac_160_cfg)},
{IWL_PCI_DEVICE(0x271B, 0x0010, iwl9160_2ac_cfg)},
{IWL_PCI_DEVICE(0x271B, 0x0014, iwl9160_2ac_cfg)},
{IWL_PCI_DEVICE(0x271B, 0x0210, iwl9160_2ac_cfg)},
......
......@@ -435,6 +435,8 @@ static void iwl_pcie_tfd_unmap(struct iwl_trans *trans,
DMA_TO_DEVICE);
}
meta->tbs = 0;
if (trans->cfg->use_tfh) {
struct iwl_tfh_tfd *tfd_fh = (void *)tfd;
......
......@@ -124,6 +124,7 @@ enum {
#define MWIFIEX_MAX_TOTAL_SCAN_TIME (MWIFIEX_TIMER_10S - MWIFIEX_TIMER_1S)
#define WPA_GTK_OUI_OFFSET 2
#define RSN_GTK_OUI_OFFSET 2
#define MWIFIEX_OUI_NOT_PRESENT 0
......
......@@ -181,7 +181,8 @@ mwifiex_is_wpa_oui_present(struct mwifiex_bssdescriptor *bss_desc, u32 cipher)
u8 ret = MWIFIEX_OUI_NOT_PRESENT;
if (has_vendor_hdr(bss_desc->bcn_wpa_ie, WLAN_EID_VENDOR_SPECIFIC)) {
iebody = (struct ie_body *) bss_desc->bcn_wpa_ie->data;
iebody = (struct ie_body *)((u8 *)bss_desc->bcn_wpa_ie->data +
WPA_GTK_OUI_OFFSET);
oui = &mwifiex_wpa_oui[cipher][0];
ret = mwifiex_search_oui_in_ie(iebody, oui);
if (ret)
......
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