Commit 21fa1004 authored by Jens Axboe's avatar Jens Axboe

Merge branch 'nvme-5.4' of git://git.infradead.org/nvme into for-5.4/block

Pull NVMe updates from Sagi:

"Highlights includes:
 - controller reset and namespace scan races fixes
 - nvme discovery log change uevent support
 - naming improvements from Keith
 - multiple discovery controllers reject fix from James
 - some regular cleanups from various people"

* 'nvme-5.4' of git://git.infradead.org/nvme:
  nvmet: fix a wrong error status returned in error log page
  nvme: send discovery log page change events to userspace
  nvme: add uevent variables for controller devices
  nvme: enable aen regardless of the presence of I/O queues
  nvme-fabrics: allow discovery subsystems accept a kato
  nvmet: Use PTR_ERR_OR_ZERO() in nvmet_init_discovery()
  nvme: Remove redundant assignment of cq vector
  nvme: Assign subsys instance from first ctrl
  nvme: tcp: remove redundant assignment to variable ret
  nvme: include admin_q sync with nvme_sync_queues
  nvme: Treat discovery subsystems as unique subsystems
  nvme: fix ns removal hang when failing to revalidate due to a transient error
  nvme: make nvme_report_ns_ids propagate error back
  nvme: make nvme_identify_ns propagate errors back
  nvme: pass status to nvme_error_status
  nvme-fc: Fail transport errors with NVME_SC_HOST_PATH
  nvme-tcp: fail command with NVME_SC_HOST_PATH_ERROR send failed
  nvme: fail cancelled commands with NVME_SC_HOST_PATH_ERROR
