subject:"RE\: \[PATCH V3 0\/8\] IOMMU probe deferral support"

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-30 Thread Lorenzo Pieralisi

Hi Sricharan,

On Wed, Nov 30, 2016 at 06:04:13AM +0530, Sricharan wrote:
> Hi Lorenzo,
> 
> >-Original Message-
> >From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] 
> >On Behalf Of Lorenzo Pieralisi
> >Sent: Monday, November 28, 2016 11:44 PM
> >To: Sricharan <sricha...@codeaurora.org>
> >Cc: linux-arm-...@vger.kernel.org; j...@8bytes.org; will.dea...@arm.com; 
> >tf...@chromium.org; iommu@lists.linux-
> >foundation.org; srinivas.kandaga...@linaro.org; 
> >laurent.pinch...@ideasonboard.com; 'Robin Murphy' <robin.mur...@arm.com>;
> >linux-arm-ker...@lists.infradead.org; m.szyprow...@samsung.com
> >Subject: Re: [PATCH V3 0/8] IOMMU probe deferral support
> >
> >On Mon, Nov 28, 2016 at 11:12:08PM +0530, Sricharan wrote:
> >
> >[...]
> >
> >> >Cool. We're rather hoping that the ACPI stuff is good to go for 4.10
> >> >now, so it's probably worth pulling the rest of that in (beyond the one
> >> >patch I picked) to make sure the of_dma_configure/acpi_dma_configure
> >> >paths don't inadvertently diverge.
> >> >
> >>
> >> I rebased and was testing your branch with Lorenzo's series. One thing
> >> i am still trying to get right is the acpi_dma_configure call. With your
> >> series dma_configure calls pci_dma/of_dma configure, so i am just adding
> >> acpi_dma_configure call there for non-pci ACPI devices as well. I see that
> >> acpi_dma_configure right now is called from acpi_bind_one and
> >> iort_add_smmu_platform_device, both go through the really_probe function
> >> path, so moving acpi_dma_configure from the above the two functions
> >> to dma_configure. I remember we discussed this on another thread, so
> >> hopefully it is correct. I do not have an platform to test the ACPI though.
> >> I will take some testing help on V4 for this.
> >
> >I am happy to test it for you please just send me a pointer at your v4
> >code.
> >
>  I posted the v4 and CCed you there. So i am little skeptical about the acpi
> changes that i have posted. I was checking for a function equivalent 
> in acpi as of_match_node in DT, to figure out if the iommu_spec.np that
> the master device is pointing to is there in the iommu_of_table and based
> on that we can decide if to defer the probe. I was seeing iort_scan_node
> was its equivalent. But if that is not correct, then last patch has to be 
> reworked.
> Anyways will be good to know your feedback on this.

Sure I will test it asap, thanks for putting it together.

Thanks,
Lorenzo
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-29 Thread Sricharan

Hi Lorenzo,

>-Original Message-
>From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] 
>On Behalf Of Lorenzo Pieralisi
>Sent: Monday, November 28, 2016 11:44 PM
>To: Sricharan <sricha...@codeaurora.org>
>Cc: linux-arm-...@vger.kernel.org; j...@8bytes.org; will.dea...@arm.com; 
>tf...@chromium.org; iommu@lists.linux-
>foundation.org; srinivas.kandaga...@linaro.org; 
>laurent.pinch...@ideasonboard.com; 'Robin Murphy' <robin.mur...@arm.com>;
>linux-arm-ker...@lists.infradead.org; m.szyprow...@samsung.com
>Subject: Re: [PATCH V3 0/8] IOMMU probe deferral support
>
>On Mon, Nov 28, 2016 at 11:12:08PM +0530, Sricharan wrote:
>
>[...]
>
>> >Cool. We're rather hoping that the ACPI stuff is good to go for 4.10
>> >now, so it's probably worth pulling the rest of that in (beyond the one
>> >patch I picked) to make sure the of_dma_configure/acpi_dma_configure
>> >paths don't inadvertently diverge.
>> >
>>
>> I rebased and was testing your branch with Lorenzo's series. One thing
>> i am still trying to get right is the acpi_dma_configure call. With your
>> series dma_configure calls pci_dma/of_dma configure, so i am just adding
>> acpi_dma_configure call there for non-pci ACPI devices as well. I see that
>> acpi_dma_configure right now is called from acpi_bind_one and
>> iort_add_smmu_platform_device, both go through the really_probe function
>> path, so moving acpi_dma_configure from the above the two functions
>> to dma_configure. I remember we discussed this on another thread, so
>> hopefully it is correct. I do not have an platform to test the ACPI though.
>> I will take some testing help on V4 for this.
>
>I am happy to test it for you please just send me a pointer at your v4
>code.
>
 I posted the v4 and CCed you there. So i am little skeptical about the acpi
changes that i have posted. I was checking for a function equivalent 
in acpi as of_match_node in DT, to figure out if the iommu_spec.np that
the master device is pointing to is there in the iommu_of_table and based
on that we can decide if to defer the probe. I was seeing iort_scan_node
was its equivalent. But if that is not correct, then last patch has to be 
reworked.
Anyways will be good to know your feedback on this.

Regards,
 Sricharan


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-28 Thread Lorenzo Pieralisi

On Mon, Nov 28, 2016 at 11:12:08PM +0530, Sricharan wrote:

[...]

> >Cool. We're rather hoping that the ACPI stuff is good to go for 4.10
> >now, so it's probably worth pulling the rest of that in (beyond the one
> >patch I picked) to make sure the of_dma_configure/acpi_dma_configure
> >paths don't inadvertently diverge.
> >
> 
> I rebased and was testing your branch with Lorenzo's series. One thing
> i am still trying to get right is the acpi_dma_configure call. With your
> series dma_configure calls pci_dma/of_dma configure, so i am just adding
> acpi_dma_configure call there for non-pci ACPI devices as well. I see that
> acpi_dma_configure right now is called from acpi_bind_one and 
> iort_add_smmu_platform_device, both go through the really_probe function
> path, so moving acpi_dma_configure from the above the two functions
> to dma_configure. I remember we discussed this on another thread, so
> hopefully it is correct. I do not have an platform to test the ACPI though.
> I will take some testing help on V4 for this.

I am happy to test it for you please just send me a pointer at your v4
code.

Thank you !
Lorenzo
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-28 Thread Sricharan

Hi Robin,

>> 
>>
>>>
 iommu_group_get_for_dev which gets called in the add_device
 callback, increases the reference count of the iommu_group,
 so we do an iommu_group_put after that. iommu_group_get_for_dev
 inturn calls device_group callback and in the case of arm-smmu
 we call generic_device_group/pci_device_group which takes
 care of increasing the group's reference. But when we return
 an already existing group(when multiple devices have same group)
 the reference is not incremented, resulting in issues when the
 remove_device callback for the devices is invoked.
 Fixing the same here.
>>>
>>> Bah, yes, this does look like my fault - after flip-flopping between
>>> about 3 different ways to keep refcounts for the S2CR entries, none of
>>> which would quite work, I ripped it all out but apparently still got
>>> things wrong, oh well. Thanks for figuring it out.
>>>
>>> On the probe-deferral angle, whilst it's useful to have uncovered this
>>> bug, I don't think we should actually be calling remove_device() from
>>> DMA teardown. I think it's preferable from a user perspective if group
>>> numbering remains stable, rather than changing depending on the order in
>>> which they unbind/rebind VFIO drivers. I'm really keen to try and get
>>> this in shape for 4.10, so I've taken the liberty of hacking up my own
>>> branch (iommu/defer) based on v3 - would you mind taking a look at the
>>> two "iommu/of:" commits to see what you think? (Ignore the PCI changes
>>> to your later patches - that was an experiment which didn't really work 
>>> out)
>>
>> Ok, will take a look at this now and respond more on this.
>>
> Sorry for the delayed response on this. I was OOO for the last few days.
> So i tested this branch and it worked fine. I tested it with a pci device
> for both normal and deferred probe cases.  The of/iommu patches
> are the cleanup/preparation patches and it looks fine. One thing is 
> without
> calling the remove_device callback, the resources like (smes for exmaple)
> and the group association of the device all remain allocated. That does 
> not
> feel correct, given that the associated device does not exist. So to
> understand that, what happens with VFIO in this case which makes the
> group renumbering/rebinding a problem ?
>

 Would it be ok if i post a V4 based on your branch above ?
>>>
>>> Sure, as long as none of the hacks slip through :) - I've just pushed
>>> out a mild rework based on Lorenzo's v9, which I hope shouldn't break
>>> anything for you.
>>>
>>
>> Ok sure, i will test and just the post out the stuff from your branch then
>> mostly by tomorrow.
>
>Cool. We're rather hoping that the ACPI stuff is good to go for 4.10
>now, so it's probably worth pulling the rest of that in (beyond the one
>patch I picked) to make sure the of_dma_configure/acpi_dma_configure
>paths don't inadvertently diverge.
>

I rebased and was testing your branch with Lorenzo's series. One thing
i am still trying to get right is the acpi_dma_configure call. With your
series dma_configure calls pci_dma/of_dma configure, so i am just adding
acpi_dma_configure call there for non-pci ACPI devices as well. I see that
acpi_dma_configure right now is called from acpi_bind_one and 
iort_add_smmu_platform_device, both go through the really_probe function
path, so moving acpi_dma_configure from the above the two functions
to dma_configure. I remember we discussed this on another thread, so
hopefully it is correct. I do not have an platform to test the ACPI though.
I will take some testing help on V4 for this.

>>> Having thought a bit more about the add/remove thing, I'm inclined to
>>> agree that the group numbering itself may not be that big an issue in
>>> practice - sure, it could break my little script, but it looks like QEMU
>>> and such work with the device ID rather than the group number directly,
>>> so might not even notice. However, the fact remains that the callbacks
>>> are intended to handle a device being added to/removed from its bus, and
>>> will continue to do so on other platforms, so I don't like the idea of
>>> introducing needlessly different behaviour. If you unbind a driver, the
>>> stream IDs and everything don't stop existing at the hardware level; the
>>> struct device to which the in-kernel data belongs still exists and
>>> doesn't stop being associated with its bus. There's no good reason for
>>> freeing SMEs that we'll only reallocate again (inadequately-specced
>>> hardware with not enough SMRs/contexts is not a *good* reason), and
>>
>> ok, so SMRs/contexts was the reason i was adding the remove_dev
>> callback, but if thats not good enough then there was no other
>> intention.
>>
>>> there are also some strong arguments against letting any stream IDs the
>>> kernel knows about

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-24 Thread Sricharan

Hi Robin,



>
>> iommu_group_get_for_dev which gets called in the add_device
>> callback, increases the reference count of the iommu_group,
>> so we do an iommu_group_put after that. iommu_group_get_for_dev
>> inturn calls device_group callback and in the case of arm-smmu
>> we call generic_device_group/pci_device_group which takes
>> care of increasing the group's reference. But when we return
>> an already existing group(when multiple devices have same group)
>> the reference is not incremented, resulting in issues when the
>> remove_device callback for the devices is invoked.
>> Fixing the same here.
>
> Bah, yes, this does look like my fault - after flip-flopping between
> about 3 different ways to keep refcounts for the S2CR entries, none of
> which would quite work, I ripped it all out but apparently still got
> things wrong, oh well. Thanks for figuring it out.
>
> On the probe-deferral angle, whilst it's useful to have uncovered this
> bug, I don't think we should actually be calling remove_device() from
> DMA teardown. I think it's preferable from a user perspective if group
> numbering remains stable, rather than changing depending on the order in
> which they unbind/rebind VFIO drivers. I'm really keen to try and get
> this in shape for 4.10, so I've taken the liberty of hacking up my own
> branch (iommu/defer) based on v3 - would you mind taking a look at the
> two "iommu/of:" commits to see what you think? (Ignore the PCI changes
> to your later patches - that was an experiment which didn't really work 
> out)

 Ok, will take a look at this now and respond more on this.

>>> Sorry for the delayed response on this. I was OOO for the last few days.
>>> So i tested this branch and it worked fine. I tested it with a pci device
>>> for both normal and deferred probe cases.  The of/iommu patches
>>> are the cleanup/preparation patches and it looks fine. One thing is without
>>> calling the remove_device callback, the resources like (smes for exmaple)
>>> and the group association of the device all remain allocated. That does not
>>> feel correct, given that the associated device does not exist. So to
>>> understand that, what happens with VFIO in this case which makes the
>>> group renumbering/rebinding a problem ?
>>>
>>
>> Would it be ok if i post a V4 based on your branch above ?
>
>Sure, as long as none of the hacks slip through :) - I've just pushed
>out a mild rework based on Lorenzo's v9, which I hope shouldn't break
>anything for you.
>

Ok sure, i will test and just the post out the stuff from your branch then
mostly by tomorrow.

>Having thought a bit more about the add/remove thing, I'm inclined to
>agree that the group numbering itself may not be that big an issue in
>practice - sure, it could break my little script, but it looks like QEMU
>and such work with the device ID rather than the group number directly,
>so might not even notice. However, the fact remains that the callbacks
>are intended to handle a device being added to/removed from its bus, and
>will continue to do so on other platforms, so I don't like the idea of
>introducing needlessly different behaviour. If you unbind a driver, the
>stream IDs and everything don't stop existing at the hardware level; the
>struct device to which the in-kernel data belongs still exists and
>doesn't stop being associated with its bus. There's no good reason for
>freeing SMEs that we'll only reallocate again (inadequately-specced
>hardware with not enough SMRs/contexts is not a *good* reason), and

ok, so SMRs/contexts was the reason i was adding the remove_dev
callback, but if thats not good enough then there was no other
intention.

>there are also some strong arguments against letting any stream IDs the
>kernel knows about go back to bypass after a driver has been bound - by

ok, but not sure why is this so ?

>keeping groups around as expected that's something we can implement
>quite easily without having to completely lock down bypass for stream
>IDs the kernel *doesn't* know about.
>

So do you mean in this case to keep the unbound device's group/context bank
to bypass rather than resetting the streamids ?

Regards,
 Sricharan

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-23 Thread Robin Murphy

On 20/11/16 15:11, Sricharan wrote:
> Hi Robin,
> 
>> Hi Robin,
>>
>>> Hi Robin,
>>>
 On 04/11/16 15:16, Sricharan wrote:
> Hi Robin,
>
 Yikes, on second look, that definitely shouldn't be happening.
 Everything below is probably the resulting fallout.
>>>
>>> [   40.206703] vfio-pci :08:00.0: Failed to setup iommu ops
>>>
>>> I think the above print which says "failed to setup iommu_ops"
>>> because the call ops->add_device failed in of_pci_iommu_configure
>>> is the reason for the failure, in my case i simply do not get this even 
>>> with
>>> your scripts. ops->add_device succeeds in the rebind as well. So still
>>> checking what could be happening in your case.
>>
>> I was looking at your code base from [1].The ops->add_device
>> callback from of_pci_iommu_configure on the rebind is the
>> one which is causing the failure. But not able to spot out
> >from code which point is causing the failure. It would be very helpful
>> if i can know which is the return value from the add_device callback
>> or point inside add_device callback which fails in your setup.
>>
>>
>> [1] git://linux-arm.org/linux-rm iommu/misc
>
> With little more try, i saw an issue where i had an failure
> similar to what you reported. The issue happens when multiple
> devices fall in to same group due to matching sids. I ended up
> doing a fix like below and it would be nice to verify if it is the same
> that we are seeing in your setup and if the fix makes a difference ?
>
> From: Sricharan R 
> Date: Fri, 4 Nov 2016 20:28:49 +0530
> Subject: [PATCH] iommu/arm-smmu: Fix group's reference counting
>
> iommu_group_get_for_dev which gets called in the add_device
> callback, increases the reference count of the iommu_group,
> so we do an iommu_group_put after that. iommu_group_get_for_dev
> inturn calls device_group callback and in the case of arm-smmu
> we call generic_device_group/pci_device_group which takes
> care of increasing the group's reference. But when we return
> an already existing group(when multiple devices have same group)
> the reference is not incremented, resulting in issues when the
> remove_device callback for the devices is invoked.
> Fixing the same here.

 Bah, yes, this does look like my fault - after flip-flopping between
 about 3 different ways to keep refcounts for the S2CR entries, none of
 which would quite work, I ripped it all out but apparently still got
 things wrong, oh well. Thanks for figuring it out.

 On the probe-deferral angle, whilst it's useful to have uncovered this
 bug, I don't think we should actually be calling remove_device() from
 DMA teardown. I think it's preferable from a user perspective if group
 numbering remains stable, rather than changing depending on the order in
 which they unbind/rebind VFIO drivers. I'm really keen to try and get
 this in shape for 4.10, so I've taken the liberty of hacking up my own
 branch (iommu/defer) based on v3 - would you mind taking a look at the
 two "iommu/of:" commits to see what you think? (Ignore the PCI changes
 to your later patches - that was an experiment which didn't really work 
 out)
>>>
>>> Ok, will take a look at this now and respond more on this.
>>>
>> Sorry for the delayed response on this. I was OOO for the last few days.
>> So i tested this branch and it worked fine. I tested it with a pci device
>> for both normal and deferred probe cases.  The of/iommu patches
>> are the cleanup/preparation patches and it looks fine. One thing is without
>> calling the remove_device callback, the resources like (smes for exmaple)
>> and the group association of the device all remain allocated. That does not
>> feel correct, given that the associated device does not exist. So to
>> understand that, what happens with VFIO in this case which makes the
>> group renumbering/rebinding a problem ?
>>
> 
> Would it be ok if i post a V4 based on your branch above ?

Sure, as long as none of the hacks slip through :) - I've just pushed
out a mild rework based on Lorenzo's v9, which I hope shouldn't break
anything for you.

Having thought a bit more about the add/remove thing, I'm inclined to
agree that the group numbering itself may not be that big an issue in
practice - sure, it could break my little script, but it looks like QEMU
and such work with the device ID rather than the group number directly,
so might not even notice. However, the fact remains that the callbacks
are intended to handle a device being added to/removed from its bus, and
will continue to do so on other platforms, so I don't like the idea of
introducing needlessly different behaviour. If you unbind a driver, the
stream IDs and everything don't stop existing at the hardware level; the
struct device to which

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-13 Thread Sricharan

Hi Robin,

>Hi Robin,
>
>>On 04/11/16 15:16, Sricharan wrote:
>>> Hi Robin,
>>>
>> Yikes, on second look, that definitely shouldn't be happening.
>> Everything below is probably the resulting fallout.
>
> [   40.206703] vfio-pci :08:00.0: Failed to setup iommu ops
>
> I think the above print which says "failed to setup iommu_ops"
> because the call ops->add_device failed in of_pci_iommu_configure
> is the reason for the failure, in my case i simply do not get this even 
> with
> your scripts. ops->add_device succeeds in the rebind as well. So still
> checking what could be happening in your case.

 I was looking at your code base from [1].The ops->add_device
 callback from of_pci_iommu_configure on the rebind is the
 one which is causing the failure. But not able to spot out
from code which point is causing the failure. It would be very helpful
 if i can know which is the return value from the add_device callback
 or point inside add_device callback which fails in your setup.


 [1] git://linux-arm.org/linux-rm iommu/misc
>>>
>>> With little more try, i saw an issue where i had an failure
>>> similar to what you reported. The issue happens when multiple
>>> devices fall in to same group due to matching sids. I ended up
>>> doing a fix like below and it would be nice to verify if it is the same
>>> that we are seeing in your setup and if the fix makes a difference ?
>>>
>>> From: Sricharan R 
>>> Date: Fri, 4 Nov 2016 20:28:49 +0530
>>> Subject: [PATCH] iommu/arm-smmu: Fix group's reference counting
>>>
>>> iommu_group_get_for_dev which gets called in the add_device
>>> callback, increases the reference count of the iommu_group,
>>> so we do an iommu_group_put after that. iommu_group_get_for_dev
>>> inturn calls device_group callback and in the case of arm-smmu
>>> we call generic_device_group/pci_device_group which takes
>>> care of increasing the group's reference. But when we return
>>> an already existing group(when multiple devices have same group)
>>> the reference is not incremented, resulting in issues when the
>>> remove_device callback for the devices is invoked.
>>> Fixing the same here.
>>
>>Bah, yes, this does look like my fault - after flip-flopping between
>>about 3 different ways to keep refcounts for the S2CR entries, none of
>>which would quite work, I ripped it all out but apparently still got
>>things wrong, oh well. Thanks for figuring it out.
> >
>>On the probe-deferral angle, whilst it's useful to have uncovered this
>>bug, I don't think we should actually be calling remove_device() from
>>DMA teardown. I think it's preferable from a user perspective if group
>>numbering remains stable, rather than changing depending on the order in
>>which they unbind/rebind VFIO drivers. I'm really keen to try and get
>>this in shape for 4.10, so I've taken the liberty of hacking up my own
>>branch (iommu/defer) based on v3 - would you mind taking a look at the
>>two "iommu/of:" commits to see what you think? (Ignore the PCI changes
>>to your later patches - that was an experiment which didn't really work out)
>
>Ok, will take a look at this now and respond more on this.
>
Sorry for the delayed response on this. I was OOO for the last few days.
So i tested this branch and it worked fine. I tested it with a pci device
for both normal and deferred probe cases.  The of/iommu patches
are the cleanup/preparation patches and it looks fine. One thing is without
calling the remove_device callback, the resources like (smes for exmaple)
and the group association of the device all remain allocated. That does not
feel correct, given that the associated device does not exist. So to
understand that, what happens with VFIO in this case which makes the
group renumbering/rebinding a problem ?

Regards,
 Sricharan


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-09 Thread Will Deacon

On Wed, Nov 09, 2016 at 11:54:20AM +0530, Sricharan wrote:
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 71ce4b6..a1d0b3c 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -1516,8 +1516,10 @@ static struct iommu_group 
> >> *arm_smmu_device_group(struct device *dev)
> >>group = smmu->s2crs[idx].group;
> >>}
> >>
> >> -  if (group)
> >> +  if (group) {
> >> +  iommu_group_get_by_id(iommu_group_id(group));
> >>return group;
> >
> >This might as well just be inline, i.e.:
> >
> > return iommu_group_get_by_id(iommu_group_id(group));
> >
> >It's a shame we have to go all round the houses when we have the group
> >right there, but this is probably the most expedient fix. I guess we can
> >extend the API with some sort of iommu_group_get(group) overload in
> >future if we really want to.
> >
> 
> ok, i can send this fix separately then. Otherwise, Will was suggesting on the
> other thread that there should probably be a separate API to increment
> the group refcount or get the group from the existing aliasing device.
> As per me adding the api, looks like another option or post the above ?

I think adding a new function to the API is the way to go -- having code
like what you propose above littered around the drivers is hard to read and
pretty error-prone, since it looks like a NOP to people who aren't already
thinking about ref counts.

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-08 Thread Sricharan

Hi Robin,

>On 04/11/16 15:16, Sricharan wrote:
>> Hi Robin,
>>
> Yikes, on second look, that definitely shouldn't be happening.
> Everything below is probably the resulting fallout.

 [   40.206703] vfio-pci :08:00.0: Failed to setup iommu ops

 I think the above print which says "failed to setup iommu_ops"
 because the call ops->add_device failed in of_pci_iommu_configure
 is the reason for the failure, in my case i simply do not get this even 
 with
 your scripts. ops->add_device succeeds in the rebind as well. So still
 checking what could be happening in your case.
>>>
>>> I was looking at your code base from [1].The ops->add_device
>>> callback from of_pci_iommu_configure on the rebind is the
>>> one which is causing the failure. But not able to spot out
>>>from code which point is causing the failure. It would be very helpful
>>> if i can know which is the return value from the add_device callback
>>> or point inside add_device callback which fails in your setup.
>>>
>>>
>>> [1] git://linux-arm.org/linux-rm iommu/misc
>>
>> With little more try, i saw an issue where i had an failure
>> similar to what you reported. The issue happens when multiple
>> devices fall in to same group due to matching sids. I ended up
>> doing a fix like below and it would be nice to verify if it is the same
>> that we are seeing in your setup and if the fix makes a difference ?
>>
>> From: Sricharan R 
>> Date: Fri, 4 Nov 2016 20:28:49 +0530
>> Subject: [PATCH] iommu/arm-smmu: Fix group's reference counting
>>
>> iommu_group_get_for_dev which gets called in the add_device
>> callback, increases the reference count of the iommu_group,
>> so we do an iommu_group_put after that. iommu_group_get_for_dev
>> inturn calls device_group callback and in the case of arm-smmu
>> we call generic_device_group/pci_device_group which takes
>> care of increasing the group's reference. But when we return
>> an already existing group(when multiple devices have same group)
>> the reference is not incremented, resulting in issues when the
>> remove_device callback for the devices is invoked.
>> Fixing the same here.
>
>Bah, yes, this does look like my fault - after flip-flopping between
>about 3 different ways to keep refcounts for the S2CR entries, none of
>which would quite work, I ripped it all out but apparently still got
>things wrong, oh well. Thanks for figuring it out.
 >
>On the probe-deferral angle, whilst it's useful to have uncovered this
>bug, I don't think we should actually be calling remove_device() from
>DMA teardown. I think it's preferable from a user perspective if group
>numbering remains stable, rather than changing depending on the order in
>which they unbind/rebind VFIO drivers. I'm really keen to try and get
>this in shape for 4.10, so I've taken the liberty of hacking up my own
>branch (iommu/defer) based on v3 - would you mind taking a look at the
>two "iommu/of:" commits to see what you think? (Ignore the PCI changes
>to your later patches - that was an experiment which didn't really work out)

Ok, will take a look at this now and respond more on this.

>
>> Signed-off-by: Sricharan R 
>> ---
>>  drivers/iommu/arm-smmu.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 71ce4b6..a1d0b3c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1516,8 +1516,10 @@ static struct iommu_group 
>> *arm_smmu_device_group(struct device *dev)
>>  group = smmu->s2crs[idx].group;
>>  }
>>
>> -if (group)
>> +if (group) {
>> +iommu_group_get_by_id(iommu_group_id(group));
>>  return group;
>
>This might as well just be inline, i.e.:
>
>   return iommu_group_get_by_id(iommu_group_id(group));
>
>It's a shame we have to go all round the houses when we have the group
>right there, but this is probably the most expedient fix. I guess we can
>extend the API with some sort of iommu_group_get(group) overload in
>future if we really want to.
>

ok, i can send this fix separately then. Otherwise, Will was suggesting on the
other thread that there should probably be a separate API to increment
the group refcount or get the group from the existing aliasing device.
As per me adding the api, looks like another option or post the above ?

Regards,
 Sricharan

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-07 Thread Will Deacon

On Fri, Nov 04, 2016 at 08:46:06PM +0530, Sricharan wrote:
> >>>Yikes, on second look, that definitely shouldn't be happening.
> >>>Everything below is probably the resulting fallout.
> >>
> >>[   40.206703] vfio-pci :08:00.0: Failed to setup iommu ops
> >>
> >>I think the above print which says "failed to setup iommu_ops"
> >>because the call ops->add_device failed in of_pci_iommu_configure
> >>is the reason for the failure, in my case i simply do not get this even with
> >>your scripts. ops->add_device succeeds in the rebind as well. So still
> >>checking what could be happening in your case.
> >
> >I was looking at your code base from [1].The ops->add_device
> >callback from of_pci_iommu_configure on the rebind is the
> >one which is causing the failure. But not able to spot out
> >from code which point is causing the failure. It would be very helpful
> >if i can know which is the return value from the add_device callback
> >or point inside add_device callback which fails in your setup.
> >
> >
> >[1] git://linux-arm.org/linux-rm iommu/misc

So this also applies to mainline.

> With little more try, i saw an issue where i had an failure
> similar to what you reported. The issue happens when multiple
> devices fall in to same group due to matching sids. I ended up
> doing a fix like below and it would be nice to verify if it is the same
> that we are seeing in your setup and if the fix makes a difference ?
> 
> From: Sricharan R 
> Date: Fri, 4 Nov 2016 20:28:49 +0530
> Subject: [PATCH] iommu/arm-smmu: Fix group's reference counting
> 
> iommu_group_get_for_dev which gets called in the add_device
> callback, increases the reference count of the iommu_group,
> so we do an iommu_group_put after that. iommu_group_get_for_dev
> inturn calls device_group callback and in the case of arm-smmu
> we call generic_device_group/pci_device_group which takes
> care of increasing the group's reference. But when we return
> an already existing group(when multiple devices have same group)
> the reference is not incremented, resulting in issues when the
> remove_device callback for the devices is invoked.
> Fixing the same here.
> 
> Signed-off-by: Sricharan R 
> ---
>  drivers/iommu/arm-smmu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 71ce4b6..a1d0b3c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1516,8 +1516,10 @@ static struct iommu_group 
> *arm_smmu_device_group(struct device *dev)
>   group = smmu->s2crs[idx].group;
>   }
>  
> - if (group)
> + if (group) {
> + iommu_group_get_by_id(iommu_group_id(group));
>   return group;
> + }

This is foul :(

I think we should either add a function for incrementing the refcount on a
group, or we should get a handle on the existing aliasing device and get
the group from that. As written, this patch is far too subtle.

Joerg -- do you have any preference?

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-04 Thread Sricharan

Hi Robin,

>>>Yikes, on second look, that definitely shouldn't be happening.
>>>Everything below is probably the resulting fallout.
>>
>>[   40.206703] vfio-pci :08:00.0: Failed to setup iommu ops
>>
>>I think the above print which says "failed to setup iommu_ops"
>>because the call ops->add_device failed in of_pci_iommu_configure
>>is the reason for the failure, in my case i simply do not get this even with
>>your scripts. ops->add_device succeeds in the rebind as well. So still
>>checking what could be happening in your case.
>
>I was looking at your code base from [1].The ops->add_device
>callback from of_pci_iommu_configure on the rebind is the
>one which is causing the failure. But not able to spot out
>from code which point is causing the failure. It would be very helpful
>if i can know which is the return value from the add_device callback
>or point inside add_device callback which fails in your setup.
>
>
>[1] git://linux-arm.org/linux-rm iommu/misc

With little more try, i saw an issue where i had an failure
similar to what you reported. The issue happens when multiple
devices fall in to same group due to matching sids. I ended up
doing a fix like below and it would be nice to verify if it is the same
that we are seeing in your setup and if the fix makes a difference ?

From: Sricharan R 
Date: Fri, 4 Nov 2016 20:28:49 +0530
Subject: [PATCH] iommu/arm-smmu: Fix group's reference counting

iommu_group_get_for_dev which gets called in the add_device
callback, increases the reference count of the iommu_group,
so we do an iommu_group_put after that. iommu_group_get_for_dev
inturn calls device_group callback and in the case of arm-smmu
we call generic_device_group/pci_device_group which takes
care of increasing the group's reference. But when we return
an already existing group(when multiple devices have same group)
the reference is not incremented, resulting in issues when the
remove_device callback for the devices is invoked.
Fixing the same here.

Signed-off-by: Sricharan R 
---
 drivers/iommu/arm-smmu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 71ce4b6..a1d0b3c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1516,8 +1516,10 @@ static struct iommu_group *arm_smmu_device_group(struct 
device *dev)
group = smmu->s2crs[idx].group;
}
 
-   if (group)
+   if (group) {
+   iommu_group_get_by_id(iommu_group_id(group));
return group;
+   }
 
if (dev_is_pci(dev))
group = pci_device_group(dev);
-- 
1.8.2.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-11-03 Thread Sricharan

Hi Robin,

 [   39.901592] iommu: Removing device :08:00.0 from group 0
>>
>>Yikes, on second look, that definitely shouldn't be happening.
>>Everything below is probably the resulting fallout.
>
>[   40.206703] vfio-pci :08:00.0: Failed to setup iommu ops
>
>I think the above print which says "failed to setup iommu_ops"
>because the call ops->add_device failed in of_pci_iommu_configure
>is the reason for the failure, in my case i simply do not get this even with
>your scripts. ops->add_device succeeds in the rebind as well. So still
>checking what could be happening in your case.

I was looking at your code base from [1].The ops->add_device
callback from of_pci_iommu_configure on the rebind is the
one which is causing the failure. But not able to spot out
from code which point is causing the failure. It would be very helpful
if i can know which is the return value from the add_device callback
or point inside add_device callback which fails in your setup.


[1] git://linux-arm.org/linux-rm iommu/misc
>
>
>Regards,
>  Sricharan
>

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-27 Thread Sricharan

Hi Robin,

>>
>>
>>
>>> [   39.901592] iommu: Removing device :08:00.0 from group 0
>
>Yikes, on second look, that definitely shouldn't be happening.
>Everything below is probably the resulting fallout.

[   40.206703] vfio-pci :08:00.0: Failed to setup iommu ops

I think the above print which says "failed to setup iommu_ops"
because the call ops->add_device failed in of_pci_iommu_configure
is the reason for the failure, in my case i simply do not get this even with
your scripts. ops->add_device succeeds in the rebind as well. So still
checking what could be happening in your case.


Regards,
  Sricharan


>
>Robin.
>
>>> [   39.907383] [ cut here ]
>>> [   39.911969] WARNING: CPU: 0 PID: 174 at
>>> arch/arm64/mm/dma-mapping.c:856 arch_teardown_dma_ops+0x48/0x68
>>> [   39.921266] Modules linked in:
>>> [   39.924290]
>>> [   39.925766] CPU: 0 PID: 174 Comm: vfio Not tainted 4.9.0-rc2+ #1249
>>> [   39.931967] Hardware name: ARM Juno development board (r1) (DT)
>>> [   39.937826] task: ffc975ee9900 task.stack: ffc974d6
>>> [   39.943687] PC is at arch_teardown_dma_ops+0x48/0x68
>>> [   39.948603] LR is at arch_teardown_dma_ops+0x34/0x68
>>> [   39.953516] pc : [] lr : []
>>> pstate: 6145
>>> [   39.960834] sp : ffc974d63ca0
>>> [   39.964112] x29: ffc974d63ca0 x28: ffc974d6
>>> [   39.969377] x27: ff80088a2000 x26: 0040
>>> [   39.974642] x25: 0123 x24: ffc976a48918
>>> [   39.979907] x23: ffc974d63eb8 x22: ff8008db7550
>>> [   39.985171] x21: 000d x20: ffc9763e9b50
>>> [   39.990435] x19: ffc9763f20a0 x18: 0010
>>> [   39.995699] x17: 007f99c18018 x16: ff80080c0580
>>> [   40.000964] x15: ff8008bb7000 x14: 000137c100013798
>>> [   40.006228] x13: ff00 x12: 
>>> [   40.011492] x11: 0018 x10: 000d
>>> [   40.016757] x9 : 4000 x8 : 00210d00
>>> [   40.022021] x7 : 0049771bc000 x6 : 001f17ed
>>> [   40.027286] x5 : ff80084c4208 x4 : 0080
>>> [   40.032551] x3 : ffc975ea9800 x2 : ffbf25d7aa50
>>> [   40.037815] x1 :  x0 : 0080
>>> [   40.043078]
>>> [   40.044549] ---[ end trace 35c1e743d6e6c035 ]---
>>> [   40.049117] Call trace:
>>> [   40.051537] Exception stack(0xffc974d63ad0 to 0xffc974d63c00)
>>> [   40.057914] 3ac0:   ffc9763f20a0
>>> 0080
>>> [   40.065668] 3ae0: ffc974d63ca0 ff80080948f8 ffbf25d7aa40
>>> ffc975ea9800
>>> [   40.073421] 3b00: ffc974d6 0002fc80 ffc976801e00
>>> ffc974d6
>>> [   40.081175] 3b20: ff80084c4208 0040 ff80088a2000
>>> ffc974d6
>>> [   40.088928] 3b40: ff80084caf78 ffc974d6 ffc974d6
>>> ffc974d6
>>> [   40.096682] 3b60: ffc975ea9800 ffc974d6 0080
>>> 
>>> [   40.104435] 3b80: ffbf25d7aa50 ffc975ea9800 0080
>>> ff80084c4208
>>> [   40.112188] 3ba0: 001f17ed 0049771bc000 00210d00
>>> 4000
>>> [   40.119941] 3bc0: 000d 0018 
>>> ff00
>>> [   40.127695] 3be0: 000137c100013798 ff8008bb7000 ff80080c0580
>>> 007f99c18018
>>> [   40.135450] [] arch_teardown_dma_ops+0x48/0x68
>>> [   40.141400] [] of_dma_deconfigure+0xc/0x18
>>> [   40.147005] [] dma_deconfigure+0xc/0x18
>>> [   40.152353] [] __device_release_driver+0x88/0x120
>>> [   40.158560] [] device_release_driver+0x24/0x38
>>> [   40.164507] [] unbind_store+0xe8/0x110
>>> [   40.169767] [] drv_attr_store+0x20/0x30
>>> [   40.175113] [] sysfs_kf_write+0x48/0x58
>>> [   40.180458] [] kernfs_fop_write+0xb0/0x1d8
>>> [   40.186063] [] __vfs_write+0x1c/0x100
>>> [   40.191237] [] vfs_write+0xa0/0x1b8
>>> [   40.196239] [] SyS_write+0x44/0xa0
>>> [   40.201155] [] el0_svc_naked+0x24/0x28
>>> [   40.206703] vfio-pci :08:00.0: Failed to setup iommu ops
>>> [   40.212382] vfio-pci: probe of :08:00.0 failed with error -22
>>> [   40.228075] [ cut here ]
>>> [   40.235263] WARNING: CPU: 1 PID: 174 at ./include/linux/kref.h:46
>>> kobject_get+0x64/0x88
>>> [   40.243181] Modules linked in:
>>> [   40.246201]
>>> [   40.247673] CPU: 1 PID: 174 Comm: vfio Tainted: GW
>>> 4.9.0-rc2+ #1249
>>> [   40.255076] Hardware name: ARM Juno development board (r1) (DT)
>>> [   40.260932] task: ffc975ee9900 task.stack: ffc974d6
>>> [   40.266787] PC is at kobject_get+0x64/0x88
>>> [   40.270840] LR is at iommu_bus_notifier+0x40/0x110
>>> [   40.275577] pc : [] lr : []
>>> pstate: 8145
>>> [   40.282894] sp : ffc974d63c00
>>> [   40.286169] x29: ffc974d63c00 x28: ffc974d6
>>> [   40.291431] x27: ff80088a2000 x26: 0040
>>> [   40.296692] x25: 0123 x24:

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-26 Thread Robin Murphy

On 26/10/16 15:44, Sricharan wrote:
> Hi Robin,
> 
>> On 04/10/16 18:03, Sricharan R wrote:
>>> Initial post from Laurent Pinchart[1]. This is
>>> series calls the dma ops configuration for the devices
>>> at a generic place so that it works for all busses.
>>> The dma_configure_ops for a device is now called during
>>> the device_attach callback just before the probe of the
>>> bus/driver is called. Similarly dma_deconfigure is called during
>>> device/driver_detach path.
>>>
>>>
>>> pci_bus_add_devices(platform/amba)(_device_create/driver_register)
>>>| |
>>> pci_bus_add_device (device_add/driver_register)
>>>| |
>>> device_attach   device_initial_probe
>>>| |
>>> __device_attach_driver__device_attach_driver
>>>|
>>> driver_probe_device
>>>|
>>> really_probe
>>>|
>>> dma_configure
>>>
>>>  Similarly on the device/driver_unregister path __device_release_driver is
>>>  called which inturn calls dma_deconfigure.
>>>
>>>  If the ACPI bus code follows the same, we can add acpi_dma_configure
>>>  at the same place as of_dma_configure.
>>>
>>>  This series is based on the recently merged Generic DT bindings for
>>>  PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]
>>>
>>>  This time tested this with platform and pci device for probe deferral
>>>  and reprobe on arm64 based platform. There is an issue on the cleanup
>>>  path for arm64 though, where there is WARN_ON if the dma_ops is reset while
>>>  device is attached to an domain in arch_teardown_dma_ops.
>>>  But with iommu_groups created from the iommu driver, the device is always
>>>  attached to a domain/default_domain. So so the WARN has to be 
>>> removed/handled
>>>  probably.
>>
>> I've finally got the chance to take a proper look at this series (sorry
>> for the delay). First up, with these patches on top of 4.9-rc2, my
>> little script for unbinding some PCI devices and rebinding them to VFIO
>> now goes horribly, horribly wrong.
>>
>> Robin.
>>
> 
>Thanks for looking in to this.
> I was trying to reproduce the below with a command like this in my setup.
> 
> echo 0002\:00\:00.0 >  
> /sys/bus/pci/devices/0002\:00\:000.0/driver/unbind0.0/driver/unbind
> echo 0x17cb 0x0104 > /sys/bus/pci/drivers/vfio-pci/new_id
> 
> But for me the unbind and reconfiguring/adding the iommus_ops to vfio-pci did
> succeed, although the WARN_ON in arch_teardown_dma_ops was there, that

