Re: [PATCH] arm: fault.c: fix unhandled page fault message

[Levente Kurusa]

Hi,

- Original Message -
> On Wed, Jul 15, 2015 at 05:30:50PM +0200, Levente Kurusa wrote:
> > Even if the signal was handled using signal(2) the message
> > would be printed. Fix that by checking whether the signal
> > is handled.
> 
> Why?

One of the reasons is that arm64 prints the same message only when the signal
is unhandled.

The other is the message saying "unhandled". :-)

But, don't get me wrong, I found the 'problem' by having a quick glimpse at the 
code
(even though I have a testcase now...), so if you think this is right this way,
then so be it.

> 
> Even if the application handles the signal, the point of this debugging is
> to have the kernel report the reason for the fault.
> 
> Just because the application has installed a SIGSEGV handler to print some
> nice "oops" message, and to cleanly shut down (eg, like Xorg) doesn't mean
> we should hide this debugging.  In fact, as such handlers generally get in
> the way of getting a decent dump from the application, having the kernel
> report this information is even more valuable in this situation.

I agree, but I find this being controlled by a kernel config option _and_ a
parameter makes it harder to use. Maybe we could switch to the sysctl,
"debug.exception-trace" like some other architectures do? What do you think?

Thanks,
Levente

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] arm: fault.c: fix unhandled page fault message

[Levente Kurusa]

Hi,

- Original Message -
 On Wed, Jul 15, 2015 at 05:30:50PM +0200, Levente Kurusa wrote:
  Even if the signal was handled using signal(2) the message
  would be printed. Fix that by checking whether the signal
  is handled.
 
 Why?

One of the reasons is that arm64 prints the same message only when the signal
is unhandled.

The other is the message saying unhandled. :-)

But, don't get me wrong, I found the 'problem' by having a quick glimpse at the 
code
(even though I have a testcase now...), so if you think this is right this way,
then so be it.

 
 Even if the application handles the signal, the point of this debugging is
 to have the kernel report the reason for the fault.
 
 Just because the application has installed a SIGSEGV handler to print some
 nice oops message, and to cleanly shut down (eg, like Xorg) doesn't mean
 we should hide this debugging.  In fact, as such handlers generally get in
 the way of getting a decent dump from the application, having the kernel
 report this information is even more valuable in this situation.

I agree, but I find this being controlled by a kernel config option _and_ a
parameter makes it harder to use. Maybe we could switch to the sysctl,
debug.exception-trace like some other architectures do? What do you think?

Thanks,
Levente

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] arm: fault.c: fix unhandled page fault message

[Levente Kurusa]

Even if the signal was handled using signal(2) the message
would be printed. Fix that by checking whether the signal
is handled.

Signed-off-by: Levente Kurusa 
---
 arch/arm/mm/fault.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 0d629b8..bbf5d73 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -164,8 +164,9 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr,
struct siginfo si;
 
 #ifdef CONFIG_DEBUG_USER
-   if (((user_debug & UDBG_SEGV) && (sig == SIGSEGV)) ||
-   ((user_debug & UDBG_BUS)  && (sig == SIGBUS))) {
+   if user_debug & UDBG_SEGV) && (sig == SIGSEGV)) ||
+((user_debug & UDBG_BUS)  && (sig == SIGBUS))) &&
+unhandled_signal(tsk, sig)) {
printk(KERN_DEBUG "%s: unhandled page fault (%d) at 0x%08lx, 
code 0x%03x\n",
   tsk->comm, sig, addr, fsr);
show_pte(tsk->mm, addr);
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] arm: fault.c: fix unhandled page fault message

[Levente Kurusa]

Even if the signal was handled using signal(2) the message
would be printed. Fix that by checking whether the signal
is handled.

Signed-off-by: Levente Kurusa lkur...@redhat.com
---
 arch/arm/mm/fault.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 0d629b8..bbf5d73 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -164,8 +164,9 @@ __do_user_fault(struct task_struct *tsk, unsigned long addr,
struct siginfo si;
 
 #ifdef CONFIG_DEBUG_USER
-   if (((user_debug  UDBG_SEGV)  (sig == SIGSEGV)) ||
-   ((user_debug  UDBG_BUS)   (sig == SIGBUS))) {
+   if user_debug  UDBG_SEGV)  (sig == SIGSEGV)) ||
+((user_debug  UDBG_BUS)   (sig == SIGBUS))) 
+unhandled_signal(tsk, sig)) {
printk(KERN_DEBUG %s: unhandled page fault (%d) at 0x%08lx, 
code 0x%03x\n,
   tsk-comm, sig, addr, fsr);
show_pte(tsk-mm, addr);
-- 
2.4.3

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] lib/kstrtox.c break if overflow is detected

[Levente Kurusa]

Hi,

On Wed, Jan 21, 2015 at 11:26:26AM -0800, Anshul Garg wrote:
> From: Anshul Garg 
> 
> 1. While converting string representation to integer
> break the loop if overflow is detected.
> 2. Clean kstrtoll function
> 
> Signed-off-by: Anshul Garg 
> ---
>  lib/kstrtox.c |   28 +---
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index ec8da78..8cbe5ca 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -70,8 +70,10 @@ unsigned int _parse_integer(const char *s, unsigned int 
> base, unsigned long long
>* it in the max base we support (16)
>*/
>   if (unlikely(res & (~0ull << 60))) {
> - if (res > div_u64(ULLONG_MAX - val, base))
> + if (res > div_u64(ULLONG_MAX - val, base)) {
>   overflow = 1;
> + break;
> + }
>   }
>   res = res * base + val;
>   rv++;
> @@ -146,23 +148,19 @@ EXPORT_SYMBOL(kstrtoull);
>  int kstrtoll(const char *s, unsigned int base, long long *res)
>  {
>   unsigned long long tmp;
> - int rv;
> + int rv, sign = 1;
>  
>   if (s[0] == '-') {
> - rv = _kstrtoull(s + 1, base, );
> - if (rv < 0)
> - return rv;
> - if ((long long)(-tmp) >= 0)
> - return -ERANGE;
> - *res = -tmp;
> - } else {
> - rv = kstrtoull(s, base, );
> - if (rv < 0)
> - return rv;
> - if ((long long)tmp < 0)
> - return -ERANGE;
> - *res = tmp;
> + sign = -1;
> + s++;
>   }
> +
> + rv = kstrtoull(s, base, );
> + if (rv < 0)
> +     return rv;
> + if ((long long)tmp < 0)
> + return -ERANGE;
> + *res = sign * tmp;
>   return 0;
>  }
>  EXPORT_SYMBOL(kstrtoll);

Looks correct to me, so:

Reviewed-by: Levente Kurusa 

But I believe the two hunks are completely unrelated to
eachother and hence should split into a patch series.

Could you please do that and resend? Or if Andrew is OK
with picking it up as is, then it's fine.

Thanks,
Levente.


signature.asc
Description: Digital signature

Re: [PATCH] lib/kstrtox.c break if overflow is detected

[Levente Kurusa]

Hi,

On Wed, Jan 21, 2015 at 11:26:26AM -0800, Anshul Garg wrote:
 From: Anshul Garg aksgarg1...@gmail.com
 
 1. While converting string representation to integer
 break the loop if overflow is detected.
 2. Clean kstrtoll function
 
 Signed-off-by: Anshul Garg aksgarg1...@gmail.com
 ---
  lib/kstrtox.c |   28 +---
  1 file changed, 13 insertions(+), 15 deletions(-)
 
 diff --git a/lib/kstrtox.c b/lib/kstrtox.c
 index ec8da78..8cbe5ca 100644
 --- a/lib/kstrtox.c
 +++ b/lib/kstrtox.c
 @@ -70,8 +70,10 @@ unsigned int _parse_integer(const char *s, unsigned int 
 base, unsigned long long
* it in the max base we support (16)
*/
   if (unlikely(res  (~0ull  60))) {
 - if (res  div_u64(ULLONG_MAX - val, base))
 + if (res  div_u64(ULLONG_MAX - val, base)) {
   overflow = 1;
 + break;
 + }
   }
   res = res * base + val;
   rv++;
 @@ -146,23 +148,19 @@ EXPORT_SYMBOL(kstrtoull);
  int kstrtoll(const char *s, unsigned int base, long long *res)
  {
   unsigned long long tmp;
 - int rv;
 + int rv, sign = 1;
  
   if (s[0] == '-') {
 - rv = _kstrtoull(s + 1, base, tmp);
 - if (rv  0)
 - return rv;
 - if ((long long)(-tmp) = 0)
 - return -ERANGE;
 - *res = -tmp;
 - } else {
 - rv = kstrtoull(s, base, tmp);
 - if (rv  0)
 - return rv;
 - if ((long long)tmp  0)
 - return -ERANGE;
 - *res = tmp;
 + sign = -1;
 + s++;
   }
 +
 + rv = kstrtoull(s, base, tmp);
 + if (rv  0)
 + return rv;
 + if ((long long)tmp  0)
 + return -ERANGE;
 + *res = sign * tmp;
   return 0;
  }
  EXPORT_SYMBOL(kstrtoll);

Looks correct to me, so:

Reviewed-by: Levente Kurusa le...@linux.com

But I believe the two hunks are completely unrelated to
eachother and hence should split into a patch series.

Could you please do that and resend? Or if Andrew is OK
with picking it up as is, then it's fine.

Thanks,
Levente.


signature.asc
Description: Digital signature

Re: [PATCH] dma: xilinx: Remove .owner field for driver

[Levente Kurusa]

2014-08-13 13:57 GMT+02:00 Michal Simek :
> There is no need to init .owner field.
>
> Based on the patch from Peter Griffin 
> "mmc: remove .owner field for drivers using module_platform_driver"
>
> This patch removes the superflous .owner field for drivers which
> use the module_platform_driver API, as this is overriden in
> platform_driver_register anyway."
>
> Signed-off-by: Michal Simek 

Reviewed-by: Levente Kurusa 

Cheers,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] dma: xilinx: Remove .owner field for driver

[Levente Kurusa]

2014-08-13 13:57 GMT+02:00 Michal Simek michal.si...@xilinx.com:
 There is no need to init .owner field.

 Based on the patch from Peter Griffin peter.grif...@linaro.org
 mmc: remove .owner field for drivers using module_platform_driver

 This patch removes the superflous .owner field for drivers which
 use the module_platform_driver API, as this is overriden in
 platform_driver_register anyway.

 Signed-off-by: Michal Simek michal.si...@xilinx.com

Reviewed-by: Levente Kurusa lkur...@redhat.com

Cheers,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] edac: Remove fixmes in e7xxx_edac.c

[Levente Kurusa]

2014-07-22 19:40 GMT+02:00 Nick Krause :
> [...]
>
> Tony,
>
> What is the value of Page shift then ? If you can tell me it would be
> much easier for
> me to fix this.
> Cheers Nick

Nick,

Help is always welcome. However, what you are doing is effectively a
waste of time for you and for the maintainers as well, and hence cannot
be called help. You are writing emails that make absolutely no sense at all.
If you want to help us out, please consider learning three things:

* The language that the kernel is written in: C
* The language that most kernel developers understand: English
* The environment the kernel is (in most cases) written in: UNIX

If you had known these, then:

* ... you would have already known that FIXMEs exist for a reason which is
   not at all simple.

* ... you would have understood that fixing FIXMEs without ANY reasoning and
   asking about it is completely useless and wastes precious maintainer time.

* ... you could have used grep for finding out PAGE_SHIFT and not need to ask
   for it.

Nick. Look, it is good that you are enthusiastic. However, this kind
of 'work' gives
you an undesirable reputation, which is {IF FROM:xerofoify@* THEN TRASH}...

If you really want to help out, I suggest you to find yourself a piece
of hardware from
the store and try to write a driver for it. That way, you will learn
how it all works, and if
you are intelligent enough then you might get a "Kernel Job - Payed"
like you asked us
for them in November.

Until then, please stop trying for FIXMEs. They will not get you a
commit in the kernel
99% of the time.

Hope you understand,

Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] edac: Remove fixmes in e7xxx_edac.c

[Levente Kurusa]

2014-07-22 19:40 GMT+02:00 Nick Krause xerofo...@gmail.com:
 [...]

 Tony,

 What is the value of Page shift then ? If you can tell me it would be
 much easier for
 me to fix this.
 Cheers Nick

Nick,

Help is always welcome. However, what you are doing is effectively a
waste of time for you and for the maintainers as well, and hence cannot
be called help. You are writing emails that make absolutely no sense at all.
If you want to help us out, please consider learning three things:

* The language that the kernel is written in: C
* The language that most kernel developers understand: English
* The environment the kernel is (in most cases) written in: UNIX

If you had known these, then:

* ... you would have already known that FIXMEs exist for a reason which is
   not at all simple.

* ... you would have understood that fixing FIXMEs without ANY reasoning and
   asking about it is completely useless and wastes precious maintainer time.

* ... you could have used grep for finding out PAGE_SHIFT and not need to ask
   for it.

Nick. Look, it is good that you are enthusiastic. However, this kind
of 'work' gives
you an undesirable reputation, which is {IF FROM:xerofoify@* THEN TRASH}...

If you really want to help out, I suggest you to find yourself a piece
of hardware from
the store and try to write a driver for it. That way, you will learn
how it all works, and if
you are intelligent enough then you might get a Kernel Job - Payed
like you asked us
for them in November.

Until then, please stop trying for FIXMEs. They will not get you a
commit in the kernel
99% of the time.

Hope you understand,

Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [media] davinci-vpfe: Fix retcode check

[Levente Kurusa]

2014-07-08 16:08 GMT+02:00 Andrey Utkin :
> Use signed type to check correctly for negative error code. The issue
> was reported with static analyser:
>
> [linux-3.13/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c:270]:
> (style) A pointer can not be negative so it is either pointless or an
> error to check if it is.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=69071
> Reported-by: David Binderman 
> Signed-off-by: Andrey Utkin 

Hmm, while it is true that get_ipipe_mode returns an int, but
the consequent call to regw_ip takes an u32 as its second
argument. Did it cause a build warning for you? (Can't really
check since I don't have ARM cross compilers close-by)
If not, then:

Reviewed-by: Levente Kurusa 

Thanks,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [media] davinci-vpfe: Fix retcode check

[Levente Kurusa]

2014-07-08 16:08 GMT+02:00 Andrey Utkin andrey.krieger.ut...@gmail.com:
 Use signed type to check correctly for negative error code. The issue
 was reported with static analyser:

 [linux-3.13/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c:270]:
 (style) A pointer can not be negative so it is either pointless or an
 error to check if it is.

 Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=69071
 Reported-by: David Binderman dcb...@hotmail.com
 Signed-off-by: Andrey Utkin andrey.krieger.ut...@gmail.com

Hmm, while it is true that get_ipipe_mode returns an int, but
the consequent call to regw_ip takes an u32 as its second
argument. Did it cause a build warning for you? (Can't really
check since I don't have ARM cross compilers close-by)
If not, then:

Reviewed-by: Levente Kurusa lkur...@redhat.com

Thanks,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Cleanup of Kernel Bugzilla

[Levente Kurusa]

Hi,

[Why was stable on Cc?]

On 06/29/2014 08:33 AM, Gideon D'souza wrote:

Why do you want an up to date list of kernel bugs?


Even I'm a newbie and was looking for the same thing. I read
eventually somewhere that bugzilla isn't maintained by kernel devs so
I entirely gave up trying to find a bug to adopt.

Many open source projects I've worked on (web frameworks mostly) have
a well maintained list of bugs and tag the right ones "newbie" or
"beginner suitable" and newbies can learn the ropes and get started
easy. Even chromium has a "GoodFirstBug" tag for newbies.


I think that is because they are relatively young and they are generally
used for one direct purpose. The kernel has to make sure it works in a lot
of different situations and hence a lot of different bugs arise.

The kernel is old, and hence most of the JuniorJobs (as KDE calls them)
are either already fixed, or fixed right on the spot when they are discovered.



With the linux kernel not only doesn't anything exist, the project
itself is so bloody hard right, kernel programming, most of the
bugzilla bugs I can barely understand let alone even begin to deduce
what is going on. Now given that the list itself isn't maintained
makes things extremely hard.


There are still methods to extract various unresolved bugs from the
bugzilla though. Look in any subsystem you find delicious and then
just sort the bugs by the date they were modified. This will yield
a list of nice fresh bugs along with some recently fixed bugs.



Thanks @Nick for bringing this up. I would love to help you clean up
the bugs, give me an email of any ideas you have on how to start.



I brought this up as well on the Kernel Summit list. There wasn't any
feedback on this :-), I guess there are some maintainers who care about
bugzilla, but the rest (and the majority probably) does not care.

I tend to think that even if we clean up the bugzilla, it will only be a
question of time until it gets to the state that it is in now.

Thanks,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Cleanup of Kernel Bugzilla

[Levente Kurusa]

Hi,

[Why was stable on Cc?]

On 06/29/2014 08:33 AM, Gideon D'souza wrote:

Why do you want an up to date list of kernel bugs?


Even I'm a newbie and was looking for the same thing. I read
eventually somewhere that bugzilla isn't maintained by kernel devs so
I entirely gave up trying to find a bug to adopt.

Many open source projects I've worked on (web frameworks mostly) have
a well maintained list of bugs and tag the right ones newbie or
beginner suitable and newbies can learn the ropes and get started
easy. Even chromium has a GoodFirstBug tag for newbies.


I think that is because they are relatively young and they are generally
used for one direct purpose. The kernel has to make sure it works in a lot
of different situations and hence a lot of different bugs arise.

The kernel is old, and hence most of the JuniorJobs (as KDE calls them)
are either already fixed, or fixed right on the spot when they are discovered.



With the linux kernel not only doesn't anything exist, the project
itself is so bloody hard right, kernel programming, most of the
bugzilla bugs I can barely understand let alone even begin to deduce
what is going on. Now given that the list itself isn't maintained
makes things extremely hard.


There are still methods to extract various unresolved bugs from the
bugzilla though. Look in any subsystem you find delicious and then
just sort the bugs by the date they were modified. This will yield
a list of nice fresh bugs along with some recently fixed bugs.



Thanks @Nick for bringing this up. I would love to help you clean up
the bugs, give me an email of any ideas you have on how to start.



I brought this up as well on the Kernel Summit list. There wasn't any
feedback on this :-), I guess there are some maintainers who care about
bugzilla, but the rest (and the majority probably) does not care.

I tend to think that even if we clean up the bugzilla, it will only be a
question of time until it gets to the state that it is in now.

Thanks,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4]Checks for Null value in function *get_skub

[Levente Kurusa]

On 06/16/2014 05:37 PM, Nick Krause wrote:

Signed-off-by: Nicholas Krause 
---
  drivers/infiniband/hw/cxgb4/
cm.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index f9477e2..2d56983 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
  if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
 skb_trim(skb, 0);
 skb_get(skb);
 skb_reset_transport_header(skb);
 } else {
+   if (skb)
/* NULL is ignored */.
+   kfree_skb (skb);
 skb = alloc_skb(len, gfp);
 }
 t4_set_arp_err_handler(skb, NULL, NULL);


This patch is severely whitespace damaged. Can you fix your MUA
or try to use git-send-email?

The changelog as well is of low quality. Maybe something like this:

infiniband: cxgb4: fix memory skb memory leak

... would be better.

Also, this fails the build, due to the extra dot character after
the comment. :-(

Thanks,
    Levente Kurusa.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4]Checks for Null value in function *get_skub

[Levente Kurusa]

On 06/16/2014 05:30 PM, Steve Wise wrote:

On 6/16/2014 10:25 AM, Nicholas Krause wrote:

Signed-off-by: Nicholas Krause 
---
  drivers/infiniband/hw/cxgb4/cm.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index f9477e2..2d56983 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
  if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
 skb_trim(skb, 0);
 skb_get(skb);
 skb_reset_transport_header(skb);
  } else {
+if (skb)
+kfree_skb (skb);
  skb = alloc_skb(len, gfp);
  }
  t4_set_arp_err_handler(skb, NULL, NULL);


Can you change the comment?  This patch is now fixing a potential skb leak.
Also, kfree_sb() will ignore NULL ptrs, so we could just always call it.

> But I'd add a comment like /* NULL is ignored */.

AFAIK, checkpatch.pl will show you a message if it detects this:

if (x)
kfree(x);

so I guess it is not really needed.

Thanks,
Levente Kurusa.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3]Checks for Null value in function *get_skub

[Levente Kurusa]

On 06/16/2014 05:08 PM, Nicholas Krause wrote:

Signed-off-by: Nicholas Krause 
---
  drivers/infiniband/hw/cxgb4/cm.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index f9477e2..2d56983 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
  if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
 skb_trim(skb, 0);
 skb_get(skb);
 skb_reset_transport_header(skb);
} else {
+   if (skb)
+   kfree (skb);
skb = alloc_skb(len, gfp);
}
t4_set_arp_err_handler(skb, NULL, NULL);



Isn't kfree(NULL) legal?

(i.e. the if statement is useless)

Thanks,
    Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3]Checks for Null value in function *get_skub

[Levente Kurusa]

On 06/16/2014 05:08 PM, Nicholas Krause wrote:

Signed-off-by: Nicholas Krause xerofo...@gmail.com
---
  drivers/infiniband/hw/cxgb4/cm.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index f9477e2..2d56983 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
  if (skb  !skb_is_nonlinear(skb)  !skb_cloned(skb)) {
 skb_trim(skb, 0);
 skb_get(skb);
 skb_reset_transport_header(skb);
} else {
+   if (skb)
+   kfree (skb);
skb = alloc_skb(len, gfp);
}
t4_set_arp_err_handler(skb, NULL, NULL);



Isn't kfree(NULL) legal?

(i.e. the if statement is useless)

Thanks,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4]Checks for Null value in function *get_skub

[Levente Kurusa]

On 06/16/2014 05:30 PM, Steve Wise wrote:

On 6/16/2014 10:25 AM, Nicholas Krause wrote:

Signed-off-by: Nicholas Krause xerofo...@gmail.com
---
  drivers/infiniband/hw/cxgb4/cm.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index f9477e2..2d56983 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
  if (skb  !skb_is_nonlinear(skb)  !skb_cloned(skb)) {
 skb_trim(skb, 0);
 skb_get(skb);
 skb_reset_transport_header(skb);
  } else {
+if (skb)
+kfree_skb (skb);
  skb = alloc_skb(len, gfp);
  }
  t4_set_arp_err_handler(skb, NULL, NULL);


Can you change the comment?  This patch is now fixing a potential skb leak.
Also, kfree_sb() will ignore NULL ptrs, so we could just always call it.

 But I'd add a comment like /* NULL is ignored */.

AFAIK, checkpatch.pl will show you a message if it detects this:

if (x)
kfree(x);

so I guess it is not really needed.

Thanks,
Levente Kurusa.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4]Checks for Null value in function *get_skub

[Levente Kurusa]

On 06/16/2014 05:37 PM, Nick Krause wrote:

Signed-off-by: Nicholas Krause xerofo...@gmail.com
---
  drivers/infiniband/hw/cxgb4/
cm.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index f9477e2..2d56983 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
  if (skb  !skb_is_nonlinear(skb)  !skb_cloned(skb)) {
 skb_trim(skb, 0);
 skb_get(skb);
 skb_reset_transport_header(skb);
 } else {
+   if (skb)
/* NULL is ignored */.
+   kfree_skb (skb);
 skb = alloc_skb(len, gfp);
 }
 t4_set_arp_err_handler(skb, NULL, NULL);


