* [PATCH net] be2net: call napi_disable() for all event queues
@ 2013-11-26 16:54 Ivan Vecera
2013-11-26 21:33 ` Ajit Khaparde
2013-11-27 5:28 ` Sathya Perla
0 siblings, 2 replies; 4+ messages in thread
From: Ivan Vecera @ 2013-11-26 16:54 UTC (permalink / raw)
To: netdev; +Cc: Sathya Perla, Subbu Seetharaman, Ajit Khaparde
The recent be2net commit 6384a4d (adds a support for busy polling)
introduces a regression that results in kernel crash. It incorrectly
modified be_close() so napi_disable() is called only for the first queue.
This breaks a correct pairing of napi_enable/_disable for the rest
of event queues and causes a crash in subsequent be_open() call.
Cc: Sathya Perla <sathya.perla@emulex.com>
Cc: Subbu Seetharaman <subbu.seetharaman@emulex.com>
Cc: Ajit Khaparde <ajit.khaparde@emulex.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
drivers/net/ethernet/emulex/benet/be_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 78a0e85..0aec4d2 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -2663,8 +2663,8 @@ static int be_close(struct net_device *netdev)
napi_disable(&eqo->napi);
be_disable_busy_poll(eqo);
}
- adapter->flags &= ~BE_FLAGS_NAPI_ENABLED;
}
+ adapter->flags &= ~BE_FLAGS_NAPI_ENABLED;
be_async_mcc_disable(adapter);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH net] be2net: call napi_disable() for all event queues
2013-11-26 16:54 [PATCH net] be2net: call napi_disable() for all event queues Ivan Vecera
@ 2013-11-26 21:33 ` Ajit Khaparde
2013-11-27 5:28 ` Sathya Perla
1 sibling, 0 replies; 4+ messages in thread
From: Ajit Khaparde @ 2013-11-26 21:33 UTC (permalink / raw)
To: Ivan Vecera, netdev@vger.kernel.org; +Cc: Sathya Perla, Subramanian Seetharaman
> -----Original Message-----
> From: Ivan Vecera [mailto:ivecera@redhat.com]
> Sent: Tuesday, November 26, 2013 10:55 AM
> To: netdev@vger.kernel.org
> Cc: Sathya Perla; Subramanian Seetharaman; Ajit Khaparde
> Subject: [PATCH net] be2net: call napi_disable() for all event queues
>
> The recent be2net commit 6384a4d (adds a support for busy polling)
> introduces a regression that results in kernel crash. It incorrectly
> modified be_close() so napi_disable() is called only for the first queue.
> This breaks a correct pairing of napi_enable/_disable for the rest
> of event queues and causes a crash in subsequent be_open() call.
>
> Cc: Sathya Perla <sathya.perla@emulex.com>
> Cc: Subbu Seetharaman <subbu.seetharaman@emulex.com>
> Cc: Ajit Khaparde <ajit.khaparde@emulex.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
Acked-by: Ajit Khaparde <ajit.khaparde@emulex.com>
> ---
> drivers/net/ethernet/emulex/benet/be_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c
> b/drivers/net/ethernet/emulex/benet/be_main.c
> index 78a0e85..0aec4d2 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -2663,8 +2663,8 @@ static int be_close(struct net_device *netdev)
> napi_disable(&eqo->napi);
> be_disable_busy_poll(eqo);
> }
> - adapter->flags &= ~BE_FLAGS_NAPI_ENABLED;
> }
> + adapter->flags &= ~BE_FLAGS_NAPI_ENABLED;
>
> be_async_mcc_disable(adapter);
>
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH net] be2net: call napi_disable() for all event queues
2013-11-26 16:54 [PATCH net] be2net: call napi_disable() for all event queues Ivan Vecera
2013-11-26 21:33 ` Ajit Khaparde
@ 2013-11-27 5:28 ` Sathya Perla
2013-11-27 8:00 ` Ivan Vecera
1 sibling, 1 reply; 4+ messages in thread
From: Sathya Perla @ 2013-11-27 5:28 UTC (permalink / raw)
To: Ivan Vecera, netdev@vger.kernel.org
Cc: Subramanian Seetharaman, Ajit Khaparde
> -----Original Message-----
> From: Ivan Vecera [mailto:ivecera@redhat.com]
>
> The recent be2net commit 6384a4d (adds a support for busy polling)
> introduces a regression that results in kernel crash. It incorrectly
> modified be_close() so napi_disable() is called only for the first queue.
> This breaks a correct pairing of napi_enable/_disable for the rest
> of event queues and causes a crash in subsequent be_open() call.
>
> Cc: Sathya Perla <sathya.perla@emulex.com>
> Cc: Subbu Seetharaman <subbu.seetharaman@emulex.com>
> Cc: Ajit Khaparde <ajit.khaparde@emulex.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> drivers/net/ethernet/emulex/benet/be_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c
> b/drivers/net/ethernet/emulex/benet/be_main.c
> index 78a0e85..0aec4d2 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -2663,8 +2663,8 @@ static int be_close(struct net_device *netdev)
> napi_disable(&eqo->napi);
> be_disable_busy_poll(eqo);
> }
> - adapter->flags &= ~BE_FLAGS_NAPI_ENABLED;
> }
> + adapter->flags &= ~BE_FLAGS_NAPI_ENABLED;
>
Ivan, Thanks for spotting this.
Your fix is correct but it'll be nice if the for-loop for napi_disable
is not even entered when FLAGS_NAPI_ENABLED is not set, as in:
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 603b662..40d2adb 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -2658,8 +2658,8 @@ static int be_close(struct net_device *netdev)
be_roce_dev_close(adapter);
- for_all_evt_queues(adapter, eqo, i) {
- if (adapter->flags & BE_FLAGS_NAPI_ENABLED) {
+ if (adapter->flags & BE_FLAGS_NAPI_ENABLED) {
+ for_all_evt_queues(adapter, eqo, i) {
napi_disable(&eqo->napi);
be_disable_busy_poll(eqo);
}
Could you also add the following line to your patch as it fixes a
regression caused by an older be2net commit:
Fixes: 6384a4d0dcf9 ("be2net: add support for ndo_busy_poll")
-Sathya
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] be2net: call napi_disable() for all event queues
2013-11-27 5:28 ` Sathya Perla
@ 2013-11-27 8:00 ` Ivan Vecera
0 siblings, 0 replies; 4+ messages in thread
From: Ivan Vecera @ 2013-11-27 8:00 UTC (permalink / raw)
To: Sathya Perla
Cc: netdev@vger.kernel.org, Subramanian Seetharaman, Ajit Khaparde
On 11/27/2013 06:28 AM, Sathya Perla wrote:
>> -----Original Message-----
>> From: Ivan Vecera [mailto:ivecera@redhat.com]
>>
>> The recent be2net commit 6384a4d (adds a support for busy polling)
>> introduces a regression that results in kernel crash. It incorrectly
>> modified be_close() so napi_disable() is called only for the first queue.
>> This breaks a correct pairing of napi_enable/_disable for the rest
>> of event queues and causes a crash in subsequent be_open() call.
>>
>> Cc: Sathya Perla <sathya.perla@emulex.com>
>> Cc: Subbu Seetharaman <subbu.seetharaman@emulex.com>
>> Cc: Ajit Khaparde <ajit.khaparde@emulex.com>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>> drivers/net/ethernet/emulex/benet/be_main.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c
>> b/drivers/net/ethernet/emulex/benet/be_main.c
>> index 78a0e85..0aec4d2 100644
>> --- a/drivers/net/ethernet/emulex/benet/be_main.c
>> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
>> @@ -2663,8 +2663,8 @@ static int be_close(struct net_device *netdev)
>> napi_disable(&eqo->napi);
>> be_disable_busy_poll(eqo);
>> }
>> - adapter->flags &= ~BE_FLAGS_NAPI_ENABLED;
>> }
>> + adapter->flags &= ~BE_FLAGS_NAPI_ENABLED;
>>
>
> Ivan, Thanks for spotting this.
> Your fix is correct but it'll be nice if the for-loop for napi_disable
> is not even entered when FLAGS_NAPI_ENABLED is not set, as in:
>
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
> index 603b662..40d2adb 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -2658,8 +2658,8 @@ static int be_close(struct net_device *netdev)
>
> be_roce_dev_close(adapter);
>
> - for_all_evt_queues(adapter, eqo, i) {
> - if (adapter->flags & BE_FLAGS_NAPI_ENABLED) {
> + if (adapter->flags & BE_FLAGS_NAPI_ENABLED) {
> + for_all_evt_queues(adapter, eqo, i) {
> napi_disable(&eqo->napi);
> be_disable_busy_poll(eqo);
> }
>
> Could you also add the following line to your patch as it fixes a
> regression caused by an older be2net commit:
> Fixes: 6384a4d0dcf9 ("be2net: add support for ndo_busy_poll")
>
Sure, I'm sending v2.
Ivan
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-11-27 8:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-26 16:54 [PATCH net] be2net: call napi_disable() for all event queues Ivan Vecera
2013-11-26 21:33 ` Ajit Khaparde
2013-11-27 5:28 ` Sathya Perla
2013-11-27 8:00 ` Ivan Vecera
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).