netdev - Re: [PATCH v3 3/6] net: stmmac: stmmac_platform: fix parsing of DT binding

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: <62474778-78da-d34e-bc36-022b2206a8db@st.com>
Date:   Thu, 8 Dec 2016 10:02:45 +0100
From:   Alexandre Torgue <alexandre.torgue@...com>
To:     Niklas Cassel <niklas.cassel@...s.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>
CC:     Niklas Cassel <niklass@...s.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/6] net: stmmac: stmmac_platform: fix parsing of DT
 binding

Hi Niklas

On 12/07/2016 03:20 PM, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@...s.com>
>
> commit 64c3b252e9fc ("net: stmmac: fixed the pbl setting with DT")
> changed the parsing of the DT binding.
>
> Before 64c3b252e9fc, snps,fixed-burst and snps,mixed-burst were parsed
> regardless if the property snps,pbl existed or not.
> After the commit, fixed burst and mixed burst are only parsed if
> snps,pbl exists. Now when snps,aal has been added, it too is only
> parsed if snps,pbl exists.
>
> Since the DT binding does not specify that fixed burst, mixed burst
> or aal depend on snps,pbl being specified, undo changes introduced
> by 64c3b252e9fc.
>
> The issue commit 64c3b252e9fc ("net: stmmac: fixed the pbl setting with
> DT") tries to address is solved in another way:
> The databook specifies that all values other than
> 1, 2, 4, 8, 16, or 32 results in undefined behavior,
> so snps,pbl = <0> is invalid.
>
> If pbl is 0 after parsing, set pbl to DEFAULT_DMA_PBL.
> This handles the case where the property is omitted, and also handles
> the case where the property is specified without any data.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@...s.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  4 +--
>  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  | 29 +++++++++++-----------
>  2 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b1e42ddf0370..b5188122bc15 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1581,8 +1581,8 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
>  	int atds = 0;
>  	int ret = 0;
>
> -	if (!priv->plat->dma_cfg) {
> -		dev_err(priv->device, "DMA configuration not found\n");
> +	if (!priv->plat->dma_cfg || !priv->plat->dma_cfg->pbl) {

How "priv->plat->dma_cfg->pbl" could be equal to 0 if you force it to 
DEFAULT_DMA_PBL in "stmmac_probe_config_dt" in case of DT doesn't set 
pbl value?


> +		dev_err(priv->device, "Invalid DMA configuration\n");
>  		return -EINVAL;
>  	}
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index d3b6f92f350a..81800f23a9c4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -304,21 +304,22 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac)
>  		plat->force_sf_dma_mode = 1;
>  	}
>
> -	if (of_find_property(np, "snps,pbl", NULL)) {
> -		dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*dma_cfg),
> -				       GFP_KERNEL);
> -		if (!dma_cfg) {
> -			stmmac_remove_config_dt(pdev, plat);
> -			return ERR_PTR(-ENOMEM);
> -		}
> -		plat->dma_cfg = dma_cfg;
> -		of_property_read_u32(np, "snps,pbl", &dma_cfg->pbl);
> -		dma_cfg->aal = of_property_read_bool(np, "snps,aal");
> -		dma_cfg->fixed_burst =
> -			of_property_read_bool(np, "snps,fixed-burst");
> -		dma_cfg->mixed_burst =
> -			of_property_read_bool(np, "snps,mixed-burst");
> +	dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*dma_cfg),
> +			       GFP_KERNEL);
> +	if (!dma_cfg) {
> +		stmmac_remove_config_dt(pdev, plat);
> +		return ERR_PTR(-ENOMEM);
>  	}
> +	plat->dma_cfg = dma_cfg;
> +
> +	of_property_read_u32(np, "snps,pbl", &dma_cfg->pbl);
> +	if (!dma_cfg->pbl)
> +		dma_cfg->pbl = DEFAULT_DMA_PBL;
> +
> +	dma_cfg->aal = of_property_read_bool(np, "snps,aal");
> +	dma_cfg->fixed_burst = of_property_read_bool(np, "snps,fixed-burst");
> +	dma_cfg->mixed_burst = of_property_read_bool(np, "snps,mixed-burst");
> +
>  	plat->force_thresh_dma_mode = of_property_read_bool(np, "snps,force_thresh_dma_mode");
>  	if (plat->force_thresh_dma_mode) {
>  		plat->force_sf_dma_mode = 0;
>