[PATCH] powerpc/eeh:Fix docstrings in eeh

[PATCH] powerpc/eeh:Fix docstrings in eeh

[Kai Song]

We fix the following warnings when building kernel with W=1:
arch/powerpc/kernel/eeh.c:598: warning: Function parameter or member 'function' not described in 'eeh_pci_enable'
arch/powerpc/kernel/eeh.c:774: warning: Function parameter or member 'edev' not described in 'eeh_set_dev_freset'
arch/powerpc/kernel/eeh.c:774: warning: expecting prototype for eeh_set_pe_freset(). Prototype was for eeh_set_dev_freset() instead
arch/powerpc/kernel/eeh.c:814: warning: Function parameter or member 'include_passed' not described in 'eeh_pe_reset_full'
arch/powerpc/kernel/eeh.c:944: warning: Function parameter or member 'ops' not described in 'eeh_init'
arch/powerpc/kernel/eeh.c:1451: warning: Function parameter or member 'include_passed' not described in 'eeh_pe_reset'
arch/powerpc/kernel/eeh.c:1526: warning: Function parameter or member 'func' not described in 'eeh_pe_inject_err'
arch/powerpc/kernel/eeh.c:1526: warning: Excess function parameter 'function' described in 'eeh_pe_inject_err'

Signed-off-by: Kai Song <songkai01@inspur.com>
---
 arch/powerpc/kernel/eeh.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index e9b597ed423c..57a6868a41ab 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -589,6 +589,7 @@ EXPORT_SYMBOL(eeh_check_failure);
 /**
  * eeh_pci_enable - Enable MMIO or DMA transfers for this slot
  * @pe: EEH PE
+ * @function : EEH function
  *
  * This routine should be called to reenable frozen MMIO or DMA
  * so that it would work correctly again. It's useful while doing
@@ -761,8 +762,8 @@ int pcibios_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state stat
 }
 
 /**
- * eeh_set_pe_freset - Check the required reset for the indicated device
- * @data: EEH device
+ * eeh_set_dev_freset - Check the required reset for the indicated device
+ * @edev: EEH device
  * @flag: return value
  *
  * Each device might have its preferred reset type: fundamental or
@@ -801,6 +802,7 @@ static void eeh_pe_refreeze_passed(struct eeh_pe *root)
 /**
  * eeh_pe_reset_full - Complete a full reset process on the indicated PE
  * @pe: EEH PE
+ * @include_passed: include passed-through devices?
  *
  * This function executes a full reset procedure on a PE, including setting
  * the appropriate flags, performing a fundamental or hot reset, and then
@@ -937,6 +939,7 @@ static struct notifier_block eeh_device_nb = {
 
 /**
  * eeh_init - System wide EEH initialization
+ * @ops: struct to trace EEH operation callback functions
  *
  * It's the platform's job to call this from an arch_initcall().
  */
@@ -1442,6 +1445,7 @@ static int eeh_pe_reenable_devices(struct eeh_pe *pe, bool include_passed)
  * eeh_pe_reset - Issue PE reset according to specified type
  * @pe: EEH PE
  * @option: reset type
+ * @include_passed: include passed-through devices?
  *
  * The routine is called to reset the specified PE with the
  * indicated type, either fundamental reset or hot reset.
@@ -1513,12 +1517,12 @@ EXPORT_SYMBOL_GPL(eeh_pe_configure);
  * eeh_pe_inject_err - Injecting the specified PCI error to the indicated PE
  * @pe: the indicated PE
  * @type: error type
- * @function: error function
+ * @func: error function
  * @addr: address
  * @mask: address mask
  *
  * The routine is called to inject the specified PCI error, which
- * is determined by @type and @function, to the indicated PE for
+ * is determined by @type and @func, to the indicated PE for
  * testing purpose.
  */
 int eeh_pe_inject_err(struct eeh_pe *pe, int type, int func,
-- 
2.27.0

Re: [PATCH] powerpc/eeh:Fix docstrings in eeh

[Daniel Axtens]

Hi Kai,

Thank you for your patch! I have 3 very minor tweaks and I am otherwise
very happy with it.

Firstly, in your commit name, there should be a space between
"powerpc/eeh:" and "Fix docstrings". You might also want to say "in
eeh.c" rather than "in eeh" because there is eeh code in a number of
other files too.

> We fix the following warnings when building kernel with W=1:
> arch/powerpc/kernel/eeh.c:598: warning: Function parameter or member 'function' not described in 'eeh_pci_enable'
> arch/powerpc/kernel/eeh.c:774: warning: Function parameter or member 'edev' not described in 'eeh_set_dev_freset'
> arch/powerpc/kernel/eeh.c:774: warning: expecting prototype for eeh_set_pe_freset(). Prototype was for eeh_set_dev_freset() instead
> arch/powerpc/kernel/eeh.c:814: warning: Function parameter or member 'include_passed' not described in 'eeh_pe_reset_full'
> arch/powerpc/kernel/eeh.c:944: warning: Function parameter or member 'ops' not described in 'eeh_init'
> arch/powerpc/kernel/eeh.c:1451: warning: Function parameter or member 'include_passed' not described in 'eeh_pe_reset'
> arch/powerpc/kernel/eeh.c:1526: warning: Function parameter or member 'func' not described in 'eeh_pe_inject_err'
> arch/powerpc/kernel/eeh.c:1526: warning: Excess function parameter 'function' described in 'eeh_pe_inject_err'
>
> Signed-off-by: Kai Song <songkai01@inspur.com>
> ---
>  arch/powerpc/kernel/eeh.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index e9b597ed423c..57a6868a41ab 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -589,6 +589,7 @@ EXPORT_SYMBOL(eeh_check_failure);
>  /**
>   * eeh_pci_enable - Enable MMIO or DMA transfers for this slot
>   * @pe: EEH PE
> + * @function : EEH function
I don't think there should be a space between '@function' and ':'.

I know the parameter is called 'function' and I think "EEH function" was
a good guess for the docstring. However, if we look at the way
'function' is used, it is compared with EEH_OPT_xxx constants, and then
it's passed to eeh_ops->set_option(), eeh_pci_enable() is also called in
eeh_pe_set_option() with a parameter called 'option'. So I think maybe
'function' should be described as the "EEH option"?

This is still very unsatisfactory but it's not the fault of your patch -
the EEH codebase is very messy and it's worth fixing the W=1 warnings
even if we don't fully clean up the EEH codebase.

> - * eeh_set_pe_freset - Check the required reset for the indicated device
> - * @data: EEH device
> + * eeh_set_dev_freset - Check the required reset for the indicated device
> + * @edev: EEH device
>   * @flag: return value
>   *

This is good.

>  /**
>   * eeh_pe_reset_full - Complete a full reset process on the indicated PE
>   * @pe: EEH PE
> + * @include_passed: include passed-through devices?
>   *
>   * This function executes a full reset procedure on a PE, including setting
>   * the appropriate flags, performing a fundamental or hot reset, and then
> @@ -937,6 +939,7 @@ static struct notifier_block eeh_device_nb = {

This is OK.

 
>  /**
>   * eeh_init - System wide EEH initialization
> + * @ops: struct to trace EEH operation callback functions

I think "@ops: platform-specific functions for EEH operations" is
probably clearer?

>   *
>   * It's the platform's job to call this from an arch_initcall().
>   */
> @@ -1442,6 +1445,7 @@ static int eeh_pe_reenable_devices(struct eeh_pe *pe, bool include_passed)
>   * eeh_pe_reset - Issue PE reset according to specified type
>   * @pe: EEH PE
>   * @option: reset type
> + * @include_passed: include passed-through devices?
>   *

This is OK.

>   * The routine is called to reset the specified PE with the
>   * indicated type, either fundamental reset or hot reset.
> @@ -1513,12 +1517,12 @@ EXPORT_SYMBOL_GPL(eeh_pe_configure);
>   * eeh_pe_inject_err - Injecting the specified PCI error to the indicated PE
>   * @pe: the indicated PE
>   * @type: error type
> - * @function: error function
> + * @func: error function

This is good.

>   * @addr: address
>   * @mask: address mask
>   *
>   * The routine is called to inject the specified PCI error, which
> - * is determined by @type and @function, to the indicated PE for
> + * is determined by @type and @func, to the indicated PE for

This is good.

When you resend, you can include:
 Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel