Re: [PATCH 1/2] vfio, platform: add support for ACPI while detecting the reset driver
From: Eric Auger
Date: Thu Mar 10 2016 - 22:49:39 EST
Hi Sinan,
On 03/10/2016 06:13 PM, Sinan Kaya wrote:
> On 3/10/2016 5:11 AM, Eric Auger wrote:
>> Hi Sinan,
>> On 03/08/2016 04:33 PM, Sinan Kaya wrote:
>>> The code is using the compatible DT string to associate a reset driver with
>>> the actual device itself. The compatible string does not exist on ACPI
>>> based systems. HID is the unique identifier for a device driver instead.
>>> The change allows a driver to register with DT compatible string or ACPI
>>> HID and then match the object with one of these conditions.
>>>
>>> Rules for loading the reset driver are as follow:
>>> - ACPI HID needs match for ACPI systems
>>> - DT compat needs to match for OF systems
>>>
>>> Tested-by: Eric Auger <eric.auger@xxxxxxxxxx> (device tree only)
>>> Tested-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx> (ACPI only)
>>> Signed-off-by: Sinan Kaya <okaya@xxxxxxxxxxxxxx>
>>> ---
>>> .../vfio/platform/reset/vfio_platform_amdxgbe.c | 4 +-
>>> .../platform/reset/vfio_platform_calxedaxgmac.c | 4 +-
>>> drivers/vfio/platform/vfio_platform_common.c | 112 +++++++++++++++++----
>>> drivers/vfio/platform/vfio_platform_private.h | 43 ++++----
>>> 4 files changed, 125 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
>>> index d4030d0..170dcf5 100644
>>> --- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
>>> +++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
>>> @@ -119,7 +119,9 @@ int vfio_platform_amdxgbe_reset(struct vfio_platform_device *vdev)
>>> return 0;
>>> }
>>>
>>> -module_vfio_reset_handler("amd,xgbe-seattle-v1a", vfio_platform_amdxgbe_reset);
>>> +module_vfio_reset_handler("amd,xgbe-seattle-v1a", NULL,
>>> + vfio_platform_amdxgbe_reset);
>>> +VFIO_PLATFORM_MODULE_ALIAS("amd,xgbe-seattle-v1a");
>> Looks the other overridden MODULE_ALIAS have a naming like
>> MODULE_ALIAS_something? what about MODULE_ALIAS_VFIO_PLATFORM_RESET?
>> not very compact but maybe closer to what it stands for.
>
> alright, I'll follow that.
>
>>>
>>> MODULE_VERSION("0.1");
>>> MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>>> index e3d3d94..635c6e0 100644
>>> --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>>> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>>> @@ -77,7 +77,9 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
>>> return 0;
>>> }
>>>
>>> -module_vfio_reset_handler("calxeda,hb-xgmac", vfio_platform_calxedaxgmac_reset);
>>> +module_vfio_reset_handler("calxeda,hb-xgmac", NULL,
>>> + vfio_platform_calxedaxgmac_reset);
>>> +VFIO_PLATFORM_MODULE_ALIAS("calxeda,hb-xgmac");
>>>
>>> MODULE_VERSION(DRIVER_VERSION);
>>> MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>>> index 418cdd9..c758e72 100644
>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>> @@ -13,6 +13,7 @@
>>> */
>>>
>>> #include <linux/device.h>
>>> +#include <linux/acpi.h>
>>> #include <linux/iommu.h>
>>> #include <linux/module.h>
>>> #include <linux/mutex.h>
>>> @@ -31,14 +32,22 @@ static LIST_HEAD(reset_list);
>>> static DEFINE_MUTEX(driver_lock);
>>>
>>> static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>> - struct module **module)
>>> + const char *acpihid, struct module **module)
>>> {
>>> struct vfio_platform_reset_node *iter;
>>> vfio_platform_reset_fn_t reset_fn = NULL;
>>>
>>> mutex_lock(&driver_lock);
>>> list_for_each_entry(iter, &reset_list, link) {
>>> - if (!strcmp(iter->compat, compat) &&
>>> + if (acpihid && iter->acpihid &&
>>> + !strcmp(iter->acpihid, acpihid) &&
>>> + try_module_get(iter->owner)) {
>>> + *module = iter->owner;
>>> + reset_fn = iter->reset;
>>> + break;
>>> + }
>>> + if (compat && iter->compat &&
>>> + !strcmp(iter->compat, compat) &&
>>> try_module_get(iter->owner)) {
>>> *module = iter->owner;
>>> reset_fn = iter->reset;
>>> @@ -49,15 +58,30 @@ static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
>>> return reset_fn;
>>> }
>>>
>>> -static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
>>> +static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>> What is the point returning a value here? See my comment at the end.
>
> I was trying to do some error handling here. If a driver does not have a
> reset implementation, we are letting it go right now.
>
> I think we need to make reset driver a requirement. Mark Rutland reminded me
> that I need a reset driver.
>
> I did not think about it during implementation and I have not seen any
> warnings like you said.
the warning currently is emitted on vfio_platform_open/release:
dev_warn(vdev->device, "no reset function found!\n");
>
>>> {
>>> - vdev->reset = vfio_platform_lookup_reset(vdev->compat,
>>> - &vdev->reset_module);
>>> - if (!vdev->reset) {
>>> - request_module("vfio-reset:%s", vdev->compat);
>>> - vdev->reset = vfio_platform_lookup_reset(vdev->compat,
>>> - &vdev->reset_module);
>>> - }
>
>
>>>
>>> - vfio_platform_get_reset(vdev);
>>> + ret = vfio_platform_get_reset(vdev);
>>> + if (ret) {
>>> + iommu_group_put(group);
>>> + return ret;
>>> + }
>> This change is not related to your commit message. Also here you change
>> the use model of VFIO platform and forbid any usage if no reset module
>> is available, right? I don't think this is something we discussed and I
>> think it removes some flexibility. Currently a warning is emitted in
>> case we don't have a reset function.
>
> Well, I haven't seen that warning during testing. I was trying to be more
> proactive.
Thank you for that. Personally I agree to proceed with the proposed
change, ie. forbid vfio platform driver to be used if there is no reset
function found but this should be in a separate patch with explicit
commit mesg and you should remove dev_warn & related dead code.
Best Regards
Eric
>
> I'm fine removing these checks but not having a reset driver needs a really
> big warning here.
>
>>
>> Best Regards
>>
>> Eric
>>>
>
>
>