LKML Archive on lore.kernel.org
 help / Atom feed
* fpga: fpga_mgr_free usage
@ 2018-07-11 12:38 Federico Vaga
  2018-07-11 15:59 ` Alan Tull
  0 siblings, 1 reply; 3+ messages in thread
From: Federico Vaga @ 2018-07-11 12:38 UTC (permalink / raw)
  To: Alan Tull; +Cc: linux-fpga, linux-kernel

Hi Alan,

I have another point that I would like to discuss. It is about the 
usage of 'fpga_mgr_free()' which does not look like consistent.

This function, according to the current implementation, can be used by 
an FPGA manager user and it is used by the FPGA manager itself on 
device release. This means that the user can only use this function if 
fpga_mgr_register() fails (to clean up), otherwise the user must NOT 
use this function, otherwise we most likely get an oops (NULL or 
invalid pointer). The example here is correct, this is what we should 
do:

https://www.kernel.org/doc/html/latest/driver-api/fpga/fpga-mgr.html

But I suggest to document it better or prevent this type of mistakes 
from happening. Following a couple of proposals

------
1.
Document it better. This is easy, in the fpga_mgr_free() kernel-doc 
comment we explain that the use of this function must be limited to 
clean up the memory on a registration failure. If an FPGA manager has 
been successfully registered then it will be freed by the framework 
itself.

But still, this does not prevent an oops from happening.
------
2.
Remove the fpga_mgr_free() from fpga_mgr_dev_release() and ask the 
user to free the manager when necessary.

This makes the usage consistent: the user creates and destroy its own 
objects. This is also consistent with our other discussion where we 
said, among the other things, that the module that uses the FPGA 
manager can the owner of the fpga_manager data structure.
------
3.
Not sure how, but perhaps we can be able to understand if we can 
safely continue to run fpga_mgr_free() or we should stop.
------



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: fpga: fpga_mgr_free usage
  2018-07-11 12:38 fpga: fpga_mgr_free usage Federico Vaga
@ 2018-07-11 15:59 ` Alan Tull
  2018-07-11 18:25   ` Moritz Fischer
  0 siblings, 1 reply; 3+ messages in thread
From: Alan Tull @ 2018-07-11 15:59 UTC (permalink / raw)
  To: federico.vaga; +Cc: linux-fpga, linux-kernel

On Wed, Jul 11, 2018 at 7:38 AM, Federico Vaga <federico.vaga@cern.ch> wrote:

Hi Federico,

> Hi Alan,
>
> I have another point that I would like to discuss. It is about the
> usage of 'fpga_mgr_free()' which does not look like consistent.
>
> This function, according to the current implementation, can be used by
> an FPGA manager user and it is used by the FPGA manager itself on
> device release. This means that the user can only use this function if
> fpga_mgr_register() fails (to clean up), otherwise the user must NOT
> use this function, otherwise we most likely get an oops (NULL or
> invalid pointer). The example here is correct, this is what we should
> do:
>
> https://www.kernel.org/doc/html/latest/driver-api/fpga/fpga-mgr.html
>
> But I suggest to document it better or prevent this type of mistakes
> from happening. Following a couple of proposals
>
> ------
> 1.
> Document it better. This is easy, in the fpga_mgr_free() kernel-doc
> comment we explain that the use of this function must be limited to
> clean up the memory on a registration failure. If an FPGA manager has
> been successfully registered then it will be freed by the framework
> itself.
>
> But still, this does not prevent an oops from happening.
> ------
> 2.
> Remove the fpga_mgr_free() from fpga_mgr_dev_release() and ask the
> user to free the manager when necessary.
>
> This makes the usage consistent: the user creates and destroy its own
> objects. This is also consistent with our other discussion where we
> said, among the other things, that the module that uses the FPGA
> manager can the owner of the fpga_manager data structure.

You're not the first to complain about this.  I think I'll err on the
side of consistency and implement your option 2 here.

Alan

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: fpga: fpga_mgr_free usage
  2018-07-11 15:59 ` Alan Tull
@ 2018-07-11 18:25   ` Moritz Fischer
  0 siblings, 0 replies; 3+ messages in thread
From: Moritz Fischer @ 2018-07-11 18:25 UTC (permalink / raw)
  To: Alan Tull; +Cc: federico.vaga, linux-fpga, linux-kernel

Hi Alan,

On Wed, Jul 11, 2018 at 10:59:01AM -0500, Alan Tull wrote:
> On Wed, Jul 11, 2018 at 7:38 AM, Federico Vaga <federico.vaga@cern.ch> wrote:

[..]
> > This makes the usage consistent: the user creates and destroy its own
> > objects. This is also consistent with our other discussion where we
> > said, among the other things, that the module that uses the FPGA
> > manager can the owner of the fpga_manager data structure.
> 
> You're not the first to complain about this.  I think I'll err on the
> side of consistency and implement your option 2 here.

I agree, that seems like a good approach.

Cheers,

Moritz

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-11 12:38 fpga: fpga_mgr_free usage Federico Vaga
2018-07-11 15:59 ` Alan Tull
2018-07-11 18:25   ` Moritz Fischer

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox