netdev - Re: [PATCH] net: stmmac: fix stmmac_pci_probe failed when CONFIG_HAVE

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]

Date:	Thu, 18 Sep 2014 16:49:58 +0200
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	Kweh Hock Leong <hock.leong.kweh@...el.com>,
	"David S. Miller" <davem@...emloft.net>, <rayagond@...avyalabs.com>
Cc:	Vince Bridgers <vbridgers2013@...il.com>,
	Chen-Yu Tsai <wens@...e.org>, <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Ong Boon Leong <boon.leong.ong@...el.com>,
	Tobias Klausmann <tobias.johannes.klausmann@....thm.de>
Subject: Re: [PATCH] net: stmmac: fix stmmac_pci_probe failed when CONFIG_HAVE_CLK
 is selected

On 9/18/2014 2:34 PM, Kweh Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@...el.com>
>
> When the CONFIG_HAVE_CLK is selected for the system, the stmmac_pci_probe
> will fail with dmesg:
> [    2.167225] stmmaceth 0000:00:14.6: enabling device (0000 -> 0002)
> [    2.178267] stmmaceth 0000:00:14.6: enabling bus mastering
> [    2.178436] stmmaceth 0000:00:14.6: irq 24 for MSI/MSI-X
> [    2.178703] stmmaceth 0000:00:14.6: stmmac_dvr_probe: warning: cannot
> get CSR clock
> [    2.186503] stmmac_pci_probe: main driver probe failed
> [    2.194003] stmmaceth 0000:00:14.6: disabling bus mastering
> [    2.196473] stmmaceth: probe of 0000:00:14.6 failed with error -2
>
> This patch fix the issue by breaking the dependency to devm_clk_get()
> as the CSR clock can be obtained at priv->plat->clk_csr from pci driver.
>
> Reported-by: Tobias Klausmann <tobias.johannes.klausmann@....thm.de>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@...el.com>
> ---
>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |   13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 08addd6..ea3859a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2714,10 +2714,15 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>
>   	priv->stmmac_clk = devm_clk_get(priv->device, STMMAC_RESOURCE_NAME);
>   	if (IS_ERR(priv->stmmac_clk)) {
> -		dev_warn(priv->device, "%s: warning: cannot get CSR clock\n",


Hmm I am not sure this is the right fix. The driver has to fail if the
main clock is not found. Indeed dev_warn has to be changed in dev_err.

Take a look at Documentation/networking/stmmac.txt but I will post some
patch to improve the documentation adding further detail for clocks too.

The the logic behind the code is that the CSR clock will be set at
runtime if in case of priv->plat->clk_csr ==0 or it will be forced to
a fixed value if passed from the platform instead of.
IIRC This was required on some platforms time ago.
For sure the driver is designed to fail in case of no main clock is
found.

Peppe


> -			 __func__);
> -		ret = PTR_ERR(priv->stmmac_clk);
> -		goto error_clk_get;
> +		if (!priv->plat->clk_csr) {
> +			dev_warn(priv->device,
> +				 "%s: warning: cannot get CSR clock\n",
> +				 __func__);
> +			ret = PTR_ERR(priv->stmmac_clk);
> +			goto error_clk_get;
> +		} else {
> +			priv->stmmac_clk = NULL;
> +		}
>   	}
>   	clk_prepare_enable(priv->stmmac_clk);
>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html