Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Bharat Kumar Gogada <bharatku@xilinx.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
CC:     "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Ravikiran Gummaluri <rgummal@xilinx.com>,
        Michal Simek <michals@xilinx.com>,
        Ley Foon Tan <lftan@altera.com>,
        "rfi@lists.rocketboards.org" <rfi@lists.rocketboards.org>,
        Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Subject: RE: [PATCH 2/2] PCI: Versal CPM: Add support for Versal CPM Root Port
 driver
Thread-Topic: [PATCH 2/2] PCI: Versal CPM: Add support for Versal CPM Root
 Port driver
Thread-Index: AQHVtypsQVCQA4Pdl0W2IKkLYMD11KfDHY8AgArLTXA=
Date:   Fri, 27 Dec 2019 11:55:41 +0000
Message-ID: <MN2PR02MB6336D981AA38923276DAAF43A52A0@MN2PR02MB6336.namprd02.prod.outlook.com>
References: <1576842072-32027-3-git-send-email-bharat.kumar.gogada@xilinx.com>
 <20191220145832.GA93984@google.com>
In-Reply-To: <20191220145832.GA93984@google.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: xilinx.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: eadb79e3-3a72-49c4-2d25-08d78ac3b290
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Dec 2019 11:55:41.1965
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: bopY3yQ0JL/oMqMEsdY9bNxHzmyEGIBZlaFeKvZ1wjsNt13saSPLc+6cUmnw72tF1aK4J+ZRtue8wVQJHeukhw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR02MB6383
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> Subject: Re: [PATCH 2/2] PCI: Versal CPM: Add support for Versal CPM Root=
 Port
> driver
>=20
> [+cc other drivers that use the broken "is link up" test in config access=
]
>=20
> On Fri, Dec 20, 2019 at 05:11:12PM +0530, Bharat Kumar Gogada wrote:
> > - Adding support for versal CPM as Root Port.
> > - The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrate=
d
> >   block for CPM along with the integrated bridge can function
> >   as PCIe Root Port.
> > - Versal CPM uses GICv3 ITS feature for assigning MSI/MSI-X
> >   vectors and handling MSI/MSI-X interrupts.
> > - Bridge error and legacy interrupts in versal CPM are handled using
> >   versal CPM specific MISC interrupt line.
>=20
> "Versal" appears to be a brand name and should be capitalized consistentl=
y.
>=20
> > +#define INTX_NUM                        4
>=20
> Don't we have a generic PCI core definition for this?
Thanks Bjorn, will fix this with core definition.
>=20
> > +static inline bool cpm_pcie_link_is_up(struct xilinx_cpm_pcie_port
> > +*port)
>=20
> Please follow the example of other drivers and name this "cpm_pcie_link_u=
p()".
Agreed, will change the name.
>=20
> > +{
> > +	return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) &
> > +		XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0; }
>=20
> > +/**
> > + * xilinx_cpm_pcie_valid_device - Check if a valid device is present
> > +on bus
> > + * @bus: PCI Bus structure
> > + * @devfn: device/function
> > + *
> > + * Return: 'true' on success and 'false' if invalid device is found
> > +*/ static bool xilinx_cpm_pcie_valid_device(struct pci_bus *bus,
> > +					 unsigned int devfn)
> > +{
> > +	struct xilinx_cpm_pcie_port *port =3D bus->sysdata;
> > +
> > +	/* Check if link is up when trying to access downstream ports */
> > +	if (bus->number !=3D port->root_busno)
> > +		if (!cpm_pcie_link_is_up(port))
> > +			return false;
>=20
> This check for the link being up is racy and should be removed.  The link=
 may go
> down after the check and before the config access.
>=20
> A config access to a device where the link is down is an error, but it's =
an error we
> expect to happen because of enumeration, surprise unplug, or electrical i=
ssue.
> It's impossible to avoid so we must be able to handle and recover from it=
.
>=20
> I know there are other drivers that do this (dwc, altera, xilinix-nwl, xi=
linx), and
> I've pointed this out many times in the past.  This needs to be fixed.
Agreed, will fix this check.
>=20
> > +
> > +	/* Only one device down on each root port */
> > +	if (bus->number =3D=3D port->root_busno && devfn > 0)
> > +		return false;
> > +
> > +	return true;
> > +}
>=20
> > +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data)
> > +{
> > +	struct xilinx_cpm_pcie_port *port =3D
> > +				(struct xilinx_cpm_pcie_port *)data;
>=20
>   struct device *dev =3D port->dev;
>=20
> to reduce repetition of "port->dev" below.
Agreed, will fix this.

> > +	u32 val, mask, status, bit;
> > +	unsigned long intr_val;
> > +
> > +	/* Read interrupt decode and mask registers */
> > +	val =3D pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
> > +	mask =3D pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
> > +
> > +	status =3D val & mask;
> > +	if (!status)
> > +		return IRQ_NONE;
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_LINK_DOWN)
> > +		dev_warn(port->dev, "Link Down\n");
> > +
> > +	if (status & XILINX_CPM_PCIE_INTR_HOT_RESET)
> > +		dev_info(port->dev, "Hot reset\n");
> > ...