[PATCH net-next v2 0/2] lib8390: Remove custom padding solution

[PATCH net-next v2 0/2] lib8390: Remove custom padding solution

[Armin Wolf]

When padding undersized frames, lib8390.c utilizes a stack scratch area
plus memset/memcpy. In doing so, it is overwriting content already
zeroed with memset, which seems not optimal even when commented as
being more efficient. Using eth_skb_pad() allows us to remove
memset/memcpy and the stack scratch area altogether.

v2 changes:
- split cleanup of variables in seperate patch
- revise commit description


Armin Wolf (2):
  lib8390: Use eth_skb_pad()
  lib8390: Cleanup variables

 drivers/net/ethernet/8390/lib8390.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

--
2.20.1

[PATCH net-next v2 1/2] lib8390: Use eth_skb_pad()

[Armin Wolf]

According to Joe Perches, this bit seems less than optimal
because it overwrites already zeroed content. But instead of
fixing the custom padding solution, replace them entirely
with generic eth_skb_pad().

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/net/ethernet/8390/lib8390.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/8390/lib8390.c b/drivers/net/ethernet/8390/lib8390.c
index e84021282edf..b3499714f7e0 100644
--- a/drivers/net/ethernet/8390/lib8390.c
+++ b/drivers/net/ethernet/8390/lib8390.c
@@ -305,17 +305,17 @@ static netdev_tx_t __ei_start_xmit(struct sk_buff *skb,
 {
 	unsigned long e8390_base = dev->base_addr;
 	struct ei_device *ei_local = netdev_priv(dev);
-	int send_length = skb->len, output_page;
+	int send_length, output_page;
 	unsigned long flags;
-	char buf[ETH_ZLEN];
-	char *data = skb->data;
-
-	if (skb->len < ETH_ZLEN) {
-		memset(buf, 0, ETH_ZLEN);	/* more efficient than doing just the needed bits */
-		memcpy(buf, data, skb->len);
-		send_length = ETH_ZLEN;
-		data = buf;
+	char *data;
+
+	/* The Hardware does not pad undersized frames */
+	if (eth_skb_pad(skb)) {
+		dev->stats.tx_dropped++;
+		return NETDEV_TX_OK;
 	}
+	data = skb->data;
+	send_length = skb->len;

 	/* Mask interrupts from the ethercard.
 	   SMP: We have to grab the lock here otherwise the IRQ handler
--
2.20.1

[PATCH net-next v2 2/2] lib8390: Cleanup variables

[Armin Wolf]

Replace variables associated with the former padding
solution with skb->* expressions. They are not needed
anymore.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/net/ethernet/8390/lib8390.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/8390/lib8390.c b/drivers/net/ethernet/8390/lib8390.c
index b3499714f7e0..47e2962eff56 100644
--- a/drivers/net/ethernet/8390/lib8390.c
+++ b/drivers/net/ethernet/8390/lib8390.c
@@ -305,17 +305,14 @@ static netdev_tx_t __ei_start_xmit(struct sk_buff *skb,
 {
 	unsigned long e8390_base = dev->base_addr;
 	struct ei_device *ei_local = netdev_priv(dev);
-	int send_length, output_page;
+	int output_page;
 	unsigned long flags;
-	char *data;

 	/* The Hardware does not pad undersized frames */
 	if (eth_skb_pad(skb)) {
 		dev->stats.tx_dropped++;
 		return NETDEV_TX_OK;
 	}
-	data = skb->data;
-	send_length = skb->len;

 	/* Mask interrupts from the ethercard.
 	   SMP: We have to grab the lock here otherwise the IRQ handler
@@ -347,7 +344,7 @@ static netdev_tx_t __ei_start_xmit(struct sk_buff *skb,

 	if (ei_local->tx1 == 0) {
 		output_page = ei_local->tx_start_page;
-		ei_local->tx1 = send_length;
+		ei_local->tx1 = skb->len;
 		if ((netif_msg_tx_queued(ei_local)) &&
 		    ei_local->tx2 > 0)
 			netdev_dbg(dev,
@@ -355,7 +352,7 @@ static netdev_tx_t __ei_start_xmit(struct sk_buff *skb,
 				   ei_local->tx2, ei_local->lasttx, ei_local->txing);
 	} else if (ei_local->tx2 == 0) {
 		output_page = ei_local->tx_start_page + TX_PAGES/2;
-		ei_local->tx2 = send_length;
+		ei_local->tx2 = skb->len;
 		if ((netif_msg_tx_queued(ei_local)) &&
 		    ei_local->tx1 > 0)
 			netdev_dbg(dev,
@@ -380,11 +377,11 @@ static netdev_tx_t __ei_start_xmit(struct sk_buff *skb,
 	 * trigger the send later, upon receiving a Tx done interrupt.
 	 */

-	ei_block_output(dev, send_length, data, output_page);
+	ei_block_output(dev, skb->len, skb->data, output_page);

 	if (!ei_local->txing) {
 		ei_local->txing = 1;
-		NS8390_trigger_send(dev, send_length, output_page);
+		NS8390_trigger_send(dev, skb->len, output_page);
 		if (output_page == ei_local->tx_start_page) {
 			ei_local->tx1 = -1;
 			ei_local->lasttx = -1;
@@ -407,8 +404,8 @@ static netdev_tx_t __ei_start_xmit(struct sk_buff *skb,
 	spin_unlock(&ei_local->page_lock);
 	enable_irq_lockdep_irqrestore(dev->irq, &flags);
 	skb_tx_timestamp(skb);
+	dev->stats.tx_bytes += skb->len;
 	dev_consume_skb_any(skb);
-	dev->stats.tx_bytes += send_length;

 	return NETDEV_TX_OK;
 }
--
2.20.1

Re: [PATCH net-next v2 2/2] lib8390: Cleanup variables

[Jakub Kicinski]

On Wed, 18 Nov 2020 17:51:07 +0100 Armin Wolf wrote:
>  	unsigned long e8390_base = dev->base_addr;
>  	struct ei_device *ei_local = netdev_priv(dev);
> -	int send_length, output_page;
> +	int output_page;
>  	unsigned long flags;

The last two lines should be swapped to follow the reverse xmas tree
ordering of variables.

More importantly this driver is marked as:

S:	Orphan / Obsolete

in MAINTAINERS. Are you actually using hardware which needs this code,
or are those changes purely based on code inspection?

You should either change the state of the driver in MAINTAINERS, or find
another driver / part of the kernel to refactor.

Re: [PATCH net-next v2 2/2] lib8390: Cleanup variables

[Armin Wolf]

<20201120120235.1925e713@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

I would indeed like to become the maintainer of this driver,
as I do in fact still have working hardware using this code
(a Realtek 8029AS).
Unfortunately, i am still considering myself too inexperienced in
kernel development to become a maintainer.

So maybe i should at first gather more experience by studying
other nic drivers.

Sorry for bothering.