From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hanjun Guo Subject: Re: [PATCH 06/11] ACPI / gsi: Add gsi_mutex to synchronize acpi_register_gsi()/acpi_unregister_gsi() Date: Thu, 11 Jun 2015 21:16:24 +0800 Message-ID: <55798A28.3080000@linaro.org> References: <1431953961-22706-1-git-send-email-hanjun.guo@linaro.org> <1431953961-22706-7-git-send-email-hanjun.guo@linaro.org> <55785E8D.1000909@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pa0-f47.google.com ([209.85.220.47]:36821 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752351AbbFKNQr (ORCPT ); Thu, 11 Jun 2015 09:16:47 -0400 Received: by pabqy3 with SMTP id qy3so3951370pab.3 for ; Thu, 11 Jun 2015 06:16:46 -0700 (PDT) In-Reply-To: <55785E8D.1000909@arm.com> Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Marc Zyngier , Jason Cooper , Will Deacon , Catalin Marinas , "Rafael J. Wysocki" Cc: Jiang Liu , Lorenzo Pieralisi , Arnd Bergmann , Tomasz Nowicki , "grant.likely@linaro.org" , Thomas Gleixner , Olof Johansson , "linux-arm-kernel@lists.infradead.org" , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linaro-acpi@lists.linaro.org" On 06/10/2015 11:58 PM, Marc Zyngier wrote: > On 18/05/15 13:59, Hanjun Guo wrote: >> Add a mutex for acpi_register_gsi()/acpi_unregister_gsi() to avoid >> concurrency issues. >> >> Signed-off-by: Hanjun Guo >> --- >> drivers/acpi/gsi.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c >> index 55b5f31..ab0dcb4 100644 >> --- a/drivers/acpi/gsi.c >> +++ b/drivers/acpi/gsi.c >> @@ -16,6 +16,7 @@ >> enum acpi_irq_model_id acpi_irq_model; >> /* ACPI core domian pointing to GICv2/3 core domain */ >> struct irq_domain *acpi_irq_domain __read_mostly; >> +static DEFINE_MUTEX(gsi_mutex); >> >> static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) >> { >> @@ -73,20 +74,24 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, >> int irq; >> unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity); >> >> + mutex_lock(&gsi_mutex); >> irq = irq_find_mapping(acpi_irq_domain, gsi); >> if (irq > 0) >> - return irq; >> + goto out; >> >> irq = irq_domain_alloc_irqs(acpi_irq_domain, 1, dev_to_node(dev), >> &gsi); >> if (irq <= 0) >> - return -EINVAL; >> + goto out; >> >> /* Set irq type if specified and different than the current one */ >> if (irq_type != IRQ_TYPE_NONE && >> irq_type != irq_get_trigger_type(irq)) >> irq_set_irq_type(irq, irq_type); >> - return irq; >> + >> +out: >> + mutex_unlock(&gsi_mutex); >> + return irq > 0 ? irq : -EINVAL; >> } >> EXPORT_SYMBOL_GPL(acpi_register_gsi); >> >> @@ -96,8 +101,12 @@ EXPORT_SYMBOL_GPL(acpi_register_gsi); >> */ >> void acpi_unregister_gsi(u32 gsi) >> { >> - int irq = irq_find_mapping(acpi_irq_domain, gsi); >> + int irq; >> + >> + mutex_lock(&gsi_mutex); >> + irq = irq_find_mapping(acpi_irq_domain, gsi); >> >> irq_dispose_mapping(irq); >> + mutex_unlock(&gsi_mutex); >> } >> EXPORT_SYMBOL_GPL(acpi_unregister_gsi); >> > > Can you point out why we need this locking? The rest of the kernel seems > to live without it pretty well. And if we really have an issue, I'd Hmm, I'm not so sure, I will look deep into that and come back later. > prefer seeing it fixed in the core code rather than in something that is > very much firmware-specific. I agree if there are real issues. Thanks Hanjun From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932533AbbFKNQz (ORCPT ); Thu, 11 Jun 2015 09:16:55 -0400 Received: from mail-pa0-f48.google.com ([209.85.220.48]:35249 "EHLO mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752738AbbFKNQr (ORCPT ); Thu, 11 Jun 2015 09:16:47 -0400 Message-ID: <55798A28.3080000@linaro.org> Date: Thu, 11 Jun 2015 21:16:24 +0800 From: Hanjun Guo User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Marc Zyngier , Jason Cooper , Will Deacon , Catalin Marinas , "Rafael J. Wysocki" CC: Jiang Liu , Lorenzo Pieralisi , Arnd Bergmann , Tomasz Nowicki , "grant.likely@linaro.org" , Thomas Gleixner , Olof Johansson , "linux-arm-kernel@lists.infradead.org" , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linaro-acpi@lists.linaro.org" Subject: Re: [PATCH 06/11] ACPI / gsi: Add gsi_mutex to synchronize acpi_register_gsi()/acpi_unregister_gsi() References: <1431953961-22706-1-git-send-email-hanjun.guo@linaro.org> <1431953961-22706-7-git-send-email-hanjun.guo@linaro.org> <55785E8D.1000909@arm.com> In-Reply-To: <55785E8D.1000909@arm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/10/2015 11:58 PM, Marc Zyngier wrote: > On 18/05/15 13:59, Hanjun Guo wrote: >> Add a mutex for acpi_register_gsi()/acpi_unregister_gsi() to avoid >> concurrency issues. >> >> Signed-off-by: Hanjun Guo >> --- >> drivers/acpi/gsi.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c >> index 55b5f31..ab0dcb4 100644 >> --- a/drivers/acpi/gsi.c >> +++ b/drivers/acpi/gsi.c >> @@ -16,6 +16,7 @@ >> enum acpi_irq_model_id acpi_irq_model; >> /* ACPI core domian pointing to GICv2/3 core domain */ >> struct irq_domain *acpi_irq_domain __read_mostly; >> +static DEFINE_MUTEX(gsi_mutex); >> >> static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) >> { >> @@ -73,20 +74,24 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, >> int irq; >> unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity); >> >> + mutex_lock(&gsi_mutex); >> irq = irq_find_mapping(acpi_irq_domain, gsi); >> if (irq > 0) >> - return irq; >> + goto out; >> >> irq = irq_domain_alloc_irqs(acpi_irq_domain, 1, dev_to_node(dev), >> &gsi); >> if (irq <= 0) >> - return -EINVAL; >> + goto out; >> >> /* Set irq type if specified and different than the current one */ >> if (irq_type != IRQ_TYPE_NONE && >> irq_type != irq_get_trigger_type(irq)) >> irq_set_irq_type(irq, irq_type); >> - return irq; >> + >> +out: >> + mutex_unlock(&gsi_mutex); >> + return irq > 0 ? irq : -EINVAL; >> } >> EXPORT_SYMBOL_GPL(acpi_register_gsi); >> >> @@ -96,8 +101,12 @@ EXPORT_SYMBOL_GPL(acpi_register_gsi); >> */ >> void acpi_unregister_gsi(u32 gsi) >> { >> - int irq = irq_find_mapping(acpi_irq_domain, gsi); >> + int irq; >> + >> + mutex_lock(&gsi_mutex); >> + irq = irq_find_mapping(acpi_irq_domain, gsi); >> >> irq_dispose_mapping(irq); >> + mutex_unlock(&gsi_mutex); >> } >> EXPORT_SYMBOL_GPL(acpi_unregister_gsi); >> > > Can you point out why we need this locking? The rest of the kernel seems > to live without it pretty well. And if we really have an issue, I'd Hmm, I'm not so sure, I will look deep into that and come back later. > prefer seeing it fixed in the core code rather than in something that is > very much firmware-specific. I agree if there are real issues. Thanks Hanjun From mboxrd@z Thu Jan 1 00:00:00 1970 From: hanjun.guo@linaro.org (Hanjun Guo) Date: Thu, 11 Jun 2015 21:16:24 +0800 Subject: [PATCH 06/11] ACPI / gsi: Add gsi_mutex to synchronize acpi_register_gsi()/acpi_unregister_gsi() In-Reply-To: <55785E8D.1000909@arm.com> References: <1431953961-22706-1-git-send-email-hanjun.guo@linaro.org> <1431953961-22706-7-git-send-email-hanjun.guo@linaro.org> <55785E8D.1000909@arm.com> Message-ID: <55798A28.3080000@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 06/10/2015 11:58 PM, Marc Zyngier wrote: > On 18/05/15 13:59, Hanjun Guo wrote: >> Add a mutex for acpi_register_gsi()/acpi_unregister_gsi() to avoid >> concurrency issues. >> >> Signed-off-by: Hanjun Guo >> --- >> drivers/acpi/gsi.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c >> index 55b5f31..ab0dcb4 100644 >> --- a/drivers/acpi/gsi.c >> +++ b/drivers/acpi/gsi.c >> @@ -16,6 +16,7 @@ >> enum acpi_irq_model_id acpi_irq_model; >> /* ACPI core domian pointing to GICv2/3 core domain */ >> struct irq_domain *acpi_irq_domain __read_mostly; >> +static DEFINE_MUTEX(gsi_mutex); >> >> static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) >> { >> @@ -73,20 +74,24 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, >> int irq; >> unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity); >> >> + mutex_lock(&gsi_mutex); >> irq = irq_find_mapping(acpi_irq_domain, gsi); >> if (irq > 0) >> - return irq; >> + goto out; >> >> irq = irq_domain_alloc_irqs(acpi_irq_domain, 1, dev_to_node(dev), >> &gsi); >> if (irq <= 0) >> - return -EINVAL; >> + goto out; >> >> /* Set irq type if specified and different than the current one */ >> if (irq_type != IRQ_TYPE_NONE && >> irq_type != irq_get_trigger_type(irq)) >> irq_set_irq_type(irq, irq_type); >> - return irq; >> + >> +out: >> + mutex_unlock(&gsi_mutex); >> + return irq > 0 ? irq : -EINVAL; >> } >> EXPORT_SYMBOL_GPL(acpi_register_gsi); >> >> @@ -96,8 +101,12 @@ EXPORT_SYMBOL_GPL(acpi_register_gsi); >> */ >> void acpi_unregister_gsi(u32 gsi) >> { >> - int irq = irq_find_mapping(acpi_irq_domain, gsi); >> + int irq; >> + >> + mutex_lock(&gsi_mutex); >> + irq = irq_find_mapping(acpi_irq_domain, gsi); >> >> irq_dispose_mapping(irq); >> + mutex_unlock(&gsi_mutex); >> } >> EXPORT_SYMBOL_GPL(acpi_unregister_gsi); >> > > Can you point out why we need this locking? The rest of the kernel seems > to live without it pretty well. And if we really have an issue, I'd Hmm, I'm not so sure, I will look deep into that and come back later. > prefer seeing it fixed in the core code rather than in something that is > very much firmware-specific. I agree if there are real issues. Thanks Hanjun