Hi,
The EBUSY hack that I'm removing imposes an order on the ifconfig
commands issued against the pppoe interface used to configure the
sppp layer below. Right now ifconfig will fail with "device busy"
after the authentication parameters have been set up because it
kickstarts the LCP logic down in the sppp. To workaround that we
have to bring the interface down first and then submit commands in
a correct order or do the reconfiguration if that's the goal.
As far as I know, this doesn't follow the behavior of other pseudo
interfaces and forces us to implement kludges and workarounds for
these arguably broken semantics. We'd rather avoid that.
To counter this we use the ENETRESET trick that other drivers use
to tell the pppoe layer that sppp has requested a stop/init reset
sequence to proceed which we oblige with in case pppoe is UP and
RUNNING.
Now I must admit the code under "if (error == ENETRESET)" is a
bit scary, but apart from calling if_down, if_up and SIOCSIFFLAGS
directly it's copied from the pppoe/SIOCSIFFLAGS handling.
sppp/SIOCSIFFLAGS handler does not use the ifreq argument so it's
passed down as a NULL. Theoretically I can factor out code doing
SIOCSIFFLAG in the sppp_ioctl to two exported functions sppp_up
and sppp_down and call them from the pppoe_ioctl, but I don't
think this will be a huge improvement in readability or otherwise.
This has been already tested by a few, so I'm fairly confident
that there are no operational issues with this diff, but I'm not
using this myself (-;
Opinions? OK?
(I've stashed a small cleanup diff for the sppp_set_params that
I'll be posting shortly in case you notice that it can lose some
weight.)
diff --git sys/net/if_pppoe.c sys/net/if_pppoe.c
index 15b2b8a..8e5f713 100644
--- sys/net/if_pppoe.c
+++ sys/net/if_pppoe.c
@@ -896,11 +896,11 @@ static int
pppoe_ioctl(struct ifnet *ifp, unsigned long cmd, caddr_t data)
{
struct proc *p = curproc; /* XXX */
struct pppoe_softc *sc = (struct pppoe_softc *)ifp;
struct ifnet *eth_if;
- int error = 0;
+ int s, error = 0;
switch (cmd) {
case PPPOESETPARMS:
{
struct pppoediscparms *parms = (struct pppoediscparms *)data;
@@ -1027,11 +1027,38 @@ pppoe_ioctl(struct ifnet *ifp, unsigned long cmd,
caddr_t data)
if_put(eth_if);
return (sppp_ioctl(ifp, cmd, data));
}
default:
- return (sppp_ioctl(ifp, cmd, data));
+ error = sppp_ioctl(ifp, cmd, data);
+ if (error == ENETRESET) {
+ error = 0;
+ if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) ==
+ (IFF_UP | IFF_RUNNING)) {
+ s = splnet();
+ if_down(ifp);
+ splx(s);
+ if (sc->sc_state >= PPPOE_STATE_PADI_SENT &&
+ sc->sc_state < PPPOE_STATE_SESSION) {
+ timeout_del(&sc->sc_timeout);
+ sc->sc_state = PPPOE_STATE_INITIAL;
+ sc->sc_padi_retried = 0;
+ sc->sc_padr_retried = 0;
+ memcpy(&sc->sc_dest,
+ etherbroadcastaddr,
+ sizeof(sc->sc_dest));
+ }
+ error = sppp_ioctl(ifp, SIOCSIFFLAGS, NULL);
+ if (error)
+ return (error);
+ s = splnet();
+ if_up(ifp);
+ splx(s);
+ return (sppp_ioctl(ifp, SIOCSIFFLAGS, NULL));
+ }
+ }
+ return (error);
}
return (0);
}
/*
diff --git sys/net/if_spppsubr.c sys/net/if_spppsubr.c
index ae8b589..7ff1156 100644
--- sys/net/if_spppsubr.c
+++ sys/net/if_spppsubr.c
@@ -4527,23 +4527,10 @@ sppp_set_params(struct sppp *sp, struct ifreq *ifr)
int cmd;
if (copyin((caddr_t)ifr->ifr_data, &cmd, sizeof cmd) != 0)
return EFAULT;
- /*
- * We have a very specific idea of which fields we allow
- * being passed back from userland, so to not clobber our
- * current state. For one, we only allow setting
- * anything if LCP is in dead phase. Once the LCP
- * negotiations started, the authentication settings must
- * not be changed again. (The administrator can force an
- * ifconfig down in order to get LCP back into dead
- * phase.)
- */
- if (sp->pp_phase != PHASE_DEAD)
- return EBUSY;
-
switch (cmd) {
case SPPPIOSDEFS:
{
struct spppreq *spr;
@@ -4636,11 +4623,11 @@ sppp_set_params(struct sppp *sp, struct ifreq *ifr)
}
default:
return EINVAL;
}
- return 0;
+ return (ENETRESET);
}
void
sppp_phase_network(struct sppp *sp)
{
