| Message ID | 20190310012219.21673-1-mateodemayo@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | media: staging: tegra-vde: Cleaned uapi.h from checkpatch warnings | expand |
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
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.
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
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?
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.
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 :)
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)
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(-)