| 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: Sat, 27 Dec 2014 13:58:41 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Gregory CLEMENT <gregory.clement@...e-electrons.com>,
Tejun Heo <tj@...nel.org>, linux-ide@...r.kernel.org,
linux-kernel@...r.kernel.org
CC: Antoine Ténart
<antoine.tenart@...e-electrons.com>,
Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
Maxime Ripard <maxime.ripard@...e-electrons.com>,
Boris BREZILLON <boris.brezillon@...e-electrons.com>,
Lior Amsalem <alior@...vell.com>,
Tawfik Bayouk <tawfik@...vell.com>,
Nadav Haklai <nadavh@...vell.com>,
Mark Rutland <mark.rutland@....com>, devicetree@...r.kernel.org
Subject: Re: [PATCH 3/3] ata: libahci: Allow to use multiple regulators
Hi Gregory,
Thanks for working on this. Overall the patch-set / concept looks good,
you can add "Acked-by: Hans de Goede <hdegoede@...hat.com>" to the first
2 patches.
I've some comments on this patch, see below.
On 27-12-14 11:34, Gregory CLEMENT wrote:
> The current implementation of the libahci allows using multiple PHYs
> but not multiple regulators. This patch adds the support of multiple
> regulators. Until now it was mandatory to have a PHY under a subnode,
> now a port subnode can contain either a regulator or a PHY (or both).
>
> There was only one driver which used directly the regulator field of
> the ahci_host_priv structure. To preserve the bisectability the change
> in the ahci_imx driver was done in the same patch.
>
> This patch also depend of a patch of the regulator framework:
> "regulator: core: Add the device tree version to the regulator_get
> family"
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@...e-electrons.com>
> ---
> drivers/ata/ahci.h | 2 +-
> drivers/ata/ahci_imx.c | 14 +--
> drivers/ata/libahci_platform.c | 206 ++++++++++++++++++++++++++++-------------
> include/linux/ahci_platform.h | 2 +
> 4 files changed, 151 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 40f0e34f17af..275358ae0b3f 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -333,7 +333,7 @@ struct ahci_host_priv {
> u32 em_msg_type; /* EM message type */
> bool got_runtime_pm; /* Did we do pm_runtime_get? */
> struct clk *clks[AHCI_MAX_CLKS]; /* Optional */
> - struct regulator *target_pwr; /* Optional */
> + struct regulator **target_pwrs; /* Optional */
> /*
> * If platform uses PHYs. There is a 1:1 relation between the port number and
> * the PHY position in this array.
> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
> index 35d51c59a370..41632e57d46f 100644
> --- a/drivers/ata/ahci_imx.c
> +++ b/drivers/ata/ahci_imx.c
> @@ -221,11 +221,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
> if (imxpriv->no_device)
> return 0;
>
> - if (hpriv->target_pwr) {
> - ret = regulator_enable(hpriv->target_pwr);
> - if (ret)
> - return ret;
> - }
> + ret = ahci_platform_enable_regulators(hpriv);
> + if (ret)
> + return ret;
>
> ret = clk_prepare_enable(imxpriv->sata_ref_clk);
> if (ret < 0)
> @@ -270,8 +268,7 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
> disable_clk:
> clk_disable_unprepare(imxpriv->sata_ref_clk);
> disable_regulator:
> - if (hpriv->target_pwr)
> - regulator_disable(hpriv->target_pwr);
> + ahci_platform_disable_regulators(hpriv);
>
> return ret;
> }
> @@ -291,8 +288,7 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
>
> clk_disable_unprepare(imxpriv->sata_ref_clk);
>
> - if (hpriv->target_pwr)
> - regulator_disable(hpriv->target_pwr);
> + ahci_platform_disable_regulators(hpriv);
> }
>
> static void ahci_imx_error_handler(struct ata_port *ap)
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index a147aaadca85..f3bf4311152d 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -138,6 +138,59 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
> EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
>
> /**
> + * ahci_platform_enable_regulators - Enable regulators
> + * @hpriv: host private area to store config values
> + *
> + * This function enables all the regulators found in
> + * hpriv->target_pwrs, if any. If a regulator fails to be enabled, it
> + * disables all the regulators already enabled in reverse order and
> + * returns an error.
> + *
> + * RETURNS:
> + * 0 on success otherwise a negative error code
> + */
> +int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
> +{
> + int rc, i;
> +
> + for (i = 0; i < hpriv->nports; i++) {
> + if (!hpriv->target_pwrs[i])
> + continue;
> +
> + rc = regulator_enable(hpriv->target_pwrs[i]);
> + if (rc)
> + goto disable_target_pwrs;
> + }
> +
> + return 0;
> +
> +disable_target_pwrs:
> + while (--i >= 0)
> + if (!hpriv->target_pwrs[i])
I'm pretty sure you want:
if (hpriv->target_pwrs[i])
here, so without the '!' .
> + regulator_disable(hpriv->target_pwrs[i]);
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
> +
> +/**
> + * ahci_platform_disable_regulators - Disable regulators
> + * @hpriv: host private area to store config values
> + *
> + * This function disables all regulators found in hpriv->target_pwrs.
> + */
> +void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
> +{
> + int i;
> +
> + for (i = 0; i < hpriv->nports; i++) {
> + if (!hpriv->target_pwrs[i])
> + continue;
> + regulator_disable(hpriv->target_pwrs[i]);
> + }
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
> +/**
> * ahci_platform_enable_resources - Enable platform resources
> * @hpriv: host private area to store config values
> *
> @@ -157,11 +210,9 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
> {
> int rc;
>
> - if (hpriv->target_pwr) {
> - rc = regulator_enable(hpriv->target_pwr);
> - if (rc)
> - return rc;
> - }
> + rc = ahci_platform_enable_regulators(hpriv);
> + if (rc)
> + return rc;
>
> rc = ahci_platform_enable_clks(hpriv);
> if (rc)
> @@ -177,8 +228,8 @@ disable_clks:
> ahci_platform_disable_clks(hpriv);
>
> disable_regulator:
> - if (hpriv->target_pwr)
> - regulator_disable(hpriv->target_pwr);
> + ahci_platform_disable_regulators(hpriv);
> +
> return rc;
> }
> EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
> @@ -199,8 +250,7 @@ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
>
> ahci_platform_disable_clks(hpriv);
>
> - if (hpriv->target_pwr)
> - regulator_disable(hpriv->target_pwr);
> + ahci_platform_disable_regulators(hpriv);
> }
> EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
>
> @@ -218,6 +268,59 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
> clk_put(hpriv->clks[c]);
> }
>
> +static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
> + struct device *dev, struct device_node *node)
> +{
> + int rc;
> +
> + hpriv->phys[port] = devm_of_phy_get(dev, node, NULL);
> +
> + if (!IS_ERR(hpriv->phys[port]))
> + return 0;
> +
> + rc = PTR_ERR(hpriv->phys[port]);
> + switch (rc) {
> + case -ENOSYS:
> + /* No PHY support. Check if PHY is required. */
> + if (of_find_property(node, "phys", NULL)) {
> + dev_err(dev,
> + "couldn't get PHY in node %s: ENOSYS\n",
> + node->name);
> + break;
> + }
> + case -ENODEV:
> + /* continue normally */
> + hpriv->phys[port] = NULL;
> + rc = 0;
> + break;
> +
> + default:
> + dev_err(dev,
> + "couldn't get PHY in node %s: %d\n",
> + node->name, rc);
> +
> + break;
> + }
> +
> + return rc;
> +}
> +
> +static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> + struct device *dev, struct device_node *node)
> +{
> + struct regulator *target_pwr;
> + int rc = 0;
> +
> + target_pwr = devm_of_regulator_get_optional(dev, "target", node);
> +
> + if (!IS_ERR(target_pwr))
> + hpriv->target_pwrs[port] = target_pwr;
> + else
> + rc = PTR_ERR(target_pwr);
> +
> + return rc;
> +}
> +
> /**
> * ahci_platform_get_resources - Get platform resources
> * @pdev: platform device to get resources for
> @@ -240,7 +343,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
> struct ahci_host_priv *hpriv;
> struct clk *clk;
> struct device_node *child;
> - int i, enabled_ports = 0, rc = -ENOMEM;
> + int i, sz, enabled_ports = 0, rc = -ENOMEM;
> u32 mask_port_map = 0;
>
> if (!devres_open_group(dev, NULL, GFP_KERNEL))
> @@ -261,14 +364,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
> goto err_out;
> }
>
> - hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
> - if (IS_ERR(hpriv->target_pwr)) {
> - rc = PTR_ERR(hpriv->target_pwr);
> - if (rc == -EPROBE_DEFER)
> - goto err_out;
> - hpriv->target_pwr = NULL;
> - }
> -
> for (i = 0; i < AHCI_MAX_CLKS; i++) {
> /*
> * For now we must use clk_get(dev, NULL) for the first clock,
> @@ -292,15 +387,24 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>
> hpriv->nports = of_get_child_count(dev->of_node);
>
> - if (hpriv->nports) {
> - hpriv->phys = devm_kzalloc(dev,
> - hpriv->nports * sizeof(*hpriv->phys),
> - GFP_KERNEL);
> - if (!hpriv->phys) {
> - rc = -ENOMEM;
> - goto err_out;
> - }
> + /* If no sub-node was found, keep this for device tree compatibility */
> + if (!hpriv->nports)
> + hpriv->nports = 1;
So now nports is always >= 1.
> +
> + sz = hpriv->nports * sizeof(*hpriv->phys);
> + hpriv->phys = devm_kzalloc(dev, sz, GFP_KERNEL);
> + if (!hpriv->phys) {
> + rc = -ENOMEM;
> + goto err_out;
> + }
> + sz = hpriv->nports * sizeof(*hpriv->target_pwrs);
> + hpriv->target_pwrs = devm_kzalloc(dev, sz, GFP_KERNEL);
> + if (!hpriv->target_pwrs) {
> + rc = -ENOMEM;
> + goto err_out;
> + }
>
> + if (hpriv->nports) {
And this is always true,
> for_each_child_of_node(dev->of_node, child) {
> u32 port;
>
> @@ -319,14 +423,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>
> mask_port_map |= BIT(port);
>
> - hpriv->phys[port] = devm_of_phy_get(dev, child, NULL);
> - if (IS_ERR(hpriv->phys[port])) {
> - rc = PTR_ERR(hpriv->phys[port]);
> - dev_err(dev,
> - "couldn't get PHY in node %s: %d\n",
> - child->name, rc);
> + rc = ahci_platform_get_regulator(hpriv, port,
> + dev, child);
> + if (rc == -EPROBE_DEFER)
> + goto err_out;
> +
> + rc = ahci_platform_get_phy(hpriv, port, dev, child);
> + if (rc)
> goto err_out;
> - }
>
> enabled_ports++;
> }
> @@ -343,38 +447,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
> * If no sub-node was found, keep this for device tree
> * compatibility
> */
And thus the code below (which is in the else) never gets executed, and you've
broken the case where there are no subnodes.
I think it is best to fix this by keeping nports == 0 when there are no subnodes
(because we really do not know nports then, so advertising 1 is sorta wrong anyways),
and use something like this to calculate the array sizes:
sz = (nports ? nports : 1) * sizeof(*hpriv->phys);
...
sz = (nports ? nports : 1) * sizeof(*hpriv->target_pwrs);
> - struct phy *phy = devm_phy_get(dev, "sata-phy");
> - if (!IS_ERR(phy)) {
> - hpriv->phys = devm_kzalloc(dev, sizeof(*hpriv->phys),
> - GFP_KERNEL);
> - if (!hpriv->phys) {
> - rc = -ENOMEM;
> - goto err_out;
> - }
> -
> - hpriv->phys[0] = phy;
> - hpriv->nports = 1;
> - } else {
> - rc = PTR_ERR(phy);
> - switch (rc) {
> - case -ENOSYS:
> - /* No PHY support. Check if PHY is required. */
> - if (of_find_property(dev->of_node, "phys", NULL)) {
> - dev_err(dev, "couldn't get sata-phy: ENOSYS\n");
> - goto err_out;
> - }
> - case -ENODEV:
> - /* continue normally */
> - hpriv->phys = NULL;
> - break;
> -
> - default:
> - goto err_out;
> + rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
> + if (rc)
> + goto err_out;
>
> - }
> - }
> + rc = ahci_platform_get_regulator(hpriv, 0, dev, dev->of_node);
> + if (rc == -EPROBE_DEFER)
> + goto err_out;
> }
> -
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
> hpriv->got_runtime_pm = true;
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index 642d6ae4030c..f65b33809170 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -24,6 +24,8 @@ struct platform_device;
>
> int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
> +int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
> +void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv);
> int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
> void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
> struct ahci_host_priv *ahci_platform_get_resources(
>
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists