[PATCH net-next 0/6] ionic updates

[PATCH net-next 0/6] ionic updates

[Shannon Nelson]

These updates are a bit of code cleaning and a minor
bit of performance tweaking.

Shannon Nelson (6):
  ionic: start queues before announcing link up
  ionic: check for link after netdev registration
  ionic: add lif quiesce
  ionic: batch rx buffer refilling
  ionic: use mc sync for multicast filters
  ionic: useful names for booleans

 .../net/ethernet/pensando/ionic/ionic_dev.h   |  4 +-
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 64 ++++++++++++++-----
 .../net/ethernet/pensando/ionic/ionic_lif.h   |  8 +++
 .../net/ethernet/pensando/ionic/ionic_txrx.c  | 18 +++---
 4 files changed, 70 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH net-next 1/6] ionic: start queues before announcing link up

[Shannon Nelson]

Change the order of operations in the link_up handling to be
sure that the queues are up and ready before we announce that
the link is up.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/ionic_lif.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index a12df3946a07..5457fb5d69ed 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -123,6 +123,12 @@ static void ionic_link_status_check(struct ionic_lif *lif)
 	link_up = link_status == IONIC_PORT_OPER_STATUS_UP;
 
 	if (link_up) {
+		if (lif->netdev->flags & IFF_UP && netif_running(lif->netdev)) {
+			mutex_lock(&lif->queue_lock);
+			ionic_start_queues(lif);
+			mutex_unlock(&lif->queue_lock);
+		}
+
 		if (!netif_carrier_ok(netdev)) {
 			u32 link_speed;
 
@@ -132,12 +138,6 @@ static void ionic_link_status_check(struct ionic_lif *lif)
 				    link_speed / 1000);
 			netif_carrier_on(netdev);
 		}
-
-		if (lif->netdev->flags & IFF_UP && netif_running(lif->netdev)) {
-			mutex_lock(&lif->queue_lock);
-			ionic_start_queues(lif);
-			mutex_unlock(&lif->queue_lock);
-		}
 	} else {
 		if (netif_carrier_ok(netdev)) {
 			netdev_info(netdev, "Link down\n");
-- 
2.17.1

[PATCH net-next 2/6] ionic: check for link after netdev registration

[Shannon Nelson]

Request a link check as soon as the netdev is registered rather
than waiting for the watchdog to go off in order to get the
interface operational a little more quickly.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/ionic_lif.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 5457fb5d69ed..519d544821af 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -2959,6 +2959,8 @@ int ionic_lif_register(struct ionic_lif *lif)
 		dev_err(lif->ionic->dev, "Cannot register net device, aborting\n");
 		return err;
 	}
+
+	ionic_link_status_check_request(lif, true);
 	lif->registered = true;
 	ionic_lif_set_netdev_info(lif);
 
-- 
2.17.1

[PATCH net-next 3/6] ionic: add lif quiesce

[Shannon Nelson]

After the queues are stopped, expressly quiesce the lif.
This assures that even if the queues were in an odd state,
the firmware will close up everything cleanly.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 519d544821af..28044240caf2 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -1625,6 +1625,28 @@ static void ionic_lif_rss_deinit(struct ionic_lif *lif)
 	ionic_lif_rss_config(lif, 0x0, NULL, NULL);
 }
 
