Re: [RFC 2/2] vim2m: add media device

[emil . velikov]

Hi Ezequiel,  

On Tue, Jun 12, 2018 at 07:48:27AM -0300, Ezequiel Garcia wrote:

> @@ -1013,10 +1016,10 @@ static int vim2m_probe(struct platform_device *pdev)
>   vfd->lock = >dev_mutex;
>   vfd->v4l2_dev = >v4l2_dev;
>  
> - ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> + ret = video_register_device(vfd, VFL_TYPE_MEM2MEM, 0);
Shouldn't the original type be used when building without
CONFIG_MEDIA_CONTROLLER?


> @@ -1050,6 +1076,11 @@ static int vim2m_remove(struct platform_device *pdev)
>   struct vim2m_dev *dev = platform_get_drvdata(pdev);
>  
>   v4l2_info(>v4l2_dev, "Removing " MEM2MEM_NAME);
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
Gut suggests that media_device_unregister() should be called here.

Then again my experience in media/ is limited so I could be miles off
;-)


HTH
Emil

Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

[Emil Velikov]

HI Maciej,

Just sharing a couple of fly-by ideas - please don't read too much into them.

On 19 February 2018 at 15:43, Maciej Purski  wrote:
> When a driver is going to use clk_bulk_get() function, it has to
> initialize an array of clk_bulk_data, by filling its id fields.
>
> Add a new function to the core, which dynamically allocates
> clk_bulk_data array and fills its id fields. Add clk_bulk_free()
> function, which frees the array allocated by clk_bulk_alloc() function.
> Add a managed version of clk_bulk_alloc().
>
Most places use a small fixed number of struct clk pointers.
Using devres + kalloc to allocate 1-4 pointers feels a bit strange.

Quick grep shows over 150 instances that could be updated to use the new API.
Adding a cocci script to simplify the transition would be a good idea.

> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
The extra header declaration should not be needed. One should be able
to forward declare any undefined structs.

HTH
Emil

Re: [PATCHv3 17/22] staging: android: ion: Collapse internal header files

[Emil Velikov]

Hi Laura,

Couple of trivial nitpicks below.

On 3 April 2017 at 19:57, Laura Abbott  wrote:

> --- a/drivers/staging/android/ion/ion.h
> +++ b/drivers/staging/android/ion/ion.h
> @@ -1,5 +1,5 @@
>  /*
> - * drivers/staging/android/ion/ion.h
> + * drivers/staging/android/ion/ion_priv.h
Does not match the actual filename.

>   *
>   * Copyright (C) 2011 Google, Inc.
>   *
> @@ -14,24 +14,26 @@
>   *
>   */
>
> -#ifndef _LINUX_ION_H
> -#define _LINUX_ION_H
> +#ifndef _ION_PRIV_H
> +#define _ION_PRIV_H
>
Ditto.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
>  #include 
> +#include 
>
>  #include "../uapi/ion.h"
>
You don't want to use "../" in includes. Perhaps address with another
patch, if you haven't already ?

Regards,
Emil

Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support

[Emil Velikov]

On 6 March 2017 at 10:29, Daniel Vetter  wrote:
> On Fri, Mar 03, 2017 at 10:46:03AM -0800, Laura Abbott wrote:
>> On 03/03/2017 08:39 AM, Laurent Pinchart wrote:
>> > Hi Daniel,
>> >
>> > On Friday 03 Mar 2017 10:56:54 Daniel Vetter wrote:
>> >> On Thu, Mar 02, 2017 at 01:44:38PM -0800, Laura Abbott wrote:
>> >>> Now that we call dma_map in the dma_buf API callbacks there is no need
>> >>> to use the existing cache APIs. Remove the sync ioctl and the existing
>> >>> bad dma_sync calls. Explicit caching can be handled with the dma_buf
>> >>> sync API.
>> >>>
>> >>> Signed-off-by: Laura Abbott 
>> >>> ---
>> >>>
>> >>>  drivers/staging/android/ion/ion-ioctl.c |  5 
>> >>>  drivers/staging/android/ion/ion.c   | 40 
>> >>> 
>> >>>  drivers/staging/android/ion/ion_carveout_heap.c |  6 
>> >>>  drivers/staging/android/ion/ion_chunk_heap.c|  6 
>> >>>  drivers/staging/android/ion/ion_page_pool.c |  3 --
>> >>>  drivers/staging/android/ion/ion_system_heap.c   |  5 
>> >>>  6 files changed, 65 deletions(-)
>> >>>
>> >>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> >>> b/drivers/staging/android/ion/ion-ioctl.c index 5b2e93f..f820d77 100644
>> >>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> >>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> >>> @@ -146,11 +146,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>> >>> unsigned long arg)>
>> >>>   data.handle.handle = handle->id;
>> >>>
>> >>>   break;
>> >>>
>> >>>   }
>> >>>
>> >>> - case ION_IOC_SYNC:
>> >>> - {
>> >>> - ret = ion_sync_for_device(client, data.fd.fd);
>> >>> - break;
>> >>> - }
>> >>
>> >> You missed the case ION_IOC_SYNC: in compat_ion.c.
>> >>
>> >> While at it: Should we also remove the entire custom_ioctl infrastructure?
>> >> It's entirely unused afaict, and for a pure buffer allocator I don't see
>> >> any need to have custom ioctl.
>> >
>> > I second that, if you want to make ion a standard API, then we certainly 
>> > don't
>> > want any custom ioctl.
>> >
>> >> More code to remove potentially:
>> >> - The entire compat ioctl stuff - would be an abi break, but I guess if we
>> >>   pick the 32bit abi and clean up the uapi headers we'll be mostly fine.
>> >>   would allow us to remove compat_ion.c entirely.
>> >>
>> >> - ION_IOC_IMPORT: With this ion is purely an allocator, so not sure we
>> >>   still need to be able to import anything. All the cache flushing/mapping
>> >>   is done through dma-buf ops/ioctls.
>> >>
>> >>
>>
>> Good point to all of the above. I was considering keeping the import around
>> for backwards compatibility reasons but given how much other stuff is being
>> potentially broken, everything should just get ripped out.
>
> If you're ok with breaking the world, then I strongly suggest we go
> through the uapi header and replace all types with the standard
> fixed-width ones (__s32, __s64 and __u32, __u64). Allows us to remove all
> the compat ioctl code :-)

I think the other comments from your "botching-up ioctls" [1] also apply ;-)
Namely - align structs to multiple of 64bit, add "flags" and properly
verity user input returning -EINVAL.

-Emil

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt

Re: [PATCH 00/35] treewide trivial patches converting pr_warning to pr_warn

[Emil Velikov]

On 23 February 2017 at 17:18, Joe Perches  wrote:
> On Thu, 2017-02-23 at 09:28 -0600, Rob Herring wrote:
>> On Fri, Feb 17, 2017 at 1:11 AM, Joe Perches  wrote:
>> > There are ~4300 uses of pr_warn and ~250 uses of the older
>> > pr_warning in the kernel source tree.
>> >
>> > Make the use of pr_warn consistent across all kernel files.
>> >
>> > This excludes all files in tools/ as there is a separate
>> > define pr_warning for that directory tree and pr_warn is
>> > not used in tools/.
>> >
>> > Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing.
> []
>> Where's the removal of pr_warning so we don't have more sneak in?
>
> After all of these actually get applied,
> and maybe a cycle or two later, one would
> get sent.
>
By which point you'll get a few reincarnation of it. So you'll have to
do the same exercise again :-(

I guess the question is - are you expecting to get the series merged
all together/via one tree ? If not, your plan is perfectly reasonable.
Fwiw in the DRM subsystem, similar cleanups does purge the respective
macros/other with the final commit. But there one can pull the lot in
one go.

Regards,
Emil

Re: [PATCH v7 1/3] create SMAF module

[Emil Velikov]

On 17 May 2016 at 22:29, Daniel Vetter  wrote:

> Please don't use __kernel_size_t, it's only for backwards compat if you
> already botched an ioctl definition ;-)
>
That explains why I've not seen it in (m)any other UAPI headers but
our legacy ones.

