v4l: Avoid unaligned access warnings when printing 4cc modifiers

PATCH In review Sakari Ailus 3 participants 11 messages Jan 10, 2022 – Jan 11, 2022 Media (V4L/DVB)

Share & tools

Sakari Ailus

author

2022-01-10 22:48 UTC #0 lore raw

PATCH 1/1 v4l: Avoid unaligned access warnings when printing 4cc modifiers

Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions. This could result in an unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.

Address the warning by copying the pixelformat or dataformat value to a local variable first.

Reported-by kernel test robot <[email protected]>

Fixes e927e1e0f0dd ("v4l: ioctl: Use %p4cc printk modifier to print FourCC codes")

Signed-off-by Sakari Ailus <[email protected]>

---
Hi Andy, Nick,

How about this one?

I believe it does address the clang warning although I haven't tested it.

Kind regards,
Sakari

 drivers/media/v4l2-core/v4l2-ioctl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/media/v4l2-core/v4l2-ioctl.c +7 -5

index 9ac557b8e146..642cb90f457c 100644
@@ -279,8 +279,8 @@ static void v4l_print_format(const void *arg, bool write_only)
 	const struct v4l2_vbi_format *vbi;
 	const struct v4l2_sliced_vbi_format *sliced;
 	const struct v4l2_window *win;
-	const struct v4l2_sdr_format *sdr;
 	const struct v4l2_meta_format *meta;
+	u32 pixelformat;
 	u32 planes;
 	unsigned i;

@@ -299,8 +299,9 @@ static void v4l_print_format(const void *arg, bool write_only)
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
 		mp = &p->fmt.pix_mp;
+		pixelformat = mp->pixelformat;
 		pr_cont(", width=%u, height=%u, format=%p4cc, field=%s, colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u, xfer_func=%u\n",
-			mp->width, mp->height, &mp->pixelformat,
+			mp->width, mp->height, &pixelformat,
 			prt_names(mp->field, v4l2_field_names),
 			mp->colorspace, mp->num_planes, mp->flags,
 			mp->ycbcr_enc, mp->quantization, mp->xfer_func);

@@ -343,14 +344,15 @@ static void v4l_print_format(const void *arg, bool write_only)
 		break;
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
 	case V4L2_BUF_TYPE_SDR_OUTPUT:
-		sdr = &p->fmt.sdr;
-		pr_cont(", pixelformat=%p4cc\n", &sdr->pixelformat);
+		pixelformat = p->fmt.sdr.pixelformat;
+		pr_cont(", pixelformat=%p4cc\n", &pixelformat);
 		break;
 	case V4L2_BUF_TYPE_META_CAPTURE:
 	case V4L2_BUF_TYPE_META_OUTPUT:
 		meta = &p->fmt.meta;
+		pixelformat = meta->dataformat;
 		pr_cont(", dataformat=%p4cc, buffersize=%u\n",
-			&meta->dataformat, meta->buffersize);
+			&pixelformat, meta->buffersize);
 		break;
 	}
 }

Signature · 1 line

2.30.2

Nick Desaulniers

Reviewed-by on v1 1/1

2022-01-10 23:11 UTC #1 lore raw

Quoted 3 lines · Sakari Ailus

Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions.

I would rephrase the below statement...

Quoted 3 lines

This could result in an
unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.

...to:

warning: taking address of packed member 'pixelformat' of class or structure 'v4l2_pix_format_mplane' may result in an unaligned pointer value [-Waddress-of-packed-member]

The warning is correct; because struct v4l2_pix_format_mplane is __packed, it's members also have __aligned(1). Taking the address of such members results in the use of underaligned pointers which is UB and may be caught by UBSAN or fault on architectures without unaligned loads should the struct instance happen to be allocated without any natural alignment.

Quoted 7 lines

Address the warning by copying the pixelformat or dataformat value to a local variable first.
Reported-by kernel test robot <[email protected]>
Fixes e927e1e0f0dd ("v4l: ioctl: Use %p4cc printk modifier to print FourCC codes")
Signed-off-by Sakari Ailus <[email protected]>

Quoted 6 lines

---
Hi Andy, Nick,
How about this one?
I believe it does address the clang warning although I haven't tested it.

