From: Liwei Song <[email protected]>
Fix the following calltrace:
kernel BUG at drivers/iommu/intel-iommu.c:3260!
invalid opcode: 0000 [#5] PREEMPT SMP
Hardware name: Intel Corp. Harcuvar/Server, BIOS HAVLCRB0.X64.0013.D39.1608311820 08/31/2016
task: ffff880175389950 ti: ffff880176bec000 task.ti: ffff880176bec000
RIP: 0010:[<ffffffff8150a83b>] [<ffffffff8150a83b>] intel_unmap+0x25b/0x260
RSP: 0018:ffff880176bef5e8 EFLAGS: 00010296
RAX: 0000000000000024 RBX: ffff8800773c7c88 RCX: 000000000000ce04
RDX: 0000000080000000 RSI: 0000000000000000 RDI: 0000000000000009
RBP: ffff880176bef638 R08: 0000000000000010 R09: 0000000000000004
R10: ffff880175389c78 R11: 0000000000000a4f R12: ffff8800773c7868
R13: 00000000ffffac88 R14: ffff8800773c7818 R15: 0000000000000001
FS: 00007fef21258700(0000) GS:ffff88017b5c0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000066d6d8 CR3: 000000007118c000 CR4: 00000000003406e0
Stack:
00000000ffffac88 ffffffff8199867f ffff880176bef5f8 ffff880100000030
ffff880176bef668 ffff8800773c7c88 ffff880178288098 ffff8800772c0010
ffff8800773c7818 0000000000000001 ffff880176bef648 ffffffff8150a86e
Call Trace:
[<ffffffff8199867f>] ? printk+0x46/0x48
[<ffffffff8150a86e>] intel_unmap_page+0xe/0x10
[<ffffffffa039d99b>] ismt_access+0x27b/0x8fa [i2c_ismt]
[<ffffffff81554420>] ? __pm_runtime_suspend+0xa0/0xa0
[<ffffffff815544a0>] ? pm_suspend_timer_fn+0x80/0x80
[<ffffffff81554420>] ? __pm_runtime_suspend+0xa0/0xa0
[<ffffffff815544a0>] ? pm_suspend_timer_fn+0x80/0x80
[<ffffffff8143dfd0>] ? pci_bus_read_dev_vendor_id+0xf0/0xf0
[<ffffffff8172b36c>] i2c_smbus_xfer+0xec/0x4b0
[<ffffffff810aa4d5>] ? vprintk_emit+0x345/0x530
[<ffffffffa038936b>] i2cdev_ioctl_smbus+0x12b/0x240 [i2c_dev]
[<ffffffff810aa829>] ? vprintk_default+0x29/0x40
[<ffffffffa0389b33>] i2cdev_ioctl+0x63/0x1ec [i2c_dev]
[<ffffffff811b04c8>] do_vfs_ioctl+0x328/0x5d0
[<ffffffff8119d8ec>] ? vfs_write+0x11c/0x190
[<ffffffff8109d449>] ? rt_up_read+0x19/0x20
[<ffffffff811b07f1>] SyS_ioctl+0x81/0xa0
[<ffffffff819a351b>] system_call_fastpath+0x16/0x6e
This happen When run "i2cdetect -y 0" detect SMBus iSMT adapter.
After finished I2C block read/write, when unmap the data buffer,
a wrong device address was pass to dma_unmap_single(), the right
device address should be "dev" not "&adap->dev", the relation is
*(&adap->dev) == dev. When come into Intel IOMMU routine, the wrong
devices address was operated.
To fix this, give dma_unmap_single() the "dev" parameter, just like
what dma_map_single() does, then unmap can find the right devices.
Signed-off-by: Liwei Song <[email protected]>
---
drivers/i2c/busses/i2c-ismt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index 1db3e0d..605d44e 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -585,7 +585,7 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
/* unmap the data buffer */
if (dma_size != 0)
- dma_unmap_single(&adap->dev, dma_addr, dma_size, dma_direction);
+ dma_unmap_single(dev, dma_addr, dma_size, dma_direction);
if (unlikely(!time_left)) {
dev_err(dev, "completion wait timed out\n");
--
2.7.4
On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <[email protected]> wrote:
> From: Liwei Song <[email protected]>
>
> Fix the following calltrace:
No, you don't fix a call trace, you are fixing a bug.
> This happen When run "i2cdetect -y 0" detect SMBus iSMT adapter.
>
> After finished I2C block read/write, when unmap the data buffer,
> a wrong device address was pass to dma_unmap_single(),
> the right
> device address should be "dev" not "&adap->dev", the relation is
> *(&adap->dev) == dev.
This is confusing. You are telling that there are two copies of struct
device here?
Otherwise if one is a pointer to the real struct device, there
shouldn't be a problem.
> When come into Intel IOMMU routine, the wrong
> devices address was operated.
This basically duplicates what you have said previously.
> To fix this, give dma_unmap_single() the "dev" parameter, just like
> what dma_map_single() does, then unmap can find the right devices.
Fix per se looks good, explanation is confusing.
>
> Signed-off-by: Liwei Song <[email protected]>
> ---
> drivers/i2c/busses/i2c-ismt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
> index 1db3e0d..605d44e 100644
> --- a/drivers/i2c/busses/i2c-ismt.c
> +++ b/drivers/i2c/busses/i2c-ismt.c
> @@ -585,7 +585,7 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>
> /* unmap the data buffer */
> if (dma_size != 0)
> - dma_unmap_single(&adap->dev, dma_addr, dma_size, dma_direction);
> + dma_unmap_single(dev, dma_addr, dma_size, dma_direction);
>
> if (unlikely(!time_left)) {
> dev_err(dev, "completion wait timed out\n");
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
On 2017-06-12 11:11, Andy Shevchenko wrote:
> On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <[email protected]> wrote:
>> From: Liwei Song <[email protected]>
>>
>
>> Fix the following calltrace:
>
> No, you don't fix a call trace, you are fixing a bug.
>
>> This happen When run "i2cdetect -y 0" detect SMBus iSMT adapter.
>>
>> After finished I2C block read/write, when unmap the data buffer,
>> a wrong device address was pass to dma_unmap_single(),
>
>
>> the right
>> device address should be "dev" not "&adap->dev", the relation is
>> *(&adap->dev) == dev.
>
> This is confusing. You are telling that there are two copies of struct
> device here?
Yes, there are two copies. There's the local dev variable that is like
this:
struct device *dev = &priv->pci_dev->dev;
And then there's the adapter device in adap->dev (inlined in adap, so
the explanation that the relations is "*(&adap->dev) == dev" is doubly
wrong).
The bug is that the first argument to dma_unmap_single is not the
same as the first argument to dma_map_single that appears a few lines
up. I cannot tell if that argument should be "dev" or "&adap->dev"
though, but the two calls must refer to the same struct device *.
Cheers,
peda
> Otherwise if one is a pointer to the real struct device, there
> shouldn't be a problem.
>
>> When come into Intel IOMMU routine, the wrong
>> devices address was operated.
>
> This basically duplicates what you have said previously.
>
>> To fix this, give dma_unmap_single() the "dev" parameter, just like
>> what dma_map_single() does, then unmap can find the right devices.
>
> Fix per se looks good, explanation is confusing.
>
>>
>> Signed-off-by: Liwei Song <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-ismt.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
>> index 1db3e0d..605d44e 100644
>> --- a/drivers/i2c/busses/i2c-ismt.c
>> +++ b/drivers/i2c/busses/i2c-ismt.c
>> @@ -585,7 +585,7 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>>
>> /* unmap the data buffer */
>> if (dma_size != 0)
>> - dma_unmap_single(&adap->dev, dma_addr, dma_size, dma_direction);
>> + dma_unmap_single(dev, dma_addr, dma_size, dma_direction);
>>
>> if (unlikely(!time_left)) {
>> dev_err(dev, "completion wait timed out\n");
>> --
>> 2.7.4
>>
>
>
>
On Mon, Jun 12, 2017 at 12:28 PM, Peter Rosin <[email protected]> wrote:
> On 2017-06-12 11:11, Andy Shevchenko wrote:
>> On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <[email protected]> wrote:
>>> From: Liwei Song <[email protected]>
>>> After finished I2C block read/write, when unmap the data buffer,
>>> a wrong device address was pass to dma_unmap_single(),
>>> the right
>>> device address should be "dev" not "&adap->dev", the relation is
>>> *(&adap->dev) == dev.
>>
>> This is confusing. You are telling that there are two copies of struct
>> device here?
>
> Yes, there are two copies.
No, there is not. See below.
> There's the local dev variable that is like
> this:
> struct device *dev = &priv->pci_dev->dev;
>
> And then there's the adapter device in adap->dev (inlined in adap, so
> the explanation that the relations is "*(&adap->dev) == dev" is doubly
> wrong).
I got the idea, but your explanation is not a penny better.
There are two struct devices, one is a real PCI device, which
represents actual device what *does* DMA.
This struct should be used according to DMA API.
Another struct device which is wrongly used is an artificial one that
represents I2C adapter in terms of Linux kernel.
The relation ship (if designed correctly) *should be* adap->dev.parent
== &pci_dev->dev.
> The bug is that the first argument to dma_unmap_single is not the
> same as the first argument to dma_map_single that appears a few lines
> up.
I understand that.
> I cannot tell if that argument should be "dev" or "&adap->dev"
> though, but the two calls must refer to the same struct device *.
See above. It's a simple choice.
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <[email protected]> wrote:
> From: Liwei Song <[email protected]>
Btw, it seems it has to include Fixes tag.
> Signed-off-by: Liwei Song <[email protected]>
> ---
> drivers/i2c/busses/i2c-ismt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
> index 1db3e0d..605d44e 100644
> --- a/drivers/i2c/busses/i2c-ismt.c
> +++ b/drivers/i2c/busses/i2c-ismt.c
> @@ -585,7 +585,7 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>
> /* unmap the data buffer */
> if (dma_size != 0)
> - dma_unmap_single(&adap->dev, dma_addr, dma_size, dma_direction);
> + dma_unmap_single(dev, dma_addr, dma_size, dma_direction);
>
> if (unlikely(!time_left)) {
> dev_err(dev, "completion wait timed out\n");
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
On 2017-06-12 11:38, Andy Shevchenko wrote:
> On Mon, Jun 12, 2017 at 12:28 PM, Peter Rosin <[email protected]> wrote:
>> On 2017-06-12 11:11, Andy Shevchenko wrote:
>>> On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <[email protected]> wrote:
>>>> From: Liwei Song <[email protected]>
>
>>>> After finished I2C block read/write, when unmap the data buffer,
>>>> a wrong device address was pass to dma_unmap_single(),
>
>>>> the right
>>>> device address should be "dev" not "&adap->dev", the relation is
>>>> *(&adap->dev) == dev.
>>>
>>> This is confusing. You are telling that there are two copies of struct
>>> device here?
>>
>> Yes, there are two copies.
>
> No, there is not. See below.
What I meant was that there are the struct device in pci_dev->dev and the
struct device in adap->dev. That seems like two copies of struct device
to me. I didn't mean that they are copies in the sense that they have the
same content, but in the sense that they are both struct device.
I guess we can argue ourselves blue over this point.
> There are two struct devices,
Hmm, two struct devices, I seem to recall that from somewhere... :-)
> one is a real PCI device, which
> represents actual device what *does* DMA.
> This struct should be used according to DMA API.
When you put it like that, it's obvious that the patch is correct. I had
this feeling that little thought had gone into the choice to pick "dev"
over "&adap->dev", that's all.
> Another struct device which is wrongly used is an artificial one that
> represents I2C adapter in terms of Linux kernel.
Cheers,
peda
On Mon, Jun 12, 2017 at 1:19 PM, Peter Rosin <[email protected]> wrote:
> On 2017-06-12 11:38, Andy Shevchenko wrote:
>> On Mon, Jun 12, 2017 at 12:28 PM, Peter Rosin <[email protected]> wrote:
>>> On 2017-06-12 11:11, Andy Shevchenko wrote:
>>>> On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <[email protected]> wrote:
>>>>> From: Liwei Song <[email protected]>
>>
>>>>> After finished I2C block read/write, when unmap the data buffer,
>>>>> a wrong device address was pass to dma_unmap_single(),
>>
>>>>> the right
>>>>> device address should be "dev" not "&adap->dev", the relation is
>>>>> *(&adap->dev) == dev.
>>>>
>>>> This is confusing. You are telling that there are two copies of struct
>>>> device here?
>>>
>>> Yes, there are two copies.
>>
>> No, there is not. See below.
>
> What I meant was that there are the struct device in pci_dev->dev and the
> struct device in adap->dev. That seems like two copies of struct device
> to me.
They are not copies. That's my point.
> I didn't mean that they are copies in the sense that they have the
> same content, but in the sense that they are both struct device.
>
> I guess we can argue ourselves blue over this point.
See above.
>> There are two struct devices,
>
> Hmm, two struct devices, I seem to recall that from somewhere... :-)
Okay, it's possible bad wording from my side.
>> one is a real PCI device, which
>> represents actual device what *does* DMA.
>> This struct should be used according to DMA API.
> When you put it like that, it's obvious that the patch is correct.
I agreed with this in the first place! See my first reply.
> I had
> this feeling that little thought had gone into the choice to pick "dev"
> over "&adap->dev", that's all.
As I said, my concern is the commit message to the change which is
totally confusing.
--
With Best Regards,
Andy Shevchenko