Oh, yes, I hacked that out already to cut the noise down.

> could be suppresed.  The vfio_pci_probe was not going through because
>  the pci hdr_type was not PCI_HEADER_TYPE_NORMAL.
>  But anyways iommu unbind/rebind seemed to be going through.
> 
> If i can get what your script is doing, i can try that and see what happens.

---8<---
#!/bin/sh

#Juno Sky2, SATA
DEVICES=':08:00.0 :03:00.0'

for DEV in $DEVICES
do
BUSDEV=/sys/bus/pci/devices/$DEV
GROUP=$(basename $(readlink $BUSDEV/iommu_group))
DRV=$(readlink -f $BUSDEV/driver)
read VID < $BUSDEV/vendor
read DID < $BUSDEV/device

echo $DEV > $BUSDEV/driver/unbind
echo $VID $DID > /sys/bus/pci/drivers/vfio-pci/new_id
done
# it would then goes on to launch kvmtool and rebind the original
# drivers afterwards, but that doesn't matter here
--->8---

The segfault doesn't always happen, but the kref warnings and the
vfio-pci driver failing to probe certainly do.

> 
> Regards,
>   Sricharan
> 
> 
> 
>> [   39.901592] iommu: Removing device :08:00.0 from group 0

Yikes, on second look, that definitely shouldn't be happening.
Everything below is probably the resulting fallout.

