Bugzilla Bug 173347

I created a /etc/init.d/pciparm script, with the same purpose of the hdparm
script to provide a config file to put pci tweakings, like pci latency_timer

I don't know if others pci registers are useful to tweak else than
latency_timer. I'm happy to hear suggestions.

I'll attach /etc/init.d/pciparm and a sample /etc/conf.d/pciparm

Reproducible: Always

Steps to Reproduce:

Created an attachment (id=115443) [edit]
/etc/init.d/pciparm

Created an attachment (id=115444) [edit]
/etc/conf.d/pciparm

You cannot use bashism in init.d scripts.

(In reply to comment #3)
> You cannot use bashism in init.d scripts.

could you be more precise?

if you refer to the FLAT_ID="${pci_id//[:.]/_}", it is a plain substitution
with globbing, used also in other init scripts (netmount, net.lo, localmount,
...)

parameter name evaluation (that is: ${!VAR_THAT_HOLDS_IT}) also is used in
other init scripts

maybe I don't get what bashism means....?

> could you be more precise?
> 
> if you refer to the FLAT_ID="${pci_id//[:.]/_}", it is a plain substitution
> with globbing, used also in other init scripts (netmount, net.lo, localmount,
> ...)
> 
> parameter name evaluation (that is: ${!VAR_THAT_HOLDS_IT}) also is used in
> other init scripts
> 
> maybe I don't get what bashism means....?
> 
No you got it; now fix it ;) Basically you have to write portable shell in init
scripts, unlike ebuilds. #shells on irc.freenode.org i understand is a good
place to ask.

