On Fri, May 8, 2020 at 5:18 AM David E. Box <david.e.box@xxxxxxxxxxxxxxx> wrote:
>
> Intel Platform Monitoring Technology (PMT) is an architecture for
> enumerating and accessing hardware monitoring facilities. PMT supports
> multiple types of monitoring capabilities. This driver creates platform
> devices for each type so that they may be managed by capability specific
> drivers (to be introduced). Capabilities are discovered using PCIe DVSEC
> ids. Support is included for the 3 current capability types, Telemetry,
> Watcher, and Crashlog. The features are available on new Intel platforms
> starting from Tiger Lake for which support is added. Tiger Lake however
> will not support Watcher and Crashlog even though the capabilities appear
> on the device. So add a quirk facility and use it to disable them.
Thank you for an update.
Some nitpicks below.
...
> + case DVSEC_INTEL_ID_TELEM:
Is this from the spec? Or can we also spell TELEMETRY ?
> + name = TELEM_DEV_NAME;
Ditto for all occurrences.
> + break;
...
> + cell = devm_kcalloc(&pdev->dev, header->num_entries,
> + sizeof(*cell), GFP_KERNEL);
I think if you use temporary
struct device *dev = &pdev->dev;
you may squeeze this to one line and make others smaller as well.
> + if (!cell)
> + return -ENOMEM;
...
> + res->start = pdev->resource[header->tbir].start +
> + header->offset +
> + (i * (INTEL_DVSEC_ENTRY_SIZE << 2));
Outer parentheses are redundant. And perhaps last two lines can be one.
...
> +static int
> +pmt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + u16 vid;
> + u32 table;
> + int ret, pos = 0, last_pos = 0;
Redundant assignment of pos.
> + while ((pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC))) {
> + pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER1, &vid);
> + if (vid != PCI_VENDOR_ID_INTEL)
> + continue;
> +
> + last_pos = pos;
Can we simple use a boolean flag?
> + }
> +
> + if (!last_pos) {
> + dev_err(&pdev->dev, "No supported PMT capabilities found.\n");
> + return -ENODEV;
> + }
> +}
...
> +};
> +
Extra blank line.
> +module_pci_driver(pmt_pci_driver);
...
+ bits.h since GENMASK() is in use.
> +#include <linux/types.h>
...
> +enum pmt_quirks {
> + /* Watcher capability not supported */
> + PMT_QUIRK_NO_WATCHER = (1 << 0),
BIT() ?
> +
> + /* Crashlog capability not supported */
> + PMT_QUIRK_NO_CRASHLOG = (1 << 1),
BIT() ?
> +};
--
With Best Regards,
Andy Shevchenko
