Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
On 5/1/19, 11:49 PM, "Andrew Jeffery" wrote:
On Thu, 2 May 2019, at 16:10, Greg Kroah-Hartman wrote:
> On Wed, May 01, 2019 at 03:38:36PM -0700, Vijay Khemka wrote:
> > Corrected some of return values with appropriate meanings.
> >
> > Signed-off-by: Vijay Khemka
> > ---
> > 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;
>
> This change is not reflected in your changelog text :(
>
> Please fix up, or break this up into multiple patches.
The return value fixes should also be squashed into the patch that
introduced those lines
given it hasn't yet been applied.
Further, IIRC I previously suggested removing the dev_err()s entirely, not
just switching
them to pr_err(). Returning an error code is enough IMO, there's no need to
pollute the
kernel logs with application-level errors. Or make them dev_dbg().
Alright, I will replace with dev_dbg(), information can still be extracted by
user with debug.
Andrew
>
> greg k-h
>
Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
On Thu, 2 May 2019, at 16:10, Greg Kroah-Hartman wrote:
> On Wed, May 01, 2019 at 03:38:36PM -0700, Vijay Khemka wrote:
> > Corrected some of return values with appropriate meanings.
> >
> > Signed-off-by: Vijay Khemka
> > ---
> > 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;
>
> This change is not reflected in your changelog text :(
>
> Please fix up, or break this up into multiple patches.
The return value fixes should also be squashed into the patch that introduced
those lines
given it hasn't yet been applied.
Further, IIRC I previously suggested removing the dev_err()s entirely, not just
switching
them to pr_err(). Returning an error code is enough IMO, there's no need to
pollute the
kernel logs with application-level errors. Or make them dev_dbg().
Andrew
>
> greg k-h
>
Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
On Wed, May 01, 2019 at 03:38:36PM -0700, Vijay Khemka wrote:
> Corrected some of return values with appropriate meanings.
>
> Signed-off-by: Vijay Khemka
> ---
> 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;
This change is not reflected in your changelog text :(
Please fix up, or break this up into multiple patches.
greg k-h
[PATCH] misc: aspeed-lpc-ctrl: Correct return values
Corrected some of return values with appropriate meanings.
Signed-off-by: Vijay Khemka
---
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");
+ 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();
--
2.17.1
Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
On 2/10/19, 9:22 PM, "Andrew Jeffery" wrote:
On Fri, 25 Jan 2019, at 05:59, Vijay Khemka wrote:
>
>
> On 1/24/19, 12:16 AM, "Greg Kroah-Hartman"
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
> > ---
> > 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
Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
On Fri, 25 Jan 2019, at 05:59, Vijay Khemka wrote:
>
>
> On 1/24/19, 12:16 AM, "Greg Kroah-Hartman"
> 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
> > ---
> > 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
Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
On 1/24/19, 12:16 AM, "Greg Kroah-Hartman" 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
> ---
> 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
Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
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
> ---
> 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
[PATCH] misc: aspeed-lpc-ctrl: Correct return values
Corrected some of return values with appropriate meanings.
Signed-off-by: Vijay Khemka
---
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");
+ 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();
--
2.17.1
