Re: [PATCH] staging: rtl8723bs: align comments

[Eric Curtin]

Hi Fabio,

> I am sorry, I fear I don't understand, checkpatch.sh script says the patch is 
> ok.
> Where have I to add a ' ' (a blank?)?
>
> thank you,
>
> fabio
>

I'm only responding to this because this email is doing a very good job
of avoiding my filters somehow :) I think what Greg means is:

Change this:

 /*
-op_mode
-Set to 0 (HT pure) under the following conditions
-   - all STAs in the BSS are 20/40 MHz HT in 20/40 MHz BSS or
-   - all STAs in the BSS are 20 MHz HT in 20 MHz BSS
-Set to 1 (HT non-member protection) if there may be non-HT STAs
-   in both the primary and the secondary channel
-Set to 2 if only HT STAs are associated in BSS,
-   however and at least one 20 MHz HT STA is associated
-Set to 3 (HT mixed mode) when one or more non-HT STAs are associated
-   (currently non-GF HT station is considered as non-HT STA also)
-*/
+ *op_mode
+ *Set to 0 (HT pure) under the following conditions
+ *  - all STAs in the BSS are 20/40 MHz HT in 20/40 MHz BSS or
+ *  - all STAs in the BSS are 20 MHz HT in 20 MHz BSS
+ *Set to 1 (HT non-member protection) if there may be non-HT STAs
+ *  in both the primary and the secondary channel
+ *Set to 2 if only HT STAs are associated in BSS,
+ *  however and at least one 20 MHz HT STA is associated
+ *Set to 3 (HT mixed mode) when one or more non-HT STAs are associated
+ *  (currently non-GF HT station is considered as non-HT STA also)
+ */

to this:

 /*
-op_mode
-Set to 0 (HT pure) under the following conditions
-   - all STAs in the BSS are 20/40 MHz HT in 20/40 MHz BSS or
-   - all STAs in the BSS are 20 MHz HT in 20 MHz BSS
-Set to 1 (HT non-member protection) if there may be non-HT STAs
-   in both the primary and the secondary channel
-Set to 2 if only HT STAs are associated in BSS,
-   however and at least one 20 MHz HT STA is associated
-Set to 3 (HT mixed mode) when one or more non-HT STAs are associated
-   (currently non-GF HT station is considered as non-HT STA also)
-*/
+ * op_mode
+ * Set to 0 (HT pure) under the following conditions
+ *  - all STAs in the BSS are 20/40 MHz HT in 20/40 MHz BSS or
+ *  - all STAs in the BSS are 20 MHz HT in 20 MHz BSS
+ * Set to 1 (HT non-member protection) if there may be non-HT STAs
+ *  in both the primary and the secondary channel
+ * Set to 2 if only HT STAs are associated in BSS,
+ *  however and at least one 20 MHz HT STA is associated
+ * Set to 3 (HT mixed mode) when one or more non-HT STAs are associated
+ *  (currently non-GF HT station is considered as non-HT STA also)
+ * /

Like Dan said, you need a space after the '*'/

Is mise le meas/Regards,

Eric Curtin

Check out this charity that's close to my heart:

https://www.idonate.ie/fundraiser/11394438_peak-for-pat.html
https://www.facebook.com/Peak-for-Pat-104470678280309
https://www.instagram.com/peakforpat/

Re: [PATCH] Increase limit of max_user_watches from 1/25 to 1/16

[Eric Curtin]

On Wed, 20 Jan 2021 at 13:02, Eric Curtin  wrote:
>
> The current default value for  max_user_watches  is the 1/16 (6.25%) of
> the available low memory, divided for the "watch" cost in bytes.
>
> Tools like inotify-tools and visual studio code, seem to hit these
> limits a little to easy.
>
> Also amending the documentation, it referred to an old value for this.
>
> Signed-off-by: Eric Curtin 
> ---
>  Documentation/admin-guide/sysctl/fs.rst | 4 ++--
>  fs/eventpoll.c  | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/admin-guide/sysctl/fs.rst 
> b/Documentation/admin-guide/sysctl/fs.rst
> index f48277a0a850..f7fe45e69c41 100644
> --- a/Documentation/admin-guide/sysctl/fs.rst
> +++ b/Documentation/admin-guide/sysctl/fs.rst
> @@ -380,5 +380,5 @@ This configuration option sets the maximum number of 
> "watches" that are
>  allowed for each user.
>  Each "watch" costs roughly 90 bytes on a 32bit kernel, and roughly 160 bytes
>  on a 64bit one.
> -The current default value for  max_user_watches  is the 1/32 of the available
> -low memory, divided for the "watch" cost in bytes.
> +The current default value for  max_user_watches  is the 1/16 (6.25%) of the
> +available low memory, divided for the "watch" cost in bytes.
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index a829af074eb5..de9ef8f6d0b2 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -2352,9 +2352,9 @@ static int __init eventpoll_init(void)
>
> si_meminfo();
> /*
> -* Allows top 4% of lomem to be allocated for epoll watches (per 
> user).
> +* Allows top 6.25% of lomem to be allocated for epoll watches (per 
> user).
>  */
> -   max_user_watches = (((si.totalram - si.totalhigh) / 25) << 
> PAGE_SHIFT) /
> +   max_user_watches = (((si.totalram - si.totalhigh) / 16) << 
> PAGE_SHIFT) /
> EP_ITEM_COST;
> BUG_ON(max_user_watches < 0);
>
> --
> 2.25.1
>

Please ignore this, this is the wrong limit (an epoll one), I sent
another patch just
to update the documentation to be correct. Weiman Long already kindly solved the
issue in 92890123749bafc317bbfacbe0a62ce08d78efb7

Separate patch is titled "[PATCH] Update
Documentation/admin-guide/sysctl/fs.rst"

[PATCH] Increase limit of max_user_watches from 1/25 to 1/16

[Eric Curtin]

The current default value for  max_user_watches  is the 1/16 (6.25%) of
the available low memory, divided for the "watch" cost in bytes.

Tools like inotify-tools and visual studio code, seem to hit these
limits a little to easy.

Also amending the documentation, it referred to an old value for this.

Signed-off-by: Eric Curtin 
---
 Documentation/admin-guide/sysctl/fs.rst | 4 ++--
 fs/eventpoll.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/sysctl/fs.rst 
b/Documentation/admin-guide/sysctl/fs.rst
index f48277a0a850..f7fe45e69c41 100644
--- a/Documentation/admin-guide/sysctl/fs.rst
+++ b/Documentation/admin-guide/sysctl/fs.rst
@@ -380,5 +380,5 @@ This configuration option sets the maximum number of 
"watches" that are
 allowed for each user.
 Each "watch" costs roughly 90 bytes on a 32bit kernel, and roughly 160 bytes
 on a 64bit one.
-The current default value for  max_user_watches  is the 1/32 of the available
-low memory, divided for the "watch" cost in bytes.
+The current default value for  max_user_watches  is the 1/16 (6.25%) of the
+available low memory, divided for the "watch" cost in bytes.
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index a829af074eb5..de9ef8f6d0b2 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -2352,9 +2352,9 @@ static int __init eventpoll_init(void)
 
si_meminfo();
/*
-* Allows top 4% of lomem to be allocated for epoll watches (per user).
+* Allows top 6.25% of lomem to be allocated for epoll watches (per 
user).
 */
-   max_user_watches = (((si.totalram - si.totalhigh) / 25) << PAGE_SHIFT) /
+   max_user_watches = (((si.totalram - si.totalhigh) / 16) << PAGE_SHIFT) /
EP_ITEM_COST;
BUG_ON(max_user_watches < 0);
 
-- 
2.25.1

[PATCH] Update Documentation/admin-guide/sysctl/fs.rst

[Eric Curtin]

max_user_watches for epoll should say 1/25, rather than 1/32

Signed-off-by: Eric Curtin 
---
 Documentation/admin-guide/sysctl/fs.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/sysctl/fs.rst 
b/Documentation/admin-guide/sysctl/fs.rst
index f48277a0a850..2a501c9ddc55 100644
--- a/Documentation/admin-guide/sysctl/fs.rst
+++ b/Documentation/admin-guide/sysctl/fs.rst
@@ -380,5 +380,5 @@ This configuration option sets the maximum number of 
"watches" that are
 allowed for each user.
 Each "watch" costs roughly 90 bytes on a 32bit kernel, and roughly 160 bytes
 on a 64bit one.
-The current default value for  max_user_watches  is the 1/32 of the available
-low memory, divided for the "watch" cost in bytes.
+The current default value for  max_user_watches  is the 1/25 (4%) of the
+available low memory, divided for the "watch" cost in bytes.
-- 
2.25.1

[PATCH] rename lpfc_sli4_dump_page_a0 to lpfc_sli4_dump_sfp_pagea0

[Eric Curtin]

Comment did not match function signature.

Signed-off-by: Eric Curtin 
---
 drivers/scsi/lpfc/lpfc_mbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/lpfc/lpfc_mbox.c b/drivers/scsi/lpfc/lpfc_mbox.c
index 3414ffcb26fe..c03a7f12dd65 100644
--- a/drivers/scsi/lpfc/lpfc_mbox.c
+++ b/drivers/scsi/lpfc/lpfc_mbox.c
@@ -2409,7 +2409,7 @@ lpfc_mbx_cmpl_rdp_page_a0(struct lpfc_hba *phba, 
LPFC_MBOXQ_t *mbox)
 
 
 /*
- * lpfc_sli4_dump_sfp_pagea0 - Dump sli4 read SFP Diagnostic.
+ * lpfc_sli4_dump_page_a0 - Dump sli4 read SFP Diagnostic.
  * @phba: pointer to the hba structure containing.
  * @mbox: pointer to lpfc mbox command to initialize.
  *
-- 
2.25.1

Solving the portable binaries problem in the elf loader

[Eric Curtin]

Hi Guys,

I remember watching this YouTube video years ago and something Linus
said really stuck with me:

https://www.youtube.com/watch?v=5PmHRSeA2c8

It was around the whole binaries, package management problem, because
it's been something that has been troublesome for me, building separate
binaries for separate Linux platforms.

I've examined the various efforts to fix this: flatpack, snap,
AppImage, FatELF, docker etc. The kindof solve the problem but not
100%, some aspects of their approaches are nice. I think this
problem can be addressed in the ELF loader. By the way I don't claim to
be an expert on ELF, I know just enough to do my job.

So ELF tends to depend on a filename with a version embedded in the
filename to determine if the shared library is the correct one to load,
/bin/true example:

  ldd /bin/true
  linux-vdso.so.1 (0x7ffe0218a000)
  libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x7f47086c9000)
  /lib64/ld-linux-x86-64.so.2 (0x7f47088e5000)

What if instead of depending on a filename on determining if the
library is the correct version, we used a checksum (like a SHA or
something even cheaper) taking a little influence from how git
determines if patches are valid, this is a relatively cheap operation:

  time sha1sum /lib/x86_64-linux-gnu/libc-2.31.so
  4fcd76645607f38d91f65654ddd6b9770b5ea54a  /lib/x86_64-linux-gnu/libc-2.31.so

  real 0m0.008s
  user 0m0.008s
  sys0m0.001s

You could store a volatile cache of checksummed libraries in memory to
ensure you don't do this more than once for each inode that contains a
library.

package managers generally add their own versioning system, because
kernelspace doesn't really give you one. With this ELF loading feature
you now have that. It also opens up the possibility of distributing
binaries and libraries in a more P2P fashion as a checksum is core to
the ELF loading process.

If this feature was added I think it would percolate down to the
various distros, package managers in time.

The end product would be more cross-platform binaries, without doing
tricks like fat binaries. In code you could maintain backwards
compatibility like:

  if (legacy_elf) {
load_libraries_based_on_filename;
return 0;
  }

  load_libraries_based_on_checksum;

I wouldn't mind hearing thoughts from Linus on this as it fixes many of
the problems he mentions like:

"you don't make binaries for Linux you, make binaries for Fedora 19 for
Oh 20, maybe there's even like rl5 from ten year ago"

with the checksummed library approach you fix this problem. When you
execute the binary and if it doesn't have all the checksummed libraries
available you pull a library with the correct checksum.

"using shared libraries is not an option when the libraries are
experimental and the libraries are used by two people and one of them
is crazy so every other day some ABI breaks right so"

Fixes this problem because you embed checksums for the libraries you
want your binary to load, of course you must update that checksum in
the main binary if you want to upgrade. That actually doesn't seem
like a huge problem as checksums are fixed width.

"they break binary compatibility left and right they update glibc and
everything breaks you a you can"

again, the checksums fix this.

"can't have application writers do 15 billion different versions"

I think if you centralize how versioning is done by using checksums in
the elf loader you relieve this problem.

"I also guarantee you that every single desktop distribution will care
about Valve binaries so the problem is valve will build everything
statically linked and create huge binaries"

You are still using dynamic linking using this model, so this problem
is solved.

I could keep going but I think it solves most of the problems he
talks about (and I tend to have the same problems), I think it could be
solved in the ELF loader. Anybody see potential here? I'd greatly
appreciate feedback.

As a side effect you also gain more security around loading libraries,
although that's not the problem I was trying to solve here, it's the
many builds, portable binaries problem I'm trying to poke.

Might even save the environment a little, many more builds of binaries
around the world than needed, the checksum when dynamically
linking/loading seems like a fair tradeoff.

Trying to run mptcp on my machine

[Eric Curtin]

Hi Guys,

I've been trying to get mptcp up and running on my machine (xubuntu
20.04) with little joy. What I did was install 5,8,5 kernel from here:

https://kernel.ubuntu.com/~kernel-ppa/mainline/v5.8.5/amd64/

Reboot, tried a curl:

curl http://www.multipath-tcp.org
Nay, Nay, Nay, your have an old computer that does not speak MPTCP.
Shame on you!

Checked this flag:

sudo cat /proc/sys/net/mptcp/enabled
1

Even tried to run this guy in the kernel repo with no joy
mptcp_connect.sh. Any pointers to get mptcp running? I couldn't find
too much documentation on how to configure it on GNU/Linux.

inotify question on modify events and openat

[Eric Curtin]

Hi Guys,

In the manpages "man inotify" it is suggested that function calls like
write and truncate are modify events. But from this opened issue it
appears like openat might be a modify event to inotify also?

https://github.com/inotify-tools/inotify-tools/issues/116

Just curious, is this expected, should I update the documentation?

Regards,
Eric

Re: Looking for an open-source thesis idea

[Eric Curtin]

Hi Sandy,

I actually have worked quite a bit with IPsec, it's not a protocol I'm
a huge fan of, it's use of multiple ports make it difficult to work
with middleboxs (be it load-balancers, TLS interceptors, reverse
proxies, proxies, firewalls, routers, switches, etc.). I've even seen
issues where some middleboxes only recognize TCP/UDP packets and not
ESP packets. There's so many implementations of IPsec with various
routers OS's and the standard seems to be only sort of universally
accepted. It can be difficult to deploy.

Although Wireshark does solve at many of these problems, it's simpler
at least, as regards VPNs I really like it. I'm actually more a fan of
protocols that applications have a little more control over like QUIC
over UDP or TLS over TCP.

I actually use HTTPS Everywhere plugin, but at the end of the day,
that simply just turns on TLS encryption if it's available right?

I like some of the problems QUIC solves, the multiple handshake
problem decreasing overall round trips, and just that it's more
modern. openssl is brilliant, but there's a lot of deadwood, older
encryption techniques in that codebase.

A monolithic secure TCP protocol seems like a nice idea, but maybe it
is too difficult.

I think it's a nice idea to explore OOM killer and compare it to the
solutions on various other OS's (FreeBSD, AIX, z/OS, Solaris, HP-UX,
macOS, iOS, Windows, Zircon, etc. and the OS I work on Powermax).
Thanks for that.

Any other ideas, keep them coming :)

On Tue, 26 May 2020 at 08:18, Sandy Harris  wrote:
>
> Eric Curtin  wrote:
>
> > Hope I'm not bothering you. I'm looking for a masters thesis idea, ...
>
> > I'm really liking this
> > new QUIC (UDP) protocol as an alternative to TCP over TLS. And with
> > the growth of new modern secure protocols like Wireguard. I was
> > wondering, would it be an idea to do a monolithic secure TCP protocol
> > (as an alternative to TCP over TLS) as a small thesis project or is it
> > as hard as the guys at Google make is sound?
> >
> > "Because TCP is implemented in operating system kernels, and middlebox
> > firmware, making significant changes to TCP is next to impossible."
>
> I'm inclined to agree with the Google folk on that. However, what about
> IPsec? That was designed to secure anything-over-IP so it should be
> a more general solution. The FreeS/WAN project added opportunistic
> encryption for wider availability
> https://freeswan.org/freeswan_trees/freeswan-2.06/doc/intro.html#goals
>
> Today some opportunistic encryption protocols -- SMTP-over-TLS and
> HTTPS Everywhere -- are quite widespread but my impression is
> that opportunistic IPsec is not. Would adding it to an open source
> router be a thesis-sized project? Or, since routers likely have IPsec
> already, just making it easier to deploy?
>
> > I'm open to any other suggestions also for my thesis :)
>
> Linux's OOM killer strikes me as a spectacularly ugly kluge,
> but people who are certainly more knowledgeable and likely
> more competent seem to think it is necessary. Is there a
> thesis in examining it, looking at how other Unix-like systems
> handle the problem & perhaps implementing an alternative
> for Linux?

Looking for an open-source thesis idea

[Eric Curtin]

Hi Guys,

Hope I'm not bothering you. I'm looking for a masters thesis idea, and
if possible doing one related to open source software (of course I
have the option of tying it in to the Powermax kernel I work on also
with Dell). One idea that sprung to mind is, I'm really liking this
new QUIC (UDP) protocol as an alternative to TCP over TLS. And with
the growth of new modern secure protocols like Wireguard. I was
wondering, would it be an idea to do a monolithic secure TCP protocol
(as an alternative to TCP over TLS) as a small thesis project or is it
as hard as the guys at Google make is sound?

"Because TCP is implemented in operating system kernels, and middlebox
firmware, making significant changes to TCP is next to impossible."

I'm open to any other suggestions also for my thesis :)

The middlebox firmware sounds like it could be the issue I guess.

Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3

[Eric Curtin]

Hi Guys,

I initially thought these patches were a joke. But I guess they are
not. I suppose 2018 is the year everything became offensive.

Could we avoid the s/fuck/hug/g though? I have nothing against
re-wording this stuff to remove the curse word, but it should at least
make sense.

What's going to happen is someone is a newbie is going to see a comment
like "We found an mark in the idr at the right wd, but it's not the
mark we were told to remove. eparis seriously hugged up somewhere",
probably google the term as they are unfamiliar with it, find out it's
an alias for "fucked" and if they are sensitive get offended anyway.

On Sat, 1 Dec 2018 at 08:20, Geert Uytterhoeven  wrote:
>
> Hi Jon,
>
> On Fri, Nov 30, 2018 at 11:15 PM Jonathan Corbet  wrote:
> > On Fri, 30 Nov 2018 14:12:19 -0800
> > Jarkko Sakkinen  wrote:
> >
> > > As a maintainer myself (and based on somewhat disturbed feedback from
> > > other maintainers) I can only make the conclusion that nobody knows what
> > > the responsibility part here means.
> > >
> > > I would interpret, if I read it like at lawyer at least, that even for
> > > existing code you would need to do the changes postmorterm.
> > >
> > > Is this wrong interpretation?  Should I conclude that I made a mistake
> > > by reading the CoC and trying to understand what it *actually* says?
> > > After this discussion, I can say that I understand it less than before.
> >
> > Have you read Documentation/process/code-of-conduct-interpretation.rst?
> > As has been pointed out, it contains a clear answer to how things should
> > be interpreted here.
>
> Indeed:
>
> | Contributions submitted for the kernel should use appropriate language.
> | Content that already exists predating the Code of Conduct will not be
> | addressed now as a violation.
>
> However:
>
> | Inappropriate language can be seen as a
> | bug, though; such bugs will be fixed more quickly if any interested
> | parties submit patches to that effect.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [PATCH] staging: fbtft: fix character limit, trailing ; warning, etc.

[Eric Curtin]

On 28 April 2017 at 11:07, Greg KH <gre...@linuxfoundation.org> wrote:
> On Sun, Apr 23, 2017 at 03:21:31PM +0100, Eric Curtin wrote:
>> checkpatch spits out a warning about the 80 character line limit. Split
>> the parameters of these functions onto different lines. Put the ; with
>> the macro caller instead. Lined up parameters as there was another
>> CHECK warning about that.
>>
>> Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
>> ---
>>  drivers/staging/fbtft/fbtft-bus.c | 36 +---
>>  1 file changed, 21 insertions(+), 15 deletions(-)
>
> Patch does not apply to my tree at all :(

Hi Greg,

Sorry I didn't realize you replied. I gave up on these drivers because
I read around and saw that these drivers are unlikely to ever be merged
properly into the mainline kernel, while they use this deprecated
framework:

https://github.com/notro/fbtft/wiki/Development

I know you were involved in some of these conversations:

https://lkml.org/lkml/2016/11/23/146

My main reason for helping with this, was so my rpi3 display would
"just work" on future distro's I installed. I'd struggle to port to
this new framework as I have no experience with drivers.

Re: [PATCH] staging: fbtft: fix character limit, trailing ; warning, etc.

[Eric Curtin]

On 28 April 2017 at 11:07, Greg KH  wrote:
> On Sun, Apr 23, 2017 at 03:21:31PM +0100, Eric Curtin wrote:
>> checkpatch spits out a warning about the 80 character line limit. Split
>> the parameters of these functions onto different lines. Put the ; with
>> the macro caller instead. Lined up parameters as there was another
>> CHECK warning about that.
>>
>> Signed-off-by: Eric Curtin 
>> ---
>>  drivers/staging/fbtft/fbtft-bus.c | 36 +---
>>  1 file changed, 21 insertions(+), 15 deletions(-)
>
> Patch does not apply to my tree at all :(

Hi Greg,

Sorry I didn't realize you replied. I gave up on these drivers because
I read around and saw that these drivers are unlikely to ever be merged
properly into the mainline kernel, while they use this deprecated
framework:

https://github.com/notro/fbtft/wiki/Development

I know you were involved in some of these conversations:

https://lkml.org/lkml/2016/11/23/146

My main reason for helping with this, was so my rpi3 display would
"just work" on future distro's I installed. I'd struggle to port to
this new framework as I have no experience with drivers.

[PATCH] staging: fbtft: fix character limit, trailing ; warning, etc.

[Eric Curtin]

checkpatch spits out a warning about the 80 character line limit. Split
the parameters of these functions onto different lines. Put the ; with
the macro caller instead. Lined up parameters as there was another
CHECK warning about that.

Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
---
 drivers/staging/fbtft/fbtft-bus.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-bus.c 
b/drivers/staging/fbtft/fbtft-bus.c
index ec45043..f1522f4 100644
--- a/drivers/staging/fbtft/fbtft-bus.c
+++ b/drivers/staging/fbtft/fbtft-bus.c
@@ -24,7 +24,9 @@ void func(struct fbtft_par *par, int len, ...)
\
buf[i] = (type)va_arg(args, unsigned int);\
} \
va_end(args); \
-   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device, 
type, buf, len, "%s: ", __func__);   \
+   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par,  \
+ par->info->device, type, buf, len, "%s: ",  \
+ __func__);  \
} \
  \
va_start(args, len);  \
@@ -41,7 +43,9 @@ void func(struct fbtft_par *par, int len, ...)
\
ret = par->fbtftops.write(par, par->buf, sizeof(type) + offset);  \
if (ret < 0) {\
va_end(args); \
-   dev_err(par->info->device, "%s: write() failed and returned 
%d\n", __func__, ret); \
+   dev_err(par->info->device,\
+   "%s: write() failed and returned %d\n", __func__, \
+   ret); \
return;   \
} \
len--;\
@@ -60,17 +64,19 @@ void func(struct fbtft_par *par, int len, ...)  
  \
  len * (sizeof(type) + offset)); \
if (ret < 0) {\
va_end(args); \
-   dev_err(par->info->device, "%s: write() failed and 
returned %d\n", __func__, ret); \
+   dev_err(par->info->device,\
+   "%s: write() failed and returned %d\n",   \
+   __func__, ret);   \
return;   \
} \
} \
va_end(args); \
 } \
-EXPORT_SYMBOL(func);
+EXPORT_SYMBOL(func)
 
-define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, )
-define_fbtft_write_reg(fbtft_write_reg16_bus8, u16, cpu_to_be16)
-define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, )
+define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, );
+define_fbtft_write_reg(fbtft_write_reg16_bus8, u16, cpu_to_be16);
+define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, );
 
 void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, ...)
 {
@@ -84,8 +90,8 @@ void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, 
...)
for (i = 0; i < len; i++)
*(((u8 *)buf) + i) = (u8)va_arg(args, unsigned int);
va_end(args);
-   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par,
-   par->info->device, u8, buf, len, "%s: ", __func__);
+   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device,
+ u8, buf, len, "%s: ", __func__);
}
if (len <= 0)
return;
@@ -135,7 +141,7 @@ int fbtft_write_vmem16_bus8(struct fbtft_par *par, size_t 
offset, size_t len)
size_t startbyte_size = 0;
 
fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "%s(offset=%zu, len=%zu)\n",
- 

[PATCH] staging: fbtft: fix character limit, trailing ; warning, etc.

[Eric Curtin]

checkpatch spits out a warning about the 80 character line limit. Split
the parameters of these functions onto different lines. Put the ; with
the macro caller instead. Lined up parameters as there was another
CHECK warning about that.

Signed-off-by: Eric Curtin 
---
 drivers/staging/fbtft/fbtft-bus.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-bus.c 
b/drivers/staging/fbtft/fbtft-bus.c
index ec45043..f1522f4 100644
--- a/drivers/staging/fbtft/fbtft-bus.c
+++ b/drivers/staging/fbtft/fbtft-bus.c
@@ -24,7 +24,9 @@ void func(struct fbtft_par *par, int len, ...)
\
buf[i] = (type)va_arg(args, unsigned int);\
} \
va_end(args); \
-   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device, 
type, buf, len, "%s: ", __func__);   \
+   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par,  \
+ par->info->device, type, buf, len, "%s: ",  \
+ __func__);  \
} \
  \
va_start(args, len);  \
@@ -41,7 +43,9 @@ void func(struct fbtft_par *par, int len, ...)
\
ret = par->fbtftops.write(par, par->buf, sizeof(type) + offset);  \
if (ret < 0) {\
va_end(args); \
-   dev_err(par->info->device, "%s: write() failed and returned 
%d\n", __func__, ret); \
+   dev_err(par->info->device,\
+   "%s: write() failed and returned %d\n", __func__, \
+   ret); \
return;   \
} \
len--;\
@@ -60,17 +64,19 @@ void func(struct fbtft_par *par, int len, ...)  
  \
  len * (sizeof(type) + offset)); \
if (ret < 0) {\
va_end(args); \
-   dev_err(par->info->device, "%s: write() failed and 
returned %d\n", __func__, ret); \
+   dev_err(par->info->device,\
+   "%s: write() failed and returned %d\n",   \
+   __func__, ret);   \
return;   \
} \
} \
va_end(args); \
 } \
-EXPORT_SYMBOL(func);
+EXPORT_SYMBOL(func)
 
-define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, )
-define_fbtft_write_reg(fbtft_write_reg16_bus8, u16, cpu_to_be16)
-define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, )
+define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, );
+define_fbtft_write_reg(fbtft_write_reg16_bus8, u16, cpu_to_be16);
+define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, );
 
 void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, ...)
 {
@@ -84,8 +90,8 @@ void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, 
...)
for (i = 0; i < len; i++)
*(((u8 *)buf) + i) = (u8)va_arg(args, unsigned int);
va_end(args);
-   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par,
-   par->info->device, u8, buf, len, "%s: ", __func__);
+   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device,
+ u8, buf, len, "%s: ", __func__);
}
if (len <= 0)
return;
@@ -135,7 +141,7 @@ int fbtft_write_vmem16_bus8(struct fbtft_par *par, size_t 
offset, size_t len)
size_t startbyte_size = 0;
 
fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "%s(offset=%zu, len=%zu)\n",
-   __func__, offset, len);
+ 

[PATCH] staging: fbtft: fix character limit, trailing ; warning, etc.

[Eric Curtin]

checkpatch spits out a warning about the 80 character line limit. Split
the parameters of these functions onto different lines. Put the ; with
the macro caller instead. Lined up parameters as there was another
CHECK warning about that.

Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
---
 drivers/staging/fbtft/fbtft-bus.c | 27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-bus.c 
b/drivers/staging/fbtft/fbtft-bus.c
index ec45043..3143050 100644
--- a/drivers/staging/fbtft/fbtft-bus.c
+++ b/drivers/staging/fbtft/fbtft-bus.c
@@ -24,7 +24,14 @@ void func(struct fbtft_par *par, int len, ...)   
 \
buf[i] = (type)va_arg(args, unsigned int);\
} \
va_end(args); \
-   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device, 
type, buf, len, "%s: ", __func__);   \
+   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER,   \
+ par,\
+ par->info->device,  \
+ type,   \
+ buf,\
+ len,\
+ "%s: ", \
+ __func__);  \
} \
  \
va_start(args, len);  \
@@ -41,7 +48,10 @@ void func(struct fbtft_par *par, int len, ...)   
 \
ret = par->fbtftops.write(par, par->buf, sizeof(type) + offset);  \
if (ret < 0) {\
va_end(args); \
-   dev_err(par->info->device, "%s: write() failed and returned 
%d\n", __func__, ret); \
+   dev_err(par->info->device,\
+   "%s: write() failed and returned %d\n",   \
+   __func__, \
+   ret); \
return;   \
} \
len--;\
@@ -60,17 +70,20 @@ void func(struct fbtft_par *par, int len, ...)  
  \
  len * (sizeof(type) + offset)); \
if (ret < 0) {\
va_end(args); \
-   dev_err(par->info->device, "%s: write() failed and 
returned %d\n", __func__, ret); \
+   dev_err(par->info->device,\
+   "%s: write() failed and returned %d\n",   \
+   __func__, \
+   ret); \
return;   \
} \
} \
va_end(args); \
 } \
-EXPORT_SYMBOL(func);
+EXPORT_SYMBOL(func)
 
-define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, )
-define_fbtft_write_reg(fbtft_write_reg16_bus8, u16, cpu_to_be16)
-define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, )
+define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, );
+define_fbtft_write_reg(fbtft_write_reg16_bus8, u16, cpu_to_be16);
+define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, );
 
 void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, ...)
 {
-- 
2.7.4

[PATCH] staging: fbtft: fix character limit, trailing ; warning, etc.

[Eric Curtin]

checkpatch spits out a warning about the 80 character line limit. Split
the parameters of these functions onto different lines. Put the ; with
the macro caller instead. Lined up parameters as there was another
CHECK warning about that.

Signed-off-by: Eric Curtin 
---
 drivers/staging/fbtft/fbtft-bus.c | 27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-bus.c 
b/drivers/staging/fbtft/fbtft-bus.c
index ec45043..3143050 100644
--- a/drivers/staging/fbtft/fbtft-bus.c
+++ b/drivers/staging/fbtft/fbtft-bus.c
@@ -24,7 +24,14 @@ void func(struct fbtft_par *par, int len, ...)   
 \
buf[i] = (type)va_arg(args, unsigned int);\
} \
va_end(args); \
-   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device, 
type, buf, len, "%s: ", __func__);   \
+   fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER,   \
+ par,\
+ par->info->device,  \
+ type,   \
+ buf,\
+ len,\
+ "%s: ", \
+ __func__);  \
} \
  \
va_start(args, len);  \
@@ -41,7 +48,10 @@ void func(struct fbtft_par *par, int len, ...)   
 \
ret = par->fbtftops.write(par, par->buf, sizeof(type) + offset);  \
if (ret < 0) {\
va_end(args); \
-   dev_err(par->info->device, "%s: write() failed and returned 
%d\n", __func__, ret); \
+   dev_err(par->info->device,\
+   "%s: write() failed and returned %d\n",   \
+   __func__, \
+   ret); \
return;   \
} \
len--;\
@@ -60,17 +70,20 @@ void func(struct fbtft_par *par, int len, ...)  
  \
  len * (sizeof(type) + offset)); \
if (ret < 0) {\
va_end(args); \
-   dev_err(par->info->device, "%s: write() failed and 
returned %d\n", __func__, ret); \
+   dev_err(par->info->device,\
+   "%s: write() failed and returned %d\n",   \
+   __func__, \
+   ret); \
return;   \
} \
} \
va_end(args); \
 } \
-EXPORT_SYMBOL(func);
+EXPORT_SYMBOL(func)
 
-define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, )
-define_fbtft_write_reg(fbtft_write_reg16_bus8, u16, cpu_to_be16)
-define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, )
+define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, );
+define_fbtft_write_reg(fbtft_write_reg16_bus8, u16, cpu_to_be16);
+define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, );
 
 void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, ...)
 {
-- 
2.7.4

Re: [PATCH] Remove ambiguous logging for "Unsupported brightness interface"

[Eric Curtin]

On 30 January 2016 at 12:20, Henrique de Moraes Holschuh  
wrote:
> On Wed, 27 Jan 2016, Joe Perches wrote:
>> On Wed, 2016-01-27 at 22:14 +0000, Eric Curtin wrote:
>> > Message gets logged on machines that are well supported.
>> >
>> > Signed-off-by: Eric Curtin 
>> > ---
>> >  drivers/platform/x86/thinkpad_acpi.c | 1 -
>> >  1 file changed, 1 deletion(-)
>> >
>> > diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> > b/drivers/platform/x86/thinkpad_acpi.c
>> > index a268a7a..4eb41aa 100644
>> > --- a/drivers/platform/x86/thinkpad_acpi.c
>> > +++ b/drivers/platform/x86/thinkpad_acpi.c
>> > @@ -6661,7 +6661,6 @@ static void __init
>> > tpacpi_detect_brightness_capabilities(void)
>> > pr_info("detected a 8-level brightness capable
>> > ThinkPad\n");
>> > break;
>> > default:
>> > -   pr_info("Unsupported brightness interface\n");
>> > tp_features.bright_unkfw = 1;
>> > bright_maxlvl = b - 1;
>> > }
>>
>> Perhaps this should be something like this instead:
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index a268a7a..bd12c71 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -6653,18 +6653,16 @@ static void __init 
>> tpacpi_detect_brightness_capabilities(void)
>>   switch (b) {
>>   case 16:
>>   bright_maxlvl = 15;
>> - pr_info("detected a 16-level brightness capable ThinkPad\n");
>>   break;
>>   case 8:
>>   case 0:
>>   bright_maxlvl = 7;
>> - pr_info("detected a 8-level brightness capable ThinkPad\n");
>>   break;
>>   default:
>> - pr_info("Unsupported brightness interface\n");
>>   tp_features.bright_unkfw = 1;
>>   bright_maxlvl = b - 1;
>>   }
>> + pr_info("detected %u brightness levels\n", bright_maxlvl + 1);
>>  }
>
> This can be made pr_debug, since we're touching it...
>
> --
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh

"Unsupported brightness interface" message gets logged on
 machines that are well supported.

Signed-off-by: Eric Curtin 
---
 drivers/platform/x86/thinkpad_acpi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index a268a7a..e305ab5 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6653,18 +6653,16 @@ static void __init
tpacpi_detect_brightness_capabilities(void)
switch (b) {
case 16:
bright_maxlvl = 15;
-   pr_info("detected a 16-level brightness capable ThinkPad\n");
break;
case 8:
case 0:
bright_maxlvl = 7;
-   pr_info("detected a 8-level brightness capable ThinkPad\n");
break;
default:
-   pr_info("Unsupported brightness interface\n");
tp_features.bright_unkfw = 1;
bright_maxlvl = b - 1;
}
+   pr_debug("detected %u brightness levels\n", bright_maxlvl + 1);
 }

 static int __init brightness_init(struct ibm_init_struct *iibm)
-- 
2.5.0

Re: [PATCH] Remove ambiguous logging for "Unsupported brightness interface"

[Eric Curtin]

On 30 January 2016 at 12:20, Henrique de Moraes Holschuh <h...@hmh.eng.br> 
wrote:
> On Wed, 27 Jan 2016, Joe Perches wrote:
>> On Wed, 2016-01-27 at 22:14 +0000, Eric Curtin wrote:
>> > Message gets logged on machines that are well supported.
>> >
>> > Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
>> > ---
>> >  drivers/platform/x86/thinkpad_acpi.c | 1 -
>> >  1 file changed, 1 deletion(-)
>> >
>> > diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> > b/drivers/platform/x86/thinkpad_acpi.c
>> > index a268a7a..4eb41aa 100644
>> > --- a/drivers/platform/x86/thinkpad_acpi.c
>> > +++ b/drivers/platform/x86/thinkpad_acpi.c
>> > @@ -6661,7 +6661,6 @@ static void __init
>> > tpacpi_detect_brightness_capabilities(void)
>> > pr_info("detected a 8-level brightness capable
>> > ThinkPad\n");
>> > break;
>> > default:
>> > -   pr_info("Unsupported brightness interface\n");
>> > tp_features.bright_unkfw = 1;
>> > bright_maxlvl = b - 1;
>> > }
>>
>> Perhaps this should be something like this instead:
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index a268a7a..bd12c71 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -6653,18 +6653,16 @@ static void __init 
>> tpacpi_detect_brightness_capabilities(void)
>>   switch (b) {
>>   case 16:
>>   bright_maxlvl = 15;
>> - pr_info("detected a 16-level brightness capable ThinkPad\n");
>>   break;
>>   case 8:
>>   case 0:
>>   bright_maxlvl = 7;
>> - pr_info("detected a 8-level brightness capable ThinkPad\n");
>>   break;
>>   default:
>> - pr_info("Unsupported brightness interface\n");
>>   tp_features.bright_unkfw = 1;
>>   bright_maxlvl = b - 1;
>>   }
>> + pr_info("detected %u brightness levels\n", bright_maxlvl + 1);
>>  }
>
> This can be made pr_debug, since we're touching it...
>
> --
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh

"Unsupported brightness interface" message gets logged on
 machines that are well supported.

Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index a268a7a..e305ab5 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6653,18 +6653,16 @@ static void __init
tpacpi_detect_brightness_capabilities(void)
switch (b) {
case 16:
bright_maxlvl = 15;
-   pr_info("detected a 16-level brightness capable ThinkPad\n");
break;
case 8:
case 0:
bright_maxlvl = 7;
-   pr_info("detected a 8-level brightness capable ThinkPad\n");
break;
default:
-   pr_info("Unsupported brightness interface\n");
tp_features.bright_unkfw = 1;
bright_maxlvl = b - 1;
}
+   pr_debug("detected %u brightness levels\n", bright_maxlvl + 1);
 }

 static int __init brightness_init(struct ibm_init_struct *iibm)
-- 
2.5.0

Re: [PATCH] Remove ambiguous logging for "Unsupported brightness interface"

[Eric Curtin]

On 28 January 2016 at 00:43, Joe Perches  wrote:
> On Thu, 2016-01-28 at 00:36 +0000, Eric Curtin wrote:
>> On 27 January 2016 at 23:26, Joe Perches  wrote:
>> > On Wed, 2016-01-27 at 22:14 +, Eric Curtin wrote:
>> > > Message gets logged on machines that are well supported.
> []
>> > diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>> > b/drivers/platform/x86/thinkpad_acpi.c
> []
>> > +   pr_info("detected %u brightness levels\n", bright_maxlvl + 1);
>> >  }
>> >
>> >  static int __init brightness_init(struct ibm_init_struct *iibm)
>>
>> Maybe, but the other logging issues kinda mean something. There are
>> many, many reports of people thinking their brightness interface is
>> broken because of this logging message, when in reality the i915 driver
>> supports their devices just fine as previously stated.
>
> That's why I suggest changing it to show the number
> of brightness levels detected on any device.
>

Sorry, I missed that line. Looks like a better fix! +1

Re: [PATCH] Remove ambiguous logging for "Unsupported brightness interface"

[Eric Curtin]

On 27 January 2016 at 23:26, Joe Perches  wrote:
> On Wed, 2016-01-27 at 22:14 +0000, Eric Curtin wrote:
>> Message gets logged on machines that are well supported.
>>
>> Signed-off-by: Eric Curtin 
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index a268a7a..4eb41aa 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -6661,7 +6661,6 @@ static void __init
>> tpacpi_detect_brightness_capabilities(void)
>> pr_info("detected a 8-level brightness capable
>> ThinkPad\n");
>> break;
>> default:
>> -   pr_info("Unsupported brightness interface\n");
>> tp_features.bright_unkfw = 1;
>> bright_maxlvl = b - 1;
>> }
>
> Perhaps this should be something like this instead:
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> index a268a7a..bd12c71 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6653,18 +6653,16 @@ static void __init 
> tpacpi_detect_brightness_capabilities(void)
> switch (b) {
> case 16:
> bright_maxlvl = 15;
> -   pr_info("detected a 16-level brightness capable ThinkPad\n");
> break;
> case 8:
> case 0:
> bright_maxlvl = 7;
> -   pr_info("detected a 8-level brightness capable ThinkPad\n");
> break;
> default:
> -   pr_info("Unsupported brightness interface\n");
> tp_features.bright_unkfw = 1;
> bright_maxlvl = b - 1;
> }
> +   pr_info("detected %u brightness levels\n", bright_maxlvl + 1);
>  }
>
>  static int __init brightness_init(struct ibm_init_struct *iibm)

Maybe, but the other logging issues kinda mean something. There are
many, many reports of people thinking their brightness interface is
broken because of this logging message, when in reality the i915 driver
supports their devices just fine as previously stated.

[PATCH] Remove ambiguous logging for "Unsupported brightness interface"

[Eric Curtin]

Message gets logged on machines that are well supported.

Signed-off-by: Eric Curtin 
---
 drivers/platform/x86/thinkpad_acpi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index a268a7a..4eb41aa 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6661,7 +6661,6 @@ static void __init
tpacpi_detect_brightness_capabilities(void)
pr_info("detected a 8-level brightness capable ThinkPad\n");
break;
default:
-   pr_info("Unsupported brightness interface\n");
tp_features.bright_unkfw = 1;
bright_maxlvl = b - 1;
}
-- 
2.5.0

Re: [PATCH] Remove ambiguous logging for "Unsupported brightness interface"

[Eric Curtin]

On 27 January 2016 at 23:26, Joe Perches <j...@perches.com> wrote:
> On Wed, 2016-01-27 at 22:14 +0000, Eric Curtin wrote:
>> Message gets logged on machines that are well supported.
>>
>> Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index a268a7a..4eb41aa 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -6661,7 +6661,6 @@ static void __init
>> tpacpi_detect_brightness_capabilities(void)
>> pr_info("detected a 8-level brightness capable
>> ThinkPad\n");
>> break;
>> default:
>> -   pr_info("Unsupported brightness interface\n");
>> tp_features.bright_unkfw = 1;
>> bright_maxlvl = b - 1;
>> }
>
> Perhaps this should be something like this instead:
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> index a268a7a..bd12c71 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6653,18 +6653,16 @@ static void __init 
> tpacpi_detect_brightness_capabilities(void)
> switch (b) {
> case 16:
> bright_maxlvl = 15;
> -   pr_info("detected a 16-level brightness capable ThinkPad\n");
> break;
> case 8:
> case 0:
> bright_maxlvl = 7;
> -   pr_info("detected a 8-level brightness capable ThinkPad\n");
> break;
> default:
> -   pr_info("Unsupported brightness interface\n");
> tp_features.bright_unkfw = 1;
> bright_maxlvl = b - 1;
> }
> +   pr_info("detected %u brightness levels\n", bright_maxlvl + 1);
>  }
>
>  static int __init brightness_init(struct ibm_init_struct *iibm)

Maybe, but the other logging issues kinda mean something. There are
many, many reports of people thinking their brightness interface is
broken because of this logging message, when in reality the i915 driver
supports their devices just fine as previously stated.

[PATCH] Remove ambiguous logging for "Unsupported brightness interface"

[Eric Curtin]

Message gets logged on machines that are well supported.

Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index a268a7a..4eb41aa 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6661,7 +6661,6 @@ static void __init
tpacpi_detect_brightness_capabilities(void)
pr_info("detected a 8-level brightness capable ThinkPad\n");
break;
default:
-   pr_info("Unsupported brightness interface\n");
tp_features.bright_unkfw = 1;
bright_maxlvl = b - 1;
}
-- 
2.5.0

Re: [PATCH] Remove ambiguous logging for "Unsupported brightness interface"

[Eric Curtin]

On 28 January 2016 at 00:43, Joe Perches <j...@perches.com> wrote:
> On Thu, 2016-01-28 at 00:36 +0000, Eric Curtin wrote:
>> On 27 January 2016 at 23:26, Joe Perches <j...@perches.com> wrote:
>> > On Wed, 2016-01-27 at 22:14 +, Eric Curtin wrote:
>> > > Message gets logged on machines that are well supported.
> []
>> > diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>> > b/drivers/platform/x86/thinkpad_acpi.c
> []
>> > +   pr_info("detected %u brightness levels\n", bright_maxlvl + 1);
>> >  }
>> >
>> >  static int __init brightness_init(struct ibm_init_struct *iibm)
>>
>> Maybe, but the other logging issues kinda mean something. There are
>> many, many reports of people thinking their brightness interface is
>> broken because of this logging message, when in reality the i915 driver
>> supports their devices just fine as previously stated.
>
> That's why I suggest changing it to show the number
> of brightness levels detected on any device.
>

Sorry, I missed that line. Looks like a better fix! +1

Re: [PATCH] Remove logging for "Unsupported brightness interface"

[Eric Curtin]

On 23 January 2016 at 00:28, Eric Curtin  wrote:
> Message gets logged on machines that are well supported.
>
> Signed-off-by: Eric Curtin 
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index a268a7a..4eb41aa 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6661,7 +6661,6 @@ static void __init
> tpacpi_detect_brightness_capabilities(void)
> pr_info("detected a 8-level brightness capable ThinkPad\n");
> break;
> default:
> -   pr_info("Unsupported brightness interface\n");
> tp_features.bright_unkfw = 1;
> bright_maxlvl = b - 1;
> }
> --
> 2.5.0
>

Hi Guys,

This patch doesn't appear on any of the Linux kernel archive sites.
Could somebody enlighten me why this is so?

Re: [PATCH] Remove logging for "Unsupported brightness interface"

[Eric Curtin]

On 23 January 2016 at 00:28, Eric Curtin <ericcurti...@gmail.com> wrote:
> Message gets logged on machines that are well supported.
>
> Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index a268a7a..4eb41aa 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6661,7 +6661,6 @@ static void __init
> tpacpi_detect_brightness_capabilities(void)
> pr_info("detected a 8-level brightness capable ThinkPad\n");
> break;
> default:
> -   pr_info("Unsupported brightness interface\n");
> tp_features.bright_unkfw = 1;
> bright_maxlvl = b - 1;
> }
> --
> 2.5.0
>

Hi Guys,

This patch doesn't appear on any of the Linux kernel archive sites.
Could somebody enlighten me why this is so?

Re: [PATCH] Remove logging for "Unsupported brightness interface"

[Eric Curtin]

On 16 January 2016 at 23:43, Eric Curtin  wrote:
> Message gets logged on machines that are well supported.
> Fixed one checkpatch.pl ERROR also.
>
> Signed-off-by: Eric Curtin 
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 0bed473..b149dec 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6434,7 +6434,7 @@ static const struct tpacpi_quirk
> brightness_quirk_table[] __initconst = {
>   */
>  static void __init tpacpi_detect_brightness_capabilities(void)
>  {
> - unsigned int b;
> + const unsigned int b = tpacpi_check_std_acpi_brightness_support();
>
>   vdbg_printk(TPACPI_DBG_INIT,
>  "detecting firmware brightness interface capabilities\n");
> @@ -6447,7 +6447,6 @@ static void __init
> tpacpi_detect_brightness_capabilities(void)
>   * Lenovo Vista BIOS to ACPI brightness mode even if we are not
>   * going to publish a backlight interface
>   */
> - b = tpacpi_check_std_acpi_brightness_support();
>   switch (b) {
>   case 16:
>   bright_maxlvl = 15;
> @@ -6459,7 +6458,6 @@ static void __init
> tpacpi_detect_brightness_capabilities(void)
>   pr_info("detected a 8-level brightness capable ThinkPad\n");
>   break;
>   default:
> - pr_info("Unsupported brightness interface\n");
>   tp_features.bright_unkfw = 1;
>   bright_maxlvl = b - 1;
>   }
> @@ -7440,7 +7438,7 @@ static struct ibm_struct volume_driver_data = {
>
>  #define alsa_card NULL
>
> -static void inline volume_alsa_notify_change(void)
> +static inline void volume_alsa_notify_change(void)
>  {
>  }
>
> --
> 2.5.0

Hi Guys,

Sorry to be bugging you. Is this going to be accepted or not? Just a poke! :)

Re: [PATCH] Remove logging for "Unsupported brightness interface"

[Eric Curtin]

On 16 January 2016 at 23:43, Eric Curtin <ericcurti...@gmail.com> wrote:
> Message gets logged on machines that are well supported.
> Fixed one checkpatch.pl ERROR also.
>
> Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 0bed473..b149dec 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6434,7 +6434,7 @@ static const struct tpacpi_quirk
> brightness_quirk_table[] __initconst = {
>   */
>  static void __init tpacpi_detect_brightness_capabilities(void)
>  {
> - unsigned int b;
> + const unsigned int b = tpacpi_check_std_acpi_brightness_support();
>
>   vdbg_printk(TPACPI_DBG_INIT,
>  "detecting firmware brightness interface capabilities\n");
> @@ -6447,7 +6447,6 @@ static void __init
> tpacpi_detect_brightness_capabilities(void)
>   * Lenovo Vista BIOS to ACPI brightness mode even if we are not
>   * going to publish a backlight interface
>   */
> - b = tpacpi_check_std_acpi_brightness_support();
>   switch (b) {
>   case 16:
>   bright_maxlvl = 15;
> @@ -6459,7 +6458,6 @@ static void __init
> tpacpi_detect_brightness_capabilities(void)
>   pr_info("detected a 8-level brightness capable ThinkPad\n");
>   break;
>   default:
> - pr_info("Unsupported brightness interface\n");
>   tp_features.bright_unkfw = 1;
>   bright_maxlvl = b - 1;
>   }
> @@ -7440,7 +7438,7 @@ static struct ibm_struct volume_driver_data = {
>
>  #define alsa_card NULL
>
> -static void inline volume_alsa_notify_change(void)
> +static inline void volume_alsa_notify_change(void)
>  {
>  }
>
> --
> 2.5.0

Hi Guys,

Sorry to be bugging you. Is this going to be accepted or not? Just a poke! :)

Re: [PATCH] thinkpad_acpi: don't yell on unsupported brightness interfaces

[Eric Curtin]

On 6 November 2015 at 17:57, Darren Hart  wrote:
>
> On Fri, Nov 06, 2015 at 03:19:43PM +0100, David Herrmann wrote:
> > Hi Darren!
> >
> > On Wed, Oct 21, 2015 at 4:44 PM, Darren Hart  wrote:
> > > On Wed, Oct 21, 2015 at 12:33:52PM -0200, Henrique de Moraes Holschuh 
> > > wrote:
> > >> On Wed, Oct 21, 2015, at 08:46, David Herrmann wrote:
> > >> > The thinkpad_acpi driver currently emits error messages on unsupported
> > >> > brightness interfaces, giving the impression that someone will 
> > >> > implement
> > >> > those. However, this error is spit out on nearly every thinkpad in
> > >> > production since 2 years now. Furthermore, the backlight interfaces on
> > >> > those devices are supported by the i915 driver just fine.
> > >> >
> > >> > Downgrade the error message to a normal pr_info() and stop telling 
> > >> > people
> > >> > to report it to IBM.
> > >>
> > >> IBM?  Those reports go directly to me.
> > >>
> > >> > Signed-off-by: David Herrmann 
> > >>
> > >> Acked-by: Henrique de Moraes Holschuh 
> > >
> > > Thanks, Queued.
> >
> > Any particular reason this didn't show up in:
> > [GIT PULL] platform-drivers-x86 for 4.4-1
> >
> > Just a kind reminder, in case it slipped through the cracks.
>
> Thank you, appears to be an error on my part, possibly due to some tooling
> changes on my end. Apologies, I'll get 4.4-2 out within the merge window and 
> be
> sure to include this.
>
> --
> Darren Hart
> Intel Open Source Technology Center
> --
> 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/

Hi Guys,

Actually could we just remove this log message altogether I don't think
it is very useful. The likelihood of this log indicating an error or
"Unsupported brightness interface" is very very low I imagine. This
appears on my Lenovo E540 (b variable in switch statement is 101 in my
case), I have been using Linux on this machine for well over a year and
the brightness interface is definitely very well supported. From
talking to this guy on a completely different thread it seems to happen
on his machine also:

https://github.com/mate-desktop/mate-power-manager/issues/179#issuecomment-169085313

Or else I can add a 101 switch case to the statement?

Also another query, could be a mini-side project for me as I'm a
beginner kernel dev when I have some spare time. Could we change the
minimum brightness value not to be off? 0 does not have to mean
backlight off from reading various things around the web:

http://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/backlight.pdf

And to be honest having the ability to completely turn off the
backlight is a useless function and is probably unintended as discussed
in the github thread above also. You would need night vision googles to
read the screen in this state! I'm pretty sure the Windows driver does
not completely turn off the screen on minimum brightness either, no
need to have different functionality on different platforms especially
when this functionality isn't very useful.

As suggested on the GitHub thread also, on other Lenovo machines (T60),
minimum brightness (0) does not mean off, so it would be nice to have
some consistency here.

Let me know what you think, I'd be happy to implement both these things
if you guys agree. I don't see much point in writing code if it won't
be accepted!
--
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] thinkpad_acpi: don't yell on unsupported brightness interfaces

[Eric Curtin]

On 6 November 2015 at 17:57, Darren Hart  wrote:
>
> On Fri, Nov 06, 2015 at 03:19:43PM +0100, David Herrmann wrote:
> > Hi Darren!
> >
> > On Wed, Oct 21, 2015 at 4:44 PM, Darren Hart  wrote:
> > > On Wed, Oct 21, 2015 at 12:33:52PM -0200, Henrique de Moraes Holschuh 
> > > wrote:
> > >> On Wed, Oct 21, 2015, at 08:46, David Herrmann wrote:
> > >> > The thinkpad_acpi driver currently emits error messages on unsupported
> > >> > brightness interfaces, giving the impression that someone will 
> > >> > implement
> > >> > those. However, this error is spit out on nearly every thinkpad in
> > >> > production since 2 years now. Furthermore, the backlight interfaces on
> > >> > those devices are supported by the i915 driver just fine.
> > >> >
> > >> > Downgrade the error message to a normal pr_info() and stop telling 
> > >> > people
> > >> > to report it to IBM.
> > >>
> > >> IBM?  Those reports go directly to me.
> > >>
> > >> > Signed-off-by: David Herrmann 
> > >>
> > >> Acked-by: Henrique de Moraes Holschuh 
> > >
> > > Thanks, Queued.
> >
> > Any particular reason this didn't show up in:
> > [GIT PULL] platform-drivers-x86 for 4.4-1
> >
> > Just a kind reminder, in case it slipped through the cracks.
>
> Thank you, appears to be an error on my part, possibly due to some tooling
> changes on my end. Apologies, I'll get 4.4-2 out within the merge window and 
> be
> sure to include this.
>
> --
> Darren Hart
> Intel Open Source Technology Center
> --
> 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/

Hi Guys,

Actually could we just remove this log message altogether I don't think
it is very useful. The likelihood of this log indicating an error or
"Unsupported brightness interface" is very very low I imagine. This
appears on my Lenovo E540 (b variable in switch statement is 101 in my
case), I have been using Linux on this machine for well over a year and
the brightness interface is definitely very well supported. From
talking to this guy on a completely different thread it seems to happen
on his machine also:

https://github.com/mate-desktop/mate-power-manager/issues/179#issuecomment-169085313

Or else I can add a 101 switch case to the statement?

Also another query, could be a mini-side project for me as I'm a
beginner kernel dev when I have some spare time. Could we change the
minimum brightness value not to be off? 0 does not have to mean
backlight off from reading various things around the web:

http://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/backlight.pdf

And to be honest having the ability to completely turn off the
backlight is a useless function and is probably unintended as discussed
in the github thread above also. You would need night vision googles to
read the screen in this state! I'm pretty sure the Windows driver does
not completely turn off the screen on minimum brightness either, no
need to have different functionality on different platforms especially
when this functionality isn't very useful.

As suggested on the GitHub thread also, on other Lenovo machines (T60),
minimum brightness (0) does not mean off, so it would be nice to have
some consistency here.

Let me know what you think, I'd be happy to implement both these things
if you guys agree. I don't see much point in writing code if it won't
be accepted!
--
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: Problems with printk logs and my driver

[Eric Curtin]

On 4 October 2015 at 16:28, Alan Stern  wrote:
> On Sun, 4 Oct 2015, Eric Curtin wrote:
>
>> Ok so for the fun of it, I changed the VENDOR_ID and DEVICE_ID of my
>> keyboard to use the driver for this samsung Wireless keyboard and
>> mouse, crazy I know since I have a different piece of hardware, but I
>> wanted to see what happens or at least does it change driver. When I
>> reboot to this kernel, my keyboard still uses the usbhid driver. Why
>> does this happen?
>
> Because the Samsung wireless keyboard and mouse also use the usbhid
> driver.  Besides, the choice of driver depends just as much on the
> bInterfaceClass, -Subclass, and -Protocol as on the vendor and product
> IDs.
>
> Basically, nothing needs to use usbkbd.  Anything that driver can
> handle will also work with usbhid.  The only reason for keeping usbkbd
> as a separate driver is because it is smaller than usbhid, so it's
> better suited to some embedded applications with limited memory.
>
> Alan Stern
>

After looking at this again today, I cannot reproduce the problem anymore.
I took out the battery on this keyboard and put it back in and the issue
is now not reproducible. Pity, I was hoping to get to the bottom of this
one.

It works in X but not in console mode. I tried another one and it had the
same result, it's a more standard usb keyboard.
--
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: Problems with printk logs and my driver

[Eric Curtin]

On 4 October 2015 at 16:28, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Sun, 4 Oct 2015, Eric Curtin wrote:
>
>> Ok so for the fun of it, I changed the VENDOR_ID and DEVICE_ID of my
>> keyboard to use the driver for this samsung Wireless keyboard and
>> mouse, crazy I know since I have a different piece of hardware, but I
>> wanted to see what happens or at least does it change driver. When I
>> reboot to this kernel, my keyboard still uses the usbhid driver. Why
>> does this happen?
>
> Because the Samsung wireless keyboard and mouse also use the usbhid
> driver.  Besides, the choice of driver depends just as much on the
> bInterfaceClass, -Subclass, and -Protocol as on the vendor and product
> IDs.
>
> Basically, nothing needs to use usbkbd.  Anything that driver can
> handle will also work with usbhid.  The only reason for keeping usbkbd
> as a separate driver is because it is smaller than usbhid, so it's
> better suited to some embedded applications with limited memory.
>
> Alan Stern
>

After looking at this again today, I cannot reproduce the problem anymore.
I took out the battery on this keyboard and put it back in and the issue
is now not reproducible. Pity, I was hoping to get to the bottom of this
one.

It works in X but not in console mode. I tried another one and it had the
same result, it's a more standard usb keyboard.
--
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] ext2: Add locking for DAX faults

[Eric Curtin]

On 13 October 2015 at 00:24, Dave Chinner  wrote:
>
> On Mon, Oct 12, 2015 at 03:41:35PM -0600, Ross Zwisler wrote:
> > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > that modify the data blocks allocation for an inode.  This is intended 
> > > > to
> > > > be analogous to the work being done in XFS by Dave Chinner:
> > > >
> > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > >
> > > > Compared with XFS the ext2 case is greatly simplified by the fact that 
> > > > ext2
> > > > already allocates and zeros new blocks before they are returned as part 
> > > > of
> > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > unwritten buffer heads.
> > > >
> > > > This means that the only work we need to do in ext2 is to isolate the 
> > > > DAX
> > > > faults from inode block allocation changes.  I believe this just means 
> > > > that
> > > > we need to isolate the DAX faults from truncate operations.
> > >
> > > Why limit this just to DAX page faults?
> >
> > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > faults.  I'll add this protection to non-DAX ext2 faults as well.
> >
> > One quick question - it looks like that dax_pmd_fault() only grabs the
> > pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
> > set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for 
> > read
> > faults as well.  Is this intentional?
>
> xfs_filemap_pfn_mkwrite() should not be called for read faults.
> We've already had to have a fault that maps the page to pfn for us
> to get a pfn based fault, and hence that code is correct.
>
> Or are you talking about xfs_filemap_pmd_fault()? In which case, I
> refer you to the commit log and it should be obvious that it was
> committed without me even looking at it.  I have another patch in my
> current series for 4.4 that will fix this.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> da...@fromorbit.com
> --
> 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/

Hi Ross,

For all those int ret declarations. Why not declare and initialize all
on the same line?

Regards,

Eric
--
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] ext2: Add locking for DAX faults

[Eric Curtin]

On 13 October 2015 at 00:24, Dave Chinner  wrote:
>
> On Mon, Oct 12, 2015 at 03:41:35PM -0600, Ross Zwisler wrote:
> > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > that modify the data blocks allocation for an inode.  This is intended 
> > > > to
> > > > be analogous to the work being done in XFS by Dave Chinner:
> > > >
> > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > >
> > > > Compared with XFS the ext2 case is greatly simplified by the fact that 
> > > > ext2
> > > > already allocates and zeros new blocks before they are returned as part 
> > > > of
> > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > unwritten buffer heads.
> > > >
> > > > This means that the only work we need to do in ext2 is to isolate the 
> > > > DAX
> > > > faults from inode block allocation changes.  I believe this just means 
> > > > that
> > > > we need to isolate the DAX faults from truncate operations.
> > >
> > > Why limit this just to DAX page faults?
> >
> > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > faults.  I'll add this protection to non-DAX ext2 faults as well.
> >
> > One quick question - it looks like that dax_pmd_fault() only grabs the
> > pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
> > set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for 
> > read
> > faults as well.  Is this intentional?
>
> xfs_filemap_pfn_mkwrite() should not be called for read faults.
> We've already had to have a fault that maps the page to pfn for us
> to get a pfn based fault, and hence that code is correct.
>
> Or are you talking about xfs_filemap_pmd_fault()? In which case, I
> refer you to the commit log and it should be obvious that it was
> committed without me even looking at it.  I have another patch in my
> current series for 4.4 that will fix this.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> da...@fromorbit.com
> --
> 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/

Hi Ross,

For all those int ret declarations. Why not declare and initialize all
on the same line?

Regards,

Eric
--
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: Problems with printk logs and my driver

[Eric Curtin]

On 30 September 2015 at 13:07, Austin S Hemmelgarn  wrote:
> On 2015-09-29 18:11, Eric Curtin wrote:
>>
>> On 25 September 2015 at 16:45, Austin S Hemmelgarn 
>> wrote:
>>>
>>> On 2015-09-25 08:02, Jiri Kosina wrote:
>>>>
>>>>
>>>> On Fri, 25 Sep 2015, Felipe Tonello wrote:
>>>>
>>>>> Maybe a better description on Kconfig and/or comments on source code
>>>>> it's enough.
>>>>
>>>>
>>>>
>>>> I personally find the current Kconfig description:
>>>>
>>>> ===
>>>> config USB_KBD
>>>>   tristate "USB HIDBP Keyboard (simple Boot) support"
>>>>   depends on USB && INPUT
>>>>   ---help---
>>>> Say Y here only if you are absolutely sure that you don't
>>>> want
>>>> to use the generic HID driver for your USB keyboard and
>>>> prefer
>>>> to use the keyboard in its limited Boot Protocol mode
>>>> instead.
>>>>
>>>> This is almost certainly not what you want.  This is mostly
>>>> useful for embedded applications or simple keyboards.
>>>>
>>>> To compile this driver as a module, choose M here: the
>>>> module will be called usbkbd.
>>>>
>>>> If even remotely unsure, say N.
>>>> ===
>>>>
>>>> shouldn't leave anyone dounting, but people are getting confused again
>>>> and
>>>> again nevertheless.
>>>>
>>> For some reason there seem to be a lot of people who go to configure
>>> there
>>> own kernel and don't read the help text (I understand if you've been
>>> building your own Linux kernel's for years and actually understand what a
>>> Kconfig option is really asking, but most people who I've heard of doing
>>> this have never built a kernel before in their life).
>>>
>>> On the other hand, can anyone think of any real reason to use this
>>> outside
>>> of embedded systems?  I know there are a lot of distros that build this
>>> and
>>> the USB HIDBP mouse support as modules, but I have yet to hear/find any
>>> reports of hardware that _only_ works with this driver and not the
>>> generic
>>> HID driver.  If this is the case, it might make sense to make this depend
>>> on
>>> EXPERT or at least remove the bit about 'simple keyboards'.
>>>
>>
>> As regards renaming usbkbd.c, @Austin there are some reasons why you would
>> not read the Kconfig. As a beginner, I didn't even configure this part or
>> read the help text as I used the configuration that comes with Fedora, I
>> don't know if that's a valid excuse or not though. I'll leave you guys
>> decide, you're the experts!
>>
>> As regards the issue with my capslock led I'm still looking into it.
>>
> Personally, I would not ever advocate not reading the help text for an
> option (although in some cases it's pretty un-helpful, especially for some
> staging drivers).
>
> Your case is one of the common ones, and it's not a bad place to start, but
> you have to keep in mind that most distro's turn on a huge amount of stuff
> that more than 90% of people aren't ever going to need (for example, I'm
> pretty sure Ubuntu still builds a module for SLIP, which has been an
> essentially dead technology for more than a decade now). For anyone starting
> from a distro's kconfig, I'd suggest at least:
> a. Turn off CONFIG_EXPERT unless you intend to actually try and understand
> the options it enables (most distro's turn this on for some of the fine
> tuning features it enables, most regular people don't actually need it).
> b. Go through using menuconfig, and turn off stuff under the drivers menu
> that you know you will never need (and take the time to use stuff like lspci
> and lsusb to figure out what actually need).
> c. Read the help text before trying to change anything, and if you don't
> understand it after that, look it up online, and even then be careful
> changing it.
> d. If you intend on actually using it with a particular distro, don't turn
> off too much outside of the drivers menu, other stuff can cause things to
> fail in unusual ways, and you often won't get a great amount of help from
> the distro maintainers when using a custom kernel.
>
> The real problem is when people just read the option name and think they
> understand it when they don't really (or just don't think abo

Re: Problems with printk logs and my driver

[Eric Curtin]

On 30 September 2015 at 13:07, Austin S Hemmelgarn <ahferro...@gmail.com> wrote:
> On 2015-09-29 18:11, Eric Curtin wrote:
>>
>> On 25 September 2015 at 16:45, Austin S Hemmelgarn <ahferro...@gmail.com>
>> wrote:
>>>
>>> On 2015-09-25 08:02, Jiri Kosina wrote:
>>>>
>>>>
>>>> On Fri, 25 Sep 2015, Felipe Tonello wrote:
>>>>
>>>>> Maybe a better description on Kconfig and/or comments on source code
>>>>> it's enough.
>>>>
>>>>
>>>>
>>>> I personally find the current Kconfig description:
>>>>
>>>> ===
>>>> config USB_KBD
>>>>   tristate "USB HIDBP Keyboard (simple Boot) support"
>>>>   depends on USB && INPUT
>>>>   ---help---
>>>> Say Y here only if you are absolutely sure that you don't
>>>> want
>>>> to use the generic HID driver for your USB keyboard and
>>>> prefer
>>>> to use the keyboard in its limited Boot Protocol mode
>>>> instead.
>>>>
>>>> This is almost certainly not what you want.  This is mostly
>>>> useful for embedded applications or simple keyboards.
>>>>
>>>> To compile this driver as a module, choose M here: the
>>>> module will be called usbkbd.
>>>>
>>>> If even remotely unsure, say N.
>>>> ===
>>>>
>>>> shouldn't leave anyone dounting, but people are getting confused again
>>>> and
>>>> again nevertheless.
>>>>
>>> For some reason there seem to be a lot of people who go to configure
>>> there
>>> own kernel and don't read the help text (I understand if you've been
>>> building your own Linux kernel's for years and actually understand what a
>>> Kconfig option is really asking, but most people who I've heard of doing
>>> this have never built a kernel before in their life).
>>>
>>> On the other hand, can anyone think of any real reason to use this
>>> outside
>>> of embedded systems?  I know there are a lot of distros that build this
>>> and
>>> the USB HIDBP mouse support as modules, but I have yet to hear/find any
>>> reports of hardware that _only_ works with this driver and not the
>>> generic
>>> HID driver.  If this is the case, it might make sense to make this depend
>>> on
>>> EXPERT or at least remove the bit about 'simple keyboards'.
>>>
>>
>> As regards renaming usbkbd.c, @Austin there are some reasons why you would
>> not read the Kconfig. As a beginner, I didn't even configure this part or
>> read the help text as I used the configuration that comes with Fedora, I
>> don't know if that's a valid excuse or not though. I'll leave you guys
>> decide, you're the experts!
>>
>> As regards the issue with my capslock led I'm still looking into it.
>>
> Personally, I would not ever advocate not reading the help text for an
> option (although in some cases it's pretty un-helpful, especially for some
> staging drivers).
>
> Your case is one of the common ones, and it's not a bad place to start, but
> you have to keep in mind that most distro's turn on a huge amount of stuff
> that more than 90% of people aren't ever going to need (for example, I'm
> pretty sure Ubuntu still builds a module for SLIP, which has been an
> essentially dead technology for more than a decade now). For anyone starting
> from a distro's kconfig, I'd suggest at least:
> a. Turn off CONFIG_EXPERT unless you intend to actually try and understand
> the options it enables (most distro's turn this on for some of the fine
> tuning features it enables, most regular people don't actually need it).
> b. Go through using menuconfig, and turn off stuff under the drivers menu
> that you know you will never need (and take the time to use stuff like lspci
> and lsusb to figure out what actually need).
> c. Read the help text before trying to change anything, and if you don't
> understand it after that, look it up online, and even then be careful
> changing it.
> d. If you intend on actually using it with a particular distro, don't turn
> off too much outside of the drivers menu, other stuff can cause things to
> fail in unusual ways, and you often won't get a great amount of help from
> the distro maintainers when using a custom kernel.
>
> The real problem is when people just read the option name and think the

Re: Problems with printk logs and my driver

[Eric Curtin]

On 25 September 2015 at 16:45, Austin S Hemmelgarn  wrote:
> On 2015-09-25 08:02, Jiri Kosina wrote:
>>
>> On Fri, 25 Sep 2015, Felipe Tonello wrote:
>>
>>> Maybe a better description on Kconfig and/or comments on source code
>>> it's enough.
>>
>>
>> I personally find the current Kconfig description:
>>
>> ===
>> config USB_KBD
>>  tristate "USB HIDBP Keyboard (simple Boot) support"
>>  depends on USB && INPUT
>>  ---help---
>>Say Y here only if you are absolutely sure that you don't want
>>to use the generic HID driver for your USB keyboard and prefer
>>to use the keyboard in its limited Boot Protocol mode instead.
>>
>>This is almost certainly not what you want.  This is mostly
>>useful for embedded applications or simple keyboards.
>>
>>To compile this driver as a module, choose M here: the
>>module will be called usbkbd.
>>
>>If even remotely unsure, say N.
>> ===
>>
>> shouldn't leave anyone dounting, but people are getting confused again and
>> again nevertheless.
>>
> For some reason there seem to be a lot of people who go to configure there
> own kernel and don't read the help text (I understand if you've been
> building your own Linux kernel's for years and actually understand what a
> Kconfig option is really asking, but most people who I've heard of doing
> this have never built a kernel before in their life).
>
> On the other hand, can anyone think of any real reason to use this outside
> of embedded systems?  I know there are a lot of distros that build this and
> the USB HIDBP mouse support as modules, but I have yet to hear/find any
> reports of hardware that _only_ works with this driver and not the generic
> HID driver.  If this is the case, it might make sense to make this depend on
> EXPERT or at least remove the bit about 'simple keyboards'.
>

As regards renaming usbkbd.c, @Austin there are some reasons why you would
not read the Kconfig. As a beginner, I didn't even configure this part or
read the help text as I used the configuration that comes with Fedora, I
don't know if that's a valid excuse or not though. I'll leave you guys
decide, you're the experts!

As regards the issue with my capslock led I'm still looking into it.
--
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: First kernel patch (optimization)

[Eric Curtin]

On 29 September 2015 at 14:51, Austin S Hemmelgarn  wrote:
> On 2015-09-26 09:28, Eric Curtin wrote:
>>
>> Hi Dimitry,
>>
>>> Is it Debian-derivative by any chance? Their capslock setup is wonky
>>> because CapsLock key does no actually set up as a CapsLock but another
>>> modifier. Also is it in X or is it on text console? Because X handles
>>> led state on its own...
>>
>>
>> I'm on Fedora 22. Yeah, you're correct X is handling this, the led does
>> not turn on at all in text console. Tried Wayland also for the first time
>> to see if it occurred there also, it does. Does this mean I should not be
>> looking at kernel code all to fix this issue and I should be looking at
>> X/Wayland?
>
> If the led doesn't turn on on the console either, then it may be worth
> looking at kernel code, but it may be in the console driver instead of the
> input layer driver.
>

I may have been incomplete earlier. The led does not turn on at all in
console. In X the led does turn on, but does not switch off correctly.
I'm looking at kernel code, still assuming I can fix it in here.
--
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: First kernel patch (optimization)

[Eric Curtin]

On 29 September 2015 at 14:51, Austin S Hemmelgarn <ahferro...@gmail.com> wrote:
> On 2015-09-26 09:28, Eric Curtin wrote:
>>
>> Hi Dimitry,
>>
>>> Is it Debian-derivative by any chance? Their capslock setup is wonky
>>> because CapsLock key does no actually set up as a CapsLock but another
>>> modifier. Also is it in X or is it on text console? Because X handles
>>> led state on its own...
>>
>>
>> I'm on Fedora 22. Yeah, you're correct X is handling this, the led does
>> not turn on at all in text console. Tried Wayland also for the first time
>> to see if it occurred there also, it does. Does this mean I should not be
>> looking at kernel code all to fix this issue and I should be looking at
>> X/Wayland?
>
> If the led doesn't turn on on the console either, then it may be worth
> looking at kernel code, but it may be in the console driver instead of the
> input layer driver.
>

I may have been incomplete earlier. The led does not turn on at all in
console. In X the led does turn on, but does not switch off correctly.
I'm looking at kernel code, still assuming I can fix it in here.
--
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: Problems with printk logs and my driver

[Eric Curtin]

On 25 September 2015 at 16:45, Austin S Hemmelgarn  wrote:
> On 2015-09-25 08:02, Jiri Kosina wrote:
>>
>> On Fri, 25 Sep 2015, Felipe Tonello wrote:
>>
>>> Maybe a better description on Kconfig and/or comments on source code
>>> it's enough.
>>
>>
>> I personally find the current Kconfig description:
>>
>> ===
>> config USB_KBD
>>  tristate "USB HIDBP Keyboard (simple Boot) support"
>>  depends on USB && INPUT
>>  ---help---
>>Say Y here only if you are absolutely sure that you don't want
>>to use the generic HID driver for your USB keyboard and prefer
>>to use the keyboard in its limited Boot Protocol mode instead.
>>
>>This is almost certainly not what you want.  This is mostly
>>useful for embedded applications or simple keyboards.
>>
>>To compile this driver as a module, choose M here: the
>>module will be called usbkbd.
>>
>>If even remotely unsure, say N.
>> ===
>>
>> shouldn't leave anyone dounting, but people are getting confused again and
>> again nevertheless.
>>
> For some reason there seem to be a lot of people who go to configure there
> own kernel and don't read the help text (I understand if you've been
> building your own Linux kernel's for years and actually understand what a
> Kconfig option is really asking, but most people who I've heard of doing
> this have never built a kernel before in their life).
>
> On the other hand, can anyone think of any real reason to use this outside
> of embedded systems?  I know there are a lot of distros that build this and
> the USB HIDBP mouse support as modules, but I have yet to hear/find any
> reports of hardware that _only_ works with this driver and not the generic
> HID driver.  If this is the case, it might make sense to make this depend on
> EXPERT or at least remove the bit about 'simple keyboards'.
>

As regards renaming usbkbd.c, @Austin there are some reasons why you would
not read the Kconfig. As a beginner, I didn't even configure this part or
read the help text as I used the configuration that comes with Fedora, I
don't know if that's a valid excuse or not though. I'll leave you guys
decide, you're the experts!

As regards the issue with my capslock led I'm still looking into it.
--
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: First kernel patch (optimization)

[Eric Curtin]

Hi Dimitry,

> Is it Debian-derivative by any chance? Their capslock setup is wonky
> because CapsLock key does no actually set up as a CapsLock but another
> modifier. Also is it in X or is it on text console? Because X handles
> led state on its own...

I'm on Fedora 22. Yeah, you're correct X is handling this, the led does
not turn on at all in text console. Tried Wayland also for the first time
to see if it occurred there also, it does. Does this mean I should not be
looking at kernel code all to fix this issue and I should be looking at
X/Wayland?

>> I think it is something to do with the LED_CAPSL variable
>> in here:
>>
>> drivers/hid/usbhid/usbkbd.c
>
> I do not think you are using usbkbd driver - it is for keyboards in
> "boot protocol" and barely anyone users them in such mode. You need to
> look into drivers/hid/hid-input.c.

Yeah, I realize this now (see "Problems with printk logs and my driver"
email thread), they are talking about renaming this file now as I am not
the first one to make this mistake.
--
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: First kernel patch (optimization)

[Eric Curtin]

Hi Dimitry,

> Is it Debian-derivative by any chance? Their capslock setup is wonky
> because CapsLock key does no actually set up as a CapsLock but another
> modifier. Also is it in X or is it on text console? Because X handles
> led state on its own...

I'm on Fedora 22. Yeah, you're correct X is handling this, the led does
not turn on at all in text console. Tried Wayland also for the first time
to see if it occurred there also, it does. Does this mean I should not be
looking at kernel code all to fix this issue and I should be looking at
X/Wayland?

>> I think it is something to do with the LED_CAPSL variable
>> in here:
>>
>> drivers/hid/usbhid/usbkbd.c
>
> I do not think you are using usbkbd driver - it is for keyboards in
> "boot protocol" and barely anyone users them in such mode. You need to
> look into drivers/hid/hid-input.c.

Yeah, I realize this now (see "Problems with printk logs and my driver"
email thread), they are talking about renaming this file now as I am not
the first one to make this mistake.
--
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/

Problems with printk logs and my driver

[Eric Curtin]

Hi Guys,

Just wondering what I am doing wrong. I can't see my logs. I figured
out what driver is used for my keyboard and started adding logging:

[curtine@localhost ~]$ sudo lsusb -v | grep eyboard -B 13
Bus 001 Device 003: ID 04ca:008d Lite-On Technology Corp.
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass0 (Defined at Interface level)
  bDeviceSubClass 0
  bDeviceProtocol 0
  bMaxPacketSize032
  idVendor   0x04ca Lite-On Technology Corp.
  idProduct  0x008d
  bcdDevice0.39
  iManufacturer   1 Lite-On Technology Corp.
  iProduct2 HP Wireless Keyboard Kit
--
iConfiguration  0
bmAttributes 0xa0
  (Bus Powered)
  Remote Wakeup
MaxPower  100mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   1
  bInterfaceClass 3 Human Interface Device
  bInterfaceSubClass  1 Boot Interface Subclass
  bInterfaceProtocol  1 Keyboard
[curtine@localhost ~]$ sudo lsusb -t
/:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/2p, 480M
|__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/6p, 480M
/:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/2p, 480M
|__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/10p, 480M
|__ Port 4: Dev 2, If 0, Class=Wireless, Driver=btusb, 12M
|__ Port 4: Dev 2, If 1, Class=Wireless, Driver=btusb, 12M
|__ Port 7: Dev 3, If 0, Class=Human Interface Device, Driver=usbhid, 12M
|__ Port 7: Dev 3, If 1, Class=Human Interface Device, Driver=usbhid, 12M
|__ Port 7: Dev 3, If 2, Class=Human Interface Device, Driver=usbhid, 12M
|__ Port 9: Dev 4, If 0, Class=Video, Driver=uvcvideo, 480M
|__ Port 9: Dev 4, If 1, Class=Video, Driver=uvcvideo, 480M

So, first I added a little logging and then some more, but I can't see
any of it (see patch at bottom of email, I used KERN_EMERG, it's just
temporary logging).

I'm think I'm doing most things right, this is how I compile my code I
wrote a little script (I'm on fedora):

#!/bin/sh

find /boot -name '*4.3*' | xargs sudo rm -rf
find /lib/modules -name '*4.3*' | xargs sudo rm -rf

set -e

cd $HOME/git/linux
# make -j5 oldconfig
printf "completed 1\n"
make -j5 bzImage
printf "completed 2\n"
make -j5 modules
printf "completed 3\n"
sudo make -j5 modules_install
printf "completed 4\n"
sudo make -j5 install

I reboot, load new kernel, blah blah. When I type keys I don't see my
logs in dmesg, I don't see them in /var/log/messages either, I don't
see them in /home/curtine/log.log either when I do a:

sudo killall klogd
sudo /sbin/klogd -f /home/curtine/log.log

What am I doing wrong here?

Also, as regards etiquette on these mailing lists, is it ok to
regularly cc linux-kernel@vger.kernel.org?

diff --git a/drivers/hid/usbhid/usbkbd.c b/drivers/hid/usbhid/usbkbd.c
index 9a332e6..2038d94 100644
--- a/drivers/hid/usbhid/usbkbd.c
+++ b/drivers/hid/usbhid/usbkbd.c
@@ -112,6 +112,7 @@ struct usb_kbd {

 static void usb_kbd_irq(struct urb *urb)
 {
+   printk(KERN_EMERG "usb_kbd_irq");
struct usb_kbd *kbd = urb->context;
int i;

@@ -166,6 +167,7 @@ resubmit:
 static int usb_kbd_event(struct input_dev *dev, unsigned int type,
 unsigned int code, int value)
 {
+   printk(KERN_EMERG "usb_kbd_event");
unsigned long flags;
struct usb_kbd *kbd = input_get_drvdata(dev);

@@ -202,6 +204,7 @@ static int usb_kbd_event(struct input_dev *dev,
unsigned int type,

 static void usb_kbd_led(struct urb *urb)
 {
+   printk(KERN_EMERG "usb_kbd_led");
unsigned long flags;
struct usb_kbd *kbd = urb->context;

@@ -230,6 +233,7 @@ static void usb_kbd_led(struct urb *urb)

 static int usb_kbd_open(struct input_dev *dev)
 {
+   printk(KERN_EMERG "usb_kbd_open");
struct usb_kbd *kbd = input_get_drvdata(dev);

kbd->irq->dev = kbd->usbdev;
@@ -241,6 +245,7 @@ static int usb_kbd_open(struct input_dev *dev)

 static void usb_kbd_close(struct input_dev *dev)
 {
+   printk(KERN_EMERG "usb_kbd_close");
struct usb_kbd *kbd = input_get_drvdata(dev);

usb_kill_urb(kbd->irq);
@@ -248,6 +253,7 @@ static void usb_kbd_close(struct input_dev *dev)

 static int usb_kbd_alloc_mem(struct usb_device *dev, struct usb_kbd *kbd)
 {
+   printk(KERN_EMERG "usb_kbd_alloc_mem");
if (!(kbd->irq = usb_alloc_urb(0, GFP_KERNEL)))
return -1;
if (!(kbd->led = usb_alloc_urb(0, GFP_KERNEL)))
@@ -264,6 +270,7 @@ static int usb_kbd_alloc_mem(struct usb_device
*dev, struct usb_kbd *kbd)

 static void usb_kbd_free_mem(struct usb_device *dev, struct usb_kbd *kbd)
 {
+   printk(KERN_EMERG "usb_kbd_free_mem");
usb_free_urb(kbd->irq);

Problems with printk logs and my driver

[Eric Curtin]

Hi Guys,

Just wondering what I am doing wrong. I can't see my logs. I figured
out what driver is used for my keyboard and started adding logging:

[curtine@localhost ~]$ sudo lsusb -v | grep eyboard -B 13
Bus 001 Device 003: ID 04ca:008d Lite-On Technology Corp.
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass0 (Defined at Interface level)
  bDeviceSubClass 0
  bDeviceProtocol 0
  bMaxPacketSize032
  idVendor   0x04ca Lite-On Technology Corp.
  idProduct  0x008d
  bcdDevice0.39
  iManufacturer   1 Lite-On Technology Corp.
  iProduct2 HP Wireless Keyboard Kit
--
iConfiguration  0
bmAttributes 0xa0
  (Bus Powered)
  Remote Wakeup
MaxPower  100mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   1
  bInterfaceClass 3 Human Interface Device
  bInterfaceSubClass  1 Boot Interface Subclass
  bInterfaceProtocol  1 Keyboard
[curtine@localhost ~]$ sudo lsusb -t
/:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/2p, 480M
|__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/6p, 480M
/:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/2p, 480M
|__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/10p, 480M
|__ Port 4: Dev 2, If 0, Class=Wireless, Driver=btusb, 12M
|__ Port 4: Dev 2, If 1, Class=Wireless, Driver=btusb, 12M
|__ Port 7: Dev 3, If 0, Class=Human Interface Device, Driver=usbhid, 12M
|__ Port 7: Dev 3, If 1, Class=Human Interface Device, Driver=usbhid, 12M
|__ Port 7: Dev 3, If 2, Class=Human Interface Device, Driver=usbhid, 12M
|__ Port 9: Dev 4, If 0, Class=Video, Driver=uvcvideo, 480M
|__ Port 9: Dev 4, If 1, Class=Video, Driver=uvcvideo, 480M

So, first I added a little logging and then some more, but I can't see
any of it (see patch at bottom of email, I used KERN_EMERG, it's just
temporary logging).

I'm think I'm doing most things right, this is how I compile my code I
wrote a little script (I'm on fedora):

#!/bin/sh

find /boot -name '*4.3*' | xargs sudo rm -rf
find /lib/modules -name '*4.3*' | xargs sudo rm -rf

set -e

cd $HOME/git/linux
# make -j5 oldconfig
printf "completed 1\n"
make -j5 bzImage
printf "completed 2\n"
make -j5 modules
printf "completed 3\n"
sudo make -j5 modules_install
printf "completed 4\n"
sudo make -j5 install

I reboot, load new kernel, blah blah. When I type keys I don't see my
logs in dmesg, I don't see them in /var/log/messages either, I don't
see them in /home/curtine/log.log either when I do a:

sudo killall klogd
sudo /sbin/klogd -f /home/curtine/log.log

What am I doing wrong here?

Also, as regards etiquette on these mailing lists, is it ok to
regularly cc linux-kernel@vger.kernel.org?

diff --git a/drivers/hid/usbhid/usbkbd.c b/drivers/hid/usbhid/usbkbd.c
index 9a332e6..2038d94 100644
--- a/drivers/hid/usbhid/usbkbd.c
+++ b/drivers/hid/usbhid/usbkbd.c
@@ -112,6 +112,7 @@ struct usb_kbd {

 static void usb_kbd_irq(struct urb *urb)
 {
+   printk(KERN_EMERG "usb_kbd_irq");
struct usb_kbd *kbd = urb->context;
int i;

@@ -166,6 +167,7 @@ resubmit:
 static int usb_kbd_event(struct input_dev *dev, unsigned int type,
 unsigned int code, int value)
 {
+   printk(KERN_EMERG "usb_kbd_event");
unsigned long flags;
struct usb_kbd *kbd = input_get_drvdata(dev);

@@ -202,6 +204,7 @@ static int usb_kbd_event(struct input_dev *dev,
unsigned int type,

 static void usb_kbd_led(struct urb *urb)
 {
+   printk(KERN_EMERG "usb_kbd_led");
unsigned long flags;
struct usb_kbd *kbd = urb->context;

@@ -230,6 +233,7 @@ static void usb_kbd_led(struct urb *urb)

 static int usb_kbd_open(struct input_dev *dev)
 {
+   printk(KERN_EMERG "usb_kbd_open");
struct usb_kbd *kbd = input_get_drvdata(dev);

kbd->irq->dev = kbd->usbdev;
@@ -241,6 +245,7 @@ static int usb_kbd_open(struct input_dev *dev)

 static void usb_kbd_close(struct input_dev *dev)
 {
+   printk(KERN_EMERG "usb_kbd_close");
struct usb_kbd *kbd = input_get_drvdata(dev);

usb_kill_urb(kbd->irq);
@@ -248,6 +253,7 @@ static void usb_kbd_close(struct input_dev *dev)

 static int usb_kbd_alloc_mem(struct usb_device *dev, struct usb_kbd *kbd)
 {
+   printk(KERN_EMERG "usb_kbd_alloc_mem");
if (!(kbd->irq = usb_alloc_urb(0, GFP_KERNEL)))
return -1;
if (!(kbd->led = usb_alloc_urb(0, GFP_KERNEL)))
@@ -264,6 +270,7 @@ static int usb_kbd_alloc_mem(struct usb_device
*dev, struct usb_kbd *kbd)

 static void usb_kbd_free_mem(struct usb_device *dev, struct usb_kbd *kbd)
 {
+   printk(KERN_EMERG "usb_kbd_free_mem");
usb_free_urb(kbd->irq);

Re: First kernel patch (optimization)

[Eric Curtin]

On 22 September 2015 at 18:38, Linus Torvalds
 wrote:
> On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin  wrote:
>> My first kernel patch, hope I did everything correctly! Instead of calling 
>> strlen on every iteration of the for loop, just call it once instead and 
>> store in a variable.
>
> Heh. Ok, that resulted in a rather long email thread.
>
> Anyway, I'd actually prefer to merge this patch, for two reasons:
>
>  - the "termination calculation is expensive" problem is a real
> problem, and while in this case the compiler may be able to notice
> that the "strlen()" is constant over the loop and can be hoisted up,
> that is not at all necessarily the case most of the time.
>
> So I actually think patches like this are good things. Not because
> this particular code site necessarily matters, but because people who
> write code with things like "strlen()" in the terminating condition
> need to learn that it's *wrong*.
>
>  - I'd much rather see this kind of trivial patch than the usual
> trivial patch that is clearly just "let's run some script on the
> kernel and fix up warnings it generates mindlessly".
>
>In contrast to that, *this* trivial patch was about somebody who
> thought about code generation and efficiency. And *that* is the kind
> of trivial patches we want to encourage, not necessariyl because this
> particular case was so important, but because that's the kind of
> people and thinking we want to encourage.
>
> So quite frankly, I'd just take this directly, but I'd like more of
> real changelog.
>
> But I wonder if Eric is even reading the emails any more ;)
>
>  Linus

Hi Linus & Everyone else,

I can deal with the patronizing remarks made at the start of the
thread, I've been called worse names! But I would encourage people to
avoid this behavior especially with newbies, as many may leave the
community and never attempt to submit a patch again. It does not leave
a great first impression.

We so not come out the womb expert kernel developers, you gotta learn
how to crawl, before you can walk.

Yeah, I'm still reading this email thread and learned lots from it.
I'm working on something more meaningful, but it's not going to be
ground breaking of course, there is a led on my capslock key on a new
machine I won at work that does not switch off properly after it is
switched on. I think it is something to do with the LED_CAPSL variable
in here:

drivers/hid/usbhid/usbkbd.c

I'll figure it out, and get it fixed. Decided, I'd do something more
meaty. Then I'll start looking at the drivers/staging stuff. I can
only look at this stuff for a few hours every second day, with my
normal 9 to 5 job being first priority.

I can clean up this patch re-submit no problem, in fact I already did
and got more feedback, it's subject is "tools: usbip: detach: avoid
calling strlen() at each iteration". But I moved on to the keyboard
driver because of the feedback I received about wasting peoples time!

Regards,

Eric
--
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: First kernel patch (optimization)

[Eric Curtin]

On 22 September 2015 at 18:38, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurti...@gmail.com> wrote:
>> My first kernel patch, hope I did everything correctly! Instead of calling 
>> strlen on every iteration of the for loop, just call it once instead and 
>> store in a variable.
>
> Heh. Ok, that resulted in a rather long email thread.
>
> Anyway, I'd actually prefer to merge this patch, for two reasons:
>
>  - the "termination calculation is expensive" problem is a real
> problem, and while in this case the compiler may be able to notice
> that the "strlen()" is constant over the loop and can be hoisted up,
> that is not at all necessarily the case most of the time.
>
> So I actually think patches like this are good things. Not because
> this particular code site necessarily matters, but because people who
> write code with things like "strlen()" in the terminating condition
> need to learn that it's *wrong*.
>
>  - I'd much rather see this kind of trivial patch than the usual
> trivial patch that is clearly just "let's run some script on the
> kernel and fix up warnings it generates mindlessly".
>
>In contrast to that, *this* trivial patch was about somebody who
> thought about code generation and efficiency. And *that* is the kind
> of trivial patches we want to encourage, not necessariyl because this
> particular case was so important, but because that's the kind of
> people and thinking we want to encourage.
>
> So quite frankly, I'd just take this directly, but I'd like more of
> real changelog.
>
> But I wonder if Eric is even reading the emails any more ;)
>
>  Linus

Hi Linus & Everyone else,

I can deal with the patronizing remarks made at the start of the
thread, I've been called worse names! But I would encourage people to
avoid this behavior especially with newbies, as many may leave the
community and never attempt to submit a patch again. It does not leave
a great first impression.

We so not come out the womb expert kernel developers, you gotta learn
how to crawl, before you can walk.

Yeah, I'm still reading this email thread and learned lots from it.
I'm working on something more meaningful, but it's not going to be
ground breaking of course, there is a led on my capslock key on a new
machine I won at work that does not switch off properly after it is
switched on. I think it is something to do with the LED_CAPSL variable
in here:

drivers/hid/usbhid/usbkbd.c

I'll figure it out, and get it fixed. Decided, I'd do something more
meaty. Then I'll start looking at the drivers/staging stuff. I can
only look at this stuff for a few hours every second day, with my
normal 9 to 5 job being first priority.

I can clean up this patch re-submit no problem, in fact I already did
and got more feedback, it's subject is "tools: usbip: detach: avoid
calling strlen() at each iteration". But I moved on to the keyboard
driver because of the feedback I received about wasting peoples time!

Regards,

Eric
--
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: First kernel patch (optimization)

[Eric Curtin]

On 16 September 2015 at 21:02, Greg KH  wrote:
> On Wed, Sep 16, 2015 at 05:03:39PM +0100, Eric Curtin wrote:
>> Hi Greg,
>>
>> As I said in the subject of the mail (which I have been since told I
>> shouldn't have done this), I'm a noob to kernel code. I tried to pick
>> off something super simple to just see what the process of getting a
>> patch in is. Youtube videos and documentation only get you so far.
>>
>> >From reading your response, should I refrain from sending in these
>> micro-optimizations in future? Getting in smaller patches is easier
>> for me as I only do this in my spare time, which I don't have a lot
>> of!
>
> micro-optimizations are great, if you can actually measure them and they
> matter :)
>
> If you are looking for someplace to start, might I recommend the
> drivers/staging/ directory?  I take all sorts of "basic" cleanup
> patches, and there are loads of things to do in there.  Look at the
> drivers/staging/*/TODO files for specific examples of what needs to be
> done to get these files cleaned up and fixed properly.
>
> Hope this helps,
>
> greg k-h

I have realized this now after reading
http://kernelnewbies.org/FirstKernelPatch, should have done a bit more
reading before I submitted something. I'm looking at the
drivers/staging/android stuff now.

Eric Curtin
--
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: First kernel patch (optimization)

[Eric Curtin]

Hi Greg,

As I said in the subject of the mail (which I have been since told I
shouldn't have done this), I'm a noob to kernel code. I tried to pick
off something super simple to just see what the process of getting a
patch in is. Youtube videos and documentation only get you so far.

>From reading your response, should I refrain from sending in these
micro-optimizations in future? Getting in smaller patches is easier
for me as I only do this in my spare time, which I don't have a lot
of!

On 16 September 2015 at 14:24, Greg KH  wrote:
>
> On Wed, Sep 16, 2015 at 07:45:53AM -0400, Austin S Hemmelgarn wrote:
> > On 2015-09-15 20:09, Steve Calfee wrote:
> > >On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin  
> > >wrote:
> > >>Signed-off-by: Eric Curtin 
> > >>
> > >>diff --git a/tools/usb/usbip/src/usbip_detach.c 
> > >>b/tools/usb/usbip/src/usbip_detach.c
> > >>index 05c6d15..9db9d21 100644
> > >>--- a/tools/usb/usbip/src/usbip_detach.c
> > >>+++ b/tools/usb/usbip/src/usbip_detach.c
> > >>@@ -47,7 +47,9 @@ static int detach_port(char *port)
> > >> uint8_t portnum;
> > >> char path[PATH_MAX+1];
> > >>
> > >>-   for (unsigned int i = 0; i < strlen(port); i++)
> > >>+   unsigned int port_len = strlen(port);
> > >>+
> > >>+   for (unsigned int i = 0; i < port_len; i++)
> > >> if (!isdigit(port[i])) {
> > >> err("invalid port %s", port);
> > >> return -1;
> > >>
> > >>--
> > >
> > >Hi Eric,
> > >
> > >This is fine, but what kind of wimpy compiler optimizer will not move
> > >the constant initializer out of the loop? I bet if you compare binary
> > >sizes/code it will be exactly the same, and you added some characters
> > >of code. Reorganizing code for readability is fine, but for compiler
> > >(in)efficiency seems like a bad idea.
> > While I agree with your argument, I would like to point out that it is a
> > well established fact that GCC's optimizers are kind of brain-dead at times
> > and need their hands held.
> >
> > I'd be willing to bet that the code will be marginally larger (because of
> > adding another variable), but might run slightly faster too (because in my
> > experience, GCC doesn't always catch things like this), and should compile a
> > little faster (because the optimizers don't have to do as much work).
>
> Fun thing is, there's no speed issues with this portion of the code, and
> it's in userspace as well, so simplicity is the key that should be
> followed here.
>
> greg k-h
--
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: First kernel patch (optimization)

[Eric Curtin]

Hi Greg,

As I said in the subject of the mail (which I have been since told I
shouldn't have done this), I'm a noob to kernel code. I tried to pick
off something super simple to just see what the process of getting a
patch in is. Youtube videos and documentation only get you so far.

>From reading your response, should I refrain from sending in these
micro-optimizations in future? Getting in smaller patches is easier
for me as I only do this in my spare time, which I don't have a lot
of!

On 16 September 2015 at 14:24, Greg KH <gre...@linuxfoundation.org> wrote:
>
> On Wed, Sep 16, 2015 at 07:45:53AM -0400, Austin S Hemmelgarn wrote:
> > On 2015-09-15 20:09, Steve Calfee wrote:
> > >On Tue, Sep 15, 2015 at 12:53 PM, Eric Curtin <ericcurti...@gmail.com> 
> > >wrote:
> > >>Signed-off-by: Eric Curtin <ericcurti...@gmail.com>
> > >>
> > >>diff --git a/tools/usb/usbip/src/usbip_detach.c 
> > >>b/tools/usb/usbip/src/usbip_detach.c
> > >>index 05c6d15..9db9d21 100644
> > >>--- a/tools/usb/usbip/src/usbip_detach.c
> > >>+++ b/tools/usb/usbip/src/usbip_detach.c
> > >>@@ -47,7 +47,9 @@ static int detach_port(char *port)
> > >> uint8_t portnum;
> > >> char path[PATH_MAX+1];
> > >>
> > >>-   for (unsigned int i = 0; i < strlen(port); i++)
> > >>+   unsigned int port_len = strlen(port);
> > >>+
> > >>+   for (unsigned int i = 0; i < port_len; i++)
> > >> if (!isdigit(port[i])) {
> > >> err("invalid port %s", port);
> > >> return -1;
> > >>
> > >>--
> > >
> > >Hi Eric,
> > >
> > >This is fine, but what kind of wimpy compiler optimizer will not move
> > >the constant initializer out of the loop? I bet if you compare binary
> > >sizes/code it will be exactly the same, and you added some characters
> > >of code. Reorganizing code for readability is fine, but for compiler
> > >(in)efficiency seems like a bad idea.
> > While I agree with your argument, I would like to point out that it is a
> > well established fact that GCC's optimizers are kind of brain-dead at times
> > and need their hands held.
> >
> > I'd be willing to bet that the code will be marginally larger (because of
> > adding another variable), but might run slightly faster too (because in my
> > experience, GCC doesn't always catch things like this), and should compile a
> > little faster (because the optimizers don't have to do as much work).
>
> Fun thing is, there's no speed issues with this portion of the code, and
> it's in userspace as well, so simplicity is the key that should be
> followed here.
>
> greg k-h
--
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: First kernel patch (optimization)

[Eric Curtin]

On 16 September 2015 at 21:02, Greg KH <gre...@linuxfoundation.org> wrote:
> On Wed, Sep 16, 2015 at 05:03:39PM +0100, Eric Curtin wrote:
>> Hi Greg,
>>
>> As I said in the subject of the mail (which I have been since told I
>> shouldn't have done this), I'm a noob to kernel code. I tried to pick
>> off something super simple to just see what the process of getting a
>> patch in is. Youtube videos and documentation only get you so far.
>>
>> >From reading your response, should I refrain from sending in these
>> micro-optimizations in future? Getting in smaller patches is easier
>> for me as I only do this in my spare time, which I don't have a lot
>> of!
>
> micro-optimizations are great, if you can actually measure them and they
> matter :)
>
> If you are looking for someplace to start, might I recommend the
> drivers/staging/ directory?  I take all sorts of "basic" cleanup
> patches, and there are loads of things to do in there.  Look at the
> drivers/staging/*/TODO files for specific examples of what needs to be
> done to get these files cleaned up and fixed properly.
>
> Hope this helps,
>
> greg k-h

I have realized this now after reading
http://kernelnewbies.org/FirstKernelPatch, should have done a bit more
reading before I submitted something. I'm looking at the
drivers/staging/android stuff now.

Eric Curtin
--
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/

tools: usbip: detach: avoid calling strlen() at each iteration

[Eric Curtin]

Instead of calling strlen on every iteration of the for loop, just call it
once and cache the result in a temporary local variable which will be used
in the for loop instead.

Signed-off-by: Eric Curtin 

diff --git a/tools/usb/usbip/src/usbip_detach.c 
b/tools/usb/usbip/src/usbip_detach.c
index 05c6d15..9db9d21 100644
--- a/tools/usb/usbip/src/usbip_detach.c
+++ b/tools/usb/usbip/src/usbip_detach.c
@@ -47,7 +47,9 @@ static int detach_port(char *port)
uint8_t portnum;
char path[PATH_MAX+1];
 
-   for (unsigned int i = 0; i < strlen(port); i++)
+   unsigned int port_len = strlen(port);
+
+   for (unsigned int i = 0; i < port_len; i++)
if (!isdigit(port[i])) {
err("invalid port %s", port);
return -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/

First kernel patch (optimization)

[Eric Curtin]

My first kernel patch, hope I did everything correctly! Instead of calling 
strlen on every iteration of the for loop, just call it once instead and store 
in a variable.

Signed-off-by: Eric Curtin 

diff --git a/tools/usb/usbip/src/usbip_detach.c 
b/tools/usb/usbip/src/usbip_detach.c
index 05c6d15..9db9d21 100644
--- a/tools/usb/usbip/src/usbip_detach.c
+++ b/tools/usb/usbip/src/usbip_detach.c
@@ -47,7 +47,9 @@ static int detach_port(char *port)
uint8_t portnum;
char path[PATH_MAX+1];
 
-   for (unsigned int i = 0; i < strlen(port); i++)
+   unsigned int port_len = strlen(port);
+
+   for (unsigned int i = 0; i < port_len; i++)
if (!isdigit(port[i])) {
err("invalid port %s", port);
return -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/

First kernel patch (optimization)

[Eric Curtin]

My first kernel patch, hope I did everything correctly! Instead of calling 
strlen on every iteration of the for loop, just call it once instead and store 
in a variable.

Signed-off-by: Eric Curtin 

diff --git a/tools/usb/usbip/src/usbip_detach.c 
b/tools/usb/usbip/src/usbip_detach.c
index 05c6d15..9db9d21 100644
--- a/tools/usb/usbip/src/usbip_detach.c
+++ b/tools/usb/usbip/src/usbip_detach.c
@@ -47,7 +47,9 @@ static int detach_port(char *port)
uint8_t portnum;
char path[PATH_MAX+1];
 
-   for (unsigned int i = 0; i < strlen(port); i++)
+   unsigned int port_len = strlen(port);
+
+   for (unsigned int i = 0; i < port_len; i++)
if (!isdigit(port[i])) {
err("invalid port %s", port);
return -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/

tools: usbip: detach: avoid calling strlen() at each iteration

[Eric Curtin]

Instead of calling strlen on every iteration of the for loop, just call it
once and cache the result in a temporary local variable which will be used
in the for loop instead.

Signed-off-by: Eric Curtin <ericcurti...@gmail.com>

diff --git a/tools/usb/usbip/src/usbip_detach.c 
b/tools/usb/usbip/src/usbip_detach.c
index 05c6d15..9db9d21 100644
--- a/tools/usb/usbip/src/usbip_detach.c
+++ b/tools/usb/usbip/src/usbip_detach.c
@@ -47,7 +47,9 @@ static int detach_port(char *port)
uint8_t portnum;
char path[PATH_MAX+1];
 
-   for (unsigned int i = 0; i < strlen(port); i++)
+   unsigned int port_len = strlen(port);
+
+   for (unsigned int i = 0; i < port_len; i++)
if (!isdigit(port[i])) {
err("invalid port %s", port);
return -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/

First kernel patch (optimization)

[Eric Curtin]

My first kernel patch, hope I did everything correctly! Instead of calling 
strlen on every iteration of the for loop, just call it once instead and store 
in a variable.

Signed-off-by: Eric Curtin <ericcurti...@gmail.com>

diff --git a/tools/usb/usbip/src/usbip_detach.c 
b/tools/usb/usbip/src/usbip_detach.c
index 05c6d15..9db9d21 100644
--- a/tools/usb/usbip/src/usbip_detach.c
+++ b/tools/usb/usbip/src/usbip_detach.c
@@ -47,7 +47,9 @@ static int detach_port(char *port)
uint8_t portnum;
char path[PATH_MAX+1];
 
-   for (unsigned int i = 0; i < strlen(port); i++)
+   unsigned int port_len = strlen(port);
+
+   for (unsigned int i = 0; i < port_len; i++)
if (!isdigit(port[i])) {
err("invalid port %s", port);
return -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/

First kernel patch (optimization)

[Eric Curtin]

My first kernel patch, hope I did everything correctly! Instead of calling 
strlen on every iteration of the for loop, just call it once instead and store 
in a variable.

Signed-off-by: Eric Curtin <ericcurti...@gmail.com>

diff --git a/tools/usb/usbip/src/usbip_detach.c 
b/tools/usb/usbip/src/usbip_detach.c
index 05c6d15..9db9d21 100644
--- a/tools/usb/usbip/src/usbip_detach.c
+++ b/tools/usb/usbip/src/usbip_detach.c
@@ -47,7 +47,9 @@ static int detach_port(char *port)
uint8_t portnum;
char path[PATH_MAX+1];
 
-   for (unsigned int i = 0; i < strlen(port); i++)
+   unsigned int port_len = strlen(port);
+
+   for (unsigned int i = 0; i < port_len; i++)
if (!isdigit(port[i])) {
err("invalid port %s", port);
return -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/