Hi Laurent, > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > Sent: Friday, July 16, 2010 4:10 AM > To: Aguirre, Sergio > Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx > Subject: Re: [RFC/PATCH 03/10] media: Entities, pads and links > > Hi Sergio, > > Thanks for the review. > > On Thursday 15 July 2010 16:35:20 Aguirre, Sergio wrote: > > On Wednesday 14 July 2010 08:30:00 Laurent Pinchart wrote: > > [snip] > > > > diff --git a/drivers/media/media-device.c b/drivers/media/media- > device.c > > > index a4d3db5..6361367 100644 > > > --- a/drivers/media/media-device.c > > > +++ b/drivers/media/media-device.c > > [snip] > > > > @@ -72,6 +77,54 @@ EXPORT_SYMBOL_GPL(media_device_register); > > [snip] > > > > + > > > +/** > > > + * media_device_register_entity - Register an entity with a media > device > > > + * @mdev: The media device > > > + * @entity: The entity > > > + */ > > > +int __must_check media_device_register_entity(struct media_device > *mdev, > > > + struct media_entity > *entity) > > > +{ > > > > What if mdev == NULL OR entity == NULL? > > It's not a valid use case. Drivers must not try to register a NULL entity, > or > an entity to a NULL media device. Ok. > > > > + /* Warn if we apparently re-register an entity */ > > > + WARN_ON(entity->parent != NULL); > > > > Shouldn't we exit with -EBUSY here instead? Or is there a usecase > > In which this is a valid scenario? > > entity->parent != NULL isn't a valid scenario. It's a driver bug, and > WARN_ON > will result in a backtrace being printed, notifying the driver developer > that > something is seriously wrong. Ok. > > > > + entity->parent = mdev; > > > + > > > + spin_lock(&mdev->lock); > > > + entity->id = mdev->entity_id++; > > > + list_add_tail(&entity->list, &mdev->entities); > > > + spin_unlock(&mdev->lock); > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(media_device_register_entity); > > > + > > > +/** > > > + * media_device_unregister_entity - Unregister an entity > > > + * @entity: The entity > > > + * > > > + * If the entity has never been registered this function will return > > > + * immediately. > > > + */ > > > +void media_device_unregister_entity(struct media_entity *entity) > > > +{ > > > + struct media_device *mdev = entity->parent; > > > > What if entity == NULL? > > Still not a valid use case, don't unregister NULL. Ok. > > > > + > > > + if (mdev == NULL) > > > + return; > > > + > > > + spin_lock(&mdev->lock); > > > + list_del(&entity->list); > > > + spin_unlock(&mdev->lock); > > > + entity->parent = NULL; > > > +} > > > +EXPORT_SYMBOL_GPL(media_device_unregister_entity); > > > diff --git a/drivers/media/media-entity.c b/drivers/media/media- > entity.c > > > new file mode 100644 > > > index 0000000..d5a4b4c > > > --- /dev/null > > > +++ b/drivers/media/media-entity.c > > > @@ -0,0 +1,145 @@ > > > +/* > > > + * Media Entity support > > > + * > > > + * Copyright (C) 2009 Laurent Pinchart > > > <laurent.pinchart@xxxxxxxxxxxxxxxx> > > > > 2010? > > Oops, yes, will fix. > > [snip] > > > > +int > > > +media_entity_create_link(struct media_entity *source, u8 source_pad, > > > + struct media_entity *sink, u8 sink_pad, u32 > flags) > > > +{ > > > + struct media_entity_link *link; > > > + struct media_entity_link *backlink; > > > + > > > + BUG_ON(source == NULL || sink == NULL); > > > + BUG_ON(source_pad >= source->num_pads); > > > + BUG_ON(sink_pad >= sink->num_pads); > > > > Isn't this too dramatic? :) > > > > I mean, does the entire system needs to be halted because you won't be > > able to link your video subdevices? > > If source or sink is NULL, the second or third BUG_ON will result in a > crash. > If the source or sink pad numbers are out of bounds, undefined memory will > be > dereferenced later. Both conditions will likely result in a crash, so it's > better to have a predictable, easy to understand crash. > > Once again this should never happen. It's a driver bug if it does, and > should > not happen randomly at runtime. Ok. > > [snip] > > > > diff --git a/include/media/media-entity.h b/include/media/media- > entity.h > > > new file mode 100644 > > > index 0000000..0929a90 > > > --- /dev/null > > > +++ b/include/media/media-entity.h > > > @@ -0,0 +1,79 @@ > > > +#ifndef _MEDIA_ENTITY_H > > > +#define _MEDIA_ENTITY_H > > > + > > > +#include <linux/list.h> > > > + > > > +#define MEDIA_ENTITY_TYPE_NODE 1 > > > +#define MEDIA_ENTITY_TYPE_SUBDEV 2 > > > + > > > +#define MEDIA_NODE_TYPE_V4L 1 > > > +#define MEDIA_NODE_TYPE_FB 2 > > > +#define MEDIA_NODE_TYPE_ALSA 3 > > > +#define MEDIA_NODE_TYPE_DVB 4 > > > + > > > +#define MEDIA_SUBDEV_TYPE_VID_DECODER 1 > > > +#define MEDIA_SUBDEV_TYPE_VID_ENCODER 2 > > > +#define MEDIA_SUBDEV_TYPE_MISC 3 > > > + > > > +#define MEDIA_LINK_FLAG_ACTIVE (1 << 0) > > > +#define MEDIA_LINK_FLAG_IMMUTABLE (1 << 1) > > > + > > > +#define MEDIA_PAD_TYPE_INPUT 1 > > > +#define MEDIA_PAD_TYPE_OUTPUT 2 > > > > Shouldn't all the above be enums? (except of course the > > MEDIA_LINK_FLAG_* defines) > > They can be enums, but the structures used in ioctls must use integer > types as > enum sizes vary depending on the ABI on some platforms (most notably ARM). > I > have no strong opinion for or against enums for the definitions of the > values. Ok, I guess either way is fine in that case. Regards, Sergio > > -- > Regards, > > Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
