Re: [PATCH V7 5/5] platform/x86: Intel PMT Crashlog capability driver

[Andy Shevchenko]

On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@xxxxxxxxxxxxxxx> 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