LGTM. Thanks Sakari and Andy for pursuing this. Just a minor nit on my side about the framing of this warning being a false positive; I don't think it is. With that amended,

Reviewed-by Nick Desaulniers <[email protected]>

Quoted 52 lines

Kind regards,
Sakari

 drivers/media/v4l2-core/v4l2-ioctl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/media/v4l2-core/v4l2-ioctl.c +7 -6

index 9ac557b8e146..642cb90f457c 100644
@@ -279,8 +279,8 @@ static void v4l_print_format(const void *arg, bool write_only)
        const struct v4l2_vbi_format *vbi;
        const struct v4l2_sliced_vbi_format *sliced;
        const struct v4l2_window *win;
-       const struct v4l2_sdr_format *sdr;
        const struct v4l2_meta_format *meta;
+       u32 pixelformat;
        u32 planes;
        unsigned i;

@@ -299,8 +299,9 @@ static void v4l_print_format(const void *arg, bool write_only)
        case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
        case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
                mp = &p->fmt.pix_mp;
+               pixelformat = mp->pixelformat;
                pr_cont(", width=%u, height=%u, format=%p4cc, field=%s, colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u, xfer_func=%u\n",
-                       mp->width, mp->height, &mp->pixelformat,
+                       mp->width, mp->height, &pixelformat,
                        prt_names(mp->field, v4l2_field_names),
                        mp->colorspace, mp->num_planes, mp->flags,
                        mp->ycbcr_enc, mp->quantization, mp->xfer_func);

@@ -343,14 +344,15 @@ static void v4l_print_format(const void *arg, bool write_only)
                break;
        case V4L2_BUF_TYPE_SDR_CAPTURE:
        case V4L2_BUF_TYPE_SDR_OUTPUT:
-               sdr = &p->fmt.sdr;
-               pr_cont(", pixelformat=%p4cc\n", &sdr->pixelformat);
+               pixelformat = p->fmt.sdr.pixelformat;
+               pr_cont(", pixelformat=%p4cc\n", &pixelformat);
                break;
        case V4L2_BUF_TYPE_META_CAPTURE:
        case V4L2_BUF_TYPE_META_OUTPUT:
                meta = &p->fmt.meta;
+               pixelformat = meta->dataformat;
                pr_cont(", dataformat=%p4cc, buffersize=%u\n",
-                       &meta->dataformat, meta->buffersize);
+                       &pixelformat, meta->buffersize);
                break;
        }
 }
--

Quoted 2 lines

2.30.2

--
Thanks,
~Nick Desaulniers

Nick Desaulniers

2022-01-10 23:12 UTC #2 lore raw

PATCH 1/1 v4l: Avoid unaligned access warnings when printing 4cc modifiers

---------- Forwarded message ---------
From: Nick Desaulniers <[email protected]>
Date: Mon, Jan 10, 2022 at 3:11 PM
Subject: Re: [PATCH 1/1] v4l: Avoid unaligned access warnings when printing 4cc modifiers
To: Sakari Ailus <[email protected]>
Cc: <[email protected]>, Andy Shevchenko <[email protected]>, <[email protected]>

Quoted 3 lines · Sakari Ailus

Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions.

I would rephrase the below statement...

Quoted 3 lines

This could result in an
unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.

...to:

warning: taking address of packed member 'pixelformat' of class or structure 'v4l2_pix_format_mplane' may result in an unaligned pointer value [-Waddress-of-packed-member]

Quoted 7 lines

Address the warning by copying the pixelformat or dataformat value to a local variable first.
Reported-by kernel test robot <[email protected]>
Fixes e927e1e0f0dd ("v4l: ioctl: Use %p4cc printk modifier to print FourCC codes")
Signed-off-by Sakari Ailus <[email protected]>

Quoted 6 lines

---
Hi Andy, Nick,
How about this one?
I believe it does address the clang warning although I haven't tested it.

LGTM. Thanks Sakari and Andy for pursuing this. Just a minor nit on my side about the framing of this warning being a false positive; I don't think it is. With that amended,

Reviewed-by Nick Desaulniers <[email protected]>

Quoted 52 lines

Kind regards,
Sakari

 drivers/media/v4l2-core/v4l2-ioctl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/media/v4l2-core/v4l2-ioctl.c +7 -6

