| 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
| ||
|
Message-ID: <9d424592-c157-417a-9d6e-d12d80e19829@amd.com>
Date: Thu, 29 Aug 2024 20:25:27 +0530
From: "Goswami, Sanket" <Sanket.Goswami@....com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Andi Shyti <andi.shyti@...nel.org>, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
Jan Dabros <jsd@...ihalf.com>, Narasimhan.V@....com,
Borislav Petkov <bp@...en8.de>, Kim Phillips <kim.phillips@....com>,
Shyam-sundar.S-k@....com
Subject: Re: [PATCH v2 1/1] i2c: designware: Consolidate PM ops
Adding Shyam (as he asked me to check this change on AMD systems)
On 8/27/2024 8:30 PM, Andy Shevchenko wrote:
> We have the same (*) PM ops in the PCI and plaform drivers.
> Instead, consolidate that PM ops under exported variable and
> deduplicate them.
>
> *)
> With the subtle ACPI and P-Unit behaviour differences in PCI case.
> But this is not a problem as for ACPI we need to take care of the
> P-Unit semaphore anyway and calling PM ops for PCI makes sense as
> it might provide specific operation regions in ACPI (however there
> are no known devices on market that are using it with PCI enabled I2C).
> Note, the clocks are not in use in the PCI case.
>
> Reviewed-by: Andi Shyti <andi.shyti@...nel.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Tested-by: Sanket Goswami <Sanket.Goswami@....com>
Thanks,
Sanket
> ---
>
> This is just a split of a single patch from bigger series [1].
> It was made in no functionality change manner, except the PCI case
> described above. But since it touches PM and it was an area where
> regressions were observed I would like to have a formal Tested-by
> from AMD people to make sure we don't break their setup /
> configurations.
>
> Link: https://lore.kernel.org/linux-i2c/20231207141653.2785124-1-andriy.shevchenko@linux.intel.com/ [1]
>
> v2: fixed rebase issue (LKP)
>
> drivers/i2c/busses/i2c-designware-common.c | 62 ++++++++++++++++++++
> drivers/i2c/busses/i2c-designware-core.h | 3 +
> drivers/i2c/busses/i2c-designware-pcidrv.c | 44 +-------------
> drivers/i2c/busses/i2c-designware-platdrv.c | 64 +--------------------
> 4 files changed, 68 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index b60c55587e48..fb65fe6d8122 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -21,6 +21,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/pm.h>
> #include <linux/pm_runtime.h>
> #include <linux/property.h>
> #include <linux/regmap.h>
> @@ -736,5 +737,66 @@ void i2c_dw_disable(struct dw_i2c_dev *dev)
> }
> EXPORT_SYMBOL_GPL(i2c_dw_disable);
>
> +static int i2c_dw_prepare(struct device *device)
> +{
> + /*
> + * If the ACPI companion device object is present for this device,
> + * it may be accessed during suspend and resume of other devices via
> + * I2C operation regions, so tell the PM core and middle layers to
> + * avoid skipping system suspend/resume callbacks for it in that case.
> + */
> + return !has_acpi_companion(device);
> +}
> +
> +static int i2c_dw_runtime_suspend(struct device *device)
> +{
> + struct dw_i2c_dev *dev = dev_get_drvdata(device);
> +
> + if (dev->shared_with_punit)
> + return 0;
> +
> + i2c_dw_disable(dev);
> + i2c_dw_prepare_clk(dev, false);
> +
> + return 0;
> +}
> +
> +static int i2c_dw_suspend(struct device *device)
> +{
> + struct dw_i2c_dev *dev = dev_get_drvdata(device);
> +
> + i2c_mark_adapter_suspended(&dev->adapter);
> +
> + return i2c_dw_runtime_suspend(device);
> +}
> +
> +static int i2c_dw_runtime_resume(struct device *device)
> +{
> + struct dw_i2c_dev *dev = dev_get_drvdata(device);
> +
> + if (!dev->shared_with_punit)
> + i2c_dw_prepare_clk(dev, true);
> +
> + dev->init(dev);
> +
> + return 0;
> +}
> +
> +static int i2c_dw_resume(struct device *device)
> +{
> + struct dw_i2c_dev *dev = dev_get_drvdata(device);
> +
> + i2c_dw_runtime_resume(device);
> + i2c_mark_adapter_resumed(&dev->adapter);
> +
> + return 0;
> +}
> +
> +EXPORT_GPL_DEV_PM_OPS(i2c_dw_dev_pm_ops) = {
> + .prepare = pm_sleep_ptr(i2c_dw_prepare),
> + LATE_SYSTEM_SLEEP_PM_OPS(i2c_dw_suspend, i2c_dw_resume)
> + RUNTIME_PM_OPS(i2c_dw_runtime_suspend, i2c_dw_runtime_resume, NULL)
> +};
> +
> MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 723d599cca93..c6bd6f65a2d3 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -15,6 +15,7 @@
> #include <linux/dev_printk.h>
> #include <linux/errno.h>
> #include <linux/i2c.h>
> +#include <linux/pm.h>
> #include <linux/regmap.h>
> #include <linux/types.h>
>
> @@ -341,6 +342,8 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev);
> int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev);
> u32 i2c_dw_func(struct i2c_adapter *adap);
>
> +extern const struct dev_pm_ops i2c_dw_dev_pm_ops;
> +
> static inline void __i2c_dw_enable(struct dw_i2c_dev *dev)
> {
> dev->status |= STATUS_ACTIVE;
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index dd188ccd961e..04377533f3ae 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -19,6 +19,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> +#include <linux/pm.h>
> #include <linux/pm_runtime.h>
> #include <linux/power_supply.h>
> #include <linux/sched.h>
> @@ -194,47 +195,6 @@ static struct dw_pci_controller dw_pci_controllers[] = {
> },
> };
>
> -static int __maybe_unused i2c_dw_pci_runtime_suspend(struct device *dev)
> -{
> - struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> - i2c_dw_disable(i_dev);
> - return 0;
> -}
> -
> -static int __maybe_unused i2c_dw_pci_suspend(struct device *dev)
> -{
> - struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> - i2c_mark_adapter_suspended(&i_dev->adapter);
> -
> - return i2c_dw_pci_runtime_suspend(dev);
> -}
> -
> -static int __maybe_unused i2c_dw_pci_runtime_resume(struct device *dev)
> -{
> - struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> - return i_dev->init(i_dev);
> -}
> -
> -static int __maybe_unused i2c_dw_pci_resume(struct device *dev)
> -{
> - struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> - int ret;
> -
> - ret = i2c_dw_pci_runtime_resume(dev);
> -
> - i2c_mark_adapter_resumed(&i_dev->adapter);
> -
> - return ret;
> -}
> -
> -static const struct dev_pm_ops i2c_dw_pm_ops = {
> - SET_SYSTEM_SLEEP_PM_OPS(i2c_dw_pci_suspend, i2c_dw_pci_resume)
> - SET_RUNTIME_PM_OPS(i2c_dw_pci_runtime_suspend, i2c_dw_pci_runtime_resume, NULL)
> -};
> -
> static const struct property_entry dgpu_properties[] = {
> /* USB-C doesn't power the system */
> PROPERTY_ENTRY_U8("scope", POWER_SUPPLY_SCOPE_DEVICE),
> @@ -402,7 +362,7 @@ static struct pci_driver dw_i2c_driver = {
> .probe = i2c_dw_pci_probe,
> .remove = i2c_dw_pci_remove,
> .driver = {
> - .pm = &i2c_dw_pm_ops,
> + .pm = pm_ptr(&i2c_dw_dev_pm_ops),
> },
> .id_table = i2c_designware_pci_ids,
> };
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index e49c68c6e142..285ba4c1803f 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -29,7 +29,6 @@
> #include <linux/reset.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> -#include <linux/suspend.h>
> #include <linux/units.h>
>
> #include "i2c-designware-core.h"
> @@ -339,67 +338,6 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
> reset_control_assert(dev->rst);
> }
>
> -static int dw_i2c_plat_prepare(struct device *dev)
> -{
> - /*
> - * If the ACPI companion device object is present for this device, it
> - * may be accessed during suspend and resume of other devices via I2C
> - * operation regions, so tell the PM core and middle layers to avoid
> - * skipping system suspend/resume callbacks for it in that case.
> - */
> - return !has_acpi_companion(dev);
> -}
> -
> -static int dw_i2c_plat_runtime_suspend(struct device *dev)
> -{
> - struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> - if (i_dev->shared_with_punit)
> - return 0;
> -
> - i2c_dw_disable(i_dev);
> - i2c_dw_prepare_clk(i_dev, false);
> -
> - return 0;
> -}
> -
> -static int dw_i2c_plat_suspend(struct device *dev)
> -{
> - struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> - i2c_mark_adapter_suspended(&i_dev->adapter);
> -
> - return dw_i2c_plat_runtime_suspend(dev);
> -}
> -
> -static int dw_i2c_plat_runtime_resume(struct device *dev)
> -{
> - struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> - if (!i_dev->shared_with_punit)
> - i2c_dw_prepare_clk(i_dev, true);
> -
> - i_dev->init(i_dev);
> -
> - return 0;
> -}
> -
> -static int dw_i2c_plat_resume(struct device *dev)
> -{
> - struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> - dw_i2c_plat_runtime_resume(dev);
> - i2c_mark_adapter_resumed(&i_dev->adapter);
> -
> - return 0;
> -}
> -
> -static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
> - .prepare = pm_sleep_ptr(dw_i2c_plat_prepare),
> - LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> - RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend, dw_i2c_plat_runtime_resume, NULL)
> -};
> -
> static const struct of_device_id dw_i2c_of_match[] = {
> { .compatible = "snps,designware-i2c", },
> { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> @@ -442,7 +380,7 @@ static struct platform_driver dw_i2c_driver = {
> .name = "i2c_designware",
> .of_match_table = dw_i2c_of_match,
> .acpi_match_table = dw_i2c_acpi_match,
> - .pm = pm_ptr(&dw_i2c_dev_pm_ops),
> + .pm = pm_ptr(&i2c_dw_dev_pm_ops),
> },
> .id_table = dw_i2c_platform_ids,
> };
Powered by blists - more mailing lists