| Message ID | 20210713002525.203840-2-kw@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/2] PCI: Use pcie_reset_state_t type in function arguments | expand |
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);
On Thu, Mar 10, 2022 at 09:51:13AM +0100, Christophe Leroy wrote: > 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. I think this is true: in the patch below, we remove use of "enum pcie_reset_state", so the compiler no longer knows that "state" is an enum, and it cannot verify that "state" has a legal value in the switch statement. And at least with "gcc -Wall", it looks like it *does* complain in that case. Whether that value is worthwhile, I don't know. AFAICT this is the only place that uses "state", so there's not *much* value. If we did apply the patch below, I think we could probably make "enum pcie_reset_state" an anonymous enum instead, like the enum for pci_channel_state_t. But let's back up for a minute. This is only used in the pci_set_pcie_reset_state() path, and that's only used by three drivers: cxl, genwqe, and ipr, and obviously only on powerpc, since that's the only arch that implements pcibios_set_pcie_reset_state(). What's special about them? Why do they need this and no other drivers do? And why only on powerpc? I wonder if that powerpc functionality could be implemented in some way that's more integrated into the PCI core reset and error handling framework. > > --- > > 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);
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);
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> --- arch/powerpc/kernel/eeh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)