Re: [PATCH] Fix OACTIVE for an(4)

On Wednesday, October 01, 2014 2:58:38 pm Adrian Chadd wrote:
> Hi,
> 
> On 1 October 2014 07:14, John Baldwin <jhb_at_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)
> > _at__at_ -2906,11 +2906,11 _at__at_
> >                 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