Message ID | 1320314302-7973-1-git-send-email-Shaohui.Xie@freescale.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
> Below are codes for accessing usb sysif_regs in driver: > > usb_sys_regs = (struct usb_sys_interface *) > ((u32)dr_regs + USB_DR_SYS_OFFSET); > > these codes work in 32-bit, but in 64-bit, use u32 to type cast the address > of ioremap is not right, and accessing members of 'usb_sys_regs' will cause > call trace, so use unsigned long for both 32-bit and 64-bit. Wouldn't a (char *) cast be even better? David
On Thu, Nov 3, 2011 at 4:58 AM, Shaohui Xie <Shaohui.Xie@freescale.com> wrote: > > usb_sys_regs = (struct usb_sys_interface *) > - ((u32)dr_regs + USB_DR_SYS_OFFSET); > + ((unsigned long)dr_regs + USB_DR_SYS_OFFSET); This makes more sense: usb_sys_regs = (void *)dr_regs + USB_DR_SYS_OFFSET;
> On Thu, Nov 3, 2011 at 4:58 AM, Shaohui Xie <Shaohui.Xie@freescale.com> wrote: > > > > usb_sys_regs = (struct usb_sys_interface *) > > - ((u32)dr_regs + USB_DR_SYS_OFFSET); > > + ((unsigned long)dr_regs + USB_DR_SYS_OFFSET); > > This makes more sense: > > usb_sys_regs = (void *)dr_regs + USB_DR_SYS_OFFSET; But that is invalid C. David
David Laight wrote: >> > usb_sys_regs = (void *)dr_regs + USB_DR_SYS_OFFSET; > But that is invalid C. What's invalid about it? I haven't tried compiling this specific line of code, but I've done stuff like it in the past many times. Are you talking about adding an integer to a void pointer? If so, then that's something that gcc supports and that the kernel uses all over the place. A char* is incorrect because a char could be more than one byte, in theory.
> >> > usb_sys_regs = (void *)dr_regs + USB_DR_SYS_OFFSET; > > > But that is invalid C. > > What's invalid about it? I haven't tried compiling this > specific line of code, but I've done stuff like it in the past many times. > > Are you talking about adding an integer to a void pointer? > If so, then that's something that gcc supports and that the kernel uses > all over the place. Arithmetic on 'void *' should not be done. I know some versions of gcc allow it (provided some warning level/option is enabled) but that doesn't mean it is valid. My suspicions are that is was allowed due to the way 'void *' was originally bodged into gcc. > A char* is incorrect because a char could be more > than one byte, in theory. It is somewhat difficult to untangle the standard, but sizeof (char) is defined to be one. Of course, the C language doesn't actually require that you can converts between pointers to different types in any well-defined manner. But most of the low level device access assumes an adequately linear address space. David
On Nov 3, 2011, at 7:16 AM, David Laight <David.Laight@ACULAB.COM> wrote: > Arithmetic on 'void *' should not be done. I know some versions of > gcc allow it (provided some warning level/option is enabled) but > that doesn't mean it is valid. > My suspicions are that is was allowed due to the way 'void *' > was originally bodged into gcc. Well, I don't know what else to say. Arithmetic on void* is done all over the kernel. You're a little late to the party if you're going to advocate its avoidance.
> Arithmetic on 'void *' should not be done. I know some versions of > gcc allow it (provided some warning level/option is enabled) but > that doesn't mean it is valid. All relevant GCC versions support it (going back to 2.x at least). It is _always_ allowed, whatever compiler options you use; -pedantic or -Wpointer-arith will warn (which you can upgrade to an error). The Linux kernel is not built with these warnings enabled. >> A char* is incorrect because a char could be more >> than one byte, in theory. > > It is somewhat difficult to untangle the standard, but > sizeof (char) is defined to be one. A char takes exactly one byte. A byte could be more than eight bits, of course. In GCC, sizeof(void) is 1 as well. > Of course, the C language doesn't actually require that > you can converts between pointers to different types in > any well-defined manner. It does actually, see 6.3.2.3/7. You can convert any pointer to object to a pointer to a different object type, as long as it is properly aligned. You cannot in general access the object as that new type of course, but it is perfectly well-defined; in particular, you can convert it back to the original type and get the same value again, and you can walk consecutive bytes of the object by using a pointer to character type. Segher
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c index c81fbad..581b7cc 100644 --- a/drivers/usb/gadget/fsl_udc_core.c +++ b/drivers/usb/gadget/fsl_udc_core.c @@ -2498,7 +2498,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev) #ifndef CONFIG_ARCH_MXC if (pdata->have_sysif_regs) usb_sys_regs = (struct usb_sys_interface *) - ((u32)dr_regs + USB_DR_SYS_OFFSET); + ((unsigned long)dr_regs + USB_DR_SYS_OFFSET); #endif /* Initialize USB clocks */
Below are codes for accessing usb sysif_regs in driver: usb_sys_regs = (struct usb_sys_interface *) ((u32)dr_regs + USB_DR_SYS_OFFSET); these codes work in 32-bit, but in 64-bit, use u32 to type cast the address of ioremap is not right, and accessing members of 'usb_sys_regs' will cause call trace, so use unsigned long for both 32-bit and 64-bit. Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com> --- drivers/usb/gadget/fsl_udc_core.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)