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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 75D6EC433EF for ; Wed, 13 Oct 2021 09:27:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 557C760EE9 for ; Wed, 13 Oct 2021 09:27:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236501AbhJMJ3d (ORCPT ); Wed, 13 Oct 2021 05:29:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35964 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236145AbhJMJ3Y (ORCPT ); Wed, 13 Oct 2021 05:29:24 -0400 Received: from mail-ed1-x535.google.com (mail-ed1-x535.google.com [IPv6:2a00:1450:4864:20::535]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9A0A3C061570; Wed, 13 Oct 2021 02:27:20 -0700 (PDT) Received: by mail-ed1-x535.google.com with SMTP id a25so7445359edx.8; Wed, 13 Oct 2021 02:27:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=+/MfPZS60YJ0eap0AJdMs6gge+XO+BZUvon0pBTzNGA=; b=evc4kDT+QZNhO898nkNWsOvYd5uoHISP2WxjXfUrXPc/LAvzY9eu3qYsrzHLMiVRP3 7ek8KrGpW83pP3+71f6NoTqJUNew57wgLSdZESolUuO3w3SjswV4oNuZrRCsx5d2Sz6Z U6Dil/exw/Z01VwoZLKDTI9+cXYT6HoMhvbPnOyg97B0PkHUENz49VLf5qJ3twBM3fy6 bHXGjIYLA3Q6jVCBIp04R0UFvkX24frr6ijNGP3vuZmaos7c/PDqm98vd8l9QZ2VLZ00 0EBJxe8ZaQK4AqNo/Rk1lDSww7yrEMjrcnUEwQvCtD2OBAhLhOYcZrSIMXdkiq7GrIG/ lN/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=+/MfPZS60YJ0eap0AJdMs6gge+XO+BZUvon0pBTzNGA=; b=sB4NJpdNsDPkUy6mxgmKe4HtKEN2nu+cK//OpsRyWcLfqCQ31MaLagEbI4kFORFqWT DXbHANfm+km5qyLipWbKv5591O8apIQedw6yhqLc6B1Lu6kV3JmypGG4iKk2WO5NLkRa zJbI6VauabL7kp7wxsO8rh1LG//nT89gz2Z0A/Ir7ZciqzBTL295g8CrovWFVz0JNd4K KMhMg5Ydf7Xmdxs2sxIPMdWWfBLbKkwgNmHyJEe1KkptIjkS7UtgLwkWzDKnP6F1oiOm 4z+NWC8E31Hcy0XwUwG6HCd8EhncS4c00qRbwmTVYNuK5UhVy/FLe+YQo9dJvL2k0Srj swAg== X-Gm-Message-State: AOAM531TZwmZkz1ZBJo16E9AqcvamrBTbCU10ocMMZ9lBNlzUFX1/LQp +Azpq2TJkpSzwoXVZ2AYXYmrGbmWlW/LJhHRc6k= X-Google-Smtp-Source: ABdhPJzL0SqFZyZBLTfBNcE8CpMsywyY4boERpEcXyUfc9yUq7Z2mLOKk/hnon0PQ/WXXVk02NgEbEHqrWvW2H64g2s= X-Received: by 2002:a17:907:7601:: with SMTP id jx1mr40092015ejc.69.1634117238956; Wed, 13 Oct 2021 02:27:18 -0700 (PDT) MIME-Version: 1.0 References: <20211004125935.2300113-1-u.kleine-koenig@pengutronix.de> <20211012233212.GA1806189@bhelgaas> In-Reply-To: <20211012233212.GA1806189@bhelgaas> From: Andy Shevchenko Date: Wed, 13 Oct 2021 12:26:42 +0300 Message-ID: Subject: Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver To: Bjorn Helgaas Cc: =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , linux-pci , Sascha Hauer , Alexander Duyck , Alexander Shishkin , Andrew Donnellan , Andy Shevchenko , Arnaldo Carvalho de Melo , Arnd Bergmann , Benjamin Herrenschmidt , Bjorn Helgaas , Borislav Petkov , Boris Ostrovsky , "David S. Miller" , Fiona Trahe , Frederic Barrat , Giovanni Cabiddu , Greg Kroah-Hartman , Herbert Xu , "H. Peter Anvin" , Ido Schimmel , Ingo Molnar , Jack Xu , Jakub Kicinski , Jesse Brandeburg , Jiri Olsa , Jiri Pirko , Juergen Gross , Konrad Rzeszutek Wilk , Marco Chiappero , Mark Rutland , Mathias Nyman , Michael Buesch , Michael Ellerman , Namhyung Kim , "Oliver O'Halloran" , Paul Mackerras , Peter Zijlstra , =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Russell Currey , Salil Mehta , Sathya Prakash , Simon Horman , Sreekanth Reddy , Stefano Stabellini , Suganath Prabu Subramani , Taras Chornyi , Thomas Gleixner , Tomaszx Kowalik , Vadym Kochan , Wojciech Ziemba , Yisen Zhuang , Zhou Wang , linux-crypto , Linux Kernel Mailing List , linux-perf-users@vger.kernel.org, "open list:LINUX FOR POWERPC PA SEMI PWRFICIENT" , linux-scsi , USB , "open list:TI WILINK WIRELES..." , MPT-FusionLinux.pdl@broadcom.com, netdev , oss-drivers@corigine.com, qat-linux@intel.com, "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" , xen-devel@lists.xenproject.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas wrote: > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-K=C3=B6nig wrote: > I split some of the bigger patches apart so they only touched one > driver or subsystem at a time. I also updated to_pci_driver() so it > returns NULL when given NULL, which makes some of the validations > quite a bit simpler, especially in the PM code in pci-driver.c. It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC. Below are some comments as well. ... > static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsign= ed short device) > { > + struct pci_driver *drv =3D to_pci_driver(pdev->dev.driver); > const struct pci_device_id *id; > > if (pdev->vendor =3D=3D vendor && pdev->device =3D=3D device) > return true; > + for (id =3D drv ? drv->id_table : NULL; id && id->vendor; id++) > + if (id->vendor =3D=3D vendor && id->device =3D=3D device) > + break; return true; > return id && id->vendor; return false; > } ... > + afu_result =3D err_handler->error_detected(afu_de= v, > + state); One line? ... > device_lock(&vf_dev->dev); > - if (vf_dev->dev.driver) { > + if (to_pci_driver(vf_dev->dev.driver)) { Hmm... ... > + if (!pci_dev->state_saved && pci_dev->current_state !=3D = PCI_D0 > + && pci_dev->current_state !=3D PCI_UNKNOWN) { Can we keep && on the previous line? > + pci_WARN_ONCE(pci_dev, pci_dev->current_state != =3D prev, > + "PCI PM: Device state not saved by = %pS\n", > + drv->suspend); > } ... > + return drv && drv->resume ? > + drv->resume(pci_dev) : pci_pm_reenable_device(pci= _dev); One line? ... > + struct pci_driver *drv =3D to_pci_driver(dev->dev.driver); > const struct pci_error_handlers *err_handler =3D > - dev->dev.driver ? to_pci_driver(dev->dev.driver)-= >err_handler : NULL; > + drv ? drv->err_handler : NULL; Isn't dev->driver =3D=3D to_pci_driver(dev->dev.driver)? ... > + struct pci_driver *drv =3D to_pci_driver(dev->dev.driver); > const struct pci_error_handlers *err_handler =3D > - dev->dev.driver ? to_pci_driver(dev->dev.driver)-= >err_handler : NULL; > + drv ? drv->err_handler : NULL; Ditto. ... > device_lock(&dev->dev); > + pdrv =3D to_pci_driver(dev->dev.driver); > if (!pci_dev_set_io_state(dev, state) || > - !dev->dev.driver || > - !(pdrv =3D to_pci_driver(dev->dev.driver))->err_handler |= | > + !pdrv || > + !pdrv->err_handler || One line now? > !pdrv->err_handler->error_detected) { Or this and the previous line? ... > + pdrv =3D to_pci_driver(dev->dev.driver); > + if (!pdrv || > + !pdrv->err_handler || > !pdrv->err_handler->mmio_enabled) > goto out; Ditto. ... > + pdrv =3D to_pci_driver(dev->dev.driver); > + if (!pdrv || > + !pdrv->err_handler || > !pdrv->err_handler->slot_reset) > goto out; Ditto. ... > if (!pci_dev_set_io_state(dev, pci_channel_io_normal) || > - !dev->dev.driver || > - !(pdrv =3D to_pci_driver(dev->dev.driver))->err_handler |= | > + !pdrv || > + !pdrv->err_handler || > !pdrv->err_handler->resume) > goto out; Ditto. > - result =3D PCI_ERS_RESULT_NONE; > > pcidev =3D pci_get_domain_bus_and_slot(domain, bus, devfn); > if (!pcidev || !pcidev->dev.driver) { > dev_err(&pdev->xdev->dev, "device or AER driver is NULL\n= "); > pci_dev_put(pcidev); > - return result; > + return PCI_ERS_RESULT_NONE; > } > pdrv =3D to_pci_driver(pcidev->dev.driver); What about splitting the conditional to two with clear error message in each and use pci_err() in the second one? ... > default: > dev_err(&pdev->xdev->dev, > - "bad request in aer recovery " > - "operation!\n"); > + "bad request in AER recovery operation!\n= "); Stray change? Or is it in a separate patch in your tree? --=20 With Best Regards, Andy Shevchenko 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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 102EAC4332F for ; Wed, 13 Oct 2021 09:33:51 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (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 6C94A600CD for ; Wed, 13 Oct 2021 09:33:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 6C94A600CD Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4HTnR10kS3z3cC5 for ; Wed, 13 Oct 2021 20:33:49 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20210112 header.b=evc4kDT+; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gmail.com (client-ip=2a00:1450:4864:20::535; helo=mail-ed1-x535.google.com; envelope-from=andy.shevchenko@gmail.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20210112 header.b=evc4kDT+; dkim-atps=neutral Received: from mail-ed1-x535.google.com (mail-ed1-x535.google.com [IPv6:2a00:1450:4864:20::535]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4HTnHf3vSfz2ymP for ; Wed, 13 Oct 2021 20:27:24 +1100 (AEDT) Received: by mail-ed1-x535.google.com with SMTP id z20so7341548edc.13 for ; Wed, 13 Oct 2021 02:27:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=+/MfPZS60YJ0eap0AJdMs6gge+XO+BZUvon0pBTzNGA=; b=evc4kDT+QZNhO898nkNWsOvYd5uoHISP2WxjXfUrXPc/LAvzY9eu3qYsrzHLMiVRP3 7ek8KrGpW83pP3+71f6NoTqJUNew57wgLSdZESolUuO3w3SjswV4oNuZrRCsx5d2Sz6Z U6Dil/exw/Z01VwoZLKDTI9+cXYT6HoMhvbPnOyg97B0PkHUENz49VLf5qJ3twBM3fy6 bHXGjIYLA3Q6jVCBIp04R0UFvkX24frr6ijNGP3vuZmaos7c/PDqm98vd8l9QZ2VLZ00 0EBJxe8ZaQK4AqNo/Rk1lDSww7yrEMjrcnUEwQvCtD2OBAhLhOYcZrSIMXdkiq7GrIG/ lN/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=+/MfPZS60YJ0eap0AJdMs6gge+XO+BZUvon0pBTzNGA=; b=CAovqLFK/ssD81qSwUimhm4cOKNOkBZdIzsM/XaeBtCSswn88C4kepfJB9mVG5dfSb MDXGfudtFhhjoiquHVEF1JYzuivLk2mq0WB4QSy8/mr0/G5nUwmWs5BLCBWbylCwCzE4 /ZCZTR2mSOAKI49rkOmwad8Qik7FiWFTM+aur6cMeSfHAOe1pJDj+Nudk/oR4EcDaWt3 QtYe8c2D6BAXQ50nkB5EpeF+g/10L5Q10hPF+oBsVxbuKeaFeHdK1IbsBqoOzwQdIeq9 ISMBMDYoUaPY5EIVtwVEomBPbXCNoUlJf85cTroRIYNu0mGyWqOd2CD4xefN1IC+/MY/ HHSw== X-Gm-Message-State: AOAM532kDt/b+bLVuw2THUZxcy2w+Hi+d+M76SLGrlixOTya3tfUS77s FmMZq5oVytoLbfpe6pj8sjI6xZcZcvshqa1MfgY= X-Google-Smtp-Source: ABdhPJzL0SqFZyZBLTfBNcE8CpMsywyY4boERpEcXyUfc9yUq7Z2mLOKk/hnon0PQ/WXXVk02NgEbEHqrWvW2H64g2s= X-Received: by 2002:a17:907:7601:: with SMTP id jx1mr40092015ejc.69.1634117238956; Wed, 13 Oct 2021 02:27:18 -0700 (PDT) MIME-Version: 1.0 References: <20211004125935.2300113-1-u.kleine-koenig@pengutronix.de> <20211012233212.GA1806189@bhelgaas> In-Reply-To: <20211012233212.GA1806189@bhelgaas> From: Andy Shevchenko Date: Wed, 13 Oct 2021 12:26:42 +0300 Message-ID: Subject: Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver To: Bjorn Helgaas Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailman-Approved-At: Wed, 13 Oct 2021 20:30:31 +1100 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Giovanni Cabiddu , Mark Rutland , Sathya Prakash , Alexander Shishkin , Alexander Duyck , "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , qat-linux@intel.com, oss-drivers@corigine.com, Oliver O'Halloran , "H. Peter Anvin" , Jiri Olsa , Thomas Gleixner , Marco Chiappero , Stefano Stabellini , Herbert Xu , linux-scsi , =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Jesse Brandeburg , Peter Zijlstra , Ingo Molnar , linux-pci , "open list:TI WILINK WIRELES..." , Jakub Kicinski , Yisen Zhuang , Suganath Prabu Subramani , Fiona Trahe , Andrew Donnellan , Arnd Bergmann , Konrad Rzeszutek Wilk , Ido Schimmel , =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , Simon Horman , "open list:LINUX FOR POWERPC PA SEMI PWRFICIENT" , Arnaldo Carvalho de Melo , Jack Xu , Borislav Petkov , Michael Buesch , Jiri Pirko , Bjorn Helgaas , Namhyung Kim , Boris Ostrovsky , Andy Shevchenko , Juergen Gross , Salil Mehta , Sreekanth Reddy , xen-devel@lists.xenproject.org, Vadym Kochan , MPT-FusionLinux.pdl@broadcom.com, Greg Kroah-Hartman , USB , Wojciech Ziemba , Linux Kernel Mailing List , Mathias Nyman , Zhou Wang , linux-crypto , Sascha Hauer , netdev , Frederic Barrat , Paul Mackerras , Tomaszx Kowalik , Taras Chornyi , "David S. Miller" , linux-perf-users@vger.kernel.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas wrote: > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-K=C3=B6nig wrote: > I split some of the bigger patches apart so they only touched one > driver or subsystem at a time. I also updated to_pci_driver() so it > returns NULL when given NULL, which makes some of the validations > quite a bit simpler, especially in the PM code in pci-driver.c. It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC. Below are some comments as well. ... > static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsign= ed short device) > { > + struct pci_driver *drv =3D to_pci_driver(pdev->dev.driver); > const struct pci_device_id *id; > > if (pdev->vendor =3D=3D vendor && pdev->device =3D=3D device) > return true; > + for (id =3D drv ? drv->id_table : NULL; id && id->vendor; id++) > + if (id->vendor =3D=3D vendor && id->device =3D=3D device) > + break; return true; > return id && id->vendor; return false; > } ... > + afu_result =3D err_handler->error_detected(afu_de= v, > + state); One line? ... > device_lock(&vf_dev->dev); > - if (vf_dev->dev.driver) { > + if (to_pci_driver(vf_dev->dev.driver)) { Hmm... ... > + if (!pci_dev->state_saved && pci_dev->current_state !=3D = PCI_D0 > + && pci_dev->current_state !=3D PCI_UNKNOWN) { Can we keep && on the previous line? > + pci_WARN_ONCE(pci_dev, pci_dev->current_state != =3D prev, > + "PCI PM: Device state not saved by = %pS\n", > + drv->suspend); > } ... > + return drv && drv->resume ? > + drv->resume(pci_dev) : pci_pm_reenable_device(pci= _dev); One line? ... > + struct pci_driver *drv =3D to_pci_driver(dev->dev.driver); > const struct pci_error_handlers *err_handler =3D > - dev->dev.driver ? to_pci_driver(dev->dev.driver)-= >err_handler : NULL; > + drv ? drv->err_handler : NULL; Isn't dev->driver =3D=3D to_pci_driver(dev->dev.driver)? ... > + struct pci_driver *drv =3D to_pci_driver(dev->dev.driver); > const struct pci_error_handlers *err_handler =3D > - dev->dev.driver ? to_pci_driver(dev->dev.driver)-= >err_handler : NULL; > + drv ? drv->err_handler : NULL; Ditto. ... > device_lock(&dev->dev); > + pdrv =3D to_pci_driver(dev->dev.driver); > if (!pci_dev_set_io_state(dev, state) || > - !dev->dev.driver || > - !(pdrv =3D to_pci_driver(dev->dev.driver))->err_handler |= | > + !pdrv || > + !pdrv->err_handler || One line now? > !pdrv->err_handler->error_detected) { Or this and the previous line? ... > + pdrv =3D to_pci_driver(dev->dev.driver); > + if (!pdrv || > + !pdrv->err_handler || > !pdrv->err_handler->mmio_enabled) > goto out; Ditto. ... > + pdrv =3D to_pci_driver(dev->dev.driver); > + if (!pdrv || > + !pdrv->err_handler || > !pdrv->err_handler->slot_reset) > goto out; Ditto. ... > if (!pci_dev_set_io_state(dev, pci_channel_io_normal) || > - !dev->dev.driver || > - !(pdrv =3D to_pci_driver(dev->dev.driver))->err_handler |= | > + !pdrv || > + !pdrv->err_handler || > !pdrv->err_handler->resume) > goto out; Ditto. > - result =3D PCI_ERS_RESULT_NONE; > > pcidev =3D pci_get_domain_bus_and_slot(domain, bus, devfn); > if (!pcidev || !pcidev->dev.driver) { > dev_err(&pdev->xdev->dev, "device or AER driver is NULL\n= "); > pci_dev_put(pcidev); > - return result; > + return PCI_ERS_RESULT_NONE; > } > pdrv =3D to_pci_driver(pcidev->dev.driver); What about splitting the conditional to two with clear error message in each and use pci_err() in the second one? ... > default: > dev_err(&pdev->xdev->dev, > - "bad request in aer recovery " > - "operation!\n"); > + "bad request in AER recovery operation!\n= "); Stray change? Or is it in a separate patch in your tree? --=20 With Best Regards, Andy Shevchenko