On Sat, Jun 24, 2006 at 01:17:46AM +0200, Manuel Bouyer wrote:

> attached are diffs to support basic ioctl interface to the amr(4) driver,
> and a port of the FreeBSD amrstat tool (diff against netbsd-3).

Cool.

> I'd like to get this in the tree and pulled up to netbsd-3.
> Comments or objections ?

In the ioctl() path:

+       default:
+               return EINVAL;
+       }

I realise we want to be compatible with FreeBSD, but EINVAL seems the wrong
thing to return here.

+               dp = malloc(au_length, M_DEVBUF, M_WAITOK|M_ZERO);

You should bounds check au_length first, eg:

	if (au_length < 0 || au_length > MAXPHYS)
		return (EINVAL);

+       /* direct command to controller */
+       s = splbio();
+       while (amr_ccb_alloc(amr, &ac) != 0) {
+               error = tsleep(NULL, PRIBIO | PCATCH, "armmbx", hz);
+               if (error == EINTR)
+                       goto out;
+       }

That's gross. The command queueing should be reworked to guarentee slots,
like the other drivers. I'll have a look at that later. For now I would
probably single thread the path, so that we can have no more than one user
command active on the controller at once.

Regarding amrstat, I'm all for including it in the base system. I would call
it amrctl (in the same vein as mlxctl/iopctl) and make stat one or more sub
commands e.g:

	amrctl stat ...

It would be interesting to look at what OpenBSD has done in this area but
admittedly that's another project!

Andrew