netdev - Re: [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config

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: <cao2zykgxyxee2f3rsrtod22qiyovyuywnck4xlheajrzfke3f@k6nfpf5bz46y>
Date: Sun, 5 May 2024 00:28:46 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com, 
	alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com, 
	chenhuacai@...nel.org, linux@...linux.org.uk, guyinggang@...ngson.cn, 
	netdev@...r.kernel.org, chris.chenfeiyang@...il.com, siyanteng01@...il.com
Subject: Re: [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add
 loongson_dwmac_config_legacy

On Thu, Apr 25, 2024 at 09:10:36PM +0800, Yanteng Si wrote:
> Move res._irq to loongson_dwmac_config_legacy().
> No function changes.

Since the code affected by this patch has just been touched by the
previous patch
[PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support
just merge this patch content into there. But add a note to the
commit message of that patch like this:

"While at it move the IRQ initialization procedure into a dedicated
method. It will be useful in one of the subsequent commit adding the
Loongson GNET device support."

> 
> Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
> Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
> Signed-off-by: Yanteng Si <siyanteng@...ngson.cn>
> ---
>  .../ethernet/stmicro/stmmac/dwmac-loongson.c  | 56 +++++++++++--------
>  1 file changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index 1022bceaa680..df5899bec91a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -68,6 +68,38 @@ static struct stmmac_pci_info loongson_gmac_pci_info = {
>  	.setup = loongson_gmac_data,
>  };
>  
> +static int loongson_dwmac_config_legacy(struct pci_dev *pdev,
> +					struct plat_stmmacenet_data *plat,
> +					struct stmmac_resources *res,
> +					struct device_node *np)
> +{
> +	if (np) {
> +		res->irq = of_irq_get_byname(np, "macirq");
> +		if (res->irq < 0) {
> +			dev_err(&pdev->dev, "IRQ macirq not found\n");
> +			return -ENODEV;
> +		}
> +
> +		res->wol_irq = of_irq_get_byname(np, "eth_wake_irq");
> +		if (res->wol_irq < 0) {
> +			dev_info(&pdev->dev,
> +				 "IRQ eth_wake_irq not found, using macirq\n");
> +			res->wol_irq = res->irq;
> +		}
> +
> +		res->lpi_irq = of_irq_get_byname(np, "eth_lpi");
> +		if (res->lpi_irq < 0) {
> +			dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
> +			return -ENODEV;
> +		}
> +	} else {
> +		res->irq = pdev->irq;

> +		res->wol_irq = res->irq;

Once again - drop this, unless you can justify it's required.

> +	}
> +
> +	return 0;
> +}
> +
>  static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct plat_stmmacenet_data *plat;
> @@ -136,28 +168,6 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>  			goto err_disable_device;
>  		}
>  		plat->phy_interface = phy_mode;
> -
> -		res.irq = of_irq_get_byname(np, "macirq");
> -		if (res.irq < 0) {
> -			dev_err(&pdev->dev, "IRQ macirq not found\n");
> -			ret = -ENODEV;
> -			goto err_disable_msi;
> -		}
> -
> -		res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
> -		if (res.wol_irq < 0) {
> -			dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
> -			res.wol_irq = res.irq;
> -		}
> -
> -		res.lpi_irq = of_irq_get_byname(np, "eth_lpi");
> -		if (res.lpi_irq < 0) {
> -			dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
> -			ret = -ENODEV;
> -			goto err_disable_msi;
> -		}
> -	} else {
> -		res.irq = pdev->irq;
>  	}
>  
>  	pci_enable_msi(pdev);
> @@ -167,6 +177,8 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>  	plat->tx_queues_to_use = 1;
>  	plat->rx_queues_to_use = 1;
>  

> +	ret = loongson_dwmac_config_legacy(pdev, plat, &res, np);
> +

By not checking the return value the patch turns to in fact containing
the functional change, which contradicts to what you say in the commit
log. Besides it's just wrong in this case. So please add the return
value check.

-Serge(y)

>  	ret = stmmac_dvr_probe(&pdev->dev, plat, &res);
>  	if (ret)
>  		goto err_disable_msi;
> -- 
> 2.31.4
> 
>