RE: [PATCH v10 03/20] dlb: add resource and device initialization

From: "Chen, Mike Ximing" <mike.ximing.chen@intel.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"Williams, Dan J" <dan.j.williams@intel.com>,
	"pierre-louis.bossart@linux.intel.com" 
	<pierre-louis.bossart@linux.intel.com>,
	Gage Eads <gage.eads@intel.com>
Subject: RE: [PATCH v10 03/20] dlb: add resource and device initialization
Date: Wed, 10 Mar 2021 01:33:24 +0000	[thread overview]
Message-ID: <BYAPR11MB3095C06792321F64E98394FCD9919@BYAPR11MB3095.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YEc+uL3SSf/T+EuG@kroah.com>

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> 
> On Wed, Feb 10, 2021 at 11:54:06AM -0600, Mike Ximing Chen wrote:
> > +
> > +#include "dlb_bitmap.h"
> > +
> > +#define BITS_SET(x, val, mask)	(x = ((x) & ~(mask))     \
> > +				 | (((val) << (mask##_LOC)) & (mask)))
> > +#define BITS_GET(x, mask)       (((x) & (mask)) >> (mask##_LOC))
> 
> Why not use the built-in kernel functions for this?  Why are you
> creating your own?
>
FIELD_GET(_mask, _val) and FIELD_PREP(_mask, _val) in include/linux/bitfield.h
are similar to our BITS_GET() and BITS_SET().  However in our case, mask##_LOC
is a known constant defined in dlb_regs.h,  so we don't need to use 
_buildin_ffs(mask) to calculate the location of mask as FIELD_GET() and FIELD_PREP()
do.  We can still use FIELD_GET and FIELD_PREP, but our macros are a little more 
efficient. Would it be OK to keep them?

> 
> 
> > -static void
> > -dlb_pf_unmap_pci_bar_space(struct dlb *dlb, struct pci_dev *pdev)
> > +static void dlb_pf_unmap_pci_bar_space(struct dlb *dlb, struct pci_dev *pdev)
> 
> Why reformat code here, and not do it right the first time around?
> 
Sorry,  this should not happen. Will fix it.

> > +/*******************************/
> > +/****** Driver management ******/
> > +/*******************************/
> > +
> > +static int dlb_pf_init_driver_state(struct dlb *dlb)
> > +{
> > +	mutex_init(&dlb->resource_mutex);
> > +
> > +	return 0;
> 
> If this can not fail, why is this not just void?

Sure, will change it void.

> 
> > diff --git a/drivers/misc/dlb/dlb_resource.h b/drivers/misc/dlb/dlb_resource.h
> > new file mode 100644
> > index 000000000000..2229813d9c45
> > --- /dev/null
> > +++ b/drivers/misc/dlb/dlb_resource.h
> 
> Why do you have lots of little .h files and not just one simple .h file
> for the driver?  That makes it much easier to maintain over time, right?
> 
I combined a couple of header files in this version.  dlb_regs.h is pretty big (3640 lines), and is
generated by SW. I will merge other .h files into one.

Thanks
Mike 