| Message ID | 333b63fa1857f6819ce64666beba969c22e2f468.1513625884.git.mchehab@s-opensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mauro, On Mon, Dec 18, 2017 at 05:53:56PM -0200, Mauro Carvalho Chehab wrote: > The V4L2_DEV_DEBUG_IOCTL macros actually define a bitmask, > but without using Kernel's modern standards. Also, > documentation looks akward. > > So, convert them into an enum with valid bits, adding > the correspoinding kernel-doc documentation for it. The pattern of using bits for flags is a well established one and I wouldn't deviate from that by requiring the use of the BIT() macro. There are no benefits that I can see from here but the approach brings additional risks: misuse of the flags and mimicing the same risky pattern. I'd also like to echo Laurent's concern that code is being changed in odd ways and not for itself, but due to deficiencies in documentation tools. I believe the tooling has to be improved to address this properly. That only needs to done once, compared to changing all flag definitions to enums. Another point I want to make is that the uAPI definitions cannot be changed: enums are thus an option in kAPI only. Improved KernelDoc tools would thus also allow improving uAPI macro documentation --- which is more important anyway.
On Tuesday, 19 December 2017 13:39:27 EET Sakari Ailus wrote: > Hi Mauro, > > On Mon, Dec 18, 2017 at 05:53:56PM -0200, Mauro Carvalho Chehab wrote: > > The V4L2_DEV_DEBUG_IOCTL macros actually define a bitmask, > > but without using Kernel's modern standards. Also, > > documentation looks akward. > > > > So, convert them into an enum with valid bits, adding > > the correspoinding kernel-doc documentation for it. > > The pattern of using bits for flags is a well established one and I > wouldn't deviate from that by requiring the use of the BIT() macro. There > are no benefits that I can see from here but the approach brings additional > risks: misuse of the flags and mimicing the same risky pattern. > > I'd also like to echo Laurent's concern that code is being changed in odd > ways and not for itself, but due to deficiencies in documentation tools. > > I believe the tooling has to be improved to address this properly. That > only needs to done once, compared to changing all flag definitions to > enums. That's my main concern too. We really must not sacrifice code readability or writing ease in order to work around limitations of the documentation system. For this reason I'm strongly opposed to patches 2 and 5 in this series. > Another point I want to make is that the uAPI definitions cannot be > changed: enums are thus an option in kAPI only. And furthermore using enum types in the uAPI is a bad idea as the enum size is architecture-dependent. That's why we use integer types in structures used as ioctl arguments. > Improved KernelDoc tools would thus also allow improving uAPI macro > documentation --- which is more important anyway.
On Tuesday, 19 December 2017 16:02:02 EET Laurent Pinchart wrote: > On Tuesday, 19 December 2017 13:39:27 EET Sakari Ailus wrote: > > Hi Mauro, > > > > On Mon, Dec 18, 2017 at 05:53:56PM -0200, Mauro Carvalho Chehab wrote: > > > The V4L2_DEV_DEBUG_IOCTL macros actually define a bitmask, > > > but without using Kernel's modern standards. Also, > > > documentation looks akward. > > > > > > So, convert them into an enum with valid bits, adding > > > the correspoinding kernel-doc documentation for it. > > > > The pattern of using bits for flags is a well established one and I > > wouldn't deviate from that by requiring the use of the BIT() macro. There > > are no benefits that I can see from here but the approach brings > > additional > > risks: misuse of the flags and mimicing the same risky pattern. > > > > I'd also like to echo Laurent's concern that code is being changed in odd > > ways and not for itself, but due to deficiencies in documentation tools. > > > > I believe the tooling has to be improved to address this properly. That > > only needs to done once, compared to changing all flag definitions to > > enums. > > That's my main concern too. We really must not sacrifice code readability or > writing ease in order to work around limitations of the documentation > system. For this reason I'm strongly opposed to patches 2 and 5 in this > series. And I forgot to mention patch 8/8. Let's drop those three and improve the documentation system instead. > > Another point I want to make is that the uAPI definitions cannot be > > changed: enums are thus an option in kAPI only. > > And furthermore using enum types in the uAPI is a bad idea as the enum size > is architecture-dependent. That's why we use integer types in structures > used as ioctl arguments. > > > Improved KernelDoc tools would thus also allow improving uAPI macro > > documentation --- which is more important anyway.
Hi Laurent, On Tue, Dec 19, 2017 at 04:02:02PM +0200, Laurent Pinchart wrote: > And furthermore using enum types in the uAPI is a bad idea as the enum size is > architecture-dependent. That's why we use integer types in structures used as > ioctl arguments. I guess we have an argeement on that, enums are a no-go for uAPI, for reasons not related to the topic at hand.
Em Tue, 19 Dec 2017 16:05:46 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > On Tuesday, 19 December 2017 16:02:02 EET Laurent Pinchart wrote: > > On Tuesday, 19 December 2017 13:39:27 EET Sakari Ailus wrote: > > > Hi Mauro, > > > > > > On Mon, Dec 18, 2017 at 05:53:56PM -0200, Mauro Carvalho Chehab wrote: > > > > The V4L2_DEV_DEBUG_IOCTL macros actually define a bitmask, > > > > but without using Kernel's modern standards. Also, > > > > documentation looks akward. > > > > > > > > So, convert them into an enum with valid bits, adding > > > > the correspoinding kernel-doc documentation for it. > > > > > > The pattern of using bits for flags is a well established one and I > > > wouldn't deviate from that by requiring the use of the BIT() macro. There > > > are no benefits that I can see from here but the approach brings > > > additional > > > risks: misuse of the flags and mimicing the same risky pattern. > > > > > > I'd also like to echo Laurent's concern that code is being changed in odd > > > ways and not for itself, but due to deficiencies in documentation tools. > > > > > > I believe the tooling has to be improved to address this properly. That > > > only needs to done once, compared to changing all flag definitions to > > > enums. > > > > That's my main concern too. We really must not sacrifice code readability or > > writing ease in order to work around limitations of the documentation > > system. For this reason I'm strongly opposed to patches 2 and 5 in this > > series. > > And I forgot to mention patch 8/8. Let's drop those three and improve the > documentation system instead. Are you volunteering yourself to write the kernel-doc patches? :-) Thanks, Mauro
Em Tue, 19 Dec 2017 16:12:35 +0200 Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > Hi Laurent, > > On Tue, Dec 19, 2017 at 04:02:02PM +0200, Laurent Pinchart wrote: > > And furthermore using enum types in the uAPI is a bad idea as the enum size is > > architecture-dependent. That's why we use integer types in structures used as > > ioctl arguments. > > I guess we have an argeement on that, enums are a no-go for uAPI, for > reasons not related to the topic at hand. Huh? We're not talking about uAPI. This is kAPI. Using enums there is OK. Thanks, Mauro
Hi Mauro, On Tuesday, 19 December 2017 17:37:58 EET Mauro Carvalho Chehab wrote: > Em Tue, 19 Dec 2017 16:12:35 +0200 Sakari Ailus escreveu: > > On Tue, Dec 19, 2017 at 04:02:02PM +0200, Laurent Pinchart wrote: > >> And furthermore using enum types in the uAPI is a bad idea as the enum > >> size is architecture-dependent. That's why we use integer types in > >> structures used as ioctl arguments. > > > > I guess we have an argeement on that, enums are a no-go for uAPI, for > > reasons not related to the topic at hand. > > Huh? We're not talking about uAPI. This is kAPI. Using enums there is OK. Sure, there's no disagreement about that. The point was that, as both uAPI and kAPI should be documented, and we can't use enums for uAPI, we need a way to document non-enum types, which we could then use to document the kAPI the same way.
Em Tue, 19 Dec 2017 19:17:12 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro, > > On Tuesday, 19 December 2017 17:37:58 EET Mauro Carvalho Chehab wrote: > > Em Tue, 19 Dec 2017 16:12:35 +0200 Sakari Ailus escreveu: > > > On Tue, Dec 19, 2017 at 04:02:02PM +0200, Laurent Pinchart wrote: > > >> And furthermore using enum types in the uAPI is a bad idea as the enum > > >> size is architecture-dependent. That's why we use integer types in > > >> structures used as ioctl arguments. > > > > > > I guess we have an argeement on that, enums are a no-go for uAPI, for > > > reasons not related to the topic at hand. > > > > Huh? We're not talking about uAPI. This is kAPI. Using enums there is OK. > > Sure, there's no disagreement about that. > The point was that, as both uAPI and kAPI should be documented, No disagreement here... > and we can't use enums for uAPI, Err... we actually do use enums on uAPI... videodev2.h is full of it. What we can't do is to use enums on ioctls. So, all such enums are replaced by u32 at the ioctl calls. Ok, this is not elegant (and it happened for historic reasons - we're now avoiding it at all costs), but that's the way it is. The fact is - for uAPI - we have enums and defines, and both are documented. > we need a way to document non-enum types, We have already a way to document all uAPI data structures, including enums and defines. > which we could then use to document the kAPI the same way. Let's not mix subjects. This patch series is all about kAPI. Specifically, we're talking about this change: -/* Just log the ioctl name + error code */ -#define V4L2_DEV_DEBUG_IOCTL 0x01 -/* Log the ioctl name arguments + error code */ -#define V4L2_DEV_DEBUG_IOCTL_ARG 0x02 -/* Log the file operations open, release, mmap and get_unmapped_area */ -#define V4L2_DEV_DEBUG_FOP 0x04 -/* Log the read and write file operations and the VIDIOC_(D)QBUF ioctls */ -#define V4L2_DEV_DEBUG_STREAMING 0x08 +/** + * enum v4l2_debug_bits - Device debug bits to be used with the video + * device debug attribute + * + * @V4L2_DEBUG_IOCTL: Just log the ioctl name + error code. + * @V4L2_DEBUG_IOCTL_ARG: Log the ioctl name arguments + error code. + * @V4L2_DEBUG_FOP: Log the file operations and open, release, + * mmap and get_unmapped_area syscalls. + * @V4L2_DEBUG_STREAMING: Log the read and write syscalls and + * :c:ref:`VIDIOC_[Q|DQ]BUF <VIDIOC_QBUF>` ioctls. + * @V4L2_DEBUG_POLL: Log poll syscalls. + */ +enum v4l2_debug_bits { + V4L2_DEBUG_IOCTL = 0, + V4L2_DEBUG_IOCTL_ARG = 1, + V4L2_DEBUG_FOP = 2, + V4L2_DEBUG_STREAMING = 3, + V4L2_DEBUG_POLL = 4, +}; I agree with the principle of having a way to document #defines and static data that belongs to kAPI and drivers. So, from my side, if someone pops up and propose a way to document #defines to linux-doc, manages to get such proposal accepted and submit patches implementing it, I'm fine. There are things like: include/media/cec.h:#define CEC_NUM_CORE_EVENTS 2 include/media/cec.h:#define CEC_MAX_MSG_RX_QUEUE_SZ (18 * 3) include/media/cec.h:#define CEC_MAX_MSG_TX_QUEUE_SZ (18 * 1) include/media/rc-core.h:#define IR_DEFAULT_TIMEOUT MS_TO_NS(125) include/media/rc-core.h:#define IR_MAX_DURATION 500000000 /* 500 ms */ include/media/v4l2-clk.h:#define V4L2_CLK_NAME_SIZE 64 ... that currently can't be documented, and do belong to "#define" namespace, as those are true macros that create an alias for a magic number. Those should *never* be converted to enums. The fact that they can't right now be documented shouldn't be used as an excuse to conver to enums: they're just magic numbers that can be used on a countless number of random places that may or may not be related. However, I do believe that, grouping namespace for values with the same meaning do belong to enums. After all, those values are bound together for life, as they're meant to be used at the same places. A define is a very poor and lazy way to define, and sometimes induce to mistakes, as, from time to time, I see values on defines that belongs to an specific field being used on some other one. From documentation PoV, they also play nicer when grouped together, as a common comment that applies to all such values are grouped together. In the specific case of this patch, all those V4L2_DEV_DEBUG_* bits (or V4L2_DEV_DEBUG_* values - before this patchset) are meant to be used only when enabling or disabling V4L2 core debug messages. Grouping them into the same "enum" namespace makes all sense. They should have grouped together since the beginning. That's all my fault, as, when I added this logic[1], I just took the lazy way. [1] Back on 2006, where there were just 2 debug values commit 401998fa96fe18b057af3f906527196522dd2d9d Author: Mauro Carvalho Chehab <mchehab@infradead.org> Date: Sun Jun 4 10:06:18 2006 -0300 V4L/DVB (4065): Several improvements at videodev.c Videodev now is capable of better handling V4L2 api, by processing V4L2 ioctls and using callbacks to the driver. The drivers should be migrated to the newer way and the older one will be obsoleted soon. Thanks, Mauro
Hi Mauro, On Tuesday, 19 December 2017 17:34:46 EET Mauro Carvalho Chehab wrote: > Em Tue, 19 Dec 2017 16:05:46 +0200 Laurent Pinchart escreveu: > > On Tuesday, 19 December 2017 16:02:02 EET Laurent Pinchart wrote: > >> On Tuesday, 19 December 2017 13:39:27 EET Sakari Ailus wrote: > >>> On Mon, Dec 18, 2017 at 05:53:56PM -0200, Mauro Carvalho Chehab wrote: > >>>> The V4L2_DEV_DEBUG_IOCTL macros actually define a bitmask, > >>>> but without using Kernel's modern standards. Also, > >>>> documentation looks akward. > >>>> > >>>> So, convert them into an enum with valid bits, adding > >>>> the correspoinding kernel-doc documentation for it. > >>> > >>> The pattern of using bits for flags is a well established one and I > >>> wouldn't deviate from that by requiring the use of the BIT() macro. > >>> There are no benefits that I can see from here but the approach brings > >>> additional risks: misuse of the flags and mimicing the same risky > >>> pattern. > >>> > >>> I'd also like to echo Laurent's concern that code is being changed in > >>> odd ways and not for itself, but due to deficiencies in documentation > >>> tools. > >>> > >>> I believe the tooling has to be improved to address this properly. > >>> That only needs to done once, compared to changing all flag > >>> definitions to enums. > >> > >> That's my main concern too. We really must not sacrifice code > >> readability or writing ease in order to work around limitations of the > >> documentation system. For this reason I'm strongly opposed to patches 2 > >> and 5 in this series. > > > > And I forgot to mention patch 8/8. Let's drop those three and improve the > > documentation system instead. > > Are you volunteering yourself to write the kernel-doc patches? :-) I thought you were the expert in this field, given the number of documentation patches that you have merged in the kernel ? :-)
Em Wed, 20 Dec 2017 12:47:23 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro, > > On Tuesday, 19 December 2017 17:34:46 EET Mauro Carvalho Chehab wrote: > > Em Tue, 19 Dec 2017 16:05:46 +0200 Laurent Pinchart escreveu: > > > On Tuesday, 19 December 2017 16:02:02 EET Laurent Pinchart wrote: > > >> On Tuesday, 19 December 2017 13:39:27 EET Sakari Ailus wrote: > > >>> On Mon, Dec 18, 2017 at 05:53:56PM -0200, Mauro Carvalho Chehab wrote: > > >>>> The V4L2_DEV_DEBUG_IOCTL macros actually define a bitmask, > > >>>> but without using Kernel's modern standards. Also, > > >>>> documentation looks akward. > > >>>> > > >>>> So, convert them into an enum with valid bits, adding > > >>>> the correspoinding kernel-doc documentation for it. > > >>> > > >>> The pattern of using bits for flags is a well established one and I > > >>> wouldn't deviate from that by requiring the use of the BIT() macro. > > >>> There are no benefits that I can see from here but the approach brings > > >>> additional risks: misuse of the flags and mimicing the same risky > > >>> pattern. > > >>> > > >>> I'd also like to echo Laurent's concern that code is being changed in > > >>> odd ways and not for itself, but due to deficiencies in documentation > > >>> tools. > > >>> > > >>> I believe the tooling has to be improved to address this properly. > > >>> That only needs to done once, compared to changing all flag > > >>> definitions to enums. > > >> > > >> That's my main concern too. We really must not sacrifice code > > >> readability or writing ease in order to work around limitations of the > > >> documentation system. For this reason I'm strongly opposed to patches 2 > > >> and 5 in this series. > > > > > > And I forgot to mention patch 8/8. Let's drop those three and improve the > > > documentation system instead. > > > > Are you volunteering yourself to write the kernel-doc patches? :-) > > I thought you were the expert in this field, given the number of documentation > patches that you have merged in the kernel ? :-) c/c linux-doc, as this kind of discussion should be happening there. Let me recap your proposal here, for the others from linux-doc to understand this reply: Em Mon, 18 Dec 2017 17:32:11 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro, > > On Monday, 18 December 2017 17:13:26 EET Mauro Carvalho Chehab wrote: > > Em Fri, 13 Oct 2017 15:38:11 +0300 Laurent Pinchart escreveu: > > > On Thursday, 28 September 2017 00:46:51 EEST Mauro Carvalho Chehab wrote: > > >> Currently, there's no way to document #define foo <value> > > >> with kernel-doc. So, convert it to an enum, and document. > > > > > > The documentation seems fine to me (except for one comment below). > > > However, converting macros to an enum just to work around a defect of the > > > documentation system doesn't seem like a good idea to me. I'd rather find > > > a way to document macros. > > > > I agree that this limitation should be fixed. > > > > Yet, in this specific case where we have an "array" of defines, all > > associated to the same field (even being a bitmask), and assuming that > > we would add a way for kernel-doc to parse this kind of defines > > (not sure how easy/doable would be), then, in order to respect the > > way kernel-doc markup is, the documentation for those macros would be: > > > > > > /** > > * define: Just log the ioctl name + error code > > */ > > #define V4L2_DEV_DEBUG_IOCTL 0x01 > > /** > > * define: Log the ioctl name arguments + error code > > */ > > #define V4L2_DEV_DEBUG_IOCTL_ARG 0x02 > > /** > > * define: Log the file operations open, release, mmap and get_unmapped_area > > */ > > #define V4L2_DEV_DEBUG_FOP 0x04 > > /** > > * define: Log the read and write file operations and the VIDIOC_(D)QBUF > > ioctls */ > > #define V4L2_DEV_DEBUG_STREAMING 0x08 > > > > IMHO, this is a way easier to read/understand by humans, and a way more > > coincise: > > > > /** > > * enum v4l2_debug_flags - Device debug flags to be used with the video > > * device debug attribute > > * > > * @V4L2_DEV_DEBUG_IOCTL: Just log the ioctl name + error code. > > * @V4L2_DEV_DEBUG_IOCTL_ARG: Log the ioctl name arguments + error code. > > * @V4L2_DEV_DEBUG_FOP: Log the file operations and open, release, > > * mmap and get_unmapped_area syscalls. > > * @V4L2_DEV_DEBUG_STREAMING: Log the read and write syscalls and > > * :c:ref:`VIDIOC_[Q|DQ]BUFF <VIDIOC_QBUF>` ioctls. > > */ > > > > It also underlines the aspect that those names are grouped altogether. > > > > So, IMHO, the main reason to place them inside an enum and document > > as such is that it looks a way better for humans to read. > > As we're talking about extending kerneldoc to document macros, we're free to > decide on a format that would make it easy and clear. Based on your above > example, we could write it > > /** > * define v4l2_debug_flags - Device debug flags to be used with the video > * device debug attribute > * > * @V4L2_DEV_DEBUG_IOCTL: Just log the ioctl name + error code. > * @V4L2_DEV_DEBUG_IOCTL_ARG: Log the ioctl name arguments + error code. > * @V4L2_DEV_DEBUG_FOP: Log the file operations and open, release, > * mmap and get_unmapped_area syscalls. > * @V4L2_DEV_DEBUG_STREAMING: Log the read and write syscalls and > * :c:ref:`VIDIOC_[Q|DQ]BUFF <VIDIOC_QBUF>` ioctls. > */ > > That would be simple, clear, and wouldn't require a code change to workaround > a limitation of the documentation system. > I could volunteer myself to write a patch that would just parse a single define when I have some time for that, and after finishing to document other things that don't depend on it. However, I won't volunteer to add a patch that would artificially group #defines on a single kernel-doc markup, because: 1) I don't believe that this is the right solution. When several name macros should be grouped altogether, C provides an specific syntax for it: enum; 2) the patch will likely be very complex; 3) the define "grouping" logic would be based on hints. Let me explain (3) a little better. Right now, kernel-doc common logic removes any comments and blank lines when processing a single markup, like struct, enum, function, etc. It should very likely need to do the same for #defines. Using your proposed, syntax, please assume the following fictional (but based on real code) example: /** * define v4l2_debug_flags - Device debug flags to be used with the video * device debug attribute * * @V4L2_DEV_DEBUG_IOCTL: Just log the ioctl name + error code. * @V4L2_DEV_DEBUG_IOCTL_ARG: Log the ioctl name arguments + error code. * @V4L2_DEV_DEBUG_FOP: Log the file operations and open, release, * mmap and get_unmapped_area syscalls. * @V4L2_DEV_DEBUG_STREAMING: Log the read and write syscalls and * :c:ref:`VIDIOC_[Q|DQ]BUFF <VIDIOC_QBUF>` ioctls. */ /* ioctl debug macros */ #define V4L2_DEV_DEBUG_IOCTL 0x01 #define V4L2_DEV_DEBUG_IOCTL_ARG 0x02 /* streaming debug macros */ #define V4L2_DEV_DEBUG_FOP 0x04 #define V4L2_DEV_DEBUG_STREAMING 0x08 #define CEC_NUM_CORE_EVENTS 2 #define CEC_NUM_EVENTS CEC_EVENT_PIN_CEC_HIGH The V4L_DEV_DEBUG_* macros will be properly documented using your notation. However, how the kernel-doc logic will know when to stop grouping defines? It should be reminded that, if later someone adds a new V4L_DEV_DEBUG_FOO (either after V4L2_DEV_DEBUG_STREAMING or before it), we want a warning to be generated, because something that belongs to this define "group" was added without documentation. It might be parsing by name, but that would break if someone ever adds a macro like: V4L2_NODEV_DEBUG, it won't work (we do have some "namespaces" like that). It would also fail if someone adds an unrelated macro that would start with V4L2_DEV_DEBUG_. Now, assume that, sometime in the future, we decide to also document CEC_NUM_CORE_EVENTS, because it is now part of a kAPI, but we don't want to document CEC_NUM_EVENTS, with is just an ancillary macro to be used internally. The "group" define syntax won't fit. In summary, trying to artificially group #defines for documentation is a very dirty hack. We should really use enums on all kAPI cases where we have different symbols that should be grouped altogether. Regards, Mauro
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c index d5e0e536ef04..ab876ddaa707 100644 --- a/drivers/media/v4l2-core/v4l2-dev.c +++ b/drivers/media/v4l2-core/v4l2-dev.c @@ -307,8 +307,8 @@ static ssize_t v4l2_read(struct file *filp, char __user *buf, return -EINVAL; if (video_is_registered(vdev)) ret = vdev->fops->read(filp, buf, sz, off); - if ((vdev->dev_debug & V4L2_DEV_DEBUG_FOP) && - (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING)) + if ((vdev->dev_debug & BIT(V4L2_DEBUG_FOP)) && + (vdev->dev_debug & BIT(V4L2_DEBUG_STREAMING))) printk(KERN_DEBUG "%s: read: %zd (%d)\n", video_device_node_name(vdev), sz, ret); return ret; @@ -324,8 +324,8 @@ static ssize_t v4l2_write(struct file *filp, const char __user *buf, return -EINVAL; if (video_is_registered(vdev)) ret = vdev->fops->write(filp, buf, sz, off); - if ((vdev->dev_debug & V4L2_DEV_DEBUG_FOP) && - (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING)) + if ((vdev->dev_debug & BIT(V4L2_DEBUG_FOP)) && + (vdev->dev_debug & BIT(V4L2_DEBUG_STREAMING))) printk(KERN_DEBUG "%s: write: %zd (%d)\n", video_device_node_name(vdev), sz, ret); return ret; @@ -340,7 +340,7 @@ static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll) return DEFAULT_POLLMASK; if (video_is_registered(vdev)) res = vdev->fops->poll(filp, poll); - if (vdev->dev_debug & V4L2_DEV_DEBUG_POLL) + if (vdev->dev_debug & BIT(V4L2_DEBUG_POLL)) printk(KERN_DEBUG "%s: poll: %08x\n", video_device_node_name(vdev), res); return res; @@ -381,7 +381,7 @@ static unsigned long v4l2_get_unmapped_area(struct file *filp, if (!video_is_registered(vdev)) return -ENODEV; ret = vdev->fops->get_unmapped_area(filp, addr, len, pgoff, flags); - if (vdev->dev_debug & V4L2_DEV_DEBUG_FOP) + if (vdev->dev_debug & BIT(V4L2_DEBUG_FOP)) printk(KERN_DEBUG "%s: get_unmapped_area (%d)\n", video_device_node_name(vdev), ret); return ret; @@ -397,7 +397,7 @@ static int v4l2_mmap(struct file *filp, struct vm_area_struct *vm) return -ENODEV; if (video_is_registered(vdev)) ret = vdev->fops->mmap(filp, vm); - if (vdev->dev_debug & V4L2_DEV_DEBUG_FOP) + if (vdev->dev_debug & BIT(V4L2_DEBUG_FOP)) printk(KERN_DEBUG "%s: mmap (%d)\n", video_device_node_name(vdev), ret); return ret; @@ -427,7 +427,7 @@ static int v4l2_open(struct inode *inode, struct file *filp) ret = -ENODEV; } - if (vdev->dev_debug & V4L2_DEV_DEBUG_FOP) + if (vdev->dev_debug & BIT(V4L2_DEBUG_FOP)) printk(KERN_DEBUG "%s: open (%d)\n", video_device_node_name(vdev), ret); /* decrease the refcount in case of an error */ @@ -444,7 +444,7 @@ static int v4l2_release(struct inode *inode, struct file *filp) if (vdev->fops->release) ret = vdev->fops->release(filp); - if (vdev->dev_debug & V4L2_DEV_DEBUG_FOP) + if (vdev->dev_debug & BIT(V4L2_DEBUG_FOP)) printk(KERN_DEBUG "%s: release\n", video_device_node_name(vdev)); diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 79614992ee21..cdd1e9470dbe 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -2760,15 +2760,16 @@ static long __video_do_ioctl(struct file *file, } done: - if (dev_debug & (V4L2_DEV_DEBUG_IOCTL | V4L2_DEV_DEBUG_IOCTL_ARG)) { - if (!(dev_debug & V4L2_DEV_DEBUG_STREAMING) && + if (dev_debug & (BIT(V4L2_DEBUG_IOCTL) + | BIT(V4L2_DEBUG_IOCTL_ARG))) { + if (!(dev_debug & BIT(V4L2_DEBUG_STREAMING)) && (cmd == VIDIOC_QBUF || cmd == VIDIOC_DQBUF)) return ret; v4l_printk_ioctl(video_device_node_name(vfd), cmd); if (ret < 0) pr_cont(": error %ld", ret); - if (!(dev_debug & V4L2_DEV_DEBUG_IOCTL_ARG)) + if (!(dev_debug & BIT(V4L2_DEBUG_IOCTL_ARG))) pr_cont("\n"); else if (_IOC_DIR(cmd) == _IOC_NONE) info->debug(arg, write_only); diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h index a7b3f7c75d62..9f8d04ad4bdd 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -589,20 +589,25 @@ struct v4l2_ioctl_ops { }; -/* v4l debugging and diagnostics */ - -/* Device debug flags to be used with the video device debug attribute */ - -/* Just log the ioctl name + error code */ -#define V4L2_DEV_DEBUG_IOCTL 0x01 -/* Log the ioctl name arguments + error code */ -#define V4L2_DEV_DEBUG_IOCTL_ARG 0x02 -/* Log the file operations open, release, mmap and get_unmapped_area */ -#define V4L2_DEV_DEBUG_FOP 0x04 -/* Log the read and write file operations and the VIDIOC_(D)QBUF ioctls */ -#define V4L2_DEV_DEBUG_STREAMING 0x08 -/* Log poll() */ -#define V4L2_DEV_DEBUG_POLL 0x10 +/** + * enum v4l2_debug_bits - Device debug bits to be used with the video + * device debug attribute + * + * @V4L2_DEBUG_IOCTL: Just log the ioctl name + error code. + * @V4L2_DEBUG_IOCTL_ARG: Log the ioctl name arguments + error code. + * @V4L2_DEBUG_FOP: Log the file operations and open, release, + * mmap and get_unmapped_area syscalls. + * @V4L2_DEBUG_STREAMING: Log the read and write syscalls and + * :c:ref:`VIDIOC_[Q|DQ]BUF <VIDIOC_QBUF>` ioctls. + * @V4L2_DEBUG_POLL: Log poll syscalls. + */ +enum v4l2_debug_bits { + V4L2_DEBUG_IOCTL = 0, + V4L2_DEBUG_IOCTL_ARG = 1, + V4L2_DEBUG_FOP = 2, + V4L2_DEBUG_STREAMING = 3, + V4L2_DEBUG_POLL = 4, +}; /* Video standard functions */
The V4L2_DEV_DEBUG_IOCTL macros actually define a bitmask, but without using Kernel's modern standards. Also, documentation looks akward. So, convert them into an enum with valid bits, adding the correspoinding kernel-doc documentation for it. In order to avoid possible conflicts, rename them from V4L2_DEV_DEBUG_foo to V4L2_DEBUG_foo. Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> --- drivers/media/v4l2-core/v4l2-dev.c | 18 +++++++++--------- drivers/media/v4l2-core/v4l2-ioctl.c | 7 ++++--- include/media/v4l2-ioctl.h | 33 +++++++++++++++++++-------------- 3 files changed, 32 insertions(+), 26 deletions(-)