FYI, I'll still be applying a few patches today for 4.13 (basically, some patches that I complained during review, whose new version was sent during the weekend)
so, if you have something that you consider we should place on 4.13, ping me today here
Can you pick up these trivial patches: https://patchwork.linuxtv.org/patch/42117/ https://patchwork.linuxtv.org/patch/42114/
they came in yesterday and it would be nice to have those little cleanups merged. Other than that I see nothing that should go in for 4.13.
sailus: what about this one: https://patchwork.linuxtv.org/patch/41957/
I should probably reassign this to you.
I think it would be nice to get this in. Yet another soc-camera sensor de-soc-camera'ed
sailus: I've reassigned that one to you
Acked it as well.
btw, I was tempted to pick that ov6650 patch :-)
mchehab: I don't see any harm in it, but it would be nice if sailus would ack it as well.
yes, sure
hverkuil1: neither 42117 nor 42114 apply
they were generated by krobot for stuff that we didn't merge yet
gah!
        if (rval > 0) {
                u32 array[ARRAY_SIZE(bus->lane_polarities)];
drivers/media/v4l2-core/v4l2-fwnode.c
ah, yes. Missed that.
dynamic arrays at stack suck!
It's not very clear from the kbuild mail for which git repo/branch it is.
it is even worse than that:
        rval = fwnode_property_read_u32_array(fwnode, "data-lanes", NULL, 0);
        if (rval > 0) {
                u32 array[ARRAY_SIZE(bus->data_lanes)];
we should kmalloc it or limit it to a maximum value
sailus: ^
hmm.. this is actually a fixed value. sorry for the noise
hverkuil1: I'll check that later today.
mchehab: It's not a dynamic array.
The size of the array is entirely deterministic.
I got confused with this:
drivers/media/v4l2-core/v4l2-fwnode.c:76 v4l2_fwnode_endpoint_parse_csi_bus() error: buffer overflow 'array' 5 <= u16max
fix for it is actually trivial
mchehab: Ah, yes, I didn't read your later comments before replying.
So we agree that this is a false positibe?
Positive.
sort of
Sort of?
if someone would call v4l2_fwnode_endpoint_parse_csi_bus() with, let's say, bus->num_data_lanes =  65535
it would cause a crash if rval = fwnode_property_read_u32_array(fwnode, "data-lanes", NULL, 0); returns < 0
so, either we should do:
        bus->num_data_lanes = 0;
at the beginning of the routine...
or add the code that parse lane-polarities inside the if (rval > 0)
sailus: fixup patch sent
Fixup of what?
v4l2_fwnode_endpoint_parse_csi_bus() is only called from v4l2_fwnode_endpoint_parse() that already zeroes the memory, including num_data_lanes field.
fixup for the warning
placing the logic inside the if() won't make it the data size any bigger, and will cleanup the warning
(nor the code size)
sorry, sent a wrong version... v2 sent
sailus: btw, as per your request, I delegated to you several patches sent from Intel people
related to sensors
mchehab: Thanks!
drivers/media/usb/pvrusb2/pvrusb2-encoder.c:263 pvr2_encoder_cmd() warn: continue to end of do { ... } while(0); loop
that's actually a bug at the driver's retry logic
mchehab: On Media summit this year --- is it decided yet at this point on which days will it take place?
not yet
I'm actually waiting some feedback from KS
if we'll either have it as part of KS or not
Ack.
sailus, I'm changing that fwnode patch per your request...
any reason why to accept lane-polarities > 1+data-lanes?
I mean, IMHO, the check there should be:
                        if (rval != 1 + bus->num_data_lanes /* clock + data */) {
                                pr_warn("invalid number of lane-polarities entries (need %u, got %u)\n",
                                        1 + bus->num_data_lanes, rval);
                                return -EINVAL;
                        }
(ok, the code will just ignore if someone would specify more than 1 + bus->num_data_lanes, but wouldn't be better to just print an error and bail out if OF is wrong?
another thing: why read it twice?
sailus: just sent a version 3 of the patch. please review
mchehab: I think that's the general approach --- if there's more data than needed, it is ignored. I can't claim to be an expert when it comes to such nuances though.
It'd be still better not to change the behaviour in the same patch.
the patch is changing the behavior already. So, why to split it on multiple patches?
It's two unrelated changes. Well, perhaps the likelihood that someone might be hit by that is still pretty low, thus suggesting that the need to revert it is equally unlikely.