linux-kernel - Re: [PATCH v2 13/15] phy: qcom-qmp-pcie: add support for pipediv2 clock

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:   Thu, 20 Oct 2022 12:49:20 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Johan Hovold <johan+linaro@...nel.org>,
        Vinod Koul <vkoul@...nel.org>, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 13/15] phy: qcom-qmp-pcie: add support for pipediv2
 clock

On Thu, Oct 20, 2022 at 12:28:14PM +0300, Dmitry Baryshkov wrote:
> On 20/10/2022 12:05, Johan Hovold wrote:

> > Here's your example diff inline:

> > @@ -2206,12 +2207,17 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np
> >   		}
> >   	}
> >   
> > -	qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL);
> > -	if (IS_ERR(qmp->pipe_clk)) {
> > -		return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
> > +	clk = devm_get_clk_from_child(dev, np, NULL);
> > +	if (IS_ERR(clk)) {
> > +		return dev_err_probe(dev, PTR_ERR(clk),
> >   				     "failed to get pipe clock\n");
> >   	}
> >   
> > +	qmp->num_pipe_clks = 1;
> > +	qmp->pipe_clks = devm_kcalloc(dev, qmp->num_pipe_clks,
> > +				      sizeof(*qmp->pipe_clks), GFP_KERNEL);
> > +	qmp->pipe_clks[0].clk = clk;
> > 
> > So here you're poking at bulk API internals and forgot to set the id
> > string, which the implementation uses.
> 
> I didn't forget, I just skipped setting it. Hmm. I thought that it is 
> used only for clk_bulk_get. But after checking, it seems it's also used 
> for error messages. Mea culpa.
> 
> But it's not that I was poking into the internals. These items are in 
> the public header.

My point is that you're not using the bulk API as it was intended (e.g.
with clk_bulk_get()) and you risk running into issues like the above.

And looking up the actual clock name for this is overkill, even in the
case were it is provided, so we'd need to set it unconditionally to
"pipe" (which is fine).

> > For reasons like this, and the fact that will likely never have a third
> > pipe clock, I'm reluctant to using the bulk API.
> 
> Let's resort to the maintainer opinion then.

I'll take another look at it too.

Johan