lkml.org 
[lkml]   [2020]   [Oct]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH V7 5/5] platform/x86: Intel PMT Crashlog capability driver
On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@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

\
 
 \ /
  Last update: 2020-10-01 18:38    [W:0.090 / U:1.928 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site