diff mbox series

[net] forcedeth: Fix an error handling path in nv_probe()

Message ID	355e9a7d351b32ad897251b6f81b5886fcdc6766.1684571393.git.christophe.jaillet@wanadoo.fr
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Christophe JAILLET <christophe.jaillet@wanadoo.fr> To: Rain River <rain.1986.08.12@gmail.com>, Zhu Yanjun <zyjzyj2000@gmail.com>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Ayaz Abdulla <aabdulla@nvidia.com> Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, Christophe JAILLET <christophe.jaillet@wanadoo.fr>, netdev@vger.kernel.org Subject: [PATCH net] forcedeth: Fix an error handling path in nv_probe() Date: Sat, 20 May 2023 10:30:17 +0200 Message-Id: <355e9a7d351b32ad897251b6f81b5886fcdc6766.1684571393.git.christophe.jaillet@wanadoo.fr> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	[net] forcedeth: Fix an error handling path in nv_probe() \| [net] forcedeth: Fix an error handling path in nv_probe()

Commit Message

Christophe JAILLET May 20, 2023, 8:30 a.m. UTC

  If an error occures after calling nv_mgmt_acquire_sema(), it should be
undone with a corresponding nv_mgmt_release_sema() call.

Add it in the error handling path of the probe as already done in the
remove function.

Fixes: cac1c52c3621 ("forcedeth: mgmt unit interface")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
I don't think that the Fixes tag is really accurate, but
nv_mgmt_release_sema() was introduced here. And cac1c52c3621 is already old
so should be good enough.
---
 drivers/net/ethernet/nvidia/forcedeth.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Zhu Yanjun May 20, 2023, 10:51 a.m. UTC | #1

On Sat, May 20, 2023 at 4:30 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> If an error occures after calling nv_mgmt_acquire_sema(), it should be
> undone with a corresponding nv_mgmt_release_sema() call.
>
> Add it in the error handling path of the probe as already done in the
> remove function.
>
> Fixes: cac1c52c3621 ("forcedeth: mgmt unit interface")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks.
Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com>

Zhu Yanjun
> ---
> I don't think that the Fixes tag is really accurate, but
> nv_mgmt_release_sema() was introduced here. And cac1c52c3621 is already old
> so should be good enough.
> ---
>  drivers/net/ethernet/nvidia/forcedeth.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index 0605d1ee490d..7a549b834e97 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -6138,6 +6138,7 @@ static int nv_probe(struct pci_dev *pci_dev, const struct pci_device_id *id)
>         return 0;
>
>  out_error:
> +       nv_mgmt_release_sema(dev);
>         if (phystate_orig)
>                 writel(phystate|NVREG_ADAPTCTL_RUNNING, base + NvRegAdapterControl);
>  out_freering:
> --
> 2.34.1
>

Simon Horman May 22, 2023, 10:12 a.m. UTC | #2

On Sat, May 20, 2023 at 10:30:17AM +0200, Christophe JAILLET wrote:
> If an error occures after calling nv_mgmt_acquire_sema(), it should be
> undone with a corresponding nv_mgmt_release_sema() call.

nit: s/occures/occurs/

> 
> Add it in the error handling path of the probe as already done in the
> remove function.

I was going to ask what happens if nv_mgmt_acquire_sema() fails.
Then I realised that it always returns 0.

Perhaps it would be worth changing it's return type to void at some point.

> Fixes: cac1c52c3621 ("forcedeth: mgmt unit interface")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> I don't think that the Fixes tag is really accurate, but
> nv_mgmt_release_sema() was introduced here. And cac1c52c3621 is already old
> so should be good enough.
> ---
>  drivers/net/ethernet/nvidia/forcedeth.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index 0605d1ee490d..7a549b834e97 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -6138,6 +6138,7 @@ static int nv_probe(struct pci_dev *pci_dev, const struct pci_device_id *id)
>  	return 0;
>  
>  out_error:
> +	nv_mgmt_release_sema(dev);
>  	if (phystate_orig)
>  		writel(phystate|NVREG_ADAPTCTL_RUNNING, base + NvRegAdapterControl);
>  out_freering:
> -- 
> 2.34.1
> 
>

Dan Carpenter May 22, 2023, 10:35 a.m. UTC | #3

On Mon, May 22, 2023 at 12:12:48PM +0200, Simon Horman wrote:
> On Sat, May 20, 2023 at 10:30:17AM +0200, Christophe JAILLET wrote:
> > If an error occures after calling nv_mgmt_acquire_sema(), it should be
> > undone with a corresponding nv_mgmt_release_sema() call.
> 
> nit: s/occures/occurs/
> 
> > 
> > Add it in the error handling path of the probe as already done in the
> > remove function.
> 
> I was going to ask what happens if nv_mgmt_acquire_sema() fails.
> Then I realised that it always returns 0.
> 
> Perhaps it would be worth changing it's return type to void at some point.
>

