linux-kernel - Re: [PATCH] PCI: kirin: Fix an error path in kirin_pcie

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: <gpmjckwgbbccsnjcexg7wtvw2oshj6wuwqomlbbo2ebok5oe3g@ky6mfxdutnt4>
Date:   Tue, 31 Oct 2023 02:32:56 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     Xiaowei Song <songxiaowei@...ilicon.com>,
        Binghui Wang <wangbinghui@...ilicon.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Rob Herring <robh@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Jingoo Han <jingoohan1@...il.com>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        linux-pci@...r.kernel.org
Subject: Re: [PATCH] PCI: kirin: Fix an error path in kirin_pcie_probe()

On Mon, Oct 30, 2023 at 09:21:16AM +0100, Christophe JAILLET wrote:
> If an error occurs after a successful kirin_pcie_power_on(),
> kirin_pcie_power_off() should be called, as already done in the remove
> function.
> 
> Fixes: fc5165db245a ("PCI: kirin: Add HiSilicon Kirin SoC PCIe controller driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
> Not sure of the Fixes tag.
> ---
>  drivers/pci/controller/dwc/pcie-kirin.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index 2ee146767971..0b93de9d2d06 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -813,7 +813,15 @@ static int kirin_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	return dw_pcie_host_init(&pci->pp);
> +	ret = dw_pcie_host_init(&pci->pp);
> +	if (ret)
> +		goto err_power_off;
> +
> +	return 0;
> +
> +err_power_off:
> +	kirin_pcie_power_off(kirin_pcie);
> +	return ret;

>From the current driver implementation point of view this looks
correct. So
Reviewed-by: Serge Semin <fancer.lancer@...il.com>

But the design of the power on/off procedures seems very unfortunate:
1. Calling antagonist from the respective protagonist is a bad
solution for maintainability, because shall you need to add something
to the protagonist you'll need to somehow take into account that it is
reverted in the antagonist only if it was executed, which in its
turn will get to be impossible if there are several conditional
steps need to be implemented.
2. There is a logical split up between the hi3660 and other
controllers. Wherein the hi3660-specific code is implemented as a set
of various coherent functions, meanwhile the code for the other
controllers is placed directly to the kirin_pcie_power_on() and
kirin_pcie_power_off() functions. It looks clumsy and hard-readable.
3. kirin_pcie->gpio_id_dwc_perst is requested and switched to output,
but is never freed and got back to input or level zero.

-Serge(y)

>  }
>  
>  static struct platform_driver kirin_pcie_driver = {
> -- 
> 2.34.1
>