linux-kernel - Re: [PATCH v3 2/3] PCI: Visconti: Add Toshiba Visconti PCIe host controller driver

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:   Fri, 28 May 2021 09:19:05 +0900
From:   Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>
To:     Krzysztof Wilczyński <kw@...ux.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh+dt@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        linux-pci@...r.kernel.org, punit1.agrawal@...hiba.co.jp,
        yuji2.ishikawa@...hiba.co.jp, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] PCI: Visconti: Add Toshiba Visconti PCIe host
 controller driver

Hi, 

Thanks for your review.

On Mon, May 24, 2021 at 01:10:51PM +0200, Krzysztof Wilczyński wrote:
> Hi Nobuhiro,
> 
> Thank you for working on this!
> 
> [...]
> > +static int visconti_get_resources(struct platform_device *pdev,
> > +				  struct visconti_pcie *pcie)
> > +{
> [...]
> > +	pcie->refclk = devm_clk_get(dev, "pcie_refclk");
> > +	if (IS_ERR(pcie->refclk)) {
> > +		dev_err(dev, "Failed to get refclk clock: %ld\n",
> > +			PTR_ERR(pcie->refclk));
> > +		return PTR_ERR(pcie->refclk);
> > +	}
> > +
> > +	pcie->sysclk = devm_clk_get(dev, "sysclk");
> > +	if (IS_ERR(pcie->sysclk)) {
> > +		dev_err(dev, "Failed to get sysclk clock: %ld\n",
> > +			PTR_ERR(pcie->sysclk));
> > +		return PTR_ERR(pcie->sysclk);
> > +	}
> > +
> > +	pcie->auxclk = devm_clk_get(dev, "auxclk");
> > +	if (IS_ERR(pcie->auxclk)) {
> > +		dev_err(dev, "Failed to get auxclk clock: %ld\n",
> > +			PTR_ERR(pcie->auxclk));
> > +		return PTR_ERR(pcie->auxclk);
> > +	}
> 
> Do you think you could use the dev_err_probe() to handle the
> devm_clk_get() failed?  Where applicable this is becoming a common
> patter drivers apply, for example:
> 
>   pcie->refclk = devm_clk_get(dev, "pcie_refclk");
>   if (IS_ERR(pcie->refclk))
>   	return dev_err_probe(dev, PTR_ERR(pcie->refclk),
> 			     "failed to get refclk clock\n");
> 
> [...]

Thanks for your suggestion. I will fix using dev_err_probe().

> > +	pci->link_gen = of_pci_get_max_link_speed(pdev->dev.of_node);
> > +	if (pci->link_gen < 0 || pci->link_gen > 3) {
> > +		pci->link_gen = 3;
> > +		dev_dbg(dev, "Applied default link speed\n");
> > +	}
> > +
> > +	dev_dbg(dev, "Link speed Gen %d", pci->link_gen);
> 
> Question about the above debug messages.
> 
> Given that both are at the same level and the link speed will be printed
> regardless of whether it was set to a default value or not, does it make
> sense to still print the message about applying the default link speed?
> Unless this is something that will be indeed useful during debugging and
> troubleshooting (and in which case just ignore this question).

I guess so, the message about the default value is not important.
I will remove this, thank you.

Best regards,
  Nobuhiro