Date: Thu, 2 Oct 2014 11:16:27 -0400 From: John Baldwin <jhb@freebsd.org> To: Adrian Chadd <adrian@freebsd.org> Cc: freebsd-current <freebsd-current@freebsd.org> Subject: Re: [PATCH] Fix OACTIVE for an(4) Message-ID: <201410021116.27583.jhb@freebsd.org> In-Reply-To: <CAJ-VmomwiRRfHAGMn6onqQMXBEze40Nmyqya-5v8wMgZFC7iBg@mail.gmail.com> References: <2113392.UOaBFTpimf@ralph.baldwin.cx> <CAJ-VmomwiRRfHAGMn6onqQMXBEze40Nmyqya-5v8wMgZFC7iBg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday, October 01, 2014 2:58:38 pm Adrian Chadd wrote:
> Hi,
>
> On 1 October 2014 07:14, John Baldwin <jhb@freebsd.org> wrote:
> > This small patch correctly sets OACTIVE when an(4) gets backed up. Right
now
> > I believe it will never set the flag. It is only an optimization, it
should
> > not affect correctness.
> >
> > Index: an/if_an.c
> > ===================================================================
> > --- an/if_an.c (revision 270968)
> > +++ an/if_an.c (working copy)
> > @@ -2906,11 +2906,11 @@
> > CSR_WRITE_2(sc, AN_INT_EN(sc->mpi350), AN_INTRS(sc-
>mpi350));
> > }
> >
> > - if (m0 != NULL)
> > + if (sc->an_rdata.an_tx_prod != idx) {
> > ifp->if_drv_flags |= IFF_DRV_OACTIVE;
> > + sc->an_rdata.an_tx_prod = idx;
> > + }
> >
> > - sc->an_rdata.an_tx_prod = idx;
> > -
> > return;
> > }
>
> I haven't looked at the rest of the driver; is everything else around
> OACTIVE locked correctly and consistently?
As well as OACTIVE is for any other driver.
> There's no single-entry into if_start(). It can be called from
> multiple paths at the same time.
Yes, I know. However, this bug is more that it will never set OACTIVE
currently (m0 is always set to NULL before it gets to this point).
This code in its stats timer is also dubious:
/* Don't do this while we're transmitting */
if (ifp->if_drv_flags & IFF_DRV_OACTIVE) {
callout_reset(&sc->an_stat_ch, hz, an_stats_update, sc);
return;
}
sc->an_stats.an_len = sizeof(struct an_ltv_stats);
sc->an_stats.an_type = AN_RID_32BITS_CUM;
if (an_read_record(sc, (struct an_ltv_gen *)&sc->an_stats.an_len))
return;
callout_reset(&sc->an_stat_ch, hz, an_stats_update, sc);
First, the callout_reset() can just be moved earlier.
Second, OACTIVE doesn't mean that anything is transmitting, it means the ring
is full (at least in terms of how all other drivers use it).
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201410021116.27583.jhb>