[v4,01/26] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"
Commit Message
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.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
drivers/media/mc/mc-device.c | 15 +++++++--------
drivers/media/mc/mc-devnode.c | 8 +-------
include/media/media-devnode.h | 16 ++--------------
3 files changed, 10 insertions(+), 29 deletions(-)
Comments
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.
Regards,
Hans
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.
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
@@ -741,7 +741,6 @@ int __must_check __media_device_register(struct media_device *mdev,
if (ret < 0) {
/* devnode free is handled in media_devnode_*() */
mdev->devnode = NULL;
- media_devnode_unregister_prepare(devnode);
media_devnode_unregister(devnode);
return ret;
}
@@ -797,9 +796,6 @@ void media_device_unregister(struct media_device *mdev)
return;
}
- /* Clear the devnode register bit to avoid races with media dev open */
- media_devnode_unregister_prepare(mdev->devnode);
-
/* Remove all entities from the media device */
list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
__media_device_unregister_entity(entity);
@@ -824,10 +820,13 @@ void media_device_unregister(struct media_device *mdev)
dev_dbg(mdev->dev, "Media device unregistered\n");
- device_remove_file(&mdev->devnode->dev, &dev_attr_model);
- media_devnode_unregister(mdev->devnode);
- /* devnode free is handled in media_devnode_*() */
- mdev->devnode = NULL;
+ /* Check if mdev devnode was registered */
+ if (media_devnode_is_registered(mdev->devnode)) {
+ device_remove_file(&mdev->devnode->dev, &dev_attr_model);
+ media_devnode_unregister(mdev->devnode);
+ /* devnode free is handled in media_devnode_*() */
+ mdev->devnode = NULL;
+ }
}
EXPORT_SYMBOL_GPL(media_device_unregister);
@@ -265,7 +265,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
return ret;
}
-void media_devnode_unregister_prepare(struct media_devnode *devnode)
+void media_devnode_unregister(struct media_devnode *devnode)
{
/* Check if devnode was ever registered at all */
if (!media_devnode_is_registered(devnode))
@@ -273,12 +273,6 @@ void media_devnode_unregister_prepare(struct media_devnode *devnode)
mutex_lock(&media_devnode_lock);
clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
- mutex_unlock(&media_devnode_lock);
-}
-
-void media_devnode_unregister(struct media_devnode *devnode)
-{
- mutex_lock(&media_devnode_lock);
/* Delete the cdev on this minor as well */
cdev_device_del(&devnode->cdev, &devnode->dev);
devnode->media_dev = NULL;
@@ -115,19 +115,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
struct media_devnode *devnode,
struct module *owner);
-/**
- * media_devnode_unregister_prepare - clear the media device node register bit
- * @devnode: the device node to prepare for unregister
- *
- * This clears the passed device register bit. Future open calls will be met
- * with errors. Should be called before media_devnode_unregister() to avoid
- * races with unregister and device file open calls.
- *
- * This function can safely be called if the device node has never been
- * registered or has already been unregistered.
- */
-void media_devnode_unregister_prepare(struct media_devnode *devnode);
-
/**
* media_devnode_unregister - unregister a media device node
* @devnode: the device node to unregister
@@ -135,7 +122,8 @@ void media_devnode_unregister_prepare(struct media_devnode *devnode);
* This unregisters the passed device. Future open calls will be met with
* errors.
*
- * Should be called after media_devnode_unregister_prepare()
+ * This function can safely be called if the device node has never been
+ * registered or has already been unregistered.
*/
void media_devnode_unregister(struct media_devnode *devnode);