Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933674AbdDGPpm (ORCPT ); Fri, 7 Apr 2017 11:45:42 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:52564 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755744AbdDGPpd (ORCPT ); Fri, 7 Apr 2017 11:45:33 -0400 Date: Fri, 7 Apr 2017 17:45:20 +0200 From: Greg Kroah-Hartman To: Varsha Rao Cc: Matthew Wilcox , Arnd Bergmann , linux-kernel@vger.kernel.org Subject: Re: [PATCH 5/5] drivers: char: Replace bit operation functions with IDA allocator. Message-ID: <20170407154520.GA11575@kroah.com> References: <58e77955.09c3620a.bb125.3e20@mx.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <58e77955.09c3620a.bb125.3e20@mx.google.com> User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2193 Lines: 73 On Fri, Apr 07, 2017 at 05:04:41PM +0530, Varsha Rao wrote: > Replace bit operation functions with IDA allocator functions. As IDA > allocation is simpler. But why does this matter? > > Signed-off-by: Varsha Rao > --- > drivers/char/misc.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/char/misc.c b/drivers/char/misc.c > index c9cd1ea..5786281 100644 > --- a/drivers/char/misc.c > +++ b/drivers/char/misc.c > @@ -60,7 +60,7 @@ static DEFINE_MUTEX(misc_mtx); > * Assigned numbers, used for dynamic minors > */ > #define DYNAMIC_MINORS 64 /* like dynamic majors */ > -static DECLARE_BITMAP(misc_minors, DYNAMIC_MINORS); > +static DEFINE_IDA(misc_minors_ida); > > #ifdef CONFIG_PROC_FS > static void *misc_seq_start(struct seq_file *seq, loff_t *pos) > @@ -193,14 +193,18 @@ int misc_register(struct miscdevice *misc) > mutex_lock(&misc_mtx); > > if (is_dynamic) { > - int i = find_first_zero_bit(misc_minors, DYNAMIC_MINORS); > + int i = ida_simple_get(&misc_minors_ida, 0, > + DYNAMIC_MINORS, GFP_KERNEL); > > if (i >= DYNAMIC_MINORS) { > err = -EBUSY; > goto out; > - } > + } else if (i < 0) { > + err = i; > + goto out; > + } else { > misc->minor = DYNAMIC_MINORS - i - 1; > - set_bit(i, misc_minors); > + } Your indentation is now incorrect :( > } else { > struct miscdevice *c; > > @@ -222,7 +226,7 @@ int misc_register(struct miscdevice *misc) > int i = DYNAMIC_MINORS - misc->minor - 1; > > if (i < DYNAMIC_MINORS && i >= 0) > - clear_bit(i, misc_minors); > + ida_simple_remove(&misc_minors_ida, i); > misc->minor = MISC_DYNAMIC_MINOR; > } > err = PTR_ERR(misc->this_device); > @@ -258,7 +262,7 @@ void misc_deregister(struct miscdevice *misc) > list_del(&misc->list); > device_destroy(misc_class, MKDEV(MISC_MAJOR, misc->minor)); > if (i < DYNAMIC_MINORS && i >= 0) > - clear_bit(i, misc_minors); > + ida_simple_remove(&misc_minors_ida, i); As much as I like the ida interface, I don't see why it is required to use it here, you have not provided any justification for this patch at all :( thanks, greg k-h