index 9ac557b8e146..642cb90f457c 100644
@@ -279,8 +279,8 @@ static void v4l_print_format(const void *arg, bool write_only)
        const struct v4l2_vbi_format *vbi;
        const struct v4l2_sliced_vbi_format *sliced;
        const struct v4l2_window *win;
-       const struct v4l2_sdr_format *sdr;
        const struct v4l2_meta_format *meta;
+       u32 pixelformat;
        u32 planes;
        unsigned i;

@@ -299,8 +299,9 @@ static void v4l_print_format(const void *arg, bool write_only)
        case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
        case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
                mp = &p->fmt.pix_mp;
+               pixelformat = mp->pixelformat;
                pr_cont(", width=%u, height=%u, format=%p4cc, field=%s, colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u, xfer_func=%u\n",
-                       mp->width, mp->height, &mp->pixelformat,
+                       mp->width, mp->height, &pixelformat,
                        prt_names(mp->field, v4l2_field_names),
                        mp->colorspace, mp->num_planes, mp->flags,
                        mp->ycbcr_enc, mp->quantization, mp->xfer_func);

@@ -343,14 +344,15 @@ static void v4l_print_format(const void *arg, bool write_only)
                break;
        case V4L2_BUF_TYPE_SDR_CAPTURE:
        case V4L2_BUF_TYPE_SDR_OUTPUT:
-               sdr = &p->fmt.sdr;
-               pr_cont(", pixelformat=%p4cc\n", &sdr->pixelformat);
+               pixelformat = p->fmt.sdr.pixelformat;
+               pr_cont(", pixelformat=%p4cc\n", &pixelformat);
                break;
        case V4L2_BUF_TYPE_META_CAPTURE:
        case V4L2_BUF_TYPE_META_OUTPUT:
                meta = &p->fmt.meta;
+               pixelformat = meta->dataformat;
                pr_cont(", dataformat=%p4cc, buffersize=%u\n",
-                       &meta->dataformat, meta->buffersize);
+                       &pixelformat, meta->buffersize);
                break;
        }
 }
--

Quoted 2 lines

2.30.2

--
Thanks,
~Nick Desaulniers

Signature · 2 lines

Thanks,
~Nick Desaulniers

Andy Shevchenko

2022-01-11 10:09 UTC #3 lore raw

In reply to Nick Desaulniers

Quoted 12 lines · Nick Desaulniers

Quoted 3 lines · Sakari Ailus
Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions.
I would rephrase the below statement...
Quoted 3 lines
This could result in an
unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.

Quoted 7 lines

...to:
warning: taking address of packed member 'pixelformat' of class or structure 'v4l2_pix_format_mplane' may result in an unaligned pointer value [-Waddress-of-packed-member]
The warning is correct;

Exactly. I'm going to repeat that in the comment to my patch.

Quoted 6 lines

because struct v4l2_pix_format_mplane is
__packed, it's members also have __aligned(1). Taking the address of such members results in the use of underaligned pointers which is UB and may be caught by UBSAN or fault on architectures without unaligned loads should the struct instance happen to be allocated without any natural alignment.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Sakari Ailus

author

2022-01-11 10:47 UTC #4 lore raw

In reply to Nick Desaulniers

Hi Nick,

Quoted 18 lines · Nick Desaulniers

Quoted 3 lines · Sakari Ailus
Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions.
I would rephrase the below statement...
Quoted 3 lines
This could result in an
unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.
...to:
warning: taking address of packed member 'pixelformat' of class or structure 'v4l2_pix_format_mplane' may result in an unaligned pointer value [-Waddress-of-packed-member]

Quoted 6 lines

The warning is correct; because struct v4l2_pix_format_mplane is __packed, it's members also have __aligned(1). Taking the address of such members results in the use of underaligned pointers which is UB and may be caught by UBSAN or fault on architectures without unaligned loads should the struct instance happen to be allocated without any natural alignment.

Wouldn't that be the case only if the __packed attribute resulted in a different memory layout than not having that attribute?

All these fields are aligned by 4 so I don't see how this could be an actual problem.

Quoted 13 lines

Quoted 13 lines
Address the warning by copying the pixelformat or dataformat value to a local variable first.
Reported-by kernel test robot <[email protected]>
Fixes e927e1e0f0dd ("v4l: ioctl: Use %p4cc printk modifier to print FourCC codes")
Signed-off-by Sakari Ailus <[email protected]>
---
Hi Andy, Nick,
How about this one?

