2022-03-17 06:47:23

by tangmeng

Subject: [PATCH] net: 3com: 3c59x: Change the conditional processing for vortex_ioctl

In the vortex_ioctl, there are two places where there can be better
and easier to understand:
First, it should be better to reverse the check on 'VORTEX_PCI(vp)'
and returned early in order to be easier to understand.
Second, no need to make two 'if(state != 0)' judgments, we can
simplify the execution process.

Signed-off-by: Meng Tang <[email protected]>
---
drivers/net/ethernet/3com/3c59x.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
index ccf07667aa5e..c22de3c8cd12 100644
--- a/drivers/net/ethernet/3com/3c59x.c
+++ b/drivers/net/ethernet/3com/3c59x.c
@@ -3032,16 +3032,19 @@ static int vortex_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
struct vortex_private *vp = netdev_priv(dev);
pci_power_t state = 0;

- if(VORTEX_PCI(vp))
- state = VORTEX_PCI(vp)->current_state;
+ if (!VORTEX_PCI(vp))
+ return -EOPNOTSUPP;

- /* The kernel core really should have pci_get_power_state() */
+ state = VORTEX_PCI(vp)->current_state;

- if(state != 0)
+ /* The kernel core really should have pci_get_power_state() */
+ if (!state) {
+ err = generic_mii_ioctl(&vp->mii, if_mii(rq), cmd, NULL);
+ } else {
pci_set_power_state(VORTEX_PCI(vp), PCI_D0);
- err = generic_mii_ioctl(&vp->mii, if_mii(rq), cmd, NULL);
- if(state != 0)
+ err = generic_mii_ioctl(&vp->mii, if_mii(rq), cmd, NULL);
pci_set_power_state(VORTEX_PCI(vp), state);
+ }

return err;
}
--
2.20.1

2022-03-17 08:18:29

by Ondrej Zary

Subject: Re: [PATCH] net: 3com: 3c59x: Change the conditional processing for vortex_ioctl

On Thursday 17 March 2022, Meng Tang wrote:
> In the vortex_ioctl, there are two places where there can be better
> and easier to understand:
> First, it should be better to reverse the check on 'VORTEX_PCI(vp)'
> and returned early in order to be easier to understand.
> Second, no need to make two 'if(state != 0)' judgments, we can
> simplify the execution process.

Congratulations, you've just added 3 lines of code and broke a driver.
Your "simplified" version does not work with EISA devices.

Why? Please stop "improving" drivers if you can't test them.

> Signed-off-by: Meng Tang <[email protected]>
> ---
> drivers/net/ethernet/3com/3c59x.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
> index ccf07667aa5e..c22de3c8cd12 100644
> --- a/drivers/net/ethernet/3com/3c59x.c
> +++ b/drivers/net/ethernet/3com/3c59x.c
> @@ -3032,16 +3032,19 @@ static int vortex_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> struct vortex_private *vp = netdev_priv(dev);
> pci_power_t state = 0;
>
> - if(VORTEX_PCI(vp))
> - state = VORTEX_PCI(vp)->current_state;
> + if (!VORTEX_PCI(vp))
> + return -EOPNOTSUPP;
>
> - /* The kernel core really should have pci_get_power_state() */
> + state = VORTEX_PCI(vp)->current_state;
>
> - if(state != 0)
> + /* The kernel core really should have pci_get_power_state() */
> + if (!state) {
> + err = generic_mii_ioctl(&vp->mii, if_mii(rq), cmd, NULL);
> + } else {
> pci_set_power_state(VORTEX_PCI(vp), PCI_D0);
> - err = generic_mii_ioctl(&vp->mii, if_mii(rq), cmd, NULL);
> - if(state != 0)
> + err = generic_mii_ioctl(&vp->mii, if_mii(rq), cmd, NULL);
> pci_set_power_state(VORTEX_PCI(vp), state);
> + }
>
> return err;
> }



--
Ondrej Zary