| Message ID | 1398792971-10379-1-git-send-email-mq@suse.cz |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
2014-04-29 21:36 GMT+04:00 Jan Moskyto Matejka <mq@suse.cz>: > Fixed different size cast warning: > > drivers/net/ethernet/via/via-rhine.c: In function ‘rhine_init_one_platform’: > drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > revision = (u32)match->data; > ^ > > That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765 > ("net: via-rhine: add OF bus binding"). > > Signed-off-by: Jan Moskyto Matejka <mq@suse.cz> Jan, thanks a lot for catching this. Have to admit that I didn't compile it on 64bit. Acked-by: Alexey Charkov <alchark@gmail.com> Looks like the same would apply to e.g. drivers/clk/samsung/clk.c and maybe some others... Also, a number of drivers cast OF data from (void *) to (int) or (unsigned int) - isn't this also problematic on 64bit? Thanks a lot, Alexey -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 29, 2014 at 10:09:01PM +0400, Alexey Charkov wrote: > 2014-04-29 21:36 GMT+04:00 Jan Moskyto Matejka <mq@suse.cz>: > > Fixed different size cast warning: > > > > drivers/net/ethernet/via/via-rhine.c: In function ‘rhine_init_one_platform’: > > drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] > > revision = (u32)match->data; > > ^ > > Looks like the same would apply to e.g. drivers/clk/samsung/clk.c and > maybe some others... Also, a number of drivers cast OF data from (void > *) to (int) or (unsigned int) - isn't this also problematic on 64bit? Some of them are, some not, sometimes nobody knows. If it were up to me, I would personally put a single comment "this throws a compilation warning because this and that" at every place where the warning is thrown and is to be ignored. So I do fix almost every warning of this kind I come across. Better to have that fixed than to believe that the warning is harmless (as this probably really was). I'm just running a script that checks the tree for new build warnings and then analysing the script's output ... and sometimes it's worth fixing, sometimes not. Moskyto
From: Jan Moskyto Matejka > Fixed different size cast warning: > > drivers/net/ethernet/via/via-rhine.c: In function rhine_init_one_platform: > drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different > size [-Wpointer-to-int-cast] > revision = (u32)match->data; > ^ > > That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765 > ("net: via-rhine: add OF bus binding"). ... > diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c > index 4fa9201..76d18e0 100644 > --- a/drivers/net/ethernet/via/via-rhine.c > +++ b/drivers/net/ethernet/via/via-rhine.c > @@ -288,7 +288,7 @@ MODULE_DEVICE_TABLE(pci, rhine_pci_tbl); > * (for quirks etc.) > */ > static struct of_device_id rhine_of_tbl[] = { > - { .compatible = "via,vt8500-rhine", .data = (void *)0x84 }, > + { .compatible = "via,vt8500-rhine", .data = (u32 []) { 0x84 } }, > { } /* terminate list */ > }; > MODULE_DEVICE_TABLE(of, rhine_of_tbl); > @@ -1129,7 +1129,7 @@ static int rhine_init_one_platform(struct platform_device *pdev) > if (!irq) > return -EINVAL; > > - revision = (u32)match->data; > + revision = *((u32 *) match->data); > if (!revision) > return -EINVAL; Both those casts look horrid. I'm not entirely convinced that the first is valid C - It would have to be something specific to C99 initialisers. Casts like *(u32 *)foo are also likely to be bugs (esp. on BE systems) so themselves start ringing alarm bells. So why not just: revision = (unsigned long)match->data; and add a comment that the 0x84 is the revision - #define ?? David
2014-04-30 12:49 GMT+04:00 David Laight <David.Laight@aculab.com>: > From: Jan Moskyto Matejka >> Fixed different size cast warning: >> >> drivers/net/ethernet/via/via-rhine.c: In function rhine_init_one_platform: >> drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different >> size [-Wpointer-to-int-cast] >> revision = (u32)match->data; >> ^ >> >> That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765 >> ("net: via-rhine: add OF bus binding"). > ... >> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c >> index 4fa9201..76d18e0 100644 >> --- a/drivers/net/ethernet/via/via-rhine.c >> +++ b/drivers/net/ethernet/via/via-rhine.c >> @@ -288,7 +288,7 @@ MODULE_DEVICE_TABLE(pci, rhine_pci_tbl); >> * (for quirks etc.) >> */ >> static struct of_device_id rhine_of_tbl[] = { >> - { .compatible = "via,vt8500-rhine", .data = (void *)0x84 }, >> + { .compatible = "via,vt8500-rhine", .data = (u32 []) { 0x84 } }, >> { } /* terminate list */ >> }; >> MODULE_DEVICE_TABLE(of, rhine_of_tbl); >> @@ -1129,7 +1129,7 @@ static int rhine_init_one_platform(struct platform_device *pdev) >> if (!irq) >> return -EINVAL; >> >> - revision = (u32)match->data; >> + revision = *((u32 *) match->data); >> if (!revision) >> return -EINVAL; > > Both those casts look horrid. > I'm not entirely convinced that the first is valid C - It would have to be > something specific to C99 initialisers. > Casts like *(u32 *)foo are also likely to be bugs (esp. on BE systems) > so themselves start ringing alarm bells. > > So why not just: > revision = (unsigned long)match->data; > and add a comment that the 0x84 is the revision - #define ?? There is no particular reason why it should be u32 now - this is a leftover from the previous iteration of code where revision was a separate property in DT (sized u32). It actually mirrors the respective field in struct pci_dev, which is u8 - don't see any issue defining it as unsigned long (and also changing the definition in struct rhine_private). The comment that it's the revision is right above the match table (cut off in the patch) :) Jan, would you prefer to adjust your patch, or shall I send another one to change rp->revision and friends to unsigned long? Thanks a lot, Alexey -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > So why not just: > > revision = (unsigned long)match->data; > > and add a comment that the 0x84 is the revision - #define ?? > > There is no particular reason why it should be u32 now - this is a > leftover from the previous iteration of code where revision was a > separate property in DT (sized u32). It actually mirrors the > respective field in struct pci_dev, which is u8 - don't see any issue > defining it as unsigned long (and also changing the definition in > struct rhine_private). > > The comment that it's the revision is right above the match table (cut > off in the patch) :) > > Jan, would you prefer to adjust your patch, or shall I send another > one to change rp->revision and friends to unsigned long? I prefer you to make another patch, you obviously know more about this driver. Also thanks for suggesting (void*)->(unsigned long), I didn't know that these two are defined to have the same size (in kernel code).
> > Also thanks for suggesting (void*)->(unsigned long), I didn't > > know that these two are defined to have the same size (in kernel code). > > The kernel assumes that throughout - the double cast is common. > The C99 type is uintptr_t - but I don't think that is defined in kernel. > The only place I know where sizeof (long) != sizeof (void *) is 64bit > windows. So anyone trying to compile a linux driver to run in the > windows kernel might have issues (never mind the GPL ones). My colleague here has just explained me the same. :) Being a linux-kernel newbie, I'm learning every day. Kernel programming is more different from userspace than I ever thought. Thank you for your patience.
diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c index 4fa9201..76d18e0 100644 --- a/drivers/net/ethernet/via/via-rhine.c +++ b/drivers/net/ethernet/via/via-rhine.c @@ -288,7 +288,7 @@ MODULE_DEVICE_TABLE(pci, rhine_pci_tbl); * (for quirks etc.) */ static struct of_device_id rhine_of_tbl[] = { - { .compatible = "via,vt8500-rhine", .data = (void *)0x84 }, + { .compatible = "via,vt8500-rhine", .data = (u32 []) { 0x84 } }, { } /* terminate list */ }; MODULE_DEVICE_TABLE(of, rhine_of_tbl); @@ -1129,7 +1129,7 @@ static int rhine_init_one_platform(struct platform_device *pdev) if (!irq) return -EINVAL; - revision = (u32)match->data; + revision = *((u32 *) match->data); if (!revision) return -EINVAL;
Fixed different size cast warning: drivers/net/ethernet/via/via-rhine.c: In function ‘rhine_init_one_platform’: drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] revision = (u32)match->data; ^ That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765 ("net: via-rhine: add OF bus binding"). Signed-off-by: Jan Moskyto Matejka <mq@suse.cz> --- drivers/net/ethernet/via/via-rhine.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)