This patch is severely whitespace damaged. Can you fix your MUA
or try to use git-send-email?

The changelog as well is of low quality. Maybe something like this:

infiniband: cxgb4: fix memory skb memory leak

... would be better.

Also, this fails the build, due to the extra dot character after
the comment. :-(

Thanks,
Levente Kurusa.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: rtl8821ae: rtl8821ae: hw.c: Cleaning up if statement that always evaluates to false

[Levente Kurusa]

On Sun, Jun 08, 2014 at 01:37:43PM -0700, David Rientjes wrote:
> >  drivers/staging/rtl8821ae/rtl8821ae/hw.c |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/rtl8821ae/rtl8821ae/hw.c 
> > b/drivers/staging/rtl8821ae/rtl8821ae/hw.c
> > index 1b8583b..52322e3 100644
> > --- a/drivers/staging/rtl8821ae/rtl8821ae/hw.c
> > +++ b/drivers/staging/rtl8821ae/rtl8821ae/hw.c
> > @@ -1623,7 +1623,7 @@ static int _rtl8821ae_set_media_status(struct 
> > ieee80211_hw *hw,
> >  
> > rtl_write_byte(rtlpriv, (MSR), bt_msr);
> > rtlpriv->cfg->ops->led_control(hw, ledaction);
> > -   if ((bt_msr & ~0xfc) == MSR_AP)
> > +   if ((bt_msr & MSR_AP) == MSR_AP)

I changed this line from '0xfc' to '~0xfc', and looking at the defines
it must have been the way your patch does, so:

Acked-by: Levente Kurusa 

if it wasn't applied yet, and you fix the problems David pointed out
with the commit message.

Thanks,
Levente Kurusa


signature.asc
Description: Digital signature

Re: [PATCH] staging: rtl8821ae: rtl8821ae: hw.c: Cleaning up if statement that always evaluates to false

[Levente Kurusa]

On Sun, Jun 08, 2014 at 01:37:43PM -0700, David Rientjes wrote:
   drivers/staging/rtl8821ae/rtl8821ae/hw.c |2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
  
  diff --git a/drivers/staging/rtl8821ae/rtl8821ae/hw.c 
  b/drivers/staging/rtl8821ae/rtl8821ae/hw.c
  index 1b8583b..52322e3 100644
  --- a/drivers/staging/rtl8821ae/rtl8821ae/hw.c
  +++ b/drivers/staging/rtl8821ae/rtl8821ae/hw.c
  @@ -1623,7 +1623,7 @@ static int _rtl8821ae_set_media_status(struct 
  ieee80211_hw *hw,
   
  rtl_write_byte(rtlpriv, (MSR), bt_msr);
  rtlpriv-cfg-ops-led_control(hw, ledaction);
  -   if ((bt_msr  ~0xfc) == MSR_AP)
  +   if ((bt_msr  MSR_AP) == MSR_AP)

I changed this line from '0xfc' to '~0xfc', and looking at the defines
it must have been the way your patch does, so:

Acked-by: Levente Kurusa le...@linux.com

if it wasn't applied yet, and you fix the problems David pointed out
with the commit message.

Thanks,
Levente Kurusa


signature.asc
Description: Digital signature

[PATCH] staging: rtl8821ae: fix pointer coding style

[Levente Kurusa]

Found by checkpatch.

Signed-off-by: Levente Kurusa 
---

 drivers/staging/rtl8821ae/pci.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8821ae/pci.c b/drivers/staging/rtl8821ae/pci.c
index a562aa6..7bbaef7 100644
--- a/drivers/staging/rtl8821ae/pci.c
+++ b/drivers/staging/rtl8821ae/pci.c
@@ -392,7 +392,7 @@ static u8 _rtl_pci_get_pciehdr_offset(struct ieee80211_hw 
*hw)
 pcicfg_addr_port +
 (num4bytes << 2));
rtl_pci_raw_read_port_ushort(PCI_CONF_DATA,
-(u16*)_hdr);
+(u16 *)_hdr);
/* Found the PCI express capability. */
if (capability_hdr.capability_id ==
PCI_CAPABILITY_ID_PCI_EXPRESS)
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: rtl8821ae: fix pointer coding style

[Levente Kurusa]

Found by checkpatch.

Signed-off-by: Levente Kurusa le...@linux.com
---

 drivers/staging/rtl8821ae/pci.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8821ae/pci.c b/drivers/staging/rtl8821ae/pci.c
index a562aa6..7bbaef7 100644
--- a/drivers/staging/rtl8821ae/pci.c
+++ b/drivers/staging/rtl8821ae/pci.c
@@ -392,7 +392,7 @@ static u8 _rtl_pci_get_pciehdr_offset(struct ieee80211_hw 
*hw)
 pcicfg_addr_port +
 (num4bytes  2));
rtl_pci_raw_read_port_ushort(PCI_CONF_DATA,
-(u16*)capability_hdr);
+(u16 *)capability_hdr);
/* Found the PCI express capability. */
if (capability_hdr.capability_id ==
PCI_CAPABILITY_ID_PCI_EXPRESS)
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] libata: clean up ZPODD when a port is detached

[Levente Kurusa]

When a ZPODD device is unbound via sysfs, the ACPI notify handler
is not removed. This causes panics as observed in Bug #74601. The
panic only happens when the wake happens from outside the kernel
(i.e. inserting a media or pressing a button). Add a loop to
ata_port_detach which loops through the port's devices and checks
if zpodd is enabled, if so call zpodd_exit.

Cc: sta...@vger.kernel.org
Reviewed-by: Aaron Lu 
Signed-off-by: Levente Kurusa 
---
 drivers/ata/libata-core.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 943cc8b..ea83828 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6314,6 +6314,8 @@ int ata_host_activate(struct ata_host *host, int irq,
 static void ata_port_detach(struct ata_port *ap)
 {
unsigned long flags;
+   struct ata_link *link;
+   struct ata_device *dev;
 
if (!ap->ops->error_handler)
goto skip_eh;
@@ -6333,6 +6335,13 @@ static void ata_port_detach(struct ata_port *ap)
cancel_delayed_work_sync(>hotplug_task);
 
  skip_eh:
+   /* clean up zpodd on port removal */
+   ata_for_each_link(link, ap, HOST_FIRST) {
+   ata_for_each_dev(dev, link, ALL) {
+   if (zpodd_dev_enabled(dev))
+   zpodd_exit(dev);
+   }
+   }
if (ap->pmp_link) {
int i;
for (i = 0; i < SATA_PMP_MAX_PORTS; i++)
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ahci: unregister acpi notify handler when a ZPODD is unbound

[Levente Kurusa]

Hi,

On 05/06/2014 08:07 AM, Aaron Lu wrote:
> On 05/06/2014 02:02 PM, Levente Kurusa wrote:
>> Hi,
>>
>> On 05/06/2014 05:16 AM, Aaron Lu wrote:
>>> On 05/01/2014 12:04 AM, Levente Kurusa wrote:
>>>> When a ZPODD device is unbound via sysfs, the acpi notify handler
>>>> is not removed. This causes panics as observed in Bug #74601. The
>>>
>>> Ah...too bad, I forgot to consider this situation, thanks for tracking
>>> this.
>>>
>>>> panic only happens when the wake happens from outside the kernel
>>>> (i.e. inserting media or pressing a button). Implement a new
>>>> ahci_remove_one function which causes zpodd_exit to be called for all
>>>> ZPODD devices on the unbound PCI device.
>>>>
>>>> Signed-off-by: Levente Kurusa 
>>>> ---
>>>>
>>>> Hi,
>>>>
>>>> I am not sure if the loop below is correct. Maybe there is a better
>>>> solution to loop through all the devices which might use ZPODD?
>>>
>>> I didn't find a proper place either. For hotplug, we did the zpodd_exit
>>> at ata_scsi_handle_link_detach. But for host controller pci device
>>> removal, we used scsi_remove_host in ata_port_detach and there is no
>>> place to add the zpodd_exit for a to-be-removed scsi device...
>>>
>>> Looks like we can only iterate the ata devices and call zpodd_exit
>>> explicitly for them if they are zpodd devices. Instead of adding a new
>>> remove callback, what about just embed that into the ata_port_detach
>>> like the following example?
>>
>> Yes, this makes more sense as this doesn't tinker with exports and such...
>> However this will throw unused variable compiler warnings if we add the
>> required #ifdefs... Maybe a new function? ata_zpodd_detach_port?
> 
> I think we can omit the #ifdefs as the loop is not called frequently and
> thus doesn't cost much. We already have stubs for zpodd_dev_enabled and
> zpodd_exit.
> 

Ah, I see. Shall I send V2? Any tags I should add for you?

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] ahci: unregister acpi notify handler when a ZPODD is unbound

[Levente Kurusa]

Hi,

On 05/06/2014 05:16 AM, Aaron Lu wrote:
> On 05/01/2014 12:04 AM, Levente Kurusa wrote:
>> When a ZPODD device is unbound via sysfs, the acpi notify handler
>> is not removed. This causes panics as observed in Bug #74601. The
> 
> Ah...too bad, I forgot to consider this situation, thanks for tracking
> this.
> 
>> panic only happens when the wake happens from outside the kernel
>> (i.e. inserting media or pressing a button). Implement a new
>> ahci_remove_one function which causes zpodd_exit to be called for all
>> ZPODD devices on the unbound PCI device.
>>
>> Signed-off-by: Levente Kurusa 
>> ---
>>
>> Hi,
>>
>> I am not sure if the loop below is correct. Maybe there is a better
>> solution to loop through all the devices which might use ZPODD?
> 
> I didn't find a proper place either. For hotplug, we did the zpodd_exit
> at ata_scsi_handle_link_detach. But for host controller pci device
> removal, we used scsi_remove_host in ata_port_detach and there is no
> place to add the zpodd_exit for a to-be-removed scsi device...
> 
> Looks like we can only iterate the ata devices and call zpodd_exit
> explicitly for them if they are zpodd devices. Instead of adding a new
> remove callback, what about just embed that into the ata_port_detach
> like the following example?

Yes, this makes more sense as this doesn't tinker with exports and such...
However this will throw unused variable compiler warnings if we add the
required #ifdefs... Maybe a new function? ata_zpodd_detach_port?

> 
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 943cc8b83e59..43652da6fea6 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -6314,6 +6314,8 @@ int ata_host_activate(struct ata_host *host, int irq,
>  static void ata_port_detach(struct ata_port *ap)
>  {
>   unsigned long flags;
> + struct ata_link *link;
> + struct ata_device *dev;
>  
>   if (!ap->ops->error_handler)
>   goto skip_eh;
> @@ -6333,6 +6335,13 @@ static void ata_port_detach(struct ata_port *ap)
>   cancel_delayed_work_sync(>hotplug_task);
>  
>   skip_eh:
> + /* clean up zpodd related stuffs on port removal */
> + ata_for_each_link(link, ap, HOST_FIRST) {
> + ata_for_each_dev(dev, link, ALL) {
> + if (zpodd_dev_enabled(dev))
> + zpodd_exit(dev);
> + }
> + }
>   if (ap->pmp_link) {
>   int i;
>   for (i = 0; i < SATA_PMP_MAX_PORTS; i++)

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] ahci: unregister acpi notify handler when a ZPODD is unbound

[Levente Kurusa]

Hi,

On 05/06/2014 05:16 AM, Aaron Lu wrote:
 On 05/01/2014 12:04 AM, Levente Kurusa wrote:
 When a ZPODD device is unbound via sysfs, the acpi notify handler
 is not removed. This causes panics as observed in Bug #74601. The
 
 Ah...too bad, I forgot to consider this situation, thanks for tracking
 this.
 
 panic only happens when the wake happens from outside the kernel
 (i.e. inserting media or pressing a button). Implement a new
 ahci_remove_one function which causes zpodd_exit to be called for all
 ZPODD devices on the unbound PCI device.

 Signed-off-by: Levente Kurusa le...@linux.com
 ---

 Hi,

 I am not sure if the loop below is correct. Maybe there is a better
 solution to loop through all the devices which might use ZPODD?
 
 I didn't find a proper place either. For hotplug, we did the zpodd_exit
 at ata_scsi_handle_link_detach. But for host controller pci device
 removal, we used scsi_remove_host in ata_port_detach and there is no
 place to add the zpodd_exit for a to-be-removed scsi device...
 
 Looks like we can only iterate the ata devices and call zpodd_exit
 explicitly for them if they are zpodd devices. Instead of adding a new
 remove callback, what about just embed that into the ata_port_detach
 like the following example?

Yes, this makes more sense as this doesn't tinker with exports and such...
However this will throw unused variable compiler warnings if we add the
required #ifdefs... Maybe a new function? ata_zpodd_detach_port?

 
 
 diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
 index 943cc8b83e59..43652da6fea6 100644
 --- a/drivers/ata/libata-core.c
 +++ b/drivers/ata/libata-core.c
 @@ -6314,6 +6314,8 @@ int ata_host_activate(struct ata_host *host, int irq,
  static void ata_port_detach(struct ata_port *ap)
  {
   unsigned long flags;
 + struct ata_link *link;
 + struct ata_device *dev;
  
   if (!ap-ops-error_handler)
   goto skip_eh;
 @@ -6333,6 +6335,13 @@ static void ata_port_detach(struct ata_port *ap)
   cancel_delayed_work_sync(ap-hotplug_task);
  
   skip_eh:
 + /* clean up zpodd related stuffs on port removal */
 + ata_for_each_link(link, ap, HOST_FIRST) {
 + ata_for_each_dev(dev, link, ALL) {
 + if (zpodd_dev_enabled(dev))
 + zpodd_exit(dev);
 + }
 + }
   if (ap-pmp_link) {
   int i;
   for (i = 0; i  SATA_PMP_MAX_PORTS; i++)

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] ahci: unregister acpi notify handler when a ZPODD is unbound

[Levente Kurusa]

Hi,

On 05/06/2014 08:07 AM, Aaron Lu wrote:
 On 05/06/2014 02:02 PM, Levente Kurusa wrote:
 Hi,

 On 05/06/2014 05:16 AM, Aaron Lu wrote:
 On 05/01/2014 12:04 AM, Levente Kurusa wrote:
 When a ZPODD device is unbound via sysfs, the acpi notify handler
 is not removed. This causes panics as observed in Bug #74601. The

 Ah...too bad, I forgot to consider this situation, thanks for tracking
 this.

 panic only happens when the wake happens from outside the kernel
 (i.e. inserting media or pressing a button). Implement a new
 ahci_remove_one function which causes zpodd_exit to be called for all
 ZPODD devices on the unbound PCI device.

 Signed-off-by: Levente Kurusa le...@linux.com
 ---

 Hi,

 I am not sure if the loop below is correct. Maybe there is a better
 solution to loop through all the devices which might use ZPODD?

 I didn't find a proper place either. For hotplug, we did the zpodd_exit
 at ata_scsi_handle_link_detach. But for host controller pci device
 removal, we used scsi_remove_host in ata_port_detach and there is no
 place to add the zpodd_exit for a to-be-removed scsi device...

 Looks like we can only iterate the ata devices and call zpodd_exit
 explicitly for them if they are zpodd devices. Instead of adding a new
 remove callback, what about just embed that into the ata_port_detach
 like the following example?

 Yes, this makes more sense as this doesn't tinker with exports and such...
 However this will throw unused variable compiler warnings if we add the
 required #ifdefs... Maybe a new function? ata_zpodd_detach_port?
 
 I think we can omit the #ifdefs as the loop is not called frequently and
 thus doesn't cost much. We already have stubs for zpodd_dev_enabled and
 zpodd_exit.
 

Ah, I see. Shall I send V2? Any tags I should add for you?

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641



signature.asc
Description: OpenPGP digital signature

[PATCH] libata: clean up ZPODD when a port is detached

[Levente Kurusa]

When a ZPODD device is unbound via sysfs, the ACPI notify handler
is not removed. This causes panics as observed in Bug #74601. The
panic only happens when the wake happens from outside the kernel
(i.e. inserting a media or pressing a button). Add a loop to
ata_port_detach which loops through the port's devices and checks
if zpodd is enabled, if so call zpodd_exit.

Cc: sta...@vger.kernel.org
Reviewed-by: Aaron Lu aaron...@intel.com
Signed-off-by: Levente Kurusa le...@linux.com
---
 drivers/ata/libata-core.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 943cc8b..ea83828 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6314,6 +6314,8 @@ int ata_host_activate(struct ata_host *host, int irq,
 static void ata_port_detach(struct ata_port *ap)
 {
unsigned long flags;
+   struct ata_link *link;
+   struct ata_device *dev;
 
if (!ap-ops-error_handler)
goto skip_eh;
@@ -6333,6 +6335,13 @@ static void ata_port_detach(struct ata_port *ap)
cancel_delayed_work_sync(ap-hotplug_task);
 
  skip_eh:
+   /* clean up zpodd on port removal */
+   ata_for_each_link(link, ap, HOST_FIRST) {
+   ata_for_each_dev(dev, link, ALL) {
+   if (zpodd_dev_enabled(dev))
+   zpodd_exit(dev);
+   }
+   }
if (ap-pmp_link) {
int i;
for (i = 0; i  SATA_PMP_MAX_PORTS; i++)
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Levente Kurusa]

Hi,

On Mon, May 05, 2014 at 01:59:49AM +0200, Dominique van den Broeck wrote:
> 
> Good evening,
> 
> Forgive my mistakes, I sent only a few patches yet and I'm still learning.
> Nevertheless:

Not a problem at all, since that is what the challenge is for.

> 
> > What is that period in the commit message? And the semicolon?
> 
> Semicolons is what one use to ponctuate an enumerated list (at least
> in french). In fact, it was their primary use before computer era.
> 
> Is there something wrong with it ? I check all my patches with
> checkpatch.pl --strict before sending them and it didn't seem to
> complain...

Oh, I see now! Well I guess it's better to have a commit message
that says WHY the change is good and WHAT did it change, not just
a list of what you did.

i.e. in this case you could also inline the sparse warning or at least
a part of it.

> 
> > You should also be a bit more specific. Also, the Subject line is
> > very bad. Better go with something like this:
> > 
> > staging: rtl8192e: fix userspace pointer dereference
> 
> Right. I used the slash as a subpart of the tree. Didn't know what
> was the best for this situation.

That is good as well, I just prefer the latter one, but feel
free to use whichever you feel better. :-)

> 
> > And when you resend a patchset, please resend the full patchset.
> 
> I usually do but I've got an acceptation message for the first one
> (see below).
> 

In that case, that's good.

> > This is totally unneccessary.
> 
> Should at least have gone in the body below indeed.
> 
> > When you cite a commit please don't include the full hash, that is
> > non informational. Better put the first 7 characters of the hash and
> > the first line of the commit message as well in parantheses, like so:
> >
> > 5169af2 ("Staging: rtl8192e: Fix declaration of symbols")
> > (I even have a command for this in vim :-) )
> 
> I'm interested. I use vim too.
> 

I have this:
--%<---
function! GetGitCommit(commit)
exec ":.!git log --oneline --pretty=\"format:\\%h ('\\%s')\" ".a:commit." 
-1"
endfunction
-->%---

> > Are you sure that 1/2 was applied to the staging tree?
> > It's unlikely that 1/2 is applied while 2/2 is left alone.
> 
> As stated before, I received the common automatic mail from
> Greg KH regarding this one. So I'll now wait before I do a v3
> for this issue. If so, I'll resent the complete set if required.

No, if that was applied it's unneccessary to resend the full set.

> 
> > Oh, I am unable to find commit b5c8d48 in Linus' or staging-next.
> > In which tree is it?
> 
> It's linux-next. If I quote a commit, I should the tree as well,
> indeed. But since the v1 was performed partially for the Eudyptula
> Project and since it was a response to a modification request, 
> I though it was implicit.

No, the tree's name is not needed. In fact, I should have checked it
in linux-next, but I only checked Linus', and staging-next thinking,
since you said it was applied, it was applied to staging-next. :-)
> 
> > Could you please as well remove that empty line in the declarations?
> 
> I'll do.
> Cheers.

Regards,
Levente Kurusa


signature.asc
Description: Digital signature

Re: [PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Levente Kurusa]

Hi,

On Mon, May 05, 2014 at 01:59:49AM +0200, Dominique van den Broeck wrote:
 
 Good evening,
 
 Forgive my mistakes, I sent only a few patches yet and I'm still learning.
 Nevertheless:

Not a problem at all, since that is what the challenge is for.

 
  What is that period in the commit message? And the semicolon?
 
 Semicolons is what one use to ponctuate an enumerated list (at least
 in french). In fact, it was their primary use before computer era.
 
 Is there something wrong with it ? I check all my patches with
 checkpatch.pl --strict before sending them and it didn't seem to
 complain...

Oh, I see now! Well I guess it's better to have a commit message
that says WHY the change is good and WHAT did it change, not just
a list of what you did.

i.e. in this case you could also inline the sparse warning or at least
a part of it.

 
  You should also be a bit more specific. Also, the Subject line is
  very bad. Better go with something like this:
  
  staging: rtl8192e: fix userspace pointer dereference
 
 Right. I used the slash as a subpart of the tree. Didn't know what
 was the best for this situation.

That is good as well, I just prefer the latter one, but feel
free to use whichever you feel better. :-)

 
  And when you resend a patchset, please resend the full patchset.
 
 I usually do but I've got an acceptation message for the first one
 (see below).
 

In that case, that's good.

  This is totally unneccessary.
 
 Should at least have gone in the body below indeed.
 
  When you cite a commit please don't include the full hash, that is
  non informational. Better put the first 7 characters of the hash and
  the first line of the commit message as well in parantheses, like so:
 
  5169af2 (Staging: rtl8192e: Fix declaration of symbols)
  (I even have a command for this in vim :-) )
 
 I'm interested. I use vim too.
 

I have this:
--%---
function! GetGitCommit(commit)
exec :.!git log --oneline --pretty=\format:\\%h ('\\%s')\ .a:commit. 
-1
endfunction
--%---

  Are you sure that 1/2 was applied to the staging tree?
  It's unlikely that 1/2 is applied while 2/2 is left alone.
 
 As stated before, I received the common automatic mail from
 Greg KH regarding this one. So I'll now wait before I do a v3
 for this issue. If so, I'll resent the complete set if required.

No, if that was applied it's unneccessary to resend the full set.

 
  Oh, I am unable to find commit b5c8d48 in Linus' or staging-next.
  In which tree is it?
 
 It's linux-next. If I quote a commit, I should the tree as well,
 indeed. But since the v1 was performed partially for the Eudyptula
 Project and since it was a response to a modification request, 
 I though it was implicit.

No, the tree's name is not needed. In fact, I should have checked it
in linux-next, but I only checked Linus', and staging-next thinking,
since you said it was applied, it was applied to staging-next. :-)
 
  Could you please as well remove that empty line in the declarations?
 
 I'll do.
 Cheers.

Regards,
Levente Kurusa


signature.asc
Description: Digital signature

Re: [PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Levente Kurusa]

Hi,

On Sun, May 04, 2014 at 04:46:27PM +0200, Dominique van den Broeck wrote:
> . userspace pointer dereference ;
> 

What is that period in the commit message? And the semicolon?
You should also be a bit more specific. Also, the Subject line is
very bad. Better go with something like this:

staging: rtl8192e: fix userspace pointer dereference

And when you resend a patchset, please resend the full patchset.

> These issues have been fixed by a concurrent patch:
> . missing inclusions of needed header files (fixed by concurrent patch);
> . unrequired static function declaration (confusing another *.c file).

This is totally unneccessary.

> 
> Signed-off-by: Dominique van den Broeck 
> ---
> v1 : I submit this patch as a result for Task #16 of the Eudyptula Challenge.
> v2 : Resubmitted because of a conflit with commit 
> 5169af2309f42bb4cb0ebfefe6bf8bc888d4ce33 .
>  Successfully tested against commit 
> b5c8d48bf8f4273a9fe680bd834f991005c8ab59 .
>  I resubmit only the 2/2 one, since the 1/2 as already been accepted.
> 
>  Levente, still agree with you about numeric values that should be 
> changed into symbols.
>  This will form another future patch.

When you cite a commit please don't include the full hash, that is
non informational. Better put the first 7 characters of the hash and
the first line of the commit message as well in parantheses, like so:

5169af2 ("Staging: rtl8192e: Fix declaration of symbols")
(I even have a command for this in vim :-) )

Are you sure that 1/2 was applied to the staging tree?
It's unlikely that 1/2 is applied while 2/2 is left alone.

Oh, I am unable to find commit b5c8d48 in Linus' or staging-next.
In which tree is it?

> 
> diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c 
> b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> index 498995d..d87cdfa 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> @@ -1131,11 +1131,18 @@ static int r8192_wx_set_PromiscuousMode(struct 
> net_device *dev,
>   struct r8192_priv *priv = rtllib_priv(dev);
>   struct rtllib_device *ieee = priv->rtllib;
>  
> - u32 *info_buf = (u32 *)(wrqu->data.pointer);
> + u32 info_buf[3];

Could you please as well remove that empty line in the declarations?

>  
> - u32 oid = info_buf[0];
> - u32 bPromiscuousOn = info_buf[1];
> - u32 bFilterSourceStationFrame = info_buf[2];
> + u32 oid;
> + u32 bPromiscuousOn;
> + u32 bFilterSourceStationFrame;
> +
> + if (copy_from_user(info_buf, wrqu->data.pointer, sizeof(info_buf)))
> + return -EFAULT;
> +
> + oid = info_buf[0];
> + bPromiscuousOn = info_buf[1];
> + bFilterSourceStationFrame = info_buf[2];
>  
>   if (OID_RT_INTEL_PROMISCUOUS_MODE == oid) {
>   ieee->IntelPromiscuousModeInfo.bPromiscuousOn =

--
Regards,
Levente Kurusa


signature.asc
Description: Digital signature

Re: [PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Levente Kurusa]

Hi,

On Sun, May 04, 2014 at 04:46:27PM +0200, Dominique van den Broeck wrote:
 . userspace pointer dereference ;
 

What is that period in the commit message? And the semicolon?
You should also be a bit more specific. Also, the Subject line is
very bad. Better go with something like this:

staging: rtl8192e: fix userspace pointer dereference

And when you resend a patchset, please resend the full patchset.

 These issues have been fixed by a concurrent patch:
 . missing inclusions of needed header files (fixed by concurrent patch);
 . unrequired static function declaration (confusing another *.c file).

This is totally unneccessary.

 
 Signed-off-by: Dominique van den Broeck domdev...@free.fr
 ---
 v1 : I submit this patch as a result for Task #16 of the Eudyptula Challenge.
 v2 : Resubmitted because of a conflit with commit 
 5169af2309f42bb4cb0ebfefe6bf8bc888d4ce33 .
  Successfully tested against commit 
 b5c8d48bf8f4273a9fe680bd834f991005c8ab59 .
  I resubmit only the 2/2 one, since the 1/2 as already been accepted.
 
  Levente, still agree with you about numeric values that should be 
 changed into symbols.
  This will form another future patch.

When you cite a commit please don't include the full hash, that is
non informational. Better put the first 7 characters of the hash and
the first line of the commit message as well in parantheses, like so:

5169af2 (Staging: rtl8192e: Fix declaration of symbols)
(I even have a command for this in vim :-) )

Are you sure that 1/2 was applied to the staging tree?
It's unlikely that 1/2 is applied while 2/2 is left alone.

Oh, I am unable to find commit b5c8d48 in Linus' or staging-next.
In which tree is it?

 
 diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c 
 b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
 index 498995d..d87cdfa 100644
 --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
 +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
 @@ -1131,11 +1131,18 @@ static int r8192_wx_set_PromiscuousMode(struct 
 net_device *dev,
   struct r8192_priv *priv = rtllib_priv(dev);
   struct rtllib_device *ieee = priv-rtllib;
  
 - u32 *info_buf = (u32 *)(wrqu-data.pointer);
 + u32 info_buf[3];

Could you please as well remove that empty line in the declarations?

  
 - u32 oid = info_buf[0];
 - u32 bPromiscuousOn = info_buf[1];
 - u32 bFilterSourceStationFrame = info_buf[2];
 + u32 oid;
 + u32 bPromiscuousOn;
 + u32 bFilterSourceStationFrame;
 +
 + if (copy_from_user(info_buf, wrqu-data.pointer, sizeof(info_buf)))
 + return -EFAULT;
 +
 + oid = info_buf[0];
 + bPromiscuousOn = info_buf[1];
 + bFilterSourceStationFrame = info_buf[2];
  
   if (OID_RT_INTEL_PROMISCUOUS_MODE == oid) {
   ieee-IntelPromiscuousModeInfo.bPromiscuousOn =

--
Regards,
Levente Kurusa


signature.asc
Description: Digital signature

[PATCH] ahci: unregister acpi notify handler when a ZPODD is unbound

[Levente Kurusa]

When a ZPODD device is unbound via sysfs, the acpi notify handler
is not removed. This causes panics as observed in Bug #74601. The
panic only happens when the wake happens from outside the kernel
(i.e. inserting media or pressing a button). Implement a new
ahci_remove_one function which causes zpodd_exit to be called for all
ZPODD devices on the unbound PCI device.

Signed-off-by: Levente Kurusa 
---

Hi,

I am not sure if the loop below is correct. Maybe there is a better
solution to loop through all the devices which might use ZPODD?

Thanks, Lev.

 drivers/ata/ahci.c | 21 +
 drivers/ata/ahci.h |  4 
 drivers/ata/libata-zpodd.c |  1 +
 3 files changed, 26 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 5a0bf8e..6d92bc9 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -475,12 +475,33 @@ static const struct pci_device_id ahci_pci_tbl[] = {
{ } /* terminate list */
 };
 
+#ifdef CONFIG_SATA_ZPODD
+void ahci_remove_one(struct pci_dev *pdev)
+{
+   struct ata_host *host = pci_get_drvdata(pdev);
+   struct ata_link *link;
+   struct ata_device *dev;
+   int i;
+
+   for (i = 0; i < host->n_ports; i++)
+   ata_for_each_link(link, host->ports[i], HOST_FIRST)
+   ata_for_each_dev(dev, link, ALL)
+   if (dev->zpodd)
+   zpodd_exit(dev);
+
+   ata_pci_remove_one(pdev);
+}
+#endif
 
 static struct pci_driver ahci_pci_driver = {
.name   = DRV_NAME,
.id_table   = ahci_pci_tbl,
.probe  = ahci_init_one,
+#ifdef CONFIG_SATA_ZPODD
+   .remove = ahci_remove_one,
+#else
.remove = ata_pci_remove_one,
+#endif
 #ifdef CONFIG_PM
.suspend= ahci_pci_device_suspend,
.resume = ahci_pci_device_resume,
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 51af275..87e4e6d 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -383,6 +383,10 @@ void ahci_print_info(struct ata_host *host, const char 
*scc_s);
 int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis);
 void ahci_error_handler(struct ata_port *ap);
 
+#ifdef CONFIG_SATA_ZPODD
+extern void zpodd_exit(struct ata_device *dev);
+#endif /* CONFIG_SATA_ZPODD */
+
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
 unsigned int port_no)
 {
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index f3a65a3..fe66949 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -281,3 +281,4 @@ void zpodd_exit(struct ata_device *dev)
kfree(dev->zpodd);
dev->zpodd = NULL;
 }
+EXPORT_SYMBOL_GPL(zpodd_exit);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] ahci: unregister acpi notify handler when a ZPODD is unbound

[Levente Kurusa]

When a ZPODD device is unbound via sysfs, the acpi notify handler
is not removed. This causes panics as observed in Bug #74601. The
panic only happens when the wake happens from outside the kernel
(i.e. inserting media or pressing a button). Implement a new
ahci_remove_one function which causes zpodd_exit to be called for all
ZPODD devices on the unbound PCI device.

Signed-off-by: Levente Kurusa le...@linux.com
---

Hi,

I am not sure if the loop below is correct. Maybe there is a better
solution to loop through all the devices which might use ZPODD?

Thanks, Lev.

 drivers/ata/ahci.c | 21 +
 drivers/ata/ahci.h |  4 
 drivers/ata/libata-zpodd.c |  1 +
 3 files changed, 26 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 5a0bf8e..6d92bc9 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -475,12 +475,33 @@ static const struct pci_device_id ahci_pci_tbl[] = {
{ } /* terminate list */
 };
 
+#ifdef CONFIG_SATA_ZPODD
+void ahci_remove_one(struct pci_dev *pdev)
+{
+   struct ata_host *host = pci_get_drvdata(pdev);
+   struct ata_link *link;
+   struct ata_device *dev;
+   int i;
+
+   for (i = 0; i  host-n_ports; i++)
+   ata_for_each_link(link, host-ports[i], HOST_FIRST)
+   ata_for_each_dev(dev, link, ALL)
+   if (dev-zpodd)
+   zpodd_exit(dev);
+
+   ata_pci_remove_one(pdev);
+}
+#endif
 
 static struct pci_driver ahci_pci_driver = {
.name   = DRV_NAME,
.id_table   = ahci_pci_tbl,
.probe  = ahci_init_one,
+#ifdef CONFIG_SATA_ZPODD
+   .remove = ahci_remove_one,
+#else
.remove = ata_pci_remove_one,
+#endif
 #ifdef CONFIG_PM
.suspend= ahci_pci_device_suspend,
.resume = ahci_pci_device_resume,
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 51af275..87e4e6d 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -383,6 +383,10 @@ void ahci_print_info(struct ata_host *host, const char 
*scc_s);
 int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis);
 void ahci_error_handler(struct ata_port *ap);
 
+#ifdef CONFIG_SATA_ZPODD
+extern void zpodd_exit(struct ata_device *dev);
+#endif /* CONFIG_SATA_ZPODD */
+
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
 unsigned int port_no)
 {
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index f3a65a3..fe66949 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -281,3 +281,4 @@ void zpodd_exit(struct ata_device *dev)
kfree(dev-zpodd);
dev-zpodd = NULL;
 }
+EXPORT_SYMBOL_GPL(zpodd_exit);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Levente Kurusa]

