Re: [Intel-wired-lan] [PATCH net v2 3/4] iavf: send MAC change request synchronously

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

I believe this is a move in right direction,
please also find some more comments

>> +/**
>> + * iavf_poll_virtchnl_response - Poll admin queue for virtchnl response
>> + * @adapter: board private structure
>> + * @condition: callback to check if desired response received
>> + * @cond_data: context data passed to condition callback
>> + * @timeout_ms: maximum time to wait in milliseconds
>> + *
>> + * Polls admin queue and processes all messages until condition 
>> returns true
>> + * or timeout expires. Caller must hold netdev_lock. This can sleep 
>> for up to
>> + * timeout_ms while polling hardware.
>> + *
>> + * Returns 0 on success (condition met), -EAGAIN on timeout or error

kdoc requires "Return:"

>> + */
>> +static int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,

please move it to iavf_virtchnl.[ch], could be useful for other also,
not need to move it then and loose your authorship in default blame view

one more thing is that this could be perhaps integrated with existing
iavf_poll_virtchnl_msg(), which does not call
iavf_virtchnl_completion(), but likely could

>> +                       bool (*condition)(struct iavf_adapter *, void *),
>> +                       void *cond_data,
> 
> this could be const, then no cast on the callsite
> 
>> +                       unsigned int timeout_ms)
>> +{
>> +    struct iavf_hw *hw = &adapter->hw;
>> +    struct iavf_arq_event_info event;
>> +    enum virtchnl_ops v_op;
>> +    enum iavf_status v_ret;
>> +    unsigned long timeout;
>> +    int ret;
>> +
>> +    netdev_assert_locked(adapter->netdev);
>> +
>> +    event.buf_len = IAVF_MAX_AQ_BUF_SIZE;
>> +    event.msg_buf = kzalloc(event.buf_len, GFP_KERNEL);
>> +    if (!event.msg_buf)
>> +        return -ENOMEM;
>> +
>> +    timeout = jiffies + msecs_to_jiffies(timeout_ms);
>> +    while (time_before(jiffies, timeout)) {

please consider do-while (after the rest of changes could be better)

>> +        if (condition(adapter, cond_data)) {
> 
> if condition is met, but timed out, there should be no error
> 
>> +            ret = 0;
>> +            goto out;
>> +        }
>> +
>> +        ret = iavf_clean_arq_element(hw, &event, NULL);

instead of NULL pass "pending" param, if not zero you could omit
next sleep

>> +        if (!ret) {
>> +            v_op = (enum 
>> virtchnl_ops)le32_to_cpu(event.desc.cookie_high);
>> +            v_ret = (enum 
>> iavf_status)le32_to_cpu(event.desc.cookie_low);
>> +
>> +            iavf_virtchnl_completion(adapter, v_op, v_ret,
>> +                         event.msg_buf, event.msg_len);
>> +
>> +            memset(event.msg_buf, 0, IAVF_MAX_AQ_BUF_SIZE);
>> +        }
>> +
>> +        usleep_range(1000, 2000);

very old commit 9e3f23f44f32 ("i40e: reduce wait time for adminq command
completion") said that 50usec is right amount to sleep between checks

> 
> no sleep after message received (ok to do on empty queue)
> 
>> +    }
>> +
>> +    ret = -EAGAIN;
>> +out:
>> +    kfree(event.msg_buf);
>> +    return ret;
>> +}
>> +
>> +/**
>> + * iavf_mac_change_done - Check if MAC change completed
>> + * @adapter: board private structure
>> + * @data: MAC address being checked (as void *)
>> + *
>> + * Callback for iavf_poll_virtchnl_response() to check if MAC change 
>> completed.
>> + *
>> + * Returns true if MAC change completed, false otherwise
>> + */
>> +static bool iavf_mac_change_done(struct iavf_adapter *adapter, void 
>> *data)
>> +{
>> +    const u8 *addr = data;
>> +
>> +    return iavf_is_mac_set_handled(adapter->netdev, addr);
>> +}
>> +
>> +/**
>> + * iavf_set_mac_sync - Synchronously change MAC address
>> + * @adapter: board private structure
>> + * @addr: MAC address to set
>> + *
>> + * Sends MAC change request to PF and polls admin queue for response.
>> + * Caller must hold netdev_lock. This can sleep for up to 2.5 seconds.
>> + *
>> + * Returns 0 on success or error
>> + */
>> +static int iavf_set_mac_sync(struct iavf_adapter *adapter, const u8 
>> *addr)
>> +{
>> +    int ret;
>> +
>> +    netdev_assert_locked(adapter->netdev);
>> +
>> +    ret = iavf_add_ether_addrs(adapter);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return iavf_poll_virtchnl_response(adapter, iavf_mac_change_done,
>> +                       (void *)addr, 2500);
> 
> this function looks elegant, thank you
> 
> I'm a little affraid that this model (if applied to other things than
> setting MAC) will skip some of our "much needed" logic in the watchdog.
> 
> I have not thinked about much yet.
> 
> unrelated: callback looks elegant, but for virtchnl, it is almost always
> the case that we wait for some VC OPCODE to come back, and this is just
> a number. It could be easily coded as a callback too, passing wanted
> value masked in pointer, but I would say that just passing a normal u32
> param will be most clean
> 
>