Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Sun, 10 Dec 2023 11:37:32 +0000
From:   Simon Horman <horms@kernel.org>
To:     Shinas Rasheed <srasheed@marvell.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        hgani@marvell.com, vimleshk@marvell.com, egallen@redhat.com,
        mschmidt@redhat.com, pabeni@redhat.com, kuba@kernel.org,
        davem@davemloft.net, wizhao@redhat.com, kheib@redhat.com,
        konguyen@redhat.com, Veerasenareddy Burru <vburru@marvell.com>,
        Sathesh Edara <sedara@marvell.com>,
        Eric Dumazet <edumazet@google.com>
Subject: Re: [PATCH net-next v2 1/4] octeon_ep: add PF-VF mailbox
 communication
Message-ID: <20231210113732.GF5817@kernel.org>
References: <20231209081450.2613561-1-srasheed@marvell.com>
 <20231209081450.2613561-2-srasheed@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231209081450.2613561-2-srasheed@marvell.com>
Precedence: bulk

On Sat, Dec 09, 2023 at 12:14:47AM -0800, Shinas Rasheed wrote:
> Implement mailbox communication between PF and VFs.
> PF-VF mailbox is used for all control commands from VF to PF and
> asynchronous notification messages from PF to VF.
> 
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>

...

> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c

...

> @@ -1315,6 +1335,7 @@ static void octep_device_cleanup(struct octep_device *oct)
>  		oct->mbox[i] = NULL;
>  	}
>  
> +	octep_delete_pfvf_mbox(oct);
>  	octep_ctrl_net_uninit(oct);
>  	cancel_delayed_work_sync(&oct->hb_task);
>  
> @@ -1411,6 +1432,13 @@ static int octep_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto err_octep_config;
>  	}
>  
> +	err = octep_setup_pfvf_mbox(octep_dev);
> +	if (err) {
> +		dev_err(&pdev->dev, " pfvf mailbox setup failed\n");
> +		octep_ctrl_net_uninit(octep_dev);
> +		return err;

Hi Shinas,

This seems inconsistent with other error handling in this function, I
suspect it is leaking resources. And even if it does not, it's likely to
lead to such problems as this function is updated in future.  Please
consider consistently handling error unwinding using goto labels in this
function.

I think that means either making sure that octep_delete_pfvf_mbox() can
handle the case whereby octep_setup_pfvf_mbox() fails. Or including) the
steps taken by octep_device_cleanup() in the unwind ladder provided by goto
labels (which could be a separate patch, and I suspect to be the best
approach). But I could well be wrong.

> +	}
> +
>  	octep_ctrl_net_get_info(octep_dev, OCTEP_CTRL_NET_INVALID_VFID,
>  				&octep_dev->conf->fw_info);
>  	dev_info(&octep_dev->pdev->dev, "Heartbeat interval %u msecs Heartbeat miss count %u\n",

...