Hi,

On Sun, Apr 27, 2014 at 07:11:16PM +0200, Dominique van den Broeck wrote:
> . userspace pointer dereference ;
> . missing inclusions of needed header files ;
> . unrequired static function declaration (confusing another *.c file).
> 
> Signed-off-by: Dominique van den Broeck 
> ---
> I submit this patch as a result for Task #16 of the Eudyptula Challenge.
> 
> diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c 
> b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> index 498995d..d87cdfa 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> @@ -17,8 +17,10 @@
>   * wlanfae 
>  
> **/
>  
> +#include 
>  #include 
>  #include "rtl_core.h"
> +#include "rtl_wx.h"
>  
>  #define RATE_COUNT 12
>  static u32 rtl8192_rates[] = {
> @@ -1130,11 +1132,18 @@ static int r8192_wx_set_PromiscuousMode(struct 
> net_device *dev,
>   struct r8192_priv *priv = rtllib_priv(dev);
>   struct rtllib_device *ieee = priv->rtllib;
>  
> - u32 *info_buf = (u32 *)(wrqu->data.pointer);
> + u32 info_buf[3];
>  
> - u32 oid = info_buf[0];
> - u32 bPromiscuousOn = info_buf[1];
> - u32 bFilterSourceStationFrame = info_buf[2];
> + u32 oid;
> + u32 bPromiscuousOn;
> + u32 bFilterSourceStationFrame;
> +
> + if (copy_from_user(info_buf, wrqu->data.pointer, sizeof(info_buf)))
> + return -EFAULT;
> +
> + oid = info_buf[0];
> + bPromiscuousOn = info_buf[1];
> + bFilterSourceStationFrame = info_buf[2];

I guess it would be better to have defines for those instead of
hard-coding the offsets. Also the size of the info_buf array
might change depending on the size of wrqu->data.pointer, right?
Maybe create a new define for that as well?

Let's just be safe and create new defines to prevent headaches in
the future, if not for futher expansion then for the sake of
legibility.

Thanks,
Levente Kurusa


signature.asc
Description: Digital signature

Re: [PATCH 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Levente Kurusa]

Hi,

On Sun, Apr 27, 2014 at 07:11:16PM +0200, Dominique van den Broeck wrote:
 . userspace pointer dereference ;
 . missing inclusions of needed header files ;
 . unrequired static function declaration (confusing another *.c file).
 
 Signed-off-by: Dominique van den Broeck domdev...@free.fr
 ---
 I submit this patch as a result for Task #16 of the Eudyptula Challenge.
 
 diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c 
 b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
 index 498995d..d87cdfa 100644
 --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
 +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
 @@ -17,8 +17,10 @@
   * wlanfae wlan...@realtek.com
  
 **/
  
 +#include linux/uaccess.h
  #include linux/string.h
  #include rtl_core.h
 +#include rtl_wx.h
  
  #define RATE_COUNT 12
  static u32 rtl8192_rates[] = {
 @@ -1130,11 +1132,18 @@ static int r8192_wx_set_PromiscuousMode(struct 
 net_device *dev,
   struct r8192_priv *priv = rtllib_priv(dev);
   struct rtllib_device *ieee = priv-rtllib;
  
 - u32 *info_buf = (u32 *)(wrqu-data.pointer);
 + u32 info_buf[3];
  
 - u32 oid = info_buf[0];
 - u32 bPromiscuousOn = info_buf[1];
 - u32 bFilterSourceStationFrame = info_buf[2];
 + u32 oid;
 + u32 bPromiscuousOn;
 + u32 bFilterSourceStationFrame;
 +
 + if (copy_from_user(info_buf, wrqu-data.pointer, sizeof(info_buf)))
 + return -EFAULT;
 +
 + oid = info_buf[0];
 + bPromiscuousOn = info_buf[1];
 + bFilterSourceStationFrame = info_buf[2];

I guess it would be better to have defines for those instead of
hard-coding the offsets. Also the size of the info_buf array
might change depending on the size of wrqu-data.pointer, right?
Maybe create a new define for that as well?

Let's just be safe and create new defines to prevent headaches in
the future, if not for futher expansion then for the sake of
legibility.

Thanks,
Levente Kurusa


signature.asc
Description: Digital signature

Re: [PATCH] Staging: octeon-usb: fixed a macro coding style issue

[Levente Kurusa]

Hi,

On 04/25/2014 03:40 PM, Nicolas Del Piano wrote:
> Fixed a coding style error, macros with comples values should be enclosed in 
> parenthesis.
> 
> Signed-off-by: Nicolas Del Piano 
> ---
>  drivers/staging/octeon-usb/octeon-hcd.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/octeon-usb/octeon-hcd.c 
> b/drivers/staging/octeon-usb/octeon-hcd.c
> index 8b8ce72..475ecc4 100644
> --- a/drivers/staging/octeon-usb/octeon-hcd.c
> +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> @@ -246,7 +246,7 @@ enum cvmx_usb_pipe_flags {
>  };
> 
>  /* Normal prefetch that use the pref instruction. */
> -#define CVMX_PREFETCH(address, offset) asm volatile ("pref %[type], 
> %[off](%[rbase])" : : [rbase] "d" (address), [off] "I" (offset), [type] "n" 
> (0))
> +#define (CVMX_PREFETCH(address, offset) asm volatile ("pref %[type], 
> %[off](%[rbase])" : : [rbase] "d" (address), [off] "I" (offset), [type] "n" 
> (0)))

Not that way around! :-)

The parantheses should be around the code block not the
name block.

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Staging: octeon-usb: fixed a macro coding style issue

[Levente Kurusa]

Hi,

On 04/25/2014 03:40 PM, Nicolas Del Piano wrote:
 Fixed a coding style error, macros with comples values should be enclosed in 
 parenthesis.
 
 Signed-off-by: Nicolas Del Piano ndel...@gmail.com
 ---
  drivers/staging/octeon-usb/octeon-hcd.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/staging/octeon-usb/octeon-hcd.c 
 b/drivers/staging/octeon-usb/octeon-hcd.c
 index 8b8ce72..475ecc4 100644
 --- a/drivers/staging/octeon-usb/octeon-hcd.c
 +++ b/drivers/staging/octeon-usb/octeon-hcd.c
 @@ -246,7 +246,7 @@ enum cvmx_usb_pipe_flags {
  };
 
  /* Normal prefetch that use the pref instruction. */
 -#define CVMX_PREFETCH(address, offset) asm volatile (pref %[type], 
 %[off](%[rbase]) : : [rbase] d (address), [off] I (offset), [type] n 
 (0))
 +#define (CVMX_PREFETCH(address, offset) asm volatile (pref %[type], 
 %[off](%[rbase]) : : [rbase] d (address), [off] I (offset), [type] n 
 (0)))

Not that way around! :-)

The parantheses should be around the code block not the
name block.

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

2014-04-08 19:29 GMT+02:00 Levente Kurusa :
> Hi,
>
> On 04/08/2014 07:20 PM, Jason Cooper wrote:
>> On Tue, Apr 08, 2014 at 05:42:00PM +0200, Levente Kurusa wrote:
>>> On 04/07/2014 05:20 PM, Jason Cooper wrote:
>>>> On Sat, Apr 05, 2014 at 11:11:02AM +0200, Levente Kurusa wrote:
>>>>> Oh and another suggestion, I think placing it in the bottom-right
>>>>> corner would be better since then we wouldn't overwrite some of
>>>>> the timestamps and messages.
>>>>
>>>> The real text is still sent to the (hopefully written to disk) logs.  If
>>>> a user (or distro) builds with this feature, I would think centered and
>>>> scaled for ease of scanning would be highest priority.
>>>
>>> Yup, I'll be traveling on the train a lot this week, so I'll
>>> have plenty of time to implement scaling and centering. Maybe
>>> we could also implement this:
>>>
>>> qr_oops=center   (center the QR code with scale 1)
>>> qr_oops=center,3 (center the QR code with scale 3)
>>>
>>> 'center' could also be 'topleft', 'bottomright', etc.
>>> Or just remain at the KISS rule? (keep it simple)
>>>
>>> Any objections?
>>
>> KISS. ;-)
>>
>> Iff we find we need the feature later, we can always add qr_oops_pos or
>> similar.
>>
>
> Alright, I'll start the work on that tomorrow.
>
> Maybe I'll also find some time to clean up the library,
> since I guess that should be our primary priority.

Just a quick reminder that scaling was just merged in [0].
I'd highly appreciate feedback. Thanks!

Now that rendering is a bit cleaner I'll start cleaning up
the library. This is what I intend to do next week:

* Extract bitstream.c into a new kernel library. No point
  in restricting this to QR only.

* Rework bitstream to propagate errors, to use a caller
  allocation scheme and remove the ugly OOPness.

* Fix up the QR library so that it propagates errors
  from the new bitstream code.

Any objections?

[0]: 
https://github.com/teobaluta/qr-linux-kernel/commit/70401a9918e0810e7b0784fa6e1bdc766df20352

--
Regards,
Levente Kurusa
PGP: 4EF5D641
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

2014-04-08 19:29 GMT+02:00 Levente Kurusa le...@linux.com:
 Hi,

 On 04/08/2014 07:20 PM, Jason Cooper wrote:
 On Tue, Apr 08, 2014 at 05:42:00PM +0200, Levente Kurusa wrote:
 On 04/07/2014 05:20 PM, Jason Cooper wrote:
 On Sat, Apr 05, 2014 at 11:11:02AM +0200, Levente Kurusa wrote:
 Oh and another suggestion, I think placing it in the bottom-right
 corner would be better since then we wouldn't overwrite some of
 the timestamps and messages.

 The real text is still sent to the (hopefully written to disk) logs.  If
 a user (or distro) builds with this feature, I would think centered and
 scaled for ease of scanning would be highest priority.

 Yup, I'll be traveling on the train a lot this week, so I'll
 have plenty of time to implement scaling and centering. Maybe
 we could also implement this:

 qr_oops=center   (center the QR code with scale 1)
 qr_oops=center,3 (center the QR code with scale 3)

 'center' could also be 'topleft', 'bottomright', etc.
 Or just remain at the KISS rule? (keep it simple)

 Any objections?

 KISS. ;-)

 Iff we find we need the feature later, we can always add qr_oops_pos or
 similar.


 Alright, I'll start the work on that tomorrow.

 Maybe I'll also find some time to clean up the library,
 since I guess that should be our primary priority.

Just a quick reminder that scaling was just merged in [0].
I'd highly appreciate feedback. Thanks!

Now that rendering is a bit cleaner I'll start cleaning up
the library. This is what I intend to do next week:

* Extract bitstream.c into a new kernel library. No point
  in restricting this to QR only.

* Rework bitstream to propagate errors, to use a caller
  allocation scheme and remove the ugly OOPness.

* Fix up the QR library so that it propagates errors
  from the new bitstream code.

Any objections?

[0]: 
https://github.com/teobaluta/qr-linux-kernel/commit/70401a9918e0810e7b0784fa6e1bdc766df20352

--
Regards,
Levente Kurusa
PGP: 4EF5D641
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/08/2014 07:20 PM, Jason Cooper wrote:
> On Tue, Apr 08, 2014 at 05:42:00PM +0200, Levente Kurusa wrote:
>> On 04/07/2014 05:20 PM, Jason Cooper wrote:
>>> On Sat, Apr 05, 2014 at 11:11:02AM +0200, Levente Kurusa wrote:
>>>> Oh and another suggestion, I think placing it in the bottom-right
>>>> corner would be better since then we wouldn't overwrite some of
>>>> the timestamps and messages.
>>>
>>> The real text is still sent to the (hopefully written to disk) logs.  If
>>> a user (or distro) builds with this feature, I would think centered and
>>> scaled for ease of scanning would be highest priority.
>>
>> Yup, I'll be traveling on the train a lot this week, so I'll
>> have plenty of time to implement scaling and centering. Maybe
>> we could also implement this:
>>
>> qr_oops=center   (center the QR code with scale 1)
>> qr_oops=center,3 (center the QR code with scale 3)
>>
>> 'center' could also be 'topleft', 'bottomright', etc.
>> Or just remain at the KISS rule? (keep it simple)
>>
>> Any objections?
> 
> KISS. ;-)
> 
> Iff we find we need the feature later, we can always add qr_oops_pos or
> similar.
> 

Alright, I'll start the work on that tomorrow.

Maybe I'll also find some time to clean up the library,
since I guess that should be our primary priority.

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/07/2014 05:20 PM, Jason Cooper wrote:
> On Sat, Apr 05, 2014 at 11:11:02AM +0200, Levente Kurusa wrote:
>> Or, we could use core_param and simply have 'oops_qr' or
>> 'qr_oops'. In my humble opinion the latter sounds better.
> 
> Ack.  My original suggestion was focused on 0=disable, >0 is scale.  I
> literally pulled the name from my nether-regions.  :-)