What? No?  It returns true on success and false on failure.

drivers/net/ethernet/nvidia/forcedeth.c
  5377  static int nv_mgmt_acquire_sema(struct net_device *dev)
  5378  {
  5379          struct fe_priv *np = netdev_priv(dev);
  5380          u8 __iomem *base = get_hwbase(dev);
  5381          int i;
  5382          u32 tx_ctrl, mgmt_sema;
  5383  
  5384          for (i = 0; i < 10; i++) {
  5385                  mgmt_sema = readl(base + NvRegTransmitterControl) & NVREG_XMITCTL_MGMT_SEMA_MASK;
  5386                  if (mgmt_sema == NVREG_XMITCTL_MGMT_SEMA_FREE)
  5387                          break;
  5388                  msleep(500);
  5389          }
  5390  
  5391          if (mgmt_sema != NVREG_XMITCTL_MGMT_SEMA_FREE)
  5392                  return 0;
  5393  
  5394          for (i = 0; i < 2; i++) {
  5395                  tx_ctrl = readl(base + NvRegTransmitterControl);
  5396                  tx_ctrl |= NVREG_XMITCTL_HOST_SEMA_ACQ;
  5397                  writel(tx_ctrl, base + NvRegTransmitterControl);
  5398  
  5399                  /* verify that semaphore was acquired */
  5400                  tx_ctrl = readl(base + NvRegTransmitterControl);
  5401                  if (((tx_ctrl & NVREG_XMITCTL_HOST_SEMA_MASK) == NVREG_XMITCTL_HOST_SEMA_ACQ) &&
  5402                      ((tx_ctrl & NVREG_XMITCTL_MGMT_SEMA_MASK) == NVREG_XMITCTL_MGMT_SEMA_FREE)) {
  5403                          np->mgmt_sema = 1;
  5404                          return 1;
                                ^^^^^^^^^
Success path.

  5405                  } else
  5406                          udelay(50);
  5407          }
  5408  
  5409          return 0;
  5410  }

regards,
dan carpenter

Simon Horman May 22, 2023, 11:10 a.m. UTC | #4

On Mon, May 22, 2023 at 01:35:38PM +0300, Dan Carpenter wrote:
> On Mon, May 22, 2023 at 12:12:48PM +0200, Simon Horman wrote:
> > On Sat, May 20, 2023 at 10:30:17AM +0200, Christophe JAILLET wrote:
> > > If an error occures after calling nv_mgmt_acquire_sema(), it should be
> > > undone with a corresponding nv_mgmt_release_sema() call.
> > 
> > nit: s/occures/occurs/
> > 
> > > 
> > > Add it in the error handling path of the probe as already done in the
> > > remove function.
> > 
> > I was going to ask what happens if nv_mgmt_acquire_sema() fails.
> > Then I realised that it always returns 0.
> > 
> > Perhaps it would be worth changing it's return type to void at some point.
> >
> 
> What? No?  It returns true on success and false on failure.
> 
> drivers/net/ethernet/nvidia/forcedeth.c
>   5377  static int nv_mgmt_acquire_sema(struct net_device *dev)
>   5378  {
>   5379          struct fe_priv *np = netdev_priv(dev);
>   5380          u8 __iomem *base = get_hwbase(dev);
>   5381          int i;
>   5382          u32 tx_ctrl, mgmt_sema;
>   5383  
>   5384          for (i = 0; i < 10; i++) {
>   5385                  mgmt_sema = readl(base + NvRegTransmitterControl) & NVREG_XMITCTL_MGMT_SEMA_MASK;
>   5386                  if (mgmt_sema == NVREG_XMITCTL_MGMT_SEMA_FREE)
>   5387                          break;
>   5388                  msleep(500);
>   5389          }
>   5390  
>   5391          if (mgmt_sema != NVREG_XMITCTL_MGMT_SEMA_FREE)
>   5392                  return 0;
>   5393  
>   5394          for (i = 0; i < 2; i++) {
>   5395                  tx_ctrl = readl(base + NvRegTransmitterControl);
>   5396                  tx_ctrl |= NVREG_XMITCTL_HOST_SEMA_ACQ;
>   5397                  writel(tx_ctrl, base + NvRegTransmitterControl);
>   5398  
>   5399                  /* verify that semaphore was acquired */
>   5400                  tx_ctrl = readl(base + NvRegTransmitterControl);
>   5401                  if (((tx_ctrl & NVREG_XMITCTL_HOST_SEMA_MASK) == NVREG_XMITCTL_HOST_SEMA_ACQ) &&
>   5402                      ((tx_ctrl & NVREG_XMITCTL_MGMT_SEMA_MASK) == NVREG_XMITCTL_MGMT_SEMA_FREE)) {
>   5403                          np->mgmt_sema = 1;
>   5404                          return 1;
>                                 ^^^^^^^^^
> Success path.
> 
>   5405                  } else
>   5406                          udelay(50);
>   5407          }
>   5408  
>   5409          return 0;
>   5410  }

