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:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.