| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| *** | sailus has quit IRC (*.net *.split)
koike has quit IRC (*.net *.split) ndec has quit IRC (*.net *.split) narmstrong has quit IRC (*.net *.split) | [05:02] |
| ........................... (idle for 2h12mn) | ||
| hverkuil | neg: sailus: what is the status of the async notifier patches? Anything I need to review? | [07:14] |
| ...................................................... (idle for 4h27mn) | ||
| svarbanov | hverkuil: could you take this for 4.14
hverkuil: https://patchwork.kernel.org/patch/9909551/ | [11:41] |
| .... (idle for 15mn) | ||
| hverkuil | svarbanov: should this be backported to 4.13 as well?
(i.e. add a CC to stable) | [11:56] |
| svarbanov | hverkuil: yes, probably | [12:10] |
| ..... (idle for 20mn) | ||
| *** | benjiG has left | [12:30] |
| ................... (idle for 1h31mn) | ||
| sailus | hverkuil: Did you ask me to check Todor's patches?
I acked the set some time ago unless I'm mistaken. That is, if we're talking about the same set. :-) | [14:01] |
| .... (idle for 15mn) | ||
| hverkuil | sailus: the camss patch series.
v4\ v4 If you think there is any need for you to do more review, then let me know and I will make a pull request for it. I'm just waiting for your OK. | [14:17] |
| ..... (idle for 20mn) | ||
| sailus | hverkuil: I think it's good to go.
It's very large and I didn't thoroughly review everything. What I looked seemed pretty good though. I had a comment on stream management changes but we'll need to sort them out for the V4L2 as a whole first, i.e. need to reach a conclusion in the stream management thread. | [14:39] |
| hverkuil | svarbanov: added patch with CC for 4.13 | [14:48] |
| svarbanov | hverkuil: thanks! I wondered who should do the Cc, patch submitter or subsystem maintainer | [14:58] |
| hverkuil | Usually you tell me to do it :-). I.e.: 'this should also go to kernels x.y and up'
Ideally you check for which kernels the patch applies, although I can do it as well if needed (it was easy in this case) | [14:58] |
| svarbanov | hverkuil: hmm, for which patch you Cc-ed stable?
hverkuil: looking into the "[GIT PULL FOR v4.14] v2: More constify, some fixes" it is for "media: venus: venc: set correct resolution on compressed stream" | [15:04] |
| hverkuil | https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=for-v4.14k&id=373ad449f9d3ad8a9c701034920a43f71885c98b
Seems to be for the right patch. | [15:06] |
| svarbanov | hverkuil: Then it is a typo in pull request description | [15:07] |
| hverkuil | Oops, you're right. Replied to the pull request correcting this. | [15:09] |
| svarbanov | hverkuil: we had to talk about v4l2-compliance and testing decoders, will you have time tomorrow? | [15:12] |
| hverkuil | Unlikely. Friday or Monday are a better bet. | [15:13] |
| svarbanov | hverkuil: that's better, I need some time to look in the v4l2-compliance code, yet again :) | [15:17] |
| ........... (idle for 53mn) | ||
| *** | benjiG has left | [16:10] |
| hverkuil | sailus: so should I make a pull request for camss or not? :-) | [16:19] |
| pinchartl | mchehab: /query hverkuil
oops not like that :-) mchehab: ping mchehab: http://paste.debian.net/982735/ bug in the em28xx driver I'd send a patch if I didn't feel fixing this properly would require rewriting half of the DVB code dinner time I'll be back later | [16:24] |
| sailus | hverkuil: Please do.
pinchartl: Which IRC client do you use? :-) | [16:37] |
| hverkuil | sailus: thanks! | [16:37] |
| ............... (idle for 1h14mn) | ||
| *** | cybrNaut has quit IRC (Ping timeout: 246 seconds) | [17:51] |
| .................... (idle for 1h38mn) | ||
| mchehab | pinchartl: pong | [19:29] |
| pinchartl | mchehab: I ran into an issue with the em28xx driver
the kernel log is at http://paste.debian.net/982735/ I've investigated the probleù | [19:30] |
| mchehab | it seems that you actually had two issues there | [19:31] |
| pinchartl | s/ù/m/
one of them is that the tuner driver isn't loaded I can fix that easily the other one is an issue in the error paths | [19:31] |
| mchehab | 1) there's something wrong with the drxk - as it didn't bail out when it detected the lack of a firmware | [19:31] |
| pinchartl | yes, that too possibly
but I don't mind | [19:31] |
| mchehab | the other one is the lack of the tuner driver....
again, it shouldn't oops due to that | [19:31] |
| pinchartl | I don't want to capture TV, I just want to use the device to test object lifetime management issues | [19:32] |
| mchehab | but just to return an error like -ENODEV | [19:32] |
| pinchartl | it didn't oops
it hit a WARN_ON | [19:32] |
| mchehab | the tuner driver should be autoselected, though, if you had auto-select enabled | [19:32] |
| pinchartl | (I recommend compiling the kernel with refcount debugging enabled)
the tuner driver was not compiled, that's my fault, I don't mind, I can fix that but I don't know how to fix the error path | [19:32] |
| mchehab | ah, I see...
that should be easy to fix basically, dvb_attach increases a kref dvb_detach decreases it | [19:33] |
| pinchartl | nope, unfortunately | [19:34] |
| mchehab | in this case, dvb_attach failed, so it shouldn't be calling dvb_detach | [19:34] |
| pinchartl | dvb_attach() is a macro that wraps a symbol_get(), call of the function, and symbol_put() | [19:34] |
| mchehab | however, it is calling it:
[ 3.064252] [<ffffff800853e814>] dvb_frontend_detach+0x74/0x80 | [19:34] |
| pinchartl | no that's not why it failed
let me show you, I've analyzed the problem | [19:34] |
| mchehab | ok | [19:35] |
| pinchartl | fetching the latest linuxtv master branch to make sure we have the same line numbers | [19:35] |
| mchehab | (btw, very few tests are done with DVB compiled builtin
in practice, everybody uses it as modules) | [19:35] |
| pinchartl | so, em28xx-dvb.c line 1505
that's my device | [19:36] |
| mchehab | the rationale between symbol_get() is just to load the tuner module | [19:36] |
| pinchartl | the dvb_attach(drxk_attach, ...) calls succeeds
s/calls/call/ the dvb_attach(tda18271_attach, ...) call fails as the driver isn't available so on line 1517 we have dvb_frontend_detach(dvb->fe[0]); so far the code looks normal | [19:36] |
| mchehab | EM2884_BOARD_PCTV_510E, right? | [19:37] |
| pinchartl | or 520E, can't remember
one of the two | [19:38] |
| mchehab | ok
yes, the code looks normal | [19:39] |
| pinchartl | dvb_frontend_detach() calls a few frontend ops
and then releases the reference on the frontend frontends are refcounted, they're freed when the last reference is released that's all nice, so far it looks good but loot at the drxk_attach() function it kzalloc()s a struct drxk_state which embeds a struct dvb_frontend it initializes the ops field of the struct dvb_frontend and leaves all other fields to 0 the kref in dvb_frontend is thus uninitialized at that point the dvb_register_frontend() function is responsible for initializing all the other fields of the dvb_frontend structure dvb_register_frontend() is called at the end of em28xx_dvb_init() after the error occurs so when dvb_frontend_detach() is called, it operates on an uninitialized frontend all frontend drivers return from their attach function an uninitialized frontend, except for the ops field lovely, isn't it ? the frontend drivers should call a frontend init function but there's not such thing | [19:39] |
| mchehab | I ever hated dvb_attach() stuff
some drivers need to explicitly call symbol_put(), depending on the frontend | [19:43] |
| pinchartl | yes it's a big hack | [19:44] |
| mchehab | yes | [19:44] |
| pinchartl | the symbol lookup is a big hack
it's been there since 2016 sorry, 2006 11 years, and still not fixed :-( | [19:44] |
| mchehab | Antti started to fix it
on several drivers, it now uses a more standard I2C attach way but the code he did is ugly see for example EM28178_BOARD_PCTV_292E | [19:44] |
| pinchartl | is dvb essentially unmaintained, in the sense that there's no core developer fixing known subsystem-wide issues ? | [19:46] |
| mchehab | right now, the same code is repeated lots of time
pinchartl: the problem with DVB is that we currently lack active maintainers one reflect of that is that we don't have a sub-maintainer for DVB for a long time | [19:46] |
| pinchartl | can we just move dvb to staging ? :-) | [19:47] |
| mchehab | well, we could have moved MC to staging while the lifetime issue is not fixed :-p
the dvb_attach hack works, and so the error logic | [19:49] |
| pinchartl | I've worked with Sakari this afternoon to fix it, and dvb is in the way :-)
the error paths are wrong in the em28xx driver and I assume they're wrong in other drivers too you can't call dvb_frontend_detach() on a frontend that has been attached but not registered | [19:50] |
| mchehab | perhaps that was caused by the addition of Antti's patches with added support for the I2C probing mechanism | [19:51] |
| pinchartl | I doubt it | [19:51] |
| mchehab | I remember we had in the past bug reports similar to the one you mentioned that got fixed | [19:51] |
| pinchartl | it might have been caused by 1f862a68df2449bc7b1cf78dce616891697b4bdf | [19:52] |
| mchehab | I'll seek for some time to look into it during the next merge window | [19:52] |
| pinchartl | "dvb_frontend: move kref to struct dvb_frontend"
the real fix is to add a dvb_frontend_init() function (well, there's one already that does something different, it should be renamed) | [19:52] |
| mchehab | yeah, that patch sounds suspicious | [19:53] |
| pinchartl | and call it in the attach function of all frontend drivers
and move all initialization of the dvb_frontend() structure from dvb_register_frontend() to dvb_frontend_init() | [19:53] |
| mchehab | I didn't test it myself. I remember I asked Shuah to take a look on it | [19:53] |
| pinchartl | the patch fixed a real issue though | [19:54] |
| mchehab | yes | [19:54] |
| pinchartl | also, why the heck is dvb_frontend_ops embedded in dvb_frontend, instead of having an ops pointer ? | [19:55] |
| mchehab | bbiab | [19:57] |
| shuah | mchehab, pinchartl: The patch is in my Inbox, but I don'r recall if I reviewed it :( | [19:59] |
| pinchartl | it's not just that patch
it's the whole frontend attach infrastructure that's complete crap frontend drivers must intialize the frontend structure before returning it, it makes no sense to just set the ops | [20:00] |
| mchehab | pinchartl: sorry, I don't have time right now to look into it deeply. I'll take a look on it soon enough | [20:04] |
| pinchartl | the split between dvb_frontend and dvb_frontend_private also makes no sense to me | [20:04] |
| mchehab | that is to hide internal stuff from drivers | [20:04] |
| pinchartl | I fear what I'll find next trying to handle the lifetime management issue | [20:04] |
| shuah | pinchartl: yes the framework is a bit hard to follow. | [20:04] |
| pinchartl | just move the internal fields at the end of the structure with a comment stating they must not be touched by drivers | [20:05] |
| mchehab | the *_private contains only stuff that the DVB core should be concerned with
pinchartl: I didn't design it | [20:05] |
| pinchartl | we have fields not meant for drivers in video_device too and we have no video_device_private structure
mchehab: I know, I'm not blaming you | [20:05] |
| mchehab | it works pretty well, actually | [20:05] |
| pinchartl | "you" meant everybody and nobody :-) | [20:05] |
| mchehab | on several aspects, it works a way better than V4L
but the dvb_attach was indeed a very bad hack it doesn't even increment refcount on lsmod | [20:05] |
| pinchartl | the little I have seen is really bad. maybe I was unlucky, but I really fear what I'll discover next | [20:06] |
| mchehab | the attach way on non-legacy drivers work better
(drxk is a damn hack itself... it only works because the vendor is not producing newer firmwares anymore... on this device, the registers are not stable... newer firmwares may change the location of the device's registers) | [20:07] |
| pinchartl | that's not nice, but it's not an excuse for the bad dvb_attach architecture :-) | [20:09] |
| mchehab | on the original SDK platform, both firmware and M$ kernel code are produced at the same time
dvb_attach() works - even being a hack I'll check what's wrong on em28xx and write a fixup patch I have a few devices with DRXK here | [20:09] |
| pinchartl | if you want to fix it properly, be prepared to patch all frontend drivers :-) | [20:10] |
| mchehab | (but not today - I'll try to do it during this weekend) | [20:11] |
| pinchartl | the right fix is to split dvb_register_frontend() into init and register
and call the init function in the *_attach function of all frontend drivers not very difficult, but they must all be fixed s/fixed/patched/ | [20:11] |
| mchehab | no, the *_attach is only called on legacy frontend drivers | [20:11] |
| pinchartl | ok, not all frontends then, but all the legacy ones | [20:12] |
| mchehab | perhaps it is time to convert the legacy ones :-)
but that require some time | [20:12] |
| pinchartl | 68 of them from a quick grep for the attach function | [20:13] |
| mchehab | yes, the amount to be converted is huge
and require boards to test I don't have all legacy frontends myself | [20:14] |
| ............... (idle for 1h11mn) | ||
| pinchartl | mchehab: I've sent you a proof of concept patch
s/patch/patch series/ | [21:25] |
| .... (idle for 19mn) | ||
| *** | zaxari has quit IRC (Ping timeout: 260 seconds) | [21:44] |
| mchehab | thanks, I'll take a look when I have some time... likely at the weekend | [21:51] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |