On Sun, Jan 23, 2011 at 05:34:46PM +0100, Javier Martinez Canillas wrote: > RxCntrlMsgBitMask is of type unsigned long so it is wrong > to copy InputLenght bytes. Only we have to copy sizeof(unsigned long) bytes. ^^ Typo. > > This patch solves the issue and also make some style cleanups. > > Signed-off-by: Javier Martinez Canillas <martinez.javier@xxxxxxxxx> > --- > drivers/staging/bcm/Bcmchar.c | 44 ++++++++++++++++++++++------------------ > 1 files changed, 24 insertions(+), 20 deletions(-) > > diff --git a/drivers/staging/bcm/Bcmchar.c b/drivers/staging/bcm/Bcmchar.c > index 31674ea..c78afe8 100644 > --- a/drivers/staging/bcm/Bcmchar.c > +++ b/drivers/staging/bcm/Bcmchar.c > @@ -2015,28 +2015,32 @@ static long bcm_char_ioctl(struct file *filp, UINT cmd, ULONG arg) > break ; > } > > - case IOCTL_BCM_CNTRLMSG_MASK: > - { > - ULONG RxCntrlMsgBitMask = 0 ; > + case IOCTL_BCM_CNTRLMSG_MASK: > + { > + ULONG RxCntrlMsgBitMask = 0; The code inside the block should be indented one level. Please don't use ULONG in new code, use "unsigned long". > > - /* Copy Ioctl Buffer structure */ > - Status = copy_from_user(&IoBuffer, argp, sizeof(IOCTL_BUFFER)); > - if(Status) > - { > - BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"copy of Ioctl buffer is failed from user space"); > - break; > - } > + /* Copy Ioctl Buffer structure */ > + Status = copy_from_user(&IoBuffer, argp, sizeof(IOCTL_BUFFER)); > + if (Status) { > + BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL, > + "copy of Ioctl buffer is failed from user space"); > + break; copy_from_user() returns the number of bytes remaining. We should return -EFAULT here. > + } > > - Status = copy_from_user(&RxCntrlMsgBitMask, IoBuffer.InputBuffer, IoBuffer.InputLength); > - if(Status) > - { > - BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"copy of control bit mask failed from user space"); > - break; > - } > - BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"\n Got user defined cntrl msg bit mask :%lx", RxCntrlMsgBitMask); > - pTarang->RxCntrlMsgBitMask = RxCntrlMsgBitMask ; > - } > - break; > + Status = copy_from_user(&RxCntrlMsgBitMask, IoBuffer.InputBuffer, > + sizeof(ULONG)); > + if (Status) { > + BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL, > + "copy of control bit mask failed from user space"); Same here. Status = -EFAULT; > + break; > + } > + > + BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL, > + "\n Got user defined cntrl msg bit mask :%lx", RxCntrlMsgBitMask); This debug output is oddly formatted. Why is there a "\n " at the start? Btw, can't we just get rid of all these useless debug output? We could add printks() on test systems if things are broken. To me it's like buying a new car from the dealership and immediately covering it in duct tape. We could debate if it makes the car stronger but it's definitely ugly. > + > + pTarang->RxCntrlMsgBitMask = RxCntrlMsgBitMask ; ^ Extra space here. > + } > + break; > case IOCTL_BCM_GET_DEVICE_DRIVER_INFO: > { > DEVICE_DRIVER_INFO DevInfo; > -- > 1.7.0.4 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel