NetBSD Problem Report #49621

From www@NetBSD.org  Sun Feb  1 09:34:06 2015
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 9601CA57FE
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  1 Feb 2015 09:34:06 +0000 (UTC)
Message-Id: <20150201093405.41317A65B0@mollari.NetBSD.org>
Date: Sun,  1 Feb 2015 09:34:05 +0000 (UTC)
From: webpages@sprow.co.uk
Reply-To: webpages@sprow.co.uk
To: gnats-bugs@NetBSD.org
Subject: Add detach function to if_cpsw
X-Send-Pr-Version: www-1.0

>Number:         49621
>Category:       port-arm
>Synopsis:       Add detach function to if_cpsw
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    port-arm-maintainer
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Feb 01 09:35:00 +0000 2015
>Closed-Date:    Thu Mar 12 20:51:30 +0000 2015
>Last-Modified:  Thu Mar 12 20:51:30 +0000 2015
>Originator:     Robert Sprowson
>Release:        
>Organization:
>Environment:
>Description:
The if_cpsw network driver has no detach function, so can't be hot swapped. This patch adds said function, as a diff against revision 1.6 of src/sys/arch/arm/omap/if_cpsw.c

Tested with a little loop to 30 that attach/detaches, and manually inspecting that no resources leaked overall.
>How-To-Repeat:

>Fix:
--- if_cpsw_1.6.c	2014-10-31 17:12:48 +0000
+++ if_cpsw_new.c	2015-02-01 09:24:24 +0000
@@ -144,8 +144,9 @@
 };

 static int cpsw_match(device_t, cfdata_t, void *);
 static void cpsw_attach(device_t, device_t, void *);
+static int cpsw_detach(device_t, int);

 static void cpsw_start(struct ifnet *);
 static int cpsw_ioctl(struct ifnet *, u_long, void *);
 static void cpsw_watchdog(struct ifnet *);
@@ -169,9 +170,9 @@

 static int cpsw_ale_update_addresses(struct cpsw_softc *, int purge);

 CFATTACH_DECL_NEW(cpsw, sizeof(struct cpsw_softc),
-    cpsw_match, cpsw_attach, NULL, NULL);
+    cpsw_match, cpsw_attach, cpsw_detach, NULL);

 #undef KERNHIST
 #include <sys/kernhist.h>
 KERNHIST_DEFINE(cpswhist);
@@ -335,8 +336,36 @@
 	}
 	return false;
 }

