[11:55] <mchehab> hi all
[11:55] <pinchartl> hi Mauro
[11:57] <mchehab> sailus: with regards to the last PR you sent, Jenkins reported:
[11:57] <mchehab> Summary: got 17/43 patches with issues, being 0 at build time, plus one error when buinding PDF document
[11:57] <mchehab> (didn't check the actual contents)
[11:58] <mchehab> could you please double-check the warnings if they're false positives or not?
[11:58] <hverkuil> hi all
[12:00] <sailus> Heippa!
[12:00] <syoung> hi all
[12:00] <mchehab> Re: [GIT PULL v2 for 5.12] More V4L2 patches (#71294)
[12:01] <hverkuil> mchehab: is tomorrow the last day for 5.12 PRs?
[12:02] <sailus> mchehab: Most of these warnings are from lines ending with opening parenthesis.
[12:02] <sailus> The function names are long so the arguments don't really fit on the same line so well inside a loop.
[12:02] <mchehab> any reason why they end with opening parenthesis?
[12:02] <sailus> I guess you could do something about this but I doubt it would improve readability of the code, rather the opposite.
[12:02] <mchehab> btw, this seems more serious:
[12:02] <mchehab>  Macro argument '__type' may be better as '(__type)' to avoid precedence issues
[12:03] <mchehab> I found very annoying when I review something with an open parenthesis at the end
[12:04] <mchehab> it takes me more time to review when that happens ;-)
[12:04] <sailus> I don't mind that personally.
[12:04] <sailus> Would you instead prefer too long lines, or split the function into two?
[12:04] <sailus> It could also mean both.
[12:05] <mchehab> yeah, longer lines are better
[12:05] <sailus> Over 80?
[12:05] <sailus> That is against the coding style.
[12:05] <mchehab> the current limit is 100
[12:05] <sailus> It's 80, that hasn't changed.
[12:05] <sailus> I really hate long lines in code I review.
[12:05] <mchehab> checkpatch starts warning after 100
[12:05] <sailus> It's only checkpatch.pl ratning limit that has changed.
[12:05] <mchehab> nowadays
[12:06] <sailus> But checkpatch.pl is not coding style.
[12:06] <sailus> The warning on the macro argument is from a type the result is cast to.
[12:07] <mchehab> see, Kernel good practices are to use 80 columns as a limit.... but exceptions are allowed
[12:07] <sailus> Putting parentheses there doesn't really make sense.
[12:07] <sailus> Yes, that's true.
[12:07] <sailus> But here I don't see a need for an exception.
[12:07] <mchehab> sailus: as I said, I didn't check the patches... just did a very quickly view at the e-mail from Jenkins
[12:08] <mchehab> if putting parenthesis doesn't make sense, that's OK
[12:08] <sailus> Ack.
[12:08] <sailus> I guess these matters are debatable, and there isn't a single ground rule that would always yield most readable code.
[12:09] <mchehab> the high number of patches with issues on this series got my attention enough to ask you to double check :-)
[12:10] <sailus> Sounds like a reasonable thing to ask about, admittedly.
[12:10] <sailus> I do run checkpatch.pl on patches I put in a pull request nowadays.
[12:10] <hverkuil> I looked at the code and it is in a gray area. I'd probably have kept the first arg after the (, but it does become a rather long line.
[12:10] <mchehab> lines ending with open parenthesis usually means that the arguments on the next lines won't be aligned with "(",
[12:10] <mchehab> which makes harder to review the code, as one needs to stop on each of those and look more carefully
[12:11] <sailus> mchehab: Yeah, usually the following lines are aligned one tab stop right of the first line of the statement.
[12:11] <mchehab> I mean, it is easiser to review:
[12:11] <mchehab> void func(foo,
[12:11] <mchehab>               bar)
[12:11] <mchehab> than
[12:11] <sailus> And that helps quite a bit if the function you're calling has a long name.
[12:11] <mchehab> void func (
[12:11] <mchehab>   foo,bar)
[12:12] <sailus> The function names in the V4L2 async framework are long and descriptive. If we made them shorter, they'd be less descriptive.
[12:13] <sailus> There are helper functions for specific cases so drivers can avoid implementing things when there's a common pattern.
[12:13] <sailus> Such as v4l2_async_notifier_add_fwnode_remote_subdev.
[12:13] <sailus> It gets hard to keep arguments under 80 column even if the function is not called inside a loop.
[12:14] <mchehab> ok. that's a good argument to have such lines > 80 columns
[12:14] <sailus> My own preference has been to allow arguments starting on the next line in such cases.
[12:15] <sailus> Well, I don't disagree, but I would wrap after the opening parenthesis.
[12:15] <mchehab> the problem is that the arguments won't be aligned with the parenthesis. That's really the issue behind this checkpatch warning
[12:15] <sailus> And it wasn't my patch btw.
[12:16] <mchehab> I won't be rejecting this PR due to that, but, specially at the core, we should be more careful about coding styles, due to 2 reasons:
[12:16] <mchehab> - it makes the core easier to be understood;
[12:16] <mchehab> - people tend to mimic the core behavior on drivers. If the core is messy, drivers may become messier
[12:17] <sailus> I fully agree.
[12:17] <sailus> The same applies to 80 column limit. :-)
[12:17] <pinchartl> (we should start using C++, we would have shorter function names)
[12:17] <sailus> Recently I've seen a lot of patches that use longer lines than 80 for no apparent reason, too.
[12:17] <pinchartl> for the 80 columns limit, I tend to sill abide by it, and sometimes use longer lines when it actually increases readability
[12:17] <pinchartl> s/sill/still/
[12:18] <sailus> pinchartl: I agree.
[12:18] <sailus> E.g. many tables fall into that category.
[12:18] <mchehab> c++ won't solve... they're usually bigger, as they have the class prepended :-)
[12:18] <sailus> I mean, arrays.
[12:18] <pinchartl> I wrap comment blocks at 80 columns for instances, but write code with longer lines when splitting for just a few characters would be weird
[12:18] <syoung> please no C++
[12:18] <sailus> :-D
[12:19] <sailus> I don't think we could decide to use C++ for the Linux kernel here in any case. Hopefully that comforts you. :-)
[12:19] <pinchartl> mchehab: it's "v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode, sizeof(...))" vs. "notifier->add_fwnode_subdev(fwnode, sizeof(...))". the latter is definitely shorter :-)
[12:19] <pinchartl> but that's not a serious suggestion of course
[12:19] <hverkuil> mchehab: is tomorrow the last day for 5.12 PRs?
[12:21] <mchehab> pinchartl: a C++ function would likely have all class inherited, like v4l2::async::notifier::fwnode::add_subdev() :-)
[12:21] <mchehab> hverkuil: my plan is to handle the last PRs tomorrow or during the weekend
[12:22] <sailus> mchehab: All patches I had are on that big pull request. If I get more, I could still send a small pull request tomorrow.
[12:22] <hverkuil> mchehab: OK, I'll see if I can put out another fixes PR this afternoon or tomorrow morning.
[12:22] <mchehab> so, yeah, if you send a PR tomorrow, I may still be able to handle it - but that will depend on what time I'll start handling them ;-)
[12:23] <hverkuil> I would appreciate it if all of you can check this patch series (not for 5.12): https://patchwork.linuxtv.org/project/linux-media/cover/20210202144926.620104-1-hverkuil-cisco@xs4all.nl/
[12:23] <mchehab> It is more likely that I'll handle those only during the weekend - although I would prefer to handle them during the working hours
[12:24] <hverkuil> I should be able to make a PR this afternoon.
[12:24] <hverkuil> I don't think there are many 'fixes' patches pending.
[12:26] <sailus> mchehab: Sure. I don't expect anything that couldn't wait for 5.13.
[12:27] <mchehab> hverkuil: if I understood the idea behind the patch is that, from userspace, /sys/class/video4linux/v4l-subdev3/media_dev will show the major:minor, right?
[12:27] <hverkuil> yes
[12:29] <mchehab> wouldn't be better, instead, to provide some information for udev to create a soft link to /dev/media0?
[12:30] <mchehab> udev already creates something:
[12:30] <mchehab> $ ls /dev/v4l/by-path/
[12:30] <mchehab> pci-0000:00:14.0-usb-0:10.3.4:1.0-video-index0  pci-0000:00:14.0-usb-0:10.3.4:1.0-video-index1
[12:31] <mchehab> (right now, in an ugly way, as right now it uses a small V4L application that runs VIDOC_QUERYCAP
[12:33] <hverkuil> Well, udev uses the index attribute to do this. So once a media_dev attribute is created, then it can make a media softlink as well based on that. But you need to provide that attribute first.
[12:33] <mchehab> the normal udev way would be to read /sys/class/*/uevent
[12:33] <mchehab> hverkuil: yeah, the attribute should be passed to userspace somewhere
[12:34] <hverkuil> When a V4L2/DVB device node is created, then udev gets a uevent, it can read the attribute and make a softlink. Of course, that would require work in the udev userspace application.
[12:35] <hverkuil> But it seems overkill to me, only a few specialized applications would want to use this.
[12:36] <mchehab> hverkuil: I guess patch 1 may break apps
[12:37] <mchehab> initializing media controller before registering the nodes can case race conditions
[12:37] <mchehab> at least for media API v1
[12:37] <mchehab> as it doesn't support hot-pluging devices at the media bus
[12:38] <hverkuil> vim2m? Why? The media_device struct is initialized, but it is not yet registered.
[12:39] <mchehab> if a program uses something like inotify() to detect when a new media device is created, it can use that information to parse the media devices and then try to access each one
[12:39] <hverkuil> Since video_register_device can add entities, links, etc. you really need a properly initialized (empty) media_device.
[12:39] <pinchartl> hverkuil: I'll review your latest series, and reply to your reply in the previous version
[12:39] <mchehab> but if those weren't created yet, you'll have a race condition
[12:40] <hverkuil> No, the media device is created in media_device_register() at the end of the probe().
[12:40] <hverkuil> media_device_init just initializes the media_device struct.
[12:40] <mchehab> ah, OK
[12:41] <mchehab> you're splitting the MC code on two parts...
[12:41] <mchehab> the first one initializing the structs, and the last one actually registering
[12:41] <mchehab> that has a race condition too...
[12:42] <mchehab> udev will call the V4L helper app before the media controller is created
[12:42] <mchehab> so, it can't create any link to the media controller
[12:42] <mchehab> because /dev/media? won't exist
[12:42] <hverkuil> Correct. That's why creating a link is a bad idea and not necessary anyway :-)
[12:43] <hverkuil> There is a short window where media_dev will return nothing.
[12:43] <hverkuil> (i.e. "")
[12:45] <mchehab> IMO, the right way to fix this is to patch /usr/lib/udev/v4l_id
[12:45] <mchehab> making it to parse the media controller when it is created
[12:45] <mchehab> and letting it create the links for the V4L2 subdevs
[12:46] <hverkuil> Hmm.
[12:47] <hverkuil> Interesting suggestion. Let me take a look at v4l_id.
[12:47] <mchehab> ok
[12:48] <hverkuil> Urg, v4l_id is *very* out of date.
[12:48] <mchehab> btw, that's the one responsible to create /dev/v4l/* stuff
[12:49] <mchehab> I guess it was written once by udev maintainer and nobody ever touched it afterwards
[12:50] <hverkuil> OK, I'll look some more into this.
[12:50] <hverkuil> Since DVB also supports the media controller, this needs to do the same thing.
[12:51] <hverkuil> Perhaps an mc_id app is needed for that.
[12:51] <mchehab> $  /usr/lib/udev/v4l_id /dev/video1
[12:51] <mchehab> ID_V4L_VERSION=2
[12:51] <mchehab> ID_V4L_PRODUCT=USB Video: USB Video
[12:51] <mchehab> ID_V4L_CAPABILITIES=:
[12:51] <mchehab> funny... it doesn't detect it as metadata :-)
[12:52] <mchehab> as you said... too old
[12:54] <mchehab> anything else for today?
[12:56] <hverkuil> not from me
[13:57] *** ChanServ sets mode: +v mchehab
[15:03] <hverkuil> pinchartl: can I pick up this uvc patch for a 'fixes' PR? https://patchwork.linuxtv.org/project/linux-media/patch/4ecb8867-6678-aa1a-3d86-65f815d34f5b@xs4all.nl/
[15:27] <pinchartl> hverkuil: please do. I've just reviewed the patch
[15:28] <hverkuil> thanks
[16:09] <hverkuil> mchehab: posted one more fixes PR.
[16:11] <hverkuil> There might be another PR tomorrow for a pwc patch: this commit 69c9e825e812 ("media: pwc: Use correct device for DMA") and this patch https://patchwork.linuxtv.org/project/linux-media/patch/20210121202855.17400-1-tiwai@suse.de/ conflict and it looks like they attempt to solve the same problem.
[16:12] <hverkuil> I think the patch from Takashi should probably replace the existing patch, but I'll have to test that tomorrow.
[18:21] <mchehab> ok