From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754109AbcCJRNg (ORCPT ); Thu, 10 Mar 2016 12:13:36 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:56228 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753173AbcCJRNd (ORCPT ); Thu, 10 Mar 2016 12:13:33 -0500 Subject: Re: [PATCH 1/2] vfio, platform: add support for ACPI while detecting the reset driver To: Eric Auger , kvm@vger.kernel.org, timur@codeaurora.org, cov@codeaurora.org, jcm@redhat.com References: <1457451209-21462-1-git-send-email-okaya@codeaurora.org> <56E14858.9050401@linaro.org> Cc: shankerd@codeaurora.org, vikrams@codeaurora.org, marc.zyngier@arm.com, mark.rutland@arm.com, devicetree@vger.kernel.org, vinod.koul@intel.com, agross@codeaurora.org, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Baptiste Reynal , Alex Williamson , Dan Carpenter , Arnd Bergmann , linux-kernel@vger.kernel.org From: Sinan Kaya Message-ID: <56E1AB38.8020702@codeaurora.org> Date: Thu, 10 Mar 2016 12:13:28 -0500 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <56E14858.9050401@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 (device tree only) >> Tested-by: Shanker Donthineni (ACPI only) >> Signed-off-by: Sinan Kaya >> --- >> .../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 >> +#include >> #include >> #include >> #include >> @@ -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. >> { >> - 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. I'm fine removing these checks but not having a reset driver needs a really big warning here. > > Best Regards > > Eric >> -- Sinan Kaya Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project