Re: [PATCH v5 05/22] powerpc/irq: Add helper to set regs->softe
Excerpts from Christophe Leroy's message of March 5, 2021 6:54 pm: > > > Le 09/02/2021 à 08:49, Nicholas Piggin a écrit : >> Excerpts from Christophe Leroy's message of February 9, 2021 4:18 pm: >>> >>> >>> Le 09/02/2021 à 02:11, Nicholas Piggin a écrit : Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am: > regs->softe doesn't exist on PPC32. > > Add irq_soft_mask_regs_set_state() helper to set regs->softe. > This helper will void on PPC32. > > Signed-off-by: Christophe Leroy > --- You could do the same with the kuap_ functions to change some ifdefs to IS_ENABLED. That's just my preference but if you prefer this way I guess that's okay. >>> >>> >>> That's also my preference on the long term. >>> >>> Here it is ephemeral, I have a follow up series implementing interrupt >>> exit/entry in C and getting >>> rid of all the assembly kuap hence getting rid of those ifdefs. >> >> I thought it might have been because you hate ifdef more tha most :) >> >>> The issue I see when using IS_ENABLED() is that you have to indent to the >>> right, then you interfere >>> with the file history and 'git blame' >> >> Valid point if it's just going to indent back the other way in your next >> series. >> >>> Thanks for reviewing my series and looking forward to your feedback on my >>> series on the interrupt >>> entry/exit that I will likely release later today. >> >> Cool, I'm eager to see them. >> > > Hi Nick, have you been able to look at it ? > > https://patchwork.ozlabs.org/project/linuxppc-dev/cover/cover.1612864003.git.christophe.le...@csgroup.eu/ Hi Christophe, I had a look at it, it's mostly ppc32 code which I don't know well but it looks like a very nice cleanup and it's good to be sharing the C code here. All the common code changes look fine to me. I'll take a closer look if you can rebase and repost the series I need to create a tree and base 64e conversion on top of yours as they touch the same common places. Thanks, Nick
Re: [PATCH v5 05/22] powerpc/irq: Add helper to set regs->softe
Le 09/02/2021 à 08:49, Nicholas Piggin a écrit : Excerpts from Christophe Leroy's message of February 9, 2021 4:18 pm: Le 09/02/2021 à 02:11, Nicholas Piggin a écrit : Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am: regs->softe doesn't exist on PPC32. Add irq_soft_mask_regs_set_state() helper to set regs->softe. This helper will void on PPC32. Signed-off-by: Christophe Leroy --- You could do the same with the kuap_ functions to change some ifdefs to IS_ENABLED. That's just my preference but if you prefer this way I guess that's okay. That's also my preference on the long term. Here it is ephemeral, I have a follow up series implementing interrupt exit/entry in C and getting rid of all the assembly kuap hence getting rid of those ifdefs. I thought it might have been because you hate ifdef more tha most :) The issue I see when using IS_ENABLED() is that you have to indent to the right, then you interfere with the file history and 'git blame' Valid point if it's just going to indent back the other way in your next series. Thanks for reviewing my series and looking forward to your feedback on my series on the interrupt entry/exit that I will likely release later today. Cool, I'm eager to see them. Hi Nick, have you been able to look at it ? https://patchwork.ozlabs.org/project/linuxppc-dev/cover/cover.1612864003.git.christophe.le...@csgroup.eu/ Thanks Christophe
Re: [PATCH v5 05/22] powerpc/irq: Add helper to set regs->softe
Excerpts from Christophe Leroy's message of February 9, 2021 4:18 pm: > > > Le 09/02/2021 à 02:11, Nicholas Piggin a écrit : >> Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am: >>> regs->softe doesn't exist on PPC32. >>> >>> Add irq_soft_mask_regs_set_state() helper to set regs->softe. >>> This helper will void on PPC32. >>> >>> Signed-off-by: Christophe Leroy >>> --- >> >> You could do the same with the kuap_ functions to change some ifdefs >> to IS_ENABLED. >> >> That's just my preference but if you prefer this way I guess that's >> okay. >> > > > That's also my preference on the long term. > > Here it is ephemeral, I have a follow up series implementing interrupt > exit/entry in C and getting > rid of all the assembly kuap hence getting rid of those ifdefs. I thought it might have been because you hate ifdef more tha most :) > The issue I see when using IS_ENABLED() is that you have to indent to the > right, then you interfere > with the file history and 'git blame' Valid point if it's just going to indent back the other way in your next series. > Thanks for reviewing my series and looking forward to your feedback on my > series on the interrupt > entry/exit that I will likely release later today. Cool, I'm eager to see them. Thanks, Nick
Re: [PATCH v5 05/22] powerpc/irq: Add helper to set regs->softe
Excerpts from Christophe Leroy's message of February 9, 2021 3:57 pm:
>
>
> Le 09/02/2021 à 02:11, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am:
>>> regs->softe doesn't exist on PPC32.
>>>
>>> Add irq_soft_mask_regs_set_state() helper to set regs->softe.
>>> This helper will void on PPC32.
>>>
>>> Signed-off-by: Christophe Leroy
>>> ---
>>> arch/powerpc/include/asm/hw_irq.h | 11 +--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/hw_irq.h
>>> b/arch/powerpc/include/asm/hw_irq.h
>>> index 614957f74cee..ed0c3b049dfd 100644
>>> --- a/arch/powerpc/include/asm/hw_irq.h
>>> +++ b/arch/powerpc/include/asm/hw_irq.h
>>> @@ -38,6 +38,8 @@
>>> #define PACA_IRQ_MUST_HARD_MASK (PACA_IRQ_EE)
>>> #endif
>>>
>>> +#endif /* CONFIG_PPC64 */
>>> +
>>> /*
>>>* flags for paca->irq_soft_mask
>>>*/
>>> @@ -46,8 +48,6 @@
>>> #define IRQS_PMI_DISABLED 2
>>> #define IRQS_ALL_DISABLED (IRQS_DISABLED | IRQS_PMI_DISABLED)
>>>
>>> -#endif /* CONFIG_PPC64 */
>>> -
>>> #ifndef __ASSEMBLY__
>>>
>>> #ifdef CONFIG_PPC64
>>> @@ -287,6 +287,10 @@ extern void irq_set_pending_from_srr1(unsigned long
>>> srr1);
>>>
>>> extern void force_external_irq_replay(void);
>>>
>>> +static inline void irq_soft_mask_regs_set_state(struct pt_regs *regs,
>>> unsigned long val)
>>> +{
>>> + regs->softe = val;
>>> +}
>>> #else /* CONFIG_PPC64 */
>>>
>>> static inline unsigned long arch_local_save_flags(void)
>>> @@ -355,6 +359,9 @@ static inline bool arch_irq_disabled_regs(struct
>>> pt_regs *regs)
>>>
>>> static inline void may_hard_irq_enable(void) { }
>>>
>>> +static inline void irq_soft_mask_regs_set_state(struct pt_regs *regs,
>>> unsigned long val)
>>> +{
>>> +}
>>> #endif /* CONFIG_PPC64 */
>>>
>>> #define ARCH_IRQ_INIT_FLAGS IRQ_NOREQUEST
>>
>> What I don't like about this where you use it is it kind of pollutes
>> the ppc32 path with this function which is not valid to use.
>>
>> I would prefer if you had this purely so it could compile with:
>>
>>if (IS_ENABLED(CONFIG_PPC64)))
>>irq_soft_mask_regs_set_state(regs, blah);
>>
>> And then you could make the ppc32 cause a link error if it did not
>> get eliminated at compile time (e.g., call an undefined function).
>>
>> You could do the same with the kuap_ functions to change some ifdefs
>> to IS_ENABLED.
>>
>> That's just my preference but if you prefer this way I guess that's
>> okay.
>
> I see you didn't change your mind since last April :)
>
> I'll see what I can do.
If you have more patches in the works and will do some cleanup passes I
don't mind so much.
Thanks,
Nick
Re: [PATCH v5 05/22] powerpc/irq: Add helper to set regs->softe
Le 09/02/2021 à 02:11, Nicholas Piggin a écrit : Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am: regs->softe doesn't exist on PPC32. Add irq_soft_mask_regs_set_state() helper to set regs->softe. This helper will void on PPC32. Signed-off-by: Christophe Leroy --- You could do the same with the kuap_ functions to change some ifdefs to IS_ENABLED. That's just my preference but if you prefer this way I guess that's okay. That's also my preference on the long term. Here it is ephemeral, I have a follow up series implementing interrupt exit/entry in C and getting rid of all the assembly kuap hence getting rid of those ifdefs. The issue I see when using IS_ENABLED() is that you have to indent to the right, then you interfere with the file history and 'git blame' Thanks for reviewing my series and looking forward to your feedback on my series on the interrupt entry/exit that I will likely release later today. Christophe
Re: [PATCH v5 05/22] powerpc/irq: Add helper to set regs->softe
Le 09/02/2021 à 02:11, Nicholas Piggin a écrit :
Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am:
regs->softe doesn't exist on PPC32.
Add irq_soft_mask_regs_set_state() helper to set regs->softe.
This helper will void on PPC32.
Signed-off-by: Christophe Leroy
---
arch/powerpc/include/asm/hw_irq.h | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/hw_irq.h
b/arch/powerpc/include/asm/hw_irq.h
index 614957f74cee..ed0c3b049dfd 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -38,6 +38,8 @@
#define PACA_IRQ_MUST_HARD_MASK (PACA_IRQ_EE)
#endif
+#endif /* CONFIG_PPC64 */
+
/*
* flags for paca->irq_soft_mask
*/
@@ -46,8 +48,6 @@
#define IRQS_PMI_DISABLED 2
#define IRQS_ALL_DISABLED (IRQS_DISABLED | IRQS_PMI_DISABLED)
-#endif /* CONFIG_PPC64 */
-
#ifndef __ASSEMBLY__
#ifdef CONFIG_PPC64
@@ -287,6 +287,10 @@ extern void irq_set_pending_from_srr1(unsigned long srr1);
extern void force_external_irq_replay(void);
+static inline void irq_soft_mask_regs_set_state(struct pt_regs *regs, unsigned long val)
+{
+ regs->softe = val;
+}
#else /* CONFIG_PPC64 */
static inline unsigned long arch_local_save_flags(void)
@@ -355,6 +359,9 @@ static inline bool arch_irq_disabled_regs(struct pt_regs
*regs)
static inline void may_hard_irq_enable(void) { }
+static inline void irq_soft_mask_regs_set_state(struct pt_regs *regs, unsigned long val)
+{
+}
#endif /* CONFIG_PPC64 */
#define ARCH_IRQ_INIT_FLAGS IRQ_NOREQUEST
What I don't like about this where you use it is it kind of pollutes
the ppc32 path with this function which is not valid to use.
I would prefer if you had this purely so it could compile with:
if (IS_ENABLED(CONFIG_PPC64)))
irq_soft_mask_regs_set_state(regs, blah);
And then you could make the ppc32 cause a link error if it did not
get eliminated at compile time (e.g., call an undefined function).
You could do the same with the kuap_ functions to change some ifdefs
to IS_ENABLED.
That's just my preference but if you prefer this way I guess that's
okay.
I see you didn't change your mind since last April :)
I'll see what I can do.
Christophe
Re: [PATCH v5 05/22] powerpc/irq: Add helper to set regs->softe
Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am:
> regs->softe doesn't exist on PPC32.
>
> Add irq_soft_mask_regs_set_state() helper to set regs->softe.
> This helper will void on PPC32.
>
> Signed-off-by: Christophe Leroy
> ---
> arch/powerpc/include/asm/hw_irq.h | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hw_irq.h
> b/arch/powerpc/include/asm/hw_irq.h
> index 614957f74cee..ed0c3b049dfd 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -38,6 +38,8 @@
> #define PACA_IRQ_MUST_HARD_MASK (PACA_IRQ_EE)
> #endif
>
> +#endif /* CONFIG_PPC64 */
> +
> /*
> * flags for paca->irq_soft_mask
> */
> @@ -46,8 +48,6 @@
> #define IRQS_PMI_DISABLED2
> #define IRQS_ALL_DISABLED(IRQS_DISABLED | IRQS_PMI_DISABLED)
>
> -#endif /* CONFIG_PPC64 */
> -
> #ifndef __ASSEMBLY__
>
> #ifdef CONFIG_PPC64
> @@ -287,6 +287,10 @@ extern void irq_set_pending_from_srr1(unsigned long
> srr1);
>
> extern void force_external_irq_replay(void);
>
> +static inline void irq_soft_mask_regs_set_state(struct pt_regs *regs,
> unsigned long val)
> +{
> + regs->softe = val;
> +}
> #else /* CONFIG_PPC64 */
>
> static inline unsigned long arch_local_save_flags(void)
> @@ -355,6 +359,9 @@ static inline bool arch_irq_disabled_regs(struct pt_regs
> *regs)
>
> static inline void may_hard_irq_enable(void) { }
>
> +static inline void irq_soft_mask_regs_set_state(struct pt_regs *regs,
> unsigned long val)
> +{
> +}
> #endif /* CONFIG_PPC64 */
>
> #define ARCH_IRQ_INIT_FLAGS IRQ_NOREQUEST
What I don't like about this where you use it is it kind of pollutes
the ppc32 path with this function which is not valid to use.
I would prefer if you had this purely so it could compile with:
if (IS_ENABLED(CONFIG_PPC64)))
irq_soft_mask_regs_set_state(regs, blah);
And then you could make the ppc32 cause a link error if it did not
get eliminated at compile time (e.g., call an undefined function).
You could do the same with the kuap_ functions to change some ifdefs
to IS_ENABLED.
That's just my preference but if you prefer this way I guess that's
okay.
Thanks,
Nick
