Received: by 2002:a05:6a10:16a7:0:0:0:0 with SMTP id gp39csp1886054pxb; Sat, 21 Nov 2020 01:23:49 -0800 (PST) X-Google-Smtp-Source: ABdhPJxfeKVwKdRN6wZkwM1l9Uco86F570NX9w1qfUaLTo3Abtg+i1/yJzXPvuH8zyTH+xkBN8QD X-Received: by 2002:a17:906:888b:: with SMTP id ak11mr35399172ejc.278.1605950629132; Sat, 21 Nov 2020 01:23:49 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1605950629; cv=none; d=google.com; s=arc-20160816; b=JUe/bFyWqOuL+v09OvbmaLbNHyJmqRhzp5Kr1Rm3sD2W0kZMJLk3nA07xwmunjd7YH lGtJ3gBC7TluZDmGcUZwKwRkWi0BPwPUfcxyYtp25IHUTmKX4+0enaliSWB3gxOwhb1f z0D2ZxSfK03SdKXEotA+Yz2HIm9NgDZDsX1pI9/NpsYFdpQLqsOcel99QKY9hGiG70UK 3W0Iuo+S8oq60rO8ulPYQEqNw3UZlIEoH8urIojTB7MABpfTQxSP7PflnkCJn+/lvf/w zqhAABeLaYjfOoZ1RdhIpK7W6MxhCDYa8iLA857NGxQovFbxnbA0a0VDVO3W1QEHyf7v YFZg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version; bh=WaMha0En6LkvNa0zqCPwgKvusypP+GNCRLE4znVs274=; b=alCP9RZLFtVn+BSgC+PSmu6UMwMOLixPMJibpn3vVxLjfFd3x6bbuIk8hMQLMcYM5a dallQL01CN0NZv3LpZQYkGq0EHL0NwoK2o0wL836GaCCI4JEVlS3KFEy8ukM5v5cmN0a DWSDlrFusbeUyk0D99sS3GIEuX2KA2bGToGy+oj2uS1gGlQ5SyLoEHsEVLdsGLtyEtQJ +BojsEvg40rcD/lyIc2PshLzpCWd8WaZr5obLeG3i3tGV962XxYgHwFeKJcc0vMBGL4s w2vtMgQvKVNGIRObtnQwQ+bf/eFDGwOLFk3VT71o601XOKhbfFHjO2oNkHoONDNmJt7Q 8OdA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id dx19si67434ejb.535.2020.11.21.01.23.06; Sat, 21 Nov 2020 01:23:49 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727082AbgKUJSm (ORCPT + 99 others); Sat, 21 Nov 2020 04:18:42 -0500 Received: from mail-oo1-f66.google.com ([209.85.161.66]:45006 "EHLO mail-oo1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726854AbgKUJSj (ORCPT ); Sat, 21 Nov 2020 04:18:39 -0500 Received: by mail-oo1-f66.google.com with SMTP id i13so2791892oou.11; Sat, 21 Nov 2020 01:18:38 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=WaMha0En6LkvNa0zqCPwgKvusypP+GNCRLE4znVs274=; b=YjKYBzAT59/F26ZJYCuI2A7KEdcHpHvLTL1Keq6hC+VwHnHtbE/zMwgl/lVqMaifvO Sp+qR6ciylkPXgGto4SSGrtbxR1SHu5E0mJGKNLDB5BBHn+xdEOXN6omoD69kZUc0x2D q3b+z6vzPgeYR0PYYHaDXOq0ss1Cp6rDWRqQGzzhcOMoqDTEccyEnQ4Sr6JGN9me+qxM D8O7yChJ3bXm4pMkeDA6Ns02ROC3MGtucRuIZJsE9nCevdrnSSI7fbTfQf73I+KMzQCk vYPOG7eIj+Fy4KgutbABeOdi9Fg8KRL72AGnMVTIEZ0OGE/JRoo5Rsvu2iq0ja05qNes GS0A== X-Gm-Message-State: AOAM533kv1O2FX2r3lE875K2APLPOY5FyCtK4S62TXlOW7MgjKOzErJj gbMFejj8B6LbDD0rMTPEsvUpnm6earFentM1o2o= X-Received: by 2002:a4a:d694:: with SMTP id i20mr16921417oot.40.1605950318178; Sat, 21 Nov 2020 01:18:38 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Geert Uytterhoeven Date: Sat, 21 Nov 2020 10:18:26 +0100 Message-ID: Subject: Re: [PATCH] m68k: Fix WARNING splat in pmac_zilog driver To: Finn Thain Cc: Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Joshua Thompson , Greg Kroah-Hartman , Jiri Slaby , stable , linux-m68k , Linux Kernel Mailing List , "open list:SERIAL DRIVERS" , linuxppc-dev Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Finn, On Sat, Nov 21, 2020 at 12:47 AM Finn Thain wrote: > On Fri, 20 Nov 2020, Geert Uytterhoeven wrote: > > On Fri, Nov 20, 2020 at 5:51 AM Finn Thain wrote: > > > Don't add platform resources that won't be used. This avoids a > > > recently-added warning from the driver core, that can show up on a > > > multi-platform kernel when !MACH_IS_MAC. > > > > > > ------------[ cut here ]------------ > > > WARNING: CPU: 0 PID: 0 at drivers/base/platform.c:224 platform_get_irq_optional+0x8e/0xce > > > 0 is an invalid IRQ number > > > Modules linked in: > > > CPU: 0 PID: 0 Comm: swapper Not tainted 5.9.0-multi #1 > > > Stack from 004b3f04: > > > 004b3f04 00462c2f 00462c2f 004b3f20 0002e128 004754db 004b6ad4 004b3f4c > > > 0002e19c 004754f7 000000e0 00285ba0 00000009 00000000 004b3f44 ffffffff > > > 004754db 004b3f64 004b3f74 00285ba0 004754f7 000000e0 00000009 004754db > > > 004fdf0c 005269e2 004fdf0c 00000000 004b3f88 00285cae 004b6964 00000000 > > > 004fdf0c 004b3fac 0051cc68 004b6964 00000000 004b6964 00000200 00000000 > > > 0051cc3e 0023c18a 004b3fc0 0051cd8a 004fdf0c 00000002 0052b43c 004b3fc8 > > > Call Trace: [<0002e128>] __warn+0xa6/0xd6 > > > [<0002e19c>] warn_slowpath_fmt+0x44/0x76 > > > [<00285ba0>] platform_get_irq_optional+0x8e/0xce > > > [<00285ba0>] platform_get_irq_optional+0x8e/0xce > > > [<00285cae>] platform_get_irq+0x12/0x4c > > > [<0051cc68>] pmz_init_port+0x2a/0xa6 > > > [<0051cc3e>] pmz_init_port+0x0/0xa6 > > > [<0023c18a>] strlen+0x0/0x22 > > > [<0051cd8a>] pmz_probe+0x34/0x88 > > > [<0051cde6>] pmz_console_init+0x8/0x28 > > > [<00511776>] console_init+0x1e/0x28 > > > [<0005a3bc>] printk+0x0/0x16 > > > [<0050a8a6>] start_kernel+0x368/0x4ce > > > [<005094f8>] _sinittext+0x4f8/0xc48 > > > random: get_random_bytes called from print_oops_end_marker+0x56/0x80 with crng_init=0 > > > ---[ end trace 392d8e82eed68d6c ]--- > > > > > > Cc: Michael Ellerman > > > Cc: Benjamin Herrenschmidt > > > Cc: Paul Mackerras > > > Cc: Joshua Thompson > > > Cc: Greg Kroah-Hartman > > > Cc: Jiri Slaby > > > Cc: stable@vger.kernel.org # v5.8+ > > > References: commit a85a6c86c25b ("driver core: platform: Clarify that IRQ 0 is invalid") > > > Reported-by: Laurent Vivier > > > Signed-off-by: Finn Thain > > > --- > > > The global platform_device structs provide the equivalent of a direct > > > search of the OpenFirmware tree, for platforms that don't have OF. > > > The purpose of that search is discussed in the comments in pmac_zilog.c: > > > > > > * First, we need to do a direct OF-based probe pass. We > > > * do that because we want serial console up before the > > > * macio stuffs calls us back > > > > > > The actual platform bus matching takes place later, with a module_initcall, > > > following the usual pattern. > > > > I think it would be good for this explanation to be part of the > > actual patch description above. > > > > Thanks for your review. > > I take that explanation as read because it was fundamental to the changes > I made to pmac_zilog.c back in 2009 with commit ec9cbe09899e ("pmac-zilog: > add platform driver"). That's a long time ago ;-) I asked because to the casual reader, it's far from obvious why the platform device use-time is different from the platform device's resources use-time. > IMO, being that it isn't news, it doesn't belong in the changelog. > However, I agree that it needs to be documented. How about I add a comment > to pmac_zilog.c? Fine for me. > > > --- a/drivers/tty/serial/pmac_zilog.c > > > +++ b/drivers/tty/serial/pmac_zilog.c > > > @@ -1697,18 +1697,17 @@ extern struct platform_device scc_a_pdev, scc_b_pdev; > > > > > > static int __init pmz_init_port(struct uart_pmac_port *uap) > > > { > > > - struct resource *r_ports; > > > - int irq; > > > + struct resource *r_ports, *r_irq; > > > > > > r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0); > > > - irq = platform_get_irq(uap->pdev, 0); > > > - if (!r_ports || irq <= 0) > > > + r_irq = platform_get_resource(uap->pdev, IORESOURCE_IRQ, 0); > > > + if (!r_ports || !r_irq) > > > return -ENODEV; > > > > > > uap->port.mapbase = r_ports->start; > > > uap->port.membase = (unsigned char __iomem *) r_ports->start; > > > uap->port.iotype = UPIO_MEM; > > > - uap->port.irq = irq; > > > + uap->port.irq = r_irq->start; > > > uap->port.uartclk = ZS_CLOCK; > > > uap->port.fifosize = 1; > > > uap->port.ops = &pmz_pops; > > > > Given the resources are no longer present on !MAC, just doing > > > > r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0); > > + if (!r_ports) > > + return -ENODEV; > > irq = platform_get_irq(uap->pdev, 0); > > > > should be sufficient? > > I think your suggestion is shorter but not better. Commit a85a6c86c25b > (which introduced the WARNING) suggests that testing for irq == 0 is > undesirable. My patch resolves that. > > As a bonus, by simply testing for the existence of both resources, I've > addressed the mistake I made when I originally added the slick > platform_get_irq() call instead of consistently using > platform_get_resource(). > > platform_get_irq() hides a bunch of architecture-specific logic that is > not appropriate here. The WARNING itself is a good example of that kind of > logic. > > Do you agree? If so, I will add this explanation to the commit log. OK, your main motivation is to get rid of the zero-check. Leaving it could indeed trigger some janitorial changes by people who don't understand the code at all, so it's good to avoid that ;-) Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds