On Wed, Nov 23, 2016 at 21:52 +0100, Stefan Fritsch wrote:
> On Wed, 23 Nov 2016, Mike Belopuhov wrote:
> > This fixes up tsleep priorities in vio(4) and makes sleeps
> > interruptable so that ifconfig can be killed with ^C. Tested
> > on qemu with a previous diff that makes vio(4) hang there.
> >
> > OK?
>
> I think the tsleep stuff is ok, but the vio_iff() changes are not.
>
The vio_iff changes are needed to get the interruptable sleep
working. I've rewritten that part of the diff.
> > - if (rxfilter) {
> > + if (rxfilter)
> > sc->sc_ctrl_mac_tbl_mc->nentries = nentries;
> > - r = vio_set_rx_filter(sc);
> > - if (r != 0) {
> > - rxfilter = 0;
> > - allmulti = 1; /* fallback */
> > - }
> > - } else {
> > - sc->sc_ctrl_mac_tbl_mc->nentries = 0;
>
> You must not remove this this in the !rxfilter case, or an old value of
> nentries may be kept.
>
Ack.
> > - vio_set_rx_filter(sc);
> > - }
> >
> > - if (allmulti) {
> > - r = vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
> > - if (r != 0) {
> > - allmulti = 0;
> > - promisc = 1; /* fallback */
> > - }
> > - } else {
> > - vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, 0);
> > - }
> > + r = vio_set_rx_filter(sc);
> > + if (r && r != ENOTSUP)
> > + return;
>
> That's wrong. If the host's filter table is not large enough,
> vio_set_rx_filter() will return EIO. In this case we should re-try with
> promisc.
>
This wasn't documented and EIO wasn't explicitely handled which
made this code look strange. This is a good reason to clean it up.
> > + else if (r == ENOTSUP && rxfilter)
> > + allmulti = 1; /* fallback */
> > +
> > + r = vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, allmulti);
> > + if (r && r != ENOTSUP)
> > + return;
> > + else if (r == ENOTSUP && allmulti)
> > + promisc = 1; /* fallback */
>
> In the ENOTSUP case, vio_ctrl_rx() will always fail, too. There is no use
> for a retry, then.
>
I think we can move the "vsc->sc_nvqs < 3" check out of those
functions to make it even more simpler.
> Maybe leave the logic unchanged, and just add a few checks to return in
> case of EINTR?
>
I propose less checks (or better ones if you will) since it's
a forest of if's as it is.
diff --git sys/dev/pci/if_vio.c sys/dev/pci/if_vio.c
index d2a56a3..3926edf 100644
--- sys/dev/pci/if_vio.c
+++ sys/dev/pci/if_vio.c
@@ -281,11 +281,11 @@ int vio_ctrl_rx(struct vio_softc *, int, int);
int vio_set_rx_filter(struct vio_softc *);
void vio_iff(struct vio_softc *);
int vio_media_change(struct ifnet *);
void vio_media_status(struct ifnet *, struct ifmediareq *);
int vio_ctrleof(struct virtqueue *);
-void vio_wait_ctrl(struct vio_softc *sc);
+int vio_wait_ctrl(struct vio_softc *sc);
int vio_wait_ctrl_done(struct vio_softc *sc);
void vio_ctrl_wakeup(struct vio_softc *, enum vio_ctrl_state);
int vio_alloc_mem(struct vio_softc *);
int vio_alloc_dmamem(struct vio_softc *);
void vio_free_dmamem(struct vio_softc *);
@@ -1216,15 +1216,14 @@ vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
{
struct virtio_softc *vsc = sc->sc_virtio;
struct virtqueue *vq = &sc->sc_vq[VQCTL];
int r, slot;
- if (vsc->sc_nvqs < 3)
- return ENOTSUP;
-
splassert(IPL_NET);
- vio_wait_ctrl(sc);
+
+ if ((r = vio_wait_ctrl(sc)) != 0)
+ return r;
sc->sc_ctrl_cmd->class = VIRTIO_NET_CTRL_RX;
sc->sc_ctrl_cmd->command = cmd;
sc->sc_ctrl_rx->onoff = onoff;
@@ -1247,14 +1246,12 @@ vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
sizeof(*sc->sc_ctrl_rx), 1);
VIO_DMAMEM_ENQUEUE(sc, vq, slot, sc->sc_ctrl_status,
sizeof(*sc->sc_ctrl_status), 0);
virtio_enqueue_commit(vsc, vq, slot, 1);
- if (vio_wait_ctrl_done(sc)) {
- r = EIO;
+ if ((r = vio_wait_ctrl_done(sc)) != 0)
goto out;
- }
VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_cmd,
sizeof(*sc->sc_ctrl_cmd), BUS_DMASYNC_POSTWRITE);
VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_rx,
sizeof(*sc->sc_ctrl_rx), BUS_DMASYNC_POSTWRITE);
@@ -1272,28 +1269,38 @@ vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
out:
vio_ctrl_wakeup(sc, FREE);
return r;
}
-void
+int
vio_wait_ctrl(struct vio_softc *sc)
{
- while (sc->sc_ctrl_inuse != FREE)
- tsleep(&sc->sc_ctrl_inuse, IPL_NET, "vio_wait", 0);
+ int r = 0;
+
+ while (sc->sc_ctrl_inuse != FREE) {
+ r = tsleep(&sc->sc_ctrl_inuse, PRIBIO|PCATCH, "viowait", 0);
+ if (r == EINTR)
+ return r;
+ }
sc->sc_ctrl_inuse = INUSE;
+
+ return r;
}
int
vio_wait_ctrl_done(struct vio_softc *sc)
{
int r = 0;
+
while (sc->sc_ctrl_inuse != DONE && sc->sc_ctrl_inuse != RESET) {
if (sc->sc_ctrl_inuse == RESET) {
r = 1;
break;
}
- tsleep(&sc->sc_ctrl_inuse, IPL_NET, "vio_wait", 0);
+ r = tsleep(&sc->sc_ctrl_inuse, PRIBIO|PCATCH, "viodone", 0);
+ if (r == EINTR)
+ break;
}
return r;
}
void
@@ -1332,14 +1339,12 @@ vio_set_rx_filter(struct vio_softc *sc)
struct virtqueue *vq = &sc->sc_vq[VQCTL];
int r, slot;
splassert(IPL_NET);
- if (vsc->sc_nvqs < 3)
- return ENOTSUP;
-
- vio_wait_ctrl(sc);
+ if ((r = vio_wait_ctrl(sc)) != 0)
+ return r;
sc->sc_ctrl_cmd->class = VIRTIO_NET_CTRL_MAC;
sc->sc_ctrl_cmd->command = VIRTIO_NET_CTRL_MAC_TABLE_SET;
VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_cmd,
@@ -1365,14 +1370,12 @@ vio_set_rx_filter(struct vio_softc *sc)
sc->sc_ctrl_mac_tbl_mc->nentries * ETHER_ADDR_LEN, 1);
VIO_DMAMEM_ENQUEUE(sc, vq, slot, sc->sc_ctrl_status,
sizeof(*sc->sc_ctrl_status), 0);
virtio_enqueue_commit(vsc, vq, slot, 1);
- if (vio_wait_ctrl_done(sc)) {
- r = EIO;
+ if ((r = vio_wait_ctrl_done(sc)) != 0)
goto out;
- }
VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_cmd,
sizeof(*sc->sc_ctrl_cmd), BUS_DMASYNC_POSTWRITE);
VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_mac_info,
VIO_CTRL_MAC_INFO_SIZE, BUS_DMASYNC_POSTWRITE);
@@ -1380,10 +1383,11 @@ vio_set_rx_filter(struct vio_softc *sc)
sizeof(*sc->sc_ctrl_status), BUS_DMASYNC_POSTREAD);
if (sc->sc_ctrl_status->ack == VIRTIO_NET_OK) {
r = 0;
} else {
+ /* The host's filter table is not large enough */
printf("%s: failed setting rx filter\n", sc->sc_dev.dv_xname);
r = EIO;
}
out:
@@ -1437,29 +1441,24 @@ vio_iff(struct vio_softc *sc)
/* set unicast address, VirtualBox wants that */
memcpy(sc->sc_ctrl_mac_tbl_uc->macs[0], ac->ac_enaddr, ETHER_ADDR_LEN);
sc->sc_ctrl_mac_tbl_uc->nentries = 1;
- if (rxfilter) {
- sc->sc_ctrl_mac_tbl_mc->nentries = nentries;
- r = vio_set_rx_filter(sc);
- if (r != 0) {
- rxfilter = 0;
- allmulti = 1; /* fallback */
- }
- } else {
- sc->sc_ctrl_mac_tbl_mc->nentries = 0;
- vio_set_rx_filter(sc);
- }
+ sc->sc_ctrl_mac_tbl_mc->nentries = rxfilter ? nentries : 0;
- if (allmulti) {
- r = vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
- if (r != 0) {
- allmulti = 0;
- promisc = 1; /* fallback */
- }
- } else {
- vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, 0);
- }
+ if (vsc->sc_nvqs < 3)
+ return;
+
+ r = vio_set_rx_filter(sc);
+ if (r == EIO)
+ allmulti = 1; /* fallback */
+ else if (r != 0)
+ return;
+
+ r = vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, allmulti);
+ if (r == EIO)
+ promisc = 1; /* fallback */
+ else if (r != 0)
+ return;
vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_PROMISC, promisc);
}
