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
