Hi,
On Thu, Apr 21, 2022 at 07:40:56AM +0000, Linyu Yuan (QUIC) wrote:
> > > > 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.
OK, that's a good point. So just re-schedule the work like you do.
thanks,
--
heikki
