[PATCH v2 net-next 0/4] ionic: driver updates

[PATCH v2 net-next 0/4] ionic: driver updates

[Shannon Nelson]

These are a few little updates for the ionic network driver.

v2: dropped IBM msi patch
    added fix for a compiler warning

Shannon Nelson (4):
  ionic: drop use of subdevice tags
  ionic: add Rx dropped packet counter
  ionic: restrict received packets to mtu size
  ionic: clear compiler warning on hb use before set

 drivers/net/ethernet/pensando/ionic/ionic.h   |  4 ----
 .../net/ethernet/pensando/ionic/ionic_lif.h   |  1 +
 .../net/ethernet/pensando/ionic/ionic_main.c  |  2 +-
 .../net/ethernet/pensando/ionic/ionic_stats.c |  1 +
 .../net/ethernet/pensando/ionic/ionic_txrx.c  | 23 +++++++++++++++----
 5 files changed, 21 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 net-next 1/4] ionic: drop use of subdevice tags

[Shannon Nelson]

The subdevice concept is not being used in the driver, so
drop the references to it.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/ionic.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
index ca0b21911ad6..bb106a32f416 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic.h
@@ -19,10 +19,6 @@ struct ionic_lif;
 #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF	0x1002
 #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF	0x1003
 
-#define IONIC_SUBDEV_ID_NAPLES_25	0x4000
-#define IONIC_SUBDEV_ID_NAPLES_100_4	0x4001
-#define IONIC_SUBDEV_ID_NAPLES_100_8	0x4002
-
 #define DEVCMD_TIMEOUT  10
 
 struct ionic_vf {
-- 
2.17.1

[PATCH v2 net-next 2/4] ionic: add Rx dropped packet counter

[Shannon Nelson]

Add a counter for packets dropped by the driver, typically
for bad size or a receive error seen by the device.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/ionic_lif.h   |  1 +
 drivers/net/ethernet/pensando/ionic/ionic_stats.c |  1 +
 drivers/net/ethernet/pensando/ionic/ionic_txrx.c  | 12 +++++++++---
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index a55fd1f8c31b..9c5a7dd45f9d 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -37,6 +37,7 @@ struct ionic_rx_stats {
 	u64 csum_complete;
 	u64 csum_error;
 	u64 buffers_posted;
+	u64 dropped;
 };
 
 #define IONIC_QCQ_F_INITED		BIT(0)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_stats.c b/drivers/net/ethernet/pensando/ionic/ionic_stats.c
index 03916b6d47f2..a1e9796a660a 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_stats.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_stats.c
@@ -39,6 +39,7 @@ static const struct ionic_stat_desc ionic_rx_stats_desc[] = {
 	IONIC_RX_STAT_DESC(csum_none),
 	IONIC_RX_STAT_DESC(csum_complete),
 	IONIC_RX_STAT_DESC(csum_error),
+	IONIC_RX_STAT_DESC(dropped),
 };
 
 static const struct ionic_stat_desc ionic_txq_stats_desc[] = {
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index 97e79949b359..a009bbe9f9be 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -152,12 +152,16 @@ static void ionic_rx_clean(struct ionic_queue *q, struct ionic_desc_info *desc_i
 	stats = q_to_rx_stats(q);
 	netdev = q->lif->netdev;
 
-	if (comp->status)
+	if (comp->status) {
+		stats->dropped++;
 		return;
+	}
 
 	/* no packet processing while resetting */
-	if (unlikely(test_bit(IONIC_LIF_QUEUE_RESET, q->lif->state)))
+	if (unlikely(test_bit(IONIC_LIF_QUEUE_RESET, q->lif->state))) {
+		stats->dropped++;
 		return;
+	}
 
 	stats->pkts++;
 	stats->bytes += le16_to_cpu(comp->len);
@@ -167,8 +171,10 @@ static void ionic_rx_clean(struct ionic_queue *q, struct ionic_desc_info *desc_i
 	else
 		skb = ionic_rx_frags(q, desc_info, cq_info);
 
-	if (unlikely(!skb))
+	if (unlikely(!skb)) {
+		stats->dropped++;
 		return;
+	}
 
 	skb_record_rx_queue(skb, q->index);
 
-- 
2.17.1

[PATCH v2 net-next 3/4] ionic: restrict received packets to mtu size

[Shannon Nelson]

Make sure the NIC drops packets that are larger than the
specified MTU.

The front end of the NIC will accept packets larger than MTU and
will copy all the data it can to fill up the driver's posted
buffers - if the buffers are not long enough the packet will
then get dropped.  With the Rx SG buffers allocagted as full
pages, we are currently setting up more space than MTU size
available and end up receiving some packets that are larger
than MTU, up to the size of buffers posted.  To be sure the
NIC doesn't waste our time with oversized packets we need to
lie a little in the SG descriptor about how long is the last
SG element.

At dealloc time, we know the allocation was a page, so the
deallocation doesn't care about what length we put in the
descriptor.

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

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index a009bbe9f9be..e452f4242ba0 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -343,6 +343,8 @@ void ionic_rx_fill(struct ionic_queue *q)
 	struct ionic_rxq_sg_desc *sg_desc;
 	struct ionic_rxq_sg_elem *sg_elem;
 	struct ionic_rxq_desc *desc;
+	unsigned int remain_len;
+	unsigned int seg_len;
 	unsigned int nfrags;
 	bool ring_doorbell;
 	unsigned int i, j;
@@ -352,6 +354,7 @@ void ionic_rx_fill(struct ionic_queue *q)
 	nfrags = round_up(len, PAGE_SIZE) / PAGE_SIZE;
 
 	for (i = ionic_q_space_avail(q); i; i--) {
+		remain_len = len;
 		desc_info = q->head;
 		desc = desc_info->desc;
 		sg_desc = desc_info->sg_desc;
@@ -375,7 +378,9 @@ void ionic_rx_fill(struct ionic_queue *q)
 			return;
 		}
 		desc->addr = cpu_to_le64(page_info->dma_addr);
-		desc->len = cpu_to_le16(PAGE_SIZE);
+		seg_len = min_t(unsigned int, PAGE_SIZE, len);
+		desc->len = cpu_to_le16(seg_len);
+		remain_len -= seg_len;
 		page_info++;
 
 		/* fill sg descriptors - pages[1..n] */
@@ -391,7 +396,9 @@ void ionic_rx_fill(struct ionic_queue *q)
 				return;
 			}
 			sg_elem->addr = cpu_to_le64(page_info->dma_addr);
-			sg_elem->len = cpu_to_le16(PAGE_SIZE);
+			seg_len = min_t(unsigned int, PAGE_SIZE, remain_len);
+			sg_elem->len = cpu_to_le16(seg_len);
+			remain_len -= seg_len;
 			page_info++;
 		}
 
-- 
2.17.1

[PATCH v2 net-next 4/4] ionic: clear compiler warning on hb use before set

[Shannon Nelson]

Build checks have pointed out that 'hb' can theoretically
be used before set, so let's initialize it and get rid
of the compiler complaint.

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

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
index 837b85f2fed9..a8e3fb73b465 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -330,9 +330,9 @@ int ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds)
 	unsigned long max_wait;
 	unsigned long duration;
 	int opcode;
+	int hb = 0;
 	int done;
 	int err;
-	int hb;
 
 	WARN_ON(in_interrupt());
 
-- 
2.17.1

Re: [PATCH v2 net-next 3/4] ionic: restrict received packets to mtu size

[Andrew Lunn]

On Mon, Jan 06, 2020 at 07:43:48PM -0800, Shannon Nelson wrote:
> Make sure the NIC drops packets that are larger than the
> specified MTU.
> 
> The front end of the NIC will accept packets larger than MTU and
> will copy all the data it can to fill up the driver's posted
> buffers - if the buffers are not long enough the packet will
> then get dropped.  With the Rx SG buffers allocagted as full
> pages, we are currently setting up more space than MTU size
> available and end up receiving some packets that are larger
> than MTU, up to the size of buffers posted.  To be sure the
> NIC doesn't waste our time with oversized packets we need to
> lie a little in the SG descriptor about how long is the last
> SG element.

Hi Shannon

Does the stack really drop them later? With DSA, the frame has an
additional header, making it longer than the MTU. Most of the NICs
i've used are happy to receive such frames. So it does seem common
practice to not implement a 'MRU' in the MAC.

If the stack really does drop them, this is a reasonable optimisation.

Thanks

	Andrew

Re: [PATCH v2 net-next 3/4] ionic: restrict received packets to mtu size

[Shannon Nelson]

On 1/7/20 5:09 AM, Andrew Lunn wrote:
> On Mon, Jan 06, 2020 at 07:43:48PM -0800, Shannon Nelson wrote:
>> Make sure the NIC drops packets that are larger than the
>> specified MTU.
>>
>> The front end of the NIC will accept packets larger than MTU and
>> will copy all the data it can to fill up the driver's posted
>> buffers - if the buffers are not long enough the packet will
>> then get dropped.  With the Rx SG buffers allocagted as full
>> pages, we are currently setting up more space than MTU size
>> available and end up receiving some packets that are larger
>> than MTU, up to the size of buffers posted.  To be sure the
>> NIC doesn't waste our time with oversized packets we need to
>> lie a little in the SG descriptor about how long is the last
>> SG element.
> Hi Shannon
>
> Does the stack really drop them later? With DSA, the frame has an
> additional header, making it longer than the MTU. Most of the NICs
> i've used are happy to receive such frames. So it does seem common
> practice to not implement a 'MRU' in the MAC.
>
> If the stack really does drop them, this is a reasonable optimisation.
>
> Thanks
>
> 	Andrew
Hi Andrew,

In my experience the driver typically tells the NIC about the current 
max_frame size (e.g. MTU + ETH_HLEN), the NIC only copies max_frame 
bytes, and the NIC returns an error indication on a packets that had 
more than max_frame.

Before this patch we were telling the Naples NIC of buffers that are 
longer than max_frame.  This NIC will happily accept oversized packets 
off of the wire and copy as much as it can into the buffers, and it will 
only set an error status when it runs out of buffer, not when it goes 
above max_frame size.  To get the "correct" behavior, we can't rely on 
setting max_frame, we have to rely on the buffer indications.

sln

Re: [PATCH v2 net-next 3/4] ionic: restrict received packets to mtu size

[Andrew Lunn]

> Hi Andrew,
> 

Hi Shannon

> In my experience the driver typically tells the NIC about the current
> max_frame size (e.g. MTU + ETH_HLEN), the NIC only copies max_frame bytes,
> and the NIC returns an error indication on a packets that had more than
> max_frame.

Having played around with a few different NICs for DSA, it seems more
like 75% don't care about the 'MRU' and will happily accept bigger
frames.

Anyway, it does not hurt to drop received frames bigger than what you
can transmit.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net-next 0/4] ionic: driver updates

[David Miller]

From: Shannon Nelson <snelson@pensando.io>
Date: Mon,  6 Jan 2020 19:43:45 -0800

> These are a few little updates for the ionic network driver.
> 
> v2: dropped IBM msi patch
>     added fix for a compiler warning

Series applied, thanks.

Re: [PATCH v2 net-next 3/4] ionic: restrict received packets to mtu size

[Vladimir Oltean]

Hi Andrew,

On Tue, 7 Jan 2020 at 21:46, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Hi Andrew,
> >
>
> Hi Shannon
>
> > In my experience the driver typically tells the NIC about the current
> > max_frame size (e.g. MTU + ETH_HLEN), the NIC only copies max_frame bytes,
> > and the NIC returns an error indication on a packets that had more than
> > max_frame.
>
> Having played around with a few different NICs for DSA, it seems more
> like 75% don't care about the 'MRU' and will happily accept bigger
> frames.
>
> Anyway, it does not hurt to drop received frames bigger than what you
> can transmit.

In the general case, would we want a knob in Linux for the MRU, or is
it ok to go ahead with patches such as this one, and e.g. set up port
policers for limiting the frame length at the value of the MTU?

>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
>     Andrew

-Vladimir