Re: [PATCH V3 3/5] PCI: Save and restore the Device 3 Control register

[Lukas Wunner <lukas@wunner.de>]

On Sat, Aug 15, 2026 at 01:46:19AM +0530, Vidya Sagar wrote:
> +static void pci_restore_dev3_state(struct pci_dev *dev)
[...]
> +	/*
> +	 * DEV3_CTL.14-Bit Tag Requester Enable is only meaningful in flit
> +	 * mode.  On devices that advertise 14-Bit Tag Requester support,
> +	 * sanitize the saved value before writing it back, so that callers
> +	 * that issue further TLPs through this device after restore see a
> +	 * coherent enable state.  Every other bit of DEV3_CTL (DMWr
> +	 * Requester Enable, DMWr Egress Blocking, L0p Enable, Target Link
> +	 * Width and any future addition) is written back unchanged.
> +	 *
> +	 * Only the value written to hardware is adjusted.  The save buffer
> +	 * keeps what was saved, so the decision is taken afresh on every
> +	 * restore and the originally programmed value is not lost.
> +	 */

This is quite verbose, can you condense it a bit?  E.g. it's not
necessary to say that every other bit is written back unchanged.
That's the default.  Just say e.g.:

       /* Clear 14-Bit Tag Requester Enable if link is no longer in Flit Mode */

> +	pci_read_config_dword(dev, pos + PCI_DEV3_CAP, &dev3_cap);
> +	if (dev3_cap & PCI_DEV3_CAP_14BIT_TAG_REQ) {

PCIe r7.0 sec 7.7.10.3 says the bit is "VF RsvdP", so you need
to check !dev->is_virtfn here.

> +		/*
> +		 * Check both LNKSTA2.Flit_Mode (link-level) and
> +		 * DEV3_STA.Segment Captured (end-to-end); both must be
> +		 * active for 14-bit tags.  Refresh bus->flit_mode and
> +		 * dev->fm_enabled in lock-step.
> +		 */

Why do we have to check both?

We usually spell out the register names fully or leave them out.
Using "LNKSTA2.Flit_Mode" and "DEV3_STA.Segment Captured" instead looks
odd to me.

> +		pci_read_config_dword(dev, pos + PCI_DEV3_STA, &dev3_sta);
> +		dev->fm_enabled = !!(dev3_sta & PCI_DEV3_STA_SEGMENT);
> +
> +		pcie_capability_read_word(dev, PCI_EXP_LNKSTA2, &lnksta2);
> +		flit_now = !!(lnksta2 & PCI_EXP_LNKSTA2_FLIT);
> +		if (dev->bus)
> +			dev->bus->flit_mode = flit_now;

When can dev->bus be NULL?

> +
> +		if ((!dev->fm_enabled || !flit_now) &&
> +		    (val & PCI_DEV3_CTL_14BIT_TAG_REQ_EN)) {
> +			val &= ~PCI_DEV3_CTL_14BIT_TAG_REQ_EN;
> +			pci_info(dev, "clearing 14-Bit Tag Requester Enable: flit mode no longer active (LNKSTA2=%#06x, DEV3_STA=%#010x)\n",
> +				 lnksta2, dev3_sta);

pci_dbg() please.

> +	/*
> +	 * Save buffer for DEV3_CTL only.  Every field in DEV3_STA is
> +	 * read-only status reported by hardware, so there is nothing there
> +	 * to restore.
> +	 */

The code comment seems unnecessary because that policy applies
to every other Extended Capability as well.  We only save
mutable registers, that's the default.

Thanks,

Lukas