| 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: <20191219130126.GE1440537@ulmo>
Date: Thu, 19 Dec 2019 14:01:26 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Dmitry Osipenko <digetx@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Peter Chen <Peter.Chen@....com>,
Jonathan Hunter <jonathanh@...dia.com>,
Felipe Balbi <balbi@...nel.org>, devicetree@...r.kernel.org,
linux-usb@...r.kernel.org, linux-tegra@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/4] usb: phy: tegra: Hook up init/shutdown callbacks
On Wed, Dec 18, 2019 at 08:53:11PM +0300, Dmitry Osipenko wrote:
> Generic PHY provides init/shutdown callbacks which allow USB-host drivers
> to abstract PHY's hardware management in a common way. This change allows
> to remove Tegra-specific PHY handling from the ChipIdea driver.
>
> Note that ChipIdea's driver shall be changed at the same time because it
> turns PHY ON without the PHY's initialization and this doesn't work now,
> resulting in a NULL dereference of phy->freq because it's set during of
> the PHY's initialization.
>
> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> ---
> drivers/usb/chipidea/ci_hdrc_tegra.c | 9 --
> drivers/usb/phy/phy-tegra-usb.c | 165 +++++++++++++++++----------
> 2 files changed, 102 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> index 0c9911d44ee5..7455df0ede49 100644
> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> @@ -83,13 +83,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
> return err;
> }
>
> - /*
> - * Tegra's USB PHY driver doesn't implement optional phy_init()
> - * hook, so we have to power on UDC controller before ChipIdea
> - * driver initialization kicks in.
> - */
> - usb_phy_set_suspend(udc->phy, 0);
> -
> /* setup and register ChipIdea HDRC device */
> udc->data.name = "tegra-udc";
> udc->data.flags = soc->flags;
> @@ -109,7 +102,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
> return 0;
>
> fail_power_off:
> - usb_phy_set_suspend(udc->phy, 1);
> clk_disable_unprepare(udc->clk);
> return err;
> }
> @@ -119,7 +111,6 @@ static int tegra_udc_remove(struct platform_device *pdev)
> struct tegra_udc *udc = platform_get_drvdata(pdev);
>
> ci_hdrc_remove_device(udc->dev);
> - usb_phy_set_suspend(udc->phy, 1);
> clk_disable_unprepare(udc->clk);
>
> return 0;
> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> index ea7ef1dc0b42..15bd253d53c9 100644
> --- a/drivers/usb/phy/phy-tegra-usb.c
> +++ b/drivers/usb/phy/phy-tegra-usb.c
> @@ -238,23 +238,6 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
> {
> int ret;
>
> - phy->pad_clk = devm_clk_get(phy->u_phy.dev, "utmi-pads");
> - if (IS_ERR(phy->pad_clk)) {
> - ret = PTR_ERR(phy->pad_clk);
> - dev_err(phy->u_phy.dev,
> - "Failed to get UTMIP pad clock: %d\n", ret);
> - return ret;
> - }
> -
> - phy->pad_rst = devm_reset_control_get_optional_shared(
> - phy->u_phy.dev, "utmi-pads");
> - if (IS_ERR(phy->pad_rst)) {
> - ret = PTR_ERR(phy->pad_rst);
> - dev_err(phy->u_phy.dev,
> - "Failed to get UTMI-pads reset: %d\n", ret);
> - return ret;
> - }
> -
> ret = clk_prepare_enable(phy->pad_clk);
> if (ret) {
> dev_err(phy->u_phy.dev,
> @@ -315,6 +298,18 @@ static int utmip_pad_close(struct tegra_usb_phy *phy)
> return ret;
> }
>
> +static void ulpi_close(struct tegra_usb_phy *phy)
> +{
> + int err;
> +
> + err = gpio_direction_output(phy->reset_gpio, 1);
> + if (err < 0) {
> + dev_err(phy->u_phy.dev,
> + "ULPI reset GPIO %d direction not asserted: %d\n",
> + phy->reset_gpio, err);
> + }
> +}
> +
> static void utmip_pad_power_on(struct tegra_usb_phy *phy)
> {
> unsigned long val, flags;
> @@ -761,14 +756,19 @@ static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
> return gpio_direction_output(phy->reset_gpio, 0);
> }
>
> -static void tegra_usb_phy_close(struct tegra_usb_phy *phy)
> +static void tegra_usb_phy_close(struct usb_phy *u_phy)
> {
> - if (!IS_ERR(phy->vbus))
> - regulator_disable(phy->vbus);
> + struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
> + u_phy);
>
> - if (!phy->is_ulpi_phy)
> + if (phy->is_ulpi_phy)
> + ulpi_close(phy);
> + else
> utmip_pad_close(phy);
>
> + if (!IS_ERR(phy->vbus))
> + regulator_disable(phy->vbus);
> +
> clk_disable_unprepare(phy->pll_u);
> }
>
> @@ -788,7 +788,7 @@ static int tegra_usb_phy_power_off(struct tegra_usb_phy *phy)
> return utmi_phy_power_off(phy);
> }
>
> -static int tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
> +static int tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
> {
> struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
> if (suspend)
> @@ -801,54 +801,25 @@ static int ulpi_open(struct tegra_usb_phy *phy)
> {
> int err;
>
> - phy->clk = devm_clk_get(phy->u_phy.dev, "ulpi-link");
> - if (IS_ERR(phy->clk)) {
> - err = PTR_ERR(phy->clk);
> - dev_err(phy->u_phy.dev, "Failed to get ULPI clock: %d\n", err);
> - return err;
> - }
> -
> - err = devm_gpio_request(phy->u_phy.dev, phy->reset_gpio,
> - "ulpi_phy_reset_b");
> - if (err < 0) {
> - dev_err(phy->u_phy.dev, "Request failed for GPIO %d: %d\n",
> - phy->reset_gpio, err);
> - return err;
> - }
> -
> err = gpio_direction_output(phy->reset_gpio, 0);
> if (err < 0) {
> dev_err(phy->u_phy.dev,
> - "GPIO %d direction not set to output: %d\n",
> + "ULPI reset GPIO %d direction not deasserted: %d\n",
> phy->reset_gpio, err);
> return err;
> }
>
> - phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
> - if (!phy->ulpi) {
> - dev_err(phy->u_phy.dev, "Failed to create ULPI OTG\n");
> - err = -ENOMEM;
> - return err;
> - }
> -
> - phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
> return 0;
> }
>
> -static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
> +static int tegra_usb_phy_init(struct usb_phy *u_phy)
> {
> + struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
> + u_phy);
> unsigned long parent_rate;
> int i;
> int err;
>
> - phy->pll_u = devm_clk_get(phy->u_phy.dev, "pll_u");
> - if (IS_ERR(phy->pll_u)) {
> - err = PTR_ERR(phy->pll_u);
> - dev_err(phy->u_phy.dev,
> - "Failed to get pll_u clock: %d\n", err);
> - return err;
> - }
> -
> err = clk_prepare_enable(phy->pll_u);
> if (err)
> return err;
> @@ -884,8 +855,17 @@ static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
> if (err < 0)
> goto fail;
>
> + err = tegra_usb_phy_power_on(phy);
> + if (err)
> + goto close_phy;
> +
> return 0;
>
> +close_phy:
> + if (phy->is_ulpi_phy)
> + ulpi_close(phy);
> + else
> + utmip_pad_close(phy);
> fail:
> clk_disable_unprepare(phy->pll_u);
> return err;
> @@ -1134,22 +1114,77 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
> tegra_phy->vbus = ERR_PTR(-ENODEV);
> }
>
> - tegra_phy->u_phy.dev = &pdev->dev;
> - err = tegra_usb_phy_init(tegra_phy);
> - if (err < 0)
> + tegra_phy->pll_u = devm_clk_get(&pdev->dev, "pll_u");
> + err = PTR_ERR_OR_ZERO(tegra_phy);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to get pll_u clock: %d\n", err);
> return err;
> + }
>
> + if (tegra_phy->is_ulpi_phy) {
> + tegra_phy->clk = devm_clk_get(&pdev->dev, "ulpi-link");
> + err = PTR_ERR_OR_ZERO(tegra_phy->clk);
> + if (err) {
> + dev_err(&pdev->dev,
> + "Failed to get ULPI clock: %d\n", err);
> + return err;
> + }
> +
> + err = devm_gpio_request(&pdev->dev, tegra_phy->reset_gpio,
> + "ulpi_phy_reset_b");
> + if (err < 0) {
> + dev_err(&pdev->dev, "Request failed for GPIO %d: %d\n",
> + tegra_phy->reset_gpio, err);
> + return err;
> + }
> +
> + tegra_phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
> + if (!tegra_phy->ulpi) {
> + dev_err(&pdev->dev, "Failed to create ULPI OTG\n");
> + err = -ENOMEM;
> + return err;
> + }
> +
> + tegra_phy->ulpi->io_priv = tegra_phy->regs + ULPI_VIEWPORT;
> + } else {
> + tegra_phy->pad_clk = devm_clk_get(&pdev->dev, "utmi-pads");
> + err = PTR_ERR_OR_ZERO(tegra_phy->pad_clk);
> + if (err) {
> + dev_err(&pdev->dev,
> + "Failed to get UTMIP pad clock: %d\n", err);
> + return err;
> + }
> +
> + tegra_phy->pad_rst = devm_reset_control_get_optional_shared(
> + &pdev->dev, "utmi-pads");
> + err = PTR_ERR_OR_ZERO(tegra_phy->pad_rst);
> + if (err) {
> + dev_err(&pdev->dev,
> + "Failed to get UTMI-pads reset: %d\n", err);
> + return err;
> + }
> + }
> +
> + tegra_phy->u_phy.dev = &pdev->dev;
> + tegra_phy->u_phy.init = tegra_usb_phy_init;
> + tegra_phy->u_phy.shutdown = tegra_usb_phy_close;
> tegra_phy->u_phy.set_suspend = tegra_usb_phy_suspend;
>
> platform_set_drvdata(pdev, tegra_phy);
>
> err = usb_add_phy_dev(&tegra_phy->u_phy);
> - if (err < 0) {
> - tegra_usb_phy_close(tegra_phy);
> - return err;
> - }
> + if (err < 0)
> + goto free_ulpi;
>
> return 0;
> +
> +free_ulpi:
> + if (tegra_phy->ulpi) {
> + kfree(tegra_phy->ulpi->otg);
> + kfree(tegra_phy->ulpi);
> + }
> +
> + return err;
> }
>
> static int tegra_usb_phy_remove(struct platform_device *pdev)
> @@ -1157,7 +1192,11 @@ static int tegra_usb_phy_remove(struct platform_device *pdev)
> struct tegra_usb_phy *tegra_phy = platform_get_drvdata(pdev);
>
> usb_remove_phy(&tegra_phy->u_phy);
> - tegra_usb_phy_close(tegra_phy);
> +
> + if (tegra_phy->ulpi) {
> + kfree(tegra_phy->ulpi->otg);
> + kfree(tegra_phy->ulpi);
> + }
It might be nicer to add a new otg_ulpi_free() (or whatever) function to
do this. Manually cleaning up the resources allocated by a public API is
a bit asymmetric and easy to get wrong.
But it's correct in this case and the new function can be added in a
separate patch, so:
Acked-by: Thierry Reding <treding@...dia.com>
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists