[nouveau] WARNING: possible circular locking dependency detected in linux-next

[Alexander Kapshuk]

u :01:00.0: Direct firmware load for
nouveau/nv84_xuc00f failed with error -2
[  358.920095] nouveau :01:00.0: vp: unable to load firmware
nouveau/nv84_xuc00f
[  358.920107] nouveau :01:00.0: vp: init failed, -2
[  358.920523] nouveau :01:00.0: Direct firmware load for
nouveau/nv84_xuc103 failed with error -2
[  358.920556] nouveau :01:00.0: bsp: unable to load firmware
nouveau/nv84_xuc103
[  358.920565] nouveau :01:00.0: bsp: init failed, -2

Thanks.
Alexander Kapshuk

Re: [PATCH] ver_linux: Eliminate duplicate code in ldconfig processing logic

[Alexander Kapshuk]

On Fri, Jan 8, 2021 at 1:26 PM Alexander Kapshuk
 wrote:
>
> The code that acquires the version strings for libc and libcpp is
> identical, as is the printversion call. The only difference being the
> name of the library being printed.
>
> Refactor the code by unifying the bits that are common to both libraries.
>
> Signed-off-by: Alexander Kapshuk 
> ---
>  scripts/ver_linux | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/ver_linux b/scripts/ver_linux
> index 0968a3070eff..a92acc703f9b 100755
> --- a/scripts/ver_linux
> +++ b/scripts/ver_linux
> @@ -15,7 +15,7 @@ BEGIN {
>
> vernum = "[0-9]+([.]?[0-9]+)+"
> libc = "libc[.]so[.][0-9]+$"
> -   libcpp = "(libg|stdc)[+]+[.]so[.][0-9]+$"
> +   libcpp = "(libg|stdc)[+]+[.]so([.][0-9]+)+$"
>
> printversion("GNU C", version("gcc -dumpversion"))
> printversion("GNU Make", version("make --version"))
> @@ -37,12 +37,10 @@ BEGIN {
> printversion("Bison", version("bison --version"))
> printversion("Flex", version("flex --version"))
>
> -   while ("ldconfig -p 2>/dev/null" | getline > 0) {
> -   if ($NF ~ libc && !seen[ver = version("readlink " $NF)]++)
> -   printversion("Linux C Library", ver)
> -   else if ($NF ~ libcpp && !seen[ver = version("readlink " 
> $NF)]++)
> -   printversion("Linux C++ Library", ver)
> -   }
> +   while ("ldconfig -p 2>/dev/null" | getline > 0)
> +   if ($NF ~ libc || $NF ~ libcpp)
> +   if (!seen[ver = version("readlink " $NF)]++)
> +   printversion("Linux C" ($NF ~ libcpp? "++" : 
> "") " Library", ver)
>
> printversion("Dynamic linker (ldd)", version("ldd --version"))
> printversion("Procps", version("ps --version"))
> --
> 2.30.0
>

I'd appreciate getting some feedback on the patch in question at your
convenience.

[PATCH] ver_linux: Eliminate duplicate code in ldconfig processing logic

[Alexander Kapshuk]

The code that acquires the version strings for libc and libcpp is
identical, as is the printversion call. The only difference being the
name of the library being printed.

Refactor the code by unifying the bits that are common to both libraries.

Signed-off-by: Alexander Kapshuk 
---
 scripts/ver_linux | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index 0968a3070eff..a92acc703f9b 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -15,7 +15,7 @@ BEGIN {

vernum = "[0-9]+([.]?[0-9]+)+"
libc = "libc[.]so[.][0-9]+$"
-   libcpp = "(libg|stdc)[+]+[.]so[.][0-9]+$"
+   libcpp = "(libg|stdc)[+]+[.]so([.][0-9]+)+$"

printversion("GNU C", version("gcc -dumpversion"))
printversion("GNU Make", version("make --version"))
@@ -37,12 +37,10 @@ BEGIN {
printversion("Bison", version("bison --version"))
printversion("Flex", version("flex --version"))

-   while ("ldconfig -p 2>/dev/null" | getline > 0) {
-   if ($NF ~ libc && !seen[ver = version("readlink " $NF)]++)
-   printversion("Linux C Library", ver)
-   else if ($NF ~ libcpp && !seen[ver = version("readlink " 
$NF)]++)
-   printversion("Linux C++ Library", ver)
-   }
+   while ("ldconfig -p 2>/dev/null" | getline > 0)
+   if ($NF ~ libc || $NF ~ libcpp)
+   if (!seen[ver = version("readlink " $NF)]++)
+   printversion("Linux C" ($NF ~ libcpp? "++" : 
"") " Library", ver)

printversion("Dynamic linker (ldd)", version("ldd --version"))
printversion("Procps", version("ps --version"))
--
2.30.0

Re: [PATCH v2] drm/nouveau/kms: Fix NULL pointer dereference in nouveau_connector_detect_depth

[Alexander Kapshuk]

On Tue, Oct 13, 2020 at 3:47 PM Alexander Kapshuk
 wrote:
>
> This oops manifests itself on the following hardware:
> 01:00.0 VGA compatible controller: NVIDIA Corporation G98M [GeForce G 103M] 
> (rev a1)
>
> Oct 09 14:17:46 lp-sasha kernel: BUG: kernel NULL pointer dereference, 
> address: 
> Oct 09 14:17:46 lp-sasha kernel: #PF: supervisor read access in kernel mode
> Oct 09 14:17:46 lp-sasha kernel: #PF: error_code(0x) - not-present page
> Oct 09 14:17:46 lp-sasha kernel: PGD 0 P4D 0
> Oct 09 14:17:46 lp-sasha kernel: Oops:  [#1] SMP PTI
> Oct 09 14:17:46 lp-sasha kernel: CPU: 1 PID: 191 Comm: systemd-udevd Not 
> tainted 5.9.0-rc8-next-20201009 #38
> Oct 09 14:17:46 lp-sasha kernel: Hardware name: Hewlett-Packard Compaq 
> Presario CQ61 Notebook PC/306A, BIOS F.03 03/23/2009
> Oct 09 14:17:46 lp-sasha kernel: RIP: 
> 0010:nouveau_connector_detect_depth+0x71/0xc0 [nouveau]
> Oct 09 14:17:46 lp-sasha kernel: Code: 0a 00 00 48 8b 49 48 c7 87 b8 00 00 00 
> 06 00 00 00 80 b9 4d 0a 00 00 00 75 1e 83 fa 41 75 05 48 85 c0 75 29 8b 81 10 
> 0d 00 00 <39> 06 7c 25 f6 81 14 0d 00 00 02 75 b7 c3 80 b9 0c 0d 00 00 00 75
> Oct 09 14:17:46 lp-sasha kernel: RSP: 0018:c928f8c0 EFLAGS: 00010297
> Oct 09 14:17:46 lp-sasha kernel: RAX: 00014c08 RBX: 8880369d4000 
> RCX: 8880369d3000
> Oct 09 14:17:46 lp-sasha kernel: RDX: 0040 RSI:  
> RDI: 8880369d4000
> Oct 09 14:17:46 lp-sasha kernel: RBP: 88800601cc00 R08: 8880051da298 
> R09: 8226201a
> Oct 09 14:17:46 lp-sasha kernel: R10: 88800469aa80 R11: 888004c84ff8 
> R12: 
> Oct 09 14:17:46 lp-sasha kernel: R13: 8880051da000 R14: 2000 
> R15: 0003
> Oct 09 14:17:46 lp-sasha kernel: FS:  7fd0192b3440() 
> GS:8880bc90() knlGS:
> Oct 09 14:17:46 lp-sasha kernel: CS:  0010 DS:  ES:  CR0: 
> 80050033
> Oct 09 14:17:46 lp-sasha kernel: CR2:  CR3: 04976000 
> CR4: 06e0
> Oct 09 14:17:46 lp-sasha kernel: Call Trace:
> Oct 09 14:17:46 lp-sasha kernel:  nouveau_connector_get_modes+0x1e6/0x240 
> [nouveau]
> Oct 09 14:17:46 lp-sasha kernel:  ? kfree+0xb9/0x240
> Oct 09 14:17:46 lp-sasha kernel:  ? drm_connector_list_iter_next+0x7c/0xa0
> Oct 09 14:17:46 lp-sasha kernel:  
> drm_helper_probe_single_connector_modes+0x1ba/0x7c0
> Oct 09 14:17:46 lp-sasha kernel:  drm_client_modeset_probe+0x27e/0x1360
> Oct 09 14:17:46 lp-sasha kernel:  ? nvif_object_sclass_put+0xc/0x20 [nouveau]
> Oct 09 14:17:46 lp-sasha kernel:  ? nouveau_cli_init+0x3cc/0x440 [nouveau]
> Oct 09 14:17:46 lp-sasha kernel:  ? ktime_get_mono_fast_ns+0x49/0xa0
> Oct 09 14:17:46 lp-sasha kernel:  ? nouveau_drm_open+0x4e/0x180 [nouveau]
> Oct 09 14:17:46 lp-sasha kernel:  
> __drm_fb_helper_initial_config_and_unlock+0x3f/0x4a0
> Oct 09 14:17:46 lp-sasha kernel:  ? drm_file_alloc+0x18f/0x260
> Oct 09 14:17:46 lp-sasha kernel:  ? mutex_lock+0x9/0x40
> Oct 09 14:17:46 lp-sasha kernel:  ? drm_client_init+0x110/0x160
> Oct 09 14:17:46 lp-sasha kernel:  nouveau_fbcon_init+0x14d/0x1c0 [nouveau]
> Oct 09 14:17:46 lp-sasha kernel:  nouveau_drm_device_init+0x1c0/0x880 
> [nouveau]
> Oct 09 14:17:46 lp-sasha kernel:  nouveau_drm_probe+0x11a/0x1e0 [nouveau]
> Oct 09 14:17:46 lp-sasha kernel:  pci_device_probe+0xcd/0x140
> Oct 09 14:17:46 lp-sasha kernel:  really_probe+0xd8/0x400
> Oct 09 14:17:46 lp-sasha kernel:  driver_probe_device+0x4a/0xa0
> Oct 09 14:17:46 lp-sasha kernel:  device_driver_attach+0x9c/0xc0
> Oct 09 14:17:46 lp-sasha kernel:  __driver_attach+0x6f/0x100
> Oct 09 14:17:46 lp-sasha kernel:  ? device_driver_attach+0xc0/0xc0
> Oct 09 14:17:46 lp-sasha kernel:  bus_for_each_dev+0x75/0xc0
> Oct 09 14:17:46 lp-sasha kernel:  bus_add_driver+0x106/0x1c0
> Oct 09 14:17:46 lp-sasha kernel:  driver_register+0x86/0xe0
> Oct 09 14:17:46 lp-sasha kernel:  ? 0xa044e000
> Oct 09 14:17:46 lp-sasha kernel:  do_one_initcall+0x48/0x1e0
> Oct 09 14:17:46 lp-sasha kernel:  ? _cond_resched+0x11/0x60
> Oct 09 14:17:46 lp-sasha kernel:  ? kmem_cache_alloc_trace+0x19c/0x1e0
> Oct 09 14:17:46 lp-sasha kernel:  do_init_module+0x57/0x220
> Oct 09 14:17:46 lp-sasha kernel:  __do_sys_finit_module+0xa0/0xe0
> Oct 09 14:17:46 lp-sasha kernel:  do_syscall_64+0x33/0x40
> Oct 09 14:17:46 lp-sasha kernel:  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> Oct 09 14:17:46 lp-sasha kernel: RIP: 0033:0x7fd01a060d5d
> Oct 09 14:17:46 lp-sasha kernel: Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 90 
> f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 
> 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e3 70 0c 00 f7 d8 64 89 01 48
> Oct 09 14:17:46 lp-

[PATCH v2] drm/nouveau/kms: Fix NULL pointer dereference in nouveau_connector_detect_depth

[Alexander Kapshuk]

uveau/nouveau_connector.c:891

It is actually line 889. See the disassembly below.
889 duallink = mode->clock >= 
bios->fp.duallink_transition_clk;

The NULL pointer being dereferenced is mode.

Git bisect has identified the following commit as bad:
f28e32d3906e drm/nouveau/kms: Don't change EDID when it hasn't actually changed

Here is the chain of events that causes the oops.
On entry to nouveau_connector_detect_lvds, edid is set to NULL.  The call
to nouveau_connector_detect sets nv_connector->edid to valid memory,
with status set to connector_status_connected and the flow of execution
branching to the out label.

The subsequent call to nouveau_connector_set_edid erronously clears
nv_connector->edid, via the local edid pointer which remains set to NULL.

Fix this by setting edid to the value of the just acquired
nv_connector->edid and executing the body of nouveau_connector_set_edid
only if nv_connector->edid and edid point to different memory addresses
thus preventing nv_connector->edid from being turned into a dangling
pointer.

Fixes: f28e32d3906e ("drm/nouveau/kms: Don't change EDID when it hasn't 
actually changed")
Signed-off-by: Alexander Kapshuk 
Reviewed-by: Lyude Paul 
---
v2:
-
- nouveau_connector_set_edid updated to do the (nv_connector->edid
!= edid) check instead of open coding it in nouveau_connector_detect_lvds
- added Reviewed-by: from Lyude Paul

 drivers/gpu/drm/nouveau/nouveau_connector.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c 
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 49dd0cbc332f..5eb322276be7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -532,11 +532,13 @@ static void
 nouveau_connector_set_edid(struct nouveau_connector *nv_connector,
   struct edid *edid)
 {
-   struct edid *old_edid = nv_connector->edid;
+   if (nv_connector->edid != edid) {
+   struct edid *old_edid = nv_connector->edid;

-   drm_connector_update_edid_property(_connector->base, edid);
-   kfree(old_edid);
-   nv_connector->edid = edid;
+   drm_connector_update_edid_property(_connector->base, edid);
+   kfree(old_edid);
+   nv_connector->edid = edid;
+   }
 }

 static enum drm_connector_status
@@ -669,8 +671,10 @@ nouveau_connector_detect_lvds(struct drm_connector 
*connector, bool force)
/* Try retrieving EDID via DDC */
if (!drm->vbios.fp_no_ddc) {
status = nouveau_connector_detect(connector, force);
-   if (status == connector_status_connected)
+   if (status == connector_status_connected) {
+   edid = nv_connector->edid;
goto out;
+   }
}

/* On some laptops (Sony, i'm looking at you) there appears to
--
2.28.0

[PATCH] drm/nouveau/kms: Fix NULL pointer dereference in nouveau_connector_detect_depth

[Alexander Kapshuk]

uveau/nouveau_connector.c:891

It is actually line 889. See the disassembly above.
889 duallink = mode->clock >= 
bios->fp.duallink_transition_clk;

The NULL pointer being dereferenced is mode.

Git bisect has identified the following commit as bad:
f28e32d3906e drm/nouveau/kms: Don't change EDID when it hasn't actually changed

Here is the chain of events that causes the oops.
On entry to nouveau_connector_detect_lvds, edid is set to NULL.
The call to nouveau_connector_detect sets nv_connector->edid to valid
memory, with status being set to connector_status_connected and the flow
of execution branching to the out label.

The subsequent call to nouveau_connector_set_edid erronously clears
nv_connector->edid, via the local edid pointer which remains set to NULL.

Fix this by setting edid to the value of the just acquired
nv_connector->edid and call nouveau_connector_set_edid in the out label
only if nv_connector->edid and edid point to different memory addresses
thus preventing nv_connector->edid from being turned into a dangling
pointer.

Fixes: f28e32d3906e ("drm/nouveau/kms: Don't change EDID when it hasn't 
actually changed")
Signed-off-by: Alexander Kapshuk 
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c 
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 49dd0cbc332f..a3f53d2ac86e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -669,8 +669,10 @@ nouveau_connector_detect_lvds(struct drm_connector 
*connector, bool force)
/* Try retrieving EDID via DDC */
if (!drm->vbios.fp_no_ddc) {
status = nouveau_connector_detect(connector, force);
-   if (status == connector_status_connected)
+   if (status == connector_status_connected) {
+   edid = nv_connector->edid;
goto out;
+   }
}

/* On some laptops (Sony, i'm looking at you) there appears to
@@ -720,7 +722,8 @@ nouveau_connector_detect_lvds(struct drm_connector 
*connector, bool force)
status = connector_status_unknown;
 #endif

-   nouveau_connector_set_edid(nv_connector, edid);
+   if (nv_connector->edid != edid)
+   nouveau_connector_set_edid(nv_connector, edid);
nouveau_connector_set_encoder(connector, nv_encoder);
return status;
 }
--
2.28.0

Re: nouveau PUSHBUFFER_ERR on 5.9.0-rc2-next-20200824

[Alexander Kapshuk]

On Mon, Aug 31, 2020 at 7:30 AM Ben Skeggs  wrote:
>
> On Tue, 25 Aug 2020 at 17:21, Alexander Kapshuk
>  wrote:
> >
> > Since upgrading to linux-next based on 5.9.0-rc1 and 5.9.0-rc2 I have
> > had my mouse pointer disappear soon after logging in, and I have
> > observed the system freezing temporarily when clicking on objects and
> > when typing text.
> > I have also found records of push buffer errors in dmesg output:
> > [ 6625.450394] nouveau :01:00.0: disp: ERROR 1 [PUSHBUFFER_ERR] 02
> > [] chid 0 mthd  data 0400
> Hey,
>
> Yeah, I'm aware of this.  Lyude and I have both seen it, but it's been
> very painful to track down to what's actually causing it so far.  It
> likely is the commit you mentioned that's at fault, and I'm still
> working to find a proper solution before I revert it.
>
> Ben.
>
> >
> > I tried setting CONFIG_NOUVEAU_DEBUG=5 (tracing) to try and collect
> > further debug info, but nothing caught the eye.
> >
> > The error message in question comes from nv50_disp_intr_error in
> > drivers/gpu/drm/nouveau/nvkm/engine/disp/nv50.c:613,645.
> > And nv50_disp_intr_error is called from nv50_disp_intr in the
> > following while block:
> > drivers/gpu/drm/nouveau/nvkm/engine/disp/nv50.c:647,658
> > void
> > nv50_disp_intr(struct nv50_disp *disp)
> > {
> > struct nvkm_device *device = disp->base.engine.subdev.device;
> > u32 intr0 = nvkm_rd32(device, 0x610020);
> > u32 intr1 = nvkm_rd32(device, 0x610024);
> >
> > while (intr0 & 0x001f) {
> > u32 chid = __ffs(intr0 & 0x001f) - 16;
> > nv50_disp_intr_error(disp, chid);
> > intr0 &= ~(0x0001 << chid);
> > }
> > ...
> > }
> >
> > Could this be in any way related to this series of commits?
> > commit 0a96099691c8cd1ac0744ef30b6846869dc2b566
> > Author: Ben Skeggs 
> > Date:   Tue Jul 21 11:34:07 2020 +1000
> >
> > drm/nouveau/kms/nv50-: implement proper push buffer control logic
> >
> > We had a, what was supposed to be temporary, hack in the KMS code where 
> > we'd
> > completely drain an EVO/NVD channel's push buffer when wrapping to the 
> > start
> > again, instead of treating it as a ring buffer.
> >
> > Let's fix that, finally.
> >
> > Signed-off-by: Ben Skeggs 
> >
> > Here are my GPU details:
> > 01:00.0 VGA compatible controller: NVIDIA Corporation GT216 [GeForce
> > 210] (rev a1)
> > Subsystem: Micro-Star International Co., Ltd. [MSI] Device 8a93
> > Kernel driver in use: nouveau
> >
> > The last linux-next kernel I built where the problem reported does not
> > manifest itself is 5.8.0-rc6-next-20200720.
> >
> > I would appreciate being given any pointers on how to further debug this.
> > Or is git bisect the only way to proceed with this?
> >
> > Thanks.
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Thanks a lot for getting back to me about this.
Please let me know if there's anything else I can do to help track this down.

Alexander.

nouveau PUSHBUFFER_ERR on 5.9.0-rc2-next-20200824

[Alexander Kapshuk]

Since upgrading to linux-next based on 5.9.0-rc1 and 5.9.0-rc2 I have
had my mouse pointer disappear soon after logging in, and I have
observed the system freezing temporarily when clicking on objects and
when typing text.
I have also found records of push buffer errors in dmesg output:
[ 6625.450394] nouveau :01:00.0: disp: ERROR 1 [PUSHBUFFER_ERR] 02
[] chid 0 mthd  data 0400

I tried setting CONFIG_NOUVEAU_DEBUG=5 (tracing) to try and collect
further debug info, but nothing caught the eye.

The error message in question comes from nv50_disp_intr_error in
drivers/gpu/drm/nouveau/nvkm/engine/disp/nv50.c:613,645.
And nv50_disp_intr_error is called from nv50_disp_intr in the
following while block:
drivers/gpu/drm/nouveau/nvkm/engine/disp/nv50.c:647,658
void
nv50_disp_intr(struct nv50_disp *disp)
{
struct nvkm_device *device = disp->base.engine.subdev.device;
u32 intr0 = nvkm_rd32(device, 0x610020);
u32 intr1 = nvkm_rd32(device, 0x610024);

while (intr0 & 0x001f) {
u32 chid = __ffs(intr0 & 0x001f) - 16;
nv50_disp_intr_error(disp, chid);
intr0 &= ~(0x0001 << chid);
}
...
}

Could this be in any way related to this series of commits?
commit 0a96099691c8cd1ac0744ef30b6846869dc2b566
Author: Ben Skeggs 
Date:   Tue Jul 21 11:34:07 2020 +1000

drm/nouveau/kms/nv50-: implement proper push buffer control logic

We had a, what was supposed to be temporary, hack in the KMS code where we'd
completely drain an EVO/NVD channel's push buffer when wrapping to the start
again, instead of treating it as a ring buffer.

Let's fix that, finally.

Signed-off-by: Ben Skeggs 

Here are my GPU details:
01:00.0 VGA compatible controller: NVIDIA Corporation GT216 [GeForce
210] (rev a1)
Subsystem: Micro-Star International Co., Ltd. [MSI] Device 8a93
Kernel driver in use: nouveau

The last linux-next kernel I built where the problem reported does not
manifest itself is 5.8.0-rc6-next-20200720.

I would appreciate being given any pointers on how to further debug this.
Or is git bisect the only way to proceed with this?

Thanks.

Re: [PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Alexander Kapshuk]

On Mon, Jun 22, 2020 at 11:40 AM Christian Brauner
 wrote:
>
> On Sun, Jun 21, 2020 at 03:54:37PM +0200, Dominique Martinet wrote:
> > Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> > > Export symbol __lock_task_sighand, so it is accessible from code compiled
> > > as modules.
> > > This fixes the following modpost error:
> > > ERROR: modpost: "__lock_task_sighand" [net/9p/9pnet.ko] undefined!
> >
> > This can't fix something that's not broken (yet)! :)
> >
> > I think it'd make more sense to describe why you think we should export
> > it, rather than describe a precise usecase e.g. justify why this would
> > be interesting to use from modules (e.g. it would help modules like 9p
> > take a lock on the current signal handler safely and cleanly through
> > lock_task_sighand())
> >
> >
> >
> > Christian, Andrew - assuming this passes reviews from someone else I'm
> > not sure how to go forward with this; it'd be simpler for me if I could
> > take it in the 9p tree as I need it for the patch Alexander pointed at,
> > but I'm not normally touching any file outside of the 9p tree.
> > Is it better to let either of you take it normally (I think it'd be
> > you?) and wait for that to land, or can I take it in my tree for the
> > next merge window?
>
> Hm, I don't think the patch is really needed though; see my other mail. :)
>
> Christian

Thanks Oleg and Christian for your feedback.
Based on what you both expressed, my patch that exported
__lock_task_sighand as well as the original patch to address a sparse
warning in net/9p/client.c
should be dropped and the warning considered a false positive.

I'll let Dominique have the final say on this.
Thanks.

Re: [PATCH] net/9p: Validate current->sighand in client.c

[Alexander Kapshuk]

On Sun, Jun 21, 2020 at 4:56 PM Dominique Martinet
 wrote:
>
> Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> > Fix rcu not being dereferenced cleanly by using the task
> > helpers (un)lock_task_sighand instead of spin_lock_irqsave and
> > spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
> > suggested in the email referenced below.
>
> Ack.
> I'll take this once symbol issue is resolved ; thanks for your time.
>
> --
> Dominique

Noted.
Thanks.

[PATCH] net/9p: Validate current->sighand in client.c

[Alexander Kapshuk]

Fix rcu not being dereferenced cleanly by using the task
helpers (un)lock_task_sighand instead of spin_lock_irqsave and
spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
suggested in the email referenced below.

Signed-off-by: Alexander Kapshuk 
Link: https://lore.kernel.org/lkml/20200618190807.GA20699@nautica/
---
 net/9p/client.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index fc1f3635e5dd..15f16f2baa8f 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -787,9 +787,14 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char 
*fmt, ...)
}
 recalc_sigpending:
if (sigpending) {
-   spin_lock_irqsave(>sighand->siglock, flags);
+   if (!lock_task_sighand(current, )) {
+   pr_warn("%s (%d): current->sighand==NULL in 
recalc_sigpending\n",
+   __func__, task_pid_nr(current));
+   err = -ESRCH;
+   goto reterr;
+   }
recalc_sigpending();
-   spin_unlock_irqrestore(>sighand->siglock, flags);
+   unlock_task_sighand(current, );
}
if (err < 0)
goto reterr;
@@ -869,9 +874,14 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client 
*c, int8_t type,
}
 recalc_sigpending:
if (sigpending) {
-   spin_lock_irqsave(>sighand->siglock, flags);
+   if (!lock_task_sighand(current, )) {
+   pr_warn("%s (%d): current->sighand==NULL in 
recalc_sigpending\n",
+   __func__, task_pid_nr(current));
+   err = -ESRCH;
+   goto reterr;
+   }
recalc_sigpending();
-   spin_unlock_irqrestore(>sighand->siglock, flags);
+   unlock_task_sighand(current, );
}
if (err < 0)
goto reterr;
--
2.27.0

[PATCH] kernel/signal.c: Export symbol __lock_task_sighand

[Alexander Kapshuk]

Export symbol __lock_task_sighand, so it is accessible from code compiled
as modules.
This fixes the following modpost error:
ERROR: modpost: "__lock_task_sighand" [net/9p/9pnet.ko] undefined!

Where __lock_task_sighand is called via lock_task_sighand in net/9p/client.c
See 
https://lore.kernel.org/lkml/20200620201456.14304-1-alexander.kaps...@gmail.com/.

Signed-off-by: Alexander Kapshuk 
Reported-by: kernel test robot 
Link: 
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org/thread/TMTLPYU6A522JH2VCN3PNZVAP6EE5MDF/
---
 kernel/signal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/signal.c b/kernel/signal.c
index 5ca48cc5da76..2612b9098cbd 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1396,6 +1396,7 @@ struct sighand_struct *__lock_task_sighand(struct 
task_struct *tsk,

return sighand;
 }
+EXPORT_SYMBOL(__lock_task_sighand);

 /*
  * send signal info to all the members of a group
--
2.27.0

Re: [PATCH] net/9p: Validate current->sighand in client.c

[Alexander Kapshuk]

On Sun, Jun 21, 2020 at 1:57 PM Dominique Martinet
 wrote:
>
> Alexander Kapshuk wrote on Sun, Jun 21, 2020:
> > Thanks for your feedback.
> > Shall I simply resend the v2 patch with the commit message reworded as
> > you suggested, or should I make it a v3 patch instead?
>
> Yes please resend the same commit reworded. v2 or v3 doesn't matter
> much, the [PATCH whatever] part of the mail isn't included in the commit
> message; I don't receive so much patches to be fussy about that :)
>
Understood. Thanks. :)


> > One other thing I wanted to clarify is I got a message from kernel
> > test robot ,
> > https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org/thread/TMTLPYU6A522JH2VCN3PNZVAP6EE5MDF/,
> > saying that on parisc my patch resulted in __lock_task_sighand being
> > undefined during modpost'ing.
> > I've noticed similar messages about other people's patches on the
> > linux-kernel mailing list with the responses stating that the issue
> > was at the compilation site rather than with the patch itself.
> > As far as I understand, that is the case with my patch also. So no
> > further action on that is required of me, is it?
>
> Thanks, I hadn't noticed this mail -- unfortunately I don't have
> anything setup to test pa risc, but __lock_task_sighand is defined in
> kernel/signal.c which is unconditionally compiled so shouldn't have
> anything to do with arch-specific failures, so I will run into the same
> problem when I test this.
>
> From just looking at the code, it looks like a real problem to me -
> __lock_task_sighand is never passed to EXPORT_SYMBOL so when building 9p
> as a module we cannot use the function. Only exported symbols can be
> called from code that can be built as modules.
>
> That looks like an oversight to me more than something on purpose, but
> it does mean I cannot take the patch right now -- we need to first get
> the symbol exported before we can use it in 9p.
>
>
> As things stand I'd rather have this patch wait one cycle for this than
> revert to manipulating rcu directly like you did first -- if you're up
> for it you can send a patch to get it exported first and I'll pick this
> patch up next cycle, or I can take care of that too if you don't want to
> bother.
>
> Letting you tell me which you prefer,
> --
> Dominique

I am willing to send in a patch to have the missing symbol exported as
well as resend my previous one with the commit message reworded.
Thanks.

Re: [PATCH] net/9p: Validate current->sighand in client.c

[Alexander Kapshuk]

On Sun, Jun 21, 2020 at 11:45 AM Dominique Martinet
 wrote:
>
> Alexander Kapshuk wrote on Sat, Jun 20, 2020:
> > Use (un)lock_task_sighand instead of spin_lock_irqsave and
> > spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
> > suggested in the email referenced below.
>
> Thanks for v2! Patch itself looks good to me.
>
> I always add another `Link:` tag to the last version of the patch at the
> time of applying, so the message might be a bit confusing.
> Feel free to keep the link to the previous discussion but I'd rather
> just repeat a bit more of what we discussed (e.g. fix rcu not being
> dereferenced cleanly by using the task helpers as suggested) rather than
> just link to the thread
>
> Sorry for nitpicking but I think commit messages are important and it's
> better if they're understandable out of context, even if you give a link
> for further details for curious readers, it helps being able to just
> skim through git log.
>
>
> Either way I'll include the patch in my test run today or tomorrow, had
> promised it for a while...
>
> Cheers,
> --
> Dominique

Hi Dominique,

Thanks for your feedback.
Shall I simply resend the v2 patch with the commit message reworded as
you suggested, or should I make it a v3 patch instead?

One other thing I wanted to clarify is I got a message from kernel
test robot ,
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org/thread/TMTLPYU6A522JH2VCN3PNZVAP6EE5MDF/,
saying that on parisc my patch resulted in __lock_task_sighand being
undefined during modpost'ing.
I've noticed similar messages about other people's patches on the
linux-kernel mailing list with the responses stating that the issue
was at the compilation site rather than with the patch itself.
As far as I understand, that is the case with my patch also. So no
further action on that is required of me, is it?
Thanks.

[PATCH] net/9p: Validate current->sighand in client.c

[Alexander Kapshuk]

Use (un)lock_task_sighand instead of spin_lock_irqsave and
spin_unlock_irqrestore to ensure current->sighand is a valid pointer as
suggested in the email referenced below.

Signed-off-by: Alexander Kapshuk 
Link: https://lore.kernel.org/lkml/20200618190807.GA20699@nautica/
---
 net/9p/client.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index fc1f3635e5dd..15f16f2baa8f 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -787,9 +787,14 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char 
*fmt, ...)
}
 recalc_sigpending:
if (sigpending) {
-   spin_lock_irqsave(>sighand->siglock, flags);
+   if (!lock_task_sighand(current, )) {
+   pr_warn("%s (%d): current->sighand==NULL in 
recalc_sigpending\n",
+   __func__, task_pid_nr(current));
+   err = -ESRCH;
+   goto reterr;
+   }
recalc_sigpending();
-   spin_unlock_irqrestore(>sighand->siglock, flags);
+   unlock_task_sighand(current, );
}
if (err < 0)
goto reterr;
@@ -869,9 +874,14 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client 
*c, int8_t type,
}
 recalc_sigpending:
if (sigpending) {
-   spin_lock_irqsave(>sighand->siglock, flags);
+   if (!lock_task_sighand(current, )) {
+   pr_warn("%s (%d): current->sighand==NULL in 
recalc_sigpending\n",
+   __func__, task_pid_nr(current));
+   err = -ESRCH;
+   goto reterr;
+   }
recalc_sigpending();
-   spin_unlock_irqrestore(>sighand->siglock, flags);
+   unlock_task_sighand(current, );
}
if (err < 0)
goto reterr;
--
2.27.0

Re: [PATCH] net/9p: Fix sparse rcu warnings in client.c

[Alexander Kapshuk]

On Thu, Jun 18, 2020 at 10:08 PM Dominique Martinet
 wrote:
>
> Alexander Kapshuk wrote on Thu, Jun 18, 2020:
> > Address sparse nonderef rcu warnings:
> > net/9p/client.c:790:17: warning: incorrect type in argument 1 (different 
> > address spaces)
> > net/9p/client.c:790:17:expected struct spinlock [usertype] *lock
> > net/9p/client.c:790:17:got struct spinlock [noderef]  *
> > net/9p/client.c:792:48: warning: incorrect type in argument 1 (different 
> > address spaces)
> > net/9p/client.c:792:48:expected struct spinlock [usertype] *lock
> > net/9p/client.c:792:48:got struct spinlock [noderef]  *
> > net/9p/client.c:872:17: warning: incorrect type in argument 1 (different 
> > address spaces)
> > net/9p/client.c:872:17:expected struct spinlock [usertype] *lock
> > net/9p/client.c:872:17:got struct spinlock [noderef]  *
> > net/9p/client.c:874:48: warning: incorrect type in argument 1 (different 
> > address spaces)
> > net/9p/client.c:874:48:expected struct spinlock [usertype] *lock
> > net/9p/client.c:874:48:got struct spinlock [noderef]  *
> >
> > Signed-off-by: Alexander Kapshuk 
>
> Thanks for this patch.
> From what I can see, there are tons of other parts of the code doing the
> same noderef access pattern to access current->sighand->siglock and I
> don't see much doing that.
> A couple of users justify this by saying SLAB_TYPESAFE_BY_RCU ensures
> we'll always get a usable lock which won't be reinitialized however we
> access it... It's a bit dubious we'll get the same lock than unlock to
> me, so I agree to some change though.
>
> After a second look I think we should use something like the following:
>
> if (!lock_task_sighand(current, ))
> warn & skip (or some error, we'd null deref if this happened 
> currently);
> recalc_sigpending();
> unlock_task_sighand(current, );
>
> As you can see, the rcu_read_lock() isn't kept until the unlock so I'm
> not sure it will be enough to please sparse, but I've convinced myself
> current->sighand cannot change while we hold the lock and there just are
> too many such patterns in the kernel.
>
> Please let me know if I missed something or if there is an ongoing
> effort to change how this works; I'll wait for a v2.
>
> --
> Dominique

Thanks for your prompt response.
I too made the same observation of the numerous patterns in the kernel
where current->sighand is accessed without being rcu_dereference()'d.
For this patch I used kernel/signal.c:1368,1398: __lock_task_sighand()
as an example.

I will give your suggestion a careful consideration and will get back
to you soon.
Thanks.

Re: [PATCH] net/9p: Fix sparse endian warning in trans_fd.c

[Alexander Kapshuk]

On Thu, Jun 18, 2020 at 10:09 PM Dominique Martinet
 wrote:
>
> Alexander Kapshuk wrote on Thu, Jun 18, 2020:
> > Address sparse endian warning:
> > net/9p/trans_fd.c:932:28: warning: incorrect type in assignment (different 
> > base types)
> > net/9p/trans_fd.c:932:28:expected restricted __be32 [addressable] 
> > [assigned] [usertype] s_addr
> > net/9p/trans_fd.c:932:28:    got unsigned long
> >
> > Signed-off-by: Alexander Kapshuk 
>
> INADDR_ANY is 0 so this really is noop but sure, less warnings is always
> good. I'll take this one for 5.9.
> Thanks!
> --
> Dominique

Noted.
Thanks.

[PATCH] net/9p: Fix sparse endian warning in trans_fd.c

[Alexander Kapshuk]

Address sparse endian warning:
net/9p/trans_fd.c:932:28: warning: incorrect type in assignment (different base 
types)
net/9p/trans_fd.c:932:28:expected restricted __be32 [addressable] 
[assigned] [usertype] s_addr
net/9p/trans_fd.c:932:28:got unsigned long

Signed-off-by: Alexander Kapshuk 
---
 net/9p/trans_fd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 13cd683a658a..2581f5145a22 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -929,7 +929,7 @@ static int p9_bind_privport(struct socket *sock)

memset(, 0, sizeof(cl));
cl.sin_family = AF_INET;
-   cl.sin_addr.s_addr = INADDR_ANY;
+   cl.sin_addr.s_addr = htonl(INADDR_ANY);
for (port = p9_ipport_resv_max; port >= p9_ipport_resv_min; port--) {
cl.sin_port = htons((ushort)port);
err = kernel_bind(sock, (struct sockaddr *), sizeof(cl));
--
2.27.0

[PATCH] net/9p: Fix sparse rcu warnings in client.c

[Alexander Kapshuk]

Address sparse nonderef rcu warnings:
net/9p/client.c:790:17: warning: incorrect type in argument 1 (different 
address spaces)
net/9p/client.c:790:17:expected struct spinlock [usertype] *lock
net/9p/client.c:790:17:got struct spinlock [noderef]  *
net/9p/client.c:792:48: warning: incorrect type in argument 1 (different 
address spaces)
net/9p/client.c:792:48:expected struct spinlock [usertype] *lock
net/9p/client.c:792:48:got struct spinlock [noderef]  *
net/9p/client.c:872:17: warning: incorrect type in argument 1 (different 
address spaces)
net/9p/client.c:872:17:expected struct spinlock [usertype] *lock
net/9p/client.c:872:17:got struct spinlock [noderef]  *
net/9p/client.c:874:48: warning: incorrect type in argument 1 (different 
address spaces)
net/9p/client.c:874:48:expected struct spinlock [usertype] *lock
net/9p/client.c:874:48:got struct spinlock [noderef]  *

Signed-off-by: Alexander Kapshuk 
---
 net/9p/client.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index fc1f3635e5dd..807e0e2e2e5a 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -787,9 +787,15 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char 
*fmt, ...)
}
 recalc_sigpending:
if (sigpending) {
-   spin_lock_irqsave(>sighand->siglock, flags);
+   struct sighand_struct *sighand;
+   rcu_read_lock();
+   sighand = rcu_dereference(current->sighand);
+
+   spin_lock_irqsave(>siglock, flags);
recalc_sigpending();
-   spin_unlock_irqrestore(>sighand->siglock, flags);
+   spin_unlock_irqrestore(>siglock, flags);
+
+   rcu_read_unlock();
}
if (err < 0)
goto reterr;
@@ -869,9 +875,15 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client 
*c, int8_t type,
}
 recalc_sigpending:
if (sigpending) {
-   spin_lock_irqsave(>sighand->siglock, flags);
+   struct sighand_struct *sighand;
+   rcu_read_lock();
+   sighand = rcu_dereference(current->sighand);
+
+   spin_lock_irqsave(>siglock, flags);
recalc_sigpending();
-   spin_unlock_irqrestore(>sighand->siglock, flags);
+   spin_unlock_irqrestore(>siglock, flags);
+
+   rcu_read_unlock();
}
if (err < 0)
goto reterr;
--
2.27.0

[tip: core/objtool] x86/insn: Fix awk regexp warnings

[tip-bot2 for Alexander Kapshuk]

The following commit has been merged into the core/objtool branch of tip:

Commit-ID: 700c1018b86d0d4b3f1f2d459708c0cdf42b521d
Gitweb:
https://git.kernel.org/tip/700c1018b86d0d4b3f1f2d459708c0cdf42b521d
Author:Alexander Kapshuk 
AuthorDate:Tue, 24 Sep 2019 07:46:59 +03:00
Committer: Borislav Petkov 
CommitterDate: Tue, 01 Oct 2019 12:13:16 +02:00

x86/insn: Fix awk regexp warnings

gawk 5.0.1 generates the following regexp warnings:

  GEN  /home/sasha/torvalds/tools/objtool/arch/x86/lib/inat-tables.c
  awk: ../arch/x86/tools/gen-insn-attr-x86.awk:260: warning: regexp escape 
sequence `\:' is not a known regexp operator
  awk: ../arch/x86/tools/gen-insn-attr-x86.awk:350: 
(FILENAME=../arch/x86/lib/x86-opcode-map.txt FNR=41) warning: regexp escape 
sequence `\&' is  not a known regexp operator

Ealier versions of gawk are not known to generate these warnings. The
gawk manual referenced below does not list characters ':' and '&' as
needing escaping, so 'unescape' them. See

  https://www.gnu.org/software/gawk/manual/html_node/Escape-Sequences.html

for more info.

Running diff on the output generated by the script before and after
applying the patch reported no differences.

 [ bp: Massage commit message. ]

[ Caught the respective tools header discrepancy. ]
Reported-by: kbuild test robot 
Signed-off-by: Alexander Kapshuk 
Signed-off-by: Borislav Petkov 
Acked-by: Masami Hiramatsu 
Cc: "H. Peter Anvin" 
Cc: "Peter Zijlstra (Intel)" 
Cc: Arnaldo Carvalho de Melo 
Cc: Ingo Molnar 
Cc: Josh Poimboeuf 
Cc: Thomas Gleixner 
Cc: x86-ml 
Link: 
https://lkml.kernel.org/r/20190924044659.3785-1-alexander.kaps...@gmail.com
---
 arch/x86/tools/gen-insn-attr-x86.awk   | 4 ++--
 tools/arch/x86/tools/gen-insn-attr-x86.awk | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/tools/gen-insn-attr-x86.awk 
b/arch/x86/tools/gen-insn-attr-x86.awk
index b02a36b..a42015b 100644
--- a/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/arch/x86/tools/gen-insn-attr-x86.awk
@@ -69,7 +69,7 @@ BEGIN {
 
lprefix1_expr = "\\((66|!F3)\\)"
lprefix2_expr = "\\(F3\\)"
-   lprefix3_expr = "\\((F2|!F3|66\\)\\)"
+   lprefix3_expr = "\\((F2|!F3|66)\\)"
lprefix_expr = "\\((66|F2|F3)\\)"
max_lprefix = 4
 
@@ -257,7 +257,7 @@ function convert_operands(count,opnd,   i,j,imm,mod)
return add_flags(imm, mod)
 }
 
-/^[0-9a-f]+\:/ {
+/^[0-9a-f]+:/ {
if (NR == 1)
next
# get index
diff --git a/tools/arch/x86/tools/gen-insn-attr-x86.awk 
b/tools/arch/x86/tools/gen-insn-attr-x86.awk
index b02a36b..a42015b 100644
--- a/tools/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/tools/arch/x86/tools/gen-insn-attr-x86.awk
@@ -69,7 +69,7 @@ BEGIN {
 
lprefix1_expr = "\\((66|!F3)\\)"
lprefix2_expr = "\\(F3\\)"
-   lprefix3_expr = "\\((F2|!F3|66\\)\\)"
+   lprefix3_expr = "\\((F2|!F3|66)\\)"
lprefix_expr = "\\((66|F2|F3)\\)"
max_lprefix = 4
 
@@ -257,7 +257,7 @@ function convert_operands(count,opnd,   i,j,imm,mod)
return add_flags(imm, mod)
 }
 
-/^[0-9a-f]+\:/ {
+/^[0-9a-f]+:/ {
if (NR == 1)
next
# get index

Re: [PATCH RESEND] gen-insn-attr-x86.awk: Fix regexp warnings

[Alexander Kapshuk]

On Mon, Sep 30, 2019 at 8:35 AM Borislav Petkov  wrote:
>
> On Tue, Sep 24, 2019 at 07:46:59AM +0300, Alexander Kapshuk wrote:
> > gawk 5.0.1 generates the regexp warnings shown below:
> > GEN  /home/sasha/torvalds/tools/objtool/arch/x86/lib/inat-tables.c
> > awk: ../arch/x86/tools/gen-insn-attr-x86.awk:260: warning: regexp escape 
> > sequence `\:' is not a known regexp operator
> > awk: ../arch/x86/tools/gen-insn-attr-x86.awk:350: 
> > (FILENAME=../arch/x86/lib/x86-opcode-map.txt FNR=41) warning: regexp escape 
> > sequence `\&' is not a known regexp operator
> >
> > Ealier versions of gawk are not known to generate these warnings.
> >
> > The gawk manual referenced below does not list characters ':' and '&'
> > as needing escaping, so 'unescape' them.
> > https://www.gnu.org/software/gawk/manual/html_node/Escape-Sequences.html
> >
> > Running diff on the output generated by the script before and after
> > applying the patch reported no differences.
> >
> > Signed-off-by: Alexander Kapshuk 
> > Reported-by: kbuild test robot 
> > Reviewed-by: Borislav Petkov 
>
> This is not how Reviewed-by works. Read here:
>
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
>
> for future reference. I fixed it up now.
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Thank you for taking in the patch and clarifying my misunderstanding
about the use of 'Reviewed-by'.
Point taken.

[PATCH RESEND] gen-insn-attr-x86.awk: Fix regexp warnings

[Alexander Kapshuk]

gawk 5.0.1 generates the regexp warnings shown below:
GEN  /home/sasha/torvalds/tools/objtool/arch/x86/lib/inat-tables.c
awk: ../arch/x86/tools/gen-insn-attr-x86.awk:260: warning: regexp escape 
sequence `\:' is not a known regexp operator
awk: ../arch/x86/tools/gen-insn-attr-x86.awk:350: 
(FILENAME=../arch/x86/lib/x86-opcode-map.txt FNR=41) warning: regexp escape 
sequence `\&' is not a known regexp operator

Ealier versions of gawk are not known to generate these warnings.

The gawk manual referenced below does not list characters ':' and '&'
as needing escaping, so 'unescape' them.
https://www.gnu.org/software/gawk/manual/html_node/Escape-Sequences.html

Running diff on the output generated by the script before and after
applying the patch reported no differences.

Signed-off-by: Alexander Kapshuk 
Reported-by: kbuild test robot 
Reviewed-by: Borislav Petkov 
Acked-by: Masami Hiramatsu 
---
 arch/x86/tools/gen-insn-attr-x86.awk   | 4 ++--
 tools/arch/x86/tools/gen-insn-attr-x86.awk | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/tools/gen-insn-attr-x86.awk 
b/arch/x86/tools/gen-insn-attr-x86.awk
index b02a36b2c14f..a42015b305f4 100644
--- a/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/arch/x86/tools/gen-insn-attr-x86.awk
@@ -69,7 +69,7 @@ BEGIN {

lprefix1_expr = "\\((66|!F3)\\)"
lprefix2_expr = "\\(F3\\)"
-   lprefix3_expr = "\\((F2|!F3|66\\)\\)"
+   lprefix3_expr = "\\((F2|!F3|66)\\)"
lprefix_expr = "\\((66|F2|F3)\\)"
max_lprefix = 4

@@ -257,7 +257,7 @@ function convert_operands(count,opnd,   i,j,imm,mod)
return add_flags(imm, mod)
 }

-/^[0-9a-f]+\:/ {
+/^[0-9a-f]+:/ {
if (NR == 1)
next
# get index
diff --git a/tools/arch/x86/tools/gen-insn-attr-x86.awk 
b/tools/arch/x86/tools/gen-insn-attr-x86.awk
index b02a36b2c14f..a42015b305f4 100644
--- a/tools/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/tools/arch/x86/tools/gen-insn-attr-x86.awk
@@ -69,7 +69,7 @@ BEGIN {

lprefix1_expr = "\\((66|!F3)\\)"
lprefix2_expr = "\\(F3\\)"
-   lprefix3_expr = "\\((F2|!F3|66\\)\\)"
+   lprefix3_expr = "\\((F2|!F3|66)\\)"
lprefix_expr = "\\((66|F2|F3)\\)"
max_lprefix = 4

@@ -257,7 +257,7 @@ function convert_operands(count,opnd,   i,j,imm,mod)
return add_flags(imm, mod)
 }

-/^[0-9a-f]+\:/ {
+/^[0-9a-f]+:/ {
if (NR == 1)
next
# get index
--
2.23.0

Re: [PATCH RESEND] gen-insn-attr-x86.awk: Fix regexp warnings

[Alexander Kapshuk]

On Mon, Sep 23, 2019 at 1:31 PM Borislav Petkov  wrote:
>
> + Masami.
>
> On Sun, Sep 22, 2019 at 06:03:28PM +0300, Alexander Kapshuk wrote:
> > This patch fixes the regexp warnings shown below:
>
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
>
> Also, do
>
> $ git grep 'This patch' Documentation/process
>
> for more details.
>
> > GEN  /home/sasha/torvalds/tools/objtool/arch/x86/lib/inat-tables.c
> > awk: ../arch/x86/tools/gen-insn-attr-x86.awk:260: warning: regexp escape 
> > sequence `\:' is not a known regexp operator
> > awk: ../arch/x86/tools/gen-insn-attr-x86.awk:350: 
> > (FILENAME=../arch/x86/lib/x86-opcode-map.txt FNR=41) warning: regexp escape 
> > sequence `\&' is not a known regexp operator
> >
> > The ':' and '&' characters need not escaping when used in string constants
> > as part of regular expressions.
>
> I could use a reasoning here, as in, "gawk manual doesn't have those two
> characters in the list here:
>
> https://www.gnu.org/software/gawk/manual/html_node/Escape-Sequences.html;
>
> or so.
>
> >
> > [Test-run]
> > awk -f arch/x86/tools/gen-insn-attr-x86.awk \
> >   arch/x86/lib/x86-opcode-map.txt >../tmp/inat-tables.c
> >
> > diff arch/x86/lib/inat-tables.c ~/tmp/inat-tables.c; echo $?
> > 0
> >
> > awk -f tools/arch/x86/tools/gen-insn-attr-x86.awk \
> >   tools/arch/x86/lib/x86-opcode-map.txt >../tmp/inat-tables.c
> >
> > diff tools/objtool/arch/x86/lib/inat-tables.c ~/tmp/inat-tables.c; echo $?
> > 0
>
> No need for that - just say that diffing the output before and after
> shows no changes.
>
> > [Debugging output]
> > DBG:ext:(66)
> > DBG:match(ext, ...):(66)
> > DBG:match(..., lprefix3_expr):\((F2|!F3|66)\)
>
> That is supposed to say what exactly? That it still does what it is
> expected to do?

That was the intention.

Thanks for reviewing the patch.
I'll wait to hear from Masami before resending the patch.

>
> Leaving in the rest for Masami.
>
> Thx.
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH RESEND] gen-insn-attr-x86.awk: Fix regexp warnings

[Alexander Kapshuk]

On Mon, Sep 23, 2019 at 12:19 PM Borislav Petkov  wrote:
>
> On Sun, Sep 22, 2019 at 06:03:28PM +0300, Alexander Kapshuk wrote:
> > This patch fixes the regexp warnings shown below:
> > GEN  /home/sasha/torvalds/tools/objtool/arch/x86/lib/inat-tables.c
> > awk: ../arch/x86/tools/gen-insn-attr-x86.awk:260: warning: regexp escape 
> > sequence `\:' is not a known regexp operator
> > awk: ../arch/x86/tools/gen-insn-attr-x86.awk:350: 
> > (FILENAME=../arch/x86/lib/x86-opcode-map.txt FNR=41) warning: regexp escape 
> > sequence `\&' is not a known regexp operator
> >
> > The ':' and '&' characters need not escaping when used in string constants
> > as part of regular expressions.
>
> How do you trigger this?
>
> I don't see it in my builds so it looks like environment thing. What
> flavor of awk is yours?
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

gawk 5.0.1-1 on Arch Linux.

[PATCH RESEND] gen-insn-attr-x86.awk: Fix regexp warnings

[Alexander Kapshuk]

This patch fixes the regexp warnings shown below:
GEN  /home/sasha/torvalds/tools/objtool/arch/x86/lib/inat-tables.c
awk: ../arch/x86/tools/gen-insn-attr-x86.awk:260: warning: regexp escape 
sequence `\:' is not a known regexp operator
awk: ../arch/x86/tools/gen-insn-attr-x86.awk:350: 
(FILENAME=../arch/x86/lib/x86-opcode-map.txt FNR=41) warning: regexp escape 
sequence `\&' is not a known regexp operator

The ':' and '&' characters need not escaping when used in string constants
as part of regular expressions.

[Test-run]
awk -f arch/x86/tools/gen-insn-attr-x86.awk \
arch/x86/lib/x86-opcode-map.txt >../tmp/inat-tables.c

diff arch/x86/lib/inat-tables.c ~/tmp/inat-tables.c; echo $?
0

awk -f tools/arch/x86/tools/gen-insn-attr-x86.awk \
tools/arch/x86/lib/x86-opcode-map.txt >../tmp/inat-tables.c

diff tools/objtool/arch/x86/lib/inat-tables.c ~/tmp/inat-tables.c; echo $?
0

[Debugging output]
DBG:ext:(66)
DBG:match(ext, ...):(66)
DBG:match(..., lprefix3_expr):\((F2|!F3|66)\)

Signed-off-by: Alexander Kapshuk 
Reported-by: kbuild test robot 
---
 arch/x86/tools/gen-insn-attr-x86.awk   | 4 ++--
 tools/arch/x86/tools/gen-insn-attr-x86.awk | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/tools/gen-insn-attr-x86.awk 
b/arch/x86/tools/gen-insn-attr-x86.awk
index b02a36b2c14f..a42015b305f4 100644
--- a/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/arch/x86/tools/gen-insn-attr-x86.awk
@@ -69,7 +69,7 @@ BEGIN {

lprefix1_expr = "\\((66|!F3)\\)"
lprefix2_expr = "\\(F3\\)"
-   lprefix3_expr = "\\((F2|!F3|66\\)\\)"
+   lprefix3_expr = "\\((F2|!F3|66)\\)"
lprefix_expr = "\\((66|F2|F3)\\)"
max_lprefix = 4

@@ -257,7 +257,7 @@ function convert_operands(count,opnd,   i,j,imm,mod)
return add_flags(imm, mod)
 }

-/^[0-9a-f]+\:/ {
+/^[0-9a-f]+:/ {
if (NR == 1)
next
# get index
diff --git a/tools/arch/x86/tools/gen-insn-attr-x86.awk 
b/tools/arch/x86/tools/gen-insn-attr-x86.awk
index b02a36b2c14f..a42015b305f4 100644
--- a/tools/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/tools/arch/x86/tools/gen-insn-attr-x86.awk
@@ -69,7 +69,7 @@ BEGIN {

lprefix1_expr = "\\((66|!F3)\\)"
lprefix2_expr = "\\(F3\\)"
-   lprefix3_expr = "\\((F2|!F3|66\\)\\)"
+   lprefix3_expr = "\\((F2|!F3|66)\\)"
lprefix_expr = "\\((66|F2|F3)\\)"
max_lprefix = 4

@@ -257,7 +257,7 @@ function convert_operands(count,opnd,   i,j,imm,mod)
return add_flags(imm, mod)
 }

-/^[0-9a-f]+\:/ {
+/^[0-9a-f]+:/ {
if (NR == 1)
next
# get index
--
2.23.0

[PATCH] gen-insn-attr-x86.awk: Fix regexp warnings

[Alexander Kapshuk]

This patch fixes the regexp warnings shown below:
  GEN  /home/sasha/torvalds/tools/objtool/arch/x86/lib/inat-tables.c
awk: ../arch/x86/tools/gen-insn-attr-x86.awk:260: warning: regexp escape 
sequence `\:' is not a known regexp operator
awk: ../arch/x86/tools/gen-insn-attr-x86.awk:350: 
(FILENAME=../arch/x86/lib/x86-opcode-map.txt FNR=41) warning: regexp escape 
sequence `\&' is not a known regexp operator

The ':' and '&' characters need not escaping when used in string constants
as part of regular expressions.

[Test-run]
awk -f /home/sasha/torvalds/arch/x86/tools/gen-insn-attr-x86.awk \
/home/sasha/torvalds/arch/x86/lib/x86-opcode-map.txt >tmp/inat-tables.c

diff -U0 /home/sasha/torvalds/tools/objtool/arch/x86/lib/inat-tables.c \
tmp/inat-tables.c; echo $?
0

[Debugging output]
DBG:ext:(66)
DBG:match(ext, ...):(66)
DBG:match(..., lprefix3_expr):\((F2|!F3|66)\)

Signed-off-by: Alexander Kapshuk 
---
 arch/x86/tools/gen-insn-attr-x86.awk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/tools/gen-insn-attr-x86.awk 
b/arch/x86/tools/gen-insn-attr-x86.awk
index b02a36b2c14f..a42015b305f4 100644
--- a/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/arch/x86/tools/gen-insn-attr-x86.awk
@@ -69,7 +69,7 @@ BEGIN {

lprefix1_expr = "\\((66|!F3)\\)"
lprefix2_expr = "\\(F3\\)"
-   lprefix3_expr = "\\((F2|!F3|66\\)\\)"
+   lprefix3_expr = "\\((F2|!F3|66)\\)"
lprefix_expr = "\\((66|F2|F3)\\)"
max_lprefix = 4

@@ -257,7 +257,7 @@ function convert_operands(count,opnd,   i,j,imm,mod)
return add_flags(imm, mod)
 }

-/^[0-9a-f]+\:/ {
+/^[0-9a-f]+:/ {
if (NR == 1)
next
# get index
--
2.23.0

Re: Kernel panic during drm/nouveau init 5.3.0-rc7-next-20190903

[Alexander Kapshuk]

On Mon, Sep 9, 2019 at 1:21 PM Stephen Rothwell  wrote:
>
> Hi,
>
> On Mon, 9 Sep 2019 20:11:59 +1000 Stephen Rothwell  
> wrote:
> >
> > If you are bisecting linux-next, I will suggest bisecting between the
> > stable branch on linux-next (which is just Linus' tree when I started
> > that day) and the top of the first linux-next that fails.  (Assuming
> > that the stable branch is good).
>
> Actually (since you won't be bisecting the latest linux-next), you
> probably want to use
>
> git merge-base stable next-20190903
> (or whatever linux-next you are bisecting)
>
> as your first good commit (assuming it id good :-)).
>
> --
> Cheers,
> Stephen Rothwell

Hi Stephen,

Thanks very much for the tips.
I'll go ahead and give those a try.

Re: Kernel panic during drm/nouveau init 5.3.0-rc7-next-20190903

[Alexander Kapshuk]

On Sat, Sep 07, 2019 at 12:32:41PM +0200, Daniel Vetter wrote:
> On Sat, Sep 7, 2019 at 11:05 AM Alexander Kapshuk
>  wrote:
> >
> > To Whom It May Concern
> >
> > Every kernel I have built since 5.3.0-rc2-next-20190730 and up to
> > 5.3.0-rc7-next-20190903 has resulted in the kernel panic described below.
> >
> > The panic occurs early on in the boot process, so no records of it get
> > written on disk. I resourted to taking photos and videos to get the info
> > for debugging.
> >
> > [Kernel panic]
> > Code: 00 48 83 bb f0 00 00 00 00 74 16 48 83 c3 18 b9 17 00 00 00 31 c0 48 
> > 89 df f3 48 ab 5b 41 5c 5d c3 4c 89 a3 f0 00 00 00 eb e1 <0f> 0b 0f 1f 40 
> > 00 55 48 89 e5 41 54 49 89 d4 53 48 89 f3 e8 7e ff
> >
> > Kernel panic - Not syncing: Attempted to kill init! exitcode=0x000b.
> >
> > Top of call stack:
> > __drm_fb_helper_initial_config_and_unlock
> > drm_fb_helper_initial_config
> >
> >  > Code: 00 48 83 bb f0 00 00 00 00 74 16 48 83 c3 18 b9 17 00 00 00 31 c0 48 
> > 89 df f3 48 ab 5b 41 5c 5d c3 4c 89 a3 f0 00 00 00 eb e1 <0f> 0b 0f 1f 40 
> > 00 55 48 89 e5 41 54 49 89 d4 53 48 89 f3 e8 7e ff
> > All code
> > 
> >0:   00 48 83add%cl,-0x7d(%rax)
> >3:   bb f0 00 00 00  mov$0xf0,%ebx
> >8:   00 74 16 48 add%dh,0x48(%rsi,%rdx,1)
> >c:   83 c3 18add$0x18,%ebx
> >f:   b9 17 00 00 00  mov$0x17,%ecx
> >   14:   31 c0   xor%eax,%eax
> >   16:   48 89 dfmov%rbx,%rdi
> >   19:   f3 48 abrep stos %rax,%es:(%rdi)
> >   1c:   5b  pop%rbx
> >   1d:   41 5c   pop%r12
> >   1f:   5d  pop%rbp
> >   20:   c3  retq
> >   21:   4c 89 a3 f0 00 00 00mov%r12,0xf0(%rbx)
> >   28:   eb e1   jmp0xb
> >   2a:*  0f 0b   ud2 <-- trapping instruction
> >   2c:   0f 1f 40 00 nopl   0x0(%rax)
> >   30:   55  push   %rbp
> >   31:   48 89 e5mov%rsp,%rbp
> >   34:   41 54   push   %r12
> >   36:   49 89 d4mov%rdx,%r12
> >   39:   53  push   %rbx
> >   3a:   48 89 f3mov%rsi,%rbx
> >   3d:   e8  .byte 0xe8
> >   3e:   7e ff   jle0x3f
> >
> > Code starting with the faulting instruction
> > ===
> >0:   0f 0b   ud2
> >2:   0f 1f 40 00 nopl   0x0(%rax)
> >6:   55  push   %rbp
> >7:   48 89 e5mov%rsp,%rbp
> >a:   41 54   push   %r12
> >c:   49 89 d4mov%rdx,%r12
> >f:   53  push   %rbx
> >   10:   48 89 f3mov%rsi,%rbx
> >   13:   e8  .byte 0xe8
> >   14:   7e ff   jle0x15
> >
> > The panic occurs after the 'Driver supports precise vblank timestamp
> > query.' line gets printed to console:
> > [2.858970] Linux agpgart interface v0.103
> > [2.859308] nouveau :01:00.0: NVIDIA G84 (084300a2)
> > [2.968950] nouveau :01:00.0: bios: version 60.84.68.00.19
> > [2.989923] nouveau :01:00.0: bios: M0203T not found
> > [2.990010] nouveau :01:00.0: bios: M0203E not matched!
> > [2.990096] nouveau :01:00.0: fb: 512 MiB DDR2
> > [3.062362] [TTM] Zone  kernel: Available graphics memory: 2015014 KiB
> > [3.062494] [TTM] Initializing pool allocator
> > [3.062581] [TTM] Initializing DMA pool allocator
> > [3.062683] nouveau :01:00.0: DRM: VRAM: 512 MiB
> > [3.062769] nouveau :01:00.0: DRM: GART: 1048576 MiB
> > [3.062859] nouveau :01:00.0: DRM: TMDS table version 2.0
> > [3.062944] nouveau :01:00.0: DRM: DCB version 4.0
> > [3.063030] nouveau :01:00.0: DRM: DCB outp 00: 02000300 0028
> > [3.063117] nouveau :01:00.0: DRM: DCB outp 01: 01000302 0030
> > [3.063203] nouveau :01:00.0: DRM: DCB outp 02: 04011310 0028
> > [3.063290] nouveau :01:00.0: DRM: DCB outp 03: 02011312 00c000b0
> > [3.063377] nouveau :01:00.0: DRM: DCB conn 00: 1030
> > [3.063462] nouveau :01:00.0: DRM: DCB conn 01: 2130
> > [3.065982] nouveau :01:00.0: DRM: MM: using CRYPT for buff

Re: [PATCH AUTOSEL 4.19 06/42] scripts/sphinx-pre-install: fix script for RHEL/CentOS

[Alexander Kapshuk]

On Sat, Aug 3, 2019 at 1:37 PM Mauro Carvalho Chehab
 wrote:
>
> Em Sat, 3 Aug 2019 13:31:30 +0300
> Alexander Kapshuk  escreveu:
>
> > > -   if (! $system_release =~ /Fedora/) {
> > > +   if (!($system_release =~ /Fedora/)) {
> > > $map{"virtualenv"} = "python-virtualenv";
> > > }
> >
> > The negated binding operator '!~' could be used here as well, and it
> > does not require the use of extra parenthesis.
> > Just a thought.
>
> Thanks for the tip! Never used such operator. Will start using
> next time we need to handle a pattern like that.
>
> Thanks,
> Mauro

No worries at all.

Re: [PATCH AUTOSEL 4.19 06/42] scripts/sphinx-pre-install: fix script for RHEL/CentOS

[Alexander Kapshuk]

On Sat, Aug 3, 2019 at 11:19 AM Sasha Levin  wrote:
>
> From: Mauro Carvalho Chehab 
>
> [ Upstream commit b308467c916aa7acc5069802ab76a9f657434701 ]
>
> There's a missing parenthesis at the script, with causes it to
> fail to detect non-Fedora releases (e. g. RHEL/CentOS).
>
> Tested with Centos 7.6.1810.
>
> Signed-off-by: Mauro Carvalho Chehab 
> Signed-off-by: Sasha Levin 
> ---
>  scripts/sphinx-pre-install | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/sphinx-pre-install b/scripts/sphinx-pre-install
> index 067459760a7b0..3524dbc313163 100755
> --- a/scripts/sphinx-pre-install
> +++ b/scripts/sphinx-pre-install
> @@ -301,7 +301,7 @@ sub give_redhat_hints()
> #
> # Checks valid for RHEL/CentOS version 7.x.
> #
> -   if (! $system_release =~ /Fedora/) {
> +   if (!($system_release =~ /Fedora/)) {
> $map{"virtualenv"} = "python-virtualenv";
> }
>
> --
> 2.20.1
>

The negated binding operator '!~' could be used here as well, and it
does not require the use of extra parenthesis.
Just a thought.

Re: Linux 5.2-rc1 - module name conflict

[Alexander Kapshuk]

On Tue, May 21, 2019 at 4:01 AM Shuah Khan  wrote:
>
> Hi all,
>
> I am seeing the following warning on Linux 5.2-rc1
>
> warning: same basename if the following are built as modules:
>drivers/regulator/88pm800.ko
>drivers/mfd/88pm800.ko
>
> My config has:
>
> CONFIG_MFD_88PM800=m
>
> CONFIG_REGULATOR_88PM800=m
>
> I don't recall seeing this warning before.

Commit 3a48a91901c516a46a3406ea576798538a8d94d2 introduced a new,
script scripts/modules-check.sh, that issued the warning in question.
See https://lkml.org/lkml/2019/5/17/419.

>
> Anyway just to let you know that there is a conflict in module naming.
>
> thanks,
> -- Shuah

Re: [PATCH v2] kbuild: check uniqueness of module names

[Alexander Kapshuk]

On Fri, May 17, 2019 at 11:58 AM Bernd Petrovitsch
 wrote:
>
> On 17/05/2019 10:16, Alexander Kapshuk wrote:
> [...]
> > The 'xargs' '-r' flag is a GNU extension.
> > If POSIX compliance is important here, the use of 'cat', 'xargs' and
> > 'basename' may be substituted with that of 'sed' to initialise
> > same_name_modules:
> > sed 's!.*/!!' modules.order modules.builtin | sort | uniq -d
>
> 's!' is TTBOMK also a GNU-extension:
> sed 's/.*\///' modules.order modules.builtin | sort | uniq -d

It isn't.
Here's an excerpt from the POSIX manpage for 'sed',
http://pubs.opengroup.org/onlinepubs/009695399/utilities/sed.html:
[2addr]s/BRE/replacement/flags
...  Any character other than backslash or  can be used
instead of a slash to delimit the BRE and the replacement

>
> > 'Sed' may also be used on its own in the 'for' loop instead of as part
> > of a pipeline along with 'grep' to generate the desired output:
> > sed '/\/'$m'/!d;s:^kernel/:  :' modules.order modules.builtin
>
> sed "/\/${m}/!d;s/^kernel\//  /" modules.order modules.builtin

The parameter expansion syntax is redundant here.
See 
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02:
The parameter name or symbol can be enclosed in braces, which are
optional except for positional parameters with more than one digit or
when parameter is a name and is followed by a character that could be
interpreted as part of the name.

Here's an alternative version using double quotes.
sed "/\/$m/!d;s:^kernel/:  :" modules.order modules.builtin

>
> MfG,
> Bernd
> --
> Bernd Petrovitsch  Email : be...@petrovitsch.priv.at
>  LUGA : http://www.luga.at

Re: [PATCH v2] kbuild: check uniqueness of module names

[Alexander Kapshuk]

On Fri, May 17, 2019 at 8:33 AM Masahiro Yamada
 wrote:
>
> In the recent build test of linux-next, Stephen saw a build error
> caused by a broken .tmp_versions/*.mod file:
>
>   https://lkml.org/lkml/2019/5/13/991
>
> drivers/net/phy/asix.ko and drivers/net/usb/asix.ko have the same
> basename, and there is a race in generating .tmp_versions/asix.mod
>
> Kbuild has not checked this before, and it suddenly shows up with
> obscure error message when this kind of race occurs.
>
> Non-unique module names cause various sort of problems, but it is
> not trivial to catch them by eyes.
>
> Hence, this script.
>
> It checks not only real modules, but also built-in modules (i.e.
> controlled by tristate CONFIG option, but currently compiled with =y).
> Non-unique names for built-in modules also cause problems because
> /sys/modules/ would fall over.
>
> I tested allmodconfig on the latest kernel, and it detected the
> following:
>
> warning: same basename if the following are built as modules:
>   drivers/regulator/88pm800.ko
>   drivers/mfd/88pm800.ko
> warning: same basename if the following are built as modules:
>   drivers/gpu/drm/bridge/adv7511/adv7511.ko
>   drivers/media/i2c/adv7511.ko
> warning: same basename if the following are built as modules:
>   drivers/net/phy/asix.ko
>   drivers/net/usb/asix.ko
> warning: same basename if the following are built as modules:
>   fs/coda/coda.ko
>   drivers/media/platform/coda/coda.ko
> warning: same basename if the following are built as modules:
>   drivers/net/phy/realtek.ko
>   drivers/net/dsa/realtek.ko
>
> Reported-by: Stephen Rothwell 
> Signed-off-by: Masahiro Yamada 
> Reviewed-by: Kees Cook 
> ---
>
> Changes in v2:
>  - redirect messages to stderr
>  - use '--' after 'basename -a'
>  - use '-r' for xargs to cope with empty modules.order/modules.builtin
>
>  Makefile |  1 +
>  scripts/modules-check.sh | 20 
>  2 files changed, 21 insertions(+)
>  create mode 100755 scripts/modules-check.sh
>
> diff --git a/Makefile b/Makefile
> index a61a95b6b38f..30792fec7a12 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1290,6 +1290,7 @@ modules: $(vmlinux-dirs) $(if 
> $(KBUILD_BUILTIN),vmlinux) modules.builtin
> $(Q)$(AWK) '!x[$$0]++' $(vmlinux-dirs:%=$(objtree)/%/modules.order) > 
> $(objtree)/modules.order
> @$(kecho) '  Building modules, stage 2.';
> $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
> +   $(Q)$(CONFIG_SHELL) $(srctree)/scripts/modules-check.sh
>
>  modules.builtin: $(vmlinux-dirs:%=%/modules.builtin)
> $(Q)$(AWK) '!x[$$0]++' $^ > $(objtree)/modules.builtin
> diff --git a/scripts/modules-check.sh b/scripts/modules-check.sh
> new file mode 100755
> index ..c875f6eab01e
> --- /dev/null
> +++ b/scripts/modules-check.sh
> @@ -0,0 +1,20 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +# Check uniqueness of module names
> +check_same_name_modules()
> +{
> +   same_name_modules=$(cat modules.order modules.builtin | \
> +   xargs -r basename -a -- | sort | uniq -d)
> +
> +   for m in $same_name_modules
> +   do
> +   echo "warning: same basename if the following are built as 
> modules:" >&2
> +   grep -h -e "/$m" modules.order modules.builtin | \
> +   sed 's:^kernel/:  :' >&2
> +   done
> +}
> +
> +check_same_name_modules
> --
> 2.17.1
>

The 'xargs' '-r' flag is a GNU extension.
If POSIX compliance is important here, the use of 'cat', 'xargs' and
'basename' may be substituted with that of 'sed' to initialise
same_name_modules:
sed 's!.*/!!' modules.order modules.builtin | sort | uniq -d

'Sed' may also be used on its own in the 'for' loop instead of as part
of a pipeline along with 'grep' to generate the desired output:
sed '/\/'$m'/!d;s:^kernel/:  :' modules.order modules.builtin

Just a suggestion.

[PATCH] ver_linux: Assign constant RE to variable name for clarity

[Alexander Kapshuk]

The regular expression that matches the version number of a utility
being queried is used as a constant expression in the current
implementation. Assigning the RE in question to a variable gives it a
meaningful name that clearly expresses the intended use of the expression
without having to think about the details of implementation.

Signed-off-by: Alexander Kapshuk 
---
 scripts/ver_linux | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index a6c728db05ce..810e608baa24 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -13,6 +13,8 @@ BEGIN {
system("uname -a")
printf("\n")
 
+   vernum = "[0-9]+([.]?[0-9]+)+"
+
printversion("GNU C", version("gcc -dumpversion"))
printversion("GNU Make", version("make --version"))
printversion("Binutils", version("ld -v"))
@@ -34,7 +36,7 @@ BEGIN {
while (getline <"/proc/self/maps" > 0) {
if (/libc.*\.so$/) {
n = split($0, procmaps, "/")
-   if (match(procmaps[n], /[0-9]+([.]?[0-9]+)+/)) {
+   if (match(procmaps[n], vernum)) {
ver = substr(procmaps[n], RSTART, RLENGTH)
printversion("Linux C Library", ver)
break
@@ -70,7 +72,7 @@ BEGIN {
 function version(cmd,ver) {
cmd = cmd " 2>&1"
while (cmd | getline > 0) {
-   if (match($0, /[0-9]+([.]?[0-9]+)+/)) {
+   if (match($0, vernum)) {
ver = substr($0, RSTART, RLENGTH)
break
}
-- 
2.20.1

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Tue, Nov 13, 2018 at 8:32 PM Brian Norris  wrote:
>
> Hi Alexander,
>
> On Tue, Nov 13, 2018 at 12:36 AM Alexander Kapshuk
>  wrote:
> >
> > On Tue, Nov 13, 2018 at 2:09 AM Brian Norris  
> > wrote:
> > >
> > > On Mon, Nov 12, 2018 at 10:42:26AM +0200, Alexander Kapshuk wrote:
> > > > An even simpler approach would be:
> > > >
> > > > {
> > > > git --no-optional-locks status -uno --porcelain 2>/dev/null ||
> > > > git diff-index --name-only HEAD
> > > > } | grep -qv scripts/package &&
> > > > printf '%s' -dirty
> > > >
> > > > Sample run:
> > > > cmd
> > > > sh: cmd: command not found
> > > >
> > > > {
> > > > cmd 2>/dev/null ||
> > > > date
> > > > } | grep -q 2018 &&
> > > > printf '%s' ok
> > > > ok
> > >
> > > You lose accuracy here, because now you're skipping any line that
> > > contains 'scripts/package', which would include, e.g., paths like
> > >
> > >   tools/some/other-scripts/package
> > >
> > > Maybe if the grep expression were more like this?
> > >
> > >   grep -qv '^\(.. \)\?scripts/package'
> > >
> > > I think it'd be safe enough to ignore paths that start with two
> > > characters and a space, like:
> > >
> > > xy scripts/package
> > > x/ scripts/package
> > >
> > > Brian
> >
> > Thanks for your input.
> > I've found the following grep command, that uses extended regular
> > expressions, to work as required:
>
> Is there any good reason you switched to extended? It looks like my
> (basic regex) grep was equivalent.
>
> > {
> > echo hello
> > echo scripts/package
> > echo '.. scripts/package'
> > echo tools/some/other-scripts/package
> > } | grep -Ev '^(.. )?scripts/package'
> >
> > [Output]
> > hello
> > tools/some/other-scripts/package
> >
> > If the participants of this email exchange consider the proposed
> > implementation as fitting the bill,
> >
> > {
> > git --no-optional-locks status -uno --porcelain 2>/dev/null ||
> > git diff-index --name-only HEAD
> > } | grep -Eqv '^(.. )?scripts/package' &&
> > printf '%s' -dirty
> >
> > Was the original committer of the patch proposed here,
> > https://lkml.org/lkml/2018/11/10/55, going to take it in, and resend it
> > as v2 of the patch, or did you want me to submit the patch instead?
> > I would be happy with either way.
>
> I can submit it. I expect Masahiro-san would prefer a proper v2 patch
> for review, given how much would change from my v1.
>
> And this time, I'll actually test it with a non-dirty tree! (Of
> course, my tree is naturally dirty when developing the patch, but I
> missed testing post-commit...)
>
> Brian

Hi Brian,

Thanks for taking my proposed patch in and submitting it.

I went with the ERE because, in my opinion, the pattern looks
'cleaner' without having the metacharacters like '()?' escaped.
EREs are POSIX-compatible, so should be supported by any POSIX-compliant shell.

Your non-ERE version does generate the same output as my ERE one. So
it just boils down to one's personal preference.

Thanks.

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Tue, Nov 13, 2018 at 8:32 PM Brian Norris  wrote:
>
> Hi Alexander,
>
> On Tue, Nov 13, 2018 at 12:36 AM Alexander Kapshuk
>  wrote:
> >
> > On Tue, Nov 13, 2018 at 2:09 AM Brian Norris  
> > wrote:
> > >
> > > On Mon, Nov 12, 2018 at 10:42:26AM +0200, Alexander Kapshuk wrote:
> > > > An even simpler approach would be:
> > > >
> > > > {
> > > > git --no-optional-locks status -uno --porcelain 2>/dev/null ||
> > > > git diff-index --name-only HEAD
> > > > } | grep -qv scripts/package &&
> > > > printf '%s' -dirty
> > > >
> > > > Sample run:
> > > > cmd
> > > > sh: cmd: command not found
> > > >
> > > > {
> > > > cmd 2>/dev/null ||
> > > > date
> > > > } | grep -q 2018 &&
> > > > printf '%s' ok
> > > > ok
> > >
> > > You lose accuracy here, because now you're skipping any line that
> > > contains 'scripts/package', which would include, e.g., paths like
> > >
> > >   tools/some/other-scripts/package
> > >
> > > Maybe if the grep expression were more like this?
> > >
> > >   grep -qv '^\(.. \)\?scripts/package'
> > >
> > > I think it'd be safe enough to ignore paths that start with two
> > > characters and a space, like:
> > >
> > > xy scripts/package
> > > x/ scripts/package
> > >
> > > Brian
> >
> > Thanks for your input.
> > I've found the following grep command, that uses extended regular
> > expressions, to work as required:
>
> Is there any good reason you switched to extended? It looks like my
> (basic regex) grep was equivalent.
>
> > {
> > echo hello
> > echo scripts/package
> > echo '.. scripts/package'
> > echo tools/some/other-scripts/package
> > } | grep -Ev '^(.. )?scripts/package'
> >
> > [Output]
> > hello
> > tools/some/other-scripts/package
> >
> > If the participants of this email exchange consider the proposed
> > implementation as fitting the bill,
> >
> > {
> > git --no-optional-locks status -uno --porcelain 2>/dev/null ||
> > git diff-index --name-only HEAD
> > } | grep -Eqv '^(.. )?scripts/package' &&
> > printf '%s' -dirty
> >
> > Was the original committer of the patch proposed here,
> > https://lkml.org/lkml/2018/11/10/55, going to take it in, and resend it
> > as v2 of the patch, or did you want me to submit the patch instead?
> > I would be happy with either way.
>
> I can submit it. I expect Masahiro-san would prefer a proper v2 patch
> for review, given how much would change from my v1.
>
> And this time, I'll actually test it with a non-dirty tree! (Of
> course, my tree is naturally dirty when developing the patch, but I
> missed testing post-commit...)
>
> Brian

Hi Brian,

Thanks for taking my proposed patch in and submitting it.

I went with the ERE because, in my opinion, the pattern looks
'cleaner' without having the metacharacters like '()?' escaped.
EREs are POSIX-compatible, so should be supported by any POSIX-compliant shell.

Your non-ERE version does generate the same output as my ERE one. So
it just boils down to one's personal preference.

Thanks.

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Tue, Nov 13, 2018 at 2:09 AM Brian Norris  wrote:
>
> On Mon, Nov 12, 2018 at 10:42:26AM +0200, Alexander Kapshuk wrote:
> > An even simpler approach would be:
> >
> > {
> > git --no-optional-locks status -uno --porcelain 2>/dev/null ||
> > git diff-index --name-only HEAD
> > } | grep -qv scripts/package &&
> > printf '%s' -dirty
> >
> > Sample run:
> > cmd
> > sh: cmd: command not found
> >
> > {
> > cmd 2>/dev/null ||
> > date
> > } | grep -q 2018 &&
> > printf '%s' ok
> > ok
>
> You lose accuracy here, because now you're skipping any line that
> contains 'scripts/package', which would include, e.g., paths like
>
>   tools/some/other-scripts/package
>
> Maybe if the grep expression were more like this?
>
>   grep -qv '^\(.. \)\?scripts/package'
>
> I think it'd be safe enough to ignore paths that start with two
> characters and a space, like:
>
> xy scripts/package
> x/ scripts/package
>
> Brian

Thanks for your input.
I've found the following grep command, that uses extended regular
expressions, to work as required:

{
echo hello
echo scripts/package
echo '.. scripts/package'
echo tools/some/other-scripts/package
} | grep -Ev '^(.. )?scripts/package'

[Output]
hello
tools/some/other-scripts/package

If the participants of this email exchange consider the proposed
implementation as fitting the bill,

{
git --no-optional-locks status -uno --porcelain 2>/dev/null ||
git diff-index --name-only HEAD
} | grep -Eqv '^(.. )?scripts/package' &&
printf '%s' -dirty

Was the original committer of the patch proposed here,
https://lkml.org/lkml/2018/11/10/55, going to take it in, and resend it
as v2 of the patch, or did you want me to submit the patch instead?
I would be happy with either way.

Thanks.

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Tue, Nov 13, 2018 at 2:09 AM Brian Norris  wrote:
>
> On Mon, Nov 12, 2018 at 10:42:26AM +0200, Alexander Kapshuk wrote:
> > An even simpler approach would be:
> >
> > {
> > git --no-optional-locks status -uno --porcelain 2>/dev/null ||
> > git diff-index --name-only HEAD
> > } | grep -qv scripts/package &&
> > printf '%s' -dirty
> >
> > Sample run:
> > cmd
> > sh: cmd: command not found
> >
> > {
> > cmd 2>/dev/null ||
> > date
> > } | grep -q 2018 &&
> > printf '%s' ok
> > ok
>
> You lose accuracy here, because now you're skipping any line that
> contains 'scripts/package', which would include, e.g., paths like
>
>   tools/some/other-scripts/package
>
> Maybe if the grep expression were more like this?
>
>   grep -qv '^\(.. \)\?scripts/package'
>
> I think it'd be safe enough to ignore paths that start with two
> characters and a space, like:
>
> xy scripts/package
> x/ scripts/package
>
> Brian

Thanks for your input.
I've found the following grep command, that uses extended regular
expressions, to work as required:

{
echo hello
echo scripts/package
echo '.. scripts/package'
echo tools/some/other-scripts/package
} | grep -Ev '^(.. )?scripts/package'

[Output]
hello
tools/some/other-scripts/package

If the participants of this email exchange consider the proposed
implementation as fitting the bill,

{
git --no-optional-locks status -uno --porcelain 2>/dev/null ||
git diff-index --name-only HEAD
} | grep -Eqv '^(.. )?scripts/package' &&
printf '%s' -dirty

Was the original committer of the patch proposed here,
https://lkml.org/lkml/2018/11/10/55, going to take it in, and resend it
as v2 of the patch, or did you want me to submit the patch instead?
I would be happy with either way.

Thanks.

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sun, Nov 11, 2018 at 9:59 PM Alexander Kapshuk
 wrote:
>
> On Sun, Nov 11, 2018 at 7:41 PM Genki Sky  wrote:
> >
> > Hi Alexander,
> >
> > On Sun, 11 Nov 2018 16:48:38 +0200, Alexander Kapshuk 
> >  wrote:
> > > Piping the output of the git command to grep and using the return status
> > > of grep as the test condition within the if block, would be sufficient
> > > to determine whether or not '-dirty' should be printed.
> > >
> > > Sample run:
> > > % if git --no-optional-locks \
> > > status -uno --porcelain \
> > > 2>/dev/null |
> > > grep -qv '^.. scripts/package'
> > > then
> > > printf '%s' -dirty
> > > fi
> >
> > I don't think this works well for us. We need to check whether
> > --no-optional-locks is available before using the output to determine
> > whether the tree is dirty or not. If it's not available, we have to
> > fall back on diff-index. Let me know if I'm misreading you.
>
> It was I who failed to read the proposed patch in its entirety in the
> first place. I did not get the full picture as a result. My apologies.
>
> Would something like this work for you?
>
> local git_status
> git_status=$(git --no-optional-locks status -uno --porcelain 2>/dev/null)
> test $? -eq 0 ||
> git_status=$(git diff-index --name-only HEAD)
> if printf '%s' "$git_status"  | grep -qv scripts/package; then
> printf '%s' -dirty
> fi

An even simpler approach would be:

{
git --no-optional-locks status -uno --porcelain 2>/dev/null ||
git diff-index --name-only HEAD
} | grep -qv scripts/package &&
printf '%s' -dirty

Sample run:
cmd
sh: cmd: command not found

{
cmd 2>/dev/null ||
date
} | grep -q 2018 &&
printf '%s' ok
ok

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sun, Nov 11, 2018 at 9:59 PM Alexander Kapshuk
 wrote:
>
> On Sun, Nov 11, 2018 at 7:41 PM Genki Sky  wrote:
> >
> > Hi Alexander,
> >
> > On Sun, 11 Nov 2018 16:48:38 +0200, Alexander Kapshuk 
> >  wrote:
> > > Piping the output of the git command to grep and using the return status
> > > of grep as the test condition within the if block, would be sufficient
> > > to determine whether or not '-dirty' should be printed.
> > >
> > > Sample run:
> > > % if git --no-optional-locks \
> > > status -uno --porcelain \
> > > 2>/dev/null |
> > > grep -qv '^.. scripts/package'
> > > then
> > > printf '%s' -dirty
> > > fi
> >
> > I don't think this works well for us. We need to check whether
> > --no-optional-locks is available before using the output to determine
> > whether the tree is dirty or not. If it's not available, we have to
> > fall back on diff-index. Let me know if I'm misreading you.
>
> It was I who failed to read the proposed patch in its entirety in the
> first place. I did not get the full picture as a result. My apologies.
>
> Would something like this work for you?
>
> local git_status
> git_status=$(git --no-optional-locks status -uno --porcelain 2>/dev/null)
> test $? -eq 0 ||
> git_status=$(git diff-index --name-only HEAD)
> if printf '%s' "$git_status"  | grep -qv scripts/package; then
> printf '%s' -dirty
> fi

An even simpler approach would be:

{
git --no-optional-locks status -uno --porcelain 2>/dev/null ||
git diff-index --name-only HEAD
} | grep -qv scripts/package &&
printf '%s' -dirty

Sample run:
cmd
sh: cmd: command not found

{
cmd 2>/dev/null ||
date
} | grep -q 2018 &&
printf '%s' ok
ok

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sun, Nov 11, 2018 at 7:41 PM Genki Sky  wrote:
>
> Hi Alexander,
>
> On Sun, 11 Nov 2018 16:48:38 +0200, Alexander Kapshuk 
>  wrote:
> > Piping the output of the git command to grep and using the return status
> > of grep as the test condition within the if block, would be sufficient
> > to determine whether or not '-dirty' should be printed.
> >
> > Sample run:
> > % if git --no-optional-locks \
> > status -uno --porcelain \
> > 2>/dev/null |
> > grep -qv '^.. scripts/package'
> > then
> > printf '%s' -dirty
> > fi
>
> I don't think this works well for us. We need to check whether
> --no-optional-locks is available before using the output to determine
> whether the tree is dirty or not. If it's not available, we have to
> fall back on diff-index. Let me know if I'm misreading you.

It was I who failed to read the proposed patch in its entirety in the
first place. I did not get the full picture as a result. My apologies.

Would something like this work for you?

local git_status
git_status=$(git --no-optional-locks status -uno --porcelain 2>/dev/null)
test $? -eq 0 ||
git_status=$(git diff-index --name-only HEAD)
if printf '%s' "$git_status"  | grep -qv scripts/package; then
printf '%s' -dirty
fi

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sun, Nov 11, 2018 at 7:41 PM Genki Sky  wrote:
>
> Hi Alexander,
>
> On Sun, 11 Nov 2018 16:48:38 +0200, Alexander Kapshuk 
>  wrote:
> > Piping the output of the git command to grep and using the return status
> > of grep as the test condition within the if block, would be sufficient
> > to determine whether or not '-dirty' should be printed.
> >
> > Sample run:
> > % if git --no-optional-locks \
> > status -uno --porcelain \
> > 2>/dev/null |
> > grep -qv '^.. scripts/package'
> > then
> > printf '%s' -dirty
> > fi
>
> I don't think this works well for us. We need to check whether
> --no-optional-locks is available before using the output to determine
> whether the tree is dirty or not. If it's not available, we have to
> fall back on diff-index. Let me know if I'm misreading you.

It was I who failed to read the proposed patch in its entirety in the
first place. I did not get the full picture as a result. My apologies.

Would something like this work for you?

local git_status
git_status=$(git --no-optional-locks status -uno --porcelain 2>/dev/null)
test $? -eq 0 ||
git_status=$(git diff-index --name-only HEAD)
if printf '%s' "$git_status"  | grep -qv scripts/package; then
printf '%s' -dirty
fi

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sun, Nov 11, 2018 at 4:48 PM Alexander Kapshuk
 wrote:
>
> On Sat, Nov 10, 2018 at 10:12 PM Genki Sky  wrote:
> >
> > Hi Andreas,
> >
> > On Sat, 10 Nov 2018 11:42:11 +0100, Andreas Schwab  
> > wrote:
> > > On Nov 10 2018, Genki Sky  wrote:
> > > > On Fri, 9 Nov 2018 10:34:37 -0800, Brian Norris 
> > > >  wrote:
> > > >> +  git_status="$(git --no-optional-locks status -uno 
> > > >> --porcelain 2>/dev/null)"
> > > >> +  if [ $? -eq 0 ]; then
> > > >> +  if echo "$git_status" | grep -qv '^.. 
> > > >> scripts/package'; then
> > > >
> > > > Shouldn't this be:
> > > >
> > > > if printf '%s' "$git_status" | grep -qv '^.. scripts/package'; then
> > > >
> > > > I.e., use printf not echo? Because of echo introducing a newline.
> > >
> > > The input to grep should be a text file, thus should end with a newline.
> >
> > Ah okay, thanks. I guess GNU grep was being lenient. Well then, I
> > think the line at least needs to be changed to:
> >
> > if [ -n "$git_status" ] && echo "$git_status" | grep -qv '^.. 
> > scripts/package'; then
> >
> > I'm just trying to say that in the proposed patch, if git doesn't
> > print anything, the echo adds a newline that wasn't there before. This
> > causes the grep -qv to exit with status 0 (because there's at least
> > one line that doesn't contain '^.. scripts/package'). Meaning it will
> > print dirty.
>
> Piping the output of the git command to grep and using the return status
> of grep as the test condition within the if block, would be sufficient
> to determine whether or not '-dirty' should be printed.
>
> Sample run:
> % if git --no-optional-locks \
> status -uno --porcelain \
> 2>/dev/null |
> grep -qv '^.. scripts/package'
> then
> printf '%s' -dirty
> fi
> %

To improve the readability, the git command may be stored in a variable
and substituted into a command within the if block like so:

% cmd='git
--no-optional-locks
status
-uno
--porcelain'

% if $cmd 2>/dev/null | grep -qv '^.. scripts/package'; then
printf '%s' -dirty
fi
%

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sun, Nov 11, 2018 at 4:48 PM Alexander Kapshuk
 wrote:
>
> On Sat, Nov 10, 2018 at 10:12 PM Genki Sky  wrote:
> >
> > Hi Andreas,
> >
> > On Sat, 10 Nov 2018 11:42:11 +0100, Andreas Schwab  
> > wrote:
> > > On Nov 10 2018, Genki Sky  wrote:
> > > > On Fri, 9 Nov 2018 10:34:37 -0800, Brian Norris 
> > > >  wrote:
> > > >> +  git_status="$(git --no-optional-locks status -uno 
> > > >> --porcelain 2>/dev/null)"
> > > >> +  if [ $? -eq 0 ]; then
> > > >> +  if echo "$git_status" | grep -qv '^.. 
> > > >> scripts/package'; then
> > > >
> > > > Shouldn't this be:
> > > >
> > > > if printf '%s' "$git_status" | grep -qv '^.. scripts/package'; then
> > > >
> > > > I.e., use printf not echo? Because of echo introducing a newline.
> > >
> > > The input to grep should be a text file, thus should end with a newline.
> >
> > Ah okay, thanks. I guess GNU grep was being lenient. Well then, I
> > think the line at least needs to be changed to:
> >
> > if [ -n "$git_status" ] && echo "$git_status" | grep -qv '^.. 
> > scripts/package'; then
> >
> > I'm just trying to say that in the proposed patch, if git doesn't
> > print anything, the echo adds a newline that wasn't there before. This
> > causes the grep -qv to exit with status 0 (because there's at least
> > one line that doesn't contain '^.. scripts/package'). Meaning it will
> > print dirty.
>
> Piping the output of the git command to grep and using the return status
> of grep as the test condition within the if block, would be sufficient
> to determine whether or not '-dirty' should be printed.
>
> Sample run:
> % if git --no-optional-locks \
> status -uno --porcelain \
> 2>/dev/null |
> grep -qv '^.. scripts/package'
> then
> printf '%s' -dirty
> fi
> %

To improve the readability, the git command may be stored in a variable
and substituted into a command within the if block like so:

% cmd='git
--no-optional-locks
status
-uno
--porcelain'

% if $cmd 2>/dev/null | grep -qv '^.. scripts/package'; then
printf '%s' -dirty
fi
%

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sat, Nov 10, 2018 at 10:12 PM Genki Sky  wrote:
>
> Hi Andreas,
>
> On Sat, 10 Nov 2018 11:42:11 +0100, Andreas Schwab  
> wrote:
> > On Nov 10 2018, Genki Sky  wrote:
> > > On Fri, 9 Nov 2018 10:34:37 -0800, Brian Norris 
> > >  wrote:
> > >> +  git_status="$(git --no-optional-locks status -uno --porcelain 
> > >> 2>/dev/null)"
> > >> +  if [ $? -eq 0 ]; then
> > >> +  if echo "$git_status" | grep -qv '^.. 
> > >> scripts/package'; then
> > >
> > > Shouldn't this be:
> > >
> > > if printf '%s' "$git_status" | grep -qv '^.. scripts/package'; then
> > >
> > > I.e., use printf not echo? Because of echo introducing a newline.
> >
> > The input to grep should be a text file, thus should end with a newline.
>
> Ah okay, thanks. I guess GNU grep was being lenient. Well then, I
> think the line at least needs to be changed to:
>
> if [ -n "$git_status" ] && echo "$git_status" | grep -qv '^.. 
> scripts/package'; then
>
> I'm just trying to say that in the proposed patch, if git doesn't
> print anything, the echo adds a newline that wasn't there before. This
> causes the grep -qv to exit with status 0 (because there's at least
> one line that doesn't contain '^.. scripts/package'). Meaning it will
> print dirty.

Piping the output of the git command to grep and using the return status
of grep as the test condition within the if block, would be sufficient
to determine whether or not '-dirty' should be printed.

Sample run:
% if git --no-optional-locks \
status -uno --porcelain \
2>/dev/null |
grep -qv '^.. scripts/package'
then
printf '%s' -dirty
fi
%

Re: [PATCH] scripts/setlocalversion: Improve -dirty check with git-status --no-optional-locks

[Alexander Kapshuk]

On Sat, Nov 10, 2018 at 10:12 PM Genki Sky  wrote:
>
> Hi Andreas,
>
> On Sat, 10 Nov 2018 11:42:11 +0100, Andreas Schwab  
> wrote:
> > On Nov 10 2018, Genki Sky  wrote:
> > > On Fri, 9 Nov 2018 10:34:37 -0800, Brian Norris 
> > >  wrote:
> > >> +  git_status="$(git --no-optional-locks status -uno --porcelain 
> > >> 2>/dev/null)"
> > >> +  if [ $? -eq 0 ]; then
> > >> +  if echo "$git_status" | grep -qv '^.. 
> > >> scripts/package'; then
> > >
> > > Shouldn't this be:
> > >
> > > if printf '%s' "$git_status" | grep -qv '^.. scripts/package'; then
> > >
> > > I.e., use printf not echo? Because of echo introducing a newline.
> >
> > The input to grep should be a text file, thus should end with a newline.
>
> Ah okay, thanks. I guess GNU grep was being lenient. Well then, I
> think the line at least needs to be changed to:
>
> if [ -n "$git_status" ] && echo "$git_status" | grep -qv '^.. 
> scripts/package'; then
>
> I'm just trying to say that in the proposed patch, if git doesn't
> print anything, the echo adds a newline that wasn't there before. This
> causes the grep -qv to exit with status 0 (because there's at least
> one line that doesn't contain '^.. scripts/package'). Meaning it will
> print dirty.

Piping the output of the git command to grep and using the return status
of grep as the test condition within the if block, would be sufficient
to determine whether or not '-dirty' should be printed.

Sample run:
% if git --no-optional-locks \
status -uno --porcelain \
2>/dev/null |
grep -qv '^.. scripts/package'
then
printf '%s' -dirty
fi
%

Re: Inclusion of commit e54192b48da7 into stable tree?

[Alexander Kapshuk]

On Sun, Sep 16, 2018 at 10:11 PM Rob Herring  wrote:
>
> On Sat, Sep 15, 2018 at 8:35 AM Alexander Kapshuk
>  wrote:
> >
> > Is this commit queued up for inclusion into the stable tree?
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/of/base.c?h=next-20180913=e54192b48da75f025ae4b277925eaf6aca1d13bd
>
> It will be. It only landed in Linus' tree on Fri. Any commit with a
> stable tag gets picked up.
>
> Rob

Terrific.
Thanks for the info.

Re: Inclusion of commit e54192b48da7 into stable tree?

[Alexander Kapshuk]

On Sun, Sep 16, 2018 at 10:11 PM Rob Herring  wrote:
>
> On Sat, Sep 15, 2018 at 8:35 AM Alexander Kapshuk
>  wrote:
> >
> > Is this commit queued up for inclusion into the stable tree?
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/of/base.c?h=next-20180913=e54192b48da75f025ae4b277925eaf6aca1d13bd
>
> It will be. It only landed in Linus' tree on Fri. Any commit with a
> stable tag gets picked up.
>
> Rob

Terrific.
Thanks for the info.

Inclusion of commit e54192b48da7 into stable tree?

[Alexander Kapshuk]

Is this commit queued up for inclusion into the stable tree?
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/of/base.c?h=next-20180913=e54192b48da75f025ae4b277925eaf6aca1d13bd

Applying the patch on top of v4.18.8 fixes the booting problem for me.
The contents of /proc/cpuinfo and .config are attached for reference.

Thanks.
processor   : 0
vendor_id   : GenuineIntel
cpu family  : 6
model   : 15
model name  : Intel(R) Pentium(R) Dual  CPU  T3400  @ 2.16GHz
stepping: 13
microcode   : 0xa3
cpu MHz : 997.524
cache size  : 1024 KB
physical id : 0
siblings: 2
core id : 0
cpu cores   : 2
apicid  : 0
initial apicid  : 0
fpu : yes
fpu_exception   : yes
cpuid level : 10
wp  : yes
flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm 
constant_tsc arch_perfmon pebs bts rep_good nopl cpuid aperfmperf pni dtes64 
monitor ds_cpl est tm2 ssse3 cx16 xtpr pdcm lahf_lm pti dtherm
bugs: cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass l1tf
bogomips: 4322.63
clflush size: 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor   : 1
vendor_id   : GenuineIntel
cpu family  : 6
model   : 15
model name  : Intel(R) Pentium(R) Dual  CPU  T3400  @ 2.16GHz
stepping: 13
microcode   : 0xa3
cpu MHz : 997.539
cache size  : 1024 KB
physical id : 0
siblings: 2
core id : 1
cpu cores   : 2
apicid  : 1
initial apicid  : 1
fpu : yes
fpu_exception   : yes
cpuid level : 10
wp  : yes
flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm 
constant_tsc arch_perfmon pebs bts rep_good nopl cpuid aperfmperf pni dtes64 
monitor ds_cpl est tm2 ssse3 cx16 xtpr pdcm lahf_lm pti dtherm
bugs: cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass l1tf
bogomips: 4322.63
clflush size: 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_X86=y
CONFIG_INSTRUCTION_DECODER=y
CONFIG_OUTPUT_FORMAT="elf64-x86-64"
CONFIG_ARCH_DEFCONFIG="arch/x86/configs/x86_64_defconfig"
CONFIG_LOCKDEP_SUPPORT=y
CONFIG_STACKTRACE_SUPPORT=y
CONFIG_MMU=y
CONFIG_ARCH_MMAP_RND_BITS_MIN=28
CONFIG_ARCH_MMAP_RND_BITS_MAX=32
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MIN=8
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MAX=16
CONFIG_GENERIC_ISA_DMA=y
CONFIG_GENERIC_BUG=y
CONFIG_GENERIC_BUG_RELATIVE_POINTERS=y
CONFIG_GENERIC_HWEIGHT=y
CONFIG_ARCH_MAY_HAVE_PC_FDC=y
CONFIG_RWSEM_XCHGADD_ALGORITHM=y
CONFIG_GENERIC_CALIBRATE_DELAY=y
CONFIG_ARCH_HAS_CPU_RELAX=y
CONFIG_ARCH_HAS_CACHE_LINE_SIZE=y
CONFIG_ARCH_HAS_FILTER_PGPROT=y
CONFIG_HAVE_SETUP_PER_CPU_AREA=y
CONFIG_NEED_PER_CPU_EMBED_FIRST_CHUNK=y
CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK=y
CONFIG_ARCH_HIBERNATION_POSSIBLE=y
CONFIG_ARCH_SUSPEND_POSSIBLE=y
CONFIG_ARCH_WANT_HUGE_PMD_SHARE=y
CONFIG_ARCH_WANT_GENERAL_HUGETLB=y
CONFIG_ZONE_DMA32=y
CONFIG_AUDIT_ARCH=y
CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING=y
CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y
CONFIG_HAVE_INTEL_TXT=y
CONFIG_X86_64_SMP=y
CONFIG_ARCH_SUPPORTS_UPROBES=y
CONFIG_FIX_EARLYCON_MEM=y
CONFIG_PGTABLE_LEVELS=4
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=80201
CONFIG_CLANG_VERSION=0
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_EXTABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y
CONFIG_INIT_ENV_ARG_LIMIT=32
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_BZIP2=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
CONFIG_KERNEL_GZIP=y
CONFIG_DEFAULT_HOSTNAME="(none)"
CONFIG_SWAP=y
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
CONFIG_POSIX_MQUEUE=y
CONFIG_POSIX_MQUEUE_SYSCTL=y
CONFIG_CROSS_MEMORY_ATTACH=y
CONFIG_USELIB=y
CONFIG_HAVE_ARCH_AUDITSYSCALL=y
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=y
CONFIG_GENERIC_PENDING_IRQ=y
CONFIG_GENERIC_IRQ_MIGRATION=y
CONFIG_IRQ_DOMAIN=y
CONFIG_IRQ_DOMAIN_HIERARCHY=y
CONFIG_GENERIC_MSI_IRQ=y
CONFIG_GENERIC_MSI_IRQ_DOMAIN=y
CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR=y
CONFIG_GENERIC_IRQ_RESERVATION_MODE=y
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_SPARSE_IRQ=y
CONFIG_CLOCKSOURCE_WATCHDOG=y
CONFIG_ARCH_CLOCKSOURCE_DATA=y
CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE=y
CONFIG_GENERIC_TIME_VSYSCALL=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=y
CONFIG_GENERIC_CMOS_UPDATE=y
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
CONFIG_NO_HZ_IDLE=y
CONFIG_NO_HZ=y
CONFIG_HIGH_RES_TIMERS=y
CONFIG_TICK_CPU_ACCOUNTING=y
CONFIG_BSD_PROCESS_ACCT=y
CONFIG_TASKSTATS=y
CONFIG_TASK_DELAY_ACCT=y
CONFIG_TASK_XACCT=y
CONFIG_TASK_IO_ACCOUNTING=y

Inclusion of commit e54192b48da7 into stable tree?

[Alexander Kapshuk]

Is this commit queued up for inclusion into the stable tree?
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/of/base.c?h=next-20180913=e54192b48da75f025ae4b277925eaf6aca1d13bd

Applying the patch on top of v4.18.8 fixes the booting problem for me.
The contents of /proc/cpuinfo and .config are attached for reference.

Thanks.
processor   : 0
vendor_id   : GenuineIntel
cpu family  : 6
model   : 15
model name  : Intel(R) Pentium(R) Dual  CPU  T3400  @ 2.16GHz
stepping: 13
microcode   : 0xa3
cpu MHz : 997.524
cache size  : 1024 KB
physical id : 0
siblings: 2
core id : 0
cpu cores   : 2
apicid  : 0
initial apicid  : 0
fpu : yes
fpu_exception   : yes
cpuid level : 10
wp  : yes
flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm 
constant_tsc arch_perfmon pebs bts rep_good nopl cpuid aperfmperf pni dtes64 
monitor ds_cpl est tm2 ssse3 cx16 xtpr pdcm lahf_lm pti dtherm
bugs: cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass l1tf
bogomips: 4322.63
clflush size: 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor   : 1
vendor_id   : GenuineIntel
cpu family  : 6
model   : 15
model name  : Intel(R) Pentium(R) Dual  CPU  T3400  @ 2.16GHz
stepping: 13
microcode   : 0xa3
cpu MHz : 997.539
cache size  : 1024 KB
physical id : 0
siblings: 2
core id : 1
cpu cores   : 2
apicid  : 1
initial apicid  : 1
fpu : yes
fpu_exception   : yes
cpuid level : 10
wp  : yes
flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm 
constant_tsc arch_perfmon pebs bts rep_good nopl cpuid aperfmperf pni dtes64 
monitor ds_cpl est tm2 ssse3 cx16 xtpr pdcm lahf_lm pti dtherm
bugs: cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass l1tf
bogomips: 4322.63
clflush size: 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_X86=y
CONFIG_INSTRUCTION_DECODER=y
CONFIG_OUTPUT_FORMAT="elf64-x86-64"
CONFIG_ARCH_DEFCONFIG="arch/x86/configs/x86_64_defconfig"
CONFIG_LOCKDEP_SUPPORT=y
CONFIG_STACKTRACE_SUPPORT=y
CONFIG_MMU=y
CONFIG_ARCH_MMAP_RND_BITS_MIN=28
CONFIG_ARCH_MMAP_RND_BITS_MAX=32
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MIN=8
CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MAX=16
CONFIG_GENERIC_ISA_DMA=y
CONFIG_GENERIC_BUG=y
CONFIG_GENERIC_BUG_RELATIVE_POINTERS=y
CONFIG_GENERIC_HWEIGHT=y
CONFIG_ARCH_MAY_HAVE_PC_FDC=y
CONFIG_RWSEM_XCHGADD_ALGORITHM=y
CONFIG_GENERIC_CALIBRATE_DELAY=y
CONFIG_ARCH_HAS_CPU_RELAX=y
CONFIG_ARCH_HAS_CACHE_LINE_SIZE=y
CONFIG_ARCH_HAS_FILTER_PGPROT=y
CONFIG_HAVE_SETUP_PER_CPU_AREA=y
CONFIG_NEED_PER_CPU_EMBED_FIRST_CHUNK=y
CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK=y
CONFIG_ARCH_HIBERNATION_POSSIBLE=y
CONFIG_ARCH_SUSPEND_POSSIBLE=y
CONFIG_ARCH_WANT_HUGE_PMD_SHARE=y
CONFIG_ARCH_WANT_GENERAL_HUGETLB=y
CONFIG_ZONE_DMA32=y
CONFIG_AUDIT_ARCH=y
CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING=y
CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC=y
CONFIG_HAVE_INTEL_TXT=y
CONFIG_X86_64_SMP=y
CONFIG_ARCH_SUPPORTS_UPROBES=y
CONFIG_FIX_EARLYCON_MEM=y
CONFIG_PGTABLE_LEVELS=4
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=80201
CONFIG_CLANG_VERSION=0
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_EXTABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y
CONFIG_INIT_ENV_ARG_LIMIT=32
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_BZIP2=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
CONFIG_KERNEL_GZIP=y
CONFIG_DEFAULT_HOSTNAME="(none)"
CONFIG_SWAP=y
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
CONFIG_POSIX_MQUEUE=y
CONFIG_POSIX_MQUEUE_SYSCTL=y
CONFIG_CROSS_MEMORY_ATTACH=y
CONFIG_USELIB=y
CONFIG_HAVE_ARCH_AUDITSYSCALL=y
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=y
CONFIG_GENERIC_PENDING_IRQ=y
CONFIG_GENERIC_IRQ_MIGRATION=y
CONFIG_IRQ_DOMAIN=y
CONFIG_IRQ_DOMAIN_HIERARCHY=y
CONFIG_GENERIC_MSI_IRQ=y
CONFIG_GENERIC_MSI_IRQ_DOMAIN=y
CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR=y
CONFIG_GENERIC_IRQ_RESERVATION_MODE=y
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_SPARSE_IRQ=y
CONFIG_CLOCKSOURCE_WATCHDOG=y
CONFIG_ARCH_CLOCKSOURCE_DATA=y
CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE=y
CONFIG_GENERIC_TIME_VSYSCALL=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=y
CONFIG_GENERIC_CMOS_UPDATE=y
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
CONFIG_NO_HZ_IDLE=y
CONFIG_NO_HZ=y
CONFIG_HIGH_RES_TIMERS=y
CONFIG_TICK_CPU_ACCOUNTING=y
CONFIG_BSD_PROCESS_ACCT=y
CONFIG_TASKSTATS=y
CONFIG_TASK_DELAY_ACCT=y
CONFIG_TASK_XACCT=y
CONFIG_TASK_IO_ACCOUNTING=y

Re: please revert commit ce8556cca6 "kbuild: verify that $DEPMOD is installed" introduced in v4.18.4.

[Alexander Kapshuk]

On Thu, Aug 23, 2018 at 05:38:27PM +0900, Masahiro Yamada wrote:
> Hi Randy,
> 
> 
> 2018-08-23 8:33 GMT+09:00 Randy Dunlap :
> > On 08/22/2018 11:53 AM, H. Nikolaus Schaller wrote:
> >> This patch requires that /sbin/depmod is installed and installable on
> >> the build host.
> >>
> >> But not all build hosts for cross compiling Linux are Linux systems
> >> and are able to provide a working port of depmod, especially at the
> >> file patch /sbin/depmod.
> >>
> >> I use, for example, a Darwin system to cross compile Linux and I run
> >> depmod -a on the embedded system once, after installing a new Linux
> >> kernel there.
> >>
> >> I have no problem with seeing a warning, but aborting the build process
> >> is IMHO a bad idea since the previous behaviour didn't harm many people
> >> as far as I see. Probably 99% of people compiling Linux kernels do that
> >> on Linux and 99% of those have depmod installed for optimal operation of
> >> their build host. So IMHO printing the warning is good enough.
> >
> > Thanks for the report and sorry about the problem.
> >
> > I'm OK with changing the error to a warning.
> > Does the patch below work for you?
> >
> > thanks.
> > ---
> > From: Randy Dunlap 
> >
> > When $DEPMOD is not found, only print a warning instead of exiting
> > with an error message and error status.
> >
> > E.g.:
> > Warning: 'make modules_install' requires /sbin/depmod. Please install it.
> > This is probably in the kmod package.
> > ../scripts/depmod.sh: line 44: /sbin/depmod: No such file or directory
> > make[1]: *** [/home/rdunlap/lnx/lnx-418/Makefile:1244: _modinst_post] Error 
> > 127
> > make: *** [Makefile:146: sub-make] Error 2
> >
> > Signed-off-by: Randy Dunlap 
> > Reported-by: H. Nikolaus Schaller 
> > ---
> >  scripts/depmod.sh |3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > --- lnx-418.orig/scripts/depmod.sh
> > +++ lnx-418/scripts/depmod.sh
> > @@ -15,9 +15,8 @@ if ! test -r System.map ; then
> >  fi
> >
> >  if [ -z $(command -v $DEPMOD) ]; then
> > -   echo "'make modules_install' requires $DEPMOD. Please install it." 
> > >&2
> > +   echo "Warning: 'make modules_install' requires $DEPMOD. Please 
> > install it." >&2
> > echo "This is probably in the kmod package." >&2
> > -   exit 1
> 
> 
> 'exit 0' is missing here.
> 
> This shell script would fail, then abort the build process.
> 
> 
> > ../scripts/depmod.sh: line 44: /sbin/depmod: No such file or directory
> > make[1]: *** [/home/rdunlap/lnx/lnx-418/Makefile:1244: _modinst_post] Error 
> > 127
> > make: *** [Makefile:146: sub-make] Error 2
> 
> 
> 
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

May I suggest the following implementation:

diff -U0 scripts/depmod.sh ~/tmp/depmod.sh 
--- scripts/depmod.sh   2018-08-10 17:14:19.036349222 +0300
+++ /home/sasha/tmp/depmod.sh   2018-08-23 18:07:23.486048827 +0300

# 'if' block may be omitted here. If System.map isn't there and isn't
readable, 'exit 0'.
@@ -13,3 +13 @@
-if ! test -r System.map ; then
-   exit 0
-fi
+test -r System.map || exit 0

# Have the 'if' test statement syntax conform with the remainder of the
script, which uses 'if test' rather then the 2nd form of the test utility,
i.e. '[ expr ]'.
# In my view, the use of the negation operator, '!', makes the intent
clearer, and reads, 'if $DEPMOD not found', rather then 'if the length
of $DEPMOD is zero'.
@@ -17 +15 @@
-if [ -z $(command -v $DEPMOD) ]; then
+if test ! $(command -v $DEPMOD); then
@@ -20 +18 @@
-   exit 1
+   exit 0

[Sample run]
# Testing for a non-existant utility:
% if test ! $(command -v cmd); then echo cmd not found; fi
% cmd not found

# Testing for a existing utility:
% if test ! $(command -v ed); then echo ed not found; fi

% if test $(command -v ed); then echo ed found; fi
% ed found

Re: please revert commit ce8556cca6 "kbuild: verify that $DEPMOD is installed" introduced in v4.18.4.

[Alexander Kapshuk]

On Thu, Aug 23, 2018 at 05:38:27PM +0900, Masahiro Yamada wrote:
> Hi Randy,
> 
> 
> 2018-08-23 8:33 GMT+09:00 Randy Dunlap :
> > On 08/22/2018 11:53 AM, H. Nikolaus Schaller wrote:
> >> This patch requires that /sbin/depmod is installed and installable on
> >> the build host.
> >>
> >> But not all build hosts for cross compiling Linux are Linux systems
> >> and are able to provide a working port of depmod, especially at the
> >> file patch /sbin/depmod.
> >>
> >> I use, for example, a Darwin system to cross compile Linux and I run
> >> depmod -a on the embedded system once, after installing a new Linux
> >> kernel there.
> >>
> >> I have no problem with seeing a warning, but aborting the build process
> >> is IMHO a bad idea since the previous behaviour didn't harm many people
> >> as far as I see. Probably 99% of people compiling Linux kernels do that
> >> on Linux and 99% of those have depmod installed for optimal operation of
> >> their build host. So IMHO printing the warning is good enough.
> >
> > Thanks for the report and sorry about the problem.
> >
> > I'm OK with changing the error to a warning.
> > Does the patch below work for you?
> >
> > thanks.
> > ---
> > From: Randy Dunlap 
> >
> > When $DEPMOD is not found, only print a warning instead of exiting
> > with an error message and error status.
> >
> > E.g.:
> > Warning: 'make modules_install' requires /sbin/depmod. Please install it.
> > This is probably in the kmod package.
> > ../scripts/depmod.sh: line 44: /sbin/depmod: No such file or directory
> > make[1]: *** [/home/rdunlap/lnx/lnx-418/Makefile:1244: _modinst_post] Error 
> > 127
> > make: *** [Makefile:146: sub-make] Error 2
> >
> > Signed-off-by: Randy Dunlap 
> > Reported-by: H. Nikolaus Schaller 
> > ---
> >  scripts/depmod.sh |3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > --- lnx-418.orig/scripts/depmod.sh
> > +++ lnx-418/scripts/depmod.sh
> > @@ -15,9 +15,8 @@ if ! test -r System.map ; then
> >  fi
> >
> >  if [ -z $(command -v $DEPMOD) ]; then
> > -   echo "'make modules_install' requires $DEPMOD. Please install it." 
> > >&2
> > +   echo "Warning: 'make modules_install' requires $DEPMOD. Please 
> > install it." >&2
> > echo "This is probably in the kmod package." >&2
> > -   exit 1
> 
> 
> 'exit 0' is missing here.
> 
> This shell script would fail, then abort the build process.
> 
> 
> > ../scripts/depmod.sh: line 44: /sbin/depmod: No such file or directory
> > make[1]: *** [/home/rdunlap/lnx/lnx-418/Makefile:1244: _modinst_post] Error 
> > 127
> > make: *** [Makefile:146: sub-make] Error 2
> 
> 
> 
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

May I suggest the following implementation:

diff -U0 scripts/depmod.sh ~/tmp/depmod.sh 
--- scripts/depmod.sh   2018-08-10 17:14:19.036349222 +0300
+++ /home/sasha/tmp/depmod.sh   2018-08-23 18:07:23.486048827 +0300

# 'if' block may be omitted here. If System.map isn't there and isn't
readable, 'exit 0'.
@@ -13,3 +13 @@
-if ! test -r System.map ; then
-   exit 0
-fi
+test -r System.map || exit 0

# Have the 'if' test statement syntax conform with the remainder of the
script, which uses 'if test' rather then the 2nd form of the test utility,
i.e. '[ expr ]'.
# In my view, the use of the negation operator, '!', makes the intent
clearer, and reads, 'if $DEPMOD not found', rather then 'if the length
of $DEPMOD is zero'.
@@ -17 +15 @@
-if [ -z $(command -v $DEPMOD) ]; then
+if test ! $(command -v $DEPMOD); then
@@ -20 +18 @@
-   exit 1
+   exit 0

[Sample run]
# Testing for a non-existant utility:
% if test ! $(command -v cmd); then echo cmd not found; fi
% cmd not found

# Testing for a existing utility:
% if test ! $(command -v ed); then echo ed not found; fi

% if test $(command -v ed); then echo ed found; fi
% ed found

[tip:perf/urgent] perf tools: Fix check-headers.sh AND list path of execution

[tip-bot for Alexander Kapshuk]

Commit-ID:  51d8aac236493833acf60d9c3b6c42437a18da36
Gitweb: https://git.kernel.org/tip/51d8aac236493833acf60d9c3b6c42437a18da36
Author: Alexander Kapshuk 
AuthorDate: Sat, 11 Aug 2018 11:39:15 +0300
Committer:  Arnaldo Carvalho de Melo 
CommitDate: Mon, 13 Aug 2018 15:46:19 -0300

perf tools: Fix check-headers.sh AND list path of execution

The '||' path of execution in the 'test' block of the check_2() function
may also be taken if file2 does not exist, in which case the warning
message about the ABI headers being different would still be printed
where it should not be.  See below.

  % file1=file1; file2=file2
  % cmd="echo diff $file1 $file2"
  % test -f $file2 && \
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
   differs from latest version at '$file2'" >&2
   Warning: Kernel ABI header at 'tools/file1' differs from 
latest
   version at 'file2'

The proposed patch converts the code following the '&&' operator into a
compound list to be executed in the current process environment only if file2
does exist. Should the files being compared differ, a diff command to compare
the files concerned is printed on standard output. E.g.

  $ diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S

Committer testing:

Remove a line from that tools/arch/x86/lib/memcpy_64.S file to test
this:

  BUILD:   Doing 'make -j4' parallel build
Warning: Kernel ABI header at 'tools/arch/x86/lib/memcpy_64.S' differs from 
latest version at 'arch/x86/lib/memcpy_64.S'
diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S
  CC   /tmp/build/perf/bench/mem-memcpy-x86-64-asm.o

Signed-off-by: Alexander Kapshuk 
Tested-by: Arnaldo Carvalho de Melo 
Acked-by: Jiri Olsa 
Cc: Alexander Shishkin 
Cc: David Ahern 
Cc: Namhyung Kim 
Cc: Peter Zijlstra 
Link: 
http://lkml.kernel.org/r/20180811083915.17471-1-alexander.kaps...@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/check-headers.sh | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index de28466c0186..ea48aa6f8d19 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -67,8 +67,12 @@ check_2 () {
 
   cmd="diff $* $file1 $file2 > /dev/null"
 
-  test -f $file2 &&
-  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from 
latest version at '$file'" >&2
+  test -f $file2 && {
+eval $cmd || {
+  echo "Warning: Kernel ABI header at 'tools/$file' differs from latest 
version at '$file'" >&2
+  echo diff -u tools/$file $file
+}
+  }
 }
 
 check () {

[tip:perf/urgent] perf tools: Fix check-headers.sh AND list path of execution

[tip-bot for Alexander Kapshuk]

Commit-ID:  51d8aac236493833acf60d9c3b6c42437a18da36
Gitweb: https://git.kernel.org/tip/51d8aac236493833acf60d9c3b6c42437a18da36
Author: Alexander Kapshuk 
AuthorDate: Sat, 11 Aug 2018 11:39:15 +0300
Committer:  Arnaldo Carvalho de Melo 
CommitDate: Mon, 13 Aug 2018 15:46:19 -0300

perf tools: Fix check-headers.sh AND list path of execution

The '||' path of execution in the 'test' block of the check_2() function
may also be taken if file2 does not exist, in which case the warning
message about the ABI headers being different would still be printed
where it should not be.  See below.

  % file1=file1; file2=file2
  % cmd="echo diff $file1 $file2"
  % test -f $file2 && \
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
   differs from latest version at '$file2'" >&2
   Warning: Kernel ABI header at 'tools/file1' differs from 
latest
   version at 'file2'

The proposed patch converts the code following the '&&' operator into a
compound list to be executed in the current process environment only if file2
does exist. Should the files being compared differ, a diff command to compare
the files concerned is printed on standard output. E.g.

  $ diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S

Committer testing:

Remove a line from that tools/arch/x86/lib/memcpy_64.S file to test
this:

  BUILD:   Doing 'make -j4' parallel build
Warning: Kernel ABI header at 'tools/arch/x86/lib/memcpy_64.S' differs from 
latest version at 'arch/x86/lib/memcpy_64.S'
diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S
  CC   /tmp/build/perf/bench/mem-memcpy-x86-64-asm.o

Signed-off-by: Alexander Kapshuk 
Tested-by: Arnaldo Carvalho de Melo 
Acked-by: Jiri Olsa 
Cc: Alexander Shishkin 
Cc: David Ahern 
Cc: Namhyung Kim 
Cc: Peter Zijlstra 
Link: 
http://lkml.kernel.org/r/20180811083915.17471-1-alexander.kaps...@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/check-headers.sh | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index de28466c0186..ea48aa6f8d19 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -67,8 +67,12 @@ check_2 () {
 
   cmd="diff $* $file1 $file2 > /dev/null"
 
-  test -f $file2 &&
-  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from 
latest version at '$file'" >&2
+  test -f $file2 && {
+eval $cmd || {
+  echo "Warning: Kernel ABI header at 'tools/$file' differs from latest 
version at '$file'" >&2
+  echo diff -u tools/$file $file
+}
+  }
 }
 
 check () {

Re: [PATCH] perf tools: Fix check-headers.sh AND list path of execution

[Alexander Kapshuk]

On Mon, Aug 13, 2018 at 9:58 PM Arnaldo Carvalho de Melo
 wrote:
> Thanks, applied the three patches to acme/perf/core.
>
> - Arnaldo

Thanks.

Re: [PATCH] perf tools: Fix check-headers.sh AND list path of execution

[Alexander Kapshuk]

On Mon, Aug 13, 2018 at 9:58 PM Arnaldo Carvalho de Melo
 wrote:
> Thanks, applied the three patches to acme/perf/core.
>
> - Arnaldo

Thanks.

Re: [PATCH] perf tools: Fix check-headers.sh AND list path of execution

[Alexander Kapshuk]

On Mon, Aug 13, 2018 at 2:16 PM Jiri Olsa  wrote:
>
> On Sat, Aug 11, 2018 at 11:39:15AM +0300, Alexander Kapshuk wrote:
> > The '||' path of execution in the 'test' block of the check_2() function
> > may also be taken if file2 does not exist, in which case the warning
> > message about the ABI headers being different would still be printed
> > where it should not be.  See below.
> >
> > % file1=file1; file2=file2
> > % cmd="echo diff $file1 $file2"
> > % test -f $file2 &&
> > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> > differs from latest version at '$file2'" >&2
> > Warning: Kernel ABI header at 'tools/file1' differs from latest
> > version at 'file2'
> >
> > The proposed patch converts the code following the '&&' operator into a
> > compound list to be executed in the current process environment only if
> > file2 does exist. Should the files being compared differ, a diff command
> > to compare the files concerned is printed on standard output. E.g.
> >
> > diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S
> >
> > Signed-off-by: Alexander Kapshuk 
>
> I posted follow up patches based on this one
> as a reply on this patch
>
> for this one:
>
> Acked-by: Jiri Olsa 
>
> thanks,
> jirka

Noted.
Thanks.


>
> > ---
> >  tools/perf/check-headers.sh | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> > index de28466c0186..ea48aa6f8d19 100755
> > --- a/tools/perf/check-headers.sh
> > +++ b/tools/perf/check-headers.sh
> > @@ -67,8 +67,12 @@ check_2 () {
> >
> >cmd="diff $* $file1 $file2 > /dev/null"
> >
> > -  test -f $file2 &&
> > -  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs 
> > from latest version at '$file'" >&2
> > +  test -f $file2 && {
> > +eval $cmd || {
> > +  echo "Warning: Kernel ABI header at 'tools/$file' differs from 
> > latest version at '$file'" >&2
> > +  echo diff -u tools/$file $file
> > +}
> > +  }
> >  }
> >
> >  check () {
> > --
> > 2.18.0
> >

Re: [PATCH] perf tools: Fix check-headers.sh AND list path of execution

[Alexander Kapshuk]

On Mon, Aug 13, 2018 at 2:16 PM Jiri Olsa  wrote:
>
> On Sat, Aug 11, 2018 at 11:39:15AM +0300, Alexander Kapshuk wrote:
> > The '||' path of execution in the 'test' block of the check_2() function
> > may also be taken if file2 does not exist, in which case the warning
> > message about the ABI headers being different would still be printed
> > where it should not be.  See below.
> >
> > % file1=file1; file2=file2
> > % cmd="echo diff $file1 $file2"
> > % test -f $file2 &&
> > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> > differs from latest version at '$file2'" >&2
> > Warning: Kernel ABI header at 'tools/file1' differs from latest
> > version at 'file2'
> >
> > The proposed patch converts the code following the '&&' operator into a
> > compound list to be executed in the current process environment only if
> > file2 does exist. Should the files being compared differ, a diff command
> > to compare the files concerned is printed on standard output. E.g.
> >
> > diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S
> >
> > Signed-off-by: Alexander Kapshuk 
>
> I posted follow up patches based on this one
> as a reply on this patch
>
> for this one:
>
> Acked-by: Jiri Olsa 
>
> thanks,
> jirka

Noted.
Thanks.


>
> > ---
> >  tools/perf/check-headers.sh | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> > index de28466c0186..ea48aa6f8d19 100755
> > --- a/tools/perf/check-headers.sh
> > +++ b/tools/perf/check-headers.sh
> > @@ -67,8 +67,12 @@ check_2 () {
> >
> >cmd="diff $* $file1 $file2 > /dev/null"
> >
> > -  test -f $file2 &&
> > -  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs 
> > from latest version at '$file'" >&2
> > +  test -f $file2 && {
> > +eval $cmd || {
> > +  echo "Warning: Kernel ABI header at 'tools/$file' differs from 
> > latest version at '$file'" >&2
> > +  echo diff -u tools/$file $file
> > +}
> > +  }
> >  }
> >
> >  check () {
> > --
> > 2.18.0
> >

[PATCH] perf tools: Fix check-headers.sh AND list path of execution

[Alexander Kapshuk]

The '||' path of execution in the 'test' block of the check_2() function
may also be taken if file2 does not exist, in which case the warning
message about the ABI headers being different would still be printed
where it should not be.  See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest
version at 'file2'

The proposed patch converts the code following the '&&' operator into a
compound list to be executed in the current process environment only if
file2 does exist. Should the files being compared differ, a diff command
to compare the files concerned is printed on standard output. E.g.

diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S

Signed-off-by: Alexander Kapshuk 
---
 tools/perf/check-headers.sh | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index de28466c0186..ea48aa6f8d19 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -67,8 +67,12 @@ check_2 () {
 
   cmd="diff $* $file1 $file2 > /dev/null"
 
-  test -f $file2 &&
-  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from 
latest version at '$file'" >&2
+  test -f $file2 && {
+eval $cmd || {
+  echo "Warning: Kernel ABI header at 'tools/$file' differs from latest 
version at '$file'" >&2
+  echo diff -u tools/$file $file
+}
+  }
 }
 
 check () {
-- 
2.18.0

[PATCH] perf tools: Fix check-headers.sh AND list path of execution

[Alexander Kapshuk]

The '||' path of execution in the 'test' block of the check_2() function
may also be taken if file2 does not exist, in which case the warning
message about the ABI headers being different would still be printed
where it should not be.  See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest
version at 'file2'

The proposed patch converts the code following the '&&' operator into a
compound list to be executed in the current process environment only if
file2 does exist. Should the files being compared differ, a diff command
to compare the files concerned is printed on standard output. E.g.

diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S

Signed-off-by: Alexander Kapshuk 
---
 tools/perf/check-headers.sh | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index de28466c0186..ea48aa6f8d19 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -67,8 +67,12 @@ check_2 () {
 
   cmd="diff $* $file1 $file2 > /dev/null"
 
-  test -f $file2 &&
-  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from 
latest version at '$file'" >&2
+  test -f $file2 && {
+eval $cmd || {
+  echo "Warning: Kernel ABI header at 'tools/$file' differs from latest 
version at '$file'" >&2
+  echo diff -u tools/$file $file
+}
+  }
 }
 
 check () {
-- 
2.18.0

Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

[Alexander Kapshuk]

On Mon, Jul 23, 2018, 08:01 Jiri Olsa  wrote:

> On Fri, Jul 20, 2018 at 06:22:49PM +0300, Alexander Kapshuk wrote:
> > On Fri, Jul 20, 2018 at 6:16 PM Jiri Olsa  wrote:
> > >
> > > On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo
> wrote:
> > > > Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> > > > > The warning message in check_w function uses wrongly
> > > > > the $file variable instead of $file1 and $file2.
> > > >
> > > > Humm,
> > > >
> > > > Before:
> > > >
> > > > Warning: Kernel ABI header at
> 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version
> at 'arch/powerpc/include/uapi/asm/unistd.h'
> > > >
> > > > After:
> > > >
> > > > Warning: Kernel ABI header at
> '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at
> '../../arch/powerpc/include/uapi/asm/unistd.h'
> > > >
> > > >
> > > > The previous version is better, I can then just use:
> > > >
> > > > diff -u tools/arch/powerpc/include/uapi/asm/unistd.h
> arch/powerpc/include/uapi/asm/unistd.h
> > > >
> > > > and get what changed, with your change I have to go to tools/perf
> before
> > > > doing that diff, which is an unnecessary extra step in at least my
> > > > workflow.
> > >
> > > so all paths output based in kernel tree root then, will change
> > >
> > > jirka
> >
> > I was going to ask about this in a separate email initially, but then
> > thought I'd use this email exchange instead, as my question is about
> > the code in question. Hope you don't mind.
> >
> > If I'm reading this right, the intended behavoir of the block of code
> > below is to test file2 for existance, and if it exists, to evaluate $cmd.
> > If file1 and file2 are found to differ, print the warning.
> >
> > test -f $file2 &&
> >   eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'
> > differs from latest version at '$file'" >&2
> >
> > The '||' path of execution is however also taken if file2 doesn't exist,
> > which is probably very unlikely to happen. See below.
> >
> > % file1=file1; file2=file2
> > % cmd="echo diff $file1 $file2"
> > % test -f $file2 &&
> > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> > differs from latest version at '$file2'" >&2
> > Warning: Kernel ABI header at 'tools/file1' differs from latest
> > version at 'file2'
> >
> > Is this something you would rather leave as is, or perhaps use something
> > along the lines of the code below instead:
> >
> > test -f $file2 && {
> > eval $cmd ||
> > echo "Warning: Kernel ABI header at 'tools/$file' differs from latest
> > version at '$file'" >&2
> > }
>
> hi,
> yea, probably..  please feel free to post a patch.. just make sure all
> the displayed files paths are based on the kernel root
>
> thanks,
> jirka
>

I'm away traveling till August 10th, and I may not be able to send the
patch in until I get back. Is that OK?
Thanks.

Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

[Alexander Kapshuk]

On Mon, Jul 23, 2018, 08:01 Jiri Olsa  wrote:

> On Fri, Jul 20, 2018 at 06:22:49PM +0300, Alexander Kapshuk wrote:
> > On Fri, Jul 20, 2018 at 6:16 PM Jiri Olsa  wrote:
> > >
> > > On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo
> wrote:
> > > > Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> > > > > The warning message in check_w function uses wrongly
> > > > > the $file variable instead of $file1 and $file2.
> > > >
> > > > Humm,
> > > >
> > > > Before:
> > > >
> > > > Warning: Kernel ABI header at
> 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version
> at 'arch/powerpc/include/uapi/asm/unistd.h'
> > > >
> > > > After:
> > > >
> > > > Warning: Kernel ABI header at
> '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at
> '../../arch/powerpc/include/uapi/asm/unistd.h'
> > > >
> > > >
> > > > The previous version is better, I can then just use:
> > > >
> > > > diff -u tools/arch/powerpc/include/uapi/asm/unistd.h
> arch/powerpc/include/uapi/asm/unistd.h
> > > >
> > > > and get what changed, with your change I have to go to tools/perf
> before
> > > > doing that diff, which is an unnecessary extra step in at least my
> > > > workflow.
> > >
> > > so all paths output based in kernel tree root then, will change
> > >
> > > jirka
> >
> > I was going to ask about this in a separate email initially, but then
> > thought I'd use this email exchange instead, as my question is about
> > the code in question. Hope you don't mind.
> >
> > If I'm reading this right, the intended behavoir of the block of code
> > below is to test file2 for existance, and if it exists, to evaluate $cmd.
> > If file1 and file2 are found to differ, print the warning.
> >
> > test -f $file2 &&
> >   eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'
> > differs from latest version at '$file'" >&2
> >
> > The '||' path of execution is however also taken if file2 doesn't exist,
> > which is probably very unlikely to happen. See below.
> >
> > % file1=file1; file2=file2
> > % cmd="echo diff $file1 $file2"
> > % test -f $file2 &&
> > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> > differs from latest version at '$file2'" >&2
> > Warning: Kernel ABI header at 'tools/file1' differs from latest
> > version at 'file2'
> >
> > Is this something you would rather leave as is, or perhaps use something
> > along the lines of the code below instead:
> >
> > test -f $file2 && {
> > eval $cmd ||
> > echo "Warning: Kernel ABI header at 'tools/$file' differs from latest
> > version at '$file'" >&2
> > }
>
> hi,
> yea, probably..  please feel free to post a patch.. just make sure all
> the displayed files paths are based on the kernel root
>
> thanks,
> jirka
>

I'm away traveling till August 10th, and I may not be able to send the
patch in until I get back. Is that OK?
Thanks.

Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

[Alexander Kapshuk]

On Fri, Jul 20, 2018 at 6:16 PM Jiri Olsa  wrote:
>
> On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> > > The warning message in check_w function uses wrongly
> > > the $file variable instead of $file1 and $file2.
> >
> > Humm,
> >
> > Before:
> >
> > Warning: Kernel ABI header at 
> > 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version 
> > at 'arch/powerpc/include/uapi/asm/unistd.h'
> >
> > After:
> >
> > Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' 
> > differs from latest version at 
> > '../../arch/powerpc/include/uapi/asm/unistd.h'
> >
> >
> > The previous version is better, I can then just use:
> >
> > diff -u tools/arch/powerpc/include/uapi/asm/unistd.h 
> > arch/powerpc/include/uapi/asm/unistd.h
> >
> > and get what changed, with your change I have to go to tools/perf before
> > doing that diff, which is an unnecessary extra step in at least my
> > workflow.
>
> so all paths output based in kernel tree root then, will change
>
> jirka

I was going to ask about this in a separate email initially, but then
thought I'd use this email exchange instead, as my question is about
the code in question. Hope you don't mind.

If I'm reading this right, the intended behavoir of the block of code
below is to test file2 for existance, and if it exists, to evaluate $cmd.
If file1 and file2 are found to differ, print the warning.

test -f $file2 &&
  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'
differs from latest version at '$file'" >&2

The '||' path of execution is however also taken if file2 doesn't exist,
which is probably very unlikely to happen. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest
version at 'file2'

Is this something you would rather leave as is, or perhaps use something
along the lines of the code below instead:

test -f $file2 && {
eval $cmd ||
echo "Warning: Kernel ABI header at 'tools/$file' differs from latest
version at '$file'" >&2
}

Thanks.

Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

[Alexander Kapshuk]

On Fri, Jul 20, 2018 at 6:16 PM Jiri Olsa  wrote:
>
> On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> > > The warning message in check_w function uses wrongly
> > > the $file variable instead of $file1 and $file2.
> >
> > Humm,
> >
> > Before:
> >
> > Warning: Kernel ABI header at 
> > 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version 
> > at 'arch/powerpc/include/uapi/asm/unistd.h'
> >
> > After:
> >
> > Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' 
> > differs from latest version at 
> > '../../arch/powerpc/include/uapi/asm/unistd.h'
> >
> >
> > The previous version is better, I can then just use:
> >
> > diff -u tools/arch/powerpc/include/uapi/asm/unistd.h 
> > arch/powerpc/include/uapi/asm/unistd.h
> >
> > and get what changed, with your change I have to go to tools/perf before
> > doing that diff, which is an unnecessary extra step in at least my
> > workflow.
>
> so all paths output based in kernel tree root then, will change
>
> jirka

I was going to ask about this in a separate email initially, but then
thought I'd use this email exchange instead, as my question is about
the code in question. Hope you don't mind.

If I'm reading this right, the intended behavoir of the block of code
below is to test file2 for existance, and if it exists, to evaluate $cmd.
If file1 and file2 are found to differ, print the warning.

test -f $file2 &&
  eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'
differs from latest version at '$file'" >&2

The '||' path of execution is however also taken if file2 doesn't exist,
which is probably very unlikely to happen. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest
version at 'file2'

Is this something you would rather leave as is, or perhaps use something
along the lines of the code below instead:

test -f $file2 && {
eval $cmd ||
echo "Warning: Kernel ABI header at 'tools/$file' differs from latest
version at '$file'" >&2
}

Thanks.

[PATCH 2/2] ver_linux: Do not check for ver_linux pattern in version function

[Alexander Kapshuk]

Checking whether output of commands matches the ver_linux pattern in
the version function is original shell implementation legacy code. When
the original implementation failed to locate a particular utility,
it generated error output along the lines of:

ver_linux:line number: command not found.

The awk implementation, does not contain the name of the script within the
body of the error message returned by the subshell when a given utility
fails to be located. The error message returned is along the lines of:

sh: name of utility: command not found

Safeguarding against the ver_linux pattern being found in the output
being parsed may thus be safely omitted.

Signed-off-by: Alexander Kapshuk 
---
 scripts/ver_linux | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index e1dc041f903f..a6c728db05ce 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -70,7 +70,7 @@ BEGIN {
 function version(cmd,ver) {
cmd = cmd " 2>&1"
while (cmd | getline > 0) {
-   if (!/ver_linux/ && match($0, /[0-9]+([.]?[0-9]+)+/)) {
+   if (match($0, /[0-9]+([.]?[0-9]+)+/)) {
ver = substr($0, RSTART, RLENGTH)
break
}
-- 
2.17.1

[PATCH 2/2] ver_linux: Do not check for ver_linux pattern in version function

[Alexander Kapshuk]

Checking whether output of commands matches the ver_linux pattern in
the version function is original shell implementation legacy code. When
the original implementation failed to locate a particular utility,
it generated error output along the lines of:

ver_linux:line number: command not found.

The awk implementation, does not contain the name of the script within the
body of the error message returned by the subshell when a given utility
fails to be located. The error message returned is along the lines of:

sh: name of utility: command not found

Safeguarding against the ver_linux pattern being found in the output
being parsed may thus be safely omitted.

Signed-off-by: Alexander Kapshuk 
---
 scripts/ver_linux | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index e1dc041f903f..a6c728db05ce 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -70,7 +70,7 @@ BEGIN {
 function version(cmd,ver) {
cmd = cmd " 2>&1"
while (cmd | getline > 0) {
-   if (!/ver_linux/ && match($0, /[0-9]+([.]?[0-9]+)+/)) {
+   if (match($0, /[0-9]+([.]?[0-9]+)+/)) {
ver = substr($0, RSTART, RLENGTH)
break
}
-- 
2.17.1

[PATCH 1/2] ver_linux: Process input coming from procmaps that matches libc only

[Alexander Kapshuk]

Currently, input coming from /proc/self/maps is split into fields without
checking whether or not it matches libc.so. This is not efficient.
All text processing should only be performed on lines of input that
match libc.so.

Signed-off-by: Alexander Kapshuk 
---
 scripts/ver_linux | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index 7227994ccf63..e1dc041f903f 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -32,11 +32,13 @@ BEGIN {
printversion("Nfs-utils", version("showmount --version"))
 
while (getline <"/proc/self/maps" > 0) {
-   n = split($0, procmaps, "/")
-   if (/libc.*so$/ && match(procmaps[n], /[0-9]+([.]?[0-9]+)+/)) {
-   ver = substr(procmaps[n], RSTART, RLENGTH)
-   printversion("Linux C Library", ver)
-   break
+   if (/libc.*\.so$/) {
+   n = split($0, procmaps, "/")
+   if (match(procmaps[n], /[0-9]+([.]?[0-9]+)+/)) {
+   ver = substr(procmaps[n], RSTART, RLENGTH)
+   printversion("Linux C Library", ver)
+   break
+   }
}
}
 
-- 
2.17.1

[PATCH 1/2] ver_linux: Process input coming from procmaps that matches libc only

[Alexander Kapshuk]

Currently, input coming from /proc/self/maps is split into fields without
checking whether or not it matches libc.so. This is not efficient.
All text processing should only be performed on lines of input that
match libc.so.

Signed-off-by: Alexander Kapshuk 
---
 scripts/ver_linux | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index 7227994ccf63..e1dc041f903f 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -32,11 +32,13 @@ BEGIN {
printversion("Nfs-utils", version("showmount --version"))
 
while (getline <"/proc/self/maps" > 0) {
-   n = split($0, procmaps, "/")
-   if (/libc.*so$/ && match(procmaps[n], /[0-9]+([.]?[0-9]+)+/)) {
-   ver = substr(procmaps[n], RSTART, RLENGTH)
-   printversion("Linux C Library", ver)
-   break
+   if (/libc.*\.so$/) {
+   n = split($0, procmaps, "/")
+   if (match(procmaps[n], /[0-9]+([.]?[0-9]+)+/)) {
+   ver = substr(procmaps[n], RSTART, RLENGTH)
+   printversion("Linux C Library", ver)
+   break
+   }
}
}
 
-- 
2.17.1

[PATCH 2/2] ver_linux: Drop redundant calls to system() to test if file is readable

[Alexander Kapshuk]

Running 'test -r' on an awk variable name whose value is an empty
string results in test being run with no arguments, and causes system()
to return 0, which indicates success when used to test values returned
by function calls. This results in code within the if blocks being run
when it should not be.
Instead of testing if a file is accessible and readable via calls to
system("test -r " file), rely on the value returned by getline to perform
this kind of testing. Getline returns -1 on error, with the code within
the while loops not being run.

Signed-off-by: Alexander Kapshuk <alexander.kaps...@gmail.com>
---
 scripts/ver_linux | 28 +++-
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index 0b301bd1637d..7227994ccf63 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -31,14 +31,12 @@ BEGIN {
printversion("Isdn4k-utils", version("isdnctrl"))
printversion("Nfs-utils", version("showmount --version"))
 
-   if (system("test -r /proc/self/maps") == 0) {
-   while (getline <"/proc/self/maps" > 0) {
-   n = split($0, procmaps, "/")
-   if (/libc.*so$/ && match(procmaps[n], 
/[0-9]+([.]?[0-9]+)+/)) {
-   ver = substr(procmaps[n], RSTART, RLENGTH)
-   printversion("Linux C Library", ver)
-   break
-   }
+   while (getline <"/proc/self/maps" > 0) {
+   n = split($0, procmaps, "/")
+   if (/libc.*so$/ && match(procmaps[n], /[0-9]+([.]?[0-9]+)+/)) {
+   ver = substr(procmaps[n], RSTART, RLENGTH)
+   printversion("Linux C Library", ver)
+   break
}
}
 
@@ -50,9 +48,7 @@ BEGIN {
break
}
}
-   if (system("test -r " libcpp) == 0)
-   printversion("Linux C++ Library", version("readlink " libcpp))
-
+   printversion("Linux C++ Library", version("readlink " libcpp))
printversion("Procps", version("ps --version"))
printversion("Net-tools", version("ifconfig --version"))
printversion("Kbd", version("loadkeys -V"))
@@ -62,13 +58,11 @@ BEGIN {
printversion("Udev", version("udevadm --version"))
printversion("Wireless-tools", version("iwconfig --version"))
 
-   if (system("test -r /proc/modules") == 0) {
-   while ("sort /proc/modules" | getline > 0) {
-   mods = mods sep $1
-   sep = " "
-   }
-   printversion("Modules Loaded", mods)
+   while ("sort /proc/modules" | getline > 0) {
+   mods = mods sep $1
+   sep = " "
}
+   printversion("Modules Loaded", mods)
 }
 
 function version(cmd,ver) {
-- 
2.17.0

[PATCH 2/2] ver_linux: Drop redundant calls to system() to test if file is readable

[Alexander Kapshuk]

Running 'test -r' on an awk variable name whose value is an empty
string results in test being run with no arguments, and causes system()
to return 0, which indicates success when used to test values returned
by function calls. This results in code within the if blocks being run
when it should not be.
Instead of testing if a file is accessible and readable via calls to
system("test -r " file), rely on the value returned by getline to perform
this kind of testing. Getline returns -1 on error, with the code within
the while loops not being run.

Signed-off-by: Alexander Kapshuk 
---
 scripts/ver_linux | 28 +++-
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index 0b301bd1637d..7227994ccf63 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -31,14 +31,12 @@ BEGIN {
printversion("Isdn4k-utils", version("isdnctrl"))
printversion("Nfs-utils", version("showmount --version"))
 
-   if (system("test -r /proc/self/maps") == 0) {
-   while (getline <"/proc/self/maps" > 0) {
-   n = split($0, procmaps, "/")
-   if (/libc.*so$/ && match(procmaps[n], 
/[0-9]+([.]?[0-9]+)+/)) {
-   ver = substr(procmaps[n], RSTART, RLENGTH)
-   printversion("Linux C Library", ver)
-   break
-   }
+   while (getline <"/proc/self/maps" > 0) {
+   n = split($0, procmaps, "/")
+   if (/libc.*so$/ && match(procmaps[n], /[0-9]+([.]?[0-9]+)+/)) {
+   ver = substr(procmaps[n], RSTART, RLENGTH)
+   printversion("Linux C Library", ver)
+   break
}
}
 
@@ -50,9 +48,7 @@ BEGIN {
break
}
}
-   if (system("test -r " libcpp) == 0)
-   printversion("Linux C++ Library", version("readlink " libcpp))
-
+   printversion("Linux C++ Library", version("readlink " libcpp))
printversion("Procps", version("ps --version"))
printversion("Net-tools", version("ifconfig --version"))
printversion("Kbd", version("loadkeys -V"))
@@ -62,13 +58,11 @@ BEGIN {
printversion("Udev", version("udevadm --version"))
printversion("Wireless-tools", version("iwconfig --version"))
 
-   if (system("test -r /proc/modules") == 0) {
-   while ("sort /proc/modules" | getline > 0) {
-   mods = mods sep $1
-   sep = " "
-   }
-   printversion("Modules Loaded", mods)
+   while ("sort /proc/modules" | getline > 0) {
+   mods = mods sep $1
+   sep = " "
}
+   printversion("Modules Loaded", mods)
 }
 
 function version(cmd,ver) {
-- 
2.17.0

[PATCH 1/2] ver_linux: Move stderr redirection from function parameter to function body

[Alexander Kapshuk]

Remove stderr redirection to stdout from all the parameters to the
version() function, and put it with the body of the version() function
instead.
This improves code readability.

Signed-off-by: Alexander Kapshuk <alexander.kaps...@gmail.com>
---
 scripts/ver_linux | 53 ---
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index 545ec7388eb7..0b301bd1637d 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -13,23 +13,23 @@ BEGIN {
system("uname -a")
printf("\n")
 
-   printversion("GNU C", version("gcc -dumpversion 2>&1"))
-   printversion("GNU Make", version("make --version 2>&1"))
-   printversion("Binutils", version("ld -v 2>&1"))
-   printversion("Util-linux", version("mount --version 2>&1"))
-   printversion("Mount", version("mount --version 2>&1"))
-   printversion("Module-init-tools", version("depmod -V  2>&1"))
-   printversion("E2fsprogs", version("tune2fs 2>&1"))
-   printversion("Jfsutils", version("fsck.jfs -V 2>&1"))
-   printversion("Reiserfsprogs", version("reiserfsck -V 2>&1"))
-   printversion("Reiser4fsprogs", version("fsck.reiser4 -V 2>&1"))
-   printversion("Xfsprogs", version("xfs_db -V 2>&1"))
-   printversion("Pcmciautils", version("pccardctl -V 2>&1"))
-   printversion("Pcmcia-cs", version("cardmgr -V 2>&1"))
-   printversion("Quota-tools", version("quota -V 2>&1"))
-   printversion("PPP", version("pppd --version 2>&1"))
-   printversion("Isdn4k-utils", version("isdnctrl 2>&1"))
-   printversion("Nfs-utils", version("showmount --version 2>&1"))
+   printversion("GNU C", version("gcc -dumpversion"))
+   printversion("GNU Make", version("make --version"))
+   printversion("Binutils", version("ld -v"))
+   printversion("Util-linux", version("mount --version"))
+   printversion("Mount", version("mount --version"))
+   printversion("Module-init-tools", version("depmod -V"))
+   printversion("E2fsprogs", version("tune2fs"))
+   printversion("Jfsutils", version("fsck.jfs -V"))
+   printversion("Reiserfsprogs", version("reiserfsck -V"))
+   printversion("Reiser4fsprogs", version("fsck.reiser4 -V"))
+   printversion("Xfsprogs", version("xfs_db -V"))
+   printversion("Pcmciautils", version("pccardctl -V"))
+   printversion("Pcmcia-cs", version("cardmgr -V"))
+   printversion("Quota-tools", version("quota -V"))
+   printversion("PPP", version("pppd --version"))
+   printversion("Isdn4k-utils", version("isdnctrl"))
+   printversion("Nfs-utils", version("showmount --version"))
 
if (system("test -r /proc/self/maps") == 0) {
while (getline <"/proc/self/maps" > 0) {
@@ -42,7 +42,7 @@ BEGIN {
}
}
 
-   printversion("Dynamic linker (ldd)", version("ldd --version 2>&1"))
+   printversion("Dynamic linker (ldd)", version("ldd --version"))
 
while ("ldconfig -p 2>/dev/null" | getline > 0) {
if (/(libg|stdc)[+]+\.so/) {
@@ -53,14 +53,14 @@ BEGIN {
if (system("test -r " libcpp) == 0)
printversion("Linux C++ Library", version("readlink " libcpp))
 
-   printversion("Procps", version("ps --version 2>&1"))
-   printversion("Net-tools", version("ifconfig --version 2>&1"))
-   printversion("Kbd", version("loadkeys -V 2>&1"))
-   printversion("Console-tools", version("loadkeys -V 2>&1"))
-   printversion("Oprofile", version("oprofiled --version 2>&1"))
-   printversion("Sh-utils", version("expr --v 2>&1"))
-   printversion("Udev", version("udevadm --version 2>&1"))
-   printversion("Wireless-tools", version("iwconfig --version 2>&1"))
+   printversion("Procps", version("ps --version"))
+   printversion("Net-tools", version("ifconfig --version"))
+   printversion("Kbd", version("loadkeys -V"))
+   printversion("Console-tools", version("loadkeys -V"))
+   printversion("Oprofile", version("oprofiled --version"))
+   printversion("Sh-utils", version("expr --v"))
+   printversion("Udev", version("udevadm --version"))
+   printversion("Wireless-tools", version("iwconfig --version"))
 
if (system("test -r /proc/modules") == 0) {
while ("sort /proc/modules" | getline > 0) {
@@ -72,6 +72,7 @@ BEGIN {
 }
 
 function version(cmd,ver) {
+   cmd = cmd " 2>&1"
while (cmd | getline > 0) {
if (!/ver_linux/ && match($0, /[0-9]+([.]?[0-9]+)+/)) {
ver = substr($0, RSTART, RLENGTH)
-- 
2.17.0

[PATCH 1/2] ver_linux: Move stderr redirection from function parameter to function body

[Alexander Kapshuk]

Remove stderr redirection to stdout from all the parameters to the
version() function, and put it with the body of the version() function
instead.
This improves code readability.

Signed-off-by: Alexander Kapshuk 
---
 scripts/ver_linux | 53 ---
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/scripts/ver_linux b/scripts/ver_linux
index 545ec7388eb7..0b301bd1637d 100755
--- a/scripts/ver_linux
+++ b/scripts/ver_linux
@@ -13,23 +13,23 @@ BEGIN {
system("uname -a")
printf("\n")
 
-   printversion("GNU C", version("gcc -dumpversion 2>&1"))
-   printversion("GNU Make", version("make --version 2>&1"))
-   printversion("Binutils", version("ld -v 2>&1"))
-   printversion("Util-linux", version("mount --version 2>&1"))
-   printversion("Mount", version("mount --version 2>&1"))
-   printversion("Module-init-tools", version("depmod -V  2>&1"))
-   printversion("E2fsprogs", version("tune2fs 2>&1"))
-   printversion("Jfsutils", version("fsck.jfs -V 2>&1"))
-   printversion("Reiserfsprogs", version("reiserfsck -V 2>&1"))
-   printversion("Reiser4fsprogs", version("fsck.reiser4 -V 2>&1"))
-   printversion("Xfsprogs", version("xfs_db -V 2>&1"))
-   printversion("Pcmciautils", version("pccardctl -V 2>&1"))
-   printversion("Pcmcia-cs", version("cardmgr -V 2>&1"))
-   printversion("Quota-tools", version("quota -V 2>&1"))
-   printversion("PPP", version("pppd --version 2>&1"))
-   printversion("Isdn4k-utils", version("isdnctrl 2>&1"))
-   printversion("Nfs-utils", version("showmount --version 2>&1"))
+   printversion("GNU C", version("gcc -dumpversion"))
+   printversion("GNU Make", version("make --version"))
+   printversion("Binutils", version("ld -v"))
+   printversion("Util-linux", version("mount --version"))
+   printversion("Mount", version("mount --version"))
+   printversion("Module-init-tools", version("depmod -V"))
+   printversion("E2fsprogs", version("tune2fs"))
+   printversion("Jfsutils", version("fsck.jfs -V"))
+   printversion("Reiserfsprogs", version("reiserfsck -V"))
+   printversion("Reiser4fsprogs", version("fsck.reiser4 -V"))
+   printversion("Xfsprogs", version("xfs_db -V"))
+   printversion("Pcmciautils", version("pccardctl -V"))
+   printversion("Pcmcia-cs", version("cardmgr -V"))
+   printversion("Quota-tools", version("quota -V"))
+   printversion("PPP", version("pppd --version"))
+   printversion("Isdn4k-utils", version("isdnctrl"))
+   printversion("Nfs-utils", version("showmount --version"))
 
if (system("test -r /proc/self/maps") == 0) {
while (getline <"/proc/self/maps" > 0) {
@@ -42,7 +42,7 @@ BEGIN {
}
}
 
-   printversion("Dynamic linker (ldd)", version("ldd --version 2>&1"))
+   printversion("Dynamic linker (ldd)", version("ldd --version"))
 
while ("ldconfig -p 2>/dev/null" | getline > 0) {
if (/(libg|stdc)[+]+\.so/) {
@@ -53,14 +53,14 @@ BEGIN {
if (system("test -r " libcpp) == 0)
printversion("Linux C++ Library", version("readlink " libcpp))
 
-   printversion("Procps", version("ps --version 2>&1"))
-   printversion("Net-tools", version("ifconfig --version 2>&1"))
-   printversion("Kbd", version("loadkeys -V 2>&1"))
-   printversion("Console-tools", version("loadkeys -V 2>&1"))
-   printversion("Oprofile", version("oprofiled --version 2>&1"))
-   printversion("Sh-utils", version("expr --v 2>&1"))
-   printversion("Udev", version("udevadm --version 2>&1"))
-   printversion("Wireless-tools", version("iwconfig --version 2>&1"))
+   printversion("Procps", version("ps --version"))
+   printversion("Net-tools", version("ifconfig --version"))
+   printversion("Kbd", version("loadkeys -V"))
+   printversion("Console-tools", version("loadkeys -V"))
+   printversion("Oprofile", version("oprofiled --version"))
+   printversion("Sh-utils", version("expr --v"))
+   printversion("Udev", version("udevadm --version"))
+   printversion("Wireless-tools", version("iwconfig --version"))
 
if (system("test -r /proc/modules") == 0) {
while ("sort /proc/modules" | getline > 0) {
@@ -72,6 +72,7 @@ BEGIN {
 }
 
 function version(cmd,ver) {
+   cmd = cmd " 2>&1"
while (cmd | getline > 0) {
if (!/ver_linux/ && match($0, /[0-9]+([.]?[0-9]+)+/)) {
ver = substr($0, RSTART, RLENGTH)
-- 
2.17.0

Re: [PATCH 1/3] leaking_addresses: skip all /proc/PID except /proc/1

[Alexander Kapshuk]

On Thu, Mar 1, 2018 at 11:06 PM, Tobin C. Harding  wrote:
> On Tue, Feb 27, 2018 at 03:45:09PM +1100, Tobin C. Harding wrote:
>> When the system is idle it is likely that most files under /proc/PID
>> will be identical for various processes.  Scanning _all_ the PIDs under
>> /proc is unnecessary and implies that we are thoroughly scanning /proc.
>> This is _not_ the case because there may be ways userspace can trigger
>> creation of /proc files that leak addresses but were not present during
>> a scan.  For these two reasons we should exclude all PID directories
>> under /proc except '1/'
>>
>> Exclude all /proc/PID except /proc/1.
>>
>> Signed-off-by: Tobin C. Harding 
>> ---
>>  scripts/leaking_addresses.pl | 11 +++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> index 6e5bc57caeaa..fb40e2828f43 100755
>> --- a/scripts/leaking_addresses.pl
>> +++ b/scripts/leaking_addresses.pl
>> @@ -10,6 +10,14 @@
>>  # Use --debug to output path before parsing, this is useful to find files 
>> that
>>  # cause the script to choke.
>>
>> +#
>> +# When the system is idle it is likely that most files under /proc/PID will 
>> be
>> +# identical for various processes.  Scanning _all_ the PIDs under /proc is
>> +# unnecessary and implies that we are thoroughly scanning /proc.  This is 
>> _not_
>> +# the case because there may be ways userspace can trigger creation of /proc
>> +# files that leak addresses but were not present during a scan.  For these 
>> two
>> +# reasons we exclude all PID directories under /proc except '1/'
>> +
>>  use warnings;
>>  use strict;
>>  use POSIX;
>> @@ -472,6 +480,9 @@ sub walk
>>   my $path = "$pwd/$file";
>>   next if (-l $path);
>>
>> + # skip /proc/PID except /proc/1
>> + next if ($path =~ /\/proc\/(?:[2-9][0-9]*|1[0-9]+)/);
>
> Perhaps the intent of this is clearer?
>
> next if (($path =~ /^\/proc\/[0-9]+$/) &&
>  ($path !~ /^\/proc\/1$/));
>
>
> thanks,
> Tobin.

Hi Tobin,

The intent is crystal clear now. Thanks.

Here's something that generates the same output as the code above:
next if ($path !~ "^/proc/(1|[^0-9]+)\$");

I'm not insisting this be given any preference whatsoever.

Thanks.

Re: [PATCH 1/3] leaking_addresses: skip all /proc/PID except /proc/1

[Alexander Kapshuk]

On Thu, Mar 1, 2018 at 11:06 PM, Tobin C. Harding  wrote:
> On Tue, Feb 27, 2018 at 03:45:09PM +1100, Tobin C. Harding wrote:
>> When the system is idle it is likely that most files under /proc/PID
>> will be identical for various processes.  Scanning _all_ the PIDs under
>> /proc is unnecessary and implies that we are thoroughly scanning /proc.
>> This is _not_ the case because there may be ways userspace can trigger
>> creation of /proc files that leak addresses but were not present during
>> a scan.  For these two reasons we should exclude all PID directories
>> under /proc except '1/'
>>
>> Exclude all /proc/PID except /proc/1.
>>
>> Signed-off-by: Tobin C. Harding 
>> ---
>>  scripts/leaking_addresses.pl | 11 +++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> index 6e5bc57caeaa..fb40e2828f43 100755
>> --- a/scripts/leaking_addresses.pl
>> +++ b/scripts/leaking_addresses.pl
>> @@ -10,6 +10,14 @@
>>  # Use --debug to output path before parsing, this is useful to find files 
>> that
>>  # cause the script to choke.
>>
>> +#
>> +# When the system is idle it is likely that most files under /proc/PID will 
>> be
>> +# identical for various processes.  Scanning _all_ the PIDs under /proc is
>> +# unnecessary and implies that we are thoroughly scanning /proc.  This is 
>> _not_
>> +# the case because there may be ways userspace can trigger creation of /proc
>> +# files that leak addresses but were not present during a scan.  For these 
>> two
>> +# reasons we exclude all PID directories under /proc except '1/'
>> +
>>  use warnings;
>>  use strict;
>>  use POSIX;
>> @@ -472,6 +480,9 @@ sub walk
>>   my $path = "$pwd/$file";
>>   next if (-l $path);
>>
>> + # skip /proc/PID except /proc/1
>> + next if ($path =~ /\/proc\/(?:[2-9][0-9]*|1[0-9]+)/);
>
> Perhaps the intent of this is clearer?
>
> next if (($path =~ /^\/proc\/[0-9]+$/) &&
>  ($path !~ /^\/proc\/1$/));
>
>
> thanks,
> Tobin.

Hi Tobin,

The intent is crystal clear now. Thanks.

Here's something that generates the same output as the code above:
next if ($path !~ "^/proc/(1|[^0-9]+)\$");

I'm not insisting this be given any preference whatsoever.

Thanks.

Re: [PATCH 1/3] leaking_addresses: skip all /proc/PID except /proc/1

[Alexander Kapshuk]

On Tue, Feb 27, 2018 at 6:45 AM, Tobin C. Harding  wrote:
> When the system is idle it is likely that most files under /proc/PID
> will be identical for various processes.  Scanning _all_ the PIDs under
> /proc is unnecessary and implies that we are thoroughly scanning /proc.
> This is _not_ the case because there may be ways userspace can trigger
> creation of /proc files that leak addresses but were not present during
> a scan.  For these two reasons we should exclude all PID directories
> under /proc except '1/'
>
> Exclude all /proc/PID except /proc/1.
>
> Signed-off-by: Tobin C. Harding 
> ---
>  scripts/leaking_addresses.pl | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 6e5bc57caeaa..fb40e2828f43 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -10,6 +10,14 @@
>  # Use --debug to output path before parsing, this is useful to find files 
> that
>  # cause the script to choke.
>
> +#
> +# When the system is idle it is likely that most files under /proc/PID will 
> be
> +# identical for various processes.  Scanning _all_ the PIDs under /proc is
> +# unnecessary and implies that we are thoroughly scanning /proc.  This is 
> _not_
> +# the case because there may be ways userspace can trigger creation of /proc
> +# files that leak addresses but were not present during a scan.  For these 
> two
> +# reasons we exclude all PID directories under /proc except '1/'
> +
>  use warnings;
>  use strict;
>  use POSIX;
> @@ -472,6 +480,9 @@ sub walk
> my $path = "$pwd/$file";
> next if (-l $path);
>
> +   # skip /proc/PID except /proc/1
> +   next if ($path =~ /\/proc\/(?:[2-9][0-9]*|1[0-9]+)/);
> +
> next if (skip($path));
>
> if (-d $path) {
> --
> 2.7.4
>

Would something like this do the trick?
perl -e 'foreach my $dir (`ls -d /proc/[0-9]*`){next if($dir !~
"/proc/1\$"); print $dir}'
/proc/1

Re: [PATCH 1/3] leaking_addresses: skip all /proc/PID except /proc/1

[Alexander Kapshuk]

On Tue, Feb 27, 2018 at 6:45 AM, Tobin C. Harding  wrote:
> When the system is idle it is likely that most files under /proc/PID
> will be identical for various processes.  Scanning _all_ the PIDs under
> /proc is unnecessary and implies that we are thoroughly scanning /proc.
> This is _not_ the case because there may be ways userspace can trigger
> creation of /proc files that leak addresses but were not present during
> a scan.  For these two reasons we should exclude all PID directories
> under /proc except '1/'
>
> Exclude all /proc/PID except /proc/1.
>
> Signed-off-by: Tobin C. Harding 
> ---
>  scripts/leaking_addresses.pl | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 6e5bc57caeaa..fb40e2828f43 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -10,6 +10,14 @@
>  # Use --debug to output path before parsing, this is useful to find files 
> that
>  # cause the script to choke.
>
> +#
> +# When the system is idle it is likely that most files under /proc/PID will 
> be
> +# identical for various processes.  Scanning _all_ the PIDs under /proc is
> +# unnecessary and implies that we are thoroughly scanning /proc.  This is 
> _not_
> +# the case because there may be ways userspace can trigger creation of /proc
> +# files that leak addresses but were not present during a scan.  For these 
> two
> +# reasons we exclude all PID directories under /proc except '1/'
> +
>  use warnings;
>  use strict;
>  use POSIX;
> @@ -472,6 +480,9 @@ sub walk
> my $path = "$pwd/$file";
> next if (-l $path);
>
> +   # skip /proc/PID except /proc/1
> +   next if ($path =~ /\/proc\/(?:[2-9][0-9]*|1[0-9]+)/);
> +
> next if (skip($path));
>
> if (-d $path) {
> --
> 2.7.4
>

Would something like this do the trick?
perl -e 'foreach my $dir (`ls -d /proc/[0-9]*`){next if($dir !~
"/proc/1\$"); print $dir}'
/proc/1

Re: [PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

On Sat, Jan 6, 2018 at 10:00 PM, Hans de Goede <hdego...@redhat.com> wrote:
> Hi,
>
>
> On 06-01-18 20:56, Alexander Kapshuk wrote:
>>
>> On Sat, Jan 6, 2018 at 9:43 PM, Hans de Goede <hdego...@redhat.com> wrote:
>>>
>>> HI,
>>>
>>>
>>> On 06-01-18 20:30, Alexander Kapshuk wrote:
>>>>
>>>>
>>>> On Sat, Jan 6, 2018 at 6:00 PM, Hans de Goede <hdego...@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 06-01-18 15:20, Alexander Kapshuk wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Mon, Dec 25, 2017 at 04:42:59PM +0200, Alexander Kapshuk wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Sparse emitted the following warning:
>>>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect
>>>>>>> type
>>>>>>> in
>>>>>>> assignment (different address spaces)
>>>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char
>>>>>>> [noderef]
>>>>>>> *screen_base
>>>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual
>>>>>>>
>>>>>>> The vbox_bo buffer object kernel mapping is handled by a call
>>>>>>> to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
>>>>>>> info->screen_base of type char __iomem*.
>>>>>>> Casting bo->kmap.virtual to char __iomem* in this assignment fixes
>>>>>>> the warning.
>>>>>>>
>>>>>>> vboxvideo: Fix address space of expression removal sparse warning
>>>>>>>
>>>>>>> Sparse emitted the following warning:
>>>>>>> ../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes
>>>>>>> address space of expression
>>>>>>>
>>>>>>> vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
>>>>>>> from vbox_hw_init().
>>>>>>> __force attribute is used in assignment to vbva to fix the warning.
>>>>>>>
>>>>>>> Signed-off-by: Alexander Kapshuk <alexander.kaps...@gmail.com>
>>>>>>> ---
>>>>>>> drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
>>>>>>> drivers/staging/vboxvideo/vbox_main.c | 2 +-
>>>>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/staging/vboxvideo/vbox_fb.c
>>>>>>> b/drivers/staging/vboxvideo/vbox_fb.c
>>>>>>> index 8aed248db6e2..43c39eca4ae1 100644
>>>>>>> --- a/drivers/staging/vboxvideo/vbox_fb.c
>>>>>>> +++ b/drivers/staging/vboxvideo/vbox_fb.c
>>>>>>> @@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper
>>>>>>> *helper,
>>>>>>>   drm_fb_helper_fill_var(info, >helper,
>>>>>>> sizes->fb_width,
>>>>>>>  sizes->fb_height);
>>>>>>>
>>>>>>> -   info->screen_base = bo->kmap.virtual;
>>>>>>> +   info->screen_base = (char __iomem *)bo->kmap.virtual;
>>>>>>>   info->screen_size = size;
>>>>>>>
>>>>>>> #ifdef CONFIG_DRM_KMS_FB_HELPER
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> This fix looks good to me.
>>>>>
>>>>>>> diff --git a/drivers/staging/vboxvideo/vbox_main.c
>>>>>>> b/drivers/staging/vboxvideo/vbox_main.c
>>>>>>> index 80bd039fa08e..973b3bcc04b1 100644
>>>>>>> --- a/drivers/staging/vboxvideo/vbox_main.c
>>>>>>> +++ b/drivers/staging/vboxvideo/vbox_main.c
>>>>>>> @@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
>>>>>>>   if (vbox->vbva_info[i].vbva)
>>>>>>>   continue;
>>>>>>>
>>>>>>> -   vbva = (void *)vbox->vbva_buffers + i *
>>>>>>> 

Re: [PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

On Sat, Jan 6, 2018 at 10:00 PM, Hans de Goede  wrote:
> Hi,
>
>
> On 06-01-18 20:56, Alexander Kapshuk wrote:
>>
>> On Sat, Jan 6, 2018 at 9:43 PM, Hans de Goede  wrote:
>>>
>>> HI,
>>>
>>>
>>> On 06-01-18 20:30, Alexander Kapshuk wrote:
>>>>
>>>>
>>>> On Sat, Jan 6, 2018 at 6:00 PM, Hans de Goede 
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 06-01-18 15:20, Alexander Kapshuk wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Mon, Dec 25, 2017 at 04:42:59PM +0200, Alexander Kapshuk wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Sparse emitted the following warning:
>>>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect
>>>>>>> type
>>>>>>> in
>>>>>>> assignment (different address spaces)
>>>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char
>>>>>>> [noderef]
>>>>>>> *screen_base
>>>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual
>>>>>>>
>>>>>>> The vbox_bo buffer object kernel mapping is handled by a call
>>>>>>> to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
>>>>>>> info->screen_base of type char __iomem*.
>>>>>>> Casting bo->kmap.virtual to char __iomem* in this assignment fixes
>>>>>>> the warning.
>>>>>>>
>>>>>>> vboxvideo: Fix address space of expression removal sparse warning
>>>>>>>
>>>>>>> Sparse emitted the following warning:
>>>>>>> ../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes
>>>>>>> address space of expression
>>>>>>>
>>>>>>> vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
>>>>>>> from vbox_hw_init().
>>>>>>> __force attribute is used in assignment to vbva to fix the warning.
>>>>>>>
>>>>>>> Signed-off-by: Alexander Kapshuk 
>>>>>>> ---
>>>>>>> drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
>>>>>>> drivers/staging/vboxvideo/vbox_main.c | 2 +-
>>>>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/staging/vboxvideo/vbox_fb.c
>>>>>>> b/drivers/staging/vboxvideo/vbox_fb.c
>>>>>>> index 8aed248db6e2..43c39eca4ae1 100644
>>>>>>> --- a/drivers/staging/vboxvideo/vbox_fb.c
>>>>>>> +++ b/drivers/staging/vboxvideo/vbox_fb.c
>>>>>>> @@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper
>>>>>>> *helper,
>>>>>>>   drm_fb_helper_fill_var(info, >helper,
>>>>>>> sizes->fb_width,
>>>>>>>  sizes->fb_height);
>>>>>>>
>>>>>>> -   info->screen_base = bo->kmap.virtual;
>>>>>>> +   info->screen_base = (char __iomem *)bo->kmap.virtual;
>>>>>>>   info->screen_size = size;
>>>>>>>
>>>>>>> #ifdef CONFIG_DRM_KMS_FB_HELPER
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> This fix looks good to me.
>>>>>
>>>>>>> diff --git a/drivers/staging/vboxvideo/vbox_main.c
>>>>>>> b/drivers/staging/vboxvideo/vbox_main.c
>>>>>>> index 80bd039fa08e..973b3bcc04b1 100644
>>>>>>> --- a/drivers/staging/vboxvideo/vbox_main.c
>>>>>>> +++ b/drivers/staging/vboxvideo/vbox_main.c
>>>>>>> @@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
>>>>>>>   if (vbox->vbva_info[i].vbva)
>>>>>>>   continue;
>>>>>>>
>>>>>>> -   vbva = (void *)vbox->vbva_buffers + i *
>>>>>>> VBVA_MIN_BUFFER_SIZE;
>>>>>>> +   vbva = (void __force *)vbox->vbva_buffers + i *
>>>>>>> VBVA_MIN_BUFFER_SIZE;
>>>>>>>   if (!vbva_enable(>vbva_info[i],
>>>>>>>vbox->guest_pool, vbva, i)) {
>>>>>>>   /* very old host or driver error. */
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Hmm, isn't there a cleaner way to fix this ? Maybe make vbva_enable's
>>>>> argument (and the local vbva variable) of type u8 __iomem * too ?
>>>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>>
>>>>
>>>>
>>>> Hi Hans,
>>>>
>>>> Thanks for your prompt response.
>>>>
>>>> I had a good look at the vbva_enable() function's definition and to
>>>> the best of my knowledge, changing vbva's type from 'struct
>>>> vbva_buffer *' to 'u8 __iomem *' wouldn't work.
>>>> vbva_enable() relies on vbva's type being a pointer to 'struct
>>>> vbva_buffer':
>>>> vbva's memory gets set to zero;
>>>> some of vbva's members are initialised to particular values;
>>>> vbva_ctx->vbva expects vbva to be a pointer to 'struct vbva_buffer' as
>>>> well;
>>>>
>>>> Or am I misreading this?
>>>
>>>
>>>
>>> No your not misreading this I did not check the function myself before
>>> commenting myself, my bad.
>>
>>
>> Thanks for confirming my understanding of this.
>>
>>>
>>>> What are your thoughts on this?
>>>
>>>
>>>
>>> Lets just move ahead with your original fix:
>>
>>
>> Did you want me to resend the patch, or is someone going to pull the
>> original one I sent into their tree?
>> Thanks.
>
>
> I expect Greg to pick it up without a resend. But with all the Spectre
> stuff going on right now he might miss it, so I would say give it 14 days
> and if he has not picked it up by then, resend it with my reviewed-by
> added.
>
> Regards,
>
> Hans

Understood.
Thanks.

Re: [PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

On Sat, Jan 6, 2018 at 9:43 PM, Hans de Goede <hdego...@redhat.com> wrote:
> HI,
>
>
> On 06-01-18 20:30, Alexander Kapshuk wrote:
>>
>> On Sat, Jan 6, 2018 at 6:00 PM, Hans de Goede <hdego...@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 06-01-18 15:20, Alexander Kapshuk wrote:
>>>>
>>>>
>>>> On Mon, Dec 25, 2017 at 04:42:59PM +0200, Alexander Kapshuk wrote:
>>>>>
>>>>>
>>>>> Sparse emitted the following warning:
>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect type
>>>>> in
>>>>> assignment (different address spaces)
>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char
>>>>> [noderef]
>>>>> *screen_base
>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual
>>>>>
>>>>> The vbox_bo buffer object kernel mapping is handled by a call
>>>>> to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
>>>>> info->screen_base of type char __iomem*.
>>>>> Casting bo->kmap.virtual to char __iomem* in this assignment fixes
>>>>> the warning.
>>>>>
>>>>> vboxvideo: Fix address space of expression removal sparse warning
>>>>>
>>>>> Sparse emitted the following warning:
>>>>> ../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes
>>>>> address space of expression
>>>>>
>>>>> vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
>>>>> from vbox_hw_init().
>>>>> __force attribute is used in assignment to vbva to fix the warning.
>>>>>
>>>>> Signed-off-by: Alexander Kapshuk <alexander.kaps...@gmail.com>
>>>>> ---
>>>>>drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
>>>>>drivers/staging/vboxvideo/vbox_main.c | 2 +-
>>>>>2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/vboxvideo/vbox_fb.c
>>>>> b/drivers/staging/vboxvideo/vbox_fb.c
>>>>> index 8aed248db6e2..43c39eca4ae1 100644
>>>>> --- a/drivers/staging/vboxvideo/vbox_fb.c
>>>>> +++ b/drivers/staging/vboxvideo/vbox_fb.c
>>>>> @@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper
>>>>> *helper,
>>>>>  drm_fb_helper_fill_var(info, >helper, sizes->fb_width,
>>>>> sizes->fb_height);
>>>>>
>>>>> -   info->screen_base = bo->kmap.virtual;
>>>>> +   info->screen_base = (char __iomem *)bo->kmap.virtual;
>>>>>  info->screen_size = size;
>>>>>
>>>>>#ifdef CONFIG_DRM_KMS_FB_HELPER
>>>
>>>
>>>
>>> This fix looks good to me.
>>>
>>>>> diff --git a/drivers/staging/vboxvideo/vbox_main.c
>>>>> b/drivers/staging/vboxvideo/vbox_main.c
>>>>> index 80bd039fa08e..973b3bcc04b1 100644
>>>>> --- a/drivers/staging/vboxvideo/vbox_main.c
>>>>> +++ b/drivers/staging/vboxvideo/vbox_main.c
>>>>> @@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
>>>>>  if (vbox->vbva_info[i].vbva)
>>>>>  continue;
>>>>>
>>>>> -   vbva = (void *)vbox->vbva_buffers + i *
>>>>> VBVA_MIN_BUFFER_SIZE;
>>>>> +   vbva = (void __force *)vbox->vbva_buffers + i *
>>>>> VBVA_MIN_BUFFER_SIZE;
>>>>>  if (!vbva_enable(>vbva_info[i],
>>>>>   vbox->guest_pool, vbva, i)) {
>>>>>  /* very old host or driver error. */
>>>
>>>
>>>
>>> Hmm, isn't there a cleaner way to fix this ? Maybe make vbva_enable's
>>> argument (and the local vbva variable) of type u8 __iomem * too ?
>>>
>>> Regards,
>>>
>>> Hans
>>
>>
>> Hi Hans,
>>
>> Thanks for your prompt response.
>>
>> I had a good look at the vbva_enable() function's definition and to
>> the best of my knowledge, changing vbva's type from 'struct
>> vbva_buffer *' to 'u8 __iomem *' wouldn't work.
>> vbva_enable() relies on vbva's type being a pointer to 'struct
>> vbva_buffer':
>> vbva's memory gets set to zero;
>> some of vbva's members are initialised to particular values;
>> vbva_ctx->vbva expects vbva to be a pointer to 'struct vbva_buffer' as
>> well;
>>
>> Or am I misreading this?
>
>
> No your not misreading this I did not check the function myself before
> commenting myself, my bad.

Thanks for confirming my understanding of this.

>
>> What are your thoughts on this?
>
>
> Lets just move ahead with your original fix:

Did you want me to resend the patch, or is someone going to pull the
original one I sent into their tree?
Thanks.

>
> Reviewed-by: Hans de Goede <hdego...@redhat.com>
>
> Regards,
>
> Hans
>

Re: [PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

On Sat, Jan 6, 2018 at 9:43 PM, Hans de Goede  wrote:
> HI,
>
>
> On 06-01-18 20:30, Alexander Kapshuk wrote:
>>
>> On Sat, Jan 6, 2018 at 6:00 PM, Hans de Goede  wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 06-01-18 15:20, Alexander Kapshuk wrote:
>>>>
>>>>
>>>> On Mon, Dec 25, 2017 at 04:42:59PM +0200, Alexander Kapshuk wrote:
>>>>>
>>>>>
>>>>> Sparse emitted the following warning:
>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect type
>>>>> in
>>>>> assignment (different address spaces)
>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char
>>>>> [noderef]
>>>>> *screen_base
>>>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual
>>>>>
>>>>> The vbox_bo buffer object kernel mapping is handled by a call
>>>>> to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
>>>>> info->screen_base of type char __iomem*.
>>>>> Casting bo->kmap.virtual to char __iomem* in this assignment fixes
>>>>> the warning.
>>>>>
>>>>> vboxvideo: Fix address space of expression removal sparse warning
>>>>>
>>>>> Sparse emitted the following warning:
>>>>> ../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes
>>>>> address space of expression
>>>>>
>>>>> vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
>>>>> from vbox_hw_init().
>>>>> __force attribute is used in assignment to vbva to fix the warning.
>>>>>
>>>>> Signed-off-by: Alexander Kapshuk 
>>>>> ---
>>>>>drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
>>>>>drivers/staging/vboxvideo/vbox_main.c | 2 +-
>>>>>2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/vboxvideo/vbox_fb.c
>>>>> b/drivers/staging/vboxvideo/vbox_fb.c
>>>>> index 8aed248db6e2..43c39eca4ae1 100644
>>>>> --- a/drivers/staging/vboxvideo/vbox_fb.c
>>>>> +++ b/drivers/staging/vboxvideo/vbox_fb.c
>>>>> @@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper
>>>>> *helper,
>>>>>  drm_fb_helper_fill_var(info, >helper, sizes->fb_width,
>>>>> sizes->fb_height);
>>>>>
>>>>> -   info->screen_base = bo->kmap.virtual;
>>>>> +   info->screen_base = (char __iomem *)bo->kmap.virtual;
>>>>>  info->screen_size = size;
>>>>>
>>>>>#ifdef CONFIG_DRM_KMS_FB_HELPER
>>>
>>>
>>>
>>> This fix looks good to me.
>>>
>>>>> diff --git a/drivers/staging/vboxvideo/vbox_main.c
>>>>> b/drivers/staging/vboxvideo/vbox_main.c
>>>>> index 80bd039fa08e..973b3bcc04b1 100644
>>>>> --- a/drivers/staging/vboxvideo/vbox_main.c
>>>>> +++ b/drivers/staging/vboxvideo/vbox_main.c
>>>>> @@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
>>>>>  if (vbox->vbva_info[i].vbva)
>>>>>  continue;
>>>>>
>>>>> -   vbva = (void *)vbox->vbva_buffers + i *
>>>>> VBVA_MIN_BUFFER_SIZE;
>>>>> +   vbva = (void __force *)vbox->vbva_buffers + i *
>>>>> VBVA_MIN_BUFFER_SIZE;
>>>>>  if (!vbva_enable(>vbva_info[i],
>>>>>   vbox->guest_pool, vbva, i)) {
>>>>>  /* very old host or driver error. */
>>>
>>>
>>>
>>> Hmm, isn't there a cleaner way to fix this ? Maybe make vbva_enable's
>>> argument (and the local vbva variable) of type u8 __iomem * too ?
>>>
>>> Regards,
>>>
>>> Hans
>>
>>
>> Hi Hans,
>>
>> Thanks for your prompt response.
>>
>> I had a good look at the vbva_enable() function's definition and to
>> the best of my knowledge, changing vbva's type from 'struct
>> vbva_buffer *' to 'u8 __iomem *' wouldn't work.
>> vbva_enable() relies on vbva's type being a pointer to 'struct
>> vbva_buffer':
>> vbva's memory gets set to zero;
>> some of vbva's members are initialised to particular values;
>> vbva_ctx->vbva expects vbva to be a pointer to 'struct vbva_buffer' as
>> well;
>>
>> Or am I misreading this?
>
>
> No your not misreading this I did not check the function myself before
> commenting myself, my bad.

Thanks for confirming my understanding of this.

>
>> What are your thoughts on this?
>
>
> Lets just move ahead with your original fix:

Did you want me to resend the patch, or is someone going to pull the
original one I sent into their tree?
Thanks.

>
> Reviewed-by: Hans de Goede 
>
> Regards,
>
> Hans
>

[PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

On Sat, Jan 6, 2018 at 6:00 PM, Hans de Goede <hdego...@redhat.com> wrote:
> Hi,
>
>
> On 06-01-18 15:20, Alexander Kapshuk wrote:
>>
>> On Mon, Dec 25, 2017 at 04:42:59PM +0200, Alexander Kapshuk wrote:
>>>
>>> Sparse emitted the following warning:
>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect type in
>>> assignment (different address spaces)
>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char [noderef]
>>> *screen_base
>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual
>>>
>>> The vbox_bo buffer object kernel mapping is handled by a call
>>> to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
>>> info->screen_base of type char __iomem*.
>>> Casting bo->kmap.virtual to char __iomem* in this assignment fixes
>>> the warning.
>>>
>>> vboxvideo: Fix address space of expression removal sparse warning
>>>
>>> Sparse emitted the following warning:
>>> ../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes
>>> address space of expression
>>>
>>> vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
>>> from vbox_hw_init().
>>> __force attribute is used in assignment to vbva to fix the warning.
>>>
>>> Signed-off-by: Alexander Kapshuk <alexander.kaps...@gmail.com>
>>> ---
>>>   drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
>>>   drivers/staging/vboxvideo/vbox_main.c | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/vboxvideo/vbox_fb.c
>>> b/drivers/staging/vboxvideo/vbox_fb.c
>>> index 8aed248db6e2..43c39eca4ae1 100644
>>> --- a/drivers/staging/vboxvideo/vbox_fb.c
>>> +++ b/drivers/staging/vboxvideo/vbox_fb.c
>>> @@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper
>>> *helper,
>>> drm_fb_helper_fill_var(info, >helper, sizes->fb_width,
>>>sizes->fb_height);
>>>
>>> -   info->screen_base = bo->kmap.virtual;
>>> +   info->screen_base = (char __iomem *)bo->kmap.virtual;
>>> info->screen_size = size;
>>>
>>>   #ifdef CONFIG_DRM_KMS_FB_HELPER
>
>
> This fix looks good to me.
>
>>> diff --git a/drivers/staging/vboxvideo/vbox_main.c
>>> b/drivers/staging/vboxvideo/vbox_main.c
>>> index 80bd039fa08e..973b3bcc04b1 100644
>>> --- a/drivers/staging/vboxvideo/vbox_main.c
>>> +++ b/drivers/staging/vboxvideo/vbox_main.c
>>> @@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
>>> if (vbox->vbva_info[i].vbva)
>>> continue;
>>>
>>> -   vbva = (void *)vbox->vbva_buffers + i *
>>> VBVA_MIN_BUFFER_SIZE;
>>> +   vbva = (void __force *)vbox->vbva_buffers + i *
>>> VBVA_MIN_BUFFER_SIZE;
>>> if (!vbva_enable(>vbva_info[i],
>>>  vbox->guest_pool, vbva, i)) {
>>> /* very old host or driver error. */
>
>
> Hmm, isn't there a cleaner way to fix this ? Maybe make vbva_enable's
> argument (and the local vbva variable) of type u8 __iomem * too ?
>
> Regards,
>
> Hans

Hi Hans,

Thanks for your prompt response.

I had a good look at the vbva_enable() function's definition and to
the best of my knowledge, changing vbva's type from 'struct
vbva_buffer *' to 'u8 __iomem *' wouldn't work.
vbva_enable() relies on vbva's type being a pointer to 'struct vbva_buffer':
vbva's memory gets set to zero;
some of vbva's members are initialised to particular values;
vbva_ctx->vbva expects vbva to be a pointer to 'struct vbva_buffer' as well;

Or am I misreading this?
What are your thoughts on this?
Thanks.

Alexander.

[PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

On Sat, Jan 6, 2018 at 6:00 PM, Hans de Goede  wrote:
> Hi,
>
>
> On 06-01-18 15:20, Alexander Kapshuk wrote:
>>
>> On Mon, Dec 25, 2017 at 04:42:59PM +0200, Alexander Kapshuk wrote:
>>>
>>> Sparse emitted the following warning:
>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect type in
>>> assignment (different address spaces)
>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char [noderef]
>>> *screen_base
>>> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual
>>>
>>> The vbox_bo buffer object kernel mapping is handled by a call
>>> to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
>>> info->screen_base of type char __iomem*.
>>> Casting bo->kmap.virtual to char __iomem* in this assignment fixes
>>> the warning.
>>>
>>> vboxvideo: Fix address space of expression removal sparse warning
>>>
>>> Sparse emitted the following warning:
>>> ../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes
>>> address space of expression
>>>
>>> vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
>>> from vbox_hw_init().
>>> __force attribute is used in assignment to vbva to fix the warning.
>>>
>>> Signed-off-by: Alexander Kapshuk 
>>> ---
>>>   drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
>>>   drivers/staging/vboxvideo/vbox_main.c | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/vboxvideo/vbox_fb.c
>>> b/drivers/staging/vboxvideo/vbox_fb.c
>>> index 8aed248db6e2..43c39eca4ae1 100644
>>> --- a/drivers/staging/vboxvideo/vbox_fb.c
>>> +++ b/drivers/staging/vboxvideo/vbox_fb.c
>>> @@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper
>>> *helper,
>>> drm_fb_helper_fill_var(info, >helper, sizes->fb_width,
>>>sizes->fb_height);
>>>
>>> -   info->screen_base = bo->kmap.virtual;
>>> +   info->screen_base = (char __iomem *)bo->kmap.virtual;
>>> info->screen_size = size;
>>>
>>>   #ifdef CONFIG_DRM_KMS_FB_HELPER
>
>
> This fix looks good to me.
>
>>> diff --git a/drivers/staging/vboxvideo/vbox_main.c
>>> b/drivers/staging/vboxvideo/vbox_main.c
>>> index 80bd039fa08e..973b3bcc04b1 100644
>>> --- a/drivers/staging/vboxvideo/vbox_main.c
>>> +++ b/drivers/staging/vboxvideo/vbox_main.c
>>> @@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
>>> if (vbox->vbva_info[i].vbva)
>>> continue;
>>>
>>> -   vbva = (void *)vbox->vbva_buffers + i *
>>> VBVA_MIN_BUFFER_SIZE;
>>> +   vbva = (void __force *)vbox->vbva_buffers + i *
>>> VBVA_MIN_BUFFER_SIZE;
>>> if (!vbva_enable(>vbva_info[i],
>>>  vbox->guest_pool, vbva, i)) {
>>> /* very old host or driver error. */
>
>
> Hmm, isn't there a cleaner way to fix this ? Maybe make vbva_enable's
> argument (and the local vbva variable) of type u8 __iomem * too ?
>
> Regards,
>
> Hans

Hi Hans,

Thanks for your prompt response.

I had a good look at the vbva_enable() function's definition and to
the best of my knowledge, changing vbva's type from 'struct
vbva_buffer *' to 'u8 __iomem *' wouldn't work.
vbva_enable() relies on vbva's type being a pointer to 'struct vbva_buffer':
vbva's memory gets set to zero;
some of vbva's members are initialised to particular values;
vbva_ctx->vbva expects vbva to be a pointer to 'struct vbva_buffer' as well;

Or am I misreading this?
What are your thoughts on this?
Thanks.

Alexander.

[PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

On Mon, Dec 25, 2017 at 04:42:59PM +0200, Alexander Kapshuk wrote:
> Sparse emitted the following warning:
> ../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect type in 
> assignment (different address spaces)
> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char [noderef] 
> *screen_base
> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual
> 
> The vbox_bo buffer object kernel mapping is handled by a call
> to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
> info->screen_base of type char __iomem*.
> Casting bo->kmap.virtual to char __iomem* in this assignment fixes
> the warning.
> 
> vboxvideo: Fix address space of expression removal sparse warning
> 
> Sparse emitted the following warning:
> ../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes address 
> space of expression
> 
> vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
> from vbox_hw_init().
> __force attribute is used in assignment to vbva to fix the warning.
> 
> Signed-off-by: Alexander Kapshuk <alexander.kaps...@gmail.com>
> ---
>  drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
>  drivers/staging/vboxvideo/vbox_main.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/vboxvideo/vbox_fb.c 
> b/drivers/staging/vboxvideo/vbox_fb.c
> index 8aed248db6e2..43c39eca4ae1 100644
> --- a/drivers/staging/vboxvideo/vbox_fb.c
> +++ b/drivers/staging/vboxvideo/vbox_fb.c
> @@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper *helper,
>   drm_fb_helper_fill_var(info, >helper, sizes->fb_width,
>  sizes->fb_height);
> 
> - info->screen_base = bo->kmap.virtual;
> + info->screen_base = (char __iomem *)bo->kmap.virtual;
>   info->screen_size = size;
> 
>  #ifdef CONFIG_DRM_KMS_FB_HELPER
> diff --git a/drivers/staging/vboxvideo/vbox_main.c 
> b/drivers/staging/vboxvideo/vbox_main.c
> index 80bd039fa08e..973b3bcc04b1 100644
> --- a/drivers/staging/vboxvideo/vbox_main.c
> +++ b/drivers/staging/vboxvideo/vbox_main.c
> @@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
>   if (vbox->vbva_info[i].vbva)
>   continue;
> 
> - vbva = (void *)vbox->vbva_buffers + i * VBVA_MIN_BUFFER_SIZE;
> + vbva = (void __force *)vbox->vbva_buffers + i * 
> VBVA_MIN_BUFFER_SIZE;
>   if (!vbva_enable(>vbva_info[i],
>vbox->guest_pool, vbva, i)) {
>   /* very old host or driver error. */
> --
> 2.13.6
> 
Ping.

Could someone please comment on the patch above?

Thanks.

[PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

On Mon, Dec 25, 2017 at 04:42:59PM +0200, Alexander Kapshuk wrote:
> Sparse emitted the following warning:
> ../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect type in 
> assignment (different address spaces)
> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char [noderef] 
> *screen_base
> ../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual
> 
> The vbox_bo buffer object kernel mapping is handled by a call
> to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
> info->screen_base of type char __iomem*.
> Casting bo->kmap.virtual to char __iomem* in this assignment fixes
> the warning.
> 
> vboxvideo: Fix address space of expression removal sparse warning
> 
> Sparse emitted the following warning:
> ../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes address 
> space of expression
> 
> vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
> from vbox_hw_init().
> __force attribute is used in assignment to vbva to fix the warning.
> 
> Signed-off-by: Alexander Kapshuk 
> ---
>  drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
>  drivers/staging/vboxvideo/vbox_main.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/vboxvideo/vbox_fb.c 
> b/drivers/staging/vboxvideo/vbox_fb.c
> index 8aed248db6e2..43c39eca4ae1 100644
> --- a/drivers/staging/vboxvideo/vbox_fb.c
> +++ b/drivers/staging/vboxvideo/vbox_fb.c
> @@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper *helper,
>   drm_fb_helper_fill_var(info, >helper, sizes->fb_width,
>  sizes->fb_height);
> 
> - info->screen_base = bo->kmap.virtual;
> + info->screen_base = (char __iomem *)bo->kmap.virtual;
>   info->screen_size = size;
> 
>  #ifdef CONFIG_DRM_KMS_FB_HELPER
> diff --git a/drivers/staging/vboxvideo/vbox_main.c 
> b/drivers/staging/vboxvideo/vbox_main.c
> index 80bd039fa08e..973b3bcc04b1 100644
> --- a/drivers/staging/vboxvideo/vbox_main.c
> +++ b/drivers/staging/vboxvideo/vbox_main.c
> @@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
>   if (vbox->vbva_info[i].vbva)
>   continue;
> 
> - vbva = (void *)vbox->vbva_buffers + i * VBVA_MIN_BUFFER_SIZE;
> + vbva = (void __force *)vbox->vbva_buffers + i * 
> VBVA_MIN_BUFFER_SIZE;
>   if (!vbva_enable(>vbva_info[i],
>vbox->guest_pool, vbva, i)) {
>   /* very old host or driver error. */
> --
> 2.13.6
> 
Ping.

Could someone please comment on the patch above?

Thanks.

[PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

Sparse emitted the following warning:
../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect type in 
assignment (different address spaces)
../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char [noderef] 
*screen_base
../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual

The vbox_bo buffer object kernel mapping is handled by a call
to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
info->screen_base of type char __iomem*.
Casting bo->kmap.virtual to char __iomem* in this assignment fixes
the warning.

vboxvideo: Fix address space of expression removal sparse warning

Sparse emitted the following warning:
../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes address 
space of expression

vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
from vbox_hw_init().
__force attribute is used in assignment to vbva to fix the warning.

Signed-off-by: Alexander Kapshuk <alexander.kaps...@gmail.com>
---
 drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
 drivers/staging/vboxvideo/vbox_main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_fb.c 
b/drivers/staging/vboxvideo/vbox_fb.c
index 8aed248db6e2..43c39eca4ae1 100644
--- a/drivers/staging/vboxvideo/vbox_fb.c
+++ b/drivers/staging/vboxvideo/vbox_fb.c
@@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper *helper,
drm_fb_helper_fill_var(info, >helper, sizes->fb_width,
   sizes->fb_height);

-   info->screen_base = bo->kmap.virtual;
+   info->screen_base = (char __iomem *)bo->kmap.virtual;
info->screen_size = size;

 #ifdef CONFIG_DRM_KMS_FB_HELPER
diff --git a/drivers/staging/vboxvideo/vbox_main.c 
b/drivers/staging/vboxvideo/vbox_main.c
index 80bd039fa08e..973b3bcc04b1 100644
--- a/drivers/staging/vboxvideo/vbox_main.c
+++ b/drivers/staging/vboxvideo/vbox_main.c
@@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
if (vbox->vbva_info[i].vbva)
continue;

-   vbva = (void *)vbox->vbva_buffers + i * VBVA_MIN_BUFFER_SIZE;
+   vbva = (void __force *)vbox->vbva_buffers + i * 
VBVA_MIN_BUFFER_SIZE;
if (!vbva_enable(>vbva_info[i],
 vbox->guest_pool, vbva, i)) {
/* very old host or driver error. */
--
2.13.6

[PATCH] vboxvideo: Fix incorrect type in assignment sparse warning

[Alexander Kapshuk]

Sparse emitted the following warning:
../drivers/staging/vboxvideo/vbox_fb.c:173:27: warning: incorrect type in 
assignment (different address spaces)
../drivers/staging/vboxvideo/vbox_fb.c:173:27:expected char [noderef] 
*screen_base
../drivers/staging/vboxvideo/vbox_fb.c:173:27:got void *virtual

The vbox_bo buffer object kernel mapping is handled by a call
to ttm_bo_kmap() prior to the assignment of bo->kmap.virtual to
info->screen_base of type char __iomem*.
Casting bo->kmap.virtual to char __iomem* in this assignment fixes
the warning.

vboxvideo: Fix address space of expression removal sparse warning

Sparse emitted the following warning:
../drivers/staging/vboxvideo/vbox_main.c:64:25: warning: cast removes address 
space of expression

vbox->vbva_buffers iomapping is handled by calling vbox_accel_init()
from vbox_hw_init().
__force attribute is used in assignment to vbva to fix the warning.

Signed-off-by: Alexander Kapshuk 
---
 drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
 drivers/staging/vboxvideo/vbox_main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_fb.c 
b/drivers/staging/vboxvideo/vbox_fb.c
index 8aed248db6e2..43c39eca4ae1 100644
--- a/drivers/staging/vboxvideo/vbox_fb.c
+++ b/drivers/staging/vboxvideo/vbox_fb.c
@@ -170,7 +170,7 @@ static int vboxfb_create(struct drm_fb_helper *helper,
drm_fb_helper_fill_var(info, >helper, sizes->fb_width,
   sizes->fb_height);

-   info->screen_base = bo->kmap.virtual;
+   info->screen_base = (char __iomem *)bo->kmap.virtual;
info->screen_size = size;

 #ifdef CONFIG_DRM_KMS_FB_HELPER
diff --git a/drivers/staging/vboxvideo/vbox_main.c 
b/drivers/staging/vboxvideo/vbox_main.c
index 80bd039fa08e..973b3bcc04b1 100644
--- a/drivers/staging/vboxvideo/vbox_main.c
+++ b/drivers/staging/vboxvideo/vbox_main.c
@@ -61,7 +61,7 @@ void vbox_enable_accel(struct vbox_private *vbox)
if (vbox->vbva_info[i].vbva)
continue;

-   vbva = (void *)vbox->vbva_buffers + i * VBVA_MIN_BUFFER_SIZE;
+   vbva = (void __force *)vbox->vbva_buffers + i * 
VBVA_MIN_BUFFER_SIZE;
if (!vbva_enable(>vbva_info[i],
 vbox->guest_pool, vbva, i)) {
/* very old host or driver error. */
--
2.13.6

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Mon, Dec 4, 2017 at 12:20 PM,   wrote:
> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
>> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
>> > > ---
>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> > index 9906dcf8b807..260b52e456f1 100755
>> > --- a/scripts/leaking_addresses.pl
>> > +++ b/scripts/leaking_addresses.pl
>> > @@ -266,7 +266,7 @@ sub is_false_positive
>> >  sub is_false_positive_ix86_32
>> >  {
>> > my ($match) = @_;
>> > -   state $page_offset = eval get_page_offset(); # only gets called 
>> > once
>> > +   state $page_offset = hex get_page_offset(); # only gets called once
>>
>> I don't think this is valid ;) I meant use hex() to convert the string
>> to an int so it doesn't throw the warning (inside get_page_offset()).
>
> Yup, got it, thanks   :-p
> Combined patch below:
>
>
> ---
>  scripts/leaking_addresses.pl | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 9906dcf8b807..a595a2c66b12 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -266,8 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
> my ($match) = @_;
> -   state $page_offset = eval get_page_offset(); # only gets called once
> -
> +   state $page_offset = get_page_offset(); # only gets called once
> if ($match =~ '\b(0x)?(f|F){8}\b') {
> return 1;
> }
> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>  sub get_page_offset
>  {
> my $page_offset;
> -   my $default_offset = "0xc000";
> +   my $default_offset = hex("0xc000");
> my @config_files;
>
> # Allow --page-offset-32bit to override.
> @@ -306,23 +305,23 @@ sub get_page_offset
> } else {
> $page_offset = parse_kernel_config_file($tmp_file);
> if ($page_offset ne "") {
> -   return $page_offset;
> +   return hex($page_offset);
> }
> }
> system("rm -f $tmp_file");
> }
>
> foreach my $config_file (@config_files) {
> -   $config_file =~ s/\R*//g;
> +   chomp $config_file;
> $page_offset = parse_kernel_config_file($config_file);
> if ($page_offset ne "") {
> -   return $page_offset;
> +   return hex($page_offset);
> }
> }
>
> printf STDERR "\nFailed to parse kernel config files\n";
> printf STDERR "*** NOTE ***\n";
> -   printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
> +   printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", 
> $default_offset;

Better use the '#' flag with the 'x' conversion specifier:
perl -e 'my $default_offset = hex("0xc000");printf "%#x\n", $default_offset'
0xc000

>
> return $default_offset;
>  }
> --
> 2.14.3
>
> Thanks,
> Kaiwan.
>
>> thanks,
>> Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Mon, Dec 4, 2017 at 12:20 PM,   wrote:
> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
>> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
>> > > ---
>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> > index 9906dcf8b807..260b52e456f1 100755
>> > --- a/scripts/leaking_addresses.pl
>> > +++ b/scripts/leaking_addresses.pl
>> > @@ -266,7 +266,7 @@ sub is_false_positive
>> >  sub is_false_positive_ix86_32
>> >  {
>> > my ($match) = @_;
>> > -   state $page_offset = eval get_page_offset(); # only gets called 
>> > once
>> > +   state $page_offset = hex get_page_offset(); # only gets called once
>>
>> I don't think this is valid ;) I meant use hex() to convert the string
>> to an int so it doesn't throw the warning (inside get_page_offset()).
>
> Yup, got it, thanks   :-p
> Combined patch below:
>
>
> ---
>  scripts/leaking_addresses.pl | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 9906dcf8b807..a595a2c66b12 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -266,8 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
> my ($match) = @_;
> -   state $page_offset = eval get_page_offset(); # only gets called once
> -
> +   state $page_offset = get_page_offset(); # only gets called once
> if ($match =~ '\b(0x)?(f|F){8}\b') {
> return 1;
> }
> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>  sub get_page_offset
>  {
> my $page_offset;
> -   my $default_offset = "0xc000";
> +   my $default_offset = hex("0xc000");
> my @config_files;
>
> # Allow --page-offset-32bit to override.
> @@ -306,23 +305,23 @@ sub get_page_offset
> } else {
> $page_offset = parse_kernel_config_file($tmp_file);
> if ($page_offset ne "") {
> -   return $page_offset;
> +   return hex($page_offset);
> }
> }
> system("rm -f $tmp_file");
> }
>
> foreach my $config_file (@config_files) {
> -   $config_file =~ s/\R*//g;
> +   chomp $config_file;
> $page_offset = parse_kernel_config_file($config_file);
> if ($page_offset ne "") {
> -   return $page_offset;
> +   return hex($page_offset);
> }
> }
>
> printf STDERR "\nFailed to parse kernel config files\n";
> printf STDERR "*** NOTE ***\n";
> -   printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
> +   printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", 
> $default_offset;

Better use the '#' flag with the 'x' conversion specifier:
perl -e 'my $default_offset = hex("0xc000");printf "%#x\n", $default_offset'
0xc000

>
> return $default_offset;
>  }
> --
> 2.14.3
>
> Thanks,
> Kaiwan.
>
>> thanks,
>> Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Wed, Nov 29, 2017 at 12:16 PM, Tobin C. Harding <m...@tobin.cc> wrote:
> On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <m...@tobin.cc> wrote:
>> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <m...@tobin.cc> wrote:
>> >> > Currently, leaking_addresses.pl only supports scanning 64 bit
>> >> > architectures. This is due to how the regular expressions are formed. We
>> >> > can do better than this. 32 architectures can be supported if we take
>> >> > into consideration the kernel virtual address split.
>> >> >
>> >> > Add support for ix86 32 bit architectures.
>> >> >  - Add command line option for page offset.
>> >> >  - Add command line option for kernel configuration file.
>> >> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>> >> >  - Use page offset when checking for kernel virtual addresses.
>> >> >
>> >> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimo...@gmail.com>
>> >> > Signed-off-by: Tobin C. Harding <m...@tobin.cc>
>> >> > ---
>> >> >
>> >> > As discussed this is a patch based on Kaiwan's previous patch. This
>> >> > patch represents co development by Kaiwan and Tobin.
>> >> >
>> >> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>> >> >
>> >> > thanks,
>> >> > Tobin.
>> >> >
>> >> >  scripts/leaking_addresses.pl | 168 
>> >> > +--
>> >> >  1 file changed, 148 insertions(+), 20 deletions(-)
>> >> >
>> >> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> >> > index bc578818..f03f2f140e0a 100755
>> >> > --- a/scripts/leaking_addresses.pl
>> >> > +++ b/scripts/leaking_addresses.pl
>> >> > @@ -1,9 +1,11 @@
>> >> >  #!/usr/bin/env perl
>> >> >  #
>> >> >  # (c) 2017 Tobin C. Harding <m...@tobin.cc>
>> >> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimo...@gmail.com> (ix86 
>> >> > stuff)
>> >> > +#
>> >> >  # Licensed under the terms of the GNU GPL License version 2
>> >> >  #
>> >> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking 
>> >> > addresses.
>> >> > +# leaking_addresses.pl: Scan the kernel for potential leaking 
>> >> > addresses.
>> >> >  #  - Scans dmesg output.
>> >> >  #  - Walks directory tree and parses each file (for each directory in 
>> >> > @DIRS).
>> >> >  #
>> >> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>> >> >  use Term::ANSIColor qw(:constants);
>> >> >  use Getopt::Long qw(:config no_auto_abbrev);
>> >> >  use Config;
>> >> > +use feature 'state';
>> >> >
>> >> >  my $P = $0;
>> >> >  my $V = '0.01';
>> >> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>> >> >  # Script can only grep for kernel addresses on the following 
>> >> > architectures. If
>> >> >  # your architecture is not listed here and has a grep'able kernel 
>> >> > address please
>> >> >  # consider submitting a patch.
>> >> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
>> >> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>> >> >
>> >> >  # Command line options.
>> >> >  my $help = 0;
>> >> >  my $debug = 0;
>> >> > -my $raw = 0;
>> >> > -my $output_raw = "";   # Write raw results to file.
>> >> > -my $input_raw = "";# Read raw results from file instead of 
>> >> > scanning.
>> >> > -
>> >> > +my $raw = 0;   # Show raw output.
>> >> > +my $output_raw = "";   # Write raw results to file.
>> >> > +my $input_raw = "";# Read raw results from file instead of 
>> >> > scanning.
>> >> >  my $suppress_dmesg = 0;# Don't show dmesg in output.
>> >> >  my $squash_by_path = 0;# Summa

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Wed, Nov 29, 2017 at 12:16 PM, Tobin C. Harding  wrote:
> On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding  wrote:
>> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding  wrote:
>> >> > Currently, leaking_addresses.pl only supports scanning 64 bit
>> >> > architectures. This is due to how the regular expressions are formed. We
>> >> > can do better than this. 32 architectures can be supported if we take
>> >> > into consideration the kernel virtual address split.
>> >> >
>> >> > Add support for ix86 32 bit architectures.
>> >> >  - Add command line option for page offset.
>> >> >  - Add command line option for kernel configuration file.
>> >> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>> >> >  - Use page offset when checking for kernel virtual addresses.
>> >> >
>> >> > Signed-off-by: Kaiwan N Billimoria 
>> >> > Signed-off-by: Tobin C. Harding 
>> >> > ---
>> >> >
>> >> > As discussed this is a patch based on Kaiwan's previous patch. This
>> >> > patch represents co development by Kaiwan and Tobin.
>> >> >
>> >> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>> >> >
>> >> > thanks,
>> >> > Tobin.
>> >> >
>> >> >  scripts/leaking_addresses.pl | 168 
>> >> > +--
>> >> >  1 file changed, 148 insertions(+), 20 deletions(-)
>> >> >
>> >> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> >> > index bc578818..f03f2f140e0a 100755
>> >> > --- a/scripts/leaking_addresses.pl
>> >> > +++ b/scripts/leaking_addresses.pl
>> >> > @@ -1,9 +1,11 @@
>> >> >  #!/usr/bin/env perl
>> >> >  #
>> >> >  # (c) 2017 Tobin C. Harding 
>> >> > +# (c) 2017 Kaiwan N Billimoria  (ix86 
>> >> > stuff)
>> >> > +#
>> >> >  # Licensed under the terms of the GNU GPL License version 2
>> >> >  #
>> >> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking 
>> >> > addresses.
>> >> > +# leaking_addresses.pl: Scan the kernel for potential leaking 
>> >> > addresses.
>> >> >  #  - Scans dmesg output.
>> >> >  #  - Walks directory tree and parses each file (for each directory in 
>> >> > @DIRS).
>> >> >  #
>> >> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>> >> >  use Term::ANSIColor qw(:constants);
>> >> >  use Getopt::Long qw(:config no_auto_abbrev);
>> >> >  use Config;
>> >> > +use feature 'state';
>> >> >
>> >> >  my $P = $0;
>> >> >  my $V = '0.01';
>> >> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>> >> >  # Script can only grep for kernel addresses on the following 
>> >> > architectures. If
>> >> >  # your architecture is not listed here and has a grep'able kernel 
>> >> > address please
>> >> >  # consider submitting a patch.
>> >> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
>> >> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>> >> >
>> >> >  # Command line options.
>> >> >  my $help = 0;
>> >> >  my $debug = 0;
>> >> > -my $raw = 0;
>> >> > -my $output_raw = "";   # Write raw results to file.
>> >> > -my $input_raw = "";# Read raw results from file instead of 
>> >> > scanning.
>> >> > -
>> >> > +my $raw = 0;   # Show raw output.
>> >> > +my $output_raw = "";   # Write raw results to file.
>> >> > +my $input_raw = "";# Read raw results from file instead of 
>> >> > scanning.
>> >> >  my $suppress_dmesg = 0;# Don't show dmesg in output.
>> >> >  my $squash_by_path = 0;# Summary report grouped by 
>> >> > absolute path.
>> >> >  my $squash_by_filename = 0;# Summary report grouped by filename.
>> >> > 

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <m...@tobin.cc> wrote:
> On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <m...@tobin.cc> wrote:
>> > Currently, leaking_addresses.pl only supports scanning 64 bit
>> > architectures. This is due to how the regular expressions are formed. We
>> > can do better than this. 32 architectures can be supported if we take
>> > into consideration the kernel virtual address split.
>> >
>> > Add support for ix86 32 bit architectures.
>> >  - Add command line option for page offset.
>> >  - Add command line option for kernel configuration file.
>> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>> >  - Use page offset when checking for kernel virtual addresses.
>> >
>> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimo...@gmail.com>
>> > Signed-off-by: Tobin C. Harding <m...@tobin.cc>
>> > ---
>> >
>> > As discussed this is a patch based on Kaiwan's previous patch. This
>> > patch represents co development by Kaiwan and Tobin.
>> >
>> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>> >
>> > thanks,
>> > Tobin.
>> >
>> >  scripts/leaking_addresses.pl | 168 
>> > +--
>> >  1 file changed, 148 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> > index bc578818..f03f2f140e0a 100755
>> > --- a/scripts/leaking_addresses.pl
>> > +++ b/scripts/leaking_addresses.pl
>> > @@ -1,9 +1,11 @@
>> >  #!/usr/bin/env perl
>> >  #
>> >  # (c) 2017 Tobin C. Harding <m...@tobin.cc>
>> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimo...@gmail.com> (ix86 stuff)
>> > +#
>> >  # Licensed under the terms of the GNU GPL License version 2
>> >  #
>> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking 
>> > addresses.
>> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>> >  #  - Scans dmesg output.
>> >  #  - Walks directory tree and parses each file (for each directory in 
>> > @DIRS).
>> >  #
>> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>> >  use Term::ANSIColor qw(:constants);
>> >  use Getopt::Long qw(:config no_auto_abbrev);
>> >  use Config;
>> > +use feature 'state';
>> >
>> >  my $P = $0;
>> >  my $V = '0.01';
>> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>> >  # Script can only grep for kernel addresses on the following 
>> > architectures. If
>> >  # your architecture is not listed here and has a grep'able kernel address 
>> > please
>> >  # consider submitting a patch.
>> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
>> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>> >
>> >  # Command line options.
>> >  my $help = 0;
>> >  my $debug = 0;
>> > -my $raw = 0;
>> > -my $output_raw = "";   # Write raw results to file.
>> > -my $input_raw = "";# Read raw results from file instead of scanning.
>> > -
>> > +my $raw = 0;   # Show raw output.
>> > +my $output_raw = "";   # Write raw results to file.
>> > +my $input_raw = "";# Read raw results from file instead of 
>> > scanning.
>> >  my $suppress_dmesg = 0;# Don't show dmesg in output.
>> >  my $squash_by_path = 0;# Summary report grouped by 
>> > absolute path.
>> >  my $squash_by_filename = 0;# Summary report grouped by filename.
>> > +my $page_offset_32bit = 0; # 32-bit: value of CONFIG_PAGE_OFFSET
>> > +my $kernel_config_file = "";   # Kernel configuration file.
>> >
>> >  # Do not parse these files (absolute path).
>> >  my @skip_parse_files_abs = ('/proc/kmsg',
>> > @@ -95,14 +99,16 @@ Version: $V
>> >
>> >  Options:
>> >
>> > -   -o, --output-raw=  Save results for future processing.
>> > -   -i, --input-raw=   Read results from file instead of 
>> > scanning.
>> > -   --rawShow raw results (default).
>> > -   --suppress-dmesg Do not show dmesg results.
>> > -   --squash-by-path Show one result per unique 

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding  wrote:
> On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding  wrote:
>> > Currently, leaking_addresses.pl only supports scanning 64 bit
>> > architectures. This is due to how the regular expressions are formed. We
>> > can do better than this. 32 architectures can be supported if we take
>> > into consideration the kernel virtual address split.
>> >
>> > Add support for ix86 32 bit architectures.
>> >  - Add command line option for page offset.
>> >  - Add command line option for kernel configuration file.
>> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>> >  - Use page offset when checking for kernel virtual addresses.
>> >
>> > Signed-off-by: Kaiwan N Billimoria 
>> > Signed-off-by: Tobin C. Harding 
>> > ---
>> >
>> > As discussed this is a patch based on Kaiwan's previous patch. This
>> > patch represents co development by Kaiwan and Tobin.
>> >
>> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>> >
>> > thanks,
>> > Tobin.
>> >
>> >  scripts/leaking_addresses.pl | 168 
>> > +--
>> >  1 file changed, 148 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> > index bc578818..f03f2f140e0a 100755
>> > --- a/scripts/leaking_addresses.pl
>> > +++ b/scripts/leaking_addresses.pl
>> > @@ -1,9 +1,11 @@
>> >  #!/usr/bin/env perl
>> >  #
>> >  # (c) 2017 Tobin C. Harding 
>> > +# (c) 2017 Kaiwan N Billimoria  (ix86 stuff)
>> > +#
>> >  # Licensed under the terms of the GNU GPL License version 2
>> >  #
>> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking 
>> > addresses.
>> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>> >  #  - Scans dmesg output.
>> >  #  - Walks directory tree and parses each file (for each directory in 
>> > @DIRS).
>> >  #
>> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>> >  use Term::ANSIColor qw(:constants);
>> >  use Getopt::Long qw(:config no_auto_abbrev);
>> >  use Config;
>> > +use feature 'state';
>> >
>> >  my $P = $0;
>> >  my $V = '0.01';
>> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>> >  # Script can only grep for kernel addresses on the following 
>> > architectures. If
>> >  # your architecture is not listed here and has a grep'able kernel address 
>> > please
>> >  # consider submitting a patch.
>> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
>> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>> >
>> >  # Command line options.
>> >  my $help = 0;
>> >  my $debug = 0;
>> > -my $raw = 0;
>> > -my $output_raw = "";   # Write raw results to file.
>> > -my $input_raw = "";# Read raw results from file instead of scanning.
>> > -
>> > +my $raw = 0;   # Show raw output.
>> > +my $output_raw = "";   # Write raw results to file.
>> > +my $input_raw = "";# Read raw results from file instead of 
>> > scanning.
>> >  my $suppress_dmesg = 0;# Don't show dmesg in output.
>> >  my $squash_by_path = 0;# Summary report grouped by 
>> > absolute path.
>> >  my $squash_by_filename = 0;# Summary report grouped by filename.
>> > +my $page_offset_32bit = 0; # 32-bit: value of CONFIG_PAGE_OFFSET
>> > +my $kernel_config_file = "";   # Kernel configuration file.
>> >
>> >  # Do not parse these files (absolute path).
>> >  my @skip_parse_files_abs = ('/proc/kmsg',
>> > @@ -95,14 +99,16 @@ Version: $V
>> >
>> >  Options:
>> >
>> > -   -o, --output-raw=  Save results for future processing.
>> > -   -i, --input-raw=   Read results from file instead of 
>> > scanning.
>> > -   --rawShow raw results (default).
>> > -   --suppress-dmesg Do not show dmesg results.
>> > -   --squash-by-path Show one result per unique path.
>> > -   --squash-by-filename Show one result per unique filename.
>> > -   -d, --debug  Display debugging out

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding  wrote:
> Currently, leaking_addresses.pl only supports scanning 64 bit
> architectures. This is due to how the regular expressions are formed. We
> can do better than this. 32 architectures can be supported if we take
> into consideration the kernel virtual address split.
>
> Add support for ix86 32 bit architectures.
>  - Add command line option for page offset.
>  - Add command line option for kernel configuration file.
>  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>  - Use page offset when checking for kernel virtual addresses.
>
> Signed-off-by: Kaiwan N Billimoria 
> Signed-off-by: Tobin C. Harding 
> ---
>
> As discussed this is a patch based on Kaiwan's previous patch. This
> patch represents co development by Kaiwan and Tobin.
>
> Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>
> thanks,
> Tobin.
>
>  scripts/leaking_addresses.pl | 168 
> +--
>  1 file changed, 148 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index bc578818..f03f2f140e0a 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -1,9 +1,11 @@
>  #!/usr/bin/env perl
>  #
>  # (c) 2017 Tobin C. Harding 
> +# (c) 2017 Kaiwan N Billimoria  (ix86 stuff)
> +#
>  # Licensed under the terms of the GNU GPL License version 2
>  #
> -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>  #  - Scans dmesg output.
>  #  - Walks directory tree and parses each file (for each directory in @DIRS).
>  #
> @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>  use Term::ANSIColor qw(:constants);
>  use Getopt::Long qw(:config no_auto_abbrev);
>  use Config;
> +use feature 'state';
>
>  my $P = $0;
>  my $V = '0.01';
> @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>  # Script can only grep for kernel addresses on the following architectures. 
> If
>  # your architecture is not listed here and has a grep'able kernel address 
> please
>  # consider submitting a patch.
> -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>
>  # Command line options.
>  my $help = 0;
>  my $debug = 0;
> -my $raw = 0;
> -my $output_raw = "";   # Write raw results to file.
> -my $input_raw = "";# Read raw results from file instead of scanning.
> -
> +my $raw = 0;   # Show raw output.
> +my $output_raw = "";   # Write raw results to file.
> +my $input_raw = "";# Read raw results from file instead of 
> scanning.
>  my $suppress_dmesg = 0;# Don't show dmesg in output.
>  my $squash_by_path = 0;# Summary report grouped by absolute 
> path.
>  my $squash_by_filename = 0;# Summary report grouped by filename.
> +my $page_offset_32bit = 0; # 32-bit: value of CONFIG_PAGE_OFFSET
> +my $kernel_config_file = "";   # Kernel configuration file.
>
>  # Do not parse these files (absolute path).
>  my @skip_parse_files_abs = ('/proc/kmsg',
> @@ -95,14 +99,16 @@ Version: $V
>
>  Options:
>
> -   -o, --output-raw=  Save results for future processing.
> -   -i, --input-raw=   Read results from file instead of scanning.
> -   --rawShow raw results (default).
> -   --suppress-dmesg Do not show dmesg results.
> -   --squash-by-path Show one result per unique path.
> -   --squash-by-filename Show one result per unique filename.
> -   -d, --debug  Display debugging output.
> -   -h, --help, --versionDisplay this help and exit.
> +   -o, --output-raw= Save results for future processing.
> +   -i, --input-raw=  Read results from file instead of 
> scanning.
> + --raw   Show raw results (default).
> + --suppress-dmesgDo not show dmesg results.
> + --squash-by-pathShow one result per unique path.
> + --squash-by-filenameShow one result per unique filename.
> +   --page-offset-32bit=   PAGE_OFFSET value (for 32-bit 
> kernels).
> +   --kernel-config-file= Kernel configuration file (e.g 
> /boot/config)
> +   -d, --debug Display debugging output.
> +   -h, --help, --version   Display this help and exit.
>
>  Examples:
>
> @@ -115,7 +121,10 @@ Examples:
> # View summary report.
> $0 --input-raw scan.out --squash-by-filename
>
> -Scans the running (64 bit) kernel for potential leaking addresses.
> +   # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
> +   $0 --page-offset-32bit=0x8000
> +
> +Scans the running kernel for potential leaking addresses.
>
>  EOM
>  

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding  wrote:
> Currently, leaking_addresses.pl only supports scanning 64 bit
> architectures. This is due to how the regular expressions are formed. We
> can do better than this. 32 architectures can be supported if we take
> into consideration the kernel virtual address split.
>
> Add support for ix86 32 bit architectures.
>  - Add command line option for page offset.
>  - Add command line option for kernel configuration file.
>  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>  - Use page offset when checking for kernel virtual addresses.
>
> Signed-off-by: Kaiwan N Billimoria 
> Signed-off-by: Tobin C. Harding 
> ---
>
> As discussed this is a patch based on Kaiwan's previous patch. This
> patch represents co development by Kaiwan and Tobin.
>
> Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>
> thanks,
> Tobin.
>
>  scripts/leaking_addresses.pl | 168 
> +--
>  1 file changed, 148 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index bc578818..f03f2f140e0a 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -1,9 +1,11 @@
>  #!/usr/bin/env perl
>  #
>  # (c) 2017 Tobin C. Harding 
> +# (c) 2017 Kaiwan N Billimoria  (ix86 stuff)
> +#
>  # Licensed under the terms of the GNU GPL License version 2
>  #
> -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>  #  - Scans dmesg output.
>  #  - Walks directory tree and parses each file (for each directory in @DIRS).
>  #
> @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>  use Term::ANSIColor qw(:constants);
>  use Getopt::Long qw(:config no_auto_abbrev);
>  use Config;
> +use feature 'state';
>
>  my $P = $0;
>  my $V = '0.01';
> @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>  # Script can only grep for kernel addresses on the following architectures. 
> If
>  # your architecture is not listed here and has a grep'able kernel address 
> please
>  # consider submitting a patch.
> -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>
>  # Command line options.
>  my $help = 0;
>  my $debug = 0;
> -my $raw = 0;
> -my $output_raw = "";   # Write raw results to file.
> -my $input_raw = "";# Read raw results from file instead of scanning.
> -
> +my $raw = 0;   # Show raw output.
> +my $output_raw = "";   # Write raw results to file.
> +my $input_raw = "";# Read raw results from file instead of 
> scanning.
>  my $suppress_dmesg = 0;# Don't show dmesg in output.
>  my $squash_by_path = 0;# Summary report grouped by absolute 
> path.
>  my $squash_by_filename = 0;# Summary report grouped by filename.
> +my $page_offset_32bit = 0; # 32-bit: value of CONFIG_PAGE_OFFSET
> +my $kernel_config_file = "";   # Kernel configuration file.
>
>  # Do not parse these files (absolute path).
>  my @skip_parse_files_abs = ('/proc/kmsg',
> @@ -95,14 +99,16 @@ Version: $V
>
>  Options:
>
> -   -o, --output-raw=  Save results for future processing.
> -   -i, --input-raw=   Read results from file instead of scanning.
> -   --rawShow raw results (default).
> -   --suppress-dmesg Do not show dmesg results.
> -   --squash-by-path Show one result per unique path.
> -   --squash-by-filename Show one result per unique filename.
> -   -d, --debug  Display debugging output.
> -   -h, --help, --versionDisplay this help and exit.
> +   -o, --output-raw= Save results for future processing.
> +   -i, --input-raw=  Read results from file instead of 
> scanning.
> + --raw   Show raw results (default).
> + --suppress-dmesgDo not show dmesg results.
> + --squash-by-pathShow one result per unique path.
> + --squash-by-filenameShow one result per unique filename.
> +   --page-offset-32bit=   PAGE_OFFSET value (for 32-bit 
> kernels).
> +   --kernel-config-file= Kernel configuration file (e.g 
> /boot/config)
> +   -d, --debug Display debugging output.
> +   -h, --help, --version   Display this help and exit.
>
>  Examples:
>
> @@ -115,7 +121,10 @@ Examples:
> # View summary report.
> $0 --input-raw scan.out --squash-by-filename
>
> -Scans the running (64 bit) kernel for potential leaking addresses.
> +   # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
> +   $0 --page-offset-32bit=0x8000
> +
> +Scans the running kernel for potential leaking addresses.
>
>  EOM
> exit($exitcode);
> @@ -131,6 +140,8 @@ GetOptions(
> 'squash-by-path'=> 

Re: [PATCH] rtl8188eu: Correcting macro ROUND macro defination

[Alexander Kapshuk]

On Tue, Mar 14, 2017 at 1:44 PM, Pushkar Jambhlekar
<pushkar@gmail.com> wrote:
> Hi Alexander,
>
> It is not needed for a macro. I am modifying do---while() loop for ROUND 
> macro.
>
> /**
>  * Expand the cipher key into the encryption key schedule.
>  *
>  * @return  the number of rounds for the given cipher key size.
>  */
> #define ROUND(i, d, s) \
> do {\
> d##0 = TE0(s##0) ^ TE1(s##1) ^ TE2(s##2) ^ TE3(s##3) ^ rk[4 * i]; \
> d##1 = TE0(s##1) ^ TE1(s##2) ^ TE2(s##3) ^ TE3(s##0) ^ rk[4 * i + 1]; 
> \
> d##2 = TE0(s##2) ^ TE1(s##3) ^ TE2(s##0) ^ TE3(s##1) ^ rk[4 * i + 2]; 
> \
> d##3 = TE0(s##3) ^ TE1(s##0) ^ TE2(s##1) ^ TE3(s##2) ^ rk[4 * i + 3]; 
> \
> } while (0)
>
> On Tue, Mar 14, 2017 at 5:02 PM, Alexander Kapshuk
> <alexander.kaps...@gmail.com> wrote:
>> On Tue, Mar 14, 2017 at 1:26 PM, Pushkar Jambhlekar
>> <pushkar@gmail.com> wrote:
>>> Description:
>>> There should not be ';' after do ... while(0) in macro defination
>>>
>>> Signed-off-by: Pushkar Jambhlekar <pushkar@gmail.com>
>>> ---
>>>  drivers/staging/rtl8188eu/core/rtw_security.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c 
>>> b/drivers/staging/rtl8188eu/core/rtw_security.c
>>> index b283a490..5b1ef22 100644
>>> --- a/drivers/staging/rtl8188eu/core/rtw_security.c
>>> +++ b/drivers/staging/rtl8188eu/core/rtw_security.c
>>> @@ -1690,4 +1690,4 @@ do {  
>>> \
>>> d##1 = TE0(s##1) ^ TE1(s##2) ^ TE2(s##3) ^ TE3(s##0) ^ rk[4 * i + 
>>> 1]; \
>>> d##2 = TE0(s##2) ^ TE1(s##3) ^ TE2(s##0) ^ TE3(s##1) ^ rk[4 * i + 
>>> 2]; \
>>> d##3 = TE0(s##3) ^ TE1(s##0) ^ TE2(s##1) ^ TE3(s##2) ^ rk[4 * i + 
>>> 3]; \
>>> -} while (0);
>>> +} while (0)
>>> --
>>> 2.7.4
>>>
>>
>> A semicolon is required after 'while(bool);' in 'do..while'.
>> Without it, you get a compile time error.
>
>
>
> --
> Jambhlekar Pushkar Arun
> M.Tech IIT Roorkee

You're right. My apologies.

Re: [PATCH] rtl8188eu: Correcting macro ROUND macro defination

[Alexander Kapshuk]

On Tue, Mar 14, 2017 at 1:44 PM, Pushkar Jambhlekar
 wrote:
> Hi Alexander,
>
> It is not needed for a macro. I am modifying do---while() loop for ROUND 
> macro.
>
> /**
>  * Expand the cipher key into the encryption key schedule.
>  *
>  * @return  the number of rounds for the given cipher key size.
>  */
> #define ROUND(i, d, s) \
> do {\
> d##0 = TE0(s##0) ^ TE1(s##1) ^ TE2(s##2) ^ TE3(s##3) ^ rk[4 * i]; \
> d##1 = TE0(s##1) ^ TE1(s##2) ^ TE2(s##3) ^ TE3(s##0) ^ rk[4 * i + 1]; 
> \
> d##2 = TE0(s##2) ^ TE1(s##3) ^ TE2(s##0) ^ TE3(s##1) ^ rk[4 * i + 2]; 
> \
> d##3 = TE0(s##3) ^ TE1(s##0) ^ TE2(s##1) ^ TE3(s##2) ^ rk[4 * i + 3]; 
> \
> } while (0)
>
> On Tue, Mar 14, 2017 at 5:02 PM, Alexander Kapshuk
>  wrote:
>> On Tue, Mar 14, 2017 at 1:26 PM, Pushkar Jambhlekar
>>  wrote:
>>> Description:
>>> There should not be ';' after do ... while(0) in macro defination
>>>
>>> Signed-off-by: Pushkar Jambhlekar 
>>> ---
>>>  drivers/staging/rtl8188eu/core/rtw_security.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c 
>>> b/drivers/staging/rtl8188eu/core/rtw_security.c
>>> index b283a490..5b1ef22 100644
>>> --- a/drivers/staging/rtl8188eu/core/rtw_security.c
>>> +++ b/drivers/staging/rtl8188eu/core/rtw_security.c
>>> @@ -1690,4 +1690,4 @@ do {  
>>> \
>>> d##1 = TE0(s##1) ^ TE1(s##2) ^ TE2(s##3) ^ TE3(s##0) ^ rk[4 * i + 
>>> 1]; \
>>> d##2 = TE0(s##2) ^ TE1(s##3) ^ TE2(s##0) ^ TE3(s##1) ^ rk[4 * i + 
>>> 2]; \
>>> d##3 = TE0(s##3) ^ TE1(s##0) ^ TE2(s##1) ^ TE3(s##2) ^ rk[4 * i + 
>>> 3]; \
>>> -} while (0);
>>> +} while (0)
>>> --
>>> 2.7.4
>>>
>>
>> A semicolon is required after 'while(bool);' in 'do..while'.
>> Without it, you get a compile time error.
>
>
>
> --
> Jambhlekar Pushkar Arun
> M.Tech IIT Roorkee

You're right. My apologies.

Re: [PATCH] rtl8188eu: Correcting macro ROUND macro defination

[Alexander Kapshuk]

On Tue, Mar 14, 2017 at 1:26 PM, Pushkar Jambhlekar
 wrote:
> Description:
> There should not be ';' after do ... while(0) in macro defination
>
> Signed-off-by: Pushkar Jambhlekar 
> ---
>  drivers/staging/rtl8188eu/core/rtw_security.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c 
> b/drivers/staging/rtl8188eu/core/rtw_security.c
> index b283a490..5b1ef22 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_security.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_security.c
> @@ -1690,4 +1690,4 @@ do {
>   \
> d##1 = TE0(s##1) ^ TE1(s##2) ^ TE2(s##3) ^ TE3(s##0) ^ rk[4 * i + 1]; 
> \
> d##2 = TE0(s##2) ^ TE1(s##3) ^ TE2(s##0) ^ TE3(s##1) ^ rk[4 * i + 2]; 
> \
> d##3 = TE0(s##3) ^ TE1(s##0) ^ TE2(s##1) ^ TE3(s##2) ^ rk[4 * i + 3]; 
> \
> -} while (0);
> +} while (0)
> --
> 2.7.4
>

A semicolon is required after 'while(bool);' in 'do..while'.
Without it, you get a compile time error.

Re: [PATCH] rtl8188eu: Correcting macro ROUND macro defination

[Alexander Kapshuk]

On Tue, Mar 14, 2017 at 1:26 PM, Pushkar Jambhlekar
 wrote:
> Description:
> There should not be ';' after do ... while(0) in macro defination
>
> Signed-off-by: Pushkar Jambhlekar 
> ---
>  drivers/staging/rtl8188eu/core/rtw_security.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c 
> b/drivers/staging/rtl8188eu/core/rtw_security.c
> index b283a490..5b1ef22 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_security.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_security.c
> @@ -1690,4 +1690,4 @@ do {
>   \
> d##1 = TE0(s##1) ^ TE1(s##2) ^ TE2(s##3) ^ TE3(s##0) ^ rk[4 * i + 1]; 
> \
> d##2 = TE0(s##2) ^ TE1(s##3) ^ TE2(s##0) ^ TE3(s##1) ^ rk[4 * i + 2]; 
> \
> d##3 = TE0(s##3) ^ TE1(s##0) ^ TE2(s##1) ^ TE3(s##2) ^ rk[4 * i + 3]; 
> \
> -} while (0);
> +} while (0)
> --
> 2.7.4
>

A semicolon is required after 'while(bool);' in 'do..while'.
Without it, you get a compile time error.

Re: Linux 4.10.2

[Alexander Kapshuk]

On Sun, Mar 12, 2017 at 10:11 PM, Ilia Mirkin  wrote:
> On Sun, Mar 12, 2017 at 3:05 PM, Greg KH  wrote:
>> On Sun, Mar 12, 2017 at 01:51:24PM -0500, Robert Nelson wrote:
>>> On Sun, Mar 12, 2017 at 8:05 AM, Greg KH  wrote:
>>> > I'm announcing the release of the 4.10.2 kernel.
>>> >
>>> > All users of the 4.10 kernel series must upgrade.
>>> >
>>> > The updated 4.10.y git tree can be found at:
>>> > 
>>> > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git 
>>> > linux-4.10.y
>>> > and can be browsed at the normal kernel.org git web browser:
>>> > 
>>> > http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=summary
>>> >
>>> > thanks,
>>> >
>>> > greg k-h
>>>
>>> Hey Greg,
>>>
>>> As of this email, it looks like your script's git push failed:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/?h=linux-4.10.y
>>
>> That's kind of impossible, as the scripts that generate the tarball on
>> kernel.org rely on the tag being there.  And that link shows the v4.10.2
>> tag, what are you thinking is missing here?
>
> The link shows v4.10.1 for me and says that the v4.10.2 tag is
> unknown. Fetching from
> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git,
> I don't see a v4.10.2 tag. The head of linux-4.10.y is v4.10.1.
>
> Cheers,
>
>   -ilia

v4.10.2 tag showing here alright:

git config remote.origin.url
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git

git tag -l *4.10.[12]
v4.10.1
v4.10.2

Re: Linux 4.10.2

[Alexander Kapshuk]

On Sun, Mar 12, 2017 at 10:11 PM, Ilia Mirkin  wrote:
> On Sun, Mar 12, 2017 at 3:05 PM, Greg KH  wrote:
>> On Sun, Mar 12, 2017 at 01:51:24PM -0500, Robert Nelson wrote:
>>> On Sun, Mar 12, 2017 at 8:05 AM, Greg KH  wrote:
>>> > I'm announcing the release of the 4.10.2 kernel.
>>> >
>>> > All users of the 4.10 kernel series must upgrade.
>>> >
>>> > The updated 4.10.y git tree can be found at:
>>> > 
>>> > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git 
>>> > linux-4.10.y
>>> > and can be browsed at the normal kernel.org git web browser:
>>> > 
>>> > http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=summary
>>> >
>>> > thanks,
>>> >
>>> > greg k-h
>>>
>>> Hey Greg,
>>>
>>> As of this email, it looks like your script's git push failed:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/?h=linux-4.10.y
>>
>> That's kind of impossible, as the scripts that generate the tarball on
>> kernel.org rely on the tag being there.  And that link shows the v4.10.2
>> tag, what are you thinking is missing here?
>
> The link shows v4.10.1 for me and says that the v4.10.2 tag is
> unknown. Fetching from
> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git,
> I don't see a v4.10.2 tag. The head of linux-4.10.y is v4.10.1.
>
> Cheers,
>
>   -ilia

v4.10.2 tag showing here alright:

git config remote.origin.url
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git

git tag -l *4.10.[12]
v4.10.1
v4.10.2