Quoted 7 lines

Quoted 1 line
I believe it does address the clang warning although I haven't tested it.
LGTM. Thanks Sakari and Andy for pursuing this. Just a minor nit on my side about the framing of this warning being a false positive; I don't think it is. With that amended,
Reviewed-by Nick Desaulniers <[email protected]>

Thanks!

Signature · 3 lines

Kind regards,

Sakari Ailus

Andy Shevchenko

2022-01-11 12:28 UTC #5 lore raw

Quoted 28 lines · Sakari Ailus

Hi Nick,
Quoted 18 lines · Nick Desaulniers
Quoted 3 lines · Sakari Ailus
Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions.
I would rephrase the below statement...
Quoted 3 lines
This could result in an
unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.
...to:
warning: taking address of packed member 'pixelformat' of class or structure 'v4l2_pix_format_mplane' may result in an unaligned pointer value [-Waddress-of-packed-member]
Quoted 6 lines
The warning is correct; because struct v4l2_pix_format_mplane is __packed, it's members also have __aligned(1). Taking the address of such members results in the use of underaligned pointers which is UB and may be caught by UBSAN or fault on architectures without unaligned loads should the struct instance happen to be allocated without any natural alignment.

Quoted 5 lines

Wouldn't that be the case only if the __packed attribute resulted in a different memory layout than not having that attribute?
All these fields are aligned by 4 so I don't see how this could be an actual problem.

packed means two things and developers often forgot about the second one:
- the gaps between members in the data structures are removed
- the instance of the data object may be on unaligned address

Here is the second one which results in the warning.

That's why my patch against vsprintf as I explained in that thread.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Nick Desaulniers

2022-01-11 20:49 UTC #6 lore raw

Quoted 33 lines · Andy Shevchenko

Quoted 31 lines · Sakari Ailus
Hi Nick,
Quoted 18 lines · Nick Desaulniers
Quoted 3 lines · Sakari Ailus
Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions.
I would rephrase the below statement...
Quoted 3 lines
This could result in an
unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.
...to:
warning: taking address of packed member 'pixelformat' of class or structure 'v4l2_pix_format_mplane' may result in an unaligned pointer value [-Waddress-of-packed-member]
Quoted 6 lines
The warning is correct; because struct v4l2_pix_format_mplane is __packed, it's members also have __aligned(1). Taking the address of such members results in the use of underaligned pointers which is UB and may be caught by UBSAN or fault on architectures without unaligned loads should the struct instance happen to be allocated without any natural alignment.
Wouldn't that be the case only if the __packed attribute resulted in a different memory layout than not having that attribute?

Quoted 6 lines

Quoted 2 lines
All these fields are aligned by 4 so I don't see how this could be an actual problem.
packed means two things and developers often forgot about the second one:
- the gaps between members in the data structures are removed
- the instance of the data object may be on unaligned address

Well put; the second is something that surprised me yesterday. I'd like to say I'd forgotten, but I'm not sure I ever really knew that in the first place...marking a struct as being packed seems like shorthand for marking all of the members as having alignment of 1, which makes sense since natural alignment requirements are what prevent structure packing in the first place.

