[Intel-wired-lan] [PATCH net v1] iavf-linux: Fix reporting when setting descriptor count

[Paul Menzel]

Dear Anirudh,


Am 05.10.21 um 22:14 schrieb Venkataramanan, Anirudh:
> On Thu, 2021-09-23 at 09:44 +0200, Paul Menzel wrote:

>> Am 23.09.21 um 09:14 schrieb Michal Maloszewski:
>>> iavf_set_ringparams doesn't communicate to the user that
>>>
>>> 1. The user requested descriptor count is out of range. Instead it
>>>      just quietly sets descriptors to the "clamped" value and calls it
>>>      done. This makes it look an invalid value was successfully set as
>>>      the descriptor count when this isn't actually true.
>>>
>>> 2. The user provided descriptor count needs to be inflated for alignment
>>>      reasons.
>>>
>>> This behavior is confusing. The ice driver has already addressed
>>> this
>>
>> Please reference the commit.
> 
> That'd be commit fcea6f3da546 ("ice: Add stats and ethtool support").
> 
>>> by rejecting invalid values for and messaging for alignment
>>> adjustments.
>>
>> Values for what?
> 
> This should have been
> "by rejecting invalid values for descriptor count".
> 
>>> Do the same thing here.
>>
>> … by adding the error and info messages.
> 
> Not necessary IMHO, given that the commit message does explain the
> issue being addressed.

“reporting” can mean a lot and “messaging” is not clear to me either. If 
a v2 is sent, then “logging” might make it clear.

>>> Title: iavf-linux: Fix reporting when setting descriptor count
>>
>> This tag is not needed, is it?
> 
> Yeah, this is some extra commit metadata from our internal repo. This
> should have been removed before the patch was hit IWL.

> Please remove this line, and also change the title "iavf-linux: Fix
> reporting when setting descriptor count" to be "iavf: Fix reporting
> when setting descriptor count"


Kind regards,

Paul


>>> Fixes: 129cf89e5856 ("iavf: rename functions and structs to new name")
>>> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan at intel.com>
>>> Signed-off-by: Michal Maloszewski <michal.maloszewski at intel.com>
>>> ---
>>>    .../net/ethernet/intel/iavf/iavf_ethtool.c    | 43 ++++++++++++++-----
>>>    1 file changed, 32 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>> b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>> index 7cbe59bee..cbfc8d07a 100644
>>> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>> @@ -612,23 +612,44 @@ static int iavf_set_ringparam(struct
>>> net_device *netdev,
>>>          if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
>>>                  return -EINVAL;
>>>    
>>> -       new_tx_count = clamp_t(u32, ring->tx_pending,
>>> -                              IAVF_MIN_TXD,
>>> -                              IAVF_MAX_TXD);
>>> -       new_tx_count = ALIGN(new_tx_count,
>>> IAVF_REQ_DESCRIPTOR_MULTIPLE);
>>> +       if (ring->tx_pending > IAVF_MAX_TXD ||
>>> +           ring->tx_pending < IAVF_MIN_TXD ||
>>> +           ring->rx_pending > IAVF_MAX_RXD ||
>>> +           ring->rx_pending < IAVF_MIN_RXD) {
>>> +                 netdev_err(netdev, "Descriptors requested (Tx: %d
>>> / Rx: %d) out of range [%d-%d] (increment %d)\n",
>>> +                            ring->tx_pending, ring->rx_pending,
>>> IAVF_MIN_TXD,
>>> +                            IAVF_MAX_RXD,
>>> IAVF_REQ_DESCRIPTOR_MULTIPLE);
>>> +                 return -EINVAL;
>>> +       }
>>> +
>>> +       new_tx_count = ALIGN(ring->tx_pending,
>>> IAVF_REQ_DESCRIPTOR_MULTIPLE);
>>> +       if (new_tx_count != ring->tx_pending)
>>> +               netdev_info(netdev, "Requested Tx descriptor count
>>> rounded up to %d\n",
>>> +                           new_tx_count);
>>>    
>>> -       new_rx_count = clamp_t(u32, ring->rx_pending,
>>> -                              IAVF_MIN_RXD,
>>> -                              IAVF_MAX_RXD);
>>> -       new_rx_count = ALIGN(new_rx_count,
>>> IAVF_REQ_DESCRIPTOR_MULTIPLE);
>>> +       new_rx_count = ALIGN(ring->rx_pending,
>>> IAVF_REQ_DESCRIPTOR_MULTIPLE);
>>> +       if (new_rx_count != ring->rx_pending)
>>> +               netdev_info(netdev, "Requested Rx descriptor count
>>> rounded up to %d\n",
>>> +                           new_rx_count);
>>>    
>>>          /* if nothing to do return success */
>>>          if ((new_tx_count == adapter->tx_desc_count) &&
>>> -           (new_rx_count == adapter->rx_desc_count))
>>> +           (new_rx_count == adapter->rx_desc_count)) {
>>> +               netdev_dbg(netdev, "Nothing to change, descriptor
>>> count is same as requested\n");
>>>                  return 0;
>>> +       }
>>>    
>>> -       adapter->tx_desc_count = new_tx_count;
>>> -       adapter->rx_desc_count = new_rx_count;
>>> +       if (new_tx_count != adapter->tx_desc_count) {
>>> +               netdev_info(netdev, "Changing Tx descriptor count
>>> from %d to %d\n",
>>> +                           adapter->tx_desc_count, new_tx_count);
>>> +               adapter->tx_desc_count = new_tx_count;
>>> +       }
>>> +
>>> +       if (new_rx_count != adapter->rx_desc_count) {
>>> +               netdev_info(netdev, "Changing Rx descriptor count
>>> from %d to %d\n",
>>> +                           adapter->rx_desc_count, new_rx_count);
>>> +               adapter->rx_desc_count = new_rx_count;
>>> +       }
>>>    
>>>          if (netif_running(netdev)) {
>>>                  adapter->flags |= IAVF_FLAG_RESET_NEEDED;
>>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan at osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>