[PATCH] Documentation: Fix config_acs= example

[PATCH] Documentation: Fix config_acs= example

[Akihiko Odaki]

The documentation used to say:
> For example,
>   pci=config_acs=10x
> would configure all devices that support ACS to enable P2P Request
> Redirect, disable Translation Blocking, and leave Source Validation
> unchanged from whatever power-up or firmware set it to.

However, a flag specification always needs to be suffixed with "@" and a
PCI device string, which is missing in this example. It needs to be
suffixed with "@pci:0:0" to configure all devices that support ACS in
particular.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index ee2984e46c06..5611903c27a9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4604,7 +4604,7 @@
 				  '1' – force enabled
 				  'x' – unchanged
 				For example,
-				  pci=config_acs=10x
+				  pci=config_acs=10x@pci:0:0
 				would configure all devices that support
 				ACS to enable P2P Request Redirect, disable
 				Translation Blocking, and leave Source

---
base-commit: 46a0057a5853cbdb58211c19e89ba7777dc6fd50
change-id: 20240911-acs-3043a2737cc9

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH] Documentation: Fix config_acs= example

[Akihiko Odaki]

Hi,

It seems this patch has been forgotten for a while but it is still 
valid. Can anyone take a look at this?

On 2024/09/15 10:36, Akihiko Odaki wrote:
> The documentation used to say:
>> For example,
>>    pci=config_acs=10x
>> would configure all devices that support ACS to enable P2P Request
>> Redirect, disable Translation Blocking, and leave Source Validation
>> unchanged from whatever power-up or firmware set it to.
> 
> However, a flag specification always needs to be suffixed with "@" and a
> PCI device string, which is missing in this example. It needs to be
> suffixed with "@pci:0:0" to configure all devices that support ACS in
> particular.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   Documentation/admin-guide/kernel-parameters.txt | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index ee2984e46c06..5611903c27a9 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4604,7 +4604,7 @@
>   				  '1' – force enabled
>   				  'x' – unchanged
>   				For example,
> -				  pci=config_acs=10x
> +				  pci=config_acs=10x@pci:0:0
>   				would configure all devices that support
>   				ACS to enable P2P Request Redirect, disable
>   				Translation Blocking, and leave Source
> 
> ---
> base-commit: 46a0057a5853cbdb58211c19e89ba7777dc6fd50
> change-id: 20240911-acs-3043a2737cc9
> 
> Best regards,

Re: [PATCH] Documentation: Fix config_acs= example

[Bjorn Helgaas]

[+cc folks from related discussion at
https://lore.kernel.org/r/20241213202942.44585-1-tdave@nvidia.com]

On Sun, Sep 15, 2024 at 10:36:58AM +0900, Akihiko Odaki wrote:
> The documentation used to say:
> > For example,
> >   pci=config_acs=10x
> > would configure all devices that support ACS to enable P2P Request
> > Redirect, disable Translation Blocking, and leave Source Validation
> > unchanged from whatever power-up or firmware set it to.
> 
> However, a flag specification always needs to be suffixed with "@" and a
> PCI device string, which is missing in this example. It needs to be
> suffixed with "@pci:0:0" to configure all devices that support ACS in
> particular.

Thanks for the patch.  Krzysztof added a bit to the commit log at:

https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?id=57722057d762

but I think we should also include the flags template:

  config_acs =
                Format:
                <ACS flags>@<pci_dev>[; ...]

so the commit log includes a hint about what "flag specification"
means.

Also, I think the "pci_dev" documentation is poor and should be
improved in a second patch.  The only current mention I see is here:

        pci=option[,option...]  [PCI,EARLY] various PCI subsystem options.

                                Some options herein operate on a specific device
                                or a set of devices (<pci_dev>). These are
                                specified in one of the following formats:

                                [<domain>:]<bus>:<dev>.<func>[/<dev>.<func>]*
                                pci:<vendor>:<device>[:<subvendor>:<subdevice>]

                                Note: the first format specifies a PCI
                                bus/device/function address which may change
                                if new hardware is inserted, if motherboard
                                firmware changes, or due to changes caused
                                by other kernel parameters. If the
                                domain is left unspecified, it is
                                taken to be zero. Optionally, a path
                                to a device through multiple device/function
                                addresses can be specified after the base
                                address (this is more robust against
                                renumbering issues).  The second format
                                selects devices using IDs from the
                                configuration space which may match multiple
                                devices in the system.

where I guess "pci_dev" means the second format:

  pci:<vendor>:<device>[:<subvendor>:<subdevice>]

and apparently "pci:0:0" means all devices with vendor==0 and
device==0, and it's not completely obvious to me that this means "all
devices".

> ---
>  Documentation/admin-guide/kernel-parameters.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index ee2984e46c06..5611903c27a9 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4604,7 +4604,7 @@
>  				  '1' – force enabled
>  				  'x' – unchanged
>  				For example,
> -				  pci=config_acs=10x
> +				  pci=config_acs=10x@pci:0:0
>  				would configure all devices that support
>  				ACS to enable P2P Request Redirect, disable
>  				Translation Blocking, and leave Source
> 
> ---
> base-commit: 46a0057a5853cbdb58211c19e89ba7777dc6fd50
> change-id: 20240911-acs-3043a2737cc9
> 
> Best regards,
> -- 
> Akihiko Odaki <akihiko.odaki@daynix.com>
> 

Re: [PATCH] Documentation: Fix config_acs= example

[Krzysztof Wilczyński]

Hello,

[...]
> Thanks for the patch.  Krzysztof added a bit to the commit log at:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?id=57722057d762
> 
> but I think we should also include the flags template:
> 
>   config_acs =
>                 Format:
>                 <ACS flags>@<pci_dev>[; ...]
> 
> so the commit log includes a hint about what "flag specification"
> means.

I amended the commit log.  Should now include the above for reference.

	Krzysztof

Re: [PATCH] Documentation: Fix config_acs= example

[Krzysztof Wilczyński]

Hello,

> The documentation used to say:
> > For example,
> >   pci=config_acs=10x
> > would configure all devices that support ACS to enable P2P Request
> > Redirect, disable Translation Blocking, and leave Source Validation
> > unchanged from whatever power-up or firmware set it to.
> 
> However, a flag specification always needs to be suffixed with "@" and a
> PCI device string, which is missing in this example. It needs to be
> suffixed with "@pci:0:0" to configure all devices that support ACS in
> particular.

Applied to config-acs for v6.14, thank you!

	Krzysztof