Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sat, Aug 24, 2019 at 08:53:01PM +0800, zhangfei wrote:
> 
> 
> On 2019/8/20 下午10:33, Greg Kroah-Hartman wrote:
> > On Tue, Aug 20, 2019 at 08:36:50PM +0800, zhangfei wrote:
> > > Hi, Greg
> > > 
> > > On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
> > > > > > > +static int uacce_create_chrdev(struct uacce *uacce)
> > > > > > > +{
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> > > > > > > +	if (ret < 0)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > Shouldn't this function create the memory needed for this structure?
> > > > > > You are relying ont he caller to do it for you, why?
> > > > > I think you mean uacce structure here.
> > > > > Yes, currently we count on caller to prepare uacce structure and call
> > > > > uacce_register(uacce).
> > > > > We still think this method is simpler, prepare uacce, register uacce.
> > > > > And there are other system using the same method, like crypto
> > > > > (crypto_register_acomp), nand, etc.
> > > > crypto is not a subsystem to ever try to emulate :)
> > > > 
> > > > You are creating a structure with a lifetime that you control, don't
> > > > have someone else create your memory, that's almost never what you want
> > > > to do.  Most all driver subsystems create their own memory chunks for
> > > > what they need to do, it's a much better pattern.
> > > > 
> > > > Especially when you get into pointer lifetime issues...
> > > OK, understand now, thanks for your patience.
> > > will use this instead.
> > > struct uacce_interface {
> > >          char name[32];
> > >          unsigned int flags;
> > >          struct uacce_ops *ops;
> > > };
> > > struct uacce *uacce_register(struct device *dev, struct uacce_interface
> > > *interface);
> > What?  Why do you need a structure?  A pointer to the name and the ops
> > should be all that is needed, right?
> We are thinking transfer structure will be more flexible.
> And modify api later would be difficult, requiring many drivers modify
> together.
> Currently parameters need a flag, a pointer to the name, and ops, but in
> case more requirement from future drivers usage.
> Also refer usb_register_dev, sdhci_pltfm_init etc, and the structure para
> can be set as static.

Ok, I can live with that, but it is a bit more complex.  However, if you
are creating a new device structure, your "core" has to do it, do not
rely on the driver to do it for you as that is just lots of duplicated
logic everywhere.  Not to mention a reference counting nightmare.

> > And 'dev' here is a pointer to the parent, right?  Might want to make
> > that explicit in the name of the variable :)
> Yes, 'dev' is parent, will change to 'pdev', thanks.

Use "struct device *parent" it's much more obvious that way and does not
look like crazy hungarian notation :)

thanks,

greg k-h