linux-kernel - Re: [RFC Patch net-next v3 1/3] net: dsa: microchip: use dev_ops->reset instead of exit in ksz_switch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yw4GQgIMSCmF3UMC@lunn.ch>
Date:   Tue, 30 Aug 2022 14:44:50 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Arun Ramadoss <arun.ramadoss@...rochip.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Woojung Huh <woojung.huh@...rochip.com>,
        UNGLinuxDriver@...rochip.com,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        Tristram Ha <Tristram.Ha@...rochip.com>
Subject: Re: [RFC Patch net-next v3 1/3] net: dsa: microchip: use
 dev_ops->reset instead of exit in ksz_switch_register

On Tue, Aug 30, 2022 at 04:23:01PM +0530, Arun Ramadoss wrote:
> ksz8_switch_exit, ksz9477_switch_exit and lan937x_switch_exit functions
> all call the reset function which is assigned to dev_ops->reset hooks.
> So instead of calling the dev_ops->exit in ksz_switch_register during
> the error condition of dsa_register_switch, dev_ops->reset is used now.
> The dev_ops->exit can be extended in lan937x for freeing up the irq
> during the ksz_spi_remove. If we add the irq remove in the exit function
> and it is called during the dsa_switch_register error condition, kernel
> panic happens since irq is setup only in setup operation. To avoid the
> kernel panic, dev_ops->reset is used instead of exit.
> 
> Signed-off-by: Arun Ramadoss <arun.ramadoss@...rochip.com>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 6bd69a7e6809..da9bdf753f7a 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1963,7 +1963,7 @@ int ksz_switch_register(struct ksz_device *dev)
>  
>  	ret = dsa_register_switch(dev->ds);
>  	if (ret) {
> -		dev->dev_ops->exit(dev);
> +		dev->dev_ops->reset(dev);
>  		return ret;

I don't like this change. You are supposed to be undoing whatever
ksz_switch_register() has done so far. It appears to of called
ops->init() so far, so calling ops->exit() should be the opposite.

If you are making exit() not the opposite of init() in later patches,
it suggests your code structure is wrong. If exit() is not already the
opposite of init() then fix that.

   Andrew