Robin.

>> [   39.907383] [ cut here ]
>> [   39.911969] WARNING: CPU: 0 PID: 174 at
>> arch/arm64/mm/dma-mapping.c:856 arch_teardown_dma_ops+0x48/0x68
>> [   39.921266] Modules linked in:
>> [   39.924290]
>> [   39.925766] CPU: 0 PID: 174 Comm: vfio Not tainted 4.9.0-rc2+ #1249
>> [   39.931967] Hardware name: ARM Juno development board (r1) (DT)
>> [   39.937826] task: ffc975ee9900 task.stack: ffc974d6
>> [   39.943687] PC is at arch_teardown_dma_ops+0x48/0x68
>> [   39.948603] LR is at arch_teardown_dma_ops+0x34/0x68
>> [   39.953516] pc : [] lr : []
>> pstate: 6145
>> [   39.960834] sp : ffc974d63ca0
>> [   39.964112] x29: ffc974d63ca0 x28: ffc974d6
>> [   39.969377] x27: ff80088a2000 x26: 0040
>> [   39.974642] x25: 0123 x24: ffc976a48918
>> [   39.979907] x23: ffc974d63eb8 x22: ff8008db7550
>> [   39.985171] x21: 000d x20: ffc9763e9b50
>> [   39.990435] x19: ffc9763f20a0 x18: 0010
>> [   39.995699] x17: 007f99c18018 x16: ff80080c0580
>> [   40.000964] x15: ff8008bb7000 x14: 000137c100013798
>> [   40.006228] x13: ff00 x12: 
>> [   40.011492] x11: 0018 x10:

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-26 Thread Sricharan

Hi Robin,

>On 04/10/16 18:03, Sricharan R wrote:
>> Initial post from Laurent Pinchart[1]. This is
>> series calls the dma ops configuration for the devices
>> at a generic place so that it works for all busses.
>> The dma_configure_ops for a device is now called during
>> the device_attach callback just before the probe of the
>> bus/driver is called. Similarly dma_deconfigure is called during
>> device/driver_detach path.
>>
>>
>> pci_bus_add_devices(platform/amba)(_device_create/driver_register)
>>| |
>> pci_bus_add_device (device_add/driver_register)
>>| |
>> device_attach   device_initial_probe
>>| |
>> __device_attach_driver__device_attach_driver
>>|
>> driver_probe_device
>>|
>> really_probe
>>|
>> dma_configure
>>
>>  Similarly on the device/driver_unregister path __device_release_driver is
>>  called which inturn calls dma_deconfigure.
>>
>>  If the ACPI bus code follows the same, we can add acpi_dma_configure
>>  at the same place as of_dma_configure.
>>
>>  This series is based on the recently merged Generic DT bindings for
>>  PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]
>>
>>  This time tested this with platform and pci device for probe deferral
>>  and reprobe on arm64 based platform. There is an issue on the cleanup
>>  path for arm64 though, where there is WARN_ON if the dma_ops is reset while
>>  device is attached to an domain in arch_teardown_dma_ops.
>>  But with iommu_groups created from the iommu driver, the device is always
>>  attached to a domain/default_domain. So so the WARN has to be 
>> removed/handled
>>  probably.
>
>I've finally got the chance to take a proper look at this series (sorry
>for the delay). First up, with these patches on top of 4.9-rc2, my
>little script for unbinding some PCI devices and rebinding them to VFIO
>now goes horribly, horribly wrong.
>
>Robin.
>

   Thanks for looking in to this.
I was trying to reproduce the below with a command like this in my setup.

echo 0002\:00\:00.0 >  
/sys/bus/pci/devices/0002\:00\:000.0/driver/unbind0.0/driver/unbind
echo 0x17cb 0x0104 > /sys/bus/pci/drivers/vfio-pci/new_id

But for me the unbind and reconfiguring/adding the iommus_ops to vfio-pci did
succeed, although the WARN_ON in arch_teardown_dma_ops was there, that
could be suppresed.  The vfio_pci_probe was not going through because
 the pci hdr_type was not PCI_HEADER_TYPE_NORMAL.
 But anyways iommu unbind/rebind seemed to be going through.

If i can get what your script is doing, i can try that and see what happens.

Regards,
  Sricharan