(In reply to comment #4)
> (In reply to comment #3)
> > You cannot use bashism in init.d scripts.
> 
> could you be more precise?
> 
> if you refer to the FLAT_ID="${pci_id//[:.]/_}", it is a plain substitution
> with globbing, used also in other init scripts (netmount, net.lo, localmount,
> ...)

You'll have to change that to sed :)

> parameter name evaluation (that is: ${!VAR_THAT_HOLDS_IT}) also is used in
> other init scripts

And you could write
eval foo=\$VAR_THAT_HOLDS_IT

> maybe I don't get what bashism means....?

It means don't use features specific to bash.
baselayout-2 will use /bin/sh as the shell to run init scripts with, which
although will default to bash on Linux, could be dash or busybox.

Created an attachment (id=115475) [edit]
/etc/init.d/pciparm (proposed fix)

okay I got it.
Let me say that the concept of `shell portability' is a bit vague: to make
things work for zsh is one thing, but make them work for tcsh is another
business =)

I used to refer to
http://www.gnu.org/software/autoconf/manual/autoconf-2.57/html_node/autoconf_114.html

some tips could go in the devmanual

(attached diff with fixes)

this is too specific ... it should be a "set pci options for pci devices", not
"set latency timings for pci devices"

(In reply to comment #8)
> this is too specific ... it should be a "set pci options for pci devices", I I concur. The script doesn't look bash-specific to me. Question is: does it work? If so, maybe it'll go in after the 2007.0 release has bedded in.. ie at least a month or two after release. Other params would be useful in the meantime.

(In reply to comment #8)
> this is too specific ... it should be a "set pci options for pci devices", not
> "set latency timings for pci devices"


OK

I've done generalized version of this script (going to upload it, naming it
-r1).

I'm sorry previous script wasn't working cause eval foo=\$VAR_THAT_HOLDS_IT
doesn't work. this time I used ${!USE_VAR_FROM_MY_VALUE} wich is also prettier

also conf.d syntax has changed to something more sane:

PCIPARM_ALL="latency_timer=b0"
PCIPARM_01_08_0="latency_timer=ff"
PCIPARM_00_04_0="latency_timer=ff"
PCIPARM_01_04_0="latency_timer=ff"
VERBOSE="no"

you can specify any pci register you want

Created an attachment (id=116247) [edit]
/etc/init.d/pciparm-r1

generalized version. tested.

Created an attachment (id=116248) [edit]
/etc/conf.d/pciparm-r1

config for new generalized version. removed ugly TWEAK_CARDS variable

Are there any other changes needed for this to be included? The syntax looks 
cool to me FWIW.

(In reply to comment #13)
> Are there any other changes needed for this to be included? The syntax looks 
> cool to me FWIW.

it's the script currently I am using on my systems.

I'm fine with it (no corrections since 2 months! should be enough :))

that script is better, but still restricts things to the bus view without
allowing vendor:device as well

at this point, it might be easier to just have a syntax like:
PCI_OPTS_BUS="
[[[[<domain>]:]<bus>]:][<slot>][.[<func>]] <options>
"

PCI_OPTS_VENDOR="
[<vendor>]:[<device>] <options>
"

(In reply to comment #15)

sorry for the delay. I'll be busy with exams til next week...

> at this point, it might be easier to just have a syntax like:
> PCI_OPTS_BUS="
> [[[[<domain>]:]<bus>]:][<slot>][.[<func>]] <options>
> "
> 
> PCI_OPTS_VENDOR="
> [<vendor>]:[<device>] <options>
> "

how do you mean that syntax working? perhaps not being parsed by a shell
interpreter (like bash) but instead parsed with sed/awk?
what I mean is: that way you can specify just one option.

maybe you mean to write config as:


PCI_OPTS_BUS[0]="..."
PCI_OPTS_BUS[1]="..."

PCI_OPTS_VENDOR[0]="..."
PCI_OPTS_VENDOR[1]="..."

?

Created an attachment (id=124859) [edit]
/etc/init.d/pciparm-r2

this one implements the bus/vendor specification.
syntax is the following:

PCIPARM_BUS_#="[[[[<domain>]:]<bus>]:][<slot>][.[<func>]] <options>"
PCIPARM_VENDOR_#="[<vendor>]:[<device>] <options>"

it will follow an updated example of /etc/conf.d/pciparm

Created an attachment (id=124861) [edit]
/etc/conf.d/pciparm-r2

I updated the script. The same conf syntax it is used by /etc/init.d/lm_sensor,
so I decided to borrow its code, not introducing anything new.

let me know ....

This construct is horrible:
VENDOR_OPT=`eval echo '$'PCIPARM_VENDOR_${SEQ_VENDOR}`

Here's a MUCH better one:
v="PCIPARM_VENDOR_${SEQ_VENDOR}"
VENDOR_OPT="${!v}"

Please fix.

Also, could you please either fix or document that the variables must be
sequentially numbered from zero?

Your examples should also all be commented out.

the problem with the latter is you'll get people complaining about non-POSIX

incvs

(In reply to comment #20)
> This construct is horrible:
> VENDOR_OPT=`eval echo '$'PCIPARM_VENDOR_${SEQ_VENDOR}`
> 
> Here's a MUCH better one:
> v="PCIPARM_VENDOR_${SEQ_VENDOR}"
> VENDOR_OPT="${!v}"
>
What Roy gave in comment #6 is the correct way to do this (given that we don't
have bash): 
eval foo=\$VAR_THAT_HOLDS_IT
so:
eval VENDOR_OPT=\$PCIPARM_VENDOR_$SEQ_VENDOR

You're right the above is horrible, and forks a subshell for no purpose.

Created an attachment (id=167638) [edit]
pci-parm.patch

Here's a patch, should be a bit faster, perhaps, and more extensible. (Didn't
bother error checking array names as passed in from literal in script.) Bit
more error-checking elsewhere (not sure if eerror is right, you get the gist
Xaero;)

SteveL: I got the gist of your changes, but in future, please patch against
what's the newest side, in this case what I had in the tree had already
diverged significantly from the submission here on the bug.

Created an attachment (id=167962) [edit]
init.d-pciparm.patch

Whoops my bad, robbat; just got curious about the code what with the discussion
on sh arrays.
Here's a patch against cvs; there were some POSIX incompatibilities, all use of
[[; bash as sh doesn't run in true POSIX mode by default. == is another no-no
unless it's in $((. Also [[ -z "$@" ]] might work in bash but it definitely
won't work in sh if we change it to [, as "$@" expands to multiple parameters.
The best ref is:
http://www.opengroup.org/onlinepubs/009695399/utilities/contents.html
(chapter 2 for syntax, but knowing the utility options is obviously very useful
too.)
While I was there I added back in the stuff to make the array loop a bit faster
(it is an initscript after all.) Using 'foo' as opposed to "foo" is in the same
vein.
I didn't change the calls to do_setpci_array back to an && sequence but I left
the code in place to deal with error (need to uncomment: # || return 1 on l.
63) My thinking was to bail as quickly as possible if there were an error, but
I can see why a user might want the ones which can be set, to be done (eg for a
stability fix.)
Regards, igli.

> While I was there I added back in the stuff to make the array loop a bit faster
No, your stuff is worse now. Readability trumps fast. If you'd hoisted the case
statement entirely out to the do_setpci_array, and removed it from do_setpci I
might have believed you, but you didn't.

What's with your crazy style of ;; on the next line, with no pattern in
whitespace? It should be on the end of branch of the case statement, on the
same line if it's one line, or if it's multiple, all indented.

> if [ "$VERBOSE" = yes ]; then
Why do you keep adding this? It was moved to making the user add -v in the
conf.d, and you're introducing a useless variable now AND clobbering any
SETPCI_OPTS they might have.

Also, the changes you made don't work at all. do_setpci_array needs the start
of the variable name as the first argument, not the switch. It looks like you
blindly ported the patch without any testing or actually understanding the
code.

Regardless, some of the changes are there.

(In reply to comment #27)
> > While I was there I added back in the stuff to make the array loop a bit faster
> No, your stuff is worse now. Readability trumps fast. If you'd hoisted the
> case statement entirely out to the do_setpci_array, and removed it from
> do_setpci I might have believed you, but you didn't.
>
Eh? What on Earth does belief have to do with anything? It either works or it
doesn't.

> What's with your crazy style of ;; on the next line, with no pattern in
> whitespace? It should be on the end of branch of the case statement, on the
> same line if it's one line, or if it's multiple, all indented.
>
Says who?
It's simply easier to maintain (and I _do_ know what I'm talking about in that
regard, believe it or not;) I'm not sure what you mean by "no pattern in
whitespace" unless you mean you'd like an extra space here or there.

> > if [ "$VERBOSE" = yes ]; then
> Why do you keep adding this? It was moved to making the user add -v in the
> conf.d, and you're introducing a useless variable now AND clobbering any
> SETPCI_OPTS they might have.
>
Ah well thanks for noting that.

> Also, the changes you made don't work at all. do_setpci_array needs the start
> of the variable name as the first argument, not the switch. It looks like you
> blindly ported the patch without any testing or actually understanding the
> code.
>
Yes that's exactly what I did for several reasons:
1) I don't use this.
2) I have no interest in it. I was just curious code-wise, as noted above and
in my earlier comment.
3) You'd have to test it and understand what it was doing in any event. Based
on your comments above, you have done, or at least more than before. I was just
re-integrating stuff that I thought useful into a throwaway patch on bugzilla,
and explaining why.

> Regardless, some of the changes are there.
> 
Great. Glad I could be of assistance. You'll forgive me for not wanting to do a
cvs browse to check it atm, I'm knackered and I really don't give an aerial
intercourse about it.