From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-21.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E84A5C4320E for ; Wed, 25 Aug 2021 12:33:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CDC82610E5 for ; Wed, 25 Aug 2021 12:33:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240911AbhHYMel (ORCPT ); Wed, 25 Aug 2021 08:34:41 -0400 Received: from mail.kernel.org ([198.145.29.99]:40324 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240864AbhHYMej (ORCPT ); Wed, 25 Aug 2021 08:34:39 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 96DC261184; Wed, 25 Aug 2021 12:33:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1629894833; bh=qrESRDAEVzdBOBV/LmDq/fGG6ZFyDzbuKD741OD8lw8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=ptaaKz6BM9sC8AF20cxv7ay0tp6xTyNsiqtL2al4Vfzn5R+ZpYOzYOA1xvXnZIzYv PK+FJmuXp6S3mHqbiW2UKQq6mAI2X8nyGt/HfhWPtyoTfmjANXs7zRq2qa4ZK9R00a 2GeLpDdA1fYdEBma/psBZD4Vpv/wfkgjqrgTLUCITNmWf9Oxby2bMf5JhgfhTFCFUf YiiPXUeOLv2Sv2hvGK2Pz4oSisNhom7rLCUthmjn6Fd56XJpAfaMUfktEi+uer9p5n lViqpSR/iT8mLu6A5dsUDndWcW36EM/rO8SqPM5nBHRvYaZPpsOKtrUy10JY7idyE6 /3fn8EfzOk/3g== Received: by mail-ed1-f46.google.com with SMTP id q3so36753667edt.5; Wed, 25 Aug 2021 05:33:53 -0700 (PDT) X-Gm-Message-State: AOAM530zSDlSjn0+Gtpqi8tvzNtBCXLK104Yj6Atbz9mByARJFqx9oLZ /YJ7J7NLVzk//Y3EOaBsUL7fb90M0bgMcLpEmg== X-Google-Smtp-Source: ABdhPJxKPMFYHuam6oE3ysnmdaKPmXcVvdn2Bl4IdLfOhvyu9U/HF8ucAh/dyuGU/eEYLlFwdzzAFp8v/IGuoot3VpU= X-Received: by 2002:aa7:cb19:: with SMTP id s25mr49263006edt.194.1629894832158; Wed, 25 Aug 2021 05:33:52 -0700 (PDT) MIME-Version: 1.0 References: <20210816074619.177383-1-wangkefeng.wang@huawei.com> <20210816074619.177383-4-wangkefeng.wang@huawei.com> In-Reply-To: From: Rob Herring Date: Wed, 25 Aug 2021 07:33:40 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 3/3] amba: Properly handle device probe without IRQ domain To: Kefeng Wang Cc: Saravana Kannan , "linux-kernel@vger.kernel.org" , Frank Rowand , devicetree@vger.kernel.org, Russell King , Linus Walleij , linux-arm-kernel , Ruizhe Lin Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 24, 2021 at 11:05 PM Kefeng Wang wrote: > > > On 2021/8/25 4:08, Saravana Kannan wrote: > > On Tue, Aug 24, 2021 at 1:05 PM Rob Herring wrote: > >> +Saravana > >> > >> Saravana mentioned to me there may be some issues with this one... > >> > >> > >> On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang wrote: > >>> of_amba_device_create() uses irq_of_parse_and_map() to translate > >>> a DT interrupt specification into a Linux virtual interrupt number. > >>> > >>> But it doesn't properly handle the case where the interrupt controller > >>> is not yet available, eg, when pl011 interrupt is connected to MBIGEN > >>> interrupt controller, because the mbigen initialization is too late, > >>> which will lead to no IRQ due to no IRQ domain found, log is shown below, > >>> "irq: no irq domain found for uart0 !" > >>> > >>> use of_irq_get() to return -EPROBE_DEFER as above, and in the function > >>> amba_device_try_add()/amba_device_add(), it will properly handle in such > >>> case, also return 0 in other fail cases to be consistent as before. > >>> > >>> Cc: Russell King > >>> Cc: Rob Herring > >>> Cc: Frank Rowand > >>> Reported-by: Ruizhe Lin > >>> Signed-off-by: Kefeng Wang > >>> --- > >>> drivers/amba/bus.c | 27 +++++++++++++++++++++++++++ > >>> drivers/of/platform.c | 6 +----- > >>> 2 files changed, 28 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > >>> index 36f2f42c8014..720aa6cdd402 100644 > >>> --- a/drivers/amba/bus.c > >>> +++ b/drivers/amba/bus.c > >>> @@ -19,6 +19,7 @@ > >>> #include > >>> #include > >>> #include > >>> +#include > >>> > >>> #include > >>> > >>> @@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev) > >>> kfree(d); > >>> } > >>> > >>> +static int of_amba_device_decode_irq(struct amba_device *dev) > >>> +{ > >>> + struct device_node *node = dev->dev.of_node; > >>> + int i, irq = 0; > >>> + > >>> + if (IS_ENABLED(CONFIG_OF_IRQ) && node) { > >>> + /* Decode the IRQs and address ranges */ > >>> + for (i = 0; i < AMBA_NR_IRQS; i++) { > >>> + irq = of_irq_get(node, i); > >>> + if (irq < 0) { > >>> + if (irq == -EPROBE_DEFER) > >>> + return irq; > >>> + irq = 0; > >>> + } > >>> + > >>> + dev->irq[i] = irq; > >>> + } > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > >>> { > >>> u32 size; > >>> void __iomem *tmp; > >>> int i, ret; > >>> > >>> + ret = of_amba_device_decode_irq(dev); > >>> + if (ret) > >>> + goto err_out; > >>> + > > Similar to other resources the AMBA bus "gets" for the device, I think > > this should be moved into amba_probe() and not here. There's no reason > > to delay the addition of the device (and loading its module) because > > the IRQ isn't ready yet. > > The following code in the amba_device_try_add() will be called, it uses irq[0] > and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add(). > > 470 if (dev->irq[0]) > 471 ret = device_create_file(&dev->dev, &dev_attr_irq0); > 472 if (ret == 0 && dev->irq[1]) > 473 ret = device_create_file(&dev->dev, &dev_attr_irq1); > 474 if (ret == 0) > 475 return ret; I wonder if we could just remove these. Why does userspace need them in the first place? It's only an ABI if someone notices. Looking at the history, AMBA bus was added in 2003 with just 'irq' and then changed (ABI break) in 2004 to 'irq0' and 'irq1'. Rob [1] https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/log/arch/arm/common/amba.c From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-19.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C9A8BC4338F for ; Wed, 25 Aug 2021 12:36:18 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 860CC610D1 for ; Wed, 25 Aug 2021 12:36:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 860CC610D1 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=+M9ZHsbUH7i0YB4rtrkCd3mEPfCdPwtRbfjYeivBp+w=; b=ixzczJbZdJ98/n dW4nL/bunuoly9yWbanqZNJuoh2HmSuR8PZYafxJtTv6NkILH4YSvnIMEAtIA72nOyq1s8GWcLbjl luDCdoz/PBIsmpPQTDG1MiKHBzldv407XO8+aTHlVO/LFI6NqxCr/ZBeJVDiYbVrsxunOtPnH47C3 i8JPQezuB7MqmyWTbWhJFYeTxujjl8wiIuv/xLPPHLOrznR54wzhN70czKvpAozidAn+RQvk+amlB ltgWN52O4BrZrzMDadGje1LPtM/ELVJdLGsiROX+d0TPGu1X4XxLUQzk96T0nWg2ScyrFpAva31be npXOPUWqUE5eDiJC7WYA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mIs6Q-006wi8-S6; Wed, 25 Aug 2021 12:33:59 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mIs6M-006whJ-K1 for linux-arm-kernel@lists.infradead.org; Wed, 25 Aug 2021 12:33:56 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id 92E13610E5 for ; Wed, 25 Aug 2021 12:33:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1629894833; bh=qrESRDAEVzdBOBV/LmDq/fGG6ZFyDzbuKD741OD8lw8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=ptaaKz6BM9sC8AF20cxv7ay0tp6xTyNsiqtL2al4Vfzn5R+ZpYOzYOA1xvXnZIzYv PK+FJmuXp6S3mHqbiW2UKQq6mAI2X8nyGt/HfhWPtyoTfmjANXs7zRq2qa4ZK9R00a 2GeLpDdA1fYdEBma/psBZD4Vpv/wfkgjqrgTLUCITNmWf9Oxby2bMf5JhgfhTFCFUf YiiPXUeOLv2Sv2hvGK2Pz4oSisNhom7rLCUthmjn6Fd56XJpAfaMUfktEi+uer9p5n lViqpSR/iT8mLu6A5dsUDndWcW36EM/rO8SqPM5nBHRvYaZPpsOKtrUy10JY7idyE6 /3fn8EfzOk/3g== Received: by mail-ed1-f46.google.com with SMTP id i6so36706421edu.1 for ; Wed, 25 Aug 2021 05:33:53 -0700 (PDT) X-Gm-Message-State: AOAM5313iAFrNunonIwVYv/1bWu1WCKWhN3xn3d5zRirHPRH7JJhUsKq kwkH+eVxEpQU1lKmKQEcuDxeHZw/VWc5i1VNjA== X-Google-Smtp-Source: ABdhPJxKPMFYHuam6oE3ysnmdaKPmXcVvdn2Bl4IdLfOhvyu9U/HF8ucAh/dyuGU/eEYLlFwdzzAFp8v/IGuoot3VpU= X-Received: by 2002:aa7:cb19:: with SMTP id s25mr49263006edt.194.1629894832158; Wed, 25 Aug 2021 05:33:52 -0700 (PDT) MIME-Version: 1.0 References: <20210816074619.177383-1-wangkefeng.wang@huawei.com> <20210816074619.177383-4-wangkefeng.wang@huawei.com> In-Reply-To: From: Rob Herring Date: Wed, 25 Aug 2021 07:33:40 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 3/3] amba: Properly handle device probe without IRQ domain To: Kefeng Wang Cc: Saravana Kannan , "linux-kernel@vger.kernel.org" , Frank Rowand , devicetree@vger.kernel.org, Russell King , Linus Walleij , linux-arm-kernel , Ruizhe Lin X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210825_053354_735625_E07087D2 X-CRM114-Status: GOOD ( 36.43 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Aug 24, 2021 at 11:05 PM Kefeng Wang wrote: > > > On 2021/8/25 4:08, Saravana Kannan wrote: > > On Tue, Aug 24, 2021 at 1:05 PM Rob Herring wrote: > >> +Saravana > >> > >> Saravana mentioned to me there may be some issues with this one... > >> > >> > >> On Mon, Aug 16, 2021 at 2:43 AM Kefeng Wang wrote: > >>> of_amba_device_create() uses irq_of_parse_and_map() to translate > >>> a DT interrupt specification into a Linux virtual interrupt number. > >>> > >>> But it doesn't properly handle the case where the interrupt controller > >>> is not yet available, eg, when pl011 interrupt is connected to MBIGEN > >>> interrupt controller, because the mbigen initialization is too late, > >>> which will lead to no IRQ due to no IRQ domain found, log is shown below, > >>> "irq: no irq domain found for uart0 !" > >>> > >>> use of_irq_get() to return -EPROBE_DEFER as above, and in the function > >>> amba_device_try_add()/amba_device_add(), it will properly handle in such > >>> case, also return 0 in other fail cases to be consistent as before. > >>> > >>> Cc: Russell King > >>> Cc: Rob Herring > >>> Cc: Frank Rowand > >>> Reported-by: Ruizhe Lin > >>> Signed-off-by: Kefeng Wang > >>> --- > >>> drivers/amba/bus.c | 27 +++++++++++++++++++++++++++ > >>> drivers/of/platform.c | 6 +----- > >>> 2 files changed, 28 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > >>> index 36f2f42c8014..720aa6cdd402 100644 > >>> --- a/drivers/amba/bus.c > >>> +++ b/drivers/amba/bus.c > >>> @@ -19,6 +19,7 @@ > >>> #include > >>> #include > >>> #include > >>> +#include > >>> > >>> #include > >>> > >>> @@ -371,12 +372,38 @@ static void amba_device_release(struct device *dev) > >>> kfree(d); > >>> } > >>> > >>> +static int of_amba_device_decode_irq(struct amba_device *dev) > >>> +{ > >>> + struct device_node *node = dev->dev.of_node; > >>> + int i, irq = 0; > >>> + > >>> + if (IS_ENABLED(CONFIG_OF_IRQ) && node) { > >>> + /* Decode the IRQs and address ranges */ > >>> + for (i = 0; i < AMBA_NR_IRQS; i++) { > >>> + irq = of_irq_get(node, i); > >>> + if (irq < 0) { > >>> + if (irq == -EPROBE_DEFER) > >>> + return irq; > >>> + irq = 0; > >>> + } > >>> + > >>> + dev->irq[i] = irq; > >>> + } > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > >>> { > >>> u32 size; > >>> void __iomem *tmp; > >>> int i, ret; > >>> > >>> + ret = of_amba_device_decode_irq(dev); > >>> + if (ret) > >>> + goto err_out; > >>> + > > Similar to other resources the AMBA bus "gets" for the device, I think > > this should be moved into amba_probe() and not here. There's no reason > > to delay the addition of the device (and loading its module) because > > the IRQ isn't ready yet. > > The following code in the amba_device_try_add() will be called, it uses irq[0] > and irq[1], so I put of_amba_device_decode_irq() into amba_device_try_add(). > > 470 if (dev->irq[0]) > 471 ret = device_create_file(&dev->dev, &dev_attr_irq0); > 472 if (ret == 0 && dev->irq[1]) > 473 ret = device_create_file(&dev->dev, &dev_attr_irq1); > 474 if (ret == 0) > 475 return ret; I wonder if we could just remove these. Why does userspace need them in the first place? It's only an ABI if someone notices. Looking at the history, AMBA bus was added in 2003 with just 'irq' and then changed (ABI break) in 2004 to 'irq0' and 'irq1'. Rob [1] https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/log/arch/arm/common/amba.c _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel