| Message ID | 18daa194878d8815beef39d6fed2a838e1c2cb68.1637781097.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Solve the remaining issues with clang and W=1 on media | expand |
On 24/Nov/2021 20:13, Mauro Carvalho Chehab wrote: > There are several unused helper macros there, meant to parse some > fields. > > While there's not wrong with that, it generates clang warnings > with W=1, causing build to break with CONFIG_WERROR. > > So, add __maybe_unused to fix such warnings. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > > To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. > See [PATCH 00/20] at: https://lore.kernel.org/all/cover.1637781097.git.mchehab+huawei@kernel.org/ > > drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c > index 0abcad4e84fa..85eaf5d00e9b 100644 > --- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c > +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c > @@ -391,12 +391,12 @@ static int solo_send_desc(struct solo_enc_dev *solo_enc, int skip, > } > > /* Extract values from VOP header - VE_STATUSxx */ > -static inline int vop_interlaced(const vop_header *vh) > +static inline __maybe_unused int vop_interlaced(const vop_header *vh) > { > return (__le32_to_cpu((*vh)[0]) >> 30) & 1; > } > > -static inline u8 vop_channel(const vop_header *vh) > +static inline __maybe_unused u8 vop_channel(const vop_header *vh) > { > return (__le32_to_cpu((*vh)[0]) >> 24) & 0x1F; > } > @@ -411,12 +411,12 @@ static inline u32 vop_mpeg_size(const vop_header *vh) > return __le32_to_cpu((*vh)[0]) & 0xFFFFF; > } > > -static inline u8 vop_hsize(const vop_header *vh) > +static inline u8 __maybe_unused vop_hsize(const vop_header *vh) > { > return (__le32_to_cpu((*vh)[1]) >> 8) & 0xFF; > } > > -static inline u8 vop_vsize(const vop_header *vh) > +static inline u8 __maybe_unused vop_vsize(const vop_header *vh) > { > return __le32_to_cpu((*vh)[1]) & 0xFF; > } > @@ -436,12 +436,12 @@ static inline u32 vop_jpeg_size(const vop_header *vh) > return __le32_to_cpu((*vh)[4]) & 0xFFFFF; > } > > -static inline u32 vop_sec(const vop_header *vh) > +static inline u32 __maybe_unused vop_sec(const vop_header *vh) > { > return __le32_to_cpu((*vh)[5]); > } > > -static inline u32 vop_usec(const vop_header *vh) > +static inline __maybe_unused u32 vop_usec(const vop_header *vh) > { > return __le32_to_cpu((*vh)[6]); > } > -- > 2.33.1 > Acked-by: Ismael Luceno <ismael@iodev.co.uk>
On Wed, Nov 24, 2021 at 08:13:11PM +0100, Mauro Carvalho Chehab wrote: > There are several unused helper macros there, meant to parse some > fields. > > While there's not wrong with that, it generates clang warnings > with W=1, causing build to break with CONFIG_WERROR. > > So, add __maybe_unused to fix such warnings. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> I'll comment on this one patch but my opinion applies for all the patches adding '__maybe_unused' to truly unused functions. I agree with Laurent's comment [1]: unless this code is going to be used soon, it should be deleted and resurrected when it is actually needed. We have git for a reason and by adding this attribute, you are making it harder to catch and eliminate unused functions, as no compiler will catch them with an unused attribute (it is possible other static analysis tools will but I doubt those are run as frequently as compilers with W=1). However, you are the maintainer so if you really want to keep these around, I would recommend adding '__always_unused' instead of '__maybe_unused' for documentation and auditing purposes, even though they evaluate to the same thing: $ rg __always_unused | wc -l 337 $ rg __maybe_unused | wc -l 4335 [1]: https://lore.kernel.org/r/YZtpnjPcGxVwhe61@pendragon.ideasonboard.com/ Cheers, Nathan > --- > > To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. > See [PATCH 00/20] at: https://lore.kernel.org/all/cover.1637781097.git.mchehab+huawei@kernel.org/ > > drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c > index 0abcad4e84fa..85eaf5d00e9b 100644 > --- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c > +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c > @@ -391,12 +391,12 @@ static int solo_send_desc(struct solo_enc_dev *solo_enc, int skip, > } > > /* Extract values from VOP header - VE_STATUSxx */ > -static inline int vop_interlaced(const vop_header *vh) > +static inline __maybe_unused int vop_interlaced(const vop_header *vh) > { > return (__le32_to_cpu((*vh)[0]) >> 30) & 1; > } > > -static inline u8 vop_channel(const vop_header *vh) > +static inline __maybe_unused u8 vop_channel(const vop_header *vh) > { > return (__le32_to_cpu((*vh)[0]) >> 24) & 0x1F; > } > @@ -411,12 +411,12 @@ static inline u32 vop_mpeg_size(const vop_header *vh) > return __le32_to_cpu((*vh)[0]) & 0xFFFFF; > } > > -static inline u8 vop_hsize(const vop_header *vh) > +static inline u8 __maybe_unused vop_hsize(const vop_header *vh) > { > return (__le32_to_cpu((*vh)[1]) >> 8) & 0xFF; > } > > -static inline u8 vop_vsize(const vop_header *vh) > +static inline u8 __maybe_unused vop_vsize(const vop_header *vh) > { > return __le32_to_cpu((*vh)[1]) & 0xFF; > } > @@ -436,12 +436,12 @@ static inline u32 vop_jpeg_size(const vop_header *vh) > return __le32_to_cpu((*vh)[4]) & 0xFFFFF; > } > > -static inline u32 vop_sec(const vop_header *vh) > +static inline u32 __maybe_unused vop_sec(const vop_header *vh) > { > return __le32_to_cpu((*vh)[5]); > } > > -static inline u32 vop_usec(const vop_header *vh) > +static inline __maybe_unused u32 vop_usec(const vop_header *vh) > { > return __le32_to_cpu((*vh)[6]); > } > -- > 2.33.1 > >
Em Fri, 26 Nov 2021 12:58:33 -0700 Nathan Chancellor <nathan@kernel.org> escreveu: > On Wed, Nov 24, 2021 at 08:13:11PM +0100, Mauro Carvalho Chehab wrote: > > There are several unused helper macros there, meant to parse some > > fields. > > > > While there's not wrong with that, it generates clang warnings > > with W=1, causing build to break with CONFIG_WERROR. > > > > So, add __maybe_unused to fix such warnings. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > I'll comment on this one patch but my opinion applies for all the > patches adding '__maybe_unused' to truly unused functions. > > I agree with Laurent's comment [1]: unless this code is going to be used > soon, it should be deleted and resurrected when it is actually needed. > We have git for a reason and by adding this attribute, you are making it > harder to catch and eliminate unused functions, as no compiler will > catch them with an unused attribute (it is possible other static > analysis tools will but I doubt those are run as frequently as compilers > with W=1). In this specific case (and on a few other patches on this series), those macros are used to document a field. That's why I opted to keep it. > > However, you are the maintainer so if you really want to keep these > around, I would recommend adding '__always_unused' instead of > '__maybe_unused' for documentation and auditing purposes, even though > they evaluate to the same thing: > > $ rg __always_unused | wc -l > 337 > > $ rg __maybe_unused | wc -l > 4335 Good point. I'll use __always_unused on such patches. > > [1]: https://lore.kernel.org/r/YZtpnjPcGxVwhe61@pendragon.ideasonboard.com/ > > Cheers, > Nathan > > > --- > > > > To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. > > See [PATCH 00/20] at: https://lore.kernel.org/all/cover.1637781097.git.mchehab+huawei@kernel.org/ > > > > drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c > > index 0abcad4e84fa..85eaf5d00e9b 100644 > > --- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c > > +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c > > @@ -391,12 +391,12 @@ static int solo_send_desc(struct solo_enc_dev *solo_enc, int skip, > > } > > > > /* Extract values from VOP header - VE_STATUSxx */ > > -static inline int vop_interlaced(const vop_header *vh) > > +static inline __maybe_unused int vop_interlaced(const vop_header *vh) > > { > > return (__le32_to_cpu((*vh)[0]) >> 30) & 1; > > } > > > > -static inline u8 vop_channel(const vop_header *vh) > > +static inline __maybe_unused u8 vop_channel(const vop_header *vh) > > { > > return (__le32_to_cpu((*vh)[0]) >> 24) & 0x1F; > > } > > @@ -411,12 +411,12 @@ static inline u32 vop_mpeg_size(const vop_header *vh) > > return __le32_to_cpu((*vh)[0]) & 0xFFFFF; > > } > > > > -static inline u8 vop_hsize(const vop_header *vh) > > +static inline u8 __maybe_unused vop_hsize(const vop_header *vh) > > { > > return (__le32_to_cpu((*vh)[1]) >> 8) & 0xFF; > > } > > > > -static inline u8 vop_vsize(const vop_header *vh) > > +static inline u8 __maybe_unused vop_vsize(const vop_header *vh) > > { > > return __le32_to_cpu((*vh)[1]) & 0xFF; > > } > > @@ -436,12 +436,12 @@ static inline u32 vop_jpeg_size(const vop_header *vh) > > return __le32_to_cpu((*vh)[4]) & 0xFFFFF; > > } > > > > -static inline u32 vop_sec(const vop_header *vh) > > +static inline u32 __maybe_unused vop_sec(const vop_header *vh) > > { > > return __le32_to_cpu((*vh)[5]); > > } > > > > -static inline u32 vop_usec(const vop_header *vh) > > +static inline __maybe_unused u32 vop_usec(const vop_header *vh) > > { > > return __le32_to_cpu((*vh)[6]); > > } > > -- > > 2.33.1 > > > > Thanks, Mauro
diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c index 0abcad4e84fa..85eaf5d00e9b 100644 --- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c @@ -391,12 +391,12 @@ static int solo_send_desc(struct solo_enc_dev *solo_enc, int skip, } /* Extract values from VOP header - VE_STATUSxx */ -static inline int vop_interlaced(const vop_header *vh) +static inline __maybe_unused int vop_interlaced(const vop_header *vh) { return (__le32_to_cpu((*vh)[0]) >> 30) & 1; } -static inline u8 vop_channel(const vop_header *vh) +static inline __maybe_unused u8 vop_channel(const vop_header *vh) { return (__le32_to_cpu((*vh)[0]) >> 24) & 0x1F; } @@ -411,12 +411,12 @@ static inline u32 vop_mpeg_size(const vop_header *vh) return __le32_to_cpu((*vh)[0]) & 0xFFFFF; } -static inline u8 vop_hsize(const vop_header *vh) +static inline u8 __maybe_unused vop_hsize(const vop_header *vh) { return (__le32_to_cpu((*vh)[1]) >> 8) & 0xFF; } -static inline u8 vop_vsize(const vop_header *vh) +static inline u8 __maybe_unused vop_vsize(const vop_header *vh) { return __le32_to_cpu((*vh)[1]) & 0xFF; } @@ -436,12 +436,12 @@ static inline u32 vop_jpeg_size(const vop_header *vh) return __le32_to_cpu((*vh)[4]) & 0xFFFFF; } -static inline u32 vop_sec(const vop_header *vh) +static inline u32 __maybe_unused vop_sec(const vop_header *vh) { return __le32_to_cpu((*vh)[5]); } -static inline u32 vop_usec(const vop_header *vh) +static inline __maybe_unused u32 vop_usec(const vop_header *vh) { return __le32_to_cpu((*vh)[6]); }
There are several unused helper macros there, meant to parse some fields. While there's not wrong with that, it generates clang warnings with W=1, causing build to break with CONFIG_WERROR. So, add __maybe_unused to fix such warnings. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH 00/20] at: https://lore.kernel.org/all/cover.1637781097.git.mchehab+huawei@kernel.org/ drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)