linux-kernel - Re: [PATCH] PCI: xilinx-nwl: Enable the clock through CCF

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] [day] [month] [year] [list]

Message-ID: <d7dd7dfe-2c48-c155-793d-1431256a070e@xilinx.com>
Date:   Wed, 23 Jun 2021 15:08:05 +0200
From:   Michal Simek <michal.simek@...inx.com>
To:     Krzysztof Wilczyński <kw@...ux.com>,
        Michal Simek <michal.simek@...inx.com>
CC:     <linux-kernel@...r.kernel.org>, <monstr@...str.eu>,
        <git@...inx.com>, "Hyun Kwon" <hyun.kwon@...inx.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "Lorenzo Pieralisi" <lorenzo.pieralisi@....com>,
        Rob Herring <robh@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-pci@...r.kernel.org>
Subject: Re: [PATCH] PCI: xilinx-nwl: Enable the clock through CCF

Hi Krzysztof,

On 6/23/21 2:20 PM, Krzysztof Wilczyński wrote:
> Hi Michal,
> 
> Thank you for sending the patch over!

Thanks for review.

> 
>> Simply enable clocks. There is no remove function that's why
>> this should be enough for simple operation.
> 
> What clock is this?  Would it be worth mentioning what it is for
> a reference (and for posterity) the commit message?

It is reference clock coming to the IP. I will update commit message.


> 
> Also why it would need to be enabled and wasn't before?  Would this be
> a fix for some problem?  Would this warrant a "Fixes:" tag?  And would
> it need to be back-ported to stable kernels?

I will update commit message. Normally reference clock is enabled by
firmware but on some configurations this doesn't need to be truth that's
why it is necessary to enable it. It also records refcount for this
reference clock is good.

I will add Fixes tag to v2.

> 
> [...]
>> @@ -823,6 +825,11 @@ static int nwl_pcie_probe(struct platform_device *pdev)
>>  		return err;
>>  	}
>>  
>> +	pcie->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(pcie->clk))
>> +		return PTR_ERR(pcie->clk);
>> +	clk_prepare_enable(pcie->clk);
>> +
> [...]
> 
> Almost every other user of clk_prepare_enable() would check for
> potential failure, print an appropriate message, and then do the
> necessary clean-up before bailing out and returning an error.
> 
> Would adding an error check for clk_prepare_enable() and printing an
> error message using dev_err() be too much in this case?  If not, then
> I would rather follow the pattern that other users established and
> handle errors as needed.  What do you think?

Agree. I have added it. It is called very early and devm_ functions are
used that's why cleanup shouldn't be necessary.

I have also found that clock wasn't documented in dt binding for this IP
but we are setting it up for quite a long time.
9c8a47b484ed ("arm64: dts: xilinx: Add the clock nodes for zynqmp")

Thanks,
Michal