Re: [PATCH 1/1] powerpc/pseries: Use the system workqueue as fallback to hotplug workqueue

[joserz]

On Fri, Dec 22, 2017 at 11:54:10AM +1100, David Gibson wrote:
> On Thu, Dec 21, 2017 at 01:44:48PM -0200, Jose Ricardo Ziviani wrote:
> > The hotplug engine uses its own workqueue to handle IRQ requests, the
> > problem is that such workqueue is initialized not so early in the boot
> > process.
> > 
> > Thus, when the kernel is ready to handle IRQ requests, after the system
> > workqueue is initialized, we have a timeframe where any hotplug issued
> > by the client will result in a kernel panic. That timeframe goes until
> > the hotplug workqueue is initialized.
> > 
> > It would be good to have the hotplug workqueue initialized as soon as
> > the system workqueue but I don't think it is possible. So, this patch
> > uses the system workqueue as a fallback the handle such IRQs.
> > 
> > Signed-off-by: Jose Ricardo Ziviani 
> 
> I don't think this is the right approach.
> 
> It seems to me the bug is that the hotplug interrupt is registered in
> init_ras_IRQ(), before the work queue is initialized in
> pseries_dlpar_init().  We need to correct that ordering.
> 

Oh, this makes much more sense. I'm going to make some tests and send a
v2 then.

Thanks for reviewing it!

> > ---
> >  arch/powerpc/platforms/pseries/dlpar.c | 10 +-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/platforms/pseries/dlpar.c 
> > b/arch/powerpc/platforms/pseries/dlpar.c
> > index 6e35780c5962..0474aa14b5f6 100644
> > --- a/arch/powerpc/platforms/pseries/dlpar.c
> > +++ b/arch/powerpc/platforms/pseries/dlpar.c
> > @@ -399,7 +399,15 @@ void queue_hotplug_event(struct pseries_hp_errorlog 
> > *hp_errlog,
> > work->errlog = hp_errlog_copy;
> > work->hp_completion = hotplug_done;
> > work->rc = rc;
> > -   queue_work(pseries_hp_wq, (struct work_struct *)work);
> > +
> > +   /* The hotplug workqueue may happen to be NULL at the moment
> > +* this code is executed, during the boot phase. So, in this
> > +* scenario, we can fallback to the system workqueue.
> > +*/
> > +   if (unlikely(pseries_hp_wq == NULL))
> > +   schedule_work((struct work_struct *)work);
> > +   else
> > +   queue_work(pseries_hp_wq, (struct work_struct *)work);
> > } else {
> > *rc = -ENOMEM;
> > kfree(hp_errlog_copy);
> 
> -- 
> David Gibson  | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au| minimalist, thank you.  NOT _the_ 
> _other_
>   | _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [PATCH 1/1] powerpc/pseries: Use the system workqueue as fallback to hotplug workqueue

[David Gibson]

On Thu, Dec 21, 2017 at 01:44:48PM -0200, Jose Ricardo Ziviani wrote:
> The hotplug engine uses its own workqueue to handle IRQ requests, the
> problem is that such workqueue is initialized not so early in the boot
> process.
> 
> Thus, when the kernel is ready to handle IRQ requests, after the system
> workqueue is initialized, we have a timeframe where any hotplug issued
> by the client will result in a kernel panic. That timeframe goes until
> the hotplug workqueue is initialized.
> 
> It would be good to have the hotplug workqueue initialized as soon as
> the system workqueue but I don't think it is possible. So, this patch
> uses the system workqueue as a fallback the handle such IRQs.
> 
> Signed-off-by: Jose Ricardo Ziviani 

I don't think this is the right approach.

It seems to me the bug is that the hotplug interrupt is registered in
init_ras_IRQ(), before the work queue is initialized in
pseries_dlpar_init().  We need to correct that ordering.

> ---
>  arch/powerpc/platforms/pseries/dlpar.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c 
> b/arch/powerpc/platforms/pseries/dlpar.c
> index 6e35780c5962..0474aa14b5f6 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -399,7 +399,15 @@ void queue_hotplug_event(struct pseries_hp_errorlog 
> *hp_errlog,
>   work->errlog = hp_errlog_copy;
>   work->hp_completion = hotplug_done;
>   work->rc = rc;
> - queue_work(pseries_hp_wq, (struct work_struct *)work);
> +
> + /* The hotplug workqueue may happen to be NULL at the moment
> +  * this code is executed, during the boot phase. So, in this
> +  * scenario, we can fallback to the system workqueue.
> +  */
> + if (unlikely(pseries_hp_wq == NULL))
> + schedule_work((struct work_struct *)work);
> + else
> + queue_work(pseries_hp_wq, (struct work_struct *)work);
>   } else {
>   *rc = -ENOMEM;
>   kfree(hp_errlog_copy);

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[PATCH 1/1] powerpc/pseries: Use the system workqueue as fallback to hotplug workqueue

[Jose Ricardo Ziviani]

The hotplug engine uses its own workqueue to handle IRQ requests, the
problem is that such workqueue is initialized not so early in the boot
process.

Thus, when the kernel is ready to handle IRQ requests, after the system
workqueue is initialized, we have a timeframe where any hotplug issued
by the client will result in a kernel panic. That timeframe goes until
the hotplug workqueue is initialized.

It would be good to have the hotplug workqueue initialized as soon as
the system workqueue but I don't think it is possible. So, this patch
uses the system workqueue as a fallback the handle such IRQs.

Signed-off-by: Jose Ricardo Ziviani 
---
 arch/powerpc/platforms/pseries/dlpar.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c 
b/arch/powerpc/platforms/pseries/dlpar.c
index 6e35780c5962..0474aa14b5f6 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -399,7 +399,15 @@ void queue_hotplug_event(struct pseries_hp_errorlog 
*hp_errlog,
work->errlog = hp_errlog_copy;
work->hp_completion = hotplug_done;
work->rc = rc;
-   queue_work(pseries_hp_wq, (struct work_struct *)work);
+
+   /* The hotplug workqueue may happen to be NULL at the moment
+* this code is executed, during the boot phase. So, in this
+* scenario, we can fallback to the system workqueue.
+*/
+   if (unlikely(pseries_hp_wq == NULL))
+   schedule_work((struct work_struct *)work);
+   else
+   queue_work(pseries_hp_wq, (struct work_struct *)work);
} else {
*rc = -ENOMEM;
kfree(hp_errlog_copy);
-- 
2.14.1