Message ID | 20190123230634.1815021-1-vijaykhemka@fb.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | misc: aspeed-lpc-ctrl: Correct return values | expand |
On Wed, Jan 23, 2019 at 03:06:34PM -0800, Vijay Khemka wrote: > Corrected some of return values with appropriate meanings. > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > --- > drivers/misc/aspeed-lpc-ctrl.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c > index 332210e06e98..97ae341109d5 100644 > --- a/drivers/misc/aspeed-lpc-ctrl.c > +++ b/drivers/misc/aspeed-lpc-ctrl.c > @@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, > unsigned long param) > { > struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file); > - struct device *dev = file->private_data; > void __user *p = (void __user *)param; > struct aspeed_lpc_ctrl_mapping map; > u32 addr; > @@ -93,8 +92,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, > > /* If memory-region is not described in device tree */ > if (!lpc_ctrl->mem_size) { > - dev_err(dev, "Didn't find reserved memory\n"); > - return -EINVAL; > + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find reserved memory\n"); Why did you change from dev_err() to pr_err()? You just lost a lot of information that the user previously was getting from dev_err() :( > + return -ENXIO; > } > > map.size = lpc_ctrl->mem_size; > @@ -134,16 +133,16 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, > > if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) { > if (!lpc_ctrl->pnor_size) { > - dev_err(dev, "Didn't find host pnor flash\n"); > - return -EINVAL; > + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find host pnor flash\n"); Again, don't do that :( thanks, greg k-h
On 1/24/19, 12:16 AM, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> wrote: On Wed, Jan 23, 2019 at 03:06:34PM -0800, Vijay Khemka wrote: > Corrected some of return values with appropriate meanings. > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > --- > drivers/misc/aspeed-lpc-ctrl.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c > index 332210e06e98..97ae341109d5 100644 > --- a/drivers/misc/aspeed-lpc-ctrl.c > +++ b/drivers/misc/aspeed-lpc-ctrl.c > @@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, > unsigned long param) > { > struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file); > - struct device *dev = file->private_data; > void __user *p = (void __user *)param; > struct aspeed_lpc_ctrl_mapping map; > u32 addr; > @@ -93,8 +92,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, > > /* If memory-region is not described in device tree */ > if (!lpc_ctrl->mem_size) { > - dev_err(dev, "Didn't find reserved memory\n"); > - return -EINVAL; > + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find reserved memory\n"); Why did you change from dev_err() to pr_err()? You just lost a lot of information that the user previously was getting from dev_err() :( I did this as per review comment from Andrew Jeffery, as we don't want to put this error for driver. It has to be handled by userspace. But I am still reporting some error here. > + return -ENXIO; > } > > map.size = lpc_ctrl->mem_size; > @@ -134,16 +133,16 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, > > if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) { > if (!lpc_ctrl->pnor_size) { > - dev_err(dev, "Didn't find host pnor flash\n"); > - return -EINVAL; > + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find host pnor flash\n"); Again, don't do that :( thanks, greg k-h
On Fri, 25 Jan 2019, at 05:59, Vijay Khemka wrote: > > > On 1/24/19, 12:16 AM, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> wrote: > > On Wed, Jan 23, 2019 at 03:06:34PM -0800, Vijay Khemka wrote: > > Corrected some of return values with appropriate meanings. > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > --- > > drivers/misc/aspeed-lpc-ctrl.c | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed- > lpc-ctrl.c > > index 332210e06e98..97ae341109d5 100644 > > --- a/drivers/misc/aspeed-lpc-ctrl.c > > +++ b/drivers/misc/aspeed-lpc-ctrl.c > > @@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file > *file, unsigned int cmd, > > unsigned long param) > > { > > struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file); > > - struct device *dev = file->private_data; > > void __user *p = (void __user *)param; > > struct aspeed_lpc_ctrl_mapping map; > > u32 addr; > > @@ -93,8 +92,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file > *file, unsigned int cmd, > > > > /* If memory-region is not described in device tree */ > > if (!lpc_ctrl->mem_size) { > > - dev_err(dev, "Didn't find reserved memory\n"); > > - return -EINVAL; > > + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find reserved memory\n"); > > Why did you change from dev_err() to pr_err()? You just lost a lot of > information that the user previously was getting from dev_err() :( > > I did this as per review comment from Andrew Jeffery, as we don't want > to put this error for driver. It has to be handled by userspace. But I > am still reporting some error here. Sorry? What I was trying to suggest was removing the logging altogether and just returning the error code. Not simply changing how the logging is done. Andrew
On 2/10/19, 9:22 PM, "Andrew Jeffery" <andrew@aj.id.au> wrote: On Fri, 25 Jan 2019, at 05:59, Vijay Khemka wrote: > > > On 1/24/19, 12:16 AM, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> wrote: > > On Wed, Jan 23, 2019 at 03:06:34PM -0800, Vijay Khemka wrote: > > Corrected some of return values with appropriate meanings. > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > --- > > drivers/misc/aspeed-lpc-ctrl.c | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed- > lpc-ctrl.c > > index 332210e06e98..97ae341109d5 100644 > > --- a/drivers/misc/aspeed-lpc-ctrl.c > > +++ b/drivers/misc/aspeed-lpc-ctrl.c > > @@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file > *file, unsigned int cmd, > > unsigned long param) > > { > > struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file); > > - struct device *dev = file->private_data; > > void __user *p = (void __user *)param; > > struct aspeed_lpc_ctrl_mapping map; > > u32 addr; > > @@ -93,8 +92,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file > *file, unsigned int cmd, > > > > /* If memory-region is not described in device tree */ > > if (!lpc_ctrl->mem_size) { > > - dev_err(dev, "Didn't find reserved memory\n"); > > - return -EINVAL; > > + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find reserved memory\n"); > > Why did you change from dev_err() to pr_err()? You just lost a lot of > information that the user previously was getting from dev_err() :( > > I did this as per review comment from Andrew Jeffery, as we don't want > to put this error for driver. It has to be handled by userspace. But I > am still reporting some error here. Sorry? What I was trying to suggest was removing the logging altogether and just returning the error code. Not simply changing how the logging is done. No issue, I can remove logging, will just return error code. Will send new patch with update. Andrew
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c index 332210e06e98..97ae341109d5 100644 --- a/drivers/misc/aspeed-lpc-ctrl.c +++ b/drivers/misc/aspeed-lpc-ctrl.c @@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, unsigned long param) { struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file); - struct device *dev = file->private_data; void __user *p = (void __user *)param; struct aspeed_lpc_ctrl_mapping map; u32 addr; @@ -93,8 +92,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, /* If memory-region is not described in device tree */ if (!lpc_ctrl->mem_size) { - dev_err(dev, "Didn't find reserved memory\n"); - return -EINVAL; + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find reserved memory\n"); + return -ENXIO; } map.size = lpc_ctrl->mem_size; @@ -134,16 +133,16 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) { if (!lpc_ctrl->pnor_size) { - dev_err(dev, "Didn't find host pnor flash\n"); - return -EINVAL; + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find host pnor flash\n"); + return -ENXIO; } addr = lpc_ctrl->pnor_base; size = lpc_ctrl->pnor_size; } else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) { /* If memory-region is not described in device tree */ if (!lpc_ctrl->mem_size) { - dev_err(dev, "Didn't find reserved memory\n"); - return -EINVAL; + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find reserved memory\n"); + return -ENXIO; } addr = lpc_ctrl->mem_base; size = lpc_ctrl->mem_size; @@ -239,7 +238,7 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev) of_node_put(node); if (rc) { dev_err(dev, "Couldn't address to resource for reserved memory\n"); - return -ENOMEM; + return -ENXIO; } lpc_ctrl->mem_size = resource_size(&resm);
Corrected some of return values with appropriate meanings. Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> --- drivers/misc/aspeed-lpc-ctrl.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)