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@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxx>
>>> *
>>> @@ -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?
