| Message ID | 20220624152331.4009502-2-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: Enable Platform Runtime Mechanism(PRM) support on ARM64 | expand |
On Fri, 24 Jun 2022 at 17:23, Sudeep Holla <sudeep.holla@arm.com> wrote: > > Currently, the arch_efi_call_virt() assumes all users of it will have > defined a type 'efi_##f##_t' to make use of it. It is unnecessarily > forcing the users to create a new typedef when __efi_rt_asm_wrapper() > actually expects void pointer. > > Simplify the arch_efi_call_virt() macro by typecasting p->f to (void *) > as required by __efi_rt_asm_wrapper() and eliminate the explicit need > for efi_##f##_t type for every user of this macro. > Can't we just use typeof() here? __efi_rt_asm_wrapper() was intended as a temporary thing, so I'd prefer to avoid starting to rely on the void* type of its first argument. > This is needed now in preparation to enable PRMT support on ARM64. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > arch/arm64/include/asm/efi.h | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > Hi Ard, > > I am not sure if you prefer to add type for each users of this or this > is acceptable. I see only compile time advantage but I am not sure if > it make sense to add typedefs in ACPI PRMT driver just for this reason. > > Let me know. > > Regards, > Sudeep > > diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h > index ad55079abe47..263d7fd67207 100644 > --- a/arch/arm64/include/asm/efi.h > +++ b/arch/arm64/include/asm/efi.h > @@ -29,9 +29,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); > > #define arch_efi_call_virt(p, f, args...) \ > ({ \ > - efi_##f##_t *__f; \ > - __f = p->f; \ > - __efi_rt_asm_wrapper(__f, #f, args); \ > + __efi_rt_asm_wrapper((void *)p->f, #f, args); \ > }) > > #define arch_efi_call_virt_teardown() \ > -- > 2.36.1 >
On Fri, Jun 24, 2022 at 07:45:14PM +0200, Ard Biesheuvel wrote: > On Fri, 24 Jun 2022 at 17:23, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > Currently, the arch_efi_call_virt() assumes all users of it will have > > defined a type 'efi_##f##_t' to make use of it. It is unnecessarily > > forcing the users to create a new typedef when __efi_rt_asm_wrapper() > > actually expects void pointer. > > > > Simplify the arch_efi_call_virt() macro by typecasting p->f to (void *) > > as required by __efi_rt_asm_wrapper() and eliminate the explicit need > > for efi_##f##_t type for every user of this macro. > > > > Can't we just use typeof() here? I had tried that, but unless p->f is pointer of some type, we will get the warning as it is passed without a cast to __efi_rt_asm_wrapper(). > __efi_rt_asm_wrapper() was intended as a temporary thing, so I'd > prefer to avoid starting to rely on the void* type of its first > argument. > Fair enough. Can we expect p->f to be some pointer then ? If yes, then PRMT driver needs to change the handler_addr from u64 to some pointer which sounds OK to me. -- Regards, Sudeep
On Sat, 25 Jun 2022 at 12:11, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Fri, Jun 24, 2022 at 07:45:14PM +0200, Ard Biesheuvel wrote: > > On Fri, 24 Jun 2022 at 17:23, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > Currently, the arch_efi_call_virt() assumes all users of it will have > > > defined a type 'efi_##f##_t' to make use of it. It is unnecessarily > > > forcing the users to create a new typedef when __efi_rt_asm_wrapper() > > > actually expects void pointer. > > > > > > Simplify the arch_efi_call_virt() macro by typecasting p->f to (void *) > > > as required by __efi_rt_asm_wrapper() and eliminate the explicit need > > > for efi_##f##_t type for every user of this macro. > > > > > > > Can't we just use typeof() here? > > I had tried that, but unless p->f is pointer of some type, we will get > the warning as it is passed without a cast to __efi_rt_asm_wrapper(). > > > __efi_rt_asm_wrapper() was intended as a temporary thing, so I'd > > prefer to avoid starting to rely on the void* type of its first > > argument. > > > > Fair enough. Can we expect p->f to be some pointer then ? If yes, then > PRMT driver needs to change the handler_addr from u64 to some pointer > which sounds OK to me. > We are dealing with function pointers here, so passing those as u64 is just sloppy.
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index ad55079abe47..263d7fd67207 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -29,9 +29,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); #define arch_efi_call_virt(p, f, args...) \ ({ \ - efi_##f##_t *__f; \ - __f = p->f; \ - __efi_rt_asm_wrapper(__f, #f, args); \ + __efi_rt_asm_wrapper((void *)p->f, #f, args); \ }) #define arch_efi_call_virt_teardown() \
Currently, the arch_efi_call_virt() assumes all users of it will have defined a type 'efi_##f##_t' to make use of it. It is unnecessarily forcing the users to create a new typedef when __efi_rt_asm_wrapper() actually expects void pointer. Simplify the arch_efi_call_virt() macro by typecasting p->f to (void *) as required by __efi_rt_asm_wrapper() and eliminate the explicit need for efi_##f##_t type for every user of this macro. This is needed now in preparation to enable PRMT support on ARM64. Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- arch/arm64/include/asm/efi.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) Hi Ard, I am not sure if you prefer to add type for each users of this or this is acceptable. I see only compile time advantage but I am not sure if it make sense to add typedefs in ACPI PRMT driver just for this reason. Let me know. Regards, Sudeep