`I think this case should demonstrate the second point:
https://godbolt.org/z/77P484e4o

Signature · 2 lines

Thanks,
~Nick Desaulniers

Sakari Ailus

author

2022-01-11 21:07 UTC #7 lore raw

Hi Nick, Andy,

Quoted 42 lines · Nick Desaulniers

Quoted 33 lines · Andy Shevchenko
Quoted 31 lines · Sakari Ailus
Hi Nick,
Quoted 18 lines · Nick Desaulniers
Quoted 3 lines · Sakari Ailus
Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions.
I would rephrase the below statement...
Quoted 3 lines
This could result in an
unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.
...to:
warning: taking address of packed member 'pixelformat' of class or structure 'v4l2_pix_format_mplane' may result in an unaligned pointer value [-Waddress-of-packed-member]
Quoted 6 lines
The warning is correct; because struct v4l2_pix_format_mplane is __packed, it's members also have __aligned(1). Taking the address of such members results in the use of underaligned pointers which is UB and may be caught by UBSAN or fault on architectures without unaligned loads should the struct instance happen to be allocated without any natural alignment.
Wouldn't that be the case only if the __packed attribute resulted in a different memory layout than not having that attribute?
Quoted 6 lines
Quoted 2 lines
All these fields are aligned by 4 so I don't see how this could be an actual problem.
packed means two things and developers often forgot about the second one:
- the gaps between members in the data structures are removed
- the instance of the data object may be on unaligned address

Quoted 6 lines

Well put; the second is something that surprised me yesterday. I'd like to say I'd forgotten, but I'm not sure I ever really knew that in the first place...marking a struct as being packed seems like shorthand for marking all of the members as having alignment of 1, which makes sense since natural alignment requirements are what prevent structure packing in the first place.

I don't disagree with __packed allowing this but it is not the case here. The fields clang warns about are always aligned by 4. In other words, this warning is a false positive.

Signature · 3 lines

Kind regards,

Sakari Ailus

Nick Desaulniers

2022-01-11 21:36 UTC #8 lore raw

Quoted 53 lines · Sakari Ailus

Hi Nick, Andy,
Quoted 42 lines · Nick Desaulniers
Quoted 33 lines · Andy Shevchenko
Quoted 31 lines · Sakari Ailus
Hi Nick,
Quoted 18 lines · Nick Desaulniers
Quoted 3 lines · Sakari Ailus
Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions.
I would rephrase the below statement...
Quoted 3 lines
This could result in an
unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.
...to:
warning: taking address of packed member 'pixelformat' of class or structure 'v4l2_pix_format_mplane' may result in an unaligned pointer value [-Waddress-of-packed-member]
Quoted 6 lines
The warning is correct; because struct v4l2_pix_format_mplane is __packed, it's members also have __aligned(1). Taking the address of such members results in the use of underaligned pointers which is UB and may be caught by UBSAN or fault on architectures without unaligned loads should the struct instance happen to be allocated without any natural alignment.
Wouldn't that be the case only if the __packed attribute resulted in a different memory layout than not having that attribute?
Quoted 6 lines
Quoted 2 lines
All these fields are aligned by 4 so I don't see how this could be an actual problem.
packed means two things and developers often forgot about the second one:
- the gaps between members in the data structures are removed
- the instance of the data object may be on unaligned address
Quoted 6 lines
Well put; the second is something that surprised me yesterday. I'd like to say I'd forgotten, but I'm not sure I ever really knew that in the first place...marking a struct as being packed seems like shorthand for marking all of the members as having alignment of 1, which makes sense since natural alignment requirements are what prevent structure packing in the first place.

Quoted 3 lines

I don't disagree with __packed allowing this but it is not the case here. The fields clang warns about are always aligned by 4. In other words, this warning is a false positive.

The member pixelformat has a natural alignment of 4, but due to being a member of a packed struct, it now has an alignment of 1.

Signature · 2 lines

Thanks,
~Nick Desaulniers

Sakari Ailus

author

2022-01-11 21:57 UTC #9 lore raw

Quoted 55 lines · Nick Desaulniers

Quoted 53 lines · Sakari Ailus
Hi Nick, Andy,
Quoted 48 lines · Nick Desaulniers
Quoted 33 lines · Andy Shevchenko
Quoted 31 lines · Sakari Ailus
Hi Nick,
Quoted 18 lines · Nick Desaulniers
Quoted 3 lines · Sakari Ailus
Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions.
I would rephrase the below statement...
Quoted 3 lines
This could result in an
unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.
...to:
warning: taking address of packed member 'pixelformat' of class or structure 'v4l2_pix_format_mplane' may result in an unaligned pointer value [-Waddress-of-packed-member]
Quoted 6 lines
The warning is correct; because struct v4l2_pix_format_mplane is __packed, it's members also have __aligned(1). Taking the address of such members results in the use of underaligned pointers which is UB and may be caught by UBSAN or fault on architectures without unaligned loads should the struct instance happen to be allocated without any natural alignment.
Wouldn't that be the case only if the __packed attribute resulted in a different memory layout than not having that attribute?
Quoted 6 lines
Quoted 2 lines
All these fields are aligned by 4 so I don't see how this could be an actual problem.
packed means two things and developers often forgot about the second one:
- the gaps between members in the data structures are removed
- the instance of the data object may be on unaligned address
Well put; the second is something that surprised me yesterday. I'd like to say I'd forgotten, but I'm not sure I ever really knew that in the first place...marking a struct as being packed seems like shorthand for marking all of the members as having alignment of 1, which makes sense since natural alignment requirements are what prevent structure packing in the first place.

Quoted 6 lines

Quoted 3 lines
I don't disagree with __packed allowing this but it is not the case here. The fields clang warns about are always aligned by 4. In other words, this warning is a false positive.
The member pixelformat has a natural alignment of 4, but due to being a member of a packed struct, it now has an alignment of 1.

I think you're discussing the general case and the meaning of __packed whereas I'm talking about the circumstances where the struct is actually used in the kernel. Yes, different kind of use of the struct could lead to unaligned accesses (and crash on some archs) but that just does not happen here i.e. there is no bug to fix.

Signature · 1 line

Sakari Ailus

Andy Shevchenko

Reviewed-by on v1 1/1

2022-01-11 11:39 UTC #10 lore raw

In reply to Sakari Ailus

Quoted 11 lines · Sakari Ailus

Pointers V4L2 pixelformat and dataformat fields in a few packed structs are directly passed to printk family of functions. This could result in an unaligned access albeit no such possibility appears to exist at the moment i.e. this clang warning appears to be a false positive.
Address the warning by copying the pixelformat or dataformat value to a local variable first.
Reported-by kernel test robot <[email protected]>
Fixes e927e1e0f0dd ("v4l: ioctl: Use %p4cc printk modifier to print FourCC codes")
Signed-off-by Sakari Ailus <[email protected]>

Quoted 6 lines

---
Hi Andy, Nick,
How about this one?
I believe it does address the clang warning although I haven't tested it.

With addressed comments, pointed by Nick,

Reviewed-by Andy Shevchenko <[email protected]>

Thanks!

Note, applying this doesn't automatically discard my patch.

Quoted 50 lines

 drivers/media/v4l2-core/v4l2-ioctl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/media/v4l2-core/v4l2-ioctl.c +7 -5

index 9ac557b8e146..642cb90f457c 100644
@@ -279,8 +279,8 @@ static void v4l_print_format(const void *arg, bool write_only)
 	const struct v4l2_vbi_format *vbi;
 	const struct v4l2_sliced_vbi_format *sliced;
 	const struct v4l2_window *win;
-	const struct v4l2_sdr_format *sdr;
 	const struct v4l2_meta_format *meta;
+	u32 pixelformat;
 	u32 planes;
 	unsigned i;

@@ -299,8 +299,9 @@ static void v4l_print_format(const void *arg, bool write_only)
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
 		mp = &p->fmt.pix_mp;
+		pixelformat = mp->pixelformat;
 		pr_cont(", width=%u, height=%u, format=%p4cc, field=%s, colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u, xfer_func=%u\n",
-			mp->width, mp->height, &mp->pixelformat,
+			mp->width, mp->height, &pixelformat,
 			prt_names(mp->field, v4l2_field_names),
 			mp->colorspace, mp->num_planes, mp->flags,
 			mp->ycbcr_enc, mp->quantization, mp->xfer_func);

@@ -343,14 +344,15 @@ static void v4l_print_format(const void *arg, bool write_only)
 		break;
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
 	case V4L2_BUF_TYPE_SDR_OUTPUT:
-		sdr = &p->fmt.sdr;
-		pr_cont(", pixelformat=%p4cc\n", &sdr->pixelformat);
+		pixelformat = p->fmt.sdr.pixelformat;
+		pr_cont(", pixelformat=%p4cc\n", &pixelformat);
 		break;
 	case V4L2_BUF_TYPE_META_CAPTURE:
 	case V4L2_BUF_TYPE_META_OUTPUT:
 		meta = &p->fmt.meta;
+		pixelformat = meta->dataformat;
 		pr_cont(", dataformat=%p4cc, buffersize=%u\n",
-			&meta->dataformat, meta->buffersize);
+			&pixelformat, meta->buffersize);
 		break;
 	}
 }

Signature · 1 line

2.30.2

Signature · 2 lines

With Best Regards,
Andy Shevchenko