Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()

2023-09-23 Thread Song Liu
On Sat, Sep 23, 2023 at 8:39 AM Mike Rapoport  wrote:
>
> On Thu, Sep 21, 2023 at 03:34:18PM -0700, Song Liu wrote:
> > On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport  wrote:
> > >
> >
> > [...]
> >
> > > diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> > > index 42215f9404af..db5561d0c233 100644
> > > --- a/arch/s390/kernel/module.c
> > > +++ b/arch/s390/kernel/module.c
> > > @@ -21,6 +21,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -76,7 +77,7 @@ void *module_alloc(unsigned long size)
> > >  #ifdef CONFIG_FUNCTION_TRACER
> > >  void module_arch_cleanup(struct module *mod)
> > >  {
> > > -   module_memfree(mod->arch.trampolines_start);
> > > +   execmem_free(mod->arch.trampolines_start);
> > >  }
> > >  #endif
> > >
> > > @@ -510,7 +511,7 @@ static int 
> > > module_alloc_ftrace_hotpatch_trampolines(struct module *me,
> > >
> > > size = FTRACE_HOTPATCH_TRAMPOLINES_SIZE(s->sh_size);
> > > numpages = DIV_ROUND_UP(size, PAGE_SIZE);
> > > -   start = module_alloc(numpages * PAGE_SIZE);
> > > +   start = execmem_text_alloc(EXECMEM_FTRACE, numpages * PAGE_SIZE);
> >
> > This should be EXECMEM_MODULE_TEXT?
>
> This is an ftrace trampoline, so I think it should be FTRACE type of
> allocation.

Yeah, I was aware of the ftrace trampoline. My point was, ftrace trampoline
doesn't seem to have any special requirements. Therefore, it is probably not
necessary to have a separate type just for it.

AFAICT, kprobe, ftrace, and BPF (JIT and trampoline) can share the same
execmem_type. We may need some work for some archs, but nothing is
fundamentally different among these.

Thanks,
Song



Re: [PATCH] ARM: dts: qcom: sdx65-mtp: Specify PM7250B SID to use

2023-09-23 Thread Bjorn Andersson


On Thu, 21 Sep 2023 08:34:02 +0200, Luca Weiss wrote:
> Now that the pm7250b.dtsi can be configured to be on a different SID, we
> also need to specify it for this dts file. Set it to the SID 2/3 like it
> was before commit 8e2d56f64572 ("arm64: dts: qcom: pm7250b: make SID
> configurable").
> 
> 

Applied, thanks!

[1/1] ARM: dts: qcom: sdx65-mtp: Specify PM7250B SID to use
  commit: 4d8b5d7171722d2cdccc880d8e449f7ca9c7b6bf

Best regards,
-- 
Bjorn Andersson 


Re: [PATCH] ARM: dts: qcom: msm8226: provide dsi phy clocks to mmcc

2023-09-23 Thread Dmitry Baryshkov
On Wed, 12 Jul 2023 at 10:53, Luca Weiss  wrote:
>
> Some mmcc clocks have dsi0pll & dsi0pllbyte as clock parents so we
> should provide them in the dt, which I missed in the commit adding the
> mdss nodes.
>
> Fixes: d5fb01ad5eb4 ("ARM: dts: qcom: msm8226: Add mdss nodes")
> Signed-off-by: Luca Weiss 
> ---
>  arch/arm/boot/dts/qcom/qcom-msm8226.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Dmitry Baryshkov 

-- 
With best wishes
Dmitry


Re: [PATCH v2] ARM: dts: qcom: msm8974: correct qfprom node size

2023-09-23 Thread Dmitry Baryshkov
On Fri, 22 Sept 2023 at 19:57, Luca Weiss  wrote:
>
> On Sonntag, 6. August 2023 12:47:51 CEST Luca Weiss wrote:
> > Hi Bjorn,
> >
> > On Montag, 31. Juli 2023 23:45:21 CEST Bjorn Andersson wrote:
> > > On Thu, Jun 15, 2023 at 08:20:41PM +0200, Konrad Dybcio wrote:
> > > > On 15.06.2023 20:17, Luca Weiss wrote:
> > > > > From: Craig Tatlor 
> > > > >
> > > > > The qfprom actually has size 0x3000, so adjust the reg.
> > > > >
> > > > > Note that the non-ECC-corrected qfprom can be found at 0xfc4b8000
> > > > > (-0x4000). The current reg points to the ECC-corrected qfprom block
> > > > > which should have equivalent values at all offsets compared to the
> > > > > non-corrected version.
> > > > >
> > > > > [l...@z3ntu.xyz: extract to standalone patch and adjust for review
> > > > > comments]
> > > > >
> > > > > Fixes: c59ffb519357 ("arm: dts: msm8974: Add thermal zones, tsens and
> > > > > qfprom nodes") Signed-off-by: Craig Tatlor 
> > > > > Signed-off-by: Luca Weiss 
> > > > > ---
> > > >
> > > > Not sure of the actual size of the region, maybe Bjorn can help..
> > > >
> > > > Downstream 3.10 suggests 0x60F0, 0x20F0 after adjusting for the ECC
> > > > offset
> > >
> > > There is indeed 0x3000 bytes until the next region, but afaict the
> > > corrected ECC values only cover the first 0x800 bytes thereof.
> > >
> > > Can you please let me know if this patch fixes a problem, or just
> > > makes the numbers look better?
> >
> > Initially this patch came from a different direction, to make space to use
> > the PVS bits for cpufreq. Since Konrad said in earlier revisions that I
> > should always use the +0x4000 space for the ECC-corrected variant I've
> > switched to that.
> >
> > If you think it's not useful to have the qfprom size reflect the actual
> > size, we can also drop this patch since I don't think it's actually
> > necessary for anything that I have lying around in some branches.
> >
> > I think I've just sent the current patch to make sure the hardware
> > description (dts) is as accurate as possible, but of course since any info
> > on Qualcomm is very restricted it could also be a bit wrong.
>
> Hi Bjorn,
>
> this patch is still lying in my inbox. Do you think it's correct or incorrect
> - so should we drop it?

There are JTAG and coresight fuses at 0xfc4be024. So, I think, the
regions should be extended to 0x20f0 or 0x2100. BTW: could you please
also fix msm8974 and apq8084 in a similar way?

>
> Regards
> Luca
>
> >
> > Regards
> > Luca
> >
> > > Regards,
> > > Bjorn
> > >
> > > > Konrad
> > > >
> > > > > Changes in v2:
> > > > > - Keep base offset but expand reg from 0x1000 to 0x3000 (Konrad)
> > > > > - Link to v1:
> > > > > https://lore.kernel.org/r/20230130-msm8974-qfprom-v1-1-975aa0e5e083@z3
> > > > > n
> > > > > tu.xyz ---
> > > > >
> > > > >  arch/arm/boot/dts/qcom-msm8974.dtsi | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi
> > > > > b/arch/arm/boot/dts/qcom-msm8974.dtsi index 7ed0d925a4e9..3156fe25967f
> > > > > 100644
> > > > > --- a/arch/arm/boot/dts/qcom-msm8974.dtsi
> > > > > +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
> > > > > @@ -1194,7 +1194,7 @@ restart@fc4ab000 {
> > > > >
> > > > > qfprom: qfprom@fc4bc000 {
> > > > >
> > > > > compatible = "qcom,msm8974-qfprom",
> >
> > "qcom,qfprom";
> >
> > > > > -   reg = <0xfc4bc000 0x1000>;
> > > > > +   reg = <0xfc4bc000 0x3000>;
> > > > >
> > > > > #address-cells = <1>;
> > > > > #size-cells = <1>;
> > > > >
> > > > > ---
> > > > > base-commit: 858fd168a95c5b9669aac8db6c14a9aeab446375
> > > > > change-id: 20230130-msm8974-qfprom-619c0e8f26eb
> > > > >
> > > > > Best regards,
>
>
>
>


-- 
With best wishes
Dmitry


Re: [PATCH v2 3/3] ARM: dts: qcom: msm8226: Add blsp1_i2c6 and blsp1_uart2

2023-09-23 Thread Dmitry Baryshkov
On Fri, 22 Sept 2023 at 19:56, Luca Weiss  wrote:
>
> Add more busses found on msm8226 SoC.
>
> Signed-off-by: Luca Weiss 
> ---
>  arch/arm/boot/dts/qcom/qcom-msm8226.dtsi | 33 
> 
>  1 file changed, 33 insertions(+)
>

Reviewed-by: Dmitry Baryshkov 

-- 
With best wishes
Dmitry


Re: [PATCH v2 2/3] pinctrl: qcom: msm8226: Add blsp_i2c6 function

2023-09-23 Thread Dmitry Baryshkov
On Fri, 22 Sept 2023 at 19:56, Luca Weiss  wrote:
>
> On GPIO22 and GPIO23 there is another I2C bus. Add the function for it.
>
> Signed-off-by: Luca Weiss 
> ---
>  drivers/pinctrl/qcom/pinctrl-msm8226.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>

Reviewed-by: Dmitry Baryshkov 

-- 
With best wishes
Dmitry


Re: [PATCH v1] samples: kprobes: Fixes a typo

2023-09-23 Thread Atul Kumar Pant
On Thu, Aug 17, 2023 at 10:38:19PM +0530, Atul Kumar Pant wrote:
> Fixes typo in a function name.
> 
> Signed-off-by: Atul Kumar Pant 
> ---
>  samples/kprobes/kretprobe_example.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/samples/kprobes/kretprobe_example.c 
> b/samples/kprobes/kretprobe_example.c
> index cbf16542d84e..ed79fd3d48fb 100644
> --- a/samples/kprobes/kretprobe_example.c
> +++ b/samples/kprobes/kretprobe_example.c
> @@ -35,7 +35,7 @@ struct my_data {
>   ktime_t entry_stamp;
>  };
>  
> -/* Here we use the entry_hanlder to timestamp function entry */
> +/* Here we use the entry_handler to timestamp function entry */
>  static int entry_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
>  {
>   struct my_data *data;
> -- 
> 2.25.1
> 

Hi all, can someone provide comments on this change.



Re: [PATCH v3 09/13] powerpc: extend execmem_params for kprobes allocations

2023-09-23 Thread Mike Rapoport
Hi Christophe,

On Fri, Sep 22, 2023 at 10:32:46AM +, Christophe Leroy wrote:
> Hi Mike,
> 
> Le 18/09/2023 à 09:29, Mike Rapoport a écrit :
> > From: "Mike Rapoport (IBM)" 
> > 
> > powerpc overrides kprobes::alloc_insn_page() to remove writable
> > permissions when STRICT_MODULE_RWX is on.
> > 
> > Add definition of EXECMEM_KRPOBES to execmem_params to allow using the
> > generic kprobes::alloc_insn_page() with the desired permissions.
> > 
> > As powerpc uses breakpoint instructions to inject kprobes, it does not
> > need to constrain kprobe allocations to the modules area and can use the
> > entire vmalloc address space.
> 
> I don't understand what you mean here. Does it mean kprobe allocation 
> doesn't need to be executable ? I don't think so based on the pgprot you 
> set.
> 
> On powerpc book3s/32, vmalloc space is not executable. Only modules 
> space is executable. X/NX cannot be set on a per page basis, it can only 
> be set on a 256 Mbytes segment basis.
> 
> See commit c49643319715 ("powerpc/32s: Only leave NX unset on segments 
> used for modules") and 6ca055322da8 ("powerpc/32s: Use dedicated segment 
> for modules with STRICT_KERNEL_RWX") and 7bee31ad8e2f ("powerpc/32s: Fix 
> is_module_segment() when MODULES_VADDR is defined").
> 
> So if your intention is still to have an executable kprobes, then you 
> can't use vmalloc address space.

Right, and I've fixed the KPROBES range to uses the same range as MODULES.
The commit message is stale and I need to update it.
 
> Christophe
> 
> > 
> > Signed-off-by: Mike Rapoport (IBM) 
> > ---
> >   arch/powerpc/kernel/kprobes.c | 14 --
> >   arch/powerpc/kernel/module.c  | 11 +++
> >   2 files changed, 11 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> > index 62228c7072a2..14c5ddec3056 100644
> > --- a/arch/powerpc/kernel/kprobes.c
> > +++ b/arch/powerpc/kernel/kprobes.c
> > @@ -126,20 +126,6 @@ kprobe_opcode_t *arch_adjust_kprobe_addr(unsigned long 
> > addr, unsigned long offse
> > return (kprobe_opcode_t *)(addr + offset);
> >   }
> >   
> > -void *alloc_insn_page(void)
> > -{
> > -   void *page;
> > -
> > -   page = execmem_text_alloc(EXECMEM_KPROBES, PAGE_SIZE);
> > -   if (!page)
> > -   return NULL;
> > -
> > -   if (strict_module_rwx_enabled())
> > -   set_memory_rox((unsigned long)page, 1);
> > -
> > -   return page;
> > -}
> > -
> >   int arch_prepare_kprobe(struct kprobe *p)
> >   {
> > int ret = 0;
> > diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
> > index 824d9541a310..bf2c62aef628 100644
> > --- a/arch/powerpc/kernel/module.c
> > +++ b/arch/powerpc/kernel/module.c
> > @@ -95,6 +95,9 @@ static struct execmem_params execmem_params 
> > __ro_after_init = {
> > [EXECMEM_DEFAULT] = {
> > .alignment = 1,
> > },
> > +   [EXECMEM_KPROBES] = {
> > +   .alignment = 1,
> > +   },
> > [EXECMEM_MODULE_DATA] = {
> > .alignment = 1,
> > },
> > @@ -135,5 +138,13 @@ struct execmem_params __init *execmem_arch_params(void)
> >   
> > range->pgprot = prot;
> >   
> > +   execmem_params.ranges[EXECMEM_KPROBES].start = VMALLOC_START;
> > +   execmem_params.ranges[EXECMEM_KPROBES].start = VMALLOC_END;
> > +
> > +   if (strict_module_rwx_enabled())
> > +   execmem_params.ranges[EXECMEM_KPROBES].pgprot = PAGE_KERNEL_ROX;
> > +   else
> > +   execmem_params.ranges[EXECMEM_KPROBES].pgprot = 
> > PAGE_KERNEL_EXEC;
> > +
> > return _params;
> >   }

-- 
Sincerely yours,
Mike.



Re: [PATCH v3 09/13] powerpc: extend execmem_params for kprobes allocations

2023-09-23 Thread Mike Rapoport
On Thu, Sep 21, 2023 at 03:30:46PM -0700, Song Liu wrote:
> On Mon, Sep 18, 2023 at 12:31 AM Mike Rapoport  wrote:
> >
> [...]
> > @@ -135,5 +138,13 @@ struct execmem_params __init *execmem_arch_params(void)
> >
> > range->pgprot = prot;
> >
> > +   execmem_params.ranges[EXECMEM_KPROBES].start = VMALLOC_START;
> > +   execmem_params.ranges[EXECMEM_KPROBES].start = VMALLOC_END;
> 
> .end = VMALLOC_END.

Thanks, this should have been

execmem_params.ranges[EXECMEM_KPROBES].start = range->start;
execmem_params.ranges[EXECMEM_KPROBES].end = range->end;

where range points to the same range as EXECMEM_MODULE_TEXT.

 
> Thanks,
> Song
> 
> > +
> > +   if (strict_module_rwx_enabled())
> > +   execmem_params.ranges[EXECMEM_KPROBES].pgprot = 
> > PAGE_KERNEL_ROX;
> > +   else
> > +   execmem_params.ranges[EXECMEM_KPROBES].pgprot = 
> > PAGE_KERNEL_EXEC;
> > +
> > return _params;
> >  }
> > --
> > 2.39.2
> >
> >

-- 
Sincerely yours,
Mike.



Re: [PATCH v3 08/13] riscv: extend execmem_params for generated code allocations

2023-09-23 Thread Mike Rapoport
On Fri, Sep 22, 2023 at 12:37:07PM +0200, Alexandre Ghiti wrote:
> Hi Mike,
> 
> On 18/09/2023 09:29, Mike Rapoport wrote:
> > From: "Mike Rapoport (IBM)" 
> > 
> > The memory allocations for kprobes and BPF on RISC-V are not placed in
> > the modules area and these custom allocations are implemented with
> > overrides of alloc_insn_page() and  bpf_jit_alloc_exec().
> > 
> > Slightly reorder execmem_params initialization to support both 32 and 64
> > bit variants, define EXECMEM_KPROBES and EXECMEM_BPF ranges in
> > riscv::execmem_params and drop overrides of alloc_insn_page() and
> > bpf_jit_alloc_exec().
> > 
> > Signed-off-by: Mike Rapoport (IBM) 
> > ---
> >   arch/riscv/kernel/module.c | 21 -
> >   arch/riscv/kernel/probes/kprobes.c | 10 --
> >   arch/riscv/net/bpf_jit_core.c  | 13 -
> >   3 files changed, 20 insertions(+), 24 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> > index 343a0edfb6dd..31505ecb5c72 100644
> > --- a/arch/riscv/kernel/module.c
> > +++ b/arch/riscv/kernel/module.c
> > @@ -436,20 +436,39 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char 
> > *strtab,
> > return 0;
> >   }
> > -#if defined(CONFIG_MMU) && defined(CONFIG_64BIT)
> > +#ifdef CONFIG_MMU
> >   static struct execmem_params execmem_params __ro_after_init = {
> > .ranges = {
> > [EXECMEM_DEFAULT] = {
> > .pgprot = PAGE_KERNEL,
> > .alignment = 1,
> > },
> > +   [EXECMEM_KPROBES] = {
> > +   .pgprot = PAGE_KERNEL_READ_EXEC,
> > +   .alignment = 1,
> > +   },
> > +   [EXECMEM_BPF] = {
> > +   .pgprot = PAGE_KERNEL,
> > +   .alignment = 1,
> 
> 
> Not entirely sure it is the same alignment (sorry did not go through the
> entire series), but if it is, the alignment above ^ is not the same that is
> requested by our current bpf_jit_alloc_exec() implementation which is
> PAGE_SIZE.
 
This literally translates vmalloc() in alloc_insn_page() to a set of
parameters, so "1" comes from there. And using alignment of 1 with
vmalloc() implicitly sets it to PAGE_SIZE.

> > +   },
> > },
> >   };
> >   struct execmem_params __init *execmem_arch_params(void)
> >   {
> > +#ifdef CONFIG_64BIT
> > execmem_params.ranges[EXECMEM_DEFAULT].start = MODULES_VADDR;
> > execmem_params.ranges[EXECMEM_DEFAULT].end = MODULES_END;
> > +#else
> > +   execmem_params.ranges[EXECMEM_DEFAULT].start = VMALLOC_START;
> > +   execmem_params.ranges[EXECMEM_DEFAULT].end = VMALLOC_END;
> > +#endif
> > +
> > +   execmem_params.ranges[EXECMEM_KPROBES].start = VMALLOC_START;
> > +   execmem_params.ranges[EXECMEM_KPROBES].end = VMALLOC_END;
> > +
> > +   execmem_params.ranges[EXECMEM_BPF].start = BPF_JIT_REGION_START;
> > +   execmem_params.ranges[EXECMEM_BPF].end = BPF_JIT_REGION_END;
> > return _params;
> >   }
> > diff --git a/arch/riscv/kernel/probes/kprobes.c 
> > b/arch/riscv/kernel/probes/kprobes.c
> > index 2f08c14a933d..e64f2f3064eb 100644
> > --- a/arch/riscv/kernel/probes/kprobes.c
> > +++ b/arch/riscv/kernel/probes/kprobes.c
> > @@ -104,16 +104,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > return 0;
> >   }
> > -#ifdef CONFIG_MMU
> > -void *alloc_insn_page(void)
> > -{
> > -   return  __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> > -GFP_KERNEL, PAGE_KERNEL_READ_EXEC,
> > -VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
> > -__builtin_return_address(0));
> > -}
> > -#endif
> > -
> >   /* install breakpoint in text */
> >   void __kprobes arch_arm_kprobe(struct kprobe *p)
> >   {
> > diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
> > index 7b70ccb7fec3..c8a758f0882b 100644
> > --- a/arch/riscv/net/bpf_jit_core.c
> > +++ b/arch/riscv/net/bpf_jit_core.c
> > @@ -218,19 +218,6 @@ u64 bpf_jit_alloc_exec_limit(void)
> > return BPF_JIT_REGION_SIZE;
> >   }
> > -void *bpf_jit_alloc_exec(unsigned long size)
> > -{
> > -   return __vmalloc_node_range(size, PAGE_SIZE, BPF_JIT_REGION_START,
> > -   BPF_JIT_REGION_END, GFP_KERNEL,
> > -   PAGE_KERNEL, 0, NUMA_NO_NODE,
> > -   __builtin_return_address(0));
> > -}
> > -
> > -void bpf_jit_free_exec(void *addr)
> > -{
> > -   return vfree(addr);
> > -}
> > -
> >   void *bpf_arch_text_copy(void *dst, void *src, size_t len)
> >   {
> > int ret;
> 
> 
> Otherwise, you can add:
> 
> Reviewed-by: Alexandre Ghiti 
> 
> Thanks,
> 
> Alex
> 
> 

-- 
Sincerely yours,
Mike.



Re: [PATCH v3 06/13] mm/execmem: introduce execmem_data_alloc()

2023-09-23 Thread Mike Rapoport
On Thu, Sep 21, 2023 at 03:52:21PM -0700, Song Liu wrote:
> On Mon, Sep 18, 2023 at 12:31 AM Mike Rapoport  wrote:
> >
> [...]
> > diff --git a/include/linux/execmem.h b/include/linux/execmem.h
> > index 519bdfdca595..09d45ac786e9 100644
> > --- a/include/linux/execmem.h
> > +++ b/include/linux/execmem.h
> > @@ -29,6 +29,7 @@
> >   * @EXECMEM_KPROBES: parameters for kprobes
> >   * @EXECMEM_FTRACE: parameters for ftrace
> >   * @EXECMEM_BPF: parameters for BPF
> > + * @EXECMEM_MODULE_DATA: parameters for module data sections
> >   * @EXECMEM_TYPE_MAX:
> >   */
> >  enum execmem_type {
> > @@ -37,6 +38,7 @@ enum execmem_type {
> > EXECMEM_KPROBES,
> > EXECMEM_FTRACE,
> 
> In longer term, I think we can improve the JITed code and merge
> kprobe/ftrace/bpf. to use the same ranges. Also, do we need special
> setting for FTRACE? If not, let's just remove it.

I don't think we need to limit how the JITed code is generated because we
want to support fewer address space ranges for it. 

As for FTRACE, now it's only needed on x86 and s390 and there it happens
to use the same ranges as MODULES and the rest, but it still gives some
notion of potential semantic differences and the overhead of keeping it is
really negligible.
 
> > EXECMEM_BPF,
> > +   EXECMEM_MODULE_DATA,
> > EXECMEM_TYPE_MAX,
> >  };
> 
> Overall, it is great that kprobe/ftrace/bpf no longer depend on modules.
> 
> OTOH, I think we should merge execmem_type and existing mod_mem_type.
> Otherwise, we still need to handle page permissions in multiple places.
> What is our plan for that?

Maybe, but I think this is too early. There are several things missing
before we could remove set_memory usage from modules. E.g. to use ROX
allocations on x86 we at least should update alternatives handling and
reach a consensus about synchronization Andy mentioned in his comments to
v2.
 
> Thanks,
> Song
> 
> 
> >
> > @@ -107,6 +109,23 @@ struct execmem_params *execmem_arch_params(void);
> >   */
> >  void *execmem_text_alloc(enum execmem_type type, size_t size);
> >
> > +/**
> > + * execmem_data_alloc - allocate memory for data coupled to code
> > + * @type: type of the allocation
> > + * @size: how many bytes of memory are required
> > + *
> > + * Allocates memory that will contain data coupled with executable code,
> > + * like data sections in kernel modules.
> > + *
> > + * The memory will have protections defined by architecture.
> > + *
> > + * The allocated memory will reside in an area that does not impose
> > + * restrictions on the addressing modes.
> > + *
> > + * Return: a pointer to the allocated memory or %NULL
> > + */
> > +void *execmem_data_alloc(enum execmem_type type, size_t size);
> > +
> >  /**
> >   * execmem_free - free executable memory
> >   * @ptr: pointer to the memory that should be freed
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index c4146bfcd0a7..2ae83a6abf66 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -1188,25 +1188,16 @@ void __weak module_arch_freeing_init(struct module 
> > *mod)
> >  {
> >  }
> >
> > -static bool mod_mem_use_vmalloc(enum mod_mem_type type)
> > -{
> > -   return IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) &&
> > -   mod_mem_type_is_core_data(type);
> > -}
> > -
> >  static void *module_memory_alloc(unsigned int size, enum mod_mem_type type)
> >  {
> > -   if (mod_mem_use_vmalloc(type))
> > -   return vzalloc(size);
> > +   if (mod_mem_type_is_data(type))
> > +   return execmem_data_alloc(EXECMEM_MODULE_DATA, size);
> > return execmem_text_alloc(EXECMEM_MODULE_TEXT, size);
> >  }
> >
> >  static void module_memory_free(void *ptr, enum mod_mem_type type)
> >  {
> > -   if (mod_mem_use_vmalloc(type))
> > -   vfree(ptr);
> > -   else
> > -   execmem_free(ptr);
> > +   execmem_free(ptr);
> >  }
> >
> >  static void free_mod_mem(struct module *mod)
> > diff --git a/mm/execmem.c b/mm/execmem.c
> > index abcbd07e05ac..aeff85261360 100644
> > --- a/mm/execmem.c
> > +++ b/mm/execmem.c
> > @@ -53,11 +53,23 @@ static void *execmem_alloc(size_t size, struct 
> > execmem_range *range)
> > return kasan_reset_tag(p);
> >  }
> >
> > +static inline bool execmem_range_is_data(enum execmem_type type)
> > +{
> > +   return type == EXECMEM_MODULE_DATA;
> > +}
> > +
> >  void *execmem_text_alloc(enum execmem_type type, size_t size)
> >  {
> > return execmem_alloc(size, _params.ranges[type]);
> >  }
> >
> > +void *execmem_data_alloc(enum execmem_type type, size_t size)
> > +{
> > +   WARN_ON_ONCE(!execmem_range_is_data(type));
> > +
> > +   return execmem_alloc(size, _params.ranges[type]);
> > +}
> > +
> >  void execmem_free(void *ptr)
> >  {
> > /*
> > @@ -93,7 +105,10 @@ static void execmem_init_missing(struct execmem_params 
> > *p)
> > struct execmem_range *r = >ranges[i];
> >
> > 

Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()

2023-09-23 Thread Mike Rapoport
On Thu, Sep 21, 2023 at 03:10:26PM -0700, Song Liu wrote:
> On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport  wrote:
> >
> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +static void *execmem_alloc(size_t size)
> > +{
> > +   return module_alloc(size);
> > +}
> > +
> > +void *execmem_text_alloc(enum execmem_type type, size_t size)
> > +{
> > +   return execmem_alloc(size);
> > +}
> 
> execmem_text_alloc (and later execmem_data_alloc) both take "type" as
> input. I guess we can just use execmem_alloc(type, size) for everything?

We could but I still prefer to keep this distinction.
 
> Thanks,
> Song
> 
> > +
> > +void execmem_free(void *ptr)
> > +{
> > +   /*
> > +* This memory may be RO, and freeing RO memory in an interrupt is 
> > not
> > +* supported by vmalloc.
> > +*/
> > +   WARN_ON(in_interrupt());
> > +   vfree(ptr);
> > +}
> > --
> > 2.39.2
> >

-- 
Sincerely yours,
Mike.



Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()

2023-09-23 Thread Mike Rapoport
On Thu, Sep 21, 2023 at 03:14:54PM -0700, Song Liu wrote:
> On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport  wrote:
> >
> [...]
> > +
> > +/**
> > + * enum execmem_type - types of executable memory ranges
> > + *
> > + * There are several subsystems that allocate executable memory.
> > + * Architectures define different restrictions on placement,
> > + * permissions, alignment and other parameters for memory that can be used
> > + * by these subsystems.
> > + * Types in this enum identify subsystems that allocate executable memory
> > + * and let architectures define parameters for ranges suitable for
> > + * allocations by each subsystem.
> > + *
> > + * @EXECMEM_DEFAULT: default parameters that would be used for types that
> > + * are not explcitly defined.
> > + * @EXECMEM_MODULE_TEXT: parameters for module text sections
> > + * @EXECMEM_KPROBES: parameters for kprobes
> > + * @EXECMEM_FTRACE: parameters for ftrace
> > + * @EXECMEM_BPF: parameters for BPF
> > + * @EXECMEM_TYPE_MAX:
> > + */
> > +enum execmem_type {
> > +   EXECMEM_DEFAULT,
> 
> I found EXECMEM_DEFAULT more confusing than helpful.

I hesitated a lot about that, but in the end decided to have
EXECMEM_DEFAULT and alias EXECMEM_MODULE_TEXT to it because this is what we
essentially have now for the most architectures.

If you'll take a look at arch-specific patches, in many cases there is only
EXECMEM_DEFAULT that an architecture defines and that default is used by
all the subsystems.
 
> Song
> 
> > +   EXECMEM_MODULE_TEXT = EXECMEM_DEFAULT,
> > +   EXECMEM_KPROBES,
> > +   EXECMEM_FTRACE,
> > +   EXECMEM_BPF,
> > +   EXECMEM_TYPE_MAX,
> > +};
> > +
> [...]

-- 
Sincerely yours,
Mike.



Re: [PATCH v3 02/13] mm: introduce execmem_text_alloc() and execmem_free()

2023-09-23 Thread Mike Rapoport
On Thu, Sep 21, 2023 at 03:34:18PM -0700, Song Liu wrote:
> On Mon, Sep 18, 2023 at 12:30 AM Mike Rapoport  wrote:
> >
> 
> [...]
> 
> > diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> > index 42215f9404af..db5561d0c233 100644
> > --- a/arch/s390/kernel/module.c
> > +++ b/arch/s390/kernel/module.c
> > @@ -21,6 +21,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -76,7 +77,7 @@ void *module_alloc(unsigned long size)
> >  #ifdef CONFIG_FUNCTION_TRACER
> >  void module_arch_cleanup(struct module *mod)
> >  {
> > -   module_memfree(mod->arch.trampolines_start);
> > +   execmem_free(mod->arch.trampolines_start);
> >  }
> >  #endif
> >
> > @@ -510,7 +511,7 @@ static int 
> > module_alloc_ftrace_hotpatch_trampolines(struct module *me,
> >
> > size = FTRACE_HOTPATCH_TRAMPOLINES_SIZE(s->sh_size);
> > numpages = DIV_ROUND_UP(size, PAGE_SIZE);
> > -   start = module_alloc(numpages * PAGE_SIZE);
> > +   start = execmem_text_alloc(EXECMEM_FTRACE, numpages * PAGE_SIZE);
> 
> This should be EXECMEM_MODULE_TEXT?

This is an ftrace trampoline, so I think it should be FTRACE type of
allocation.
 
> Thanks,
> Song
> 
> > if (!start)
> > return -ENOMEM;
> > set_memory_rox((unsigned long)start, numpages);
> [...]

-- 
Sincerely yours,
Mike.



Re: [PATCH v2] vmscan: add trace events for lru_gen

2023-09-23 Thread T.J. Mercier
On Thu, Sep 21, 2023 at 7:27 PM 김재원  wrote:
>
> >On Thu, 21 Sep 2023 09:12:30 -0700
> >"T.J. Mercier"  wrote:
> >
> >> > +   TP_fast_assign(
> >> > +   __entry->nid = nid;
> >> > +   __entry->nr_reclaimed = nr_reclaimed;
> >> > +   __entry->nr_dirty = stat->nr_dirty;
> >> > +   __entry->nr_writeback = stat->nr_writeback;
> >> > +   __entry->nr_congested = stat->nr_congested;
> >> > +   __entry->nr_immediate = stat->nr_immediate;
> >> > +   __entry->nr_activate0 = stat->nr_activate[0];
> >> > +   __entry->nr_activate1 = stat->nr_activate[1];
> >> > +   __entry->nr_ref_keep = stat->nr_ref_keep;
> >> > +   __entry->nr_unmap_fail = stat->nr_unmap_fail;
> >> > +   __entry->priority = priority;
> >> > +   __entry->reclaim_flags = trace_reclaim_flags(file);
> >> > +   ),
> >> > +
> >> > +   TP_printk("nid=%d nr_reclaimed=%ld nr_dirty=%ld nr_writeback=%ld 
> >> > nr_congested=%ld nr_immediate=%ld nr_activate_anon=%d 
> >> > nr_activate_file=%d nr_ref_keep=%ld nr_unmap_fail=%ld priority=%d 
> >> > flags=%s",
> >>
> >> Many of these values are unsigned so I think many of these format
> >> specifiers should be %lu instead of %ld.
>
> Hello T.J.
> Thank you for your comment
> As you expected I got this from the legacy lru trace.
> I've changed as you recommended.
> Actually I changed isolate_mode, too. Please let me know if this is not 
> actually needed.
>
Great, looks good to me.
Reviewed-by: T.J. Mercier 

> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
> @@ -367,7 +367,7 @@ TRACE_EVENT_CONDITION(mm_vmscan_lru_gen_scan,
>  * classzone is previous name of the highest_zoneidx.
>  * Reason not to change it is the ABI requirement of the tracepoint.
>  */
> -   TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
> +   TP_printk("isolate_mode=%u classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
> __entry->isolate_mode,
> __entry->highest_zoneidx,
> __entry->order,
> @@ -525,7 +525,7 @@ TRACE_EVENT(mm_vmscan_lru_gen_evict,
> __entry->reclaim_flags = trace_reclaim_flags(file);
> ),
>
> -   TP_printk("nid=%d nr_reclaimed=%ld nr_dirty=%ld nr_writeback=%ld 
> nr_congested=%ld nr_immediate=%ld nr_activate_anon=%d nr_activate_file=%d 
> nr_ref_keep=%ld nr_unmap_fail=%ld priority=%d flags=%s",
> +   TP_printk("nid=%d nr_reclaimed=%lu nr_dirty=%lu nr_writeback=%lu 
> nr_congested=%lu nr_immediate=%lu nr_activate_anon=%u nr_activate_file=%u 
> nr_ref_keep=%lu nr_unmap_fail=%lu priority=%d flags=%s",
> __entry->nid, __entry->nr_reclaimed,
> __entry->nr_dirty, __entry->nr_writeback,
> __entry->nr_congested, __entry->nr_immediate,
>
> >
> >Other than this, from the tracing POV:
> >
> >Reviewed-by: Steven Rostedt (Google) 
>
>
> Hello Steven
> I've appreciated your Reviewed-by, let me take this on my next v3 patch.
>
> >
> >-- Steve



[PATCH v4 2/2] leds: add ktd202x driver

2023-09-23 Thread André Apitzsch
This commit adds support for Kinetic KTD2026/7 RGB/White LED driver.

Signed-off-by: André Apitzsch 
---
 drivers/leds/rgb/Kconfig|  13 +
 drivers/leds/rgb/Makefile   |   1 +
 drivers/leds/rgb/leds-ktd202x.c | 625 
 3 files changed, 639 insertions(+)

diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index 183bccc06cf3..a6a21f564673 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -14,6 +14,19 @@ config LEDS_GROUP_MULTICOLOR
  To compile this driver as a module, choose M here: the module
  will be called leds-group-multicolor.
 
+config LEDS_KTD202X
+   tristate "LED support for KTD202x Chips"
+   depends on I2C
+   depends on OF
+   select REGMAP_I2C
+   help
+ This option enables support for the Kinetic KTD2026/KTD2027
+ RGB/White LED driver found in different BQ mobile phones.
+ It is a 3 or 4 channel LED driver programmed via an I2C interface.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-ktd202x.
+
 config LEDS_PWM_MULTICOLOR
tristate "PWM driven multi-color LED Support"
depends on PWM
diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
index c11cc56384e7..243f31e4d70d 100644
--- a/drivers/leds/rgb/Makefile
+++ b/drivers/leds/rgb/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_LEDS_GROUP_MULTICOLOR)+= leds-group-multicolor.o
+obj-$(CONFIG_LEDS_KTD202X) += leds-ktd202x.o
 obj-$(CONFIG_LEDS_PWM_MULTICOLOR)  += leds-pwm-multicolor.o
 obj-$(CONFIG_LEDS_QCOM_LPG)+= leds-qcom-lpg.o
 obj-$(CONFIG_LEDS_MT6370_RGB)  += leds-mt6370-rgb.o
diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
new file mode 100644
index ..b328ecd34664
--- /dev/null
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -0,0 +1,625 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Kinetic KTD2026/7 RGB/White LED driver with I2C interface
+ *
+ * Copyright 2023 André Apitzsch 
+ *
+ * Datasheet: https://www.kinet-ic.com/uploads/KTD2026-7-04h.pdf
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define KTD2026_NUM_LEDS 3
+#define KTD2027_NUM_LEDS 4
+#define KTD202X_MAX_LEDS 4
+
+/* Register bank */
+#define KTD202X_REG_RESET_CONTROL  0x00
+#define KTD202X_REG_FLASH_PERIOD   0x01
+#define KTD202X_REG_PWM1_TIMER 0x02
+#define KTD202X_REG_PWM2_TIMER 0x03
+#define KTD202X_REG_CHANNEL_CTRL   0x04
+#define KTD202X_REG_TRISE_FALL 0x05
+#define KTD202X_REG_LED_IOUT(x)(0x06 + (x))
+
+/* Register 0 */
+#define KTD202X_TIMER_SLOT_CONTROL_TSLOT1  0x00
+#define KTD202X_TIMER_SLOT_CONTROL_TSLOT2  0x01
+#define KTD202X_TIMER_SLOT_CONTROL_TSLOT3  0x02
+#define KTD202X_TIMER_SLOT_CONTROL_TSLOT4  0x03
+#define KTD202X_RSTR_RESET 0x07
+
+#define KTD202X_ENABLE_CTRL_WAKE   0x00 /* SCL High & SDA High */
+#define KTD202X_ENABLE_CTRL_SLEEP  0x08 /* SCL High & SDA Toggling */
+
+#define KTD202X_TRISE_FALL_SCALE_NORMAL0x00
+#define KTD202X_TRISE_FALL_SCALE_SLOW_X2   0x20
+#define KTD202X_TRISE_FALL_SCALE_SLOW_X4   0x40
+#define KTD202X_TRISE_FALL_SCALE_FAST_X8   0x60
+
+/* Register 1 */
+#define KTD202X_FLASH_PERIOD_256_MS_LOG_RAMP   0x00
+
+/* Register 2-3 */
+#define KTD202X_FLASH_ON_TIME_0_4_PERCENT  0x01
+
+/* Register 4 */
+#define KTD202X_CHANNEL_CTRL_MASK(x) (BIT(2 * (x)) | BIT(2 * (x) + 1))
+#define KTD202X_CHANNEL_CTRL_OFF 0x00
+#define KTD202X_CHANNEL_CTRL_ON(x) BIT(2 * (x))
+#define KTD202X_CHANNEL_CTRL_PWM1(x) BIT(2 * (x) + 1)
+#define KTD202X_CHANNEL_CTRL_PWM2(x) (BIT(2 * (x)) | BIT(2 * (x) + 1))
+
+/* Register 5 */
+#define KTD202X_RAMP_TIMES_2_MS0x00
+
+/* Register 6-9 */
+#define KTD202X_LED_CURRENT_10_mA  0x4f
+
+#define KTD202X_FLASH_PERIOD_MIN_MS 256
+#define KTD202X_FLASH_PERIOD_STEP_MS 128
+#define KTD202X_FLASH_PERIOD_MAX_STEPS 126
+#define KTD202X_FLASH_ON_MAX 256
+
+#define KTD202X_MAX_BRIGHTNESS 192
+
+static const struct reg_default ktd202x_reg_defaults[] = {
+   { KTD202X_REG_RESET_CONTROL, KTD202X_TIMER_SLOT_CONTROL_TSLOT1 |
+   KTD202X_ENABLE_CTRL_WAKE | KTD202X_TRISE_FALL_SCALE_NORMAL },
+   { KTD202X_REG_FLASH_PERIOD, KTD202X_FLASH_PERIOD_256_MS_LOG_RAMP },
+   { KTD202X_REG_PWM1_TIMER, KTD202X_FLASH_ON_TIME_0_4_PERCENT },
+   { KTD202X_REG_PWM2_TIMER, KTD202X_FLASH_ON_TIME_0_4_PERCENT },
+   { KTD202X_REG_CHANNEL_CTRL, KTD202X_CHANNEL_CTRL_OFF },
+   { KTD202X_REG_TRISE_FALL, KTD202X_RAMP_TIMES_2_MS },
+   { KTD202X_REG_LED_IOUT(0), KTD202X_LED_CURRENT_10_mA },
+   { KTD202X_REG_LED_IOUT(1), KTD202X_LED_CURRENT_10_mA },
+   { KTD202X_REG_LED_IOUT(2), KTD202X_LED_CURRENT_10_mA },
+   { KTD202X_REG_LED_IOUT(3), 

[PATCH v2 0/2] MPM pin mappings for MSM8226 and MSM8974

2023-09-23 Thread Matti Lehtimäki
This series adds the MPM wakeirq mappings for MSM8226 and MSM8974.

Changes in v2:
  - Add missing entry to MSM8226 mapping
  - Add Reviewed-by tag to MSM8974 patch

Matti Lehtimäki (2):
  pinctrl: qcom: msm8226: Add MPM pin mappings
  pinctrl: qcom: msm8974: Add MPM pin mappings

 drivers/pinctrl/qcom/pinctrl-msm8226.c | 12 
 drivers/pinctrl/qcom/pinctrl-msm8x74.c | 12 
 2 files changed, 24 insertions(+)

-- 
2.39.2



[PATCH v2 1/2] pinctrl: qcom: msm8226: Add MPM pin mappings

2023-09-23 Thread Matti Lehtimäki
Add pin <-> wakeirq mappings to allow for waking up the AP from sleep
through MPM-connected pins.

Signed-off-by: Matti Lehtimäki 
---
Changes in v2:
  - Add missing entry to mapping
---
 drivers/pinctrl/qcom/pinctrl-msm8226.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm8226.c 
b/drivers/pinctrl/qcom/pinctrl-msm8226.c
index 994619840a70..4030baa3715f 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm8226.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm8226.c
@@ -612,6 +612,16 @@ static const struct msm_pingroup msm8226_groups[] = {
 
 #define NUM_GPIO_PINGROUPS 117
 
+static const struct msm_gpio_wakeirq_map msm8226_mpm_map[] = {
+   { 1, 3 }, { 4, 4 }, { 5, 5 }, { 9, 6 }, { 13, 7 }, { 17, 8 },
+   { 21, 9 }, { 27, 10 }, { 29, 11 }, { 31, 12 }, { 33, 13 }, { 35, 14 },
+   { 37, 15 }, { 38, 16 }, { 39, 17 }, { 41, 18 }, { 46, 19 }, { 48, 20 },
+   { 49, 21 }, { 50, 22 }, { 51, 23 }, { 52, 24 }, { 54, 25 }, { 62, 26 },
+   { 63, 27 }, { 64, 28 }, { 65, 29 }, { 66, 30 }, { 67, 31 }, { 68, 32 },
+   { 69, 33 }, { 71, 34 }, { 72, 35 }, { 106, 36 }, { 107, 37 }, { 108, 38 
},
+   { 109, 39 }, { 110, 40 }, { 111, 54 }, { 113, 55 }, { 115, 41 },
+};
+
 static const struct msm_pinctrl_soc_data msm8226_pinctrl = {
.pins = msm8226_pins,
.npins = ARRAY_SIZE(msm8226_pins),
@@ -620,6 +630,8 @@ static const struct msm_pinctrl_soc_data msm8226_pinctrl = {
.groups = msm8226_groups,
.ngroups = ARRAY_SIZE(msm8226_groups),
.ngpios = NUM_GPIO_PINGROUPS,
+   .wakeirq_map = msm8226_mpm_map,
+   .nwakeirq_map = ARRAY_SIZE(msm8226_mpm_map),
 };
 
 static int msm8226_pinctrl_probe(struct platform_device *pdev)
-- 
2.39.2



[PATCH v4 1/2] dt-bindings: leds: Add Kinetic KTD2026/2027 LED

2023-09-23 Thread André Apitzsch
Document Kinetic KTD2026/2027 LED driver devicetree bindings.

Reviewed-by: Krzysztof Kozlowski 
Signed-off-by: André Apitzsch 
---
 .../devicetree/bindings/leds/kinetic,ktd202x.yaml  | 171 +
 1 file changed, 171 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/kinetic,ktd202x.yaml 
b/Documentation/devicetree/bindings/leds/kinetic,ktd202x.yaml
new file mode 100644
index ..832c030a5acf
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/kinetic,ktd202x.yaml
@@ -0,0 +1,171 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/kinetic,ktd202x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Kinetic KTD2026/7 RGB/White LED Driver
+
+maintainers:
+  - André Apitzsch 
+
+description: |
+  The KTD2026/7 is a RGB/White LED driver with I2C interface.
+
+  The data sheet can be found at:
+https://www.kinet-ic.com/uploads/KTD2026-7-04h.pdf
+
+properties:
+  compatible:
+enum:
+  - kinetic,ktd2026
+  - kinetic,ktd2027
+
+  reg:
+maxItems: 1
+
+  vin-supply:
+description: Regulator providing power to the "VIN" pin.
+
+  vio-supply:
+description: Regulator providing power for pull-up of the I/O lines.
+  Note that this regulator does not directly connect to KTD2026, but is
+  needed for the correct operation of the status ("ST") and I2C lines.
+
+  "#address-cells":
+const: 1
+
+  "#size-cells":
+const: 0
+
+  multi-led:
+type: object
+$ref: leds-class-multicolor.yaml#
+unevaluatedProperties: false
+
+properties:
+  "#address-cells":
+const: 1
+
+  "#size-cells":
+const: 0
+
+patternProperties:
+  "^led@[0-3]$":
+type: object
+$ref: common.yaml#
+unevaluatedProperties: false
+
+properties:
+  reg:
+description: Index of the LED.
+minimum: 0
+maximum: 3
+
+required:
+  - reg
+  - color
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+
+patternProperties:
+  "^led@[0-3]$":
+type: object
+$ref: common.yaml#
+unevaluatedProperties: false
+
+properties:
+  reg:
+description: Index of the LED.
+minimum: 0
+maximum: 3
+
+required:
+  - reg
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+#include 
+
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+
+led-controller@30 {
+compatible = "kinetic,ktd2026";
+reg = <0x30>;
+#address-cells = <1>;
+#size-cells = <0>;
+
+vin-supply = <_l17>;
+vio-supply = <_l6>;
+
+led@0 {
+reg = <0>;
+function = LED_FUNCTION_STATUS;
+color = ;
+};
+
+led@1 {
+reg = <1>;
+function = LED_FUNCTION_STATUS;
+color = ;
+};
+
+led@2 {
+reg = <2>;
+function = LED_FUNCTION_STATUS;
+color = ;
+};
+};
+};
+  - |
+#include 
+
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+
+led-controller@30 {
+compatible = "kinetic,ktd2026";
+reg = <0x30>;
+#address-cells = <1>;
+#size-cells = <0>;
+
+vin-supply = <_l17>;
+vio-supply = <_l6>;
+
+multi-led {
+color = ;
+function = LED_FUNCTION_STATUS;
+
+#address-cells = <1>;
+#size-cells = <0>;
+
+led@0 {
+reg = <0>;
+color = ;
+};
+
+led@1 {
+reg = <1>;
+color = ;
+};
+
+led@2 {
+reg = <2>;
+color = ;
+};
+};
+};
+};

-- 
2.42.0



[PATCH v4 0/2] leds: Add a driver for KTD202x

2023-09-23 Thread André Apitzsch
Add the binding description and the corresponding driver for
the Kinetic KTD2026 and KTD2027.

Signed-off-by: André Apitzsch 
---
Changes in v4:
- Annotate struct ktd202x with __counted_by
- Link to v3: 
https://lore.kernel.org/r/20230906-ktd202x-v3-0-7fcb91c65...@apitzsch.eu

Changes in v3:
- Add r-b to bindings patch
- Replace .probe_new by .probe
- Link to v2: 
https://lore.kernel.org/r/20230901-ktd202x-v2-0-3cb8b0ca0...@apitzsch.eu

Changes in v2:
- Make binding description filename match compatible
- Address comments by Lee Jones
  - Extend driver description in Kconfig
  - Add copyright + link to datasheet
  - Add unit to definition/variable names, where needed
  - Define magic numbers
  - Remove forward declaration of 'struct ktd202x'
  - Remove superfluous comments
  - Get rid of struct ktd202x_info
  - Join ktd202x_chip_init() with ktd202x_chip_enable()
  - Return the error on ktd202x_chip_disable()
  - Remove unreachable case from chip_in_use()
  - Rename ktd202x_brightness_set() argument from num_colors to num_channels
  - Forward errors received in ktd202x_brightness_set()
  - Remove variable for 'num_channels = 1'
  - Add some explanations to blink time calculation
  - Remove unneeded lcdev from ktd202x_blink_*_set()
  - Add define for max brightness and replace deprecated LED_FULL by it
  - Move setting led_classdev.brightness to ktd202x_brightness_*_set()
  - Move mutex_lock inside ktd202x_blink_set()
  - Add comment that 'color' property is optional (allow EINVAL)
  - Replace escaped double quotes by single quotes
  - Avoid overloading variable 'color'
  - Do not lock during probe
  - Remove usage of 'of_match_ptr'
- Document interrupt and pull-up supply, like done for aw2013[1]
- Fix error in num_steps calculation
- Link to v1: 
https://lore.kernel.org/r/20230618-ktd202x-v1-0-fc182fefa...@apitzsch.eu

[1] 
https://lore.kernel.org/linux-leds/20230815-aw2013-vio-v3-0-2505296b0...@gerhold.net/

---
André Apitzsch (2):
  dt-bindings: leds: Add Kinetic KTD2026/2027 LED
  leds: add ktd202x driver

 .../devicetree/bindings/leds/kinetic,ktd202x.yaml  | 171 ++
 drivers/leds/rgb/Kconfig   |  13 +
 drivers/leds/rgb/Makefile  |   1 +
 drivers/leds/rgb/leds-ktd202x.c| 625 +
 4 files changed, 810 insertions(+)
---
base-commit: 9641a13970d832d821ad304e187b80dbe70bae30
change-id: 20230618-ktd202x-546b2a7d240b

Best regards,
-- 
André Apitzsch 



Re: [PATCH 1/2] pinctrl: qcom: msm8226: Add MPM pin mappings

2023-09-23 Thread Luca Weiss
On Samstag, 23. September 2023 13:35:25 CEST Stephan Gerhold wrote:
> On Sat, Sep 23, 2023 at 01:19:46PM +0200, Luca Weiss wrote:
> > On Samstag, 23. September 2023 12:00:52 CEST Stephan Gerhold wrote:
> > > On Sat, Sep 23, 2023 at 11:32:47AM +0200, Luca Weiss wrote:
> > > > Hi Matti,
> > > > 
> > > > On Samstag, 23. September 2023 00:40:26 CEST Matti Lehtimäki wrote:
> > > > > Add pin <-> wakeirq mappings to allow for waking up the AP from
> > > > > sleep
> > > > > through MPM-connected pins.
> > > > > 
> > > > > Signed-off-by: Matti Lehtimäki 
> > > > > ---
> > > > > 
> > > > >  drivers/pinctrl/qcom/pinctrl-msm8226.c | 12 
> > > > >  1 file changed, 12 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > > > > b/drivers/pinctrl/qcom/pinctrl-msm8226.c index
> > > > > 994619840a70..1e46a9ab382f
> > > > > 100644
> > > > > --- a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > > > > +++ b/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > > > > @@ -612,6 +612,16 @@ static const struct msm_pingroup
> > > > > msm8226_groups[] =
> > > > > {
> > > > > 
> > > > >  #define NUM_GPIO_PINGROUPS 117
> > > > > 
> > > > > +static const struct msm_gpio_wakeirq_map msm8226_mpm_map[] = {
> > > > > + { 1, 3 }, { 4, 4 }, { 5, 5 }, { 9, 6 }, { 13, 7 }, { 17, 8 
},
> > > > 
> > > > I'm not really convinced this is the correct order of values...
> > > > 
> > > > Let's look at downstream:
> > > >   qcom,gpio-map = <3  1>,
> > > >   
> > > >   <4  4 >,
> > > >   <5  5 >,
> > > >   <6  9 >,
> > > >   [...]
> > > > 
> > > > From Documentation/devicetree/bindings/arm/msm/mpm.txt downstream:
> > > >   Each tuple represents a MPM pin and which GIC interrupt is routed to
> > > >   it.
> > > > 
> > > > So first is pin number, second is interrupt number.
> > > > 
> > > > And check mainline:
> > > >   /**
> > > >   
> > > >* struct msm_gpio_wakeirq_map - Map of GPIOs and their wakeup pins
> > > >* @gpio:  The GPIOs that are wakeup capable
> > > >* @wakeirq:   The interrupt at the always-on interrupt
> > > >controller
> > > >*/
> > > >   
> > > >   struct msm_gpio_wakeirq_map {
> > > >   
> > > > unsigned int gpio;
> > > > unsigned int wakeirq;
> > > >   
> > > >   };
> > > > 
> > > > So here we also have the order pin-interrupt, not the reverse order.
> > > > 
> > > > Therefore I believe the order in this patch is incorrect, and it
> > > > should
> > > > rather>
> > > > 
> > > > be:
> > > >   { 3, 1 }, { 4, 4 }, { 5, 5 }, { 6, 9 }, { 7, 13 }, { 8, 17 },
> > > >   [...]
> > > > 
> > > > Or do you think I'm missing something?
> > > 
> > > Yes :)
> > > 
> > > Let's look at the later entries:
> > > > > + { 21, 9 }, { 27, 10 }, { 29, 11 }, { 31, 12 }, { 33, 13 }, 
{ 35,
> > > > > 14
> > > > 
> > > > },
> > > > 
> > > > > + { 37, 15 }, { 38, 16 }, { 39, 17 }, { 41, 18 }, { 46, 19 
}, { 48,
> > > > > 20
> > > > 
> > > > },
> > > > 
> > > > > + { 49, 21 }, { 50, 22 }, { 51, 23 }, { 52, 24 }, { 54, 25 
}, { 62,
> > > > > 26
> > > > 
> > > > },
> > > > 
> > > > > + { 63, 27 }, { 64, 28 }, { 65, 29 }, { 66, 30 }, { 67, 31 
}, { 68,
> > > > > 32
> > > > 
> > > > },
> > > > 
> > > > > + { 69, 33 }, { 71, 34 }, { 72, 35 }, { 106, 36 }, { 107, 37 
},
> > > > > + { 108, 38 }, { 109, 39 }, { 110, 40 }, { 111, 54 }, { 113, 
55 },
> > > > > +};
> > > > > +
> > > 
> > > For example: { 113, 55 }, i.e. { .gpio = 113, .wakeirq = 55 }.
> > > 
> > > MSM8226 has GPIOs 0-116 and 64 MPM pins/interrupts. The order in this
> > > patch is the only one that can be correct because the definition would
> > > be invalid the other way around. 113 must be the GPIO number because it
> > > is larger than the 64 available MPM interrupt pins. :)
> > 
> > So basically you're saying downstream is wrong / buggy?
> 
> "Misleading" or "confusing" would be the words I would use. :-)

;)

> 
> > From qcom,gpio-map = [...], <55 113>; it's taking the properties like this
> > 
> > (drivers/soc/qcom/mpm-of.c):
> >   unsigned long pin = be32_to_cpup(list++);
> >   irq_hw_number_t hwirq = be32_to_cpup(list++);
> > 
> > Your explanation does make sense I guess but somewhere the link downstream
> > -> mainline must be broken, no?
> 
> After staring at mpm-of.c for a while I would say that there:
>  - downstream "pin" = MPM pin = mainline "wakeirq"
>- because this is used as index to msm_mpm_irqs_m2a, which has a size
>  of MSM_MPM_NR_MPM_IRQS (64)
>  - downstream "hwirq" = GPIO / GIC IRQ = mainline "gpio"
> 
> This means for <55 113>: pin = wakeirq = 55 and hwirq = gpio = 113.
> Which matches the definition in this patch:
>   { .gpio = 113, .wakeirq = 55 } = { 113, 55 }

Fun, thanks for digging into it!

@Matti: I think I see one missing entry here "<41  115>," on downstream, so
{ 115, 41 } appears to be missing in this patch? Or is there a reason you 
omitted that one? The rest looks correct :)

Regards
Luca



Re: [PATCH 1/2] pinctrl: qcom: msm8226: Add MPM pin mappings

2023-09-23 Thread Stephan Gerhold
On Sat, Sep 23, 2023 at 01:19:46PM +0200, Luca Weiss wrote:
> On Samstag, 23. September 2023 12:00:52 CEST Stephan Gerhold wrote:
> > On Sat, Sep 23, 2023 at 11:32:47AM +0200, Luca Weiss wrote:
> > > Hi Matti,
> > > 
> > > On Samstag, 23. September 2023 00:40:26 CEST Matti Lehtimäki wrote:
> > > > Add pin <-> wakeirq mappings to allow for waking up the AP from sleep
> > > > through MPM-connected pins.
> > > > 
> > > > Signed-off-by: Matti Lehtimäki 
> > > > ---
> > > > 
> > > >  drivers/pinctrl/qcom/pinctrl-msm8226.c | 12 
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > > > b/drivers/pinctrl/qcom/pinctrl-msm8226.c index
> > > > 994619840a70..1e46a9ab382f
> > > > 100644
> > > > --- a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > > > +++ b/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > > > @@ -612,6 +612,16 @@ static const struct msm_pingroup msm8226_groups[] =
> > > > {
> > > > 
> > > >  #define NUM_GPIO_PINGROUPS 117
> > > > 
> > > > +static const struct msm_gpio_wakeirq_map msm8226_mpm_map[] = {
> > > > +   { 1, 3 }, { 4, 4 }, { 5, 5 }, { 9, 6 }, { 13, 7 }, { 17, 8 },
> > > 
> > > I'm not really convinced this is the correct order of values...
> > > 
> > > Let's look at downstream:
> > >   qcom,gpio-map = <3  1>,
> > >   
> > >   <4  4 >,
> > >   <5  5 >,
> > >   <6  9 >,
> > >   [...]
> > > 
> > > From Documentation/devicetree/bindings/arm/msm/mpm.txt downstream:
> > >   Each tuple represents a MPM pin and which GIC interrupt is routed to it.
> > > 
> > > So first is pin number, second is interrupt number.
> > > 
> > > And check mainline:
> > >   /**
> > >   
> > >* struct msm_gpio_wakeirq_map - Map of GPIOs and their wakeup pins
> > >* @gpio:  The GPIOs that are wakeup capable
> > >* @wakeirq:   The interrupt at the always-on interrupt controller
> > >*/
> > >   
> > >   struct msm_gpio_wakeirq_map {
> > >   
> > >   unsigned int gpio;
> > >   unsigned int wakeirq;
> > >   
> > >   };
> > > 
> > > So here we also have the order pin-interrupt, not the reverse order.
> > > 
> > > Therefore I believe the order in this patch is incorrect, and it should
> > > rather> 
> > > be:
> > >   { 3, 1 }, { 4, 4 }, { 5, 5 }, { 6, 9 }, { 7, 13 }, { 8, 17 },
> > >   [...]
> > > 
> > > Or do you think I'm missing something?
> > 
> > Yes :)
> > 
> > Let's look at the later entries:
> > > > +   { 21, 9 }, { 27, 10 }, { 29, 11 }, { 31, 12 }, { 33, 13 }, { 
> > > > 35, 14
> > > 
> > > },
> > > 
> > > > +   { 37, 15 }, { 38, 16 }, { 39, 17 }, { 41, 18 }, { 46, 19 }, { 
> > > > 48, 20
> > > 
> > > },
> > > 
> > > > +   { 49, 21 }, { 50, 22 }, { 51, 23 }, { 52, 24 }, { 54, 25 }, { 
> > > > 62, 26
> > > 
> > > },
> > > 
> > > > +   { 63, 27 }, { 64, 28 }, { 65, 29 }, { 66, 30 }, { 67, 31 }, { 
> > > > 68, 32
> > > 
> > > },
> > > 
> > > > +   { 69, 33 }, { 71, 34 }, { 72, 35 }, { 106, 36 }, { 107, 37 },
> > > > +   { 108, 38 }, { 109, 39 }, { 110, 40 }, { 111, 54 }, { 113, 55 },
> > > > +};
> > > > +
> > 
> > For example: { 113, 55 }, i.e. { .gpio = 113, .wakeirq = 55 }.
> > 
> > MSM8226 has GPIOs 0-116 and 64 MPM pins/interrupts. The order in this
> > patch is the only one that can be correct because the definition would
> > be invalid the other way around. 113 must be the GPIO number because it
> > is larger than the 64 available MPM interrupt pins. :)
> 
> So basically you're saying downstream is wrong / buggy?
> 

"Misleading" or "confusing" would be the words I would use. :-)

> From qcom,gpio-map = [...], <55 113>; it's taking the properties like this
> (drivers/soc/qcom/mpm-of.c):
> 
>   unsigned long pin = be32_to_cpup(list++);
>   irq_hw_number_t hwirq = be32_to_cpup(list++);
> 
> Your explanation does make sense I guess but somewhere the link downstream -> 
> mainline must be broken, no?
> 

After staring at mpm-of.c for a while I would say that there:
 - downstream "pin" = MPM pin = mainline "wakeirq"
   - because this is used as index to msm_mpm_irqs_m2a, which has a size
 of MSM_MPM_NR_MPM_IRQS (64)
 - downstream "hwirq" = GPIO / GIC IRQ = mainline "gpio"

This means for <55 113>: pin = wakeirq = 55 and hwirq = gpio = 113.
Which matches the definition in this patch:
  { .gpio = 113, .wakeirq = 55 } = { 113, 55 }

Stephan


Re: [PATCH 1/2] pinctrl: qcom: msm8226: Add MPM pin mappings

2023-09-23 Thread Luca Weiss
On Samstag, 23. September 2023 12:00:52 CEST Stephan Gerhold wrote:
> On Sat, Sep 23, 2023 at 11:32:47AM +0200, Luca Weiss wrote:
> > Hi Matti,
> > 
> > On Samstag, 23. September 2023 00:40:26 CEST Matti Lehtimäki wrote:
> > > Add pin <-> wakeirq mappings to allow for waking up the AP from sleep
> > > through MPM-connected pins.
> > > 
> > > Signed-off-by: Matti Lehtimäki 
> > > ---
> > > 
> > >  drivers/pinctrl/qcom/pinctrl-msm8226.c | 12 
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > > b/drivers/pinctrl/qcom/pinctrl-msm8226.c index
> > > 994619840a70..1e46a9ab382f
> > > 100644
> > > --- a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > > +++ b/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > > @@ -612,6 +612,16 @@ static const struct msm_pingroup msm8226_groups[] =
> > > {
> > > 
> > >  #define NUM_GPIO_PINGROUPS 117
> > > 
> > > +static const struct msm_gpio_wakeirq_map msm8226_mpm_map[] = {
> > > + { 1, 3 }, { 4, 4 }, { 5, 5 }, { 9, 6 }, { 13, 7 }, { 17, 8 },
> > 
> > I'm not really convinced this is the correct order of values...
> > 
> > Let's look at downstream:
> >   qcom,gpio-map = <3  1>,
> >   
> >   <4  4 >,
> >   <5  5 >,
> >   <6  9 >,
> >   [...]
> > 
> > From Documentation/devicetree/bindings/arm/msm/mpm.txt downstream:
> >   Each tuple represents a MPM pin and which GIC interrupt is routed to it.
> > 
> > So first is pin number, second is interrupt number.
> > 
> > And check mainline:
> >   /**
> >   
> >* struct msm_gpio_wakeirq_map - Map of GPIOs and their wakeup pins
> >* @gpio:  The GPIOs that are wakeup capable
> >* @wakeirq:   The interrupt at the always-on interrupt controller
> >*/
> >   
> >   struct msm_gpio_wakeirq_map {
> >   
> > unsigned int gpio;
> > unsigned int wakeirq;
> >   
> >   };
> > 
> > So here we also have the order pin-interrupt, not the reverse order.
> > 
> > Therefore I believe the order in this patch is incorrect, and it should
> > rather> 
> > be:
> >   { 3, 1 }, { 4, 4 }, { 5, 5 }, { 6, 9 }, { 7, 13 }, { 8, 17 },
> >   [...]
> > 
> > Or do you think I'm missing something?
> 
> Yes :)
> 
> Let's look at the later entries:
> > > + { 21, 9 }, { 27, 10 }, { 29, 11 }, { 31, 12 }, { 33, 13 }, { 35, 14
> > 
> > },
> > 
> > > + { 37, 15 }, { 38, 16 }, { 39, 17 }, { 41, 18 }, { 46, 19 }, { 48, 20
> > 
> > },
> > 
> > > + { 49, 21 }, { 50, 22 }, { 51, 23 }, { 52, 24 }, { 54, 25 }, { 62, 26
> > 
> > },
> > 
> > > + { 63, 27 }, { 64, 28 }, { 65, 29 }, { 66, 30 }, { 67, 31 }, { 68, 32
> > 
> > },
> > 
> > > + { 69, 33 }, { 71, 34 }, { 72, 35 }, { 106, 36 }, { 107, 37 },
> > > + { 108, 38 }, { 109, 39 }, { 110, 40 }, { 111, 54 }, { 113, 55 },
> > > +};
> > > +
> 
> For example: { 113, 55 }, i.e. { .gpio = 113, .wakeirq = 55 }.
> 
> MSM8226 has GPIOs 0-116 and 64 MPM pins/interrupts. The order in this
> patch is the only one that can be correct because the definition would
> be invalid the other way around. 113 must be the GPIO number because it
> is larger than the 64 available MPM interrupt pins. :)

So basically you're saying downstream is wrong / buggy?

From qcom,gpio-map = [...], <55 113>; it's taking the properties like this
(drivers/soc/qcom/mpm-of.c):

  unsigned long pin = be32_to_cpup(list++);
  irq_hw_number_t hwirq = be32_to_cpup(list++);

Your explanation does make sense I guess but somewhere the link downstream -> 
mainline must be broken, no?

Regards
Luca


> 
> Thanks,
> Stephan






Re: [PATCH 1/2] pinctrl: qcom: msm8226: Add MPM pin mappings

2023-09-23 Thread Stephan Gerhold
On Sat, Sep 23, 2023 at 11:32:47AM +0200, Luca Weiss wrote:
> Hi Matti,
> 
> On Samstag, 23. September 2023 00:40:26 CEST Matti Lehtimäki wrote:
> > Add pin <-> wakeirq mappings to allow for waking up the AP from sleep
> > through MPM-connected pins.
> > 
> > Signed-off-by: Matti Lehtimäki 
> > ---
> >  drivers/pinctrl/qcom/pinctrl-msm8226.c | 12 
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > b/drivers/pinctrl/qcom/pinctrl-msm8226.c index 994619840a70..1e46a9ab382f
> > 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > @@ -612,6 +612,16 @@ static const struct msm_pingroup msm8226_groups[] = {
> > 
> >  #define NUM_GPIO_PINGROUPS 117
> > 
> > +static const struct msm_gpio_wakeirq_map msm8226_mpm_map[] = {
> > +   { 1, 3 }, { 4, 4 }, { 5, 5 }, { 9, 6 }, { 13, 7 }, { 17, 8 },
> 
> I'm not really convinced this is the correct order of values...
> 
> Let's look at downstream:
> 
>   qcom,gpio-map = <3  1>,
>   <4  4 >,
>   <5  5 >,
>   <6  9 >,
>   [...]
> 
> From Documentation/devicetree/bindings/arm/msm/mpm.txt downstream:
> 
>   Each tuple represents a MPM pin and which GIC interrupt is routed to it.
> 
> So first is pin number, second is interrupt number.
> 
> And check mainline: 
> 
>   /**
>* struct msm_gpio_wakeirq_map - Map of GPIOs and their wakeup pins
>* @gpio:  The GPIOs that are wakeup capable
>* @wakeirq:   The interrupt at the always-on interrupt controller
>*/
>   struct msm_gpio_wakeirq_map {
>   unsigned int gpio;
>   unsigned int wakeirq;
>   };
> 
> So here we also have the order pin-interrupt, not the reverse order.
> 
> Therefore I believe the order in this patch is incorrect, and it should 
> rather 
> be:
> 
>   { 3, 1 }, { 4, 4 }, { 5, 5 }, { 6, 9 }, { 7, 13 }, { 8, 17 },
>   [...]
> 
> Or do you think I'm missing something?
> 

Yes :)

Let's look at the later entries:

> > +   { 21, 9 }, { 27, 10 }, { 29, 11 }, { 31, 12 }, { 33, 13 }, { 35, 14 
> },
> > +   { 37, 15 }, { 38, 16 }, { 39, 17 }, { 41, 18 }, { 46, 19 }, { 48, 20 
> },
> > +   { 49, 21 }, { 50, 22 }, { 51, 23 }, { 52, 24 }, { 54, 25 }, { 62, 26 
> },
> > +   { 63, 27 }, { 64, 28 }, { 65, 29 }, { 66, 30 }, { 67, 31 }, { 68, 32 
> },
> > +   { 69, 33 }, { 71, 34 }, { 72, 35 }, { 106, 36 }, { 107, 37 },
> > +   { 108, 38 }, { 109, 39 }, { 110, 40 }, { 111, 54 }, { 113, 55 },
> > +};
> > +

For example: { 113, 55 }, i.e. { .gpio = 113, .wakeirq = 55 }.

MSM8226 has GPIOs 0-116 and 64 MPM pins/interrupts. The order in this
patch is the only one that can be correct because the definition would
be invalid the other way around. 113 must be the GPIO number because it
is larger than the 64 available MPM interrupt pins. :)

Thanks,
Stephan


Re: [PATCH v9 1/5] lib: objpool added: ring-array based lockless MPMC

2023-09-23 Thread Google
Hi Wuqiang,

Sorry for replying later.

On Tue,  5 Sep 2023 09:52:51 +0800
"wuqiang.matt"  wrote:

> The object pool is a scalable implementaion of high performance queue
> for object allocation and reclamation, such as kretprobe instances.
> 
> With leveraging percpu ring-array to mitigate the hot spot of memory
> contention, it could deliver near-linear scalability for high parallel
> scenarios. The objpool is best suited for following cases:
> 1) Memory allocation or reclamation are prohibited or too expensive
> 2) Consumers are of different priorities, such as irqs and threads
> 
> Limitations:
> 1) Maximum objects (capacity) is determined during pool initializing
>and can't be modified (extended) after objpool creation

So the pool size is fixed in initialization.

> 2) The memory of objects won't be freed until objpool is finalized
> 3) Object allocation (pop) may fail after trying all cpu slots