Thank you !
Emil
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/3] create SMAF module

[Emil Velikov]

On 17 May 2016 at 14:50, Benjamin Gaignard  wrote:
> Hello Emil,
>
> thanks for your review.
> I have understand most of your remarks and I'm fixing them
> but some points aren't obvious for me...
>
Sure thing. Thanks for being honest.

>
> No because a device could attach itself on the buffer and the
> allocator will only
> be selected at the first map_attach call.
> The goal is to delay the allocation until all devices are attached to
> select the best allocator.
>
I see. Makes sense.


>>> +static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long 
>>> arg)
>>> +{
>>> +   switch (cmd) {
>>> +   case SMAF_IOC_CREATE:
>>> +   {
>>> +   struct smaf_create_data data;
>>> +   struct smaf_handle *handle;
>>> +
>>> +   if (copy_from_user(, (void __user *)arg, 
>>> _IOC_SIZE(cmd)))
>>> +   return -EFAULT;
>>> +
>>> +   handle = smaf_create_handle(data.length, data.flags);
>> We want to sanitise the input data.{length,flags} before sending it
>> deeper in the kernel.
>
> Sorry but can you elaborate little more here ?
> I don't understand your expectations.
>
You want to determine which flags are 'considered useful' at this
stage, and reject anything else. As-is you inject any flags that the
user gives you directly into the 'guts' of the kernel. This is not
good from security and future expandability POV.

About the length you want a similar thing. size_t is unsigned (great),
although ideally you'll want to check/determine if one cannot exploit
it, integer overflow being the more common suspect. This may be quite
hard to track, so I'd stick with the flags checking at least.


> It is useless the add this function in this .h file I will remove it
> and fix the comment in structure defintion

I've seen both approaches - description next to the declaration or
definition. I'd rather not pick sides, as people might throw rotten
fruit at me ;-)


>>> +/**
>>> + * struct smaf_create_data - allocation parameters
>>> + * @length:size of the allocation
>>> + * @flags: flags passed to allocator
>>> + * @name:  name of the allocator to be selected, could be NULL
Just occurred to - you might want to comment what the user should
expect if NULL. Any at random one will be used or otherwise. Very
quick description on the heuristics used might be good as well.

>> Is it guaranteed to be null terminated ? If so one should mentioned it
>> otherwise your userspace should be fixed.
>> Same comments apply for smaf_info::name.
>
> I have used strncpy everywhere to avoid this problem but maybe it is not 
> enough
>
According to the man page

"The strncpy() function is similar, except that at most n bytes of src
are copied.  Warning: If there is no null byte among the first n bytes
of src, the string placed in dest will _not_ be null-terminated."

Annotation is mine obviously. I believe that after the strncpy 'name'
is/was assumed (used as) a NULL terminated string.

>>
>>
>>> + * @fd:returned file descriptor
>>> + */
>>> +struct smaf_create_data {
>>> +   size_t length;
>>> +   unsigned int flags;
>>> +   char name[ALLOCATOR_NAME_LENGTH];
>>> +   int fd;
>> The structs here feels quite fragile. Please read up on Daniel
>> Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
>> useful is such process.
>>
> if I describe the structures like this:
> /**
>  * struct smaf_create_data - allocation parameters
>  * @length: size of the allocation
>  * @flags: flags passed to allocator
>  * @name_len: length of name
>  * @name: name of the allocator to be selected, could be NULL
>  * @fd: returned file descriptor
>  */
> struct smaf_create_data {
> size_t length;
> unsigned int flags;
> size_t name_len;
> char __user *name;
> int fd;
> char padding[44];
> };
>
> does it sound more robust for you ?
>
Seems like you changed 'name' from fixed size array to char *. Which
actually gets us slightly further away from robust.

As Daniel said, please read through the hole file.

Here is a (slightly incomplete) gist of it all:
- you want to to use __[us]{8,16,32,64} and __kernel_size_t types everywhere
- each member of the struct must be the same offset for both 32bit and
64bit builds. ^^ helps with that
- double check for gaps in the struct - I think you have a few
- each struct should have it's old 'flags' which you'll use to
indicate the capability of the ioctl and thus the size of struct used.
i.e. it's for future use.

Obviously the other two structs need similar polish.

Here is how you can check things with pahole:
 - Create a simple C file that includes the header and has an instance
of each struct - it doesn't have to be use any of them.
 - Compile for 32 and 64 bit with -g -O0. Compare the struct layout -
it should be identical in both cases.

Hope that makes things a bit clearer.

Emil
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the 

Re: [PATCH v7 3/3] SMAF: add fake secure module

[Emil Velikov]

Hi Benjamin,

On 9 May 2016 at 16:07, Benjamin Gaignard  wrote:
> This module is allow testing secure calls of SMAF.
>
"Add fake secure module" does sound like something not (m)any people
want to hear ;-)
Have you considered calling it 'dummy', 'test' or similar ?


> --- /dev/null
> +++ b/drivers/smaf/smaf-fakesecure.c
> @@ -0,0 +1,85 @@
> +/*
> + * smaf-fakesecure.c
> + *
> + * Copyright (C) Linaro SA 2015
> + * Author: Benjamin Gaignard  for Linaro.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#include 
> +#include 
> +#include 
> +
> +#define MAGIC 0xDEADBEEF
> +
> +struct fake_private {
> +   int magic;
> +};
> +
> +static void *smaf_fakesecure_create(void)
> +{
> +   struct fake_private *priv;
> +
> +   priv = kzalloc(sizeof(*priv), GFP_KERNEL);
Missing ENOMEM handling ?

> +   priv->magic = MAGIC;
> +
> +   return priv;
> +}
> +
> +static int smaf_fakesecure_destroy(void *ctx)
> +{
> +   struct fake_private *priv = (struct fake_private *)ctx;
You might want to flesh this cast into a (inline) helper and use it throughout ?


... and that is all. Hope these were useful, or at the very least not
utterly wrong, suggestions :-)


Regards,
Emil

P.S. From a quick look userspace has some subtle bugs/odd practises.
Let me know if you're interested in my input.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 2/3] SMAF: add CMA allocator

[Emil Velikov]

Hi Benjamin,

On 9 May 2016 at 16:07, Benjamin Gaignard  wrote:
> SMAF CMA allocator implement helpers functions to allow SMAF
> to allocate contiguous memory.
>
> match() each if at least one of the attached devices have coherent_dma_mask
> set to DMA_BIT_MASK(32).
>
What is the idea behind the hardcoded 32. Wouldn't it be better to avoid that ?


> +static void smaf_cma_release(struct dma_buf *dmabuf)
> +{
> +   struct smaf_cma_buffer_info *info = dmabuf->priv;
> +   DEFINE_DMA_ATTRS(attrs);
> +
> +   dma_set_attr(DMA_ATTR_WRITE_COMBINE, );
> +
Imho it's worth storing the dma_attrs within smaf_cma_buffer_info.
This way it's less likely for things to go wrong, if one forgets to
update one of the three in the future.


> +static void smaf_cma_unmap(struct dma_buf_attachment *attachment,
> +  struct sg_table *sgt,
> +  enum dma_data_direction direction)
> +{
> +   /* do nothing */
There could/should really be a comment explaining why we "do nothing"
here, right ?

> +}
> +
> +static int smaf_cma_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> +{
> +   struct smaf_cma_buffer_info *info = dmabuf->priv;
> +   int ret;
> +   DEFINE_DMA_ATTRS(attrs);
> +
> +   dma_set_attr(DMA_ATTR_WRITE_COMBINE, );
> +
> +   if (info->size < vma->vm_end - vma->vm_start)
> +   return -EINVAL;
> +
> +   vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> +   ret = dma_mmap_attrs(info->dev, vma, info->vaddr, info->paddr,
> +info->size, );
> +
> +   return ret;
Kill the temporary variable 'ret' ?


> +static struct dma_buf_ops smaf_cma_ops = {
const ? Afaict the compiler would/should warn you about discarding it
as the ops are defined const.


> +static struct dma_buf *smaf_cma_allocate(struct dma_buf *dmabuf,
> +size_t length, unsigned int flags)
> +{
> +   struct dma_buf_attachment *attach_obj;
> +   struct smaf_cma_buffer_info *info;
> +   struct dma_buf *cma_dmabuf;
> +   int ret;
> +
> +   DEFINE_DMA_BUF_EXPORT_INFO(export);
> +   DEFINE_DMA_ATTRS(attrs);
> +
> +   dma_set_attr(DMA_ATTR_WRITE_COMBINE, );
> +
> +   info = kzalloc(sizeof(*info), GFP_KERNEL);
> +   if (!info)
> +   return NULL;
> +
> +   info->dev = find_matching_device(dmabuf);
find_matching_device() can return NULL. We should handle that imho.

> +   info->size = length;
> +   info->vaddr = dma_alloc_attrs(info->dev, info->size, >paddr,
> + GFP_KERNEL | __GFP_NOWARN, );
> +   if (!info->vaddr) {
> +   ret = -ENOMEM;
set-but-unused-variable 'ret' ?

> +   goto error;
> +   }
> +
> +   export.ops = _cma_ops;
> +   export.size = info->size;
> +   export.flags = flags;
> +   export.priv = info;
> +
> +   cma_dmabuf = dma_buf_export();
> +   if (IS_ERR(cma_dmabuf))
Missing dma_free_attrs() ? I'd add another label in the error path and
handle it there.

> +   goto error;
> +
> +   list_for_each_entry(attach_obj, >attachments, node) {
> +   dma_buf_attach(cma_dmabuf, attach_obj->dev);
Imho one should error out if attach fails. Or warn at the very least ?


> +static int __init smaf_cma_init(void)
> +{
> +   INIT_LIST_HEAD(_cma.list_node);
Isn't this something that smaf_register_allocator() should be doing ?


Regards,
Emil
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/3] create SMAF module

[Emil Velikov]

 Hi Benjamin,

I'd suspect you're interested in some feedback on these, so here is a few :-)
Sadly (ideally?) nothing serious, but a bunch minor suggestions, plus
the odd bug.

On 9 May 2016 at 16:07, Benjamin Gaignard  wrote:

> --- /dev/null
> +++ b/drivers/smaf/smaf-core.c
> @@ -0,0 +1,794 @@
> +/*
> + * smaf.c
The comment does not match the actual file name.

You could give a brief summary of the file(s), if you're feeling gracious ;-)


> +
> +/**
> + * smaf_grant_access - return true if the specified device can get access
> + * to the memory area
> + *
Reading this makes me wonder if {request,allow}_access won't be better name ?


> +static int smaf_secure_handle(struct smaf_handle *handle)
> +{
> +   if (atomic_read(>is_secure))
> +   return 0;
> +
> +   if (!have_secure_module())
> +   return -EINVAL;
> +
> +   handle->secure_ctx = smaf_dev.secure->create_ctx();
> +
Should one use a temporary variable so that the caller provided
storage is unchanged in case of an error ?

> +   if (!handle->secure_ctx)
> +   return -EINVAL;
> +
> +   atomic_set(>is_secure, 1);
> +   return 0;
> +}
> +


> +int smaf_register_secure(struct smaf_secure *s)
> +{
> +   /* make sure that secure module have all required functions
> +* to avoid test them each time later
> +*/
> +   WARN_ON(!s || !s->create_ctx || !s->destroy_ctx ||
> +   !s->grant_access || !s->revoke_access);
> +
Is something like below reasonable thing to do in the kernel ?
Same question goes for smaf_register_allocator() further down.

if (!s || )
  return -ESHOULDNEVERHAPPEN;



> +static struct vm_operations_struct smaf_vma_ops = {
Ops/vfucs normally are const data. Is there something preventing us here ?

> +   .close = smaf_vm_close,
> +};
> +
> +static int smaf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> +{
> +   struct smaf_handle *handle = dmabuf->priv;
> +   bool ret;
> +   enum dma_data_direction dir;
> +
> +   /* if no allocator attached, get the first allocator */
> +   if (!handle->allocator) {
> +   struct smaf_allocator *alloc;
> +
> +   mutex_lock(_dev.lock);
> +   alloc = smaf_get_first_allocator(dmabuf);
> +   mutex_unlock(_dev.lock);
> +
> +   /* still no allocator ? */
> +   if (!alloc)
> +   return -EINVAL;
> +
> +   handle->allocator = alloc;
> +   }
> +
> +   if (!handle->db_alloc) {
> +   struct dma_buf *db_alloc;
> +
> +   db_alloc = handle->allocator->allocate(dmabuf,
> +  handle->length,
> +  handle->flags);
> +   if (!db_alloc)
> +   return -EINVAL;
> +
> +   handle->db_alloc = db_alloc;
> +   }
> +
The above half of the function looks identical to smaf_map_dma_buf().
Worth factoring it out to a helper function ?


> +static int smaf_attach(struct dma_buf *dmabuf, struct device *dev,
> +  struct dma_buf_attachment *attach)
> +{
> +   struct smaf_handle *handle = dmabuf->priv;
> +   struct dma_buf_attachment *db_attach;
> +
> +   if (!handle->db_alloc)
> +   return 0;
> +
Shouldn't one return an error (-EINVAL or similar) here ?


> +static struct dma_buf_ops smaf_dma_buf_ops = {
const ? From a very quick look the compiler should warn us about it -
"smaf_dma_buf_ops discards const qualifier" or similar.


> +struct smaf_handle *smaf_create_handle(size_t length, unsigned int flags)
> +{
> +   struct smaf_handle *handle;
> +
> +   DEFINE_DMA_BUF_EXPORT_INFO(info);
> +
> +   handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> +   if (!handle)
> +   return ERR_PTR(-ENOMEM);
> +
Err this should be return NULL; correct ?

> +   info.ops = _dma_buf_ops;
> +   info.size = round_up(length, PAGE_SIZE);
> +   info.flags = flags;
> +   info.priv = handle;
> +
> +   handle->dmabuf = dma_buf_export();
> +   if (IS_ERR(handle->dmabuf)) {
> +   kfree(handle);
> +   return NULL;
> +   }
> +
> +   handle->length = info.size;
> +   handle->flags = flags;
> +
> +   return handle;
> +}
> +EXPORT_SYMBOL(smaf_create_handle);
> +
> +static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long 
> arg)
> +{
> +   switch (cmd) {
> +   case SMAF_IOC_CREATE:
> +   {
> +   struct smaf_create_data data;
> +   struct smaf_handle *handle;
> +
> +   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
> +   return -EFAULT;
> +
> +   handle = smaf_create_handle(data.length, data.flags);
We want to sanitise the input data.{length,flags} before sending it
deeper in the kernel.

> +

Re: [PATCH v3] libgencec: Add userspace library for the generic CEC kernel interface

[Emil Velikov]

Hi Kamil,

There are a couple of bits that I've missed out previously. All but
the missing install of libgencec.pc are general cleanups.

On 6 May 2015 at 13:37, Kamil Debski k.deb...@samsung.com wrote:

 --- /dev/null
 +++ b/.gitignore
 @@ -0,0 +1,26 @@

 +/ar-lib
You can drop this (see the configure.ac note).

 +/aclocal.m4
 +/autom4te.cache/*
 +/config.*
 +/configure
 +/depcomp
 +/install-sh
 +/missing
Normally one can drop the leading forward slash in .gitignore files.
This way git won't bother if you build in a subdirectory - for example
$(project_top)/build.

 +/examples/.deps/*
 +/examples/.libs/*
 +src/.deps/*
 +src/.libs/*
One can replace these four with
.deps/
.libs/

 +/examples/cectest
echo cectest  examples/.gitignore


 --- /dev/null
 +++ b/Makefile.am
 @@ -0,0 +1,4 @@
 +SUBDIRS = src examples
 +ACLOCAL_AMFLAGS = -I m4
 +library_includedir=$(includedir)
 +library_include_HEADERS = include/gencec.h
We want to install the pc file. Otherwise one cannot really use it.

pkgconfigdir = $(libdir)/pkgconfig
pkgconfig_DATA = libgencec.pc


 --- /dev/null
 +++ b/configure.ac
 @@ -0,0 +1,27 @@
 +AC_PREREQ(2.60)
 +
 +AC_INIT([libgencec], [0.1], [k.deb...@samsung.com])
 +AM_INIT_AUTOMAKE([-Wall -Werror foreign])
 +
 +AC_PROG_CC
 +AM_PROG_AR
There is not plan to use the library on Windows is there ? If so we
can remove this as per the manual [1].

You must use this macro when you use the archiver in your project, if
you want support for unusual archivers such as Microsoft lib

Cheers,
Emil

[1] http://www.gnu.org/software/automake/manual/html_node/Public-Macros.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libgencec: Add userspace library for the generic CEC kernel interface

[Emil Velikov]

Hi Kamil,

It seems that you've only incorporated the libgencec.pc suggestion.
Did you change your mind about the others, found out something funny
with them (if so can you let me know what) or simply forgot about them
?

On 30 April 2015 at 11:25, Kamil Debski k.deb...@samsung.com wrote:
 Hi Emil,

 From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
 ow...@vger.kernel.org] On Behalf Of Emil Velikov
 Sent: Wednesday, April 29, 2015 5:00 PM

 Hi Kamil,

 Allow me to put a few suggestions:

 On 29 April 2015 at 11:02, Kamil Debski k.deb...@samsung.com wrote:

  diff --git a/m4/.gitkeep b/m4/.gitkeep new file mode 100644 index
  000..e69de29
 Haven't seen any projects doing this. Curious what the benefits of
 keeping and empty folder might be ?

 When I run autoreconf -i it complained about missing m4 folder, hence I added
 this filler file such that the folder is created. Any suggestion on how to do
 this better?

Ahh yes - that lovely message. It turns out that older versions of
automake will even error out [1], rather than just printing out a
warning. A handy workaround would be to add a .gitignore (and a second
one in the top folder) list. Plus it will slim down the untracked
files list and let you clearly see when git add was missed :-)

Cheers
Emil

[1] https://bugzilla.redhat.com/show_bug.cgi?id=901333
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libgencec: Add userspace library for the generic CEC kernel interface

[Emil Velikov]

Hi Kamil,

Allow me to put a few suggestions:

On 29 April 2015 at 11:02, Kamil Debski k.deb...@samsung.com wrote:
 This is the first version of the libGenCEC library. It was designed to
 act as an interface between the generic CEC kernel API and userspace
 applications. It provides a simple interface for applications and an
 example application that can be used to test the CEC functionality.

 signed-off-by: Kamil Debski k.deb...@samsung.com
 ---
  AUTHORS  |1 +
  INSTALL  |9 +
  LICENSE  |  202 
  Makefile.am  |4 +
  README   |   22 ++
  configure.ac |   24 ++
  doc/index.html   |  345 +++
  examples/Makefile.am |4 +
  examples/cectest.c   |  631 
 ++
  include/gencec.h |  255 
  src/Makefile.am  |4 +
  src/gencec.c |  445 +++
  12 files changed, 1946 insertions(+)
  create mode 100644 AUTHORS
  create mode 100644 INSTALL
  create mode 100644 LICENSE
  create mode 100644 Makefile.am
  create mode 100644 README
  create mode 100644 configure.ac
  create mode 100644 doc/index.html
  create mode 100644 examples/Makefile.am
  create mode 100644 examples/cectest.c
  create mode 100644 include/gencec.h
  create mode 100644 m4/.gitkeep
  create mode 100644 src/Makefile.am
  create mode 100644 src/gencec.c

 diff --git a/AUTHORS b/AUTHORS
 new file mode 100644
 index 000..e4b7117
 --- /dev/null
 +++ b/AUTHORS
 @@ -0,0 +1 @@
 +Kamil Debski k.deb...@samsung.com
 diff --git a/INSTALL b/INSTALL
 new file mode 100644
 index 000..aac6101
 --- /dev/null
 +++ b/INSTALL
 @@ -0,0 +1,9 @@
 +To install libgencec run following commands:
 +
 +autoreconf -i
You might want add --force in here, otherwise the files will stay
as-is if you update libtool and friends mid-flight.
Many projects include autogen.sh like the following. Feel free to add
it to libgencec.

$cat autogen.sh
#! /bin/sh

srcdir=`dirname $0`
test -z $srcdir  srcdir=.

ORIGDIR=`pwd`
cd $srcdir

autoreconf --force --verbose --install || exit 1
cd $ORIGDIR || exit $?

if test -z $NOCONFIGURE; then
$srcdir/configure $@
fi



 --- /dev/null
 +++ b/configure.ac
 @@ -0,0 +1,24 @@

You can save yourself some headaches if you restrict old and/or buggy
autoconf versions which don't work with the project.
If I have to guess 2.60 should be ok.
AC_PREREQ([XXX])

 +AC_INIT([libgencec], [0.1], [k.deb...@samsung.com])
 +AM_INIT_AUTOMAKE([-Wall -Werror foreign])
 +
 +AC_PROG_CC
 +AM_PROG_AR
 +AC_CONFIG_MACRO_DIR([m4])
 +AC_DEFINE([_GNU_SOURCE], [], [Use GNU extensions])
 +

Same for libtool - 2.2 perhaps ?
LT_PREREQ([XXX])

 +LT_INIT
 +
 +# Checks for typedefs, structures, and compiler characteristics.
 +AC_C_INLINE
 +AC_TYPE_SIZE_T
 +AC_TYPE_UINT16_T
 +AC_TYPE_UINT32_T
 +AC_TYPE_UINT8_T
 +
 +#AC_CHECK_LIB([c], [malloc])
 +# Checks for library functions.
 +#AC_FUNC_MALLOC
 +
 +AC_CONFIG_FILES([Makefile src/Makefile examples/Makefile])
Would be nice if the library provides libgencec.pc file. This way any
users can avoid the explicit header/library check, amongst other
useful bits.

 --- /dev/null
 +++ b/examples/Makefile.am
 @@ -0,0 +1,4 @@
 +bin_PROGRAMS = cectest
 +cectest_SOURCES = cectest.c
 +AM_CPPFLAGS=-I$(top_srcdir)/include/
 +AM_LDFLAGS=-L../src/ -lgencec
The following seems more common (in the projects I've seen at least)
cectest_LDADD = $(top_builddir)/src/libgencec.la

 diff --git a/m4/.gitkeep b/m4/.gitkeep
 new file mode 100644
 index 000..e69de29
Haven't seen any projects doing this. Curious what the benefits of
keeping and empty folder might be ?

 diff --git a/src/Makefile.am b/src/Makefile.am
 new file mode 100644
 index 000..cb024f0
 --- /dev/null
 +++ b/src/Makefile.am
 @@ -0,0 +1,4 @@
 +lib_LTLIBRARIES = libgencec.la
 +libgencec_la_SOURCES = gencec.c
 +libgencec_la_LDFLAGS = -version-info 0:1:0
You might want to add -no-undefined in order to prevent having a
library with unresolved symbols.

Hope you find the above useful :-)

Cheers
Emil
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html