linux-kernel - Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs

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]

Date:   Fri, 7 Oct 2016 09:11:33 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Markus Mayer <mmayer@...adcom.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Broadcom Kernel List <bcm-kernel-feedback-list@...adcom.com>,
        Device Tree List <devicetree@...r.kernel.org>,
        Power Management List <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq
 driver for Broadcom STB SoCs

On 06-10-16, 14:04, Markus Mayer wrote:
> Unfortunately, I'll have to take back one agreed-upon change.
> 
> In this piece of code, brcm_avs_is_firmware_loaded() has to come after
> requesting the IRQ.
> 
> 
> 	priv->base = __map_region(BRCM_AVS_CPU_DATA);
> 	if (!priv->base) {
> 		dev_err(dev, "Couldn't find property %s in device tree.\n",
> 			BRCM_AVS_CPU_DATA);
> 		return -ENOENT;
> 	}
> 
> 	priv->avs_intr_base = __map_region(BRCM_AVS_CPU_INTR);
> 	if (!priv->avs_intr_base) {
> 		dev_err(dev, "Couldn't find property %s in device tree.\n",
> 			BRCM_AVS_CPU_INTR);
> 		ret = -ENOENT;
> 		goto unmap_base;
> 	}
> 
> 	host_irq = platform_get_irq_byname(pdev, BRCM_AVS_HOST_INTR);
> 	if (host_irq < 0) {
> 		dev_err(dev, "Couldn't find interrupt %s -- %d\n",
> 			BRCM_AVS_HOST_INTR, host_irq);
> 		ret = host_irq;
> 		goto unmap_intr_base;
> 	}
> 
> 	ret = devm_request_irq(dev, host_irq, irq_handler, IRQF_TRIGGER_RISING,
> 			       BRCM_AVS_HOST_INTR, priv);
> 	if (ret) {
> 		dev_err(dev, "IRQ request failed: %s (%d) -- %d\n",
> 			BRCM_AVS_HOST_INTR, host_irq, ret);
> 		goto unmap_intr_base;
> 	}
> 
> 	if (brcm_avs_is_firmware_loaded(priv))
> 		return 0;
> 
> The reason being that we need to be ready to receive interrupts from the
> co-processor to tell us the GET_PMAP command completed.
> brcm_avs_is_firmware_loaded() issues the GET_PMAP command to ensure the
> firmware supports it. If I try to run brcm_avs_is_firmware_loaded() before
> setting up interrupts, I get a timeout in the driver -- because it never
> receives the interrupt saying the command is done.
> 
> And I have to check the firmware supports the GET_PMAP command, because it
> is possible for somebody to choose to run a firmware that does not support
> DVFS, even though the hardware would support it.

Fair enough.

> Okay, I looked some more and compared it to what cpufreq-dt is doing to
> initialize. That lead me onto the right track. They simply do things they
> only want done once via the driver's probe function rather than CPUfreq's
> init function. So, I broke my initialization code up into two parts. 

I wanted to suggest that earlier, but then didn't do it :)

> Everything up to checking if the firmware is loaded is now being called from
> brcm_avs_cpufreq_probe(). The frequency table code is still being called
> from brcm_avs_cpufreq_init().

Looks fine.

> That means that if the initial hardware checks fail, it won't try again.

Yeah, but if init fails for some reason, then it will get called again for other
CPUs. Which is of course the right thing IMO.

-- 
viresh