Re: [PATCH 2/3] fpga: manager: don't use drvdata in common fpga code

[Moritz Fischer]

On Mon, Nov 13, 2017 at 11:51:59AM -0600, Alan Tull wrote:
> On Tue, Oct 31, 2017 at 9:22 PM, Alan Tull <atull@xxxxxxxxxx> wrote:
> 
> Any further comments on v5?  I'm getting ready to send v6.  If I do it
> today, most of these patches will have no changes (again), the only
> changes will be in the patches that move drvdata out of the common
> code.
> 
> I've gone to a lot of trouble to break out functional patches to make
> them easy to review and to keep what I was trying to accomplish clear
> here.  I think this stuff is necessary going forward.  If this stuff
> doesn't have errors, let's move forward and make whatever changes we
> want to make on top of this.

Sounds good, haven't found anything else. Sorry for dropping the ball.
> 
> > On Tue, Oct 31, 2017 at 8:34 PM, Moritz Fischer <mdf@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>> >> Reported-by: Jiuyue Ma <majiuyue@xxxxxxxxxx>
> >>> >> ---
> >>> >>  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);
> >>> >>  }

Cheers,

Moritz

Attachment: signature.asc
Description: PGP signature