Re: [PATCH 2/2] powerpc/eeh: Use pcie_reset_state_t type in function arguments

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 13/07/2021 à 02:25, Krzysztof Wilczyński a écrit :
> The pcie_reset_state_t type has been introduced in the commit
> f7bdd12d234d ("pci: New PCI-E reset API") along with the enum
> pcie_reset_state, but it has never been used for anything else
> other than to define the members of the enumeration set in the
> enum pcie_reset_state.
> 
> Thus, replace the direct use of enum pcie_reset_state in function
> arguments and replace it with pcie_reset_state_t type so that the
> argument type matches the type used in enum pcie_reset_state.
> 
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>

I don't understand the purpose of this change. Does any tool like sparse 
of so reports an error here ?

My feeling is that by doing this you loose the added value of using an 
enumerate.

state is used in a switch/case, that's exactly what we expect from an enum.

By the way, I think you can't change the prototype of a weak function in 
a patch and not change it at the same time for the overloading function.

So should you still think this change is necessary, I think patch 1 and 
2 should be squashed together in one.

Christophe


> ---
>   arch/powerpc/kernel/eeh.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 3bbdcc86d01b..15485abb89ff 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -714,7 +714,7 @@ static void eeh_restore_dev_state(struct eeh_dev *edev, void *userdata)
>    * Return value:
>    * 	0 if success
>    */
> -int pcibios_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
> +int pcibios_set_pcie_reset_state(struct pci_dev *dev, pcie_reset_state_t state)
>   {
>   	struct eeh_dev *edev = pci_dev_to_eeh_dev(dev);
>   	struct eeh_pe *pe = eeh_dev_to_pe(edev);