+static int
+cpsw_detach(device_t self, int flags)
+{
+	struct cpsw_softc * const sc = device_private(self);
+	u_int i;
+
+	/* Let go of the interrupts */
+	intr_disestablish(sc->sc_rxthih);
+	intr_disestablish(sc->sc_rxih);
+	intr_disestablish(sc->sc_txih);
+	intr_disestablish(sc->sc_miscih);
+
+	/* Free the packet padding buffer */
+	kmem_free(sc->sc_txpad, ETHER_MIN_LEN);
+	bus_dmamap_destroy(sc->sc_bdt, sc->sc_txpad_dm);
+
+	/* Destroy all the descriptors */
+	for (i = 0; i < CPSW_NTXDESCS; i++)
+		bus_dmamap_destroy(sc->sc_bdt, sc->sc_rdp->tx_dm[i]);
+	for (i = 0; i < CPSW_NRXDESCS; i++)
+		bus_dmamap_destroy(sc->sc_bdt, sc->sc_rdp->rx_dm[i]);
+	kmem_free(sc->sc_rdp, sizeof(*sc->sc_rdp));
+
+	aprint_normal_dev("detached CPSW driver\n");
+
+	return 0;
+}
+
 static void
 cpsw_attach(device_t parent, device_t self, void *aux)
 {
 	struct obio_attach_args * const oa = aux;

>Release-Note:

>Audit-Trail:
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, port-arm-maintainer@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Cc: 
Subject: Re: port-arm/49621: Add detach function to if_cpsw
Date: Sun, 1 Feb 2015 09:37:39 -0500

 On Feb 1,  9:35am, webpages@sprow.co.uk (webpages@sprow.co.uk) wrote:
 -- Subject: port-arm/49621: Add detach function to if_cpsw

 Doesn't it need to do at least a bus_space_unmap(), call cpsw_stop()
 and do all the other interface cleanup that the other drivers do
 when detaching?

 christos

From: Robert Sprowson <webpages@sprow.co.uk>
To: gnats-bugs@NetBSD.org
Cc: port-arm-maintainer@netbsd.org,
	gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org
Subject: Re: port-arm/49621: Add detach function to if_cpsw
Date: Sun, 01 Feb 2015 19:00:58 +0000 (GMT)

 > Doesn't it need to do at least a bus_space_unmap(), call cpsw_stop()
 > and do all the other interface cleanup that the other drivers do
 > when detaching?

 You're quite right, I'd only been considering the equivalents of
 malloc/free, but cpsw_match has other (non allocating) side effects. This
 updated patch should address those too, inspired by the i82577 driver:

 --- if_cpsw_1.6.c	2014-10-31 17:12:48 +0000
 +++ if_cpsw_new.c	2015-02-01 18:41:06 +0000
 @@ -114,16 +114,18 @@
  struct cpsw_softc {
  	device_t sc_dev;
  	bus_space_tag_t sc_bst;
  	bus_space_handle_t sc_bsh;
 +	bus_size_t sc_bss;
  	bus_dma_tag_t sc_bdt;
  	bus_space_handle_t sc_bsh_txdescs;
  	bus_space_handle_t sc_bsh_rxdescs;
  	bus_addr_t sc_txdescs_pa;
  	bus_addr_t sc_rxdescs_pa;
  	struct ethercom sc_ec;
  	struct mii_data sc_mii;
  	bool sc_phy_has_1000t;
 +	bool sc_attached;
  	callout_t sc_tick_ch;
  	void *sc_ih;
  	struct cpsw_ring_data *sc_rdp;
  	volatile u_int sc_txnext;
 @@ -144,8 +146,9 @@
  };

  static int cpsw_match(device_t, cfdata_t, void *);
  static void cpsw_attach(device_t, device_t, void *);
 +static int cpsw_detach(device_t, int);

  static void cpsw_start(struct ifnet *);
  static int cpsw_ioctl(struct ifnet *, u_long, void *);
  static void cpsw_watchdog(struct ifnet *);
 @@ -169,9 +172,9 @@

  static int cpsw_ale_update_addresses(struct cpsw_softc *, int purge);

  CFATTACH_DECL_NEW(cpsw, sizeof(struct cpsw_softc),
 -    cpsw_match, cpsw_attach, NULL, NULL);
 +    cpsw_match, cpsw_attach, cpsw_detach, NULL);

  #undef KERNHIST
  #include <sys/kernhist.h>
  KERNHIST_DEFINE(cpswhist);
 @@ -335,8 +338,57 @@
  	}
  	return false;
  }

 +static int
 +cpsw_detach(device_t self, int flags)
 +{
 +	struct cpsw_softc * const sc = device_private(self);
 +	struct ifnet *ifp = &sc->sc_ec.ec_if;
 +	u_int i;
 +
 +	/* Succeed now if there's no work to do. */
 +	if (!sc->sc_attached)
 +		return (0);
 +
 +	/* Stop the interface. Callouts are stopped in it. */
 +	cpsw_stop(ifp, 1);
 +
 +	/* Destroy our callout. */
 +	callout_destroy(&sc->sc_tick_ch);
 +
 +	/* Delete all media. */
 +	ifmedia_delete_instance(&sc->sc_mii.mii_media, IFM_INST_ANY);
 +
 +	ether_ifdetach(ifp);
 +	if_detach(ifp);
 +
 +	/* Let go of the interrupts */
 +	intr_disestablish(sc->sc_rxthih);
 +	intr_disestablish(sc->sc_rxih);
 +	intr_disestablish(sc->sc_txih);
 +	intr_disestablish(sc->sc_miscih);
 +
 +	/* Free the packet padding buffer */
 +	kmem_free(sc->sc_txpad, ETHER_MIN_LEN);
 +	bus_dmamap_destroy(sc->sc_bdt, sc->sc_txpad_dm);
 +
 +	/* Destroy all the descriptors */
 +	for (i = 0; i < CPSW_NTXDESCS; i++)
 +		bus_dmamap_destroy(sc->sc_bdt, sc->sc_rdp->tx_dm[i]);
 +	for (i = 0; i < CPSW_NRXDESCS; i++)
 +		bus_dmamap_destroy(sc->sc_bdt, sc->sc_rdp->rx_dm[i]);
 +	kmem_free(sc->sc_rdp, sizeof(*sc->sc_rdp));
 +
 +	/* Unmap */
 +	bus_space_unmap(sc->sc_bst, sc->sc_bsh, sc->sc_bss);
 +
 +	sc->sc_attached = false;
 +	aprint_normal_dev("detached CPSW driver\n");
 +
 +	return 0;
 +}
 +
  static void
  cpsw_attach(device_t parent, device_t self, void *aux)
  {
  	struct obio_attach_args * const oa = aux;
 @@ -397,8 +449,9 @@
  	sc->sc_miscih = intr_establish(oa->obio_intrbase + CPSW_INTROFF_MISC,
  	    IPL_VM, IST_LEVEL, cpsw_miscintr, sc);

  	sc->sc_bst = oa->obio_iot;
 +	sc->sc_bss = oa->obio_size;
  	sc->sc_bdt = oa->obio_dmat;

  	error = bus_space_map(sc->sc_bst, oa->obio_addr, oa->obio_size, 0,
  	    &sc->sc_bsh);
 @@ -514,8 +567,11 @@

  	if_attach(ifp);
  	ether_ifattach(ifp, sc->sc_enaddr);

 +	/* The attach is successful. */
 +	sc->sc_attached = true;
 +
  	return;
  }

  static void

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/49621 CVS commit: src/sys/arch/arm/omap
Date: Sun, 1 Feb 2015 14:32:59 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Sun Feb  1 19:32:59 UTC 2015

 Modified Files:
 	src/sys/arch/arm/omap: if_cpsw.c

 Log Message:
 PR/49621: Robert Sprowson: Add detach function to if_cpsw


 To generate a diff of this commit:
 cvs rdiff -u -r1.6 -r1.7 src/sys/arch/arm/omap/if_cpsw.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, port-arm-maintainer@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, webpages@sprow.co.uk
Cc: 
Subject: Re: port-arm/49621: Add detach function to if_cpsw
Date: Sun, 1 Feb 2015 14:34:02 -0500

 On Feb 1,  7:05pm, webpages@sprow.co.uk (Robert Sprowson) wrote:
 -- Subject: Re: port-arm/49621: Add detach function to if_cpsw

 | The following reply was made to PR port-arm/49621; it has been noted by GNATS.
 | 
 | From: Robert Sprowson <webpages@sprow.co.uk>
 | To: gnats-bugs@NetBSD.org
 | Cc: port-arm-maintainer@netbsd.org,
 | 	gnats-admin@netbsd.org,
 | 	netbsd-bugs@netbsd.org
 | Subject: Re: port-arm/49621: Add detach function to if_cpsw
 | Date: Sun, 01 Feb 2015 19:00:58 +0000 (GMT)
 | 
 |  > Doesn't it need to do at least a bus_space_unmap(), call cpsw_stop()
 |  > and do all the other interface cleanup that the other drivers do
 |  > when detaching?
 |  
 |  You're quite right, I'd only been considering the equivalents of
 |  malloc/free, but cpsw_match has other (non allocating) side effects. This
 |  updated patch should address those too, inspired by the i82577 driver:

 Much better now! I've committed it, with a couple of changes to make it
 safer...

 Thanks!

 christos

State-Changed-From-To: open->closed
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Thu, 12 Mar 2015 20:51:30 +0000
State-Changed-Why:
Fixes committed.


>Unformatted:

Home	PR Database Search