+static int ionic_lif_quiesce(struct ionic_lif *lif)
+{
+	struct ionic_admin_ctx ctx = {
+		.work = COMPLETION_INITIALIZER_ONSTACK(ctx.work),
+		.cmd.lif_setattr = {
+			.opcode = IONIC_CMD_LIF_SETATTR,
+			.index = cpu_to_le16(lif->index),
+			.attr = IONIC_LIF_ATTR_STATE,
+			.state = IONIC_LIF_QUIESCE,
+		},
+	};
+	int err;
+
+	err = ionic_adminq_post_wait(lif, &ctx);
+	if (err) {
+		netdev_err(lif->netdev, "lif quiesce failed %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
 static void ionic_txrx_disable(struct ionic_lif *lif)
 {
 	unsigned int i;
@@ -1639,6 +1661,8 @@ static void ionic_txrx_disable(struct ionic_lif *lif)
 		for (i = 0; i < lif->nxqs; i++)
 			err = ionic_qcq_disable(lif->rxqcqs[i], (err != -ETIMEDOUT));
 	}
+
+	ionic_lif_quiesce(lif);
 }
 
 static void ionic_txrx_deinit(struct ionic_lif *lif)
-- 
2.17.1

Re: [PATCH net-next 3/6] ionic: add lif quiesce

[Saeed Mahameed]

On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
> After the queues are stopped, expressly quiesce the lif.
> This assures that even if the queues were in an odd state,
> the firmware will close up everything cleanly.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  .../net/ethernet/pensando/ionic/ionic_lif.c   | 24
> +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 519d544821af..28044240caf2 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -1625,6 +1625,28 @@ static void ionic_lif_rss_deinit(struct
> ionic_lif *lif)
>  	ionic_lif_rss_config(lif, 0x0, NULL, NULL);
>  }
>  
> +static int ionic_lif_quiesce(struct ionic_lif *lif)
> +{
> +	struct ionic_admin_ctx ctx = {
> +		.work = COMPLETION_INITIALIZER_ONSTACK(ctx.work),
> +		.cmd.lif_setattr = {
> +			.opcode = IONIC_CMD_LIF_SETATTR,
> +			.index = cpu_to_le16(lif->index),
> +			.attr = IONIC_LIF_ATTR_STATE,
> +			.state = IONIC_LIF_QUIESCE,
> +		},
> +	};
> +	int err;
> +
> +	err = ionic_adminq_post_wait(lif, &ctx);
> +	if (err) {
> +		netdev_err(lif->netdev, "lif quiesce failed %d\n",
> err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
>  static void ionic_txrx_disable(struct ionic_lif *lif)
>  {
>  	unsigned int i;
> @@ -1639,6 +1661,8 @@ static void ionic_txrx_disable(struct ionic_lif
> *lif)
>  		for (i = 0; i < lif->nxqs; i++)
>  			err = ionic_qcq_disable(lif->rxqcqs[i], (err !=
> -ETIMEDOUT));
>  	}
> +
> +	ionic_lif_quiesce(lif);

if you don't care about return value just void out the function retval

Re: [PATCH net-next 3/6] ionic: add lif quiesce

[Shannon Nelson]

On 11/4/20 4:50 PM, Saeed Mahameed wrote:
> On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
>> After the queues are stopped, expressly quiesce the lif.
>> This assures that even if the queues were in an odd state,
>> the firmware will close up everything cleanly.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
>>   .../net/ethernet/pensando/ionic/ionic_lif.c   | 24
>> +++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> index 519d544821af..28044240caf2 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> @@ -1625,6 +1625,28 @@ static void ionic_lif_rss_deinit(struct
>> ionic_lif *lif)
>>   	ionic_lif_rss_config(lif, 0x0, NULL, NULL);
>>   }
>>   
>> +static int ionic_lif_quiesce(struct ionic_lif *lif)
>> +{
>> +	struct ionic_admin_ctx ctx = {
>> +		.work = COMPLETION_INITIALIZER_ONSTACK(ctx.work),
>> +		.cmd.lif_setattr = {
>> +			.opcode = IONIC_CMD_LIF_SETATTR,
>> +			.index = cpu_to_le16(lif->index),
>> +			.attr = IONIC_LIF_ATTR_STATE,
>> +			.state = IONIC_LIF_QUIESCE,
>> +		},
>> +	};
>> +	int err;
>> +
>> +	err = ionic_adminq_post_wait(lif, &ctx);
>> +	if (err) {
>> +		netdev_err(lif->netdev, "lif quiesce failed %d\n",
>> err);
>> +		return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static void ionic_txrx_disable(struct ionic_lif *lif)
>>   {
>>   	unsigned int i;
>> @@ -1639,6 +1661,8 @@ static void ionic_txrx_disable(struct ionic_lif
>> *lif)
>>   		for (i = 0; i < lif->nxqs; i++)
>>   			err = ionic_qcq_disable(lif->rxqcqs[i], (err !=
>> -ETIMEDOUT));
>>   	}
>> +
>> +	ionic_lif_quiesce(lif);
> if you don't care about return value just void out the function retval
>

Sure.
sln

[PATCH net-next 4/6] ionic: batch rx buffer refilling

[Shannon Nelson]

We don't need to refill the rx descriptors on every napi
if only a few were handled.  Waiting until we can batch up
a few together will save us a few Rx cycles.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 .../net/ethernet/pensando/ionic/ionic_dev.h    |  4 +++-
 .../net/ethernet/pensando/ionic/ionic_txrx.c   | 18 ++++++++++--------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
index 6c243b17312c..9064222a087a 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
@@ -12,8 +12,10 @@
 
 #define IONIC_MAX_TX_DESC		8192
 #define IONIC_MAX_RX_DESC		16384
-#define IONIC_MIN_TXRX_DESC		16
+#define IONIC_MIN_TXRX_DESC		64
 #define IONIC_DEF_TXRX_DESC		4096
+#define IONIC_RX_FILL_THRESHOLD		64
+#define IONIC_RX_FILL_DIV		8
 #define IONIC_LIFS_MAX			1024
 #define IONIC_WATCHDOG_SECS		5
 #define IONIC_ITR_COAL_USEC_DEFAULT	64
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index b3d2250c77d0..9156c9825a16 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -392,11 +392,6 @@ void ionic_rx_fill(struct ionic_queue *q)
 			 q->dbval | q->head_idx);
 }
 
-static void ionic_rx_fill_cb(void *arg)
-{
-	ionic_rx_fill(arg);
-}
-
 void ionic_rx_empty(struct ionic_queue *q)
 {
 	struct ionic_desc_info *desc_info;
@@ -480,6 +475,7 @@ int ionic_rx_napi(struct napi_struct *napi, int budget)
 	struct ionic_cq *cq = napi_to_cq(napi);
 	struct ionic_dev *idev;
 	struct ionic_lif *lif;
+	u16 rx_fill_threshold;
 	u32 work_done = 0;
 	u32 flags = 0;
 
@@ -489,7 +485,9 @@ int ionic_rx_napi(struct napi_struct *napi, int budget)
 	work_done = ionic_cq_service(cq, budget,
 				     ionic_rx_service, NULL, NULL);
 
-	if (work_done)
+	rx_fill_threshold = min_t(u16, IONIC_RX_FILL_THRESHOLD,
+				  cq->num_descs / IONIC_RX_FILL_DIV);
+	if (work_done && ionic_q_space_avail(cq->bound_q) >= rx_fill_threshold)
 		ionic_rx_fill(cq->bound_q);
 
 	if (work_done < budget && napi_complete_done(napi, work_done)) {
@@ -518,6 +516,7 @@ int ionic_txrx_napi(struct napi_struct *napi, int budget)
 	struct ionic_dev *idev;
 	struct ionic_lif *lif;
 	struct ionic_cq *txcq;
+	u16 rx_fill_threshold;
 	u32 rx_work_done = 0;
 	u32 tx_work_done = 0;
 	u32 flags = 0;
@@ -531,8 +530,11 @@ int ionic_txrx_napi(struct napi_struct *napi, int budget)
 
 	rx_work_done = ionic_cq_service(rxcq, budget,
 					ionic_rx_service, NULL, NULL);
-	if (rx_work_done)
-		ionic_rx_fill_cb(rxcq->bound_q);
+
+	rx_fill_threshold = min_t(u16, IONIC_RX_FILL_THRESHOLD,
+				  rxcq->num_descs / IONIC_RX_FILL_DIV);
+	if (rx_work_done && ionic_q_space_avail(rxcq->bound_q) >= rx_fill_threshold)
+		ionic_rx_fill(rxcq->bound_q);
 
 	if (rx_work_done < budget && napi_complete_done(napi, rx_work_done)) {
 		ionic_dim_update(qcq);
-- 
2.17.1

Re: [PATCH net-next 4/6] ionic: batch rx buffer refilling

[Saeed Mahameed]

On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
> We don't need to refill the rx descriptors on every napi
> if only a few were handled.  Waiting until we can batch up
> a few together will save us a few Rx cycles.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  .../net/ethernet/pensando/ionic/ionic_dev.h    |  4 +++-
>  .../net/ethernet/pensando/ionic/ionic_txrx.c   | 18 ++++++++++----
> ----
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> index 6c243b17312c..9064222a087a 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> @@ -12,8 +12,10 @@
>  
>  #define IONIC_MAX_TX_DESC		8192
>  #define IONIC_MAX_RX_DESC		16384
> -#define IONIC_MIN_TXRX_DESC		16
> +#define IONIC_MIN_TXRX_DESC		64
>  #define IONIC_DEF_TXRX_DESC		4096
> +#define IONIC_RX_FILL_THRESHOLD		64

isn't 64 a bit high ? 64 is the default napi budget 

Many drivers do this with bulks of 8/16 but I couldn't find any with
higher numbers.

also, just for a reference, GRO and XDP they bulk up to 8. but not
more.

IMHO i think you need to relax the re-fill threshold a bit.

> +#define IONIC_RX_FILL_DIV		8
> 
...

> -	if (work_done)
> +	rx_fill_threshold = min_t(u16, IONIC_RX_FILL_THRESHOLD,
> +				  cq->num_descs / IONIC_RX_FILL_DIV);
> +	if (work_done && ionic_q_space_avail(cq->bound_q) >=
> rx_fill_threshold)
>  		ionic_rx_fill(cq->bound_q);
>  

Re: [PATCH net-next 4/6] ionic: batch rx buffer refilling

[Shannon Nelson]

On 11/4/20 5:08 PM, Saeed Mahameed wrote:
> On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
>> We don't need to refill the rx descriptors on every napi
>> if only a few were handled.  Waiting until we can batch up
>> a few together will save us a few Rx cycles.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
>>   .../net/ethernet/pensando/ionic/ionic_dev.h    |  4 +++-
>>   .../net/ethernet/pensando/ionic/ionic_txrx.c   | 18 ++++++++++----
>> ----
>>   2 files changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>> b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>> index 6c243b17312c..9064222a087a 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>> @@ -12,8 +12,10 @@
>>   
>>   #define IONIC_MAX_TX_DESC		8192
>>   #define IONIC_MAX_RX_DESC		16384
>> -#define IONIC_MIN_TXRX_DESC		16
>> +#define IONIC_MIN_TXRX_DESC		64
>>   #define IONIC_DEF_TXRX_DESC		4096
>> +#define IONIC_RX_FILL_THRESHOLD		64
> isn't 64 a bit high ? 64 is the default napi budget
>
> Many drivers do this with bulks of 8/16 but I couldn't find any with
> higher numbers.
>
> also, just for a reference, GRO and XDP they bulk up to 8. but not
> more.
>
> IMHO i think you need to relax the re-fill threshold a bit.

Yeah, the work being done internally on tuning has been a bit 
aggressive, I can dial this back a bit.
sln

>
>> +#define IONIC_RX_FILL_DIV		8
>>
> ...
>
>> -	if (work_done)
>> +	rx_fill_threshold = min_t(u16, IONIC_RX_FILL_THRESHOLD,
>> +				  cq->num_descs / IONIC_RX_FILL_DIV);
>> +	if (work_done && ionic_q_space_avail(cq->bound_q) >=
>> rx_fill_threshold)
>>   		ionic_rx_fill(cq->bound_q);
>>   
>

[PATCH net-next 5/6] ionic: use mc sync for multicast filters

[Shannon Nelson]

We should be using the multicast sync routines for the
multicast filters.

Fixes: 1800eee16676 ("net: ionic: Replace in_interrupt() usage.")
Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/ionic_lif.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 28044240caf2..a58bb572b23b 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -1158,6 +1158,14 @@ static void ionic_dev_uc_sync(struct net_device *netdev, bool from_ndo)
 
 }
 
+static void ionic_dev_mc_sync(struct net_device *netdev, bool from_ndo)
+{
+	if (from_ndo)
+		__dev_mc_sync(netdev, ionic_ndo_addr_add, ionic_ndo_addr_del);
+	else
+		__dev_mc_sync(netdev, ionic_addr_add, ionic_addr_del);
+}
+
 static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo)
 {
 	struct ionic_lif *lif = netdev_priv(netdev);
@@ -1189,7 +1197,7 @@ static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo)
 	}
 
 	/* same for multicast */
-	ionic_dev_uc_sync(netdev, from_ndo);
+	ionic_dev_mc_sync(netdev, from_ndo);
 	nfilters = le32_to_cpu(lif->identity->eth.max_mcast_filters);
 	if (netdev_mc_count(netdev) > nfilters) {
 		rx_mode |= IONIC_RX_MODE_F_ALLMULTI;
-- 
2.17.1

Re: [PATCH net-next 5/6] ionic: use mc sync for multicast filters

[Saeed Mahameed]

On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
> We should be using the multicast sync routines for the
> multicast filters.
> 
> Fixes: 1800eee16676 ("net: ionic: Replace in_interrupt() usage.")
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_lif.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 28044240caf2..a58bb572b23b 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -1158,6 +1158,14 @@ static void ionic_dev_uc_sync(struct
> net_device *netdev, bool from_ndo)
>  
>  }
>  
> +static void ionic_dev_mc_sync(struct net_device *netdev, bool
> from_ndo)
> +{
> +	if (from_ndo)
> +		__dev_mc_sync(netdev, ionic_ndo_addr_add,
> ionic_ndo_addr_del);
> +	else
> +		__dev_mc_sync(netdev, ionic_addr_add, ionic_addr_del);
> +}
> +

I don't see any point of this function since it is used in one place.
just unfold it in the caller and you will endup with less code.

also keep in mind passing boolean to functions is usually a bad idea, 
and only complicates things, keep things simple and explicit, let the
caller do what is necessary to be done, so if you must do this if
condition, do it at the caller level.

and for a future patch i strongly recommend to remove this from_ndo
flag, it is really straight forward to do for this function
1) you can just pass _addr_add/del function pointers directly
to ionic_set_rx_mode
2) remove _ionic_lif_rx_mode from ionic_set_rx_mode and unfold it in
the caller since the function is basically one giant if condition which
is called only from two places.

>  static void ionic_set_rx_mode(struct net_device *netdev, bool
> from_ndo)
>  {
>  	struct ionic_lif *lif = netdev_priv(netdev);
> @@ -1189,7 +1197,7 @@ static void ionic_set_rx_mode(struct net_device
> *netdev, bool from_ndo)
>  	}
>  
>  	/* same for multicast */
> -	ionic_dev_uc_sync(netdev, from_ndo);
> +	ionic_dev_mc_sync(netdev, from_ndo);
>  	nfilters = le32_to_cpu(lif->identity->eth.max_mcast_filters);
>  	if (netdev_mc_count(netdev) > nfilters) {
>  		rx_mode |= IONIC_RX_MODE_F_ALLMULTI;

Re: [PATCH net-next 5/6] ionic: use mc sync for multicast filters

[Shannon Nelson]

On 11/4/20 5:18 PM, Saeed Mahameed wrote:
> On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
>> We should be using the multicast sync routines for the
>> multicast filters.
>>
>> Fixes: 1800eee16676 ("net: ionic: Replace in_interrupt() usage.")
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
>>   drivers/net/ethernet/pensando/ionic/ionic_lif.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> index 28044240caf2..a58bb572b23b 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> @@ -1158,6 +1158,14 @@ static void ionic_dev_uc_sync(struct
>> net_device *netdev, bool from_ndo)
>>   
>>   }
>>   
>> +static void ionic_dev_mc_sync(struct net_device *netdev, bool
>> from_ndo)
>> +{
>> +	if (from_ndo)
>> +		__dev_mc_sync(netdev, ionic_ndo_addr_add,
>> ionic_ndo_addr_del);
>> +	else
>> +		__dev_mc_sync(netdev, ionic_addr_add, ionic_addr_del);
>> +}
>> +
> I don't see any point of this function since it is used in one place.
> just unfold it in the caller and you will endup with less code.
>
> also keep in mind passing boolean to functions is usually a bad idea,
> and only complicates things, keep things simple and explicit, let the
> caller do what is necessary to be done, so if you must do this if
> condition, do it at the caller level.
>
> and for a future patch i strongly recommend to remove this from_ndo
> flag, it is really straight forward to do for this function
> 1) you can just pass _addr_add/del function pointers directly
> to ionic_set_rx_mode
> 2) remove _ionic_lif_rx_mode from ionic_set_rx_mode and unfold it in
> the caller since the function is basically one giant if condition which
> is called only from two places.

