Re: Call for testers, users with scsi cards

[Jim Harris]

On Wed, Dec 5, 2012 at 1:56 PM, Jeff Roberson wrote:

> 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"

Re: Call for testers, users with scsi cards

[Jeff Roberson]

On Wed, 5 Dec 2012, Jim Harris wrote:




On Tue, 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.


+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

[Jim Harris]

On Tue, 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.
>
>
+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

[Jim Harris]

On Tue, 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.
>
>
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

[Ian Lepore]

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

[Justin T. Gibbs]

On Dec 4, 2012, at 6:18 PM, Matthew Jacob  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"

Re: Call for testers, users with scsi cards

[Warner Losh]

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

[Matthew Jacob]

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

[Jeff Roberson]

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

[Ian Lepore]

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

[Warner Losh]

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"