[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230905104608.74490e60@donnerap.manchester.arm.com>
Date: Tue, 5 Sep 2023 10:46:08 +0100
From: Andre Przywara <andre.przywara@....com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Vinod Koul <vkoul@...nel.org>,
Kishon Vijay Abraham I <kishon@...nel.org>,
Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Samuel Holland <samuel@...lland.org>,
linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
linux-phy@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure
On Mon, 4 Sep 2023 19:57:33 +0200
Christophe JAILLET <christophe.jaillet@...adoo.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@...adoo.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@...adoo.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;
> >
> >
>
Powered by blists - more mailing lists