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

[zhangfei]

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.
> 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.
> > > > > > +
> > > > > > +static int uacce_dev_match(struct device *dev, void *data)
> > > > > > +{
> > > > > > +	if (dev->parent == data)
> > > > > > +		return -EBUSY;
> > > > > There should be in-kernel functions for this now, no need for you to
> > > > > roll your own.
> > > > Sorry, do not find this function.
> > > > Only find class_find_device, which still require match.
> > > It is in linux-next, look there...
> > Suppose you mean the funcs: device_match_name,
> > device_match_of_node,device_match_devt etc.
> > Here we need dev->parent, there still no such func.
> You should NEVER be matching on a parent.  If so, your use of the driver
> model is wrong :)
>
> Remind me to really review the use of the driver core code in your next
> submission of this series please, I think it needs it.

OK, thanks Greg.