Received-SPF: pass (google.com: domain of linux-kernel+bounces-66582-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Shradha Todi" <shradha.t@samsung.com>
To: "'Manivannan Sadhasivam'" <manivannan.sadhasivam@linaro.org>
Cc: <lpieralisi@kernel.org>, <kw@linux.com>, <robh@kernel.org>,
	<bhelgaas@google.com>, <jingoohan1@gmail.com>,
	<gustavo.pimentel@synopsys.com>, <josh@joshtriplett.org>,
	<lukas.bulwahn@gmail.com>, <hongxing.zhu@nxp.com>,
	<pankaj.dubey@samsung.com>, <linux-kernel@vger.kernel.org>,
	<linux-pci@vger.kernel.org>, <vidyas@nvidia.com>, <gost.dev@samsung.com>
In-Reply-To: <20240104055030.GA3031@thinkpad>
Subject: RE: [PATCH v2 0/3] Add support for RAS DES feature in PCIe DW
 controller
Date: Thu, 15 Feb 2024 14:55:06 +0530
Message-ID: <0df701da5ff0$df1165a0$9d3430e0$@samsung.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQG/Q0WxGB9EWFeblm7N70OL8blxhQGR1/IBAmU4G1oB/2O7JgJFQ8VesP9yRcA=
Content-Language: en-in
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
DLP-Filter: Pass
References: <CGME20231130115055epcas5p4e29befa80877be45dbee308846edc0ba@epcas5p4.samsung.com>
	<20231130115044.53512-1-shradha.t@samsung.com>
	<20231130165514.GW3043@thinkpad>
	<000601da3e07$c39e5e00$4adb1a00$@samsung.com>
	<20240104055030.GA3031@thinkpad>


> -----Original Message-----
> From: 'Manivannan Sadhasivam' <manivannan.sadhasivam=40linaro.org>
> Sent: 04 January 2024 11:21
> To: Shradha Todi <shradha.t=40samsung.com>
> Cc: lpieralisi=40kernel.org; kw=40linux.com; robh=40kernel.org;
> bhelgaas=40google.com; jingoohan1=40gmail.com;
> gustavo.pimentel=40synopsys.com; josh=40joshtriplett.org;
> lukas.bulwahn=40gmail.com; hongxing.zhu=40nxp.com;
> pankaj.dubey=40samsung.com; linux-kernel=40vger.kernel.org; linux-
> pci=40vger.kernel.org
> Subject: Re: =5BPATCH v2 0/3=5D Add support for RAS DES feature in PCIe D=
W
> controller
>=20
> On Wed, Jan 03, 2024 at 11:13:20AM +0530, Shradha Todi wrote:
> >
> >
> > > -----Original Message-----
> > > From: Shradha Todi <shradha.t=40samsung.com>
> > > Sent: 04 December 2023 14:10
> > > To: 'Manivannan Sadhasivam' <manivannan.sadhasivam=40linaro.org>
> > > Cc: 'lpieralisi=40kernel.org' <lpieralisi=40kernel.org>; 'kw=40linux.=
com'
> > > <kw=40linux.com>; 'robh=40kernel.org' <robh=40kernel.org>;
> > > 'bhelgaas=40google.com' <bhelgaas=40google.com>; 'jingoohan1=40gmail.=
com'
> > > <jingoohan1=40gmail.com>; 'gustavo.pimentel=40synopsys.com'
> > > <gustavo.pimentel=40synopsys.com>; 'josh=40joshtriplett.org'
> > > <josh=40joshtriplett.org>; 'lukas.bulwahn=40gmail.com'
> > > <lukas.bulwahn=40gmail.com>; 'hongxing.zhu=40nxp.com'
> > > <hongxing.zhu=40nxp.com>; 'pankaj.dubey=40samsung.com'
> > > <pankaj.dubey=40samsung.com>; 'linux-kernel=40vger.kernel.org' <linux=
-
> > > kernel=40vger.kernel.org>; 'linux-pci=40vger.kernel.org' <linux-
> > > pci=40vger.kernel.org>
> > > Subject: RE: =5BPATCH v2 0/3=5D Add support for RAS DES feature in PC=
Ie
> > > DW controller
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Manivannan Sadhasivam
> > > > =5Bmailto:manivannan.sadhasivam=40linaro.org=5D
> > > > Sent: 30 November 2023 22:25
> > > > To: Shradha Todi <shradha.t=40samsung.com>
> > > > Cc: lpieralisi=40kernel.org; kw=40linux.com; robh=40kernel.org;
> > > > bhelgaas=40google.com; jingoohan1=40gmail.com;
> > > > gustavo.pimentel=40synopsys.com; josh=40joshtriplett.org;
> > > > lukas.bulwahn=40gmail.com; hongxing.zhu=40nxp.com;
> > > > pankaj.dubey=40samsung.com; linux-kernel=40vger.kernel.org; linux-
> > > > pci=40vger.kernel.org
> > > > Subject: Re: =5BPATCH v2 0/3=5D Add support for RAS DES feature in
> > > > PCIe DW controller
> > > >
> > > > On Thu, Nov 30, 2023 at 05:20:41PM +0530, Shradha Todi wrote:
> > > > > DesignWare controller provides a vendor specific extended
> > > > > capability called RASDES as an IP feature. This extended
> > > > > capability  provides hardware information like:
> > > > >  - Debug registers to know the state of the link or controller.
> > > > >  - Error injection mechanisms to inject various PCIe errors inclu=
ding
> > > > >    sequence number, CRC
> > > > >  - Statistical counters to know how many times a particular event
> > > > >    occurred
> > > > >
> > > > > However, in Linux we do not have any generic or custom support
> > > > > to be able to use this feature in an efficient manner. This is
> > > > > the reason we are proposing this framework. Debug and bring up
> > > > > time of high-speed IPs are highly dependent on costlier hardware
> > > > > analyzers and this solution will in some ways help to reduce the =
HW
> analyzer usage.
> > > > >
> > > > > The debugfs entries can be used to get information about
> > > > > underlying hardware and can be shared with user space. Separate
> > > > > debugfs entries has been created to cater to all the DES hooks pr=
ovided
> by the controller.
> > > > > The debugfs entries interacts with the RASDES registers in the
> > > > > required sequence and provides the meaningful data to the user.
> > > > > This eases the effort to understand and use the register
> > > > > information for
> > > > debugging.
> > > > >
> > > > > v1 version was posted long back and for some reasons I couldn't
> > > > > work on it. I apologize for the long break. I'm restarting this
> > > > > activity and have taken care of all previous review comments shar=
ed.
> > > > > v1:
> > > > > https://lore.kernel.org/all/20210518174618.42089-1-shradha.t=40sa=
m
> > > > > sung
> > > > > .c
> > > > > om/T/
> > > > >
> > > >
> > > > There is already a series floating to add similar functionality
> > > > via perf
> > > > subsystem:
> > > > https://lore.kernel.org/linux-pci/20231121013400.18367-1-
> > > > xueshuai=40linux.alibaba.com/
> > > >
> > > > - Mani
> > > >
> > >
> > > Hi Mani,
> > >
> > > The series proposed in perf includes only time based-analysis and
> > > event counters which will monitor performance (Group 6 and 7). The
> > > patch or framework that we have proposed includes debug information,
> > > error injection facility and error counters (Group 0 - 5) which are
> > > not included as part of the functionality implemented via perf. In
> > > my opinion, these functionalities don't count as performance
> > > monitoring or counters but rather as debug counters. How about we tak=
e this
> up as a debugfs framework as proposed in my patch?
> > > Or if others feel it can be taken via perf driver then I am happy to
> > > extend the perf driver if authors do not have objection. Let me know =
what
> you think of this?
> > > Meanwhile I will review the perf patches and share my feedback.
> > >
> >
> > Hello Mani,
> > Any update on the above comment? IMO, even though the perf patches and
> > this patchset are both part of the DWC vendor specific capability -
> > RASDES,  they cover different features. The perf file includes
> > performance based parameters like time-based analysis and event
> > counters for count of packets whereas this patchset includes debugging
> > fields, error injection and event counters for count of errors. I
> > think having a separate debugfs file fits more but would you suggest we=
 extend
> the perf file itself?
> >
>=20
> For the error injection and counters, we already have the EDAC framework.=
 So
> adding them in the DWC driver doesn't make sense to me.
>=20

Sorry for late response, was going through the EDAC framework to understand=
 better how we can fit RAS DES support in it. Below are some technical chal=
lenges found so far:
1: This debugfs framework proposed =5B1=5D can run on both side of the link=
 i.e. RC and EP as it will be a part of the link controller platform driver=
 Here for the EP side the assumption is that it has Linux running, which i=
s primarily a use case for chip-to-chip communication.  After your suggesti=
on to migrate to EDAC framework we studied and here are the findings:
- If we move to EDAC framework, we need to have RAS DES as a pci_driver whi=
ch will be binded based on vendor_id and device_id. Our observation is that=
 on EP side system we are unable to bind two function driver (pci_driver), =
as pci_endpoint_test function driver or some other chip-to-chip function dr=
iver will already be bound. On the other hand, on RC side we observed that =
if we have portdrv enabled in Linux running on RC system, it gets bound to =
RC controller and then it does not allow EDAC pci_driver to bind. So basica=
lly we see a problem here, that we can't have two pci_driver binding to sam=
e PCI device
2: Another point is even though we use EDAC driver framework, we may not be=
 able to use any of EDAC framework APIs as they are mostly suitable for mem=
ory controller devices sitting on PCI BUS. We will end up using debugfs ent=
ries just via a pci_driver placed inside EDAC framework.

Please let me know if my understanding is wrong.

> But first check with the perf driver author if they have any plans on add=
ing the
> proposed functionality. If they do not have any plan or not working on it=
, then
> look into EDAC.
>=20
> - Mani
>=20

Since we already worked and posted patches =5B1=5D, =5B2=5D, we will contin=
ue to work on this and based on consent from community we will adopt to mos=
t suitable framework.
We see many subsystems like ethernet, usb, gpu, cxl having debugfs files th=
at give information about the current status of the running system and as o=
f now based on our findings, we still feel there is no harm in having debug=
fs entry based support in DesignWare controller driver itself.=20
Others any opinion please?

=5B1=5D: https://lkml.org/lkml/2021/5/18/1371
=5B2=5D: https://lkml.org/lkml/2023/11/30/574

> > Shradha
> >
> > > > > Shradha Todi (3):
> > > > >   PCI: dwc: Add support for vendor specific capability search
> > > > >   PCI: debugfs: Add support for RASDES framework in DWC
> > > > >   PCI: dwc: Create debugfs files in DWC driver
> > > > >
> > > > >  drivers/pci/controller/dwc/Kconfig            =7C   8 +
> > > > >  drivers/pci/controller/dwc/Makefile           =7C   1 +
> > > > >  .../controller/dwc/pcie-designware-debugfs.c  =7C 476
> > > > ++++++++++++++++++
> > > > >  .../controller/dwc/pcie-designware-debugfs.h  =7C   0
> > > > >  drivers/pci/controller/dwc/pcie-designware.c  =7C  20 +
> > > > > drivers/pci/controller/dwc/pcie-designware.h  =7C  18 +
> > > > >  6 files changed, 523 insertions(+)  create mode 100644
> > > > > drivers/pci/controller/dwc/pcie-designware-debugfs.c
> > > > >  create mode 100644
> > > > > drivers/pci/controller/dwc/pcie-designware-debugfs.h
> > > > >
> > > > > --
> > > > > 2.17.1
> > > > >
> > > >
> > > > --
> > > > =E0=AE=AE=E0=AE=A3=E0=AE=BF=E0=AE=B5=E0=AE=A3=E0=AF=8D=E0=AE=A3=E0=
=AE=A9=E0=AF=8D=20=E0=AE=9A=E0=AE=A4=E0=AE=BE=E0=AE=9A=E0=AE=BF=E0=AE=B5=E0=
=AE=AE=E0=AF=8D=0D=0A>=20>=0D=0A>=20>=0D=0A>=20=0D=0A>=20--=0D=0A>=20=E0=AE=
=AE=E0=AE=A3=E0=AE=BF=E0=AE=B5=E0=AE=A3=E0=AF=8D=E0=AE=A3=E0=AE=A9=E0=AF=8D=
=20=E0=AE=9A=E0=AE=A4=E0=AE=BE=E0=AE=9A=E0=AE=BF=E0=AE=B5=E0=AE=AE=E0=AF=8D=
=0D=0A=0D=0A