> From: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
> Sent: Wednesday, April 13, 2022 9:43 PM
> To: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>; Linyu Yuan (QUIC)
> <quic_linyyuan@xxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-
> usb@xxxxxxxxxxxxxxx; Jack Pham (QUIC) <quic_jackp@xxxxxxxxxxx>
> Subject: RE: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when
> module load late
>
> > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, April 13, 2022 8:40 PM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-
> > usb@xxxxxxxxxxxxxxx; Jack Pham (QUIC) <quic_jackp@xxxxxxxxxxx>
> > Subject: Re: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when
> > module load late
> >
> > Hi,
> >
> > You have to make the subject line a bit more clear. Perhaps you could
> > just say "Wait for the USB role switches".
>
> Sound great, will change once all discussion in this thread done.
>
> >
> > On Wed, Apr 13, 2022 at 05:58:11PM +0800, Linyu Yuan wrote:
> > > When role switch enabled, return -EAGAIN if fail to find it due to
> > > module load ordering issue, then restart ucsi init work to find
> > > it again every 100ms.
> >
> > It looks like you did not update that.
>
> Sorry, will update later.
>
> >
> > > It also means change ucsi init work to delayed_work.
> >
> > So from where does that 100ms come from?
>
> Yes, no place define it, just an experiment value.
> Any suggestion ?
>
> >
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> > > ---
> > > v2: keep original con->num in debug log
> > > v3: change return value from -EAGAIN to PTR_ERR()
> > >
> > > drivers/usb/typec/ucsi/ucsi.c | 28 ++++++++++++++++------------
> > > drivers/usb/typec/ucsi/ucsi.h | 2 +-
> > > 2 files changed, 17 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > index ce9192e..63c25dd 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > @@ -1053,6 +1053,14 @@ static int ucsi_register_port(struct ucsi *ucsi,
> int
> > index)
> > > con->num = index + 1;
> > > con->ucsi = ucsi;
> > >
> > > + cap->fwnode = ucsi_find_fwnode(con);
> > > + con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> > > + if (IS_ERR(con->usb_role_sw)) {
> > > + dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> > > + con->num);
> > > + return PTR_ERR(con->usb_role_sw);
> > > + }
> > > +
> > > /* Delay other interactions with the con until registration is complete
> > */
> > > mutex_lock(&con->lock);
> > >
> > > @@ -1088,7 +1096,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> > index)
> > > if (con->cap.op_mode &
> > UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
> > > *accessory = TYPEC_ACCESSORY_DEBUG;
> > >
> > > - cap->fwnode = ucsi_find_fwnode(con);
> > > cap->driver_data = con;
> > > cap->ops = &ucsi_ops;
> > >
> > > @@ -1147,13 +1154,6 @@ static int ucsi_register_port(struct ucsi *ucsi,
> int
> > index)
> > > ucsi_port_psy_changed(con);
> > > }
> > >
> > > - con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> > > - if (IS_ERR(con->usb_role_sw)) {
> > > - dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> > > - con->num);
> > > - con->usb_role_sw = NULL;
> > > - }
> > > -
> > > /* Only notify USB controller if partner supports USB data */
> > > if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
> > UCSI_CONSTAT_PARTNER_FLAG_USB))
> > > u_role = USB_ROLE_NONE;
> > > @@ -1286,12 +1286,16 @@ static int ucsi_init(struct ucsi *ucsi)
> > >
> > > static void ucsi_init_work(struct work_struct *work)
> > > {
> > > - struct ucsi *ucsi = container_of(work, struct ucsi, work);
> > > + struct ucsi *ucsi = container_of(work, struct ucsi, work.work);
> > > int ret;
> > >
> > > ret = ucsi_init(ucsi);
> > > if (ret)
> > > dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
> > > +
> > > +
> >
> > Extra line.
> >
> > > + if (ret == -EPROBE_DEFER)
> > > + queue_delayed_work(system_long_wq, &ucsi->work,
> > HZ/10);
> >
> > You have to stop queueing that eventually. You need a counter.
> >
> > I'm still not sure why do you want to queue this same work again and
> > again? Why not just call ucsi_init() in a loop?
>
> Will follow your suggestion below.
>
> >
> > int my_counter = 1000;
> So you prefer 10 second in total ?
> If so, next version, I will change it to 500, as msleep(20), check next.
> >
> > while (ucsi_init(ucsi) == -EPROBE_DEFER && my_counter--)
> > msleep(10);
> In checkpatch.pl, it suggest msleep no less than 20ms each time.
> msleep(20) ?
>
> >
> > > }
>
> Great idea, it will be less code change.
Sorry, I think there is one concern of your suggestion is that
As the while loop inside the worker, if we can this worker,
It may spent too much time.
Although my original design is crazy(queue worker again inside it),
but it allow cancel worker wait short time.
please let me know what is your suggestion now.
>
> > >
> > > /**
> > > @@ -1331,7 +1335,7 @@ struct ucsi *ucsi_create(struct device *dev,
> const
> > struct ucsi_operations *ops)
> > > if (!ucsi)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > - INIT_WORK(&ucsi->work, ucsi_init_work);
> > > + INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
> > > mutex_init(&ucsi->ppm_lock);
> > > ucsi->dev = dev;
> > > ucsi->ops = ops;
> > > @@ -1366,7 +1370,7 @@ int ucsi_register(struct ucsi *ucsi)
> > > if (!ucsi->version)
> > > return -ENODEV;
> > >
> > > - queue_work(system_long_wq, &ucsi->work);
> > > + queue_delayed_work(system_long_wq, &ucsi->work, 0);
> > >
> > > return 0;
> > > }
> > > @@ -1383,7 +1387,7 @@ void ucsi_unregister(struct ucsi *ucsi)
> > > u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
> > >
> > > /* Make sure that we are not in the middle of driver initialization */
> > > - cancel_work_sync(&ucsi->work);
> > > + cancel_delayed_work_sync(&ucsi->work);
> > >
> > > /* Disable notifications */
> > > ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > > index 280f1e1..3812017 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.h
> > > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > > @@ -287,7 +287,7 @@ struct ucsi {
> > > struct ucsi_capability cap;
> > > struct ucsi_connector *connector;
> > >
> > > - struct work_struct work;
> > > + struct delayed_work work;
> > >
> > > /* PPM Communication lock */
> > > struct mutex ppm_lock;
> > > --
> > > 2.7.4
> >
> > thanks,
> >
> > --
> > heikki
