Linux-RTC Archive on lore.kernel.org
 help / Atom feed
* [PATCH] rtc: rv8803: Check return value of rv8803_write_reg
@ 2018-12-27 20:28 Aditya Pakki
  2018-12-27 22:31 ` Heiner Kallweit
  2018-12-28  0:43 ` Alexandre Belloni
  0 siblings, 2 replies; 5+ messages in thread
From: Aditya Pakki @ 2018-12-27 20:28 UTC (permalink / raw)
  To: pakki001
  Cc: kjlu, Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-kernel

In rv8803_handle_irq, rv8803_write_reg can return a failed return
value when attempting to write to the bus. The fix checks the output
and throws a dev_warn notifying of the failure.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>
---
 drivers/rtc/rtc-rv8803.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
index 450a0b831a2d..5a19d5ecbf57 100644
--- a/drivers/rtc/rtc-rv8803.c
+++ b/drivers/rtc/rtc-rv8803.c
@@ -180,8 +180,13 @@ static irqreturn_t rv8803_handle_irq(int irq, void *dev_id)
 
 	if (events) {
 		rtc_update_irq(rv8803->rtc, 1, events);
-		rv8803_write_reg(client, RV8803_FLAG, flags);
-		rv8803_write_reg(rv8803->client, RV8803_CTRL, rv8803->ctrl);
+		if (rv8803_write_reg(client, RV8803_FLAG, flags))
+			dev_warn(&client->dev, "Failed to write RV8803 reg.\n");
+
+		if (rv8803_write_reg(rv8803->client, RV8803_CTRL,
+					rv8803->ctrl))
+			dev_warn(&rv8803->client->dev,
+				"Failed to write RV8803_CTRL reg.\n");
 	}
 
 	mutex_unlock(&rv8803->flags_lock);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: rv8803: Check return value of rv8803_write_reg
  2018-12-27 20:28 [PATCH] rtc: rv8803: Check return value of rv8803_write_reg Aditya Pakki
@ 2018-12-27 22:31 ` Heiner Kallweit
       [not found]   ` <CAK8KejrntOUb7Q2MrPR6mYDKyHKc=pzO5hOYv=ZfLaP_xg0Zsw@mail.gmail.com>
  2018-12-28  0:43 ` Alexandre Belloni
  1 sibling, 1 reply; 5+ messages in thread
From: Heiner Kallweit @ 2018-12-27 22:31 UTC (permalink / raw)
  To: Aditya Pakki
  Cc: kjlu, Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-kernel

On 27.12.2018 21:28, Aditya Pakki wrote:
> In rv8803_handle_irq, rv8803_write_reg can return a failed return
> value when attempting to write to the bus. The fix checks the output
> and throws a dev_warn notifying of the failure.
> 
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> ---
>  drivers/rtc/rtc-rv8803.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
You seem to submit the same type of changes throughout very
different subsystems. And you do it w/o thinking and testing.
If you would have looked at rv8803_write_reg() you would have
seen that it prints an error in case of failure. So your
patch achieves nothing.
You got David Miller upset already and it looks like you
want to achieve the same with other maintainers too.
I'd strongly suggest that you stop sending patches until
you better understand the kernel code.





^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: rv8803: Check return value of rv8803_write_reg
       [not found]   ` <CAK8KejrntOUb7Q2MrPR6mYDKyHKc=pzO5hOYv=ZfLaP_xg0Zsw@mail.gmail.com>
@ 2018-12-27 23:43     ` Heiner Kallweit
  2018-12-28  0:46     ` Alexandre Belloni
  1 sibling, 0 replies; 5+ messages in thread
From: Heiner Kallweit @ 2018-12-27 23:43 UTC (permalink / raw)
  To: Kangjie Lu
  Cc: Aditya Pakki, Alessandro Zummo, Alexandre Belloni, linux-rtc, open list

