diff mbox series

[net-next] ibmvnic: Don't handle RX interrupts when not up.

Message ID 151542776271.7885.5034544146387685418.stgit@ltcalpine2-lp14.aus.stglabs.ibm.com
State Superseded, archived
Delegated to: David Miller
Headers show
Series [net-next] ibmvnic: Don't handle RX interrupts when not up. | expand

Commit Message

Nathan Fontenot Jan. 8, 2018, 4:09 p.m. UTC 
Initiating a kdump via the command line can cause a pending interrupt
to be handled by the ibmvnic driver when initializing the sub-CRQ
irqs during driver initialization.

NIP [d000000000ca34f0] ibmvnic_interrupt_rx+0x40/0xd0 [ibmvnic]
LR [c000000008132ef0] __handle_irq_event_percpu+0xa0/0x2f0
Call Trace:
[c000000047fcfde0] [c000000008132ef0] __handle_irq_event_percpu+0xa0/0x2f0
[c000000047fcfea0] [c00000000813317c] handle_irq_event_percpu+0x3c/0x90
[c000000047fcfee0] [c00000000813323c] handle_irq_event+0x6c/0xd0
[c000000047fcff10] [c0000000081385e0] handle_fasteoi_irq+0xf0/0x250
[c000000047fcff40] [c0000000081320a0] generic_handle_irq+0x50/0x80
[c000000047fcff60] [c000000008014984] __do_irq+0x84/0x1d0
[c000000047fcff90] [c000000008027564] call_do_irq+0x14/0x24
[c00000003c92af00] [c000000008014b70] do_IRQ+0xa0/0x120
[c00000003c92af50] [c000000008002594] hardware_interrupt_common+0x114/0x180
--- interrupt: 501 at arch_local_irq_restore+0x74/0x90
    LR = arch_local_irq_restore+0x74/0x90
[c00000003c92b240] [c000000008138a20] irq_startup+0xa0/0xe0 (unreliable)
[c00000003c92b260] [c0000000087fc440] _raw_spin_unlock_irqrestore+0x40/0x70
[c00000003c92b280] [c0000000081361f4] __setup_irq+0x584/0x7a0
[c00000003c92b420] [c0000000081366a0] request_threaded_irq+0x140/0x290
[c00000003c92b480] [d000000000ca3754] init_sub_crq_irqs+0x1d4/0x370 [ibmvnic]
[c00000003c92b520] [d000000000ca6b50] ibmvnic_init+0x140/0x760 [ibmvnic]
[c00000003c92b5c0] [d000000000ca7308] ibmvnic_probe+0x198/0x330 [ibmvnic]
[c00000003c92b650] [c000000008037270] vio_bus_probe+0x1c0/0x480
[c00000003c92b6f0] [c00000000859a140] driver_probe_device+0x1f0/0x540
[c00000003c92b780] [c00000000859a59c] __driver_attach+0x10c/0x110
[c00000003c92b7c0] [c000000008596dac] bus_for_each_dev+0x8c/0xf0
[c00000003c92b810] [c000000008599798] driver_attach+0x38/0x50
[c00000003c92b830] [c000000008598f68] bus_add_driver+0x2b8/0x380
[c00000003c92b8c0] [c00000000859b660] driver_register+0xa0/0x180
[c00000003c92b930] [c000000008035c5c] __vio_register_driver+0x7c/0xc0
[c00000003c92b9b0] [d000000000caaf3c] ibmvnic_module_init+0x5c/0x70 [ibmvnic]
[c00000003c92ba10] [c00000000800b6cc] do_one_initcall+0x12c/0x280
[c00000003c92bae0] [c000000008805614] do_init_module+0x98/0x24c
[c00000003c92bb70] [c000000008181a80] load_module+0x1470/0x16c0
[c00000003c92bd40] [c000000008181fb0] SyS_finit_module+0xd0/0x120
[c00000003c92be30] [c000000008009204] system_call+0x38/0xb4

At this poibnt the driver is not prepared to handle traffic and
should not try to handle the interrupt. This patch adds a check to
ensure the driver is up when handling RX interrupots.

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c |    3 +++
 1 file changed, 3 insertions(+)

Comments

Thomas Falcon Jan. 9, 2018, 8:33 p.m. UTC | #1 
On 01/08/2018 10:09 AM, Nathan Fontenot wrote:
> Initiating a kdump via the command line can cause a pending interrupt
> to be handled by the ibmvnic driver when initializing the sub-CRQ
> irqs during driver initialization.
>
> NIP [d000000000ca34f0] ibmvnic_interrupt_rx+0x40/0xd0 [ibmvnic]
> LR [c000000008132ef0] __handle_irq_event_percpu+0xa0/0x2f0
> Call Trace:
> [c000000047fcfde0] [c000000008132ef0] __handle_irq_event_percpu+0xa0/0x2f0
> [c000000047fcfea0] [c00000000813317c] handle_irq_event_percpu+0x3c/0x90
> [c000000047fcfee0] [c00000000813323c] handle_irq_event+0x6c/0xd0
> [c000000047fcff10] [c0000000081385e0] handle_fasteoi_irq+0xf0/0x250
> [c000000047fcff40] [c0000000081320a0] generic_handle_irq+0x50/0x80
> [c000000047fcff60] [c000000008014984] __do_irq+0x84/0x1d0
> [c000000047fcff90] [c000000008027564] call_do_irq+0x14/0x24
> [c00000003c92af00] [c000000008014b70] do_IRQ+0xa0/0x120
> [c00000003c92af50] [c000000008002594] hardware_interrupt_common+0x114/0x180
> --- interrupt: 501 at arch_local_irq_restore+0x74/0x90
>     LR = arch_local_irq_restore+0x74/0x90
> [c00000003c92b240] [c000000008138a20] irq_startup+0xa0/0xe0 (unreliable)
> [c00000003c92b260] [c0000000087fc440] _raw_spin_unlock_irqrestore+0x40/0x70
> [c00000003c92b280] [c0000000081361f4] __setup_irq+0x584/0x7a0
> [c00000003c92b420] [c0000000081366a0] request_threaded_irq+0x140/0x290
> [c00000003c92b480] [d000000000ca3754] init_sub_crq_irqs+0x1d4/0x370 [ibmvnic]
> [c00000003c92b520] [d000000000ca6b50] ibmvnic_init+0x140/0x760 [ibmvnic]
> [c00000003c92b5c0] [d000000000ca7308] ibmvnic_probe+0x198/0x330 [ibmvnic]
> [c00000003c92b650] [c000000008037270] vio_bus_probe+0x1c0/0x480
> [c00000003c92b6f0] [c00000000859a140] driver_probe_device+0x1f0/0x540
> [c00000003c92b780] [c00000000859a59c] __driver_attach+0x10c/0x110
> [c00000003c92b7c0] [c000000008596dac] bus_for_each_dev+0x8c/0xf0
> [c00000003c92b810] [c000000008599798] driver_attach+0x38/0x50
> [c00000003c92b830] [c000000008598f68] bus_add_driver+0x2b8/0x380
> [c00000003c92b8c0] [c00000000859b660] driver_register+0xa0/0x180
> [c00000003c92b930] [c000000008035c5c] __vio_register_driver+0x7c/0xc0
> [c00000003c92b9b0] [d000000000caaf3c] ibmvnic_module_init+0x5c/0x70 [ibmvnic]
> [c00000003c92ba10] [c00000000800b6cc] do_one_initcall+0x12c/0x280
> [c00000003c92bae0] [c000000008805614] do_init_module+0x98/0x24c
> [c00000003c92bb70] [c000000008181a80] load_module+0x1470/0x16c0
> [c00000003c92bd40] [c000000008181fb0] SyS_finit_module+0xd0/0x120
> [c00000003c92be30] [c000000008009204] system_call+0x38/0xb4
>
> At this poibnt the driver is not prepared to handle traffic and
> should not try to handle the interrupt. This patch adds a check to
> ensure the driver is up when handling RX interrupots.
>
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> ---

Minor comments:

There is a typo in the commit message.

>  drivers/net/ethernet/ibm/ibmvnic.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 6911b7cc06c5..46990b84cb2d 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -2453,6 +2453,9 @@ static irqreturn_t ibmvnic_interrupt_rx(int irq, void *instance)
>  	struct ibmvnic_sub_crq_queue *scrq = instance;
>  	struct ibmvnic_adapter *adapter = scrq->adapter;
>
> +	if (adapter->state != VNIC_OPEN)
> +		return IRQ_NONE;
> +

Could you include a comment that this is a kdump specific check, since in any other case interrupts should be disabled if the device is closed?  Maybe you could use unlikely() here too.

Thanks,
Tom

