Re: [PATCH v3 02/10] fwctl: Basic ioctl dispatch for the character device

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Aug 23, 2024 at 03:02:07PM +0100, Jonathan Cameron wrote:

> > @@ -71,6 +183,9 @@ _alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size)
> >  
> >  	fwctl->dev.class = &fwctl_class;
> >  	fwctl->dev.parent = parent;
> > +	init_rwsem(&fwctl->registration_lock);
> > +	mutex_init(&fwctl->uctx_list_lock);
> 
> If the ida_alloc_max() fails,I don't think you destroy the mutex as the
> device isn't yet initialized / put in the error path.

Right
 
> Whilst i find it hard to care, it's nice to always destroy mutex, or not do it at all.
> Feels odd to only do it if things go well.

Indeed, mutex_destroy is just a sanity check. Still, lets just change
the order then and move the ida up.

> > @@ -26,6 +49,15 @@ struct fwctl_device {
> >  	struct device dev;
> >  	/* private: */
> >  	struct cdev cdev;
> > +
> > +	/*
> > +	 * Protect ops, held for write when ops becomes NULL during unregister,
> > +	 * held for read whenver ops is loaded or an ops function is running.
> > +	 */
> > +	struct rw_semaphore registration_lock;
> 
> Maybe move down to just above ops?

Yeah

> > +/**
> > + * DOC: General ioctl format
> > + *
> > + * The ioctl interface follows a general format to allow for extensibility. Each
> > + * ioctl is passed in a structure pointer as the argument providing the size of
> Pedantic Englishman time:
> passed a structure pointer
> 
> (otherwise I read that as passing an ioctl in a pointer which is weird).

Done

Thanks,
Jason