Re: [PATCH 1/2] net: ethernet: ti: cpts: convert to use kthread_worker

[Richard Cochran]

On Mon, Jul 24, 2017 at 07:34:38PM -0500, Grygorii Strashko wrote:
> Below if pure TBD/RFC version of patch which add kthread worker to PTP core.
> I'm sending it to get you opinion about implementation in general, before 
> continue with more changes. Pls, take a look if you have time?
> - are you ok with names (API, callbacks, ptp structs members)?

The API and naming looks good to me.
 
> I can prepare, update and resend proper patches tom if feedback is positive.

Please do.

> I also can convert dp83640 driver to use new feature, but I can't test it.

No need for that.  It would be enough to have cpts as the first user
and example.
 
> +     if (ptp->info->do_aux_work) {
> +             struct sched_param param = {
> +                     .sched_priority = MAX_RT_PRIO - 1 };
> +
> +             kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
> +             ptp->kworker = kthread_create_worker(0, info->name);
> +             if (IS_ERR(ptp->kworker)) {
> +                     pr_err("failed to create ptp aux_worker task %ld\n",
> +                            PTR_ERR(ptp->kworker));
> +                     return ERR_CAST(ptp->kworker);
> +             }
> +             err = sched_setscheduler_nocheck(ptp->kworker->task,
> +                                              SCHED_FIFO, &param);

I think we should not hard code the scheduler and priority here but
rather leave it to the sysadmin to configure these using chrt(1).
After all, a normal work item is has served just in many situations.

> +             if (err)
> +                     pr_err("sched_setscheduler_nocheck err %d\n", err);
> +     }
> +
>       err = ptp_populate_pin_groups(ptp);
>       if (err)
>               goto no_pin_groups;
> @@ -274,6 +305,9 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
>       ptp->defunct = 1;
>       wake_up_interruptible(&ptp->tsev_wq);
>  
> +     kthread_cancel_delayed_work_sync(&ptp->aux_work);
> +     kthread_destroy_worker(ptp->kworker);

These can't be called unconditionally.

>       /* Release the clock's resources. */
>       if (ptp->pps_source)
>               pps_unregister_source(ptp->pps_source);

Thanks,
Richard