>  	adapter->rx_stats_buffers[scrq->scrq_num].interrupts++;
>
>  	if (napi_schedule_prep(&adapter->napi[scrq->scrq_num])) {
>
Nathan Fontenot Jan. 10, 2018, 4:46 p.m. UTC | #2 
On 01/09/2018 02:33 PM, Thomas Falcon wrote:
> On 01/08/2018 10:09 AM, Nathan Fontenot wrote:
>> Initiating a kdump via the command line can cause a pending interrupt
>> to be handled by the ibmvnic driver when initializing the sub-CRQ
>> irqs during driver initialization.
>>
>> NIP [d000000000ca34f0] ibmvnic_interrupt_rx+0x40/0xd0 [ibmvnic]
>> LR [c000000008132ef0] __handle_irq_event_percpu+0xa0/0x2f0
>> Call Trace:
>> [c000000047fcfde0] [c000000008132ef0] __handle_irq_event_percpu+0xa0/0x2f0
>> [c000000047fcfea0] [c00000000813317c] handle_irq_event_percpu+0x3c/0x90
>> [c000000047fcfee0] [c00000000813323c] handle_irq_event+0x6c/0xd0
>> [c000000047fcff10] [c0000000081385e0] handle_fasteoi_irq+0xf0/0x250
>> [c000000047fcff40] [c0000000081320a0] generic_handle_irq+0x50/0x80
>> [c000000047fcff60] [c000000008014984] __do_irq+0x84/0x1d0
>> [c000000047fcff90] [c000000008027564] call_do_irq+0x14/0x24
>> [c00000003c92af00] [c000000008014b70] do_IRQ+0xa0/0x120
>> [c00000003c92af50] [c000000008002594] hardware_interrupt_common+0x114/0x180
>> --- interrupt: 501 at arch_local_irq_restore+0x74/0x90
>>     LR = arch_local_irq_restore+0x74/0x90
>> [c00000003c92b240] [c000000008138a20] irq_startup+0xa0/0xe0 (unreliable)
>> [c00000003c92b260] [c0000000087fc440] _raw_spin_unlock_irqrestore+0x40/0x70
>> [c00000003c92b280] [c0000000081361f4] __setup_irq+0x584/0x7a0
>> [c00000003c92b420] [c0000000081366a0] request_threaded_irq+0x140/0x290
>> [c00000003c92b480] [d000000000ca3754] init_sub_crq_irqs+0x1d4/0x370 [ibmvnic]
>> [c00000003c92b520] [d000000000ca6b50] ibmvnic_init+0x140/0x760 [ibmvnic]
>> [c00000003c92b5c0] [d000000000ca7308] ibmvnic_probe+0x198/0x330 [ibmvnic]
>> [c00000003c92b650] [c000000008037270] vio_bus_probe+0x1c0/0x480
>> [c00000003c92b6f0] [c00000000859a140] driver_probe_device+0x1f0/0x540
>> [c00000003c92b780] [c00000000859a59c] __driver_attach+0x10c/0x110
>> [c00000003c92b7c0] [c000000008596dac] bus_for_each_dev+0x8c/0xf0
>> [c00000003c92b810] [c000000008599798] driver_attach+0x38/0x50
>> [c00000003c92b830] [c000000008598f68] bus_add_driver+0x2b8/0x380
>> [c00000003c92b8c0] [c00000000859b660] driver_register+0xa0/0x180
>> [c00000003c92b930] [c000000008035c5c] __vio_register_driver+0x7c/0xc0
>> [c00000003c92b9b0] [d000000000caaf3c] ibmvnic_module_init+0x5c/0x70 [ibmvnic]
>> [c00000003c92ba10] [c00000000800b6cc] do_one_initcall+0x12c/0x280
>> [c00000003c92bae0] [c000000008805614] do_init_module+0x98/0x24c
>> [c00000003c92bb70] [c000000008181a80] load_module+0x1470/0x16c0
>> [c00000003c92bd40] [c000000008181fb0] SyS_finit_module+0xd0/0x120
>> [c00000003c92be30] [c000000008009204] system_call+0x38/0xb4
>>
>> At this poibnt the driver is not prepared to handle traffic and
>> should not try to handle the interrupt. This patch adds a check to
>> ensure the driver is up when handling RX interrupots.
>>
>> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
>> ---
> 
> Minor comments:
> 
> There is a typo in the commit message.

Oops! Will fix in next version.

> 
>>  drivers/net/ethernet/ibm/ibmvnic.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> index 6911b7cc06c5..46990b84cb2d 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -2453,6 +2453,9 @@ static irqreturn_t ibmvnic_interrupt_rx(int irq, void *instance)
>>  	struct ibmvnic_sub_crq_queue *scrq = instance;
>>  	struct ibmvnic_adapter *adapter = scrq->adapter;
>>
>> +	if (adapter->state != VNIC_OPEN)
>> +		return IRQ_NONE;
>> +
> 
> Could you include a comment that this is a kdump specific check, since in any other case interrupts should be disabled if the device is closed?  Maybe you could use unlikely() here too.

Yes, I can add this update to the next version.

-Nathan

> 
> Thanks,
> Tom
> 
>>  	adapter->rx_stats_buffers[scrq->scrq_num].interrupts++;
>>
>>  	if (napi_schedule_prep(&adapter->napi[scrq->scrq_num])) {
>>
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 6911b7cc06c5..46990b84cb2d 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2453,6 +2453,9 @@  static irqreturn_t ibmvnic_interrupt_rx(int irq, void *instance)
 	struct ibmvnic_sub_crq_queue *scrq = instance;
 	struct ibmvnic_adapter *adapter = scrq->adapter;
 
+	if (adapter->state != VNIC_OPEN)
+		return IRQ_NONE;
+
 	adapter->rx_stats_buffers[scrq->scrq_num].interrupts++;
 
 	if (napi_schedule_prep(&adapter->napi[scrq->scrq_num])) {