diff mbox series

[v2,01/20] ethernet: alteon: convert tasklets to use new tasklet_setup() API

Message ID 20200909084510.648706-2-allen.lkml@gmail.com
State New
Headers show
Series [v2,01/20] ethernet: alteon: convert tasklets to use new tasklet_setup() API | expand

Commit Message

Allen Sept. 9, 2020, 8:44 a.m. UTC 
In preparation for unconditionally passing the
struct tasklet_struct pointer to all tasklet
callbacks, switch to using the new tasklet_setup()
and from_tasklet() to pass the tasklet pointer explicitly.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
Signed-off-by: Allen Pais <allen.lkml@gmail.com>
---
 drivers/net/ethernet/alteon/acenic.c | 9 +++++----
 drivers/net/ethernet/alteon/acenic.h | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

Comments

David Miller Sept. 9, 2020, 6:09 p.m. UTC | #1 
From: Allen Pais <allen.lkml@gmail.com>
Date: Wed,  9 Sep 2020 14:14:51 +0530

> @@ -1562,10 +1562,11 @@ static void ace_watchdog(struct net_device *data, unsigned int txqueue)
>  }
>  
>  
> -static void ace_tasklet(unsigned long arg)
> +static void ace_tasklet(struct tasklet_struct *t)
>  {
> -	struct net_device *dev = (struct net_device *) arg;
> -	struct ace_private *ap = netdev_priv(dev);
> +	struct ace_private *ap = from_tasklet(ap, t, ace_tasklet);
> +	struct net_device *dev = (struct net_device *)((char *)ap -
> +				ALIGN(sizeof(struct net_device), NETDEV_ALIGN));
>  	int cur_size;
>  

I don't see this is as an improvement.  The 'dev' assignment looks so
incredibly fragile and exposes so many internal details about netdev
object allocation, alignment, and layout.

Who is going to find and fix this if someone changes how netdev object
allocation works?

I don't want to apply this, it sets a very bad precedent.  The existing
code is so much cleaner and easier to understand and audit.
Allen Sept. 11, 2020, 5:53 a.m. UTC | #2 
>
> >>
> >> > @@ -1562,10 +1562,11 @@ static void ace_watchdog(struct net_device *data, unsigned int txqueue)
> >> >  }
> >> >
> >> >
> >> > -static void ace_tasklet(unsigned long arg)
> >> > +static void ace_tasklet(struct tasklet_struct *t)
> >> >  {
> >> > -     struct net_device *dev = (struct net_device *) arg;
> >> > -     struct ace_private *ap = netdev_priv(dev);
> >> > +     struct ace_private *ap = from_tasklet(ap, t, ace_tasklet);
> >> > +     struct net_device *dev = (struct net_device *)((char *)ap -
> >> > +                             ALIGN(sizeof(struct net_device), NETDEV_ALIGN));
> >> >       int cur_size;
> >> >
> >>
> >> I don't see this is as an improvement.  The 'dev' assignment looks so
> >> incredibly fragile and exposes so many internal details about netdev
> >> object allocation, alignment, and layout.
> >>
> >> Who is going to find and fix this if someone changes how netdev object
> >> allocation works?
> >>
> >
> > Thanks for pointing it out. I'll see if I can fix it to keep it simple.
>
> Just add a backpointer to the netdev from the netdev_priv() if you
> absolutely have too.
>

Okay.

> >> I don't want to apply this, it sets a very bad precedent.  The existing
> >> code is so much cleaner and easier to understand and audit.
> >
> > Will you pick the rest of the patches or would they have to wait till
> > this one is
> > fixed.
>
> I never pick up a partial series, ever.  So yes you will have to fix these
> two patches up and resubmit the entire thing.
>

Sure, let me get these fixed up and ready. Thanks.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/alteon/acenic.c b/drivers/net/ethernet/alteon/acenic.c
index 8470c836fa18..056afc8bb519 100644
--- a/drivers/net/ethernet/alteon/acenic.c
+++ b/drivers/net/ethernet/alteon/acenic.c
@@ -1562,10 +1562,11 @@  static void ace_watchdog(struct net_device *data, unsigned int txqueue)
 }
 
 
-static void ace_tasklet(unsigned long arg)
+static void ace_tasklet(struct tasklet_struct *t)
 {
-	struct net_device *dev = (struct net_device *) arg;
-	struct ace_private *ap = netdev_priv(dev);
+	struct ace_private *ap = from_tasklet(ap, t, ace_tasklet);
+	struct net_device *dev = (struct net_device *)((char *)ap -
+				ALIGN(sizeof(struct net_device), NETDEV_ALIGN));
 	int cur_size;
 
 	cur_size = atomic_read(&ap->cur_rx_bufs);
@@ -2269,7 +2270,7 @@  static int ace_open(struct net_device *dev)
 	/*
 	 * Setup the bottom half rx ring refill handler
 	 */
-	tasklet_init(&ap->ace_tasklet, ace_tasklet, (unsigned long)dev);
+	tasklet_setup(&ap->ace_tasklet, ace_tasklet);
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/alteon/acenic.h b/drivers/net/ethernet/alteon/acenic.h
index c670067b1541..4b02c705859c 100644
--- a/drivers/net/ethernet/alteon/acenic.h
+++ b/drivers/net/ethernet/alteon/acenic.h
@@ -776,7 +776,7 @@  static int ace_open(struct net_device *dev);
 static netdev_tx_t ace_start_xmit(struct sk_buff *skb,
 				  struct net_device *dev);
 static int ace_close(struct net_device *dev);
-static void ace_tasklet(unsigned long dev);
+static void ace_tasklet(struct tasklet_struct *t);
 static void ace_dump_trace(struct ace_private *ap);
 static void ace_set_multicast_list(struct net_device *dev);
 static int ace_change_mtu(struct net_device *dev, int new_mtu);