Re: [Xen-devel] [PATCH V2 2/2] xenbus: defer xenbus frontend resume if xenstored is not running

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.13 at 16:32, Aurelien Chartier <aurelien.chartier@xxxxxxxxxx> 
>>> wrote:
> --- a/drivers/xen/xenbus/xenbus_probe.h
> +++ b/drivers/xen/xenbus/xenbus_probe.h
> @@ -47,6 +47,11 @@ struct xen_bus_type {
>       struct bus_type bus;
>  };
>  
> +struct xenbus_resume_work {
> +     struct work_struct w;
> +     struct device *dev;
> +};
> +

I don't think this structure needs to be in a header - it's being used
in a single source file only.

> --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
> +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
> @@ -28,6 +28,7 @@
>  #include "xenbus_comms.h"
>  #include "xenbus_probe.h"
>  
> +static struct workqueue_struct *xenbus_frontend_resume_wq;
>  
>  /* device/<type>/<id> => <type>-<id> */
>  static int frontend_bus_id(char bus_id[XEN_BUS_ID_SIZE], const char 
> *nodename)
> @@ -89,9 +90,38 @@ static void backend_changed(struct xenbus_watch *watch,
>       xenbus_otherend_changed(watch, vec, len, 1);
>  }
>  
> +static void xenbus_frontend_delayed_resume(struct work_struct *w)
> +{
> +     struct xenbus_resume_work *resume_work = (struct xenbus_resume_work *) 
> w;

container_of()

> +
> +     xenbus_dev_resume(dev);

Does this build at all? I don't see where "dev" is being declared/
initialized?

> +
> +     kfree(w);

kfree(resume_work) - otherwise you have a hidden dependency
on "w" being the first member of struct xenbus_resume_work.

> +}
> +
> +static int xenbus_frontend_dev_resume(struct device *dev)
> +{
> +     /* 
> +      * If xenstored is running in that domain, we cannot access the backend
> +      * state at the moment, so we need to defer xenbus_dev_resume
> +      */
> +     if (xen_store_domain == XS_LOCAL) {
> +             struct xenbus_resume_work *work =
> +                     kmalloc(sizeof(struct xenbus_resume), GFP_KERNEL);

Missing NULL return check (don't know what you should do in that
case). Perhaps dynamic allocation is the wrong approach here, and
you want to rather add a new field to struct xenbus_device (which
gets populated only for frontend devices, and - if possible - only
when xen_store_domain == XS_LOCAL.

Also - GFP_KERNEL really suitable here?

> +
> +             INIT_WORK((struct work_struct *) work, 
> xenbus_frontend_delayed_resume);

&work->w (without any cast).

> +             resume_work->dev = dev;
> +             queue_work(xenbus_frontend_resume_wq, (struct work_struct *) 
> work)

Again.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel