| Message ID | 355e9a7d351b32ad897251b6f81b5886fcdc6766.1684571393.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5b17a4971d3b2a073f4078dd65331efbe35baa2d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] forcedeth: Fix an error handling path in nv_probe() | expand |
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 >
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 > >
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 <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!
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 --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:
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(+)