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: <20180329170356.GC8501@kroah.com>
Date:   Thu, 29 Mar 2018 19:03:56 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Moritz Fischer <mdf@...nel.org>
Cc:     atull@...nel.org, linux-kernel@...r.kernel.org,
        linux-fpga@...r.kernel.org
Subject: Re: [PATCH 2/6] fpga: manager: don't use drvdata in common fpga code

On Thu, Mar 29, 2018 at 08:36:54AM -0700, Moritz Fischer wrote:
> From: Alan Tull <atull@...nel.org>
> 
> Change fpga_mgr_register to not set or use drvdata.
> 
> Change the register/unregister function parameters to take the mgr
> struct:
> * int fpga_mgr_register(struct fpga_manager *mgr);
> * void fpga_mgr_unregister(struct fpga_manager *mgr);
> 
> Change the drivers that call fpga_mgr_register to alloc the struct
> fpga_manager (using devm_kzalloc) and partly fill it, adding name,
> ops, parent device, and priv.
> 
> The rationale is that setting drvdata is fine for DT based devices
> that will have one manager, bridge, or region per platform device.
> However PCIe based devices may have multiple FPGA mgr/bridge/regions
> under one PCIe device.  Without these changes, the PCIe solution has
> to create an extra device for each child mgr/bridge/region to hold
> drvdata.
> 
> Signed-off-by: Alan Tull <atull@...nel.org>
> Reported-by: Jiuyue Ma <majiuyue@...wei.com>
> Signed-off-by: Moritz Fischer <mdf@...nel.org>
> ---
>  Documentation/fpga/fpga-mgr.txt  | 24 +++++++++++++++++-------
>  drivers/fpga/altera-cvp.c        | 18 ++++++++++++++----
>  drivers/fpga/altera-pr-ip-core.c | 17 +++++++++++++++--
>  drivers/fpga/altera-ps-spi.c     | 18 +++++++++++++++---
>  drivers/fpga/fpga-mgr.c          | 39 ++++++++++++++-------------------------
>  drivers/fpga/ice40-spi.c         | 20 ++++++++++++++++----
>  drivers/fpga/socfpga-a10.c       | 16 +++++++++++++---
>  drivers/fpga/socfpga.c           | 18 +++++++++++++++---
>  drivers/fpga/ts73xx-fpga.c       | 18 +++++++++++++++---
>  drivers/fpga/xilinx-spi.c        | 18 +++++++++++++++---
>  drivers/fpga/zynq-fpga.c         | 16 +++++++++++++---
>  include/linux/fpga/fpga-mgr.h    |  8 ++++----
>  12 files changed, 166 insertions(+), 64 deletions(-)
> 
> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
> index cc6413ed6fc9..3cea6b57c156 100644
> --- a/Documentation/fpga/fpga-mgr.txt
> +++ b/Documentation/fpga/fpga-mgr.txt
> @@ -67,11 +67,9 @@ fpga_mgr_unlock when done programming the FPGA.
>  To register or unregister the low level FPGA-specific driver:
>  -------------------------------------------------------------
>  
> -	int fpga_mgr_register(struct device *dev, const char *name,
> -			      const struct fpga_manager_ops *mops,
> -			      void *priv);
> +	int fpga_mgr_register(struct fpga_manager *mgr);
>  
> -	void fpga_mgr_unregister(struct device *dev);
> +	void fpga_mgr_unregister(struct fpga_manager *mgr);
>  
>  Use of these two functions is described below in "How To Support a new FPGA
>  device."
> @@ -148,8 +146,13 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct socfpga_fpga_priv *priv;
> +	struct fpga_manager *mgr;
>  	int ret;
>  
> +	mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
> +	if (!mgr)
> +		return -ENOMEM;

This feels odd to have to do for every driver.  Why can't the fpga core
handle this all for you?


> +
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> @@ -157,13 +160,20 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
>  	/* ... do ioremaps, get interrupts, etc. and save
>  	   them in priv... */
>  
> -	return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> -				 &socfpga_fpga_ops, priv);

Why can't this be:
	fpga_mgr_create(...)
that returns the correct structure?  That way each driver always gets
the proper things set (you don't have to remember and audit each driver
to ensure they do it all correctly), and all is good?

That should make this a lot simpler to use, and change.

thanks,

greg k-h

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