Re: Call for testers, users with scsi cards
On Tue, 2012-12-04 at 14:58 -1000, Jeff Roberson wrote: On Tue, 4 Dec 2012, Ian Lepore wrote: On Tue, 2012-12-04 at 14:49 -0700, Warner Losh wrote: On Dec 4, 2012, at 2:36 PM, Jeff Roberson wrote: http://people.freebsd.org/~jeff/loadccb.diff This patch consolidates all of the functions that map cam control blocks for DMA into one central function. This change is a precursor to adding new features to the I/O stack. It is mostly mechanical. If you are running current on a raid or scsi card, especially if it is a lesser used one, I would really like you to apply this patch and report back any problems. If it works you should notice nothing. If it doesn't work you will probably panic immediately on I/O or otherwise no I/O will happen. I haven't tested it yet. My only comment from reading it though would be to make subr_busdma.c be dependent on cam, since it can only used from cam. We've grown sloppy about noting these dependencies in the tree... Warner Hmmm, if it's only used by cam, why isn't it in cam/ rather than kern/ ? kib pointed out drivers that use ccbs but do not depend on cam. Ahh, I didn't realize. I also intend to consolidate many of the busdma_load_* functions into this subr_busdma.c eventually. I will add a load_bio and things like load_uio and load_mbuf don't need to be re-implemented for every machine. I will define a MD function that allows you to add virtual or physical segments piecemeal (as they all currently have) so that function may be called for each member in the uio, mbuf, ccb, or bio. I'm afraid the current near-identicalness of things like the load_mbuf implementations have more to do with the cut-and-paste nature of how the non-x86 implementations came to be, rather than actual correctness. A proper implementation of the load_mbuf routines on architectures with VIVT cache should involve setting some flags in the map so that the sync operations can be handled differently for mbufs than for anonymous memory. (Mbufs are allowed to bend the rules about DMA buffers being aligned to cacheline boundaries.) The uio-related busdma operations for VIVT cache platforms are probably just plain wrong -- like would cause a panic type wrong if they were actually invoked. I posted a set of patches that fix all the problems I know of in the armv4 busdma implementation, except for the uio stuff. It didn't get much comment at the time and lacks a champion who can actually commit the code. They won't even apply cleanly anymore because of other changes that have happened, I guess I should go re-spin the patchset and post it again. -- Ian ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On Tue, Dec 4, 2012 at 2:36 PM, Jeff Roberson jrober...@jroberson.netwrote:
http://people.freebsd.org/~**jeff/loadccb.diffhttp://people.freebsd.org/~jeff/loadccb.diff
This patch consolidates all of the functions that map cam control blocks
for DMA into one central function. This change is a precursor to adding
new features to the I/O stack. It is mostly mechanical. If you are
running current on a raid or scsi card, especially if it is a lesser used
one, I would really like you to apply this patch and report back any
problems. If it works you should notice nothing. If it doesn't work you
will probably panic immediately on I/O or otherwise no I/O will happen.
Hi Jeff,
This patch breaks both ahci and isci on my system. I still need to root
cause the isci panic, but I have some details on ahci.
Fatal trap 12: page fault while in kernel mode
cpuid = 0; apic id = 00
fault virtual address = 0x6c
fault code = supervisor read data, page not present
instruction pointer = 0x20:0x80314f98
stack pointer = 0x28:0xff884433a130
frame pointer = 0x28:0xff884433a1d0
code segment= base 0x0, limit 0xf, type 0x1b
= DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags= interrupt enabled, resume, IOPL = 0
current process = 4 (xpt_thrd)
[ thread pid 4 tid 100174 ]
Stopped at ahci_dmasetprd+0xb8:movl0x6c(%rcx),%eax
db bt
Tracing pid 4 tid 100174 td 0xfe002c080480
ahci_dmasetprd() at ahci_dmasetprd+0xb8/frame 0xff884433a1d0
bus_dmamap_load() at bus_dmamap_load+0x91/frame 0xff884433a230
bus_dmamap_load_ccb() at bus_dmamap_load_ccb+0xf0/frame 0xff884433a280
ahci_dmasetprd() at ahci_dmasetprd+0x82e/frame 0xff884433a320
bus_dmamap_load_ccb() at bus_dmamap_load_ccb+0x3b/frame 0xff884433a370
ahciaction() at ahciaction+0x7d4/frame 0xff884433a3b0
xpt_run_dev_sendq() at xpt_run_dev_sendq+0x2a1/frame 0xff884433a3f0
xpt_action_default() at xpt_action_default+0x10bd/frame 0xff884433a480
probestart() at probestart+0x1e5/frame 0xff884433a5d0
xpt_run_dev_allocq() at xpt_run_dev_allocq+0x192/frame 0xff884433a610
proberegister() at proberegister+0xf9/frame 0xff884433a630
cam_periph_alloc() at cam_periph_alloc+0x571/frame 0xff884433a710
ata_scan_lun() at ata_scan_lun+0x147/frame 0xff884433a920
ata_scan_bus() at ata_scan_bus+0x2c0/frame 0xff884433aa40
xpt_scanner_thread() at xpt_scanner_thread+0x161/frame 0xff884433aa70
fork_exit() at fork_exit+0x9a/frame 0xff884433aab0
fork_trampoline() at fork_trampoline+0xe/frame 0xff884433aab0
The following patch to ahci.c works, but hopefully someone more familiar
with ahci can chime in.
Index: sys/dev/ahci/ahci.c
===
--- sys/dev/ahci/ahci.c (revision 243900)
+++ sys/dev/ahci/ahci.c (working copy)
@@ -1669,19 +1669,9 @@
slot-dma.nsegs = 0;
/* If request moves data, setup and load SG list */
if ((ccb-ccb_h.flags CAM_DIR_MASK) != CAM_DIR_NONE) {
- void *buf;
- bus_size_t size;
-
slot-state = AHCI_SLOT_LOADING;
- if (ccb-ccb_h.func_code == XPT_ATA_IO) {
- buf = ccb-ataio.data_ptr;
- size = ccb-ataio.dxfer_len;
- } else {
- buf = ccb-csio.data_ptr;
- size = ccb-csio.dxfer_len;
- }
- bus_dmamap_load(ch-dma.data_tag, slot-dma.data_map,
- buf, size, ahci_dmasetprd, slot, 0);
+ bus_dmamap_load_ccb(ch-dma.data_tag, slot-dma.data_map,
ccb,
+ ahci_dmasetprd, slot, 0);
} else
ahci_execute_transaction(slot);
}
-Jim
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On Tue, Dec 4, 2012 at 2:36 PM, Jeff Roberson jrober...@jroberson.netwrote:
http://people.freebsd.org/~**jeff/loadccb.diffhttp://people.freebsd.org/~jeff/loadccb.diff
This patch consolidates all of the functions that map cam control blocks
for DMA into one central function. This change is a precursor to adding
new features to the I/O stack. It is mostly mechanical. If you are
running current on a raid or scsi card, especially if it is a lesser used
one, I would really like you to apply this patch and report back any
problems. If it works you should notice nothing. If it doesn't work you
will probably panic immediately on I/O or otherwise no I/O will happen.
+int
+bus_dmamap_load_ccb(bus_dma_tag_t dmat, bus_dmamap_t map, union ccb *ccb,
+bus_dmamap_callback_t *callback, void *callback_arg,
+int flags)
+{
+struct ccb_ataio *ataio;
+struct ccb_scsiio *csio;
+struct ccb_hdr *ccb_h;
+void *data_ptr;
+uint32_t dxfer_len;
+uint16_t sglist_cnt;
+
+ccb_h = ccb-ccb_h;
+if ((ccb_h-flags CAM_DIR_MASK) == CAM_DIR_NONE) {
+callback(callback_arg, NULL, 0, 0);
+}
+
I think you need to return here after invoking the callback. Otherwise you
drop through and then either invoke the callback again or call
bus_dmamap_load (which will in turn invoke the callback again).
This fix allows the ahci.c change to go back to:
Index: sys/dev/ahci/ahci.c
===
--- sys/dev/ahci/ahci.c (revision 243900)
+++ sys/dev/ahci/ahci.c (working copy)
@@ -1667,23 +1667,9 @@
(ccb-ataio.cmd.flags (CAM_ATAIO_CONTROL |
CAM_ATAIO_NEEDRESULT)))
ch-aslots |= (1 slot-slot);
slot-dma.nsegs = 0;
- /* If request moves data, setup and load SG list */
- if ((ccb-ccb_h.flags CAM_DIR_MASK) != CAM_DIR_NONE) {
- void *buf;
- bus_size_t size;
-
- slot-state = AHCI_SLOT_LOADING;
- if (ccb-ccb_h.func_code == XPT_ATA_IO) {
- buf = ccb-ataio.data_ptr;
- size = ccb-ataio.dxfer_len;
- } else {
- buf = ccb-csio.data_ptr;
- size = ccb-csio.dxfer_len;
- }
- bus_dmamap_load(ch-dma.data_tag, slot-dma.data_map,
- buf, size, ahci_dmasetprd, slot, 0);
- } else
- ahci_execute_transaction(slot);
+ slot-state = AHCI_SLOT_LOADING;
+ bus_dmamap_load_ccb(ch-dma.data_tag, slot-dma.data_map, ccb,
+ ahci_dmasetprd, slot, 0);
}
/* Locked by busdma engine. */
This is almost what you head earlier, but adding back setting of the slot's
state to AHCI_SLOT_LOADING, to cover the case where the load is deferred.
It seems OK to do this even in case where no load is actually happening
(i.e. CAM_DIR_NONE).
-Jim
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On Wed, 5 Dec 2012, Jim Harris wrote:
On Tue, Dec 4, 2012 at 2:36 PM, Jeff Roberson jrober...@jroberson.net
wrote:
http://people.freebsd.org/~jeff/loadccb.diff
This patch consolidates all of the functions that map cam
control blocks for DMA into one central function. This change
is a precursor to adding new features to the I/O stack. It is
mostly mechanical. If you are running current on a raid or scsi
card, especially if it is a lesser used one, I would really like
you to apply this patch and report back any problems. If it
works you should notice nothing. If it doesn't work you will
probably panic immediately on I/O or otherwise no I/O will
happen.
+int
+bus_dmamap_load_ccb(bus_dma_tag_t dmat, bus_dmamap_t map, union ccb *ccb,
+ bus_dmamap_callback_t *callback, void *callback_arg,
+ int flags)
+{
+ struct ccb_ataio *ataio;
+ struct ccb_scsiio *csio;
+ struct ccb_hdr *ccb_h;
+ void *data_ptr;
+ uint32_t dxfer_len;
+ uint16_t sglist_cnt;
+
+ ccb_h = ccb-ccb_h;
+ if ((ccb_h-flags CAM_DIR_MASK) == CAM_DIR_NONE) {
+ callback(callback_arg, NULL, 0, 0);
+ }
+
I think you need to return here after invoking the callback. Otherwise you
drop through and then either invoke the callback again or call
bus_dmamap_load (which will in turn invoke the callback again).
This fix allows the ahci.c change to go back to:
Thanks Jim. That was silly of me. I have decided to move this work to a
branch and keep expanding on it. I'll solicit more testing once the
branch is closer to the ultimate goal.
Thanks,
Jeff
Index: sys/dev/ahci/ahci.c
===
--- sys/dev/ahci/ahci.c (revision 243900)
+++ sys/dev/ahci/ahci.c (working copy)
@@ -1667,23 +1667,9 @@
(ccb-ataio.cmd.flags (CAM_ATAIO_CONTROL |
CAM_ATAIO_NEEDRESULT)))
ch-aslots |= (1 slot-slot);
slot-dma.nsegs = 0;
- /* If request moves data, setup and load SG list */
- if ((ccb-ccb_h.flags CAM_DIR_MASK) != CAM_DIR_NONE) {
- void *buf;
- bus_size_t size;
-
- slot-state = AHCI_SLOT_LOADING;
- if (ccb-ccb_h.func_code == XPT_ATA_IO) {
- buf = ccb-ataio.data_ptr;
- size = ccb-ataio.dxfer_len;
- } else {
- buf = ccb-csio.data_ptr;
- size = ccb-csio.dxfer_len;
- }
- bus_dmamap_load(ch-dma.data_tag, slot-dma.data_map,
- buf, size, ahci_dmasetprd, slot, 0);
- } else
- ahci_execute_transaction(slot);
+ slot-state = AHCI_SLOT_LOADING;
+ bus_dmamap_load_ccb(ch-dma.data_tag, slot-dma.data_map, ccb,
+ ahci_dmasetprd, slot, 0);
}
/* Locked by busdma engine. */
This is almost what you head earlier, but adding back setting of the slot's
state to AHCI_SLOT_LOADING, to cover the case where the load is deferred.
It seems OK to do this even in case where no load is actually happening
(i.e. CAM_DIR_NONE).
-Jim
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On Wed, Dec 5, 2012 at 1:56 PM, Jeff Roberson jrober...@jroberson.netwrote: Thanks Jim. That was silly of me. I have decided to move this work to a branch and keep expanding on it. I'll solicit more testing once the branch is closer to the ultimate goal. Thanks, Jeff Sounds good. FYI - that same change (returning after invoking the callback) fixes the isci panic as well. Your patch also uncovered a different issue in the isci driver that I just fixed in r243904. It will likely cause a merge conflict next time you rebase your physbio branch. Thanks, -Jim ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Call for testers, users with scsi cards
http://people.freebsd.org/~jeff/loadccb.diff This patch consolidates all of the functions that map cam control blocks for DMA into one central function. This change is a precursor to adding new features to the I/O stack. It is mostly mechanical. If you are running current on a raid or scsi card, especially if it is a lesser used one, I would really like you to apply this patch and report back any problems. If it works you should notice nothing. If it doesn't work you will probably panic immediately on I/O or otherwise no I/O will happen. Thanks, Jeff ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On Dec 4, 2012, at 2:36 PM, Jeff Roberson wrote: http://people.freebsd.org/~jeff/loadccb.diff This patch consolidates all of the functions that map cam control blocks for DMA into one central function. This change is a precursor to adding new features to the I/O stack. It is mostly mechanical. If you are running current on a raid or scsi card, especially if it is a lesser used one, I would really like you to apply this patch and report back any problems. If it works you should notice nothing. If it doesn't work you will probably panic immediately on I/O or otherwise no I/O will happen. I haven't tested it yet. My only comment from reading it though would be to make subr_busdma.c be dependent on cam, since it can only used from cam. We've grown sloppy about noting these dependencies in the tree... Warner ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On Tue, 2012-12-04 at 14:49 -0700, Warner Losh wrote: On Dec 4, 2012, at 2:36 PM, Jeff Roberson wrote: http://people.freebsd.org/~jeff/loadccb.diff This patch consolidates all of the functions that map cam control blocks for DMA into one central function. This change is a precursor to adding new features to the I/O stack. It is mostly mechanical. If you are running current on a raid or scsi card, especially if it is a lesser used one, I would really like you to apply this patch and report back any problems. If it works you should notice nothing. If it doesn't work you will probably panic immediately on I/O or otherwise no I/O will happen. I haven't tested it yet. My only comment from reading it though would be to make subr_busdma.c be dependent on cam, since it can only used from cam. We've grown sloppy about noting these dependencies in the tree... Warner Hmmm, if it's only used by cam, why isn't it in cam/ rather than kern/ ? -- Ian ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On Tue, 4 Dec 2012, Ian Lepore wrote: On Tue, 2012-12-04 at 14:49 -0700, Warner Losh wrote: On Dec 4, 2012, at 2:36 PM, Jeff Roberson wrote: http://people.freebsd.org/~jeff/loadccb.diff This patch consolidates all of the functions that map cam control blocks for DMA into one central function. This change is a precursor to adding new features to the I/O stack. It is mostly mechanical. If you are running current on a raid or scsi card, especially if it is a lesser used one, I would really like you to apply this patch and report back any problems. If it works you should notice nothing. If it doesn't work you will probably panic immediately on I/O or otherwise no I/O will happen. I haven't tested it yet. My only comment from reading it though would be to make subr_busdma.c be dependent on cam, since it can only used from cam. We've grown sloppy about noting these dependencies in the tree... Warner Hmmm, if it's only used by cam, why isn't it in cam/ rather than kern/ ? kib pointed out drivers that use ccbs but do not depend on cam. I also intend to consolidate many of the busdma_load_* functions into this subr_busdma.c eventually. I will add a load_bio and things like load_uio and load_mbuf don't need to be re-implemented for every machine. I will define a MD function that allows you to add virtual or physical segments piecemeal (as they all currently have) so that function may be called for each member in the uio, mbuf, ccb, or bio. Thanks, Jeff -- Ian ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On 12/4/2012 1:36 PM, Jeff Roberson wrote: http://people.freebsd.org/~jeff/loadccb.diff looks ok for isp. This doesn't do diddly for target mode- do you know off the top of your head if it will break anything there? ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On Dec 4, 2012, at 5:58 PM, Jeff Roberson wrote: On Tue, 4 Dec 2012, Ian Lepore wrote: On Tue, 2012-12-04 at 14:49 -0700, Warner Losh wrote: On Dec 4, 2012, at 2:36 PM, Jeff Roberson wrote: http://people.freebsd.org/~jeff/loadccb.diff This patch consolidates all of the functions that map cam control blocks for DMA into one central function. This change is a precursor to adding new features to the I/O stack. It is mostly mechanical. If you are running current on a raid or scsi card, especially if it is a lesser used one, I would really like you to apply this patch and report back any problems. If it works you should notice nothing. If it doesn't work you will probably panic immediately on I/O or otherwise no I/O will happen. I haven't tested it yet. My only comment from reading it though would be to make subr_busdma.c be dependent on cam, since it can only used from cam. We've grown sloppy about noting these dependencies in the tree... Warner Hmmm, if it's only used by cam, why isn't it in cam/ rather than kern/ ? kib pointed out drivers that use ccbs but do not depend on cam. I also intend to consolidate many of the busdma_load_* functions into this subr_busdma.c eventually. I will add a load_bio and things like load_uio and load_mbuf don't need to be re-implemented for every machine. I will define a MD function that allows you to add virtual or physical segments piecemeal (as they all currently have) so that function may be called for each member in the uio, mbuf, ccb, or bio. Sounds like a good reason to me... Look forward to it... Warner Thanks, Jeff -- Ian ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Call for testers, users with scsi cards
On Dec 4, 2012, at 6:18 PM, Matthew Jacob m...@feral.com wrote: On 12/4/2012 1:36 PM, Jeff Roberson wrote: http://people.freebsd.org/~jeff/loadccb.diff looks ok for isp. This doesn't do diddly for target mode- do you know off the top of your head if it will break anything there? It will. I believe all that's required to fix it is to add XPT_CONT_TARGET_IO to the XPT_SCSI_IO case. -- Justin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
