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

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 10 Mar 2021 01:33:24 +0000
From:   "Chen, Mike Ximing" <mike.ximing.chen@...el.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "pierre-louis.bossart@...ux.intel.com" 
        <pierre-louis.bossart@...ux.intel.com>,
        Gage Eads <gage.eads@...el.com>
Subject: RE: [PATCH v10 03/20] dlb: add resource and device initialization


> -----Original Message-----
> From: Greg KH <gregkh@...uxfoundation.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