Re: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when module load late

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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