[PATCH] power: supply: axp20x_usb_power: fix 64-bit build warning

[PATCH] power: supply: axp20x_usb_power: fix 64-bit build warning

[Arnd Bergmann]

Casting a pointer to 'int' is not always valid:

drivers/power/supply/axp20x_usb_power.c: In function 'axp20x_usb_power_probe':
drivers/power/supply/axp20x_usb_power.c:297:21: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]

This makes the code use uintptr_t explicitly.

Fixes: 0dcc70ca8644 ("power: supply: axp20x_usb_power: use of_device_id data field instead of device_is_compatible")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/power/supply/axp20x_usb_power.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index 1bcb02551e02..ecad8b6f9b84 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -294,7 +294,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	if (!power)
 		return -ENOMEM;
 
-	power->axp20x_id = (int)of_device_get_match_data(&pdev->dev);
+	power->axp20x_id = (uintptr_t)of_device_get_match_data(&pdev->dev);
 
 	power->np = pdev->dev.of_node;
 	power->regmap = axp20x->regmap;
-- 
2.9.0

Re: [PATCH] power: supply: axp20x_usb_power: fix 64-bit build warning

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

Hi Arnd,

On Wed, Jan 11, 2017 at 03:51:55PM +0100, Arnd Bergmann wrote:
> Casting a pointer to 'int' is not always valid:
> 
> drivers/power/supply/axp20x_usb_power.c: In function 'axp20x_usb_power_probe':
> drivers/power/supply/axp20x_usb_power.c:297:21: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> 
> This makes the code use uintptr_t explicitly.
> 
> Fixes: 0dcc70ca8644 ("power: supply: axp20x_usb_power: use of_device_id data field instead of device_is_compatible")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I queued Michal's patch instead:

https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=for-next&id=15df6d98ec3b40775918fc6ef73d7f1c2d0cf870

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] power: supply: axp20x_usb_power: fix 64-bit build warning

[Arnd Bergmann]

On Thursday, January 12, 2017 3:58:24 AM CET Sebastian Reichel wrote:
> Hi Arnd,
> 
> On Wed, Jan 11, 2017 at 03:51:55PM +0100, Arnd Bergmann wrote:
> > Casting a pointer to 'int' is not always valid:
> > 
> > drivers/power/supply/axp20x_usb_power.c: In function 'axp20x_usb_power_probe':
> > drivers/power/supply/axp20x_usb_power.c:297:21: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> > 
> > This makes the code use uintptr_t explicitly.
> > 
> > Fixes: 0dcc70ca8644 ("power: supply: axp20x_usb_power: use of_device_id data field instead of device_is_compatible")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> I queued Michal's patch instead:
> 
> https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=for-next&id=15df6d98ec3b40775918fc6ef73d7f1c2d0cf870

Hmm, that doesn't look right: You can't really rely on an 'enum' type to
have a specific size, especially not the same size as a pointer, in portable
code.

IIRC on many architectures it defaults to 'int' rather than 'long', and
it might also be 'short' on architectures that default to enums being
the smallest integer type that fits.

	Arnd

Re: [PATCH] power: supply: axp20x_usb_power: fix 64-bit build warning

[Michal Suchánek]

Hello,

On Thu, 12 Jan 2017 09:32:26 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Thursday, January 12, 2017 3:58:24 AM CET Sebastian Reichel wrote:
> > Hi Arnd,
> > 
> > On Wed, Jan 11, 2017 at 03:51:55PM +0100, Arnd Bergmann wrote:  
> > > Casting a pointer to 'int' is not always valid:
> > > 
> > > drivers/power/supply/axp20x_usb_power.c: In function
> > > 'axp20x_usb_power_probe':
> > > drivers/power/supply/axp20x_usb_power.c:297:21: error: cast from
> > > pointer to integer of different size [-Werror=pointer-to-int-cast]
> > > 
> > > This makes the code use uintptr_t explicitly.
> > > 
> > > Fixes: 0dcc70ca8644 ("power: supply: axp20x_usb_power: use
> > > of_device_id data field instead of device_is_compatible")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>  
> > 
> > I queued Michal's patch instead:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=for-next&id=15df6d98ec3b40775918fc6ef73d7f1c2d0cf870  
> 
> Hmm, that doesn't look right: You can't really rely on an 'enum' type
> to have a specific size, especially not the same size as a pointer,
> in portable code.
> 
> IIRC on many architectures it defaults to 'int' rather than 'long',
> and it might also be 'short' on architectures that default to enums
> being the smallest integer type that fits.

Technically to fit the pointer into an integer you should be using
uintptr_t or unsigned long. However, gcc does not issue a warning when
casting to enum either. The reason might possibly be that when casting
to an enum you make it clear that you are expecting one of the values
that are part of the enumeration and the value should fit into the enum
even if its actual size is char.

Either way handling of casts from the match pointer to integer varies
between drivers and some that are possibly never built on 64bit even use
int.

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:
adv7511->type = (enum adv7511_type)of_device_get_match_data(dev);
drivers/i2c/busses/i2c-rcar.c:	priv->devtype = (enum
rcar_i2c_type)of_device_get_match_data(dev);
drivers/net/ethernet/renesas/ravb_main.c:	chip_id = (enum
ravb_chip_id)of_device_get_match_data(&pdev->dev);
drivers/pci/host/pci-imx6.c:		(enum
imx6_pcie_variants)of_device_get_match_data(dev);
drivers/reset/hisilicon/hi6220_reset.c:	type = (enum
hi6220_reset_ctrl_type)of_device_get_match_data(dev);
drivers/usb/phy/phy-msm-usb.c:	pdata->phy_type = (enum
msm_usb_phy_type)of_device_get_match_data(&pdev->dev);

drivers/firmware/qcom_scm.c:	clks = (unsigned
long)of_device_get_match_data(&pdev->dev);
drivers/gpu/drm/exynos/exynos5433_drm_decon.c:	ctx->out_type =
(unsigned long)of_device_get_match_data(dev);
drivers/pinctrl/sunxi/pinctrl-sun5i.c:	unsigned long variant =
(unsigned long)of_device_get_match_data(&pdev->dev);
drivers/spi/spi-sun6i.c:	sspi->fifo_depth = (unsigned
long)of_device_get_match_data(&pdev->dev);
drivers/thermal/rcar_thermal.c:#define rcar_of_data(dev)
((unsigned long)of_device_get_match_data(dev))
sound/soc/sh/rcar/core.c:	priv->flags	= (unsigned
long)of_device_get_match_data(dev);

drivers/spi/spi-mpc512x-psc.c:	mps->type =
(int)of_device_get_match_data(dev);
drivers/leds/leds-pm8058.c:	led->ledtype =
(u32)of_device_get_match_data(&pdev->dev);


So what is the preferred way to do the cast to be portable across Linux
architectures?

Thanks

Michal

Re: [PATCH] power: supply: axp20x_usb_power: fix 64-bit build warning

[Arnd Bergmann]

On Thursday, January 12, 2017 4:53:55 PM CET Michal Suchánek wrote:
> Hello,
> 
> On Thu, 12 Jan 2017 09:32:26 +0100
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Thursday, January 12, 2017 3:58:24 AM CET Sebastian Reichel wrote:
> > > Hi Arnd,
> > > 
> > > On Wed, Jan 11, 2017 at 03:51:55PM +0100, Arnd Bergmann wrote:  
> > > > Casting a pointer to 'int' is not always valid:
> > > > 
> > > > drivers/power/supply/axp20x_usb_power.c: In function
> > > > 'axp20x_usb_power_probe':
> > > > drivers/power/supply/axp20x_usb_power.c:297:21: error: cast from
> > > > pointer to integer of different size [-Werror=pointer-to-int-cast]
> > > > 
> > > > This makes the code use uintptr_t explicitly.
> > > > 
> > > > Fixes: 0dcc70ca8644 ("power: supply: axp20x_usb_power: use
> > > > of_device_id data field instead of device_is_compatible")
> > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>  
> > > 
> > > I queued Michal's patch instead:
> > > 
> > > https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=for-next&id=15df6d98ec3b40775918fc6ef73d7f1c2d0cf870  
> > 
> > Hmm, that doesn't look right: You can't really rely on an 'enum' type
> > to have a specific size, especially not the same size as a pointer,
> > in portable code.
> > 
> > IIRC on many architectures it defaults to 'int' rather than 'long',
> > and it might also be 'short' on architectures that default to enums
> > being the smallest integer type that fits.
> 
> Technically to fit the pointer into an integer you should be using
> uintptr_t or unsigned long. However, gcc does not issue a warning when
> casting to enum either. The reason might possibly be that when casting
> to an enum you make it clear that you are expecting one of the values
> that are part of the enumeration and the value should fit into the enum
> even if its actual size is char.

Interesting. I've confirmed that here, your patch is fine then!

> Either way handling of casts from the match pointer to integer varies
> between drivers and some that are possibly never built on 64bit even use
> int.
> 
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:
> adv7511->type = (enum adv7511_type)of_device_get_match_data(dev);
> drivers/i2c/busses/i2c-rcar.c:	priv->devtype = (enum
> rcar_i2c_type)of_device_get_match_data(dev);
> drivers/net/ethernet/renesas/ravb_main.c:	chip_id = (enum
> ravb_chip_id)of_device_get_match_data(&pdev->dev);
> drivers/pci/host/pci-imx6.c:		(enum
> imx6_pcie_variants)of_device_get_match_data(dev);
> drivers/reset/hisilicon/hi6220_reset.c:	type = (enum
> hi6220_reset_ctrl_type)of_device_get_match_data(dev);
> drivers/usb/phy/phy-msm-usb.c:	pdata->phy_type = (enum
> msm_usb_phy_type)of_device_get_match_data(&pdev->dev);
> 
> drivers/firmware/qcom_scm.c:	clks = (unsigned
> long)of_device_get_match_data(&pdev->dev);
> drivers/gpu/drm/exynos/exynos5433_drm_decon.c:	ctx->out_type =
> (unsigned long)of_device_get_match_data(dev);
> drivers/pinctrl/sunxi/pinctrl-sun5i.c:	unsigned long variant =
> (unsigned long)of_device_get_match_data(&pdev->dev);
> drivers/spi/spi-sun6i.c:	sspi->fifo_depth = (unsigned
> long)of_device_get_match_data(&pdev->dev);
> drivers/thermal/rcar_thermal.c:#define rcar_of_data(dev)
> ((unsigned long)of_device_get_match_data(dev))
> sound/soc/sh/rcar/core.c:	priv->flags	= (unsigned
> long)of_device_get_match_data(dev);
> 
> drivers/spi/spi-mpc512x-psc.c:	mps->type =
> (int)of_device_get_match_data(dev);
> drivers/leds/leds-pm8058.c:	led->ledtype =
> (u32)of_device_get_match_data(&pdev->dev);
> 
> 
> So what is the preferred way to do the cast to be portable across Linux
> architectures?

Traditionally, we use a cast to 'unsigned long' in the kernel, though
I tend to use 'uintptr_t' when I do patches because that makes the
intention clearer. Both of these always work on all architectures that
Linux supports. The two last cases you cite that use 'int' or 'u32'
are obviously nonportable.

	Arnd

Re: [PATCH] power: supply: axp20x_usb_power: fix 64-bit build warning

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2392 bytes --]


On Thu, Jan 12, 2017 at 09:32:26AM +0100, Arnd Bergmann wrote:
> On Thursday, January 12, 2017 3:58:24 AM CET Sebastian Reichel wrote:
> > On Wed, Jan 11, 2017 at 03:51:55PM +0100, Arnd Bergmann wrote:
> > > Casting a pointer to 'int' is not always valid:
> > > 
> > > drivers/power/supply/axp20x_usb_power.c: In function 'axp20x_usb_power_probe':
> > > drivers/power/supply/axp20x_usb_power.c:297:21: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> > > 
> > > This makes the code use uintptr_t explicitly.
> > > 
> > > Fixes: 0dcc70ca8644 ("power: supply: axp20x_usb_power: use of_device_id data field instead of device_is_compatible")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > I queued Michal's patch instead:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=for-next&id=15df6d98ec3b40775918fc6ef73d7f1c2d0cf870
> 
> Hmm, that doesn't look right: You can't really rely on an 'enum' type to
> have a specific size, especially not the same size as a pointer, in portable
> code.
> 
> IIRC on many architectures it defaults to 'int' rather than 'long', and
> it might also be 'short' on architectures that default to enums being
> the smallest integer type that fits.

gcc does not generate warnings as far as I can see and the precision
loss itself is not a problem in this case, since we only store the
enum values in the pointer anyways.

I don't mind adding the uintptr_t cast. But in that case it should
probably be added in a few other places, too:

$ git grep of_device_get_match_data | grep "(enum"
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:		adv7511->type = (enum adv7511_type)of_device_get_match_data(dev);
drivers/i2c/busses/i2c-rcar.c:	priv->devtype = (enum rcar_i2c_type)of_device_get_match_data(dev);
drivers/net/ethernet/renesas/ravb_main.c:	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
drivers/pci/host/pci-imx6.c:		(enum imx6_pcie_variants)of_device_get_match_data(dev);
drivers/power/supply/axp20x_usb_power.c:	power->axp20x_id = (enum axp20x_variants)of_device_get_match_data(
drivers/reset/hisilicon/hi6220_reset.c:	type = (enum hi6220_reset_ctrl_type)of_device_get_match_data(dev);
drivers/usb/phy/phy-msm-usb.c:	pdata->phy_type = (enum msm_usb_phy_type)of_device_get_match_data(&pdev->dev);

--Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]