From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type fails Date: Thu, 17 Mar 2016 16:20:11 +0000 Message-ID: <56EAD93B.7040105@nvidia.com> References: <1458224359-32665-1-git-send-email-jonathanh@nvidia.com> <1458224359-32665-5-git-send-email-jonathanh@nvidia.com> <56EAC761.1040801@nvidia.com> <20160317151800.GH1184@io.lakedaemon.net> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20160317151800.GH1184-fahSIxCzskDQ+YiMSub0/l6hYfS7NtTn@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jason Cooper Cc: Thomas Gleixner , Marc Zyngier , =?UTF-8?Q?Beno=c3=aet_Cousson?= , Tony Lindgren , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Stephen Warren , Thierry Reding , Kevin Hilman , Geert Uytterhoeven , Grygorii Strashko , Lars-Peter Clausen , Linus Walleij , linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-tegra@vger.kernel.org On 17/03/16 15:18, Jason Cooper wrote: > On Thu, Mar 17, 2016 at 03:04:01PM +0000, Jon Hunter wrote: >> >> On 17/03/16 14:51, Thomas Gleixner wrote: >>> On Thu, 17 Mar 2016, Jon Hunter wrote: >>> >>>> Setting the interrupt type for private peripheral interrupts (PPIs) may >>>> not be supported by a given GIC because it is IMPLEMENTATION DEFINED >>>> whether this is allowed. There is no way to know if setting the type is >>>> supported for a given GIC and so the value written is read back to >>>> verify it matches the desired configuration. If it does not match then >>>> an error is return. >>>> >>>> There are cases where the interrupt configuration read from firmware >>>> (such as a device-tree blob), has been incorrect and hence >>>> gic_configure_irq() has returned an error. This error has gone >>>> undetected because the error code returned was ignored but the interrupt >>>> still worked fine because the configuration for the interrupt could not >>>> be overwritten. >>>> >>>> Given that this has done undetected and we should only fail to set the >>>> type for PPIs whose configuration cannot be changed anyway, don't return >>>> an error and simply WARN if this fails. This will allows us to fix up any >>>> places in the kernel where we should be checking the return status and >>>> maintain back compatibility with firmware images that may have incorrect >>>> interrupt configurations. >>> >>> Though silently returning 0 is really the wrong thing to do. You can add the >>> warn, but why do you want to return success? >> >> Yes that would be the correct thing to do I agree. However, the problem >> is that if we do this, then after the patch "irqdomain: Don't set type >> when mapping an IRQ" is applied, we may break interrupts for some >> existing device-tree binaries that have bad configuration (such as omap4 >> and tegra20/30 ... see patches 1 and 2) that have gone unnoticed. So it >> is a back compatibility issue. > > This sounds like a textbook case for adding a boolean dt property. If > "can-set-ppi-type" is absent (old DT blobs and new blobs without the > ability), warn and return zero. If it's present, the driver can set the > type, returning errors as encountered. True. However, if we did have this "can-set-ppi-type" property set for a device, it really should never fail (unless someone specified it incorrectly). So I am trying to understand the value in adding a new DT property. Please note that gic_configure_irq() never used to return an error and only when adding support for setting the type of PPIs was this added. However, given that this has gone unnoticed and does not have a real functional impact on the device behaviour, I wonder now if this function should return an error? Yes, ideally, it should, but does it still make sense? Cheers Jon From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936073AbcCQQUZ (ORCPT ); Thu, 17 Mar 2016 12:20:25 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:7304 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752698AbcCQQUU (ORCPT ); Thu, 17 Mar 2016 12:20:20 -0400 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Thu, 17 Mar 2016 09:18:48 -0700 Subject: Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type fails To: Jason Cooper References: <1458224359-32665-1-git-send-email-jonathanh@nvidia.com> <1458224359-32665-5-git-send-email-jonathanh@nvidia.com> <56EAC761.1040801@nvidia.com> <20160317151800.GH1184@io.lakedaemon.net> CC: Thomas Gleixner , Marc Zyngier , =?UTF-8?Q?Beno=c3=aet_Cousson?= , Tony Lindgren , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "Stephen Warren" , Thierry Reding , Kevin Hilman , Geert Uytterhoeven , Grygorii Strashko , Lars-Peter Clausen , Linus Walleij , , , , From: Jon Hunter Message-ID: <56EAD93B.7040105@nvidia.com> Date: Thu, 17 Mar 2016 16:20:11 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160317151800.GH1184@io.lakedaemon.net> X-Originating-IP: [10.21.132.114] X-ClientProxiedBy: UKMAIL101.nvidia.com (10.26.138.13) To UKMAIL101.nvidia.com (10.26.138.13) 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 17/03/16 15:18, Jason Cooper wrote: > On Thu, Mar 17, 2016 at 03:04:01PM +0000, Jon Hunter wrote: >> >> On 17/03/16 14:51, Thomas Gleixner wrote: >>> On Thu, 17 Mar 2016, Jon Hunter wrote: >>> >>>> Setting the interrupt type for private peripheral interrupts (PPIs) may >>>> not be supported by a given GIC because it is IMPLEMENTATION DEFINED >>>> whether this is allowed. There is no way to know if setting the type is >>>> supported for a given GIC and so the value written is read back to >>>> verify it matches the desired configuration. If it does not match then >>>> an error is return. >>>> >>>> There are cases where the interrupt configuration read from firmware >>>> (such as a device-tree blob), has been incorrect and hence >>>> gic_configure_irq() has returned an error. This error has gone >>>> undetected because the error code returned was ignored but the interrupt >>>> still worked fine because the configuration for the interrupt could not >>>> be overwritten. >>>> >>>> Given that this has done undetected and we should only fail to set the >>>> type for PPIs whose configuration cannot be changed anyway, don't return >>>> an error and simply WARN if this fails. This will allows us to fix up any >>>> places in the kernel where we should be checking the return status and >>>> maintain back compatibility with firmware images that may have incorrect >>>> interrupt configurations. >>> >>> Though silently returning 0 is really the wrong thing to do. You can add the >>> warn, but why do you want to return success? >> >> Yes that would be the correct thing to do I agree. However, the problem >> is that if we do this, then after the patch "irqdomain: Don't set type >> when mapping an IRQ" is applied, we may break interrupts for some >> existing device-tree binaries that have bad configuration (such as omap4 >> and tegra20/30 ... see patches 1 and 2) that have gone unnoticed. So it >> is a back compatibility issue. > > This sounds like a textbook case for adding a boolean dt property. If > "can-set-ppi-type" is absent (old DT blobs and new blobs without the > ability), warn and return zero. If it's present, the driver can set the > type, returning errors as encountered. True. However, if we did have this "can-set-ppi-type" property set for a device, it really should never fail (unless someone specified it incorrectly). So I am trying to understand the value in adding a new DT property. Please note that gic_configure_irq() never used to return an error and only when adding support for setting the type of PPIs was this added. However, given that this has gone unnoticed and does not have a real functional impact on the device behaviour, I wonder now if this function should return an error? Yes, ideally, it should, but does it still make sense? Cheers Jon