Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Received-SPF: Pass (protection.outlook.com: domain of cern.ch designates
 188.184.36.50 as permitted sender) receiver=protection.outlook.com;
 client-ip=188.184.36.50; helo=cernmxgwlb4.cern.ch;
From:   Federico Vaga <federico.vaga@cern.ch>
To:     Alan Tull <atull@kernel.org>, Moritz Fischer <mdf@kernel.org>,
        <linux-fpga@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Reply-To: <federico.vaga@cern.ch>
Subject: fpga: fpga_mgr_get() buggy ?
Date:   Thu, 21 Jun 2018 15:13:41 +0200
Message-ID: <4617134.5euanDEBgJ@pcbe13614>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jun 2018 13:14:03.6448
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 10436b7a-4360-418d-c81b-08d5d778dc88
X-MS-Exchange-CrossTenant-Id: c80d3499-4a40-4a8c-986e-abce017d6b19
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=c80d3499-4a40-4a8c-986e-abce017d6b19;Ip=[188.184.36.50];Helo=[cernmxgwlb4.cern.ch]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0602MB3376
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello,

I believe that this patch

fpga: manager: change api, don't use drvdata
7085e2a94f7df5f419e3cfb2fe809ce6564e9629

is incomplete and buggy.

I completely agree that drvdata should not be used by the FPGA manager 
or any other subsystem like that.

What is buggy is the function fpga_mgr_get().
That patch has been done to allow multiple FPGA manager instances to 
be linked to the same device (PCI it says). But function 
fpga_mgr_get() will return only the first found: what about the 
others?

Then, all load kernel-doc comments says:

"This code assumes the caller got the mgr pointer from 
of_fpga_mgr_get() or fpga_mgr_get()"

but that function does not allow me to get, for instance, the second 
FPGA manager on my card.

Since, thanks to this patch I'm actually the creator of the 
fpga_manager structure,  I do not need to use fpga_mgr_get() to 
retrieve that data structure.
Despite this, I believe we still need to increment the module 
reference counter (which is done by fpga_mgr_get()).

We can fix this function by just replacing the argument from 'device' 
to 'fpga_manager' (the one returned by create() ). Alternatively, we 
can add an 'owner' field in "struct fpga_manager_ops" and 'get' it 
when we use it. Or again, just an 'owner' argument in the create() 
function. I'm proposing these alternatives because I'm not sure that 
this is correct:

	if (!try_module_get(dev->parent->driver->owner))

What if the device does not have a driver? Do we consider the 
following a valid use case?


probe(struct device *dev) {
  struct device *mydev;

  mydev->parent = dev;
  device_register(mydev);
  fpga_mrg_create(mydev, ....);
}


thanks :)