Re: [PATCH 0/5] PM: domains: Add helpers for multi PM domains to avoid open-coding
On 28-12-23, 12:41, Ulf Hansson wrote: > For OPP integration, as a follow up I am striving to make the > dev_pm_opp_attach_genpd() redundant. Instead I think we should move towards > using dev_pm_opp_set_config()->_opp_set_required_devs(), which would allow us > to > use the helpers that $subject series is adding. Big thanks for that :) -- viresh
Re: [PATCH v2 3/5] thermal/cpufreq: Remove arch_update_thermal_pressure()
On 21-12-23, 16:24, Vincent Guittot wrote: > arch_update_thermal_pressure() aims to update fast changing signal which > should be averaged using PELT filtering before being provided to the > scheduler which can't make smart use of fast changing signal. > cpufreq now provides the maximum freq_qos pressure on the capacity to the > scheduler, which includes cpufreq cooling device. Remove the call to > arch_update_thermal_pressure() in cpufreq cooling device as this is > handled by cpufreq_get_pressure(). > > Signed-off-by: Vincent Guittot > Reviewed-by: Lukasz Luba Acked-by: Viresh Kumar -- viresh
Re: [PATCH 1/4] cpufreq: Add a cpufreq pressure feedback for the scheduler
On 12-12-23, 15:27, Vincent Guittot wrote: > @@ -2618,6 +2663,9 @@ static int cpufreq_set_policy(struct cpufreq_policy > *policy, > policy->max = __resolve_freq(policy, policy->max, CPUFREQ_RELATION_H); > trace_cpu_frequency_limits(policy); > > + cpus = policy->related_cpus; > + cpufreq_update_pressure(cpus, policy->max); > + > policy->cached_target_freq = UINT_MAX; One more question, why are you doing this from cpufreq_set_policy ? If due to cpufreq cooling or from userspace, we end up limiting the maximum possible frequency, will this routine always get called ? -- viresh
Re: [PATCH 1/4] cpufreq: Add a cpufreq pressure feedback for the scheduler
On 13-12-23, 16:41, Tim Chen wrote: > Seems like the pressure value computed from the first CPU applies to all CPU. > Will this be valid for non-homogeneous CPUs that could have different > max_freq and max_capacity? The will be part of different cpufreq policies and so it will work fine. -- viresh
Re: [PATCH 1/4] cpufreq: Add a cpufreq pressure feedback for the scheduler
On 12-12-23, 15:27, Vincent Guittot wrote:
> Provide to the scheduler a feedback about the temporary max available
> capacity. Unlike arch_update_thermal_pressure, this doesn't need to be
> filtered as the pressure will happen for dozens ms or more.
>
> Signed-off-by: Vincent Guittot
> ---
> drivers/cpufreq/cpufreq.c | 48 +++
> include/linux/cpufreq.h | 10
> 2 files changed, 58 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 44db4f59c4cc..7d5f71be8d29 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2563,6 +2563,50 @@ int cpufreq_get_policy(struct cpufreq_policy *policy,
> unsigned int cpu)
> }
> EXPORT_SYMBOL(cpufreq_get_policy);
>
> +DEFINE_PER_CPU(unsigned long, cpufreq_pressure);
> +EXPORT_PER_CPU_SYMBOL_GPL(cpufreq_pressure);
> +
> +/**
> + * cpufreq_update_pressure() - Update cpufreq pressure for CPUs
> + * @cpus: The related CPUs for which max capacity has been reduced
> + * @capped_freq : The maximum allowed frequency that CPUs can run at
> + *
> + * Update the value of cpufreq pressure for all @cpus in the mask. The
> + * cpumask should include all (online+offline) affected CPUs, to avoid
> + * operating on stale data when hot-plug is used for some CPUs. The
> + * @capped_freq reflects the currently allowed max CPUs frequency due to
> + * freq_qos capping. It might be also a boost frequency value, which is
> bigger
> + * than the internal 'capacity_freq_ref' max frequency. In such case the
> + * pressure value should simply be removed, since this is an indication that
> + * there is no capping. The @capped_freq must be provided in kHz.
> + */
> +static void cpufreq_update_pressure(const struct cpumask *cpus,
Since this is defined as 'static', why not just pass policy here ?
> + unsigned long capped_freq)
> +{
> + unsigned long max_capacity, capacity, pressure;
> + u32 max_freq;
> + int cpu;
> +
> + cpu = cpumask_first(cpus);
> + max_capacity = arch_scale_cpu_capacity(cpu);
This anyway expects all of them to be from the same policy ..
> + max_freq = arch_scale_freq_ref(cpu);
> +
> + /*
> + * Handle properly the boost frequencies, which should simply clean
> + * the thermal pressure value.
> + */
> + if (max_freq <= capped_freq)
> + capacity = max_capacity;
> + else
> + capacity = mult_frac(max_capacity, capped_freq, max_freq);
> +
> + pressure = max_capacity - capacity;
> +
Extra blank line here.
> +
> + for_each_cpu(cpu, cpus)
> + WRITE_ONCE(per_cpu(cpufreq_pressure, cpu), pressure);
> +}
> +
> /**
> * cpufreq_set_policy - Modify cpufreq policy parameters.
> * @policy: Policy object to modify.
> @@ -2584,6 +2628,7 @@ static int cpufreq_set_policy(struct cpufreq_policy
> *policy,
> {
> struct cpufreq_policy_data new_data;
> struct cpufreq_governor *old_gov;
> + struct cpumask *cpus;
> int ret;
>
> memcpy(_data.cpuinfo, >cpuinfo, sizeof(policy->cpuinfo));
> @@ -2618,6 +2663,9 @@ static int cpufreq_set_policy(struct cpufreq_policy
> *policy,
> policy->max = __resolve_freq(policy, policy->max, CPUFREQ_RELATION_H);
> trace_cpu_frequency_limits(policy);
>
> + cpus = policy->related_cpus;
You don't need the extra variable anyway, but lets just pass policy
instead to the routine.
> + cpufreq_update_pressure(cpus, policy->max);
> +
> policy->cached_target_freq = UINT_MAX;
>
> pr_debug("new min and max freqs are %u - %u kHz\n",
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index afda5f24d3dd..b1d97edd3253 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -241,6 +241,12 @@ struct kobject *get_governor_parent_kobj(struct
> cpufreq_policy *policy);
> void cpufreq_enable_fast_switch(struct cpufreq_policy *policy);
> void cpufreq_disable_fast_switch(struct cpufreq_policy *policy);
> bool has_target_index(void);
> +
> +DECLARE_PER_CPU(unsigned long, cpufreq_pressure);
> +static inline unsigned long cpufreq_get_pressure(int cpu)
> +{
> + return per_cpu(cpufreq_pressure, cpu);
> +}
> #else
> static inline unsigned int cpufreq_get(unsigned int cpu)
> {
> @@ -263,6 +269,10 @@ static inline bool cpufreq_supports_freq_invariance(void)
> return false;
> }
> static inline void disable_cpufreq(void) { }
> +static inline unsigned long cpufreq_get_pressure(int cpu)
> +{
> + return 0;
> +}
> #endif
>
> #ifdef CONFIG_CPU_FREQ_STAT
> --
> 2.34.1
--
viresh
Re: [PATCH v4 5/7] cpufreq: qcom-hw: Implement CPRh aware OSM programming
On 19-04-21, 13:52, Bjorn Andersson wrote:
> On Tue 19 Jan 11:45 CST 2021, AngeloGioacchino Del Regno wrote:
> @Viresh, do you have any suggestion regarding my last comment?
> > static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
> > {
> > + const struct qcom_cpufreq_soc_data *soc_data;
> > + struct device_node *pd_node;
> > + struct platform_device *pd_dev;
> > struct device *cpu_dev;
> > struct clk *clk;
> > - int ret;
> > + int clk_div, ret;
> > +
> > + cpu_dev = get_cpu_device(0);
> > + if (!cpu_dev)
> > + return -EPROBE_DEFER;
> > +
> > + soc_data = of_device_get_match_data(>dev);
> > + if (!soc_data)
> > + return -EINVAL;
> > +
> > + if (!soc_data->uses_tz) {
> > + /*
> > +* When the OSM is not pre-programmed from TZ, we will
> > +* need to program the sequencer through SCM calls.
> > +*/
> > + if (!qcom_scm_is_available())
> > + return -EPROBE_DEFER;
> > +
> > + /*
> > +* If there are no power-domains, OSM programming cannot be
> > +* performed, as in that case, we wouldn't know where to take
> > +* the params from...
> > +*/
> > + pd_node = of_parse_phandle(cpu_dev->of_node,
> > + "power-domains", 0);
> > + if (!pd_node) {
> > + ret = PTR_ERR(pd_node);
> > + dev_err(cpu_dev, "power domain not found: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + /*
> > +* If the power domain device is not registered yet, then
> > +* defer probing this driver until that is available.
> > +*/
> > + pd_dev = of_find_device_by_node(pd_node);
> > + if (!pd_dev || !pd_dev->dev.driver ||
> > + !device_is_bound(_dev->dev))
> > + return -EPROBE_DEFER;
>
> I wonder if there's a more appropriate way to probe defer on resources
> described in the CPU nodes...
Recently we made some updates to the OPP core to start returning
EPROBE_DEFER on failure to acquire resources. I think you can get rid
of many checks for resources here by just trying to create the OPP
table and check its return value for EPROBE_DEFER.
--
viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 15-04-21, 09:28, Wolfram Sang wrote: > > > Now that we were able to catch you, I will use the opportunity to > > clarify the doubts I had. > > > > - struct mutex lock in struct virtio_i2c, I don't think this is > > required since the core takes care of locking in absence of this. > > This is likely correct. > > > - Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for > > new drivers. > > This is definately correct :) Glad to hear that. Thanks. -- viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 15-04-21, 09:21, Wolfram Sang wrote: > > > I didn't forget this. It is a very small change. I'm not sure if the > > maintainer Wolfram > > > > has any comments so that I can address them together in one version. > > Noted. I'll have a look in the next days. Now that we were able to catch you, I will use the opportunity to clarify the doubts I had. - struct mutex lock in struct virtio_i2c, I don't think this is required since the core takes care of locking in absence of this. - Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for new drivers. :) -- viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 15-04-21, 14:56, Jie Deng wrote: > > On 2021/4/15 14:45, Viresh Kumar wrote: > > On 23-03-21, 10:27, Arnd Bergmann wrote: > > > I usually recommend the use of __maybe_unused for the suspend/resume > > > callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers > > > that hide the exact conditions under which the functions get called. > > > > > > In this driver, there is an explicit #ifdef in the reference to the > > > functions, so > > > it would make sense to use the same #ifdef around the definition. > > Jie, > > > > I was talking about this comment when I said I was expecting a new > > version. I think you still need to make this change. > > > I didn't forget this. It is a very small change. I'm not sure if the > maintainer Wolfram > > has any comments so that I can address them together in one version. Ahh, okay then. That's fine. I have been waiting for the final version to give my Tested/reviewed by :) -- viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 23-03-21, 10:27, Arnd Bergmann wrote: > I usually recommend the use of __maybe_unused for the suspend/resume > callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers > that hide the exact conditions under which the functions get called. > > In this driver, there is an explicit #ifdef in the reference to the > functions, so > it would make sense to use the same #ifdef around the definition. Jie, I was talking about this comment when I said I was expecting a new version. I think you still need to make this change. -- viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 14-04-21, 10:07, Jie Deng wrote: > Hi maintainers, > > What's the status of this patch based on the review comments you got ? I was expecting a new version to be honest.. > Is i2c/for-next the right tree to merge it > ? It should be. -- viresh
Re: [PATCH v4 5/7] cpufreq: qcom-hw: Implement CPRh aware OSM programming
On 12-04-21, 15:01, Taniya Das wrote: > Technically the HW we are trying to program here differs in terms of > clocking, the LUT definitions and many more. It will definitely make > debugging much more troublesome if we try to accomodate multiple versions of > CPUFREQ-HW in the same code. > > Thus to keep it simple, easy to read, debug, the suggestion is to keep it > with "v1" tag as the OSM version we are trying to put here is from OSM1.0. That is a valid point and is always a case with so many drivers. What I am concerned about is how much code is common across versions, if it is 5-70%, or more, then we should definitely share, arrange to have callbacks or ops per version and call them in a generic fashion instead of writing a new driver. This is what's done across drivers/frameworks, etc. -- viresh
Re: [PATCH v4 0/7] cpufreq-qcom-hw: Implement full OSM programming
On 19-01-21, 18:45, AngeloGioacchino Del Regno wrote:
> **
> ** NOTE: To "view the full picture", please look at the following
> ** patch series:
> ** https://patchwork.kernel.org/project/linux-arm-msm/list/?series=413355
> ** This is a subset of that series.
> **
>
> Changes in v4:
> - Huge patch series has been split for better reviewability,
> as suggested by Bjorn
> - Rebased code on top of 266991721c15 ("cpufreq: qcom-hw: enable boost
> support")
Bjorn, what am I supposed to do with patchset ? Is there anyone who can give
this some reviews from qcom team ?
--
viresh
Re: [PATCH -next] ARM: spear: Fix build error with CONFIG_ARCH_SPEAR3XX
On 09-04-21, 09:55, Chen Lifu wrote:
> commit 77f983a9df42 ("spi: pl022: Use GPIOs looked up by the core")
> deleted 'struct pl022_ssp_controller' member 'num_chipselect'.
> We get build error when CONFIG_ARCH_SPEAR3XX is set:
> arch/arm/mach-spear/spear3xx.c:42:3: error: 'struct pl022_ssp_controller' has
> no member named 'num_chipselect'
>42 | .num_chipselect = 2,
> | ^~
> arch/arm/mach-spear/spear3xx.c:42:20: warning: initialization of 'void *'
> from 'int' makes pointer from integer without a cast [-Wint-conversion]
>42 | .num_chipselect = 2,
> |^
>
> Fix the issue by deleting the initialization of 'num_chipselect'
> in spear3xx.c.
>
> Fixes: 77f983a9df42 ("spi: pl022: Use GPIOs looked up by the core")
> Reported-by: Hulk Robot
> Signed-off-by: Chen Lifu
> ---
> arch/arm/mach-spear/spear3xx.c | 10 --
> 1 file changed, 10 deletions(-)
>
> diff --git a/arch/arm/mach-spear/spear3xx.c b/arch/arm/mach-spear/spear3xx.c
> index 8537fcffe5a8..f83321d5e353 100644
> --- a/arch/arm/mach-spear/spear3xx.c
> +++ b/arch/arm/mach-spear/spear3xx.c
> @@ -30,16 +30,6 @@ struct pl022_ssp_controller pl022_plat_data = {
> .dma_filter = pl08x_filter_id,
> .dma_tx_param = "ssp0_tx",
> .dma_rx_param = "ssp0_rx",
> - /*
> - * This is number of spi devices that can be connected to spi. There are
> - * two type of chipselects on which slave devices can work. One is chip
> - * select provided by spi masters other is controlled through external
> - * gpio's. We can't use chipselect provided from spi master (because as
> - * soon as FIFO becomes empty, CS is disabled and transfer ends). So
> - * this number now depends on number of gpios available for spi. each
> - * slave on each master requires a separate gpio pin.
> - */
> - .num_chipselect = 2,
> };
A patch is already applied by Mark to fix this issue.
--
viresh
Re: [RFC PATCH v2 10/10] firmware: arm_scmi: Add virtio transport
On Fri, Nov 6, 2020 at 2:59 AM Peter Hilber
wrote:
> +static int scmi_vio_probe(struct virtio_device *vdev)
> +{
> + struct device *dev = >dev;
> + struct scmi_vio_channel **vioch;
> + bool have_vq_rx;
> + int vq_cnt;
> + int i;
> + struct virtqueue *vqs[VIRTIO_SCMI_VQ_MAX_CNT];
> +
> + vioch = devm_kcalloc(dev, VIRTIO_SCMI_VQ_MAX_CNT, sizeof(*vioch),
> +GFP_KERNEL);
> + if (!vioch)
> + return -ENOMEM;
> +
> + have_vq_rx = virtio_has_feature(vdev, VIRTIO_SCMI_F_P2A_CHANNELS);
> + vq_cnt = have_vq_rx ? VIRTIO_SCMI_VQ_MAX_CNT : 1;
> +
> + for (i = 0; i < vq_cnt; i++) {
> + vioch[i] = devm_kzalloc(dev, sizeof(**vioch), GFP_KERNEL);
> + if (!vioch[i])
> + return -ENOMEM;
> + }
> +
> + if (have_vq_rx)
> + vioch[VIRTIO_SCMI_VQ_RX]->is_rx = true;
> +
> + if (virtio_find_vqs(vdev, vq_cnt, vqs, scmi_vio_complete_callbacks,
> + scmi_vio_vqueue_names, NULL)) {
> + dev_err(dev, "Failed to get %d virtqueue(s)\n", vq_cnt);
> + return -1;
> + }
> + dev_info(dev, "Found %d virtqueue(s)\n", vq_cnt);
> +
> + for (i = 0; i < vq_cnt; i++) {
> + spin_lock_init([i]->lock);
> + vioch[i]->vqueue = vqs[i];
> + vioch[i]->vqueue->priv = vioch[i];
The vqueue->priv field is used by core, you can't update it else
notifications won't work.
> + }
> +
> + vdev->priv = vioch;
> +
> + virtio_device_ready(vdev);
> +
> + return 0;
> +}
diff --git a/drivers/firmware/arm_scmi/virtio.c
b/drivers/firmware/arm_scmi/virtio.c
index f70aa72f34f1..b1af77341b30 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -80,7 +80,8 @@ static int scmi_vio_populate_vq_rx(struct
scmi_vio_channel *vioch,
static void scmi_vio_complete_cb(struct virtqueue *vqueue)
{
- struct scmi_vio_channel *vioch = vqueue->priv;
+ struct scmi_vio_channel **_vioch = vqueue->vdev->priv;
+ struct scmi_vio_channel *vioch = _vioch[vqueue->index];
unsigned long iflags;
unsigned int length;
@@ -454,7 +455,6 @@ static int scmi_vio_probe(struct virtio_device *vdev)
for (i = 0; i < vq_cnt; i++) {
spin_lock_init([i]->lock);
vioch[i]->vqueue = vqs[i];
- vioch[i]->vqueue->priv = vioch[i];
}
vdev->priv = vioch;
Re: [PATCH V8 2/8] cpufreq: mediatek: Enable clock and regulator
On 31-03-21, 13:21, andrew-sh.cheng wrote: > Hi Viresh, > Yes. > As you mentioned, it will be enable by OPP core. > > Per discuss with hotplug owner and regulator owner, > they suggest that "users should not suppose other module, will enable > regulators for them". > They suggest to add enable_regulator here. Which is fine if the modules in question aren't closely related to each other, but OPP core and cpufreq are too closely bound to each other. So much that the cpufreq driver can depend on the OPP core for doing it. Though I won't Nack a patch just for that, but it was just a suggestion. -- viresh
Re: [PATCH V8 2/8] cpufreq: mediatek: Enable clock and regulator
On 23-03-21, 19:33, Andrew-sh.Cheng wrote:
> From: "Andrew-sh.Cheng"
>
> Need to enable regulator,
> so that the max/min requested value will be recorded
> even it is not applied right away.
>
> Intermediate clock is not always enabled by ccf in different projects,
> so cpufreq should enable it by itself.
>
> Signed-off-by: Andrew-sh.Cheng
> ---
> drivers/cpufreq/mediatek-cpufreq.c | 33 +
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> b/drivers/cpufreq/mediatek-cpufreq.c
> index f2e491b25b07..432368707ea6 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -350,6 +350,11 @@ static int mtk_cpu_dvfs_info_init(struct
> mtk_cpu_dvfs_info *info, int cpu)
> ret = PTR_ERR(proc_reg);
> goto out_free_resources;
> }
> + ret = regulator_enable(proc_reg);
> + if (ret) {
> + pr_warn("enable vproc for cpu%d fail\n", cpu);
> + goto out_free_resources;
> + }
Regulators are enabled by OPP core as well now, you sure this is
required ?
--
viresh
Re: [PATCH 2/2] dt-bindings: cpufreq: update cpu type and clock name for MT8173 SoC
On 26-03-21, 11:12, Seiya Wang wrote:
> Update the cpu type of cpu2 and cpu3 since MT8173 used Cortex-a72.
>
> Signed-off-by: Seiya Wang
> ---
> Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
> b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
> index ea4994b35207..ef68711716fb 100644
> --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
> @@ -202,11 +202,11 @@ Example 2 (MT8173 SoC):
>
> cpu2: cpu@100 {
> device_type = "cpu";
> - compatible = "arm,cortex-a57";
> + compatible = "arm,cortex-a72";
> reg = <0x100>;
> enable-method = "psci";
> cpu-idle-states = <_SLEEP_0>;
> - clocks = < CLK_INFRA_CA57SEL>,
> + clocks = < CLK_INFRA_CA72SEL>,
>< CLK_APMIXED_MAINPLL>;
> clock-names = "cpu", "intermediate";
> operating-points-v2 = <_opp_table_b>;
> @@ -214,11 +214,11 @@ Example 2 (MT8173 SoC):
>
> cpu3: cpu@101 {
> device_type = "cpu";
> - compatible = "arm,cortex-a57";
> + compatible = "arm,cortex-a72";
> reg = <0x101>;
> enable-method = "psci";
> cpu-idle-states = <_SLEEP_0>;
> - clocks = < CLK_INFRA_CA57SEL>,
> + clocks = < CLK_INFRA_CA72SEL>,
>< CLK_APMIXED_MAINPLL>;
> clock-names = "cpu", "intermediate";
> operating-points-v2 = <_opp_table_b>;
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
On 25-03-21, 18:19, Jian Dong wrote: > From: Jian Dong > > fixes coccicheck Error: > > drivers/staging/greybus/bootrom.c:301:41-45: ERROR: > fw is NULL but dereferenced. > > if procedure goto label directly, ret will be nefative, so the fw is NULL > and the if(condition) end with dereferenced fw. let's fix it. No, fw is accessed only for !ret case. > Signed-off-by: Jian Dong > --- > drivers/staging/greybus/bootrom.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/greybus/bootrom.c > b/drivers/staging/greybus/bootrom.c > index a8efb86..0439efa 100644 > --- a/drivers/staging/greybus/bootrom.c > +++ b/drivers/staging/greybus/bootrom.c > @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation > *op) > struct gb_bootrom_get_firmware_response *firmware_response; > struct device *dev = >connection->bundle->dev; > unsigned int offset, size; > - enum next_request_type next_request; > + enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE; > int ret = 0; > > /* Disable timeouts */ > @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation > *op) > > queue_work: > /* Refresh timeout */ > - if (!ret && (offset + size == fw->size)) > - next_request = NEXT_REQ_READY_TO_BOOT; > - else > + if (!!ret) > next_request = NEXT_REQ_GET_FIRMWARE; > + else if (offset + size == fw->size) > + next_request = NEXT_REQ_READY_TO_BOOT; > > gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS); The code is fine AFAICT, the coccicheck is buggy as it is detecting a bug here. -- viresh
Re: linux-next: manual merge of the opp tree with the v4l-dvb tree
On 25-03-21, 10:14, Stanimir Varbanov wrote: > I guess you meant this thread. > > https://lore.kernel.org/lkml/20210314163408.22292-1-dig...@gmail.com/ I actually thought some other stuff is having the merge issues (as I was expecting something to happen there) :) Anyway, you did the right thing. Thanks. -- viresh
Re: [PATCH v3 14/15] media: venus: Convert to use resource-managed OPP API
On 25-03-21, 10:13, Stanimir Varbanov wrote: > Hi, > > On 3/14/21 6:34 PM, Dmitry Osipenko wrote: > > From: Yangtao Li > > > > Use resource-managed OPP API to simplify code. > > > > Signed-off-by: Yangtao Li > > Signed-off-by: Dmitry Osipenko > > --- > > drivers/media/platform/qcom/venus/core.h | 1 - > > .../media/platform/qcom/venus/pm_helpers.c| 35 +-- > > 2 files changed, 8 insertions(+), 28 deletions(-) > > > I'll take this through media-tree once OPP API changes are merged. Okay, dropped from my tree. Thanks. -- viresh
Re: [V2][PATCH] cpufreq: dt: check the -EPROBE_DEFER error returned by dev_pm_opp_of_cpumask_add_table
On 25-03-21, 14:42, quanyang.w...@windriver.com wrote:
> From: Quanyang Wang
Made some changes and applied this:
Author: Quanyang Wang
Date: Thu Mar 25 14:42:08 2021 +0800
cpufreq: dt: dev_pm_opp_of_cpumask_add_table() may return -EPROBE_DEFER
The function dev_pm_opp_of_cpumask_add_table() may return -EPROBE_DEFER,
which needs to be propagated to the caller to try probing the driver
later on.
Signed-off-by: Quanyang Wang
[ Viresh: Massage changelog/subject, improve code. ]
Signed-off-by: Viresh Kumar
---
drivers/cpufreq/cpufreq-dt.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index b1e1bdc63b01..ece52863ba62 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -255,10 +255,15 @@ static int dt_cpufreq_early_init(struct device *dev, int
cpu)
* before updating priv->cpus. Otherwise, we will end up creating
* duplicate OPPs for the CPUs.
*
-* OPPs might be populated at runtime, don't check for error here.
+* OPPs might be populated at runtime, don't fail for error here unless
+* it is -EPROBE_DEFER.
*/
- if (!dev_pm_opp_of_cpumask_add_table(priv->cpus))
+ ret = dev_pm_opp_of_cpumask_add_table(priv->cpus);
+ if (!ret) {
priv->have_static_opps = true;
+ } else if (ret == -EPROBE_DEFER) {
+ goto out;
+ }
/*
* The OPP table must be initialized, statically or dynamically, by this
--
viresh
Re: [PATCH] cpufreq: dt: check the error returned by dev_pm_opp_of_cpumask_add_table
On 25-03-21, 13:15, quanyang.wang wrote: > Thank you for pointing it out. Do you mean that even if > dev_pm_opp_of_cpumask_add_table returns > > an error, dev_pm_opp_get_opp_count may still return count > 0 because > someone may call dev_pm_opp_add > > to add OPP to cpu succcessfully at somewhere else? Yes. There are two ways we can add OPPs today: - Statically via device tree. This is what dev_pm_opp_of_cpumask_add_table() tries to do. - Dynamically via call to dev_pm_opp_add(), which I described earlier. What failed here is the static way of adding OPPs, we still need to check if OPPs were added dynamically. -- viresh
Re: [PATCH] cpufreq: dt: check the error returned by dev_pm_opp_of_cpumask_add_table
On 25-03-21, 12:31, quanyang.w...@windriver.com wrote:
> From: Quanyang Wang
>
> The function dev_pm_opp_of_cpumask_add_table may return zero or an
> error. When it returns an error, this means that no OPP table is
> added for the cpumask because _dev_pm_opp_cpumask_remove_table is
> called to free all OPPs associated with the cpu devices in the error
> label "remove_table". So continuing to run the next function
> dev_pm_opp_get_opp_count is meaningless since it always return the
> count value as 0.
>
> There is another reason why we should check the error returned by
> dev_pm_opp_of_cpumask_add_table is that it may return -EPROBE_DEFER
> which comes from clk_get(dev, NULL) in _update_opp_table_clk. When
> the clk for cpu device isn't ready, dt_cpufreq_probe should be deferred
> and wait to be called again. But if we ignore the return error of
> dev_pm_opp_of_cpumask_add_table, dt_cpufreq_probe will return -ENODEV
> because dev_pm_opp_get_opp_count returns the count value as 0,
> the cpufreq-dt driver will fail with the error log as below:
>
> [0.724069] cpu cpu0: OPP table can't be empty
>
> Signed-off-by: Quanyang Wang
> ---
> drivers/cpufreq/cpufreq-dt.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index b1e1bdc63b01..f24359f47b1a 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -255,10 +255,16 @@ static int dt_cpufreq_early_init(struct device *dev,
> int cpu)
>* before updating priv->cpus. Otherwise, we will end up creating
>* duplicate OPPs for the CPUs.
>*
> - * OPPs might be populated at runtime, don't check for error here.
As the comment (which you removed) clearly says, the OPPs maybe added
at runtime, don't check for error here.
When we say runtime, we mean someone may have called dev_pm_opp_add()
for the devices.
> + * We need check the return value here, if it is non-zero, there is
> + * need to go on.
>*/
> - if (!dev_pm_opp_of_cpumask_add_table(priv->cpus))
> - priv->have_static_opps = true;
> + ret = dev_pm_opp_of_cpumask_add_table(priv->cpus);
> + if (ret) {
> + dev_err(cpu_dev, "Failed to add OPP table for CPUs\n");
> + goto out;
> + }
> +
> + priv->have_static_opps = true;
>
> /*
>* The OPP table must be initialized, statically or dynamically, by this
--
viresh
[PATCH V4] kbuild: Add rule to build .dt.yaml files for overlays
The overlay source files are named with .dtso extension now, add a new rule to generate .dt.yaml for them. Reviewed-by: Geert Uytterhoeven Tested-by: Geert Uytterhoeven Signed-off-by: Viresh Kumar --- V4: - Rebase over Frank's cleanup patch: https://lore.kernel.org/lkml/20210324223713.1334666-1-frowand.l...@gmail.com/ - Drop changes to drivers/of/unittest-data/Makefile. - Drop modifications to the rule that builds .dtbo files (as it is already updated by Frank). scripts/Makefile.lib | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 814b430b407e..a682869d8f4b 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -376,6 +376,9 @@ endef $(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE $(call if_changed_rule,dtc,yaml) +$(obj)/%.dt.yaml: $(src)/%.dtso $(DTC) $(DT_TMP_SCHEMA) FORCE + $(call if_changed_rule,dtc,yaml) + dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) # Bzip2 -- 2.25.0.rc1.19.g042ed3e048af
Re: linux-next: manual merge of the opp tree with the v4l-dvb tree
On 24-03-21, 16:49, Stanimir Varbanov wrote:
> Thanks Stephen!
>
> On 3/23/21 2:27 AM, Stephen Rothwell wrote:
> > Hi all,
> >
> > Today's linux-next merge of the opp tree got a conflict in:
> >
> > drivers/media/platform/qcom/venus/pm_helpers.c
> >
> > between commit:
> >
> > 08b1cf474b7f ("media: venus: core, venc, vdec: Fix probe dependency
> > error")
> >
> > from the v4l-dvb tree and commit:
> >
> > 857219ae4043 ("media: venus: Convert to use resource-managed OPP API")
> >
> > from the opp tree.
> >
> > I fixed it up (see below) and can carry the fix as necessary. This
> > is now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your tree
> > is submitted for merging. You may also want to consider cooperating
> > with the maintainer of the conflicting tree to minimise any particularly
> > complex conflicts.
> >
>
> I don't know what is the best solution here.
>
> Viresh, Can I take the OPP API changes through media-tree to avoid
> conflicts?
I already suggested something similar earlier, and I was expecting
Thierry to respond to that.. Not sure who should pick those patches.
https://lore.kernel.org/lkml/20210318103250.shjyd66pxw2g2nsd@vireshk-i7/
Can you please respond to this series then ?
--
viresh
Re: [PATCH v2 1/1] dmaengine: dw: Make it dependent to HAS_IOMEM
On 24-03-21, 16:17, Andy Shevchenko wrote:
> Some architectures do not provide devm_*() APIs. Hence make the driver
> dependent on HAVE_IOMEM.
>
> Fixes: dbde5c2934d1 ("dw_dmac: use devm_* functions to simplify code")
> Reported-by: kernel test robot
> Signed-off-by: Andy Shevchenko
> ---
> v2: used proper option (Serge)
> drivers/dma/dw/Kconfig | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/dma/dw/Kconfig b/drivers/dma/dw/Kconfig
> index e5162690de8f..db25f9b7778c 100644
> --- a/drivers/dma/dw/Kconfig
> +++ b/drivers/dma/dw/Kconfig
> @@ -10,6 +10,7 @@ config DW_DMAC_CORE
>
> config DW_DMAC
> tristate "Synopsys DesignWare AHB DMA platform driver"
> + depends on HAS_IOMEM
> select DW_DMAC_CORE
> help
> Support the Synopsys DesignWare AHB DMA controller. This
> @@ -18,6 +19,7 @@ config DW_DMAC
> config DW_DMAC_PCI
> tristate "Synopsys DesignWare AHB DMA PCI driver"
> depends on PCI
> + depends on HAS_IOMEM
> select DW_DMAC_CORE
> help
> Support the Synopsys DesignWare AHB DMA controller on the
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 24-03-21, 14:41, Jie Deng wrote: > > On 2021/3/24 14:09, Viresh Kumar wrote: > > On 24-03-21, 14:05, Jie Deng wrote: > > Or, now that I think about it a bit more, another thing we can do here is > > see if > > virtqueue_get_buf() returns NULL, if it does then we should keep expecting > > more > > messages as it may be early interrupt. What do you say ? > > I don't think we really need this because for this device, early interrupt > is a bad operation > which should be avoided. I can't think of why this device need to send early > interrupt, what > we can do is to clarify that this means loss of the remaining requests. A > device should never > do this, if it does then loss is the expected result. Fair enough. -- viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 24-03-21, 14:05, Jie Deng wrote: > For simplicity, the original patch sent only 1 message to vq each time . I > changed the way to send I missed those earlier discussions :) > a batch of requests in one time in order to improve efficiency according to > Jason' suggestion. I agree. > As we discussed in the previous emails, the device can raise interrupt when > some requests are still not completed > > though this is not a good operation. In this case, the remaining requests > in the vq will be ignored and > > the i2c_algorithm. master_xfer will return 1 for your example. I will > clarify this in the specs. Right, this needs to be clarified that the receiver shall generate the interrupt only once the virtqueue is empty, not in the middle of it. Or, now that I think about it a bit more, another thing we can do here is see if virtqueue_get_buf() returns NULL, if it does then we should keep expecting more messages as it may be early interrupt. What do you say ? -- viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 23-03-21, 22:19, Jie Deng wrote:
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> int num)
> +{
> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> + struct virtqueue *vq = vi->vq;
> + struct virtio_i2c_req *reqs;
> + unsigned long time_left;
> + int ret, nr;
> +
> + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> + if (!reqs)
> + return -ENOMEM;
> +
> + mutex_lock(>lock);
> +
> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> + if (ret == 0)
> + goto err_unlock_free;
> +
> + nr = ret;
> + reinit_completion(>completion);
> + virtqueue_kick(vq);
Coming back to this again, what is the expectation from the other side for this
? I mean there is no obvious relation between the *msgs* which we are going to
transfer (from the other side's or host's point of view). When should the host
OS call its virtqueue_kick() counterpart ?
Lemme give an example for this. Lets say that we need to transfer 3 messages
here in this routine. What we did was we prepared virtqueue for all 3 messages
together and then called virtqueue_kick().
Now if the other side (host) processes the first message and sends its reply
(with virtqueue_kick() counterpart) before processing the other two messages,
then it will end up calling virtio_i2c_msg_done() here. That will make us call
virtio_i2c_complete_reqs(), while only the first messages is processed until
now and so we will fail for the other two messages straight away.
Should we send only 1 message from i2c-virtio linux driver and then wait for
virtio_i2c_msg_done() to be called, before sending the next message to make sure
it doesn't break ?
--
viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 24-03-21, 12:00, Jie Deng wrote:
>
> On 2021/3/24 11:52, Viresh Kumar wrote:
> > On 24-03-21, 08:53, Jie Deng wrote:
> > > On 2021/3/23 17:38, Viresh Kumar wrote:
> > > > On 23-03-21, 14:31, Viresh Kumar wrote:
> > > > > On 23-03-21, 22:19, Jie Deng wrote:
> > > > > > +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct
> > > > > > i2c_msg *msgs, int num)
> > > > > > +{
> > > > > > + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> > > > > > + struct virtqueue *vq = vi->vq;
> > > > > > + struct virtio_i2c_req *reqs;
> > > > > > + unsigned long time_left;
> > > > > > + int ret, nr;
> > > > > > +
> > > > > > + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> > > > > > + if (!reqs)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + mutex_lock(>lock);
> > > > > > +
> > > > > > + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> > > > > > + if (ret == 0)
> > > > > > + goto err_unlock_free;
> > > > > > +
> > > > > > + nr = ret;
> > > > > > + reinit_completion(>completion);
> > > > > I think I may have found a possible bug here. This
> > > > > reinit_completion() must
> > > > > happen before we call virtio_i2c_send_reqs(). It is certainly
> > > > > possible (surely
> > > > > in corner cases) that virtio_i2c_msg_done() may get called right after
> > > > > virtio_i2c_send_reqs() and before we were able to call
> > > > > reinit_completion(). And
> > > > > in that case we will never see the completion happen at all.
> > > > >
> > > > > > + virtqueue_kick(vq);
> > > > I may have misread this. Can the actually start before virtqueue_kick()
> > > > is
> > > > called ?
> > I didn't write it properly here. I wanted to say,
> >
> > "Can the _transfer_ actually start before virtqueue_kick() is called ?"
>
>
> It can't start until the virtqueue_kick() is called. Call virtqueue_kick
> then wait.
Great, thanks for the confirmation. The code is fine then.
--
viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 24-03-21, 09:17, Jie Deng wrote: > I didn't see the "struct virtio_driver" has a member "struct dev_pm_ops *pm" > > It defines its own hooks (freeze and restore) though it includes "struct > device_driver" > > which has a "struct dev_pm_ops *pm". > > I just follow other virtio drivers to directly use the hooks defined in > "struct virtio_driver". Right, I think we can't use the SIMPLE PM OPS for virtio yet, the core calls only the freeze and restore callbacks which are part of struct virtio_driver. > For this driver, Both __maybe_unused and #ifdef are OK to me. Yes, and so you should use only #ifdef here as Arnd confirmed. You shouldn't be using __maybe_unused as there is nothing confusing here related to the macros. -- viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 24-03-21, 08:53, Jie Deng wrote:
>
> On 2021/3/23 17:38, Viresh Kumar wrote:
> > On 23-03-21, 14:31, Viresh Kumar wrote:
> > > On 23-03-21, 22:19, Jie Deng wrote:
> > > > +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
> > > > *msgs, int num)
> > > > +{
> > > > + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> > > > + struct virtqueue *vq = vi->vq;
> > > > + struct virtio_i2c_req *reqs;
> > > > + unsigned long time_left;
> > > > + int ret, nr;
> > > > +
> > > > + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> > > > + if (!reqs)
> > > > + return -ENOMEM;
> > > > +
> > > > + mutex_lock(>lock);
> > > > +
> > > > + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> > > > + if (ret == 0)
> > > > + goto err_unlock_free;
> > > > +
> > > > + nr = ret;
> > > > + reinit_completion(>completion);
> > > I think I may have found a possible bug here. This reinit_completion()
> > > must
> > > happen before we call virtio_i2c_send_reqs(). It is certainly possible
> > > (surely
> > > in corner cases) that virtio_i2c_msg_done() may get called right after
> > > virtio_i2c_send_reqs() and before we were able to call
> > > reinit_completion(). And
> > > in that case we will never see the completion happen at all.
> > >
> > > > + virtqueue_kick(vq);
> > I may have misread this. Can the actually start before virtqueue_kick() is
> > called ?
I didn't write it properly here. I wanted to say,
"Can the _transfer_ actually start before virtqueue_kick() is called ?"
> No. It starts when wait_for_completion_timeout is called.
No, the transfer doesn't have anything to do with wait_for_completion_timeout().
And if complete() gets called before wait_for_completion_timeout() is called,
then wait_for_completion_timeout() will simply return back.
> So it should be fine here.
>
>
> > If not, then completion may be fine where it is.
> >
--
viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 23-03-21, 14:31, Viresh Kumar wrote:
> On 23-03-21, 22:19, Jie Deng wrote:
> > +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > int num)
> > +{
> > + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> > + struct virtqueue *vq = vi->vq;
> > + struct virtio_i2c_req *reqs;
> > + unsigned long time_left;
> > + int ret, nr;
> > +
> > + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> > + if (!reqs)
> > + return -ENOMEM;
> > +
> > + mutex_lock(>lock);
> > +
> > + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> > + if (ret == 0)
> > + goto err_unlock_free;
> > +
> > + nr = ret;
> > + reinit_completion(>completion);
>
> I think I may have found a possible bug here. This reinit_completion() must
> happen before we call virtio_i2c_send_reqs(). It is certainly possible (surely
> in corner cases) that virtio_i2c_msg_done() may get called right after
> virtio_i2c_send_reqs() and before we were able to call reinit_completion().
> And
> in that case we will never see the completion happen at all.
>
> > + virtqueue_kick(vq);
I may have misread this. Can the actually start before virtqueue_kick() is
called ? If not, then completion may be fine where it is.
--
viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 23-03-21, 22:19, Jie Deng wrote:
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> int num)
> +{
> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> + struct virtqueue *vq = vi->vq;
> + struct virtio_i2c_req *reqs;
> + unsigned long time_left;
> + int ret, nr;
> +
> + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> + if (!reqs)
> + return -ENOMEM;
> +
> + mutex_lock(>lock);
> +
> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> + if (ret == 0)
> + goto err_unlock_free;
> +
> + nr = ret;
> + reinit_completion(>completion);
I think I may have found a possible bug here. This reinit_completion() must
happen before we call virtio_i2c_send_reqs(). It is certainly possible (surely
in corner cases) that virtio_i2c_msg_done() may get called right after
virtio_i2c_send_reqs() and before we were able to call reinit_completion(). And
in that case we will never see the completion happen at all.
> + virtqueue_kick(vq);
--
viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 23-03-21, 16:33, Jie Deng wrote:
> On 2021/3/23 15:27, Viresh Kumar wrote:
>
> > On 23-03-21, 22:19, Jie Deng wrote:
> > > +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
> > > +{
> > > + virtio_i2c_del_vqs(vdev);
> > > + return 0;
> > > +}
> > > +
> > > +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
> > > +{
> > > + return virtio_i2c_setup_vqs(vdev->priv);
> > > +}
> > Sorry for not looking at this earlier, but shouldn't we enclose the above
> > two
> > within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?
>
>
> I remembered I was suggested to use "__maybe_unused" instead of "#ifdef".
>
> You may check this https://lore.kernel.org/patchwork/patch/732981/
>
> The reason may be something like that.
Ahh, thanks for the link Jie. Okay you can leave it as is.
--
viresh
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On 23-03-21, 22:19, Jie Deng wrote:
> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
> +{
> + virtio_i2c_del_vqs(vdev);
> + return 0;
> +}
> +
> +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
> +{
> + return virtio_i2c_setup_vqs(vdev->priv);
> +}
Sorry for not looking at this earlier, but shouldn't we enclose the above two
within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?
> +
> +static struct virtio_driver virtio_i2c_driver = {
> + .id_table = id_table,
> + .probe = virtio_i2c_probe,
> + .remove = virtio_i2c_remove,
> + .driver = {
> + .name = "i2c_virtio",
> + },
> +#ifdef CONFIG_PM_SLEEP
> + .freeze = virtio_i2c_freeze,
> + .restore = virtio_i2c_restore,
> +#endif
> +};
--
viresh
Re: drivers/opp/of.c:959:12: warning: stack frame size of 1056 bytes in function '_of_add_table_indexed'
On 23-03-21, 15:23, kernel test robot wrote: > Hi Viresh, > > FYI, the error/warning still remains. > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > master > head: 84196390620ac0e5070ae36af84c137c6216a7dc > commit: 406e47652161d4f0d9bc4cd6237b36c51497ec75 opp: Create > _of_add_table_indexed() to reduce code duplication > date: 7 weeks ago > config: powerpc64-randconfig-r023-20210323 (attached as .config) > compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project > 14696baaf4c43fe53f738bc292bbe169eed93d5d) > reproduce (this is a W=1 build): > wget > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > ~/bin/make.cross > chmod +x ~/bin/make.cross > # install powerpc64 cross compiling tool for clang build > # apt-get install binutils-powerpc64-linux-gnu > # > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=406e47652161d4f0d9bc4cd6237b36c51497ec75 > git remote add linus > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > git fetch --no-tags linus master > git checkout 406e47652161d4f0d9bc4cd6237b36c51497ec75 > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross > ARCH=powerpc64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > All warnings (new ones prefixed by >>): > > >> drivers/opp/of.c:959:12: warning: stack frame size of 1056 bytes in > >> function '_of_add_table_indexed' [-Wframe-larger-than=] >static int _of_add_table_indexed(struct device *dev, int index) > ^ >1 warning generated. I have reported this on 1st march as well. Looks to be false positive. -- viresh
Re: [PATCH] thermal/drivers/cpuidle_cooling: Fix use after error
On 22-03-21, 09:08, Daniel Lezcano wrote:
> On 22/03/2021 04:29, Viresh Kumar wrote:
> > On 19-03-21, 21:25, Daniel Lezcano wrote:
> >> When the function successfully finishes it logs an information about
> >> the registration of the cooling device and use its name to build the
> >> message. Unfortunately it was freed right before:
> >>
> >> drivers/thermal/cpuidle_cooling.c:218 __cpuidle_cooling_register()
> >>warn: 'name' was already freed.
> >>
> >> Fix this by freeing after the message happened.
> >>
> >> Fixes: 6fd1b186d900 ("thermal/drivers/cpuidle_cooling: Use device name
> >> instead of auto-numbering")
> >
> > Why not merge this with the Fixes patch itself since it isn't there in
> > Linus's
> > tree yet ?
> >
> > Or is your branch strictly immutable ?
>
> Hi Viresh;
>
> The changes follow the path:
>
> testing -> linux-next -> next
>
> The branch next is never rebased. The patch above reached it. This is
> notified by the thermal-bot [1].
Ahh, I see.
Here you go :)
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH v9] i2c: virtio: add a virtio i2c frontend driver
On 22-03-21, 15:53, Jie Deng wrote: > I think your optimization has problems... > > > > bool err_found = timeout; While at it, see if you want to rename this variable as well to something smaller, like "failed". I didn't touch it as it is a matter of personal choice, so leaving it to you.. -- viresh
Re: [PATCH v9] i2c: virtio: add a virtio i2c frontend driver
On 22-03-21, 15:53, Jie Deng wrote:
> On 2021/3/22 14:41, Viresh Kumar wrote:
> I think your optimization has problems...
>
>
> > bool err_found = timeout;
> >
> > for (i = 0; i < nr; i++) {
> > /* Detach the ith request from the vq */
> > req = virtqueue_get_buf(vq, );
> >
> > /*
> > * Condition (req && req == [i]) should always meet since
> > * we have total nr requests in the vq.
> > */
> > if (!err_found &&
> > (WARN_ON(!(req && req == [i])) ||
> > (req->in_hdr.status != VIRTIO_I2C_MSG_OK))) {
> > err_found = true;
> > continue;
>
>
> Just continue here, the ith buf leaks ?
Ahh, this needs to be dropped. You are fight.
> > }
> >
> > i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], err_found);
>
>
> i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], !err_found); ?
Yes again, my mistake :)
--
viresh
Re: [PATCH v9] i2c: virtio: add a virtio i2c frontend driver
On 22-03-21, 21:35, Jie Deng wrote:
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 000..316986e
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,286 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * The Virtio I2C Specification:
> + *
> https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @adap: I2C adapter for this controller
> + * @i2c_lock: lock for virtqueue processing
Name mismatch here.
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> + struct virtio_device *vdev;
> + struct completion completion;
> + struct i2c_adapter adap;
> + struct mutex lock;
> + struct virtqueue *vq;
> +};
> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr,
> + bool timeout)
> +{
> + struct virtio_i2c_req *req;
> + bool err_found = false;
> + unsigned int len;
> + int i, j = 0;
> +
> + for (i = 0; i < nr; i++) {
> + /* Detach the ith request from the vq */
> + req = virtqueue_get_buf(vq, );
> +
> + if (timeout || err_found) {
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], false);
> + continue;
> + }
> +
> + /*
> + * Condition (req && req == [i]) should always meet since
> + * we have total nr requests in the vq.
> + */
> + if (WARN_ON(!(req && req == [i])) ||
> + (req->in_hdr.status != VIRTIO_I2C_MSG_OK)) {
> + err_found = true;
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], false);
> + continue;
> + }
> +
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], true);
> + ++j;
> + }
I think you can simplify the code like this here:
bool err_found = timeout;
for (i = 0; i < nr; i++) {
/* Detach the ith request from the vq */
req = virtqueue_get_buf(vq, );
/*
* Condition (req && req == [i]) should always meet since
* we have total nr requests in the vq.
*/
if (!err_found &&
(WARN_ON(!(req && req == [i])) ||
(req->in_hdr.status != VIRTIO_I2C_MSG_OK))) {
err_found = true;
continue;
}
i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], err_found);
if (!err_found)
++j;
> +
> + return (timeout ? -ETIMEDOUT : j);
> +}
> +
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> int num)
> +{
> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> + struct virtqueue *vq = vi->vq;
> + struct virtio_i2c_req *reqs;
> + unsigned long time_left;
> + int ret, nr;
> +
> + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> + if (!reqs)
> + return -ENOMEM;
> +
> + mutex_lock(>lock);
> +
> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> + if (ret == 0)
> + goto err_unlock_free;
> +
> + nr = ret;
> + reinit_completion(>completion);
> + virtqueue_kick(vq);
> +
> + time_left = wait_for_completion_timeout(>completion, adap->timeout);
> + if (!time_left) {
> + dev_err(>dev, "virtio i2c backend timeout.\n");
> + ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr, true);
> + goto err_unlock_free;
> + }
> +
> + ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr, false);
And this can be optimized as well:
time_left = wait_for_completion_timeout(>completion, adap->timeout);
if (!time_left)
dev_err(>dev, "virtio i2c backend timeout.\n");
ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr, !time_left);
--
viresh
Re: [PATCH] thermal/drivers/cpuidle_cooling: Fix use after error
On 19-03-21, 21:25, Daniel Lezcano wrote:
> When the function successfully finishes it logs an information about
> the registration of the cooling device and use its name to build the
> message. Unfortunately it was freed right before:
>
> drivers/thermal/cpuidle_cooling.c:218 __cpuidle_cooling_register()
> warn: 'name' was already freed.
>
> Fix this by freeing after the message happened.
>
> Fixes: 6fd1b186d900 ("thermal/drivers/cpuidle_cooling: Use device name
> instead of auto-numbering")
Why not merge this with the Fixes patch itself since it isn't there in Linus's
tree yet ?
Or is your branch strictly immutable ?
--
viresh
Re: [PATCH V6 4/4] cpufreq: CPPC: Add support for frequency invariance
On 19-03-21, 17:20, Rafael J. Wysocki wrote: > Sorry for the delay. > > Acked-by: Rafael J. Wysocki Thanks. > and I'm assuming that either you or the sched guys will take care of it. Yeah, I have already queued this up. -- viresh
Re: [RFC][PATCH] sched: Optimize cpufreq_update_util
On 19-03-21, 15:35, Rafael J. Wysocki wrote:
> On Friday, March 19, 2021 8:37:51 AM CET Viresh Kumar wrote:
> > On 18-03-21, 22:28, Peter Zijlstra wrote:
> > > Also, is there a lock order comment in cpufreq somewhere?
> >
> > I don't think so.
> >
> > > I tried
> > > following it, but eventually gave up and figured 'asking' lockdep was
> > > far simpler.
> >
> > This will get called from CPU's online/offline path at worst, nothing more.
>
> I'm not sure if I understand you correctly, but for completeness the callback
> is also set/unset on driver registration and governor switch.
Right, I believe that those cases don't have any specific locking constraints
and so scheduler code doesn't need to worry about them. cpuslocked stuff needs
to be considered though.
> > > +static void cpufreq_update_optimize(void)
> > > +{
> > > + struct update_util_data *data;
> > > + cpu_util_update_f func = NULL, dfunc;
> > > + int cpu;
> > > +
> > > + for_each_online_cpu(cpu) {
> > > + data = per_cpu(cpufreq_update_util_data, cpu);
> > > + dfunc = data ? READ_ONCE(data->func) : NULL;
> > > +
> > > + if (dfunc) {
> > > + if (!func)
> > > + func = dfunc;
> > > + else if (func != dfunc)
> > > + return;
> > > + } else if (func)
> > > + return;
> > > + }
> >
> > So there is nothing cpufreq specific IIRC that can help make this better,
> > this
> > is basically per policy.
>
> Well, in some cases the driver knows that there will never be more that 1 CPU
> per policy and so schedutil will never use the "shared" variant.
>
> For instance, with intel_pstate all CPUs will always use the same callback.
Right, only for single policy cases we can have some optimization (though I
don't feel its worth here) as this isn't going to happen in hotpath.
--
viresh
Re: [RFC][PATCH] sched: Optimize cpufreq_update_util
On 18-03-21, 22:28, Peter Zijlstra wrote:
> Also, is there a lock order comment in cpufreq somewhere?
I don't think so.
> I tried
> following it, but eventually gave up and figured 'asking' lockdep was
> far simpler.
This will get called from CPU's online/offline path at worst, nothing more.
> +static void cpufreq_update_optimize(void)
> +{
> + struct update_util_data *data;
> + cpu_util_update_f func = NULL, dfunc;
> + int cpu;
> +
> + for_each_online_cpu(cpu) {
> + data = per_cpu(cpufreq_update_util_data, cpu);
> + dfunc = data ? READ_ONCE(data->func) : NULL;
> +
> + if (dfunc) {
> + if (!func)
> + func = dfunc;
> + else if (func != dfunc)
> + return;
> + } else if (func)
> + return;
> + }
So there is nothing cpufreq specific IIRC that can help make this better, this
is basically per policy.
For example, on an ARM platform we have two cpufreq policies with one policy
covering 4 CPUs, while the other one covering only 1 (maybe because we didn't
add those CPUs in DT or something else), then also we will end up separate
routines.
Or if we take all CPUs of a policy offline and then bring them up one by one, I
think for the first CPU online event in that policy we will end up using the
sugov_update_single_freq() variant for some time, until the time more CPUs come
up.
So traversing the way you did this is probably something that will work properly
in all corner cases.
--
viresh
Re: [PATCH v8] i2c: virtio: add a virtio i2c frontend driver
On 19-03-21, 14:29, Jie Deng wrote:
> I also see example drivers/i2c/busses/i2c-xiic.c. Some people might think
> this way is more clearer than
>
> updating each member in probe. Basically, I think it's just a matter of
> personal preference which doesn't
Memory used by one instance of struct i2c_adapter (on arm64):
struct i2c_adapter {
struct module *owner;/* 0 8 */
unsigned int class;/* 8 4 */
/* XXX 4 bytes hole, try to pack */
const struct i2c_algorithm * algo; /*16 8 */
void * algo_data;/*24 8 */
const struct i2c_lock_operations * lock_ops;/*32 8 */
struct rt_mutexbus_lock; /*4032 */
/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
struct rt_mutexmux_lock; /*7232 */
inttimeout; /* 104 4 */
intretries; /* 108 4 */
struct device dev; /* 112 744 */
/* XXX last struct has 7 bytes of padding */
/* --- cacheline 13 boundary (832 bytes) was 24 bytes ago --- */
long unsigned int locked_flags; /* 856 8 */
intnr; /* 864 4 */
char name[48]; /* 86848 */
/* XXX 4 bytes hole, try to pack */
/* --- cacheline 14 boundary (896 bytes) was 24 bytes ago --- */
struct completion dev_released; /* 92032 */
struct mutex userspace_clients_lock; /* 95232 */
/* --- cacheline 15 boundary (960 bytes) was 24 bytes ago --- */
struct list_head userspace_clients;/* 98416 */
struct i2c_bus_recovery_info * bus_recovery_info; /* 1000 8 */
const struct i2c_adapter_quirks * quirks; /* 1008 8 */
struct irq_domain *host_notify_domain; /* 1016 8 */
/* --- cacheline 16 boundary (1024 bytes) --- */
/* size: 1024, cachelines: 16, members: 19 */
/* sum members: 1016, holes: 2, sum holes: 8 */
/* paddings: 1, sum paddings: 7 */
};
So, this extra instance that i2c-xiic.c is creating (and that you want to
create) is going to waste 1KB of memory and it will never be used.
This is bad coding practice IMHO and it is not really about personal preference.
--
viresh
Re: [PATCH v8] i2c: virtio: add a virtio i2c frontend driver
On 16-03-21, 18:35, Jie Deng wrote:
> +++ b/drivers/i2c/busses/i2c-virtio.c
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i, outcnt, incnt, err = 0;
> +
> + for (i = 0; i < nr; i++) {
> + if (!msgs[i].len)
> + break;
> +
> + /*
> + * Only 7-bit mode supported for this moment. For the address
> format,
> + * Please check the Virtio I2C Specification.
> + */
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != nr - 1)
> + reqs[i].out_hdr.flags =
> cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
> +
> + outcnt = incnt = 0;
> + sg_init_one(_hdr, [i].out_hdr,
> sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = _hdr;
> +
> + reqs[i].buf = i2c_get_dma_safe_msg_buf([i], 1);
You allocate a buffer here, lets see if they are freeing properly or not (I
remember that I gave same feedback earlier as well, but anyway).
> + if (!reqs[i].buf)
> + break;
> +
> + sg_init_one(_buf, reqs[i].buf, msgs[i].len);
> +
> + if (msgs[i].flags & I2C_M_RD)
> + sgs[outcnt + incnt++] = _buf;
> + else
> + sgs[outcnt++] = _buf;
> +
> + sg_init_one(_hdr, [i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = _hdr;
> +
> + err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, [i],
> GFP_KERNEL);
> + if (err < 0) {
> + pr_err("failed to add msg[%d] to virtqueue.\n", i);
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], false);
On failure here, you freed the buffers for request "i" but not others..
> + break;
> + }
> + }
> +
> + return i;
> +}
> +
> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct virtio_i2c_req *req;
> + unsigned int len;
> + int i, j;
> +
> + for (i = 0; i < nr; i++) {
> + req = virtqueue_get_buf(vq, );
> + if (!(req && req == [i])) {
> + pr_err("msg[%d]: addr=0x%x is out of order.\n", i,
> msgs[i].addr);
> + break;
Since you break here, what will happen to the buffer ? I thought
virtqueue_get_buf() will return a req only once and then you can't access it ?
> + }
> +
> + if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
> + pr_err("msg[%d]: addr=0x%x backend error.\n", i,
> msgs[i].addr);
> + break;
> + }
> +
> + i2c_put_dma_safe_msg_buf(req->buf, [i], true);
> + }
> +
> + /*
> + * Detach all the used buffers from the vq and
> + * Release unused DMA safe buffer if any.
> + */
> + for (j = i; j < nr; j++) {
> + req = virtqueue_get_buf(vq, );
> + if (req)
> + i2c_put_dma_safe_msg_buf(req->buf, [j], false);
This will come in play only if something failed in the earlier loop ? Or my
understanding incorrect ? Also this should be merged with the above for loop
itself, it is just doing part of it.
> + }
> +
> + return i;
> +}
> +
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> int num)
> +{
> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> + struct virtqueue *vq = vi->vq;
> + struct virtio_i2c_req *reqs;
> + unsigned long time_left;
> + int ret, nr;
> +
> + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> + if (!reqs)
> + return -ENOMEM;
> +
> + mutex_lock(>lock);
> +
> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> + if (ret == 0)
> + goto err_unlock_free;
> +
> + nr = ret;
> + reinit_completion(>completion);
> + virtqueue_kick(vq);
> +
> + time_left = wait_for_completion_timeout(>completion, adap->timeout);
> + if (!time_left) {
On error here, we will surely not free the buffers, isn't it ?
> + dev_err(>dev, "virtio i2c backend timeout.\n");
> + ret = -ETIMEDOUT;
> + goto err_unlock_free;
> + }
> +
> + ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);
> +
> +err_unlock_free:
> + mutex_unlock(>lock);
> + kfree(reqs);
> + return ret;
> +}
--
viresh
Re: [PATCH v8] i2c: virtio: add a virtio i2c frontend driver
On 19-03-21, 13:31, Jie Deng wrote: > > On 2021/3/19 11:54, Viresh Kumar wrote: > > On 18-03-21, 15:52, Arnd Bergmann wrote: > > > Allowing multiple virtio-i2c controllers in one system, and multiple i2c > > > devices attached to each controller is clearly something that has to work. > > Good. > > > > > I don't actually see a limitation though. Viresh, what is the problem > > > you see for having multiple controllers? > > I thought this would be a problem in that case as we are using the global > > virtio_adapter here. > > > > + vi->adap = _adapter; > > + i2c_set_adapdata(vi->adap, vi); > > > > Multiple calls to probe() will end up updating the same pointer inside adap. > > > > + vi->adap->dev.parent = >dev; > > > > Same here, overwrite. > > > > + /* Setup ACPI node for controlled devices which will be probed > > through ACPI */ > > + ACPI_COMPANION_SET(>adap->dev, ACPI_COMPANION(pdev)); > > + vi->adap->timeout = HZ / 10; > > > > These may be fine, but still not ideal I believe. > > > > + ret = i2c_add_adapter(vi->adap); > > i > > This should be a problem as well, we must be adding this to some sort of > > list, > > doing some RPM stuff, etc ? > > > > Jie, the solution is to allocate memory for adap at runtime in probe and > > remove > > the virtio_adapter structure completely. > > > If you want to support that. Then I think we don't need to change the > following at all. > > > + .algo = _algorithm, > > + > > + return ret; > > + > > + vi->adap = virtio_adapter; > This is strange, why are you allocating memory for adapter twice ? > Once for virtio_adapter and once for vi->adap ? Either fill the fields > directly for v->adap here and remove virtio_adapter or make vi->adap a > pointer. Yes, your previous version was partly okay but you don't need the virtio_algorithm structure to be allocated. There are only 4 fields you are updating here, just fill them directly in vi->adap. (FWIW, I also suggested the same when I said "Either fill the fields directly for v->adap here and remove virtio_adapter". ) See how drivers/i2c/busses/i2c-versatile.c and most of the other drivers have done it. -- viresh
Re: [PATCH v8] i2c: virtio: add a virtio i2c frontend driver
On 18-03-21, 15:52, Arnd Bergmann wrote: > Allowing multiple virtio-i2c controllers in one system, and multiple i2c > devices attached to each controller is clearly something that has to work. Good. > I don't actually see a limitation though. Viresh, what is the problem > you see for having multiple controllers? I thought this would be a problem in that case as we are using the global virtio_adapter here. + vi->adap = _adapter; + i2c_set_adapdata(vi->adap, vi); Multiple calls to probe() will end up updating the same pointer inside adap. + vi->adap->dev.parent = >dev; Same here, overwrite. + /* Setup ACPI node for controlled devices which will be probed through ACPI */ + ACPI_COMPANION_SET(>adap->dev, ACPI_COMPANION(pdev)); + vi->adap->timeout = HZ / 10; These may be fine, but still not ideal I believe. + ret = i2c_add_adapter(vi->adap); i This should be a problem as well, we must be adding this to some sort of list, doing some RPM stuff, etc ? Jie, the solution is to allocate memory for adap at runtime in probe and remove the virtio_adapter structure completely. -- viresh
Re: [PATCH v4 1/6] soc/tegra: Add devm_tegra_core_dev_init_opp_table()
On 18-03-21, 13:27, Dmitry Osipenko wrote: > 14.03.2021 19:48, Dmitry Osipenko пишет: > > Add common helper which initializes OPP table for Tegra SoC core devices. > > > > Tested-by: Peter Geis # Ouya T30 > > Tested-by: Paul Fertser # PAZ00 T20 > > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > > Tested-by: Matt Merhar # Ouya T30 > > Signed-off-by: Dmitry Osipenko > > --- > > drivers/soc/tegra/common.c | 137 + > > include/soc/tegra/common.h | 30 > > 2 files changed, 167 insertions(+) > > Viresh, do you think it will be possible to take this patch via the OPP > tree along with the devres patches if Thierry will give an ack? This > will allow us to start adding power management support to Tegra drivers > once 5.13 will be released. I can do that.. OR I can give an immutable to Thierry over which he can base these patches.. -- viresh
Re: [PATCH v8] i2c: virtio: add a virtio i2c frontend driver
On 16-03-21, 18:35, Jie Deng wrote:
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> +struct virtio_i2c {
> + struct virtio_device *vdev;
> + struct completion completion;
> + struct i2c_adapter *adap;
> + struct mutex lock;
> + struct virtqueue *vq;
> +};
> +static struct i2c_adapter virtio_adapter = {
> + .owner = THIS_MODULE,
> + .name = "Virtio I2C Adapter",
> + .class = I2C_CLASS_DEPRECATED,
> + .algo = _algorithm,
> +};
And FWIW, I still have my concerns about mutex-lock and
I2C_CLASS_DEPRECATED, but yeah we need Wolfram to confirm on them.
LGTM otherwise.
--
viresh
Re: [PATCH v8] i2c: virtio: add a virtio i2c frontend driver
On 16-03-21, 18:35, Jie Deng wrote:
> +static struct i2c_adapter virtio_adapter = {
> + .owner = THIS_MODULE,
> + .name = "Virtio I2C Adapter",
> + .class = I2C_CLASS_DEPRECATED,
> + .algo = _algorithm,
> +};
> +
> +static int virtio_i2c_probe(struct virtio_device *vdev)
> +{
> + struct device *pdev = vdev->dev.parent;
> + struct virtio_i2c *vi;
> + int ret;
> +
> + vi = devm_kzalloc(>dev, sizeof(*vi), GFP_KERNEL);
> + if (!vi)
> + return -ENOMEM;
> +
> + vdev->priv = vi;
> + vi->vdev = vdev;
> +
> + mutex_init(>lock);
> + init_completion(>completion);
> +
> + ret = virtio_i2c_setup_vqs(vi);
> + if (ret)
> + return ret;
> +
> + vi->adap = _adapter;
> + i2c_set_adapdata(vi->adap, vi);
> + vi->adap->dev.parent = >dev;
FWIW, this limits this driver to support a single device ever. We
can't bind multiple devices to this driver now. Yeah, perhaps we will
never be required to do so, but who knows.
--
viresh
Re: [PATCH v7] i2c: virtio: add a virtio i2c frontend driver
On 12-03-21, 13:41, Viresh Kumar wrote:
> > > > +static struct i2c_adapter virtio_adapter = {
> > > > + .owner = THIS_MODULE,
> > > > + .name = "Virtio I2C Adapter",
> > > > + .class = I2C_CLASS_DEPRECATED,
> > > What happened to this discussion ?
> > >
> > > https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/
> >
> > My understanding is that new driver sets this to warn users that the adapter
> > doesn't support classes anymore.
>
> I think the warning is relevant for old drivers who used to support
> classes and not for new ones.
>
> > I'm not sure if Wolfram can explain it more clear for you.
>
> Okay, lemme dig in a bit then.
>
> $ git grep -l i2c_add_adapter drivers/i2c/busses/ | wc -l
> 77
>
> $ git grep -l I2C_CLASS_DEPRECATED drivers/i2c/busses/
> drivers/i2c/busses/i2c-at91-core.c
> drivers/i2c/busses/i2c-bcm2835.c
> drivers/i2c/busses/i2c-davinci.c
> drivers/i2c/busses/i2c-designware-platdrv.c
> drivers/i2c/busses/i2c-mv64xxx.c
> drivers/i2c/busses/i2c-nomadik.c
> drivers/i2c/busses/i2c-ocores.c
> drivers/i2c/busses/i2c-omap.c
> drivers/i2c/busses/i2c-rcar.c
> drivers/i2c/busses/i2c-s3c2410.c
> drivers/i2c/busses/i2c-tegra.c
> drivers/i2c/busses/i2c-virtio.c
> drivers/i2c/busses/i2c-xiic.c
>
> i.e. only 13 of 77 drivers are using this flag.
>
> The latest addition among these drivers is i2c-bcm2835.c and it was
> added back in 2013 and the flag was added to it in 2014:
>
> commit 37888f71e2c9 ("i2c: i2c-bcm2835: deprecate class based instantiation")
>
> FWIW, I also checked all the new drivers added since kernel release
> v4.0 and none of them set this flag.
What happened with this ? I didn't get a reply to it and the new
version still has it.
--
viresh
[PATCH V3] kbuild: Allow .dtso format for overlay source files
Since the overlays dtb files are now named as .dtbo, there is a lot of interest in similarly naming the overlay source dts files as .dtso. This patch makes the necessary changes to allow .dtso format for overlay source files. Note that the device-tree unit-tests name their overlay files as .dts and it would take substantial amount of changes to update them to .dtso and that would probably involve some local rules in unit-test's Makefile as well. This patch goes for a simpler solution instead, i.e. allow .dts to .dtbo conversion for device-tree unit-tests. Reviewed-by: Geert Uytterhoeven Tested-by: Geert Uytterhoeven Signed-off-by: Viresh Kumar --- This was made part of the bigger patchset earlier (most of it already got merged) whose last version was V11, but this patch was only sent twice earlier and so starting with Version 3. Changes since V2: - Add the dts -> dtbo rule in unittest-data/Makefile. - Remove the -I parameter to dtc. drivers/of/unittest-data/Makefile | 6 ++ scripts/Makefile.lib | 5 - 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index a5d2d9254b2c..e8dd839bdcbb 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -86,3 +86,9 @@ static_test_1-dtbs := static_base_1.dtb $(apply_static_overlay_1) static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2) dtb-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb + +# We can't name the overlay files .dtso, it would require much more substantial +# changes in Makefile. Instead allow building the overlay .dtbo files from .dts +# source files for unittests. +$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE + $(call if_changed_dep,dtc) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 90b095c60f79..a682869d8f4b 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -347,7 +347,7 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) -$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE +$(obj)/%.dtbo: $(src)/%.dtso $(DTC) FORCE $(call if_changed_dep,dtc) overlay-y := $(addprefix $(obj)/, $(overlay-y)) @@ -376,6 +376,9 @@ endef $(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE $(call if_changed_rule,dtc,yaml) +$(obj)/%.dt.yaml: $(src)/%.dtso $(DTC) $(DT_TMP_SCHEMA) FORCE + $(call if_changed_rule,dtc,yaml) + dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) # Bzip2 -- 2.25.0.rc1.19.g042ed3e048af
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 16-03-21, 00:36, Frank Rowand wrote: > I should have looked at patch 3/5 more carefully instead of counting on > Masahiro to check it out and simply build testing. > > Patch 3/5 does not seem correct. I'll look over all the makefile related > changes that have been accepted (if any) and patch 3/5 later today (Tuesday). What is already merged by Rob is what everyone reviewed and it wasn't related to this .dtso/.dtbo discussion we are having. I will resend a new patchset with the stuff left for merging (which will involve the thing we are discussing here), so we can get a clear picture of it. -- viresh
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 16-03-21, 02:43, Masahiro Yamada wrote: > On Mon, Mar 15, 2021 at 3:40 PM Viresh Kumar wrote: > > On 14-03-21, 20:16, Frank Rowand wrote: > > What about doing this then in unittest's Makefile instead (which I > > already suggested earlier), that will make everything work just fine > > without any other changes ? > > > > +# Required for of unittest files as they can't be renamed to .dtso > > +$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE > > + $(call if_changed_dep,dtc) > > If those rules are only needed by drivers/of/unittest-data/Makefile, > they should not be located in scripts/Makefile.lib. Right, this is exactly what I suggested. > But how can we fix drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a779*.dts > if these are doing bad things. > They seem to be overlay files even though the file name suffix is .dts > > $ find drivers -name '*.dts' > drivers/staging/pi433/Documentation/devicetree/pi433-overlay.dts > drivers/staging/mt7621-dts/gbpc2.dts > drivers/staging/mt7621-dts/gbpc1.dts > drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts > drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts > drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts > drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts > drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts For all the above files, even if they are really overlay files, we won't use fdtoverlay tool to apply them to some base dtb and so if we leave them as is, i.e. .dts->.dtb, it won't break anything. The problem only happens if someone wants to generate .dtbo for them instead and then they should be named .dtso as we won't allow .dts -> .dtbo conversion there. -- viresh
Re: linux-next: build warning after merge of the opp tree
On 16-03-21, 11:15, Stephen Rothwell wrote:
> Hi all,
>
> After merging the opp tree, today's linux-next build (powerpc
> ppc64_defconfig) produced this warning:
>
> In file included from include/linux/devfreq.h:15,
> from drivers/base/power/main.c:36:
> include/linux/pm_opp.h:341:1: warning: 'devm_pm_opp_register_set_opp_helper'
> defined but not used [-Wunused-function]
> 341 | devm_pm_opp_register_set_opp_helper(struct device *dev,
> | ^~~
>
> Introduced by commit
>
> 357b804aa0b9 ("opp: Change return type of
> devm_pm_opp_register_set_opp_helper()")
>
> The "inline" was removed :-(
Fixed and pushed. Thanks.
--
viresh
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 14-03-21, 20:16, Frank Rowand wrote: > On 3/12/21 11:11 PM, Frank Rowand wrote: > > On 3/12/21 1:13 AM, Viresh Kumar wrote: > >> On 12-03-21, 01:09, Frank Rowand wrote: > >>> I suggested having the .dtso files include the .dts file because that is > >>> a relatively > >>> small and easy change to test. What would probably make more sense is > >>> the rename > >>> the existing overlay .dts files to be .dtso files and then for each > >>> overlay .dtso > >>> file create a new .dts file that #includes the corresponding .dtso file. > >>> This is > >>> more work and churn, but easier to document that the .dts files are a > >>> hack that is > >>> needed so that the corresponding .dtb.S files will be generated. > >> > >> What about creating links instead then ? > >> > > > > I don't really like the idea of using links here. > > > > Maybe it is best to make the changes needed to allow the unittest > > overlays to be .dtso instead of .dts. > > > > Off the top of my head: > > > > scripts/Makefile.lib: > > The rule for %.dtb.S invokes cmd_dt_S_dtb, which puts the > > overlay data in section .dtb.init.rodata, with a label > > pointing to the beginning of the overlay __dtb_XXX_begin and > > a label pointing to the end of the overlay __dtb_XXX_end, > > for the overlay named XXX. I _think_ that you could simply > > add a corresponding rule for %.dtbo.S using a new command > > cmd_dt_S_dtbo (the same as cmd_dt_S_dtb, except use labels > > __dtbo_XXX_begin and __dtbo_XXX_end). > > If you do the above, please put it in drivers/of/unittest-data/Makefile > instead of scripts/Makefile.lib because it is unittest.c specific and > not meant to be anywhere else in the kernel. What about doing this then in unittest's Makefile instead (which I already suggested earlier), that will make everything work just fine without any other changes ? +# Required for of unittest files as they can't be renamed to .dtso +$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE + $(call if_changed_dep,dtc) -- viresh
Re: [PATCH] cpufreq: cppc: simplify default delay_us setting
On 12-03-21, 19:50, Tom Saeger wrote:
> Simplify case when setting default in cppc_cpufreq_get_transition_delay_us.
>
> Signed-off-by: Tom Saeger
> ---
> drivers/cpufreq/cppc_cpufreq.c | 14 ++
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 8a482c434ea6..2f769b1630c5 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -216,26 +216,16 @@ static unsigned int
> cppc_cpufreq_get_transition_delay_us(unsigned int cpu)
> {
> unsigned long implementor = read_cpuid_implementor();
> unsigned long part_num = read_cpuid_part_number();
> - unsigned int delay_us = 0;
>
> switch (implementor) {
> case ARM_CPU_IMP_QCOM:
> switch (part_num) {
> case QCOM_CPU_PART_FALKOR_V1:
> case QCOM_CPU_PART_FALKOR:
> - delay_us = 1;
> - break;
> - default:
> - delay_us = cppc_get_transition_latency(cpu) /
> NSEC_PER_USEC;
> - break;
> + return 1;
> }
> - break;
> - default:
> - delay_us = cppc_get_transition_latency(cpu) / NSEC_PER_USEC;
> - break;
> }
> -
> - return delay_us;
> + return cppc_get_transition_latency(cpu) / NSEC_PER_USEC;
> }
Applied. Thanks.
--
viresh
Re: [PATCH v3 00/15] Introduce devm_pm_opp_* API
On 14-03-21, 19:33, Dmitry Osipenko wrote: > This series adds resource-managed OPP API helpers and makes drivers > to use them. > > Changelog: > > v3: - Dropped dev_pm_opp_register_notifier(). > > - Changed return type of the devm helpers from opp_table pointer > to errno. > > - Corrected drm/msm patch which missed to remove opp_put_supported_hw() > from a6xx_gpu. Note that the a5xx_gpu driver was missing the > opp_put_supported_hw() at all. > > - Corrected spelling of the ack from Mark Brown. Applied all patches except 11/15. Thanks. -- viresh
Re: [PATCH v3 11/15] drm/msm: Convert to use resource-managed OPP API
On 14-03-21, 19:34, Dmitry Osipenko wrote: > From: Yangtao Li > > Use resource-managed OPP API to simplify code. > > Signed-off-by: Yangtao Li > Signed-off-by: Dmitry Osipenko > --- > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 +- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++-- > drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 -- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 23 +++ > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 -- > drivers/gpu/drm/msm/dp/dp_ctrl.c| 30 + > drivers/gpu/drm/msm/dp/dp_ctrl.h| 1 - > drivers/gpu/drm/msm/dp/dp_display.c | 5 + > drivers/gpu/drm/msm/dsi/dsi_host.c | 13 --- > 11 files changed, 25 insertions(+), 68 deletions(-) This patch has some updates in linux-next, which I don't have. Please get this merged with the drm tree over 5.13-rc1 later. Acked-by: Viresh Kumar -- viresh
Re: [PATCH] ia64: fix format string for ia64-acpi-cpu-freq
On 13-03-21, 10:42, Sergei Trofimovich wrote:
> Fix warning with %lx / s64 mismatch:
>
> CC [M] drivers/cpufreq/ia64-acpi-cpufreq.o
> drivers/cpufreq/ia64-acpi-cpufreq.c: In function 'processor_get_pstate':
> warning: format '%lx' expects argument of type 'long unsigned int',
> but argument 3 has type 's64' {aka 'long long int'} [-Wformat=]
>
> CC: "Rafael J. Wysocki"
> CC: Viresh Kumar
> CC: linux...@vger.kernel.org
> Signed-off-by: Sergei Trofimovich
> ---
> drivers/cpufreq/ia64-acpi-cpufreq.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/ia64-acpi-cpufreq.c
> b/drivers/cpufreq/ia64-acpi-cpufreq.c
> index 2efe7189ccc4..c6bdc455517f 100644
> --- a/drivers/cpufreq/ia64-acpi-cpufreq.c
> +++ b/drivers/cpufreq/ia64-acpi-cpufreq.c
> @@ -54,7 +54,7 @@ processor_set_pstate (
> retval = ia64_pal_set_pstate((u64)value);
>
> if (retval) {
> - pr_debug("Failed to set freq to 0x%x, with error 0x%lx\n",
> + pr_debug("Failed to set freq to 0x%x, with error 0x%llx\n",
> value, retval);
> return -ENODEV;
> }
> @@ -77,7 +77,7 @@ processor_get_pstate (
>
> if (retval)
> pr_debug("Failed to get current freq with "
> - "error 0x%lx, idx 0x%x\n", retval, *value);
> + "error 0x%llx, idx 0x%x\n", retval, *value);
>
> return (int)retval;
> }
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH V3] cpufreq: Rudimentary typos fix in the file s5pv210-cpufreq.c
On 13-03-21, 09:19, Bhaskar Chowdhury wrote:
>
> Trivial spelling fixes throughout the file.
>
> Signed-off-by: Bhaskar Chowdhury
> ---
> Changes from V2:
> Incoporated the findings of Tom Saeger
>
> drivers/cpufreq/s5pv210-cpufreq.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
Applied with this change.
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c
b/drivers/cpufreq/s5pv210-cpufreq.c
index 73110b005716..ad7d4f272ddc 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -91,7 +91,7 @@ static DEFINE_MUTEX(set_freq_lock);
/* Use 800MHz when entering sleep mode */
#define SLEEP_FREQ (800 * 1000)
-/* Tracks if cpu frequency can be updated anymore */
+/* Tracks if CPU frequency can be updated anymore */
static bool no_cpufreq_access;
/*
@@ -439,8 +439,8 @@ static int s5pv210_target(struct cpufreq_policy *policy,
unsigned int index)
}
/*
-* L4 level needs to change memory bus speed, hence onedram clock
divider
-* and memory refresh parameter should be changed
+* L4 level needs to change memory bus speed, hence ONEDRAM clock
+* divider and memory refresh parameter should be changed
*/
if (bus_speed_changing) {
reg = readl_relaxed(S5P_CLK_DIV6);
--
viresh
Re: [PATCH v2 4/5] thermal/drivers/cpuidle_cooling: Use device name instead of auto-numbering
On 12-03-21, 18:03, Daniel Lezcano wrote: > Currently the naming of a cooling device is just a cooling technique > followed by a number. When there are multiple cooling devices using > the same technique, it is impossible to clearly identify the related > device as this one is just a number. > > For instance: > > thermal-idle-0 > thermal-idle-1 > thermal-idle-2 > thermal-idle-3 > etc ... > > The 'thermal' prefix is redundant with the subsystem namespace. This > patch removes the 'thermal prefix and changes the number by the device > name. So the naming above becomes: > > idle-cpu0 > idle-cpu1 > idle-cpu2 > idle-cpu3 > etc ... > > Signed-off-by: Daniel Lezcano > Reviewed-by: Lukasz Luba I acked for both the patches :( -- viresh
Re: [PATCH v7] i2c: virtio: add a virtio i2c frontend driver
On 12-03-21, 15:51, Jie Deng wrote:
>
> On 2021/3/12 14:10, Viresh Kumar wrote:
> > I saw your email about wrong version being sent, I already wrote some
> > reviews. Sending them anyway for FWIW :)
> >
> > On 12-03-21, 21:33, Jie Deng wrote:
> > > +struct virtio_i2c {
> > > + struct virtio_device *vdev;
> > > + struct completion completion;
> > > + struct i2c_adapter adap;
> > > + struct mutex lock;
> > As I said in the previous version (Yes, we were both waiting for
> > Wolfram to answer that), this lock shouldn't be required at all.
> >
> > And since none of us have a use-case at hand where we will have a
> > problem without this lock, we should really skip it. We can always
> > come back and add it if we find an issue somewhere. Until then, better
> > to keep it simple.
> The problem is you can't guarantee that adap->algo->master_xfer
> is only called from i2c_transfer. Any function who holds the adapter can
> call
> adap->algo->master_xfer directly.
See my last reply here, (almost) no one in the mainline kernel call it
directly. And perhaps you can consider the caller broken in that case
and so there is no need of an extra lock, unless you have a case that
is broken.
https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/
> I prefer to avoid potential issues rather
> than
> find a issue then fix.
This is a very hypothetical issue IMHO as the kernel code doesn't have
such a user. There is no need of locks here, else the i2c core won't
have handled it by itself.
> >
> > > +
> > > +static struct i2c_adapter virtio_adapter = {
> > > + .owner = THIS_MODULE,
> > > + .name = "Virtio I2C Adapter",
> > > + .class = I2C_CLASS_DEPRECATED,
> > What happened to this discussion ?
> >
> > https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/
>
> My understanding is that new driver sets this to warn users that the adapter
> doesn't support classes anymore.
I think the warning is relevant for old drivers who used to support
classes and not for new ones.
> I'm not sure if Wolfram can explain it more clear for you.
Okay, lemme dig in a bit then.
$ git grep -l i2c_add_adapter drivers/i2c/busses/ | wc -l
77
$ git grep -l I2C_CLASS_DEPRECATED drivers/i2c/busses/
drivers/i2c/busses/i2c-at91-core.c
drivers/i2c/busses/i2c-bcm2835.c
drivers/i2c/busses/i2c-davinci.c
drivers/i2c/busses/i2c-designware-platdrv.c
drivers/i2c/busses/i2c-mv64xxx.c
drivers/i2c/busses/i2c-nomadik.c
drivers/i2c/busses/i2c-ocores.c
drivers/i2c/busses/i2c-omap.c
drivers/i2c/busses/i2c-rcar.c
drivers/i2c/busses/i2c-s3c2410.c
drivers/i2c/busses/i2c-tegra.c
drivers/i2c/busses/i2c-virtio.c
drivers/i2c/busses/i2c-xiic.c
i.e. only 13 of 77 drivers are using this flag.
The latest addition among these drivers is i2c-bcm2835.c and it was
added back in 2013 and the flag was added to it in 2014:
commit 37888f71e2c9 ("i2c: i2c-bcm2835: deprecate class based instantiation")
FWIW, I also checked all the new drivers added since kernel release
v4.0 and none of them set this flag.
--
viresh
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 12-03-21, 01:09, Frank Rowand wrote: > I suggested having the .dtso files include the .dts file because that is a > relatively > small and easy change to test. What would probably make more sense is the > rename > the existing overlay .dts files to be .dtso files and then for each overlay > .dtso > file create a new .dts file that #includes the corresponding .dtso file. > This is > more work and churn, but easier to document that the .dts files are a hack > that is > needed so that the corresponding .dtb.S files will be generated. What about creating links instead then ? -- viresh
Re: [PATCH v7] i2c: virtio: add a virtio i2c frontend driver
I saw your email about wrong version being sent, I already wrote some
reviews. Sending them anyway for FWIW :)
On 12-03-21, 21:33, Jie Deng wrote:
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 000..bbde8de
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,287 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * The Virtio I2C Specification:
> + *
> https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @adap: I2C adapter for this controller
> + * @i2c_lock: lock for virtqueue processing
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> + struct virtio_device *vdev;
> + struct completion completion;
> + struct i2c_adapter adap;
> + struct mutex lock;
As I said in the previous version (Yes, we were both waiting for
Wolfram to answer that), this lock shouldn't be required at all.
And since none of us have a use-case at hand where we will have a
problem without this lock, we should really skip it. We can always
come back and add it if we find an issue somewhere. Until then, better
to keep it simple.
> + struct virtqueue *vq;
> +};
> +
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @buf: the buffer into which data is read, or from which it's written
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> + struct virtio_i2c_out_hdr out_hdr;
> + uint8_t *buf;
> + struct virtio_i2c_in_hdr in_hdr;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> + struct virtio_i2c *vi = vq->vdev->priv;
> +
> + complete(>completion);
> +}
> +
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i, outcnt, incnt, err = 0;
> +
> + for (i = 0; i < nr; i++) {
> + if (!msgs[i].len)
> + break;
> +
> + /*
> + * Only 7-bit mode supported for this moment. For the address
> format,
> + * Please check the Virtio I2C Specification.
> + */
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != nr - 1)
> + reqs[i].out_hdr.flags =
> cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
> +
> + outcnt = incnt = 0;
> + sg_init_one(_hdr, [i].out_hdr,
> sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = _hdr;
> +
> + reqs[i].buf = i2c_get_dma_safe_msg_buf([i], 1);
> + if (!reqs[i].buf)
> + break;
> +
> + sg_init_one(_buf, reqs[i].buf, msgs[i].len);
> +
> + if (msgs[i].flags & I2C_M_RD)
> + sgs[outcnt + incnt++] = _buf;
> + else
> + sgs[outcnt++] = _buf;
> +
> + sg_init_one(_hdr, [i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = _hdr;
> +
> + err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, [i],
> GFP_KERNEL);
> + if (err < 0) {
> + pr_err("failed to add msg[%d] to virtqueue.\n", i);
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], false);
> + break;
> + }
> + }
> +
> + return i;
> +}
> +
> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct virtio_i2c_req *req;
> + unsigned int len, unused;
> + int i, j;
> +
> + for (i = 0; i < nr; i++) {
> + req = virtqueue_get_buf(vq, );
> + if (!(req && req == [i])) {
> + pr_err("msg[%d]: addr=0x%x is out of order.\n", i,
> msgs[i].addr);
> + break;
> + }
> +
> + if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
> + pr_err("msg[%d]: addr=0x%x backend error.\n", i,
> msgs[i].addr);
> + break;
> + }
> +
> + i2c_put_dma_safe_msg_buf(req->buf, [i], true);
> + }
> +
> + /* Release unused DMA safe buffer if any */
> + for (j = i; j < nr; j++)
> + i2c_put_dma_safe_msg_buf(req->buf, [j], false);
> +
> + /*
Re: [PATCH v2 01/14] opp: Add devres wrapper for dev_pm_opp_set_clkname
On 11-03-21, 22:20, Dmitry Osipenko wrote:
> +struct opp_table *devm_pm_opp_set_clkname(struct device *dev, const char
> *name)
> +{
> + struct opp_table *opp_table;
> + int err;
> +
> + opp_table = dev_pm_opp_set_clkname(dev, name);
> + if (IS_ERR(opp_table))
> + return opp_table;
> +
> + err = devm_add_action_or_reset(dev, devm_pm_opp_clkname_release,
> opp_table);
> + if (err)
> + opp_table = ERR_PTR(err);
> +
> + return opp_table;
> +}
I wonder if we still need to return opp_table from here, or a simple
integer is fine.. The callers shouldn't be required to use the OPP
table directly anymore I believe and so better simplify the return
part of this and all other routines you are adding here..
If there is a user which needs the opp_table, let it use the regular
non-devm variant.
--
viresh
Re: [PATCH v2 05/14] opp: Add devres wrapper for dev_pm_opp_register_notifier
On 11-03-21, 22:20, Dmitry Osipenko wrote: > From: Yangtao Li > > Add devres wrapper for dev_pm_opp_register_notifier() to simplify driver > code. > > Signed-off-by: Yangtao Li > Signed-off-by: Dmitry Osipenko > --- > drivers/opp/core.c | 38 ++ > include/linux/pm_opp.h | 6 ++ > 2 files changed, 44 insertions(+) As I said in the previous version, I am not sure if we need this patch at all. This has only one user. -- viresh
Re: [PATCH 1/3] thermal/drivers/cpufreq_cooling: Use device name instead of auto-numbering
On 10-03-21, 12:45, Daniel Lezcano wrote: > Currently the naming of a cooling device is just a cooling technique > followed by a number. When there are multiple cooling devices using > the same technique, it is impossible to clearly identify the related > device as this one is just a number. > > For instance: > > thermal-cpufreq-0 > thermal-cpufreq-1 > etc ... > > The 'thermal' prefix is redundant with the subsystem namespace. This > patch removes the 'thermal prefix and changes the number by the device > name. So the naming above becomes: > > cpufreq-cpu0 > cpufreq-cpu4 > etc ... > > Signed-off-by: Daniel Lezcano > --- > drivers/thermal/cpufreq_cooling.c | 28 +++- > 1 file changed, 7 insertions(+), 21 deletions(-) For 1,3/3 Acked-by: Viresh Kumar -- viresh
Re: [PATCH V6 3/4] arch_topology: Export arch_freq_scale and helpers
On 10-03-21, 10:53, Viresh Kumar wrote:
> It is possible now for other parts of the kernel to provide their own
> implementation of sched_freq_tick() and they can very well be modules
> themselves (like CPPC cpufreq driver, which is going to use these in a
> later commit).
>
> Export arch_freq_scale and topology_{set|clear}_scale_freq_source().
>
> Reviewed-by: Ionela Voinescu
> Tested-by: Ionela Voinescu
> Tested-by: Vincent Guittot
> Signed-off-by: Viresh Kumar
> ---
> drivers/base/arch_topology.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index ebcd2ea3091f..995e52b9eca4 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -78,6 +78,7 @@ void topology_set_scale_freq_source(struct scale_freq_data
> *data,
>
> update_scale_freq_invariant(true);
> }
> +EXPORT_SYMBOL_GPL(topology_set_scale_freq_source);
>
> void topology_clear_scale_freq_source(enum scale_freq_source source,
> const struct cpumask *cpus)
> @@ -96,6 +97,7 @@ void topology_clear_scale_freq_source(enum
> scale_freq_source source,
>
> update_scale_freq_invariant(false);
> }
> +EXPORT_SYMBOL_GPL(topology_clear_scale_freq_source);
>
> void topology_scale_freq_tick(void)
> {
> @@ -106,6 +108,7 @@ void topology_scale_freq_tick(void)
> }
>
> DEFINE_PER_CPU(unsigned long, arch_freq_scale) = SCHED_CAPACITY_SCALE;
> +EXPORT_SYMBOL_GPL(arch_freq_scale);
Did minor change here after testing bot complaint of sparse warning.
-EXPORT_SYMBOL_GPL(arch_freq_scale);
+EXPORT_PER_CPU_SYMBOL_GPL(arch_freq_scale);
--
viresh
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 10-03-21, 20:24, Masahiro Yamada wrote: > Even without "-I dts", > >inform = guess_input_format(arg, "dts"); > > seems to fall back to "dts" anyway, > but I guess you wanted to make this explicit, correct? > > +# Required for of unit-test files as they can't be renamed to .dtso > > If you go with *.dtso, I think you will rename > all *.dts under the drivers/ directory. > > What is blocking you from making this consistent? What about this patch instead ? This localizes the dts->dtbo hack to unitest's Makefile at least. diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index a5d2d9254b2c..9f3426ec3fab 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -86,3 +86,7 @@ static_test_1-dtbs := static_base_1.dtb $(apply_static_overlay_1) static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2) dtb-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb + +# Required for of unittest files as they can't be renamed to .dtso +$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE + $(call if_changed_dep,dtc) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index bc045a54a34e..77a9be055e51 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -347,7 +347,7 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) -$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE +$(obj)/%.dtbo: $(src)/%.dtso $(DTC) FORCE $(call if_changed_dep,dtc) overlay-y := $(addprefix $(obj)/, $(overlay-y)) @@ -375,6 +375,9 @@ endef $(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE $(call if_changed_rule,dtc,yaml) +$(obj)/%.dt.yaml: $(src)/%.dtso $(DTC) $(DT_TMP_SCHEMA) FORCE + $(call if_changed_rule,dtc,yaml) + dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) # Bzip2 -- viresh
Re: [PATCH V11 0/5] dt: Add fdtoverlay rule and statically build unittest
On 10-03-21, 11:05, Viresh Kumar wrote: > Hi, > > This patchset adds a generic rule for applying overlays using fdtoverlay > tool and then updates unittests to get built statically using the same. > > V10->V11: > - Update patch 4/5 to fix checkpatch warning on spaces and tabs. > - Added Acked-by from Masahiro for patch 2/5. > > > Rob Herring (1): > kbuild: Add generic rule to apply fdtoverlay > > Viresh Kumar (4): > kbuild: Simplify builds with CONFIG_OF_ALL_DTBS > kbuild: Allow .dtso format for overlay source files > of: unittest: Create overlay_common.dtsi and testcases_common.dtsi > of: unittest: Statically apply overlays using fdtoverlay Rob, can you please apply patch 1-2,4-5 from this series ? I will send a new version of the patch 3/5 (related to .dtso) separately. -- viresh
Re: [PATCH V11 0/5] dt: Add fdtoverlay rule and statically build unittest
On 11-03-21, 17:27, Frank Rowand wrote: > On 3/9/21 11:35 PM, Viresh Kumar wrote: > > Viresh Kumar (4): > > kbuild: Simplify builds with CONFIG_OF_ALL_DTBS > > kbuild: Allow .dtso format for overlay source files > > of: unittest: Create overlay_common.dtsi and testcases_common.dtsi > > of: unittest: Statically apply overlays using fdtoverlay > > > > drivers/of/unittest-data/Makefile | 48 ++ > > drivers/of/unittest-data/overlay_base.dts | 90 +- > > drivers/of/unittest-data/overlay_common.dtsi | 91 +++ > > drivers/of/unittest-data/static_base_1.dts| 4 + > > drivers/of/unittest-data/static_base_2.dts| 4 + > > drivers/of/unittest-data/testcases.dts| 23 ++--- > > .../of/unittest-data/testcases_common.dtsi| 19 > > .../of/unittest-data/tests-interrupts.dtsi| 11 +-- > > scripts/Makefile.lib | 40 ++-- > > 9 files changed, 218 insertions(+), 112 deletions(-) > > create mode 100644 drivers/of/unittest-data/overlay_common.dtsi > > create mode 100644 drivers/of/unittest-data/static_base_1.dts > > create mode 100644 drivers/of/unittest-data/static_base_2.dts > > create mode 100644 drivers/of/unittest-data/testcases_common.dtsi > > > > > > base-commit: a38fd8748464831584a19438cbb3082b5a2dab15 > > > > Does not apply to 5.12-rc2 I was based right over the 5.12-rc2 tag. > because of a dependency on a patch to > scripts/Makefile.lib. That patch has been merged by Linus > somewhere between -rc2 and -rc3. git log --oneline v5.12-rc2..origin/master -- scripts/Makefile.lib gives no results to me. > I had a working version > between -rc2 and -rc3 at commit e6f197677b2e I have tried both Linus' tree and linux-next, and I don't see this commit. > that does have > the required patch, so that is the version I used to test > this series. > > There is still confusion caused by the contortions that unittest > goes through to mis-use base DTBs vs overlay DTBs, so _after_ > this series is merged by Rob, I will poke around and see if > I can change unittest so that it does not look like it is > mis-using DTBs and overlay DTBs. > > > Reviewed-by: Frank Rowand > Tested-by: Frank Rowand Thanks. -- viresh
[PATCH V2] opp: Don't drop extra references to OPPs accidentally
From: Beata Michalska
We are required to call dev_pm_opp_put() from outside of the
opp_table->lock as debugfs removal needs to happen lock-less to avoid
circular dependency issues.
commit cf1fac943c63 ("opp: Reduce the size of critical section in
_opp_kref_release()") tried to fix that introducing a new routine
_opp_get_next() which keeps returning OPPs that can be freed by the
callers and this routine shall be called without holding the
opp_table->lock.
Though the commit overlooked the fact that the OPPs can be referenced by
other users as well and this routine will end up dropping references
which were taken by other users and hence freeing the OPPs prematurely.
In effect, other users of the OPPs will end up having invalid pointers
at hand. We didn't see any crash reports earlier as the exact situation
never happened, though it is certainly possible.
We need a way to mark which OPPs are no longer referenced by the OPP
core, so we don't drop extra references to them accidentally.
This commit adds another OPP flag, "removed", which is used to track
this. And now we should never end up dropping extra references to the
OPPs.
Cc: v5.11+ # v5.11+
Fixes: cf1fac943c63 ("opp: Reduce the size of critical section in
_opp_kref_release()")
Signed-off-by: Beata Michalska
[ Viresh: Almost rewrote entire patch, added new "removed" field,
rewrote commit log and added the correct Fixes tag. ]
Co-developed-by: Viresh Kumar
Signed-off-by: Viresh Kumar
---
Sending it formally again so others don't miss it.
drivers/opp/core.c | 48 --
drivers/opp/opp.h | 2 ++
2 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c2689386a906..1556998425d5 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1492,7 +1492,11 @@ static struct dev_pm_opp *_opp_get_next(struct opp_table
*opp_table,
mutex_lock(_table->lock);
list_for_each_entry(temp, _table->opp_list, node) {
- if (dynamic == temp->dynamic) {
+ /*
+* Refcount must be dropped only once for each OPP by OPP core,
+* do that with help of "removed" flag.
+*/
+ if (!temp->removed && dynamic == temp->dynamic) {
opp = temp;
break;
}
@@ -1502,10 +1506,27 @@ static struct dev_pm_opp *_opp_get_next(struct
opp_table *opp_table,
return opp;
}
-bool _opp_remove_all_static(struct opp_table *opp_table)
+/*
+ * Can't call dev_pm_opp_put() from under the lock as debugfs removal needs to
+ * happen lock less to avoid circular dependency issues. This routine must be
+ * called without the opp_table->lock held.
+ */
+static void _opp_remove_all(struct opp_table *opp_table, bool dynamic)
{
struct dev_pm_opp *opp;
+ while ((opp = _opp_get_next(opp_table, dynamic))) {
+ opp->removed = true;
+ dev_pm_opp_put(opp);
+
+ /* Drop the references taken by dev_pm_opp_add() */
+ if (dynamic)
+ dev_pm_opp_put_opp_table(opp_table);
+ }
+}
+
+bool _opp_remove_all_static(struct opp_table *opp_table)
+{
mutex_lock(_table->lock);
if (!opp_table->parsed_static_opps) {
@@ -1520,13 +1541,7 @@ bool _opp_remove_all_static(struct opp_table *opp_table)
mutex_unlock(_table->lock);
- /*
-* Can't remove the OPP from under the lock, debugfs removal needs to
-* happen lock less to avoid circular dependency issues.
-*/
- while ((opp = _opp_get_next(opp_table, false)))
- dev_pm_opp_put(opp);
-
+ _opp_remove_all(opp_table, false);
return true;
}
@@ -1539,25 +1554,12 @@ bool _opp_remove_all_static(struct opp_table *opp_table)
void dev_pm_opp_remove_all_dynamic(struct device *dev)
{
struct opp_table *opp_table;
- struct dev_pm_opp *opp;
- int count = 0;
opp_table = _find_opp_table(dev);
if (IS_ERR(opp_table))
return;
- /*
-* Can't remove the OPP from under the lock, debugfs removal needs to
-* happen lock less to avoid circular dependency issues.
-*/
- while ((opp = _opp_get_next(opp_table, true))) {
- dev_pm_opp_put(opp);
- count++;
- }
-
- /* Drop the references taken by dev_pm_opp_add() */
- while (count--)
- dev_pm_opp_put_opp_table(opp_table);
+ _opp_remove_all(opp_table, true);
/* Drop the reference taken by _find_opp_table() */
dev_pm_opp_put_opp_table(opp_table);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 50fb9dced3c5..407c3bfe51d9 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -56,6 +56,7 @@ extern struct list_head opp_tab
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 11-03-21, 00:18, Masahiro Yamada wrote: > On Wed, Mar 10, 2021 at 11:48 PM Viresh Kumar wrote: > > > > On 10-03-21, 20:29, Masahiro Yamada wrote: > > > BTW, is the attached patch good for DTC? > > > > > > I do not know when '-O dtbo' is useful, > > > unless I am missing something. > > > > It is useful if we are sending the -O option all the time (I have > > already given more details to your patch) as outform will not be NULL. > > > "-O dtbo" was useful to make your buggy patch work. > > That is not justification. I wasn't giving justification, but rather saying why it was required earlier. And I agree that it isn't required once we drop the -O parameter here. -- viresh
Re: [PATCH] opp: Invalidate current opp when draining the opp list
On 10-03-21, 23:03, Beata Michalska wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index c2689386a906..150be4c28c99 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1492,7 +1492,11 @@ static struct dev_pm_opp *_opp_get_next(struct
> > opp_table *opp_table,
> >
> > mutex_lock(_table->lock);
> > list_for_each_entry(temp, _table->opp_list, node) {
> > - if (dynamic == temp->dynamic) {
> > + /*
> > +* Refcount must be dropped only once for each OPP by OPP core,
> > +* do that with help of "removed" flag.
> > +*/
> > + if (!temp->removed && dynamic == temp->dynamic) {
> > opp = temp;
> > break;
> > }
> How about tweaking the _opp_get_next to use list_for_each_entry_continue
> instead ? It would eliminate the need of tracking the 'removed' status
> and could save few cycles as it wouldn't have to go through the list
> starting from it's beginning each time it is being called.
> Happy to draft another version.
I tried that as well, but the problem is that we need to drop locks in
between and if the next OPP somehow gets freed or another one gets
added there, we can be in trouble. To make this work without any side
effects, the new field was kind of required.
--
viresh
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 10-03-21, 20:29, Masahiro Yamada wrote: > BTW, is the attached patch good for DTC? > > I do not know when '-O dtbo' is useful, > unless I am missing something. It is useful if we are sending the -O option all the time (I have already given more details to your patch) as outform will not be NULL. -- viresh
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 10-03-21, 20:24, Masahiro Yamada wrote: > On Wed, Mar 10, 2021 at 2:35 PM Viresh Kumar wrote: > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > > index bc045a54a34e..59e86f67f9e0 100644 > > --- a/scripts/Makefile.lib > > +++ b/scripts/Makefile.lib > > @@ -339,7 +339,7 @@ $(obj)/%.dtb.S: $(obj)/%.dtb FORCE > > > > quiet_cmd_dtc = DTC $@ > > cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o > > $(dtc-tmp) $< ; \ > > - $(DTC) -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ > > + $(DTC) -I dts -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ > > Even without "-I dts", > >inform = guess_input_format(arg, "dts"); > > seems to fall back to "dts" anyway, I missed this TBH. > but I guess you wanted to make this explicit, correct? That can be a reason now :) > I will drop the ugly -O. > https://patchwork.kernel.org/project/linux-kbuild/patch/20210310110824.782209-1-masahi...@kernel.org/ But if we are going to depend on DTC to guess it right, then we shouldn't add -I at all.. > I will queue it to linux-kbuild/fixes. > > > > > $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \ > > -d $(depfile).dtc.tmp $(dtc-tmp) ; \ > > cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile) > > @@ -347,9 +347,13 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x > > assembler-with-cpp -o $(dtc-tmp) $< ; > > $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE > > $(call if_changed_dep,dtc) > > > > +# Required for of unit-test files as they can't be renamed to .dtso > > If you go with *.dtso, I think you will rename > all *.dts under the drivers/ directory. > > What is blocking you from making this consistent? The unit-test dts files are designed differently (we have had lots of discussion between Frank and David on that) and they aren't purely overlay or base files. They are designed to do some tricky testing and renaming them to .dtso won't be right, we are just reusing them to do static (build time) testing as well. I think it would be better if we can drop the existing %.dtbo rule here (i.e. dtbo from .dts) and do some magic in unit-test's Makefile, so it is localised at least instead of it here. Any ideas for that ? -- viresh
Re: [PATCH] kbuild: remove unneeded -O option to dtc
On 10-03-21, 20:08, Masahiro Yamada wrote:
> This piece of code converts the target suffix to the dtc -O option:
>
> *.dtb -> -O dtb
> *.dt.yaml -> -O yaml
>
> Commit ce88c9c79455 ("kbuild: Add support to build overlays (%.dtbo)")
> added the third case:
>
> *.dtbo -> -O dtbo
>
> This works thanks to commit 163f0469bf2e ("dtc: Allow overlays to have
> .dtbo extension") in the upstream DTC, which has already been pulled in
> the kernel.
>
> However, I think it is a bit odd because "dtbo" is not a format name.
> At least, it does not show up in the help message of dtc.
>
> $ scripts/dtc/dtc --help
> [ snip ]
> -O, --out-format
> Output formats are:
> dts - device tree source text
> dtb - device tree blob
> yaml - device tree encoded as YAML
> asm - assembler source
>
> So, I am not a big fan of the second hunk of that change:
>
> } else if (streq(outform, "dtbo")) {
> dt_to_blob(outf, dti, outversion);
Looking at the very first version of the patch I sent,
https://lore.kernel.org/lkml/7aa70809eff3f32d821761d2a763e4fb72ecc8fb.1609844956.git.viresh.ku...@linaro.org/
this change was the only thing that was required to make it work.
I think the reason was that "outform != NULL" as -O option was passed
by the kernel.
> Anyway, we did not need to do this in Makefile in the first place.
>
> guess_type_by_name() had already understood ".yaml" before commit
> 4f0e3a57d6eb ("kbuild: Add support for DT binding schema checks"),
> and now does ".dtbo" as well.
>
> Makefile does not need to duplicate the same logic. Let's leave it
> to dtc.
>
> Signed-off-by: Masahiro Yamada
> ---
>
> scripts/Makefile.lib | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index eee59184de64..90a4e04cd8f5 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -327,7 +327,7 @@ $(obj)/%.dtb.S: $(obj)/%.dtb FORCE
>
> quiet_cmd_dtc = DTC $@
> cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp)
> $< ; \
> - $(DTC) -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \
> + $(DTC) -o $@ -b 0 \
> $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \
> -d $(depfile).dtc.tmp $(dtc-tmp) ; \
> cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
LGTM.
Reviewed-by: Viresh Kumar
--
viresh
Re: [PATCH] opp: Invalidate current opp when draining the opp list
On 09-03-21, 12:14, Beata Michalska wrote:
> With the current version, once the _opp_get_next returns opp
> that is the current_opp, the while loop will break, leaving all
> the opps that are on the list after current_opp
Thanks for your excellent review Beata. I have been missing a lot
lately :(
Though now I have spent more time on it and the bug seems to be more
severe. This is what I have applied now :)
I was able to test that the OPP table gets freed without any reference
counts issues now.
--
viresh
-8<-
From: Beata Michalska
Date: Thu, 4 Mar 2021 15:07:34 +
Subject: [PATCH] opp: Don't drop extra references to OPPs accidentally
We are required to call dev_pm_opp_put() from outside of the
opp_table->lock as debugfs removal needs to happen lock-less to avoid
circular dependency issues.
commit cf1fac943c63 ("opp: Reduce the size of critical section in
_opp_kref_release()") tried to fix that introducing a new routine
_opp_get_next() which keeps returning OPPs that can be freed by the
callers and this routine shall be called without holding the
opp_table->lock.
Though the commit overlooked the fact that the OPPs can be referenced by
other users as well and this routine will end up dropping references
which were taken by other users and hence freeing the OPPs prematurely.
In effect, other users of the OPPs will end up having invalid pointers
at hand. We didn't see any crash reports earlier as the exact situation
never happened, though it is certainly possible.
We need a way to mark which OPPs are no longer referenced by the OPP
core, so we don't drop extra references to them accidentally.
This commit adds another OPP flag, "removed", which is used to track
this. And now we should never end up dropping extra references to the
OPPs.
Cc: v5.11+ # v5.11+
Fixes: cf1fac943c63 ("opp: Reduce the size of critical section in
_opp_kref_release()")
Signed-off-by: Beata Michalska
[ Viresh: Almost rewrote entire patch, added new "removed" field,
rewrote commit log and added the correct Fixes tag. ]
Co-developed-by: Viresh Kumar
Signed-off-by: Viresh Kumar
---
drivers/opp/core.c | 48 --
drivers/opp/opp.h | 2 ++
2 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c2689386a906..150be4c28c99 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1492,7 +1492,11 @@ static struct dev_pm_opp *_opp_get_next(struct opp_table
*opp_table,
mutex_lock(_table->lock);
list_for_each_entry(temp, _table->opp_list, node) {
- if (dynamic == temp->dynamic) {
+ /*
+* Refcount must be dropped only once for each OPP by OPP core,
+* do that with help of "removed" flag.
+*/
+ if (!temp->removed && dynamic == temp->dynamic) {
opp = temp;
break;
}
@@ -1502,10 +1506,27 @@ static struct dev_pm_opp *_opp_get_next(struct
opp_table *opp_table,
return opp;
}
-bool _opp_remove_all_static(struct opp_table *opp_table)
+/*
+ * Can't call dev_pm_opp_put() from under the lock as debugfs removal needs to
+ * happen lock less to avoid circular dependency issues. This routine must be
+ * called without the opp_table->lock held.
+ */
+static void _opp_drain_list(struct opp_table *opp_table, bool dynamic)
{
struct dev_pm_opp *opp;
+ while ((opp = _opp_get_next(opp_table, dynamic))) {
+ opp->removed = true;
+ dev_pm_opp_put(opp);
+
+ /* Drop the references taken by dev_pm_opp_add() */
+ if (dynamic)
+ dev_pm_opp_put_opp_table(opp_table);
+ }
+}
+
+bool _opp_remove_all_static(struct opp_table *opp_table)
+{
mutex_lock(_table->lock);
if (!opp_table->parsed_static_opps) {
@@ -1520,13 +1541,7 @@ bool _opp_remove_all_static(struct opp_table *opp_table)
mutex_unlock(_table->lock);
- /*
-* Can't remove the OPP from under the lock, debugfs removal needs to
-* happen lock less to avoid circular dependency issues.
-*/
- while ((opp = _opp_get_next(opp_table, false)))
- dev_pm_opp_put(opp);
-
+ _opp_drain_list(opp_table, false);
return true;
}
@@ -1539,25 +1554,12 @@ bool _opp_remove_all_static(struct opp_table *opp_table)
void dev_pm_opp_remove_all_dynamic(struct device *dev)
{
struct opp_table *opp_table;
- struct dev_pm_opp *opp;
- int count = 0;
opp_table = _find_opp_table(dev);
if (IS_ERR(opp_table))
return;
- /*
-* Can't remove the OPP from under the lock, debugfs removal needs to
-* happen lock
[PATCH V11 5/5] of: unittest: Statically apply overlays using fdtoverlay
Now that fdtoverlay is part of the kernel build, start using it to test the unitest overlays we have by applying them statically. Create two new base files static_base_1.dts and static_base_2.dts which includes other .dtsi files. Some unittest overlays deliberately contain errors that unittest checks for. These overlays will cause fdtoverlay to fail, and are thus not included for static builds. Signed-off-by: Viresh Kumar --- drivers/of/unittest-data/Makefile | 48 ++ drivers/of/unittest-data/static_base_1.dts | 4 ++ drivers/of/unittest-data/static_base_2.dts | 4 ++ 3 files changed, 56 insertions(+) create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index 009f4045c8e4..a5d2d9254b2c 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -38,3 +38,51 @@ DTC_FLAGS_testcases += -@ # suppress warnings about intentional errors DTC_FLAGS_testcases += -Wno-interrupts_property + +# Apply overlays statically with fdtoverlay. This is a build time test that +# the overlays can be applied successfully by fdtoverlay. This does not +# guarantee that the overlays can be applied successfully at run time by +# unittest, but it provides a bit of build time test coverage for those +# who do not execute unittest. +# +# The overlays are applied on top of static_base_1.dtb and static_base_2.dtb to +# create static_test_1.dtb and static_test_2.dtb. If fdtoverlay detects an +# error than the kernel build will fail. static_test_1.dtb and +# static_test_2.dtb are not consumed by unittest. +# +# Some unittest overlays deliberately contain errors that unittest checks for. +# These overlays will cause fdtoverlay to fail, and are thus not included +# in the static test: +#overlay_bad_add_dup_node.dtbo \ +#overlay_bad_add_dup_prop.dtbo \ +#overlay_bad_phandle.dtbo \ +#overlay_bad_symbol.dtbo \ + +apply_static_overlay_1 := overlay_0.dtbo \ + overlay_1.dtbo \ + overlay_2.dtbo \ + overlay_3.dtbo \ + overlay_4.dtbo \ + overlay_5.dtbo \ + overlay_6.dtbo \ + overlay_7.dtbo \ + overlay_8.dtbo \ + overlay_9.dtbo \ + overlay_10.dtbo \ + overlay_11.dtbo \ + overlay_12.dtbo \ + overlay_13.dtbo \ + overlay_15.dtbo \ + overlay_gpio_01.dtbo \ + overlay_gpio_02a.dtbo \ + overlay_gpio_02b.dtbo \ + overlay_gpio_03.dtbo \ + overlay_gpio_04a.dtbo \ + overlay_gpio_04b.dtbo + +apply_static_overlay_2 := overlay.dtbo + +static_test_1-dtbs := static_base_1.dtb $(apply_static_overlay_1) +static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2) + +dtb-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb diff --git a/drivers/of/unittest-data/static_base_1.dts b/drivers/of/unittest-data/static_base_1.dts new file mode 100644 index ..10556cb3f01f --- /dev/null +++ b/drivers/of/unittest-data/static_base_1.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "testcases_common.dtsi" diff --git a/drivers/of/unittest-data/static_base_2.dts b/drivers/of/unittest-data/static_base_2.dts new file mode 100644 index ..b0ea9504d6f3 --- /dev/null +++ b/drivers/of/unittest-data/static_base_2.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "overlay_common.dtsi" -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
Since the overlays dtb files are now named as .dtbo, there is a lot of interest in similarly naming the overlay source dts files as .dtso. This patch makes the necessary changes to allow .dtso format for overlay source files. Note that we still support generating .dtbo files from .dts files. This is required for the source files present in drivers/of/unittest-data/, because they can't be renamed to .dtso as they are used for some runtime testing as well. Reviewed-by: Geert Uytterhoeven Tested-by: Geert Uytterhoeven Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index bc045a54a34e..59e86f67f9e0 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -339,7 +339,7 @@ $(obj)/%.dtb.S: $(obj)/%.dtb FORCE quiet_cmd_dtc = DTC $@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; \ - $(DTC) -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ + $(DTC) -I dts -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \ -d $(depfile).dtc.tmp $(dtc-tmp) ; \ cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile) @@ -347,9 +347,13 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +# Required for of unit-test files as they can't be renamed to .dtso $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +$(obj)/%.dtbo: $(src)/%.dtso $(DTC) FORCE + $(call if_changed_dep,dtc) + overlay-y := $(addprefix $(obj)/, $(overlay-y)) quiet_cmd_fdtoverlay = DTOVL $@ @@ -375,6 +379,9 @@ endef $(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE $(call if_changed_rule,dtc,yaml) +$(obj)/%.dt.yaml: $(src)/%.dtso $(DTC) $(DT_TMP_SCHEMA) FORCE + $(call if_changed_rule,dtc,yaml) + dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) # Bzip2 -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V11 1/5] kbuild: Simplify builds with CONFIG_OF_ALL_DTBS
We update 'extra-y' based on CONFIG_OF_ALL_DTBS three times. It would be far more straight forward if we rather update dtb-y to include all .dtb files if CONFIG_OF_ALL_DTBS is enabled. Acked-by: Masahiro Yamada Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index eee59184de64..a2658242d956 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -73,14 +73,13 @@ always-y += $(userprogs-always-y) $(userprogs-always-m) # DTB # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built +dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-) + always-y += $(dtb-y) -always-$(CONFIG_OF_ALL_DTBS) += $(dtb-) ifneq ($(CHECK_DTBS),) always-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) always-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y)) -always-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) -always-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-)) endif # Add subdir path -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V11 0/5] dt: Add fdtoverlay rule and statically build unittest
Hi, This patchset adds a generic rule for applying overlays using fdtoverlay tool and then updates unittests to get built statically using the same. V10->V11: - Update patch 4/5 to fix checkpatch warning on spaces and tabs. - Added Acked-by from Masahiro for patch 2/5. V9->V10: - Add a new patch to allow .dtso files. - Update 2/5 to be more efficient and also generate symbols for base files automatically. - No need to add lines like DTC_FLAGS_foo_base += -@ in patch 5/5. - Add Ack by Masahiro for 1/5. V8->V9: - Added some comment in patch 3/4 based on Frank's suggestions. V7->V8: - Patch 1 is new. - Platforms need to use dtb-y += foo.dtb instead of overlay-y += foo.dtb. - Use multi_depend instead of .SECONDEXPANSION. - Use dtb-y for unittest instead of overlay-y. - Rename the commented dtb filess in unittest Makefile as .dtbo. - Improved Makefile code (I am learning a lot every day :) V6->V7: - Dropped the first 4 patches, already merged. - Patch 1/3 is new, suggested by Rob and slightly modified by me. - Adapt Patch 3/3 to the new rule and name the overlay dtbs as .dtbo. -- Viresh Rob Herring (1): kbuild: Add generic rule to apply fdtoverlay Viresh Kumar (4): kbuild: Simplify builds with CONFIG_OF_ALL_DTBS kbuild: Allow .dtso format for overlay source files of: unittest: Create overlay_common.dtsi and testcases_common.dtsi of: unittest: Statically apply overlays using fdtoverlay drivers/of/unittest-data/Makefile | 48 ++ drivers/of/unittest-data/overlay_base.dts | 90 +- drivers/of/unittest-data/overlay_common.dtsi | 91 +++ drivers/of/unittest-data/static_base_1.dts| 4 + drivers/of/unittest-data/static_base_2.dts| 4 + drivers/of/unittest-data/testcases.dts| 23 ++--- .../of/unittest-data/testcases_common.dtsi| 19 .../of/unittest-data/tests-interrupts.dtsi| 11 +-- scripts/Makefile.lib | 40 ++-- 9 files changed, 218 insertions(+), 112 deletions(-) create mode 100644 drivers/of/unittest-data/overlay_common.dtsi create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts create mode 100644 drivers/of/unittest-data/testcases_common.dtsi base-commit: a38fd8748464831584a19438cbb3082b5a2dab15 -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V11 4/5] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
In order to build-test the same unit-test files using fdtoverlay tool,
move the device nodes from the existing overlay_base.dts and
testcases_common.dts files to .dtsi counterparts. The .dts files now
include the new .dtsi files, resulting in exactly the same behavior as
earlier.
The .dtsi files can now be reused for compile time tests using
fdtoverlay (will be done by a later commit).
This is required because the base files passed to fdtoverlay tool
shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
tag).
Note that this commit also moves "testcase-device2" node to
testcases.dts from tests-interrupts.dtsi, as this node has a deliberate
error in it and is only relevant for runtime testing done with
unittest.c.
Signed-off-by: Viresh Kumar
---
drivers/of/unittest-data/overlay_base.dts | 90 +-
drivers/of/unittest-data/overlay_common.dtsi | 91 +++
drivers/of/unittest-data/testcases.dts| 23 ++---
.../of/unittest-data/testcases_common.dtsi| 19
.../of/unittest-data/tests-interrupts.dtsi| 11 +--
5 files changed, 128 insertions(+), 106 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
diff --git a/drivers/of/unittest-data/overlay_base.dts
b/drivers/of/unittest-data/overlay_base.dts
index 99ab9d12d00b..ab9014589c5d 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -2,92 +2,4 @@
/dts-v1/;
/plugin/;
-/*
- * Base device tree that overlays will be applied against.
- *
- * Do not add any properties in node "/".
- * Do not add any nodes other than "/testcase-data-2" in node "/".
- * Do not add anything that would result in dtc creating node "/__fixups__".
- * dtc will create nodes "/__symbols__" and "/__local_fixups__".
- */
-
-/ {
- testcase-data-2 {
- #address-cells = <1>;
- #size-cells = <1>;
-
- electric_1: substation@100 {
- compatible = "ot,big-volts-control";
- reg = < 0x0100 0x100 >;
- status = "disabled";
-
- hvac_1: hvac-medium-1 {
- compatible = "ot,hvac-medium";
- heat-range = < 50 75 >;
- cool-range = < 60 80 >;
- };
-
- spin_ctrl_1: motor-1 {
- compatible = "ot,ferris-wheel-motor";
- spin = "clockwise";
- rpm_avail = < 50 >;
- };
-
- spin_ctrl_2: motor-8 {
- compatible = "ot,roller-coaster-motor";
- };
- };
-
- rides_1: fairway-1 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,rides";
- status = "disabled";
- orientation = < 127 >;
-
- ride@100 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,roller-coaster";
- reg = < 0x0100 0x100 >;
- hvac-provider = < _1 >;
- hvac-thermostat = < 29 > ;
- hvac-zones = < 14 >;
- hvac-zone-names = "operator";
- spin-controller = < _ctrl_2 5 _ctrl_2
7 >;
- spin-controller-names = "track_1", "track_2";
- queues = < 2 >;
-
- track@30 {
- reg = < 0x0030 0x10 >;
- };
-
- track@40 {
- reg = < 0x0040 0x10 >;
- };
-
- };
- };
-
- lights_1: lights@3 {
- compatible = "ot,work-lights";
- reg = < 0x0003 0x1000 >;
- status = "disabled";
- };
-
- lights_2: lights@4 {
- compatible = "ot,show-lights";
- reg = < 0x0004 0x1000 >;
- status = "disabled";
- rate = < 13 138 >;
-
[PATCH V11 2/5] kbuild: Add generic rule to apply fdtoverlay
From: Rob Herring Add a generic rule to apply fdtoverlay in Makefile.lib, so every platform doesn't need to carry the complex rule. This also automatically adds "DTC_FLAGS_foo_base += -@" for all base files. The platform's Makefile only needs to have this now: foo-dtbs := foo_base.dtb foo_overlay1.dtbo foo_overlay2.dtbo dtb-y := foo.dtb We don't want to run schema checks on foo.dtb (as foo.dts doesn't exist) and the Makefile is updated accordingly. Acked-by: Masahiro Yamada Signed-off-by: Rob Herring Co-developed-by: Viresh Kumar Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 26 -- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index a2658242d956..bc045a54a34e 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -75,11 +75,24 @@ always-y += $(userprogs-always-y) $(userprogs-always-m) # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-) +# List all dtbs to be generated by fdtoverlay +overlay-y := $(foreach m,$(dtb-y), $(if $(strip $($(m:.dtb=-dtbs))),$(m),)) + +# Generate symbols for the base files so overlays can be applied to them. +$(foreach m,$(overlay-y), $(eval DTC_FLAGS_$(basename $(firstword $($(m:.dtb=-dtbs += -@)) + +# Add base dtb and overlay dtbo +dtb-y += $(foreach m,$(overlay-y), $($(m:.dtb=-dtbs))) + always-y += $(dtb-y) ifneq ($(CHECK_DTBS),) -always-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) -always-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y)) +# Don't run schema checks for dtbs created by fdtoverlay as they don't +# have corresponding dts files. +dt-yaml-y := $(filter-out $(overlay-y),$(dtb-y)) + +always-y += $(patsubst %.dtb,%.dt.yaml, $(dt-yaml-y)) +always-y += $(patsubst %.dtbo,%.dt.yaml, $(dt-yaml-y)) endif # Add subdir path @@ -337,6 +350,15 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +overlay-y := $(addprefix $(obj)/, $(overlay-y)) + +quiet_cmd_fdtoverlay = DTOVL $@ + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs) + +$(overlay-y): FORCE + $(call if_changed,fdtoverlay) +$(call multi_depend, $(overlay-y), .dtb, -dtbs) + DT_CHECKER ?= dt-validate DT_BINDING_DIR := Documentation/devicetree/bindings # DT_TMP_SCHEMA may be overridden from Documentation/devicetree/bindings/Makefile -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V6 4/4] cpufreq: CPPC: Add support for frequency invariance
The Frequency Invariance Engine (FIE) is providing a frequency scaling
correction factor that helps achieve more accurate load-tracking.
Normally, this scaling factor can be obtained directly with the help of
the cpufreq drivers as they know the exact frequency the hardware is
running at. But that isn't the case for CPPC cpufreq driver.
Another way of obtaining that is using the arch specific counter
support, which is already present in kernel, but that hardware is
optional for platforms.
This patch updates the CPPC driver to register itself with the topology
core to provide its own implementation (cppc_scale_freq_tick()) of
topology_scale_freq_tick() which gets called by the scheduler on every
tick. Note that the arch specific counters have higher priority than
CPPC counters, if available, though the CPPC driver doesn't need to have
any special handling for that.
On an invocation of cppc_scale_freq_tick(), we schedule an irq work
(since we reach here from hard-irq context), which then schedules a
normal work item and cppc_scale_freq_workfn() updates the per_cpu
arch_freq_scale variable based on the counter updates since the last
tick.
To allow platforms to disable this CPPC counter-based frequency
invariance support, this is all done under CONFIG_ACPI_CPPC_CPUFREQ_FIE,
which is enabled by default.
This also exports sched_setattr_nocheck() as the CPPC driver can be
built as a module.
Cc: linux-a...@vger.kernel.org
Reviewed-by: Ionela Voinescu
Tested-by: Ionela Voinescu
Tested-by: Vincent Guittot
Signed-off-by: Viresh Kumar
---
drivers/cpufreq/Kconfig.arm| 10 ++
drivers/cpufreq/cppc_cpufreq.c | 245 +++--
include/linux/arch_topology.h | 1 +
kernel/sched/core.c| 1 +
4 files changed, 245 insertions(+), 12 deletions(-)
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index e65e0a43be64..a5c5f70acfc9 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -19,6 +19,16 @@ config ACPI_CPPC_CPUFREQ
If in doubt, say N.
+config ACPI_CPPC_CPUFREQ_FIE
+ bool "Frequency Invariance support for CPPC cpufreq driver"
+ depends on ACPI_CPPC_CPUFREQ && GENERIC_ARCH_TOPOLOGY
+ default y
+ help
+ This extends frequency invariance support in the CPPC cpufreq driver,
+ by using CPPC delivered and reference performance counters.
+
+ If in doubt, say N.
+
config ARM_ALLWINNER_SUN50I_CPUFREQ_NVMEM
tristate "Allwinner nvmem based SUN50I CPUFreq driver"
depends on ARCH_SUNXI
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 8a482c434ea6..b8e1b8ea628c 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -10,14 +10,18 @@
#define pr_fmt(fmt)"CPPC Cpufreq:" fmt
+#include
#include
#include
#include
#include
#include
#include
+#include
+#include
#include
#include
+#include
#include
@@ -57,6 +61,204 @@ static struct cppc_workaround_oem_info wa_info[] = {
}
};
+#ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
+
+/* Frequency invariance support */
+struct cppc_freq_invariance {
+ int cpu;
+ struct irq_work irq_work;
+ struct kthread_work work;
+ struct cppc_perf_fb_ctrs prev_perf_fb_ctrs;
+ struct cppc_cpudata *cpu_data;
+};
+
+static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
+static struct kthread_worker *kworker_fie;
+static bool fie_disabled;
+
+static struct cpufreq_driver cppc_cpufreq_driver;
+static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu);
+static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
+struct cppc_perf_fb_ctrs fb_ctrs_t0,
+struct cppc_perf_fb_ctrs fb_ctrs_t1);
+
+/**
+ * cppc_scale_freq_workfn - CPPC arch_freq_scale updater for frequency
invariance
+ * @work: The work item.
+ *
+ * The CPPC driver register itself with the topology core to provide its own
+ * implementation (cppc_scale_freq_tick()) of topology_scale_freq_tick() which
+ * gets called by the scheduler on every tick.
+ *
+ * Note that the arch specific counters have higher priority than CPPC
counters,
+ * if available, though the CPPC driver doesn't need to have any special
+ * handling for that.
+ *
+ * On an invocation of cppc_scale_freq_tick(), we schedule an irq work (since
we
+ * reach here from hard-irq context), which then schedules a normal work item
+ * and cppc_scale_freq_workfn() updates the per_cpu arch_freq_scale variable
+ * based on the counter updates since the last tick.
+ */
+static void cppc_scale_freq_workfn(struct kthread_work *work)
+{
+ struct cppc_freq_invariance *cppc_fi;
+ struct cppc_perf_fb_ctrs fb_ctrs = {0};
+ struct cppc_cpudata *cpu_data;
+ unsigned long local_freq_scale;
+ u64 perf;
+
+ cppc_fi = container_of(work, struct cppc_fr
[PATCH V6 0/4] cpufreq: cppc: Add support for frequency invariance
Hello, CPPC cpufreq driver is used for ARM servers and this patch series tries to provide counter-based frequency invariance support for them in the absence for architecture specific counters (like AMUs). This is tested by: - Vincent Guittot on ThunderX2. - Ionela Voinescu on Juno R2. - /me with hacks on Hikey, as I don't have access to the right hardware. This is based of 5.12-rc2. I will merge these via the arm-cpufreq tree directly. Changes since V5: - New patch to rename freq_scale to arch_freq_scale (Will Deacon). - Separate patch to export arch_freq_scale and helpers (Will Deacon). - Some improvements in the last patch like commit log, moving more stuff to policy init, new fie_disabled flag, etc. (Ionela Voinescu). - Added Reviewed/Acked/Tested-by tags. Changes since V4: - Move some code to policy specific initialization for cppc driver. - Initialize kthread specific stuff only once in cppc driver. - Added a kerneldoc comment in cppc driver and improved changelog as well. Changes since V3: - rebuild_sched_domains_energy() stuff moved from arm64 to drivers/base. - Added Reviewed/Tested-by Ionela for the first patch. - Remove unused max_freq field from structure in cppc driver. - s/cppc_f_i/cppc_freq_inv. - Fix an per-cpu access, there was a bug in earlier version. - Create a single kthread which can run on any CPU and takes care of work from all the CPUs. - Do the whole FIE thing under a new CONFIG option for cppc driver. - Few minor improvements. Changes since V2: - Not sending as an RFC anymore. - Several renames, reordering of code in 1/2 based on Ionela's comments. - Several rebase changes for 2/2. - The freq_scale calculations are optimized a bit. - Better overall commenting and commit logs. Changes since V1: - The interface for setting the callbacks is improved, so different parts looking to provide their callbacks don't need to think about each other. - Moved to per-cpu storage for storing the callback related data, AMU counters have higher priority with this. -- Viresh Viresh Kumar (4): arch_topology: Rename freq_scale as arch_freq_scale arch_topology: Allow multiple entities to provide sched_freq_tick() callback arch_topology: Export arch_freq_scale and helpers cpufreq: CPPC: Add support for frequency invariance arch/arm64/include/asm/topology.h | 10 +- arch/arm64/kernel/topology.c | 109 + drivers/base/arch_topology.c | 89 ++- drivers/cpufreq/Kconfig.arm | 10 ++ drivers/cpufreq/cppc_cpufreq.c| 245 -- include/linux/arch_topology.h | 19 ++- kernel/sched/core.c | 1 + 7 files changed, 385 insertions(+), 98 deletions(-) base-commit: a38fd8748464831584a19438cbb3082b5a2dab15 -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V6 3/4] arch_topology: Export arch_freq_scale and helpers
It is possible now for other parts of the kernel to provide their own
implementation of sched_freq_tick() and they can very well be modules
themselves (like CPPC cpufreq driver, which is going to use these in a
later commit).
Export arch_freq_scale and topology_{set|clear}_scale_freq_source().
Reviewed-by: Ionela Voinescu
Tested-by: Ionela Voinescu
Tested-by: Vincent Guittot
Signed-off-by: Viresh Kumar
---
drivers/base/arch_topology.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index ebcd2ea3091f..995e52b9eca4 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -78,6 +78,7 @@ void topology_set_scale_freq_source(struct scale_freq_data
*data,
update_scale_freq_invariant(true);
}
+EXPORT_SYMBOL_GPL(topology_set_scale_freq_source);
void topology_clear_scale_freq_source(enum scale_freq_source source,
const struct cpumask *cpus)
@@ -96,6 +97,7 @@ void topology_clear_scale_freq_source(enum scale_freq_source
source,
update_scale_freq_invariant(false);
}
+EXPORT_SYMBOL_GPL(topology_clear_scale_freq_source);
void topology_scale_freq_tick(void)
{
@@ -106,6 +108,7 @@ void topology_scale_freq_tick(void)
}
DEFINE_PER_CPU(unsigned long, arch_freq_scale) = SCHED_CAPACITY_SCALE;
+EXPORT_SYMBOL_GPL(arch_freq_scale);
void topology_set_freq_scale(const struct cpumask *cpus, unsigned long
cur_freq,
unsigned long max_freq)
--
2.25.0.rc1.19.g042ed3e048af
[PATCH V6 2/4] arch_topology: Allow multiple entities to provide sched_freq_tick() callback
This patch attempts to make it generic enough so other parts of the
kernel can also provide their own implementation of scale_freq_tick()
callback, which is called by the scheduler periodically to update the
per-cpu arch_freq_scale variable.
The implementations now need to provide 'struct scale_freq_data' for the
CPUs for which they have hardware counters available, and a callback
gets registered for each possible CPU in a per-cpu variable.
The arch specific (or ARM AMU) counters are updated to adapt to this and
they take the highest priority if they are available, i.e. they will be
used instead of CPPC based counters for example.
The special code to rebuild the sched domains, in case invariance status
change for the system, is moved out of arm64 specific code and is added
to arch_topology.c.
Note that this also defines SCALE_FREQ_SOURCE_CPUFREQ but doesn't use it
and it is added to show that cpufreq is also acts as source of
information for FIE and will be used by default if no other counters are
supported for a platform.
Reviewed-by: Ionela Voinescu
Tested-by: Ionela Voinescu
Acked-by: Will Deacon # for arm64
Tested-by: Vincent Guittot
Signed-off-by: Viresh Kumar
---
arch/arm64/include/asm/topology.h | 10 +--
arch/arm64/kernel/topology.c | 105 +++---
drivers/base/arch_topology.c | 82 +--
include/linux/arch_topology.h | 14 +++-
4 files changed, 131 insertions(+), 80 deletions(-)
diff --git a/arch/arm64/include/asm/topology.h
b/arch/arm64/include/asm/topology.h
index 3b8dca4eb08d..ec2db3419c41 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -17,17 +17,9 @@ int pcibus_to_node(struct pci_bus *bus);
#include
void update_freq_counters_refs(void);
-void topology_scale_freq_tick(void);
-
-#ifdef CONFIG_ARM64_AMU_EXTN
-/*
- * Replace task scheduler's default counter-based
- * frequency-invariance scale factor setting.
- */
-#define arch_scale_freq_tick topology_scale_freq_tick
-#endif /* CONFIG_ARM64_AMU_EXTN */
/* Replace task scheduler's default frequency-invariant accounting */
+#define arch_scale_freq_tick topology_scale_freq_tick
#define arch_set_freq_scale topology_set_freq_scale
#define arch_scale_freq_capacity topology_get_freq_scale
#define arch_scale_freq_invariant topology_scale_freq_invariant
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index aa8d893619ed..4dd14a6620c1 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -199,12 +199,47 @@ static int freq_inv_set_max_ratio(int cpu, u64 max_rate,
u64 ref_rate)
return 0;
}
-static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
-#define amu_freq_invariant() static_branch_unlikely(_fie_key)
+static void amu_scale_freq_tick(void)
+{
+ u64 prev_core_cnt, prev_const_cnt;
+ u64 core_cnt, const_cnt, scale;
+
+ prev_const_cnt = this_cpu_read(arch_const_cycles_prev);
+ prev_core_cnt = this_cpu_read(arch_core_cycles_prev);
+
+ update_freq_counters_refs();
+
+ const_cnt = this_cpu_read(arch_const_cycles_prev);
+ core_cnt = this_cpu_read(arch_core_cycles_prev);
+
+ if (unlikely(core_cnt <= prev_core_cnt ||
+const_cnt <= prev_const_cnt))
+ return;
+
+ /*
+* /\corearch_max_freq_scale
+* scale = --- *
+* /\const SCHED_CAPACITY_SCALE
+*
+* See validate_cpu_freq_invariance_counters() for details on
+* arch_max_freq_scale and the use of SCHED_CAPACITY_SHIFT.
+*/
+ scale = core_cnt - prev_core_cnt;
+ scale *= this_cpu_read(arch_max_freq_scale);
+ scale = div64_u64(scale >> SCHED_CAPACITY_SHIFT,
+ const_cnt - prev_const_cnt);
+
+ scale = min_t(unsigned long, scale, SCHED_CAPACITY_SCALE);
+ this_cpu_write(arch_freq_scale, (unsigned long)scale);
+}
+
+static struct scale_freq_data amu_sfd = {
+ .source = SCALE_FREQ_SOURCE_ARCH,
+ .set_freq_scale = amu_scale_freq_tick,
+};
static void amu_fie_setup(const struct cpumask *cpus)
{
- bool invariant;
int cpu;
/* We are already set since the last insmod of cpufreq driver */
@@ -221,25 +256,10 @@ static void amu_fie_setup(const struct cpumask *cpus)
cpumask_or(amu_fie_cpus, amu_fie_cpus, cpus);
- invariant = topology_scale_freq_invariant();
-
- /* We aren't fully invariant yet */
- if (!invariant && !cpumask_equal(amu_fie_cpus, cpu_present_mask))
- return;
-
- static_branch_enable(_fie_key);
+ topology_set_scale_freq_source(_sfd, amu_fie_cpus);
pr_debug("CPUs[%*pbl]: counters will be used for FIE.",
cpumask_pr_args(cpus));
-
- /*
-* Task scheduler behavior depends on frequency invariance support,
-* either cpufreq or counter
[PATCH V6 1/4] arch_topology: Rename freq_scale as arch_freq_scale
Rename freq_scale to a less generic name, as it will get exported soon
for modules. Since x86 already names its own implementation of this as
arch_freq_scale, lets stick to that.
Suggested-by: Will Deacon
Signed-off-by: Viresh Kumar
---
arch/arm64/kernel/topology.c | 6 +++---
drivers/base/arch_topology.c | 4 ++--
include/linux/arch_topology.h | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index e08a4126453a..aa8d893619ed 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -256,8 +256,8 @@ static int init_amu_fie_callback(struct notifier_block *nb,
unsigned long val,
* initialized AMU support and enabled invariance. The AMU counters will
* keep on working just fine in the absence of the cpufreq driver, and
* for the CPUs for which there are no counters available, the last set
-* value of freq_scale will remain valid as that is the frequency those
-* CPUs are running at.
+* value of arch_freq_scale will remain valid as that is the frequency
+* those CPUs are running at.
*/
return 0;
@@ -327,7 +327,7 @@ void topology_scale_freq_tick(void)
const_cnt - prev_const_cnt);
scale = min_t(unsigned long, scale, SCHED_CAPACITY_SCALE);
- this_cpu_write(freq_scale, (unsigned long)scale);
+ this_cpu_write(arch_freq_scale, (unsigned long)scale);
}
#ifdef CONFIG_ACPI_CPPC_LIB
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index de8587cc119e..2a1cecbde0a4 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -31,7 +31,7 @@ __weak bool arch_freq_counters_available(const struct cpumask
*cpus)
{
return false;
}
-DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
+DEFINE_PER_CPU(unsigned long, arch_freq_scale) = SCHED_CAPACITY_SCALE;
void topology_set_freq_scale(const struct cpumask *cpus, unsigned long
cur_freq,
unsigned long max_freq)
@@ -53,7 +53,7 @@ void topology_set_freq_scale(const struct cpumask *cpus,
unsigned long cur_freq,
scale = (cur_freq << SCHED_CAPACITY_SHIFT) / max_freq;
for_each_cpu(i, cpus)
- per_cpu(freq_scale, i) = scale;
+ per_cpu(arch_freq_scale, i) = scale;
}
DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 0f6cd6b73a61..583af517f123 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -23,11 +23,11 @@ static inline unsigned long topology_get_cpu_scale(int cpu)
void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity);
-DECLARE_PER_CPU(unsigned long, freq_scale);
+DECLARE_PER_CPU(unsigned long, arch_freq_scale);
static inline unsigned long topology_get_freq_scale(int cpu)
{
- return per_cpu(freq_scale, cpu);
+ return per_cpu(arch_freq_scale, cpu);
}
void topology_set_freq_scale(const struct cpumask *cpus, unsigned long
cur_freq,
--
2.25.0.rc1.19.g042ed3e048af
Re: [PATCH V5 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
On 09-03-21, 15:11, Ionela Voinescu wrote:
> On Monday 01 Mar 2021 at 12:21:17 (+0530), Viresh Kumar wrote:
> > diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
> > index 0f6cd6b73a61..3bcfba5c21a7 100644
> > --- a/include/linux/arch_topology.h
> > +++ b/include/linux/arch_topology.h
> > @@ -34,7 +34,19 @@ void topology_set_freq_scale(const struct cpumask *cpus,
> > unsigned long cur_freq,
> > unsigned long max_freq);
> > bool topology_scale_freq_invariant(void);
We don't normally add blank lines between declarations (variables or
function), like what's done here.
> >
> > -bool arch_freq_counters_available(const struct cpumask *cpus);
> > +enum scale_freq_source {
> > + SCALE_FREQ_SOURCE_CPUFREQ = 0,
> > + SCALE_FREQ_SOURCE_ARCH,
> > +};
> > +
> > +struct scale_freq_data {
> > + enum scale_freq_source source;
> > + void (*set_freq_scale)(void);
> > +};
> > +
> > +void topology_scale_freq_tick(void);
> > +void topology_set_scale_freq_source(struct scale_freq_data *data, const
> > struct cpumask *cpus);
> > +void topology_clear_scale_freq_source(enum scale_freq_source source, const
> > struct cpumask *cpus);
>
> Nit: can you split these lines?
And so in order to be consistent across the file, I haven't added one
here.
--
viresh
Re: [PATCH] opp: Invalidate current opp when draining the opp list
On 08-03-21, 18:14, Beata Michalska wrote:
> > -bool _opp_remove_all_static(struct opp_table *opp_table)
> > +/*
> > + * Can't remove the OPP from under the lock, debugfs removal needs to
> > happen
> > + * lock less to avoid circular dependency issues. This must be called
> > without
> > + * the opp_table->lock held.
> > + */
> > +static int _opp_drain_list(struct opp_table *opp_table, bool dynamic)
> > {
> > - struct dev_pm_opp *opp;
> > + struct dev_pm_opp *opp, *current_opp = NULL;
> > + int count = 0;
> > +
> > + while ((opp = _opp_get_next(opp_table, dynamic))) {
> > + if (opp_table->current_opp == opp) {
> > + /*
> > +* Reached at current OPP twice, no other OPPs left. The
> > +* last reference to current_opp is dropped from
> > +* _opp_table_kref_release().
> > +*/
> > + if (current_opp)
> > + break;
> > +
> > + current_opp = opp;
> > + }
> Having a quick look at the code ...
> Shouldn't the current_opp be moved at the end of the list ?
> Otherwise there is a risk of leaving unreferenced opps (and opp_table).
How exactly ? Note that it is expected that the OPP table isn't being
used by anyone anymore at this point and all the users went away.
> Might be also worth adding warning (?)
>
> WARN_ONCE(!list_is_singular())
It is allowed for the list to contain both static and dynamic OPPs,
and so the list may have more OPPs here.
--
viresh
Re: [PATCH 06/10] staging: greybus: spilib: use 'spi_delay_to_ns' for getting xfer delay
On 08-03-21, 16:54, Alexandru Ardelean wrote:
> The intent is the removal of the 'delay_usecs' field from the
> spi_transfer struct, as there is a 'delay' field that does the same
> thing.
>
> The spi_delay_to_ns() can be used to get the transfer delay. It works by
> using the 'delay_usecs' field first (if it is non-zero), and finally
> uses the 'delay' field.
>
> Since the 'delay_usecs' field is going away, this change makes use of the
> spi_delay_to_ns() function. This also means dividing the return value of
> the function by 1000, to convert it to microseconds.
> To prevent any potential faults when converting to microseconds and since
> the result of spi_delay_to_ns() is int, the delay is being computed in 32
> bits and then clamped between 0 & U16_MAX.
>
> Signed-off-by: Alexandru Ardelean
> ---
> drivers/staging/greybus/spilib.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/spilib.c
> b/drivers/staging/greybus/spilib.c
> index 672d540d3365..30655153df6a 100644
> --- a/drivers/staging/greybus/spilib.c
> +++ b/drivers/staging/greybus/spilib.c
> @@ -245,6 +245,7 @@ static struct gb_operation
> *gb_spi_operation_create(struct gb_spilib *spi,
> /* Fill in the transfers array */
> xfer = spi->first_xfer;
> while (msg->state != GB_SPI_STATE_OP_DONE) {
> + int xfer_delay;
> if (xfer == spi->last_xfer)
> xfer_len = spi->last_xfer_size;
> else
> @@ -259,7 +260,9 @@ static struct gb_operation
> *gb_spi_operation_create(struct gb_spilib *spi,
>
> gb_xfer->speed_hz = cpu_to_le32(xfer->speed_hz);
> gb_xfer->len = cpu_to_le32(xfer_len);
> - gb_xfer->delay_usecs = cpu_to_le16(xfer->delay_usecs);
> + xfer_delay = spi_delay_to_ns(>delay, xfer) / 1000;
> + xfer_delay = clamp_t(u16, xfer_delay, 0, U16_MAX);
> + gb_xfer->delay_usecs = cpu_to_le16(xfer_delay);
> gb_xfer->cs_change = xfer->cs_change;
> gb_xfer->bits_per_word = xfer->bits_per_word;
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH V5 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
On 08-03-21, 14:52, Will Deacon wrote: > On Mon, Mar 01, 2021 at 12:21:17PM +0530, Viresh Kumar wrote: > > +EXPORT_SYMBOL_GPL(topology_set_scale_freq_source); > > I don't get why you need to export this in this patch. The arm64 topology > code is never built as a module. > > > +EXPORT_SYMBOL_GPL(topology_clear_scale_freq_source); > > Same here. > > > +EXPORT_SYMBOL_GPL(freq_scale); > > And here. After this patch, any part of the kernel can use these helpers/variables to run their own implementation of tick-freq-scale and so this patch looked to be the right place for that to me. And the second patch in the series updates the CPPC cpufreq driver (tristate) to use these exported symbols, so we have the first user who needs the exported symbols as well. > This one probably wants a less generic name as well if it's going > to be exported. x86 names it arch_freq_scale, perhaps we should stick to that instead. -- viresh
Re: [PATCH] opp: Invalidate current opp when draining the opp list
On 05-03-21, 13:55, Beata Michalska wrote:
> Actually, that one might be problematic: by the time the
> _opp_table_kref_release is being reached, the opp pointed to
> by current_opp may no longer be valid.
> _opp_remove_all_static and/or dev_pm_opp_remove_all_dynamic
> will release all the opps by going through opp_table->opp_list.
> It will drop the reference for each opp on the list, until
> the list gets empty(for given opp type), which means,
> all the opps will actually get released
> (only upon _opp_kref_release the opp will get removed
> from the list).
Sorry for missing the context completely, I get it now.
This is what I have applied instead, please see if it breaks anything
or works as expected.
-8<-
From: Beata Michalska
Date: Thu, 4 Mar 2021 15:07:34 +
Subject: [PATCH] opp: Invalidate current opp when draining the opp list
The current_opp when set, grabs additional reference on the opp,
which is then supposed to be dropped upon releasing the opp table.
Still both dev_pm_opp_remove_table and dev_pm_opp_remove_all_dynamic
will completely drain the OPPs list, including dropping the additional
reference on current_opp because they run until the time list gets
empty.
This will lead releasing the current_opp one more time when the OPP
table gets removed and so will raise ref counting issues.
Fix that by making sure we don't release the extra reference to the
current_opp.
Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
Signed-off-by: Beata Michalska
[ Viresh: Rewrite _opp_drain_list() to not drop the extra count instead
of depending on reference counting. Update commit log and
other minor changes. ]
Signed-off-by: Viresh Kumar
---
drivers/opp/core.c | 52 +-
1 file changed, 33 insertions(+), 19 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c2689386a906..3cc0a1b82adc 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1502,10 +1502,38 @@ static struct dev_pm_opp *_opp_get_next(struct
opp_table *opp_table,
return opp;
}
-bool _opp_remove_all_static(struct opp_table *opp_table)
+/*
+ * Can't remove the OPP from under the lock, debugfs removal needs to happen
+ * lock less to avoid circular dependency issues. This must be called without
+ * the opp_table->lock held.
+ */
+static int _opp_drain_list(struct opp_table *opp_table, bool dynamic)
{
- struct dev_pm_opp *opp;
+ struct dev_pm_opp *opp, *current_opp = NULL;
+ int count = 0;
+
+ while ((opp = _opp_get_next(opp_table, dynamic))) {
+ if (opp_table->current_opp == opp) {
+ /*
+* Reached at current OPP twice, no other OPPs left. The
+* last reference to current_opp is dropped from
+* _opp_table_kref_release().
+*/
+ if (current_opp)
+ break;
+
+ current_opp = opp;
+ }
+
+ dev_pm_opp_put(opp);
+ count++;
+ }
+
+ return count;
+}
+bool _opp_remove_all_static(struct opp_table *opp_table)
+{
mutex_lock(_table->lock);
if (!opp_table->parsed_static_opps) {
@@ -1520,13 +1548,7 @@ bool _opp_remove_all_static(struct opp_table *opp_table)
mutex_unlock(_table->lock);
- /*
-* Can't remove the OPP from under the lock, debugfs removal needs to
-* happen lock less to avoid circular dependency issues.
-*/
- while ((opp = _opp_get_next(opp_table, false)))
- dev_pm_opp_put(opp);
-
+ _opp_drain_list(opp_table, false);
return true;
}
@@ -1539,21 +1561,13 @@ bool _opp_remove_all_static(struct opp_table *opp_table)
void dev_pm_opp_remove_all_dynamic(struct device *dev)
{
struct opp_table *opp_table;
- struct dev_pm_opp *opp;
- int count = 0;
+ int count;
opp_table = _find_opp_table(dev);
if (IS_ERR(opp_table))
return;
- /*
-* Can't remove the OPP from under the lock, debugfs removal needs to
-* happen lock less to avoid circular dependency issues.
-*/
- while ((opp = _opp_get_next(opp_table, true))) {
- dev_pm_opp_put(opp);
- count++;
- }
+ count = _opp_drain_list(opp_table, true);
/* Drop the references taken by dev_pm_opp_add() */
while (count--)
[PATCH V10 5/5] of: unittest: Statically apply overlays using fdtoverlay
Now that fdtoverlay is part of the kernel build, start using it to test the unitest overlays we have by applying them statically. Create two new base files static_base_1.dts and static_base_2.dts which includes other .dtsi files. Some unittest overlays deliberately contain errors that unittest checks for. These overlays will cause fdtoverlay to fail, and are thus not included for static builds. Signed-off-by: Viresh Kumar --- drivers/of/unittest-data/Makefile | 48 ++ drivers/of/unittest-data/static_base_1.dts | 4 ++ drivers/of/unittest-data/static_base_2.dts | 4 ++ 3 files changed, 56 insertions(+) create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index 009f4045c8e4..a5d2d9254b2c 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -38,3 +38,51 @@ DTC_FLAGS_testcases += -@ # suppress warnings about intentional errors DTC_FLAGS_testcases += -Wno-interrupts_property + +# Apply overlays statically with fdtoverlay. This is a build time test that +# the overlays can be applied successfully by fdtoverlay. This does not +# guarantee that the overlays can be applied successfully at run time by +# unittest, but it provides a bit of build time test coverage for those +# who do not execute unittest. +# +# The overlays are applied on top of static_base_1.dtb and static_base_2.dtb to +# create static_test_1.dtb and static_test_2.dtb. If fdtoverlay detects an +# error than the kernel build will fail. static_test_1.dtb and +# static_test_2.dtb are not consumed by unittest. +# +# Some unittest overlays deliberately contain errors that unittest checks for. +# These overlays will cause fdtoverlay to fail, and are thus not included +# in the static test: +#overlay_bad_add_dup_node.dtbo \ +#overlay_bad_add_dup_prop.dtbo \ +#overlay_bad_phandle.dtbo \ +#overlay_bad_symbol.dtbo \ + +apply_static_overlay_1 := overlay_0.dtbo \ + overlay_1.dtbo \ + overlay_2.dtbo \ + overlay_3.dtbo \ + overlay_4.dtbo \ + overlay_5.dtbo \ + overlay_6.dtbo \ + overlay_7.dtbo \ + overlay_8.dtbo \ + overlay_9.dtbo \ + overlay_10.dtbo \ + overlay_11.dtbo \ + overlay_12.dtbo \ + overlay_13.dtbo \ + overlay_15.dtbo \ + overlay_gpio_01.dtbo \ + overlay_gpio_02a.dtbo \ + overlay_gpio_02b.dtbo \ + overlay_gpio_03.dtbo \ + overlay_gpio_04a.dtbo \ + overlay_gpio_04b.dtbo + +apply_static_overlay_2 := overlay.dtbo + +static_test_1-dtbs := static_base_1.dtb $(apply_static_overlay_1) +static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2) + +dtb-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb diff --git a/drivers/of/unittest-data/static_base_1.dts b/drivers/of/unittest-data/static_base_1.dts new file mode 100644 index ..10556cb3f01f --- /dev/null +++ b/drivers/of/unittest-data/static_base_1.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "testcases_common.dtsi" diff --git a/drivers/of/unittest-data/static_base_2.dts b/drivers/of/unittest-data/static_base_2.dts new file mode 100644 index ..b0ea9504d6f3 --- /dev/null +++ b/drivers/of/unittest-data/static_base_2.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "overlay_common.dtsi" -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V10 4/5] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
In order to build-test the same unit-test files using fdtoverlay tool,
move the device nodes from the existing overlay_base.dts and
testcases_common.dts files to .dtsi counterparts. The .dts files now
include the new .dtsi files, resulting in exactly the same behavior as
earlier.
The .dtsi files can now be reused for compile time tests using
fdtoverlay (will be done by a later commit).
This is required because the base files passed to fdtoverlay tool
shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
tag).
Note that this commit also moves "testcase-device2" node to
testcases.dts from tests-interrupts.dtsi, as this node has a deliberate
error in it and is only relevant for runtime testing done with
unittest.c.
Signed-off-by: Viresh Kumar
---
drivers/of/unittest-data/overlay_base.dts | 90 +-
drivers/of/unittest-data/overlay_common.dtsi | 91 +++
drivers/of/unittest-data/testcases.dts| 23 ++---
.../of/unittest-data/testcases_common.dtsi| 19
.../of/unittest-data/tests-interrupts.dtsi| 11 +--
5 files changed, 128 insertions(+), 106 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
diff --git a/drivers/of/unittest-data/overlay_base.dts
b/drivers/of/unittest-data/overlay_base.dts
index 99ab9d12d00b..ab9014589c5d 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -2,92 +2,4 @@
/dts-v1/;
/plugin/;
-/*
- * Base device tree that overlays will be applied against.
- *
- * Do not add any properties in node "/".
- * Do not add any nodes other than "/testcase-data-2" in node "/".
- * Do not add anything that would result in dtc creating node "/__fixups__".
- * dtc will create nodes "/__symbols__" and "/__local_fixups__".
- */
-
-/ {
- testcase-data-2 {
- #address-cells = <1>;
- #size-cells = <1>;
-
- electric_1: substation@100 {
- compatible = "ot,big-volts-control";
- reg = < 0x0100 0x100 >;
- status = "disabled";
-
- hvac_1: hvac-medium-1 {
- compatible = "ot,hvac-medium";
- heat-range = < 50 75 >;
- cool-range = < 60 80 >;
- };
-
- spin_ctrl_1: motor-1 {
- compatible = "ot,ferris-wheel-motor";
- spin = "clockwise";
- rpm_avail = < 50 >;
- };
-
- spin_ctrl_2: motor-8 {
- compatible = "ot,roller-coaster-motor";
- };
- };
-
- rides_1: fairway-1 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,rides";
- status = "disabled";
- orientation = < 127 >;
-
- ride@100 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,roller-coaster";
- reg = < 0x0100 0x100 >;
- hvac-provider = < _1 >;
- hvac-thermostat = < 29 > ;
- hvac-zones = < 14 >;
- hvac-zone-names = "operator";
- spin-controller = < _ctrl_2 5 _ctrl_2
7 >;
- spin-controller-names = "track_1", "track_2";
- queues = < 2 >;
-
- track@30 {
- reg = < 0x0030 0x10 >;
- };
-
- track@40 {
- reg = < 0x0040 0x10 >;
- };
-
- };
- };
-
- lights_1: lights@3 {
- compatible = "ot,work-lights";
- reg = < 0x0003 0x1000 >;
- status = "disabled";
- };
-
- lights_2: lights@4 {
- compatible = "ot,show-lights";
- reg = < 0x0004 0x1000 >;
- status = "disabled";
- rate = < 13 138 >;
-
