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 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
Attachment:
signature.asc
Description: PGP signature
