RESEND: V3 i2c-eg20t:fix race between i2c init and interrupt enable

RESEND: V3 i2c-eg20t:fix race between i2c init and interrupt enable

[Yadi Hu]

the eg20t driver call request_irq() function before the pch_base_address,
base address of i2c controller's register, isassigned an effective value.

there is one possible scenario that an interrupt which isn't inside eg20t
arrives immediately after request_irq() is executed when i2c controller
shares an interrupt number with others.  since the interrupt handler
pch_i2c_handler() has already active as shared action, it will be called
and read its own register to determine if this interrupt is from itself.

At that moment, since base address of i2c registers is not remapped
in kernel space yet,so the INT handler will access an illegal address
and then a error occurs.

Bad IO access at port 0x18 (return inl(port))
 Call Trace:
  [<c102c733>] warn_slowpath_common+0x73/0xa0
  [<c13109f5>] ? bad_io_access+0x45/0x50
  [<c13109f5>] ? bad_io_access+0x45/0x50
  [<c102c804>] warn_slowpath_fmt+0x34/0x40
  [<c13109f5>] bad_io_access+0x45/0x50
  [<c1310b62>] ioread32+0x22/0x40
  [<c14c60db>] pch_i2c_handler+0x3b/0x120
  [<c1092f34>] handle_irq_event_percpu+0x64/0x330
  [<c109323b>] handle_irq_event+0x3b/0x60
  [<c1095b50>] ? unmask_irq+0x30/0x30
  [<c1095ba0>] handle_fasteoi_irq+0x50/0xe0
  <IRQ>  [<c16e7e78>] ? do_IRQ+0x48/0xc0
  [<c16e7f6f>] ? smp_apic_timer_interrupt+0x7f/0x17a
  [<c16e7da9>] ? common_interrupt+0x29/0x30
  [<c1008ebb>] ? mwait_idle+0x8b/0x2c0
  [<c1009e8e>] ? cpu_idle+0x9e/0xc0
  [<c16be408>] ? rest_init+0x6c/0x74
  [<c19f770a>] ? start_kernel+0x311/0x318
  [<c19f7231>] ? repair_env_string+0x51/0x51
  [<c19f7078>] ? i386_start_kernel+0x78/0x7d
 ---[ end trace eb3a1028f468a140 ]---
 i2c_eg20t 0000:05:0c.2: pch_i2c_handler :I2C-3 mode(0) is not supported


Yadi Hu (1)
    i2c-eg20t: fix race between i2c init and interrupt enable

drivers/i2c/busses/i2c-eg20t.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

[PATCH] i2c-eg20t: fix race between i2c init and interrupt enable

[Yadi Hu]

From: "Yadi.hu" <yadi.hu@windriver.com>

the eg20t driver call request_irq() function before the pch_base_address,
base address of i2c controller's register, is assigned an effective value.

there is one possible scenario that an interrupt which isn't inside eg20t
arrives immediately after request_irq() is executed when i2c controller
shares an interrupt number with others. since the interrupt handler
pch_i2c_handler() has already active as shared action, it will be called
and read its own register to determine if this interrupt is from itself.

At that moment, since base address of i2c registers is not remapped
in kernel space yet,so the INT handler will access an illegal address
and then a error occurs.

Signed-off-by: Yadi.hu <yadi.hu@windriver.com>
---
 drivers/i2c/busses/i2c-eg20t.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
