Re: CVS commit: [jdolecek-ncq] src/sys/dev/ata

[Jaromír Doleček]

2017-06-17 20:00 GMT+02:00 Jonathan A. Kollasch :
>
> I really think we need to make ata_xfer_init() private to
> ata_queue_alloc(), and use ata_get_xfer() everywhere.  That we can
> fabricate an xfer on the stack anywhere that will trample all over an
> active command slot is fundamentally wrong.

Any non-NCQ command which goes via atastart() is safe, since that routine
makes sure there is only one non-NCQ command being executed by HBA.

The on-stack xfer was carried over from previous design. I kept it mostly
since I didn't want to change those codepaths to deal with xfer not being
available.

As far as I see, ahcisata(4) reset should be actually fine, since it does
the reset
without using the xfers/slots.

I had a look at siisata_reset_drive(). That thing indeed can't reliably
work as-is.
Arguably it should be changed to use the on-stack xfer, currently it e.g.
doesn't
do anything if all slots are taken, which is wrong. I'll have a look.

> ata_queue_downsize() currently appears to leak xfers.  Additionally once
> you put a QD1 drive on the channel you're stuck there forever, even if
> you replace the channel population entirely with QD32 drives.

Okay, I need to look at this - original assumption was that it's never
called while there are other transfers. This is false with PMP.

Jaromir

Re: CVS commit: [jdolecek-ncq] src/sys/dev/ata

[Jonathan A. Kollasch]

On Sat, Jun 17, 2017 at 02:01:36PM +, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Sat Jun 17 14:01:36 UTC 2017
> 
> Modified Files:
>   src/sys/dev/ata [jdolecek-ncq]: TODO.ncq ata.c satapmp_subr.c
> 
> Log Message:
> make PMP working great again
> 
> tested with mvsata(4), my ahcisata(4) controller unfortunately doesn't
> support PMP

but mvsata(4) is special snowflake compared to ahcisata(4) and siisata(4):

siisata0 port 1: device present, speed: 3.0Gb/s
atabus1: SATA port multiplier, 6 ports
atabus1 PMP port 0: device present, speed: 3.0Gb/s
uvm_fault(0x8161c3c0, 0x0, 2) -> e
fatal page fault in supervisor mode
trap type 6 code 0x2 rip 0x80268550 cs 0x8 rflags 0x10246 cr2 0
ilevel 0 rsp 0xfe80029e6d50
curlwp 0xfe8002a541a0 pid 0.51 lowest kstack 0xfe80029e32c0
kernel: page fault trap, code=0
Stopped in pid 0.51 (system) at netbsd:ata_activate_xfer+0x69:  movq 
%rdx,0(%rax)
db{0}> bt
ata_activate_xfer() at netbsd:ata_activate_xfer+0x69
siisata_reset_drive() at netbsd:siisata_reset_drive+0xf8
satapmp_rescan() at netbsd:satapmp_rescan+0x216
satapmp_attach() at netbsd:satapmp_attach+0xfe
atabusconfig_thread() at netbsd:atabusconfig_thread+0x381
db{0}> 


I really think we need to make ata_xfer_init() private to
ata_queue_alloc(), and use ata_get_xfer() everywhere.  That we can
fabricate an xfer on the stack anywhere that will trample all over an
active command slot is fundamentally wrong.

ata_queue_downsize() currently appears to leak xfers.  Additionally once
you put a QD1 drive on the channel you're stuck there forever, even if
you replace the channel population entirely with QD32 drives.

Jonathan Kollasch

Re: CVS commit: [jdolecek-ncq] src/sys/dev/ata

[coypu]