linux-kernel - Re: [PATCH v14 2/6] media: imagination: Add E5010 JPEG Encoder driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 19:38:16 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Sebastian Fricke <sebastian.fricke@...labora.com>
CC: <mchehab@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
        <conor+dt@...nel.org>, <hverkuil-cisco@...all.nl>,
        <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-rockchip@...ts.infradead.org>,
        <benjamin.gaignard@...labora.com>, <laurent.pinchart@...asonboard.com>,
        <praneeth@...com>, <nm@...com>, <vigneshr@...com>, <a-bhatia1@...com>,
        <j-luthra@...com>, <b-brnich@...com>, <detheridge@...com>,
        <p-mantena@...com>, <vijayp@...com>, <andrzej.p@...labora.com>,
        <nicolas@...fresne.ca>, <afd@...com>
Subject: Re: [PATCH v14 2/6] media: imagination: Add E5010 JPEG Encoder driver

Hi Sebastian,

On 21/06/24 18:56, Sebastian Fricke wrote:
> Hey Devarsh,
> 
> On 21.06.2024 18:14, Devarsh Thakkar wrote:
>> Hi Sebastian
>>
>> On 21/06/24 18:07, Sebastian Fricke wrote:
>>> Hey Devarsh,
>>>
>>> This doesn't compile without errors for me, curious, it probably did
>>> compile without problems for you right?
>>>
>>> drivers/media/platform/imagination/e5010-jpeg-enc.c:1622:19: error:
>>> initialization of ‘int (*)(struct platform_device *)’ from incompatible
>>> pointer type ‘void (*)(struct platform_device *)’
>>> [-Werror=incompatible-pointer-types]
>>>  1622 |         .remove = e5010_remove,
>>>       |                   ^~~~~~~~~~~~
>>> drivers/media/platform/imagination/e5010-jpeg-enc.c:1622:19: note: (near
>>> initialization for ‘e5010_driver.remove’)
>>> cc1: some warnings being treated as errors
>>>
>>
>> Yes I think it did compile fine for me.
>> Did you try this on tip of linux-next ?
> 
> Nope the media subsystem is not based on linux-next, but instead on:
> https://git.linuxtv.org/media_stage.git/log/
> Please make sure that your patches compile on top of that.
> 

Sure, will take care of this in future.

>>
>> As mentioned in changelog, there was update in platform driver for return type
>> of remove function which got changed to void return type. Please check if you
>> have this patch [1] in your tree which got recently merged.
> 
> No that patch is not part of the tree yet.
> 
> Please note also the following section in that patch:
> 

I think the said patch removes below comment.

>      /*
>      * Traditionally the remove callback returned an int which however is
>      * ignored by the driver core. This led to wrong expectations by driver
>      * authors who thought returning an error code was a valid error
>      * handling strategy. To convert to a callback returning void, new
>      * drivers should implement .remove_new() until the conversion it done
>      * that eventually makes .remove() return void.
> 
> So your driver is expected to implement remove_new().
> 

If I look at the tip of linux-next (which is rc4+) [1], the above comment is
now removed and it says that new drivers are supposed to implement .remove()
only. But yeah, as media_tree doesn't have this patch it may require to have
int remove() and void remove_new() as you mention, but eventually when
media_stage tree gets synced with above patch pulled in, we may require to
update again to what is present in this patch.

> I will take care of this for the PR, but please note for the future:
> It has to work with media_stage, unless you have a very good reason for
> something different.
> 

Sure, thanks for taking care of this. Feel free to let me know if any issues
faced.

Regards
Devarsh

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/include/linux/platform_device.h?h=next-20240620#n240