linux-kernel - Re: [PATCH v4 37/38] platform/x86: intel_pmc

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Jan 2020 13:27:57 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Darren Hart <dvhart@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Zha Qipeng <qipeng.zha@...el.com>,
        "David E . Box" <david.e.box@...ux.intel.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Mark Brown <broonie@...nel.org>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 37/38] platform/x86: intel_pmc_ipc: Convert to MFD

On Wed, 22 Jan 2020, Mika Westerberg wrote:

> On Wed, Jan 22, 2020 at 12:34:54PM +0000, Lee Jones wrote:
> > > +static int intel_pmc_probe(struct platform_device *pdev)
> > > +{
> > > +	struct intel_scu_ipc_pdata pdata = {};
> > > +	struct intel_pmc_dev *pmc;
> > > +	int ret;
> > > +
> > > +	pmc = devm_kzalloc(&pdev->dev, sizeof(*pmc), GFP_KERNEL);
> > > +	if (!pmc)
> > > +		return -ENOMEM;
> > > +
> > > +	pmc->dev = &pdev->dev;
> > > +	spin_lock_init(&pmc->gcr_lock);
> > > +
> > > +	ret = intel_pmc_get_resources(pdev, pmc, &pdata);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Failed to request resources\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	pmc->scu = devm_intel_scu_ipc_register(&pdev->dev, &pdata);
> > > +	if (IS_ERR(pmc->scu))
> > > +		return PTR_ERR(pmc->scu);
> > 
> > *_register is better than *_probe.  If it was called that (or maybe
> > *_init) initially I may have missed the issue altogether ...
> > 
> > However, I still think it the SCU IPC *device* needs to be a device
> > driver and abide by the rules, ensuring it uses the device driver
> > model/API.  As such, it should be registered and probed as a device.
> 
> Which type of device you suggest here? And which bus it should be
> registered to? I think we can make this create a platform_device but
> then we would need to do that from the PCI driver as well which seems
> unnecessary since we already have the struct pci_dev.

What kind of device is it?

Refrain from using platform device, unless it is one please.

> For instance in drivers/mfd/intel-lpss* we use similar approach (the
> core part is library that gets called by probe drivers (ACPI, PCI). We
> don't create any additional platform_devices.

That's different.  Here the *-acpi.c and *-pci.c are only used as
registration hooks into the same device.  The semantics we're
discussing are seemingly used to probe/init a different device in a
separate subsystem.

> There is another twist. Ideally we would like to see the SCU IPC probed
> and intialized before the MFD children so that we know the SCU IPC is
> ready by the time the children devices are created. I guess we could
> work it around by returning -EPROBE_DEFER but that does not feel right
> to be honest.

That's precisely what -EPROBE_DEFER was designed for.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog