Re: [PATCH V3 3/6] platform/x86/intel: Move intel_pmt from MFD to Auxiliary Bus

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Dec 13, 2021 at 09:59:18AM -0800, David E. Box wrote:
> Intel Platform Monitoring Technology (PMT) support is indicated by presence
> of an Intel defined PCIe Designated Vendor Specific Extended Capabilities
> (DVSEC) structure with a PMT specific ID. The current MFD implementation
> creates child devices for each PMT feature, currently telemetry, watcher,
> and crashlog. However DVSEC structures may also be used by Intel to
> indicate support for other features. The Out Of Band Management Services
> Module (OOBMSM) uses DVSEC to enumerate several features, including PMT.
> In order to support them it is necessary to modify the intel_pmt driver to
> handle the creation of the child devices more generically. To that end,
> modify the driver to create child devices for any VSEC/DVSEC features on
> supported devices (indicated by PCI ID).  Additionally, move the
> implementation from MFD to the Auxiliary bus.  VSEC/DVSEC features are
> really multifunctional PCI devices, not platform devices as MFD was
> designed for. Auxiliary bus gives more flexibility by allowing the
> definition of custom structures that can be shared between associated
> auxiliary devices and the parent device. Also, rename the driver from
> intel_pmt to intel_vsec to better reflect the purpose.
> 
> This series also removes the current runtime pm support which was not
> complete to begin with. None of the current devices require runtime pm.
> However the support will be replaced when a device is added that requires
> it.

...

> +static bool intel_vsec_walk_dvsec(struct pci_dev *pdev, unsigned long quirks)
> +{
> +	bool have_devices = false;
> +	int pos = 0;
> +
> +	do {
> +		struct intel_vsec_header header;
> +		u32 table, hdr;
> +		u16 vid;
> +		int ret;
> +
> +		pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC);
> +		if (!pos)
> +			break;
> +
> +		pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER1, &hdr);
> +		vid = PCI_DVSEC_HEADER1_VID(hdr);
> +		if (vid != PCI_VENDOR_ID_INTEL)
> +			continue;
> +
> +		/* Support only revision 1 */
> +		header.rev = PCI_DVSEC_HEADER1_REV(hdr);
> +		if (header.rev != 1) {
> +			dev_info(&pdev->dev, "Unsupported DVSEC revision %d\n", header.rev);
> +			continue;
> +		}
> +
> +		header.length = PCI_DVSEC_HEADER1_LEN(hdr);
> +
> +		pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES, &header.num_entries);
> +		pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE, &header.entry_size);
> +		pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE, &table);
> +
> +		header.tbir = INTEL_DVSEC_TABLE_BAR(table);
> +		header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
> +
> +		pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER2, &hdr);
> +		header.id = PCI_DVSEC_HEADER2_ID(hdr);
> +
> +		ret = intel_vsec_add_dev(pdev, &header, quirks);
> +		if (ret)
> +			continue;
> +
> +		have_devices = true;
> +	} while (true);
> +
> +	return have_devices;
> +}
> +
> +static bool intel_vsec_walk_vsec(struct pci_dev *pdev, unsigned long quirks)
> +{
> +	bool have_devices = false;
> +	int pos = 0;
> +
> +	do {
> +		struct intel_vsec_header header;
> +		u32 table, hdr;
> +		int ret;
> +
> +		pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_VNDR);
> +		if (!pos)
> +			break;
> +
> +		pci_read_config_dword(pdev, pos + PCI_VNDR_HEADER, &hdr);
> +
> +		/* Support only revision 1 */
> +		header.rev = PCI_VNDR_HEADER_REV(hdr);
> +		if (header.rev != 1) {
> +			dev_info(&pdev->dev, "Unsupported VSEC revision %d\n", header.rev);
> +			continue;
> +		}
> +
> +		header.id = PCI_VNDR_HEADER_ID(hdr);
> +		header.length = PCI_VNDR_HEADER_LEN(hdr);
> +
> +		/* entry, size, and table offset are the same as DVSEC */
> +		pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES, &header.num_entries);
> +		pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE, &header.entry_size);
> +		pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE, &table);
> +
> +		header.tbir = INTEL_DVSEC_TABLE_BAR(table);
> +		header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
> +
> +		ret = intel_vsec_add_dev(pdev, &header, quirks);
> +		if (ret)
> +			continue;
> +
> +		have_devices = true;
> +	} while (true);
> +
> +	return have_devices;
> +}


I'm wondering if it makes sense to refactor each of the above to something like

int intel_vsec_extract_vsec(...)
{
	...
}

static bool intel_vsec_walk_dvsec(struct pci_dev *pdev, unsigned long quirks)
{
	bool have_devices = false;
	int pos;

	while ((pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC))) {
		if (intel_vsec_extract_vsec())
			continue;

		have_devices = true;
	}

	return have_devices;
}

Either way, it may be worth to convert infinite loops to ones with the clear
exit condition.

...

> +	/*
> +	 * Driver cleanup handled by intel_vsec_remove_aux() which is added
> +	 * to the pci device as a devm action

PCI

Grammatical period at the end.

> +	 */

-- 
With Best Regards,
Andy Shevchenko