On 28.12.2018 00:28, Kangjie Lu wrote:
> 
> 
> On Thu, Dec 27, 2018 at 4:31 PM Heiner Kallweit <hkallweit1@gmail.com <mailto:hkallweit1@gmail.com>> wrote:
> 
>     On 27.12.2018 21:28, Aditya Pakki wrote:
>     > In rv8803_handle_irq, rv8803_write_reg can return a failed return
>     > value when attempting to write to the bus. The fix checks the output
>     > and throws a dev_warn notifying of the failure.
>     >
>     > Signed-off-by: Aditya Pakki <pakki001@umn.edu <mailto:pakki001@umn.edu>>
>     > ---
>     >  drivers/rtc/rtc-rv8803.c | 9 +++++++--
>     >  1 file changed, 7 insertions(+), 2 deletions(-)
>     >
>     You seem to submit the same type of changes throughout very
>     different subsystems. And you do it w/o thinking and testing.
>     If you would have looked at rv8803_write_reg() you would have
>     seen that it prints an error in case of failure. So your
>     patch achieves nothing.
>     You got David Miller upset already and it looks like you
>     want to achieve the same with other maintainers too.
>     I'd strongly suggest that you stop sending patches until
>     you better understand the kernel code.
> 
> 
> Hello Heiner,
> 
> Thanks for your suggestion. Sure, we will try to better understand
> how the kernel works when we are preparing other patches. We recently
> found a lot of potential bugs; due to the significant workload but 
> limited labor force, we may make some mistakes, but yes, we will try 
> to avoid them. 
> 
> One main reason we submit the patches is to seek feedback from Linux
> maintainers who know how the kernel works best.  We hope to get: (1) 
> confirmation: if this is indeed a bug; (2) improvement feedback: if
> it is a bug and our fix is problematic, how can we improve it? 
> 
> Taking the case in this email as an example, rv8803_write_reg could
> fail, so returning IRQ_HANDLED even when it failed doesn't seem to be
> a good practice. Would "returning IRQ_NONE upon failure" be a better
> fix?
> 
> Thanks again for your suggestion.

If you want to understand whether a certain code piece includes a bug
or not, first you need to understand what the code does. In addition
to own analysis you could post a question to the respective mailing
list.
Maintainers usually have a tough workload and can't explain to every
beginner what the code in their respective subsystem does. It's not
the smartest approach to bother and upset them with dumb patches.

Last but not least: You seem to submit all your patches w/o even
compile-testing. Also that's not a smart approach.
Read "SubmittingPatches" carefully, understand it, and follow the
rules.

Heiner

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: rv8803: Check return value of rv8803_write_reg
  2018-12-27 20:28 [PATCH] rtc: rv8803: Check return value of rv8803_write_reg Aditya Pakki
  2018-12-27 22:31 ` Heiner Kallweit
@ 2018-12-28  0:43 ` Alexandre Belloni
  1 sibling, 0 replies; 5+ messages in thread
From: Alexandre Belloni @ 2018-12-28  0:43 UTC (permalink / raw)
  To: Aditya Pakki; +Cc: kjlu, Alessandro Zummo, linux-rtc, linux-kernel

On 27/12/2018 14:28:55-0600, Aditya Pakki wrote:
> In rv8803_handle_irq, rv8803_write_reg can return a failed return
> value when attempting to write to the bus. The fix checks the output
> and throws a dev_warn notifying of the failure.
> 

Is there any point in doing that as the error will self correct later
anyway?

I really doubt there is any user reading the logs on the systems with an
rv8803 and there is no user action needed anyway.

> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> ---
>  drivers/rtc/rtc-rv8803.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
> index 450a0b831a2d..5a19d5ecbf57 100644
> --- a/drivers/rtc/rtc-rv8803.c
> +++ b/drivers/rtc/rtc-rv8803.c
> @@ -180,8 +180,13 @@ static irqreturn_t rv8803_handle_irq(int irq, void *dev_id)
>  
>  	if (events) {
>  		rtc_update_irq(rv8803->rtc, 1, events);
> -		rv8803_write_reg(client, RV8803_FLAG, flags);
> -		rv8803_write_reg(rv8803->client, RV8803_CTRL, rv8803->ctrl);
> +		if (rv8803_write_reg(client, RV8803_FLAG, flags))
> +			dev_warn(&client->dev, "Failed to write RV8803 reg.\n");
> +
> +		if (rv8803_write_reg(rv8803->client, RV8803_CTRL,
> +					rv8803->ctrl))
> +			dev_warn(&rv8803->client->dev,
> +				"Failed to write RV8803_CTRL reg.\n");
>  	}
>  
>  	mutex_unlock(&rv8803->flags_lock);
> -- 
> 2.17.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: rv8803: Check return value of rv8803_write_reg
       [not found]   ` <CAK8KejrntOUb7Q2MrPR6mYDKyHKc=pzO5hOYv=ZfLaP_xg0Zsw@mail.gmail.com>
  2018-12-27 23:43     ` Heiner Kallweit
@ 2018-12-28  0:46     ` Alexandre Belloni
  1 sibling, 0 replies; 5+ messages in thread
From: Alexandre Belloni @ 2018-12-28  0:46 UTC (permalink / raw)
  To: Kangjie Lu
  Cc: Heiner Kallweit, Aditya Pakki, Alessandro Zummo, linux-rtc, open list

On 27/12/2018 17:28:33-0600, Kangjie Lu wrote:
> On Thu, Dec 27, 2018 at 4:31 PM Heiner Kallweit <hkallweit1@gmail.com>
> wrote:
> 
> > On 27.12.2018 21:28, Aditya Pakki wrote:
> > > In rv8803_handle_irq, rv8803_write_reg can return a failed return
> > > value when attempting to write to the bus. The fix checks the output
> > > and throws a dev_warn notifying of the failure.
> > >
> > > Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> > > ---
> > >  drivers/rtc/rtc-rv8803.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > You seem to submit the same type of changes throughout very
> > different subsystems. And you do it w/o thinking and testing.
> > If you would have looked at rv8803_write_reg() you would have
> > seen that it prints an error in case of failure. So your
> > patch achieves nothing.
> > You got David Miller upset already and it looks like you
> > want to achieve the same with other maintainers too.
> > I'd strongly suggest that you stop sending patches until
> > you better understand the kernel code.
> >
> 
> Hello Heiner,
> 
> Thanks for your suggestion. Sure, we will try to better understand
> how the kernel works when we are preparing other patches. We recently
> found a lot of potential bugs; due to the significant workload but
> limited labor force, we may make some mistakes, but yes, we will try
> to avoid them.
> 
> One main reason we submit the patches is to seek feedback from Linux
> maintainers who know how the kernel works best.  We hope to get: (1)
> confirmation: if this is indeed a bug;

Come on, this is your job, not the maintainer job to check whether there
is indeed a bug. Else, the maintainer may as well just remove your
authorship because he did all the real work.

> (2) improvement feedback: if
> it is a bug and our fix is problematic, how can we improve it?
> 
> Taking the case in this email as an example, rv8803_write_reg could
> fail, so returning IRQ_HANDLED even when it failed doesn't seem to be
> a good practice. Would "returning IRQ_NONE upon failure" be a better
> fix?
> 
> Thanks again for your suggestion.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-27 20:28 [PATCH] rtc: rv8803: Check return value of rv8803_write_reg Aditya Pakki
2018-12-27 22:31 ` Heiner Kallweit
     [not found]   ` <CAK8KejrntOUb7Q2MrPR6mYDKyHKc=pzO5hOYv=ZfLaP_xg0Zsw@mail.gmail.com>
2018-12-27 23:43     ` Heiner Kallweit
2018-12-28  0:46     ` Alexandre Belloni
2018-12-28  0:43 ` Alexandre Belloni

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org linux-rtc@archiver.kernel.org
	public-inbox-index linux-rtc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox