2019-05-24 11:09:54

by Simon Sandström

Subject: [PATCH 3/4] staging: kpc2000: add missing spaces in core.c

Fixes checkpatch.pl errors "space required before the open brace '{'"
and "(foo*)" should be "(foo *)".

Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/kpc2000/kpc2000/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000/core.c b/drivers/staging/kpc2000/kpc2000/core.c
index 40f65f96986b..bb3b427a72b1 100644
--- a/drivers/staging/kpc2000/kpc2000/core.c
+++ b/drivers/staging/kpc2000/kpc2000/core.c
@@ -308,7 +308,7 @@ static long kp2000_cdev_ioctl(struct file *filp, unsigned int ioctl_num,

dev_dbg(&pcard->pdev->dev, "kp2000_cdev_ioctl(filp = [%p], ioctl_num = 0x%08x, ioctl_param = 0x%016lx) pcard = [%p]\n", filp, ioctl_num, ioctl_param, pcard);

- switch (ioctl_num){
+ switch (ioctl_num) {
case KP2000_IOCTL_GET_CPLD_REG: return readq(pcard->sysinfo_regs_base + REG_CPLD_CONFIG);
case KP2000_IOCTL_GET_PCIE_ERROR_REG: return readq(pcard->sysinfo_regs_base + REG_PCIE_ERROR_COUNT);

@@ -326,7 +326,7 @@ static long kp2000_cdev_ioctl(struct file *filp, unsigned int ioctl_num,
temp.ddna = pcard->ddna;
temp.cpld_reg = readq(pcard->sysinfo_regs_base + REG_CPLD_CONFIG);

- ret = copy_to_user((void*)ioctl_param, (void*)&temp, sizeof(temp));
+ ret = copy_to_user((void *)ioctl_param, (void *)&temp, sizeof(temp));
if (ret)
return -EFAULT;

--
2.20.1

2019-05-27 07:38:38

by Dan Carpenter

Subject: Re: [PATCH 3/4] staging: kpc2000: add missing spaces in core.c

On Fri, May 24, 2019 at 01:08:01PM +0200, Simon Sandstr?m wrote:
> Fixes checkpatch.pl errors "space required before the open brace '{'"
> and "(foo*)" should be "(foo *)".
>
> Signed-off-by: Simon Sandstr?m <[email protected]>
> ---
> drivers/staging/kpc2000/kpc2000/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/kpc2000/kpc2000/core.c b/drivers/staging/kpc2000/kpc2000/core.c
> index 40f65f96986b..bb3b427a72b1 100644
> --- a/drivers/staging/kpc2000/kpc2000/core.c
> +++ b/drivers/staging/kpc2000/kpc2000/core.c
> @@ -308,7 +308,7 @@ static long kp2000_cdev_ioctl(struct file *filp, unsigned int ioctl_num,
>
> dev_dbg(&pcard->pdev->dev, "kp2000_cdev_ioctl(filp = [%p], ioctl_num = 0x%08x, ioctl_param = 0x%016lx) pcard = [%p]\n", filp, ioctl_num, ioctl_param, pcard);
>
> - switch (ioctl_num){
> + switch (ioctl_num) {
> case KP2000_IOCTL_GET_CPLD_REG: return readq(pcard->sysinfo_regs_base + REG_CPLD_CONFIG);
> case KP2000_IOCTL_GET_PCIE_ERROR_REG: return readq(pcard->sysinfo_regs_base + REG_PCIE_ERROR_COUNT);
>
> @@ -326,7 +326,7 @@ static long kp2000_cdev_ioctl(struct file *filp, unsigned int ioctl_num,
> temp.ddna = pcard->ddna;
> temp.cpld_reg = readq(pcard->sysinfo_regs_base + REG_CPLD_CONFIG);
>
> - ret = copy_to_user((void*)ioctl_param, (void*)&temp, sizeof(temp));
> + ret = copy_to_user((void *)ioctl_param, (void *)&temp, sizeof(temp));
> if (ret)
> return -EFAULT;

This should really be written like so:

if (copy_to_user((void __user *)ioctl_param, &temp,
sizeof(temp)))
return -EFAULT;

temp is really the wrong name. "temp" is for temperatures. "tmp" means
temporary. But also "tmp" is wrong here because it's not a temporary
variable. It's better to call it "regs" here.

regards,
dan carpenter

2019-05-29 15:56:21

by Simon Sandström

Subject: Re: [PATCH 3/4] staging: kpc2000: add missing spaces in core.c

On Mon, May 27, 2019 at 10:31:59AM +0300, Dan Carpenter wrote:
> On Fri, May 24, 2019 at 01:08:01PM +0200, Simon Sandström wrote:
> > [..]
> > - ret = copy_to_user((void*)ioctl_param, (void*)&temp, sizeof(temp));
> > + ret = copy_to_user((void *)ioctl_param, (void *)&temp, sizeof(temp));
> > if (ret)
> > return -EFAULT;
>
> This should really be written like so:
>
> if (copy_to_user((void __user *)ioctl_param, &temp,
> sizeof(temp)))
> return -EFAULT;
>
> temp is really the wrong name. "temp" is for temperatures. "tmp" means
> temporary. But also "tmp" is wrong here because it's not a temporary
> variable. It's better to call it "regs" here.
>
> regards,
> dan carpenter
>

I agree, but I don't think it fits within this patch. I can send a
separate patch with this change.

Thanks

- Simon

2019-05-29 18:17:18

by Dan Carpenter

Subject: Re: [PATCH 3/4] staging: kpc2000: add missing spaces in core.c

On Wed, May 29, 2019 at 05:54:19PM +0200, Simon Sandstr?m wrote:
> On Mon, May 27, 2019 at 10:31:59AM +0300, Dan Carpenter wrote:
> > On Fri, May 24, 2019 at 01:08:01PM +0200, Simon Sandstr?m wrote:
> > > [..]
> > > - ret = copy_to_user((void*)ioctl_param, (void*)&temp, sizeof(temp));
> > > + ret = copy_to_user((void *)ioctl_param, (void *)&temp, sizeof(temp));
> > > if (ret)
> > > return -EFAULT;
> >
> > This should really be written like so:
> >
> > if (copy_to_user((void __user *)ioctl_param, &temp,
> > sizeof(temp)))
> > return -EFAULT;
> >
> > temp is really the wrong name. "temp" is for temperatures. "tmp" means
> > temporary. But also "tmp" is wrong here because it's not a temporary
> > variable. It's better to call it "regs" here.
> >
> > regards,
> > dan carpenter
> >
>
> I agree, but I don't think it fits within this patch. I can send a
> separate patch with this change.

You could send the other chunk as a separate patch, but I don't think it
makes sense to apply this chunk when really it just needs to be
re-written.

I normally don't complain too much about mechanical no-thought patches,
but in this case the function is very sub-par and should be re-written.

regards,
dan carpenter