Thanks Dan,

my eyes deceived me.

In that case, my question is: what if nv_mgmt_acquire_sema() fails?
But I think the answer is that nv_mgmt_release_sema() will do
nothing because mgmt_sema is not set.

So I think we are good.

Christophe JAILLET May 22, 2023, 5:13 p.m. UTC | #5

Le 22/05/2023 à 13:10, Simon Horman a écrit :
> On Mon, May 22, 2023 at 01:35:38PM +0300, Dan Carpenter wrote:
>> On Mon, May 22, 2023 at 12:12:48PM +0200, Simon Horman wrote:
>>> On Sat, May 20, 2023 at 10:30:17AM +0200, Christophe JAILLET wrote:
>>>> If an error occures after calling nv_mgmt_acquire_sema(), it should be
>>>> undone with a corresponding nv_mgmt_release_sema() call.
>>>
>>> nit: s/occures/occurs/
>>>
>>>>
>>>> Add it in the error handling path of the probe as already done in the
>>>> remove function.
>>>
>>> I was going to ask what happens if nv_mgmt_acquire_sema() fails.
>>> Then I realised that it always returns 0.
>>>
>>> Perhaps it would be worth changing it's return type to void at some point.
>>>
>>
>> What? No?  It returns true on success and false on failure.
>>
>> drivers/net/ethernet/nvidia/forcedeth.c
>>    5377  static int nv_mgmt_acquire_sema(struct net_device *dev)
>>    5378  {
>>    5379          struct fe_priv *np = netdev_priv(dev);
>>    5380          u8 __iomem *base = get_hwbase(dev);
>>    5381          int i;
>>    5382          u32 tx_ctrl, mgmt_sema;
>>    5383
>>    5384          for (i = 0; i < 10; i++) {
>>    5385                  mgmt_sema = readl(base + NvRegTransmitterControl) & NVREG_XMITCTL_MGMT_SEMA_MASK;
>>    5386                  if (mgmt_sema == NVREG_XMITCTL_MGMT_SEMA_FREE)
>>    5387                          break;
>>    5388                  msleep(500);
>>    5389          }
>>    5390
>>    5391          if (mgmt_sema != NVREG_XMITCTL_MGMT_SEMA_FREE)
>>    5392                  return 0;
>>    5393
>>    5394          for (i = 0; i < 2; i++) {
>>    5395                  tx_ctrl = readl(base + NvRegTransmitterControl);
>>    5396                  tx_ctrl |= NVREG_XMITCTL_HOST_SEMA_ACQ;
>>    5397                  writel(tx_ctrl, base + NvRegTransmitterControl);
>>    5398
>>    5399                  /* verify that semaphore was acquired */
>>    5400                  tx_ctrl = readl(base + NvRegTransmitterControl);
>>    5401                  if (((tx_ctrl & NVREG_XMITCTL_HOST_SEMA_MASK) == NVREG_XMITCTL_HOST_SEMA_ACQ) &&
>>    5402                      ((tx_ctrl & NVREG_XMITCTL_MGMT_SEMA_MASK) == NVREG_XMITCTL_MGMT_SEMA_FREE)) {
>>    5403                          np->mgmt_sema = 1;
>>    5404                          return 1;
>>                                  ^^^^^^^^^
>> Success path.
>>
>>    5405                  } else
>>    5406                          udelay(50);
>>    5407          }
>>    5408
>>    5409          return 0;
>>    5410  }
> 
> Thanks Dan,
> 
> my eyes deceived me.
> 
> In that case, my question is: what if nv_mgmt_acquire_sema() fails?
> But I think the answer is that nv_mgmt_release_sema() will do
> nothing because mgmt_sema is not set.

At least, it is my understanding.

Can you fix the typo s/occures/occurs/ when applying the patch, or do 
you really need a v2 only for that?

CJ.

> 
> So I think we are good.
> 
> 
>

patchwork-bot+netdevbpf@kernel.org May 23, 2023, 2:30 a.m. UTC | #6

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sat, 20 May 2023 10:30:17 +0200 you wrote:
> If an error occures after calling nv_mgmt_acquire_sema(), it should be
> undone with a corresponding nv_mgmt_release_sema() call.
> 
> Add it in the error handling path of the probe as already done in the
> remove function.
> 
> Fixes: cac1c52c3621 ("forcedeth: mgmt unit interface")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 
> [...]

Here is the summary with links:
  - [net] forcedeth: Fix an error handling path in nv_probe()
    https://git.kernel.org/netdev/net/c/5b17a4971d3b

You are awesome, thank you!

Simon Horman May 23, 2023, 8:39 a.m. UTC | #7

On Mon, May 22, 2023 at 07:13:43PM +0200, Christophe JAILLET wrote:
> Le 22/05/2023 à 13:10, Simon Horman a écrit :
> > On Mon, May 22, 2023 at 01:35:38PM +0300, Dan Carpenter wrote:
> > > On Mon, May 22, 2023 at 12:12:48PM +0200, Simon Horman wrote:
> > > > On Sat, May 20, 2023 at 10:30:17AM +0200, Christophe JAILLET wrote:
> > > > > If an error occures after calling nv_mgmt_acquire_sema(), it should be
> > > > > undone with a corresponding nv_mgmt_release_sema() call.
> > > > 
> > > > nit: s/occures/occurs/
> > > > 
> > > > > 
> > > > > Add it in the error handling path of the probe as already done in the
> > > > > remove function.
> > > > 
> > > > I was going to ask what happens if nv_mgmt_acquire_sema() fails.
> > > > Then I realised that it always returns 0.
> > > > 
> > > > Perhaps it would be worth changing it's return type to void at some point.
> > > > 
> > > 
> > > What? No?  It returns true on success and false on failure.
> > > 
> > > drivers/net/ethernet/nvidia/forcedeth.c
> > >    5377  static int nv_mgmt_acquire_sema(struct net_device *dev)
> > >    5378  {
> > >    5379          struct fe_priv *np = netdev_priv(dev);
> > >    5380          u8 __iomem *base = get_hwbase(dev);
> > >    5381          int i;
> > >    5382          u32 tx_ctrl, mgmt_sema;
> > >    5383
> > >    5384          for (i = 0; i < 10; i++) {
> > >    5385                  mgmt_sema = readl(base + NvRegTransmitterControl) & NVREG_XMITCTL_MGMT_SEMA_MASK;
> > >    5386                  if (mgmt_sema == NVREG_XMITCTL_MGMT_SEMA_FREE)
> > >    5387                          break;
> > >    5388                  msleep(500);
> > >    5389          }
> > >    5390
> > >    5391          if (mgmt_sema != NVREG_XMITCTL_MGMT_SEMA_FREE)
> > >    5392                  return 0;
> > >    5393
> > >    5394          for (i = 0; i < 2; i++) {
> > >    5395                  tx_ctrl = readl(base + NvRegTransmitterControl);
> > >    5396                  tx_ctrl |= NVREG_XMITCTL_HOST_SEMA_ACQ;
> > >    5397                  writel(tx_ctrl, base + NvRegTransmitterControl);
> > >    5398
> > >    5399                  /* verify that semaphore was acquired */
> > >    5400                  tx_ctrl = readl(base + NvRegTransmitterControl);
> > >    5401                  if (((tx_ctrl & NVREG_XMITCTL_HOST_SEMA_MASK) == NVREG_XMITCTL_HOST_SEMA_ACQ) &&
> > >    5402                      ((tx_ctrl & NVREG_XMITCTL_MGMT_SEMA_MASK) == NVREG_XMITCTL_MGMT_SEMA_FREE)) {
> > >    5403                          np->mgmt_sema = 1;
> > >    5404                          return 1;
> > >                                  ^^^^^^^^^
> > > Success path.
> > > 
> > >    5405                  } else
> > >    5406                          udelay(50);
> > >    5407          }
> > >    5408
> > >    5409          return 0;
> > >    5410  }
> > 
> > Thanks Dan,
> > 
> > my eyes deceived me.
> > 
> > In that case, my question is: what if nv_mgmt_acquire_sema() fails?
> > But I think the answer is that nv_mgmt_release_sema() will do
> > nothing because mgmt_sema is not set.
> 
> At least, it is my understanding.
> 
> Can you fix the typo s/occures/occurs/ when applying the patch, or do you
> really need a v2 only for that?

It's beyond my powers to fix things in that way.
But I'd say there is no need to respin just for a spelling error.

FWIIW,

Reviewed-by: Simon Horman <simon.horman@corigine.com>

diff mbox series

Patch

diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 0605d1ee490d..7a549b834e97 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -6138,6 +6138,7 @@  static int nv_probe(struct pci_dev *pci_dev, const struct pci_device_id *id)
 	return 0;
 
 out_error:
+	nv_mgmt_release_sema(dev);
 	if (phystate_orig)
 		writel(phystate|NVREG_ADAPTCTL_RUNNING, base + NvRegAdapterControl);
 out_freering: