On Thu, Mar 06, 2025 at 12:21:22AM +0100, Uwe Kleine-König wrote:
> Hello David,
>
> On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote:
> > On Fri, 28 Feb 2025 17:44:27 +0100
> > Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx> wrote:
> > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote:
> > > [...]
> > > > +static int motion_open(struct inode *inode, struct file *file)
> > > > +{
> > > > + int minor = iminor(inode);
> > > > + struct motion_device *mdev = NULL, *iter;
> > > > + int err;
> > > > +
> > > > + mutex_lock(&motion_mtx);
> > >
> > > If you use guard(), error handling gets a bit easier.
> >
> > This looks interesting. I didn't know about guard(). Thanks. I see the
> > benefits, but in some cases it also makes the locked region less clearly
> > visible. While I agree that guard() in this particular place is nice,
> > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard().
> > Let me know if my assessment of the intended use of guard() is incorrect.
>
> I agree that guard() makes it harder for non-trivial functions to spot
> the critical section. In my eyes this is outweight by not having to
> unlock in all exit paths, but that might be subjective. Annother
> downside of guard is that sparse doesn't understand it and reports
> unbalanced locking.
>
> > > > + list_for_each_entry(iter, &motion_list, list) {
> > > > + if (iter->minor != minor)
> > > > + continue;
> > > > + mdev = iter;
> > > > + break;
> > > > + }
> > >
> > > This should be easier. If you use a cdev you can just do
> > > container_of(inode->i_cdev, ...);
> >
> > Hmm... I don't yet really understand what you mean. I will have to study the
> > involved code a bit more.
>
> The code that I'm convinced is correct is
> https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@xxxxxxxxxxxx/
>
> This isn't in mainline because there is some feedback I still have to
> address, but I think it might serve as an example anyhow.
>
> > > > [...]
> > > > +
> > > > +static const struct class motion_class = {
> > > > + .name = "motion",
> > > > + .devnode = motion_devnode,
> > >
> > > IIRC it's recommended to not create new classes, but a bus.
> >
> > Interesting. I did some searching, and all I could find was that the chapter
> > in driver-api/driver-model about classes magically vanished between versions
> > 5.12 and 5.13. Does anyone know where I can find some information about this?
> > Sorry if I'm being blind...
>
> Half knowledge on my end at best. I would hope that Greg knows some
> details (which might even be "no, classes are fine"). I added him to Cc:
A class is there for when you have a common api that devices of
different types can talk to userspace (i.e. the UAPI is common, not the
hardware type). Things like input devices, tty, disks, etc. A bus is
there to be able to write different drivers to bind to for that hardware
bus type (pci, usb, i2c, platform, etc.)
So you need both, a bus to talk to the hardware, and a class to talk to
userspace in a common way (ignore the fact that we can also talk to
hardware directly from userspace like raw USB or i2c or PCI config
space, that's all bus-specific stuff).
Did that help?
thanks,
greg k-h
