MC minor cleanups

PATCH In review Mauro Carvalho Chehab 3 participants 7 messages Sep 9, 2015 – Nov 23, 2015

Subsystems (2)

Media (V4L/DVB) Media controller framework

lore

Share & tools

lore

Mauro Carvalho Chehab

author

2015-09-09 11:32 UTC #0 lore raw

cover MC minor cleanups

This two patch series do some minor cleanups as suggested by Sakari.

Mauro Carvalho Chehab (2):
[media] media_entity: remove gfp_flags argument [media] media-device: use unsigned ints on some places

 drivers/media/dvb-core/dvbdev.c    | 3 +--
 drivers/media/media-device.c       | 7 ++++---
 drivers/media/media-entity.c       | 5 ++---
 drivers/media/v4l2-core/v4l2-dev.c | 3 +--
 include/media/media-entity.h       | 4 +---
 5 files changed, 9 insertions(+), 13 deletions(-)

Signature · 1 line

2.4.3

Mauro Carvalho Chehab

author

2015-09-09 11:32 UTC #1 lore raw

PATCH 1/2 media_entity: remove gfp_flags argument

We should not be creating device nodes at IRQ contexts. So, the only flags we'll be using will be GFP_KERNEL. Let's remove the gfp_flags, in order to make the interface simpler.

If we ever need it, it would be easy to revert those changes.

While here, remove an extra blank line.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/dvb-core/dvbdev.c +1 -2

index e9f24c1479dd..60828a9537a0 100644
@@ -379,8 +379,7 @@ static int dvb_register_media_device(struct dvb_device *dvbdev,
 
 	dvbdev->intf_devnode = media_devnode_create(dvbdev->adapter->mdev,
 						    intf_type, 0,
-						    DVB_MAJOR, minor,
-						    GFP_KERNEL);
+						    DVB_MAJOR, minor);
 
 	if (!dvbdev->intf_devnode)
 		return -ENOMEM;

drivers/media/media-entity.c +2 -3

index 4868b8269204..f28265864b76 100644
@@ -887,12 +887,11 @@ static void media_interface_init(struct media_device *mdev,
 
 struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
 						u32 type, u32 flags,
-						u32 major, u32 minor,
-						gfp_t gfp_flags)
+						u32 major, u32 minor)
 {
 	struct media_intf_devnode *devnode;
 
-	devnode = kzalloc(sizeof(*devnode), gfp_flags);
+	devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
 	if (!devnode)
 		return NULL;

drivers/media/v4l2-core/v4l2-dev.c +1 -2

index 430aa2330d07..982255d2063f 100644
@@ -777,8 +777,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
 	vdev->intf_devnode = media_devnode_create(vdev->v4l2_dev->mdev,
 						  intf_type,
 						  0, VIDEO_MAJOR,
-						  vdev->minor,
-						  GFP_KERNEL);
+						  vdev->minor);
 	if (!vdev->intf_devnode) {
 		media_device_unregister_entity(&vdev->entity);
 		return -ENOMEM;

include/media/media-entity.h +1 -3

index 9cbb10079024..44ab153c37fc 100644
@@ -71,7 +71,6 @@ struct media_gobj {
 	struct list_head	list;
 };
 
-
 struct media_pipeline {
 };

@@ -373,8 +372,7 @@ void media_entity_pipeline_stop(struct media_entity *entity);
 struct media_intf_devnode *
 __must_check media_devnode_create(struct media_device *mdev,
 				  u32 type, u32 flags,
-				  u32 major, u32 minor,
-				  gfp_t gfp_flags);
+				  u32 major, u32 minor);
 void media_devnode_remove(struct media_intf_devnode *devnode);
 struct media_link *
 __must_check media_create_intf_link(struct media_entity *entity,

Signature · 1 line

2.4.3

Hans Verkuil

Acked-by on v1 1/2

2015-09-09 11:37 UTC #2 lore raw

Re: [PATCH 1/2] [media] media_entity: remove gfp_flags argument

Quoted 10 lines · Mauro Carvalho Chehab

We should not be creating device nodes at IRQ contexts. So, the only flags we'll be using will be GFP_KERNEL. Let's remove the gfp_flags, in order to make the interface simpler.
If we ever need it, it would be easy to revert those changes.
While here, remove an extra blank line.
Suggested-by Sakari Ailus <[email protected]>
Signed-off-by Mauro Carvalho Chehab <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Quoted 71 lines

drivers/media/dvb-core/dvbdev.c +1 -2

index e9f24c1479dd..60828a9537a0 100644
@@ -379,8 +379,7 @@ static int dvb_register_media_device(struct dvb_device *dvbdev,
 
 	dvbdev->intf_devnode = media_devnode_create(dvbdev->adapter->mdev,
 						    intf_type, 0,
-						    DVB_MAJOR, minor,
-						    GFP_KERNEL);
+						    DVB_MAJOR, minor);
 
 	if (!dvbdev->intf_devnode)
 		return -ENOMEM;

drivers/media/media-entity.c +2 -3

index 4868b8269204..f28265864b76 100644
@@ -887,12 +887,11 @@ static void media_interface_init(struct media_device *mdev,
 
 struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
 						u32 type, u32 flags,
-						u32 major, u32 minor,
-						gfp_t gfp_flags)
+						u32 major, u32 minor)
 {
 	struct media_intf_devnode *devnode;
 
-	devnode = kzalloc(sizeof(*devnode), gfp_flags);
+	devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
 	if (!devnode)
 		return NULL;

drivers/media/v4l2-core/v4l2-dev.c +1 -2

index 430aa2330d07..982255d2063f 100644
@@ -777,8 +777,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
 	vdev->intf_devnode = media_devnode_create(vdev->v4l2_dev->mdev,
 						  intf_type,
 						  0, VIDEO_MAJOR,
-						  vdev->minor,
-						  GFP_KERNEL);
+						  vdev->minor);
 	if (!vdev->intf_devnode) {
 		media_device_unregister_entity(&vdev->entity);
 		return -ENOMEM;

include/media/media-entity.h +1 -3

index 9cbb10079024..44ab153c37fc 100644
@@ -71,7 +71,6 @@ struct media_gobj {
 	struct list_head	list;
 };
 
-
 struct media_pipeline {
 };

@@ -373,8 +372,7 @@ void media_entity_pipeline_stop(struct media_entity *entity);
 struct media_intf_devnode *
 __must_check media_devnode_create(struct media_device *mdev,
 				  u32 type, u32 flags,
-				  u32 major, u32 minor,
-				  gfp_t gfp_flags);
+				  u32 major, u32 minor);
 void media_devnode_remove(struct media_intf_devnode *devnode);
 struct media_link *
 __must_check media_create_intf_link(struct media_entity *entity,

Laurent Pinchart

maintainer

2015-11-23 16:41 UTC #3 lore raw

Re: [PATCH 1/2] [media] media_entity: remove gfp_flags argument

In reply to Mauro Carvalho Chehab

Hi Mauro,

Thank you for the patch.

Quoted 67 lines · On Wednesday 09 September 2015 08:32:02 Mauro Carvalho Chehab wrote:

We should not be creating device nodes at IRQ contexts. So, the only flags we'll be using will be GFP_KERNEL. Let's remove the gfp_flags, in order to make the interface simpler.
If we ever need it, it would be easy to revert those changes.
While here, remove an extra blank line.
Suggested-by Sakari Ailus <[email protected]>
Signed-off-by Mauro Carvalho Chehab <[email protected]>
(quoted diff) +0 -0
b/drivers/media/dvb-core/dvbdev.c index e9f24c1479dd..60828a9537a0 100644
drivers/media/dvb-core/dvbdev.c +1 -2
@@ -379,8 +379,7 @@ static int dvb_register_media_device(struct dvb_device
 *dvbdev,
 
 	dvbdev->intf_devnode = media_devnode_create(dvbdev->adapter->mdev,
 						    intf_type, 0,
-						    DVB_MAJOR, minor,
-						    GFP_KERNEL);
+						    DVB_MAJOR, minor);
 
 	if (!dvbdev->intf_devnode)
drivers/media/dvb-core/dvbdev.c +0 -0
 		return -ENOMEM;
drivers/media/media-entity.c +2 -3
index 4868b8269204..f28265864b76 100644
@@ -887,12 +887,11 @@ static void media_interface_init(struct media_device
 *mdev,
 
 struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
 						u32 type, u32 flags,
-						u32 major, u32 minor,
-						gfp_t gfp_flags)
+						u32 major, u32 minor)
 {
 	struct media_intf_devnode *devnode;
 
-	devnode = kzalloc(sizeof(*devnode), gfp_flags);
+	devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
 	if (!devnode)
 		return NULL;
(quoted diff) +0 -0
b/drivers/media/v4l2-core/v4l2-dev.c index 430aa2330d07..982255d2063f 100644
drivers/media/v4l2-core/v4l2-dev.c +0 -0
@@ -777,8 +777,7 @@ static int video_register_media_controller(struct
video_device *vdev, int type) vdev->intf_devnode = media_devnode_create(vdev->v4l2_dev->mdev,
drivers/media/v4l2-core/v4l2-dev.c +1 -2
 						  intf_type,
 						  0, VIDEO_MAJOR,
-						  vdev->minor,
-						  GFP_KERNEL);
+						  vdev->minor);
 	if (!vdev->intf_devnode) {
 		media_device_unregister_entity(&vdev->entity);
 		return -ENOMEM;
include/media/media-entity.h +0 -0
index 9cbb10079024..44ab153c37fc 100644

include/media/media-entity.h struct media_gobj { · line 74

Quoted 4 lines

include/media/media-entity.h +0 -1

@@ -71,7 +71,6 @@ struct media_gobj {
 	struct list_head	list;
 };
 
-

This belongs to "media: add a common struct to be embed on media graph objects". I know that you're reluctant to rebase, split and squash patches from fear of introducing breakages, but white space fixes should be really harmless :-)

The rest of the patch looks good to me, but as mentioned over IRC, I still think it would be beneficial to do a final round of rebase, split and squash as otherwise it's pretty difficult to perform a final review and ensure everything is right. This patch looks like a pretty safe candidate in that regard.

Quoted 13 lines

include/media/media-entity.h +1 -2

 struct media_pipeline {
 };

@@ -373,8 +372,7 @@ void media_entity_pipeline_stop(struct media_entity
 *entity); struct media_intf_devnode *
 __must_check media_devnode_create(struct media_device *mdev,
 				  u32 type, u32 flags,
-				  u32 major, u32 minor,
-				  gfp_t gfp_flags);
+				  u32 major, u32 minor);
 void media_devnode_remove(struct media_intf_devnode *devnode);
 struct media_link *
 __must_check media_create_intf_link(struct media_entity *entity,

Signature · 3 lines

Regards,

Laurent Pinchart

Mauro Carvalho Chehab

author

2015-09-09 11:32 UTC #4 lore raw

PATCH 2/2 media-device: use unsigned ints on some places

In reply to Mauro Carvalho Chehab

The entity->num_pads are defined as u16. So, better to use an unsigned int, as this prevents additional warnings when W=2 (or W=1 on some architectures).

The "i" counter at __media_device_get_topology() is also a monotonic counter that should never be below zero. So, make it unsigned too.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/media-device.c +4 -3

index 13987710e5bc..1312e93ebd6e 100644
@@ -243,7 +243,8 @@ static long __media_device_get_topology(struct media_device *mdev,
 	struct media_v2_interface uintf;
 	struct media_v2_pad upad;
 	struct media_v2_link ulink;
-	int ret = 0, i;
+	int ret = 0;
+	unsigned int i;
 
 	topo->topology_version = mdev->topology_version;

@@ -613,7 +614,7 @@ EXPORT_SYMBOL_GPL(media_device_unregister);
 int __must_check media_device_register_entity(struct media_device *mdev,
 					      struct media_entity *entity)
 {
-	int i;
+	unsigned int i;
 
 	if (entity->function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN ||
 	    entity->function == MEDIA_ENT_F_UNKNOWN)

@@ -650,9 +651,9 @@ EXPORT_SYMBOL_GPL(media_device_register_entity);
  */
 void media_device_unregister_entity(struct media_entity *entity)
 {
-	int i;
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct media_link *link, *tmp;
+	unsigned int i;
 
 	if (mdev == NULL)
 		return;

Signature · 1 line

2.4.3

Hans Verkuil

Acked-by on v1 2/2

2015-09-09 11:37 UTC #5 lore raw

Re: [PATCH 2/2] [media] media-device: use unsigned ints on some places

Quoted 10 lines · Mauro Carvalho Chehab

The entity->num_pads are defined as u16. So, better to use an unsigned int, as this prevents additional warnings when W=2 (or W=1 on some architectures).
The "i" counter at __media_device_get_topology() is also a monotonic counter that should never be below zero. So, make it unsigned too.
Suggested-by Sakari Ailus <[email protected]>
Signed-off-by Mauro Carvalho Chehab <[email protected]>

Acked-by Hans Verkuil <[email protected]>

Quoted 36 lines

drivers/media/media-device.c +4 -3

index 13987710e5bc..1312e93ebd6e 100644
@@ -243,7 +243,8 @@ static long __media_device_get_topology(struct media_device *mdev,
 	struct media_v2_interface uintf;
 	struct media_v2_pad upad;
 	struct media_v2_link ulink;
-	int ret = 0, i;
+	int ret = 0;
+	unsigned int i;
 
 	topo->topology_version = mdev->topology_version;

@@ -613,7 +614,7 @@ EXPORT_SYMBOL_GPL(media_device_unregister);
 int __must_check media_device_register_entity(struct media_device *mdev,
 					      struct media_entity *entity)
 {
-	int i;
+	unsigned int i;
 
 	if (entity->function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN ||
 	    entity->function == MEDIA_ENT_F_UNKNOWN)

@@ -650,9 +651,9 @@ EXPORT_SYMBOL_GPL(media_device_register_entity);
  */
 void media_device_unregister_entity(struct media_entity *entity)
 {
-	int i;
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct media_link *link, *tmp;
+	unsigned int i;
 
 	if (mdev == NULL)
 		return;

Laurent Pinchart

maintainer Reviewed-by on v1 2/2

2015-11-23 16:25 UTC #6 lore raw

Re: [PATCH 2/2] [media] media-device: use unsigned ints on some places

In reply to Mauro Carvalho Chehab

Hi Mauro,

Thank you for the patch.

Quoted 16 lines · On Wednesday 09 September 2015 08:32:03 Mauro Carvalho Chehab wrote:

The entity->num_pads are defined as u16. So, better to use an unsigned int, as this prevents additional warnings when W=2 (or W=1 on some architectures).
The "i" counter at __media_device_get_topology() is also a monotonic counter that should never be below zero. So, make it unsigned too.
Suggested-by Sakari Ailus <[email protected]>
Signed-off-by Mauro Carvalho Chehab <[email protected]>
drivers/media/media-device.c +0 -0
index 13987710e5bc..1312e93ebd6e 100644

drivers/media/media-device.c static long __media_device_get_topology(struct · line 247

Quoted 6 lines

drivers/media/media-device.c +2 -1

@@ -243,7 +243,8 @@ static long __media_device_get_topology(struct
 media_device *mdev, struct media_v2_interface uintf;
 	struct media_v2_pad upad;
 	struct media_v2_link ulink;
-	int ret = 0, i;
+	int ret = 0;
+	unsigned int i;

Very small nitpicking, I'd put the unsigned int i line before the int ret line to match the coding style of the rest of the file (with shorter variable declaration lines at the bottom).

Quoted 13 lines

drivers/media/media-device.c +1 -1

 	topo->topology_version = mdev->topology_version;

@@ -613,7 +614,7 @@ EXPORT_SYMBOL_GPL(media_device_unregister);
 int __must_check media_device_register_entity(struct media_device *mdev,
 					      struct media_entity *entity)
 {
-	int i;
+	unsigned int i;
 
 	if (entity->function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN ||
 	    entity->function == MEDIA_ENT_F_UNKNOWN)

drivers/media/media-device.c EXPORT_SYMBOL_GPL(media_device_register_entity); · line 656

Quoted 7 lines

drivers/media/media-device.c +1 -1

@@ -650,9 +651,9 @@ EXPORT_SYMBOL_GPL(media_device_register_entity);
  */
 void media_device_unregister_entity(struct media_entity *entity)
 {
-	int i;
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct media_link *link, *tmp;
+	unsigned int i;

Like you've done here, that's very good :-)

drivers/media/media-device.c EXPORT_SYMBOL_GPL(media_device_register_entity); · line 659

Quoted 3 lines

drivers/media/media-device.c +0 -0
 	if (mdev == NULL)
 		return;

With that fixed,

Reviewed-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart