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]

Message-ID: <YEc+uL3SSf/T+EuG@kroah.com>
Date:   Tue, 9 Mar 2021 10:24:08 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Mike Ximing Chen <mike.ximing.chen@...el.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        arnd@...db.de, dan.j.williams@...el.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

On Wed, Feb 10, 2021 at 11:54:06AM -0600, Mike Ximing Chen wrote:
> --- a/drivers/misc/dlb/dlb_hw_types.h
> +++ b/drivers/misc/dlb/dlb_hw_types.h
> @@ -5,6 +5,13 @@
>  #define __DLB_HW_TYPES_H
>  
>  #include <linux/io.h>
> +#include <linux/types.h>
> +
> +#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?



> -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?

>  {
>  	pcim_iounmap(pdev, dlb->hw.csr_kva);
>  	pcim_iounmap(pdev, dlb->hw.func_kva);
>  }
>  
> -static int
> -dlb_pf_map_pci_bar_space(struct dlb *dlb, struct pci_dev *pdev)
> +static int dlb_pf_map_pci_bar_space(struct dlb *dlb, struct pci_dev *pdev)

Same here.

>  {
>  	dlb->hw.func_kva = pcim_iomap_table(pdev)[DLB_FUNC_BAR];
>  	dlb->hw.func_phys_addr = pci_resource_start(pdev, DLB_FUNC_BAR);
> @@ -40,6 +42,59 @@ dlb_pf_map_pci_bar_space(struct dlb *dlb, struct pci_dev *pdev)
>  	return 0;
>  }
>  
> +/*******************************/
> +/****** 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?

> 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?

thanks,

greg k-h