>[   39.901592] iommu: Removing device :08:00.0 from group 0
>[   39.907383] [ cut here ]
>[   39.911969] WARNING: CPU: 0 PID: 174 at
>arch/arm64/mm/dma-mapping.c:856 arch_teardown_dma_ops+0x48/0x68
>[   39.921266] Modules linked in:
>[   39.924290]
>[   39.925766] CPU: 0 PID: 174 Comm: vfio Not tainted 4.9.0-rc2+ #1249
>[   39.931967] Hardware name: ARM Juno development board (r1) (DT)
>[   39.937826] task: ffc975ee9900 task.stack: ffc974d6
>[   39.943687] PC is at arch_teardown_dma_ops+0x48/0x68
>[   39.948603] LR is at arch_teardown_dma_ops+0x34/0x68
>[   39.953516] pc : [] lr : []
>pstate: 6145
>[   39.960834] sp : ffc974d63ca0
>[   39.964112] x29: ffc974d63ca0 x28: ffc974d6
>[   39.969377] x27: ff80088a2000 x26: 0040
>[   39.974642] x25: 0123 x24: ffc976a48918
>[   39.979907] x23: ffc974d63eb8 x22: ff8008db7550
>[   39.985171] x21: 000d x20: ffc9763e9b50
>[   39.990435] x19: ffc9763f20a0 x18: 0010
>[   39.995699] x17: 007f99c18018 x16: ff80080c0580
>[   40.000964] x15: ff8008bb7000 x14: 000137c100013798
>[   40.006228] x13: ff00 x12: 
>[   40.011492] x11: 0018 x10: 000d
>[   40.016757] x9 : 4000 x8 : 00210d00
>[   40.022021] x7 : 0049771bc000 x6 : 001f17ed
>[   40.027286] x5 : ff80084c4208 x4 : 0080
>[   40.032551] x3 : ffc975ea9800 x2 : ffbf25d7aa50
>[   40.037815] x1 :  x0 : 0080
>[   40.043078]
>[   40.044549] ---[ end trace 35c1e743d6e6c035 ]---
>[   40.049117] Call trace:
>[   40.051537] Exception stack(0xffc974d63ad0 to 0xffc974d63c00)
>[   40.057914] 3ac0:   ffc9763f20a0
>0080
>[   40.065668] 3ae0: ffc974d63ca0 ff80080948f8 ffbf25d7aa40
>ffc975ea9800
>[   40.073421] 3b00: ffc974d6 0002fc80 ffc976801e00
>ffc974d6
>[   40.081175] 3b20: ff80084c4208 0040 ff80088a2000
>ffc974d6
>[   40.088928] 3b40: ff80084caf78 ffc974d6 ffc974d6
>ffc974d6
>[   40.096682] 3b60: ffc975ea9800 ffc974d6 0080

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-25 Thread Robin Murphy

Hi Sricharan,

On 04/10/16 18:03, Sricharan R wrote:
> Initial post from Laurent Pinchart[1]. This is
> series calls the dma ops configuration for the devices
> at a generic place so that it works for all busses.
> The dma_configure_ops for a device is now called during
> the device_attach callback just before the probe of the
> bus/driver is called. Similarly dma_deconfigure is called during
> device/driver_detach path.
> 
> 
> pci_bus_add_devices(platform/amba)(_device_create/driver_register)
>| |
> pci_bus_add_device (device_add/driver_register)
>| |
> device_attach   device_initial_probe
>| |
> __device_attach_driver__device_attach_driver
>|
> driver_probe_device
>|
> really_probe
>|
> dma_configure
> 
>  Similarly on the device/driver_unregister path __device_release_driver is
>  called which inturn calls dma_deconfigure.
> 
>  If the ACPI bus code follows the same, we can add acpi_dma_configure
>  at the same place as of_dma_configure.
> 
>  This series is based on the recently merged Generic DT bindings for
>  PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]
> 
>  This time tested this with platform and pci device for probe deferral
>  and reprobe on arm64 based platform. There is an issue on the cleanup
>  path for arm64 though, where there is WARN_ON if the dma_ops is reset while
>  device is attached to an domain in arch_teardown_dma_ops.
>  But with iommu_groups created from the iommu driver, the device is always
>  attached to a domain/default_domain. So so the WARN has to be removed/handled
>  probably.

I've finally got the chance to take a proper look at this series (sorry
for the delay). First up, with these patches on top of 4.9-rc2, my
little script for unbinding some PCI devices and rebinding them to VFIO
now goes horribly, horribly wrong.

Robin.

[   39.901592] iommu: Removing device :08:00.0 from group 0
[   39.907383] [ cut here ]
[   39.911969] WARNING: CPU: 0 PID: 174 at
arch/arm64/mm/dma-mapping.c:856 arch_teardown_dma_ops+0x48/0x68
[   39.921266] Modules linked in:
[   39.924290]
[   39.925766] CPU: 0 PID: 174 Comm: vfio Not tainted 4.9.0-rc2+ #1249
[   39.931967] Hardware name: ARM Juno development board (r1) (DT)
[   39.937826] task: ffc975ee9900 task.stack: ffc974d6
[   39.943687] PC is at arch_teardown_dma_ops+0x48/0x68
[   39.948603] LR is at arch_teardown_dma_ops+0x34/0x68
[   39.953516] pc : [] lr : []
pstate: 6145
[   39.960834] sp : ffc974d63ca0
[   39.964112] x29: ffc974d63ca0 x28: ffc974d6
[   39.969377] x27: ff80088a2000 x26: 0040
[   39.974642] x25: 0123 x24: ffc976a48918
[   39.979907] x23: ffc974d63eb8 x22: ff8008db7550
[   39.985171] x21: 000d x20: ffc9763e9b50
[   39.990435] x19: ffc9763f20a0 x18: 0010
[   39.995699] x17: 007f99c18018 x16: ff80080c0580
[   40.000964] x15: ff8008bb7000 x14: 000137c100013798
[   40.006228] x13: ff00 x12: 
[   40.011492] x11: 0018 x10: 000d
[   40.016757] x9 : 4000 x8 : 00210d00
[   40.022021] x7 : 0049771bc000 x6 : 001f17ed
[   40.027286] x5 : ff80084c4208 x4 : 0080
[   40.032551] x3 : ffc975ea9800 x2 : ffbf25d7aa50
[   40.037815] x1 :  x0 : 0080
[   40.043078]
[   40.044549] ---[ end trace 35c1e743d6e6c035 ]---
[   40.049117] Call trace:
[   40.051537] Exception stack(0xffc974d63ad0 to 0xffc974d63c00)
[   40.057914] 3ac0:   ffc9763f20a0
0080
[   40.065668] 3ae0: ffc974d63ca0 ff80080948f8 ffbf25d7aa40
ffc975ea9800
[   40.073421] 3b00: ffc974d6 0002fc80 ffc976801e00
ffc974d6
[   40.081175] 3b20: ff80084c4208 0040 ff80088a2000
ffc974d6
[   40.088928] 3b40: ff80084caf78 ffc974d6 ffc974d6
ffc974d6
[   40.096682] 3b60: ffc975ea9800 ffc974d6 0080

[   40.104435] 3b80: ffbf25d7aa50 ffc975ea9800 0080
ff80084c4208
[   40.112188] 3ba0: 001f17ed 0049771bc000 00210d00
4000
[   40.119941] 3bc0: 000d 0018 
ff00
[   40.127695] 3be0: 000137c100013798 ff8008bb7000 ff80080c0580
007f99c18018
[   40.135450] [] arch_teardown_dma_ops+0x48/0x68
[   40.141400] [] of_dma_deconfigure+0xc/0x18
[   40.147005] [] dma_deconfigure+0xc/0x18
[   40.152353] [] __device_release_driver+0x88/0x120
[   40.158560] [] device_release_driver+0x24/0x38
[   40.164507] [] unbind_store+0xe8/0x110
[   40.169767] [] drv_attr_store+0x20/0x30
[   40.175113] [] sysfs_kf_write+0x48/0x58
[   40.180458] [] kernfs_fop_write+0xb0/0x1d8
[

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-25 Thread Archit Taneja




On 10/04/2016 10:33 PM, Sricharan R wrote:

Initial post from Laurent Pinchart[1]. This is
series calls the dma ops configuration for the devices
at a generic place so that it works for all busses.
The dma_configure_ops for a device is now called during
the device_attach callback just before the probe of the
bus/driver is called. Similarly dma_deconfigure is called during
device/driver_detach path.


pci_bus_add_devices(platform/amba)(_device_create/driver_register)
   | |
pci_bus_add_device (device_add/driver_register)
   | |
device_attach   device_initial_probe
   | |
__device_attach_driver__device_attach_driver
   |
driver_probe_device
   |
really_probe
   |
dma_configure

 Similarly on the device/driver_unregister path __device_release_driver is
 called which inturn calls dma_deconfigure.

 If the ACPI bus code follows the same, we can add acpi_dma_configure
 at the same place as of_dma_configure.

 This series is based on the recently merged Generic DT bindings for
 PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]

 This time tested this with platform and pci device for probe deferral
 and reprobe on arm64 based platform. There is an issue on the cleanup
 path for arm64 though, where there is WARN_ON if the dma_ops is reset while
 device is attached to an domain in arch_teardown_dma_ops.
 But with iommu_groups created from the iommu driver, the device is always
 attached to a domain/default_domain. So so the WARN has to be removed/handled
 probably.



Tested-by: Archit Taneja 


 Previous post of this series [3].

 [V3]
 * Removed the patch to split dma_masks/dma_ops configuration separately
   based on review comments that both masks and ops are required only
   during the device probe time.

 * Reworked the series based on Generic DT bindings series [2].

 * Added call to iommu's remove_device in the cleanup path for arm and 
arm64.

 * Removed the notifier trick in arm64 to handle early device registration.

 * Added reset of dma_ops in cleanup path for arm based on comments.

 * Fixed the pci_iommu_configure path and tested with PCI device as well.

 * Fixed a bug to return the correct iommu_ops from patch 7 [4] in last 
post.

 * Fixed few other cosmetic comments.

 [V2]
 * Updated the Initial post to call dma_configure/deconfigure from generic 
code

 * Added iommu add_device callback from of_iommu_configure path

 [V1]
 * Initial post

[1] http://lists.linuxfoundation.org/pipermail/iommu/2015-May/013016.html
[2] http://www.spinics.net/lists/devicetree/msg142943.html
[3] https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg13941.html
[4] https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg13940.html



Laurent Pinchart (4):
  arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
  of: dma: Move range size workaround to of_dma_get_range()
  of: dma: Make of_dma_deconfigure() public
  iommu: of: Handle IOMMU lookup failure with deferred probing or error

Sricharan R (4):
  drivers: platform: Configure dma operations at probe time
  arm: dma-mapping: Reset the device's dma_ops
  arm/arm64: dma-mapping: Call iommu's remove_device callback during
device detach
  arm64: dma-mapping: Remove the notifier trick to handle early setting
of dma_ops

 arch/arm/mm/dma-mapping.c   |  18 
 arch/arm64/mm/dma-mapping.c | 107 +---
 drivers/base/dd.c   |  10 +
 drivers/base/dma-mapping.c  |  11 +
 drivers/iommu/of_iommu.c|  47 +--
 drivers/of/address.c|  20 -
 drivers/of/device.c |  34 +++---
 drivers/of/platform.c   |   9 
 drivers/pci/probe.c |   5 +--
 include/linux/dma-mapping.h |   3 ++
 include/linux/of_device.h   |   7 ++-
 11 files changed, 138 insertions(+), 133 deletions(-)



--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-24 Thread Sricharan

Hi Marek,

 Initial post from Laurent Pinchart[1]. This is
 series calls the dma ops configuration for the devices
 at a generic place so that it works for all busses.
 The dma_configure_ops for a device is now called during
 the device_attach callback just before the probe of the
 bus/driver is called. Similarly dma_deconfigure is called during
 device/driver_detach path.


 pci_bus_add_devices(platform/amba)(_device_create/driver_register)
  | |
 pci_bus_add_device (device_add/driver_register)
  | |
 device_attach   device_initial_probe
  | |
 __device_attach_driver__device_attach_driver
  |
 driver_probe_device
  |
 really_probe
  |
 dma_configure

Similarly on the device/driver_unregister path __device_release_driver 
 is
called which inturn calls dma_deconfigure.

If the ACPI bus code follows the same, we can add acpi_dma_configure
at the same place as of_dma_configure.

This series is based on the recently merged Generic DT bindings for
PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]

This time tested this with platform and pci device for probe deferral
and reprobe on arm64 based platform. There is an issue on the cleanup
path for arm64 though, where there is WARN_ON if the dma_ops is reset 
 while
device is attached to an domain in arch_teardown_dma_ops.
But with iommu_groups created from the iommu driver, the device is 
 always
attached to a domain/default_domain. So so the WARN has to be 
 removed/handled
probably.
>>> Thanks for continuing work on this feature! Your can add my:
>>>
>>> Tested-by: Marek Szyprowski 
>>>
>>Thanks for testing this. So the for the below fix, the remove_device 
>> callback
>>gets called on the dma_ops cleanup path, so would it be easy to remove the
>>data for the device there ?
>
>I assumed that IOMMU driver cannot be removed reliably, so all
>structures that it
>creates are permanent. I didn't use device_add()/device_remove()
>callbacks, because
>in current implementation device_add() is called too late (after
>dma-mapping glue
>triggers device_attach_iommu()).
>
>Maybe once your patchset is merged, I will move creation and management
>of the all
>IOMMU related structures to device_add/remove callbacks.
>
ok understand it.

Regards,
Sricharan

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-24 Thread Marek Szyprowski


Hi Sricharan,


On 2016-10-12 08:24, Sricharan wrote:

On 2016-10-04 19:03, Sricharan R wrote:

Initial post from Laurent Pinchart[1]. This is
series calls the dma ops configuration for the devices
at a generic place so that it works for all busses.
The dma_configure_ops for a device is now called during
the device_attach callback just before the probe of the
bus/driver is called. Similarly dma_deconfigure is called during
device/driver_detach path.


pci_bus_add_devices(platform/amba)(_device_create/driver_register)
 | |
pci_bus_add_device (device_add/driver_register)
 | |
device_attach   device_initial_probe
 | |
__device_attach_driver__device_attach_driver
 |
driver_probe_device
 |
really_probe
 |
dma_configure

   Similarly on the device/driver_unregister path __device_release_driver is
   called which inturn calls dma_deconfigure.

   If the ACPI bus code follows the same, we can add acpi_dma_configure
   at the same place as of_dma_configure.

   This series is based on the recently merged Generic DT bindings for
   PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]

   This time tested this with platform and pci device for probe deferral
   and reprobe on arm64 based platform. There is an issue on the cleanup
   path for arm64 though, where there is WARN_ON if the dma_ops is reset while
   device is attached to an domain in arch_teardown_dma_ops.
   But with iommu_groups created from the iommu driver, the device is always
   attached to a domain/default_domain. So so the WARN has to be removed/handled
   probably.

Thanks for continuing work on this feature! Your can add my:

Tested-by: Marek Szyprowski 


   Thanks for testing this. So the for the below fix, the remove_device callback
   gets called on the dma_ops cleanup path, so would it be easy to remove the
   data for the device there ?


I assumed that IOMMU driver cannot be removed reliably, so all 
structures that it
creates are permanent. I didn't use device_add()/device_remove() 
callbacks, because
in current implementation device_add() is called too late (after 
dma-mapping glue

triggers device_attach_iommu()).

Maybe once your patchset is merged, I will move creation and management 
of the all

IOMMU related structures to device_add/remove callbacks.

Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-17 Thread Sricharan

Resending, missed out on the link last time.

>-Original Message-
>From: linux-arm-msm-ow...@vger.kernel.org 
>[mailto:linux-arm-msm-ow...@vger.kernel.org] On Behalf Of Marek Szyprowski
>Sent: Monday, October 10, 2016 6:07 PM
>To: Sricharan R <sricha...@codeaurora.org>; will.dea...@arm.com; 
>robin.mur...@arm.com; j...@8bytes.org; iommu@lists.linux-
>foundation.org; linux-arm-ker...@lists.infradead.org; 
>linux-arm-...@vger.kernel.org; laurent.pinch...@ideasonboard.com;
>tf...@chromium.org; srinivas.kandaga...@linaro.org
>Subject: Re: [PATCH V3 0/8] IOMMU probe deferral support
>
>Hi Sricharan,
>
>
>On 2016-10-04 19:03, Sricharan R wrote:
>> Initial post from Laurent Pinchart[1]. This is
>> series calls the dma ops configuration for the devices
>> at a generic place so that it works for all busses.
>> The dma_configure_ops for a device is now called during
>> the device_attach callback just before the probe of the
>> bus/driver is called. Similarly dma_deconfigure is called during
>> device/driver_detach path.
>>
>>
>> pci_bus_add_devices(platform/amba)(_device_create/driver_register)
>> | |
>> pci_bus_add_device (device_add/driver_register)
>> | |
>> device_attach   device_initial_probe
>> | |
>> __device_attach_driver__device_attach_driver
>> |
>> driver_probe_device
>> |
>> really_probe
>> |
>> dma_configure
>>
>>   Similarly on the device/driver_unregister path __device_release_driver
>> is
>>   called which inturn calls dma_deconfigure.
>>
>>   If the ACPI bus code follows the same, we can add acpi_dma_configure
>>   at the same place as of_dma_configure.
>>
>>   This series is based on the recently merged Generic DT bindings for
>>   PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]
>>
>>   This time tested this with platform and pci device for probe deferral
>>   and reprobe on arm64 based platform. There is an issue on the cleanup
>>   path for arm64 though, where there is WARN_ON if the dma_ops is reset
>> while
>>   device is attached to an domain in arch_teardown_dma_ops.
>>   But with iommu_groups created from the iommu driver, the device is
>> always
>>   attached to a domain/default_domain. So so the WARN has to be
>> removed/handled
>>   probably.
>
>Thanks for continuing work on this feature! Your can add my:
>
>Tested-by: Marek Szyprowski <m.szyprow...@samsung.com>
>

Hi Will,Robin,Joerg,
   
   I have tested the probe deferral for platform/pcie bus devices based on 
latest Generic DT bindings
series merged [1], for pci/arm-smmu.
   It will be good to know from you on whats the right way to take this 
forward ?

[1] http://www.spinics.net/lists/devicetree/msg142943.html

Regards,
 Sricharan

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-17 Thread Sricharan

>-Original Message-
>From: Sricharan [mailto:sricha...@codeaurora.org]
>Sent: Wednesday, October 12, 2016 11:55 AM
>To: 'Marek Szyprowski' <m.szyprow...@samsung.com>; 'will.dea...@arm.com' 
><will.dea...@arm.com>; 'robin.mur...@arm.com'
><robin.mur...@arm.com>; 'j...@8bytes.org' <j...@8bytes.org>; 
>'iommu@lists.linux-foundation.org' <iommu@lists.linux-
>foundation.org>; 'linux-arm-ker...@lists.infradead.org' 
><linux-arm-ker...@lists.infradead.org>; 'linux-arm-...@vger.kernel.org'
><linux-arm-...@vger.kernel.org>; 'laurent.pinch...@ideasonboard.com' 
><laurent.pinch...@ideasonboard.com>;
>'tf...@chromium.org' <tf...@chromium.org>; 'srinivas.kandaga...@linaro.org' 
><srinivas.kandaga...@linaro.org>
>Subject: RE: [PATCH V3 0/8] IOMMU probe deferral support
>
>Hi Marek,
>
>>Hi Sricharan,
>>
>>
>>On 2016-10-04 19:03, Sricharan R wrote:
>>> Initial post from Laurent Pinchart[1]. This is
>>> series calls the dma ops configuration for the devices
>>> at a generic place so that it works for all busses.
>>> The dma_configure_ops for a device is now called during
>>> the device_attach callback just before the probe of the
>>> bus/driver is called. Similarly dma_deconfigure is called during
>>> device/driver_detach path.
>>>
>>>
>>> pci_bus_add_devices(platform/amba)(_device_create/driver_register)
>>> | |
>>> pci_bus_add_device (device_add/driver_register)
>>> | |
>>> device_attach   device_initial_probe
>>> | |
>>> __device_attach_driver__device_attach_driver
>>> |
>>> driver_probe_device
>>> |
>>> really_probe
>>> |
>>> dma_configure
>>>
>>>   Similarly on the device/driver_unregister path __device_release_driver is
>>>   called which inturn calls dma_deconfigure.
>>>
>>>   If the ACPI bus code follows the same, we can add acpi_dma_configure
>>>   at the same place as of_dma_configure.
>>>
>>>   This series is based on the recently merged Generic DT bindings for
>>>   PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]
>>>
>>>   This time tested this with platform and pci device for probe deferral
>>>   and reprobe on arm64 based platform. There is an issue on the cleanup
>>>   path for arm64 though, where there is WARN_ON if the dma_ops is reset 
>>> while
>>>   device is attached to an domain in arch_teardown_dma_ops.
>>>   But with iommu_groups created from the iommu driver, the device is always
>>>   attached to a domain/default_domain. So so the WARN has to be 
>>> removed/handled
>>>   probably.
>>
>>Thanks for continuing work on this feature! Your can add my:
>>
>>Tested-by: Marek Szyprowski <m.szyprow...@samsung.com>
>>
Hi Will,Robin,Joerg,
   
   I have tested the probe deferral for platform/pcie bus devices based on 