Pushed to Teodora. Hopefully she will pull it soon.

> 
>> Oh and another suggestion, I think placing it in the bottom-right
>> corner would be better since then we wouldn't overwrite some of
>> the timestamps and messages.
> 
> The real text is still sent to the (hopefully written to disk) logs.  If
> a user (or distro) builds with this feature, I would think centered and
> scaled for ease of scanning would be highest priority.

Yup, I'll be traveling on the train a lot this week, so I'll
have plenty of time to implement scaling and centering. Maybe
we could also implement this:

qr_oops=center   (center the QR code with scale 1)
qr_oops=center,3 (center the QR code with scale 3)

'center' could also be 'topleft', 'bottomright', etc.
Or just remain at the KISS rule? (keep it simple)

Any objections?

> 
> I don't think there is a 'safe' part of the framebuffer real estate
> where the QR could be written for all scenarios.  Best to make it easy
> to scan.

Yea we also need to prevent it from happening on panics. Currently on
panics, (i.e. exit when init=/bin/sh) will cause half of the QR code
not rendered on screen due to some reason. It looks like it is due
to scrolling, but I am not sure then why doesn't happen when I
do 'echo c > /proc/sysrq-trigger' which as well causes a panic.

Any ideas why this happens?

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/07/2014 05:20 PM, Jason Cooper wrote:
 On Sat, Apr 05, 2014 at 11:11:02AM +0200, Levente Kurusa wrote:
 Or, we could use core_param and simply have 'oops_qr' or
 'qr_oops'. In my humble opinion the latter sounds better.
 
 Ack.  My original suggestion was focused on 0=disable, 0 is scale.  I
 literally pulled the name from my nether-regions.  :-)

Pushed to Teodora. Hopefully she will pull it soon.

 
 Oh and another suggestion, I think placing it in the bottom-right
 corner would be better since then we wouldn't overwrite some of
 the timestamps and messages.
 
 The real text is still sent to the (hopefully written to disk) logs.  If
 a user (or distro) builds with this feature, I would think centered and
 scaled for ease of scanning would be highest priority.

Yup, I'll be traveling on the train a lot this week, so I'll
have plenty of time to implement scaling and centering. Maybe
we could also implement this:

qr_oops=center   (center the QR code with scale 1)
qr_oops=center,3 (center the QR code with scale 3)

'center' could also be 'topleft', 'bottomright', etc.
Or just remain at the KISS rule? (keep it simple)

Any objections?

 
 I don't think there is a 'safe' part of the framebuffer real estate
 where the QR could be written for all scenarios.  Best to make it easy
 to scan.

Yea we also need to prevent it from happening on panics. Currently on
panics, (i.e. exit when init=/bin/sh) will cause half of the QR code
not rendered on screen due to some reason. It looks like it is due
to scrolling, but I am not sure then why doesn't happen when I
do 'echo c  /proc/sysrq-trigger' which as well causes a panic.

Any ideas why this happens?

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/08/2014 07:20 PM, Jason Cooper wrote:
 On Tue, Apr 08, 2014 at 05:42:00PM +0200, Levente Kurusa wrote:
 On 04/07/2014 05:20 PM, Jason Cooper wrote:
 On Sat, Apr 05, 2014 at 11:11:02AM +0200, Levente Kurusa wrote:
 Oh and another suggestion, I think placing it in the bottom-right
 corner would be better since then we wouldn't overwrite some of
 the timestamps and messages.

 The real text is still sent to the (hopefully written to disk) logs.  If
 a user (or distro) builds with this feature, I would think centered and
 scaled for ease of scanning would be highest priority.

 Yup, I'll be traveling on the train a lot this week, so I'll
 have plenty of time to implement scaling and centering. Maybe
 we could also implement this:

 qr_oops=center   (center the QR code with scale 1)
 qr_oops=center,3 (center the QR code with scale 3)

 'center' could also be 'topleft', 'bottomright', etc.
 Or just remain at the KISS rule? (keep it simple)

 Any objections?
 
 KISS. ;-)
 
 Iff we find we need the feature later, we can always add qr_oops_pos or
 similar.
 

Alright, I'll start the work on that tomorrow.

Maybe I'll also find some time to clean up the library,
since I guess that should be our primary priority.

-- 
Regards,
Levente Kurusa
PGP: 4EF5D641
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/04/2014 11:42 PM, Teodora Băluţă wrote:
> On Fri, Apr 4, 2014 at 7:17 PM, Levente Kurusa  wrote:
>> Hi,
>>
>> On 04/04/2014 05:15 PM, Jason Cooper wrote:
>>> On Thu, Apr 03, 2014 at 01:57:04PM -0700, David Lang wrote:
>>>> On Tue, 1 Apr 2014, Jason Cooper wrote:
>>>>
>>>>>> Now I guess we need to think how to make it work without a
>>>>>> framebuffer. I already suggested using the ASCII characters,
>>>>>> but seeing the resolution of this QR code for example (147x147),
>>>>>> made me realize that we can't shuffle that into a 80x25 textmode
>>>>>> display. Any ideas how to fix that or should we just simply depend
>>>>>> on a framebuffer being present?
>>>>>
>>>>> I think depending on the framebuffer being present (via kconfig) is
>>>>> sane.  Folks running old systems know what they're in for, like missing
>>>>> shiny new features. ;-)
>>>>
>>>> First get it working and into acceptable form, but after that, take
>>>> a look at the various ASCII-art tools out there. While the display
>>>> may be limited to 80x25, that's not a hard requirement (and I'd
>>>> happily run systems with a smaller text console if this was an
>>>> option), and then you can look at the possibility of using
>>>> characters that represent more than one pixel per character. While
>>>> this may not be able to render everything perfectly, remember that
>>>> qr codes can include redundancy to correct for bad pixels, you may
>>>> be able to get something working.
>>
>> I am not sure depending on the error recovery is good practice.
>> We also have to take into account that scanners themselves also
>> create noise and may not be perfect. Better reserve the error
>> recovery for those details instead of messing the QR code with
>> characters...
> 
> You do have the option of error recovery for up to 30% recovery (H
> level), but that means the space you get for storing is smaller.
> 
>>
>>>
>>> I'm not sure this will work.  The screen space allocated to a single
>>> character isn't square.  However, the QR pixels are square.  I see a lot
>>> of fragile complexity ahead...
>>>
>>
>> ... not to mention this as well.
>>
>>
>> IMHO supporting textmode is just not worth the effort. Besides,
>> what would we gain from it? Supporting those devices without
>> a framebuffer? Do devices like that even exist anymore? In fact,
>> even to make this you need a screen, and AFAIK most screens come
>> with some kind of a framebuffer to drive them.
> 
> Guys, first things first is cleaning the library up. I haven't managed
> to do anything yet as I am working on my thesis (bachelor's degree,
> yay!). I will do some this weekend and that is removing the kanji mode
> support. So, Levente, pleaso do that parameter thing you mentioned.
> Merging that with the cleanup shouldn't be a problem. :-)

Awesome, good luck on your thesis, take your time, we are not
rushing. :-)

Yea, I began the work on the parameter and scaling but using
'oops.qr=' isn't easy to use in a file called 'print_oops.c'.
Reason is that KBUILD_MODNAME will become 'print_oops' and then
MODULE_PARAM_PREFIX will be 'print_oops.' (note the dot character)
and so the final parameter will be 'print_oops.qr'. I have solved
this with:

#undef MODULE_PARAM_PREFIX
#define MODULE_PARAM_PREFIX "oops."

but I think this is ugly and is a hack. The good solution
would be to change KBUILD_MODNAME to 'oops' but I am not sure
how to do that, since I have little to no knowledge (and
experience) in how kbuild works.

Or, we could use core_param and simply have 'oops_qr' or
'qr_oops'. In my humble opinion the latter sounds better.

Or, there is __setup as well and that could achieve 'oops.qr',
but that is for *very* core stuff and this is probably not *that*
core. :-)

So, yea, if anyone knows how to change KBUILD_MODNAME without
ugly hacks, I would be grateful to be informed.


> 
> I think writing the QR to the frame buffer is the way to do it for
> now. Doing a QR in text mode (as in displaying it, not as previously
> mentioned idea with the link base64 encoding &/ compression) would
> mean that for each square you get an ASCII character filling up your
> screen. To get an idea of how the QR looks on the frame buffer I've
> made a screenshot. That's the whole Oops message being encoded and
> compressed. [0]

I am not sure if we ever wanted to output a link, but yes filling the
screen with ASCII characters and relying on the error recovery to
ensure readability

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/04/2014 11:42 PM, Teodora Băluţă wrote:
 On Fri, Apr 4, 2014 at 7:17 PM, Levente Kurusa le...@linux.com wrote:
 Hi,

 On 04/04/2014 05:15 PM, Jason Cooper wrote:
 On Thu, Apr 03, 2014 at 01:57:04PM -0700, David Lang wrote:
 On Tue, 1 Apr 2014, Jason Cooper wrote:

 Now I guess we need to think how to make it work without a
 framebuffer. I already suggested using the ASCII characters,
 but seeing the resolution of this QR code for example (147x147),
 made me realize that we can't shuffle that into a 80x25 textmode
 display. Any ideas how to fix that or should we just simply depend
 on a framebuffer being present?

 I think depending on the framebuffer being present (via kconfig) is
 sane.  Folks running old systems know what they're in for, like missing
 shiny new features. ;-)

 First get it working and into acceptable form, but after that, take
 a look at the various ASCII-art tools out there. While the display
 may be limited to 80x25, that's not a hard requirement (and I'd
 happily run systems with a smaller text console if this was an
 option), and then you can look at the possibility of using
 characters that represent more than one pixel per character. While
 this may not be able to render everything perfectly, remember that
 qr codes can include redundancy to correct for bad pixels, you may
 be able to get something working.

 I am not sure depending on the error recovery is good practice.
 We also have to take into account that scanners themselves also
 create noise and may not be perfect. Better reserve the error
 recovery for those details instead of messing the QR code with
 characters...
 
 You do have the option of error recovery for up to 30% recovery (H
 level), but that means the space you get for storing is smaller.
 


 I'm not sure this will work.  The screen space allocated to a single
 character isn't square.  However, the QR pixels are square.  I see a lot
 of fragile complexity ahead...


 ... not to mention this as well.


 IMHO supporting textmode is just not worth the effort. Besides,
 what would we gain from it? Supporting those devices without
 a framebuffer? Do devices like that even exist anymore? In fact,
 even to make this you need a screen, and AFAIK most screens come
 with some kind of a framebuffer to drive them.
 
 Guys, first things first is cleaning the library up. I haven't managed
 to do anything yet as I am working on my thesis (bachelor's degree,
 yay!). I will do some this weekend and that is removing the kanji mode
 support. So, Levente, pleaso do that parameter thing you mentioned.
 Merging that with the cleanup shouldn't be a problem. :-)

Awesome, good luck on your thesis, take your time, we are not
rushing. :-)

Yea, I began the work on the parameter and scaling but using
'oops.qr=' isn't easy to use in a file called 'print_oops.c'.
Reason is that KBUILD_MODNAME will become 'print_oops' and then
MODULE_PARAM_PREFIX will be 'print_oops.' (note the dot character)
and so the final parameter will be 'print_oops.qr'. I have solved
this with:

#undef MODULE_PARAM_PREFIX
#define MODULE_PARAM_PREFIX oops.

but I think this is ugly and is a hack. The good solution
would be to change KBUILD_MODNAME to 'oops' but I am not sure
how to do that, since I have little to no knowledge (and
experience) in how kbuild works.

Or, we could use core_param and simply have 'oops_qr' or
'qr_oops'. In my humble opinion the latter sounds better.

Or, there is __setup as well and that could achieve 'oops.qr',
but that is for *very* core stuff and this is probably not *that*
core. :-)

So, yea, if anyone knows how to change KBUILD_MODNAME without
ugly hacks, I would be grateful to be informed.


 
 I think writing the QR to the frame buffer is the way to do it for
 now. Doing a QR in text mode (as in displaying it, not as previously
 mentioned idea with the link base64 encoding / compression) would
 mean that for each square you get an ASCII character filling up your
 screen. To get an idea of how the QR looks on the frame buffer I've
 made a screenshot. That's the whole Oops message being encoded and
 compressed. [0]

I am not sure if we ever wanted to output a link, but yes filling the
screen with ASCII characters and relying on the error recovery to
ensure readability is very bad.

Nice screenshot, I had as well successfully set up a testsuite
with qemu that allows me to test if it displays correctly. I can
share the testsuite if needed.

 
 The problem with frame buffer is that I currently implemented it using
 the generic frame buffer API. There are some issues as mentioned in
 the first post of this RFC [1]. Would making it work with KMS be
 better? Any opinions?

Not sure, since we are already in a very bad situation when the
Oops happens, I think it is better use something that has existed
for ages and seems to be a bit more simple, and has less chance to
fail. Adding a lot of new code to a fragile part of the kernel
is a hotbed for a recursive oops so I would say just

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/04/2014 05:15 PM, Jason Cooper wrote:
> On Thu, Apr 03, 2014 at 01:57:04PM -0700, David Lang wrote:
>> On Tue, 1 Apr 2014, Jason Cooper wrote:
>>
>>>> Now I guess we need to think how to make it work without a
>>>> framebuffer. I already suggested using the ASCII characters,
>>>> but seeing the resolution of this QR code for example (147x147),
>>>> made me realize that we can't shuffle that into a 80x25 textmode
>>>> display. Any ideas how to fix that or should we just simply depend
>>>> on a framebuffer being present?
>>>
>>> I think depending on the framebuffer being present (via kconfig) is
>>> sane.  Folks running old systems know what they're in for, like missing
>>> shiny new features. ;-)
>>
>> First get it working and into acceptable form, but after that, take
>> a look at the various ASCII-art tools out there. While the display
>> may be limited to 80x25, that's not a hard requirement (and I'd
>> happily run systems with a smaller text console if this was an
>> option), and then you can look at the possibility of using
>> characters that represent more than one pixel per character. While
>> this may not be able to render everything perfectly, remember that
>> qr codes can include redundancy to correct for bad pixels, you may
>> be able to get something working.

I am not sure depending on the error recovery is good practice.
We also have to take into account that scanners themselves also
create noise and may not be perfect. Better reserve the error
recovery for those details instead of messing the QR code with
characters...

> 
> I'm not sure this will work.  The screen space allocated to a single
> character isn't square.  However, the QR pixels are square.  I see a lot
> of fragile complexity ahead...
> 

... not to mention this as well.


IMHO supporting textmode is just not worth the effort. Besides,
what would we gain from it? Supporting those devices without
a framebuffer? Do devices like that even exist anymore? In fact,
even to make this you need a screen, and AFAIK most screens come
with some kind of a framebuffer to drive them.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/04/2014 05:12 PM, Jason Cooper wrote:
> On Thu, Apr 03, 2014 at 10:21:39PM +0200, Levente Kurusa wrote:
> ...
>> Oh and I had an idea of adding a new kernel parameter, something
>> like 'qr_oops.*'. (Looking for a better name! :-) )
>> Basically, I thought of the following options so far:
>>
>> *  qr_oops.disable=1  - disable it
>> *  qr_oops.scale=600x600 - scale the qr code so its easier to read
>>with a phone. In my testing I had huge difficulties reading the
>>QR codes, but when scaled to be a bit bigger it worked magically.
>>This might not be so easy to implement this way, but with preset
>>values, i.e. 4x4 squares instead of a pixel, it could work.
> 
> oops.qr=0 - disabled
> oops.qr=3 - make each QR pixel 3x3 screen pixels.
> 
> I've found 3x3 works well for business cards and such.
> 

Yea this makes more sense. I'll go and implement this
right now and send the changes to Teodora once finished.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/04/2014 05:12 PM, Jason Cooper wrote:
 On Thu, Apr 03, 2014 at 10:21:39PM +0200, Levente Kurusa wrote:
 ...
 Oh and I had an idea of adding a new kernel parameter, something
 like 'qr_oops.*'. (Looking for a better name! :-) )
 Basically, I thought of the following options so far:

 *  qr_oops.disable=1  - disable it
 *  qr_oops.scale=600x600 - scale the qr code so its easier to read
with a phone. In my testing I had huge difficulties reading the
QR codes, but when scaled to be a bit bigger it worked magically.
This might not be so easy to implement this way, but with preset
values, i.e. 4x4 squares instead of a pixel, it could work.
 
 oops.qr=0 - disabled
 oops.qr=3 - make each QR pixel 3x3 screen pixels.
 
 I've found 3x3 works well for business cards and such.
 

Yea this makes more sense. I'll go and implement this
right now and send the changes to Teodora once finished.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 04/04/2014 05:15 PM, Jason Cooper wrote:
 On Thu, Apr 03, 2014 at 01:57:04PM -0700, David Lang wrote:
 On Tue, 1 Apr 2014, Jason Cooper wrote:

 Now I guess we need to think how to make it work without a
 framebuffer. I already suggested using the ASCII characters,
 but seeing the resolution of this QR code for example (147x147),
 made me realize that we can't shuffle that into a 80x25 textmode
 display. Any ideas how to fix that or should we just simply depend
 on a framebuffer being present?

 I think depending on the framebuffer being present (via kconfig) is
 sane.  Folks running old systems know what they're in for, like missing
 shiny new features. ;-)

 First get it working and into acceptable form, but after that, take
 a look at the various ASCII-art tools out there. While the display
 may be limited to 80x25, that's not a hard requirement (and I'd
 happily run systems with a smaller text console if this was an
 option), and then you can look at the possibility of using
 characters that represent more than one pixel per character. While
 this may not be able to render everything perfectly, remember that
 qr codes can include redundancy to correct for bad pixels, you may
 be able to get something working.

I am not sure depending on the error recovery is good practice.
We also have to take into account that scanners themselves also
create noise and may not be perfect. Better reserve the error
recovery for those details instead of messing the QR code with
characters...

 
 I'm not sure this will work.  The screen space allocated to a single
 character isn't square.  However, the QR pixels are square.  I see a lot
 of fragile complexity ahead...
 

... not to mention this as well.


IMHO supporting textmode is just not worth the effort. Besides,
what would we gain from it? Supporting those devices without
a framebuffer? Do devices like that even exist anymore? In fact,
even to make this you need a screen, and AFAIK most screens come
with some kind of a framebuffer to drive them.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

