Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Use workqueue for port update

[Guenter Roeck]

On Mon, Jun 29, 2020 at 9:13 AM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
>
> Use a work queue to call the port update routines, instead of doing it
> directly in the PD notifier callback. This will prevent other drivers
> with PD notifier callbacks from being blocked on the port update routine
> completing.
>
> Signed-off-by: Prashant Malani <pmalani@xxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - No changes.
>
>  drivers/platform/chrome/cros_ec_typec.c | 28 ++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 0c041b79cbba..630170fb2cbe 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -58,6 +58,7 @@ struct cros_typec_data {
>         /* Array of ports, indexed by port number. */
>         struct cros_typec_port *ports[EC_USB_PD_MAX_PORTS];
>         struct notifier_block nb;
> +       struct work_struct port_work;
>  };
>
>  static int cros_typec_parse_port_props(struct typec_capability *cap,
> @@ -619,18 +620,29 @@ static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
>         return 0;
>  }
>
> -static int cros_ec_typec_event(struct notifier_block *nb,
> -                              unsigned long host_event, void *_notify)
> +static void cros_typec_port_work(struct work_struct *work)
>  {
> -       struct cros_typec_data *typec = container_of(nb, struct cros_typec_data,
> -                                                    nb);
> -       int ret, i;
> +       struct cros_typec_data *typec = container_of(work,
> +                                                    struct cros_typec_data,
> +                                                    port_work);

Nit, but the upstream line length limit is now 100 characters, and
this is a perfect example where this would come handy.

> +       int ret;
> +       int i;
>
>         for (i = 0; i < typec->num_ports; i++) {
>                 ret = cros_typec_port_update(typec, i);
>                 if (ret < 0)
>                         dev_warn(typec->dev, "Update failed for port: %d\n", i);
>         }
> +}
> +
> +
> +static int cros_ec_typec_event(struct notifier_block *nb,
> +                              unsigned long host_event, void *_notify)
> +{
> +       struct cros_typec_data *typec = container_of(nb, struct cros_typec_data,
> +                                                    nb);
> +
... and even more so here.

> +       schedule_work(&typec->port_work);
>
>         return NOTIFY_OK;
>  }
> @@ -689,6 +701,12 @@ static int cros_typec_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 return ret;
>
> +       INIT_WORK(&typec->port_work, cros_typec_port_work);
> +
> +       /*
> +        * Safe to call port update here, since we haven't registered the
> +        * PD notifier yet.
> +        */
>         for (i = 0; i < typec->num_ports; i++) {
>                 ret = cros_typec_port_update(typec, i);
>                 if (ret < 0)
> --
> 2.27.0.212.ge8ba1cc988-goog
>