linux-kernel - Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Jun 2021 23:28:49 -0500
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc:     lorenzo.pieralisi@....com, robh@...nel.org, bhelgaas@...gle.com,
        linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Siddartha Mohanadoss <smohanad@...eaurora.org>
Subject: Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller
 driver

On Wed 09 Jun 03:51 CDT 2021, Manivannan Sadhasivam wrote:
> On Sat, Jun 05, 2021 at 10:07:15PM -0500, Bjorn Andersson wrote:
> > On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
[..]
> > > +static irqreturn_t qcom_pcie_ep_perst_threaded_irq(int irq, void *data)
> > > +{
> > > +	struct qcom_pcie_ep *pcie_ep = data;
> > > +	struct dw_pcie *pci = &pcie_ep->pci;
> > > +	struct device *dev = pci->dev;
> > > +	u32 perst;
> > > +
> > > +	perst = gpiod_get_value(pcie_ep->reset);
> > > +
> > > +	if (perst) {
> > > +		/* Start link training */
> > > +		dev_info(dev, "PERST de-asserted by host. Starting link training!\n");
> > > +		qcom_pcie_establish_link(pci);
> > > +	} else {
> > > +		/* Shutdown the link if the link is already on */
> > > +		dev_info(dev, "PERST asserted by host. Shutting down the PCIe link!\n");
> > > +		qcom_pcie_disable_link(pci);
> > > +	}
> > > +
> > > +	/* Set trigger type based on the next expected value of perst gpio */
> > > +	irq_set_irq_type(gpiod_to_irq(pcie_ep->reset),
> > > +			 (perst ? IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH));
> > 
> > Looks like you're manually implementing edge triggering, is there any
> > reason for that? EDGE_BOTH seems to do the same thing...
> > 
> 
> PERST is a level based signal, so I don't think we can use EDGE_BOTH here.
> 

Afaict it's just a gpio and you define if the hardware should fire of
interrupts given its level or if it should detect transitions.

That said, if the gpio is already high when registering the irq handler
there's no transition.

> > > +
> > > +	return IRQ_HANDLED;
> > > +}
[..]
> > > +static struct platform_driver qcom_pcie_ep_driver = {
> > > +	.probe	= qcom_pcie_ep_probe,
> > > +	.driver	= {
> > > +		.name		= "qcom-pcie-ep",
> > 
> > Skip the indentation of the '='.
> > 
> > > +		.suppress_bind_attrs = true,
> > 
> > Why do we suppress_bind_attrs?
> > 
> 
> This driver doesn't support remove() callback and I don't think it is necessary
> for this platform driver. So this flag is here to prevent unbind from sysfs.
> 

Right, that part makes sense. But do you know why this is, why it's not
possible to have the PCI controller built as a module? (GKI should
want this).

Regards,
Bjorn