From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AH8x226+kxQrg+9tYvDPmPAcg5rfDfQ1M55UZcft4vB8mEoaLxee3lv19xKMSkRVHYTLAelTAWxy ARC-Seal: i=1; a=rsa-sha256; t=1519711587; cv=none; d=google.com; s=arc-20160816; b=qvzVMAOgsdOfGGryT0Hu0smZp/2EMRnkAo5b4xShygzqryiFqGTydnsiPickqOEsg0 XeL3VTxDhOIW4KSRwCvlsJyt8ZnrOsyYhwPx8IIyAzloJ9DBm1lpNRw8KEF9p/pq/I5K Cmx1zxKwfaPEkIlVOqjsfTFW2Bs71XfTIZO3DUI/QNqZ7Vy3CyT/KCJGUycyA91UKCDS 3yo+NCi2uJOsxSI+oM+JYdBSd1RSLuVOYl9IIorBH6JdSZbYvQgCQNO4yhFz4BFVG2Cn 0Twmrx9NagD6bZfv75mYOGzJXsbhJBaUbmuTqJsv13odrzL8xJW8eEXmu+vgq9eCcHpu 20cg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:message-id:references:in-reply-to:subject:cc:to:from :date:content-transfer-encoding:mime-version:dkim-signature :dkim-signature:arc-authentication-results; bh=tnkErdlrKI57NClZVyHZG7u58230G2FnvKvCPUsb1MQ=; b=tgHsL4PuFAff8pWRQ8Jd8qy7OhxOyTMyJp0lokcPWoxip/+L+xZExE4TsAFlyhjS1r hZmiTAsMViBMfzMzOPic4+1/GZDg8f0XbNxMhTRcS5wIU2UybDxU474xPBnIBcAbUWw5 g9QgNKlsC6bnVREUc+YGx4VxDE0L9m73XhDJh06VB8bmL6Uy+5B6XRg3CeprNYtlVIsp PkOsE2DmVJTSd5+O/e9c19MFMKgTTZe2jobApst6nXPFA73Cl/WynGlUz8+O843DFt/F ZMquJ/lIhgsnuktTFYAgDZ/GD8RmcmNrJFzXV4suxVIvjnqY/4NZeDzy376w76f7ADRb AKLA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=Qc0qU8Si; dkim=pass header.i=@codeaurora.org header.s=default header.b=kSKZ2eps; spf=pass (google.com: domain of poza@codeaurora.org designates 198.145.29.96 as permitted sender) smtp.mailfrom=poza@codeaurora.org Authentication-Results: mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=Qc0qU8Si; dkim=pass header.i=@codeaurora.org header.s=default header.b=kSKZ2eps; spf=pass (google.com: domain of poza@codeaurora.org designates 198.145.29.96 as permitted sender) smtp.mailfrom=poza@codeaurora.org MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 27 Feb 2018 11:36:25 +0530 From: poza@codeaurora.org To: Bjorn Helgaas Cc: Bjorn Helgaas , Philippe Ombredanne , Thomas Gleixner , Greg Kroah-Hartman , Kate Stewart , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, Dongdong Liu , Keith Busch , Wei Zhang , Sinan Kaya , Timur Tabi Subject: Re: [PATCH v11 4/7] PCI/DPC: Unify and plumb error handling into DPC In-Reply-To: <20180224000754.GR14632@bhelgaas-glaptop.roam.corp.google.com> References: <1519374244-20539-1-git-send-email-poza@codeaurora.org> <1519374244-20539-5-git-send-email-poza@codeaurora.org> <20180224000754.GR14632@bhelgaas-glaptop.roam.corp.google.com> Message-ID: User-Agent: Roundcube Webmail/1.2.5 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1593086415695868818?= X-GMAIL-MSGID: =?utf-8?q?1593533098292077500?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On 2018-02-24 05:37, Bjorn Helgaas wrote: > On Fri, Feb 23, 2018 at 01:54:01PM +0530, Oza Pawandeep wrote: >> Current DPC driver does not do recovery, e.g. calling end-point's >> driver's >> callbacks, which sanitize the sw. >> >> DPC driver implements link_reset callback, and calls pcie_do_recovery. > > s/pcie_do_recovery/pcie_do_recovery()/ > sure. >> Signed-off-by: Oza Pawandeep >> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index abc514e..f8575da 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -343,6 +343,8 @@ static inline resource_size_t >> pci_resource_alignment(struct pci_dev *dev, >> void pci_enable_acs(struct pci_dev *dev); >> >> /* PCI error reporting and recovery */ >> +#define DPC_FATAL 4 > > This needs to go next to the AER_FATAL, etc., definitions because > DPC_FATAL shares the namespace and they all need to have distinct > values. I can't tell from this patch whether they do or not. > sure. >> void pcie_do_recovery(struct pci_dev *dev, int severity); >> >> #ifdef CONFIG_PCIEASPM >> diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c >> index 38e40c6..5c01c63 100644 >> --- a/drivers/pci/pcie/pcie-dpc.c >> +++ b/drivers/pci/pcie/pcie-dpc.c >> @@ -13,6 +13,7 @@ >> #include >> #include "../pci.h" >> #include "aer/aerdrv.h" >> +#include "portdrv.h" >> >> struct dpc_dev { >> struct pcie_device *dev; >> @@ -45,6 +46,58 @@ struct dpc_dev { >> "Memory Request Completion Timeout", /* Bit Position 18 */ >> }; >> >> +static int find_dpc_dev_iter(struct device *device, void *data) >> +{ >> + struct pcie_port_service_driver *service_driver; >> + struct device **dev = (struct device **) data;; >> + >> + if (device->bus == &pcie_port_bus_type && device->driver) { >> + service_driver = to_service_driver(device->driver); >> + if (service_driver->service == PCIE_PORT_SERVICE_DPC) { >> + *dev = device; >> + return 1; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static struct device *pci_find_dpc_dev(struct pci_dev *pdev) >> +{ >> + struct device *dev = NULL; >> + >> + device_for_each_child(&pdev->dev, &dev, find_dpc_dev_iter); >> + >> + return dev; >> +} > > Ugh. You're not responsible for this and you don't need to do > anything, but hanging the struct dpc_dev off the struct pcie_device > and then having to grub around like this to locate it from the pci_dev > is just ... clunky. OK, rant over, sorry :) > :) I keep it for now. >> +static int find_dpc_service_iter(struct device *device, void *data) >> +{ >> + struct pcie_port_service_driver *service_driver; >> + struct pcie_port_service_driver **drv = >> + (struct pcie_port_service_driver **) data; >> + >> + if (device->bus == &pcie_port_bus_type && device->driver) { >> + service_driver = to_service_driver(device->driver); >> + if (service_driver->service == PCIE_PORT_SERVICE_DPC) { >> + *drv = service_driver; >> + return 1; >> + } >> + } >> + >> + return 0; >> +} >> + >> +struct pcie_port_service_driver *pci_find_dpc_service(struct pci_dev >> *dev) >> +{ >> + struct pcie_port_service_driver *drv = NULL; >> + >> + device_for_each_child(&dev->dev, &drv, find_dpc_service_iter); >> + >> + return drv; >> +} >> +EXPORT_SYMBOL_GPL(pci_find_dpc_service); > > No module uses this, so it doesn't need to be exported. > > This is a clone of find_aer_service(). Can you add a preliminary patch > to > make a generic "find service" interface that accepts the service type > (PCIE_PORT_SERVICE_AER, PCIE_PORT_SERVICE_DPC) as a parameter? > > This whole "find service" thing is ugly as sin. You're not responsible > for > cleaning it up, but maybe we can at least limit the proliferation of > it. > I have taken care of making this as a generic find_Service in pcie port driver now. >> static int dpc_wait_rp_inactive(struct dpc_dev *dpc) >> { >> unsigned long timeout = jiffies + HZ; >> @@ -82,12 +135,25 @@ static void dpc_wait_link_inactive(struct dpc_dev >> *dpc) >> dev_warn(dev, "Link state not disabled for DPC event\n"); >> } >> >> -static void dpc_work(struct work_struct *work) >> +/** >> + * dpc_reset_link - reset link DPC routine > > s/ / / (remove extra space) sure. > >> + * @dev: pointer to Root Port's pci_dev data structure >> + * >> + * Invoked by Port Bus driver when performing link reset at Root >> Port. >> + */ >> +static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev) >> { >> - struct dpc_dev *dpc = container_of(work, struct dpc_dev, work); >> - struct pci_dev *dev, *temp, *pdev = dpc->dev->port; >> struct pci_bus *parent = pdev->subordinate; >> - u16 cap = dpc->cap_pos, ctl; >> + struct pci_dev *dev, *temp; >> + struct dpc_dev *dpc; >> + struct pcie_device *pciedev; >> + struct device *devdpc; >> + u16 cap, ctl; >> + >> + devdpc = pci_find_dpc_dev(pdev); >> + pciedev = to_pcie_device(devdpc); >> + dpc = get_service_data(pciedev); >> + cap = dpc->cap_pos; >> >> pci_lock_rescan_remove(); >> list_for_each_entry_safe_reverse(dev, temp, &parent->devices, >> @@ -104,21 +170,31 @@ static void dpc_work(struct work_struct *work) >> >> dpc_wait_link_inactive(dpc); >> if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc)) >> - return; >> + return PCI_ERS_RESULT_DISCONNECT; >> if (dpc->rp_extensions && dpc->rp_pio_status) { >> pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, >> dpc->rp_pio_status); >> dpc->rp_pio_status = 0; >> } >> >> - pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS, >> + pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, >> PCI_EXP_DPC_STATUS_TRIGGER | PCI_EXP_DPC_STATUS_INTERRUPT); >> >> pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl); >> pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL, >> - ctl | PCI_EXP_DPC_CTL_INT_EN); >> + ctl | PCI_EXP_DPC_CTL_INT_EN); > > Align "ctl" with "pdev". sure. > >> + return PCI_ERS_RESULT_RECOVERED; >> } >> >> +static void dpc_work(struct work_struct *work) >> +{ >> + struct dpc_dev *dpc = container_of(work, struct dpc_dev, work); >> + struct pci_dev *pdev = dpc->dev->port; >> + >> + /* From DPC point of view error is always FATAL. */ >> + pcie_do_recovery(pdev, DPC_FATAL); >> +} >> static void dpc_process_rp_pio_error(struct dpc_dev *dpc) >> { >> struct device *dev = &dpc->dev->device; >> @@ -297,6 +373,7 @@ static void dpc_remove(struct pcie_device *dev) >> .service = PCIE_PORT_SERVICE_DPC, >> .probe = dpc_probe, >> .remove = dpc_remove, >> + .reset_link = dpc_reset_link, >> }; >> >> static int __init dpc_service_init(void) >> diff --git a/drivers/pci/pcie/pcie-err.c b/drivers/pci/pcie/pcie-err.c >> index f830975..1ea4b9a 100644 >> --- a/drivers/pci/pcie/pcie-err.c >> +++ b/drivers/pci/pcie/pcie-err.c >> @@ -19,6 +19,7 @@ >> #include >> #include >> #include "portdrv.h" >> +#include "./../pci.h" >> >> static DEFINE_MUTEX(pci_err_recovery_lock); >> >> @@ -181,7 +182,7 @@ static pci_ers_result_t default_reset_link(struct >> pci_dev *dev) >> return PCI_ERS_RESULT_RECOVERED; >> } >> >> -static pci_ers_result_t reset_link(struct pci_dev *dev) >> +static pci_ers_result_t reset_link(struct pci_dev *dev, int severity) >> { >> struct pci_dev *udev; >> pci_ers_result_t status; >> @@ -195,9 +196,17 @@ static pci_ers_result_t reset_link(struct pci_dev >> *dev) >> udev = dev->bus->self; >> } >> >> + >> + /* Use the service driver of the component firstly */ >> +#if IS_ENABLED(CONFIG_PCIE_DPC) > > #ifdef CONFIG_PCIE_DPC > >> + if (severity == DPC_FATAL) >> + driver = pci_find_dpc_service(udev); >> +#endif >> #if IS_ENABLED(CONFIG_PCIEAER) >> - /* Use the aer driver of the component firstly */ >> - driver = pci_find_aer_service(udev); >> + if (severity == AER_FATAL || >> + severity == AER_NONFATAL || >> + severity == AER_CORRECTABLE) > > This change (to check for AER_FATAL, etc) looks like it belongs in a > different patch. This patch doesn't change any places that set the > severity. I have made generic service and probably now it does not look like this. > >> + driver = pci_find_aer_service(udev); >> #endif >> >> if (driver && driver->reset_link) { >> @@ -287,7 +296,8 @@ void pcie_do_recovery(struct pci_dev *dev, int >> severity) >> >> mutex_lock(&pci_err_recovery_lock); >> >> - if (severity == AER_FATAL) >> + if (severity == AER_FATAL || >> + severity == DPC_FATAL) >> state = pci_channel_io_frozen; >> else >> state = pci_channel_io_normal; >> @@ -297,10 +307,14 @@ void pcie_do_recovery(struct pci_dev *dev, int >> severity) >> "error_detected", >> report_error_detected); >> >> - if (severity == AER_FATAL) { >> - result = reset_link(dev); >> + if (severity == AER_FATAL || >> + severity == DPC_FATAL) { >> + result = reset_link(dev, severity); >> if (result != PCI_ERS_RESULT_RECOVERED) >> goto failed; >> + else if (severity == DPC_FATAL) >> + goto resume; >> + >> } >> >> if (status == PCI_ERS_RESULT_CAN_RECOVER) >> @@ -324,6 +338,7 @@ void pcie_do_recovery(struct pci_dev *dev, int >> severity) >> if (status != PCI_ERS_RESULT_RECOVERED) >> goto failed; >> >> +resume: >> broadcast_error_message(dev, >> state, >> "resume", >> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h >> index 4f1992d..b013e24 100644 >> --- a/drivers/pci/pcie/portdrv.h >> +++ b/drivers/pci/pcie/portdrv.h >> @@ -80,4 +80,5 @@ static inline void pcie_port_platform_notify(struct >> pci_dev *port, int *mask){} >> #endif /* !CONFIG_ACPI */ >> >> struct pcie_port_service_driver *pci_find_aer_service(struct pci_dev >> *dev); >> +struct pcie_port_service_driver *pci_find_dpc_service(struct pci_dev >> *dev); >> #endif /* _PORTDRV_H_ */ >> -- >> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm >> Technologies, Inc., >> a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a >> Linux Foundation Collaborative Project. >>