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