This was specifically following work that was done a couple of weeks ago 
by Thomas Gleixner et al to clean up questionable uses of 
in_interrupt(), similar to how they used booleans to patch mlx5 and 
other drivers.  They split this out, but later I noticed this issue with 
how multicast got handled.  I agree, I'm not thrilled with the new 
booleans either, which is part of the reason for patch 6/6 in this series.

Yes, I can pull these back into ionic_set_rx_mode() for a v2 patch, 
which will clean up some of this.

I'll look at those future patch ideas: (2) is easy enough and I might 
just add it to this patch series, but I'm not sure about (1) yet, partly 
because I like the current separation of knowledge.

Thanks,
sln

>
>>   static void ionic_set_rx_mode(struct net_device *netdev, bool
>> from_ndo)
>>   {
>>   	struct ionic_lif *lif = netdev_priv(netdev);
>> @@ -1189,7 +1197,7 @@ static void ionic_set_rx_mode(struct net_device
>> *netdev, bool from_ndo)
>>   	}
>>   
>>   	/* same for multicast */
>> -	ionic_dev_uc_sync(netdev, from_ndo);
>> +	ionic_dev_mc_sync(netdev, from_ndo);
>>   	nfilters = le32_to_cpu(lif->identity->eth.max_mcast_filters);
>>   	if (netdev_mc_count(netdev) > nfilters) {
>>   		rx_mode |= IONIC_RX_MODE_F_ALLMULTI;

[PATCH net-next 6/6] ionic: useful names for booleans

[Shannon Nelson]

With a few more uses of true and false in function calls, we
need to give them some useful names so we can tell from the
calling point what we're doing.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/ionic_lif.c | 16 ++++++++--------
 drivers/net/ethernet/pensando/ionic/ionic_lif.h |  8 ++++++++
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index a58bb572b23b..a0d2989a0d8d 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -1074,22 +1074,22 @@ static int ionic_lif_addr(struct ionic_lif *lif, const u8 *addr, bool add,
 
 static int ionic_addr_add(struct net_device *netdev, const u8 *addr)
 {
-	return ionic_lif_addr(netdev_priv(netdev), addr, true, true);
+	return ionic_lif_addr(netdev_priv(netdev), addr, ADD_ADDR, CAN_SLEEP);
 }
 
 static int ionic_ndo_addr_add(struct net_device *netdev, const u8 *addr)
 {
-	return ionic_lif_addr(netdev_priv(netdev), addr, true, false);
+	return ionic_lif_addr(netdev_priv(netdev), addr, ADD_ADDR, CAN_NOT_SLEEP);
 }
 
 static int ionic_addr_del(struct net_device *netdev, const u8 *addr)
 {
-	return ionic_lif_addr(netdev_priv(netdev), addr, false, true);
+	return ionic_lif_addr(netdev_priv(netdev), addr, DEL_ADDR, CAN_SLEEP);
 }
 
 static int ionic_ndo_addr_del(struct net_device *netdev, const u8 *addr)
 {
-	return ionic_lif_addr(netdev_priv(netdev), addr, false, false);
+	return ionic_lif_addr(netdev_priv(netdev), addr, DEL_ADDR, CAN_NOT_SLEEP);
 }
 
 static void ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode)
@@ -1214,7 +1214,7 @@ static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo)
 
 static void ionic_ndo_set_rx_mode(struct net_device *netdev)
 {
-	ionic_set_rx_mode(netdev, true);
+	ionic_set_rx_mode(netdev, FROM_NDO);
 }
 
 static __le64 ionic_netdev_features_to_nic(netdev_features_t features)
@@ -1805,7 +1805,7 @@ static int ionic_txrx_init(struct ionic_lif *lif)
 	if (lif->netdev->features & NETIF_F_RXHASH)
 		ionic_lif_rss_init(lif);
 
-	ionic_set_rx_mode(lif->netdev, false);
+	ionic_set_rx_mode(lif->netdev, NOT_FROM_NDO);
 
 	return 0;
 
@@ -2813,7 +2813,7 @@ static int ionic_station_set(struct ionic_lif *lif)
 		 */
 		if (!ether_addr_equal(ctx.comp.lif_getattr.mac,
 				      netdev->dev_addr))
-			ionic_lif_addr(lif, netdev->dev_addr, true, true);
+			ionic_lif_addr(lif, netdev->dev_addr, ADD_ADDR, CAN_SLEEP);
 	} else {
 		/* Update the netdev mac with the device's mac */
 		memcpy(addr.sa_data, ctx.comp.lif_getattr.mac, netdev->addr_len);
@@ -2830,7 +2830,7 @@ static int ionic_station_set(struct ionic_lif *lif)
 
 	netdev_dbg(lif->netdev, "adding station MAC addr %pM\n",
 		   netdev->dev_addr);
-	ionic_lif_addr(lif, netdev->dev_addr, true, true);
+	ionic_lif_addr(lif, netdev->dev_addr, ADD_ADDR, CAN_SLEEP);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index 0224dfd24b8a..493de679b498 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -13,6 +13,14 @@
 
 #define IONIC_MAX_NUM_NAPI_CNTR		(NAPI_POLL_WEIGHT + 1)
 #define IONIC_MAX_NUM_SG_CNTR		(IONIC_TX_MAX_SG_ELEMS + 1)
+
+#define ADD_ADDR	true
+#define DEL_ADDR	false
+#define CAN_SLEEP	true
+#define CAN_NOT_SLEEP	false
+#define FROM_NDO	true
+#define NOT_FROM_NDO	false
+
 #define IONIC_RX_COPYBREAK_DEFAULT	256
 #define IONIC_TX_BUDGET_DEFAULT		256
 
-- 
2.17.1

Re: [PATCH net-next 6/6] ionic: useful names for booleans

[Saeed Mahameed]

On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
> With a few more uses of true and false in function calls, we
> need to give them some useful names so we can tell from the
> calling point what we're doing.
> 

Aha! The root cause of the issue is passing booleans to functions in
first place, it is usually a bad idea that could lead to complication
and overloading the design for no reason, please see my suggestion in
the previous patch maybe you can apply the same approach on some of the
booleans below.


> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_lif.c | 16 ++++++++-------
> -
>  drivers/net/ethernet/pensando/ionic/ionic_lif.h |  8 ++++++++
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index a58bb572b23b..a0d2989a0d8d 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -1074,22 +1074,22 @@ static int ionic_lif_addr(struct ionic_lif
> *lif, const u8 *addr, bool add,
>  
>  static int ionic_addr_add(struct net_device *netdev, const u8 *addr)
>  {
> -	return ionic_lif_addr(netdev_priv(netdev), addr, true, true);
> +	return ionic_lif_addr(netdev_priv(netdev), addr, ADD_ADDR,
> CAN_SLEEP);
>  }
>  
>  static int ionic_ndo_addr_add(struct net_device *netdev, const u8
> *addr)
>  {
> -	return ionic_lif_addr(netdev_priv(netdev), addr, true, false);
> +	return ionic_lif_addr(netdev_priv(netdev), addr, ADD_ADDR,
> CAN_NOT_SLEEP);
>  }
>  
>  static int ionic_addr_del(struct net_device *netdev, const u8 *addr)
>  {
> -	return ionic_lif_addr(netdev_priv(netdev), addr, false, true);
> +	return ionic_lif_addr(netdev_priv(netdev), addr, DEL_ADDR,
> CAN_SLEEP);
>  }
>  
>  static int ionic_ndo_addr_del(struct net_device *netdev, const u8
> *addr)
>  {
> -	return ionic_lif_addr(netdev_priv(netdev), addr, false, false);
> +	return ionic_lif_addr(netdev_priv(netdev), addr, DEL_ADDR,
> CAN_NOT_SLEEP);
>  }
>  
>  static void ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int
> rx_mode)
> @@ -1214,7 +1214,7 @@ static void ionic_set_rx_mode(struct net_device
> *netdev, bool from_ndo)
>  
>  static void ionic_ndo_set_rx_mode(struct net_device *netdev)
>  {
> -	ionic_set_rx_mode(netdev, true);
> +	ionic_set_rx_mode(netdev, FROM_NDO);
>  }
>  
>  static __le64 ionic_netdev_features_to_nic(netdev_features_t
> features)
> @@ -1805,7 +1805,7 @@ static int ionic_txrx_init(struct ionic_lif
> *lif)
>  	if (lif->netdev->features & NETIF_F_RXHASH)
>  		ionic_lif_rss_init(lif);
>  
> -	ionic_set_rx_mode(lif->netdev, false);
> +	ionic_set_rx_mode(lif->netdev, NOT_FROM_NDO);
>  
>  	return 0;
>  
> @@ -2813,7 +2813,7 @@ static int ionic_station_set(struct ionic_lif
> *lif)
>  		 */
>  		if (!ether_addr_equal(ctx.comp.lif_getattr.mac,
>  				      netdev->dev_addr))
> -			ionic_lif_addr(lif, netdev->dev_addr, true,
> true);
> +			ionic_lif_addr(lif, netdev->dev_addr, ADD_ADDR,
> CAN_SLEEP);
>  	} else {
>  		/* Update the netdev mac with the device's mac */
>  		memcpy(addr.sa_data, ctx.comp.lif_getattr.mac, netdev-
> >addr_len);
> @@ -2830,7 +2830,7 @@ static int ionic_station_set(struct ionic_lif
> *lif)
>  
>  	netdev_dbg(lif->netdev, "adding station MAC addr %pM\n",
>  		   netdev->dev_addr);
> -	ionic_lif_addr(lif, netdev->dev_addr, true, true);
> +	ionic_lif_addr(lif, netdev->dev_addr, ADD_ADDR, CAN_SLEEP);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> index 0224dfd24b8a..493de679b498 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> @@ -13,6 +13,14 @@
>  
>  #define IONIC_MAX_NUM_NAPI_CNTR		(NAPI_POLL_WEIGHT + 1)
>  #define IONIC_MAX_NUM_SG_CNTR		(IONIC_TX_MAX_SG_ELEMS
> + 1)
> +
> +#define ADD_ADDR	true
> +#define DEL_ADDR	false
> +#define CAN_SLEEP	true
> +#define CAN_NOT_SLEEP	false
> +#define FROM_NDO	true
> +#define NOT_FROM_NDO	false
> +
>  #define IONIC_RX_COPYBREAK_DEFAULT	256
>  #define IONIC_TX_BUDGET_DEFAULT		256
>  