parents 0a67b5a9 5f8badbc
......@@ -81,7 +81,6 @@ EXPORT_SYMBOL_GPL(nvme_reset_wq);
struct workqueue_struct *nvme_delete_wq;
EXPORT_SYMBOL_GPL(nvme_delete_wq);
static DEFINE_IDA(nvme_subsystems_ida);
static LIST_HEAD(nvme_subsystems);
static DEFINE_MUTEX(nvme_subsystems_lock);
......@@ -197,9 +196,9 @@ static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
return ns->pi_type && ns->ms == sizeof(struct t10_pi_tuple);
}
static blk_status_t nvme_error_status(struct request *req)
static blk_status_t nvme_error_status(u16 status)
{
switch (nvme_req(req)->status & 0x7ff) {
switch (status & 0x7ff) {
case NVME_SC_SUCCESS:
return BLK_STS_OK;
case NVME_SC_CAP_EXCEEDED:
......@@ -226,6 +225,8 @@ static blk_status_t nvme_error_status(struct request *req)
return BLK_STS_PROTECTION;
case NVME_SC_RESERVATION_CONFLICT:
return BLK_STS_NEXUS;
case NVME_SC_HOST_PATH_ERROR:
return BLK_STS_TRANSPORT;
default:
return BLK_STS_IOERR;
}
......@@ -260,7 +261,7 @@ static void nvme_retry_req(struct request *req)
void nvme_complete_rq(struct request *req)
{
blk_status_t status = nvme_error_status(req);
blk_status_t status = nvme_error_status(nvme_req(req)->status);
trace_nvme_complete_rq(req);
......@@ -294,7 +295,7 @@ bool nvme_cancel_request(struct request *req, void *data, bool reserved)
if (blk_mq_request_completed(req))
return true;
nvme_req(req)->status = NVME_SC_ABORT_REQ;
nvme_req(req)->status = NVME_SC_HOST_PATH_ERROR;
blk_mq_complete_request(req);
return true;
}
......@@ -1094,10 +1095,9 @@ static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *n
NVME_IDENTIFY_DATA_SIZE);
}
static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
unsigned nsid)
static int nvme_identify_ns(struct nvme_ctrl *ctrl,
unsigned nsid, struct nvme_id_ns **id)
{
struct nvme_id_ns *id;
struct nvme_command c = { };
int error;
......@@ -1106,18 +1106,17 @@ static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
c.identify.nsid = cpu_to_le32(nsid);
c.identify.cns = NVME_ID_CNS_NS;
id = kmalloc(sizeof(*id), GFP_KERNEL);
if (!id)
return NULL;
*id = kmalloc(sizeof(**id), GFP_KERNEL);
if (!*id)
return -ENOMEM;
error = nvme_submit_sync_cmd(ctrl->admin_q, &c, id, sizeof(*id));
error = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id, sizeof(**id));
if (error) {
dev_warn(ctrl->device, "Identify namespace failed (%d)\n", error);
kfree(id);
return NULL;
kfree(*id);
}
return id;
return error;
}
static int nvme_features(struct nvme_ctrl *dev, u8 op, unsigned int fid,
......@@ -1186,7 +1185,8 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
EXPORT_SYMBOL_GPL(nvme_set_queue_count);
#define NVME_AEN_SUPPORTED \
(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | NVME_AEN_CFG_ANA_CHANGE)
(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | \
NVME_AEN_CFG_ANA_CHANGE | NVME_AEN_CFG_DISC_CHANGE)
static void nvme_enable_aen(struct nvme_ctrl *ctrl)
{
......@@ -1201,6 +1201,8 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
if (status)
dev_warn(ctrl->device, "Failed to configure AEN (cfg %x)\n",
supported_aens);
queue_work(nvme_wq, &ctrl->async_event_work);
}
static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
......@@ -1595,9 +1597,11 @@ static void nvme_config_write_zeroes(struct gendisk *disk, struct nvme_ns *ns)
blk_queue_max_write_zeroes_sectors(disk->queue, max_sectors);
}
static void nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
static int nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
struct nvme_id_ns *id, struct nvme_ns_ids *ids)
{
int ret = 0;
memset(ids, 0, sizeof(*ids));
if (ctrl->vs >= NVME_VS(1, 1, 0))
......@@ -1608,10 +1612,12 @@ static void nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
/* Don't treat error as fatal we potentially
* already have a NGUID or EUI-64
*/
if (nvme_identify_ns_descs(ctrl, nsid, ids))
ret = nvme_identify_ns_descs(ctrl, nsid, ids);
if (ret)
dev_warn(ctrl->device,
"%s: Identify Descriptors failed\n", __func__);
"Identify Descriptors failed (%d)\n", ret);
}
return ret;
}
static bool nvme_ns_ids_valid(struct nvme_ns_ids *ids)
......@@ -1738,25 +1744,37 @@ static int nvme_revalidate_disk(struct gendisk *disk)
return -ENODEV;
}
id = nvme_identify_ns(ctrl, ns->head->ns_id);
if (!id)
return -ENODEV;
ret = nvme_identify_ns(ctrl, ns->head->ns_id, &id);
if (ret)
goto out;
if (id->ncap == 0) {
ret = -ENODEV;
goto out;
goto free_id;
}
__nvme_revalidate_disk(disk, id);
nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
ret = nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
if (ret)
goto free_id;
if (!nvme_ns_ids_equal(&ns->head->ids, &ids)) {
dev_err(ctrl->device,
"identifiers changed for nsid %d\n", ns->head->ns_id);
ret = -ENODEV;
}
out:
free_id:
kfree(id);
out:
/*
* Only fail the function if we got a fatal error back from the
* device, otherwise ignore the error and just move on.
*/
if (ret == -ENOMEM || (ret > 0 && !(ret & NVME_SC_DNR)))
ret = 0;
else if (ret > 0)
ret = blk_status_to_errno(nvme_error_status(ret));
return ret;
}
......@@ -2329,7 +2347,8 @@ static void nvme_release_subsystem(struct device *dev)
struct nvme_subsystem *subsys =
container_of(dev, struct nvme_subsystem, dev);
ida_simple_remove(&nvme_subsystems_ida, subsys->instance);
if (subsys->instance >= 0)
ida_simple_remove(&nvme_instance_ida, subsys->instance);
kfree(subsys);
}
......@@ -2358,6 +2377,17 @@ static struct nvme_subsystem *__nvme_find_get_subsystem(const char *subsysnqn)
lockdep_assert_held(&nvme_subsystems_lock);
/*
* Fail matches for discovery subsystems. This results
* in each discovery controller bound to a unique subsystem.
* This avoids issues with validating controller values
* that can only be true when there is a single unique subsystem.
* There may be multiple and completely independent entities
* that provide discovery controllers.
*/
if (!strcmp(subsysnqn, NVME_DISC_SUBSYS_NAME))
return NULL;
list_for_each_entry(subsys, &nvme_subsystems, entry) {
if (strcmp(subsys->subnqn, subsysnqn))
continue;
......@@ -2458,12 +2488,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
subsys = kzalloc(sizeof(*subsys), GFP_KERNEL);
if (!subsys)
return -ENOMEM;
ret = ida_simple_get(&nvme_subsystems_ida, 0, 0, GFP_KERNEL);
if (ret < 0) {
kfree(subsys);
return ret;
}
subsys->instance = ret;
subsys->instance = -1;
mutex_init(&subsys->lock);
kref_init(&subsys->ref);
INIT_LIST_HEAD(&subsys->ctrls);
......@@ -2482,7 +2508,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
subsys->dev.class = nvme_subsys_class;
subsys->dev.release = nvme_release_subsystem;
subsys->dev.groups = nvme_subsys_attrs_groups;
dev_set_name(&subsys->dev, "nvme-subsys%d", subsys->instance);
dev_set_name(&subsys->dev, "nvme-subsys%d", ctrl->instance);
device_initialize(&subsys->dev);
mutex_lock(&nvme_subsystems_lock);
......@@ -2513,6 +2539,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
goto out_put_subsystem;
}
if (!found)
subsys->instance = ctrl->instance;
ctrl->subsys = subsys;
list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
mutex_unlock(&nvme_subsystems_lock);
......@@ -3173,7 +3201,9 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
head->ns_id = nsid;
kref_init(&head->ref);
nvme_report_ns_ids(ctrl, nsid, id, &head->ids);
ret = nvme_report_ns_ids(ctrl, nsid, id, &head->ids);
if (ret)
goto out_cleanup_srcu;
ret = __nvme_check_ids(ctrl->subsys, head);
if (ret) {
......@@ -3198,6 +3228,8 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
out_free_head:
kfree(head);
out:
if (ret > 0)
ret = blk_status_to_errno(nvme_error_status(ret));
return ERR_PTR(ret);
}
......@@ -3221,7 +3253,10 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
} else {
struct nvme_ns_ids ids;
nvme_report_ns_ids(ctrl, nsid, id, &ids);
ret = nvme_report_ns_ids(ctrl, nsid, id, &ids);
if (ret)
goto out_unlock;
if (!nvme_ns_ids_equal(&head->ids, &ids)) {
dev_err(ctrl->device,
"IDs don't match for shared namespace %d\n",
......@@ -3236,6 +3271,8 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
out_unlock:
mutex_unlock(&ctrl->subsys->lock);
if (ret > 0)
ret = blk_status_to_errno(nvme_error_status(ret));
return ret;
}
......@@ -3327,11 +3364,9 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
blk_queue_logical_block_size(ns->queue, 1 << ns->lba_shift);
nvme_set_queue_limits(ctrl, ns->queue);
id = nvme_identify_ns(ctrl, nsid);
if (!id) {
ret = -EIO;
ret = nvme_identify_ns(ctrl, nsid, &id);
if (ret)
goto out_free_queue;
}
if (id->ncap == 0) {
ret = -EINVAL;
......@@ -3393,6 +3428,8 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
blk_cleanup_queue(ns->queue);
out_free_ns:
kfree(ns);
if (ret > 0)
ret = blk_status_to_errno(nvme_error_status(ret));
return ret;
}
......@@ -3599,6 +3636,33 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
}
EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
static int nvme_class_uevent(struct device *dev, struct kobj_uevent_env *env)
{
struct nvme_ctrl *ctrl =
container_of(dev, struct nvme_ctrl, ctrl_device);
struct nvmf_ctrl_options *opts = ctrl->opts;
int ret;
ret = add_uevent_var(env, "NVME_TRTYPE=%s", ctrl->ops->name);
if (ret)
return ret;
if (opts) {
ret = add_uevent_var(env, "NVME_TRADDR=%s", opts->traddr);
if (ret)
return ret;
ret = add_uevent_var(env, "NVME_TRSVCID=%s",
opts->trsvcid ?: "none");
if (ret)
return ret;
ret = add_uevent_var(env, "NVME_HOST_TRADDR=%s",
opts->host_traddr ?: "none");
}
return ret;
}
static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
{
char *envp[2] = { NULL, NULL };
......@@ -3705,6 +3769,9 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
queue_work(nvme_wq, &ctrl->ana_work);
break;
#endif
case NVME_AER_NOTICE_DISC_CHANGED:
ctrl->aen_result = result;
break;
default:
dev_warn(ctrl->device, "async event result %08x\n", result);
}
......@@ -3751,10 +3818,10 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
if (ctrl->kato)
nvme_start_keep_alive(ctrl);
nvme_enable_aen(ctrl);
if (ctrl->queue_count > 1) {
nvme_queue_scan(ctrl);
nvme_enable_aen(ctrl);
queue_work(nvme_wq, &ctrl->async_event_work);
nvme_start_queues(ctrl);
}
}
......@@ -3774,7 +3841,9 @@ static void nvme_free_ctrl(struct device *dev)
container_of(dev, struct nvme_ctrl, ctrl_device);
struct nvme_subsystem *subsys = ctrl->subsys;
ida_simple_remove(&nvme_instance_ida, ctrl->instance);
if (subsys && ctrl->instance != subsys->instance)
ida_simple_remove(&nvme_instance_ida, ctrl->instance);
kfree(ctrl->effects);
nvme_mpath_uninit(ctrl);
__free_page(ctrl->discard_page);
......@@ -3974,6 +4043,9 @@ void nvme_sync_queues(struct nvme_ctrl *ctrl)
list_for_each_entry(ns, &ctrl->namespaces, list)
blk_sync_queue(ns->queue);
up_read(&ctrl->namespaces_rwsem);
if (ctrl->admin_q)
blk_sync_queue(ctrl->admin_q);
}
EXPORT_SYMBOL_GPL(nvme_sync_queues);
......@@ -4032,6 +4104,7 @@ static int __init nvme_core_init(void)
result = PTR_ERR(nvme_class);
goto unregister_chrdev;
}
nvme_class->dev_uevent = nvme_class_uevent;
nvme_subsys_class = class_create(THIS_MODULE, "nvme-subsystem");
if (IS_ERR(nvme_subsys_class)) {
......@@ -4056,7 +4129,6 @@ static int __init nvme_core_init(void)
static void __exit nvme_core_exit(void)
{
ida_destroy(&nvme_subsystems_ida);
class_destroy(nvme_subsys_class);
class_destroy(nvme_class);
unregister_chrdev_region(nvme_chr_devt, NVME_MINORS);
......
......@@ -381,8 +381,8 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
* Set keep-alive timeout in seconds granularity (ms * 1000)
* and add a grace period for controller kato enforcement
*/
cmd.connect.kato = ctrl->opts->discovery_nqn ? 0 :
cpu_to_le32((ctrl->kato + NVME_KATO_GRACE) * 1000);
cmd.connect.kato = ctrl->kato ?
cpu_to_le32((ctrl->kato + NVME_KATO_GRACE) * 1000) : 0;
if (ctrl->opts->disable_sqflow)
cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
......@@ -740,13 +740,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
pr_warn("keep_alive_tmo 0 won't execute keep alives!!!\n");
}
opts->kato = token;
if (opts->discovery_nqn && opts->kato) {
pr_err("Discovery controllers cannot accept KATO != 0\n");
ret = -EINVAL;
goto out;
}
break;
case NVMF_OPT_CTRL_LOSS_TMO:
if (match_int(args, &token)) {
......@@ -883,7 +876,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
}
if (opts->discovery_nqn) {
opts->kato = 0;
opts->nr_io_queues = 0;
opts->nr_write_queues = 0;
opts->nr_poll_queues = 0;
......
......@@ -1608,9 +1608,13 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
sizeof(op->rsp_iu), DMA_FROM_DEVICE);
if (opstate == FCPOP_STATE_ABORTED)
status = cpu_to_le16(NVME_SC_ABORT_REQ << 1);
else if (freq->status)
status = cpu_to_le16(NVME_SC_INTERNAL << 1);
status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
else if (freq->status) {
status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
dev_info(ctrl->ctrl.device,
"NVME-FC{%d}: io failed due to lldd error %d\n",
ctrl->cnum, freq->status);
}
/*
* For the linux implementation, if we have an unsuccesful
......@@ -1637,8 +1641,13 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
* no payload in the CQE by the transport.
*/
if (freq->transferred_length !=
be32_to_cpu(op->cmd_iu.data_len)) {
status = cpu_to_le16(NVME_SC_INTERNAL << 1);
be32_to_cpu(op->cmd_iu.data_len)) {
status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
dev_info(ctrl->ctrl.device,
"NVME-FC{%d}: io failed due to bad transfer "
"length: %d vs expected %d\n",
ctrl->cnum, freq->transferred_length,
be32_to_cpu(op->cmd_iu.data_len));
goto done;
}
result.u64 = 0;
......@@ -1655,7 +1664,17 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
freq->transferred_length ||
op->rsp_iu.status_code ||
sqe->common.command_id != cqe->command_id)) {
status = cpu_to_le16(NVME_SC_INTERNAL << 1);
status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
dev_info(ctrl->ctrl.device,
"NVME-FC{%d}: io failed due to bad NVMe_ERSP: "
"iu len %d, xfr len %d vs %d, status code "
"%d, cmdid %d vs %d\n",
ctrl->cnum, be16_to_cpu(op->rsp_iu.iu_len),
be32_to_cpu(op->rsp_iu.xfrd_len),
freq->transferred_length,
op->rsp_iu.status_code,
sqe->common.command_id,
cqe->command_id);
goto done;
}
result = cqe->result;
......@@ -1663,7 +1682,11 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
break;
default:
status = cpu_to_le16(NVME_SC_INTERNAL << 1);
status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
dev_info(ctrl->ctrl.device,
"NVME-FC{%d}: io failed due to odd NVMe_xRSP iu "
"len %d\n",
ctrl->cnum, freq->rcv_rsplen);
goto done;
}
......
......@@ -1555,7 +1555,6 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled)
nvme_init_queue(nvmeq, qid);
if (!polled) {
nvmeq->cq_vector = vector;
result = queue_request_irq(nvmeq);
if (result < 0)
goto release_sq;
......
......@@ -842,7 +842,7 @@ static inline void nvme_tcp_done_send_req(struct nvme_tcp_queue *queue)
static void nvme_tcp_fail_request(struct nvme_tcp_request *req)
{
nvme_tcp_end_request(blk_mq_rq_from_pdu(req), NVME_SC_DATA_XFER_ERROR);
nvme_tcp_end_request(blk_mq_rq_from_pdu(req), NVME_SC_HOST_PATH_ERROR);
}
static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
......@@ -1824,7 +1824,7 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
{
struct nvmf_ctrl_options *opts = ctrl->opts;
int ret = -EINVAL;
int ret;
ret = nvme_tcp_configure_admin_queue(ctrl, new);
if (ret)
......
......@@ -37,7 +37,6 @@ static void nvmet_execute_get_log_page_noop(struct nvmet_req *req)
static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
{
struct nvmet_ctrl *ctrl = req->sq->ctrl;
u16 status = NVME_SC_SUCCESS;
unsigned long flags;
off_t offset = 0;
u64 slot;
......@@ -47,9 +46,8 @@ static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
slot = ctrl->err_counter % NVMET_ERROR_LOG_SLOTS;
for (i = 0; i < NVMET_ERROR_LOG_SLOTS; i++) {
status = nvmet_copy_to_sgl(req, offset, &ctrl->slots[slot],
sizeof(struct nvme_error_slot));
if (status)
if (nvmet_copy_to_sgl(req, offset, &ctrl->slots[slot],
sizeof(struct nvme_error_slot)))
break;
if (slot == 0)
......@@ -59,7 +57,7 @@ static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
offset += sizeof(struct nvme_error_slot);
}
spin_unlock_irqrestore(&ctrl->error_lock, flags);
nvmet_req_complete(req, status);
nvmet_req_complete(req, 0);
}
static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
......
......@@ -381,9 +381,7 @@ int __init nvmet_init_discovery(void)
{
nvmet_disc_subsys =
nvmet_subsys_alloc(NVME_DISC_SUBSYS_NAME, NVME_NQN_DISC);
if (IS_ERR(nvmet_disc_subsys))
return PTR_ERR(nvmet_disc_subsys);
return 0;
return PTR_ERR_OR_ZERO(nvmet_disc_subsys);
}
void nvmet_exit_discovery(void)
......
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