latest Generic bindings
series merged [1], for pci/arm-smmu.
   It will good to know from you on whats the right way to take this 
forward ?

Regards,
 Sricharan

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-12 Thread Sricharan

Hi Marek,

>Hi Sricharan,
>
>
>On 2016-10-04 19:03, Sricharan R wrote:
>> Initial post from Laurent Pinchart[1]. This is
>> series calls the dma ops configuration for the devices
>> at a generic place so that it works for all busses.
>> The dma_configure_ops for a device is now called during
>> the device_attach callback just before the probe of the
>> bus/driver is called. Similarly dma_deconfigure is called during
>> device/driver_detach path.
>>
>>
>> pci_bus_add_devices(platform/amba)(_device_create/driver_register)
>> | |
>> pci_bus_add_device (device_add/driver_register)
>> | |
>> device_attach   device_initial_probe
>> | |
>> __device_attach_driver__device_attach_driver
>> |
>> driver_probe_device
>> |
>> really_probe
>> |
>> dma_configure
>>
>>   Similarly on the device/driver_unregister path __device_release_driver is
>>   called which inturn calls dma_deconfigure.
>>
>>   If the ACPI bus code follows the same, we can add acpi_dma_configure
>>   at the same place as of_dma_configure.
>>
>>   This series is based on the recently merged Generic DT bindings for
>>   PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]
>>
>>   This time tested this with platform and pci device for probe deferral
>>   and reprobe on arm64 based platform. There is an issue on the cleanup
>>   path for arm64 though, where there is WARN_ON if the dma_ops is reset while
>>   device is attached to an domain in arch_teardown_dma_ops.
>>   But with iommu_groups created from the iommu driver, the device is always
>>   attached to a domain/default_domain. So so the WARN has to be 
>> removed/handled
>>   probably.
>
>Thanks for continuing work on this feature! Your can add my:
>
>Tested-by: Marek Szyprowski 
>
  Thanks for testing this. So the for the below fix, the remove_device callback
  gets called on the dma_ops cleanup path, so would it be easy to remove the
  data for the device there ?

Regards,
 Sricharan


>It works fine with Exynos SYSMMU driver, although a patch is needed to fix
>infinite loop due to list corruption (same element is added twice if master
>device fails with deferred probe):
>
>From: Marek Szyprowski 
>Date: Mon, 10 Oct 2016 14:22:42 +0200
>Subject: [PATCH] iommu/exynos: ensure that sysmmu is added only once to its
>  master
>
>Since adding IOMMU deferred probing support, of_xlate() callback might
>be called more than once for given master device (for example it happens
>when masters device driver fails with EPROBE_DEFER), so ensure that
>SYSMMU controller is added to its master device (owner) only once.
>
>Signed-off-by: Marek Szyprowski 
>---
>  drivers/iommu/exynos-iommu.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>index 30808e91b775..1525a86eb829 100644
>--- a/drivers/iommu/exynos-iommu.c
>+++ b/drivers/iommu/exynos-iommu.c
>@@ -1253,7 +1253,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
>  {
>  struct exynos_iommu_owner *owner = dev->archdata.iommu;
>  struct platform_device *sysmmu = of_find_device_by_node(spec->np);
>-struct sysmmu_drvdata *data;
>+struct sysmmu_drvdata *data, *entry;
>
>  if (!sysmmu)
>  return -ENODEV;
>@@ -1271,6 +1271,10 @@ static int exynos_iommu_of_xlate(struct device *dev,
>  dev->archdata.iommu = owner;
>  }
>
>+list_for_each_entry(entry, >controllers, owner_node)
>+if (entry == data)
>+return 0;
>+
>  list_add_tail(>owner_node, >controllers);
>  return 0;
>  }
>--
>1.9.1
>

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V3 0/8] IOMMU probe deferral support

2016-10-10 Thread Marek Szyprowski


Hi Sricharan,


On 2016-10-04 19:03, Sricharan R wrote:

Initial post from Laurent Pinchart[1]. This is
series calls the dma ops configuration for the devices
at a generic place so that it works for all busses.
The dma_configure_ops for a device is now called during
the device_attach callback just before the probe of the
bus/driver is called. Similarly dma_deconfigure is called during
device/driver_detach path.


pci_bus_add_devices(platform/amba)(_device_create/driver_register)
| |
pci_bus_add_device (device_add/driver_register)
| |
device_attach   device_initial_probe
| |
__device_attach_driver__device_attach_driver
|
driver_probe_device
|
really_probe
|
dma_configure

  Similarly on the device/driver_unregister path __device_release_driver is
  called which inturn calls dma_deconfigure.

  If the ACPI bus code follows the same, we can add acpi_dma_configure
  at the same place as of_dma_configure.

  This series is based on the recently merged Generic DT bindings for
  PCI IOMMUs and ARM SMMU from Robin Murphy robin.mur...@arm.com [2]

  This time tested this with platform and pci device for probe deferral
  and reprobe on arm64 based platform. There is an issue on the cleanup
  path for arm64 though, where there is WARN_ON if the dma_ops is reset while
  device is attached to an domain in arch_teardown_dma_ops.
  But with iommu_groups created from the iommu driver, the device is always
  attached to a domain/default_domain. So so the WARN has to be removed/handled
  probably.


Thanks for continuing work on this feature! Your can add my:

Tested-by: Marek Szyprowski 

It works fine with Exynos SYSMMU driver, although a patch is needed to fix
infinite loop due to list corruption (same element is added twice if master
device fails with deferred probe):

From: Marek Szyprowski 
Date: Mon, 10 Oct 2016 14:22:42 +0200
Subject: [PATCH] iommu/exynos: ensure that sysmmu is added only once to its
 master

Since adding IOMMU deferred probing support, of_xlate() callback might
be called more than once for given master device (for example it happens
when masters device driver fails with EPROBE_DEFER), so ensure that
SYSMMU controller is added to its master device (owner) only once.

Signed-off-by: Marek Szyprowski 
---
 drivers/iommu/exynos-iommu.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 30808e91b775..1525a86eb829 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1253,7 +1253,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
 {
 struct exynos_iommu_owner *owner = dev->archdata.iommu;
 struct platform_device *sysmmu = of_find_device_by_node(spec->np);
-struct sysmmu_drvdata *data;
+struct sysmmu_drvdata *data, *entry;

 if (!sysmmu)
 return -ENODEV;
@@ -1271,6 +1271,10 @@ static int exynos_iommu_of_xlate(struct device *dev,
 dev->archdata.iommu = owner;
 }

+list_for_each_entry(entry, >controllers, owner_node)
+if (entry == data)
+return 0;
+
 list_add_tail(>owner_node, >controllers);
 return 0;
 }
--
1.9.1




  Previous post of this series [3].

  [V3]
  * Removed the patch to split dma_masks/dma_ops configuration separately
based on review comments that both masks and ops are required only
during the device probe time.

  * Reworked the series based on Generic DT bindings series [2].

  * Added call to iommu's remove_device in the cleanup path for arm and 
arm64.

  * Removed the notifier trick in arm64 to handle early device registration.

  * Added reset of dma_ops in cleanup path for arm based on comments.

  * Fixed the pci_iommu_configure path and tested with PCI device as well.
  
  * Fixed a bug to return the correct iommu_ops from patch 7 [4] in last post.


  * Fixed few other cosmetic comments.
   
  [V2]

  * Updated the Initial post to call dma_configure/deconfigure from generic 
code
  
  * Added iommu add_device callback from of_iommu_configure path


  [V1]
  * Initial post

[1] http://lists.linuxfoundation.org/pipermail/iommu/2015-May/013016.html
[2] http://www.spinics.net/lists/devicetree/msg142943.html
[3] https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg13941.html
[4] https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg13940.html



Laurent Pinchart (4):
   arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
   of: dma: Move range size workaround to of_dma_get_range()
   of: dma: Make of_dma_deconfigure() public
   iommu: of: Handle IOMMU lookup failure with deferred probing or error

Sricharan R (4):
   drivers: platform: Configure dma operations at probe time
   arm:

Re: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

Re: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

Re: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

Re: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

Re: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

Re: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

Re: [PATCH V3 0/8] IOMMU probe deferral support

Re: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

Re: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

RE: [PATCH V3 0/8] IOMMU probe deferral support

Re: [PATCH V3 0/8] IOMMU probe deferral support

23 matches

Site Navigation

Mail list logo

Footer information