linux-kernel - Re: [PATCH] phy: cadence: Sierra: Add support for skipping configuration

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]

Message-ID: <20220127101938.GD1978@kadam>
Date:   Thu, 27 Jan 2022 13:19:38 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Aswath Govindraju <a-govindraju@...com>
Cc:     Kishon Vijay Abraham I <kishon@...com>,
        Vinod Koul <vkoul@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Swapnil Jakhade <sjakhade@...ence.com>,
        linux-phy@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] phy: cadence: Sierra: Add support for skipping
 configuration

On Thu, Jan 27, 2022 at 02:26:58PM +0530, Aswath Govindraju wrote:
> Skip the phy configuration if the required configurations were done in an
> earlier boot stage.
> 

Why are you doing this?  Could you please put in the commit message if
the user will see an improvement from this change.

> Signed-off-by: Aswath Govindraju <a-govindraju@...com>
> ---

[ snip ]

> @@ -1382,16 +1401,24 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = cdns_sierra_phy_get_resets(sp, dev);
> -	if (ret)
> -		goto unregister_clk;
> -
>  	ret = cdns_sierra_phy_enable_clocks(sp);
>  	if (ret)
>  		goto unregister_clk;
>  
> -	/* Enable APB */
> -	reset_control_deassert(sp->apb_rst);
> +	regmap_field_read(sp->pma_cmn_ready, &sp->already_configured);
> +
> +	if (!(sp->already_configured)) {

Delete extra parens.

> +		ret = cdns_sierra_phy_clk(sp);
> +		if (ret)
> +			goto unregister_clk;

The goto should release the most recent successful allocation which is
cdns_sierra_phy_enable_clocks().  So this should be goto clk_disable.
Except that will also call reset_control_assert() which is wrong...  The
rules are generally that error handling should be in the reverse order
from how we allocated it.  If allocation is optional the cleanup should
be optional.  The allocation and unwind code should mirror each other.

> +
> +		ret = cdns_sierra_phy_get_resets(sp, dev);
> +		if (ret)
> +			goto unregister_clk;

goto clk_disable;

> +
> +		/* Enable APB */
> +		reset_control_deassert(sp->apb_rst);

Since this is now optional it should be optional in the cleanup.

> +	}
>  

Since the order of allocations has changed, the other gotos need to be
updated to free the most recent allocation as well.  Then the error
handling looks like this:

	return 0;

put_control:
	while (--node >= 0)
		reset_control_put(sp->phys[node].lnk_rst);
ctrl_assert:
	if (!sp->already_configured)
		reset_control_assert(sp->apb_rst);
clk_disable:
	cdns_sierra_phy_disable_clocks(sp);
unregister_clk:
	cdns_sierra_clk_unregister(sp);
	return ret;


>  	/* Check that PHY is present */
>  	regmap_field_read(sp->macro_id_type, &id_value);
> @@ -1433,8 +1460,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>  
>  		sp->num_lanes += sp->phys[node].num_lanes;
>  
> -		gphy = devm_phy_create(dev, child, &ops);
> -
> +		if (!(sp->already_configured))

Delete parens.

> +			gphy = devm_phy_create(dev, child, &ops);
> +		else
> +			gphy = devm_phy_create(dev, child, &noop_ops);
>  		if (IS_ERR(gphy)) {
>  			ret = PTR_ERR(gphy);
>  			of_node_put(child);
> @@ -1455,7 +1484,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>  	}
>  
>  	/* If more than one subnode, configure the PHY as multilink */
> -	if (!sp->autoconf && sp->nsubnodes > 1) {
> +	if (!(sp->already_configured && sp->autoconf) && sp->nsubnodes > 1) {

It's normally easier to understand conditions when you push the ! as
far in as possible:

	if ((!sp->already_configured || !sp->autoconf) &&
	    sp->nsubnodes > 1) {

Is this condition right?  Shouldn't it be:

	if (!sp->already_configured && !sp->autoconf && sp->nsubnodes > 1) {

The ->already_configured is set/stored in firmware so I don't know when
that happens.  Please, add that information to the commit message when
you resend.

regards,
dan carpenter