linux-kernel - Re: [PATCH v3 1/4] PCI: portdrv: pciehp: Move PCIe hotplug command waiting logic to port 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] [day] [month] [year] [list]

Message-ID: <Z7183Dbg5QbgQj10@U-2FWC9VHC-2323.local>
Date: Tue, 25 Feb 2025 16:18:36 +0800
From: Feng Tang <feng.tang@...ux.alibaba.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Lukas Wunner <lukas@...ner.de>,
	Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@...ux.intel.com>,
	Liguang Zhang <zhangliguang@...ux.alibaba.com>,
	Guanghui Feng <guanghuifeng@...ux.alibaba.com>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Markus Elfring <Markus.Elfring@....de>, lkp@...el.com,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/4] PCI: portdrv: pciehp: Move PCIe hotplug command
 waiting logic to port driver

Hi Ilpo,

On Mon, Feb 24, 2025 at 05:06:26PM +0200, Ilpo Järvinen wrote:
> On Mon, 24 Feb 2025, Feng Tang wrote:
> 
> > According to PCIe spec, after sending a hotplug command, software should
> > wait some time for the command completion. Currently the waiting logic
> 
> Where is it in the spec, please put a more precise reference.

Will do, if the patch is kept.
> 
> > is implemented in pciehp driver, as the same logic will be reused by
> > PCIe port driver, move it to port driver, which complies with the logic
> > of CONFIG_HOTPLUG_PCI_PCIE depending on CONFIG_PCIEPORTBUS.
> > 
> > Also convert the loop wait logic to helper read_poll_timeout() as
> > suggested by Sathyanarayanan Kuppuswamy.
> 
> You could express the second part of this with a tag:
> 
> Suggested-by: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@...ux.intel.com> # Use to read_poll_timeout()

Thanks for the suggestion!

> 
> > Signed-off-by: Feng Tang <feng.tang@...ux.alibaba.com>
> > ---
> >  drivers/pci/hotplug/pciehp_hpc.c | 38 ++++++++------------------------
> >  drivers/pci/pci.h                |  5 +++++
> >  drivers/pci/pcie/portdrv.c       | 25 +++++++++++++++++++++
> >  3 files changed, 39 insertions(+), 29 deletions(-)
[...]

> >  
> > +/* Return 0 on command completed on time, otherwise return -ETIMEOUT */
> 
> Since you're making this visible outside of the file, please document this 
> properly using a kerneldoc compliant comment.

OK.

> 
> > +int pcie_poll_sltctl_cmd(struct pci_dev *dev, int timeout_ms)
> > +{
> > +	u16 slot_status = 0;
> > +	u32 slot_cap;
> > +	int ret = 0;
> 
> Unnecessary initialization.

The initialization is actually used below.
> 
> > +	int __maybe_unused ret1;
> > +
> > +	/* Don't wait if the command complete event is not well supported */
> > +	pcie_capability_read_dword(dev, PCI_EXP_SLTCAP, &slot_cap);
> > +	if (!(slot_cap & PCI_EXP_SLTCAP_HPC) || slot_cap & PCI_EXP_SLTCAP_NCCS)
> > +		return ret;

Used here to return early if the event is not supported.

> > +
> > +	ret = read_poll_timeout(pcie_capability_read_word, ret1,
> > +				(slot_status & PCI_EXP_SLTSTA_CC), 10000,
> > +				timeout_ms * 1000, true, dev, PCI_EXP_SLTSTA,
> 
> Replace:
>         10000 -> 10 * USEC_PER_MSEC
>         timeout_ms * 1000 -> USEC_PER_SEC (the variable can be dropped)
> 
> Please also check you have linux/units.h included for those defines.

Will use the macros, which are indeed self-describing. 

If you are referring 'timeout_ms', I don't think it can be dropped as
it is needed in the logic of pcie_poll_cmd().

> > +				&slot_status);
> > +	if (!ret)
> 
> Use the normal error handling logic by reversing the condition.
> 
> > +		pcie_capability_write_word(dev, PCI_EXP_SLTSTA,
> > +						PCI_EXP_SLTSTA_CC);
> > +
> > +	return  ret;
> 
> Remove extra space but this will become return 0; once the error handling 
> is done with the usual pattern.

This is not error handling, but rather "normal" and "expected", that
the command-completed bit is set and will be cleared here.

Thanks,
Feng

> 
> > +}
> > +
> >  /**
> >   * get_port_device_capability - discover capabilities of a PCI Express port
> >   * @dev: PCI Express port to examine
> > 
> 
> -- 
>  i.