[PATCH] drm/panthor: Clean up FW version information display

[PATCH] drm/panthor: Clean up FW version information display

[Steven Price]

Assigning a string to an array which is too small to include the NUL
byte at the end causes a warning on some compilers. But this function
also has some other oddities like the 'header' array which is only ever
used within sizeof().

Tidy up the function by removing the 'header' array, allow the NUL byte
to be present in git_sha_header, and calculate the length directly from
git_sha_header.

Reported-by: Will Deacon <will@kernel.org>
Fixes: 9d443deb0441 ("drm/panthor: Display FW version information")
Signed-off-by: Steven Price <steven.price@arm.com>
---
Note that there should be no functional change from this patch.
---
 drivers/gpu/drm/panthor/panthor_fw.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 4a9c4afa9ad7..645fc6d2e63b 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -636,8 +636,8 @@ static int panthor_fw_read_build_info(struct panthor_device *ptdev,
 				      u32 ehdr)
 {
 	struct panthor_fw_build_info_hdr hdr;
-	char header[9];
-	const char git_sha_header[sizeof(header)] = "git_sha: ";
+	const char git_sha_header[] = "git_sha: ";
+	const int header_len = sizeof(git_sha_header) - 1;
 	int ret;
 
 	ret = panthor_fw_binary_iter_read(ptdev, iter, &hdr, sizeof(hdr));
@@ -651,8 +651,7 @@ static int panthor_fw_read_build_info(struct panthor_device *ptdev,
 		return 0;
 	}
 
-	if (memcmp(git_sha_header, fw->data + hdr.meta_start,
-		   sizeof(git_sha_header))) {
+	if (memcmp(git_sha_header, fw->data + hdr.meta_start, header_len)) {
 		/* Not the expected header, this isn't metadata we understand */
 		return 0;
 	}
@@ -665,7 +664,7 @@ static int panthor_fw_read_build_info(struct panthor_device *ptdev,
 	}
 
 	drm_info(&ptdev->base, "Firmware git sha: %s\n",
-		 fw->data + hdr.meta_start + sizeof(git_sha_header));
+		 fw->data + hdr.meta_start + header_len);
 
 	return 0;
 }
-- 
2.39.5

Re: [PATCH] drm/panthor: Clean up FW version information display

[Will Deacon]

On Thu, Feb 13, 2025 at 04:12:48PM +0000, Steven Price wrote:
> Assigning a string to an array which is too small to include the NUL
> byte at the end causes a warning on some compilers. But this function
> also has some other oddities like the 'header' array which is only ever
> used within sizeof().
> 
> Tidy up the function by removing the 'header' array, allow the NUL byte
> to be present in git_sha_header, and calculate the length directly from
> git_sha_header.
> 
> Reported-by: Will Deacon <will@kernel.org>
> Fixes: 9d443deb0441 ("drm/panthor: Display FW version information")
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> Note that there should be no functional change from this patch.
> ---
>  drivers/gpu/drm/panthor/panthor_fw.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 4a9c4afa9ad7..645fc6d2e63b 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -636,8 +636,8 @@ static int panthor_fw_read_build_info(struct panthor_device *ptdev,
>  				      u32 ehdr)
>  {
>  	struct panthor_fw_build_info_hdr hdr;
> -	char header[9];
> -	const char git_sha_header[sizeof(header)] = "git_sha: ";
> +	const char git_sha_header[] = "git_sha: ";
> +	const int header_len = sizeof(git_sha_header) - 1;

nit: strlen()?

In any case, this fixes the arm64 CI [1], so:

Acked-by: Will Deacon <will@kernel.org>

It would be great to land this as a fix to save us having to carry it
on our CI branch.

Cheers,

Will

[1] https://lore.kernel.org/r/20250213154237.GA11897@willie-the-truck

Re: [PATCH] drm/panthor: Clean up FW version information display

[Boris Brezillon]

On Thu, 13 Feb 2025 16:12:48 +0000
Steven Price <steven.price@arm.com> wrote:

> Assigning a string to an array which is too small to include the NUL
> byte at the end causes a warning on some compilers. But this function
> also has some other oddities like the 'header' array which is only ever
> used within sizeof().
> 
> Tidy up the function by removing the 'header' array, allow the NUL byte
> to be present in git_sha_header, and calculate the length directly from
> git_sha_header.
> 
> Reported-by: Will Deacon <will@kernel.org>
> Fixes: 9d443deb0441 ("drm/panthor: Display FW version information")
> Signed-off-by: Steven Price <steven.price@arm.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
> Note that there should be no functional change from this patch.
> ---
>  drivers/gpu/drm/panthor/panthor_fw.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 4a9c4afa9ad7..645fc6d2e63b 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -636,8 +636,8 @@ static int panthor_fw_read_build_info(struct panthor_device *ptdev,
>  				      u32 ehdr)
>  {
>  	struct panthor_fw_build_info_hdr hdr;
> -	char header[9];
> -	const char git_sha_header[sizeof(header)] = "git_sha: ";
> +	const char git_sha_header[] = "git_sha: ";
> +	const int header_len = sizeof(git_sha_header) - 1;
>  	int ret;
>  
>  	ret = panthor_fw_binary_iter_read(ptdev, iter, &hdr, sizeof(hdr));
> @@ -651,8 +651,7 @@ static int panthor_fw_read_build_info(struct panthor_device *ptdev,
>  		return 0;
>  	}
>  
> -	if (memcmp(git_sha_header, fw->data + hdr.meta_start,
> -		   sizeof(git_sha_header))) {
> +	if (memcmp(git_sha_header, fw->data + hdr.meta_start, header_len)) {
>  		/* Not the expected header, this isn't metadata we understand */
>  		return 0;
>  	}
> @@ -665,7 +664,7 @@ static int panthor_fw_read_build_info(struct panthor_device *ptdev,
>  	}
>  
>  	drm_info(&ptdev->base, "Firmware git sha: %s\n",
> -		 fw->data + hdr.meta_start + sizeof(git_sha_header));
> +		 fw->data + hdr.meta_start + header_len);
>  
>  	return 0;
>  }