index 137125b..5ce71ce 100644
--- a/drivers/i2c/busses/i2c-eg20t.c
+++ b/drivers/i2c/busses/i2c-eg20t.c
@@ -773,13 +773,6 @@ static int pch_i2c_probe(struct pci_dev *pdev,
 	/* Set the number of I2C channel instance */
 	adap_info->ch_num = id->driver_data;
 
-	ret = request_irq(pdev->irq, pch_i2c_handler, IRQF_SHARED,
-		  KBUILD_MODNAME, adap_info);
-	if (ret) {
-		pch_pci_err(pdev, "request_irq FAILED\n");
-		goto err_request_irq;
-	}
-
 	for (i = 0; i < adap_info->ch_num; i++) {
 		pch_adap = &adap_info->pch_data[i].pch_adapter;
 		adap_info->pch_i2c_suspended = false;
@@ -797,6 +790,17 @@ static int pch_i2c_probe(struct pci_dev *pdev,
 
 		pch_adap->dev.of_node = pdev->dev.of_node;
 		pch_adap->dev.parent = &pdev->dev;
+	}
+
+	ret = request_irq(pdev->irq, pch_i2c_handler, IRQF_SHARED,
+		  KBUILD_MODNAME, adap_info);
+	if (ret) {
+		pch_pci_err(pdev, "request_irq FAILED\n");
+		goto err_request_irq;
+	}
+
+	for (i = 0; i < adap_info->ch_num; i++) {
+		pch_adap = &adap_info->pch_data[i].pch_adapter;
 
 		pch_i2c_init(&adap_info->pch_data[i]);
 
-- 
2.9.3

Re: [PATCH] i2c-eg20t: fix race between i2c init and interrupt enable

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On Sun, Sep 18, 2016 at 06:52:31PM +0800, Yadi Hu wrote:
> From: "Yadi.hu" <yadi.hu@windriver.com>
> 
> the eg20t driver call request_irq() function before the pch_base_address,
> base address of i2c controller's register, is assigned an effective value.
> 
> there is one possible scenario that an interrupt which isn't inside eg20t
> arrives immediately after request_irq() is executed when i2c controller
> shares an interrupt number with others. since the interrupt handler
> pch_i2c_handler() has already active as shared action, it will be called
> and read its own register to determine if this interrupt is from itself.
> 
> At that moment, since base address of i2c registers is not remapped
> in kernel space yet,so the INT handler will access an illegal address
> and then a error occurs.
> 
> Signed-off-by: Yadi.hu <yadi.hu@windriver.com>

Applied to for-next, thanks!

Please make sure "V3" also appears in the patch subject since patch
management tools pick this up. "-v <nr>" in recent git versions makes
this super easy.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-eg20t: fix race between i2c init and interrupt enable

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Wed, Sep 21, 2016 at 06:16:09PM +0200, Wolfram Sang wrote:
> On Sun, Sep 18, 2016 at 06:52:31PM +0800, Yadi Hu wrote:
> > From: "Yadi.hu" <yadi.hu@windriver.com>
> > 
> > the eg20t driver call request_irq() function before the pch_base_address,
> > base address of i2c controller's register, is assigned an effective value.
> > 
> > there is one possible scenario that an interrupt which isn't inside eg20t
> > arrives immediately after request_irq() is executed when i2c controller
> > shares an interrupt number with others. since the interrupt handler
> > pch_i2c_handler() has already active as shared action, it will be called
> > and read its own register to determine if this interrupt is from itself.
> > 
> > At that moment, since base address of i2c registers is not remapped
> > in kernel space yet,so the INT handler will access an illegal address
> > and then a error occurs.
> > 
> > Signed-off-by: Yadi.hu <yadi.hu@windriver.com>
> 
> Applied to for-next, thanks!

I meant: applied to for-current!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-eg20t: fix race between i2c init and interrupt enable

[Yadi]

On 2016年09月22日 00:16, Wolfram Sang wrote:
> On Sun, Sep 18, 2016 at 06:52:31PM +0800, Yadi Hu wrote:
>> From: "Yadi.hu" <yadi.hu@windriver.com>
>>
>> the eg20t driver call request_irq() function before the pch_base_address,
>> base address of i2c controller's register, is assigned an effective value.
>>
>> there is one possible scenario that an interrupt which isn't inside eg20t
>> arrives immediately after request_irq() is executed when i2c controller
>> shares an interrupt number with others. since the interrupt handler
>> pch_i2c_handler() has already active as shared action, it will be called
>> and read its own register to determine if this interrupt is from itself.
>>
>> At that moment, since base address of i2c registers is not remapped
>> in kernel space yet,so the INT handler will access an illegal address
>> and then a error occurs.
>>
>> Signed-off-by: Yadi.hu <yadi.hu@windriver.com>
> Applied to for-next, thanks!
>
> Please make sure "V3" also appears in the patch subject since patch
> management tools pick this up. "-v <nr>" in recent git versions makes
> this super easy.

Got it, I will resend the patch with V3 tag.


Yadi

Re: [PATCH] i2c-eg20t: fix race between i2c init and interrupt enable

[Yadi]

On 2016年09月22日 00:18, Wolfram Sang wrote:
> On Wed, Sep 21, 2016 at 06:16:09PM +0200, Wolfram Sang wrote:
>> On Sun, Sep 18, 2016 at 06:52:31PM +0800, Yadi Hu wrote:
>>> From: "Yadi.hu" <yadi.hu@windriver.com>
>>>
>>> the eg20t driver call request_irq() function before the pch_base_address,
>>> base address of i2c controller's register, is assigned an effective value.
>>>
>>> there is one possible scenario that an interrupt which isn't inside eg20t
>>> arrives immediately after request_irq() is executed when i2c controller
>>> shares an interrupt number with others. since the interrupt handler
>>> pch_i2c_handler() has already active as shared action, it will be called
>>> and read its own register to determine if this interrupt is from itself.
>>>
>>> At that moment, since base address of i2c registers is not remapped
>>> in kernel space yet,so the INT handler will access an illegal address
>>> and then a error occurs.
>>>
>>> Signed-off-by: Yadi.hu <yadi.hu@windriver.com>
>> Applied to for-next, thanks!
> I meant: applied to for-current!
one quick and stupid question, what's different between for-current and 
for-next branch?

Yadi
>

Re: [PATCH] i2c-eg20t: fix race between i2c init and interrupt enable

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]


> >Applied to for-next, thanks!
> >
> >Please make sure "V3" also appears in the patch subject since patch
> >management tools pick this up. "-v <nr>" in recent git versions makes
> >this super easy.
> 
> Got it, I will resend the patch with V3 tag.

No need, it is already applied.

for-current carries patches for 4.8, for-next for 4.9.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RESEND:v3 i2c-eg20t:fix race between i2c init and interrupt enable

[Yadi Hu]

the eg20t driver call request_irq() function before the pch_base_address,
base address of i2c controller's register, isassigned an effective value.

there is one possible scenario that an interrupt which isn't inside eg20t
arrives immediately after request_irq() is executed when i2c controller
shares an interrupt number with others.  since the interrupt handler
pch_i2c_handler() has already active as shared action, it will be called
and read its own register to determine if this interrupt is from itself.

At that moment, since base address of i2c registers is not remapped
in kernel space yet,so the INT handler will access an illegal address
and then a error occurs.

Bad IO access at port 0x18 (return inl(port))
 Call Trace:
  [<c102c733>] warn_slowpath_common+0x73/0xa0
  [<c13109f5>] ? bad_io_access+0x45/0x50
  [<c13109f5>] ? bad_io_access+0x45/0x50
  [<c102c804>] warn_slowpath_fmt+0x34/0x40
  [<c13109f5>] bad_io_access+0x45/0x50
  [<c1310b62>] ioread32+0x22/0x40
  [<c14c60db>] pch_i2c_handler+0x3b/0x120
  [<c1092f34>] handle_irq_event_percpu+0x64/0x330
  [<c109323b>] handle_irq_event+0x3b/0x60
  [<c1095b50>] ? unmask_irq+0x30/0x30
  [<c1095ba0>] handle_fasteoi_irq+0x50/0xe0
  <IRQ>  [<c16e7e78>] ? do_IRQ+0x48/0xc0
  [<c16e7f6f>] ? smp_apic_timer_interrupt+0x7f/0x17a
  [<c16e7da9>] ? common_interrupt+0x29/0x30
  [<c1008ebb>] ? mwait_idle+0x8b/0x2c0
  [<c1009e8e>] ? cpu_idle+0x9e/0xc0
  [<c16be408>] ? rest_init+0x6c/0x74
  [<c19f770a>] ? start_kernel+0x311/0x318
  [<c19f7231>] ? repair_env_string+0x51/0x51
  [<c19f7078>] ? i386_start_kernel+0x78/0x7d
 ---[ end trace eb3a1028f468a140 ]---
 i2c_eg20t 0000:05:0c.2: pch_i2c_handler :I2C-3 mode(0) is not supported

Yadi Hu (1)
    i2c-eg20t: fix race between i2c init and interrupt enable

drivers/i2c/busses/i2c-eg20t.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)