On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e....@linux.intel.com> wrote:
> Add support for the Intel Platform Monitoring Technology crashlog
> interface. This interface provides a few sysfs values to allow for
> controlling the crashlog telemetry interface as well as a character driver
> to allow for mapping the crashlog memory region so that it can be accessed
> after a crashlog has been recorded.
>
> This driver is meant to only support the server version of the crashlog
> which is identified as crash_type 1 with a version of zero. Currently no
> other types are supported.
...
> + The crashlog<x> directory contains files for configuring an
> + instance of a PMT crashlog device that can perform crash data
> + recoring. Each crashlog<x> device has an associated crashlog
recording
> + file. This file can be opened and mapped or read to access the
> + resulting crashlog buffer. The register layout for the buffer
> + can be determined from an XML file of specified guid for the
> + parent device.
...
> + (RO) The guid for this crashlog device. The guid identifies
> the
guid -> GUID
Please, spell check all ABI files in this series.
...
> +config INTEL_PMT_CRASHLOG
> + tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
> + select INTEL_PMT_CLASS
> + help
> + The Intel Platform Monitoring Technology (PMT) crashlog driver
> provides
> + access to hardware crashlog capabilities on devices that support the
> + feature.
Name of the module?
...
> + /*
> + * Currenty we only recognize OOBMSM version 0 devices.
Currently. Please spell check all comments in the code.
> + * We can ignore all other crashlog devices in the system.
> + */
...
> + /* clear control bits */
What new information readers get from this comment?
> + control &= ~(CRASHLOG_FLAG_MASK | CRASHLOG_FLAG_DISABLE);
How does the second constant play any role here?
...
> + /* clear control bits */
Ditto. And moreover it's ambiguous due to joined two lines below.
> + control &= ~CRASHLOG_FLAG_MASK;
> + control |= CRASHLOG_FLAG_EXECUTE;
...
> + return strnlen(buf, count);
How is this different to count?
...
> + struct crashlog_entry *entry;
> + bool trigger;
> + int result;
> +
> + entry = dev_get_drvdata(dev);
You may reduce LOCs by direct assigning in the definition block above.
...
> + result = strnlen(buf, count);
How is it different from count?
...
> +static DEFINE_XARRAY_ALLOC(crashlog_array);
> +static struct intel_pmt_namespace pmt_crashlog_ns = {
> + .name = "crashlog",
> + .xa = &crashlog_array,
> + .attr_grp = &pmt_crashlog_group
Leave the comma here.
> +};
...
> + ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, parent);
> + if (!ret)
> + return 0;
> +
> + dev_err(parent, "Failed to add crashlog controls\n");
> + intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
> +
> + return ret;
Can we use traditional patterns?
if (ret) {
...
}
return ret;
...
> + size = offsetof(struct pmt_crashlog_priv, entry[pdev->num_resources]);
> + priv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
struct_size()
...
> + /* initialize control mutex */
> + mutex_init(&priv->entry[i].control_mutex);
> +
> + disc_res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> + if (!disc_res)
> + goto abort_probe;
> +
> + ret = intel_pmt_ioremap_discovery_table(entry, pdev, i);
> + if (ret)
> + goto abort_probe;
> +
> + if (!pmt_crashlog_supported(entry))
> + continue;
> +
> + ret = pmt_crashlog_add_entry(entry, &pdev->dev, disc_res);
> + if (ret)
> + goto abort_probe;
> +
> + priv->num_entries++;
Are you going to duplicate this in each driver? Consider to refactor
to avoid duplication of a lot of code.
...
> + .name = DRV_NAME,
> +MODULE_ALIAS("platform:" DRV_NAME);
I'm not sure I have interpreted this:
- Use 'raw' string instead of defines for device names
correctly. Can you elaborate?
--
With Best Regards,
Andy Shevchenko
