On Thu, Mar 06, 2025 at 03:25:29PM +0100, David Jander wrote:
> On Thu, 6 Mar 2025 14:39:16 +0100
> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> > On Thu, Mar 06, 2025 at 10:34:02AM +0100, David Jander wrote:
> > > On Thu, 6 Mar 2025 10:03:26 +0100
> > > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > > On Thu, Mar 06, 2025 at 09:20:13AM +0100, David Jander wrote:
> > > > > On Thu, 6 Mar 2025 08:18:46 +0100
> > > > > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > > 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).
> > > > >
> > > > > Thanks for chiming in. Let me see if I understand this correctly: In this
> > > > > case, I have a UAPI that is common to different types of motion control
> > > > > devices. So I need a class. check.
> > > >
> > > > Correct.
> > > >
> > > > > Do I need a bus? If one can conceive other drivers or kernel parts that talk to
> > > > > motion drivers, I would need a bus. If that doesn't make sense, I don't. Right?
> > > >
> > > > Correct.
> > > >
> > > > > I actually can think of a new motion device that acts as an aggregator of
> > > > > several single-channel motion devices into a single "virtual" multi-channel
> > > > > device... so do I need also a bus? I suppose...?
> > > >
> > > > Nope, that should just be another class driver. Think about how input
> > > > does this, some input /dev/ nodes are the sum of ALL input /dev/ nodes
> > > > together, while others are just for individual input devices.
> > >
> > > Understood. Thanks!
> > >
> > > > > Then the question remains: why did the chapter about classes vanish?
> > > >
> > > > What are you specifically referring to? I don't remember deleting any
> > > > documentation, did files move around somehow and the links not get
> > > > updated?
> > >
> > > This:
> > > https://www.kernel.org/doc/html/v5.12/driver-api/driver-model/index.html
> > >
> > > vs this:
> > > https://www.kernel.org/doc/html/v5.13/driver-api/driver-model/index.html
> > >
> > > Maybe it moved somewhere else, but I can't find it... I'd have to git bisect
> > > or git blame between the two releases maybe.
> >
> > Ah, this was removed in:
> > 1364c6787525 ("docs: driver-model: Remove obsolete device class documentation")
> > as the information there was totally incorrect, since the 2.5.69 kernel
> > release. "device classes" aren't a thing, "classes" are a thing :)
>
> Aha. Thanks for pointing this out. The sheer removal of this, combined with
> other indirect indications, such as /sys/class/gpio being replaced with
> /sys/bus/gpio in the new api, Uwe's comment, etc... derailed my interpretation.
> :-)
>
> Btw, sorry to ask here and now @Greg: I didn't CC you with this whole series
> while I probably should have... now I am tempted to move V2 of this series to
> staging, due to higher chances of potentially breaking UAPI changes during
> initial development, and in order to have a more flexible discussions over the
> UAPI of LMC in general. Is that advisable or should we better make sure that
> the version to get merged upstream (I hope it eventually will be) is set in
> stone?
Just because something is in drivers/staging/ does not mean you can
break the user/kernel api, that is NOT what staging is for at all.
Take the time to get this right, there's no rush here. Make sure
userspace works well with what you have before committing to it.
If you want to cc: me on the next series so I can review the driver-core
interaction bits, I'll be glad to do so.
thanks,
greg k-h
