[PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[Christophe JAILLET]

With gcc 12.3.0, when this file is built, we get errors such as:

drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
  790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
      |                                                    ^~~~~
drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
  790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Because of the possible value of 'i', this can't be an issue in real world
application, but in order to have "make W=1" work correctly, give more
space for 'name'.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
index ec551464dd4f..e53a9a9317bc 100644
--- a/drivers/phy/allwinner/phy-sun4i-usb.c
+++ b/drivers/phy/allwinner/phy-sun4i-usb.c
@@ -782,7 +782,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
 
 	for (i = 0; i < data->cfg->num_phys; i++) {
 		struct sun4i_usb_phy *phy = data->phys + i;
-		char name[16];
+		char name[32];
 
 		if (data->cfg->missing_phys & BIT(i))
 			continue;
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[Andre Przywara]

On Sun,  3 Sep 2023 12:11:06 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> With gcc 12.3.0, when this file is built, we get errors such as:
> 
> drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
> drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
>   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
>       |                                                    ^~~~~
> drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
>   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Because of the possible value of 'i', this can't be an issue in real world

Would using "u8 i;" help? After all currently there are only 4 PHYs
max, and in general this isn't expected to be more than a "handful", so
8 bits should be plenty. An unsigned is better anyway.
It leaves a bit of a bitter taste, though, as we shouldn't do this kind
type tweaking, especially not to work around the compiler trying to be
clever, but then not seeing the whole picture (that "i" is bounded by
compile time constants not exceeding "4").

Cheers,
Andre

> application, but in order to have "make W=1" work correctly, give more
> space for 'name'.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index ec551464dd4f..e53a9a9317bc 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -782,7 +782,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
>  
>  	for (i = 0; i < data->cfg->num_phys; i++) {
>  		struct sun4i_usb_phy *phy = data->phys + i;
> -		char name[16];
> +		char name[32];
>  
>  		if (data->cfg->missing_phys & BIT(i))
>  			continue;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[Christophe JAILLET]

Le 04/09/2023 à 01:58, Andre Przywara a écrit :
> On Sun,  3 Sep 2023 12:11:06 +0200
> Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> 
>> With gcc 12.3.0, when this file is built, we get errors such as:
>>
>> drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
>> drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
>>    790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
>>        |                                                    ^~~~~
>> drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
>>    790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
>>        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Because of the possible value of 'i', this can't be an issue in real world
> 
> Would using "u8 i;" help? After all currently there are only 4 PHYs
> max, and in general this isn't expected to be more than a "handful", so
> 8 bits should be plenty. An unsigned is better anyway.
> It leaves a bit of a bitter taste, though, as we shouldn't do this kind
> type tweaking, especially not to work around the compiler trying to be
> clever, but then not seeing the whole picture (that "i" is bounded by
> compile time constants not exceeding "4").

data->cfg->num_phys is also an int, and having 'i' as an char is really 
unusual.

So, if changing the size of name (only to waste some stack in order to 
silence a compiler warning) is not acceptable, I think that the best is 
to leave things as-is.

CJ


> 
> Cheers,
> Andre
> 
>> application, but in order to have "make W=1" work correctly, give more
>> space for 'name'.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
>> index ec551464dd4f..e53a9a9317bc 100644
>> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
>> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
>> @@ -782,7 +782,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
>>   
>>   	for (i = 0; i < data->cfg->num_phys; i++) {
>>   		struct sun4i_usb_phy *phy = data->phys + i;
>> -		char name[16];
>> +		char name[32];
>>   
>>   		if (data->cfg->missing_phys & BIT(i))
>>   			continue;
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[Andre Przywara]

On Mon, 4 Sep 2023 19:57:33 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

Hi,

> Le 04/09/2023 à 01:58, Andre Przywara a écrit :
> > On Sun,  3 Sep 2023 12:11:06 +0200
> > Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> >   
> >> With gcc 12.3.0, when this file is built, we get errors such as:
> >>
> >> drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
> >> drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
> >>    790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> >>        |                                                    ^~~~~
> >> drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
> >>    790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> >>        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> Because of the possible value of 'i', this can't be an issue in real world  
> > 
> > Would using "u8 i;" help? After all currently there are only 4 PHYs
> > max, and in general this isn't expected to be more than a "handful", so
> > 8 bits should be plenty. An unsigned is better anyway.
> > It leaves a bit of a bitter taste, though, as we shouldn't do this kind
> > type tweaking, especially not to work around the compiler trying to be
> > clever, but then not seeing the whole picture (that "i" is bounded by
> > compile time constants not exceeding "4").  
> 
> data->cfg->num_phys is also an int, and having 'i' as an char is really 
> unusual.

So 'i' is just used as the phy index is this loop, nothing else in the
function uses that. So we could just rename it to "idx" or even "phy_idx",
then the u8 might look less odd?

> So, if changing the size of name (only to waste some stack in order to 
> silence a compiler warning) is not acceptable, I think that the best is 
> to leave things as-is.

But that's not really an option, is it? Since we normally don't tolerate
warnings?

And I am not against increasing the size, that's probably indeed the
simplest solution, and given that it's indeed on the stack shouldn't
affect much else. I just wanted to suggest an alternative, since the
increased buffer size is not necessary.

Cheers,
Andre

> > 
> > Cheers,
> > Andre
> >   
> >> application, but in order to have "make W=1" work correctly, give more
> >> space for 'name'.
> >>
> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >> ---
> >>   drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> >> index ec551464dd4f..e53a9a9317bc 100644
> >> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> >> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> >> @@ -782,7 +782,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
> >>   
> >>   	for (i = 0; i < data->cfg->num_phys; i++) {
> >>   		struct sun4i_usb_phy *phy = data->phys + i;
> >> -		char name[16];
> >> +		char name[32];
> >>   
> >>   		if (data->cfg->missing_phys & BIT(i))
> >>   			continue;  
> > 
> >   
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[Dan Carpenter]

On Mon, Sep 04, 2023 at 12:58:55AM +0100, Andre Przywara wrote:
> On Sun,  3 Sep 2023 12:11:06 +0200
> Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> 
> > With gcc 12.3.0, when this file is built, we get errors such as:
> > 
> > drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
> > drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
> >   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> >       |                                                    ^~~~~
> > drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
> >   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Because of the possible value of 'i', this can't be an issue in real world
> 
> Would using "u8 i;" help? After all currently there are only 4 PHYs
> max, and in general this isn't expected to be more than a "handful", so
> 8 bits should be plenty. An unsigned is better anyway.

Generally unsigned types are trickier and cause bugs.  I've blogged
about this before.  The title is a probably more negative than it should
have been.

https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/

My blog mentions u8 i.  I would say avoid that unless forced by an API.

> It leaves a bit of a bitter taste, though, as we shouldn't do this kind
> type tweaking, especially not to work around the compiler trying to be
> clever, but then not seeing the whole picture (that "i" is bounded by
> compile time constants not exceeding "4").

Yeah.  There is always the option of just ignoring the static checker
when it tells you to write bad code.

You don't have to even look at the *whole* picture to know that GCC is
wrong.  The BIT(i) would overflow if i is more than 31.

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[Andre Przywara]

On Tue, 5 Sep 2023 13:32:08 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

Hi,

> On Mon, Sep 04, 2023 at 12:58:55AM +0100, Andre Przywara wrote:
> > On Sun,  3 Sep 2023 12:11:06 +0200
> > Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> >   
> > > With gcc 12.3.0, when this file is built, we get errors such as:
> > > 
> > > drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
> > > drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
> > >   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> > >       |                                                    ^~~~~
> > > drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
> > >   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> > >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > Because of the possible value of 'i', this can't be an issue in real world  
> > 
> > Would using "u8 i;" help? After all currently there are only 4 PHYs
> > max, and in general this isn't expected to be more than a "handful", so
> > 8 bits should be plenty. An unsigned is better anyway.  
> 
> Generally unsigned types are trickier and cause bugs.  I've blogged
> about this before.  The title is a probably more negative than it should
> have been.
> 
> https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/
> 
> My blog mentions u8 i.  I would say avoid that unless forced by an API.

Fair enough, the reason I suggested u8 was to allow us using "%u" in the
snprintf, so any static checker would not try to account for a potential
'-' character. Because not doing so would spoil that approach for the
"usb%d_hsic_12M" string further down.

> > It leaves a bit of a bitter taste, though, as we shouldn't do this kind
> > type tweaking, especially not to work around the compiler trying to be
> > clever, but then not seeing the whole picture (that "i" is bounded by
> > compile time constants not exceeding "4").  
> 
> Yeah.  There is always the option of just ignoring the static checker
> when it tells you to write bad code.

Agreed on that, though I find those diagnostics useful, and just ignoring
or masking them might come back and haunt us later.

So I still think we should fix this, one way or the other.

But I feel this goes quite far into bikeshedding territory, so we should
probably just go with name[32].

Cheers,
Andre.

> You don't have to even look at the *whole* picture to know that GCC is
> wrong.  The BIT(i) would overflow if i is more than 31.
> 
> regards,
> dan carpenter
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[Jernej Škrabec]

Dne nedelja, 03. september 2023 ob 12:11:06 CEST je Christophe JAILLET napisal(a):
> With gcc 12.3.0, when this file is built, we get errors such as:
> 
> drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
> drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
>   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
>       |                                                    ^~~~~
> drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
>   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Because of the possible value of 'i', this can't be an issue in real world
> application, but in order to have "make W=1" work correctly, give more
> space for 'name'.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index ec551464dd4f..e53a9a9317bc 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -782,7 +782,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
>  
>  	for (i = 0; i < data->cfg->num_phys; i++) {
>  		struct sun4i_usb_phy *phy = data->phys + i;
> -		char name[16];
> +		char name[32];
>  
>  		if (data->cfg->missing_phys & BIT(i))
>  			continue;
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[Vinod Koul]

On Sun, 03 Sep 2023 12:11:06 +0200, Christophe JAILLET wrote:
> With gcc 12.3.0, when this file is built, we get errors such as:
> 
> drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
> drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
>   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
>       |                                                    ^~~~~
> drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
>   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> [...]

Applied, thanks!

[1/1] phy: sun4i-usb: Fix a W=1 compilation failure
      commit: 9e34abc7abfac781df909891c8d53781f607105d

Best regards,
-- 
~Vinod



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

[Vinod Koul]

On 13-10-23, 16:13, Vinod Koul wrote:
> 
> On Sun, 03 Sep 2023 12:11:06 +0200, Christophe JAILLET wrote:
> > With gcc 12.3.0, when this file is built, we get errors such as:
> > 
> > drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
> > drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
> >   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> >       |                                                    ^~~~~
> > drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
> >   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/1] phy: sun4i-usb: Fix a W=1 compilation failure
>       commit: 9e34abc7abfac781df909891c8d53781f607105d

FWIW, I have modified patch title to reflect the change it introduces
while applying

-- 
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel