On 27/06/2024 09:04, Sakari Ailus wrote:
> Hi Hans,
>
> On Thu, Jun 27, 2024 at 08:53:22AM +0200, Hans Verkuil wrote:
>> On 10/06/2024 12:05, Sakari Ailus wrote:
>>> This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
>>> ioctl/syscall and unregister race"). The commit was part of an original
>>> patchset to avoid crashes when an unregistering device is in use.
>>
>> Reverting all these old commits and then adding back some of the code that
>> was removed is IMHO a bad idea.
>>
>> I took patches 1-8 and just folded them all together, and the end result
>> was *much* easier to review. And the resulting patch can be cleaned up a
>> bit as there are some unnecessary changes included (e.g. a cec change).
>>
>> With all the reverts and then reinstating code I also have little confidence
>> in the quality of the code if you have to do a git bisect later and you
>> end up in the middle of patches 1-8: it is far better to just do a single
>> patch. Effectively it is embedding devnode in media_device, so just do
>> that.
>
> The reverts shouldn't really need a review as we're just reverting to an
> earlier state of affairs in MC codebase. As you probably noticed, over the
> first 8 patches there is little more than reverts while the rest are fixes
> (as well as some preparation for what follows). So from review point of
> view I do prefer the current state of the first 8 patches.
>
> I'd like to have Laurent's opinion on this, too.
>
Just try folding patches 1-8 together. The end result is far, far easier
to understand.
In addition, those reverts also change things like cec, which I really
don't like.
And if there are fixes, should those be done first?
Basically what I want to see in this patch series is:
- first fix any existing bugs in the existing code: those can
also go in right now, no need to wait for the whole series to
be approved. E.g. patch 10/26 is a good example of that.
- then (TBD) have a single patch that embeds the devnode,
- then add the new lifetime features.
Right now it is an unholy mix of bug fixes, reverts, reinstatements,
and new features, and it is hard to see what is what.
Regards,
Hans
