Message ID | 1497238928-41066-1-git-send-email-liwei.song@windriver.com |
---|---|
State | Superseded |
Headers | show |
On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <liwei.song@windriver.com> wrote: > From: Liwei Song <liwei.song@windriver.com> > > 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 <liwei.song@windriver.com> > --- > 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 2017-06-12 11:11, Andy Shevchenko wrote: > On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <liwei.song@windriver.com> wrote: >> From: Liwei Song <liwei.song@windriver.com> >> > >> 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 <liwei.song@windriver.com> >> --- >> 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 >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 12, 2017 at 12:28 PM, Peter Rosin <peda@axentia.se> wrote: > On 2017-06-12 11:11, Andy Shevchenko wrote: >> On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <liwei.song@windriver.com> wrote: >>> From: Liwei Song <liwei.song@windriver.com> >>> 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.
On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <liwei.song@windriver.com> wrote: > From: Liwei Song <liwei.song@windriver.com> Btw, it seems it has to include Fixes tag. > Signed-off-by: Liwei Song <liwei.song@windriver.com> > --- > 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 2017-06-12 11:38, Andy Shevchenko wrote: > On Mon, Jun 12, 2017 at 12:28 PM, Peter Rosin <peda@axentia.se> wrote: >> On 2017-06-12 11:11, Andy Shevchenko wrote: >>> On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <liwei.song@windriver.com> wrote: >>>> From: Liwei Song <liwei.song@windriver.com> > >>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 12, 2017 at 1:19 PM, Peter Rosin <peda@axentia.se> wrote: > On 2017-06-12 11:38, Andy Shevchenko wrote: >> On Mon, Jun 12, 2017 at 12:28 PM, Peter Rosin <peda@axentia.se> wrote: >>> On 2017-06-12 11:11, Andy Shevchenko wrote: >>>> On Mon, Jun 12, 2017 at 6:42 AM, Song liwei <liwei.song@windriver.com> wrote: >>>>> From: Liwei Song <liwei.song@windriver.com> >> >>>>> 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.
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");