2014-04-01 23:07 GMT+02:00 Teodora Băluţă :
> On Tue, Apr 1, 2014 at 5:20 PM, Jason Cooper  wrote:
>> On Sun, Mar 30, 2014 at 12:17:17PM +0200, Levente Kurusa wrote:
>>> Hi all,
>>>
>>> (sorry for the late reply, looks like this mail has ran away from my 
>>> clients)
>
> same here.
>
>>>
>>> 2014-03-23 20:38 GMT+01:00 Jason Cooper :
>>> > All,
>>> >
>>> > On Sat, Mar 22, 2014 at 08:20:01PM +0200, Teodora Băluţă wrote:
>>> >> On Sat, Mar 22, 2014 at 7:09 PM, Levente Kurusa  wrote:
>>> >> > On 03/21/2014 02:28 PM, Jason Cooper wrote:
>>> > ...
>>> >> >> I would definitely like to see the QR output incorporated into a
>>> >> >> kernel.org url.  That would remove the need for installing another 
>>> >> >> app,
>>> >> >> and would ease bug reporting.
>>> >> >
>>> >> > I still struggle to understand how could that be done. We can encode 
>>> >> > the
>>> >> > QR code as ASCII. Okay, that's fine, however it is very long. Encoding
>>> >> > 'Unable to handle kernel paging request at 000f' gave a 449 
>>> >> > character
>>> >> > long sequence with very strange characters [0]. We should try to 
>>> >> > shorten
>>> >> > it, imho. Not sure how to do that though.
>>> >
>>> > The man page for qrencode says you can have up to 4000 characters in a
>>> > qrcode.  However, I've seen readers have trouble with a 2048bit ascii
>>> > armored PGP public key (3929 characters).
>>> >
>>> > I grabbed a random oops from oops.kernel.org, it weighed in at 1544
>>> > bytes, not too bad.  I then did:
>>> >
>>> > $ echo "https://oops.kernel.org/?qr=`cat oops.txt | gzip -9 | base64 
>>> > -wrap=0`" | wc -c
>>> > 993
>>>
>>> I did the same with another OOPS and it had 1953 characters. That's quite a 
>>> big
>>> a big difference! :-)
>>>
>>> I created a QR image from the URL then, and it was 147x147, which is
>>> pretty small.
>>> It took me quite a long time to make my phone recognize it, but it
>>> worked nicely.
>>>
>>> Result of work is in this directory:
>>>
>>> http://levex.fedorapeople.org/kernel/qr/
>>
>> nice!
>>
>>> > The benefit of a url is that any QR reader can automagically report an
>>> > oops.  While a specific app could parse the URL/oops locally if the
>>> > user desires.
>>> >
>>> >> it misses the point of having a QR code in the first place. The way I
>>> >> see it, having a QR decoder app installed that can do an offline
>>> >> decoding is a less greater effort than popping out a browser on the
>>> >> machine you're working on.
>>> >
>>> > I think you're selling the advantage of the QR code short.  Automated
>>> > reporting (via the url) is a _huge_ plus.  The app you conceive of could
>>> > still parse it in place if the user desires.
>>> >
>>> > My point for the URL isn't to use the internet/server to automate oops
>>> > parsing for the user.  Rather it's to make it easy to report oopses to
>>> > developers.  While still preserving the ability of your app to parse it
>>> > for the user.
>>>
>>> Ah I see now. oops.kernel.org/?qr= would simply parse the
>>> base64'd+gzip'd oops message and then report it.
>>
>> If you mean the server behind oops.k.o would parse it, then yes.  No
>> special app should be required other than a QR code scanner for the
>> usecase of reporting oopses to developers.

Yup, clear now.

>>
>>> Now I guess we need to think how to make it work without a
>>> framebuffer. I already suggested using the ASCII characters,
>>> but seeing the resolution of this QR code for example (147x147),
>>> made me realize that we can't shuffle that into a 80x25 textmode
>>> display. Any ideas how to fix that or should we just simply depend
>>> on a framebuffer being present?
>>
>> I think depending on the framebuffer being present (via kconfig) is
>> sane.  Folks running old systems know what they're in for, like missing
>> shiny new features. ;-)
>
> Ok, this may work. I agree that doing this with the help of the frame
> buffer is more natural.

Okay, Teodora I'll send you a commit A

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

2014-04-01 23:07 GMT+02:00 Teodora Băluţă teobal...@gmail.com:
 On Tue, Apr 1, 2014 at 5:20 PM, Jason Cooper ja...@lakedaemon.net wrote:
 On Sun, Mar 30, 2014 at 12:17:17PM +0200, Levente Kurusa wrote:
 Hi all,

 (sorry for the late reply, looks like this mail has ran away from my 
 clients)

 same here.


 2014-03-23 20:38 GMT+01:00 Jason Cooper ja...@lakedaemon.net:
  All,
 
  On Sat, Mar 22, 2014 at 08:20:01PM +0200, Teodora Băluţă wrote:
  On Sat, Mar 22, 2014 at 7:09 PM, Levente Kurusa le...@linux.com wrote:
   On 03/21/2014 02:28 PM, Jason Cooper wrote:
  ...
   I would definitely like to see the QR output incorporated into a
   kernel.org url.  That would remove the need for installing another 
   app,
   and would ease bug reporting.
  
   I still struggle to understand how could that be done. We can encode 
   the
   QR code as ASCII. Okay, that's fine, however it is very long. Encoding
   'Unable to handle kernel paging request at 000f' gave a 449 
   character
   long sequence with very strange characters [0]. We should try to 
   shorten
   it, imho. Not sure how to do that though.
 
  The man page for qrencode says you can have up to 4000 characters in a
  qrcode.  However, I've seen readers have trouble with a 2048bit ascii
  armored PGP public key (3929 characters).
 
  I grabbed a random oops from oops.kernel.org, it weighed in at 1544
  bytes, not too bad.  I then did:
 
  $ echo https://oops.kernel.org/?qr=`cat oops.txt | gzip -9 | base64 
  -wrap=0` | wc -c
  993

 I did the same with another OOPS and it had 1953 characters. That's quite a 
 big
 a big difference! :-)

 I created a QR image from the URL then, and it was 147x147, which is
 pretty small.
 It took me quite a long time to make my phone recognize it, but it
 worked nicely.

 Result of work is in this directory:

 http://levex.fedorapeople.org/kernel/qr/

 nice!

  The benefit of a url is that any QR reader can automagically report an
  oops.  While a specific app could parse the URL/oops locally if the
  user desires.
 
  it misses the point of having a QR code in the first place. The way I
  see it, having a QR decoder app installed that can do an offline
  decoding is a less greater effort than popping out a browser on the
  machine you're working on.
 
  I think you're selling the advantage of the QR code short.  Automated
  reporting (via the url) is a _huge_ plus.  The app you conceive of could
  still parse it in place if the user desires.
 
  My point for the URL isn't to use the internet/server to automate oops
  parsing for the user.  Rather it's to make it easy to report oopses to
  developers.  While still preserving the ability of your app to parse it
  for the user.

 Ah I see now. oops.kernel.org/?qr=QR would simply parse the
 base64'd+gzip'd oops message and then report it.

 If you mean the server behind oops.k.o would parse it, then yes.  No
 special app should be required other than a QR code scanner for the
 usecase of reporting oopses to developers.

Yup, clear now.


 Now I guess we need to think how to make it work without a
 framebuffer. I already suggested using the ASCII characters,
 but seeing the resolution of this QR code for example (147x147),
 made me realize that we can't shuffle that into a 80x25 textmode
 display. Any ideas how to fix that or should we just simply depend
 on a framebuffer being present?

 I think depending on the framebuffer being present (via kconfig) is
 sane.  Folks running old systems know what they're in for, like missing
 shiny new features. ;-)

 Ok, this may work. I agree that doing this with the help of the frame
 buffer is more natural.

Okay, Teodora I'll send you a commit ASAP and then
we should start working on getting libqr into an acceptable
state. I am not sure if we can shuffle it into staging though,
it's not a driver and AFAIK staging.git is for drivers which aren't
yet finished. So, I would say, let's start working on fixing the
OOPness of libqr first. If we were to rework that, then
we can as well avoid having GFP_ATOMIC in library code
by using the more conventional kmalloc(struct); init_struct(ptr);
scheme.

Oh and I had an idea of adding a new kernel parameter, something
like 'qr_oops.*'. (Looking for a better name! :-) )
Basically, I thought of the following options so far:

*  qr_oops.disable=1  - disable it
*  qr_oops.scale=600x600 - scale the qr code so its easier to read
   with a phone. In my testing I had huge difficulties reading the
   QR codes, but when scaled to be a bit bigger it worked magically.
   This might not be so easy to implement this way, but with preset
   values, i.e. 4x4 squares instead of a pixel, it could work.

Objections, ideas are welcome!

Teodora, have you worked on anything recently in qr-linux-kernel?
Just to make sure we aren't working on the same.

Thanks,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More

[PATCH RESEND] tc: account for device_register() failure

[Levente Kurusa]

This patch makes the TURBOchannel driver bail out if the call
to device_register() failed.

Signed-off-by: Levente Kurusa 
---
Resending as per Maciej's request to proper list and
people.
---
 tc.c |   10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/tc/tc.c b/drivers/tc/tc.c
index a8aaf6a..6b3a038 100644
--- a/drivers/tc/tc.c
+++ b/drivers/tc/tc.c
@@ -129,7 +129,10 @@ static void __init tc_bus_add_devices(struct tc_bus *tbus)

tc_device_get_irq(tdev);

-   device_register(>dev);
+   if (device_register(>dev)) {
+   put_device(>dev);
+   goto out_err;
+   }
list_add_tail(>node, >devices);

 out_err:
@@ -148,7 +151,10 @@ static int __init tc_init(void)

INIT_LIST_HEAD(_bus.devices);
dev_set_name(_bus.dev, "tc");
-   device_register(_bus.dev);
+   if (device_register(_bus.dev)) {
+   put_device(_bus.dev);
+   return 0;   
+   }

if (tc_bus.info.slot_size) {
unsigned int tc_clock = tc_get_speed(_bus) / 10;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH RESEND] tc: account for device_register() failure

[Levente Kurusa]

This patch makes the TURBOchannel driver bail out if the call
to device_register() failed.

Signed-off-by: Levente Kurusa le...@linux.com
---
Resending as per Maciej's request to proper list and
people.
---
 tc.c |   10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/tc/tc.c b/drivers/tc/tc.c
index a8aaf6a..6b3a038 100644
--- a/drivers/tc/tc.c
+++ b/drivers/tc/tc.c
@@ -129,7 +129,10 @@ static void __init tc_bus_add_devices(struct tc_bus *tbus)

tc_device_get_irq(tdev);

-   device_register(tdev-dev);
+   if (device_register(tdev-dev)) {
+   put_device(tdev-dev);
+   goto out_err;
+   }
list_add_tail(tdev-node, tbus-devices);

 out_err:
@@ -148,7 +151,10 @@ static int __init tc_init(void)

INIT_LIST_HEAD(tc_bus.devices);
dev_set_name(tc_bus.dev, tc);
-   device_register(tc_bus.dev);
+   if (device_register(tc_bus.dev)) {
+   put_device(tc_bus.dev);
+   return 0;   
+   }

if (tc_bus.info.slot_size) {
unsigned int tc_clock = tc_get_speed(tc_bus) / 10;

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tc: account for device_register() failure

[Levente Kurusa]

Hi,

2014-04-01 2:30 GMT+02:00 Maciej W. Rozycki :
> On Fri, 15 Nov 2013, Levente Kurusa wrote:
>
>> This patch makes the TURBOchannel driver bail out if the call
>> to device_register() failed.
>>
>> Signed-off-by: Levente Kurusa 
>
> Acked-by: Maciej W. Rozycki 
>
> This fixes some build warnings:
>
> drivers/tc/tc.c: In function 'tc_bus_add_devices':
> drivers/tc/tc.c:132: warning: ignoring return value of 'device_register',
> declared with attribute warn_unused_result
> drivers/tc/tc.c: In function 'tc_init':
> drivers/tc/tc.c:151: warning: ignoring return value of 'device_register',
> declared with attribute warn_unused_result
>
> Levente, thanks for your fix and apologies for the long RTT -- can you
> please resend your patch to  and Ralf so that
> it'll be pulled via the MIPS tree?  I'll post a follow-up update to fix
> some issues with `tc_init' that I noticed thanks to your change.

Sure, I will repost in a few hours, I just need to get home.
Thanks for the Ack!

> [...]

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tc: account for device_register() failure

[Levente Kurusa]

Hi,

