Re: CVS commit: [jdolecek-ncq] src/sys/dev/ata
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: src/usr.bin/make
On Jun 18, 12:36am, jo...@bec.de (Joerg Sonnenberger) wrote:
-- Subject: Re: CVS commit: src/usr.bin/make
| Please do not unilaterally change behavior. Especially if it has been
| discussed in the past. This is rude at best and not everyone shares your
| opinion.
Please explain the use case (aside for looking at the internal representation
of a variable). For example, if .OBJDIR, LIB, etc. ended up unexpanded, many
things would break. I just don't think it is useful to be the default, and
using -V '${var}' to get it to expand is counter-intuitive. Why should it
behave differently in the first place?
christos
Re: CVS commit: src/usr.bin/make
On Sun, Jun 18, 2017 at 12:40:20AM +0200, Kamil Rytarowski wrote:
> Can we reuse show-var from pkgsrc?
>
> $ make show-var VARNAME=MACHINE_CPU
> x86_64
It's no better than just using -V '${expr}' directly.
Joerg
Re: CVS commit: src/usr.bin/make
On 18.06.2017 00:31, Christos Zoulas wrote:
> In article <20170617222558.ga24...@britannica.bec.de>,
> Joerg Sonnenberger wrote:
>> On Sun, Jun 18, 2017 at 12:22:13AM +0200, Kamil Rytarowski wrote:
>>> On 18.06.2017 00:16, Christos Zoulas wrote:
In article <20170617213136.ga21...@britannica.bec.de>,
Joerg Sonnenberger wrote:
> On Sat, Jun 17, 2017 at 05:28:07PM -0400, Christos Zoulas wrote:
>> On Jun 17, 9:38pm, jo...@bec.de (Joerg Sonnenberger) wrote:
>> -- Subject: Re: CVS commit: src/usr.bin/make
>>
>> | Agreed, please revert. This was discussed at the time and FreeBSD
>> | behavior you have now implemented is much less useful.
>>
>> You can get the original with -V '\VAR'
>
> That's no better than the behavior before.
Now you get:
$ make -V MACHINE_CPU
arm
$ make -V \\MACHINE_CPU
>> ${MACHINE_ARCH:C/mipse[bl]/mips/:C/mips64e[bl]/mips/:C/sh3e[bl]/sh3/:S/coldfire/m68k/:S/m68000/m68k/:C/arm.*/arm/:C/earm.*/arm/:S/earm/arm/:S/powerpc64/powerpc/:S/aarch64eb/aarch64/:S/or1knd/or1k/:C/riscv../riscv/}
The second is the original version.
christos
>>>
>>> How about "make -V" getting the original behavior and "make -VV"
>>> resulting with evaluated one?
>>
>> I find -V '${foo}' a perfectly reasonable way to spell it, especially
>> since it works consistently with modifiers. No need for more complexity.
>
> And it still does. You cannot use -VV because of getopt(3). You can use
> a different letter. The complexity is when I get this long string instead
> of the evaluated variable.
>
> christos
>
Can we reuse show-var from pkgsrc?
$ make show-var VARNAME=MACHINE_CPU
x86_64
# show-var:
# show-vars:
# show-subdir-var:
# Convenience targets, to display make variables from the command
# line. Examples:
#
# make show-var VARNAME=PKGNAME
# make show-vars VARNAMES="PKGNAME PKGVERSION PKGREVISION"
# make show-subdir-var VARNAME=DISTFILES
#
# In category directories, show-var and show-vars descend
# recursively into each subdirectory, printing the variables of
# the individual packages. To show a variable from the category
# itself, use show-subdir-var.
.PHONY: show-var
show-var:
@${ECHO} ${${VARNAME}:Q
-- /usr/pkgsrc/mk/bsd.pkg.mk
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/usr.bin/make
On Sat, Jun 17, 2017 at 10:31:27PM +, Christos Zoulas wrote:
> In article <20170617222558.ga24...@britannica.bec.de>,
> Joerg Sonnenberger wrote:
> >On Sun, Jun 18, 2017 at 12:22:13AM +0200, Kamil Rytarowski wrote:
> >> On 18.06.2017 00:16, Christos Zoulas wrote:
> >> > In article <20170617213136.ga21...@britannica.bec.de>,
> >> > Joerg Sonnenberger wrote:
> >> >> On Sat, Jun 17, 2017 at 05:28:07PM -0400, Christos Zoulas wrote:
> >> >>> On Jun 17, 9:38pm, jo...@bec.de (Joerg Sonnenberger) wrote:
> >> >>> -- Subject: Re: CVS commit: src/usr.bin/make
> >> >>>
> >> >>> | Agreed, please revert. This was discussed at the time and FreeBSD
> >> >>> | behavior you have now implemented is much less useful.
> >> >>>
> >> >>> You can get the original with -V '\VAR'
> >> >>
> >> >> That's no better than the behavior before.
> >> >
> >> > Now you get:
> >> >
> >> > $ make -V MACHINE_CPU
> >> > arm
> >> > $ make -V \\MACHINE_CPU
> >> >
> >${MACHINE_ARCH:C/mipse[bl]/mips/:C/mips64e[bl]/mips/:C/sh3e[bl]/sh3/:S/coldfire/m68k/:S/m68000/m68k/:C/arm.*/arm/:C/earm.*/arm/:S/earm/arm/:S/powerpc64/powerpc/:S/aarch64eb/aarch64/:S/or1knd/or1k/:C/riscv../riscv/}
> >> >
> >> > The second is the original version.
> >> >
> >> > christos
> >> >
> >>
> >> How about "make -V" getting the original behavior and "make -VV"
> >> resulting with evaluated one?
> >
> >I find -V '${foo}' a perfectly reasonable way to spell it, especially
> >since it works consistently with modifiers. No need for more complexity.
>
> And it still does. You cannot use -VV because of getopt(3). You can use
> a different letter. The complexity is when I get this long string instead
> of the evaluated variable.
Please do not unilaterally change behavior. Especially if it has been
discussed in the past. This is rude at best and not everyone shares your
opinion.
Joerg
Re: CVS commit: src/usr.bin/make
In article <20170617222558.ga24...@britannica.bec.de>,
Joerg Sonnenberger wrote:
>On Sun, Jun 18, 2017 at 12:22:13AM +0200, Kamil Rytarowski wrote:
>> On 18.06.2017 00:16, Christos Zoulas wrote:
>> > In article <20170617213136.ga21...@britannica.bec.de>,
>> > Joerg Sonnenberger wrote:
>> >> On Sat, Jun 17, 2017 at 05:28:07PM -0400, Christos Zoulas wrote:
>> >>> On Jun 17, 9:38pm, jo...@bec.de (Joerg Sonnenberger) wrote:
>> >>> -- Subject: Re: CVS commit: src/usr.bin/make
>> >>>
>> >>> | Agreed, please revert. This was discussed at the time and FreeBSD
>> >>> | behavior you have now implemented is much less useful.
>> >>>
>> >>> You can get the original with -V '\VAR'
>> >>
>> >> That's no better than the behavior before.
>> >
>> > Now you get:
>> >
>> > $ make -V MACHINE_CPU
>> > arm
>> > $ make -V \\MACHINE_CPU
>> >
>${MACHINE_ARCH:C/mipse[bl]/mips/:C/mips64e[bl]/mips/:C/sh3e[bl]/sh3/:S/coldfire/m68k/:S/m68000/m68k/:C/arm.*/arm/:C/earm.*/arm/:S/earm/arm/:S/powerpc64/powerpc/:S/aarch64eb/aarch64/:S/or1knd/or1k/:C/riscv../riscv/}
>> >
>> > The second is the original version.
>> >
>> > christos
>> >
>>
>> How about "make -V" getting the original behavior and "make -VV"
>> resulting with evaluated one?
>
>I find -V '${foo}' a perfectly reasonable way to spell it, especially
>since it works consistently with modifiers. No need for more complexity.
And it still does. You cannot use -VV because of getopt(3). You can use
a different letter. The complexity is when I get this long string instead
of the evaluated variable.
christos
Re: CVS commit: src/usr.bin/make
On Sun, Jun 18, 2017 at 12:22:13AM +0200, Kamil Rytarowski wrote:
> On 18.06.2017 00:16, Christos Zoulas wrote:
> > In article <20170617213136.ga21...@britannica.bec.de>,
> > Joerg Sonnenberger wrote:
> >> On Sat, Jun 17, 2017 at 05:28:07PM -0400, Christos Zoulas wrote:
> >>> On Jun 17, 9:38pm, jo...@bec.de (Joerg Sonnenberger) wrote:
> >>> -- Subject: Re: CVS commit: src/usr.bin/make
> >>>
> >>> | Agreed, please revert. This was discussed at the time and FreeBSD
> >>> | behavior you have now implemented is much less useful.
> >>>
> >>> You can get the original with -V '\VAR'
> >>
> >> That's no better than the behavior before.
> >
> > Now you get:
> >
> > $ make -V MACHINE_CPU
> > arm
> > $ make -V \\MACHINE_CPU
> > ${MACHINE_ARCH:C/mipse[bl]/mips/:C/mips64e[bl]/mips/:C/sh3e[bl]/sh3/:S/coldfire/m68k/:S/m68000/m68k/:C/arm.*/arm/:C/earm.*/arm/:S/earm/arm/:S/powerpc64/powerpc/:S/aarch64eb/aarch64/:S/or1knd/or1k/:C/riscv../riscv/}
> >
> > The second is the original version.
> >
> > christos
> >
>
> How about "make -V" getting the original behavior and "make -VV"
> resulting with evaluated one?
I find -V '${foo}' a perfectly reasonable way to spell it, especially
since it works consistently with modifiers. No need for more complexity.
Joerg
Re: CVS commit: src/usr.bin/make
On 18.06.2017 00:16, Christos Zoulas wrote:
> In article <20170617213136.ga21...@britannica.bec.de>,
> Joerg Sonnenberger wrote:
>> On Sat, Jun 17, 2017 at 05:28:07PM -0400, Christos Zoulas wrote:
>>> On Jun 17, 9:38pm, jo...@bec.de (Joerg Sonnenberger) wrote:
>>> -- Subject: Re: CVS commit: src/usr.bin/make
>>>
>>> | Agreed, please revert. This was discussed at the time and FreeBSD
>>> | behavior you have now implemented is much less useful.
>>>
>>> You can get the original with -V '\VAR'
>>
>> That's no better than the behavior before.
>
> Now you get:
>
> $ make -V MACHINE_CPU
> arm
> $ make -V \\MACHINE_CPU
> ${MACHINE_ARCH:C/mipse[bl]/mips/:C/mips64e[bl]/mips/:C/sh3e[bl]/sh3/:S/coldfire/m68k/:S/m68000/m68k/:C/arm.*/arm/:C/earm.*/arm/:S/earm/arm/:S/powerpc64/powerpc/:S/aarch64eb/aarch64/:S/or1knd/or1k/:C/riscv../riscv/}
>
> The second is the original version.
>
> christos
>
How about "make -V" getting the original behavior and "make -VV"
resulting with evaluated one?
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/usr.bin/make
In article <20170617213136.ga21...@britannica.bec.de>,
Joerg Sonnenberger wrote:
>On Sat, Jun 17, 2017 at 05:28:07PM -0400, Christos Zoulas wrote:
>> On Jun 17, 9:38pm, jo...@bec.de (Joerg Sonnenberger) wrote:
>> -- Subject: Re: CVS commit: src/usr.bin/make
>>
>> | Agreed, please revert. This was discussed at the time and FreeBSD
>> | behavior you have now implemented is much less useful.
>>
>> You can get the original with -V '\VAR'
>
>That's no better than the behavior before.
Now you get:
$ make -V MACHINE_CPU
arm
$ make -V \\MACHINE_CPU
${MACHINE_ARCH:C/mipse[bl]/mips/:C/mips64e[bl]/mips/:C/sh3e[bl]/sh3/:S/coldfire/m68k/:S/m68000/m68k/:C/arm.*/arm/:C/earm.*/arm/:S/earm/arm/:S/powerpc64/powerpc/:S/aarch64eb/aarch64/:S/or1knd/or1k/:C/riscv../riscv/}
The second is the original version.
christos
Re: CVS commit: src/usr.bin/make
On Sat, Jun 17, 2017 at 05:28:07PM -0400, Christos Zoulas wrote: > On Jun 17, 9:38pm, jo...@bec.de (Joerg Sonnenberger) wrote: > -- Subject: Re: CVS commit: src/usr.bin/make > > | Agreed, please revert. This was discussed at the time and FreeBSD > | behavior you have now implemented is much less useful. > > You can get the original with -V '\VAR' That's no better than the behavior before. Joerg
Re: CVS commit: src/usr.bin/make
On Jun 17, 9:38pm, jo...@bec.de (Joerg Sonnenberger) wrote: -- Subject: Re: CVS commit: src/usr.bin/make | Agreed, please revert. This was discussed at the time and FreeBSD | behavior you have now implemented is much less useful. You can get the original with -V '\VAR' christos
Re: CVS commit: src/usr.bin/make
On Sun, Jun 18, 2017 at 05:15:48AM +1000, matthew green wrote:
> "Christos Zoulas" writes:
> > Module Name:src
> > Committed By: christos
> > Date: Sat Jun 17 15:49:56 UTC 2017
> >
> > Modified Files:
> > src/usr.bin/make: main.c
> >
> > Log Message:
> > -V: try to expand the variable again if the value contains a variable.
>
> how do i get the original behaviour? i like having both
> that and the -V "${FOO}" version to expand. they're both
> useful in different ways.
Agreed, please revert. This was discussed at the time and FreeBSD
behavior you have now implemented is much less useful.
Joerg
re: CVS commit: src/usr.bin/make
"Christos Zoulas" writes:
> Module Name: src
> Committed By: christos
> Date: Sat Jun 17 15:49:56 UTC 2017
>
> Modified Files:
> src/usr.bin/make: main.c
>
> Log Message:
> -V: try to expand the variable again if the value contains a variable.
how do i get the original behaviour? i like having both
that and the -V "${FOO}" version to expand. they're both
useful in different ways.
.mrg.
Re: CVS commit: [jdolecek-ncq] src/sys/dev/ata
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
On Fri, Jun 16, 2017 at 08:40:49PM +, Jaromir Dolecek wrote: > Module Name: src > Committed By: jdolecek > Date: Fri Jun 16 20:40:49 UTC 2017 > > Modified Files: > src/sys/dev/ata [jdolecek-ncq]: ata.c ata_wdc.c atavar.h wd.c > src/sys/dev/ic [jdolecek-ncq]: ahcisata_core.c mvsata.c siisata.c wdc.c > src/sys/dev/scsipi [jdolecek-ncq]: atapi_wdc.c > > Log Message: > adjust reset channel and dump paths > - channel reset now always kills active transfer, even on dump path, but > now doesn't touch the queued waiting transfers; also kill_xfer hook is > always called, so that HBA can free any private xfer resources and thus > the dump request has chance to work Sounds like a recipe for more lost/corrupt data. Remember, channel reset is requestable from userland. > - kill_xfer routines now always call ata_deactivate_xfer(); added KASSERT()s > to ata_free_xfer() to expect deactivated xfer > - when called during channel reset before dump, ata_kill_active() drops > any queued waiting transfers without processing Again, as above. > - do not (re)queue any transfers in wddone() when dumping > - kill AT_RST_NOCMD flag > > This should also hopefully fix the 'polled command has been queued' panic > as reported in: > PR kern/11811 by John Hawkinson > PR kern/47041 by Taylor R Campbell > PR kern/51979 by Martin Husemann > > dump tested working with piixide(4) and ahci(4). mvsata(4) dump times out, > but otherwise tested working, will be fixed separately. siisata(4) > mechanically > changed and not tested. > > > To generate a diff of this commit: > cvs rdiff -u -r1.132.8.8 -r1.132.8.9 src/sys/dev/ata/ata.c > cvs rdiff -u -r1.105.6.3 -r1.105.6.4 src/sys/dev/ata/ata_wdc.c > cvs rdiff -u -r1.92.8.8 -r1.92.8.9 src/sys/dev/ata/atavar.h > cvs rdiff -u -r1.428.2.15 -r1.428.2.16 src/sys/dev/ata/wd.c > cvs rdiff -u -r1.57.6.12 -r1.57.6.13 src/sys/dev/ic/ahcisata_core.c > cvs rdiff -u -r1.35.6.10 -r1.35.6.11 src/sys/dev/ic/mvsata.c > cvs rdiff -u -r1.30.4.15 -r1.30.4.16 src/sys/dev/ic/siisata.c > cvs rdiff -u -r1.283.2.4 -r1.283.2.5 src/sys/dev/ic/wdc.c > cvs rdiff -u -r1.123.4.4 -r1.123.4.5 src/sys/dev/scsipi/atapi_wdc.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
