* [PATCH 2/4] pinctrl: samsung: Fix device node refcount leaks in S3C24xx wakeup controller init
From: Krzysztof Kozlowski @ 2019-08-05 16:27 UTC (permalink / raw)
To: Tomasz Figa, Krzysztof Kozlowski, Sylwester Nawrocki,
Linus Walleij, Kukjin Kim, linux-arm-kernel, linux-samsung-soc,
linux-gpio, linux-kernel
Cc: Chanwoo Choi, notify, Marek Szyprowski
In-Reply-To: <20190805162710.7789-1-krzk@kernel.org>
In s3c24xx_eint_init() the for_each_child_of_node() loop is used with a
break to find a matching child node. Although each iteration of
for_each_child_of_node puts the previous node, but early exit from loop
misses it. This leads to leak of device node.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
index 7e824e4d20f4..9bd0a3de101d 100644
--- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
+++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
@@ -490,8 +490,10 @@ static int s3c24xx_eint_init(struct samsung_pinctrl_drv_data *d)
return -ENODEV;
eint_data = devm_kzalloc(dev, sizeof(*eint_data), GFP_KERNEL);
- if (!eint_data)
+ if (!eint_data) {
+ of_node_put(eint_np);
return -ENOMEM;
+ }
eint_data->drvdata = d;
@@ -503,12 +505,14 @@ static int s3c24xx_eint_init(struct samsung_pinctrl_drv_data *d)
irq = irq_of_parse_and_map(eint_np, i);
if (!irq) {
dev_err(dev, "failed to get wakeup EINT IRQ %d\n", i);
+ of_node_put(eint_np);
return -ENXIO;
}
eint_data->parents[i] = irq;
irq_set_chained_handler_and_data(irq, handlers[i], eint_data);
}
+ of_node_put(eint_np);
bank = d->pin_banks;
for (i = 0; i < d->nr_banks; ++i, ++bank) {
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 1/4] pinctrl: samsung: Fix device node refcount leaks in Exynos wakeup controller init
From: Krzysztof Kozlowski @ 2019-08-05 16:27 UTC (permalink / raw)
To: Tomasz Figa, Krzysztof Kozlowski, Sylwester Nawrocki,
Linus Walleij, Kukjin Kim, linux-arm-kernel, linux-samsung-soc,
linux-gpio, linux-kernel
Cc: Chanwoo Choi, notify, Marek Szyprowski
In exynos_eint_wkup_init() the for_each_child_of_node() loop is used
with a break to find a matching child node. Although each iteration of
for_each_child_of_node puts the previous node, but early exit from loop
misses it. This leads to leak of device node.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
drivers/pinctrl/samsung/pinctrl-exynos.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index ebc27b06718c..8d2c33bba13e 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -504,6 +504,7 @@ int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
bank->nr_pins, &exynos_eint_irqd_ops, bank);
if (!bank->irq_domain) {
dev_err(dev, "wkup irq domain add failed\n");
+ of_node_put(wkup_np);
return -ENXIO;
}
@@ -518,8 +519,10 @@ int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
weint_data = devm_kcalloc(dev,
bank->nr_pins, sizeof(*weint_data),
GFP_KERNEL);
- if (!weint_data)
+ if (!weint_data) {
+ of_node_put(wkup_np);
return -ENOMEM;
+ }
for (idx = 0; idx < bank->nr_pins; ++idx) {
irq = irq_of_parse_and_map(bank->of_node, idx);
@@ -536,10 +539,13 @@ int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
}
}
- if (!muxed_banks)
+ if (!muxed_banks) {
+ of_node_put(wkup_np);
return 0;
+ }
irq = irq_of_parse_and_map(wkup_np, 0);
+ of_node_put(wkup_np);
if (!irq) {
dev_err(dev, "irq number for muxed EINTs not found\n");
return 0;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH] pinctrl: samsung: exynos: Add of_node_put() before return
From: Krzysztof Kozlowski @ 2019-08-05 16:21 UTC (permalink / raw)
To: Nishka Dasgupta
Cc: linux-samsung-soc, linus.walleij, tomasz.figa, linux-gpio, kgene,
s.nawrocki, linux-arm-kernel
In-Reply-To: <20190804160200.5139-1-nishkadg.linux@gmail.com>
On Sun, Aug 04, 2019 at 09:32:00PM +0530, Nishka Dasgupta wrote:
> Each iteration of for_each_child_of_node puts the previous node, but in
> the case of a return from the middle of the loop, there is no put, thus
> causing a memory leak. Hence add an of_node_put before the return.
> Issue found with Coccinelle.
>
> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
> ---
> drivers/pinctrl/samsung/pinctrl-exynos.c | 4 +++-
Thanks, applied.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 6/9] KVM: arm64: Provide a PV_TIME device to user space
From: Steven Price @ 2019-08-05 16:10 UTC (permalink / raw)
To: Marc Zyngier
Cc: Radim Krčmář, kvm, Suzuki K Pouloze,
Catalin Marinas, linux-doc, Russell King, linux-kernel,
James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <20190803135113.6cdf500c@why>
On 03/08/2019 13:51, Marc Zyngier wrote:
> On Fri, 2 Aug 2019 15:50:14 +0100
> Steven Price <steven.price@arm.com> wrote:
>
>> Allow user space to inform the KVM host where in the physical memory
>> map the paravirtualized time structures should be located.
>>
>> A device is created which provides the base address of an array of
>> Stolen Time (ST) structures, one for each VCPU. There must be (64 *
>> total number of VCPUs) bytes of memory available at this location.
>>
>> The address is given in terms of the physical address visible to
>> the guest and must be 64 byte aligned. The memory should be marked as
>> reserved to the guest to stop it allocating it for other purposes.
>
> Why? You seem to be allocating the memory from the kernel, so as far as
> the guest is concerned, this isn't generally usable memory.
I obviously didn't word it very well - that's what I meant. The "memory"
that represents the stolen time structure shouldn't be shown to the
guest as normal memory, but "reserved" for the purpose of stolen time.
To be honest it looks like I forgot to rewrite this commit message -
which 64 byte alignment is all that the guest can rely on (because each
vCPU has it's own structure), the actual array of structures needs to be
page aligned to ensure we can safely map it into the guest.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> arch/arm64/include/asm/kvm_mmu.h | 2 +
>> arch/arm64/include/uapi/asm/kvm.h | 6 +
>> arch/arm64/kvm/Makefile | 1 +
>> include/uapi/linux/kvm.h | 2 +
>> virt/kvm/arm/mmu.c | 44 +++++++
>> virt/kvm/arm/pvtime.c | 190 ++++++++++++++++++++++++++++++
>> 6 files changed, 245 insertions(+)
>> create mode 100644 virt/kvm/arm/pvtime.c
>>
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index befe37d4bc0e..88c8a4b2836f 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -157,6 +157,8 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm);
>> void kvm_free_stage2_pgd(struct kvm *kvm);
>> int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>> phys_addr_t pa, unsigned long size, bool writable);
>> +int kvm_phys_addr_memremap(struct kvm *kvm, phys_addr_t guest_ipa,
>> + phys_addr_t pa, unsigned long size, bool writable);
>>
>> int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index 9a507716ae2f..95516a4198ea 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -367,6 +367,12 @@ struct kvm_vcpu_events {
>> #define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS
>> #define KVM_PSCI_RET_DENIED PSCI_RET_DENIED
>>
>> +/* Device Control API: PV_TIME */
>> +#define KVM_DEV_ARM_PV_TIME_PADDR 0
>> +#define KVM_DEV_ARM_PV_TIME_ST 0
>> +#define KVM_DEV_ARM_PV_TIME_STATE_SIZE 1
>> +#define KVM_DEV_ARM_PV_TIME_STATE 2
>> +
>> #endif
>>
>> #endif /* __ARM_KVM_H__ */
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index 73dce4d47d47..5ffbdc39e780 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -14,6 +14,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/e
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arm.o $(KVM)/arm/mmu.o $(KVM)/arm/mmio.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hypercalls.o
>> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/pvtime.o
>>
>> kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index a7c19540ce21..04bffafa0708 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1222,6 +1222,8 @@ enum kvm_device_type {
>> #define KVM_DEV_TYPE_ARM_VGIC_ITS KVM_DEV_TYPE_ARM_VGIC_ITS
>> KVM_DEV_TYPE_XIVE,
>> #define KVM_DEV_TYPE_XIVE KVM_DEV_TYPE_XIVE
>> + KVM_DEV_TYPE_ARM_PV_TIME,
>> +#define KVM_DEV_TYPE_ARM_PV_TIME KVM_DEV_TYPE_ARM_PV_TIME
>> KVM_DEV_TYPE_MAX,
>> };
>>
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index 38b4c910b6c3..be28a4aee451 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -1368,6 +1368,50 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>> return ret;
>> }
>>
>> +/**
>> + * kvm_phys_addr_memremap - map a memory range to guest IPA
>> + *
>> + * @kvm: The KVM pointer
>> + * @guest_ipa: The IPA at which to insert the mapping
>> + * @pa: The physical address of the memory
>> + * @size: The size of the mapping
>> + */
>> +int kvm_phys_addr_memremap(struct kvm *kvm, phys_addr_t guest_ipa,
>> + phys_addr_t pa, unsigned long size, bool writable)
>> +{
>> + phys_addr_t addr, end;
>> + int ret = 0;
>> + unsigned long pfn;
>> + struct kvm_mmu_memory_cache cache = { 0, };
>> +
>> + end = (guest_ipa + size + PAGE_SIZE - 1) & PAGE_MASK;
>> + pfn = __phys_to_pfn(pa);
>> +
>> + for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
>> + pte_t pte = pfn_pte(pfn, PAGE_S2);
>> +
>> + if (writable)
>> + pte = kvm_s2pte_mkwrite(pte);
>> +
>> + ret = mmu_topup_memory_cache(&cache,
>> + kvm_mmu_cache_min_pages(kvm),
>> + KVM_NR_MEM_OBJS);
>> + if (ret)
>> + goto out;
>> + spin_lock(&kvm->mmu_lock);
>> + ret = stage2_set_pte(kvm, &cache, addr, &pte, 0);
>> + spin_unlock(&kvm->mmu_lock);
>> + if (ret)
>> + goto out;
>> +
>> + pfn++;
>> + }
>> +
>> +out:
>> + mmu_free_memory_cache(&cache);
>> + return ret;
>> +}
>
> This is an exact copy of kvm_phys_addr_ioremap(), with only the memory
> attributes changing. Surely we can have a shared implementation that
> takes the memory attribute as a parameter.
Good point - although due to below I'm going to need something which can
deal with non-contiguous memory...
>> +
>> static bool transparent_hugepage_adjust(kvm_pfn_t *pfnp, phys_addr_t *ipap)
>> {
>> kvm_pfn_t pfn = *pfnp;
>> diff --git a/virt/kvm/arm/pvtime.c b/virt/kvm/arm/pvtime.c
>> new file mode 100644
>> index 000000000000..9051bc07eae1
>> --- /dev/null
>> +++ b/virt/kvm/arm/pvtime.c
>> @@ -0,0 +1,190 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2019 Arm Ltd.
>> +
>> +#include <linux/kvm_host.h>
>> +#include <asm/kvm_mmu.h>
>> +
>> +/* We currently only support PV time on ARM64 */
>> +#ifdef CONFIG_ARM64
>
> And we're only compiling it on arm64, so why the #ifdef?
Another good point - will remove.
>> +
>> +#include <asm/pvclock-abi.h>
>> +
>> +static int max_stolen_size(void)
>> +{
>> + int size = KVM_MAX_VCPUS * sizeof(struct pvclock_vcpu_stolen_time_info);
>
> So we're always allocating enough memory for 512 CPUs? That's an
> additional 32kB of contiguous memory...
>
>> +
>> + return ALIGN(size, PAGE_SIZE);
>> +}
>> +
>> +static int kvm_arm_pvtime_create(struct kvm_device *dev, u32 type)
>> +{
>> + struct kvm_arch_pvtime *pvtime = &dev->kvm->arch.pvtime;
>> +
>> + pvtime->st = alloc_pages_exact(max_stolen_size(),
>> + GFP_KERNEL | __GFP_ZERO);
>> + if (!pvtime->st)
>> + return -ENOMEM;
>
> Is there any chance we could use a vmalloc allocation instead? This
> would lift the requirement on having physically contiguous memory.
Yes I think it should be possible to use non-contiguous memory and vmalloc.
>> +
>> + return 0;
>> +}
>> +
>> +static void kvm_arm_pvtime_destroy(struct kvm_device *dev)
>> +{
>> + struct kvm_arch_pvtime *pvtime = &dev->kvm->arch.pvtime;
>> +
>> + pvtime->st_base = GPA_INVALID;
>> + free_pages_exact(pvtime->st, max_stolen_size());
>> + kfree(dev);
>> +}
>> +
>> +static int pvtime_map_pages(struct kvm *kvm, gpa_t guest_paddr,
>> + void *kaddr, int size)
>> +{
>> + return kvm_phys_addr_memremap(kvm, guest_paddr,
>> + virt_to_phys(kaddr),
>> + size, false);
>> +}
>> +
>> +static int pvtime_save_state(struct kvm *kvm, u64 type, void __user *user)
>> +{
>> + void *source;
>> + size_t size;
>> +
>> + switch (type) {
>> + case KVM_DEV_ARM_PV_TIME_ST:
>> + source = kvm->arch.pvtime.st;
>> + size = sizeof(struct pvclock_vcpu_stolen_time_info) *
>> + atomic_read(&kvm->online_vcpus);
>> + break;
>> + default:
>> + return -ENXIO;
>> + }
>> +
>> + if (copy_to_user(user, source, size))
>> + return -EFAULT;
>> + return 0;
>> +}
>> +
>> +static int pvtime_restore_state(struct kvm *kvm, u64 type, void __user *user)
>> +{
>> + void *dest;
>> + size_t size;
>> +
>> + switch (type) {
>> + case KVM_DEV_ARM_PV_TIME_ST:
>> + dest = kvm->arch.pvtime.st;
>> + size = sizeof(struct pvclock_vcpu_stolen_time_info) *
>> + atomic_read(&kvm->online_vcpus);
>> + break;
>> + default:
>> + return -ENXIO;
>> + }
>> +
>> + if (copy_from_user(dest, user, size))
>> + return -EFAULT;
>> +
>> + return 0;
>> +}
>> +
>> +static int kvm_arm_pvtime_set_attr(struct kvm_device *dev,
>> + struct kvm_device_attr *attr)
>> +{
>> + struct kvm_arch_pvtime *pvtime = &dev->kvm->arch.pvtime;
>> + u64 __user *user = (u64 __user *)attr->addr;
>> + u64 paddr;
>> + int ret;
>> +
>> + switch (attr->group) {
>> + case KVM_DEV_ARM_PV_TIME_PADDR:
>> + if (get_user(paddr, user))
>> + return -EFAULT;
>> + if (paddr & 63)
>> + return -EINVAL;
>
> You should check whether the device fits into the IPA space for this
> guest, and whether it overlaps with anything else.
pvtime_map_pages() should fail in the case of overlap. That seems
sufficient to me - do you think we need something stronger?
>> + switch (attr->attr) {
>> + case KVM_DEV_ARM_PV_TIME_ST:
>> + if (pvtime->st_base != GPA_INVALID)
>> + return -EEXIST;
>> + ret = pvtime_map_pages(dev->kvm, paddr, pvtime->st,
>> + max_stolen_size());
>
> Consider moving the size directly into pvtime_map_pages(), and dropping
> the pvtime->st parameter. All you need is kvm and paddr.
Will do.
Thanks,
Steve
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] interconnect: Add pre_aggregate() callback
From: Evan Green @ 2019-08-05 16:04 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-pm, Sean Sweeney, LKML, David Dai, Doug Anderson,
amit.kucheria, Bjorn Andersson, linux-arm-msm,
linux-arm Mailing List
In-Reply-To: <20190805153332.10047-1-georgi.djakov@linaro.org>
On Mon, Aug 5, 2019 at 8:33 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
>
> Introduce an optional callback in interconnect provider drivers. It can be
> used for implementing actions, that need to be executed before the actual
> aggregation of the bandwidth requests has started.
>
> The benefit of this for now is that it will significantly simplify the code
> in provider drivers.
>
> Suggested-by: Evan Green <evgreen@chromium.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
Thanks Georgi, I like it! We should confirm that it actually does
allow David to remove the sum_avg_cached and max_peak_cached shadow
arrays.
> ---
> drivers/interconnect/core.c | 3 +++
> include/linux/interconnect-provider.h | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 251354bb7fdc..7b971228df38 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -205,6 +205,9 @@ static int aggregate_requests(struct icc_node *node)
> node->avg_bw = 0;
> node->peak_bw = 0;
>
> + if (p->pre_aggregate)
> + p->pre_aggregate(node);
> +
> hlist_for_each_entry(r, &node->req_list, req_node)
> p->aggregate(node, r->tag, r->avg_bw, r->peak_bw,
> &node->avg_bw, &node->peak_bw);
> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> index 4ee19fd41568..fd42bd19302d 100644
> --- a/include/linux/interconnect-provider.h
> +++ b/include/linux/interconnect-provider.h
> @@ -36,6 +36,8 @@ struct icc_node *of_icc_xlate_onecell(struct of_phandle_args *spec,
> * @nodes: internal list of the interconnect provider nodes
> * @set: pointer to device specific set operation function
> * @aggregate: pointer to device specific aggregate operation function
> + * @pre_aggregate: pointer to device specific function that is called
> + * before the aggregation begins (optional)
> * @xlate: provider-specific callback for mapping nodes from phandle arguments
> * @dev: the device this interconnect provider belongs to
> * @users: count of active users
> @@ -47,6 +49,7 @@ struct icc_provider {
> int (*set)(struct icc_node *src, struct icc_node *dst);
> int (*aggregate)(struct icc_node *node, u32 tag, u32 avg_bw,
> u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
> + int (*pre_aggregate)(struct icc_node *node);
> struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data);
> struct device *dev;
> int users;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/8] of/fdt: add function to get the SoC wide DMA addressable memory size
From: Nicolas Saenz Julienne @ 2019-08-05 16:03 UTC (permalink / raw)
To: Rob Herring, Catalin Marinas, Will Deacon
Cc: phill, devicetree,
moderated list:BROADCOM BCM2835 ARM ARCHITECTURE,
Florian Fainelli, Eric Anholt, Marc Zyngier, Frank Rowand,
linux-kernel@vger.kernel.org, linux-mm, Linux IOMMU,
Matthias Brugger, wahrenst, Andrew Morton, Robin Murphy,
Christoph Hellwig,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Marek Szyprowski
In-Reply-To: <CAL_JsqKF5nh3hcdLTG5+6RU3_TnFrNX08vD6qZ8wawoA3WSRpA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3219 bytes --]
Hi Rob,
Thanks for the review!
On Fri, 2019-08-02 at 11:17 -0600, Rob Herring wrote:
> On Wed, Jul 31, 2019 at 9:48 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Some SoCs might have multiple interconnects each with their own DMA
> > addressing limitations. This function parses the 'dma-ranges' on each of
> > them and tries to guess the maximum SoC wide DMA addressable memory
> > size.
> >
> > This is specially useful for arch code in order to properly setup CMA
> > and memory zones.
>
> We already have a way to setup CMA in reserved-memory, so why is this
> needed for that?
Correct me if I'm wrong but I got the feeling you got the point of the patch
later on.
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >
> > drivers/of/fdt.c | 72 ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/of_fdt.h | 2 ++
> > 2 files changed, 74 insertions(+)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 9cdf14b9aaab..f2444c61a136 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -953,6 +953,78 @@ int __init early_init_dt_scan_chosen_stdout(void)
> > }
> > #endif
> >
> > +/**
> > + * early_init_dt_dma_zone_size - Look at all 'dma-ranges' and provide the
> > + * maximum common dmable memory size.
> > + *
> > + * Some devices might have multiple interconnects each with their own DMA
> > + * addressing limitations. For example the Raspberry Pi 4 has the
> > following:
> > + *
> > + * soc {
> > + * dma-ranges = <0xc0000000 0x0 0x00000000 0x3c000000>;
> > + * [...]
> > + * }
> > + *
> > + * v3dbus {
> > + * dma-ranges = <0x00000000 0x0 0x00000000 0x3c000000>;
> > + * [...]
> > + * }
> > + *
> > + * scb {
> > + * dma-ranges = <0x0 0x00000000 0x0 0x00000000 0xfc000000>;
> > + * [...]
> > + * }
> > + *
> > + * Here the area addressable by all devices is [0x00000000-0x3bffffff].
> > Hence
> > + * the function will write in 'data' a size of 0x3c000000.
> > + *
> > + * Note that the implementation assumes all interconnects have the same
> > physical
> > + * memory view and that the mapping always start at the beginning of RAM.
>
> Not really a valid assumption for general code.
Fair enough. On my defence I settled on that assumption after grepping all dts
and being unable to find a board that behaved otherwise.
[...]
> It's possible to have multiple levels of nodes and dma-ranges. You need to
> handle that case too. Doing that and handling differing address translations
> will be complicated.
Understood.
> IMO, I'd just do:
>
> if (of_fdt_machine_is_compatible(blob, "brcm,bcm2711"))
> dma_zone_size = XX;
>
> 2 lines of code is much easier to maintain than 10s of incomplete code
> and is clearer who needs this. Maybe if we have dozens of SoCs with
> this problem we should start parsing dma-ranges.
FYI that's what arm32 is doing at the moment and was my first instinct. But it
seems that arm64 has been able to survive so far without any machine specific
code and I have the feeling Catalin and Will will not be happy about this
solution. Am I wrong?
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: dts: renesas: r8a77990-ebisu: sort nodes
From: Geert Uytterhoeven @ 2019-08-05 15:42 UTC (permalink / raw)
To: Yoshihiro Kaneko; +Cc: Linux-Renesas, Simon Horman, Magnus Damm, Linux ARM
In-Reply-To: <1564669452-22631-1-git-send-email-ykaneko0929@gmail.com>
Hi Kaneko-san,
On Thu, Aug 1, 2019 at 4:24 PM Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> Sort nodes.
>
> If node address is present
> * Sort by node address, grouping all nodes with the same compat string
> and sorting the group alphabetically.
> Else
> * Sort alphabetically
>
> This should not have any run-time effect.
>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Thanks for your patch!
> --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> @@ -157,15 +125,13 @@
> regulator-always-on;
> };
>
> - vbus0_usb2: regulator-vbus0-usb2 {
> + reg_12p0v: regulator2 {
> compatible = "regulator-fixed";
> -
> - regulator-name = "USB20_VBUS_CN";
> - regulator-min-microvolt = <5000000>;
> - regulator-max-microvolt = <5000000>;
> -
> - gpio = <&gpio6 4 GPIO_ACTIVE_HIGH>;
> - enable-active-high;
> + regulator-name = "D12.0V";
> + regulator-min-microvolt = <12000000>;
> + regulator-max-microvolt = <12000000>;
> + regulator-boot-on;
> + regulator-always-on;
> };
>
> rsnd_ak4613: sound {
You forgot to sort the sound subnodes.
@@ -142,13 +142,13 @@
simple-audio-card,bitclock-master = <&sndcpu>;
simple-audio-card,frame-master = <&sndcpu>;
- sndcpu: simple-audio-card,cpu {
- sound-dai = <&rcar_sound>;
- };
-
sndcodec: simple-audio-card,codec {
sound-dai = <&ak4613>;
};
+
+ sndcpu: simple-audio-card,cpu {
+ sound-dai = <&rcar_sound>;
+ };
};
vbus0_usb2: regulator-vbus0-usb2 {
As this kind of patches is a hell to rebase/rework, I'll fix that up myself
while appying.
Apart from that:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v5.4.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: dts: renesas: r8a77980-v3hsk: sort nodes
From: Geert Uytterhoeven @ 2019-08-05 15:42 UTC (permalink / raw)
To: Yoshihiro Kaneko; +Cc: Linux-Renesas, Simon Horman, Magnus Damm, Linux ARM
In-Reply-To: <1564669432-22593-1-git-send-email-ykaneko0929@gmail.com>
On Thu, Aug 1, 2019 at 4:24 PM Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> Sort nodes.
>
> If node address is present
> * Sort by node address, grouping all nodes with the same compat string
> and sorting the group alphabetically.
> Else
> * Sort alphabetically
>
> This should not have any run-time effect.
>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v5.4.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: dts: renesas: r8a77980-condor: sort nodes
From: Geert Uytterhoeven @ 2019-08-05 15:42 UTC (permalink / raw)
To: Yoshihiro Kaneko; +Cc: Linux-Renesas, Simon Horman, Magnus Damm, Linux ARM
In-Reply-To: <1564669414-22553-1-git-send-email-ykaneko0929@gmail.com>
On Thu, Aug 1, 2019 at 4:23 PM Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> Sort nodes.
>
> If node address is present
> * Sort by node address, grouping all nodes with the same compat string
> and sorting the group alphabetically.
> Else
> * Sort alphabetically
>
> This should not have any run-time effect.
>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v5.4.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: dts: renesas: r8a77970-v3msk: sort nodes
From: Geert Uytterhoeven @ 2019-08-05 15:42 UTC (permalink / raw)
To: Yoshihiro Kaneko; +Cc: Linux-Renesas, Simon Horman, Magnus Damm, Linux ARM
In-Reply-To: <1564669397-22515-1-git-send-email-ykaneko0929@gmail.com>
On Thu, Aug 1, 2019 at 4:23 PM Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> Sort nodes.
>
> If node address is present
> * Sort by node address, grouping all nodes with the same compat string
> and sorting the group alphabetically.
> Else
> * Sort alphabetically
>
> This should not have any run-time effect.
>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v5.4.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: dts: renesas: r8a77970-eagle: sort nodes
From: Geert Uytterhoeven @ 2019-08-05 15:41 UTC (permalink / raw)
To: Yoshihiro Kaneko; +Cc: Linux-Renesas, Simon Horman, Magnus Damm, Linux ARM
In-Reply-To: <1564669313-22427-1-git-send-email-ykaneko0929@gmail.com>
On Thu, Aug 1, 2019 at 4:22 PM Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> Sort nodes.
>
> If node address is present
> * Sort by node address, grouping all nodes with the same compat string
> and sorting the group alphabetically.
> Else
> * Sort alphabetically
>
> This should not have any run-time effect.
>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v5.4.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: dts: renesas: r8a77995: sort nodes
From: Geert Uytterhoeven @ 2019-08-05 15:41 UTC (permalink / raw)
To: Yoshihiro Kaneko; +Cc: Linux-Renesas, Simon Horman, Magnus Damm, Linux ARM
In-Reply-To: <1564669513-22756-1-git-send-email-ykaneko0929@gmail.com>
On Thu, Aug 1, 2019 at 4:25 PM Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> If node address is present
> * Sort by node address, grouping all nodes with the same compat string
> and sorting the group alphabetically.
> Else
> * Sort alphabetically
>
> This should not have any run-time effect.
>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v5.4.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: io: Relax implicit barriers in default I/O accessors
From: Dave Martin @ 2019-08-05 15:39 UTC (permalink / raw)
To: Will Deacon; +Cc: Catalin Marinas, Will Deacon, linux-arm-kernel
In-Reply-To: <20190805113503.tdjkuqsjm7j3e5vx@willie-the-truck>
On Mon, Aug 05, 2019 at 12:35:04PM +0100, Will Deacon wrote:
> On Mon, Aug 05, 2019 at 11:39:05AM +0100, Catalin Marinas wrote:
> > On Mon, Jul 29, 2019 at 06:05:18PM +0100, Will Deacon wrote:
> > > As a concrete example, consider the following:
> > >
> > > memcpy(dma_buffer, data, bufsz);
> > > writel(DMA_START, dev->ctrl_reg);
> > >
> > > A DMB ST instruction between the final write to the DMA buffer and the
> > > write to the control register will ensure that the writes to the DMA
> > > buffer are observed before the write to the control register by all
> > > observers. Put another way, if an observer can see the write to the
> > > control register, it can also see the writes to memory.
> >
> > I think one of the counter arguments here were that a device does not
> > "observe" the write to the control register as that's not a master
> > access (by the device). Do you mean that if another CPU (not the device)
> > can observe the writel(), it would have also observed the write to the
> > DMA buffer (assuming the DMB)? Since the device is also an observer of
> > the DMA buffer accesses, the multi-copy atomicity ensures that the
> > device is also seeing the buffer updates following a DMB.
>
> Yes, that's right.
>
> > > This has always
> > > been the case and is not sufficient to provide the ordering required by
> > > Linux, since there is no guarantee that the master interface of the
> > > DMA-capable device has observed either of the accesses. However, in an
> > > other-multi-copy atomic world, we can infer two things:
> > >
> > > 1. A write arriving at an endpoint shared between multiple CPUs is
> > > visible to all CPUs
> > >
> > > 2. A write that is visible to all CPUs is also visible to all other
> > > observers in the shareability domain
> > >
> > > Pieced together, this allows us to use DMB OSHST for our default I/O
> > > write accessors and DMB OSHLD for our default I/O read accessors (the
> > > outer-shareability is for handling non-cacheable mappings) for shared
> > > devices. Memory-mapped, DMA-capable peripherals that are private to a
> > > CPU (i.e. inaccessible to other CPUs) still require the DSB, however
> > > these are few and far between and typically require special treatment
> > > anyway which is outside of the scope of the portable driver API (e.g.
> > > GIC, page-table walker, SPE profiler).
[...]
I think there may be something missing from the argument:
One supposes some kind of causal dependency between the writel() and
any dependent read done by "the device". This is entirely up to the
device implementation, but sanity seems to require that this depencency
is at least as strong as an address dependency, so that the device
mustn't speculatively read dma_buffer in advance of receiving the
DMA_START command etc. If not, you probably have a badly-designed or
broken device and you deserve to have to carry ugly workarounds in
your drivers.
The multi-copy-atomicity requirement effectively rules out some
bus topologies: if a device masters directly onto a bus that doesn't
share a common ancestor and shareability domain with the bus its slave
interface is connected to, there would be no single place to resolve
the DMB (unless some explicit logic were added to handle that somehow).
It's reasonable to assume that the hardware is somewhat sane for the
default I/O accessors: for weird hardware, drivers would have to work
around it explicitly with extra synchronisation but we wouldn't expect
this to be common.
The per-CPU device case is one where there may be an explicitly weird
topology, but this only applies to a few specific devices and we can
work around those as appropriate.
Does that makes sense? This might be "obvious", so I'm not sure we
need to write anything. Just checking my understanding.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2] arm64/mm: fix variable 'tag' set but not used
From: Andrey Konovalov @ 2019-08-05 15:37 UTC (permalink / raw)
To: Qian Cai
Cc: Catalin Marinas, LKML, kasan-dev, Linux Memory Management List,
Alexander Potapenko, Dmitry Vyukov, Andrey Ryabinin, Will Deacon,
Linux ARM
In-Reply-To: <1564670825-4050-1-git-send-email-cai@lca.pw>
On Thu, Aug 1, 2019 at 4:47 PM Qian Cai <cai@lca.pw> wrote:
>
> When CONFIG_KASAN_SW_TAGS=n, set_tag() is compiled away. GCC throws a
> warning,
>
> mm/kasan/common.c: In function '__kasan_kmalloc':
> mm/kasan/common.c:464:5: warning: variable 'tag' set but not used
> [-Wunused-but-set-variable]
> u8 tag = 0xff;
> ^~~
>
> Fix it by making __tag_set() a static inline function the same as
> arch_kasan_set_tag() in mm/kasan/kasan.h for consistency because there
> is a macro in arch/arm64/include/asm/kasan.h,
>
> #define arch_kasan_set_tag(addr, tag) __tag_set(addr, tag)
>
> However, when CONFIG_DEBUG_VIRTUAL=n and CONFIG_SPARSEMEM_VMEMMAP=y,
> page_to_virt() will call __tag_set() with incorrect type of a
> parameter, so fix that as well. Also, still let page_to_virt() return
> "void *" instead of "const void *", so will not need to add a similar
> cast in lowmem_page_address().
>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>
> v2: Fix compilation warnings of CONFIG_DEBUG_VIRTUAL=n spotted by Will.
>
> arch/arm64/include/asm/memory.h | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index b7ba75809751..fb04f10a78ab 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -210,7 +210,11 @@ static inline unsigned long kaslr_offset(void)
> #define __tag_reset(addr) untagged_addr(addr)
> #define __tag_get(addr) (__u8)((u64)(addr) >> 56)
> #else
> -#define __tag_set(addr, tag) (addr)
> +static inline const void *__tag_set(const void *addr, u8 tag)
> +{
> + return addr;
> +}
> +
> #define __tag_reset(addr) (addr)
> #define __tag_get(addr) 0
> #endif
> @@ -301,8 +305,8 @@ static inline void *phys_to_virt(phys_addr_t x)
> #define page_to_virt(page) ({ \
> unsigned long __addr = \
> ((__page_to_voff(page)) | PAGE_OFFSET); \
> - unsigned long __addr_tag = \
> - __tag_set(__addr, page_kasan_tag(page)); \
> + const void *__addr_tag = \
> + __tag_set((void *)__addr, page_kasan_tag(page)); \
> ((void *)__addr_tag); \
> })
>
> --
> 1.8.3.1
>
Reviewed-by: Andrey Konovalov <andreyknvl@google.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: dts: renesas: r8a77995-draak: sort nodes
From: Geert Uytterhoeven @ 2019-08-05 15:36 UTC (permalink / raw)
To: Yoshihiro Kaneko; +Cc: Linux-Renesas, Simon Horman, Magnus Damm, Linux ARM
In-Reply-To: <1564669495-22718-1-git-send-email-ykaneko0929@gmail.com>
On Thu, Aug 1, 2019 at 4:25 PM Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> Sort nodes.
>
> If node address is present
> * Sort by node address, grouping all nodes with the same compat string
> and sorting the group alphabetically.
> Else
> * Sort alphabetically
>
> This should not have any run-time effect.
>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v5.4.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: dts: renesas: r8a77990: sort nodes
From: Geert Uytterhoeven @ 2019-08-05 15:35 UTC (permalink / raw)
To: Yoshihiro Kaneko; +Cc: Linux-Renesas, Simon Horman, Magnus Damm, Linux ARM
In-Reply-To: <1564669476-22680-1-git-send-email-ykaneko0929@gmail.com>
On Thu, Aug 1, 2019 at 4:24 PM Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> Sort nodes.
>
> If node address is present
> * Sort by node address, grouping all nodes with the same compat string
> and sorting the group alphabetically.
> Else
> * Sort alphabetically
>
> This should not have any run-time effect.
>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v5.4.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] interconnect: Add pre_aggregate() callback
From: Georgi Djakov @ 2019-08-05 15:33 UTC (permalink / raw)
To: linux-pm, evgreen, daidavid1
Cc: seansw, linux-arm-msm, linux-kernel, amit.kucheria, dianders,
bjorn.andersson, Georgi Djakov, linux-arm-kernel
In-Reply-To: <752aca6f-4f69-301d-81ef-ff29bc25b614@linaro.org>
Introduce an optional callback in interconnect provider drivers. It can be
used for implementing actions, that need to be executed before the actual
aggregation of the bandwidth requests has started.
The benefit of this for now is that it will significantly simplify the code
in provider drivers.
Suggested-by: Evan Green <evgreen@chromium.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
drivers/interconnect/core.c | 3 +++
include/linux/interconnect-provider.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 251354bb7fdc..7b971228df38 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -205,6 +205,9 @@ static int aggregate_requests(struct icc_node *node)
node->avg_bw = 0;
node->peak_bw = 0;
+ if (p->pre_aggregate)
+ p->pre_aggregate(node);
+
hlist_for_each_entry(r, &node->req_list, req_node)
p->aggregate(node, r->tag, r->avg_bw, r->peak_bw,
&node->avg_bw, &node->peak_bw);
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index 4ee19fd41568..fd42bd19302d 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -36,6 +36,8 @@ struct icc_node *of_icc_xlate_onecell(struct of_phandle_args *spec,
* @nodes: internal list of the interconnect provider nodes
* @set: pointer to device specific set operation function
* @aggregate: pointer to device specific aggregate operation function
+ * @pre_aggregate: pointer to device specific function that is called
+ * before the aggregation begins (optional)
* @xlate: provider-specific callback for mapping nodes from phandle arguments
* @dev: the device this interconnect provider belongs to
* @users: count of active users
@@ -47,6 +49,7 @@ struct icc_provider {
int (*set)(struct icc_node *src, struct icc_node *dst);
int (*aggregate)(struct icc_node *node, u32 tag, u32 avg_bw,
u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
+ int (*pre_aggregate)(struct icc_node *node);
struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data);
struct device *dev;
int users;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH 5/6] tty: serial: Add linflexuart driver for S32V234
From: gregkh @ 2019-08-05 15:31 UTC (permalink / raw)
To: Stefan-gabriel Mirea
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, corbet@lwn.net,
catalin.marinas@arm.com, linux-doc@vger.kernel.org,
Larisa Ileana Grigore, linux-kernel@vger.kernel.org, Leo Li,
Cosmin Stefan Stoica, robh+dt@kernel.org,
linux-serial@vger.kernel.org, jslaby@suse.com,
shawnguo@kernel.org, will@kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190802194702.30249-6-stefan-gabriel.mirea@nxp.com>
On Fri, Aug 02, 2019 at 07:47:23PM +0000, Stefan-gabriel Mirea wrote:
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -293,4 +293,7 @@
> /* SiFive UART */
> #define PORT_SIFIVE_V0 120
>
> +/* Freescale Linflex UART */
> +#define PORT_LINFLEXUART 121
Do you really need this modified?
thanks,
greg k-h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/4 v2] drm/panel: simple: Add TI nspire panel bindings
From: Rob Herring @ 2019-08-05 15:28 UTC (permalink / raw)
To: Linus Walleij
Cc: Fabian Vogt, Daniel Tang, dri-devel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
devicetree
In-Reply-To: <20190805085847.25554-3-linus.walleij@linaro.org>
On Mon, Aug 5, 2019 at 2:59 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Add bindings for the TI NSPIRE simple display panels.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - New patch as bindings are required
> - Let's use YAML
> ---
> .../bindings/display/panel/ti,nspire.yaml | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/panel/ti,nspire.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/panel/ti,nspire.yaml b/Documentation/devicetree/bindings/display/panel/ti,nspire.yaml
> new file mode 100644
> index 000000000000..fa81602a922a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/ti,nspire.yaml
> @@ -0,0 +1,36 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR X11)
I think you want MIT rather than X11. However, the preference on new
bindings is (GPL-2.0-only OR BSD-2-Clause).
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/ti,nspire.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments NSPIRE Display Panels
> +
> +maintainers:
> + - Linus Walleij <linus.walleij@linaro.org>
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
You can drop this. Just 'enum' is sufficient.
> + - enum:
> + - ti,nspire-cx-lcd-panel
> + - ti,nspire-classic-lcd-panel
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + panel {
> + compatible = "ti,nspire-cx-lcd-panel";
> + ports {
> + port {
You need to capture that there's a single port in the schema. There's
not really a lot of examples for this yet, but you should add:
allOf:
- $ref: panel-common.yaml#
With a single port, you can drop 'ports' or you can keep it. If you do
the latter, then you need to define 'ports' and then 'port' in your
schema. The common schema is only sufficient if you have a single
'port' node otherwise you need to define what's under 'ports'.
Rob
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] drm/rockchip: Suspend DP late
From: Sean Paul @ 2019-08-05 15:11 UTC (permalink / raw)
To: Douglas Anderson
Cc: Heiko Stuebner, Tomeu Vizoso, David Airlie, Sandy Huang,
dri-devel, linux-kernel, linux-rockchip, mka, seanpaul,
Daniel Vetter, linux-arm-kernel
In-Reply-To: <20190802192629.GX104440@art_vandelay>
On Fri, Aug 02, 2019 at 03:26:29PM -0400, Sean Paul wrote:
> On Fri, Aug 02, 2019 at 11:46:16AM -0700, Douglas Anderson wrote:
> > In commit fe64ba5c6323 ("drm/rockchip: Resume DP early") we moved
> > resume to be early but left suspend at its normal time. This seems
> > like it could be OK, but casues problems if a suspend gets interrupted
> > partway through. The OS only balances matching suspend/resume levels.
> > ...so if suspend was called then resume will be called. If suspend
> > late was called then resume early will be called. ...but if suspend
> > was called resume early might not get called. This leads to an
> > unbalance in the clock enables / disables.
> >
> > Lets take the simple fix and just move suspend to be late to match.
> > This makes the PM core take proper care in keeping things balanced.
> >
> > Fixes: fe64ba5c6323 ("drm/rockchip: Resume DP early")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Reviewed-by: Sean Paul <sean@poorly.run>
>
> This should go in -misc-fixes and due to some... administrative reasons... I
> will leave it on the list until Maarten has a chance to ff to -rc4 on Monday.
> I'll apply it then so as to not require a backmerge.
We're no longer able to ff drm-misc-fixes since a commit came in on Saturday, so
I've piled this on as well.
Thanks,
Sean
>
> Sean
>
> > ---
> >
> > drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > index 7d7cb57410fc..f38f5e113c6b 100644
> > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > @@ -436,7 +436,7 @@ static int rockchip_dp_resume(struct device *dev)
> >
> > static const struct dev_pm_ops rockchip_dp_pm_ops = {
> > #ifdef CONFIG_PM_SLEEP
> > - .suspend = rockchip_dp_suspend,
> > + .suspend_late = rockchip_dp_suspend,
> > .resume_early = rockchip_dp_resume,
> > #endif
> > };
> > --
> > 2.22.0.770.g0f2c4a37fd-goog
> >
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: next/master boot: 256 boots: 6 failed, 177 passed with 72 offline, 1 conflict (next-20190805)
From: Mark Brown @ 2019-08-05 14:24 UTC (permalink / raw)
To: Nick Desaulniers, Nathan Chancellor, Tri Vo
Cc: Kevin Hilman, linux-next, Matt Hart, linux-arm-kernel,
kernel-build-reports
In-Reply-To: <5d482ab9.1c69fb81.684ee.d95e@mx.google.com>
[-- Attachment #1.1: Type: text/plain, Size: 850 bytes --]
On Mon, Aug 05, 2019 at 06:10:17AM -0700, kernelci.org bot wrote:
> arm64:
> defconfig+CONFIG_CPU_BIG_ENDIAN=y:
> clang-8:
> meson-gxl-s805x-p241: 1 failed lab
> meson-gxl-s905x-khadas-vim: 1 failed lab
> meson-gxl-s905x-libretech-cc: 1 failed lab
We're still seeing failures with all clang builds failing to understand
the executable format of /init in big endian arm64 builds; GCC builds of
the same configuration boot successfully. This has been going on for
about as long as we've been trying to boot clang builds AFAICT, it's a
huge proportion of the failures we're seeing in -next.
I did notice one similar failure for v7 GCC today:
https://kernelci.org/boot/id/5d4807f559b514d97f31b28e/
which is also happening for other trees so that one at least looks
suspiciously like infrastructure.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/9] KVM: arm64: Support stolen time reporting via shared structure
From: Steven Price @ 2019-08-05 14:18 UTC (permalink / raw)
To: Marc Zyngier
Cc: kvm, linux-doc, Catalin Marinas, Russell King, linux-kernel,
Paolo Bonzini, Will Deacon, kvmarm, linux-arm-kernel
In-Reply-To: <20190803191303.02e9bcc9@why>
On 03/08/2019 19:13, Marc Zyngier wrote:
> On Sat, 3 Aug 2019 18:58:17 +0100
> Marc Zyngier <maz@kernel.org> wrote:
>
>> On Fri, 2 Aug 2019 15:50:12 +0100
>> Steven Price <steven.price@arm.com> wrote:
>>
>>> Implement the service call for configuring a shared structre between a
>>> VCPU and the hypervisor in which the hypervisor can write the time
>>> stolen from the VCPU's execution time by other tasks on the host.
>>>
>>> The hypervisor allocates memory which is placed at an IPA chosen by user
>>> space. The hypervisor then uses WRITE_ONCE() to update the shared
>>> structre ensuring single copy atomicity of the 64-bit unsigned value
>>> that reports stolen time in nanoseconds.
>>>
>>> Whenever stolen time is enabled by the guest, the stolen time counter is
>>> reset.
>>>
>>> The stolen time itself is retrieved from the sched_info structure
>>> maintained by the Linux scheduler code. We enable SCHEDSTATS when
>>> selecting KVM Kconfig to ensure this value is meaningful.
>>>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>> ---
>>> arch/arm64/include/asm/kvm_host.h | 13 +++++-
>>> arch/arm64/kvm/Kconfig | 1 +
>>> include/kvm/arm_hypercalls.h | 1 +
>>> include/linux/kvm_types.h | 2 +
>>> virt/kvm/arm/arm.c | 18 ++++++++
>>> virt/kvm/arm/hypercalls.c | 70 +++++++++++++++++++++++++++++++
>>> 6 files changed, 104 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index f656169db8c3..78f270190d43 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -44,6 +44,7 @@
>>> KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>>> #define KVM_REQ_IRQ_PENDING KVM_ARCH_REQ(1)
>>> #define KVM_REQ_VCPU_RESET KVM_ARCH_REQ(2)
>>> +#define KVM_REQ_RECORD_STEAL KVM_ARCH_REQ(3)
>>>
>>> DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
>>>
>>> @@ -83,6 +84,11 @@ struct kvm_arch {
>>>
>>> /* Mandated version of PSCI */
>>> u32 psci_version;
>>> +
>>> + struct kvm_arch_pvtime {
>>> + void *st;
>>> + gpa_t st_base;
>>> + } pvtime;
>>> };
>>>
>>> #define KVM_NR_MEM_OBJS 40
>>> @@ -338,8 +344,13 @@ struct kvm_vcpu_arch {
>>> /* True when deferrable sysregs are loaded on the physical CPU,
>>> * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
>>> bool sysregs_loaded_on_cpu;
>>> -};
>>>
>>> + /* Guest PV state */
>>> + struct {
>>> + u64 steal;
>>> + u64 last_steal;
>>> + } steal;
>>> +};
>>> /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>>> #define vcpu_sve_pffr(vcpu) ((void *)((char *)((vcpu)->arch.sve_state) + \
>>> sve_ffr_offset((vcpu)->arch.sve_max_vl)))
>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>>> index a67121d419a2..d8b88e40d223 100644
>>> --- a/arch/arm64/kvm/Kconfig
>>> +++ b/arch/arm64/kvm/Kconfig
>>> @@ -39,6 +39,7 @@ config KVM
>>> select IRQ_BYPASS_MANAGER
>>> select HAVE_KVM_IRQ_BYPASS
>>> select HAVE_KVM_VCPU_RUN_PID_CHANGE
>>> + select SCHEDSTATS
>>> ---help---
>>> Support hosting virtualized guest machines.
>>> We don't support KVM with 16K page tables yet, due to the multiple
>>> diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
>>> index 35a5abcc4ca3..9f0710ab4292 100644
>>> --- a/include/kvm/arm_hypercalls.h
>>> +++ b/include/kvm/arm_hypercalls.h
>>> @@ -7,6 +7,7 @@
>>> #include <asm/kvm_emulate.h>
>>>
>>> int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
>>> +int kvm_update_stolen_time(struct kvm_vcpu *vcpu);
>>>
>>> static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)
>>> {
>>> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
>>> index bde5374ae021..1c88e69db3d9 100644
>>> --- a/include/linux/kvm_types.h
>>> +++ b/include/linux/kvm_types.h
>>> @@ -35,6 +35,8 @@ typedef unsigned long gva_t;
>>> typedef u64 gpa_t;
>>> typedef u64 gfn_t;
>>>
>>> +#define GPA_INVALID (~(gpa_t)0)
>>> +
>>> typedef unsigned long hva_t;
>>> typedef u64 hpa_t;
>>> typedef u64 hfn_t;
>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>> index f645c0fbf7ec..ebd963d2580b 100644
>>> --- a/virt/kvm/arm/arm.c
>>> +++ b/virt/kvm/arm/arm.c
>>> @@ -40,6 +40,10 @@
>>> #include <asm/kvm_coproc.h>
>>> #include <asm/sections.h>
>>>
>>> +#include <kvm/arm_hypercalls.h>
>>> +#include <kvm/arm_pmu.h>
>>> +#include <kvm/arm_psci.h>
>>> +
>>> #ifdef REQUIRES_VIRT
>>> __asm__(".arch_extension virt");
>>> #endif
>>> @@ -135,6 +139,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>> kvm->arch.max_vcpus = vgic_present ?
>>> kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
>>>
>>> + kvm->arch.pvtime.st_base = GPA_INVALID;
>>> return ret;
>>> out_free_stage2_pgd:
>>> kvm_free_stage2_pgd(kvm);
>>> @@ -371,6 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>> kvm_vcpu_load_sysregs(vcpu);
>>> kvm_arch_vcpu_load_fp(vcpu);
>>> kvm_vcpu_pmu_restore_guest(vcpu);
>>> + kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
>>>
>>> if (single_task_running())
>>> vcpu_clear_wfe_traps(vcpu);
>>> @@ -617,6 +623,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
>>> smp_rmb();
>>> }
>>>
>>> +static void vcpu_req_record_steal(struct kvm_vcpu *vcpu)
>>> +{
>>> + int idx;
>>> +
>>> + idx = srcu_read_lock(&vcpu->kvm->srcu);
>>> + kvm_update_stolen_time(vcpu);
>>> + srcu_read_unlock(&vcpu->kvm->srcu, idx);
>>> +}
>>> +
>>> static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>>> {
>>> return vcpu->arch.target >= 0;
>>> @@ -636,6 +651,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>>> * that a VCPU sees new virtual interrupts.
>>> */
>>> kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
>>> +
>>> + if (kvm_check_request(KVM_REQ_RECORD_STEAL, vcpu))
>>> + vcpu_req_record_steal(vcpu);
>>
>> Something troubles me. Here, you've set the request on load. But you
>> can be preempted at any time (preemption gets disabled just after).
>>
>> I have the feeling that should you get preempted right here, you'll
>> end-up having accumulated the wrong amount of steal time, as the
>> request put via load when you'll get scheduled back in will only get
>> processed after a full round of entry/exit/entry, which doesn't look
>> great.
>
> Ah, no. We're saved by the check for pending requests right before we
> jump in the guest, causing an early exit and the whole shebang to be
> restarted.
Yes, that's my understanding. Obviously not ideal if it happens in that
small window, but everything is redone to get the right values in the end.
Steve
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/9] KVM: arm64: Support stolen time reporting via shared structure
From: Steven Price @ 2019-08-05 14:09 UTC (permalink / raw)
To: Marc Zyngier
Cc: Radim Krčmář, kvm, Suzuki K Pouloze,
Catalin Marinas, linux-doc, Russell King, linux-kernel,
James Morse, Julien Thierry, Paolo Bonzini, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <20190803125515.6aa50084@why>
On 03/08/2019 12:55, Marc Zyngier wrote:
> On Fri, 2 Aug 2019 15:50:12 +0100
> Steven Price <steven.price@arm.com> wrote:
>
>> Implement the service call for configuring a shared structre between a
>
> structure
>
>> VCPU and the hypervisor in which the hypervisor can write the time
>> stolen from the VCPU's execution time by other tasks on the host.
>>
>> The hypervisor allocates memory which is placed at an IPA chosen by user
>> space. The hypervisor then uses WRITE_ONCE() to update the shared
>> structre ensuring single copy atomicity of the 64-bit unsigned value
>
> structure
Twice in one commit message... thanks for spotting! :)
>> that reports stolen time in nanoseconds.
>>
>> Whenever stolen time is enabled by the guest, the stolen time counter is
>> reset.
>>
>> The stolen time itself is retrieved from the sched_info structure
>> maintained by the Linux scheduler code. We enable SCHEDSTATS when
>> selecting KVM Kconfig to ensure this value is meaningful.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> arch/arm64/include/asm/kvm_host.h | 13 +++++-
>> arch/arm64/kvm/Kconfig | 1 +
>> include/kvm/arm_hypercalls.h | 1 +
>> include/linux/kvm_types.h | 2 +
>> virt/kvm/arm/arm.c | 18 ++++++++
>> virt/kvm/arm/hypercalls.c | 70 +++++++++++++++++++++++++++++++
>> 6 files changed, 104 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index f656169db8c3..78f270190d43 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -44,6 +44,7 @@
>> KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>> #define KVM_REQ_IRQ_PENDING KVM_ARCH_REQ(1)
>> #define KVM_REQ_VCPU_RESET KVM_ARCH_REQ(2)
>> +#define KVM_REQ_RECORD_STEAL KVM_ARCH_REQ(3)
>>
>> DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
>>
>> @@ -83,6 +84,11 @@ struct kvm_arch {
>>
>> /* Mandated version of PSCI */
>> u32 psci_version;
>> +
>> + struct kvm_arch_pvtime {
>> + void *st;
>
> Is it really a void *? I'm sure you can use a proper type here...
Indeed that sounds like a good idea!
>> + gpa_t st_base;
>> + } pvtime;
>> };
>>
>> #define KVM_NR_MEM_OBJS 40
>> @@ -338,8 +344,13 @@ struct kvm_vcpu_arch {
>> /* True when deferrable sysregs are loaded on the physical CPU,
>> * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
>> bool sysregs_loaded_on_cpu;
>> -};
>>
>> + /* Guest PV state */
>> + struct {
>> + u64 steal;
>> + u64 last_steal;
>> + } steal;
>> +};
>> /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>> #define vcpu_sve_pffr(vcpu) ((void *)((char *)((vcpu)->arch.sve_state) + \
>> sve_ffr_offset((vcpu)->arch.sve_max_vl)))
>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>> index a67121d419a2..d8b88e40d223 100644
>> --- a/arch/arm64/kvm/Kconfig
>> +++ b/arch/arm64/kvm/Kconfig
>> @@ -39,6 +39,7 @@ config KVM
>> select IRQ_BYPASS_MANAGER
>> select HAVE_KVM_IRQ_BYPASS
>> select HAVE_KVM_VCPU_RUN_PID_CHANGE
>> + select SCHEDSTATS
>> ---help---
>> Support hosting virtualized guest machines.
>> We don't support KVM with 16K page tables yet, due to the multiple
>> diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
>> index 35a5abcc4ca3..9f0710ab4292 100644
>> --- a/include/kvm/arm_hypercalls.h
>> +++ b/include/kvm/arm_hypercalls.h
>> @@ -7,6 +7,7 @@
>> #include <asm/kvm_emulate.h>
>>
>> int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
>> +int kvm_update_stolen_time(struct kvm_vcpu *vcpu);
>>
>> static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)
>> {
>> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
>> index bde5374ae021..1c88e69db3d9 100644
>> --- a/include/linux/kvm_types.h
>> +++ b/include/linux/kvm_types.h
>> @@ -35,6 +35,8 @@ typedef unsigned long gva_t;
>> typedef u64 gpa_t;
>> typedef u64 gfn_t;
>>
>> +#define GPA_INVALID (~(gpa_t)0)
>> +
>> typedef unsigned long hva_t;
>> typedef u64 hpa_t;
>> typedef u64 hfn_t;
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index f645c0fbf7ec..ebd963d2580b 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -40,6 +40,10 @@
>> #include <asm/kvm_coproc.h>
>> #include <asm/sections.h>
>>
>> +#include <kvm/arm_hypercalls.h>
>> +#include <kvm/arm_pmu.h>
>> +#include <kvm/arm_psci.h>
>> +
>> #ifdef REQUIRES_VIRT
>> __asm__(".arch_extension virt");
>> #endif
>> @@ -135,6 +139,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>> kvm->arch.max_vcpus = vgic_present ?
>> kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
>>
>> + kvm->arch.pvtime.st_base = GPA_INVALID;
>> return ret;
>> out_free_stage2_pgd:
>> kvm_free_stage2_pgd(kvm);
>> @@ -371,6 +376,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>> kvm_vcpu_load_sysregs(vcpu);
>> kvm_arch_vcpu_load_fp(vcpu);
>> kvm_vcpu_pmu_restore_guest(vcpu);
>> + kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
>>
>> if (single_task_running())
>> vcpu_clear_wfe_traps(vcpu);
>> @@ -617,6 +623,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
>> smp_rmb();
>> }
>>
>> +static void vcpu_req_record_steal(struct kvm_vcpu *vcpu)
>> +{
>> + int idx;
>> +
>> + idx = srcu_read_lock(&vcpu->kvm->srcu);
>> + kvm_update_stolen_time(vcpu);
>> + srcu_read_unlock(&vcpu->kvm->srcu, idx);
>> +}
>> +
>> static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
>> {
>> return vcpu->arch.target >= 0;
>> @@ -636,6 +651,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
>> * that a VCPU sees new virtual interrupts.
>> */
>> kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
>> +
>> + if (kvm_check_request(KVM_REQ_RECORD_STEAL, vcpu))
>> + vcpu_req_record_steal(vcpu);
>> }
>> }
>>
>> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c
>> index 2906b2df99df..196c71c8dd87 100644
>> --- a/virt/kvm/arm/hypercalls.c
>> +++ b/virt/kvm/arm/hypercalls.c
>> @@ -10,6 +10,70 @@
>> #include <kvm/arm_hypercalls.h>
>> #include <kvm/arm_psci.h>
>>
>> +
>> +static struct pvclock_vcpu_stolen_time_info *pvtime_get_st(
>> + struct kvm_vcpu *vcpu)
>
> nit: on a single line.
>
>> +{
>> + struct pvclock_vcpu_stolen_time_info *st = vcpu->kvm->arch.pvtime.st;
>> +
>> + if (!st)
>> + return NULL;
>> +
>> + return &st[kvm_vcpu_get_idx(vcpu)];
>> +}
>> +
>> +int kvm_update_stolen_time(struct kvm_vcpu *vcpu)
>> +{
>> + u64 steal;
>> + struct pvclock_vcpu_stolen_time_info *kaddr;
>> +
>> + if (vcpu->kvm->arch.pvtime.st_base == GPA_INVALID)
>> + return -ENOTSUPP;
>
> So for a guest that doesn't have stolen time support (which is 100% of
> them for the foreseeable future), we still set a request, take the srcu
> lock and end-up here for nothing. I'd rather we test this st_base
> early, as it should never change once the guest has started.
Good point - I'll make the call to kvm_make_request() conditional on
st_base being set.
>> +
>> + kaddr = pvtime_get_st(vcpu);
>> +
>> + if (!kaddr)
>> + return -ENOTSUPP;
>
> How can this happen?
Good question, and it makes the pvtime_get_st() helper rather pointless.
Will remove.
>> +
>> + kaddr->revision = 0;
>> + kaddr->attributes = 0;
>
> Why does this need to be written each time we update the stolen time? I
> have the feeling this would be better moved to the hypercall
> initializing the data structure.
Sure.
>> +
>> + /* Let's do the local bookkeeping */
>> + steal = vcpu->arch.steal.steal;
>> + steal += current->sched_info.run_delay - vcpu->arch.steal.last_steal;
>> + vcpu->arch.steal.last_steal = current->sched_info.run_delay;
>> + vcpu->arch.steal.steal = steal;
>> +
>> + /* Now write out the value to the shared page */
>> + WRITE_ONCE(kaddr->stolen_time, cpu_to_le64(steal));
>
> Is there any requirement for this to be visible to another CPU than the
> one this is being written from?
I don't believe there is a requirement for this to be visible
synchronously - there is an implicit race here if another vCPU is
accessing the structure (because the hypervisor can preempt and update
at any point), so we don't need to push this out with a barrier. However
we must use 64-bit single-copy atomicity writes to ensure that another
vCPU can't see a half-updated value.
Thanks,
Steve
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH V13 12/12] PCI: tegra: Add Tegra194 PCIe support
From: Lorenzo Pieralisi @ 2019-08-05 14:01 UTC (permalink / raw)
To: Vidya Sagar
Cc: mark.rutland, devicetree, mperttunen, mmaddireddy, linux-pci,
catalin.marinas, will.deacon, linux-kernel, kthota, kishon,
linux-tegra, robh+dt, thierry.reding, gustavo.pimentel,
jingoohan1, bhelgaas, digetx, jonathanh, linux-arm-kernel,
sagar.tv
In-Reply-To: <f09c79fc-c724-5290-d630-cac3fdd7a996@nvidia.com>
On Fri, Aug 02, 2019 at 05:36:43PM +0530, Vidya Sagar wrote:
> On 7/30/2019 9:19 PM, Lorenzo Pieralisi wrote:
> > On Tue, Jul 23, 2019 at 08:14:08PM +0530, Vidya Sagar wrote:
> > > On 7/16/2019 4:52 PM, Lorenzo Pieralisi wrote:
> > > > On Sat, Jul 13, 2019 at 12:34:34PM +0530, Vidya Sagar wrote:
> > > >
> > > > [...]
> > > >
> > > > > > > > > +static int tegra_pcie_bpmp_set_ctrl_state(struct tegra_pcie_dw *pcie,
> > > > > > > > > + bool enable)
> > > > > > > > > +{
> > > > > > > > > + struct mrq_uphy_response resp;
> > > > > > > > > + struct tegra_bpmp_message msg;
> > > > > > > > > + struct mrq_uphy_request req;
> > > > > > > > > + int err;
> > > > > > > > > +
> > > > > > > > > + if (pcie->cid == 5)
> > > > > > > > > + return 0;
> > > > > > > >
> > > > > > > > What's wrong with cid == 5 ? Explain please.
> > > > > > > Controller with ID=5 doesn't need any programming to enable it which is
> > > > > > > done here through calling firmware API.
> > > > > > >
> > > > > > > >
> > > > > > > > > + memset(&req, 0, sizeof(req));
> > > > > > > > > + memset(&resp, 0, sizeof(resp));
> > > > > > > > > +
> > > > > > > > > + req.cmd = CMD_UPHY_PCIE_CONTROLLER_STATE;
> > > > > > > > > + req.controller_state.pcie_controller = pcie->cid;
> > > > > > > > > + req.controller_state.enable = enable;
> > > > > > > > > +
> > > > > > > > > + memset(&msg, 0, sizeof(msg));
> > > > > > > > > + msg.mrq = MRQ_UPHY;
> > > > > > > > > + msg.tx.data = &req;
> > > > > > > > > + msg.tx.size = sizeof(req);
> > > > > > > > > + msg.rx.data = &resp;
> > > > > > > > > + msg.rx.size = sizeof(resp);
> > > > > > > > > +
> > > > > > > > > + if (irqs_disabled())
> > > > > > > >
> > > > > > > > Can you explain to me what this check is meant to achieve please ?
> > > > > > > Firmware interface provides different APIs to be called when there are
> > > > > > > no interrupts enabled in the system (noirq context) and otherwise
> > > > > > > hence checking that situation here and calling appropriate API.
> > > > > >
> > > > > > That's what I am questioning. Being called from {suspend/resume}_noirq()
> > > > > > callbacks (if that's the code path this check caters for) does not mean
> > > > > > irqs_disabled() == true.
> > > > > Agree.
> > > > > Actually, I got a hint of having this check from the following.
> > > > > Both tegra_bpmp_transfer_atomic() and tegra_bpmp_transfer() are indirectly
> > > > > called by APIs registered with .master_xfer() and .master_xfer_atomic() hooks of
> > > > > struct i2c_algorithm and the decision to call which one of these is made using the
> > > > > following check in i2c-core.h file.
> > > > > static inline bool i2c_in_atomic_xfer_mode(void)
> > > > > {
> > > > > return system_state > SYSTEM_RUNNING && irqs_disabled();
> > > > > }
> > > > > I think I should use this condition as is IIUC.
> > > > > Please let me know if there are any concerns with this.
> > > >
> > > > It is not a concern, it is just that I don't understand how this code
> > > > can be called with IRQs disabled, if you can give me an execution path I
> > > > am happy to leave the check there. On top of that, when called from
> > > > suspend NOIRQ context, it is likely to use the blocking API (because
> > > > IRQs aren't disabled at CPU level) behind which there is most certainly
> > > > an IRQ required to wake the thread up and if the IRQ in question was
> > > > disabled in the suspend NOIRQ phase this code is likely to deadlock.
> > > >
> > > > I want to make sure we can justify adding this check, I do not
> > > > want to add it because we think it can be needed when it may not
> > > > be needed at all (and it gets copy and pasted over and over again
> > > > in other drivers).
> > > I had a discussion internally about this and the prescribed usage of these APIs
> > > seem to be that
> > > use tegra_bpmp_transfer() in .probe() and other paths where interrupts are
> > > enabled as this API needs interrupts to be enabled for its working.
> > > Use tegra_bpmp_transfer_atomic() surrounded by local_irq_save()/local_irq_restore()
> > > in other paths where interrupt servicing is disabled.
> >
> > Why tegra_bpmp_transfer_atomic() needs IRQs to be disabled ? And why
> > is it needed in this piece of code where IRQs are _never_ disabled
> > at CPU level ?
> >
> > IRQs are enabled when you call a suspend_noirq() callback, so the
> > blocking API can be used as long as the IRQ descriptor backing
> > the IRQ that will wake-up the blocked call is marked as
> > IRQF_NO_SUSPEND.
> >
> > The problem is not IRQs enabled/disabled at CPU level, the problem is
> > the IRQ descriptor of the IRQ required to handle the blocking BPMP call,
> > mark it as IRQF_NO_SUSPEND and remove the tegra_bpmp_transfer_atomic()
> > call from this code (or please give me a concrete example pinpointing
> > why it is needed).
> Ideally, using tegra_bpmp_transfer() alone in all paths (.probe() as
> well as .resume_noirq()) should have worked as the corresponding IRQ
> is already flagged as IRQF_NO_SUSPEND, but, because of the way BPMP-FW
> driver in kernel making its interface available through
> .resume_early(), tegra_bpmp_transfer() wasn't working as expected and
> I pushed a patch (CC'ing you) at
> http://patchwork.ozlabs.org/patch/1140973/ to make it .resume_noirq()
> from .resume_early(). With that in place, we can just use
> tegra_bpmp_trasnfer(). I'll push a new patch with this change once my
> BPMP-FW driver patch is approved.
Does this leave you with a resume_noirq() callbacks ordering issue to
sort out ?
a.k.a How will you guarantee that the BPMP will resume before the host
bridge ?
Thanks,
Lorenzo
> Thanks,
> Vidya Sagar
> >
> > Thanks,
> > Lorenzo
> >
> > > I'll go ahead and make next patch series with this if this looks fine to you.
> > >
> > > >
> > > > > > Actually, if tegra_bpmp_transfer() requires IRQs to be enabled you may
> > > > > > even end up in a situation where that blocking call does not wake up
> > > > > > because the IRQ in question was disabled in the NOIRQ suspend/resume
> > > > > > phase.
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > > +static int tegra_pcie_dw_probe(struct platform_device *pdev)
> > > > > > > > > +{
> > > > > > > > > + const struct tegra_pcie_soc *data;
> > > > > > > > > + struct device *dev = &pdev->dev;
> > > > > > > > > + struct resource *atu_dma_res;
> > > > > > > > > + struct tegra_pcie_dw *pcie;
> > > > > > > > > + struct resource *dbi_res;
> > > > > > > > > + struct pcie_port *pp;
> > > > > > > > > + struct dw_pcie *pci;
> > > > > > > > > + struct phy **phys;
> > > > > > > > > + char *name;
> > > > > > > > > + int ret;
> > > > > > > > > + u32 i;
> > > > > > > > > +
> > > > > > > > > + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > > > > > > > > + if (!pcie)
> > > > > > > > > + return -ENOMEM;
> > > > > > > > > +
> > > > > > > > > + pci = &pcie->pci;
> > > > > > > > > + pci->dev = &pdev->dev;
> > > > > > > > > + pci->ops = &tegra_dw_pcie_ops;
> > > > > > > > > + pp = &pci->pp;
> > > > > > > > > + pcie->dev = &pdev->dev;
> > > > > > > > > +
> > > > > > > > > + data = (struct tegra_pcie_soc *)of_device_get_match_data(dev);
> > > > > > > > > + if (!data)
> > > > > > > > > + return -EINVAL;
> > > > > > > > > + pcie->mode = (enum dw_pcie_device_mode)data->mode;
> > > > > > > > > +
> > > > > > > > > + ret = tegra_pcie_dw_parse_dt(pcie);
> > > > > > > > > + if (ret < 0) {
> > > > > > > > > + dev_err(dev, "Failed to parse device tree: %d\n", ret);
> > > > > > > > > + return ret;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl");
> > > > > > > > > + if (IS_ERR(pcie->pex_ctl_supply)) {
> > > > > > > > > + dev_err(dev, "Failed to get regulator: %ld\n",
> > > > > > > > > + PTR_ERR(pcie->pex_ctl_supply));
> > > > > > > > > + return PTR_ERR(pcie->pex_ctl_supply);
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + pcie->core_clk = devm_clk_get(dev, "core");
> > > > > > > > > + if (IS_ERR(pcie->core_clk)) {
> > > > > > > > > + dev_err(dev, "Failed to get core clock: %ld\n",
> > > > > > > > > + PTR_ERR(pcie->core_clk));
> > > > > > > > > + return PTR_ERR(pcie->core_clk);
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + pcie->appl_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > > > > > + "appl");
> > > > > > > > > + if (!pcie->appl_res) {
> > > > > > > > > + dev_err(dev, "Failed to find \"appl\" region\n");
> > > > > > > > > + return PTR_ERR(pcie->appl_res);
> > > > > > > > > + }
> > > > > > > > > + pcie->appl_base = devm_ioremap_resource(dev, pcie->appl_res);
> > > > > > > > > + if (IS_ERR(pcie->appl_base))
> > > > > > > > > + return PTR_ERR(pcie->appl_base);
> > > > > > > > > +
> > > > > > > > > + pcie->core_apb_rst = devm_reset_control_get(dev, "apb");
> > > > > > > > > + if (IS_ERR(pcie->core_apb_rst)) {
> > > > > > > > > + dev_err(dev, "Failed to get APB reset: %ld\n",
> > > > > > > > > + PTR_ERR(pcie->core_apb_rst));
> > > > > > > > > + return PTR_ERR(pcie->core_apb_rst);
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + phys = devm_kcalloc(dev, pcie->phy_count, sizeof(*phys), GFP_KERNEL);
> > > > > > > > > + if (!phys)
> > > > > > > > > + return PTR_ERR(phys);
> > > > > > > > > +
> > > > > > > > > + for (i = 0; i < pcie->phy_count; i++) {
> > > > > > > > > + name = kasprintf(GFP_KERNEL, "p2u-%u", i);
> > > > > > > > > + if (!name) {
> > > > > > > > > + dev_err(dev, "Failed to create P2U string\n");
> > > > > > > > > + return -ENOMEM;
> > > > > > > > > + }
> > > > > > > > > + phys[i] = devm_phy_get(dev, name);
> > > > > > > > > + kfree(name);
> > > > > > > > > + if (IS_ERR(phys[i])) {
> > > > > > > > > + ret = PTR_ERR(phys[i]);
> > > > > > > > > + dev_err(dev, "Failed to get PHY: %d\n", ret);
> > > > > > > > > + return ret;
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + pcie->phys = phys;
> > > > > > > > > +
> > > > > > > > > + dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > > > > > > > > + if (!dbi_res) {
> > > > > > > > > + dev_err(dev, "Failed to find \"dbi\" region\n");
> > > > > > > > > + return PTR_ERR(dbi_res);
> > > > > > > > > + }
> > > > > > > > > + pcie->dbi_res = dbi_res;
> > > > > > > > > +
> > > > > > > > > + pci->dbi_base = devm_ioremap_resource(dev, dbi_res);
> > > > > > > > > + if (IS_ERR(pci->dbi_base))
> > > > > > > > > + return PTR_ERR(pci->dbi_base);
> > > > > > > > > +
> > > > > > > > > + /* Tegra HW locates DBI2 at a fixed offset from DBI */
> > > > > > > > > + pci->dbi_base2 = pci->dbi_base + 0x1000;
> > > > > > > > > +
> > > > > > > > > + atu_dma_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > > > > > + "atu_dma");
> > > > > > > > > + if (!atu_dma_res) {
> > > > > > > > > + dev_err(dev, "Failed to find \"atu_dma\" region\n");
> > > > > > > > > + return PTR_ERR(atu_dma_res);
> > > > > > > > > + }
> > > > > > > > > + pcie->atu_dma_res = atu_dma_res;
> > > > > > > > > + pci->atu_base = devm_ioremap_resource(dev, atu_dma_res);
> > > > > > > > > + if (IS_ERR(pci->atu_base))
> > > > > > > > > + return PTR_ERR(pci->atu_base);
> > > > > > > > > +
> > > > > > > > > + pcie->core_rst = devm_reset_control_get(dev, "core");
> > > > > > > > > + if (IS_ERR(pcie->core_rst)) {
> > > > > > > > > + dev_err(dev, "Failed to get core reset: %ld\n",
> > > > > > > > > + PTR_ERR(pcie->core_rst));
> > > > > > > > > + return PTR_ERR(pcie->core_rst);
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + pp->irq = platform_get_irq_byname(pdev, "intr");
> > > > > > > > > + if (!pp->irq) {
> > > > > > > > > + dev_err(dev, "Failed to get \"intr\" interrupt\n");
> > > > > > > > > + return -ENODEV;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + ret = devm_request_irq(dev, pp->irq, tegra_pcie_irq_handler,
> > > > > > > > > + IRQF_SHARED, "tegra-pcie-intr", pcie);
> > > > > > > > > + if (ret) {
> > > > > > > > > + dev_err(dev, "Failed to request IRQ %d: %d\n", pp->irq, ret);
> > > > > > > > > + return ret;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + pcie->bpmp = tegra_bpmp_get(dev);
> > > > > > > > > + if (IS_ERR(pcie->bpmp))
> > > > > > > > > + return PTR_ERR(pcie->bpmp);
> > > > > > > > > +
> > > > > > > > > + platform_set_drvdata(pdev, pcie);
> > > > > > > > > +
> > > > > > > > > + if (pcie->mode == DW_PCIE_RC_TYPE) {
> > > > > > > > > + ret = tegra_pcie_config_rp(pcie);
> > > > > > > > > + if (ret && ret != -ENOMEDIUM)
> > > > > > > > > + goto fail;
> > > > > > > > > + else
> > > > > > > > > + return 0;
> > > > > > > >
> > > > > > > > So if the link is not up we still go ahead and make probe
> > > > > > > > succeed. What for ?
> > > > > > > We may need root port to be available to support hot-plugging of
> > > > > > > endpoint devices, so, we don't fail the probe.
> > > > > >
> > > > > > We need it or we don't. If you do support hotplugging of endpoint
> > > > > > devices point me at the code, otherwise link up failure means
> > > > > > failure to probe.
> > > > > Currently hotplugging of endpoint is not supported, but it is one of
> > > > > the use cases that we may add support for in future.
> > > >
> > > > You should elaborate on this, I do not understand what you mean,
> > > > either the root port(s) supports hotplug or it does not.
> > > >
> > > > > But, why should we fail probe if link up doesn't happen? As such,
> > > > > nothing went wrong in terms of root port initialization right? I
> > > > > checked other DWC based implementations and following are not failing
> > > > > the probe pci-dra7xx.c, pcie-armada8k.c, pcie-artpec6.c, pcie-histb.c,
> > > > > pcie-kirin.c, pcie-spear13xx.c, pci-exynos.c, pci-imx6.c,
> > > > > pci-keystone.c, pci-layerscape.c
> > > > >
> > > > > Although following do fail the probe if link is not up. pcie-qcom.c,
> > > > > pcie-uniphier.c, pci-meson.c
> > > > >
> > > > > So, to me, it looks more like a choice we can make whether to fail the
> > > > > probe or not and in this case we are choosing not to fail.
> > > >
> > > > I disagree. I had an offline chat with Bjorn and whether link-up should
> > > > fail the probe or not depends on whether the root port(s) is hotplug
> > > > capable or not and this in turn relies on the root port "Slot
> > > > implemented" bit in the PCI Express capabilities register.
> > > >
> > > > It is a choice but it should be based on evidence.
> > > >
> > > > Lorenzo
> > > With Bjorn's latest comment on top of this, I think we are good not to fail
> > > the probe here.
> > >
> > > - Vidya Sagar
> > > >
> > >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64/cache: fix -Woverride-init compiler warnings
From: Will Deacon @ 2019-08-05 14:01 UTC (permalink / raw)
To: Qian Cai; +Cc: Mark Rutland, Catalin Marinas, linux-kernel, linux-arm-kernel
In-Reply-To: <771C4B4C-6D79-419D-9778-5DE1BFC67FBE@lca.pw>
On Mon, Aug 05, 2019 at 07:47:37AM -0400, Qian Cai wrote:
>
>
> > On Aug 5, 2019, at 5:52 AM, Will Deacon <will@kernel.org> wrote:
> >
> > On Fri, Aug 02, 2019 at 11:32:24AM -0400, Qian Cai wrote:
> >> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged
> >> VIVT I-caches") introduced some compiation warnings from GCC,
> >>
> >> arch/arm64/kernel/cpuinfo.c:38:26: warning: initialized field
> >> overwritten [-Woverride-init]
> >> [ICACHE_POLICY_VIPT] = "VIPT",
> >> ^~~~~~
> >> arch/arm64/kernel/cpuinfo.c:38:26: note: (near initialization for
> >> 'icache_policy_str[2]')
> >> arch/arm64/kernel/cpuinfo.c:39:26: warning: initialized field
> >> overwritten [-Woverride-init]
> >> [ICACHE_POLICY_PIPT] = "PIPT",
> >> ^~~~~~
> >> arch/arm64/kernel/cpuinfo.c:39:26: note: (near initialization for
> >> 'icache_policy_str[3]')
> >> arch/arm64/kernel/cpuinfo.c:40:27: warning: initialized field
> >> overwritten [-Woverride-init]
> >> [ICACHE_POLICY_VPIPT] = "VPIPT",
> >> ^~~~~~~
> >> arch/arm64/kernel/cpuinfo.c:40:27: note: (near initialization for
> >> 'icache_policy_str[0]')
> >>
> >> because it initializes icache_policy_str[0 ... 3] twice.
> >>
> >> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
> >> Signed-off-by: Qian Cai <cai@lca.pw>
> >> ---
> >> arch/arm64/kernel/cpuinfo.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> >> index 876055e37352..193b38da8d96 100644
> >> --- a/arch/arm64/kernel/cpuinfo.c
> >> +++ b/arch/arm64/kernel/cpuinfo.c
> >> @@ -34,10 +34,10 @@
> >> static struct cpuinfo_arm64 boot_cpu_data;
> >>
> >> static char *icache_policy_str[] = {
> >> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN",
> >> + [ICACHE_POLICY_VPIPT] = "VPIPT",
> >> + [ICACHE_POLICY_VPIPT + 1] = "RESERVED/UNKNOWN",
> >> [ICACHE_POLICY_VIPT] = "VIPT",
> >> [ICACHE_POLICY_PIPT] = "PIPT",
> >> - [ICACHE_POLICY_VPIPT] = "VPIPT",
> >
> > I really don't like this patch. Using "[0 ... MAXIDX] = <default>" is a
> > useful idiom and I think the code is more error-prone the way you have
> > restructured it.
> >
> > Why are you passing -Woverride-init to the compiler anyway? There's only
> > one Makefile that references that option, and it's specific to a pinctrl
> > driver.
>
> Those extra warnings can be enabled by “make W=1”. “-Woverride-init “ seems to be useful
> to catch potential developer mistakes with unintented double-initializations. It is normal to
> start to fix the most of false-positives first before globally enabling the flag by default just like
> “-Wimplicit-fallthrough” mentioned in,
>
> https://lwn.net/Articles/794944/
I think this case is completely different to the implicit fallthrough stuff.
The solution there was simply to add a comment without restructuring the
surrounding code. What your patch does here is actively make the code harder
to understand.
Initialising a static array with a non-zero pattern is a useful idiom and I
don't think we should throw that away just to appease a silly compiler
warning that appears only with non-default build options. Have a look at
the way we use PERF_MAP_ALL_UNSUPPORTED in the Arm PMU code, for example.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox