Subject: Re: control tool for amr(4)
To: Andrew Doran <ad@NetBSD.org>
From: Manuel Bouyer <bouyer@antioche.eu.org>
List: tech-kern
Date: 06/27/2006 23:01:58
--pWyiEgJYm5f9v55/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Tue, Jun 27, 2006 at 10:43:09AM +0100, Andrew Doran wrote:
> 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.
Sure, and I don't think it's very important for compat anyway. I changed it
to ENOTTY.
>
> + 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);
Ops ! I added this, thanks !
>
> + /* 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.
This should already be the case, because no more than one process
can have the device open at a time.
>
> 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 ...
This tool can only retrieve status actually. Maybe this can be changed later,
but I didn't find open-source tools to get the information on configuration
(and I'm mostly interested in status report anyway :)
But I can call it amrctl and implement stat as the only sub command.
>
> It would be interesting to look at what OpenBSD has done in this area but
> admittedly that's another project!
Sure, and I don't think it would be appropriate for pullup in netbsd-3 :)
On Tue, Jun 27, 2006 at 11:27:45AM +0100, Andrew Doran wrote:
> Few more comments..
>
> + if (au_length != 0 && au_cmd[0] != 0x06) {
> + dp = malloc(au_length, M_DEVBUF, M_WAITOK|M_ZERO);
>
> What does 0x06 signify?
No idea. This comes from FreeBSD, and this command isn't defined
in amrreg.h
>
> + error = amr_ccb_map(amr, ac, dp, au_length, 0);
>
> What if au_length == 0 or dp == NULL in this case? Which direction is the
> transfer in (for bus_dmamap_sync())? We might need to add a bi-directional
> flag.
Yes, just pass AC_XFER_IN|AC_XFER_OUT to amr_ccb_map() instead of a boolean
for direction.
> If you alter amr_ccb_map() to take a "proc *" argument and pass this to
> bus_dmamap_load() then the double buffering step becomes un-necessary.
>
> + if (au_length != 0) {
> + error = copyout(dp, au_buffer, au_length);
>
> It looks like this doesn't cover the case of (au_length != 0 and au_cmd[0]
> == 0x06).
Right, but it fact 0x06 isn't used by amrstat, so I changed it to just
return EINVAL if au_length == 0 or au_cmd[0] == 0x06.
Attached is a new diff for the driver part.
Thanks for the comments !
--
Manuel Bouyer <bouyer@antioche.eu.org>
NetBSD: 26 ans d'experience feront toujours la difference
--
--pWyiEgJYm5f9v55/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=diff
? sys/dev/pci/amrio.h
Index: sys/dev/pci/Makefile
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/Makefile,v
retrieving revision 1.11
diff -u -r1.11 Makefile
--- sys/dev/pci/Makefile 6 Dec 2005 11:53:56 -0000 1.11
+++ sys/dev/pci/Makefile 27 Jun 2006 21:00:06 -0000
@@ -5,7 +5,7 @@
INCSDIR= /usr/include/dev/pci
# Only install includes which are used by userland
-INCS= if_lmc.h mlyio.h mlyreg.h \
+INCS= amrreg.h amrio.h if_lmc.h mlyio.h mlyreg.h \
pcidevs.h pcidevs_data.h pciio.h pcireg.h \
tgareg.h twereg.h tweio.h
Index: sys/dev/pci/amr.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/amr.c,v
retrieving revision 1.35
diff -u -r1.35 amr.c
--- sys/dev/pci/amr.c 7 Jun 2006 22:33:36 -0000 1.35
+++ sys/dev/pci/amr.c 27 Jun 2006 21:00:06 -0000
@@ -81,6 +81,7 @@
#include <sys/proc.h>
#include <sys/buf.h>
#include <sys/malloc.h>
+#include <sys/conf.h>
#include <sys/kthread.h>
#include <uvm/uvm_extern.h>
@@ -92,6 +93,7 @@
#include <dev/pci/pcivar.h>
#include <dev/pci/amrreg.h>
#include <dev/pci/amrvar.h>
+#include <dev/pci/amrio.h>
#include "locators.h"
@@ -115,9 +117,20 @@
static int amr_std_get_work(struct amr_softc *, struct amr_mailbox_resp *);
static int amr_std_submit(struct amr_softc *, struct amr_ccb *);
+static dev_type_open(amropen);
+static dev_type_close(amrclose);
+static dev_type_ioctl(amrioctl);
+
CFATTACH_DECL(amr, sizeof(struct amr_softc),
amr_match, amr_attach, NULL, NULL);
+const struct cdevsw amr_cdevsw = {
+ amropen, amrclose, noread, nowrite, amrioctl,
+ nostop, notty, nopoll, nommap,
+};
+
+extern struct cfdriver amr_cd;
+
#define AT_QUARTZ 0x01 /* `Quartz' chipset */
#define AT_SIG 0x02 /* Check for signature */
@@ -260,7 +273,7 @@
const char *intrstr;
pcireg_t reg;
int rseg, i, j, size, rv, memreg, ioreg;
- struct amr_ccb *ac;
+ struct amr_ccb *ac;
int locs[AMRCF_NLOCS];
aprint_naive(": RAID controller\n");
@@ -719,7 +732,7 @@
static void
amr_shutdown(void *cookie)
{
- extern struct cfdriver amr_cd;
+ extern struct cfdriver amr_cd;
struct amr_softc *amr;
struct amr_ccb *ac;
int i, rv, s;
@@ -872,7 +885,7 @@
ac->ac_cmd.mb_command = AMR_CMD_ENQUIRY;
rv = amr_ccb_map(amr, ac, amr->amr_enqbuf,
- AMR_ENQUIRY_BUFSIZE, 0);
+ AMR_ENQUIRY_BUFSIZE, AC_XFER_IN);
if (rv != 0) {
printf("%s: ccb_map failed (%d)\n",
amr->amr_dv.dv_xname, rv);
@@ -948,7 +961,7 @@
mb[2] = cmdsub;
mb[3] = cmdqual;
- rv = amr_ccb_map(amr, ac, sbuf, AMR_ENQUIRY_BUFSIZE, 0);
+ rv = amr_ccb_map(amr, ac, sbuf, AMR_ENQUIRY_BUFSIZE, AC_XFER_IN);
if (rv == 0) {
rv = amr_ccb_poll(amr, ac, 2000);
amr_ccb_unmap(amr, ac);
@@ -1027,12 +1040,13 @@
*/
int
amr_ccb_map(struct amr_softc *amr, struct amr_ccb *ac, void *data, int size,
- int out)
+ int tflag)
{
struct amr_sgentry *sge;
struct amr_mailbox_cmd *mb;
int nsegs, i, rv, sgloff;
bus_dmamap_t xfer;
+ int dmaflag = 0;
xfer = ac->ac_xfer_map;
@@ -1043,9 +1057,14 @@
mb = &ac->ac_cmd;
ac->ac_xfer_size = size;
- ac->ac_flags |= (out ? AC_XFER_OUT : AC_XFER_IN);
+ ac->ac_flags |= (tflag & (AC_XFER_OUT | AC_XFER_IN));
sgloff = AMR_SGL_SIZE * ac->ac_ident;
+ if (tflag & AC_XFER_OUT)
+ dmaflag |= BUS_DMASYNC_PREWRITE;
+ if (tflag & AC_XFER_IN)
+ dmaflag |= BUS_DMASYNC_PREREAD;
+
/* We don't need to use a scatter/gather list for just 1 segment. */
nsegs = xfer->dm_nsegs;
if (nsegs == 1) {
@@ -1063,8 +1082,7 @@
}
}
- bus_dmamap_sync(amr->amr_dmat, xfer, 0, ac->ac_xfer_size,
- out ? BUS_DMASYNC_PREWRITE : BUS_DMASYNC_PREREAD);
+ bus_dmamap_sync(amr->amr_dmat, xfer, 0, ac->ac_xfer_size, dmaflag);
if ((ac->ac_flags & AC_NOSGL) == 0)
bus_dmamap_sync(amr->amr_dmat, amr->amr_dmamap, sgloff,
@@ -1079,14 +1097,19 @@
void
amr_ccb_unmap(struct amr_softc *amr, struct amr_ccb *ac)
{
+ int dmaflag = 0;
+
+ if (ac->ac_flags & AC_XFER_IN)
+ dmaflag |= BUS_DMASYNC_POSTREAD;
+ if (ac->ac_flags & AC_XFER_OUT)
+ dmaflag |= BUS_DMASYNC_POSTWRITE;
if ((ac->ac_flags & AC_NOSGL) == 0)
bus_dmamap_sync(amr->amr_dmat, amr->amr_dmamap,
AMR_SGL_SIZE * ac->ac_ident, AMR_SGL_SIZE,
BUS_DMASYNC_POSTWRITE);
bus_dmamap_sync(amr->amr_dmat, ac->ac_xfer_map, 0, ac->ac_xfer_size,
- (ac->ac_flags & AC_XFER_IN) != 0 ?
- BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE);
+ dmaflag);
bus_dmamap_unload(amr->amr_dmat, ac->ac_xfer_map);
}
@@ -1305,3 +1328,112 @@
printf("%08x ", ((u_int32_t *)&ac->ac_cmd)[i]);
printf("\n");
}
+
+static int
+amropen(dev_t dev, int flag, int mode, struct lwp *l)
+{
+ struct amr_softc *amr;
+
+ if ((amr = device_lookup(&amr_cd, minor(dev))) == NULL)
+ return (ENXIO);
+ if ((amr->amr_flags & AMRF_OPEN) != 0)
+ return (EBUSY);
+
+ amr->amr_flags |= AMRF_OPEN;
+ return (0);
+}
+
+static int
+amrclose(dev_t dev, int flag, int mode, struct lwp *l)
+{
+ struct amr_softc *amr;
+
+ amr = device_lookup(&amr_cd, minor(dev));
+ amr->amr_flags &= ~AMRF_OPEN;
+ return (0);
+}
+
+static int
+amrioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct lwp *l)
+{
+ struct amr_softc *amr;
+ struct amr_user_ioctl *au;
+ int *versionp;
+ struct amr_ccb *ac;
+ struct amr_mailbox_ioctl *mbi;
+ void *dp = NULL, *au_buffer;
+ unsigned long au_length;
+ unsigned char*au_cmd;
+ int error, s;
+
+ if (securelevel >= 2)
+ return (EPERM);
+
+ amr = device_lookup(&amr_cd, minor(dev));
+
+ /* This should be compatible with the FreeBSD interface */
+
+ switch(cmd) {
+ case AMR_IO_VERSION:
+ versionp = (void *)data;
+ *versionp = AMR_IO_VERSION_NUMBER;
+ return 0;
+ case AMR_IO_COMMAND:
+ au = (void *)data;
+ au_cmd = au->au_cmd;
+ au_buffer = au->au_buffer;
+ au_length = au->au_length;
+ break;
+ default:
+ return ENOTTY;
+ }
+
+ if (au_cmd[0] == AMR_CMD_PASS) {
+ /* not yet */
+ return EOPNOTSUPP;
+ }
+
+ if (au_length != 0 && au_cmd[0] != 0x06) {
+ if (au_length < 0 || au_length > MAXPHYS)
+ return (EINVAL);
+ dp = malloc(au_length, M_DEVBUF, M_WAITOK|M_ZERO);
+ if (dp == NULL)
+ return ENOMEM;
+ if ((error = copyin(au_buffer, dp, au_length)) != 0) {
+ free(dp, M_DEVBUF);
+ return (error);
+ }
+ } else {
+ 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;
+ }
+
+ mbi = (struct amr_mailbox_ioctl *)&ac->ac_cmd;
+ /* copy pertinent mailbox items */
+ mbi->mb_command = au_cmd[0];
+ mbi->mb_channel = au_cmd[1];
+ mbi->mb_param = au_cmd[2];
+ mbi->mb_pad[0] = au_cmd[3];
+ mbi->mb_drive = au_cmd[4];
+ /* build the command */
+ error = amr_ccb_map(amr, ac, dp, au_length, AC_XFER_IN | AC_XFER_OUT);
+ if (error)
+ goto out;
+ error = amr_ccb_wait(amr, ac);
+ amr_ccb_unmap(amr, ac);
+ amr_ccb_free(amr, ac);
+ if (error)
+ goto out;
+ error = copyout(dp, au_buffer, au_length);
+out:
+ splx(s);
+ if (dp)
+ free(dp, M_DEVBUF);
+ return error;
+}
Index: sys/dev/pci/amrreg.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/amrreg.h,v
retrieving revision 1.2
diff -u -r1.2 amrreg.h
--- sys/dev/pci/amrreg.h 4 May 2003 16:15:36 -0000 1.2
+++ sys/dev/pci/amrreg.h 27 Jun 2006 21:00:06 -0000
@@ -319,6 +319,7 @@
u_int32_t ae_drivesize[AMR_40LD_MAXDRIVES]; /* logical drive size */
u_int8_t ae_driveprop[AMR_40LD_MAXDRIVES]; /* logical drive properties */
u_int8_t ae_drivestate[AMR_40LD_MAXDRIVES]; /* physical drive state */
+ u_int8_t ae_pdrivestate[AMR_40LD_MAXPHYSDRIVES]; /* physical drive state */
u_int16_t ae_driveformat[AMR_40LD_MAXPHYSDRIVES];
u_int8_t ae_targxfer[80]; /* physical drive transfer rates */
Index: sys/dev/pci/amrvar.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/amrvar.h,v
retrieving revision 1.5
diff -u -r1.5 amrvar.h
--- sys/dev/pci/amrvar.h 11 Dec 2005 12:22:48 -0000 1.5
+++ sys/dev/pci/amrvar.h 27 Jun 2006 21:00:06 -0000
@@ -107,6 +107,7 @@
/* General flags. */
#define AMRF_THREAD_EXIT 0x00010000
+#define AMRF_OPEN 0x00020000
/*
* Command control block.
--pWyiEgJYm5f9v55/--