Re: New DaynaPort SCSI/Link ethernet driver

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

> Please find my diff at ftp://ftp.netbsd.org/pub/NetBSD/misc/nat/if_dse_all.diff
> 
> for review.

Several quick comments:

> +#include "opt_ccitt.h"
> +#include "opt_llc.h"
> +#include "opt_ns.h"

These network supports has been obsolete.

> +#ifdef NS
> +#include <netns/ns.h>
> +#include <netns/ns_if.h>
> +#endif

> +#if defined(CCITT) && defined(LLC)
> +#include <sys/socketvar.h>
> +#include <netccitt/x25.h>
> +#include <netccitt/pk.h>
> +#include <netccitt/pk_var.h>
> +#include <netccitt/pk_extern.h>
> +#endif

Ditto. (and also in dse_ioctl)

> +typedef struct {
> +	/* standard */
> +	u_int8_t	device;			/* 3 (T_CPU) */
> +	u_int8_t	dev_qual2;		/* 0 (fixed) */
> +	u_int8_t	version;		/* 0 */
> +	u_int8_t	response_format;	/* 0 */
> +	u_int8_t	additional_len;		/* 31 */
> +	u_int8_t	unused[2];		/* 0,0 */
> +	u_int8_t	flags;			/* 0x00 */
> +	char	vendor[8];			/* ie; "SonicSys" */
> +	char	product[16];			/* ie; "MicroSCSI" */
> +	char	revision[4];			/* ie; "2.00" */
> +	char	extra[8];			/* none */
> +} scsi_dayna_ether_inquiry_data;

This should be struct scsipi_inquiry_data defined
in <dev/scsipi/scsipi_all.h>?

Also C99 int types are better in all places.

> +static int	dsematch(struct device *, struct cfdata *, void *);
> +static void	dseattach(struct device *, struct device *, void *);

Maybe modern drivers should use device_t and cfdata_t.

> +dseattach(device_t parent, device_t self, void *aux)
> +{
> +	struct dse_softc *sc = device_private(self);
> +	struct scsipibus_attach_args *sa = aux;
> +	struct scsipi_periph *periph = sa->sa_periph;
> +	struct ifnet *ifp = &sc->sc_ethercom.ec_if;
> +	uint8_t myaddr[ETHER_ADDR_LEN];
> +
> +	sc->sc_dev = self;
> +
> +	printf("\n");

aprint_normal(9) is better for boot(8) options.

> +	sc->sc_tbuf = malloc(ETHERMTU + sizeof(struct ether_header) +
> +	    DSE_EXTRAS_TX + 16, M_DEVBUF, M_NOWAIT);
> +	if (sc->sc_tbuf == NULL)
> +		panic("dseattach: can't allocate transmit buffer");
> +
> +	sc->sc_rbuf = malloc(RBUF_LEN + 16, M_DEVBUF, M_NOWAIT);
> +	if (sc->sc_rbuf == NULL)

I guess M_WAITOK without check is better in attachments nowadays.

> +static void
> +dse_ifstart(struct ifnet *ifp)
> +{

It looks if_se.c has been rewriteen to use workqueue(9):
 https://nxr.netbsd.org/diff/src/sys/dev/scsipi/if_se.c?r2=%2Fsrc%2Fsys%2Fdev%2Fscsipi%2Fif_se.c%401.106&r1=%2Fsrc%2Fsys%2Fdev%2Fscsipi%2Fif_se.c%401.105

---
Izumi Tsutsui