This means that object allocation will fail if the all pools are empty,
right?

> 
> Signed-off-by: wuqiang.matt 
> ---
>  include/linux/objpool.h | 174 +
>  lib/Makefile|   2 +-
>  lib/objpool.c   | 338 
>  3 files changed, 513 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/objpool.h
>  create mode 100644 lib/objpool.c
> 
> diff --git a/include/linux/objpool.h b/include/linux/objpool.h
> new file mode 100644
> index ..33c832216b98
> --- /dev/null
> +++ b/include/linux/objpool.h
> @@ -0,0 +1,174 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _LINUX_OBJPOOL_H
> +#define _LINUX_OBJPOOL_H
> +
> +#include 
> +#include 
> +
> +/*
> + * objpool: ring-array based lockless MPMC queue
> + *
> + * Copyright: wuqiang.m...@bytedance.com
> + *
> + * The object pool is a scalable implementaion of high performance queue
> + * for objects allocation and reclamation, such as kretprobe instances.
> + *
> + * With leveraging per-cpu ring-array to mitigate the hot spots of memory
> + * contention, it could deliver near-linear scalability for high parallel
> + * scenarios. The ring-array is compactly managed in a single cache-line
> + * to benefit from warmed L1 cache for most cases (<= 4 objects per-core).
> + * The body of pre-allocated objects is stored in continuous cache-lines
> + * just after the ring-array.

I consider the size of entries may be big if we have larger number of
CPU cores, like 72-cores. And if user specifies (2^n) + 1 entries.
In this case, each CPU has (2^n - 1)/72 objects, but has 2^(n + 1)
entries in ring buffer. So it should be noted.

> + *
> + * The object pool is interrupt safe. Both allocation and reclamation
> + * (object pop and push operations) can be preemptible or interruptable.

You've added raw_spinlock_disable/enable(), so it is not preemptible
or interruptible anymore. (Anyway, caller doesn't take care of that)

> + *
> + * It's best suited for following cases:
> + * 1) Memory allocation or reclamation are prohibited or too expensive
> + * 2) Consumers are of different priorities, such as irqs and threads
> + *
> + * Limitations:
> + * 1) Maximum objects (capacity) is determined during pool initializing
> + * 2) The memory of objects won't be freed until the pool is finalized
> + * 3) Object allocation (pop) may fail after trying all cpu slots
> + */
> +
> +/**
> + * struct objpool_slot - percpu ring array of objpool
> + * @head: head of the local ring array (to retrieve at)
> + * @tail: tail of the local ring array (to append at)
> + * @bits: log2 of capacity (for bitwise operations)
> + * @mask: capacity - 1

These description doesn not give idea what those roles are.

> + *
> + * Represents a cpu-local array-based ring buffer, its size is specialized
> + * during initialization of object pool. The percpu objpool slot is to be
> + * allocated from local memory for NUMA system, and to be kept compact in
> + * continuous memory: ages[] is stored just after the body of objpool_slot,
> + * and then entries[]. ages[] describes revision of each item, solely used
> + * to avoid ABA; entries[] contains pointers of the actual objects
> + *
> + * Layout of objpool_slot in memory:
> + *
> + * 64bit:
> + *4  8  12 1632 64
> + * | head | tail | bits | mask | ages[4] | ents[4]: (8 * 4) | objects
> + *
> + * 32bit:
> + *4  8  12 163248   64
> + * | head | tail | bits | mask | ages[4] | ents[4] | unused | objects

Hm, the '4' here means number of objects after this objpool_slot?
I don't recommend you to allocate several arraies after the header, instead,
using another data structure like this;

|head|tail|bits|mask|ents[N]{age:4|offs:4}|padding|objects

here N means the number of total objects.

struct objpool_entry {
uint32_tage;
void *  ptr;
} __attribute__((packed,aligned(8))) ;

> + *
> + */
> +struct objpool_slot {
> + uint32_thead;
> + 

Re: [PATCH 1/2] pinctrl: qcom: msm8226: Add MPM pin mappings

2023-09-23 Thread Luca Weiss
Hi Matti,

On Samstag, 23. September 2023 00:40:26 CEST Matti Lehtimäki wrote:
> Add pin <-> wakeirq mappings to allow for waking up the AP from sleep
> through MPM-connected pins.
> 
> Signed-off-by: Matti Lehtimäki 
> ---
>  drivers/pinctrl/qcom/pinctrl-msm8226.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> b/drivers/pinctrl/qcom/pinctrl-msm8226.c index 994619840a70..1e46a9ab382f
> 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm8226.c
> @@ -612,6 +612,16 @@ static const struct msm_pingroup msm8226_groups[] = {
> 
>  #define NUM_GPIO_PINGROUPS 117
> 
> +static const struct msm_gpio_wakeirq_map msm8226_mpm_map[] = {
> + { 1, 3 }, { 4, 4 }, { 5, 5 }, { 9, 6 }, { 13, 7 }, { 17, 8 },

I'm not really convinced this is the correct order of values...

Let's look at downstream:

  qcom,gpio-map = <3  1>,
  <4  4 >,
  <5  5 >,
  <6  9 >,
  [...]

From Documentation/devicetree/bindings/arm/msm/mpm.txt downstream:

  Each tuple represents a MPM pin and which GIC interrupt is routed to it.

So first is pin number, second is interrupt number.

And check mainline: 

  /**
   * struct msm_gpio_wakeirq_map - Map of GPIOs and their wakeup pins
   * @gpio:  The GPIOs that are wakeup capable
   * @wakeirq:   The interrupt at the always-on interrupt controller
   */
  struct msm_gpio_wakeirq_map {
unsigned int gpio;
unsigned int wakeirq;
  };

So here we also have the order pin-interrupt, not the reverse order.

Therefore I believe the order in this patch is incorrect, and it should rather 
be:

  { 3, 1 }, { 4, 4 }, { 5, 5 }, { 6, 9 }, { 7, 13 }, { 8, 17 },
  [...]

Or do you think I'm missing something?

Regards
Luca

> + { 21, 9 }, { 27, 10 }, { 29, 11 }, { 31, 12 }, { 33, 13 }, { 35, 14 
},
> + { 37, 15 }, { 38, 16 }, { 39, 17 }, { 41, 18 }, { 46, 19 }, { 48, 20 
},
> + { 49, 21 }, { 50, 22 }, { 51, 23 }, { 52, 24 }, { 54, 25 }, { 62, 26 
},
> + { 63, 27 }, { 64, 28 }, { 65, 29 }, { 66, 30 }, { 67, 31 }, { 68, 32 
},
> + { 69, 33 }, { 71, 34 }, { 72, 35 }, { 106, 36 }, { 107, 37 },
> + { 108, 38 }, { 109, 39 }, { 110, 40 }, { 111, 54 }, { 113, 55 },
> +};
> +
>  static const struct msm_pinctrl_soc_data msm8226_pinctrl = {
>   .pins = msm8226_pins,
>   .npins = ARRAY_SIZE(msm8226_pins),
> @@ -620,6 +630,8 @@ static const struct msm_pinctrl_soc_data msm8226_pinctrl
> = { .groups = msm8226_groups,
>   .ngroups = ARRAY_SIZE(msm8226_groups),
>   .ngpios = NUM_GPIO_PINGROUPS,
> + .wakeirq_map = msm8226_mpm_map,
> + .nwakeirq_map = ARRAY_SIZE(msm8226_mpm_map),
>  };
> 
>  static int msm8226_pinctrl_probe(struct platform_device *pdev)






Re: [PATCH v9 0/5] lib,kprobes: kretprobe scalability improvement

2023-09-23 Thread Google
Hi Wuqiang,

I dug my mail box and found this. Sorry for replying late.

On Tue,  5 Sep 2023 09:52:50 +0800
"wuqiang.matt"  wrote:

> This patch series introduces a scalable and lockless ring-array based
> object pool and replaces the original freelist (a LIFO queue based on
> singly linked list) to improve scalability of kretprobed routines.
> 
> v9:
>   1) objpool: raw_local_irq_save/restore added to prevent interruption
> 
>  To avoid possible ABA issues, we must ensure objpool_try_add_slot
>  and objpool_try_add_slot are uninterruptible. If these operations
>  are blocked or interrupted in the middle, other cores could overrun
>  the same slot's ages[] of uint32, then after resuming back, the
>  interrupted pop() or push() could see same value of ages[], which
>  is a typical ABA problem though the possibility is small.
> 
>  The pair of pop()/push() costs about 8.53 cpu cycles, measured
>  by IACA (Intel Architecture Code Analyzer). That is, on a 4Ghz
>  core dedicated for pop() & push(), theoretically it would only
>  need 8.53 seconds to overflow a 32bit value. Testings upon Intel
>  i7-10700 (2.90GHz) cost 71.88 seconds to overrun a 32bit integer.

What does this mean? This sounds like "There is a timing issue if it's enough 
fast".

Let me reivew the patch itself.

Thanks,

> 
>   2) codes improvements: thanks to Masami for the thorough inspection
> 
> v8:
>   1) objpool: refcount added for objpool lifecycle management
> 
> wuqiang.matt (5):
>   lib: objpool added: ring-array based lockless MPMC
>   lib: objpool test module added
>   kprobes: kretprobe scalability improvement with objpool
>   kprobes: freelist.h removed
>   MAINTAINERS: objpool added
> 
>  MAINTAINERS  |   7 +
>  include/linux/freelist.h | 129 
>  include/linux/kprobes.h  |  11 +-
>  include/linux/objpool.h  | 174 ++
>  include/linux/rethook.h  |  16 +-
>  kernel/kprobes.c |  93 +++---
>  kernel/trace/fprobe.c|  32 +-
>  kernel/trace/rethook.c   |  90 +++--
>  lib/Kconfig.debug|  11 +
>  lib/Makefile |   4 +-
>  lib/objpool.c| 338 +++
>  lib/test_objpool.c   | 689 +++
>  12 files changed, 1320 insertions(+), 274 deletions(-)
>  delete mode 100644 include/linux/freelist.h
>  create mode 100644 include/linux/objpool.h
>  create mode 100644 lib/objpool.c
>  create mode 100644 lib/test_objpool.c
> 
> -- 
> 2.40.1
> 


-- 
Masami Hiramatsu (Google)