Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 14 Apr 2020 15:18:32 -0500
From:   Corey Minyard <minyard@acm.org>
To:     Tang Bin <tangbin@cmss.chinamobile.com>
Cc:     arnd@arndb.de, gregkh@linuxfoundation.org,
        openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org,
        Shengju Zhang <zhangshengju@cmss.chinamobile.com>
Subject: Re: [PATCH 3/3] ipmi:bt-bmc: Fix error handling and status check
Message-ID: <20200414201832.GJ3587@minyard.net>
Reply-To: minyard@acm.org
References: <20200414141423.4968-1-tangbin@cmss.chinamobile.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200414141423.4968-1-tangbin@cmss.chinamobile.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 14, 2020 at 10:14:24PM +0800, Tang Bin wrote:
> If the function platform_get_irq() failed, the negative
> value returned will not be detected here. So fix error
> handling in bt_bmc_config_irq(). And if devm_request_irq()
> failed, 'bt_bmc->irq' is assigned to zero maybe redundant,
> it may be more suitable for using the correct negative values
> to make the status check in the function bt_bmc_remove().

Comments inline..

> 
> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
> Signed-off-by: Shengju Zhang <zhangshengju@cmss.chinamobile.com>
> ---
>  drivers/char/ipmi/bt-bmc.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
> index 1d4bf5c65..1740c6dc8 100644
> --- a/drivers/char/ipmi/bt-bmc.c
> +++ b/drivers/char/ipmi/bt-bmc.c
> @@ -399,16 +399,14 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
>  	struct device *dev = &pdev->dev;
>  	int rc;
>  
> -	bt_bmc->irq = platform_get_irq(pdev, 0);
> -	if (!bt_bmc->irq)
> -		return -ENODEV;
> +	bt_bmc->irq = platform_get_irq_optional(pdev, 0);
> +	if (bt_bmc->irq < 0)
> +		return bt_bmc->irq;
>  
>  	rc = devm_request_irq(dev, bt_bmc->irq, bt_bmc_irq, IRQF_SHARED,
>  			      DEVICE_NAME, bt_bmc);
> -	if (rc < 0) {
> -		bt_bmc->irq = 0;
> +	if (rc < 0)
>  		return rc;

I don't think this part is correct.  You will want to set bt_bmc->irq to
rc here to match what is done elsewhere so it's the error if negative.

Also, I believe this function should no longer return an error.  It
should just set the irq to the error if one happens.  The driver needs
to continue to operate even if it can't get its interrupt.

The rest of the changes are correct, I believe.

-corey

> -	}
>  
>  	/*
>  	 * Configure IRQs on the bmc clearing the H2B and HBUSY bits;
> @@ -499,7 +497,7 @@ static int bt_bmc_remove(struct platform_device *pdev)
>  	struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);
>  
>  	misc_deregister(&bt_bmc->miscdev);
> -	if (!bt_bmc->irq)
> +	if (bt_bmc->irq < 0)
>  		del_timer_sync(&bt_bmc->poll_timer);
>  	return 0;
>  }
> -- 
> 2.20.1.windows.1
> 
> 
>