RE: [PATCH] mlxbf_gige: Add missing check for platform_get_irq

[David Thompson]

> -----Original Message-----
> From: Jiasheng Jiang <jiasheng@xxxxxxxxxxx>
> Sent: Thursday, June 1, 2023 2:19 AM
> To: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; David Thompson <davthompson@xxxxxxxxxx>; Asmaa
> Mnebhi <asmaa@xxxxxxxxxx>; mkl@xxxxxxxxxxxxxx; Liming Sun
> <limings@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jiasheng Jiang
> <jiasheng@xxxxxxxxxxx>
> Subject: [PATCH] mlxbf_gige: Add missing check for platform_get_irq
> 
> Add the check for the return value of the platform_get_irq and return error if it
> fails.
> 
> Fixes: f92e1869d74e ("Add Mellanox BlueField Gigabit Ethernet driver")
> Signed-off-by: Jiasheng Jiang <jiasheng@xxxxxxxxxxx>
> ---
>  drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> index 694de9513b9f..a38e1c68874f 100644
> --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> @@ -427,6 +427,10 @@ static int mlxbf_gige_probe(struct platform_device
> *pdev)
>  	priv->error_irq = platform_get_irq(pdev,
> MLXBF_GIGE_ERROR_INTR_IDX);
>  	priv->rx_irq = platform_get_irq(pdev,
> MLXBF_GIGE_RECEIVE_PKT_INTR_IDX);
>  	priv->llu_plu_irq = platform_get_irq(pdev,
> MLXBF_GIGE_LLU_PLU_INTR_IDX);
> +	if (priv->error_irq < 0 || priv->rx_irq < 0 || priv->llu_plu_irq < 0) {
> +		err = -ENODEV;
> +		goto out;
> +	}
> 

It is preferred to maintain the error code from "platform_get_irq" and return
that instead of -ENODEV.  Please review the API for "platform_get_irq" :

/**
 * platform_get_irq - get an IRQ for a device
 * @dev: platform device
 * @num: IRQ number index
 *
 * Gets an IRQ for a platform device and prints an error message if finding the
 * IRQ fails. Device drivers should check the return value for errors so as to
 * not pass a negative integer value to the request_irq() APIs.
 *
 * For example::
 *
 *              int irq = platform_get_irq(pdev, 0);
 *              if (irq < 0)
 *                      return irq;
 *
 * Return: non-zero IRQ number on success, negative error number on failure.
 */

So, the code could look something like this, for each of 3 IRQs:
	
	priv->error_irq = platform_get_irq(...)
	if (priv->error_irq < 0) {
		err = priv->error_irq;
		goto out;
	}

Just a thought.

Thanks,
- Dave

>  	phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev),
> "phy-gpios", 0);
>  	if (phy_irq < 0) {
> --
> 2.25.1