Re: [PATCH net-next 6/6] ionic: useful names for booleans

[Shannon Nelson]

On 11/4/20 5:21 PM, Saeed Mahameed wrote:
> On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
>> With a few more uses of true and false in function calls, we
>> need to give them some useful names so we can tell from the
>> calling point what we're doing.
>>
> Aha! The root cause of the issue is passing booleans to functions in
> first place, it is usually a bad idea that could lead to complication
> and overloading the design for no reason, please see my suggestion in
> the previous patch maybe you can apply the same approach on some of the
> booleans below.

Yes, this is similar to what I was commenting on when I responded to the 
in_interrupt() patches.  However, when the code around Add and Delete is 
identicle and called from multiple places, it is handy to put it in one 
place with a boolean so as to not have multiple instances to maintain.  
Yes, it's a tradeoff, and these #defines are meant to help ease the 
readability.

The v2 for patch 5/6 in this patchset might help this a little, I'll see 
what I can do.

Thanks for you time in looking through these patches.

Cheers,
sln


>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
>>   drivers/net/ethernet/pensando/ionic/ionic_lif.c | 16 ++++++++-------
>> -
>>   drivers/net/ethernet/pensando/ionic/ionic_lif.h |  8 ++++++++
>>   2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> index a58bb572b23b..a0d2989a0d8d 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> @@ -1074,22 +1074,22 @@ static int ionic_lif_addr(struct ionic_lif
>> *lif, const u8 *addr, bool add,
>>   
>>   static int ionic_addr_add(struct net_device *netdev, const u8 *addr)
>>   {
>> -	return ionic_lif_addr(netdev_priv(netdev), addr, true, true);
>> +	return ionic_lif_addr(netdev_priv(netdev), addr, ADD_ADDR,
>> CAN_SLEEP);
>>   }
>>   
>>   static int ionic_ndo_addr_add(struct net_device *netdev, const u8
>> *addr)
>>   {
>> -	return ionic_lif_addr(netdev_priv(netdev), addr, true, false);
>> +	return ionic_lif_addr(netdev_priv(netdev), addr, ADD_ADDR,
>> CAN_NOT_SLEEP);
>>   }
>>   
>>   static int ionic_addr_del(struct net_device *netdev, const u8 *addr)
>>   {
>> -	return ionic_lif_addr(netdev_priv(netdev), addr, false, true);
>> +	return ionic_lif_addr(netdev_priv(netdev), addr, DEL_ADDR,
>> CAN_SLEEP);
>>   }
>>   
>>   static int ionic_ndo_addr_del(struct net_device *netdev, const u8
>> *addr)
>>   {
>> -	return ionic_lif_addr(netdev_priv(netdev), addr, false, false);
>> +	return ionic_lif_addr(netdev_priv(netdev), addr, DEL_ADDR,
>> CAN_NOT_SLEEP);
>>   }
>>   
>>   static void ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int
>> rx_mode)
>> @@ -1214,7 +1214,7 @@ static void ionic_set_rx_mode(struct net_device
>> *netdev, bool from_ndo)
>>   
>>   static void ionic_ndo_set_rx_mode(struct net_device *netdev)
>>   {
>> -	ionic_set_rx_mode(netdev, true);
>> +	ionic_set_rx_mode(netdev, FROM_NDO);
>>   }
>>   
>>   static __le64 ionic_netdev_features_to_nic(netdev_features_t
>> features)
>> @@ -1805,7 +1805,7 @@ static int ionic_txrx_init(struct ionic_lif
>> *lif)
>>   	if (lif->netdev->features & NETIF_F_RXHASH)
>>   		ionic_lif_rss_init(lif);
>>   
>> -	ionic_set_rx_mode(lif->netdev, false);
>> +	ionic_set_rx_mode(lif->netdev, NOT_FROM_NDO);
>>   
>>   	return 0;
>>   
>> @@ -2813,7 +2813,7 @@ static int ionic_station_set(struct ionic_lif
>> *lif)
>>   		 */
>>   		if (!ether_addr_equal(ctx.comp.lif_getattr.mac,
>>   				      netdev->dev_addr))
>> -			ionic_lif_addr(lif, netdev->dev_addr, true,
>> true);
>> +			ionic_lif_addr(lif, netdev->dev_addr, ADD_ADDR,
>> CAN_SLEEP);
>>   	} else {
>>   		/* Update the netdev mac with the device's mac */
>>   		memcpy(addr.sa_data, ctx.comp.lif_getattr.mac, netdev-
>>> addr_len);
>> @@ -2830,7 +2830,7 @@ static int ionic_station_set(struct ionic_lif
>> *lif)
>>   
>>   	netdev_dbg(lif->netdev, "adding station MAC addr %pM\n",
>>   		   netdev->dev_addr);
>> -	ionic_lif_addr(lif, netdev->dev_addr, true, true);
>> +	ionic_lif_addr(lif, netdev->dev_addr, ADD_ADDR, CAN_SLEEP);
>>   
>>   	return 0;
>>   }
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> index 0224dfd24b8a..493de679b498 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> @@ -13,6 +13,14 @@
>>   
>>   #define IONIC_MAX_NUM_NAPI_CNTR		(NAPI_POLL_WEIGHT + 1)
>>   #define IONIC_MAX_NUM_SG_CNTR		(IONIC_TX_MAX_SG_ELEMS
>> + 1)
>> +
>> +#define ADD_ADDR	true
>> +#define DEL_ADDR	false
>> +#define CAN_SLEEP	true
>> +#define CAN_NOT_SLEEP	false
>> +#define FROM_NDO	true
>> +#define NOT_FROM_NDO	false
>> +
>>   #define IONIC_RX_COPYBREAK_DEFAULT	256
>>   #define IONIC_TX_BUDGET_DEFAULT		256
>>