Re: [PATCH] powerpc/eeh: Fix wrong flag passed to eeh_unfreeze_pe()
Gavin Shan writes:
> In __eeh_clear_pe_frozen_state(), we should pass the flag's value
> instead of its address to eeh_unfreeze_pe(). This doesn't introduce
> any problems, but the code is just wrong.
It means any caller that passes false, will be getting the wrong
behaviour. eg. I see at least one call in eeh_reset_device() which
passes false to eeh_clear_pe_frozen_state(), which is then passed to
__eeh_clear_pe_frozen_state().
But I guess you're saying that caller doesn't actually see a bug because
of this?
> This fixes the code by passing flag's value to eeh_unfreeze_pe().
>
> Cc: sta...@vger.kernel.org #3.18+
> Fixes: 5cfb20b96f6 ("powerpc/eeh: Emulate EEH recovery for VFIO devices")
> Signed-off-by: Gavin Shan
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c
> b/arch/powerpc/kernel/eeh_driver.c
> index d88573b..fa15fa6 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -549,7 +549,7 @@ static void *__eeh_clear_pe_frozen_state(void *data, void
> *flag)
bool *clear_sw_state = flag;
> int i, rc = 1;
>
> for (i = 0; rc && i < 3; i++)
> - rc = eeh_unfreeze_pe(pe, clear_sw_state);
> + rc = eeh_unfreeze_pe(pe, *clear_sw_state);
I think it would be better to just do the dereference once:
bool clear_sw_state = *(bool *)flag;
int i, rc = 1;
for (i = 0; rc && i < 3; i++)
rc = eeh_unfreeze_pe(pe, clear_sw_state);
cheers
Re: [PATCH v5 2/2] KVM: PPC: Exit guest upon MCE when FWNMI capability is enabled
On 01/16/2017 10:05 AM, Paul Mackerras wrote:
> On Fri, Jan 13, 2017 at 04:51:45PM +0530, Aravinda Prasad wrote:
>> Enhance KVM to cause a guest exit with KVM_EXIT_NMI
>> exit reason upon a machine check exception (MCE) in
>> the guest address space if the KVM_CAP_PPC_FWNMI
>> capability is enabled (instead of delivering a 0x200
>> interrupt to guest). This enables QEMU to build error
>> log and deliver machine check exception to guest via
>> guest registered machine check handler.
>>
>> This approach simplifies the delivery of machine
>> check exception to guest OS compared to the earlier
>> approach of KVM directly invoking 0x200 guest interrupt
>> vector.
>>
>> This design/approach is based on the feedback for the
>> QEMU patches to handle machine check exception. Details
>> of earlier approach of handling machine check exception
>> in QEMU and related discussions can be found at:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00813.html
>>
>> Note:
>>
>> This patch introduces a hook which is invoked at the time
>> of guest exit to facilitate the host-side handling of
>> machine check exception before the exception is passed
>> on to the guest. Hence, the host-side handling which was
>> performed earlier via machine_check_fwnmi is removed.
>>
>> The reasons for this approach is (i) it is not possible
>> to distinguish whether the exception occurred in the
>> guest or the host from the pt_regs passed on the
>> machine_check_exception(). Hence machine_check_exception()
>> calls panic, instead of passing on the exception to
>> the guest, if the machine check exception is not
>> recoverable. (ii) the approach introduced in this
>> patch gives opportunity to the host kernel to perform
>> actions in virtual mode before passing on the exception
>> to the guest. This approach does not require complex
>> tweaks to machine_check_fwnmi and friends.
>>
>> Signed-off-by: Aravinda Prasad
>> Reviewed-by: David Gibson
>
> This patch mostly looks OK. I have a few relatively minor comments
> below.
>
>> ---
>> arch/powerpc/kvm/book3s_hv.c| 27 +-
>> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 47
>> ---
>> arch/powerpc/platforms/powernv/opal.c | 10 +++
>> 3 files changed, 54 insertions(+), 30 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 3686471..cae4921 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -123,6 +123,7 @@ MODULE_PARM_DESC(halt_poll_ns_shrink, "Factor halt poll
>> time is shrunk by");
>>
>> static void kvmppc_end_cede(struct kvm_vcpu *vcpu);
>> static int kvmppc_hv_setup_htab_rma(struct kvm_vcpu *vcpu);
>> +static void kvmppc_machine_check_hook(void);
>>
>> static inline struct kvm_vcpu *next_runnable_thread(struct kvmppc_vcore *vc,
>> int *ip)
>> @@ -954,15 +955,14 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run,
>> struct kvm_vcpu *vcpu,
>> r = RESUME_GUEST;
>> break;
>> case BOOK3S_INTERRUPT_MACHINE_CHECK:
>> +/* Exit to guest with KVM_EXIT_NMI as exit reason */
>> +run->exit_reason = KVM_EXIT_NMI;
>> +r = RESUME_HOST;
>> /*
>> - * Deliver a machine check interrupt to the guest.
>> - * We have to do this, even if the host has handled the
>> - * machine check, because machine checks use SRR0/1 and
>> - * the interrupt might have trashed guest state in them.
>> + * Invoke host-kernel handler to perform any host-side
>> + * handling before exiting the guest.
>> */
>> -kvmppc_book3s_queue_irqprio(vcpu,
>> -BOOK3S_INTERRUPT_MACHINE_CHECK);
>> -r = RESUME_GUEST;
>> +kvmppc_machine_check_hook();
>
> Note that this won't necessarily be called on the same CPU that
> received the machine check. This will be called on thread 0 of the
> core (or subcore), whereas the machine check could have occurred on
> some other thread. Are you sure that the machine check handling code
> will be OK with that?
That will have only one problem. get_mce_event() from
opal_machine_check() may not be able to pull mce event for error on
non-zero thread. We should hook the mce event into vcpu structure during
kvmppc_realmode_machine_check() and then pass it to
ppc_md.machine_check_exception() as an additional argument.
>
>> break;
>> case BOOK3S_INTERRUPT_PROGRAM:
>> {
>> @@ -3491,6 +3491,19 @@ static void kvmppc_irq_bypass_del_producer_hv(struct
>> irq_bypass_consumer *cons,
>> }
>> #endif
>>
>> +/*
>> + * Hook to handle machine check exceptions occurred inside a guest.
>> + * This hook is invoked from host virtual mode from KVM before exiting
>> + * the guest with KVM_EXIT_NMI exit reason. This gives an opportunity
>> + * for the host to take action (i
Re: [PATCH] powerpc/eeh: Enable IO path on permanent error
On Fri, 2017-01-06 at 10:39 +1100, Gavin Shan wrote: > We give up recovery on permanent error, simply shutdown the affected > devices and remove them. If the devices can't be put into quiet state, > they spew more traffic that is likely to cause another unexpected EEH > error. This was observed on "p8dtu2u" machine: > > 0002:00:00.0 PCI bridge: IBM Device 03dc > 0002:01:00.0 Ethernet controller: Intel Corporation \ > Ethernet Controller X710/X557-AT 10GBASE-T (rev 02) > 0002:01:00.1 Ethernet controller: Intel Corporation \ > Ethernet Controller X710/X557-AT 10GBASE-T (rev 02) > 0002:01:00.2 Ethernet controller: Intel Corporation \ > Ethernet Controller X710/X557-AT 10GBASE-T (rev 02) > 0002:01:00.3 Ethernet controller: Intel Corporation \ > Ethernet Controller X710/X557-AT 10GBASE-T (rev 02) > > On P8 PowerNV platform, the IO path is frozen when shutdowning the > devices, meaning the memory registers are inaccessible. It is why > the devices can't be put into quiet state before removing them. > This fixes the issue by enabling IO path prior to putting the devices > into quiet state. > > Link: https://github.com/open-power/supermicro-openpower/issues/419 > Reported-by: Pridhiviraj Paidipeddi > Signed-off-by: Gavin Shan > --- (forgot to ack this) Acked-by: Russell Currey
[PATCH v2] powerpc/perf: use MSR to report privilege level on P9 DD1
Since SIER and SIAR updates are not valid for some
samples, patch forces the use of MSR and regs->nip instead
for misc_flag updates. This is done by adding a new ppmu
flag and updating user_siar value in perf_read_regs() accordingly.
Signed-off-by: Madhavan Srinivasan
---
Changelog:
1)Added a new ppmu flag
2)Added the new flag to the check in perf_read_regs()
3)Made changes to commit message
arch/powerpc/include/asm/perf_event_server.h | 1 +
arch/powerpc/perf/core-book3s.c | 2 ++
arch/powerpc/perf/power9-pmu.c | 2 +-
3 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/perf_event_server.h
b/arch/powerpc/include/asm/perf_event_server.h
index e157489ee7a1..acab0ff4e7a3 100644
--- a/arch/powerpc/include/asm/perf_event_server.h
+++ b/arch/powerpc/include/asm/perf_event_server.h
@@ -65,6 +65,7 @@ struct power_pmu {
#define PPMU_HAS_SSLOT 0x0020 /* Has sampled slot in MMCRA */
#define PPMU_HAS_SIER 0x0040 /* Has SIER */
#define PPMU_ARCH_207S 0x0080 /* PMC is architecture v2.07S */
+#define PPMU_DISABLE_USE_SIAR 0x0100 /* Do not use SIER and SIAR */
/*
* Values for flags to get_alternatives()
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index fd3e4034c04d..8827bea50b91 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -295,6 +295,8 @@ static inline void perf_read_regs(struct pt_regs *regs)
*/
if (TRAP(regs) != 0xf00)
use_siar = 0;
+ else if ((ppmu->flags & PPMU_DISABLE_USE_SIAR))
+ use_siar = 0;
else if (marked)
use_siar = 1;
else if ((ppmu->flags & PPMU_NO_CONT_SAMPLING))
diff --git a/arch/powerpc/perf/power9-pmu.c b/arch/powerpc/perf/power9-pmu.c
index 346010e8d463..4eec7ad9f187 100644
--- a/arch/powerpc/perf/power9-pmu.c
+++ b/arch/powerpc/perf/power9-pmu.c
@@ -384,7 +384,7 @@ static struct power_pmu power9_isa207_pmu = {
.bhrb_filter_map= power9_bhrb_filter_map,
.get_constraint = isa207_get_constraint,
.disable_pmc= isa207_disable_pmc,
- .flags = PPMU_HAS_SIER | PPMU_ARCH_207S,
+ .flags = PPMU_DISABLE_USE_SIAR | PPMU_HAS_SIER |
PPMU_ARCH_207S,
.n_generic = ARRAY_SIZE(power9_generic_events),
.generic_events = power9_generic_events,
.cache_events = &power9_cache_events,
--
2.7.4
Re: [PATCH v5 4/4] powerpc/mm: unstub radix__vmemmap_remove_mapping()
On Tue, Jan 17, 2017 at 12:36:53PM -0600, Reza Arbab wrote: > On Tue, Jan 17, 2017 at 12:55:13PM +0530, Balbir Singh wrote: > > On Mon, Jan 16, 2017 at 01:07:46PM -0600, Reza Arbab wrote: > > > Use remove_pagetable() and friends for radix vmemmap removal. > > > > > > We do not require the special-case handling of vmemmap done in the x86 > > > versions of these functions. This is because vmemmap_free() has already > > > freed the mapped pages, and calls us with an aligned address range. > > > > > > So, add a few failsafe WARNs, but otherwise the code to remove physical > > > mappings is already sufficient for vmemmap. > > > > I wonder if we really need them? > > Not sure what the guideline is for a "this shouldn't happen" WARN. It could > save us some grief, should our vmemmap code ever start calling with an > unaligned range, like it does on x86. > Fair enough Acked-by: Balbir Singh
Re: [PATCH v5 3/4] powerpc/mm: add radix__remove_section_mapping()
On Tue, Jan 17, 2017 at 12:36:21PM -0600, Reza Arbab wrote:
> On Tue, Jan 17, 2017 at 12:52:51PM +0530, Balbir Singh wrote:
> > Shouldn't most of these functions have __meminit?
>
> I don't think so. The mapping functions are __meminit, but the unmapping
> functions are completely within #ifdef CONFIG_MEMORY_HOTPLUG already.
>
> > On Mon, Jan 16, 2017 at 01:07:45PM -0600, Reza Arbab wrote:
> > > #ifdef CONFIG_MEMORY_HOTPLUG
> > > +static void free_pte_table(pte_t *pte_start, pmd_t *pmd)
> > > +{
> > > + pte_t *pte;
> > > + int i;
> > > +
> > > + for (i = 0; i < PTRS_PER_PTE; i++) {
> > > + pte = pte_start + i;
> > > + if (!pte_none(*pte))
> > > + return;
> >
> > If !pte_none() we fail the hotplug? Or silently
> > leave the allocated pte's around. I guess this is
> > the same as x86
>
> The latter--it's not a failure. If you provided remove_pagetable() an
> unaligned address range, there could be a pte left unremoved at either end.
>
OK.
> > > +static void remove_pmd_table(pmd_t *pmd_start, unsigned long addr,
> > > + unsigned long end)
> > > +{
> > > + unsigned long next;
> > > + pte_t *pte_base;
> > > + pmd_t *pmd;
> > > +
> > > + pmd = pmd_start + pmd_index(addr);
> > > + for (; addr < end; addr = next, pmd++) {
> > > + next = pmd_addr_end(addr, end);
> > > +
> > > + if (!pmd_present(*pmd))
> > > + continue;
> > > +
> > > + if (pmd_huge(*pmd)) {
> > > + pte_clear(&init_mm, addr, (pte_t *)pmd);
> >
> > pmd_clear()?
>
> I used pte_clear() to mirror what happens in radix__map_kernel_page():
>
> if (map_page_size == PMD_SIZE) {
> ptep = (pte_t *)pmdp;
> goto set_the_pte;
> }
>
> [...]
>
> set_the_pte:
> set_pte_at(&init_mm, ea, ptep, pfn_pte(pa >> PAGE_SHIFT,
> flags));
>
> Would pmd_clear() be equivalent, since the pointer got set like a pte?
But we are still setting a pmdp. pmd_clear() will set the pmd to 0,
pte_clear() will go through the pte_update() mechanism which is expensive
IMHO and we may not need to do it.
>
> > > +static void remove_pagetable(unsigned long start, unsigned long end)
> > > +{
> > > + unsigned long addr, next;
> > > + pud_t *pud_base;
> > > + pgd_t *pgd;
> > > +
> > > + spin_lock(&init_mm.page_table_lock);
> > > +
> >
> > x86 does more granular lock acquisition only during
> > clearing the relevant entries. I suppose we don't have
> > to worry about it since its not fast path and frequent.
>
> Yep. Ben thought the locking in remove_pte_table() was actually too
> granular, and Aneesh questioned what was being protected in the first place.
> So I left one lock/unlock in the outermost function for now.
>
Fair enough
Balbir Singh.
Re: [PATCH v5 1/4] powerpc/mm: refactor radix physical page mapping
On Tue, Jan 17, 2017 at 12:34:56PM -0600, Reza Arbab wrote:
> Thanks for your review!
>
> On Tue, Jan 17, 2017 at 12:16:35PM +0530, Balbir Singh wrote:
> > On Mon, Jan 16, 2017 at 01:07:43PM -0600, Reza Arbab wrote:
> > > --- a/arch/powerpc/mm/pgtable-radix.c
> > > +++ b/arch/powerpc/mm/pgtable-radix.c
> > > @@ -107,54 +107,66 @@ int radix__map_kernel_page(unsigned long ea,
> > > unsigned long pa,
> > > return 0;
> > > }
> > >
> > > +static inline void __meminit print_mapping(unsigned long start,
> > > +unsigned long end,
> > > +unsigned long size)
> > > +{
> > > + if (end <= start)
> > > + return;
> >
> > Should we pr_err for start > end?
>
> I think that would be overkill. The way this little inline is called, start
> > end is not possible. The real point is not to print anything if start ==
> end. Using <= just seemed better in context.
>
Agreed
> >
> > Should we try a lower page size if map_kernel_page fails for this
> > mapping_size?
>
> The only way map_kernel_page can fail is -ENOMEM. If that's the case,
> there's no way we're going to be able to map this range at all. Better to
> fail fast here, I would think.
>
I think I am OK with this implementation for now.
Balbir Singh.
Re: [PATCH] powerpc: Use octal numbers for file permissions
On Tue, 2017-01-17 at 20:52 +1100, Michael Ellerman wrote: > Cyril Bur writes: > > > On Thu, 2017-01-12 at 14:54 +1100, Russell Currey wrote: > > > Symbolic macros are unintuitive and hard to read, whereas octal constants > > > are much easier to interpret. Replace macros for the basic permission > > > flags (user/group/other read/write/execute) with numeric constants > > > instead, across the whole powerpc tree. > > > > > > Introducing a significant number of changes across the tree for no runtime > > > benefit isn't exactly desirable, but so long as these macros are still > > > used in the tree people will keep sending patches that add them. Not only > > > are they hard to parse at a glance, there are multiple ways of coming to > > > the same value (as you can see with 0444 and 0644 in this patch) which > > > hurts readability. > > > > > > Signed-off-by: Russell Currey > > > > Reviewed-by: Cyril Bur > > Did you really really review every single change? > Yes. I just went through it again and still didn't find any mistakes. > Because if you did then I don't have to, and that would be *great* :) > My pleasure :) Interestingly enough, reviewing this patch taught me to quickly parse the symbolics, which, for me now makes this patch less important haha. I'm still in favour! No matter how fast you get at the symolics the octal is still faster, also there's only one way to write the octal permissions! > cheers
Re: [PATCH v4 3/3] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs
On Tue, Jan 17, 2017 at 11:26 AM, Ard Biesheuvel
wrote:
> The modversion symbol CRCs are emitted as ELF symbols, which allows us to
> easily populate the kcrctab sections by relying on the linker to associate
> each kcrctab slot with the correct value.
>
> This has a couple of downsides:
So the whole relocation of the crc is obviously completely crazy, but
you don't actually seem to *change* that. You just work around it, and
you make the symbols 32-bit. The latter part I agree with
whole-heartedly, btw.
But do we really have to accept this relocation insanity?
So I don't actually disagree with this patch 3/3 (turning the whole
crc array into an array of "u32" is clearly the right thing to do),
but the two other patches look oddly broken.
Why are those CRC's relocatable to begin with? Shouldn't they be
absolute values? Why do they get those idiotic relocation things? They
seem to be absolute on x86-64 (just doing a 'nm vmlinux', so I might
be missing something), why aren't they on ppc?
Is there something wrong with our generation script? Can we possibly
do something like
- printf("%s__crc_%s = 0x%08lx ;\n", mod_prefix, name, crc);
+ printf("%s__crc_%s = ABSOLUTE(0x%08lx) ;\n", mod_prefix, name, crc);
in genksyms.c to get rid of the crazty relocation entries?
Linus
[PATCH v4 3/3] modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs
The modversion symbol CRCs are emitted as ELF symbols, which allows us to
easily populate the kcrctab sections by relying on the linker to associate
each kcrctab slot with the correct value.
This has a couple of downsides:
- Given that the CRCs are treated as memory addresses, we waste 4 bytes
for each CRC on 64 bit architectures,
- On architectures that support runtime relocation, a R__RELATIVE
relocation entry is emitted for each CRC value, which identifies it as
a quantity that requires fixing up based on the actual runtime load
offset of the kernel. This results in corrupted CRCs unless we
explicitly undo the fixup (and this is currently being handled in the
core module code)
- Such runtime relocation entries take up 24 bytes of __init space each,
resulting in a x8 overhead in [uncompressed] kernel size for CRCs.
Switching to explicit 32 bit values on 64 bit architectures fixes most
of these issues, given that 32 bit values are not treated as quantities
that require fixing up based on the actual runtime load offset. Note that
on some ELF64 architectures [such as PPC64], these 32-bit values are still
emitted as runtime relocatable quantities, even if the value resolves to
a build time constant. However, these can easily be distinguished from
variables that do need fixing up, and the CRCs can be emitted correctly
in the arch specific runtime relocation routines right off the bat.
So redefine all CRC fields and variables as u32, and redefine the
__CRC_SYMBOL() macro for 64 bit builds to emit the CRC reference using
inline assembler (which is necessary since 64-bit C code cannot use
32-bit types to hold memory addresses, even if they are ultimately
resolved using values that do not exceed 0x.
Note that this mostly reverts commit d4703aefdbc8 ("module: handle ppc64
relocating kcrctabs when CONFIG_RELOCATABLE=y")
Acked-by: Rusty Russell
Signed-off-by: Ard Biesheuvel
---
arch/powerpc/include/asm/module.h | 4 --
arch/powerpc/kernel/module_64.c | 8
include/asm-generic/export.h | 7 +--
include/linux/export.h| 8
include/linux/module.h| 14 +++---
kernel/module.c | 47 +++-
6 files changed, 33 insertions(+), 55 deletions(-)
diff --git a/arch/powerpc/include/asm/module.h
b/arch/powerpc/include/asm/module.h
index cc12c61ef315..53885512b8d3 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -90,9 +90,5 @@ static inline int module_finalize_ftrace(struct module *mod,
const Elf_Shdr *sec
}
#endif
-#if defined(CONFIG_MODVERSIONS) && defined(CONFIG_PPC64)
-#define ARCH_RELOCATES_KCRCTAB
-#define reloc_start PHYSICAL_START
-#endif
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_MODULE_H */
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index bb1807184bad..0b0f89685b67 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -286,14 +286,6 @@ static void dedotify_versions(struct modversion_info *vers,
for (end = (void *)vers + size; vers < end; vers++)
if (vers->name[0] == '.') {
memmove(vers->name, vers->name+1, strlen(vers->name));
-#ifdef ARCH_RELOCATES_KCRCTAB
- /* The TOC symbol has no CRC computed. To avoid CRC
-* check failing, we must force it to the expected
-* value (see CRC check in module.c).
-*/
- if (!strcmp(vers->name, "TOC."))
- vers->crc = -(unsigned long)reloc_start;
-#endif
}
}
diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 63554e9f6e0c..e3508a3d6e53 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -9,18 +9,15 @@
#ifndef KSYM_ALIGN
#define KSYM_ALIGN 8
#endif
-#ifndef KCRC_ALIGN
-#define KCRC_ALIGN 8
-#endif
#else
#define __put .long
#ifndef KSYM_ALIGN
#define KSYM_ALIGN 4
#endif
+#endif
#ifndef KCRC_ALIGN
#define KCRC_ALIGN 4
#endif
-#endif
#ifdef CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX
#define KSYM(name) _##name
@@ -52,7 +49,7 @@ KSYM(__kstrtab_\name):
.section ___kcrctab\sec+\name,"a"
.balign KCRC_ALIGN
KSYM(__kcrctab_\name):
- __put KSYM(__crc_\name)
+ .long KSYM(__crc_\name)
.weak KSYM(__crc_\name)
.previous
#endif
diff --git a/include/linux/export.h b/include/linux/export.h
index 2a0f61fbc731..a000d421526d 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -41,6 +41,7 @@ extern struct module __this_module;
#if defined(__KERNEL__) && !defined(__GENKSYMS__)
#ifdef CONFIG_MODVERSIONS
+#ifndef CONFIG_64BIT
/* Mark the CRC weak since genksyms apparently decides not to
* generate a checksums for some symbols */
#define __CRC_SYMBOL(sym, sec) \
@@ -50,6 +51,13 @@ extern struct module __this_module;
[PATCH v4 2/3] powerpc/reloc64: add support for 32-bit CRC pseudo-symbols
In preparation of modifying the core modversions code to emit the CRCs as 32-bit quantities, ensure that 64-bit PowerPC will be able to deal with this when CONFIG_RELOCATABLE=y, in which case the CRCs will be emitted into the final ELF binary as R_PPC64_ADDR32 relocations. Since 32-bit relocations cannot be used to relocate memory addresses on 64-bit architectures, and since the CRC pseudo-symbol references are emitted as anonymous relocations (i.e., against the NULL symbol in the .dynsym section) with the final value recorded in the addend (*), we can disregard any relocations where the symbol index != 0. * Note that unsatisfied CRC pseudo-symbol references are emitted as R_PPC64_ADDR32 relocations against named symbols that are typed as weak undefined in the .dynsym symbol table. These can simply be ignored (as before), considering that zero CRCs are interpreted as missing, and the module code deals with that accordingly. As it turns out, binutils for powerpc does not account for any relocations beyond R_PPC64_RELATIVE ones in the RELACOUNT field of the .dynamic section, which is unfortunate, since we need to do extra work to figure out the size of the relocation array. So with a little help from the linker scripts, grab an end pointer rather than a count, and iterate over the entire section. Signed-off-by: Ard Biesheuvel --- arch/powerpc/kernel/reloc_64.S| 60 arch/powerpc/kernel/vmlinux.lds.S | 1 + arch/powerpc/relocs_check.sh | 5 +- 3 files changed, 40 insertions(+), 26 deletions(-) diff --git a/arch/powerpc/kernel/reloc_64.S b/arch/powerpc/kernel/reloc_64.S index d88736fbece6..e50f5d778ea2 100644 --- a/arch/powerpc/kernel/reloc_64.S +++ b/arch/powerpc/kernel/reloc_64.S @@ -12,8 +12,8 @@ #include RELA = 7 -RELACOUNT = 0x6ff9 R_PPC64_RELATIVE = 22 +R_PPC64_ADDR32 = 1 /* * r3 = desired final address of kernel @@ -29,29 +29,27 @@ _GLOBAL(relocate) add r9,r9,r12 /* r9 has runtime addr of .rela.dyn section */ ld r10,(p_st - 0b)(r12) add r10,r10,r12 /* r10 has runtime addr of _stext */ + ld r8,(p_rela_end - 0b)(r12) + add r8,r8,r12 /* r8 has addr of end of .rela.dyn section */ /* -* Scan the dynamic section for the RELA and RELACOUNT entries. +* Scan the dynamic section for the RELA entry. +* NOTE: the RELACOUNT entry only covers R_PPC64_RELATIVE relocations, +* so we cannot use it here. */ li r7,0 - li r8,0 1: ld r6,0(r11) /* get tag */ cmpdi r6,0 - beq 4f /* end of list */ + beq 3f /* end of list */ cmpdi r6,RELA - bne 2f - ld r7,8(r11) /* get RELA pointer in r7 */ - b 3f -2: addis r6,r6,(-RELACOUNT)@ha - cmpdi r6,RELACOUNT@l - bne 3f - ld r8,8(r11) /* get RELACOUNT value in r8 */ -3: addir11,r11,16 + beq 2f + addir11,r11,16 b 1b -4: cmpdi r7,0/* check we have both RELA and RELACOUNT */ - cmpdi cr1,r8,0 - beq 6f - beq cr1,6f +2: ld r7,8(r11) /* get RELA pointer in r7 */ +3: cmpdi r7,0/* check we have both RELA and a non-empty */ + cmpdcr1,r8,r9 /* .rela.dyn section */ + beq 7f + beq cr1,7f /* * Work out linktime address of _stext and hence the @@ -63,26 +61,40 @@ _GLOBAL(relocate) subfr7,r7,r9/* cur_offset */ subfr10,r7,r10 subfr3,r10,r3 /* final_offset */ + b 4f /* * Run through the list of relocations and process the -* R_PPC64_RELATIVE ones. +* R_PPC64_RELATIVE and R_PPC64_ADDR32 ones. */ - mtctr r8 -5: ld r0,8(9) /* ELF64_R_TYPE(reloc->r_info) */ +3: addir9,r9,24 +4: cmpdr9,r8 + beq 7f +5: ld r0,8(9) /* reloc->r_info (type *and* symbol index) */ + ld r6,0(r9)/* reloc->r_offset */ cmpdi r0,R_PPC64_RELATIVE bne 6f - ld r6,0(r9)/* reloc->r_offset */ ld r0,16(r9) /* reloc->r_addend */ add r0,r0,r3 stdxr0,r7,r6 - addir9,r9,24 - bdnz5b + b 3b + + /* +* CRCs of exported symbols are emitted as 32-bit relocations against +* the NULL .dynsym entry, with the CRC value recorded in the addend. +*/ +6: cmpdi r0,R_PPC64_ADDR32 + bne 3b + ld r0,16(r9) /* reloc->r_addend */ + stwxr0,r7,r6 + b 3b + +7: blr -6: blr .balign 8 p_dyn: .llong __dynamic_start - 0b p_rela:.llong __rela_dyn_start - 0b +p_rela_end: + .llong __rela_dyn_end - 0
[PATCH v4 1/3] powerpc/reloc32: fix corrupted modversion CRCs
Commit 0e0ed6406e61 ("powerpc/modules: Module CRC relocation fix causes
perf issues") fixed an issue with relocatable PIE kernels in a way that
essentially reintroduced the issue again for 32-bit builds.
Since the chosen approach does is not applicable to 32-bit, fix the
issue by updating the runtime relocation routine to ignore the load
offset for the interval [__start___kcrctab, __stop___kcrctab_gpl_future),
which is where the CRCs reside. This ensures that the values of the CRC
pseudo-symbols are no longer made dependent on the runtime load offset.
Reviewed-by: Suzuki K Poulose
Signed-off-by: Ard Biesheuvel
---
arch/powerpc/kernel/reloc_32.S | 36 +---
1 file changed, 32 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/reloc_32.S b/arch/powerpc/kernel/reloc_32.S
index f366fedb0872..150686b9febb 100644
--- a/arch/powerpc/kernel/reloc_32.S
+++ b/arch/powerpc/kernel/reloc_32.S
@@ -87,12 +87,12 @@ eodyn: /* End of Dyn Table
scan */
* Work out the current offset from the link time address of .rela
* section.
* cur_offset[r7] = rela.run[r9] - rela.link [r7]
-* _stext.link[r12] = _stext.run[r10] - cur_offset[r7]
-* final_offset[r3] = _stext.final[r3] - _stext.link[r12]
+* _stext.link[r11] = _stext.run[r10] - cur_offset[r7]
+* final_offset[r3] = _stext.final[r3] - _stext.link[r11]
*/
subfr7, r7, r9 /* cur_offset */
- subfr12, r7, r10
- subfr3, r12, r3 /* final_offset */
+ subfr11, r7, r10
+ subfr3, r11, r3 /* final_offset */
subfr8, r6, r8 /* relaz -= relaent */
/*
@@ -101,6 +101,21 @@ eodyn: /* End of Dyn Table
scan */
* r13 - points to the symbol table
*/
+#ifdef CONFIG_MODVERSIONS
+ /*
+* Treat R_PPC_RELATIVE relocations differently when they target the
+* interval [__start___kcrctab, __stop___kcrctab_gpl_future): in this
+* case, the relocated quantities are CRC pseudo-symbols, which should
+* be preserved as-is, rather than be modified to take the runtime
+* offset into account.
+*/
+ lwz r10, (p_kcrc_start - 0b)(r12)
+ lwz r11, (p_kcrc_stop - 0b)(r12)
+ subfr12, r7, r12/* link time addr of 0b */
+ add r10, r10, r12
+ add r11, r11, r12
+#endif
+
/*
* Check if we have a relocation based on symbol
* r5 will hold the value of the symbol.
@@ -135,7 +150,15 @@ get_type:
bne hi16
lwz r4, 0(r9) /* r_offset */
lwz r0, 8(r9) /* r_addend */
+#ifdef CONFIG_MODVERSIONS
+ cmplw r4, r10
+ blt do_add
+ cmplw r4, r11
+ blt skip_add
+do_add:
+#endif
add r0, r0, r3 /* final addend */
+skip_add:
stwxr0, r4, r7 /* memory[r4+r7]) = (u32)r0 */
b nxtrela /* continue */
@@ -207,3 +230,8 @@ p_dyn: .long __dynamic_start - 0b
p_rela:.long __rela_dyn_start - 0b
p_sym: .long __dynamic_symtab - 0b
p_st: .long _stext - 0b
+
+#ifdef CONFIG_MODVERSIONS
+p_kcrc_start: .long __start___kcrctab - 0b
+p_kcrc_stop: .long __stop___kcrctab_gpl_future - 0b
+#endif
--
2.7.4
[PATCH v4 0/3] modversions: Fix CRC mangling under CONFIG_RELOCATABLE=y
This series is a followup to the single patch 'modversions: treat symbol
CRCs as 32 bit quantities on 64 bit archs', of which three versions have
been sent out so far [0][1][2]
Given the recent issues regarding modversions, I have added some more
people to cc this time.
As pointed out by Michael, GNU ld behaves a bit differently between arm64
and PowerPC64, and where the former gets rid of all runtime relocations
related to CRCs, the latter is not as easily convinced.
Patch #1 fixes the issue where CRCs are corrupted by the runtime relocation
routines for 32-bit PowerPC, for which the original fix was effectively
reverted by commit 0e0ed6406e61 ("powerpc/modules: Module CRC relocation fix
causes perf issues")
Patch #2 adds handling of R_PPC64_ADDR32 relocations against the NULL .dynsym
symbol entry to the PPC64 runtime relocation routines, so it is prepared to
deal with CRCs being emitted as 32-bit quantities.
Patch #3 is the original patch from the v1 and v2 submissions.
Changes since v3:
- rebased onto v4.10-rc
- add Suzuki's ack to patch #1
- update patch #2 to work around the binutils-powerpc behavior to only
account for R_PPC64_RELATIVE relocations in the RELACOUNT entry in
.dynamic
Changes since v2:
- added #1 and #2
- updated #3 to deal with CRC entries being emitted from assembler
- added Rusty's ack (#3)
Branch can be found here:
https://git.kernel.org/cgit/linux/kernel/git/ardb/linux.git/log/?h=kcrctab-reloc
[0] http://marc.info/?l=linux-kernel&m=147652300207369&w=2
[1] http://marc.info/?l=linux-kernel&m=147695629614409&w=2
[2] http://marc.info/?l=linux-kernel&m=147758564705776&w=2
Ard Biesheuvel (3):
powerpc/reloc32: fix corrupted modversion CRCs
powerpc/reloc64: add support for 32-bit CRC pseudo-symbols
modversions: treat symbol CRCs as 32 bit quantities on 64 bit archs
arch/powerpc/include/asm/module.h | 4 --
arch/powerpc/kernel/module_64.c | 8 ---
arch/powerpc/kernel/reloc_32.S| 36 ++--
arch/powerpc/kernel/reloc_64.S| 60
arch/powerpc/kernel/vmlinux.lds.S | 1 +
arch/powerpc/relocs_check.sh | 5 +-
include/asm-generic/export.h | 7 +--
include/linux/export.h| 8 +++
include/linux/module.h| 14 ++---
kernel/module.c | 47 ++-
10 files changed, 105 insertions(+), 85 deletions(-)
--
2.7.4
Re: [PATCH v5 4/4] powerpc/mm: unstub radix__vmemmap_remove_mapping()
On Tue, Jan 17, 2017 at 12:55:13PM +0530, Balbir Singh wrote: On Mon, Jan 16, 2017 at 01:07:46PM -0600, Reza Arbab wrote: Use remove_pagetable() and friends for radix vmemmap removal. We do not require the special-case handling of vmemmap done in the x86 versions of these functions. This is because vmemmap_free() has already freed the mapped pages, and calls us with an aligned address range. So, add a few failsafe WARNs, but otherwise the code to remove physical mappings is already sufficient for vmemmap. I wonder if we really need them? Not sure what the guideline is for a "this shouldn't happen" WARN. It could save us some grief, should our vmemmap code ever start calling with an unaligned range, like it does on x86. -- Reza Arbab
Re: [PATCH v5 3/4] powerpc/mm: add radix__remove_section_mapping()
On Tue, Jan 17, 2017 at 12:52:51PM +0530, Balbir Singh wrote:
Shouldn't most of these functions have __meminit?
I don't think so. The mapping functions are __meminit, but the unmapping
functions are completely within #ifdef CONFIG_MEMORY_HOTPLUG already.
On Mon, Jan 16, 2017 at 01:07:45PM -0600, Reza Arbab wrote:
#ifdef CONFIG_MEMORY_HOTPLUG
+static void free_pte_table(pte_t *pte_start, pmd_t *pmd)
+{
+ pte_t *pte;
+ int i;
+
+ for (i = 0; i < PTRS_PER_PTE; i++) {
+ pte = pte_start + i;
+ if (!pte_none(*pte))
+ return;
If !pte_none() we fail the hotplug? Or silently
leave the allocated pte's around. I guess this is
the same as x86
The latter--it's not a failure. If you provided remove_pagetable() an
unaligned address range, there could be a pte left unremoved at either
end.
+static void remove_pmd_table(pmd_t *pmd_start, unsigned long addr,
+unsigned long end)
+{
+ unsigned long next;
+ pte_t *pte_base;
+ pmd_t *pmd;
+
+ pmd = pmd_start + pmd_index(addr);
+ for (; addr < end; addr = next, pmd++) {
+ next = pmd_addr_end(addr, end);
+
+ if (!pmd_present(*pmd))
+ continue;
+
+ if (pmd_huge(*pmd)) {
+ pte_clear(&init_mm, addr, (pte_t *)pmd);
pmd_clear()?
I used pte_clear() to mirror what happens in radix__map_kernel_page():
if (map_page_size == PMD_SIZE) {
ptep = (pte_t *)pmdp;
goto set_the_pte;
}
[...]
set_the_pte:
set_pte_at(&init_mm, ea, ptep, pfn_pte(pa >> PAGE_SHIFT,
flags));
Would pmd_clear() be equivalent, since the pointer got set like a pte?
+static void remove_pagetable(unsigned long start, unsigned long end)
+{
+ unsigned long addr, next;
+ pud_t *pud_base;
+ pgd_t *pgd;
+
+ spin_lock(&init_mm.page_table_lock);
+
x86 does more granular lock acquisition only during
clearing the relevant entries. I suppose we don't have
to worry about it since its not fast path and frequent.
Yep. Ben thought the locking in remove_pte_table() was actually too
granular, and Aneesh questioned what was being protected in the first
place. So I left one lock/unlock in the outermost function for now.
--
Reza Arbab
Re: [PATCH v5 1/4] powerpc/mm: refactor radix physical page mapping
Thanks for your review!
On Tue, Jan 17, 2017 at 12:16:35PM +0530, Balbir Singh wrote:
On Mon, Jan 16, 2017 at 01:07:43PM -0600, Reza Arbab wrote:
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -107,54 +107,66 @@ int radix__map_kernel_page(unsigned long ea, unsigned
long pa,
return 0;
}
+static inline void __meminit print_mapping(unsigned long start,
+ unsigned long end,
+ unsigned long size)
+{
+ if (end <= start)
+ return;
Should we pr_err for start > end?
I think that would be overkill. The way this little inline is called,
start > end is not possible. The real point is not to print anything if
start == end. Using <= just seemed better in context.
+static int __meminit create_physical_mapping(unsigned long start,
+unsigned long end)
+{
+ unsigned long addr, mapping_size;
+
+ start = _ALIGN_UP(start, PAGE_SIZE);
+ for (addr = start; addr < end; addr += mapping_size) {
+ unsigned long gap, previous_size;
+ int rc;
+
+ gap = end - addr;
+ previous_size = mapping_size;
+
+ if (IS_ALIGNED(addr, PUD_SIZE) && gap >= PUD_SIZE &&
+ mmu_psize_defs[MMU_PAGE_1G].shift)
+ mapping_size = PUD_SIZE;
+ else if (IS_ALIGNED(addr, PMD_SIZE) && gap >= PMD_SIZE &&
+mmu_psize_defs[MMU_PAGE_2M].shift)
+ mapping_size = PMD_SIZE;
+ else
+ mapping_size = PAGE_SIZE;
+
+ if (mapping_size != previous_size) {
+ print_mapping(start, addr, previous_size);
+ start = addr;
+ }
+
+ rc = radix__map_kernel_page((unsigned long)__va(addr), addr,
+ PAGE_KERNEL_X, mapping_size);
+ if (rc)
+ return rc;
Should we try a lower page size if map_kernel_page fails for this mapping_size?
The only way map_kernel_page can fail is -ENOMEM. If that's the case,
there's no way we're going to be able to map this range at all. Better
to fail fast here, I would think.
--
Reza Arbab
Re: [PATCH v4 2/5] ia64: reuse append_elf_note() and final_note() functions
On Friday 06 January 2017 07:33 AM, Dave Young wrote:
On 01/05/17 at 11:01pm, Hari Bathini wrote:
Get rid of multiple definitions of append_elf_note() & final_note()
functions. Reuse these functions compiled under CONFIG_CRASH_CORE
Also, define Elf_Word and use it instead of generic u32 or the more
specific Elf64_Word.
Signed-off-by: Hari Bathini
---
Changes from v3:
* Dropped hard-coded values and used DIV_ROUND_UP().
Changes from v2:
* Added a definition for Elf_Word.
* Used IA64 version of append_elf_note() and final_note() functions.
arch/ia64/kernel/crash.c | 22 --
include/linux/crash_core.h |4
include/linux/elf.h|2 ++
kernel/crash_core.c| 34 ++
kernel/kexec_core.c| 28
5 files changed, 20 insertions(+), 70 deletions(-)
diff --git a/arch/ia64/kernel/crash.c b/arch/ia64/kernel/crash.c
index 2955f35..75859a0 100644
--- a/arch/ia64/kernel/crash.c
+++ b/arch/ia64/kernel/crash.c
@@ -27,28 +27,6 @@ static int kdump_freeze_monarch;
static int kdump_on_init = 1;
static int kdump_on_fatal_mca = 1;
-static inline Elf64_Word
-*append_elf_note(Elf64_Word *buf, char *name, unsigned type, void *data,
- size_t data_len)
-{
- struct elf_note *note = (struct elf_note *)buf;
- note->n_namesz = strlen(name) + 1;
- note->n_descsz = data_len;
- note->n_type = type;
- buf += (sizeof(*note) + 3)/4;
- memcpy(buf, name, note->n_namesz);
- buf += (note->n_namesz + 3)/4;
- memcpy(buf, data, data_len);
- buf += (data_len + 3)/4;
- return buf;
-}
-
-static void
-final_note(void *buf)
-{
- memset(buf, 0, sizeof(struct elf_note));
-}
-
extern void ia64_dump_cpu_regs(void *);
static DEFINE_PER_CPU(struct elf_prstatus, elf_prstatus);
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 18d0f94..541a197 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -55,6 +55,10 @@ extern u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4];
extern size_t vmcoreinfo_size;
extern size_t vmcoreinfo_max_size;
+Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
+ void *data, size_t data_len);
+void final_note(Elf_Word *buf);
+
int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
unsigned long long *crash_size, unsigned long long *crash_base);
int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
diff --git a/include/linux/elf.h b/include/linux/elf.h
index 20fa8d8..ba069e8 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -29,6 +29,7 @@ extern Elf32_Dyn _DYNAMIC [];
#define elf_note elf32_note
#define elf_addr_tElf32_Off
#define Elf_Half Elf32_Half
+#define Elf_Word Elf32_Word
#else
@@ -39,6 +40,7 @@ extern Elf64_Dyn _DYNAMIC [];
#define elf_note elf64_note
#define elf_addr_tElf64_Off
#define Elf_Half Elf64_Half
+#define Elf_Word Elf64_Word
#endif
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 80b441d..362dace 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -291,32 +291,26 @@ int __init parse_crashkernel_low(char *cmdline,
"crashkernel=", suffix_tbl[SUFFIX_LOW]);
}
-static u32 *append_elf_note(u32 *buf, char *name, unsigned int type,
- void *data, size_t data_len)
+Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
+ void *data, size_t data_len)
{
- struct elf_note note;
-
- note.n_namesz = strlen(name) + 1;
- note.n_descsz = data_len;
- note.n_type = type;
- memcpy(buf, ¬e, sizeof(note));
- buf += (sizeof(note) + 3)/4;
- memcpy(buf, name, note.n_namesz);
- buf += (note.n_namesz + 3)/4;
- memcpy(buf, data, note.n_descsz);
- buf += (note.n_descsz + 3)/4;
+ struct elf_note *note = (struct elf_note *)buf;
+
+ note->n_namesz = strlen(name) + 1;
+ note->n_descsz = data_len;
+ note->n_type = type;
+ buf += DIV_ROUND_UP(sizeof(*note), sizeof(Elf_Word));
+ memcpy(buf, name, note->n_namesz);
+ buf += DIV_ROUND_UP(note->n_namesz, sizeof(Elf_Word));
+ memcpy(buf, data, data_len);
+ buf += DIV_ROUND_UP(data_len, sizeof(Elf_Word));
return buf;
}
-static void final_note(u32 *buf)
+void final_note(Elf_Word *buf)
{
- struct elf_note note;
-
- note.n_namesz = 0;
- note.n_descsz = 0;
- note.n_type = 0;
- memcpy(buf, ¬e, sizeof(note));
+ memset(buf, 0, sizeof(struct elf_note));
}
static void update_vmcoreinfo_note(void)
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 2179a16..263d764 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -990,34 +990,6 @@ int crash_sh
Re: Normal service will resume shortly ...
Welcome Back! I believe that the following patch set should go into 4.10: https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-December/152079.html "powerpc/devtree: Add support for 2 new DRC properties" Would you please take a look at it? Michael On 01/16/2017 03:33 AM, Michael Ellerman wrote: > Hi folks, > > I'm back from paternity leave, and will start processing patches in the > next day or two. > > If you've sent a fix that should go into 4.10 that hasn't been merged > yet, please feel free to reply to this message giving me a pointer to > it. > > cheers > > -- Michael W. Bringmann Linux Technology Center IBM Corporation Tie-Line 363-5196 External: (512) 286-5196 Cell: (512) 466-0650 m...@linux.vnet.ibm.com
Re: [PATCH] powerpc: Use octal numbers for file permissions
It has been pointed out that this actually occured in 2017. My apologies. On 17/01/2017 9:50 PM, "Oliver O'Halloran" wrote: > "It's possible I missed one, but I did genuinely review all of it" > > Cyril Bur, 2016 > In a hobart pub, specifically The Winston > > On 17/01/2017 8:53 PM, "Michael Ellerman" wrote: > >> Cyril Bur writes: >> >> > On Thu, 2017-01-12 at 14:54 +1100, Russell Currey wrote: >> >> Symbolic macros are unintuitive and hard to read, whereas octal >> constants >> >> are much easier to interpret. Replace macros for the basic permission >> >> flags (user/group/other read/write/execute) with numeric constants >> >> instead, across the whole powerpc tree. >> >> >> >> Introducing a significant number of changes across the tree for no >> runtime >> >> benefit isn't exactly desirable, but so long as these macros are still >> >> used in the tree people will keep sending patches that add them. Not >> only >> >> are they hard to parse at a glance, there are multiple ways of coming >> to >> >> the same value (as you can see with 0444 and 0644 in this patch) which >> >> hurts readability. >> >> >> >> Signed-off-by: Russell Currey >> > >> > Reviewed-by: Cyril Bur >> >> Did you really really review every single change? >> >> Because if you did then I don't have to, and that would be *great* :) >> >> cheers >> >
Re: [PATCH] powerpc: Use octal numbers for file permissions
"It's possible I missed one, but I did genuinely review all of it" Cyril Bur, 2016 In a hobart pub, specifically The Winston On 17/01/2017 8:53 PM, "Michael Ellerman" wrote: > Cyril Bur writes: > > > On Thu, 2017-01-12 at 14:54 +1100, Russell Currey wrote: > >> Symbolic macros are unintuitive and hard to read, whereas octal > constants > >> are much easier to interpret. Replace macros for the basic permission > >> flags (user/group/other read/write/execute) with numeric constants > >> instead, across the whole powerpc tree. > >> > >> Introducing a significant number of changes across the tree for no > runtime > >> benefit isn't exactly desirable, but so long as these macros are still > >> used in the tree people will keep sending patches that add them. Not > only > >> are they hard to parse at a glance, there are multiple ways of coming to > >> the same value (as you can see with 0444 and 0644 in this patch) which > >> hurts readability. > >> > >> Signed-off-by: Russell Currey > > > > Reviewed-by: Cyril Bur > > Did you really really review every single change? > > Because if you did then I don't have to, and that would be *great* :) > > cheers >
Re: [PATCH] powerpc: Use octal numbers for file permissions
Cyril Bur writes: > On Thu, 2017-01-12 at 14:54 +1100, Russell Currey wrote: >> Symbolic macros are unintuitive and hard to read, whereas octal constants >> are much easier to interpret. Replace macros for the basic permission >> flags (user/group/other read/write/execute) with numeric constants >> instead, across the whole powerpc tree. >> >> Introducing a significant number of changes across the tree for no runtime >> benefit isn't exactly desirable, but so long as these macros are still >> used in the tree people will keep sending patches that add them. Not only >> are they hard to parse at a glance, there are multiple ways of coming to >> the same value (as you can see with 0444 and 0644 in this patch) which >> hurts readability. >> >> Signed-off-by: Russell Currey > > Reviewed-by: Cyril Bur Did you really really review every single change? Because if you did then I don't have to, and that would be *great* :) cheers
Re: Normal service will resume shortly ...
Hi Michael, Welcome back!! Can you please take a look at: https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-November/151426.html https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-November/150452.html Thanks, Ravi On Monday 16 January 2017 03:03 PM, Michael Ellerman wrote: > Hi folks, > > I'm back from paternity leave, and will start processing patches in the > next day or two. > > If you've sent a fix that should go into 4.10 that hasn't been merged > yet, please feel free to reply to this message giving me a pointer to > it. > > cheers >
