media: staging: tegra-vde: Cleaned uapi.h from checkpatch warnings
diff mbox series

Message ID 20190310012219.21673-1-mateodemayo@gmail.com
State Deferred
Headers show
Series
  • media: staging: tegra-vde: Cleaned uapi.h from checkpatch warnings
Related show

Commit Message

Mateo de Mayo March 10, 2019, 1:22 a.m. UTC
Added SPDX identifier and __packed structs

Signed-off-by: Mateo de Mayo <mateodemayo@gmail.com>
---
 drivers/staging/media/tegra-vde/uapi.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Dmitry Osipenko March 10, 2019, 3:31 p.m. UTC | #1
10.03.2019 4:22, Mateo de Mayo пишет:
> Added SPDX identifier and __packed structs
> 
> Signed-off-by: Mateo de Mayo <mateodemayo@gmail.com>
> ---
>  drivers/staging/media/tegra-vde/uapi.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/tegra-vde/uapi.h b/drivers/staging/media/tegra-vde/uapi.h
> index 4bce08a7a54c..d32fc97f54b1 100644
> --- a/drivers/staging/media/tegra-vde/uapi.h
> +++ b/drivers/staging/media/tegra-vde/uapi.h
> @@ -1,3 +1,4 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
>  /*
>   * Copyright (C) 2016-2017 Dmitry Osipenko <digetx@gmail.com>
>   *
> @@ -29,7 +30,7 @@ struct tegra_vde_h264_frame {
>  	__u32 flags;
>  
>  	__u32 reserved;
> -} __attribute__((packed));
> +} __packed;
>  
>  struct tegra_vde_h264_decoder_ctx {
>  	__s32 bitstream_data_fd;
> @@ -61,7 +62,7 @@ struct tegra_vde_h264_decoder_ctx {
>  	__u8  num_ref_idx_l1_active_minus1;
>  
>  	__u32 reserved;
> -} __attribute__((packed));
> +} __packed;
>  
>  #define VDE_IOCTL_BASE			('v' + 0x20)
>  
> 

Hello Mateo,

The license part is good to me. The _packed part not, there was another attempt to use the macro before [https://lkml.org/lkml/2018/11/7/603] and I suggested that it is not a very good idea since that macro isn't a part of UAPI headers. I guess it will be better to just add a definition for the __packed into this header instead of NAK'ing such attempts in the future.

Please add these lines to the patch:

#ifndef __packed
#define __packed		__attribute__((packed))
#endif

I also just noticed that the BIT() macro got into the UAPI header behind my back.. so will be nice if you'll add a macro for that case too, thanks:

#ifndef BIT(nr)
#define BIT(nr)			(1UL << (nr))
#endif
Dmitry Osipenko March 10, 2019, 3:42 p.m. UTC | #2
10.03.2019 18:31, Dmitry Osipenko пишет:
> 10.03.2019 4:22, Mateo de Mayo пишет:
>> Added SPDX identifier and __packed structs
>>
>> Signed-off-by: Mateo de Mayo <mateodemayo@gmail.com>
>> ---
>>  drivers/staging/media/tegra-vde/uapi.h | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/tegra-vde/uapi.h b/drivers/staging/media/tegra-vde/uapi.h
>> index 4bce08a7a54c..d32fc97f54b1 100644
>> --- a/drivers/staging/media/tegra-vde/uapi.h
>> +++ b/drivers/staging/media/tegra-vde/uapi.h
>> @@ -1,3 +1,4 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>>  /*
>>   * Copyright (C) 2016-2017 Dmitry Osipenko <digetx@gmail.com>
>>   *
>> @@ -29,7 +30,7 @@ struct tegra_vde_h264_frame {
>>  	__u32 flags;
>>  
>>  	__u32 reserved;
>> -} __attribute__((packed));
>> +} __packed;
>>  
>>  struct tegra_vde_h264_decoder_ctx {
>>  	__s32 bitstream_data_fd;
>> @@ -61,7 +62,7 @@ struct tegra_vde_h264_decoder_ctx {
>>  	__u8  num_ref_idx_l1_active_minus1;
>>  
>>  	__u32 reserved;
>> -} __attribute__((packed));
>> +} __packed;
>>  
>>  #define VDE_IOCTL_BASE			('v' + 0x20)
>>  
>>
> 
> Hello Mateo,
> 
> The license part is good to me. The _packed part not, there was another attempt to use the macro before [https://lkml.org/lkml/2018/11/7/603] and I suggested that it is not a very good idea since that macro isn't a part of UAPI headers. I guess it will be better to just add a definition for the __packed into this header instead of NAK'ing such attempts in the future.
> 
> Please add these lines to the patch:
> 
> #ifndef __packed
> #define __packed		__attribute__((packed))
> #endif
> 
> I also just noticed that the BIT() macro got into the UAPI header behind my back.. so will be nice if you'll add a macro for that case too, thanks:
> 
> #ifndef BIT(nr)
> #define BIT(nr)			(1UL << (nr))
> #endif
> 

Although, the license part isn't not good to me too, please remove the license text below the "Copyright" comment since it will duplicate the SPDX tag, thanks.
Thierry Reding March 11, 2019, 10:22 a.m. UTC | #3
On Sun, Mar 10, 2019 at 06:31:31PM +0300, Dmitry Osipenko wrote:
> 10.03.2019 4:22, Mateo de Mayo пишет:
> > Added SPDX identifier and __packed structs
> > 
> > Signed-off-by: Mateo de Mayo <mateodemayo@gmail.com>
> > ---
> >  drivers/staging/media/tegra-vde/uapi.h | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/tegra-vde/uapi.h b/drivers/staging/media/tegra-vde/uapi.h
> > index 4bce08a7a54c..d32fc97f54b1 100644
> > --- a/drivers/staging/media/tegra-vde/uapi.h
> > +++ b/drivers/staging/media/tegra-vde/uapi.h
> > @@ -1,3 +1,4 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> >  /*
> >   * Copyright (C) 2016-2017 Dmitry Osipenko <digetx@gmail.com>
> >   *
> > @@ -29,7 +30,7 @@ struct tegra_vde_h264_frame {
> >  	__u32 flags;
> >  
> >  	__u32 reserved;
> > -} __attribute__((packed));
> > +} __packed;
> >  
> >  struct tegra_vde_h264_decoder_ctx {
> >  	__s32 bitstream_data_fd;
> > @@ -61,7 +62,7 @@ struct tegra_vde_h264_decoder_ctx {
> >  	__u8  num_ref_idx_l1_active_minus1;
> >  
> >  	__u32 reserved;
> > -} __attribute__((packed));
> > +} __packed;
> >  
> >  #define VDE_IOCTL_BASE			('v' + 0x20)
> >  
> > 
> 
> Hello Mateo,
> 
> The license part is good to me. The _packed part not, there was
> another attempt to use the macro before
> [https://lkml.org/lkml/2018/11/7/603] and I suggested that it is not a
> very good idea since that macro isn't a part of UAPI headers. I guess
> it will be better to just add a definition for the __packed into this
> header instead of NAK'ing such attempts in the future.
> 
> Please add these lines to the patch:
> 
> #ifndef __packed
> #define __packed		__attribute__((packed))
> #endif

Couldn't we just manually pack the structure so that we don't have to
force the compiler to pack them? Other UAPI headers do that and given
this is still part of staging, might be a good cleanup?

> 
> I also just noticed that the BIT() macro got into the UAPI header
> behind my back.. so will be nice if you'll add a macro for that case
> too, thanks:
> 
> #ifndef BIT(nr)
> #define BIT(nr)			(1UL << (nr))
> #endif

Perhaps it'd de better to just revert the patch that added usage of the
BIT macros? Or perhaps make sure that the BIT macro is exported in one
of the other UAPI headers that we pull in? The latter might not be such
a good idea given that it might conflict with userspace programs or
libraries defining their own version of that macro.

Might be a good idea to also modify checkpatch to not warn about this
kind of use in UAPI headers, though, admittedly, it might be a bit
difficult to come up with a good heuristic to do that, especially for
things in drivers/staging.

Thierry
Dmitry Osipenko March 11, 2019, 3:29 p.m. UTC | #4
11.03.2019 13:22, Thierry Reding пишет:
> On Sun, Mar 10, 2019 at 06:31:31PM +0300, Dmitry Osipenko wrote:
>> 10.03.2019 4:22, Mateo de Mayo пишет:
>>> Added SPDX identifier and __packed structs
>>>
>>> Signed-off-by: Mateo de Mayo <mateodemayo@gmail.com>
>>> ---
>>>  drivers/staging/media/tegra-vde/uapi.h | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/tegra-vde/uapi.h b/drivers/staging/media/tegra-vde/uapi.h
>>> index 4bce08a7a54c..d32fc97f54b1 100644
>>> --- a/drivers/staging/media/tegra-vde/uapi.h
>>> +++ b/drivers/staging/media/tegra-vde/uapi.h
>>> @@ -1,3 +1,4 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>  /*
>>>   * Copyright (C) 2016-2017 Dmitry Osipenko <digetx@gmail.com>
>>>   *
>>> @@ -29,7 +30,7 @@ struct tegra_vde_h264_frame {
>>>  	__u32 flags;
>>>  
>>>  	__u32 reserved;
>>> -} __attribute__((packed));
>>> +} __packed;
>>>  
>>>  struct tegra_vde_h264_decoder_ctx {
>>>  	__s32 bitstream_data_fd;
>>> @@ -61,7 +62,7 @@ struct tegra_vde_h264_decoder_ctx {
>>>  	__u8  num_ref_idx_l1_active_minus1;
>>>  
>>>  	__u32 reserved;
>>> -} __attribute__((packed));
>>> +} __packed;
>>>  
>>>  #define VDE_IOCTL_BASE			('v' + 0x20)
>>>  
>>>
>>
>> Hello Mateo,
>>
>> The license part is good to me. The _packed part not, there was
>> another attempt to use the macro before
>> [https://lkml.org/lkml/2018/11/7/603] and I suggested that it is not a
>> very good idea since that macro isn't a part of UAPI headers. I guess
>> it will be better to just add a definition for the __packed into this
>> header instead of NAK'ing such attempts in the future.
>>
>> Please add these lines to the patch:
>>
>> #ifndef __packed
>> #define __packed		__attribute__((packed))
>> #endif
> 
> Couldn't we just manually pack the structure so that we don't have to
> force the compiler to pack them? Other UAPI headers do that and given
> this is still part of staging, might be a good cleanup?

I don't mind since libvdpau-tegra is the only user of this driver (AFAIK). In userspace we could just issue an update that will make VDPAU to work with both old and new IOCTL's, not a problem. Do you want to make a patch?

>> I also just noticed that the BIT() macro got into the UAPI header
>> behind my back.. so will be nice if you'll add a macro for that case
>> too, thanks:
>>
>> #ifndef BIT(nr)
>> #define BIT(nr)			(1UL << (nr))
>> #endif
> 
> Perhaps it'd de better to just revert the patch that added usage of the
> BIT macros? Or perhaps make sure that the BIT macro is exported in one
> of the other UAPI headers that we pull in? The latter might not be such
> a good idea given that it might conflict with userspace programs or
> libraries defining their own version of that macro.

I'm pretty sure that somebody will try to bring it back after the reverting. Exporting BIT macro in the linux-headers probably indeed could break some userspace, so doesn't sound very attractive. Seems there is no ideal simple solution. In the end either way will be good enough since we're in control of the userspace library.

It looks to me that this all is up to individual drivers to decide, personally I'd prefer to get a warning/error from a compiler than spending 20+ minutes trying to figure out why things don't work (that's regarding the __packed macro). I'd also prefer to have the #ifndef for the BIT() macro, just my personal preference.

> Might be a good idea to also modify checkpatch to not warn about this
> kind of use in UAPI headers, though, admittedly, it might be a bit
> difficult to come up with a good heuristic to do that, especially for
> things in drivers/staging.

Mateo, do you want to try to implement this suggestion?
Dmitry Osipenko March 11, 2019, 10:10 p.m. UTC | #5
11.03.2019 18:29, Dmitry Osipenko пишет:
> 11.03.2019 13:22, Thierry Reding пишет:
>> On Sun, Mar 10, 2019 at 06:31:31PM +0300, Dmitry Osipenko wrote:
>>> 10.03.2019 4:22, Mateo de Mayo пишет:
>>>> Added SPDX identifier and __packed structs
>>>>
>>>> Signed-off-by: Mateo de Mayo <mateodemayo@gmail.com>
>>>> ---
>>>>  drivers/staging/media/tegra-vde/uapi.h | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/tegra-vde/uapi.h b/drivers/staging/media/tegra-vde/uapi.h
>>>> index 4bce08a7a54c..d32fc97f54b1 100644
>>>> --- a/drivers/staging/media/tegra-vde/uapi.h
>>>> +++ b/drivers/staging/media/tegra-vde/uapi.h
>>>> @@ -1,3 +1,4 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>  /*
>>>>   * Copyright (C) 2016-2017 Dmitry Osipenko <digetx@gmail.com>
>>>>   *
>>>> @@ -29,7 +30,7 @@ struct tegra_vde_h264_frame {
>>>>  	__u32 flags;
>>>>  
>>>>  	__u32 reserved;
>>>> -} __attribute__((packed));
>>>> +} __packed;
>>>>  
>>>>  struct tegra_vde_h264_decoder_ctx {
>>>>  	__s32 bitstream_data_fd;
>>>> @@ -61,7 +62,7 @@ struct tegra_vde_h264_decoder_ctx {
>>>>  	__u8  num_ref_idx_l1_active_minus1;
>>>>  
>>>>  	__u32 reserved;
>>>> -} __attribute__((packed));
>>>> +} __packed;
>>>>  
>>>>  #define VDE_IOCTL_BASE			('v' + 0x20)
>>>>  
>>>>
>>>
>>> Hello Mateo,
>>>
>>> The license part is good to me. The _packed part not, there was
>>> another attempt to use the macro before
>>> [https://lkml.org/lkml/2018/11/7/603] and I suggested that it is not a
>>> very good idea since that macro isn't a part of UAPI headers. I guess
>>> it will be better to just add a definition for the __packed into this
>>> header instead of NAK'ing such attempts in the future.
>>>
>>> Please add these lines to the patch:
>>>
>>> #ifndef __packed
>>> #define __packed		__attribute__((packed))
>>> #endif
>>
>> Couldn't we just manually pack the structure so that we don't have to
>> force the compiler to pack them? Other UAPI headers do that and given
>> this is still part of staging, might be a good cleanup?
> 
> I don't mind since libvdpau-tegra is the only user of this driver (AFAIK). In userspace we could just issue an update that will make VDPAU to work with both old and new IOCTL's, not a problem. Do you want to make a patch?
> 
>>> I also just noticed that the BIT() macro got into the UAPI header
>>> behind my back.. so will be nice if you'll add a macro for that case
>>> too, thanks:
>>>
>>> #ifndef BIT(nr)
>>> #define BIT(nr)			(1UL << (nr))
>>> #endif
>>
>> Perhaps it'd de better to just revert the patch that added usage of the
>> BIT macros? Or perhaps make sure that the BIT macro is exported in one
>> of the other UAPI headers that we pull in? The latter might not be such
>> a good idea given that it might conflict with userspace programs or
>> libraries defining their own version of that macro.
> 
> I'm pretty sure that somebody will try to bring it back after the reverting. Exporting BIT macro in the linux-headers probably indeed could break some userspace, so doesn't sound very attractive. Seems there is no ideal simple solution. In the end either way will be good enough since we're in control of the userspace library.
> 
> It looks to me that this all is up to individual drivers to decide, personally I'd prefer to get a warning/error from a compiler than spending 20+ minutes trying to figure out why things don't work (that's regarding the __packed macro). I'd also prefer to have the #ifndef for the BIT() macro, just my personal preference.
> 
>> Might be a good idea to also modify checkpatch to not warn about this
>> kind of use in UAPI headers, though, admittedly, it might be a bit
>> difficult to come up with a good heuristic to do that, especially for
>> things in drivers/staging.
> 
> Mateo, do you want to try to implement this suggestion?
> 

Another variant could be to get rid of the BIT() macro and just use hex values instead (0x1 0x2 0x4..), actually that's probably the best variant.
Mateo de Mayo March 11, 2019, 10:16 p.m. UTC | #6
From v2 subthread:

> You need to leave in place the "Copyright" line because it gives
> information about who owns the copyright on the file.

Sorry about that!

> Also, please send a new version of the patch as a standalone email, don't make it as a reply to the older version.

V3 corrected and sent in another thread.

On Mon, Mar 11, 2019 at 06:29:04PM +0300, Dmitry Osipenko wrote:
> 11.03.2019 13:22, Thierry Reding пишет:
> > On Sun, Mar 10, 2019 at 06:31:31PM +0300, Dmitry Osipenko wrote:
> >> 10.03.2019 4:22, Mateo de Mayo пишет:
> >>> Added SPDX identifier and __packed structs
> >>>
> >>> Signed-off-by: Mateo de Mayo <mateodemayo@gmail.com>
> >>> ---
> >>>  drivers/staging/media/tegra-vde/uapi.h | 5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/media/tegra-vde/uapi.h b/drivers/staging/media/tegra-vde/uapi.h
> >>> index 4bce08a7a54c..d32fc97f54b1 100644
> >>> --- a/drivers/staging/media/tegra-vde/uapi.h
> >>> +++ b/drivers/staging/media/tegra-vde/uapi.h
> >>> @@ -1,3 +1,4 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>>  /*
> >>>   * Copyright (C) 2016-2017 Dmitry Osipenko <digetx@gmail.com>
> >>>   *
> >>> @@ -29,7 +30,7 @@ struct tegra_vde_h264_frame {
> >>>  	__u32 flags;
> >>>
> >>>  	__u32 reserved;
> >>> -} __attribute__((packed));
> >>> +} __packed;
> >>>
> >>>  struct tegra_vde_h264_decoder_ctx {
> >>>  	__s32 bitstream_data_fd;
> >>> @@ -61,7 +62,7 @@ struct tegra_vde_h264_decoder_ctx {
> >>>  	__u8  num_ref_idx_l1_active_minus1;
> >>>
> >>>  	__u32 reserved;
> >>> -} __attribute__((packed));
> >>> +} __packed;
> >>>
> >>>  #define VDE_IOCTL_BASE			('v' + 0x20)
> >>>
> >>>
> >>
> >> Hello Mateo,
> >>
> >> The license part is good to me. The _packed part not, there was
> >> another attempt to use the macro before
> >> [https://lkml.org/lkml/2018/11/7/603] and I suggested that it is not a
> >> very good idea since that macro isn't a part of UAPI headers. I guess
> >> it will be better to just add a definition for the __packed into this
> >> header instead of NAK'ing such attempts in the future.
> >>
> >> Please add these lines to the patch:
> >>
> >> #ifndef __packed
> >> #define __packed		__attribute__((packed))
> >> #endif
> >
> > Couldn't we just manually pack the structure so that we don't have to
> > force the compiler to pack them? Other UAPI headers do that and given
> > this is still part of staging, might be a good cleanup?
>
> I don't mind since libvdpau-tegra is the only user of this driver (AFAIK). In userspace we could just issue an update that will make VDPAU to work with both old and new IOCTL's, not a problem. Do you want to make a patch?
>
> >> I also just noticed that the BIT() macro got into the UAPI header
> >> behind my back.. so will be nice if you'll add a macro for that case
> >> too, thanks:
> >>
> >> #ifndef BIT(nr)
> >> #define BIT(nr)			(1UL << (nr))
> >> #endif
> >
> > Perhaps it'd de better to just revert the patch that added usage of the
> > BIT macros? Or perhaps make sure that the BIT macro is exported in one
> > of the other UAPI headers that we pull in? The latter might not be such
> > a good idea given that it might conflict with userspace programs or
> > libraries defining their own version of that macro.
>
> I'm pretty sure that somebody will try to bring it back after the reverting. Exporting BIT macro in the linux-headers probably indeed could break some userspace, so doesn't sound very attractive. Seems there is no ideal simple solution. In the end either way will be good enough since we're in control of the userspace library.
>
> It looks to me that this all is up to individual drivers to decide, personally I'd prefer to get a warning/error from a compiler than spending 20+ minutes trying to figure out why things don't work (that's regarding the __packed macro). I'd also prefer to have the #ifndef for the BIT() macro, just my personal preference.
>
> > Might be a good idea to also modify checkpatch to not warn about this
> > kind of use in UAPI headers, though, admittedly, it might be a bit
> > difficult to come up with a good heuristic to do that, especially for
> > things in drivers/staging.
>
> Mateo, do you want to try to implement this suggestion?

Sure! I'll see what I can do.

I'm not in a position to discuss about the other stuff though :)

Patch
diff mbox series

diff --git a/drivers/staging/media/tegra-vde/uapi.h b/drivers/staging/media/tegra-vde/uapi.h
index 4bce08a7a54c..d32fc97f54b1 100644
--- a/drivers/staging/media/tegra-vde/uapi.h
+++ b/drivers/staging/media/tegra-vde/uapi.h
@@ -1,3 +1,4 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Copyright (C) 2016-2017 Dmitry Osipenko <digetx@gmail.com>
  *
@@ -29,7 +30,7 @@  struct tegra_vde_h264_frame {
 	__u32 flags;
 
 	__u32 reserved;
-} __attribute__((packed));
+} __packed;
 
 struct tegra_vde_h264_decoder_ctx {
 	__s32 bitstream_data_fd;
@@ -61,7 +62,7 @@  struct tegra_vde_h264_decoder_ctx {
 	__u8  num_ref_idx_l1_active_minus1;
 
 	__u32 reserved;
-} __attribute__((packed));
+} __packed;
 
 #define VDE_IOCTL_BASE			('v' + 0x20)