Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753052AbcDVIlN (ORCPT ); Fri, 22 Apr 2016 04:41:13 -0400 Received: from mx2.suse.de ([195.135.220.15]:35409 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750806AbcDVIlL (ORCPT ); Fri, 22 Apr 2016 04:41:11 -0400 Date: Fri, 22 Apr 2016 10:41:06 +0200 From: Petr Mladek To: Sergey Senozhatsky Cc: Pan Xinhui , Sergey Senozhatsky , Andrew Morton , Jan Kara , Tejun Heo , Tetsuo Handa , linux-kernel@vger.kernel.org, Byungchul Park Subject: Re: [PATCH v11 3/3] printk: make printk.synchronous param rw Message-ID: <20160422084106.GC2749@pathway.suse.cz> References: <1460050307-3718-1-git-send-email-sergey.senozhatsky@gmail.com> <1460050307-3718-4-git-send-email-sergey.senozhatsky@gmail.com> <57072DE7.50806@linux.vnet.ibm.com> <20160408052927.GA614@swordfish> <20160421110715.GA2749@pathway.suse.cz> <20160422012826.GA515@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160422012826.GA515@swordfish> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2201 Lines: 69 On Fri 2016-04-22 10:28:26, Sergey Senozhatsky wrote: > Hello, > > On (04/21/16 13:07), Petr Mladek wrote: > [..] > > Please, what is the purpose of "printk_initcall_done" then? If I get > > this correctly, it will always be true when printk_sync_set() is > > called. > > well, this is a bit ugly, yes. kernel_param_ops defines ->set callback > as printk_sync_set(). and this ->set callback is getting called from 2 > different paths (but it's really about underlying __init_printk_kthread()). > > __init_printk_kthread() can be executed from: Ah, I see and feel shame. It is actually explained in the comment above printk_initcall_done declaration. Well, the explanation confused me a bit ;-) I suggest to change it sligtly: /* * printk_sync_set() can be called from two places: * * - early from start_kernel()->parse_args(). But we can't kthread_run() * at this stage, so we just set the param value. The actual * initalization happens later, from the late_initcall(). * * - even later from user space via sysfs knob. We can kthread_run() * there if needed. */ Or we could write this even more explicitely: /* * Prevent starting kthreads from start_kernel()->parse_args(). It is not * possible at this stage. Instead do so via the inticall. */ static bool printk_initcall_done; In each case, I would move the comment and the declaration right above the printk_sync_set(). > alternatively, I had this idea to re-define ->set() callback in init_printk_kthread(). > > IOW, by default we use param_set_bool(), so parse_args() will not cause any > problems: > > static /*** can't 'const' anymore ***/ struct kernel_param_ops param_ops_printk_sync = { > .set = param_set_bool, > .get = param_get_bool, > }; > > and change it to printk_sync_set() in: > > static __init int init_printk_kthread(void) > { > param_ops_printk_sync.set = printk_sync_set; > return __init_printk_kthread(); > } > > but I think that this bool flag is simpler to understand after all. In addition, there would be problems to handle a possible change via the sysfs knob. The bool flag looks much better to me :-) Thanks a lot for detailed explanation. Best Regards, Petr