| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 8 Apr 2019 11:42:37 -0700
From: Rajat Jain <rajatja@...gle.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Rajneesh Bhardwaj <rajneesh.bhardwaj@...el.com>,
Vishwanath Somayaji <vishwanath.somayaji@...el.com>,
Darren Hart <dvhart@...radead.org>,
Andy Shevchenko <andy@...radead.org>,
Platform Driver <platform-driver-x86@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Wysocki@...gle.com, Rafael J <rafael.j.wysocki@...el.com>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Furquan Shaikh <furquan@...gle.com>,
Evan Green <evgreen@...gle.com>,
Rajat Jain <rajatxjain@...il.com>
Subject: Re: [PATCH v3 1/3] platform/x86: intel_pmc_core: Convert to a platform_driver
Hi,
Thank you for the review.
On Mon, Apr 8, 2019 at 9:51 AM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <rajatja@...gle.com> wrote:
> >
> > Convert the intel_pmc_core driver to a platform driver, and attach using
> > the ACPI enumeration method (via the ACPI device "INT33A1").
> >
> > Signed-off-by: Rajat Jain <rajatja@...gle.com>
>
> > -static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > - INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
> > - INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
> > - INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
> > - INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
> > - INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
> > - INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
> > - {}
> > -};
> > -
> > -MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
> > -
>
> > +static int pmc_core_probe(struct platform_device *pdev)
> > {
> > + static bool device_initialized;
> > struct pmc_dev *pmcdev = &pmc;
> > - const struct x86_cpu_id *cpu_id;
> > u64 slp_s0_addr;
> > int err;
> >
> > - cpu_id = x86_match_cpu(intel_pmc_core_ids);
> > - if (!cpu_id)
> > + if (device_initialized)
> > return -ENODEV;
> >
> > - pmcdev->map = (struct pmc_reg_map *)cpu_id->driver_data;
>
> > + switch (boot_cpu_data.x86_model) {
>
> I didn't get why this should be boot CPU?
> Otherwise, leave the structure and leave the x86_match_cpu() call.
I didn't quite understand the concern. The x86_match_cpu() also uses
the same boot_cpu_data that I've used, am I missing something?
Thanks,
Rajat
>
> > + case INTEL_FAM6_SKYLAKE_MOBILE:
> > + case INTEL_FAM6_SKYLAKE_DESKTOP:
> > + case INTEL_FAM6_KABYLAKE_MOBILE:
> > + case INTEL_FAM6_KABYLAKE_DESKTOP:
> >
> > - /*
> > - * Coffeelake has CPU ID of Kabylake and Cannonlake PCH. So here
> > - * Sunrisepoint PCH regmap can't be used. Use Cannonlake PCH regmap
> > - * in this case.
> > - */
> > - if (pmcdev->map == &spt_reg_map && !pci_dev_present(pmc_pci_ids))
> > + pmcdev->map = &spt_reg_map;
> > +
> > + /*
> > + * Special case: Coffeelake has CPU ID of Kabylake, but has
> > + * Cannonlake PCH. So need to use cnp_reg_map instead of
> > + * spt_reg_map for this special case. The PMC core PCI device
> > + * on Coffeelake is hidden, so use that as the deciding factor.
> > + */
> > + if (!pci_dev_present(pmc_pci_ids))
> > + pmcdev->map = &cnp_reg_map;
> > +
> > + break;
> > +
> > + case INTEL_FAM6_CANNONLAKE_MOBILE:
> > pmcdev->map = &cnp_reg_map;
> > + break;
> > +
> > + case INTEL_FAM6_ICELAKE_MOBILE:
> > + pmcdev->map = &icl_reg_map;
> > + break;
> > + default:
> > + /*
> > + * Which map should we use by default? If not specified
> > + * explicitly, assume Icelake by default for now.
> > + */
> > + pmcdev->map = &icl_reg_map;
> > + break;
> > + }
> >
> > if (lpit_read_residency_count_address(&slp_s0_addr))
> > pmcdev->base_addr = PMC_BASE_ADDR_DEFAULT;
> > @@ -890,26 +903,47 @@ static int __init pmc_core_probe(void)
> >
> > err = pmc_core_dbgfs_register(pmcdev);
> > if (err < 0) {
> > - pr_warn(" debugfs register failed.\n");
> > + dev_warn(&pdev->dev, "debugfs register failed.\n");
> > iounmap(pmcdev->regbase);
> > return err;
> > }
> >
> > dmi_check_system(pmc_core_dmi_table);
> > - pr_info(" initialized\n");
> > + platform_set_drvdata(pdev, pmcdev);
> > +
> > + dev_info(&pdev->dev, " initialized\n");
> > + device_initialized = true;
> > +
> > return 0;
> > }
> > -module_init(pmc_core_probe)
> >
> > -static void __exit pmc_core_remove(void)
> > +static int pmc_core_remove(struct platform_device *pdev)
> > {
> > - struct pmc_dev *pmcdev = &pmc;
> > + struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> >
> > + platform_set_drvdata(pdev, NULL);
> > pmc_core_dbgfs_unregister(pmcdev);
> > mutex_destroy(&pmcdev->lock);
> > iounmap(pmcdev->regbase);
> > + return 0;
> > }
> > -module_exit(pmc_core_remove)
> > +
> > +static const struct acpi_device_id pmc_core_acpi_ids[] = {
> > + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, pmc_core_acpi_ids);
> > +
> > +static struct platform_driver pmc_core_driver = {
> > + .driver = {
> > + .name = "pmc_core",
> > + .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
> > + },
> > + .probe = pmc_core_probe,
> > + .remove = pmc_core_remove,
> > +};
> > +
> > +module_platform_driver(pmc_core_driver);
> >
> > MODULE_LICENSE("GPL v2");
> > MODULE_DESCRIPTION("Intel PMC Core Driver");
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko
Powered by blists - more mailing lists