linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Murtaza, Alexandru" <alexandru.murtaza@intel.com>
To: Laura Abbott <labbott@redhat.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "Baluta, Teodora" <teodora.baluta@intel.com>
Subject: RE: [RFC][PATCH] Oops messages transfer using QR codes
Date: Wed, 4 Nov 2015 11:32:54 +0000	[thread overview]
Message-ID: <70CA577BF349C64188E42B39029FE9AD135AB591@IRSMSX102.ger.corp.intel.com> (raw)
In-Reply-To: <5637B1C0.4030900@redhat.com>

> I'm glad to see someone continuing on this work. For the next version, can
> you break it up for ease of review? I'd say put the lib/qr/ addition as one
> patch and then the print_oops as a second patch.

I will do this for the next version, I just didn't really know what would be best.

> Go ahead and drop any __cplusplus wrappers.

Thanks for carefully looking into the patch, this was from the old version and
I only refactored the qr library just so it can pass checkpatch.pl. 

> The preferred style is to not have #ifdef in functions if it can be
> avoided. #ifdef in the header file for print_qr_err would be better.

Could you please refer some example? I don't exactly understand what is
the desired way in this case.

> I gave it a quick test with just a 'echo c > /proc/sysrq-trigger'
> and got a scheduling while atomic warning in addition to not seeing a
> QR code. I don't think waking up a thread on panic is the correct approach
> here. Can you elaborate more on what problem you were trying to solve by
> adding a thread?

You are right. It doesn't do anything for panics. Currently only Oopses are
handled, but this approach should work fine for panics too, with some code 
changes. Basically, what runs inside the qr_thread_func can run after the panic 
with no problem (at least the ASCII version of QR codes). 

> Not quite sure what this is doing here, can you split this out into a
> separate patch?

Oops, Teodora told me I have to change this back and I totally forgot. Sorry.

> This function is defined a few places elsewhere in the kernel, it
> might be worth it to pull it out to a generic header file (bitops.h?)

I will look into this and if applicable, I will do a separate patch.

> I'm guessing this is atomic because it's not possible to determine
> the gfp flags?

As I understood from Teodora, this was a quick fix in case multiple CPUs are 
having an Oops. I will change this and make it more clear.

> This function doesn't help anything. Just put the kzalloc inline.

> This and a bunch of the other functions need to return actual
> error codes and not just -1

> These are already defined in ctypes.h do, do they need to be redefined?

Same explanation as for __cplusplus. I will fix these too.

Thank you very much for your feedback. Please let me know what you think of it
after you test it with Oops messages.

  reply	other threads:[~2015-11-04 11:34 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-30 15:03 [RFC][PATCH] Oops messages transfer using QR codes Murtaza Alexandru
2015-10-30 22:05 ` Randy Dunlap
2015-11-04 13:40   ` Murtaza, Alexandru
2015-11-02 18:56 ` Laura Abbott
2015-11-04 11:32   ` Murtaza, Alexandru [this message]
2015-11-10  0:27     ` Laura Abbott

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=70CA577BF349C64188E42B39029FE9AD135AB591@IRSMSX102.ger.corp.intel.com \
    --to=alexandru.murtaza@intel.com \
    --cc=labbott@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=teodora.baluta@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).