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
| ||
|
Date: Tue, 31 Oct 2017 21:22:52 -0500 From: Alan Tull <atull@...nel.org> To: Moritz Fischer <mdf@...nel.org> Cc: linux-kernel <linux-kernel@...r.kernel.org>, linux-fpga@...r.kernel.org Subject: Re: [PATCH 2/3] fpga: manager: don't use drvdata in common fpga code On Tue, Oct 31, 2017 at 8:34 PM, Moritz Fischer <mdf@...nel.org> wrote: > On Tue, Oct 31, 2017 at 04:45:54PM -0500, Alan Tull wrote: >> On Tue, Oct 31, 2017 at 3:59 PM, Moritz Fischer <mdf@...nel.org> wrote: >> > On Tue, Oct 31, 2017 at 08:42:14PM +0000, Alan Tull wrote: >> >> Changes to the fpga manager code to not use drvdata in common >> >> code. >> >> >> >> 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 device *dev, >> >> 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, 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. >> > >> > This looks very common, in fact several subsystems provide this two step >> > way of registering things. >> > >> > - Allocate fpga_mgr via fpga_mgr_alloc() where you pass in the size of >> > the private data >> > - Fill in some fields >> > - fpga_mgr_register() where you pass in the fpga_mgr as suggested >> > >> > See for example the alloc_etherdev() for ethernet devices. >> > >> >> Yes, I considered it when I was writing this. I've seen it both ways. >> In the case you mention, they've got reasons they absolutely needed to >> do it that way. alloc_etherdev() calls eventually to >> alloc_netdev_mqs() which is about 100 lines of alloc'ing and >> initializing a network device struct. > > Yeah, sure. I looked around some more. Other subsystems just alloc > manually, seems fine with me. >> >> > The benefit of the API you proposed is that one could embed the fpga_mgr >> > struct inside of another struct and get to the container via >> > container_of() I guess ... >> >> Yes, let's keep it simple for now, as that gives us greater >> flexibility. We can add alloc functions when it becomes clear that it >> won't get in the way of someone's use. > > Agreed. >> >> Alan >> >> > >> >> >> >> Signed-off-by: Alan Tull <atull@...nel.org> >> >> Reported-by: Jiuyue Ma <majiuyue@...wei.com> >> >> --- >> >> Documentation/fpga/fpga-mgr.txt | 23 ++++++++++++++++------- >> >> drivers/fpga/altera-cvp.c | 17 +++++++++++++---- >> >> drivers/fpga/altera-pr-ip-core.c | 16 ++++++++++++++-- >> >> drivers/fpga/altera-ps-spi.c | 17 ++++++++++++++--- >> >> drivers/fpga/fpga-mgr.c | 28 +++++++--------------------- >> >> drivers/fpga/ice40-spi.c | 19 +++++++++++++++---- >> >> drivers/fpga/socfpga-a10.c | 15 ++++++++++++--- >> >> drivers/fpga/socfpga.c | 17 ++++++++++++++--- >> >> drivers/fpga/ts73xx-fpga.c | 17 ++++++++++++++--- >> >> drivers/fpga/xilinx-spi.c | 17 ++++++++++++++--- >> >> drivers/fpga/zynq-fpga.c | 15 ++++++++++++--- >> >> include/linux/fpga/fpga-mgr.h | 6 ++---- >> >> 12 files changed, 147 insertions(+), 60 deletions(-) >> >> >> >> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt >> >> index cc6413e..7e5e5c8 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 device *dev, struct fpga_manager *mgr); > > At that point you could also just give the struct fpga_manager a > ->parent or ->dev that you populate with &pdev->dev or &spi->dev etc instead of > making it a separate parameter, this makes an odd mix of half and half here. Yes, I'd have to call it parent as dev is taken. I also noticed that I forgot to edit the function parameter documentation in the c file. >> >> >> >> - 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; >> >> + >> >> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> >> if (!priv) >> >> return -ENOMEM; >> >> @@ -157,13 +160,19 @@ 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); >> >> + mgr->name = "Altera SOCFPGA FPGA Manager"; >> >> + mgr->mops = &socfpga_fpga_ops; >> >> + mgr->priv = priv; > Like here: mgr->dev = &pdev->dev Yes, good catch. By the way, I pushed these patches on a branch to linux-fpga (branch name is review-v4.14-rc7-non-dt-support-v5.1). Thanks for reviewing, Alan > >> >> + platform_set_drvdata(pdev, mgr); >> >> + >> >> + return fpga_mgr_register(dev, mgr); >> >> }
Powered by blists - more mailing lists