Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On 1/2/2018 12:12 PM, Keith Busch wrote: > On Tue, Jan 02, 2018 at 08:25:08AM -0500, Sinan Kaya wrote: >>> 2. A DPC event suppresses the error message required for the Linux >>> AER driver to run. How can AER and DPC run concurrently? >>> >> >> As we briefly discussed in previous email exchanges, I think you are >> looking at a use case with a switch that supports DPC functionality. > > No, I'm interested in DPC in a general. > >> Oza and I are looking at a root port functionality with DPC feature. >> >> As you already know, AER errors are logged to AER capability register >> independent of the DPC driver presence. > > The error is noted in the Uncorrectable Error Status Register if that's > what triggered the DPC event. This register has nothing to do with the > Root Error Status Register, which is required to have received an error > Message in order to have a status for the AER driver. > >> A root port is also allowed to share the MSI interrupts across DPC and >> AER. >> >> Therefore, when a DPC interrupt fires; both AER driver and DPC driver >> starts recovery work. This is the issue we are trying to deal with. > > If DPC is implemented correctly, the AER Root Status can't have an > uncorrectable status for the driver to deal with. The only thing the AER > driver could possibly see is a correctable error if DPC ERR_COR Enable > is set. > >> In the end, the driver needs to work for both root port and switches. >> I think you verified it against a switch. We are doing the same for a >> root port and submitting the plumbing code. > > I think we need to consider the possibility you are enabling a platform > that implemented DPC incorrectly. There's nothing in the specification > that says that DPC enabled root ports are not to discard the error message > if it came from downstream, or skip signalling the message for root port > detected errors. > I'll circle this with the HW team. The current code still doesn't handle outstanding transactions properly. We can probably split the patch into two and deal with this aspect later. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On 1/2/2018 12:12 PM, Keith Busch wrote: > On Tue, Jan 02, 2018 at 08:25:08AM -0500, Sinan Kaya wrote: >>> 2. A DPC event suppresses the error message required for the Linux >>> AER driver to run. How can AER and DPC run concurrently? >>> >> >> As we briefly discussed in previous email exchanges, I think you are >> looking at a use case with a switch that supports DPC functionality. > > No, I'm interested in DPC in a general. > >> Oza and I are looking at a root port functionality with DPC feature. >> >> As you already know, AER errors are logged to AER capability register >> independent of the DPC driver presence. > > The error is noted in the Uncorrectable Error Status Register if that's > what triggered the DPC event. This register has nothing to do with the > Root Error Status Register, which is required to have received an error > Message in order to have a status for the AER driver. > >> A root port is also allowed to share the MSI interrupts across DPC and >> AER. >> >> Therefore, when a DPC interrupt fires; both AER driver and DPC driver >> starts recovery work. This is the issue we are trying to deal with. > > If DPC is implemented correctly, the AER Root Status can't have an > uncorrectable status for the driver to deal with. The only thing the AER > driver could possibly see is a correctable error if DPC ERR_COR Enable > is set. > >> In the end, the driver needs to work for both root port and switches. >> I think you verified it against a switch. We are doing the same for a >> root port and submitting the plumbing code. > > I think we need to consider the possibility you are enabling a platform > that implemented DPC incorrectly. There's nothing in the specification > that says that DPC enabled root ports are not to discard the error message > if it came from downstream, or skip signalling the message for root port > detected errors. > I'll circle this with the HW team. The current code still doesn't handle outstanding transactions properly. We can probably split the patch into two and deal with this aspect later. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On Tue, Jan 02, 2018 at 08:25:08AM -0500, Sinan Kaya wrote: > > 2. A DPC event suppresses the error message required for the Linux > > AER driver to run. How can AER and DPC run concurrently? > > > > As we briefly discussed in previous email exchanges, I think you are > looking at a use case with a switch that supports DPC functionality. No, I'm interested in DPC in a general. > Oza and I are looking at a root port functionality with DPC feature. > > As you already know, AER errors are logged to AER capability register > independent of the DPC driver presence. The error is noted in the Uncorrectable Error Status Register if that's what triggered the DPC event. This register has nothing to do with the Root Error Status Register, which is required to have received an error Message in order to have a status for the AER driver. > A root port is also allowed to share the MSI interrupts across DPC and > AER. > > Therefore, when a DPC interrupt fires; both AER driver and DPC driver > starts recovery work. This is the issue we are trying to deal with. If DPC is implemented correctly, the AER Root Status can't have an uncorrectable status for the driver to deal with. The only thing the AER driver could possibly see is a correctable error if DPC ERR_COR Enable is set. > In the end, the driver needs to work for both root port and switches. > I think you verified it against a switch. We are doing the same for a > root port and submitting the plumbing code. I think we need to consider the possibility you are enabling a platform that implemented DPC incorrectly. There's nothing in the specification that says that DPC enabled root ports are not to discard the error message if it came from downstream, or skip signalling the message for root port detected errors.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On Tue, Jan 02, 2018 at 08:25:08AM -0500, Sinan Kaya wrote: > > 2. A DPC event suppresses the error message required for the Linux > > AER driver to run. How can AER and DPC run concurrently? > > > > As we briefly discussed in previous email exchanges, I think you are > looking at a use case with a switch that supports DPC functionality. No, I'm interested in DPC in a general. > Oza and I are looking at a root port functionality with DPC feature. > > As you already know, AER errors are logged to AER capability register > independent of the DPC driver presence. The error is noted in the Uncorrectable Error Status Register if that's what triggered the DPC event. This register has nothing to do with the Root Error Status Register, which is required to have received an error Message in order to have a status for the AER driver. > A root port is also allowed to share the MSI interrupts across DPC and > AER. > > Therefore, when a DPC interrupt fires; both AER driver and DPC driver > starts recovery work. This is the issue we are trying to deal with. If DPC is implemented correctly, the AER Root Status can't have an uncorrectable status for the driver to deal with. The only thing the AER driver could possibly see is a correctable error if DPC ERR_COR Enable is set. > In the end, the driver needs to work for both root port and switches. > I think you verified it against a switch. We are doing the same for a > root port and submitting the plumbing code. I think we need to consider the possibility you are enabling a platform that implemented DPC incorrectly. There's nothing in the specification that says that DPC enabled root ports are not to discard the error message if it came from downstream, or skip signalling the message for root port detected errors.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
Hi Keith, On 12/29/2017 12:23 PM, Keith Busch wrote: > On Fri, Dec 29, 2017 at 12:54:17PM +0530, Oza Pawandeep wrote: >> This patch addresses the race condition between AER and DPC for recovery. >> >> Current DPC driver does not do recovery, e.g. calling end-point's driver's >> callbacks, which sanitize the device. >> DPC driver implements link_reset callback, and calls pci_do_recovery. > > I'm not sure I see why any of this is necessary for two reasons: > > 1. A downstream port containment event disables the link. How can a driver > sanitize an end device when all the end devices below the containment are > physically inaccessible? Any attempt to access such devices will just > end with either CA or UR (depending on DPC control settings). Since we > already know the failed outcome from attempting to access such devices, > why do you want the drivers to do anything? The reset callback to the endpoint driver has a status field indicating whether the IO is frozen or not. If IO is not frozen, an endpoint driver can potentially recover from the error by reissuing the failed request. If IO is frozen, then the endpoint driver needs to clean up outstanding resources. It is not safe to just shutdown the driver while there are transactions in flight. This is the reason for the status field and a chance for driver to clean up any state machines and resources. Also note that the error callback has a result return value. An endpoint driver indicates whether it was successful on recovering or not. > > 2. A DPC event suppresses the error message required for the Linux > AER driver to run. How can AER and DPC run concurrently? > As we briefly discussed in previous email exchanges, I think you are looking at a use case with a switch that supports DPC functionality. Oza and I are looking at a root port functionality with DPC feature. As you already know, AER errors are logged to AER capability register independent of the DPC driver presence. A root port is also allowed to share the MSI interrupts across DPC and AER. Therefore, when a DPC interrupt fires; both AER driver and DPC driver starts recovery work. This is the issue we are trying to deal with. In the end, the driver needs to work for both root port and switches. I think you verified it against a switch. We are doing the same for a root port and submitting the plumbing code. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
Hi Keith, On 12/29/2017 12:23 PM, Keith Busch wrote: > On Fri, Dec 29, 2017 at 12:54:17PM +0530, Oza Pawandeep wrote: >> This patch addresses the race condition between AER and DPC for recovery. >> >> Current DPC driver does not do recovery, e.g. calling end-point's driver's >> callbacks, which sanitize the device. >> DPC driver implements link_reset callback, and calls pci_do_recovery. > > I'm not sure I see why any of this is necessary for two reasons: > > 1. A downstream port containment event disables the link. How can a driver > sanitize an end device when all the end devices below the containment are > physically inaccessible? Any attempt to access such devices will just > end with either CA or UR (depending on DPC control settings). Since we > already know the failed outcome from attempting to access such devices, > why do you want the drivers to do anything? The reset callback to the endpoint driver has a status field indicating whether the IO is frozen or not. If IO is not frozen, an endpoint driver can potentially recover from the error by reissuing the failed request. If IO is frozen, then the endpoint driver needs to clean up outstanding resources. It is not safe to just shutdown the driver while there are transactions in flight. This is the reason for the status field and a chance for driver to clean up any state machines and resources. Also note that the error callback has a result return value. An endpoint driver indicates whether it was successful on recovering or not. > > 2. A DPC event suppresses the error message required for the Linux > AER driver to run. How can AER and DPC run concurrently? > As we briefly discussed in previous email exchanges, I think you are looking at a use case with a switch that supports DPC functionality. Oza and I are looking at a root port functionality with DPC feature. As you already know, AER errors are logged to AER capability register independent of the DPC driver presence. A root port is also allowed to share the MSI interrupts across DPC and AER. Therefore, when a DPC interrupt fires; both AER driver and DPC driver starts recovery work. This is the issue we are trying to deal with. In the end, the driver needs to work for both root port and switches. I think you verified it against a switch. We are doing the same for a root port and submitting the plumbing code. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On 2017-12-29 23:43, Keith Busch wrote: On Fri, Dec 29, 2017 at 11:30:02PM +0530, p...@codeaurora.org wrote: On 2017-12-29 22:53, Keith Busch wrote: > 2. A DPC event suppresses the error message required for the Linux > AER driver to run. How can AER and DPC run concurrently? I afraid I could not grasp the first line completely. A DPC capable and enabled port discards error messages; the ERR_FATAL or ERR_NONFATAL message required to trigger AER handling won't exist in such a setup. This behavior is defined in the specification 6.2.10 for Downstream Port Containment: When DPC is triggered due to receipt of an uncorrectable error Message, the Requester ID from the Message is recorded in the DPC Error Source ID register and that Message is discarded and not forwarded Upstream. When DPC is triggered by an unmasked uncorrectable error, that error will not be signaled with an uncorrectable error Message, even if otherwise enabled. In my understanding, thiis talks about DPC enabled switch. this case is taken care as well. if you look at patchset-3, when AER is triggered, AER's pci_dev is of endpoint will traverse all the way up until it finds associated DPC service enabled. pdev = pcie_port_upstream_bridge(dev); if AER is not triggered, then at switch level DPC will take care/suppress the msg and entire SW will not come into picture then. But specifically the patches attempts to bring in some sort of coordination and understanding between AER and DPC. as I mentioned in my previous mail. Regards, Oza.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On 2017-12-29 23:43, Keith Busch wrote: On Fri, Dec 29, 2017 at 11:30:02PM +0530, p...@codeaurora.org wrote: On 2017-12-29 22:53, Keith Busch wrote: > 2. A DPC event suppresses the error message required for the Linux > AER driver to run. How can AER and DPC run concurrently? I afraid I could not grasp the first line completely. A DPC capable and enabled port discards error messages; the ERR_FATAL or ERR_NONFATAL message required to trigger AER handling won't exist in such a setup. This behavior is defined in the specification 6.2.10 for Downstream Port Containment: When DPC is triggered due to receipt of an uncorrectable error Message, the Requester ID from the Message is recorded in the DPC Error Source ID register and that Message is discarded and not forwarded Upstream. When DPC is triggered by an unmasked uncorrectable error, that error will not be signaled with an uncorrectable error Message, even if otherwise enabled. In my understanding, thiis talks about DPC enabled switch. this case is taken care as well. if you look at patchset-3, when AER is triggered, AER's pci_dev is of endpoint will traverse all the way up until it finds associated DPC service enabled. pdev = pcie_port_upstream_bridge(dev); if AER is not triggered, then at switch level DPC will take care/suppress the msg and entire SW will not come into picture then. But specifically the patches attempts to bring in some sort of coordination and understanding between AER and DPC. as I mentioned in my previous mail. Regards, Oza.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On Fri, Dec 29, 2017 at 11:30:02PM +0530, p...@codeaurora.org wrote: > On 2017-12-29 22:53, Keith Busch wrote: > > > 2. A DPC event suppresses the error message required for the Linux > > AER driver to run. How can AER and DPC run concurrently? > > I afraid I could not grasp the first line completely. A DPC capable and enabled port discards error messages; the ERR_FATAL or ERR_NONFATAL message required to trigger AER handling won't exist in such a setup. This behavior is defined in the specification 6.2.10 for Downstream Port Containment: When DPC is triggered due to receipt of an uncorrectable error Message, the Requester ID from the Message is recorded in the DPC Error Source ID register and that Message is discarded and not forwarded Upstream. When DPC is triggered by an unmasked uncorrectable error, that error will not be signaled with an uncorrectable error Message, even if otherwise enabled.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On Fri, Dec 29, 2017 at 11:30:02PM +0530, p...@codeaurora.org wrote: > On 2017-12-29 22:53, Keith Busch wrote: > > > 2. A DPC event suppresses the error message required for the Linux > > AER driver to run. How can AER and DPC run concurrently? > > I afraid I could not grasp the first line completely. A DPC capable and enabled port discards error messages; the ERR_FATAL or ERR_NONFATAL message required to trigger AER handling won't exist in such a setup. This behavior is defined in the specification 6.2.10 for Downstream Port Containment: When DPC is triggered due to receipt of an uncorrectable error Message, the Requester ID from the Message is recorded in the DPC Error Source ID register and that Message is discarded and not forwarded Upstream. When DPC is triggered by an unmasked uncorrectable error, that error will not be signaled with an uncorrectable error Message, even if otherwise enabled.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On 2017-12-29 22:53, Keith Busch wrote: On Fri, Dec 29, 2017 at 12:54:17PM +0530, Oza Pawandeep wrote: This patch addresses the race condition between AER and DPC for recovery. Current DPC driver does not do recovery, e.g. calling end-point's driver's callbacks, which sanitize the device. DPC driver implements link_reset callback, and calls pci_do_recovery. I'm not sure I see why any of this is necessary for two reasons: 1. A downstream port containment event disables the link. How can a driver sanitize an end device when all the end devices below the containment are physically inaccessible? Any attempt to access such devices will just end with either CA or UR (depending on DPC control settings). Since we already know the failed outcome from attempting to access such devices, why do you want the drivers to do anything? Ok I think my statement was misleading, not device sanitation, but the device driver making SW sanitize. for e.g. have a look at e1000_io_error_detected which is called say in case of AER ERR_FATAL msg. which sanitizes sw stack, interrupts management (synchronize_irq), delete timers etc.. yes, DPC would have made the link state disabled, and HW would have reset the internal logic with quiescence activities so yes, any transaction on will end with CA or UR. well but device driver has to handle rest of the possible things as I mentioned (error callbacks) 2. A DPC event suppresses the error message required for the Linux AER driver to run. How can AER and DPC run concurrently? I afraid I could not grasp the first line completely. but they way it is triggering AER and DPC on our platform concurrently is, we have same MSIx registered for both AER and DPC, and linux calls the shared handlers to handle both the triggers anyway. otherwise also if ERR_FATAL msg occurs, the Root port should trigger both AER and DPC (assuming both are enabled, and no FW first for AER/DPC) the problem with the current framework of AER and DPC in Linux is: both try to act independently, while we know that (for e.g. ERR_FATAL msg) is responsible for triggering both AER and DPC depending on the configuration. (currently DPC is configured for both FATAL and NONFATAL in linux anyway) It does not make sense that AER goes ahead and attempts to sanitize with the device driver's callbacks as I mentioned. and DPC being unaware, asynchronously disables the link (although this is all HW) but DPC service driver should adapt to some kind of error handling and error resume which AER has adapted. Hence this whole design changes proposed with respect to error handling. Let me give you another problem statement on the same line: when DPC is active, AER does not need to act at all...because it doesnt make sense for AER to act independently., without knowing what DPC service driver is upto! which is handled in one of the patches. the point I am trying to make is: DPC should not rely on AER to call error callbacks, and AER should not be doing it without knowing that DPC is active and it is also going to some course of action (be in HW or SW) Regards, Oza.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On 2017-12-29 22:53, Keith Busch wrote: On Fri, Dec 29, 2017 at 12:54:17PM +0530, Oza Pawandeep wrote: This patch addresses the race condition between AER and DPC for recovery. Current DPC driver does not do recovery, e.g. calling end-point's driver's callbacks, which sanitize the device. DPC driver implements link_reset callback, and calls pci_do_recovery. I'm not sure I see why any of this is necessary for two reasons: 1. A downstream port containment event disables the link. How can a driver sanitize an end device when all the end devices below the containment are physically inaccessible? Any attempt to access such devices will just end with either CA or UR (depending on DPC control settings). Since we already know the failed outcome from attempting to access such devices, why do you want the drivers to do anything? Ok I think my statement was misleading, not device sanitation, but the device driver making SW sanitize. for e.g. have a look at e1000_io_error_detected which is called say in case of AER ERR_FATAL msg. which sanitizes sw stack, interrupts management (synchronize_irq), delete timers etc.. yes, DPC would have made the link state disabled, and HW would have reset the internal logic with quiescence activities so yes, any transaction on will end with CA or UR. well but device driver has to handle rest of the possible things as I mentioned (error callbacks) 2. A DPC event suppresses the error message required for the Linux AER driver to run. How can AER and DPC run concurrently? I afraid I could not grasp the first line completely. but they way it is triggering AER and DPC on our platform concurrently is, we have same MSIx registered for both AER and DPC, and linux calls the shared handlers to handle both the triggers anyway. otherwise also if ERR_FATAL msg occurs, the Root port should trigger both AER and DPC (assuming both are enabled, and no FW first for AER/DPC) the problem with the current framework of AER and DPC in Linux is: both try to act independently, while we know that (for e.g. ERR_FATAL msg) is responsible for triggering both AER and DPC depending on the configuration. (currently DPC is configured for both FATAL and NONFATAL in linux anyway) It does not make sense that AER goes ahead and attempts to sanitize with the device driver's callbacks as I mentioned. and DPC being unaware, asynchronously disables the link (although this is all HW) but DPC service driver should adapt to some kind of error handling and error resume which AER has adapted. Hence this whole design changes proposed with respect to error handling. Let me give you another problem statement on the same line: when DPC is active, AER does not need to act at all...because it doesnt make sense for AER to act independently., without knowing what DPC service driver is upto! which is handled in one of the patches. the point I am trying to make is: DPC should not rely on AER to call error callbacks, and AER should not be doing it without knowing that DPC is active and it is also going to some course of action (be in HW or SW) Regards, Oza.
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On Fri, Dec 29, 2017 at 12:54:17PM +0530, Oza Pawandeep wrote: > This patch addresses the race condition between AER and DPC for recovery. > > Current DPC driver does not do recovery, e.g. calling end-point's driver's > callbacks, which sanitize the device. > DPC driver implements link_reset callback, and calls pci_do_recovery. I'm not sure I see why any of this is necessary for two reasons: 1. A downstream port containment event disables the link. How can a driver sanitize an end device when all the end devices below the containment are physically inaccessible? Any attempt to access such devices will just end with either CA or UR (depending on DPC control settings). Since we already know the failed outcome from attempting to access such devices, why do you want the drivers to do anything? 2. A DPC event suppresses the error message required for the Linux AER driver to run. How can AER and DPC run concurrently?
Re: [PATCH v2 2/4] PCI/DPC/AER: Address Concurrency between AER and DPC
On Fri, Dec 29, 2017 at 12:54:17PM +0530, Oza Pawandeep wrote: > This patch addresses the race condition between AER and DPC for recovery. > > Current DPC driver does not do recovery, e.g. calling end-point's driver's > callbacks, which sanitize the device. > DPC driver implements link_reset callback, and calls pci_do_recovery. I'm not sure I see why any of this is necessary for two reasons: 1. A downstream port containment event disables the link. How can a driver sanitize an end device when all the end devices below the containment are physically inaccessible? Any attempt to access such devices will just end with either CA or UR (depending on DPC control settings). Since we already know the failed outcome from attempting to access such devices, why do you want the drivers to do anything? 2. A DPC event suppresses the error message required for the Linux AER driver to run. How can AER and DPC run concurrently?
