From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
To: Simon Horman <simon.horman@corigine.com>,
Dan Carpenter <dan.carpenter@linaro.org>
Cc: 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>,
linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
netdev@vger.kernel.org
Subject: Re: [PATCH net] forcedeth: Fix an error handling path in nv_probe()
Date: Mon, 22 May 2023 19:13:43 +0200 [thread overview]
Message-ID: <8dbb4087-db01-fbbf-4e96-a5b0e170249a@wanadoo.fr> (raw)
In-Reply-To: <ZGtNwCc8ogSlwtYV@corigine.com>
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.
>
>
>
next prev parent reply other threads:[~2023-05-22 17:14 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-20 8:30 [PATCH net] forcedeth: Fix an error handling path in nv_probe() Christophe JAILLET
2023-05-20 10:51 ` Zhu Yanjun
2023-05-22 10:12 ` Simon Horman
2023-05-22 10:35 ` Dan Carpenter
2023-05-22 11:10 ` Simon Horman
2023-05-22 17:13 ` Christophe JAILLET [this message]
2023-05-23 8:39 ` Simon Horman
2023-05-23 2:30 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8dbb4087-db01-fbbf-4e96-a5b0e170249a@wanadoo.fr \
--to=christophe.jaillet@wanadoo.fr \
--cc=aabdulla@nvidia.com \
--cc=dan.carpenter@linaro.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=rain.1986.08.12@gmail.com \
--cc=simon.horman@corigine.com \
--cc=zyjzyj2000@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.