linux-kernel - Re: [PATCH] Revert "PCI: dwc: Wait for link up only if link is started"

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]

Message-ID: <ZK7m0hjQg7H5rANZ@google.com>
Date:   Wed, 12 Jul 2023 23:15:54 +0530
From:   Ajay Agarwal <ajayagarwal@...gle.com>
To:     Johan Hovold <johan@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc:     Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Rob Herring <robh@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Bjorn Andersson <quic_bjorande@...cinc.com>,
        Sajid Dalvi <sdalvi@...gle.com>
Subject: Re: [PATCH] Revert "PCI: dwc: Wait for link up only if link is
 started"

On Tue, Jul 11, 2023 at 08:52:23AM +0200, Johan Hovold wrote:
> On Tue, Jul 11, 2023 at 02:06:08AM +0900, Krzysztof Wilczyński wrote:
> 
> > > > > > > Finally, note that the intel-gw driver is the only driver currently not
> > > > > > > providing a start_link callback and instead starts the link in its
> > > > > > > host_init callback, and which may avoid an additional one-second timeout
> > > > > > > during probe by making the link-up wait conditional. If anyone cares,
> > > > > > > that can be done in a follow-up patch with a proper motivation.
> 
> > The whole conversation above about the intel-gw driver: would something
> > need to be addressed here?  Or can I pick the suggested fix?
> 
> No, it's just another indication that the offending commit was confused.
> 
> All mainline drivers already start the link before that
> wait-for-link-up, so the commit in question makes very little sense.
> That's why I prefer reverting it, so as to not pollute the git logs
> (e.g. for git blame) with misleading justifications.
Johan, Mani,
I am developing a PCIe driver which will not have the start_link
callback defined. Instead, the link will be coming up much later based
on some other trigger. So my driver will not attempt the LTSSM training
on probe. So even if the probe is made asynchronous, it will still end
up wasting 1 second of time.
> 
> > > > My apologies for adding this regression in some of the SOCs.
> > > > May I suggest to keep my patch and make the following change instead?
> > > > This shall keep the existing behavior as is, and save the boot time
> > > > for drivers that do not define the start_link()?
> > [...]
> > 
> > > I just realized that Fabio pushed exactly the same patch as I suggested
> > > here:
> > > https://lore.kernel.org/all/20230704122635.1362156-1-festevam@gmail.com/.
> > > I think it is better we take it instead of reverting my commit.
> > 
> > Will do.  I will also make sure that we have correct attributions in place.
> 
> As I mentioned in the commit message, I think the commit should just be
> reverted and if there's a valid argument to be made for a similar type
> of change (without the breakage), that can be done as a follow-up with a
> proper motivation.
> 
> Johan
I agree that my commit created regression in some of the existing SOCs.
I should not have taken the liberty to return an error if the
wait-for-link-up call fails in the probe.
But my commit's message body clearly mentions the motivation behind
calling dw_pcie_wait_for_link() only if the start_link is defined. Can
you please re-evaluate the decision to revert my patch and pick up the
suggested fix instead?

Thanks
Ajay