2014-04-01 2:30 GMT+02:00 Maciej W. Rozycki ma...@linux-mips.org:
 On Fri, 15 Nov 2013, Levente Kurusa wrote:

 This patch makes the TURBOchannel driver bail out if the call
 to device_register() failed.

 Signed-off-by: Levente Kurusa le...@linux.com

 Acked-by: Maciej W. Rozycki ma...@linux-mips.org

 This fixes some build warnings:

 drivers/tc/tc.c: In function 'tc_bus_add_devices':
 drivers/tc/tc.c:132: warning: ignoring return value of 'device_register',
 declared with attribute warn_unused_result
 drivers/tc/tc.c: In function 'tc_init':
 drivers/tc/tc.c:151: warning: ignoring return value of 'device_register',
 declared with attribute warn_unused_result

 Levente, thanks for your fix and apologies for the long RTT -- can you
 please resend your patch to linux-m...@linux-mips.org and Ralf so that
 it'll be pulled via the MIPS tree?  I'll post a follow-up update to fix
 some issues with `tc_init' that I noticed thanks to your change.

Sure, I will repost in a few hours, I just need to get home.
Thanks for the Ack!

 [...]

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi all,

(sorry for the late reply, looks like this mail has ran away from my clients)

2014-03-23 20:38 GMT+01:00 Jason Cooper :
> All,
>
> On Sat, Mar 22, 2014 at 08:20:01PM +0200, Teodora Băluţă wrote:
>> On Sat, Mar 22, 2014 at 7:09 PM, Levente Kurusa  wrote:
>> > On 03/21/2014 02:28 PM, Jason Cooper wrote:
> ...
>> >> I would definitely like to see the QR output incorporated into a
>> >> kernel.org url.  That would remove the need for installing another app,
>> >> and would ease bug reporting.
>> >
>> > I still struggle to understand how could that be done. We can encode the
>> > QR code as ASCII. Okay, that's fine, however it is very long. Encoding
>> > 'Unable to handle kernel paging request at 000f' gave a 449 character
>> > long sequence with very strange characters [0]. We should try to shorten
>> > it, imho. Not sure how to do that though.
>
> The man page for qrencode says you can have up to 4000 characters in a
> qrcode.  However, I've seen readers have trouble with a 2048bit ascii
> armored PGP public key (3929 characters).
>
> I grabbed a random oops from oops.kernel.org, it weighed in at 1544
> bytes, not too bad.  I then did:
>
> $ echo "https://oops.kernel.org/?qr=`cat oops.txt | gzip -9 | base64 
> -wrap=0`" | wc -c
> 993

I did the same with another OOPS and it had 1953 characters. That's quite a big
a big difference! :-)

I created a QR image from the URL then, and it was 147x147, which is
pretty small.
It took me quite a long time to make my phone recognize it, but it
worked nicely.

Result of work is in this directory:

http://levex.fedorapeople.org/kernel/qr/


>
> The benefit of a url is that any QR reader can automagically report an
> oops.  While a specific app could parse the URL/oops locally if the
> user desires.
>
>> it misses the point of having a QR code in the first place. The way I
>> see it, having a QR decoder app installed that can do an offline
>> decoding is a less greater effort than popping out a browser on the
>> machine you're working on.
>
> I think you're selling the advantage of the QR code short.  Automated
> reporting (via the url) is a _huge_ plus.  The app you conceive of could
> still parse it in place if the user desires.
>
> My point for the URL isn't to use the internet/server to automate oops
> parsing for the user.  Rather it's to make it easy to report oopses to
> developers.  While still preserving the ability of your app to parse it
> for the user.

Ah I see now. oops.kernel.org/?qr= would simply parse the
base64'd+gzip'd oops message and then report it.

Now I guess we need to think how to make it work without a
framebuffer. I already suggested using the ASCII characters,
but seeing the resolution of this QR code for example (147x147),
made me realize that we can't shuffle that into a 80x25 textmode
display. Any ideas how to fix that or should we just simply depend
on a framebuffer being present?

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi all,

(sorry for the late reply, looks like this mail has ran away from my clients)

2014-03-23 20:38 GMT+01:00 Jason Cooper ja...@lakedaemon.net:
 All,

 On Sat, Mar 22, 2014 at 08:20:01PM +0200, Teodora Băluţă wrote:
 On Sat, Mar 22, 2014 at 7:09 PM, Levente Kurusa le...@linux.com wrote:
  On 03/21/2014 02:28 PM, Jason Cooper wrote:
 ...
  I would definitely like to see the QR output incorporated into a
  kernel.org url.  That would remove the need for installing another app,
  and would ease bug reporting.
 
  I still struggle to understand how could that be done. We can encode the
  QR code as ASCII. Okay, that's fine, however it is very long. Encoding
  'Unable to handle kernel paging request at 000f' gave a 449 character
  long sequence with very strange characters [0]. We should try to shorten
  it, imho. Not sure how to do that though.

 The man page for qrencode says you can have up to 4000 characters in a
 qrcode.  However, I've seen readers have trouble with a 2048bit ascii
 armored PGP public key (3929 characters).

 I grabbed a random oops from oops.kernel.org, it weighed in at 1544
 bytes, not too bad.  I then did:

 $ echo https://oops.kernel.org/?qr=`cat oops.txt | gzip -9 | base64 
 -wrap=0` | wc -c
 993

I did the same with another OOPS and it had 1953 characters. That's quite a big
a big difference! :-)

I created a QR image from the URL then, and it was 147x147, which is
pretty small.
It took me quite a long time to make my phone recognize it, but it
worked nicely.

Result of work is in this directory:

http://levex.fedorapeople.org/kernel/qr/



 The benefit of a url is that any QR reader can automagically report an
 oops.  While a specific app could parse the URL/oops locally if the
 user desires.

 it misses the point of having a QR code in the first place. The way I
 see it, having a QR decoder app installed that can do an offline
 decoding is a less greater effort than popping out a browser on the
 machine you're working on.

 I think you're selling the advantage of the QR code short.  Automated
 reporting (via the url) is a _huge_ plus.  The app you conceive of could
 still parse it in place if the user desires.

 My point for the URL isn't to use the internet/server to automate oops
 parsing for the user.  Rather it's to make it easy to report oopses to
 developers.  While still preserving the ability of your app to parse it
 for the user.

Ah I see now. oops.kernel.org/?qr=QR would simply parse the
base64'd+gzip'd oops message and then report it.

Now I guess we need to think how to make it work without a
framebuffer. I already suggested using the ASCII characters,
but seeing the resolution of this QR code for example (147x147),
made me realize that we can't shuffle that into a 80x25 textmode
display. Any ideas how to fix that or should we just simply depend
on a framebuffer being present?

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] initramfs: print error and shell out for unsupported content

[Levente Kurusa]

Hi,

2014-03-26 22:16 GMT+01:00 Alexander Holler :
> Am 22.03.2014 00:07, schrieb Alexander Holler:
>>
>> Am 21.03.2014 23:55, schrieb Andrew Morton:
>>
>>> On Fri, 21 Mar 2014 23:49:57 +0100 Alexander Holler
>>>  wrote:
>>>
>>>> Am 21.03.2014 22:03, schrieb Andrew Morton:
>>>>>
>>>>> On Thu, 20 Mar 2014 23:00:45 +0100 Alexander Holler
>>>>>  wrote:
>>>>>
>>>>>> The initramfs generation is broken for file and directory names which
>>>>>> contain
>>>>>> colons or spaces. Print an error and don't try to continue.
>>>>
>>>>
>>>>> It would be better to fix the it-doesnt-work-with-all-filenames bug.
>>>>> Any details on that?
>>>>
>>>>
>>>> IMHO not worth the time. The whole process which is curently used is
>>>> extremly fragile.
>>>>
>>>> E.g it's almost guaranteed to fail trying to include arbitrary filenames
>>>> as dependencies in a Makefile. Besides the one problem I've discoverd
>>>> with colons, there could be much more things happen, e.g. with filenames
>>>> which do include other special Makefile characters you all would have to
>>>> escape correctly.
>
>
> To be a bit more verbose, (gnu)make doesn't support quoted filenames. That
> means one has to escape all kind of characters which might have a special
> meaning for make. Not really doable. Furthermore escaped characters are not
> very well supported by make and are usable only by a few rules.
>
>
>>>> And the problem with spaces isn't as easy to fix as it first does look
>>>> like. I think it might be easier to write the whole stuff new instead of
>>>> trying to escape the spaces in various ways needed to end up correctly
>>>> in the cpio (it first goes through shell code and is then feeded as some
>>>> list to a C program).
>
>
> To be a bit more verbose here, the shell script
> (scripts/gen_initramfs_list.sh) identifes filenames based on the position in
> a string which is delimited by the usual (shell) whitespace. E.g. $1 =
> filename, $2 = mode, ...
> And this stuff is then feeded to usr/gen_init_cpio.c,which uses a similiar
> approach like
>
> sscanf(line, "%" str(PATH_MAX) "s %" str(PATH_MAX) "s %o %d %d", name,
> target, ...
>
> which obviously fails for filenames with whitspace too.
>
> What I think might be reasonable is:
>
> - get rid of the dependency list in form of a include into the Makefile and
> just generate the cpio-archive every time make is called. Common initramfs
> sizes are about a few megabytes and with today machines such a cpio-archive
> is build in about a second,
>

I don't understand what kind of include would you want.

> - get rid of gen_initramfs_list.sh and rewrite gen_init_cpio.c such that it
> reads the filenames, modes and similiar itself (e.g. by using stat(2)).

This is walkable but probably not worth the effort. Besides, why would
anyone want to put spaces, colons and arbitrary characters to filenames
in the initramfs?

>
>
> But I don't have any motivation to do so. Sorry.
>
> Maybe Levente Kurusa is interested in doing so, he seems to have an interest
> in the topic as he was quiet fast to send an answer to my patch.

All I wonder now is why are you being so arrogant. You are not a 'remote
keyboard' as you had called yourself, since when you send a patch
you not only have to send it but communicate WHY maintainers
should take that patch, and if somebody has a comment
spotting an obvious mistake you shouldn't become angry.
It was only a simple typo and a suggestion. Nobody told you
to blindly accept my suggestion. Everybody can have opinions.
It's open-source, the community suggests you new changes,
it's normal, get used to it. Writing up this nonsense sentence
(I feel the irony) probably took more time than actually resending
the patch did.

Just my two cents.

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] initramfs: print error and shell out for unsupported content

[Levente Kurusa]

Hi,

2014-03-26 22:16 GMT+01:00 Alexander Holler hol...@ahsoftware.de:
 Am 22.03.2014 00:07, schrieb Alexander Holler:

 Am 21.03.2014 23:55, schrieb Andrew Morton:

 On Fri, 21 Mar 2014 23:49:57 +0100 Alexander Holler
 hol...@ahsoftware.de wrote:

 Am 21.03.2014 22:03, schrieb Andrew Morton:

 On Thu, 20 Mar 2014 23:00:45 +0100 Alexander Holler
 hol...@ahsoftware.de wrote:

 The initramfs generation is broken for file and directory names which
 contain
 colons or spaces. Print an error and don't try to continue.


 It would be better to fix the it-doesnt-work-with-all-filenames bug.
 Any details on that?


 IMHO not worth the time. The whole process which is curently used is
 extremly fragile.

 E.g it's almost guaranteed to fail trying to include arbitrary filenames
 as dependencies in a Makefile. Besides the one problem I've discoverd
 with colons, there could be much more things happen, e.g. with filenames
 which do include other special Makefile characters you all would have to
 escape correctly.


 To be a bit more verbose, (gnu)make doesn't support quoted filenames. That
 means one has to escape all kind of characters which might have a special
 meaning for make. Not really doable. Furthermore escaped characters are not
 very well supported by make and are usable only by a few rules.


 And the problem with spaces isn't as easy to fix as it first does look
 like. I think it might be easier to write the whole stuff new instead of
 trying to escape the spaces in various ways needed to end up correctly
 in the cpio (it first goes through shell code and is then feeded as some
 list to a C program).


 To be a bit more verbose here, the shell script
 (scripts/gen_initramfs_list.sh) identifes filenames based on the position in
 a string which is delimited by the usual (shell) whitespace. E.g. $1 =
 filename, $2 = mode, ...
 And this stuff is then feeded to usr/gen_init_cpio.c,which uses a similiar
 approach like

 sscanf(line, % str(PATH_MAX) s % str(PATH_MAX) s %o %d %d, name,
 target, ...

 which obviously fails for filenames with whitspace too.

 What I think might be reasonable is:

 - get rid of the dependency list in form of a include into the Makefile and
 just generate the cpio-archive every time make is called. Common initramfs
 sizes are about a few megabytes and with today machines such a cpio-archive
 is build in about a second,


I don't understand what kind of include would you want.

 - get rid of gen_initramfs_list.sh and rewrite gen_init_cpio.c such that it
 reads the filenames, modes and similiar itself (e.g. by using stat(2)).

This is walkable but probably not worth the effort. Besides, why would
anyone want to put spaces, colons and arbitrary characters to filenames
in the initramfs?



 But I don't have any motivation to do so. Sorry.

 Maybe Levente Kurusa is interested in doing so, he seems to have an interest
 in the topic as he was quiet fast to send an answer to my patch.

All I wonder now is why are you being so arrogant. You are not a 'remote
keyboard' as you had called yourself, since when you send a patch
you not only have to send it but communicate WHY maintainers
should take that patch, and if somebody has a comment
spotting an obvious mistake you shouldn't become angry.
It was only a simple typo and a suggestion. Nobody told you
to blindly accept my suggestion. Everybody can have opinions.
It's open-source, the community suggests you new changes,
it's normal, get used to it. Writing up this nonsense sentence
(I feel the irony) probably took more time than actually resending
the patch did.

Just my two cents.

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 03/22/2014 07:29 PM, Levente Kurusa wrote:
> Hi,
> 
> On 03/22/2014 07:20 PM, Teodora Băluţă wrote:
>> On Sat, Mar 22, 2014 at 7:09 PM, Levente Kurusa  wrote:
>>> Hi,
>>>
>>> On 03/21/2014 02:28 PM, Jason Cooper wrote:
>>>> On Wed, Mar 19, 2014 at 10:38:30PM +0200, Teodora Băluţă wrote:
>>>>> On Wed, Mar 19, 2014 at 10:18 PM, Dave Jones  wrote:
>>>>>> On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
>>>>>>  > This feature encodes Oops messages into a QR barcode that is 
>>>>>> scannable by
>>>>>>  > any device with a camera.
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>> That's a ton of code we're adding into one of the most fragile parts of 
>>>>>> the kernel.
>>>>>>
>>>>>> A lot of what libqrencode does would seem to be superfluous to the 
>>>>>> requirements
>>>>>> here, as we don't output kernel oopses in kanji for eg, and won't care 
>>>>>> about
>>>>>> multiple versions of the qr spec.
>>>>>
>>>>> That's true. I didn't do that much cleanup in the library afraid of
>>>>> breaking something and focused that I get this done one way or
>>>>> another. Indeed, the library is userspace and is made to be versatile
>>>>> rather than small.
>>>>
>>>> Perhaps you could add libqr to the staging tree?  As long as it
>>>> compiles, it can go there.  Then you can focus on cleanups and bloat
>>>> removal.  In the process, you'll get a larger testing base because it
>>>> will be in mainline.
>>>
>>> Yea that is a better way. However, the current state of the code has
>>> several problems:
>>>
>>> * No good error handling (simply returns -1 on failure no matter what)
>>>I have began converting this to the ERR_PTR et al interface
>>>However, I have not yet done this fully due to the vast amount
>>>of work required to do so.
>>>This shouldn't be yet merged, but I shall send it as patches once
>>>it gets into the staging tree.
>>>
>>> * All memory allocations are GFP_ATOMIC for no reason.
>>>I have converted them to GFP_KERNEL since we can block safely.
>>>This could be merged to Teodora's branch. I can send her a pull
>>>request on GitHub if she wants so.
>>
>> Since we are talking about some kernel Oopses I thought that making
>> this GFP_ATOMIC ensures that we get memory allocation. I have
>> considered using GFP_KERNEL, but I am not very sure about that.
>> Probably somewhere deep inside I wanted to make it work even for
>> panics. :(
> 
> Yea that makes sense, realized that just after I sent the mail.
> However, since this is in lib/ other parts might want to be
> able to use it and for them GFP_KERNEL makes more sense.

In order to make libqr usable in other parts of the kernel,
we need to heavily restructure libqr. It seems to use a style
that was inherited from OOP. Since we are in C, I don't like it.
I think that the allocations for the structs (e.g. QRinput) should
be done by the caller. That way we could remove most of the
allocations from libqr, and the rest may use GFP_KERNEL if they
still exist after that cleanup. Any objections?

>>>
>>> [...] 
>>>
>>> * I had trouble getting QR output.
>>>Doing 'echo c > /proc/sysrq-trigger' triggered a crash,
>>>    but it resulted in a recursive OOPS. This is a nullptr-deref
>>>and hence I think it may be related to the fact I was running
>>>it in textmode. :-)
>>>Or, it is due to the bugged error handling.
>>
>> The QR output is written to the frame buffer. That means you have to
>> get acces to a console. As I mentioned in the RFC, I am looking for an
>> alternative to using fb.h since that doesn't seem to work very well
>> atm.

Okay, I sent you a pull request that fixes a recursive OOPS when crashing
in textmode and no higher mode is available. Patch attached as well [0].

Also, have you check ASCII characters 219-223? Maybe they are usable?
Any ideas?

[0]:

%<-
From: Levente Kurusa 
Subject: [PATCH] qr: print_oops: don't try to print if there is no framebuffer

If the kernel boots in textmode, we would hit a null pointer derefernce
in compute_w(). Prevent this by actually checking the value of info.

Signed-off-by: Levente Kurusa 
---
 kernel/print_oops.c | 5 +
 1 file changed, 5 insertions(+)

diff --

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 03/22/2014 07:29 PM, Levente Kurusa wrote:
 Hi,
 
 On 03/22/2014 07:20 PM, Teodora Băluţă wrote:
 On Sat, Mar 22, 2014 at 7:09 PM, Levente Kurusa le...@linux.com wrote:
 Hi,

 On 03/21/2014 02:28 PM, Jason Cooper wrote:
 On Wed, Mar 19, 2014 at 10:38:30PM +0200, Teodora Băluţă wrote:
 On Wed, Mar 19, 2014 at 10:18 PM, Dave Jones da...@redhat.com wrote:
 On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
   This feature encodes Oops messages into a QR barcode that is 
 scannable by
   any device with a camera.

 [...]

 That's a ton of code we're adding into one of the most fragile parts of 
 the kernel.

 A lot of what libqrencode does would seem to be superfluous to the 
 requirements
 here, as we don't output kernel oopses in kanji for eg, and won't care 
 about
 multiple versions of the qr spec.

 That's true. I didn't do that much cleanup in the library afraid of
 breaking something and focused that I get this done one way or
 another. Indeed, the library is userspace and is made to be versatile
 rather than small.

 Perhaps you could add libqr to the staging tree?  As long as it
 compiles, it can go there.  Then you can focus on cleanups and bloat
 removal.  In the process, you'll get a larger testing base because it
 will be in mainline.

 Yea that is a better way. However, the current state of the code has
 several problems:

 * No good error handling (simply returns -1 on failure no matter what)
I have began converting this to the ERR_PTR et al interface
However, I have not yet done this fully due to the vast amount
of work required to do so.
This shouldn't be yet merged, but I shall send it as patches once
it gets into the staging tree.

 * All memory allocations are GFP_ATOMIC for no reason.
I have converted them to GFP_KERNEL since we can block safely.
This could be merged to Teodora's branch. I can send her a pull
request on GitHub if she wants so.

 Since we are talking about some kernel Oopses I thought that making
 this GFP_ATOMIC ensures that we get memory allocation. I have
 considered using GFP_KERNEL, but I am not very sure about that.
 Probably somewhere deep inside I wanted to make it work even for
 panics. :(
 
 Yea that makes sense, realized that just after I sent the mail.
 However, since this is in lib/ other parts might want to be
 able to use it and for them GFP_KERNEL makes more sense.

In order to make libqr usable in other parts of the kernel,
we need to heavily restructure libqr. It seems to use a style
that was inherited from OOP. Since we are in C, I don't like it.
I think that the allocations for the structs (e.g. QRinput) should
be done by the caller. That way we could remove most of the
allocations from libqr, and the rest may use GFP_KERNEL if they
still exist after that cleanup. Any objections?


 [...] 

 * I had trouble getting QR output.
Doing 'echo c  /proc/sysrq-trigger' triggered a crash,
but it resulted in a recursive OOPS. This is a nullptr-deref
and hence I think it may be related to the fact I was running
it in textmode. :-)
Or, it is due to the bugged error handling.

 The QR output is written to the frame buffer. That means you have to
 get acces to a console. As I mentioned in the RFC, I am looking for an
 alternative to using fb.h since that doesn't seem to work very well
 atm.

Okay, I sent you a pull request that fixes a recursive OOPS when crashing
in textmode and no higher mode is available. Patch attached as well [0].

Also, have you check ASCII characters 219-223? Maybe they are usable?
Any ideas?

[0]:

%-
From: Levente Kurusa le...@linux.com
Subject: [PATCH] qr: print_oops: don't try to print if there is no framebuffer

If the kernel boots in textmode, we would hit a null pointer derefernce
in compute_w(). Prevent this by actually checking the value of info.

Signed-off-by: Levente Kurusa le...@linux.com
---
 kernel/print_oops.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/kernel/print_oops.c b/kernel/print_oops.c
index f43c059..f8909ba 100644
--- a/kernel/print_oops.c
+++ b/kernel/print_oops.c
@@ -126,6 +126,10 @@ void print_qr_err(void)
qr = QRcode_encodeData(compr_len, compr_qr_buffer, 0, QR_ECLEVEL_H);

info = registered_fb[0];
+   if (!info) {
+   printk(KERN_WARNING Unable to get hand of a framebuffer!\n);
+   goto exit;
+   }
w = compute_w(info, qr-width);

rect.width = w;
@@ -167,6 +171,7 @@ void print_qr_err(void)
}
}

+exit:
QRcode_free(qr);
qr_compr_exit();
buf_pos = 0;
-- 
1.8.3.1
%-

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 03/22/2014 07:20 PM, Teodora Băluţă wrote:
> On Sat, Mar 22, 2014 at 7:09 PM, Levente Kurusa  wrote:
>> Hi,
>>
>> On 03/21/2014 02:28 PM, Jason Cooper wrote:
>>> On Wed, Mar 19, 2014 at 10:38:30PM +0200, Teodora Băluţă wrote:
>>>> On Wed, Mar 19, 2014 at 10:18 PM, Dave Jones  wrote:
>>>>> On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
>>>>>  > This feature encodes Oops messages into a QR barcode that is scannable 
>>>>> by
>>>>>  > any device with a camera.
>>>>>
>>>>> [...]
>>>>>
>>>>> That's a ton of code we're adding into one of the most fragile parts of 
>>>>> the kernel.
>>>>>
>>>>> A lot of what libqrencode does would seem to be superfluous to the 
>>>>> requirements
>>>>> here, as we don't output kernel oopses in kanji for eg, and won't care 
>>>>> about
>>>>> multiple versions of the qr spec.
>>>>
>>>> That's true. I didn't do that much cleanup in the library afraid of
>>>> breaking something and focused that I get this done one way or
>>>> another. Indeed, the library is userspace and is made to be versatile
>>>> rather than small.
>>>
>>> Perhaps you could add libqr to the staging tree?  As long as it
>>> compiles, it can go there.  Then you can focus on cleanups and bloat
>>> removal.  In the process, you'll get a larger testing base because it
>>> will be in mainline.
>>
>> Yea that is a better way. However, the current state of the code has
>> several problems:
>>
>> * No good error handling (simply returns -1 on failure no matter what)
>>I have began converting this to the ERR_PTR et al interface
>>However, I have not yet done this fully due to the vast amount
>>of work required to do so.
>>This shouldn't be yet merged, but I shall send it as patches once
>>it gets into the staging tree.
>>
>> * All memory allocations are GFP_ATOMIC for no reason.
>>I have converted them to GFP_KERNEL since we can block safely.
>>This could be merged to Teodora's branch. I can send her a pull
>>request on GitHub if she wants so.
> 
> Since we are talking about some kernel Oopses I thought that making
> this GFP_ATOMIC ensures that we get memory allocation. I have
> considered using GFP_KERNEL, but I am not very sure about that.
> Probably somewhere deep inside I wanted to make it work even for
> panics. :(

Yea that makes sense, realized that just after I sent the mail.
However, since this is in lib/ other parts might want to be
able to use it and for them GFP_KERNEL makes more sense.

> 
>>
>> * Selecting QR_OOPS and QRLIB currently does not build due to
>>undefined references to zlib_deflate* functions.
>>This is due to QRLIB not selecting ZLIB_DEFLATE.
>>Fixed this as well. Can be merged to Teodora if she wants.
> 
> Hmm, that's odd. I thought I added a 'depends' in the menu. Please
> make a pull request and I'll merge it immediately.

Sent it, also attached the patch [0].

> 
>>
>> * I had trouble getting QR output.
>>Doing 'echo c > /proc/sysrq-trigger' triggered a crash,
>>but it resulted in a recursive OOPS. This is a nullptr-deref
>>and hence I think it may be related to the fact I was running
>>it in textmode. :-)
>>Or, it is due to the bugged error handling.
> 
> The QR output is written to the frame buffer. That means you have to
> get acces to a console. As I mentioned in the RFC, I am looking for an
> alternative to using fb.h since that doesn't seem to work very well
> atm.

Yea this issue is significant. There are some ASCII codes which might
work in textmode though. (219-223) Maybe it's worth a shot to try it out.

> 
>>
>>>
>>> You may be interested in objdiff [1] which I'm using for merging code
>>> into the staging tree [2].  It provides an automated way to determine
>>> that code cleanups didn't change the resultant object code.  You can see
>>> an example run here [3].
> 
> I'll take a look. Thanks!
> 
>>>
>>> I would definitely like to see the QR output incorporated into a
>>> kernel.org url.  That would remove the need for installing another app,
>>> and would ease bug reporting.
>>
>> I still struggle to understand how could that be done. We can encode the
>> QR code as ASCII. Okay, that's fine, however it is very long. Encoding
>> 'Unable to handle kernel paging request at 000f' gave a 4

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 03/21/2014 02:28 PM, Jason Cooper wrote:
> On Wed, Mar 19, 2014 at 10:38:30PM +0200, Teodora Băluţă wrote:
>> On Wed, Mar 19, 2014 at 10:18 PM, Dave Jones  wrote:
>>> On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
>>>  > This feature encodes Oops messages into a QR barcode that is scannable by
>>>  > any device with a camera.
>>>
>>> [...]
>>>
>>> That's a ton of code we're adding into one of the most fragile parts of the 
>>> kernel.
>>>
>>> A lot of what libqrencode does would seem to be superfluous to the 
>>> requirements
>>> here, as we don't output kernel oopses in kanji for eg, and won't care about
>>> multiple versions of the qr spec.
>>
>> That's true. I didn't do that much cleanup in the library afraid of
>> breaking something and focused that I get this done one way or
>> another. Indeed, the library is userspace and is made to be versatile
>> rather than small.
> 
> Perhaps you could add libqr to the staging tree?  As long as it
> compiles, it can go there.  Then you can focus on cleanups and bloat
> removal.  In the process, you'll get a larger testing base because it
> will be in mainline.

Yea that is a better way. However, the current state of the code has
several problems:

* No good error handling (simply returns -1 on failure no matter what)
   I have began converting this to the ERR_PTR et al interface
   However, I have not yet done this fully due to the vast amount
   of work required to do so.
   This shouldn't be yet merged, but I shall send it as patches once
   it gets into the staging tree.

* All memory allocations are GFP_ATOMIC for no reason.
   I have converted them to GFP_KERNEL since we can block safely.
   This could be merged to Teodora's branch. I can send her a pull
   request on GitHub if she wants so.

* Selecting QR_OOPS and QRLIB currently does not build due to
   undefined references to zlib_deflate* functions.
   This is due to QRLIB not selecting ZLIB_DEFLATE.
   Fixed this as well. Can be merged to Teodora if she wants.

* I had trouble getting QR output.
   Doing 'echo c > /proc/sysrq-trigger' triggered a crash,
   but it resulted in a recursive OOPS. This is a nullptr-deref
   and hence I think it may be related to the fact I was running
   it in textmode. :-)
   Or, it is due to the bugged error handling.

> 
> You may be interested in objdiff [1] which I'm using for merging code
> into the staging tree [2].  It provides an automated way to determine
> that code cleanups didn't change the resultant object code.  You can see
> an example run here [3].
> 
> I would definitely like to see the QR output incorporated into a
> kernel.org url.  That would remove the need for installing another app,
> and would ease bug reporting.

I still struggle to understand how could that be done. We can encode the
QR code as ASCII. Okay, that's fine, however it is very long. Encoding
'Unable to handle kernel paging request at 000f' gave a 449 character
long sequence with very strange characters [0]. We should try to shorten
it, imho. Not sure how to do that though.

oops.kernel.org/?qr=CODE  would look cool though. :-)

> 
> Anyway, if you're interested, I'll be re-posting a patch for objdiff
> separately maybe today or this weekend.


[0]: http://paste.fedoraproject.org/87665/39550664/


-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 03/21/2014 02:28 PM, Jason Cooper wrote:
 On Wed, Mar 19, 2014 at 10:38:30PM +0200, Teodora Băluţă wrote:
 On Wed, Mar 19, 2014 at 10:18 PM, Dave Jones da...@redhat.com wrote:
 On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
   This feature encodes Oops messages into a QR barcode that is scannable by
   any device with a camera.

 [...]

 That's a ton of code we're adding into one of the most fragile parts of the 
 kernel.

 A lot of what libqrencode does would seem to be superfluous to the 
 requirements
 here, as we don't output kernel oopses in kanji for eg, and won't care about
 multiple versions of the qr spec.

 That's true. I didn't do that much cleanup in the library afraid of
 breaking something and focused that I get this done one way or
 another. Indeed, the library is userspace and is made to be versatile
 rather than small.
 
 Perhaps you could add libqr to the staging tree?  As long as it
 compiles, it can go there.  Then you can focus on cleanups and bloat
 removal.  In the process, you'll get a larger testing base because it
 will be in mainline.

Yea that is a better way. However, the current state of the code has
several problems:

* No good error handling (simply returns -1 on failure no matter what)
   I have began converting this to the ERR_PTR et al interface
   However, I have not yet done this fully due to the vast amount
   of work required to do so.
   This shouldn't be yet merged, but I shall send it as patches once
   it gets into the staging tree.

* All memory allocations are GFP_ATOMIC for no reason.
   I have converted them to GFP_KERNEL since we can block safely.
   This could be merged to Teodora's branch. I can send her a pull
   request on GitHub if she wants so.

* Selecting QR_OOPS and QRLIB currently does not build due to
   undefined references to zlib_deflate* functions.
   This is due to QRLIB not selecting ZLIB_DEFLATE.
   Fixed this as well. Can be merged to Teodora if she wants.

* I had trouble getting QR output.
   Doing 'echo c  /proc/sysrq-trigger' triggered a crash,
   but it resulted in a recursive OOPS. This is a nullptr-deref
   and hence I think it may be related to the fact I was running
   it in textmode. :-)
   Or, it is due to the bugged error handling.

 
 You may be interested in objdiff [1] which I'm using for merging code
 into the staging tree [2].  It provides an automated way to determine
 that code cleanups didn't change the resultant object code.  You can see
 an example run here [3].
 
 I would definitely like to see the QR output incorporated into a
 kernel.org url.  That would remove the need for installing another app,
 and would ease bug reporting.

I still struggle to understand how could that be done. We can encode the
QR code as ASCII. Okay, that's fine, however it is very long. Encoding
'Unable to handle kernel paging request at 000f' gave a 449 character
long sequence with very strange characters [0]. We should try to shorten
it, imho. Not sure how to do that though.

oops.kernel.org/?qr=CODE  would look cool though. :-)

 
 Anyway, if you're interested, I'll be re-posting a patch for objdiff
 separately maybe today or this weekend.


[0]: http://paste.fedoraproject.org/87665/39550664/


-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

On 03/22/2014 07:20 PM, Teodora Băluţă wrote:
 On Sat, Mar 22, 2014 at 7:09 PM, Levente Kurusa le...@linux.com wrote:
 Hi,

 On 03/21/2014 02:28 PM, Jason Cooper wrote:
 On Wed, Mar 19, 2014 at 10:38:30PM +0200, Teodora Băluţă wrote:
 On Wed, Mar 19, 2014 at 10:18 PM, Dave Jones da...@redhat.com wrote:
 On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
   This feature encodes Oops messages into a QR barcode that is scannable 
 by
   any device with a camera.

 [...]

 That's a ton of code we're adding into one of the most fragile parts of 
 the kernel.

 A lot of what libqrencode does would seem to be superfluous to the 
 requirements
 here, as we don't output kernel oopses in kanji for eg, and won't care 
 about
 multiple versions of the qr spec.

 That's true. I didn't do that much cleanup in the library afraid of
 breaking something and focused that I get this done one way or
 another. Indeed, the library is userspace and is made to be versatile
 rather than small.

 Perhaps you could add libqr to the staging tree?  As long as it
 compiles, it can go there.  Then you can focus on cleanups and bloat
 removal.  In the process, you'll get a larger testing base because it
 will be in mainline.

 Yea that is a better way. However, the current state of the code has
 several problems:

 * No good error handling (simply returns -1 on failure no matter what)
I have began converting this to the ERR_PTR et al interface
However, I have not yet done this fully due to the vast amount
of work required to do so.
This shouldn't be yet merged, but I shall send it as patches once
it gets into the staging tree.

 * All memory allocations are GFP_ATOMIC for no reason.
I have converted them to GFP_KERNEL since we can block safely.
This could be merged to Teodora's branch. I can send her a pull
request on GitHub if she wants so.
 
 Since we are talking about some kernel Oopses I thought that making
 this GFP_ATOMIC ensures that we get memory allocation. I have
 considered using GFP_KERNEL, but I am not very sure about that.
 Probably somewhere deep inside I wanted to make it work even for
 panics. :(

Yea that makes sense, realized that just after I sent the mail.
However, since this is in lib/ other parts might want to be
able to use it and for them GFP_KERNEL makes more sense.

 

 * Selecting QR_OOPS and QRLIB currently does not build due to
undefined references to zlib_deflate* functions.
This is due to QRLIB not selecting ZLIB_DEFLATE.
Fixed this as well. Can be merged to Teodora if she wants.
 
 Hmm, that's odd. I thought I added a 'depends' in the menu. Please
 make a pull request and I'll merge it immediately.

Sent it, also attached the patch [0].

 

 * I had trouble getting QR output.
Doing 'echo c  /proc/sysrq-trigger' triggered a crash,
but it resulted in a recursive OOPS. This is a nullptr-deref
and hence I think it may be related to the fact I was running
it in textmode. :-)
Or, it is due to the bugged error handling.
 
 The QR output is written to the frame buffer. That means you have to
 get acces to a console. As I mentioned in the RFC, I am looking for an
 alternative to using fb.h since that doesn't seem to work very well
 atm.

Yea this issue is significant. There are some ASCII codes which might
work in textmode though. (219-223) Maybe it's worth a shot to try it out.

 


 You may be interested in objdiff [1] which I'm using for merging code
 into the staging tree [2].  It provides an automated way to determine
 that code cleanups didn't change the resultant object code.  You can see
 an example run here [3].
 
 I'll take a look. Thanks!
 

 I would definitely like to see the QR output incorporated into a
 kernel.org url.  That would remove the need for installing another app,
 and would ease bug reporting.

 I still struggle to understand how could that be done. We can encode the
 QR code as ASCII. Okay, that's fine, however it is very long. Encoding
 'Unable to handle kernel paging request at 000f' gave a 449 character
 long sequence with very strange characters [0]. We should try to shorten
 it, imho. Not sure how to do that though.

 oops.kernel.org/?qr=CODE  would look cool though. :-)
 
 I am not very sure that could be done. Accessing the QR code through a
 link would mean you have to send it automatically to kernel.org (that
 assumes a great deal of things like working Internet connection) and
 it misses the point of having a QR code in the first place. The way I
 see it, having a QR decoder app installed that can do an offline
 decoding is a less greater effort than popping out a browser on the
 machine you're working on.

Yea I still think that mobiles are the way to go. However, why would we
need internet connection? We could just output a formatted link like
oops.kernel.org/?qr=%s

where %s will take the ASCII QR Code. Or something among those lines.

 
 And plus, as Levente said, encoding the QR code

Re: [PATCH] initramfs: print error and shell out for unsupported content

[Levente Kurusa]

Hi,

2014-03-20 22:35 GMT+01:00 Alexander Holler :
> The initramfs generation is broken for file and directory names which contain
> colons or spaces. Print an error and don't try to continue.
>
> Tests:
>
> cd linux
> make defconfig
> echo 'CONFIG_BLK_DEV_INITRD=y' >> .config
> echo 'CONFIG_INITRAMFS_ROOT_UID=0' >>.config
> echo 'CONFIG_INITRAMFS_ROOT_GID=0' >>.config
> echo 'CONFIG_INITRAMFS_COMPRESSION_NONE=y' >>.config
> echo 'CONFIG_INITRAMFS_SOURCE="/tmp/bugroot"' >>.config
>
> Problem with colons:
>
> mkdir -p /tmp/bugroot/a:b
> make -j4 bzImage # no error
> make bzImage # try again, oops
>
> Problem with spaces:
>
> mkdir -p /tmp/bugroot/a\ b
> make -j4 bzImage # no error
> zcat usr/initramfs_data.cpio.gz | cpio --extract --list # oops, no content
>
> Signed-off-by: Alexander Holler 
> ---
>  scripts/gen_initramfs_list.sh | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/scripts/gen_initramfs_list.sh b/scripts/gen_initramfs_list.sh
> index 17fa901..aacf366 100644
> --- a/scripts/gen_initramfs_list.sh
> +++ b/scripts/gen_initramfs_list.sh
> @@ -171,6 +171,17 @@ dir_filelist() {
> ${dep_list}header "$1"
>
> srcdir=$(echo "$1" | sed -e 's://*:/:g')
> +
> +   # Files and directories with spaces and colons are unsupported.
> +   local unsupported=$(find "${srcdir}" -regex '.*\(:\| \).*')
> +   if [ ! -z "${unsupported}" ]; then
> +   printf "ERROR: Unable to handle files/directories with " >&2
> +   printf "unsupported characters.\nPlease use other ways " >&2

'Please use other ways to...'
(i.e. missing "to")

> +   printf "generate a cpio-archive.\nUnsupported files and " >&2
> +   printf "directories are:\n$unsupported\n" >&2
> +   exit 1
> +   fi
> +

I think it would be worthy to tell the user what kind of characters
are unsupported. For instance, tell them that 'spaces' are unsupported.


> dirlist=$(find "${srcdir}" -printf "%p %m %U %G\n")
>
> # If $dirlist is only one line, then the directory is empty
> [...]

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] initramfs: print error and shell out for unsupported content

[Levente Kurusa]

Hi,

2014-03-20 22:35 GMT+01:00 Alexander Holler hol...@ahsoftware.de:
 The initramfs generation is broken for file and directory names which contain
 colons or spaces. Print an error and don't try to continue.

 Tests:

 cd linux
 make defconfig
 echo 'CONFIG_BLK_DEV_INITRD=y'  .config
 echo 'CONFIG_INITRAMFS_ROOT_UID=0' .config
 echo 'CONFIG_INITRAMFS_ROOT_GID=0' .config
 echo 'CONFIG_INITRAMFS_COMPRESSION_NONE=y' .config
 echo 'CONFIG_INITRAMFS_SOURCE=/tmp/bugroot' .config

 Problem with colons:

 mkdir -p /tmp/bugroot/a:b
 make -j4 bzImage # no error
 make bzImage # try again, oops

 Problem with spaces:

 mkdir -p /tmp/bugroot/a\ b
 make -j4 bzImage # no error
 zcat usr/initramfs_data.cpio.gz | cpio --extract --list # oops, no content

 Signed-off-by: Alexander Holler hol...@ahsoftware.de
 ---
  scripts/gen_initramfs_list.sh | 11 +++
  1 file changed, 11 insertions(+)

 diff --git a/scripts/gen_initramfs_list.sh b/scripts/gen_initramfs_list.sh
 index 17fa901..aacf366 100644
 --- a/scripts/gen_initramfs_list.sh
 +++ b/scripts/gen_initramfs_list.sh
 @@ -171,6 +171,17 @@ dir_filelist() {
 ${dep_list}header $1

 srcdir=$(echo $1 | sed -e 's://*:/:g')
 +
 +   # Files and directories with spaces and colons are unsupported.
 +   local unsupported=$(find ${srcdir} -regex '.*\(:\| \).*')
 +   if [ ! -z ${unsupported} ]; then
 +   printf ERROR: Unable to handle files/directories with  2
 +   printf unsupported characters.\nPlease use other ways  2

'Please use other ways to...'
(i.e. missing to)

 +   printf generate a cpio-archive.\nUnsupported files and  2
 +   printf directories are:\n$unsupported\n 2
 +   exit 1
 +   fi
 +

I think it would be worthy to tell the user what kind of characters
are unsupported. For instance, tell them that 'spaces' are unsupported.


 dirlist=$(find ${srcdir} -printf %p %m %U %G\n)

 # If $dirlist is only one line, then the directory is empty
 [...]

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

2014-03-19 21:50 GMT+01:00 Teodora Băluţă :
> On Wed, Mar 19, 2014 at 10:28 PM, Levente Kurusa  wrote:
>> Hi,
>>
>> 2014-03-19 21:18 GMT+01:00 Dave Jones :
>>> On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
>>>  > This feature encodes Oops messages into a QR barcode that is scannable by
>>>  > any device with a camera.
>>>
>>>  ...
>>>
>>>  >  include/linux/print_oops.h |   11 +
>>>  >  include/linux/qrencode.h   |  546 +
>>>  >  kernel/Makefile|1 +
>>>  >  kernel/panic.c |5 +
>>>  >  kernel/print_oops.c|  173 +
>>>  >  kernel/printk/printk.c |9 +-
>>>  >  lib/Kconfig|5 +
>>>  >  lib/Kconfig.debug  |   11 +
>>>  >  lib/Makefile   |3 +
>>>  >  lib/qr/Makefile|6 +
>>>  >  lib/qr/bitstream.c |  233 ++
>>>  >  lib/qr/bitstream.h |   37 +
>>>  >  lib/qr/mask.c  |  320 
>>>  >  lib/qr/mask.h  |   39 +
>>>  >  lib/qr/mmask.c |  175 +
>>>  >  lib/qr/mmask.h |   36 +
>>>  >  lib/qr/mqrspec.c   |  259 +++
>>>  >  lib/qr/mqrspec.h   |  155 
>>>  >  lib/qr/qrencode.c  |  871 +
>>>  >  lib/qr/qrencode.h  |  546 +
>>>  >  lib/qr/qrinput.c   | 1834 
>>> 
>>>  >  lib/qr/qrinput.h   |  129 
>>>  >  lib/qr/qrspec.c|  543 +
>>>  >  lib/qr/qrspec.h|  178 +
>>>  >  lib/qr/rscode.c|  325 
>>>  >  lib/qr/rscode.h|   38 +
>>>  >  lib/qr/split.c |  331 
>>>  >  lib/qr/split.h |   44 ++
>>>  >  28 files changed, 6860 insertions(+), 3 deletions(-)
>>
>> This idea is certainly great.
>>
>> However, there are quite a few problems with the code in terms of code style 
>> and
>> other terms as well. I am not sure how could we help you make this code
>> appliable, but it would be great if you put up a branch somewhere. This way
>> I could send you a few commits that do some fixups.
>
> Wow, that'd be great! I have set up my clone of the kernel source up
> on gitlab [0] and github [1]. I will update the remote branch asap (I
> made some coding style fixups that aren't present on github/gitlab
> right now, only in a remote branch). Is this ok?

Yea, that's fine. Push your changes and send me a ping once you
are done. I will most likely send you a pull request on github though.

>
>>
>>>
>>> That's a ton of code we're adding into one of the most fragile parts of the 
>>> kernel.
>>
>> Indeed, this should get split up.
>>
>>>
>>> A lot of what libqrencode does would seem to be superfluous to the 
>>> requirements
>>> here, as we don't output kernel oopses in kanji for eg, and won't care about
>>> multiple versions of the qr spec.
>>>
>>> How much of this could we drop ?
>>
>> A lot, most likely.
>
> Indeed.
>
>>
>> Also, I wonder if we could do the same for panic()?
>> I hate it when I receive a panic and I have no idea what's the cause
>> since my display is filled up with the stack trace.
>
> Most likely panic is harder to do for reasons discussed in this thread here 
> [2].
>

Yes I see.

>
> [0] https://gitlab.com/teobaluta/opw
> [1] https://github.com/teobaluta/qr-linux-kernel
> [2] https://lwn.net/Articles/503677/
>
> Thanks,
> Teodora
>>
>> --
>> Regards,
>> Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

2014-03-19 21:18 GMT+01:00 Dave Jones :
> On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
>  > This feature encodes Oops messages into a QR barcode that is scannable by
>  > any device with a camera.
>
>  ...
>
>  >  include/linux/print_oops.h |   11 +
>  >  include/linux/qrencode.h   |  546 +
>  >  kernel/Makefile|1 +
>  >  kernel/panic.c |5 +
>  >  kernel/print_oops.c|  173 +
>  >  kernel/printk/printk.c |9 +-
>  >  lib/Kconfig|5 +
>  >  lib/Kconfig.debug  |   11 +
>  >  lib/Makefile   |3 +
>  >  lib/qr/Makefile|6 +
>  >  lib/qr/bitstream.c |  233 ++
>  >  lib/qr/bitstream.h |   37 +
>  >  lib/qr/mask.c  |  320 
>  >  lib/qr/mask.h  |   39 +
>  >  lib/qr/mmask.c |  175 +
>  >  lib/qr/mmask.h |   36 +
>  >  lib/qr/mqrspec.c   |  259 +++
>  >  lib/qr/mqrspec.h   |  155 
>  >  lib/qr/qrencode.c  |  871 +
>  >  lib/qr/qrencode.h  |  546 +
>  >  lib/qr/qrinput.c   | 1834 
> 
>  >  lib/qr/qrinput.h   |  129 
>  >  lib/qr/qrspec.c|  543 +
>  >  lib/qr/qrspec.h|  178 +
>  >  lib/qr/rscode.c|  325 
>  >  lib/qr/rscode.h|   38 +
>  >  lib/qr/split.c |  331 
>  >  lib/qr/split.h |   44 ++
>  >  28 files changed, 6860 insertions(+), 3 deletions(-)

This idea is certainly great.

However, there are quite a few problems with the code in terms of code style and
other terms as well. I am not sure how could we help you make this code
appliable, but it would be great if you put up a branch somewhere. This way
I could send you a few commits that do some fixups.

>
> That's a ton of code we're adding into one of the most fragile parts of the 
> kernel.

Indeed, this should get split up.

>
> A lot of what libqrencode does would seem to be superfluous to the 
> requirements
> here, as we don't output kernel oopses in kanji for eg, and won't care about
> multiple versions of the qr spec.
>
> How much of this could we drop ?

A lot, most likely.

Also, I wonder if we could do the same for panic()?
I hate it when I receive a panic and I have no idea what's the cause
since my display is filled up with the stack trace.

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

2014-03-19 21:18 GMT+01:00 Dave Jones da...@redhat.com:
 On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
   This feature encodes Oops messages into a QR barcode that is scannable by
   any device with a camera.

  ...

include/linux/print_oops.h |   11 +
include/linux/qrencode.h   |  546 +
kernel/Makefile|1 +
kernel/panic.c |5 +
kernel/print_oops.c|  173 +
kernel/printk/printk.c |9 +-
lib/Kconfig|5 +
lib/Kconfig.debug  |   11 +
lib/Makefile   |3 +
lib/qr/Makefile|6 +
lib/qr/bitstream.c |  233 ++
lib/qr/bitstream.h |   37 +
lib/qr/mask.c  |  320 
lib/qr/mask.h  |   39 +
lib/qr/mmask.c |  175 +
lib/qr/mmask.h |   36 +
lib/qr/mqrspec.c   |  259 +++
lib/qr/mqrspec.h   |  155 
lib/qr/qrencode.c  |  871 +
lib/qr/qrencode.h  |  546 +
lib/qr/qrinput.c   | 1834 
 
lib/qr/qrinput.h   |  129 
lib/qr/qrspec.c|  543 +
lib/qr/qrspec.h|  178 +
lib/qr/rscode.c|  325 
lib/qr/rscode.h|   38 +
lib/qr/split.c |  331 
lib/qr/split.h |   44 ++
28 files changed, 6860 insertions(+), 3 deletions(-)

This idea is certainly great.

However, there are quite a few problems with the code in terms of code style and
other terms as well. I am not sure how could we help you make this code
appliable, but it would be great if you put up a branch somewhere. This way
I could send you a few commits that do some fixups.


 That's a ton of code we're adding into one of the most fragile parts of the 
 kernel.

Indeed, this should get split up.


 A lot of what libqrencode does would seem to be superfluous to the 
 requirements
 here, as we don't output kernel oopses in kanji for eg, and won't care about
 multiple versions of the qr spec.

 How much of this could we drop ?

A lot, most likely.

Also, I wonder if we could do the same for panic()?
I hate it when I receive a panic and I have no idea what's the cause
since my display is filled up with the stack trace.

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] QR encoding for Oops messages

[Levente Kurusa]

Hi,

2014-03-19 21:50 GMT+01:00 Teodora Băluţă teobal...@gmail.com:
 On Wed, Mar 19, 2014 at 10:28 PM, Levente Kurusa le...@linux.com wrote:
 Hi,

 2014-03-19 21:18 GMT+01:00 Dave Jones da...@redhat.com:
 On Mon, Mar 17, 2014 at 02:59:47PM -0700, Teodora Baluta wrote:
   This feature encodes Oops messages into a QR barcode that is scannable by
   any device with a camera.

  ...

include/linux/print_oops.h |   11 +
include/linux/qrencode.h   |  546 +
kernel/Makefile|1 +
kernel/panic.c |5 +
kernel/print_oops.c|  173 +
kernel/printk/printk.c |9 +-
lib/Kconfig|5 +
lib/Kconfig.debug  |   11 +
lib/Makefile   |3 +
lib/qr/Makefile|6 +
lib/qr/bitstream.c |  233 ++
lib/qr/bitstream.h |   37 +
lib/qr/mask.c  |  320 
lib/qr/mask.h  |   39 +
lib/qr/mmask.c |  175 +
lib/qr/mmask.h |   36 +
lib/qr/mqrspec.c   |  259 +++
lib/qr/mqrspec.h   |  155 
lib/qr/qrencode.c  |  871 +
lib/qr/qrencode.h  |  546 +
lib/qr/qrinput.c   | 1834 
 
lib/qr/qrinput.h   |  129 
lib/qr/qrspec.c|  543 +
lib/qr/qrspec.h|  178 +
lib/qr/rscode.c|  325 
lib/qr/rscode.h|   38 +
lib/qr/split.c |  331 
lib/qr/split.h |   44 ++
28 files changed, 6860 insertions(+), 3 deletions(-)

 This idea is certainly great.

 However, there are quite a few problems with the code in terms of code style 
 and
 other terms as well. I am not sure how could we help you make this code
 appliable, but it would be great if you put up a branch somewhere. This way
 I could send you a few commits that do some fixups.

 Wow, that'd be great! I have set up my clone of the kernel source up
 on gitlab [0] and github [1]. I will update the remote branch asap (I
 made some coding style fixups that aren't present on github/gitlab
 right now, only in a remote branch). Is this ok?

Yea, that's fine. Push your changes and send me a ping once you
are done. I will most likely send you a pull request on github though.




 That's a ton of code we're adding into one of the most fragile parts of the 
 kernel.

 Indeed, this should get split up.


 A lot of what libqrencode does would seem to be superfluous to the 
 requirements
 here, as we don't output kernel oopses in kanji for eg, and won't care about
 multiple versions of the qr spec.

 How much of this could we drop ?

 A lot, most likely.

 Indeed.


 Also, I wonder if we could do the same for panic()?
 I hate it when I receive a panic and I have no idea what's the cause
 since my display is filled up with the stack trace.

 Most likely panic is harder to do for reasons discussed in this thread here 
 [2].


Yes I see.


 [0] https://gitlab.com/teobaluta/opw
 [1] https://github.com/teobaluta/qr-linux-kernel
 [2] https://lwn.net/Articles/503677/

 Thanks,
 Teodora

 --
 Regards,
 Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] regmap: Ensure regmap_register_patch() is compatible with fast_io

[Levente Kurusa]

Hi Mark,

Just a single, mini-comment.


2014-03-18 13:02 GMT+01:00 Mark Brown :
> From: Mark Brown 
>
> With fast_io we use mutexes to lock the I/O operations so we would need
> to do GFP_ATOMIC allocations if we wanted to do allocations inside the
> lock as we do currently. Since it is unlikely that we will want to register
> a patch outside of init where concurrency shouldn't be an issue move the
> allocation of the patch data outside the lock.
>
> Reported-by: Takashi Iwai 
> Signed-off-by: Mark Brown 
> ---
>  drivers/base/regmap/regmap.c | 25 ++---
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 554119535a64..2d7d55b3bedb 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -2396,6 +2396,9 @@ EXPORT_SYMBOL_GPL(regmap_async_complete);
>   * apply them immediately.  Typically this is used to apply
>   * corrections to be applied to the device defaults on startup, such
>   * as the updates some vendors provide to undocumented registers.
> + *
> + * The caller must ensure that this function cannot be called
> + * concurrently with either itself or regcache_sync().
>   */
>  int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
>   int num_regs)
> @@ -2408,6 +2411,17 @@ int regmap_register_patch(struct regmap *map, const 
> struct reg_default *regs,
> num_regs))
> return 0;
>
> +   p = krealloc(map->patch,
> +sizeof(struct reg_default) * (map->patch_regs + 
> num_regs),
> +GFP_KERNEL);
> +   if (p) {
> +   memcpy(p + map->patch_regs, regs, num_regs * sizeof(*regs));
> +   map->patch = p;
> +   map->patch_regs += num_regs;
> +   } else {
> +   return -ENOMEM;
> +   }
> +

I think that is not checkpatch-safe :-)

> [...]

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] regmap: Ensure regmap_register_patch() is compatible with fast_io

[Levente Kurusa]

Hi Mark,

Just a single, mini-comment.


2014-03-18 13:02 GMT+01:00 Mark Brown broo...@kernel.org:
 From: Mark Brown broo...@linaro.org

 With fast_io we use mutexes to lock the I/O operations so we would need
 to do GFP_ATOMIC allocations if we wanted to do allocations inside the
 lock as we do currently. Since it is unlikely that we will want to register
 a patch outside of init where concurrency shouldn't be an issue move the
 allocation of the patch data outside the lock.

 Reported-by: Takashi Iwai ti...@suse.de
 Signed-off-by: Mark Brown broo...@linaro.org
 ---
  drivers/base/regmap/regmap.c | 25 ++---
  1 file changed, 14 insertions(+), 11 deletions(-)

 diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
 index 554119535a64..2d7d55b3bedb 100644
 --- a/drivers/base/regmap/regmap.c
 +++ b/drivers/base/regmap/regmap.c
 @@ -2396,6 +2396,9 @@ EXPORT_SYMBOL_GPL(regmap_async_complete);
   * apply them immediately.  Typically this is used to apply
   * corrections to be applied to the device defaults on startup, such
   * as the updates some vendors provide to undocumented registers.
 + *
 + * The caller must ensure that this function cannot be called
 + * concurrently with either itself or regcache_sync().
   */
  int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
   int num_regs)
 @@ -2408,6 +2411,17 @@ int regmap_register_patch(struct regmap *map, const 
 struct reg_default *regs,
 num_regs))
 return 0;

 +   p = krealloc(map-patch,
 +sizeof(struct reg_default) * (map-patch_regs + 
 num_regs),
 +GFP_KERNEL);
 +   if (p) {
 +   memcpy(p + map-patch_regs, regs, num_regs * sizeof(*regs));
 +   map-patch = p;
 +   map-patch_regs += num_regs;
 +   } else {
 +   return -ENOMEM;
 +   }
 +

I think that is not checkpatch-safe :-)

 [...]

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: comedi: fix memory leak

[Levente Kurusa]

On 03/15/2014 04:30 AM, Chase Southwood wrote:
>> On Friday, March 14, 2014 11:47 AM, Levente Kurusa  wrote:
> 
>> Call kfree() on bdev. The variable is otherwise leaked.
>>
>> Signed-off-by: Levente Kurusa 
>> [...]
>>
> 
> 
> Levente,
> 
> This change has already been made in staging-next (by me, actually :) ).  In
> order to avoid re-doing work which has already been done, please make sure to 
> base
> all of your patches off of linux-next (or for staging, staging-next).
> 
> Thanks,
> Chase
> 

Ah, yes I see now. I wonder how it didn't reach my tree.
Anyways, thanks for the heads up!

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: comedi: fix memory leak

[Levente Kurusa]

On 03/15/2014 04:30 AM, Chase Southwood wrote:
 On Friday, March 14, 2014 11:47 AM, Levente Kurusa le...@linux.com wrote:
 
 Call kfree() on bdev. The variable is otherwise leaked.

 Signed-off-by: Levente Kurusa le...@linux.com
 [...]

 
 
 Levente,
 
 This change has already been made in staging-next (by me, actually :) ).  In
 order to avoid re-doing work which has already been done, please make sure to 
 base
 all of your patches off of linux-next (or for staging, staging-next).
 
 Thanks,
 Chase
 

Ah, yes I see now. I wonder how it didn't reach my tree.
Anyways, thanks for the heads up!

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: comedi: fix memory leak

[Levente Kurusa]

Call kfree() on bdev. The variable is otherwise leaked.

Signed-off-by: Levente Kurusa 
---
 drivers/staging/comedi/drivers/comedi_bond.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c 
b/drivers/staging/comedi/drivers/comedi_bond.c
index 51a59e5..406aedb 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -254,6 +254,7 @@ static int do_dev_config(struct comedi_device *dev, struct 
comedi_devconfig *it)
if (!devs) {
dev_err(dev->class_dev,
"Could not allocate memory. Out of 
memory?\n");
+   kfree(bdev);
return -ENOMEM;
}
devpriv->devs = devs;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: comedi: fix memory leak

[Levente Kurusa]

Call kfree() on bdev. The variable is otherwise leaked.

Signed-off-by: Levente Kurusa le...@linux.com
---
 drivers/staging/comedi/drivers/comedi_bond.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/comedi/drivers/comedi_bond.c 
b/drivers/staging/comedi/drivers/comedi_bond.c
index 51a59e5..406aedb 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c
@@ -254,6 +254,7 @@ static int do_dev_config(struct comedi_device *dev, struct 
comedi_devconfig *it)
if (!devs) {
dev_err(dev-class_dev,
Could not allocate memory. Out of 
memory?\n);
+   kfree(bdev);
return -ENOMEM;
}
devpriv-devs = devs;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] Add documentation for proper usage and order of preference of calls to print diagnostic messages.

[Levente Kurusa]

Hi,

[+CC Rob]

2014-03-04 15:31 GMT+01:00 yogesh :
> This patch adds documentation that clarifies the use of various
> diagnostic printing messages. It shows the preference of subsystem_dbg
> calls to dev_dbg (whenever possible), as they first preferred format of
> logging debug messages.
> Signed-off-by: Yogesh Chaudhari 

Acked-by: Levente Kurusa 

> ---
>  Documentation/CodingStyle | 17 +
>  1 file changed, 17 insertions(+)
>
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 7fe0546..083f738 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -662,6 +662,23 @@ and driver, and are tagged with the right level:  
> dev_err(), dev_warn(),
>  dev_info(), and so forth.  For messages that aren't associated with a
>  particular device,  defines pr_debug() and pr_info().
>
> +If the subsystem has its own diagnostic macros then they should be used
> +instead of dev_dbg calls.
> +e.g. If you are using network subsystem, use netdev_dbg;
> +if you are using V4L, use v4l_dbg etc.
> +This standardises the output format in every subsystem.
> +
> +Depending on your changes, the following order of precedence
> +applies to printing messages:
> +1. [subsystem]_dbg() is preferred when the subsystem has its own
> +diagnostic macros.
> +2. dev_dbg() is preferred when you have a generic struct device object.
> +3. pr_debug() should be used when 1 and 2 above are not applicable.
> +4. printk() should be avoided.
> +
> +Note: The above order applies to diagnostic calls of all log levels viz:
> +*_emerg, *_alert, *_crit, *_err, *_warn, *_notice, *_info and *_dbg.
> +
>  Coming up with good debugging messages can be quite a challenge; and once
>  you have them, they can be a huge help for remote troubleshooting.  Such
>  messages should be compiled out when the DEBUG symbol is not defined (that
> --
>
> Regards
> Yogesh
> [...]

A lot better, but please next time send it as a separate mail
with subject [PATCH v2] or something like that. Thanks!

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] Add documentation for proper usage and order of preference of calls to print diagnostic messages.

[Levente Kurusa]

Hi,

2014-03-04 12:48 GMT+01:00 yogesh :
> This patch adds documentation that clarifies the use of various diagnostic 
> printing messages. It shows the preference of subsystem_dbg calls to dev_dbg 
> (whenever possible), as the first preferred format of logging debug messages.

Please wrap your changelog at 80 characters a line.

> Signed-off-by: Yogesh Chaudhari 
> ---
>  Documentation/CodingStyle | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 7fe0546..9e0de25 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -662,6 +662,20 @@ and driver, and are tagged with the right level:  
> dev_err(), dev_warn(),
>  dev_info(), and so forth.  For messages that aren't associated with a
>  particular device,  defines pr_debug() and pr_info().
>
> +If the subsystem has its own diagnostic macros then they should be used
> +instead of dev_dbg calls.
> +e.g. If you are using network subsystem, use netdev_dbg;
> +if you are using V4L, use v4l_dbg etc.
> +This standardises the output format in every subsystem.
> +
> +Depending on your changes, the following order of precedence
> +applies to printing messages:
> +1. [subsystem]_dbg() is preferred when you the

The 'you' is unnecessary and incorrect.

> +subsystem has its own diagnostic macros.
> +2. dev_dbg() is preferred when you have a generic struct device object.
> +3. pr_debug() is used when 1 and 2 above are not applicable.

I think it's better to say "should be used".

> +4. printk() should be avoided.
> +
>  Coming up with good debugging messages can be quite a challenge; and once
>  you have them, they can be a huge help for remote troubleshooting.  Such
>  messages should be compiled out when the DEBUG symbol is not defined (that

I think we should also mention *_warn, *_err etc not just *_dbg.

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Patch 1/1] Changes to scripts/checkpatch.pl for improving warning messages in case printk usage is detected in a patch

[Levente Kurusa]

Hi,

2014-03-04 11:54 GMT+01:00 yogesh :
> This patch modifies warning message when printk is used in a patch. It 
> mentions to use subsystem_dbg instead of netdev_dbg as the first preffered 
> format of logging debug messages.
Please wrap your changelog at 80 characters a line.
Also, prefered instead of preffered.

> Signed-off-by: Yogesh Chaudhari 

Other than that:
Acked-by: Levente Kurusa 

Thank you!
> [...]

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Patch 1/1] Changes to scripts/checkpatch.pl for improving warning messages in case printk usage is detected in a patch

[Levente Kurusa]

Hi,

2014-03-04 11:54 GMT+01:00 yogesh mr.yog...@gmail.com:
 This patch modifies warning message when printk is used in a patch. It 
 mentions to use subsystem_dbg instead of netdev_dbg as the first preffered 
 format of logging debug messages.
Please wrap your changelog at 80 characters a line.
Also, prefered instead of preffered.

 Signed-off-by: Yogesh Chaudhari mr.yog...@gmail.com

Other than that:
Acked-by: Levente Kurusa le...@linux.com

Thank you!
 [...]

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] Add documentation for proper usage and order of preference of calls to print diagnostic messages.

[Levente Kurusa]

Hi,

2014-03-04 12:48 GMT+01:00 yogesh mr.yog...@gmail.com:
 This patch adds documentation that clarifies the use of various diagnostic 
 printing messages. It shows the preference of subsystem_dbg calls to dev_dbg 
 (whenever possible), as the first preferred format of logging debug messages.

Please wrap your changelog at 80 characters a line.

 Signed-off-by: Yogesh Chaudhari mr.yog...@gmail.com
 ---
  Documentation/CodingStyle | 14 ++
  1 file changed, 14 insertions(+)

 diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
 index 7fe0546..9e0de25 100644
 --- a/Documentation/CodingStyle
 +++ b/Documentation/CodingStyle
 @@ -662,6 +662,20 @@ and driver, and are tagged with the right level:  
 dev_err(), dev_warn(),
  dev_info(), and so forth.  For messages that aren't associated with a
  particular device, linux/printk.h defines pr_debug() and pr_info().

 +If the subsystem has its own diagnostic macros then they should be used
 +instead of dev_dbg calls.
 +e.g. If you are using network subsystem, use netdev_dbg;
 +if you are using V4L, use v4l_dbg etc.
 +This standardises the output format in every subsystem.
 +
 +Depending on your changes, the following order of precedence
 +applies to printing messages:
 +1. [subsystem]_dbg() is preferred when you the

The 'you' is unnecessary and incorrect.

 +subsystem has its own diagnostic macros.
 +2. dev_dbg() is preferred when you have a generic struct device object.
 +3. pr_debug() is used when 1 and 2 above are not applicable.

I think it's better to say should be used.

 +4. printk() should be avoided.
 +
  Coming up with good debugging messages can be quite a challenge; and once
  you have them, they can be a huge help for remote troubleshooting.  Such
  messages should be compiled out when the DEBUG symbol is not defined (that

I think we should also mention *_warn, *_err etc not just *_dbg.

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] Add documentation for proper usage and order of preference of calls to print diagnostic messages.

[Levente Kurusa]

Hi,

[+CC Rob]

2014-03-04 15:31 GMT+01:00 yogesh mr.yog...@gmail.com:
 This patch adds documentation that clarifies the use of various
 diagnostic printing messages. It shows the preference of subsystem_dbg
 calls to dev_dbg (whenever possible), as they first preferred format of
 logging debug messages.
 Signed-off-by: Yogesh Chaudhari mr.yog...@gmail.com

Acked-by: Levente Kurusa le...@linux.com

 ---
  Documentation/CodingStyle | 17 +
  1 file changed, 17 insertions(+)

 diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
 index 7fe0546..083f738 100644
 --- a/Documentation/CodingStyle
 +++ b/Documentation/CodingStyle
 @@ -662,6 +662,23 @@ and driver, and are tagged with the right level:  
 dev_err(), dev_warn(),
  dev_info(), and so forth.  For messages that aren't associated with a
  particular device, linux/printk.h defines pr_debug() and pr_info().

 +If the subsystem has its own diagnostic macros then they should be used
 +instead of dev_dbg calls.
 +e.g. If you are using network subsystem, use netdev_dbg;
 +if you are using V4L, use v4l_dbg etc.
 +This standardises the output format in every subsystem.
 +
 +Depending on your changes, the following order of precedence
 +applies to printing messages:
 +1. [subsystem]_dbg() is preferred when the subsystem has its own
 +diagnostic macros.
 +2. dev_dbg() is preferred when you have a generic struct device object.
 +3. pr_debug() should be used when 1 and 2 above are not applicable.
 +4. printk() should be avoided.
 +
 +Note: The above order applies to diagnostic calls of all log levels viz:
 +*_emerg, *_alert, *_crit, *_err, *_warn, *_notice, *_info and *_dbg.
 +
  Coming up with good debugging messages can be quite a challenge; and once
  you have them, they can be a huge help for remote troubleshooting.  Such
  messages should be compiled out when the DEBUG symbol is not defined (that
 --

 Regards
 Yogesh
 [...]

A lot better, but please next time send it as a separate mail
with subject [PATCH v2] or something like that. Thanks!

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] scripts/checkpatch.pl: to give more detailed warning message in case printk is used in any patch

[Levente Kurusa]

Hi,

2014-03-02 16:40 GMT+01:00 Joe Perches :
> On Sun, 2014-03-02 at 16:20 +0100, Levente Kurusa wrote:
>> IMHO, this message is too big. The one we already have is nice and clean.
>> I would simply do: s/netdev/[subsystem]/ or something among the lines.
>
> maybe:
>
> "Prefer [subsystem eg: netdev]_$level2 then dev_$level2 then pr_$level to 
> printk(KERN_$orig ...\n"

Excellent, that looks the best and combines the best of two worlds.

>
> or reference the stackoverflow link
>

And what if that disappears? Might as well write this to
Documentation/CodingStyle
as I have previously mentioned. Yogesh, you want to do this or should I?

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] scripts/checkpatch.pl: to give more detailed warning message in case printk is used in any patch

[Levente Kurusa]

Hi,

[+CC LKML, Joe]
[Leaving full copy for LKML, Joe]

On 03/02/2014 04:29 PM, Yogesh Chaudhari wrote:
> On 2 March 2014 20:50, Levente Kurusa  wrote:
>> Hi,
>>
>> On 03/02/2014 04:01 PM, Yogesh Chaudhari wrote:
>>> Based on the discussion here:
>>> https://lkml.org/lkml/2014/3/2/17
>>>
>>> I would like to propose this patch to improve the warning message in
>>> checkpatch.pl.  Comments/Suggestions on possible improvements are
>>> welcome.
>>>
>>>
>>> =
>>>
>>> This patch modifies scripts/checkpatch.pl to give more detailed
>>> warning message in case printk is used in any patch.
>>>
>>> Signed-off-by: Yogesh Chaudhari 
>>> ---
>>>  scripts/checkpatch.pl | 5 -
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index 464dcef..526f33aa 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -2799,7 +2799,10 @@ sub process {
>>>  my $level2 = $level;
>>>  $level2 = "dbg" if ($level eq "debug");
>>>  WARN("PREFER_PR_LEVEL",
>>> - "Prefer netdev_$level2(netdev, ... then
>>> dev_$level2(dev, ... then pr_$level(...  to printk(KERN_$orig ...\n" .
>>> $herecurr);
>>
>> Whoops, that's a word-wrap!
>> Try using git-format-patch+git-send-email to send the patch.
> Ack, my bad, I will make this change.
> 
>>
>>> + "Order of preference for printing debug messages:
>>> + 1. [subsystem]_$level2([subsystem]dev, ... eg
>>> netdev_$level2(netdev, ... for netdevice object
>>> + 2. dev_$level2(dev, ... for drivers with struct device
>>> + 3. pr_$level(...  to printk(KERN_$orig ...\n" . 
>>> $herecurr);
>>>  }
>>>
>>>  if ($line =~ /\bpr_warning\s*\(/) {
>>>
>>
>> IMHO, this message is too big. The one we already have is nice and clean.
>> I would simply do: s/netdev/[subsystem]/ or something among the lines.
> 
> Seems proper way to go about. I agree that this makes it a bit too
> long, however, I was wondering, if there is a detailed
> documentation/information file about checkpatch (or patching in
> general where a detailed message would be accurate). Particularly,
> where we can make a note of proper way of using such debug calls.

Yes there is. Documentation/CodingStyle#Chapter13 is where you should
do stuff like that.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] scripts/checkpatch.pl: to give more detailed warning message in case printk is used in any patch

[Levente Kurusa]

Hi,

On 03/02/2014 04:01 PM, Yogesh Chaudhari wrote:
> Based on the discussion here:
> https://lkml.org/lkml/2014/3/2/17
> 
> I would like to propose this patch to improve the warning message in
> checkpatch.pl.  Comments/Suggestions on possible improvements are
> welcome.
> 
> 
> =
> 
> This patch modifies scripts/checkpatch.pl to give more detailed
> warning message in case printk is used in any patch.
> 
> Signed-off-by: Yogesh Chaudhari 
> ---
>  scripts/checkpatch.pl | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 464dcef..526f33aa 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2799,7 +2799,10 @@ sub process {
>  my $level2 = $level;
>  $level2 = "dbg" if ($level eq "debug");
>  WARN("PREFER_PR_LEVEL",
> - "Prefer netdev_$level2(netdev, ... then
> dev_$level2(dev, ... then pr_$level(...  to printk(KERN_$orig ...\n" .
> $herecurr);

Whoops, that's a word-wrap!
Try using git-format-patch+git-send-email to send the patch.

> + "Order of preference for printing debug messages:
> + 1. [subsystem]_$level2([subsystem]dev, ... eg
> netdev_$level2(netdev, ... for netdevice object
> + 2. dev_$level2(dev, ... for drivers with struct device
> + 3. pr_$level(...  to printk(KERN_$orig ...\n" . $herecurr);
>  }
> 
>  if ($line =~ /\bpr_warning\s*\(/) {
> 

IMHO, this message is too big. The one we already have is nice and clean.
I would simply do: s/netdev/[subsystem]/ or something among the lines.


-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] scripts/checkpatch.pl: to give more detailed warning message in case printk is used in any patch

[Levente Kurusa]

Hi,

On 03/02/2014 04:01 PM, Yogesh Chaudhari wrote:
 Based on the discussion here:
 https://lkml.org/lkml/2014/3/2/17
 
 I would like to propose this patch to improve the warning message in
 checkpatch.pl.  Comments/Suggestions on possible improvements are
 welcome.
 
 
 =
 
 This patch modifies scripts/checkpatch.pl to give more detailed
 warning message in case printk is used in any patch.
 
 Signed-off-by: Yogesh Chaudhari mr.yog...@gmail.com
 ---
  scripts/checkpatch.pl | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)
 
 diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
 index 464dcef..526f33aa 100755
 --- a/scripts/checkpatch.pl
 +++ b/scripts/checkpatch.pl
 @@ -2799,7 +2799,10 @@ sub process {
  my $level2 = $level;
  $level2 = dbg if ($level eq debug);
  WARN(PREFER_PR_LEVEL,
 - Prefer netdev_$level2(netdev, ... then
 dev_$level2(dev, ... then pr_$level(...  to printk(KERN_$orig ...\n .
 $herecurr);

Whoops, that's a word-wrap!
Try using git-format-patch+git-send-email to send the patch.

 + Order of preference for printing debug messages:
 + 1. [subsystem]_$level2([subsystem]dev, ... eg
 netdev_$level2(netdev, ... for netdevice object
 + 2. dev_$level2(dev, ... for drivers with struct device
 + 3. pr_$level(...  to printk(KERN_$orig ...\n . $herecurr);
  }
 
  if ($line =~ /\bpr_warning\s*\(/) {
 

IMHO, this message is too big. The one we already have is nice and clean.
I would simply do: s/netdev/[subsystem]/ or something among the lines.


-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] scripts/checkpatch.pl: to give more detailed warning message in case printk is used in any patch

[Levente Kurusa]

Hi,

[+CC LKML, Joe]
[Leaving full copy for LKML, Joe]

On 03/02/2014 04:29 PM, Yogesh Chaudhari wrote:
 On 2 March 2014 20:50, Levente Kurusa le...@linux.com wrote:
 Hi,

 On 03/02/2014 04:01 PM, Yogesh Chaudhari wrote:
 Based on the discussion here:
 https://lkml.org/lkml/2014/3/2/17

 I would like to propose this patch to improve the warning message in
 checkpatch.pl.  Comments/Suggestions on possible improvements are
 welcome.


 =

 This patch modifies scripts/checkpatch.pl to give more detailed
 warning message in case printk is used in any patch.

 Signed-off-by: Yogesh Chaudhari mr.yog...@gmail.com
 ---
  scripts/checkpatch.pl | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
 index 464dcef..526f33aa 100755
 --- a/scripts/checkpatch.pl
 +++ b/scripts/checkpatch.pl
 @@ -2799,7 +2799,10 @@ sub process {
  my $level2 = $level;
  $level2 = dbg if ($level eq debug);
  WARN(PREFER_PR_LEVEL,
 - Prefer netdev_$level2(netdev, ... then
 dev_$level2(dev, ... then pr_$level(...  to printk(KERN_$orig ...\n .
 $herecurr);

 Whoops, that's a word-wrap!
 Try using git-format-patch+git-send-email to send the patch.
 Ack, my bad, I will make this change.
 

 + Order of preference for printing debug messages:
 + 1. [subsystem]_$level2([subsystem]dev, ... eg
 netdev_$level2(netdev, ... for netdevice object
 + 2. dev_$level2(dev, ... for drivers with struct device
 + 3. pr_$level(...  to printk(KERN_$orig ...\n . 
 $herecurr);
  }

  if ($line =~ /\bpr_warning\s*\(/) {


 IMHO, this message is too big. The one we already have is nice and clean.
 I would simply do: s/netdev/[subsystem]/ or something among the lines.
 
 Seems proper way to go about. I agree that this makes it a bit too
 long, however, I was wondering, if there is a detailed
 documentation/information file about checkpatch (or patching in
 general where a detailed message would be accurate). Particularly,
 where we can make a note of proper way of using such debug calls.

Yes there is. Documentation/CodingStyle#Chapter13 is where you should
do stuff like that.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] scripts/checkpatch.pl: to give more detailed warning message in case printk is used in any patch

[Levente Kurusa]

Hi,

2014-03-02 16:40 GMT+01:00 Joe Perches j...@perches.com:
 On Sun, 2014-03-02 at 16:20 +0100, Levente Kurusa wrote:
 IMHO, this message is too big. The one we already have is nice and clean.
 I would simply do: s/netdev/[subsystem]/ or something among the lines.

 maybe:

 Prefer [subsystem eg: netdev]_$level2 then dev_$level2 then pr_$level to 
 printk(KERN_$orig ...\n

Excellent, that looks the best and combines the best of two worlds.


 or reference the stackoverflow link


And what if that disappears? Might as well write this to
Documentation/CodingStyle
as I have previously mentioned. Yogesh, you want to do this or should I?

--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: How to get rid of IRQF_DISABLED for good?

[Levente Kurusa]

2014-02-20 18:44 GMT+01:00 Michael Opdenacker
:
> Hi,
>
> In spite of the patches I have been sending (and resending!) over the
> past months, there are still 118 occurrences of the idle IRQF_DISABLED
> flag in the kernel code. This corresponds to 31 patches which haven't
> been accepted yet.
>
> What would you advise to get rid of IRQF_DISABLED for good?
>
>   * Send a treewide patch removing the last occurrences in one shot,
> bypassing the regular maintainers? Who could take it?

Andrew Morton would take it to his -mm tree.
This, IMO, seems to be the best solution to circumvent unresponsive/uncaring
maintainers.

>   * Remove the definition of IRQF_DISABLED to force the individual
> maintainers (and out of tree drivers!) to update their code? It
> could be a way of seeing which code isn't maintained any more ;)

No, every single patch has to be 'bisectable' meaning that when you bisect
you should be able to build every single patch as is.

>   * Continue to resend the patches for a few more cycles, until the
> corresponding maintainers can no longer bear the discredit?

Maybe once more, if they don't reply, send it to Andrew Morton as well
and CC a few people who know your work is good so that they can ACK it.

Oh and maybe you could add an __attribute__((deprecated)) to it, but
I am not sure that's possible and/or correct.


--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: How to get rid of IRQF_DISABLED for good?

[Levente Kurusa]

2014-02-20 18:44 GMT+01:00 Michael Opdenacker
michael.opdenac...@free-electrons.com:
 Hi,

 In spite of the patches I have been sending (and resending!) over the
 past months, there are still 118 occurrences of the idle IRQF_DISABLED
 flag in the kernel code. This corresponds to 31 patches which haven't
 been accepted yet.

 What would you advise to get rid of IRQF_DISABLED for good?

   * Send a treewide patch removing the last occurrences in one shot,
 bypassing the regular maintainers? Who could take it?

Andrew Morton would take it to his -mm tree.
This, IMO, seems to be the best solution to circumvent unresponsive/uncaring
maintainers.

   * Remove the definition of IRQF_DISABLED to force the individual
 maintainers (and out of tree drivers!) to update their code? It
 could be a way of seeing which code isn't maintained any more ;)

No, every single patch has to be 'bisectable' meaning that when you bisect
you should be able to build every single patch as is.

   * Continue to resend the patches for a few more cycles, until the
 corresponding maintainers can no longer bear the discredit?

Maybe once more, if they don't reply, send it to Andrew Morton as well
and CC a few people who know your work is good so that they can ACK it.

Oh and maybe you could add an __attribute__((deprecated)) to it, but
I am not sure that's possible and/or correct.


--
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: davinci_vpfe: fix error check

[Levente Kurusa]

The check would check the pointer, which is never less than 0.
According to the error message, the correct check would be
to check the return value of ipipe_mode. Check that instead.

Reported-by: David Binderman 
Signed-off-by: Levente Kurusa 
---
 drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
index 2d36b60..b2daf5e 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
@@ -267,7 +267,7 @@ int config_ipipe_hw(struct vpfe_ipipe_device *ipipe)
}
 
ipipe_mode = get_ipipe_mode(ipipe);
-   if (ipipe < 0) {
+   if (ipipe_mode < 0) {
pr_err("Failed to get ipipe mode");
return -EINVAL;
}
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC] staging: rtl8821ae: fix invalid bit mask on MSR_AP check

[Levente Kurusa]

On 02/15/2014 11:36 AM, Dan Carpenter wrote:
> On Sat, Feb 15, 2014 at 08:53:34AM +0100, Levente Kurusa wrote:
>> Thanks Dan, maybe you know some people who could test it?
>> RTLXX guys? Or maybe we can take the patch as is?
>> I cannot really think of any other solution other than removing
>> the other clause, but since that was written to the file,
>> there must have been some logic behind that. I am slightly
>> disappointed get_maintainer didn't really find anybody for
>> this patch...
> 
> This driver is going to be deleted in the next couple weeks and replaced
> with a re-written from scratch driver.
> 

Oh, I did not know that. Since this fix isn't that big, I think it won't
get into the next -rc, and hence might not hit Linus' tree before
the new driver gets into staging. I am not sure if it is worth applying
if that is the case.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: rts5208: fix always true condition

[Levente Kurusa]

ANDing anything with 0x1E and expecting it to be not 0x03 will always
be true. AND instead with 0x03, so that we check the last two bits,
which should be what was intended there.

Reported-by: David Binderman 
Signed-off-by: Levente Kurusa 
---
 drivers/staging/rts5208/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
index c7c1f54..03548e5 100644
--- a/drivers/staging/rts5208/sd.c
+++ b/drivers/staging/rts5208/sd.c
@@ -3512,7 +3512,7 @@ RTY_SEND_CMD:
TRACE_RET(chip, STATUS_FAIL);
 
} else if (rsp_type == SD_RSP_TYPE_R0) {
-   if ((ptr[3] & 0x1E) != 0x03)
+   if ((ptr[3] & 0x03) != 0x03)
TRACE_RET(chip, STATUS_FAIL);
}
}
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: rts5208: fix always true condition

[Levente Kurusa]

ANDing anything with 0x1E and expecting it to be not 0x03 will always
be true. AND instead with 0x03, so that we check the last two bits,
which should be what was intended there.

Reported-by: David Binderman dcb...@hotmail.com
Signed-off-by: Levente Kurusa le...@linux.com
---
 drivers/staging/rts5208/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
index c7c1f54..03548e5 100644
--- a/drivers/staging/rts5208/sd.c
+++ b/drivers/staging/rts5208/sd.c
@@ -3512,7 +3512,7 @@ RTY_SEND_CMD:
TRACE_RET(chip, STATUS_FAIL);
 
} else if (rsp_type == SD_RSP_TYPE_R0) {
-   if ((ptr[3]  0x1E) != 0x03)
+   if ((ptr[3]  0x03) != 0x03)
TRACE_RET(chip, STATUS_FAIL);
}
}
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC] staging: rtl8821ae: fix invalid bit mask on MSR_AP check

[Levente Kurusa]

On 02/15/2014 11:36 AM, Dan Carpenter wrote:
 On Sat, Feb 15, 2014 at 08:53:34AM +0100, Levente Kurusa wrote:
 Thanks Dan, maybe you know some people who could test it?
 RTLXX guys? Or maybe we can take the patch as is?
 I cannot really think of any other solution other than removing
 the other clause, but since that was written to the file,
 there must have been some logic behind that. I am slightly
 disappointed get_maintainer didn't really find anybody for
 this patch...
 
 This driver is going to be deleted in the next couple weeks and replaced
 with a re-written from scratch driver.
 

Oh, I did not know that. Since this fix isn't that big, I think it won't
get into the next -rc, and hence might not hit Linus' tree before
the new driver gets into staging. I am not sure if it is worth applying
if that is the case.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: davinci_vpfe: fix error check

[Levente Kurusa]

The check would check the pointer, which is never less than 0.
According to the error message, the correct check would be
to check the return value of ipipe_mode. Check that instead.

Reported-by: David Binderman dcb...@hotmail.com
Signed-off-by: Levente Kurusa le...@linux.com
---
 drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
index 2d36b60..b2daf5e 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe_hw.c
@@ -267,7 +267,7 @@ int config_ipipe_hw(struct vpfe_ipipe_device *ipipe)
}
 
ipipe_mode = get_ipipe_mode(ipipe);
-   if (ipipe  0) {
+   if (ipipe_mode  0) {
pr_err(Failed to get ipipe mode);
return -EINVAL;
}
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/