Hi Sakari, On Fri, 11 Sep 2015 13:09:03 +0300 Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > Hi all, > > This patchset adds an API for managing entity enumerations, i.e. > storing a bit of information per entity. The entity ID is no longer > limited to small integers (e.g. 31 or 63), but > MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that > instead of a fixed number. > > If the number of entities in a real use case grows beyond the > capabilities of the approach, the algorithm may be changed. But most > importantly, the API is used to perform the same operation everywhere > it's done. > > I'm sending this for review only, the code itself is untested. > > I haven't entirely made up my mind on the fourth patch. It could be > dropped, as it uses the API for a somewhat different purpose. > Sorry for not reviewing this earlier, but I'm traveling this week to China, and I was having some troubles with the Internet. Btw, I don't have my notebook here (just a chromebook without the media tree). So, please consider this as just a preliminary review. I won't be comment this series patch by patch, because it is really painful to do it while here with this Internet. Also, I want to discuss the patch series as a hole. >From what we've agreed last week, we won't be using a separate ID range for the entity ID, but this patch series is actually adding it, and, IMHO, using a confusing nomenclature: instead of calling the entity ID range as "entity_id" at the media_device struct, you're now calling it "low_id". That sounds confusing to me. If you think we should keep a separate range for entities, calling it as "entity_id" is clearer. Also, you said at the low_id comment that if an entity is unregistered and then re-registered, it would preserve the same entity ID. That doesn't seem easy to implement, as we would need to track those previously-used ID. On the other hand, if we just re-use a previously released ID for some other entity, this can be a problem, as userspace may not be aware of such changes and might be asking the Kernel to do the wrong thing. So, I can't see how non-monotonically incremented numbers would work here. Finally, the changes you did still rely on having the ID limited to a well-defined, hardcoded number (MEDIA_ENTITY_MAX_LOW_ID). I can see this working only if: - We keep a separate range ID for entities (so, having a minimum of two ranges); - the entity maximum ID is defined by the driver (as the number of entities is actually dependent on the hardware); - some other mechanism would be available for drivers that would support dynamic entity creation. So, I don't see how this would solve the problems that we discussed at the last week IRC chats. Am I missing something? Regards, Mauro PS.: sparse already complains on two places at the media-entity where bitmaps are declared at the stack. With max entities equal to 64, that's not an issue, but if we change to a higher number, those will need to be dynamically allocated, in order to avoid stack overflows. I didn't see any patches touching that. -- 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
