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@xxxxxxxxxxxxxxx> # Use to read_poll_timeout()
Thanks for the suggestion!
>
> > Signed-off-by: Feng Tang <feng.tang@xxxxxxxxxxxxxxxxx>
> > ---
> > 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.
