Re: Another update for axe(4)

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

Paul Goyette wrote:

> >> +#include <machine/bus.h>
> >
> > We should use <sys/bus.h> nowadays,
> > but is this one actually required?
> 
> Yes, it is really needed.  I need to include usb/usbdivar.h to get the 
> usb speed data, and usbdivar.h references bus_dma_tag_t.  I have changed 
> it to use <sys/bus.h>

Ok.

> >> +  sc->axe_flags = axe_lookup(uaa->vendor, uaa->product)->axe_flags;
> >
> > It looks sc->axe_flags should be set in axe_match()
> > where axe_lookup() is called first.
> 
> No, I don't think so.  The results of the lookup in axe_match() are 
> discarded until the autoconfig stuff selects the best match.  And 
> axe_match() doesn't even have a softc in which to store the results.

Ah, yes, we can only update attach args in aux, not softc. Ok.

> >> +  memcpy(&sc->init_eaddr, &eaddr, sizeof(sc->init_eaddr));
> >
> > Do we really have to save MAC address into softc?
> > Can't we refer it by "CLLADDR(ifp->if_sadl)"?
> 
> I'll be totally honest here - I really haven't dived very deeply into 
> the network interface architecture at all.  So I have no idea if your 
> suggestion would work or not.  Where does ifp->if_sadl get stored?

I think ifp is initialized once ether_ifattach() is called
and we can always pull ifp in the drive via &sc->ethercom.ec_if.
(USB drivers seem to use GET_IFP(sc) macro)

axe_init() is called only from ioctls so I think it will work.
(otherwise OpenBSD's ac_enaddr won't work either)

> >> -                  c->axe_buf = usbd_alloc_buffer(c->axe_xfer, AXE_BUFSZ);
> >> +                  c->axe_buf = usbd_alloc_buffer(c->axe_xfer,
> >> +                                                 sc->axe_bufsz);
> >
> > KNF?
> 
> :)  Fixed (although I still prefer my own format here!)

There are some more similar lines.

> >> +  u_char                  *buf;
> >
> > This should be uint8_t?
> 
> This is from the OpenBSD code.  buf is being assigned from
> 
>       buf = c->axe_buf
> 
> where c->axe_buf is a (char *), so not sure if (uint8_t *) would be 
> correct.

(char *) might cause annoying gcc's signed/unsigned comparison warnings,
so unsigned is better for any xfer buffers, IMO.

> >> +                  if ((pktlen % 2) != 0)
> >> +                          pktlen++;
> >
> > roundup2(9)?
> 
> Done.

Looks wrong arg order.

"pkglen = roundup2(pktlen, sizeof(uint16_t));"
might be better as roundup2(9) man page says :-)

> >> +struct axe_sframe_hdr {
> >> +  u_int16_t               len;
> >> +  u_int16_t               ilen;
> >> +} __packed;
> >
> > Is __packed actually required?
> 
> Probably not, but the struct does reflect the format of data on the 
> physical media.  Just to avoid any future issues on an architecture that 
> might want to align thing on 32-bit boundary, I would prefer to leave 
> __packed here.

Ok.

---
Izumi Tsutsui