* [Intel-wired-lan] [PATCH net v1] iavf: Fix reporting when setting descriptor count
@ 2021-10-26 12:59 Michal Maloszewski
2021-11-26 14:11 ` Jankowski, Konrad0
0 siblings, 1 reply; 4+ messages in thread
From: Michal Maloszewski @ 2021-10-26 12:59 UTC (permalink / raw)
To: intel-wired-lan
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
by rejecting invalid values for descriptor count and
messaging for alignment adjustments.
Do the same thing here by adding the error and info messages.
Fixes: fbb7ddfef253 ("i40evf: core ethtool functionality")
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Signed-off-by: Michal Maloszewski <michal.maloszewski@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 30b125d6f5..9522bce3d9 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -623,23 +623,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_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_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 = 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;
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Intel-wired-lan] [PATCH net v1] iavf: Fix reporting when setting descriptor count
2021-10-26 12:59 [Intel-wired-lan] [PATCH net v1] iavf: Fix reporting when setting descriptor count Michal Maloszewski
@ 2021-11-26 14:11 ` Jankowski, Konrad0
0 siblings, 0 replies; 4+ messages in thread
From: Jankowski, Konrad0 @ 2021-11-26 14:11 UTC (permalink / raw)
To: intel-wired-lan
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Michal Maloszewski
> Sent: wtorek, 26 pa?dziernika 2021 14:59
> To: intel-wired-lan at lists.osuosl.org
> Cc: Maloszewski, Michal <michal.maloszewski@intel.com>
> Subject: [Intel-wired-lan] [PATCH net v1] iavf: Fix reporting when setting
> descriptor count
>
> 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 by
> rejecting invalid values for descriptor count and messaging for alignment
> adjustments.
> Do the same thing here by adding the error and info messages.
>
> Fixes: fbb7ddfef253 ("i40evf: core ethtool functionality")
> Signed-off-by: Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com>
> Signed-off-by: Michal Maloszewski <michal.maloszewski@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 30b125d6f5..9522bce3d9 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Intel-wired-lan] [PATCH net v1] iavf: Fix reporting when setting descriptor count
@ 2021-10-12 13:52 Michal Maloszewski
2021-10-12 22:10 ` Nguyen, Anthony L
0 siblings, 1 reply; 4+ messages in thread
From: Michal Maloszewski @ 2021-10-12 13:52 UTC (permalink / raw)
To: intel-wired-lan
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
by rejecting invalid values for descriptor count and messaging for alignment adjustments.
Do the same thing here by adding the error and info messages.
Fixes: fcea6f3da546 ("ice: Add stats and ethtool support")
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Signed-off-by: Michal Maloszewski <michal.maloszewski@intel.com>
---
v3: Commit with fixes tag changed.
---
.../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 7cbe59beee..cbfc8d07a0 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;
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Intel-wired-lan] [PATCH net v1] iavf: Fix reporting when setting descriptor count
2021-10-12 13:52 Michal Maloszewski
@ 2021-10-12 22:10 ` Nguyen, Anthony L
0 siblings, 0 replies; 4+ messages in thread
From: Nguyen, Anthony L @ 2021-10-12 22:10 UTC (permalink / raw)
To: intel-wired-lan
On Tue, 2021-10-12 at 13:52 +0000, Michal Maloszewski wrote:
> 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
> by rejecting invalid values for descriptor count and messaging for
> alignment adjustments.
> Do the same thing here by adding the error and info messages.
>
> Fixes: fcea6f3da546 ("ice: Add stats and ethtool support")
I believe the commit that Ani provided was referencing the "ice driver
has already addressed this by rejecting invalid values for descriptor
count"
I don't believe this the the correct Fixes for this patch.
> Signed-off-by: Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com>
> Signed-off-by: Michal Maloszewski <michal.maloszewski@intel.com>
> ---
> v3: Commit with fixes tag changed.
Your title has v1, but there's a v3 here.
Please use checkpatch before sending your patches:
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#92:
by rejecting invalid values for descriptor count and messaging for
alignment adjustments.
WARNING: suspect code indent for conditional statements (8, 18)
#116: FILE: drivers/net/ethernet/intel/iavf/iavf_ethtool.c:615:
+ if (ring->tx_pending > IAVF_MAX_TXD ||
[...]
+ netdev_err(netdev, "Descriptors requested (Tx: %d /
Rx: %d) out of range [%d-%d] (increment %d)\n",
WARNING: Statements should start on a tabstop
#123: FILE: drivers/net/ethernet/intel/iavf/iavf_ethtool.c:622:
+ return -EINVAL;
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-26 14:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-26 12:59 [Intel-wired-lan] [PATCH net v1] iavf: Fix reporting when setting descriptor count Michal Maloszewski
2021-11-26 14:11 ` Jankowski, Konrad0
-- strict thread matches above, loose matches on Subject: below --
2021-10-12 13:52 Michal Maloszewski
2021-10-12 22:10 ` Nguyen, Anthony L
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.