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 <[email protected]>
---
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
On Sat, May 20, 2023 at 4:30 PM Christophe JAILLET
<[email protected]> 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 <[email protected]>
Thanks.
Acked-by: Zhu Yanjun <[email protected]>
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
>
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 <[email protected]>
> ---
> 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
>
>
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
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.
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.
>
>
>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
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 <[email protected]>
>
> [...]
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
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 <[email protected]>