On 26-Feb-19 23:27, Steve Wise wrote:
>> +
>> +static const char *efa_com_cmd_str(u8 cmd)
>> +{
>> +#define EFA_CMD_STR_CASE(_cmd) case EFA_ADMIN_##_cmd: return #_cmd
>> +
>> + switch (cmd) {
>> + EFA_CMD_STR_CASE(CREATE_QP);
>> + EFA_CMD_STR_CASE(MODIFY_QP);
>> + EFA_CMD_STR_CASE(QUERY_QP);
>> + EFA_CMD_STR_CASE(DESTROY_QP);
>> + EFA_CMD_STR_CASE(CREATE_AH);
>> + EFA_CMD_STR_CASE(DESTROY_AH);
>> + EFA_CMD_STR_CASE(REG_MR);
>> + EFA_CMD_STR_CASE(DEREG_MR);
>> + EFA_CMD_STR_CASE(CREATE_CQ);
>> + EFA_CMD_STR_CASE(DESTROY_CQ);
>> + EFA_CMD_STR_CASE(GET_FEATURE);
>> + EFA_CMD_STR_CASE(SET_FEATURE);
>> + EFA_CMD_STR_CASE(GET_STATS);
>> + EFA_CMD_STR_CASE(ALLOC_PD);
>> + EFA_CMD_STR_CASE(DEALLOC_PD);
>> + EFA_CMD_STR_CASE(ALLOC_UAR);
>> + EFA_CMD_STR_CASE(DEALLOC_UAR);
>> + default: return "unknown command opcode";
>> + }
>
>
> Should you #undef EFA_CMD_STR_CASE here?
Will do.
>
>
>> +static int efa_com_admin_init_aenq(struct efa_com_dev *edev,
>> + struct efa_aenq_handlers *aenq_handlers)
>> +{
>> + struct efa_com_aenq *aenq = &edev->aenq;
>> + u32 addr_low, addr_high, aenq_caps;
>> + u16 size;
>> +
>> + if (unlikely(!aenq_handlers)) {
>> + efa_err(edev->dmadev, "aenq handlers pointer is NULL\n");
>> + return -EINVAL;
>> + }
>> +
>> + size = ADMIN_AENQ_SIZE(EFA_ASYNC_QUEUE_DEPTH);
>> + aenq->entries = dma_alloc_coherent(edev->dmadev, size, &aenq->dma_addr,
>> + GFP_KERNEL);
>> + if (!aenq->entries)
>> + return -ENOMEM;
>> +
>> + aenq->aenq_handlers = aenq_handlers;
>> + aenq->depth = EFA_ASYNC_QUEUE_DEPTH;
>> + aenq->cc = 0;
>> + aenq->phase = 1;
>> +
>> + addr_low = EFA_DMA_ADDR_TO_UINT32_LOW(aenq->dma_addr);
>> + addr_high = EFA_DMA_ADDR_TO_UINT32_HIGH(aenq->dma_addr);
>> +
>> + writel(addr_low, edev->reg_bar + EFA_REGS_AENQ_BASE_LO_OFF);
>> + writel(addr_high, edev->reg_bar + EFA_REGS_AENQ_BASE_HI_OFF);
>> +
>> + aenq_caps = 0;
>> + aenq_caps |= aenq->depth & EFA_REGS_AENQ_CAPS_AENQ_DEPTH_MASK;
>> + aenq_caps |= (sizeof(struct efa_admin_aenq_entry) <<
>> + EFA_REGS_AENQ_CAPS_AENQ_ENTRY_SIZE_SHIFT) &
>> + EFA_REGS_AENQ_CAPS_AENQ_ENTRY_SIZE_MASK;
>> + aenq_caps |= (aenq->msix_vector_idx
>> + << EFA_REGS_AENQ_CAPS_AENQ_MSIX_VECTOR_SHIFT) &
>> + EFA_REGS_AENQ_CAPS_AENQ_MSIX_VECTOR_MASK;
>> + writel(aenq_caps, edev->reg_bar + EFA_REGS_AENQ_CAPS_OFF);
>> +
>> + /*
>> + * Init cons_db to mark that all entries in the queue
>> + * are initially available
>> + */
>> + writel(edev->aenq.cc, edev->reg_bar + EFA_REGS_AENQ_CONS_DB_OFF);
>> +
>
>
> Do you need any barrier type operation here to ensure these values have
> been read by HW?
writel has an implicit barrier AFAIK.
>
>
>> + return 0;
>> +}
>> +
>> +static int efa_com_wait_and_process_admin_cq_polling(struct efa_comp_ctx *comp_ctx,
>> + struct efa_com_admin_queue *aq)
>> +{
>> + unsigned long timeout;
>> + unsigned long flags;
>> + int err;
>> +
>> + timeout = jiffies + usecs_to_jiffies(aq->completion_timeout);
>> +
>> + while (1) {
>> + spin_lock_irqsave(&aq->cq.lock, flags);
>> + efa_com_handle_admin_completion(aq);
>> + spin_unlock_irqrestore(&aq->cq.lock, flags);
>> +
>> + if (comp_ctx->status != EFA_CMD_SUBMITTED)
>> + break;
>> +
>> + if (time_is_before_jiffies(timeout)) {
>> + efa_err(aq->dmadev,
>> + "Wait for completion (polling) timeout\n");
>> + /* EFA didn't have any completion */
>> + efa_admin_stat_inc(aq, aq->stats.no_completion);
>> +
>> + clear_bit(EFA_AQ_STATE_RUNNING_BIT, &aq->state);
>> + err = -ETIME;
>> + goto out;
>> + }
>> +
>> + msleep(aq->poll_interval);
>> + }
>> +
>> + if (unlikely(comp_ctx->status == EFA_CMD_ABORTED)) {
>> + efa_err(aq->dmadev, "Command was aborted\n");
>> + efa_admin_stat_inc(aq, aq->stats.aborted_cmd);
>> + err = -ENODEV;
>> + goto out;
>> + }
>> +
>> + WARN(comp_ctx->status != EFA_CMD_COMPLETED, "Invalid comp status %d\n",
>> + comp_ctx->status);
>> +
>
>
> Should the WARN() be rate limited or maybe WARN_ONCE()? Or perhaps just
> a pr_warn().
ACK.
>
>> + err = efa_com_comp_status_to_errno(aq, comp_ctx->comp_status);
>> +out:
>> + efa_com_put_comp_ctx(aq, comp_ctx);
>> + return err;
>> +}
>> +
> Are all these function comments proper kdoc or whatever format? I
> thought it should be /** to start? Or maybe it doesn't matter?
>
You're probably right.
>
>> +int efa_com_cmd_exec(struct efa_com_admin_queue *aq,
>> + struct efa_admin_aq_entry *cmd,
>> + size_t cmd_size,
>> + struct efa_admin_acq_entry *comp,
>> + size_t comp_size)
>> +{
>> + struct efa_comp_ctx *comp_ctx;
>> + int err;
>> +
>> + might_sleep();
>> +
>> + /* In case of queue FULL */
>> + down(&aq->avail_cmds);
>> +
>> + efa_dbg(aq->dmadev, "%s (opcode %d)\n",
>> + efa_com_cmd_str(cmd->aq_common_descriptor.opcode),
>> + cmd->aq_common_descriptor.opcode);
>> + comp_ctx = efa_com_submit_admin_cmd(aq, cmd, cmd_size, comp, comp_size);
>> + if (unlikely(IS_ERR(comp_ctx))) {
>> + efa_err(aq->dmadev,
>> + "Failed to submit command %s (opcode %u) err %ld\n",
>> + efa_com_cmd_str(cmd->aq_common_descriptor.opcode),
>> + cmd->aq_common_descriptor.opcode, PTR_ERR(comp_ctx));
>> +
>> + up(&aq->avail_cmds);
>> + return PTR_ERR(comp_ctx);
>> + }
>> +
>> + err = efa_com_wait_and_process_admin_cq(comp_ctx, aq);
>> + if (unlikely(err))
>> + efa_err(aq->dmadev,
>> + "Failed to process command %s (opcode %u) comp_status %d err %d\n",
>> + efa_com_cmd_str(cmd->aq_common_descriptor.opcode),
>> + cmd->aq_common_descriptor.opcode, comp_ctx->comp_status,
>> + err);
>> +
>> + up(&aq->avail_cmds);
>> +
>> + return err;
>> +}
>> +
>> +void efa_com_mmio_reg_read_destroy(struct efa_com_dev *edev)
>> +{
>> + struct efa_com_mmio_read *mmio_read = &edev->mmio_read;
>> +
>> + /* just in case someone is still spinning on a read */
>> + spin_lock(&mmio_read->lock);
>> + dma_free_coherent(edev->dmadev, sizeof(*mmio_read->read_resp),
>> + mmio_read->read_resp, mmio_read->read_resp_dma_addr);
>> + spin_unlock(&mmio_read->lock);
>> +}
>
>
> If someone can be spinning on a read, then can they actually try to grab
> this lock -after- the lock is grabbed here? If so, then when the read
> thread does acquire the lock, the read will be accessing freed memory.
This lock is not really needed here. All register reads are done in our init
flow which is guaranteed to be done by the time we call this function. I'll
remove it.
>
>
>> +
>> +/*
>> + * efa_com_validate_version - Validate the device parameters
>> + * @edev: EFA communication layer struct
>> + *
>> + * This method validate the device parameters are the same as the saved
>> + * parameters in edev.
>> + * This method is useful after device reset, to validate the device mac address
>> + * and the device offloads are the same as before the reset.
>> + *
>> + * @return - 0 on success negative value otherwise.
>> + */
>> +int efa_com_validate_version(struct efa_com_dev *edev)
>> +{
>> + u32 ctrl_ver_masked;
>> + u32 ctrl_ver;
>> + u32 ver;
>> +
>> + /*
>> + * Make sure the EFA version and the controller version are at least
>> + * as the driver expects
>> + */
>> + ver = efa_com_reg_read32(edev, EFA_REGS_VERSION_OFF);
>> + ctrl_ver = efa_com_reg_read32(edev,
>> + EFA_REGS_CONTROLLER_VERSION_OFF);
>> +
>> + efa_info(edev->dmadev,
>> + "efa device version: %d.%d\n",
>> + (ver & EFA_REGS_VERSION_MAJOR_VERSION_MASK) >>
>> + EFA_REGS_VERSION_MAJOR_VERSION_SHIFT,
>> + ver & EFA_REGS_VERSION_MINOR_VERSION_MASK);
>> +
>> + if (ver < MIN_EFA_VER) {
>> + efa_err(edev->dmadev,
>> + "EFA version is lower than the minimal version the driver supports\n");
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + efa_info(edev->dmadev,
>> + "efa controller version: %d.%d.%d implementation version %d\n",
>> + (ctrl_ver & EFA_REGS_CONTROLLER_VERSION_MAJOR_VERSION_MASK)
>> + >> EFA_REGS_CONTROLLER_VERSION_MAJOR_VERSION_SHIFT,
>> + (ctrl_ver & EFA_REGS_CONTROLLER_VERSION_MINOR_VERSION_MASK)
>> + >> EFA_REGS_CONTROLLER_VERSION_MINOR_VERSION_SHIFT,
>> + (ctrl_ver & EFA_REGS_CONTROLLER_VERSION_SUBMINOR_VERSION_MASK),
>> + (ctrl_ver & EFA_REGS_CONTROLLER_VERSION_IMPL_ID_MASK) >>
>> + EFA_REGS_CONTROLLER_VERSION_IMPL_ID_SHIFT);
>> +
>> + ctrl_ver_masked =
>> + (ctrl_ver & EFA_REGS_CONTROLLER_VERSION_MAJOR_VERSION_MASK) |
>> + (ctrl_ver & EFA_REGS_CONTROLLER_VERSION_MINOR_VERSION_MASK) |
>> + (ctrl_ver & EFA_REGS_CONTROLLER_VERSION_SUBMINOR_VERSION_MASK);
>> +
>> + /* Validate the ctrl version without the implementation ID */
>> + if (ctrl_ver_masked < MIN_EFA_CTRL_VER) {
>> + efa_err(edev->dmadev,
>> + "EFA ctrl version is lower than the minimal ctrl version the driver supports\n");
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * efa_com_get_dma_width - Retrieve physical dma address width the device
>> + * supports.
>> + * @edev: EFA communication layer struct
>> + *
>> + * Retrieve the maximum physical address bits the device can handle.
>> + *
>> + * @return: > 0 on Success and negative value otherwise.
>> + */
>> +int efa_com_get_dma_width(struct efa_com_dev *edev)
>> +{
>> + u32 caps = efa_com_reg_read32(edev, EFA_REGS_CAPS_OFF);
>> + int width;
>> +
>> + width = (caps & EFA_REGS_CAPS_DMA_ADDR_WIDTH_MASK) >>
>> + EFA_REGS_CAPS_DMA_ADDR_WIDTH_SHIFT;
>> +
>> + efa_dbg(edev->dmadev, "DMA width: %d\n", width);
>> +
>> + if (width < 32 || width > 64) {
>> + efa_err(edev->dmadev, "DMA width illegal value: %d\n", width);
>> + return -EINVAL;
>> + }
>> +
>> + edev->dma_addr_bits = width;
>> +
>> + return width;
>> +}
>> +
>> +static int wait_for_reset_state(struct efa_com_dev *edev, u32 timeout,
>> + u16 exp_state)
>> +{
>> + u32 val, i;
>> +
>> + for (i = 0; i < timeout; i++) {
>> + val = efa_com_reg_read32(edev, EFA_REGS_DEV_STS_OFF);
>> +
>> + if ((val & EFA_REGS_DEV_STS_RESET_IN_PROGRESS_MASK) ==
>> + exp_state)
>> + return 0;
>> +
>> + efa_dbg(edev->dmadev, "Reset indication val %d\n", val);
>> + msleep(EFA_POLL_INTERVAL_MS);
>> + }
>> +
>> + return -ETIME;
>> +}
>> +
>> +/*
>> + * efa_com_dev_reset - Perform device FLR to the device.
>> + * @edev: EFA communication layer struct
>> + * @reset_reason: Specify what is the trigger for the reset in case of an error.
>> + *
>> + * @return - 0 on success, negative value on failure.
>> + */
>> +int efa_com_dev_reset(struct efa_com_dev *edev,
>> + enum efa_regs_reset_reason_types reset_reason)
>> +{
>> + u32 stat, timeout, cap, reset_val;
>> + int err;
>> +
>> + stat = efa_com_reg_read32(edev, EFA_REGS_DEV_STS_OFF);
>> + cap = efa_com_reg_read32(edev, EFA_REGS_CAPS_OFF);
>> +
>> + if (!(stat & EFA_REGS_DEV_STS_READY_MASK)) {
>> + efa_err(edev->dmadev, "Device isn't ready, can't reset device\n");
>> + return -EINVAL;
>> + }
>> +
>> + timeout = (cap & EFA_REGS_CAPS_RESET_TIMEOUT_MASK) >>
>> + EFA_REGS_CAPS_RESET_TIMEOUT_SHIFT;
>> + if (!timeout) {
>> + efa_err(edev->dmadev, "Invalid timeout value\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* start reset */
>> + reset_val = EFA_REGS_DEV_CTL_DEV_RESET_MASK;
>> + reset_val |= (reset_reason << EFA_REGS_DEV_CTL_RESET_REASON_SHIFT) &
>> + EFA_REGS_DEV_CTL_RESET_REASON_MASK;
>> + writel(reset_val, edev->reg_bar + EFA_REGS_DEV_CTL_OFF);
>> +
>> + /* reset clears the mmio readless address, restore it */
>> + efa_com_mmio_reg_read_resp_addr_init(edev);
>> +
>> + err = wait_for_reset_state(edev, timeout,
>> + EFA_REGS_DEV_STS_RESET_IN_PROGRESS_MASK);
>> + if (err) {
>> + efa_err(edev->dmadev, "Reset indication didn't turn on\n");
>> + return err;
>> + }
>> +
>> + /* reset done */
>> + writel(0, edev->reg_bar + EFA_REGS_DEV_CTL_OFF);
>> + err = wait_for_reset_state(edev, timeout, 0);
>> + if (err) {
>> + efa_err(edev->dmadev, "Reset indication didn't turn off\n");
>> + return err;
>> + }
>> +
>> + timeout = (cap & EFA_REGS_CAPS_ADMIN_CMD_TO_MASK) >>
>> + EFA_REGS_CAPS_ADMIN_CMD_TO_SHIFT;
>> + if (timeout)
>> + /* the resolution of timeout reg is 100ms */
>> + edev->aq.completion_timeout = timeout * 100000;
>> + else
>> + edev->aq.completion_timeout = ADMIN_CMD_TIMEOUT_US;
>> +
>> + return 0;
>> +}
>
>
> Reviewed-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
Thanks Steve!
