Commit 7c2cde2e authored by Stanislaw Gruszka's avatar Stanislaw Gruszka

iwlegacy: partial rxon context cleanup

Signed-off-by: default avatarStanislaw Gruszka <sgruszka@redhat.com>
parent dcae1c64
......@@ -342,7 +342,7 @@ void il3945_rs_rate_init(struct il_priv *il, struct ieee80211_sta *sta, u8 sta_i
int i;
D_INFO("enter\n");
if (sta_id == il->contexts[IL_RXON_CTX_BSS].bcast_sta_id)
if (sta_id == il->ctx.bcast_sta_id)
goto out;
psta = (struct il3945_sta_priv *) sta->drv_priv;
......@@ -936,7 +936,7 @@ void il3945_rate_scale_init(struct ieee80211_hw *hw, s32 sta_id)
rcu_read_lock();
sta = ieee80211_find_sta(il->contexts[IL_RXON_CTX_BSS].vif,
sta = ieee80211_find_sta(il->ctx.vif,
il->stations[sta_id].sta.sta.addr);
if (!sta) {
D_RATE("Unable to find station to initialize rate scaling.\n");
......@@ -953,7 +953,7 @@ void il3945_rate_scale_init(struct ieee80211_hw *hw, s32 sta_id)
switch (il->band) {
case IEEE80211_BAND_2GHZ:
/* TODO: this always does G, not a regression */
if (il->contexts[IL_RXON_CTX_BSS].active.flags &
if (il->ctx.active.flags &
RXON_FLG_TGG_PROTECT_MSK) {
rs_sta->tgg = 1;
rs_sta->expected_tpt = il3945_expected_tpt_g_prot;
......
......@@ -253,7 +253,7 @@ int il3945_rs_next_rate(struct il_priv *il, int rate)
break;
case IEEE80211_BAND_2GHZ:
if (!(il->_3945.sta_supp_rates & IL_OFDM_RATES_MASK) &&
il_is_associated(il, IL_RXON_CTX_BSS)) {
il_is_associated(il)) {
if (rate == IL_RATE_11M_INDEX)
next_rate = IL_RATE_5M_INDEX;
}
......@@ -1374,7 +1374,7 @@ static int il3945_send_tx_power(struct il_priv *il)
int rate_idx, i;
const struct il_channel_info *ch_info = NULL;
struct il3945_txpowertable_cmd txpower = {
.channel = il->contexts[IL_RXON_CTX_BSS].active.channel,
.channel = il->ctx.active.channel,
};
u16 chan;
......@@ -1382,7 +1382,7 @@ static int il3945_send_tx_power(struct il_priv *il)
"TX Power requested while scanning!\n"))
return -EAGAIN;
chan = le16_to_cpu(il->contexts[IL_RXON_CTX_BSS].active.channel);
chan = le16_to_cpu(il->ctx.active.channel);
txpower.band = (il->band == IEEE80211_BAND_5GHZ) ? 0 : 1;
ch_info = il_get_channel_info(il, il->band, chan);
......@@ -1734,9 +1734,9 @@ int il3945_commit_rxon(struct il_priv *il, struct il_rxon_context *ctx)
* il3945_rxon_assoc_cmd which is used to reconfigure filter
* and other flags for the current radio configuration. */
if (!il_full_rxon_required(il,
&il->contexts[IL_RXON_CTX_BSS])) {
&il->ctx)) {
rc = il_send_rxon_assoc(il,
&il->contexts[IL_RXON_CTX_BSS]);
&il->ctx);
if (rc) {
IL_ERR("Error setting RXON_ASSOC "
"configuration (%d).\n", rc);
......@@ -1756,7 +1756,7 @@ int il3945_commit_rxon(struct il_priv *il, struct il_rxon_context *ctx)
* an RXON_ASSOC and the new config wants the associated mask enabled,
* we must clear the associated from the active configuration
* before we apply the new config */
if (il_is_associated(il, IL_RXON_CTX_BSS) && new_assoc) {
if (il_is_associated(il) && new_assoc) {
D_INFO("Toggling associated bit on current RXON\n");
active_rxon->filter_flags &= ~RXON_FILTER_ASSOC_MSK;
......@@ -1768,7 +1768,7 @@ int il3945_commit_rxon(struct il_priv *il, struct il_rxon_context *ctx)
active_rxon->reserved5 = 0;
rc = il_send_cmd_pdu(il, REPLY_RXON,
sizeof(struct il3945_rxon_cmd),
&il->contexts[IL_RXON_CTX_BSS].active);
&il->ctx.active);
/* If the mask clearing failed then we set
* active_rxon back to what it was previously */
......@@ -1779,9 +1779,9 @@ int il3945_commit_rxon(struct il_priv *il, struct il_rxon_context *ctx)
return rc;
}
il_clear_ucode_stations(il,
&il->contexts[IL_RXON_CTX_BSS]);
&il->ctx);
il_restore_stations(il,
&il->contexts[IL_RXON_CTX_BSS]);
&il->ctx);
}
D_INFO("Sending RXON\n"
......@@ -1814,9 +1814,9 @@ int il3945_commit_rxon(struct il_priv *il, struct il_rxon_context *ctx)
if (!new_assoc) {
il_clear_ucode_stations(il,
&il->contexts[IL_RXON_CTX_BSS]);
&il->ctx);
il_restore_stations(il,
&il->contexts[IL_RXON_CTX_BSS]);
&il->ctx);
}
/* If we issue a new RXON command which required a tune then we must
......@@ -2252,7 +2252,7 @@ static u16 il3945_build_addsta_hcmd(const struct il_addsta_cmd *cmd,
static int il3945_add_bssid_station(struct il_priv *il,
const u8 *addr, u8 *sta_id_r)
{
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
int ret;
u8 sta_id;
unsigned long flags;
......@@ -2346,7 +2346,7 @@ int il3945_init_hw_rate_table(struct il_priv *il)
* 1M CCK rates */
if (!(il->_3945.sta_supp_rates & IL_OFDM_RATES_MASK) &&
il_is_associated(il, IL_RXON_CTX_BSS)) {
il_is_associated(il)) {
index = IL_FIRST_CCK_RATE;
for (i = IL_RATE_6M_INDEX_TABLE;
......@@ -2401,7 +2401,7 @@ int il3945_hw_set_hw_params(struct il_priv *il)
il->hw_params.max_rxq_size = RX_QUEUE_SIZE;
il->hw_params.max_rxq_log = RX_QUEUE_SIZE_LOG;
il->hw_params.max_stations = IL3945_STATION_COUNT;
il->contexts[IL_RXON_CTX_BSS].bcast_sta_id = IL3945_BROADCAST_ID;
il->ctx.bcast_sta_id = IL3945_BROADCAST_ID;
il->sta_key_max_num = STA_KEY_MAX_NUM;
......@@ -2422,7 +2422,7 @@ unsigned int il3945_hw_get_beacon_cmd(struct il_priv *il,
memset(tx_beacon_cmd, 0, sizeof(*tx_beacon_cmd));
tx_beacon_cmd->tx.sta_id =
il->contexts[IL_RXON_CTX_BSS].bcast_sta_id;
il->ctx.bcast_sta_id;
tx_beacon_cmd->tx.stop_time.life_time = TX_CMD_LIFE_TIME_INFINITE;
frame_size = il3945_fill_beacon_frame(il,
......
......@@ -820,7 +820,7 @@ void il4965_chain_noise_calibration(struct il_priv *il, void *stat_resp)
unsigned long flags;
struct statistics_rx_non_phy *rx_info;
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
if (il->disable_chain_noise_cal)
return;
......
......@@ -782,7 +782,7 @@ int il4965_request_scan(struct il_priv *il, struct ieee80211_vif *vif)
.flags = CMD_SIZE_HUGE,
};
struct il_scan_cmd *scan;
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
u32 rate_flags = 0;
u16 cmd_len;
u16 rx_chain = 0;
......@@ -866,7 +866,7 @@ int il4965_request_scan(struct il_priv *il, struct ieee80211_vif *vif)
case IEEE80211_BAND_2GHZ:
scan->flags = RXON_FLG_BAND_24G_MSK | RXON_FLG_AUTO_DETECT_MSK;
chan_mod = le32_to_cpu(
il->contexts[IL_RXON_CTX_BSS].active.flags &
il->ctx.active.flags &
RXON_FLG_CHANNEL_MODE_MSK)
>> RXON_FLG_CHANNEL_MODE_POS;
if (chan_mod == CHANNEL_MODE_PURE_40) {
......
......@@ -277,7 +277,7 @@ int il4965_tx_skb(struct il_priv *il, struct sk_buff *skb)
struct il_device_cmd *out_cmd;
struct il_cmd_meta *out_meta;
struct il_tx_cmd *tx_cmd;
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
int txq_id;
dma_addr_t phys_addr;
dma_addr_t txcmd_phys;
......@@ -1041,7 +1041,7 @@ int il4965_txq_check_empty(struct il_priv *il,
struct il_tid_data *tid_data = &il->stations[sta_id].tid[tid];
struct il_rxon_context *ctx;
ctx = &il->contexts[il->stations[sta_id].ctxid];
ctx = &il->ctx;
lockdep_assert_held(&il->sta_lock);
......
......@@ -403,7 +403,7 @@ static int il4965_hw_set_hw_params(struct il_priv *il)
sizeof(struct il4965_scd_bc_tbl);
il->hw_params.tfd_size = sizeof(struct il_tfd);
il->hw_params.max_stations = IL4965_STATION_COUNT;
il->contexts[IL_RXON_CTX_BSS].bcast_sta_id = IL4965_BROADCAST_ID;
il->ctx.bcast_sta_id = IL4965_BROADCAST_ID;
il->hw_params.max_data_size = IL49_RTC_DATA_SIZE;
il->hw_params.max_inst_size = IL49_RTC_INST_SIZE;
il->hw_params.max_bsm_size = BSM_SRAM_SIZE;
......@@ -1121,7 +1121,7 @@ static int il4965_send_tx_power(struct il_priv *il)
u8 band = 0;
bool is_ht40 = false;
u8 ctrl_chan_high = 0;
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
if (WARN_ONCE(test_bit(STATUS_SCAN_HW, &il->status),
"TX Power requested while scanning!\n"))
......@@ -1333,7 +1333,7 @@ static int il4965_commit_rxon(struct il_priv *il, struct il_rxon_context *ctx)
static int il4965_hw_channel_switch(struct il_priv *il,
struct ieee80211_channel_switch *ch_switch)
{
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
int rc;
u8 band = 0;
bool is_ht40 = false;
......@@ -1726,7 +1726,7 @@ static u8 il4965_find_station(struct il_priv *il, const u8 *addr)
start = IL_STA_ID;
if (is_broadcast_ether_addr(addr))
return il->contexts[IL_RXON_CTX_BSS].bcast_sta_id;
return il->ctx.bcast_sta_id;
spin_lock_irqsave(&il->sta_lock, flags);
for (i = start; i < il->hw_params.max_stations; i++)
......@@ -1911,7 +1911,7 @@ static struct il_hcmd_ops il4965_hcmd = {
static void il4965_post_scan(struct il_priv *il)
{
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
/*
* Since setting the RXON may have been deferred while
......@@ -1923,7 +1923,7 @@ static void il4965_post_scan(struct il_priv *il)
static void il4965_post_associate(struct il_priv *il)
{
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
struct ieee80211_vif *vif = ctx->vif;
struct ieee80211_conf *conf = NULL;
int ret = 0;
......@@ -2000,7 +2000,7 @@ static void il4965_post_associate(struct il_priv *il)
static void il4965_config_ap(struct il_priv *il)
{
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
struct ieee80211_vif *vif = ctx->vif;
int ret = 0;
......
......@@ -853,7 +853,7 @@ EXPORT_SYMBOL(il_set_rate);
void il_chswitch_done(struct il_priv *il, bool is_success)
{
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
if (test_bit(STATUS_EXIT_PENDING, &il->status))
return;
......@@ -868,7 +868,7 @@ void il_rx_csa(struct il_priv *il, struct il_rx_mem_buffer *rxb)
struct il_rx_pkt *pkt = rxb_addr(rxb);
struct il_csa_notification *csa = &(pkt->u.csa_notif);
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
struct il_rxon_cmd *rxon = (void *)&ctx->active;
if (!test_bit(STATUS_CHANNEL_SWITCH_PENDING, &il->status))
......@@ -933,7 +933,7 @@ void il_irq_handle_error(struct il_priv *il)
#ifdef CONFIG_IWLEGACY_DEBUG
if (il_get_debug_level(il) & IL_DL_FW_ERRORS)
il_print_rx_config_cmd(il,
&il->contexts[IL_RXON_CTX_BSS]);
&il->ctx);
#endif
wake_up(&il->wait_command_queue);
......@@ -1110,7 +1110,7 @@ int il_set_tx_power(struct il_priv *il, s8 tx_power, bool force)
int ret;
s8 prev_tx_power;
bool defer;
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
lockdep_assert_held(&il->mutex);
......@@ -2069,7 +2069,7 @@ int il_mac_config(struct ieee80211_hw *hw, u32 changed)
int ret = 0;
u16 ch;
int scan_active = 0;
bool ht_changed[NUM_IL_RXON_CTX] = {};
bool ht_changed = false;
if (WARN_ON(!il->cfg->ops->legacy))
return -EOPNOTSUPP;
......@@ -2129,7 +2129,7 @@ int il_mac_config(struct ieee80211_hw *hw, u32 changed)
/* Configure HT40 channels */
if (ctx->ht.enabled != conf_is_ht(conf)) {
ctx->ht.enabled = conf_is_ht(conf);
ht_changed[ctx->ctxid] = true;
ht_changed = true;
}
if (ctx->ht.enabled) {
if (conf_is_ht40_minus(conf)) {
......@@ -2209,7 +2209,7 @@ int il_mac_config(struct ieee80211_hw *hw, u32 changed)
else
D_INFO(
"Not re-sending same RXON configuration.\n");
if (ht_changed[ctx->ctxid])
if (ht_changed)
il_update_qos(il, ctx);
}
......@@ -2225,8 +2225,7 @@ void il_mac_reset_tsf(struct ieee80211_hw *hw,
{
struct il_priv *il = hw->priv;
unsigned long flags;
/* IBSS can only be the IL_RXON_CTX_BSS context */
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
if (WARN_ON(!il->cfg->ops->legacy))
return;
......
......@@ -601,7 +601,7 @@ il_dbgfs_qos_read(struct file *file, char __user *user_buf,
struct il_priv *il = file->private_data;
struct il_rxon_context *ctx;
int pos = 0, i;
char buf[256 * NUM_IL_RXON_CTX];
char buf[256];
const size_t bufsz = sizeof(buf);
for_each_context(il, ctx) {
......@@ -1064,7 +1064,7 @@ static ssize_t il_dbgfs_rxon_flags_read(struct file *file,
char buf[20];
len = sprintf(buf, "0x%04X\n",
le32_to_cpu(il->contexts[IL_RXON_CTX_BSS].active.flags));
le32_to_cpu(il->ctx.active.flags));
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
......@@ -1077,7 +1077,7 @@ static ssize_t il_dbgfs_rxon_filter_flags_read(struct file *file,
char buf[20];
len = sprintf(buf, "0x%04X\n",
le32_to_cpu(il->contexts[IL_RXON_CTX_BSS].active.filter_flags));
le32_to_cpu(il->ctx.active.filter_flags));
return simple_read_from_buffer(user_buf, count, ppos, buf, len);
}
......
......@@ -899,12 +899,6 @@ struct il_force_reset {
*/
#define IL4965_EXT_BEACON_TIME_POS 22
enum il_rxon_context_id {
IL_RXON_CTX_BSS,
NUM_IL_RXON_CTX
};
struct il_rxon_context {
struct ieee80211_vif *vif;
......@@ -921,7 +915,7 @@ struct il_rxon_context {
bool ht_need_multiple_chains;
enum il_rxon_context_id ctxid;
int ctxid;
u32 interface_modes, exclusive_interface_modes;
u8 unused_devtype, ap_devtype, ibss_devtype, station_devtype;
......@@ -1029,9 +1023,6 @@ struct il_priv {
u32 hw_wa_rev;
u8 rev_id;
/* microcode/device supports multiple contexts */
u8 valid_contexts;
/* command queue number */
u8 cmd_queue;
......@@ -1055,7 +1046,7 @@ struct il_priv {
u8 ucode_write_complete; /* the image write is complete */
char firmware_name[25];
struct il_rxon_context contexts[NUM_IL_RXON_CTX];
struct il_rxon_context ctx;
__le16 switch_channel;
......@@ -1298,21 +1289,17 @@ il_rxon_ctx_from_vif(struct ieee80211_vif *vif)
return vif_priv->ctx;
}
#define for_each_context(il, ctx) \
for (ctx = &il->contexts[IL_RXON_CTX_BSS]; \
ctx < &il->contexts[NUM_IL_RXON_CTX]; ctx++) \
if (il->valid_contexts & BIT(ctx->ctxid))
#define for_each_context(il, _ctx) \
for (_ctx = &il->ctx; _ctx == &il->ctx; _ctx++)
static inline int il_is_associated(struct il_priv *il,
enum il_rxon_context_id ctxid)
static inline int il_is_associated(struct il_priv *il)
{
return (il->contexts[ctxid].active.filter_flags &
RXON_FILTER_ASSOC_MSK) ? 1 : 0;
return (il->ctx.active.filter_flags & RXON_FILTER_ASSOC_MSK) ? 1 : 0;
}
static inline int il_is_any_associated(struct il_priv *il)
{
return il_is_associated(il, IL_RXON_CTX_BSS);
return il_is_associated(il);
}
static inline int il_is_associated_ctx(struct il_rxon_context *ctx)
......
......@@ -241,7 +241,7 @@ int il_set_decrypted_flag(struct il_priv *il,
* All contexts have the same setting here due to it being
* a module parameter, so OK to check any context.
*/
if (il->contexts[IL_RXON_CTX_BSS].active.filter_flags &
if (il->ctx.active.filter_flags &
RXON_FILTER_DIS_DECRYPT_MSK)
return 0;
......
......@@ -1717,7 +1717,7 @@ static int il4965_alive_notify(struct il_priv *il)
static void il4965_alive_start(struct il_priv *il)
{
int ret = 0;
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
D_INFO("Runtime Alive received.\n");
......@@ -2502,7 +2502,7 @@ void il4965_mac_channel_switch(struct ieee80211_hw *hw,
struct ieee80211_channel *channel = ch_switch->channel;
struct il_ht_config *ht_conf = &il->current_ht_config;
struct il_rxon_context *ctx = &il->contexts[IL_RXON_CTX_BSS];
struct il_rxon_context *ctx = &il->ctx;
u16 ch;
D_MAC80211("enter\n");
......@@ -2786,7 +2786,7 @@ static int il4965_init_drv(struct il_priv *il)
/* Choose which receivers/antennas to use */
if (il->cfg->ops->hcmd->set_rxon_chain)
il->cfg->ops->hcmd->set_rxon_chain(il,
&il->contexts[IL_RXON_CTX_BSS]);
&il->ctx);
il_init_scan_params(il);
......@@ -2859,7 +2859,7 @@ static const u8 il4965_bss_ac_to_queue[] = {
static int
il4965_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
int err = 0, i;
int err = 0;
struct il_priv *il;
struct ieee80211_hw *hw;
struct il_cfg *cfg = (struct il_cfg *)(ent->driver_data);
......@@ -2878,36 +2878,26 @@ il4965_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
il = hw->priv;
/* At this point both hw and il are allocated. */
/*
* The default context is always valid,
* more may be discovered when firmware
* is loaded.
*/
il->valid_contexts = BIT(IL_RXON_CTX_BSS);
for (i = 0; i < NUM_IL_RXON_CTX; i++)
il->contexts[i].ctxid = i;
il->contexts[IL_RXON_CTX_BSS].always_active = true;
il->contexts[IL_RXON_CTX_BSS].is_active = true;
il->contexts[IL_RXON_CTX_BSS].rxon_cmd = REPLY_RXON;
il->contexts[IL_RXON_CTX_BSS].rxon_timing_cmd = REPLY_RXON_TIMING;
il->contexts[IL_RXON_CTX_BSS].rxon_assoc_cmd = REPLY_RXON_ASSOC;
il->contexts[IL_RXON_CTX_BSS].qos_cmd = REPLY_QOS_PARAM;
il->contexts[IL_RXON_CTX_BSS].ap_sta_id = IL_AP_ID;
il->contexts[IL_RXON_CTX_BSS].wep_key_cmd = REPLY_WEPKEY;
il->contexts[IL_RXON_CTX_BSS].ac_to_fifo = il4965_bss_ac_to_fifo;
il->contexts[IL_RXON_CTX_BSS].ac_to_queue = il4965_bss_ac_to_queue;
il->contexts[IL_RXON_CTX_BSS].exclusive_interface_modes =
il->ctx.ctxid = 0;
il->ctx.always_active = true;
il->ctx.is_active = true;
il->ctx.rxon_cmd = REPLY_RXON;
il->ctx.rxon_timing_cmd = REPLY_RXON_TIMING;
il->ctx.rxon_assoc_cmd = REPLY_RXON_ASSOC;
il->ctx.qos_cmd = REPLY_QOS_PARAM;
il->ctx.ap_sta_id = IL_AP_ID;
il->ctx.wep_key_cmd = REPLY_WEPKEY;
il->ctx.ac_to_fifo = il4965_bss_ac_to_fifo;
il->ctx.ac_to_queue = il4965_bss_ac_to_queue;
il->ctx.exclusive_interface_modes =
BIT(NL80211_IFTYPE_ADHOC);
il->contexts[IL_RXON_CTX_BSS].interface_modes =
il->ctx.interface_modes =
BIT(NL80211_IFTYPE_STATION);
il->contexts[IL_RXON_CTX_BSS].ap_devtype = RXON_DEV_TYPE_AP;
il->contexts[IL_RXON_CTX_BSS].ibss_devtype = RXON_DEV_TYPE_IBSS;
il->contexts[IL_RXON_CTX_BSS].station_devtype = RXON_DEV_TYPE_ESS;
il->contexts[IL_RXON_CTX_BSS].unused_devtype = RXON_DEV_TYPE_ESS;
BUILD_BUG_ON(NUM_IL_RXON_CTX != 1);
il->ctx.ap_devtype = RXON_DEV_TYPE_AP;
il->ctx.ibss_devtype = RXON_DEV_TYPE_IBSS;
il->ctx.station_devtype = RXON_DEV_TYPE_ESS;
il->ctx.unused_devtype = RXON_DEV_TYPE_ESS;
SET_IEEE80211_DEV(hw, &pdev->dev);
......
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