| Message ID | 0bc81612171baaa6d5dff58c8e009debc03e1ba8.1693735840.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | phy: sun4i-usb: Fix a W=1 compilation failure | expand |
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;
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; > >
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; > > > > >
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
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 >
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; >
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,
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
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;
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(-)