| 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: Fri, 26 Jan 2018 18:17:17 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com>
Cc: Darren Hart <dvhart@...radead.org>,
Andy Shevchenko <andy@...radead.org>,
Zha Qipeng <qipeng.zha@...el.com>,
"Krogerus, Heikki" <heikki.krogerus@...ux.intel.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Platform Driver <platform-driver-x86@...r.kernel.org>,
Sathyanarayanan Kuppuswamy Natarajan <sathyaosid@...il.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v1 1/2] platform/x86: intel_pmc_ipc: Use MFD framework to
create dependent devices
On Fri, Jan 26, 2018 at 12:53 AM,
<sathyanarayanan.kuppuswamy@...ux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>
> Currently, we have lot of repetitive code in dependent device resource
> allocation and device creation handling code. This logic can be improved if
> we use MFD framework for dependent device creation. This patch adds this
> support.
Thanks for an update. My comments below.
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
First of all, I barely remember what I did to this patch. In any case
this one is redundant since it will have mine when I push it to our
repo.
> @@ -508,7 +492,7 @@ static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
> pmc);
> if (ret) {
> - dev_err(&pdev->dev, "Failed to request irq\n");
> + dev_err(&pdev->dev, "Failed to request IRQ\n");
> return ret;
> }
Split this kind of changes in a separate patch.
> +static int ipc_create_punit_device(struct platform_device *pdev)
> {
> + static struct resource punit_res[PLAT_RESOURCE_MEM_MAX_INDEX];
> + static struct mfd_cell punit_cell;
> + struct resource *res;
> + int ret, mindex, pindex = 0;
> +
> + for (mindex = 0; mindex <= PLAT_RESOURCE_MEM_MAX_INDEX; mindex++) {
'<=' ??? (Why = is here?)
> + res = platform_get_resource(pdev, IORESOURCE_MEM, mindex);
> +
> + switch (mindex) {
> + /* Get PUNIT resources */
> + case PLAT_RESOURCE_BIOS_DATA_INDEX:
> + case PLAT_RESOURCE_BIOS_IFACE_INDEX:
> + /* BIOS resources are required, so return error if not
> + * available
> + */
It's not the network subsystem, please, do a proper style for
multi-line comments.
> + if (!res) {
Would the following work for you?
if (res)
break;
dev_err(&pdev->dev, "Failed to get PUNIT MEM resource %d\n", pindex);
return -ENXIO;
case ...:
...
if (res)
break;
default:
continue;
memcpy(...);
...
> + dev_err(&pdev->dev,
> + "Failed to get PUNIT MEM resource %d\n",
> + pindex);
> + return -ENXIO;
> + }
> + case PLAT_RESOURCE_ISP_DATA_INDEX:
> + case PLAT_RESOURCE_ISP_IFACE_INDEX:
> + case PLAT_RESOURCE_GTD_DATA_INDEX:
> + case PLAT_RESOURCE_GTD_IFACE_INDEX:
> + /* if valid resource found, copy the resource to PUNIT
> + * resource
> + */
> + if (res)
> + memcpy(&punit_res[pindex], res, sizeof(*res));
> + punit_res[pindex].flags = IORESOURCE_MEM;
> + pindex++;
> + break;
> };
> + }
> + ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &punit_cell,
> + 1, NULL, 0, NULL);
>
> + dev_dbg(&pdev->dev, "Successfully created PUNIT device\n");
>
Wrong. If ret is not 0 the message is misleading.
Just remove it.
Same for the rest cases.
> + return ret;
> }
> +static int ipc_create_wdt_device(struct platform_device *pdev)
> {
> + static struct resource wdt_ipc_res[2];
> + static struct mfd_cell wdt_cell;
> struct resource *res;
> + int ret;
>
> + /* If we have ACPI based watchdog use that instead, othewise create
> + * a MFD cell for iTCO watchdog
> + */
Style.
> + if (acpi_has_watchdog())
> + return 0;
> + ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, &wdt_cell,
> + 1, NULL, 0, NULL);
> +
> + dev_dbg(&pdev->dev, "Successfully created watchdog device\n");
> +
> + return ret;
What Heikki meant is to fill cells by those helper functions and call
mfd_add_devices() only once.
See lpc_ich.c as an example.
> }
> +static int ipc_create_pmc_devices(struct platform_device *pdev)
> {
> int ret;
>
> + ret = ipc_create_punit_device(pdev);
> + if (ret < 0)
> + return ret;
Is it fatal? (Hint: it's quite likely not)
> + ret = ipc_create_wdt_device(pdev);
> + if (ret < 0)
> + return ret;
Is it fatal?
> + ret = ipc_create_telemetry_device(pdev);
> + if (ret < 0)
> + return ret;
Is it fatal?
> + return 0;
> }
> + ret = devm_request_irq(&pdev->dev, ipcdev.irq, ioc, IRQF_NO_SUSPEND,
> + "intel_pmc_ipc", &ipcdev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request IRQ\n");
> + return ret;
> }
>
> ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group);
> if (ret) {
> dev_err(&pdev->dev, "Failed to create sysfs group %d\n",
> ret);
> - goto err_sys;
> + devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);
Why do you need this one?
> + return ret;
> }
>
> ipcdev.has_gcr_regs = true;
>
> return 0;
> }
And to the main question, what this is doing in PDx86 now? There
should be a patch to move it under drivers/mfd.
In _any case_ I need an Ack from Lee.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists