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 <[email protected]>
---
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/[email protected]/
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
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 <[email protected]>
> ---
>
> 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/[email protected]/
>
> 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 <[email protected]>
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 <[email protected]>
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/[email protected]/
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/[email protected]/
>
> 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 <[email protected]> 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 <[email protected]>
>
> 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/[email protected]/
>
> 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/[email protected]/
> >
> > 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