netdev - Re: [PATCH 1/2] net: ethernet: ti: cpts: convert to use kthread

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170725043134.dxootdk66iqov7jh@localhost.localdomain>
Date:   Tue, 25 Jul 2017 06:31:34 +0200
From:   Richard Cochran <richardcochran@...il.com>
To:     Grygorii Strashko <grygorii.strashko@...com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Sekhar Nori <nsekhar@...com>, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org, Wingman Kwok <w-kwok2@...com>,
        Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>
Subject: Re: [PATCH 1/2] net: ethernet: ti: cpts: convert to use
 kthread_worker

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