Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC
On 02/20/2018 01:17 PM, Daniel Vetter wrote:
On Mon, Feb 19, 2018 at 04:58:43PM +0200, Oleksandr Andrushchenko wrote:
On 02/19/2018 04:30 PM, Daniel Vetter wrote:
On Tue, Feb 13, 2018 at 10:44:16AM +0200, Oleksandr Andrushchenko wrote:
From: Oleksandr Andrushchenko
It is possible that drm_simple_kms_plane_atomic_check called
with no CRTC set, e.g. when user-space application sets CRTC_ID/FB_ID
to 0 before doing any actual drawing. This leads to NULL pointer
dereference because in this case new CRTC state is NULL and must be
checked before accessing.
Signed-off-by: Oleksandr Andrushchenko
---
drivers/gpu/drm/drm_simple_kms_helper.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 9ca8a4a59b74..a05eca9cec8b 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -121,8 +121,10 @@ static int drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
pipe = container_of(plane, struct drm_simple_display_pipe, plane);
crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
&pipe->crtc);
- if (!crtc_state->enable)
- return 0; /* nothing to check when disabling or disabled */
+
+ if (!crtc_state || !crtc_state->enable)
+ /* nothing to check when disabling or disabled or no CRTC set */
+ return 0;
if (crtc_state->enable)
drm_mode_get_hv_timing(&crtc_state->mode,
Hm, this is a bit annoying, since the can_position = false parameter to
drm_atomic_helper_check_plane_state is supposed to catch all this. Would
moving all the checks after the call to that helper, and gating them on
plane_state->visible also work?
Yes, it does work if this is what you mean:
I wasn't sure, thanks for figuring this out!
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index a05eca9cec8b..c48858bb2823 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -122,14 +122,6 @@ static int drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
&pipe->crtc);
- if (!crtc_state || !crtc_state->enable)
- /* nothing to check when disabling or disabled or no CRTC
set */
- return 0;
-
- if (crtc_state->enable)
- drm_mode_get_hv_timing(&crtc_state->mode,
- &clip.x2, &clip.y2);
-
ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
&clip,
DRM_PLANE_HELPER_NO_SCALING,
@@ -138,6 +130,13 @@ static int drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
if (ret)
return ret;
+ if (!plane_state->visible || !crtc_state->enable)
+ return 0; /* nothing to check when disabling or disabled */
if (!plane_state->visible) {
WARN_ON(crtc_state->enabled);
return 0;
}
The helper call above should guarantee this.
Yes, but I still see cases when crtc_state is NULL, thus
making crtc_state->enable to fail
+
+ if (plane_state->visible && crtc_state->enable)
Similar here.
+ drm_mode_get_hv_timing(&crtc_state->mode,
+ &clip.x2, &clip.y2);
+
if (!plane_state->visible)
return -EINVAL;
This can now be removed, the plane helper takes care of checking for
plane_state->visible != crtc_state->enable. Please also remove.
We'd need to add a guarantee to drm_atomic_helper_check_plane_state that
it can cope with crtc_state == NULL, but I think that's a good idea
anyway. Atm it shouldn't end up looking at the crtc_state pointer in that
case.
It doesn't look at it at the moment
Otherwise we'll just go with your fix, but it feels all a bit too fragile,
hence why I want to explore more robust options a bit.
At list with the change above it passes my test which failed
before. Although I cannot confirm it works for others, but it
certainly does for me.
-Daniel
Do you want me to send v1 with the code above?
Yes please, with my above cleanup suggestions.
Please see the patch under test attached (I believe it is what you mean,
with the only change that
if (!plane_state->visible) {
*if (crtc_state)*
WARN_ON(crtc_state->enable);
return 0;
}
check is used).
Whith this patch + additional logs I have:
[ 18.939204] [drm:drm_ioctl [drm]] pid=2105, dev=0xe200, auth=1,
DRM_IOCTL_MODE_ATOMIC
[...]
[ 18.939681] [drm:drm_atomic_set_crtc_for_plane [drm]] Link plane
state c302cbbf to [NOCRTC]
[ 18.939822] [drm:drm_atomic_set_fb_for_plane [drm]] Set [NOFB] for
plane state c302cbbf
[ 18.939963] [drm:drm_atomic_print_state [drm]]
[RFC PATCH tip] x86/acpi: u64_x86_init_noop() can be static
Fixes: 62d8b7fba8d3 ("x86/acpi: Add a new x86_init_acpi structure to
x86_init_ops")
Signed-off-by: Fengguang Wu
---
0 files changed
Re: [PATCH v2] staging:iio:meter: Add name to function definition arguments
On Tue, Feb 20, 2018 at 09:21:56AM -0300, Rodrigo Siqueira wrote: > On 02/20, Dan Carpenter wrote: > > > On Mon, Feb 19, 2018 at 01:28:32PM -0300, rodrigosiqueira wrote: > > ^^^ > > This looks good, but you need to fix your from email header. > > Thanks for the review, and the feedback related to the email header. > I already fixed it. Is it necessary to resend the patch? > I don't know? Sometimes mainainters will cleanup patches themselves... Probably it's simplest to just resend, yes. regards, dan carpenter
[tip:x86/boot 8/9] arch/x86/kernel/x86_init.c:33:5: sparse: symbol 'u64_x86_init_noop' was not declared. Should it be static?
tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/boot head: e9cb8fc2e5ca5996417f900ed5149bd9150f6abd commit: 62d8b7fba8d3ba9c046b7a1a95946c6aaf7c5da9 [8/9] x86/acpi: Add a new x86_init_acpi structure to x86_init_ops reproduce: # apt-get install sparse git checkout 62d8b7fba8d3ba9c046b7a1a95946c6aaf7c5da9 make ARCH=x86_64 allmodconfig make C=1 CF=-D__CHECK_ENDIAN__ sparse warnings: (new ones prefixed by >>) include/linux/init.h:134:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/init.h:135:5: sparse: attribute 'indirect_branch': unknown attribute include/linux/init.h:268:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/init.h:269:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/printk.h:200:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:32:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:34:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:37:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:38:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:40:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:42:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:43:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:45:5: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:46:5: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:49:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/qspinlock.h:53:32: sparse: attribute 'indirect_branch': unknown attribute include/linux/workqueue.h:646:5: sparse: attribute 'indirect_branch': unknown attribute include/linux/workqueue.h:647:5: sparse: attribute 'indirect_branch': unknown attribute include/linux/memory_hotplug.h:221:13: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/numa.h:34:12: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/numa.h:35:13: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/numa.h:62:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/vmalloc.h:64:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/vmalloc.h:173:8: sparse: attribute 'indirect_branch': unknown attribute include/linux/vmalloc.h:174:8: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/fixmap.h:174:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/fixmap.h:176:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/fixmap.h:178:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/fixmap.h:180:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/apic.h:254:13: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/apic.h:430:13: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/io_apic.h:184:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/mmzone.h:1292:15: sparse: attribute 'indirect_branch': unknown attribute include/linux/smp.h:113:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/smp.h:125:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/smp.h:126:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:110:33: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:112:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:114:12: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:118:12: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:126:12: sparse: attribute 'indirect_branch': unknown attribute include/linux/hrtimer.h:497:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/io.h:47:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/kmemleak.h:29:33: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/kasan.h:29:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/kasan.h:30:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/pgtable.h:28:5: sparse: attribute 'indirect_branch': unknown attribute include/linux/slab.h:135:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/slab.h:716:6: sparse: attribute
Applied "regulator: Fix resume from suspend to idle" to the regulator tree
The patch
regulator: Fix resume from suspend to idle
has been applied to the regulator tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From 35b5f14ec6dab281346a2d0ceb34abe2dba94190 Mon Sep 17 00:00:00 2001
From: Geert Uytterhoeven
Date: Tue, 13 Feb 2018 10:37:59 +0100
Subject: [PATCH] regulator: Fix resume from suspend to idle
When resuming from idle with the new suspend mode configuration support
we go through the resume callbacks with a state of PM_SUSPEND_TO_IDLE
which we don't have regulator constraints for, causing an error:
dpm_run_callback(): regulator_resume_early+0x0/0x64 returns -22
PM: Device regulator.0 failed to resume early: error -22
Avoid this and similar errors by treating missing constraints as a noop.
See also commit 57a0dd187956ea04 ("regulator: Fix suspend to idle"),
which fixed the suspend part.
Signed-off-by: Geert Uytterhoeven
Signed-off-by: Mark Brown
---
drivers/regulator/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index dd4708c58480..1fc0c0811da4 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4310,7 +4310,7 @@ static int _regulator_resume_early(struct device *dev,
void *data)
rstate = regulator_get_suspend_state(rdev, *state);
if (rstate == NULL)
- return -EINVAL;
+ return 0;
mutex_lock(&rdev->mutex);
--
2.16.1
Re: [PATCH 9/9] serial: xuartps: Fix out-of-bounds access through DT alias
On 20.2.2018 13:27, Geert Uytterhoeven wrote: > Hi Michal, > > On Tue, Feb 20, 2018 at 12:27 PM, Michal Simek > wrote: >> On 20.2.2018 11:38, Geert Uytterhoeven wrote: >>> On Tue, Feb 20, 2018 at 11:22 AM, Michal Simek >>> wrote: On 20.2.2018 10:40, Geert Uytterhoeven wrote: > The cdns_uart_port[] array is indexed using a value derived from the > "serialN" alias in DT, which may lead to an out-of-bounds access. > > Fix this by adding a range check. > >> I have checked 4 patches I have sent in past which didn't reach mainline >> (probably because of RFC) >> Take a look at >> https://www.spinics.net/lists/linux-serial/msg27106.html >> >> I have removed cdns_uart_port array completely there. > > Nice! I'd love to get rid of fixed arrays in serial... > > However, IMHO it's still worthwhile to fix the out-of-bounds access first, > as that fix can be backported to stable kernels easily. I agree with you. Not a problem with your patch and for me it won't be problem to rebase. I would love to get rid of CDNS_UART_NR_PORTS but unfortunately this is passed to core via .nr. Thanks, Michal
Re: [PATCH] regulator: core: Handle PM_SUSPEND_TO_IDLE suspend_state_t
On Tue, Feb 20, 2018 at 01:12:08PM +0100, Hans de Goede wrote: > I might just be looking at the wrong place and if so I'm sorry, but I > don't see any (additional) fixes related to this here: Seems to have got stuck somewhere, sorry - pushed now. signature.asc Description: PGP signature
Re: [PATCH v2] fs: fsnotify: account fsnotify metadata to kmemcg
On Mon 19-02-18 21:07:28, Amir Goldstein wrote: > On Mon, Feb 19, 2018 at 3:50 PM, Jan Kara wrote: > [...] > > For fanotify without FAN_UNLIMITED_QUEUE the situation is similar as for > > inotify - IMO low practical impact, apps should generally handle queue > > overflow so I don't see a need for any opt in (more accurate memcg charging > > takes precedense over possibly broken apps). > > > > For fanotify with FAN_UNLIMITED_QUEUE the situation is somewhat different - > > firstly there is a practical impact (memory consumption is not limited by > > anything else) and secondly there are higher chances of the application > > breaking (no queue overflow expected) and also that this breakage won't be > > completely harmless (e.g., the application participates in securing the > > system). I've been thinking about this "conflict of interests" for some > > time and currently I think that the best handling of this is that by > > default events for FAN_UNLIMITED_QUEUE groups will get allocated with > > GFP_NOFAIL - such groups can be created only by global CAP_SYS_ADMIN anyway > > so it is reasonably safe against misuse (and since the allocations are > > small it is in fact equivalent to current status quo, just more explicit). > > That way application won't see unexpected queue overflow. The process > > generating event may be looping in the allocator but that is the case > > currently as well. Also the memcg with the consumer of events will have > > higher chances of triggering oom-kill if events consume too much memory but > > I don't see how this is not a good thing by default - and if such reaction > > is not desirable, there's memcg's oom_control to tune the OOM behavior > > which has capabilities far beyond of what we could invent for fanotify... > > > > What do you think Amir? > > > > If I followed all your reasoning correctly, you propose to change behavior to > always account events to group memcg and never fail event allocation, > without any change of API and without opting-in for new behavior? > I think it makes sense. I can't point at any expected breakage, > so overall, this would be a good change. > > I just feel sorry about passing an opportunity to improve functionality. > The fact that fanotify does not have a way for defining the events queue > size is a deficiency IMO, one which I had to work around in the past. > I find that assigning group to memgc and configure memcg to desired > memory limit and getting Q_OVERFLOW on failure to allocate event > is going to be a proper way of addressing this deficiency. So if you don't pass FAN_Q_UNLIMITED, you will get queue with a fixed size and will get Q_OVERFLOW if that is exceeded. So is your concern that you'd like some other fixed limit? Larger one or smaller one and for what reason? > But if you don't think we should bind these 2 things together, > I'll let Shakeel decide if he want to pursue the Q_OVERFLOW change > or not. So if there is still some uncovered use case for finer tuning of event queue length than setting or not setting FAN_Q_UNLIMITED (+ possibly putting the task to memcg to limit memory usage), we can talk about how to address that but at this point I don't see a strong reason to bind this to whether / how events are accounted to memcg... And we still need to make sure we properly do ENOMEM -> Q_OVERFLOW translation and use GFP_NOFAIL for FAN_Q_UNLIMITED groups before merging Shakeel's memcg accounting patches. But Shakeel does not have to be the one implementing that (although if you want to, you are welcome Shakeel :) - otherwise I hope I'll get to it reasonably soon). Honza -- Jan Kara SUSE Labs, CR
Re: [PATCH v7 15/61] xarray: Add xa_load
On Tue, Feb 20, 2018 at 08:34:06AM +0100, Philippe Ombredanne wrote: > > +++ b/tools/testing/radix-tree/xarray-test.c > > @@ -0,0 +1,56 @@ > > +/* > > + * xarray-test.c: Test the XArray API > > + * Copyright (c) 2017 Microsoft Corporation > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms and conditions of the GNU General Public License, > > + * version 2, as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope it will be useful, but WITHOUT > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > > for > > + * more details. > > + */ > > Do you mind using SPDX tags per [1] rather that this fine but long legalese? > Unless you are a legalese lover of course. Argh, missed that one. I'm more concerned with the documentation license, though. I didn't get a response from you to the email I sent Feb 12, Subject: License documentation.
Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC
On Tue, Feb 20, 2018 at 02:36:05PM +0200, Oleksandr Andrushchenko wrote:
> On 02/20/2018 01:17 PM, Daniel Vetter wrote:
> > On Mon, Feb 19, 2018 at 04:58:43PM +0200, Oleksandr Andrushchenko wrote:
> > > On 02/19/2018 04:30 PM, Daniel Vetter wrote:
> > > > On Tue, Feb 13, 2018 at 10:44:16AM +0200, Oleksandr Andrushchenko wrote:
> > > > > From: Oleksandr Andrushchenko
> > > > >
> > > > > It is possible that drm_simple_kms_plane_atomic_check called
> > > > > with no CRTC set, e.g. when user-space application sets CRTC_ID/FB_ID
> > > > > to 0 before doing any actual drawing. This leads to NULL pointer
> > > > > dereference because in this case new CRTC state is NULL and must be
> > > > > checked before accessing.
> > > > >
> > > > > Signed-off-by: Oleksandr Andrushchenko
> > > > >
> > > > > ---
> > > > >drivers/gpu/drm/drm_simple_kms_helper.c | 6 --
> > > > >1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > > b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > > index 9ca8a4a59b74..a05eca9cec8b 100644
> > > > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > > @@ -121,8 +121,10 @@ static int
> > > > > drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> > > > > pipe = container_of(plane, struct drm_simple_display_pipe,
> > > > > plane);
> > > > > crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
> > > > > &pipe->crtc);
> > > > > - if (!crtc_state->enable)
> > > > > - return 0; /* nothing to check when disabling or
> > > > > disabled */
> > > > > +
> > > > > + if (!crtc_state || !crtc_state->enable)
> > > > > + /* nothing to check when disabling or disabled or no
> > > > > CRTC set */
> > > > > + return 0;
> > > > > if (crtc_state->enable)
> > > > > drm_mode_get_hv_timing(&crtc_state->mode,
> > > > Hm, this is a bit annoying, since the can_position = false parameter to
> > > > drm_atomic_helper_check_plane_state is supposed to catch all this. Would
> > > > moving all the checks after the call to that helper, and gating them on
> > > > plane_state->visible also work?
> > > Yes, it does work if this is what you mean:
> > I wasn't sure, thanks for figuring this out!
> >
> > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > index a05eca9cec8b..c48858bb2823 100644
> > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > @@ -122,14 +122,6 @@ static int drm_simple_kms_plane_atomic_check(struct
> > > drm_plane *plane,
> > > crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
> > > &pipe->crtc);
> > >
> > > - if (!crtc_state || !crtc_state->enable)
> > > - /* nothing to check when disabling or disabled or no CRTC
> > > set */
> > > - return 0;
> > > -
> > > - if (crtc_state->enable)
> > > - drm_mode_get_hv_timing(&crtc_state->mode,
> > > - &clip.x2, &clip.y2);
> > > -
> > > ret = drm_atomic_helper_check_plane_state(plane_state,
> > > crtc_state,
> > > &clip,
> > > DRM_PLANE_HELPER_NO_SCALING,
> > > @@ -138,6 +130,13 @@ static int drm_simple_kms_plane_atomic_check(struct
> > > drm_plane *plane,
> > > if (ret)
> > > return ret;
> > >
> > > + if (!plane_state->visible || !crtc_state->enable)
> > > + return 0; /* nothing to check when disabling or disabled
> > > */
> > if (!plane_state->visible) {
> > WARN_ON(crtc_state->enabled);
> > return 0;
> > }
> >
> > The helper call above should guarantee this.
> Yes, but I still see cases when crtc_state is NULL, thus
> making crtc_state->enable to fail
Right, when the plane is completely off there's no CRTC state. Correct
check should be
WARN_ON(crtc_state && crtc_state->enabled);
> > > +
> > > + if (plane_state->visible && crtc_state->enable)
> > Similar here.
> >
> > > + drm_mode_get_hv_timing(&crtc_state->mode,
> > > + &clip.x2, &clip.y2);
> > > +
> > > if (!plane_state->visible)
> > > return -EINVAL;
> > This can now be removed, the plane helper takes care of checking for
> > plane_state->visible != crtc_state->enable. Please also remove.
> >
> > > > We'd need to add a guarantee to drm_atomic_helper_check_plane_state that
> > > > it can cope with crtc_state == NULL, but I think that's a good idea
> > > > anyway. Atm it shouldn't end up looking at the crtc_state pointer in
> > > > that
> > > > case.
> > > It doesn't look at it at the moment
> > > > Otherwise we'll just go with your fix, but it feels all a bit too
> > > > fragile,
> > > > henc
Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC
On 02/20/2018 02:49 PM, Daniel Vetter wrote:
On Tue, Feb 20, 2018 at 02:36:05PM +0200, Oleksandr Andrushchenko wrote:
On 02/20/2018 01:17 PM, Daniel Vetter wrote:
On Mon, Feb 19, 2018 at 04:58:43PM +0200, Oleksandr Andrushchenko wrote:
On 02/19/2018 04:30 PM, Daniel Vetter wrote:
On Tue, Feb 13, 2018 at 10:44:16AM +0200, Oleksandr Andrushchenko wrote:
From: Oleksandr Andrushchenko
It is possible that drm_simple_kms_plane_atomic_check called
with no CRTC set, e.g. when user-space application sets CRTC_ID/FB_ID
to 0 before doing any actual drawing. This leads to NULL pointer
dereference because in this case new CRTC state is NULL and must be
checked before accessing.
Signed-off-by: Oleksandr Andrushchenko
---
drivers/gpu/drm/drm_simple_kms_helper.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 9ca8a4a59b74..a05eca9cec8b 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -121,8 +121,10 @@ static int drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
pipe = container_of(plane, struct drm_simple_display_pipe, plane);
crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
&pipe->crtc);
- if (!crtc_state->enable)
- return 0; /* nothing to check when disabling or disabled */
+
+ if (!crtc_state || !crtc_state->enable)
+ /* nothing to check when disabling or disabled or no CRTC set */
+ return 0;
if (crtc_state->enable)
drm_mode_get_hv_timing(&crtc_state->mode,
Hm, this is a bit annoying, since the can_position = false parameter to
drm_atomic_helper_check_plane_state is supposed to catch all this. Would
moving all the checks after the call to that helper, and gating them on
plane_state->visible also work?
Yes, it does work if this is what you mean:
I wasn't sure, thanks for figuring this out!
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index a05eca9cec8b..c48858bb2823 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -122,14 +122,6 @@ static int drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
&pipe->crtc);
- if (!crtc_state || !crtc_state->enable)
- /* nothing to check when disabling or disabled or no CRTC
set */
- return 0;
-
- if (crtc_state->enable)
- drm_mode_get_hv_timing(&crtc_state->mode,
- &clip.x2, &clip.y2);
-
ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
&clip,
DRM_PLANE_HELPER_NO_SCALING,
@@ -138,6 +130,13 @@ static int drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
if (ret)
return ret;
+ if (!plane_state->visible || !crtc_state->enable)
+ return 0; /* nothing to check when disabling or disabled */
if (!plane_state->visible) {
WARN_ON(crtc_state->enabled);
return 0;
}
The helper call above should guarantee this.
Yes, but I still see cases when crtc_state is NULL, thus
making crtc_state->enable to fail
Right, when the plane is completely off there's no CRTC state. Correct
check should be
WARN_ON(crtc_state && crtc_state->enabled);
ok, will update with this additional check
+
+ if (plane_state->visible && crtc_state->enable)
Similar here.
+ drm_mode_get_hv_timing(&crtc_state->mode,
+ &clip.x2, &clip.y2);
+
if (!plane_state->visible)
return -EINVAL;
This can now be removed, the plane helper takes care of checking for
plane_state->visible != crtc_state->enable. Please also remove.
We'd need to add a guarantee to drm_atomic_helper_check_plane_state that
it can cope with crtc_state == NULL, but I think that's a good idea
anyway. Atm it shouldn't end up looking at the crtc_state pointer in that
case.
It doesn't look at it at the moment
Otherwise we'll just go with your fix, but it feels all a bit too fragile,
hence why I want to explore more robust options a bit.
At list with the change above it passes my test which failed
before. Although I cannot confirm it works for others, but it
certainly does for me.
-Daniel
Do you want me to send v1 with the code above?
Yes please, with my above cleanup suggestions.
Please see the patch under test attached (I believe it is what you mean,
with the only change that
if (!plane_state->visible) {
*if (crtc_state)*
WARN_ON(crtc_state->enable);
return 0;
}
check is used).
Whith this patch + additional logs I have:
[ 18.939204] [drm:drm_ioctl [
Re: [PATCH v2] pinctrl: pinctrl-single: Fix pcs_request_gpio() when bits_per_mux != 0
On Mon, Feb 19, 2018 at 11:57 PM, David Lechner wrote:
> This fixes pcs_request_gpio() in the pinctrl-single driver when
> bits_per_mux != 0. It appears this was overlooked when the multiple
> pins per register feature was added.
>
> Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure multiple
> pins of different modules")
> Signed-off-by: David Lechner
> ---
>
> v2 changes:
> - don't wrap Fixes: line in commit message since it is a special machine-
> readable line.
>
> There was some discussion in v1 about using DIV_ROUND_UP(), etc. macros, but
> the consensus was to leave it as-is since it matches existing code and that
> macros can be introduced in another patch.
>
> drivers/pinctrl/pinctrl-single.c | 22 +++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-single.c
> b/drivers/pinctrl/pinctrl-single.c
> index cec7537..a7c5eb3 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -391,9 +391,25 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev,
> || pin < frange->offset)
> continue;
> mux_bytes = pcs->width / BITS_PER_BYTE;
> - data = pcs->read(pcs->base + pin * mux_bytes) & ~pcs->fmask;
> - data |= frange->gpiofunc;
> - pcs->write(data, pcs->base + pin * mux_bytes);
> +
> + if (pcs->bits_per_mux) {
> + int byte_num, offset, pin_shift;
> +
> + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE;
> + offset = (byte_num / mux_bytes) * mux_bytes;
> + pin_shift = pin % (pcs->width / pcs->bits_per_pin) *
> + pcs->bits_per_pin;
> +
> + data = pcs->read(pcs->base + offset);
> + data &= ~(pcs->fmask << pin_shift);
> + data |= frange->gpiofunc << pin_shift;
> + pcs->write(data, pcs->base + offset);
> + } else {
> + data = pcs->read(pcs->base + pin * mux_bytes);
> + data &= ~pcs->fmask;
> + data |= frange->gpiofunc;
> + pcs->write(data, pcs->base + pin * mux_bytes);
Just an idea, you may leave this almost untouched and do calculate
pin_shift and offset in condition, like
if (...) {
pin_shift = ...
offset = ...
} else {
pin_shift = 0;
offset = pin * mux_bytes;
}
data = pcs->read(pcs->base + offset);
data &= ~(pcs->fmask << pin_shift);
data |= frange->gpiofunc << pin_shift;
pcs->write(data, pcs->base + offset);
It's also possible to split to two changes, where first introduces
that variables and their default values (see 'else' branch) and second
one introduces an if branch override.
> + }
> break;
--
With Best Regards,
Andy Shevchenko
[PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.
This solves the problem that when we swapout a BO from a domain we
sometimes couldn't make room for it because holding the lock blocks all
other BOs with this reservation object.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 33 -
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d90b1cf10b27..3a44c2ee4155 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object
*bo,
EXPORT_SYMBOL(ttm_bo_eviction_valuable);
/**
- * Check the target bo is allowable to be evicted or swapout, including cases:
- *
- * a. if share same reservation object with ctx->resv, have assumption
- * reservation objects should already be locked, so not lock again and
- * return true directly when either the opreation allow_reserved_eviction
- * or the target bo already is in delayed free list;
- *
- * b. Otherwise, trylock it.
+ * Check if the target bo is allowed to be evicted or swapedout.
*/
static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
- struct ttm_operation_ctx *ctx, bool *locked)
+ struct ttm_operation_ctx *ctx,
+ bool *locked)
{
- bool ret = false;
+ /* First check if we can lock it */
+ *locked = reservation_object_trylock(bo->resv);
+ if (*locked)
+ return true;
- *locked = false;
+ /* Check if it's locked because it is part of the current operation */
if (bo->resv == ctx->resv) {
reservation_object_assert_held(bo->resv);
- if (ctx->allow_reserved_eviction || !list_empty(&bo->ddestroy))
- ret = true;
- } else {
- *locked = reservation_object_trylock(bo->resv);
- ret = *locked;
+ return ctx->allow_reserved_eviction ||
+ !list_empty(&bo->ddestroy);
}
- return ret;
+ /* Check if it's locked because it was already evicted */
+ if (ww_mutex_is_owned_by(&bo->resv->lock, NULL))
+ return true;
+
+ /* Some other thread is using it, don't touch it */
+ return false;
}
static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
--
2.14.1
[PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3
amdgpu needs to verify if userspace sends us valid addresses and the simplest
way of doing this is to check if the buffer object is locked with the ticket
of the current submission.
Clean up the access to the ww_mutex internals by providing a function
for this and extend the check to the thread owning the underlying mutex.
v2: split amdgpu changes into separate patch as suggested by Alex
v3: change logic as suggested by Daniel
Signed-off-by: Christian König
---
include/linux/ww_mutex.h | 17 +
1 file changed, 17 insertions(+)
diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 39fda195bf78..14e4149d3d9d 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex
*lock)
return mutex_is_locked(&lock->base);
}
+/**
+ * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
+ * @lock: the mutex to be queried
+ * @ctx: the w/w acquire context to test
+ *
+ * If @ctx is not NULL test if the mutex is owned by this context.
+ * If @ctx is NULL test if the mutex is owned by the current thread.
+ */
+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+ struct ww_acquire_ctx *ctx)
+{
+ if (ctx)
+ return likely(READ_ONCE(lock->ctx) == ctx);
+ else
+ return likely(__mutex_owner(&lock->base) == current);
+}
+
#endif
--
2.14.1
[PATCH 4/4] drm/ttm: keep BOs reserved until end of eviction
This avoids problems when BOs are evicted but directly moved back into
the domain from other threads.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 37 +
1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 3a44c2ee4155..593a0216faff 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -742,7 +742,8 @@ static bool ttm_bo_evict_swapout_allowable(struct
ttm_buffer_object *bo,
static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
uint32_t mem_type,
const struct ttm_place *place,
- struct ttm_operation_ctx *ctx)
+ struct ttm_operation_ctx *ctx,
+ struct list_head *evicted)
{
struct ttm_bo_global *glob = bdev->glob;
struct ttm_mem_type_manager *man = &bdev->man[mem_type];
@@ -792,17 +793,28 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
ret = ttm_bo_evict(bo, ctx);
if (locked) {
- ttm_bo_unreserve(bo);
+ list_add_tail(&bo->lru, evicted);
} else {
spin_lock(&glob->lru_lock);
ttm_bo_add_to_lru(bo);
spin_unlock(&glob->lru_lock);
+ kref_put(&bo->list_kref, ttm_bo_release_list);
}
- kref_put(&bo->list_kref, ttm_bo_release_list);
return ret;
}
+static void ttm_mem_evict_cleanup(struct list_head *evicted)
+{
+ struct ttm_buffer_object *bo, *tmp;
+
+ list_for_each_entry_safe(bo, tmp, evicted, lru) {
+ list_del_init(&bo->lru);
+ ttm_bo_unreserve(bo);
+ kref_put(&bo->list_kref, ttm_bo_release_list);
+ }
+}
+
void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem)
{
struct ttm_mem_type_manager *man = &bo->bdev->man[mem->mem_type];
@@ -852,20 +864,26 @@ static int ttm_bo_mem_force_space(struct
ttm_buffer_object *bo,
{
struct ttm_bo_device *bdev = bo->bdev;
struct ttm_mem_type_manager *man = &bdev->man[mem_type];
+ struct list_head evicted;
int ret;
+ INIT_LIST_HEAD(&evicted);
do {
ret = (*man->func->get_node)(man, bo, place, mem);
if (unlikely(ret != 0))
return ret;
if (mem->mm_node)
break;
- ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
+ ret = ttm_mem_evict_first(bdev, mem_type, place, ctx, &evicted);
if (unlikely(ret != 0))
- return ret;
+ goto error;
} while (1);
mem->mem_type = mem_type;
- return ttm_bo_add_move_fence(bo, man, mem);
+ ret = ttm_bo_add_move_fence(bo, man, mem);
+
+error:
+ ttm_mem_evict_cleanup(&evicted);
+ return ret;
}
static uint32_t ttm_bo_select_caching(struct ttm_mem_type_manager *man,
@@ -1345,6 +1363,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device
*bdev,
struct ttm_operation_ctx ctx = { false, false };
struct ttm_mem_type_manager *man = &bdev->man[mem_type];
struct ttm_bo_global *glob = bdev->glob;
+ struct list_head evicted;
struct dma_fence *fence;
int ret;
unsigned i;
@@ -1352,18 +1371,20 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device
*bdev,
/*
* Can't use standard list traversal since we're unlocking.
*/
-
+ INIT_LIST_HEAD(&evicted);
spin_lock(&glob->lru_lock);
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
while (!list_empty(&man->lru[i])) {
spin_unlock(&glob->lru_lock);
- ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx);
+ ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx,
+ &evicted);
if (ret)
return ret;
spin_lock(&glob->lru_lock);
}
}
spin_unlock(&glob->lru_lock);
+ ttm_mem_evict_cleanup(&evicted);
spin_lock(&man->move_lock);
fence = dma_fence_get(man->move);
--
2.14.1
[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply
On some boards, especially when vbus supply requires large current,
and the charge pump on the PHY isn't enough, an external vbus power switch
may be used.
Add support for this optional external vbus supply in ehci-platform.
Signed-off-by: Amelie Delaunay
---
Changes in v2:
* Address Roger Quadros comments: move regulator_enable/disable from
ehci_platform_power_on/off to ehci_platform_port_power.
---
Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
drivers/usb/host/ehci-platform.c | 31 ++
2 files changed, 32 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt
b/Documentation/devicetree/bindings/usb/usb-ehci.txt
index 3efde12..fc480cd 100644
--- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
@@ -19,6 +19,7 @@ Optional properties:
- phys : phandle + phy specifier pair
- phy-names : "usb"
- resets : phandle + reset specifier pair
+ - vbus-supply : phandle of regulator supplying vbus
Example (Sequoia 440EPx):
ehci@e300 {
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index b065a96..05be100 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -46,6 +47,7 @@ struct ehci_platform_priv {
struct reset_control *rsts;
struct phy **phys;
int num_phys;
+ struct regulator *vbus_supply;
bool reset_on_resume;
};
@@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
return 0;
}
+static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
+ bool enable)
+{
+ struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
+ int ret = 0;
+
+ if (priv->vbus_supply) {
+ if (enable)
+ ret = regulator_enable(priv->vbus_supply);
+ else
+ ret = regulator_disable(priv->vbus_supply);
+ if (ret)
+ dev_err(hcd->self.controller,
+ "failed to %s vbus supply: %d\n",
+ enable ? "enable" : "disable", ret);
+ }
+ return ret;
+}
+
static int ehci_platform_power_on(struct platform_device *dev)
{
struct usb_hcd *hcd = platform_get_drvdata(dev);
@@ -134,6 +155,7 @@ static struct hc_driver __read_mostly
ehci_platform_hc_driver;
static const struct ehci_driver_overrides platform_overrides __initconst = {
.reset =ehci_platform_reset,
.extra_priv_size = sizeof(struct ehci_platform_priv),
+ .port_power = ehci_platform_port_power,
};
static struct usb_ehci_pdata ehci_platform_defaults = {
@@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
if (err)
goto err_put_clks;
+ priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
+ if (IS_ERR(priv->vbus_supply)) {
+ err = PTR_ERR(priv->vbus_supply);
+ if (err == -ENODEV)
+ priv->vbus_supply = NULL;
+ else
+ goto err_reset;
+ }
+
if (pdata->big_endian_desc)
ehci->big_endian_desc = 1;
if (pdata->big_endian_mmio)
--
2.7.4
[PATCH 2/4] drm/amdgpu: use new ww_mutex_is_owned_by function
Instead of accessing ww_mutex internals directly use the provided
function to check if the ww_mutex was indeed locked by the current
command submission.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index eaa3cb0c3ad1..6db26a3144dc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1594,7 +1594,7 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser
*parser,
*map = mapping;
/* Double check that the BO is reserved by this CS */
- if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
+ if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, &parser->ticket))
return -EINVAL;
if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
--
2.14.1
[PATCH 0/3] drm/rockchip: VOP interrupt fixes
This small series fixes a number of issues that I found while trying to get kexec working on the Chromebook Plus (aka rk3399-gru-kevin) in order to use it as some sort of interactive bootloader. The main issue is that the vop driver expects the interrupts to be cleared and disabled when booting. Nothing could be more wrong. The device should be expected to be alive and screaming, and it is the driver's job to put it back into a sane state. This is what the first patch does, making sure the interrupt is requested only when the device has been put back into a known state. Given that this is an observable bug that has been around for a while, I've tagged it with a Cc: stable. The two following patches are less important: Using memcpy on MMIO ranges is plain wrong, and using spin_lock_irqsave in irq context is slightly pointless. With these patches in, I'm able to get kexec to work. There is still some funny issues at the iommu level, but that's for another day. Patches on top of 4.16-rc2. Marc Zyngier (3): drm/rockchip: Clear all interrupts before requesting the IRQ drm/rockchip: Do not use memcpy for MMIO addresses drm/rockchip: Don't use spin_lock_irqsave in interrupt context drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 44 +++-- 1 file changed, 23 insertions(+), 21 deletions(-) -- 2.14.2
[PATCH 2/3] drm/rockchip: Do not use memcpy for MMIO addresses
memcpy is only meant to be used for memory, and only that. MMIO accessors should be used to access MMIO regions, preferably the ones that correspond to the size of the register accessed. Let's convert the bulk register copy to writel/readl_relaxed, which is the correct API. Signed-off-by: Marc Zyngier --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 7b224e08cbf1..f2bde1c76bbe 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -528,7 +528,10 @@ static int vop_enable(struct drm_crtc *crtc) goto err_disable_aclk; } - memcpy(vop->regs, vop->regsbak, vop->len); + spin_lock(&vop->reg_lock); + for (i = 0; i < vop->len; i += 4) + writel_relaxed(vop->regsbak[i / 4], vop->regs + i); + /* * We need to make sure that all windows are disabled before we * enable the crtc. Otherwise we might try to scan from a destroyed @@ -538,10 +541,9 @@ static int vop_enable(struct drm_crtc *crtc) struct vop_win *vop_win = &vop->win[i]; const struct vop_win_data *win = vop_win->data; - spin_lock(&vop->reg_lock); VOP_WIN_SET(vop, win, enable, 0); - spin_unlock(&vop->reg_lock); } + spin_unlock(&vop->reg_lock); vop_cfg_done(vop); @@ -1417,7 +1419,8 @@ static int vop_initial(struct vop *vop) VOP_INTR_SET_TYPE(vop, clear, INTR_MASK, 1); VOP_INTR_SET_TYPE(vop, enable, INTR_MASK, 0); - memcpy(vop->regsbak, vop->regs, vop->len); + for (i = 0; i < vop->len; i += sizeof(u32)) + vop->regsbak[i / 4] = readl_relaxed(vop->regs + i); VOP_REG_SET(vop, misc, global_regdone_en, 1); VOP_REG_SET(vop, common, dsp_blank, 0); -- 2.14.2
[PATCH 3/3] drm/rockchip: Don't use spin_lock_irqsave in interrupt context
The rockchip DRM driver is quite careful to disable interrupts
when taking a lock that is also taken in interrupt context,
which is a good thing.
What is a bit over the top is to use spin_lock_irqsave when
already in interrupt context, as you cannot take another
interrupt again, and disabling interrupt is just pure
overhead.
Switching to the non _irqsave version in interrupt context is
more logical, and less heavy handed.
Signed-off-by: Marc Zyngier
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index f2bde1c76bbe..48d27f6fb16d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1160,15 +1160,14 @@ static void vop_handle_vblank(struct vop *vop)
{
struct drm_device *drm = vop->drm_dev;
struct drm_crtc *crtc = &vop->crtc;
- unsigned long flags;
- spin_lock_irqsave(&drm->event_lock, flags);
+ spin_lock(&drm->event_lock);
if (vop->event) {
drm_crtc_send_vblank_event(crtc, vop->event);
drm_crtc_vblank_put(crtc);
vop->event = NULL;
}
- spin_unlock_irqrestore(&drm->event_lock, flags);
+ spin_unlock(&drm->event_lock);
if (test_and_clear_bit(VOP_PENDING_FB_UNREF, &vop->pending))
drm_flip_work_commit(&vop->fb_unref_work, system_unbound_wq);
@@ -1179,21 +1178,20 @@ static irqreturn_t vop_isr(int irq, void *data)
struct vop *vop = data;
struct drm_crtc *crtc = &vop->crtc;
uint32_t active_irqs;
- unsigned long flags;
int ret = IRQ_NONE;
/*
* interrupt register has interrupt status, enable and clear bits, we
* must hold irq_lock to avoid a race with enable/disable_vblank().
*/
- spin_lock_irqsave(&vop->irq_lock, flags);
+ spin_lock(&vop->irq_lock);
active_irqs = VOP_INTR_GET_TYPE(vop, status, INTR_MASK);
/* Clear all active interrupt sources */
if (active_irqs)
VOP_INTR_SET_TYPE(vop, clear, active_irqs, 1);
- spin_unlock_irqrestore(&vop->irq_lock, flags);
+ spin_unlock(&vop->irq_lock);
/* This is expected for vop iommu irqs, since the irq is shared */
if (!active_irqs)
--
2.14.2
[PATCH 1/3] drm/rockchip: Clear all interrupts before requesting the IRQ
Calling request_irq() followed by disable_irq() is usually a bad idea, specially if the interrupt can be pending, and you're not yet in a position to handle it. This is exactly what happens on my kevin system when rebooting in a second kernel using kexec: Some interrupt is left pending from the previous kernel, and we take it too early, before disable_irq() could do anything. Let's clear the pending interrupts as we initialize the HW, and move the interrupt request after that point. This ensures that we're in a sane state when the interrupt is requested. Cc: sta...@vger.kernel.org Signed-off-by: Marc Zyngier --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 23 --- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index ba7505292b78..7b224e08cbf1 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1414,6 +1414,9 @@ static int vop_initial(struct vop *vop) usleep_range(10, 20); reset_control_deassert(ahb_rst); + VOP_INTR_SET_TYPE(vop, clear, INTR_MASK, 1); + VOP_INTR_SET_TYPE(vop, enable, INTR_MASK, 0); + memcpy(vop->regsbak, vop->regs, vop->len); VOP_REG_SET(vop, misc, global_regdone_en, 1); @@ -1569,17 +1572,9 @@ static int vop_bind(struct device *dev, struct device *master, void *data) mutex_init(&vop->vsync_mutex); - ret = devm_request_irq(dev, vop->irq, vop_isr, - IRQF_SHARED, dev_name(dev), vop); - if (ret) - return ret; - - /* IRQ is initially disabled; it gets enabled in power_on */ - disable_irq(vop->irq); - ret = vop_create_crtc(vop); if (ret) - goto err_enable_irq; + return ret; pm_runtime_enable(&pdev->dev); @@ -1590,13 +1585,19 @@ static int vop_bind(struct device *dev, struct device *master, void *data) goto err_disable_pm_runtime; } + ret = devm_request_irq(dev, vop->irq, vop_isr, + IRQF_SHARED, dev_name(dev), vop); + if (ret) + goto err_disable_pm_runtime; + + /* IRQ is initially disabled; it gets enabled in power_on */ + disable_irq(vop->irq); + return 0; err_disable_pm_runtime: pm_runtime_disable(&pdev->dev); vop_destroy_crtc(vop); -err_enable_irq: - enable_irq(vop->irq); /* To balance out the disable_irq above */ return ret; } -- 2.14.2
Re: [PATCH 4/7] sched/fair: Avoid unnecessary balancing of asymmetric capacity groups
On Thu, Feb 15, 2018 at 04:20:51PM +, Morten Rasmussen wrote:
> +/*
> + * group_similar_cpu_capacity: Returns true if the minimum capacity of the
> + * compared groups differ by less than 12.5%.
> + */
> +static inline bool
> +group_similar_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
> +{
> + long diff = sg->sgc->min_capacity - ref->sgc->min_capacity;
> + long max = max(sg->sgc->min_capacity, ref->sgc->min_capacity);
> +
> + return abs(diff) < max >> 3;
> +}
This seems a fairly random and dodgy heuristic.
Re: [PATCH] tools/memory-model: remove rb-dep, smp_read_barrier_depends, and lockless_dereference
On Mon, Feb 19, 2018 at 12:14:45PM -0500, Alan Stern wrote: > Note that operations like atomic_add_unless() already include memory > barriers. It is valid for atomic_add_unless() to not imply any barriers when the addition doesn't happen.
Re: [PATCH] tools/memory-model: remove rb-dep, smp_read_barrier_depends, and lockless_dereference
On Mon, Feb 19, 2018 at 11:41:23AM -0800, Paul E. McKenney wrote: > On Mon, Feb 19, 2018 at 12:14:45PM -0500, Alan Stern wrote: > > This leaves us with a question: Do we want to change the kernel by > > adding memory barriers after unsuccessful RMW operations on Alpha, or > > do we want to change the model by excluding such operations from > > address dependencies? > > I vote for adding the barrier on Alpha. However, I don't know of any > code in the Linux kernel that relies on read-to-read address dependency > ordering headed by a failing RMW operation, so I don't feel all that > strongly about this. Right, but not knowing doesn't mean doesn't exist, and most certainly doesn't mean will never exist. > > Note that operations like atomic_add_unless() already include memory > > barriers. > > And I don't see an atomic_add_unless_relaxed(), so we are good on this > one. So far, anyway! ;-) Not the point, add_unless() is a conditional operation, and therefore doesn't need to imply anything when failing.
Re: [PATCH] tools/memory-model: remove rb-dep, smp_read_barrier_depends, and lockless_dereference
On Mon, Feb 19, 2018 at 12:14:45PM -0500, Alan Stern wrote: > This leaves us with a question: Do we want to change the kernel by > adding memory barriers after unsuccessful RMW operations on Alpha, or > do we want to change the model by excluding such operations from > address dependencies? I think we want to put all the 'pain and weirdness' in Alpha.
Re: [RFC PATCH 3/5] asm-generic/bitops/atomic.h: Rewrite using atomic_fetch_*
On Mon, Feb 19, 2018 at 02:01:51PM +, Will Deacon wrote: > If it's defined, then we could consider using cmpxchg64 to build atomic64 > instead of the locks. But even then, I'm not sure we're really helping > anybody out in practice. yeah, most 64bit archs have more atomics or ll/sc and would not use it anyway I suppose.
Re: [PATCH] tools/memory-model: remove rb-dep, smp_read_barrier_depends, and lockless_dereference
On Tue, Feb 20, 2018 at 10:33:47AM +0100, Andrea Parri wrote: > I'd like to continue to treat R[once] and R*[once] equally if possible. > Given the (unconditional) smp_read_barrier_depends in READ_ONCE and in > atomics, it seems reasonable to have it unconditionally in cmpxchg. > > As with the following patch? > > Andrea > > --- > diff --git a/arch/alpha/include/asm/xchg.h b/arch/alpha/include/asm/xchg.h > index 68dfb3cb71454..e2660866ce972 100644 > --- a/arch/alpha/include/asm/xchg.h > +++ b/arch/alpha/include/asm/xchg.h > @@ -128,10 +128,9 @@ xchg(, volatile void *ptr, unsigned long x, int size) > * store NEW in MEM. Return the initial value in MEM. Success is > * indicated by comparing RETURN with OLD. > * > - * The memory barrier should be placed in SMP only when we actually > - * make the change. If we don't change anything (so if the returned > - * prev is equal to old) then we aren't acquiring anything new and > - * we don't need any memory barrier as far I can tell. > + * The memory barrier is placed in SMP unconditionally, in order to > + * guarantee that dependency ordering is preserved when a dependency > + * is headed by an unsuccessful operation. > */ > > static inline unsigned long > @@ -150,8 +149,8 @@ cmpxchg(_u8, volatile char *m, unsigned char old, > unsigned char new) > " or %1,%2,%2\n" > " stq_c %2,0(%4)\n" > " beq %2,3f\n" > - __ASM__MB > "2:\n" > + __ASM__MB > ".subsection 2\n" > "3: br 1b\n" > ".previous" > @@ -177,8 +176,8 @@ cmpxchg(_u16, volatile short *m, unsigned short old, > unsigned short new) > " or %1,%2,%2\n" > " stq_c %2,0(%4)\n" > " beq %2,3f\n" > - __ASM__MB > "2:\n" > + __ASM__MB > ".subsection 2\n" > "3: br 1b\n" > ".previous" > @@ -200,8 +199,8 @@ cmpxchg(_u32, volatile int *m, int old, int new) > " mov %4,%1\n" > " stl_c %1,%2\n" > " beq %1,3f\n" > - __ASM__MB > "2:\n" > + __ASM__MB > ".subsection 2\n" > "3: br 1b\n" > ".previous" > @@ -223,8 +222,8 @@ cmpxchg(_u64, volatile long *m, unsigned long old, > unsigned long new) > " mov %4,%1\n" > " stq_c %1,%2\n" > " beq %1,3f\n" > - __ASM__MB > "2:\n" > + __ASM__MB > ".subsection 2\n" > "3: br 1b\n" > ".previous" ACK
Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply
Hi,
Amelie Delaunay writes:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
>
> Signed-off-by: Amelie Delaunay
>
> ---
> Changes in v2:
> * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
> Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
> drivers/usb/host/ehci-platform.c | 31
> ++
> 2 files changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
> - phys : phandle + phy specifier pair
> - phy-names : "usb"
> - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>
> Example (Sequoia 440EPx):
> ehci@e300 {
> diff --git a/drivers/usb/host/ehci-platform.c
> b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
> struct reset_control *rsts;
> struct phy **phys;
> int num_phys;
> + struct regulator *vbus_supply;
> bool reset_on_resume;
> };
>
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
> return 0;
> }
>
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> + bool enable)
> +{
> + struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> + int ret = 0;
> +
> + if (priv->vbus_supply) {
you can reduce indentation here:
if (!priv->vbus_supply)
return 0;
--
balbi
Re: [PATCH 4/4] fs/dcache: Avoid the try_lock loops in dentry_kill()
On Tue, Feb 20, 2018 at 12:34:57AM +0100, John Ogness wrote:
> Implementation 2: Using switch on a dentry_lock_inode() that returns a
> tristate value. Does not support branch prediction. This approach is
> probably easiest to understand.
>
> /*
>* Lock the inode. Might drop dentry->d_lock temporarily
>* which allows inode to change. Start over if that happens.
>*/
> switch (dentry_lock_inode(dentry)) {
> case LOCK_FAST:
Bah, I just checked, you cannot use GCC label attributes on statements
:/ Otherwise you could've done:
case LOCK_FAST: __attribute__((hot));
> break;
> case LOCK_SLOW:
> /*
> * Recheck refcount as it might have been
> * incremented while d_lock was dropped.
> */
> if (unlikely(dentry->d_lockref.count != 1))
> goto drop_ref;
> break;
> case LOCK_FAILED:
> goto again;
> }
>
Re: [PATCH v5 3/3] sched: update blocked load when newly idle
On Wed, Feb 14, 2018 at 04:26:46PM +0100, Vincent Guittot wrote:
> When NEWLY_IDLE load balance is not triggered, we might need to update the
> blocked load anyway. We can kick an ilb so an idle CPU will take care of
> updating blocked load or we can try to update them locally before entering
> idle. In the latter case, we reuse part of the nohz_idle_balance.
So I still don't like this, but then I couldn't come up with anything I
did like either. Munged the patch a bit, I pulled out the code movement
into a separate patch and otherwise reduced #ifdeffery a bit.
---
Subject: sched: update blocked load when newly idle
From: Vincent Guittot
Date: Wed, 14 Feb 2018 16:26:46 +0100
When NEWLY_IDLE load balance is not triggered, we might need to update the
blocked load anyway. We can kick an ilb so an idle CPU will take care of
updating blocked load or we can try to update them locally before entering
idle. In the latter case, we reuse part of the nohz_idle_balance.
Cc: valentin.schnei...@arm.com
Cc: mi...@kernel.org
Cc: brendan.jack...@arm.com
Cc: dietmar.eggem...@arm.com
Cc: morten.rasmus...@foss.arm.com
Signed-off-by: Vincent Guittot
Signed-off-by: Peter Zijlstra (Intel)
Link:
http://lkml.kernel.org/r/1518622006-16089-4-git-send-email-vincent.guit...@linaro.org
---
kernel/sched/fair.c | 105 +++-
1 file changed, 87 insertions(+), 18 deletions(-)
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9394,10 +9394,14 @@ void nohz_balance_enter_idle(int cpu)
}
/*
- * In CONFIG_NO_HZ_COMMON case, the idle balance kickee will do the
- * rebalancing for all the cpus for whom scheduler ticks are stopped.
+ * Internal function that runs load balance for all idle cpus. The load balance
+ * can be a simple update of blocked load or a complete load balance with
+ * tasks movement depending of flags.
+ * The function returns false if the loop has stopped before running
+ * through all idle CPUs.
*/
-static bool nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)
+static bool _nohz_idle_balance(struct rq *this_rq, unsigned int flags,
+ enum cpu_idle_type idle)
{
/* Earliest time when we have to do rebalance again */
unsigned long now = jiffies;
@@ -9405,20 +9409,10 @@ static bool nohz_idle_balance(struct rq
bool has_blocked_load = false;
int update_next_balance = 0;
int this_cpu = this_rq->cpu;
- unsigned int flags;
int balance_cpu;
+ int ret = false;
struct rq *rq;
- if (!(atomic_read(nohz_flags(this_cpu)) & NOHZ_KICK_MASK))
- return false;
-
- if (idle != CPU_IDLE) {
- atomic_andnot(NOHZ_KICK_MASK, nohz_flags(this_cpu));
- return false;
- }
-
- flags = atomic_fetch_andnot(NOHZ_KICK_MASK, nohz_flags(this_cpu));
-
SCHED_WARN_ON((flags & NOHZ_KICK_MASK) == NOHZ_BALANCE_KICK);
/*
@@ -9462,10 +9456,10 @@ static bool nohz_idle_balance(struct rq
if (time_after_eq(jiffies, rq->next_balance)) {
struct rq_flags rf;
- rq_lock_irq(rq, &rf);
+ rq_lock_irqsave(rq, &rf);
update_rq_clock(rq);
cpu_load_update_idle(rq);
- rq_unlock_irq(rq, &rf);
+ rq_unlock_irqrestore(rq, &rf);
if (flags & NOHZ_BALANCE_KICK)
rebalance_domains(rq, CPU_IDLE);
@@ -9477,13 +9471,21 @@ static bool nohz_idle_balance(struct rq
}
}
- update_blocked_averages(this_cpu);
+ /* Newly idle CPU doesn't need an update */
+ if (idle != CPU_NEWLY_IDLE) {
+ update_blocked_averages(this_cpu);
+ has_blocked_load |= this_rq->has_blocked_load;
+ }
+
if (flags & NOHZ_BALANCE_KICK)
rebalance_domains(this_rq, CPU_IDLE);
WRITE_ONCE(nohz.next_blocked,
now + msecs_to_jiffies(LOAD_AVG_PERIOD));
+ /* The full idle balance loop has been done */
+ ret = true;
+
abort:
/* There is still blocked load, enable periodic update */
if (has_blocked_load)
@@ -9497,15 +9499,79 @@ static bool nohz_idle_balance(struct rq
if (likely(update_next_balance))
nohz.next_balance = next_balance;
+ return ret;
+}
+
+/*
+ * In CONFIG_NO_HZ_COMMON case, the idle balance kickee will do the
+ * rebalancing for all the cpus for whom scheduler ticks are stopped.
+ */
+static bool nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)
+{
+ int this_cpu = this_rq->cpu;
+ unsigned int flags;
+
+ if (!(atomic_read(nohz_flags(this_cpu)) & NOHZ_KICK_MASK))
+ return false;
+
+ if (idle != CPU_IDLE) {
+ atomic_andnot(NOHZ_KICK_MASK, nohz_flags(this_cpu));
+ return false;
+
Re: [PATCH 4/4] fs/dcache: Avoid the try_lock loops in dentry_kill()
On Tue, Feb 20, 2018 at 09:39:37AM +0100, Peter Zijlstra wrote:
> On Tue, Feb 20, 2018 at 12:34:57AM +0100, John Ogness wrote:
> > Implementation 2: Using switch on a dentry_lock_inode() that returns a
> > tristate value. Does not support branch prediction. This approach is
> > probably easiest to understand.
> >
> > /*
> > * Lock the inode. Might drop dentry->d_lock temporarily
> > * which allows inode to change. Start over if that happens.
> > */
> > switch (dentry_lock_inode(dentry)) {
> > case LOCK_FAST:
>
> Bah, I just checked, you cannot use GCC label attributes on statements
> :/ Otherwise you could've done:
>
> case LOCK_FAST: __attribute__((hot));
Oooh shiny, you can actually write:
switch(__builtin_expect(dentry_lock_inode(dentry), LOCK_FAST)) {
and have that work, see:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59521
> > break;
> > case LOCK_SLOW:
> > /*
> > * Recheck refcount as it might have been
> > * incremented while d_lock was dropped.
> > */
> > if (unlikely(dentry->d_lockref.count != 1))
> > goto drop_ref;
> > break;
> > case LOCK_FAILED:
> > goto again;
> > }
> >
Re: [PATCH 2/7] sched/fair: Add group_misfit_task load-balance type
On Mon, Feb 19, 2018 at 02:56:44PM +0100, Peter Zijlstra wrote: > On Thu, Feb 15, 2018 at 04:20:49PM +, Morten Rasmussen wrote: > > @@ -6733,9 +6758,12 @@ done: __maybe_unused > > if (hrtick_enabled(rq)) > > hrtick_start_fair(rq, p); > > > > + update_misfit_status(p, rq); > > + > > return p; > > > > idle: > > + update_misfit_status(NULL, rq); > > new_tasks = idle_balance(rq, rf); > > > > /* > > So we set a point when picking a task (or tick). We clear said pointer > when idle. N/m, I can't read today. You only store the load, not the actual task.
Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu
This really should've been Cc'ed to me.
On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..dd580db289e8 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex
> *lock)
> return mutex_is_locked(&lock->base);
> }
>
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that
> context
> + * @lock: the mutex to be queried
> + * @task: the task structure to check
> + * @ctx: the w/w acquire context to test
> + *
> + * Returns true if the mutex is locked in the context by the given task,
> false
> + * otherwise.
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> + struct task_struct *task,
> + struct ww_acquire_ctx *ctx)
> +{
> + return likely(__mutex_owner(&lock->base) == task) &&
> + READ_ONCE(lock->ctx) == ctx;
> +}
Nak on that interface, that's racy and broken by design.
Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking
On Tue, Mar 28, 2017 at 02:30:56PM +0200, Greg Kroah-Hartman wrote:
> 4.10-stable review patch. If anyone has any objections, please let me know.
> + if (!(auditd_test_task(current) ||
> + (current == __mutex_owner(&audit_cmd_mutex {
> + long stime = audit_backlog_wait_time;
Since I cannot find the original email on lkml, NAK on this.
__mutex_owner() is not a general purpose helper function.
Re: [PATCH 2/7] sched/fair: Add group_misfit_task load-balance type
On Thu, Feb 15, 2018 at 04:20:49PM +, Morten Rasmussen wrote: > @@ -6733,9 +6758,12 @@ done: __maybe_unused > if (hrtick_enabled(rq)) > hrtick_start_fair(rq, p); > > + update_misfit_status(p, rq); > + > return p; > > idle: > + update_misfit_status(NULL, rq); > new_tasks = idle_balance(rq, rf); > > /* So we set a point when picking a task (or tick). We clear said pointer when idle. > @@ -7822,6 +7855,10 @@ static inline void update_sg_lb_stats(struct lb_env > *env, >*/ > if (!nr_running && idle_cpu(i)) > sgs->idle_cpus++; > + > + if (env->sd->flags & SD_ASYM_CPUCAPACITY && > + !sgs->group_misfit_task_load && rq->misfit_task_load) > + sgs->group_misfit_task_load = rq->misfit_task_load; > } > > /* Adjust by relative CPU capacity of the group */ And we read said pointer from another CPU, without holding the respective rq->lock. What happens, if right after we set sgs->group_misfit_task_load, our task decides to exit?
Re: [PATCH 4/7] sched/fair: Avoid unnecessary balancing of asymmetric capacity groups
On Mon, Feb 19, 2018 at 03:50:12PM +0100, Peter Zijlstra wrote: > On Thu, Feb 15, 2018 at 04:20:51PM +, Morten Rasmussen wrote: > > On systems with asymmetric cpu capacities, a skewed load distribution > > might yield better throughput than balancing load per group capacity. > > For example, preferring high capacity cpus for compute intensive tasks > > leaving low capacity cpus idle rather than balancing the number of idle > > cpus across different cpu types. Instead, let load-balance back off if > > the busiest group isn't really overloaded. > > I'm sorry. I just can't seem to make sense of that today. What? Aah, you're saying that is we have 4+4 bit.little and 4 runnable tasks, we would like them running on our big cluster and leave the small cluster entirely idle, instead of runnint 2+2. So what about this DynamicQ nonsense? Or is that so unstructured we can't really do anything sensible with it?
Re: [PATCH 4/7] sched/fair: Avoid unnecessary balancing of asymmetric capacity groups
On Thu, Feb 15, 2018 at 04:20:51PM +, Morten Rasmussen wrote: > On systems with asymmetric cpu capacities, a skewed load distribution > might yield better throughput than balancing load per group capacity. > For example, preferring high capacity cpus for compute intensive tasks > leaving low capacity cpus idle rather than balancing the number of idle > cpus across different cpu types. Instead, let load-balance back off if > the busiest group isn't really overloaded. I'm sorry. I just can't seem to make sense of that today. What?
Re: [RFC PATCH 3/5] asm-generic/bitops/atomic.h: Rewrite using atomic_fetch_*
On Mon, Feb 19, 2018 at 02:01:43PM +, Will Deacon wrote: > > The non serializing __clear_bit() was getting "lost" > > > > 80543b8e: ld_s r2,[r13,0] <--- (A) Finds PG_locked is set > > 80543b90: or r3,r2,1<--- (B) other core unlocks right > > here > > 80543b94: st_s r3,[r13,0] <--- (C) sets PG_locked > > (overwrites unlock) > > Ah, so it's problematic for the case where atomics are built using locks. > Got it. I'll err on the side of caution here and have the asm-generic header > (which should be bitops/lock.h not bitops/atomic.h) conditionally define > __clear_bit_unlock as clear_bit_lock unless the architecture has provided > its own implementation. So I think we get it all right if we use atomic_set_release(). If the atomics are implemented using locks, atomic_set*() should be implemented like atomic_xchg() and avoid the above problem.
Re: [RFCv4 08/21] [WAR] v4l2-ctrls: do not clone non-standard controls
On 02/20/18 05:44, Alexandre Courbot wrote:
> Only standard controls can be successfully cloned: handler_new_ref, used
> by v4l2_ctrl_request_clone(), forcibly calls v4l2_ctrl_new_std() which
> fails to find custom controls names, and we eventually hit the condition
> that name == NULL in v4l2_ctrl_new().
Hmm, the core reason is that handler_new_ref tries to automatically create
a new control class if it didn't exist yet. Which is OK for standard control
classes but not for non-standard control classes such as is used in vivid.
I will have to think about this.
Regards,
Hans
>
> This prevents us from using non-standard controls with requests, but
> that is enough for testing purposes.
>
> Signed-off-by: Alexandre Courbot
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 166647817efb..7a81aa5959c3 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2772,6 +2772,11 @@ int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler
> *hdl,
> if (filter && !filter(ctrl))
> continue;
> err = handler_new_ref(hdl, ctrl, &new_ref, false);
> + if (err) {
> + printk("%s: handler_new_ref on control %x (%s) returned
> %d\n", __func__, ctrl->id, ctrl->name, err);
> + err = 0;
> + continue;
> + }
> if (err)
> break;
> if (from->is_request)
>
Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu
Am 20.02.2018 um 13:35 schrieb Peter Zijlstra:
This really should've been Cc'ed to me.
On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 39fda195bf78..dd580db289e8 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex
*lock)
return mutex_is_locked(&lock->base);
}
+/**
+ * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
+ * @lock: the mutex to be queried
+ * @task: the task structure to check
+ * @ctx: the w/w acquire context to test
+ *
+ * Returns true if the mutex is locked in the context by the given task, false
+ * otherwise.
+ */
+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+ struct task_struct *task,
+ struct ww_acquire_ctx *ctx)
+{
+ return likely(__mutex_owner(&lock->base) == task) &&
+ READ_ONCE(lock->ctx) == ctx;
+}
Nak on that interface, that's racy and broken by design.
Why?
Regards,
Christian.
[PATCH] md: raid5: avoid string overflow warning
gcc warns about a possible overflow of the kmem_cache string, when adding
four characters to a string of the same length:
drivers/md/raid5.c: In function 'setup_conf':
drivers/md/raid5.c:2207:34: error: '-alt' directive writing 4 bytes into a
region of size between 1 and 32 [-Werror=format-overflow=]
sprintf(conf->cache_name[1], "%s-alt", conf->cache_name[0]);
^~~~
drivers/md/raid5.c:2207:2: note: 'sprintf' output between 5 and 36 bytes into a
destination of size 32
sprintf(conf->cache_name[1], "%s-alt", conf->cache_name[0]);
^~~
If I'm counting correctly, we need 11 characters for the fixed part
of the string and 18 characters for a 64-bit pointer (when no gendisk
is used), so that leaves three characters for conf->level, which should
always be sufficient.
This makes the code use snprintf() with the correct length, to
make the code more robust against changes, and to get the compiler
to shut up.
In commit f4be6b43f1ac ("md/raid5: ensure we create a unique name for
kmem_cache when mddev has no gendisk") from 2010, Neil said that
the pointer could be removed "shortly" once devices without gendisk
are disallowed. I have no idea if that happened, but if it did, that
should probably be changed as well.
Signed-off-by: Arnd Bergmann
---
drivers/md/raid5.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 50d01144b805..7ef368061424 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2196,15 +2196,16 @@ static int grow_one_stripe(struct r5conf *conf, gfp_t
gfp)
static int grow_stripes(struct r5conf *conf, int num)
{
struct kmem_cache *sc;
+ size_t namelen = sizeof(conf->cache_name[0]);
int devs = max(conf->raid_disks, conf->previous_raid_disks);
if (conf->mddev->gendisk)
- sprintf(conf->cache_name[0],
+ snprintf(conf->cache_name[0], namelen,
"raid%d-%s", conf->level, mdname(conf->mddev));
else
- sprintf(conf->cache_name[0],
+ snprintf(conf->cache_name[0], namelen,
"raid%d-%p", conf->level, conf->mddev);
- sprintf(conf->cache_name[1], "%s-alt", conf->cache_name[0]);
+ snprintf(conf->cache_name[1], namelen, "%.27s-alt",
conf->cache_name[0]);
conf->active_name = 0;
sc = kmem_cache_create(conf->cache_name[conf->active_name],
--
2.9.0
Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3
On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:
> amdgpu needs to verify if userspace sends us valid addresses and the simplest
> way of doing this is to check if the buffer object is locked with the ticket
> of the current submission.
>
> Clean up the access to the ww_mutex internals by providing a function
> for this and extend the check to the thread owning the underlying mutex.
> Signed-off-by: Christian König
Much thanks for Cc'ing the relevant maintainers :/
> ---
> include/linux/ww_mutex.h | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..14e4149d3d9d 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex
> *lock)
> return mutex_is_locked(&lock->base);
> }
>
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that
> context
> + * @lock: the mutex to be queried
> + * @ctx: the w/w acquire context to test
> + *
> + * If @ctx is not NULL test if the mutex is owned by this context.
> + * If @ctx is NULL test if the mutex is owned by the current thread.
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> + struct ww_acquire_ctx *ctx)
> +{
> + if (ctx)
> + return likely(READ_ONCE(lock->ctx) == ctx);
> + else
> + return likely(__mutex_owner(&lock->base) == current);
> +}
Much better than the previous version. If you want to bike-shed, you can
leave out the 'else' and unindent the last line.
I do worry about potential users of .ctx = NULL, though. It makes it far
too easy to do recursive locking, which is something we should strongly
discourage.
Re: [PATCH] blk: optimization for classic polling
On Sun, May 30, 2083 at 09:51:06AM +0530, Nitesh Shetty wrote:
> This removes the dependency on interrupts to wake up task. Set task
> state as TASK_RUNNING, if need_resched() returns true,
> while polling for IO completion.
> Earlier, polling task used to sleep, relying on interrupt to wake it up.
> This made some IO take very long when interrupt-coalescing is enabled in
> NVMe.
This is a horrible Changelog.. it does not in fact explain why the patch
works or is correct.
Also, set_current_state(TASK_RUNNING) is dodgy (similarly in
__blk_mq_poll), why do you need that memory barrier?
> Signed-off-by: Nitesh Shetty
> ---
> fs/block_dev.c | 16
> fs/direct-io.c | 8 ++--
> fs/iomap.c | 10 +++---
> 3 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 4a181fc..a87d8b7 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -236,9 +236,13 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct
> iov_iter *iter,
> set_current_state(TASK_UNINTERRUPTIBLE);
> if (!READ_ONCE(bio.bi_private))
> break;
> - if (!(iocb->ki_flags & IOCB_HIPRI) ||
> - !blk_poll(bdev_get_queue(bdev), qc))
> + if (!(iocb->ki_flags & IOCB_HIPRI))
> io_schedule();
> + else if (!blk_poll(bdev_get_queue(bdev), qc)) {
> + if (need_resched())
> + set_current_state(TASK_RUNNING);
> + io_schedule();
> + }
> }
> __set_current_state(TASK_RUNNING);
>
Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu
On Tue, Feb 20, 2018 at 02:08:26PM +0100, Christian König wrote:
> Am 20.02.2018 um 13:35 schrieb Peter Zijlstra:
> > > +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> > > + struct task_struct *task,
> > > + struct ww_acquire_ctx *ctx)
> > > +{
> > > + return likely(__mutex_owner(&lock->base) == task) &&
> > > + READ_ONCE(lock->ctx) == ctx;
> > > +}
> > Nak on that interface, that's racy and broken by design.
>
> Why?
If task != current you can race with a concurrent mutex_unlock().
Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking
On Tue, Feb 20, 2018 at 7:37 AM, Peter Zijlstra wrote:
> On Tue, Mar 28, 2017 at 02:30:56PM +0200, Greg Kroah-Hartman wrote:
>> 4.10-stable review patch. If anyone has any objections, please let me know.
>
>> + if (!(auditd_test_task(current) ||
>> + (current == __mutex_owner(&audit_cmd_mutex {
>> + long stime = audit_backlog_wait_time;
>
> Since I cannot find the original email on lkml, NAK on this.
> __mutex_owner() is not a general purpose helper function.
Since this code also exists in the current kernel, I need to ask what
recommended alternatives exist for determining the mutex owner?
I imagine we could track the mutex owner separately in the audit
subsystem, but I'd much prefer to leverage an existing mechanism if
possible.
--
paul moore
www.paul-moore.com
Re: [RFCv4 09/21] v4l2: add request API support
On 02/20/18 05:44, Alexandre Courbot wrote:
> Add a v4l2 request entity data structure that takes care of storing the
> request-related state of a V4L2 device ; in this case, its controls.
>
> Signed-off-by: Alexandre Courbot
> ---
> drivers/media/v4l2-core/Makefile | 1 +
> drivers/media/v4l2-core/v4l2-request.c | 178 +
> include/media/v4l2-request.h | 159 ++
> 3 files changed, 338 insertions(+)
> create mode 100644 drivers/media/v4l2-core/v4l2-request.c
> create mode 100644 include/media/v4l2-request.h
>
> diff --git a/drivers/media/v4l2-core/Makefile
> b/drivers/media/v4l2-core/Makefile
> index 80de2cb9c476..13d0477535bd 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -16,6 +16,7 @@ ifeq ($(CONFIG_TRACEPOINTS),y)
>videodev-objs += vb2-trace.o v4l2-trace.o
> endif
> videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
> +videodev-$(CONFIG_MEDIA_REQUEST_API) += v4l2-request.o
>
> obj-$(CONFIG_VIDEO_V4L2) += videodev.o
> obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
> diff --git a/drivers/media/v4l2-core/v4l2-request.c
> b/drivers/media/v4l2-core/v4l2-request.c
> new file mode 100644
> index ..e8ad10e2f525
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-request.c
> @@ -0,0 +1,178 @@
> +/*
> + * Media requests support for V4L2
> + *
> + * Copyright (C) 2018, The Chromium OS Authors. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +
> +#include
> +#include
> +#include
> +
> +void v4l2_request_entity_init(struct v4l2_request_entity *entity,
> + const struct media_request_entity_ops *ops,
> + struct video_device *vdev)
> +{
> + media_request_entity_init(&entity->base,
> MEDIA_REQUEST_ENTITY_TYPE_V4L2, ops);
> + entity->vdev = vdev;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_request_entity_init);
> +
> +struct media_request_entity_data *
> +v4l2_request_entity_data_alloc(struct media_request *req,
> +struct v4l2_ctrl_handler *hdl)
> +{
> + struct v4l2_request_entity_data *data;
> + int ret;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = v4l2_ctrl_request_init(&data->ctrls);
> + if (ret) {
> + kfree(data);
> + return ERR_PTR(ret);
> + }
> + ret = v4l2_ctrl_request_clone(&data->ctrls, hdl, NULL);
> + if (ret) {
> + kfree(data);
> + return ERR_PTR(ret);
> + }
> +
> + INIT_LIST_HEAD(&data->queued_buffers);
> +
> + return &data->base;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_request_entity_data_alloc);
> +
> +void v4l2_request_entity_data_free(struct media_request_entity_data *_data)
> +{
> + struct v4l2_request_entity_data *data;
> + struct v4l2_vb2_request_buffer *qb, *n;
> +
> + data = to_v4l2_entity_data(_data);
> +
> + list_for_each_entry_safe(qb, n, &data->queued_buffers, node) {
> + struct vb2_buffer *buf;
> + dev_warn(_data->request->mgr->dev,
> + "entity data freed while buffer still queued!\n");
> +
> + /* give buffer back to user-space */
> + buf = qb->queue->bufs[qb->v4l2_buf.index];
> + buf->state = qb->pre_req_state;
> + buf->request = NULL;
> +
> + kfree(qb);
> + }
> +
> + v4l2_ctrl_handler_free(&data->ctrls);
> + kfree(data);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_request_entity_data_free);
> +
> +
> +
> +
> +
> +static struct media_request *v4l2_request_alloc(struct media_request_mgr
> *mgr)
> +{
> + struct media_request *req;
> +
> + req = kzalloc(sizeof(*req), GFP_KERNEL);
> + if (!req)
> + return ERR_PTR(-ENOMEM);
> +
> + req->mgr = mgr;
> + req->state = MEDIA_REQUEST_STATE_IDLE;
> + INIT_LIST_HEAD(&req->data);
> + init_waitqueue_head(&req->complete_wait);
> + mutex_init(&req->lock);
> +
> + mutex_lock(&mgr->mutex);
> + list_add_tail(&req->list, &mgr->requests);
> + mutex_unlock(&mgr->mutex);
> +
> + return req;
> +}
> +
> +static void v4l2_request_free(struct media_request *req)
> +{
> + struct media_request_mgr *mgr = req->mgr;
> + struct media_request_entity_data *data, *next;
> +
> + mutex_lock(&mgr->mutex);
> + list_del(&req->list);
> + mutex_unlock(&mgr->mutex);
> +
> + list_for_each_entry_safe(data, next, &req->data, list) {
> + list_del(&data->
Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3
Am 20.02.2018 um 14:12 schrieb Peter Zijlstra:
On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:
amdgpu needs to verify if userspace sends us valid addresses and the simplest
way of doing this is to check if the buffer object is locked with the ticket
of the current submission.
Clean up the access to the ww_mutex internals by providing a function
for this and extend the check to the thread owning the underlying mutex.
Signed-off-by: Christian König
Much thanks for Cc'ing the relevant maintainers :/
Sorry for that.
---
include/linux/ww_mutex.h | 17 +
1 file changed, 17 insertions(+)
diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 39fda195bf78..14e4149d3d9d 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex
*lock)
return mutex_is_locked(&lock->base);
}
+/**
+ * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
+ * @lock: the mutex to be queried
+ * @ctx: the w/w acquire context to test
+ *
+ * If @ctx is not NULL test if the mutex is owned by this context.
+ * If @ctx is NULL test if the mutex is owned by the current thread.
+ */
+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+ struct ww_acquire_ctx *ctx)
+{
+ if (ctx)
+ return likely(READ_ONCE(lock->ctx) == ctx);
+ else
+ return likely(__mutex_owner(&lock->base) == current);
+}
Much better than the previous version. If you want to bike-shed, you can
leave out the 'else' and unindent the last line.
Thanks for the suggestion, going to do this.
I do worry about potential users of .ctx = NULL, though. It makes it far
too easy to do recursive locking, which is something we should strongly
discourage.
Well, one of the addressed use cases is indeed checking for recursive
locking. But recursive locking is something rather normal for ww_mutex
and we are just exercising an existing code path.
E.g. the most common use case for the ww_mutex is in the graphics
drivers where usespace sends us a list of buffer objects to work with.
Now when userspace sends us duplicates in that buffer list the
expectation is to get -EALREADY from ww_mutex_lock when we try to lock
the same ww_mutex twice.
Depending on the driver this then results in returning an error code to
userspace or just ignoring the duplicate (because of backward
compatibility).
The intention behind this function is now to a) be able to extend those
checks to make sure user space doesn't sends us potentially harmful
nonsense and b) allow to check for recursion in TTM during buffer object
eviction which uses ww_mutex_trylock instead of ww_mutex_lock.
Regards,
Christian.
Re: [PATCH] watchdog: add SPDX identifiers for watchdog subsystem
Marcus Folkesson writes: > - Add SPDX identifier > - Remove boiler plate license text > - If MODULE_LICENSE and boiler plate does not match, go for boiler plate > license > > Signed-off-by: Marcus Folkesson For bcm2835, Reviewed-by: Eric Anholt signature.asc Description: PGP signature
[PATCH] iio:dummy: Fix coding style in Kconfig
This patch fixes the checkpatch.pl warning: drivers/iio/dummy/Kconfig:21: WARNING: please write a paragraph that describes the config symbol fully drivers/iio/dummy/Kconfig:29: WARNING: please write a paragraph that describes the config symbol fully This patch expands the explanation about IIO_DUMMY_EVGEN by using the code documentation found in iio/dummy/iio_dummy_evgen.c. In the same way, the information related to IIO_SIMPLE_DUMMY_BUFFER was extracted from file iio_simple_dummy_buffer.c. Finally, this patch apply the coding-style for Kconfig files (Documentation/process/coding-style.rst) Signed-off-by: Rodrigo Siqueira --- drivers/iio/dummy/Kconfig | 26 -- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/drivers/iio/dummy/Kconfig b/drivers/iio/dummy/Kconfig index 5a29fbd3c531..4a7127fb2979 100644 --- a/drivers/iio/dummy/Kconfig +++ b/drivers/iio/dummy/Kconfig @@ -9,20 +9,24 @@ config IIO_DUMMY_EVGEN tristate config IIO_SIMPLE_DUMMY - tristate "An example driver with no hardware requirements" - depends on IIO_SW_DEVICE - help -Driver intended mainly as documentation for how to write -a driver. May also be useful for testing userspace code -without hardware. + tristate "An example driver with no hardware requirements" + depends on IIO_SW_DEVICE + help + Driver intended mainly as documentation for how to write + a driver. May also be useful for testing userspace code + without hardware. if IIO_SIMPLE_DUMMY config IIO_SIMPLE_DUMMY_EVENTS - bool "Event generation support" - select IIO_DUMMY_EVGEN - help - Add some dummy events to the simple dummy driver. + bool "Event generation support" + select IIO_DUMMY_EVGEN + help + Add some dummy events to the simple dummy driver. + + The purpose of this is to generate 'fake' event interrupts thus + allowing that driver's code to be as close as possible to that + a normal driver talking to hardware. config IIO_SIMPLE_DUMMY_BUFFER bool "Buffered capture support" @@ -32,6 +36,8 @@ config IIO_SIMPLE_DUMMY_BUFFER help Add buffered data capture to the simple dummy driver. + Buffer handling elements of industrial I/O reference driver. + Uses the kfifo buffer. endif # IIO_SIMPLE_DUMMY endmenu -- 2.16.1
[PATCH net-next] tcp: remove the hardcode in the definition of TCPF Macro
TCPF_ macro depends on the definition of TCP_ macro.
So it is better to define them with TCP_ marco.
Signed-off-by: Yafang Shao
---
include/net/tcp_states.h | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/include/net/tcp_states.h b/include/net/tcp_states.h
index 50e78a7..2875e16 100644
--- a/include/net/tcp_states.h
+++ b/include/net/tcp_states.h
@@ -32,21 +32,21 @@ enum {
#define TCP_STATE_MASK 0xF
-#define TCP_ACTION_FIN (1 << 7)
+#define TCP_ACTION_FIN (1 << TCP_CLOSE)
enum {
- TCPF_ESTABLISHED = (1 << 1),
- TCPF_SYN_SENT= (1 << 2),
- TCPF_SYN_RECV= (1 << 3),
- TCPF_FIN_WAIT1 = (1 << 4),
- TCPF_FIN_WAIT2 = (1 << 5),
- TCPF_TIME_WAIT = (1 << 6),
- TCPF_CLOSE = (1 << 7),
- TCPF_CLOSE_WAIT = (1 << 8),
- TCPF_LAST_ACK= (1 << 9),
- TCPF_LISTEN = (1 << 10),
- TCPF_CLOSING = (1 << 11),
- TCPF_NEW_SYN_RECV = (1 << 12),
+ TCPF_ESTABLISHED = (1 << TCP_ESTABLISHED),
+ TCPF_SYN_SENT= (1 << TCP_SYN_SENT),
+ TCPF_SYN_RECV= (1 << TCP_SYN_RECV),
+ TCPF_FIN_WAIT1 = (1 << TCP_FIN_WAIT1),
+ TCPF_FIN_WAIT2 = (1 << TCP_FIN_WAIT2),
+ TCPF_TIME_WAIT = (1 << TCP_TIME_WAIT),
+ TCPF_CLOSE = (1 << TCP_CLOSE),
+ TCPF_CLOSE_WAIT = (1 << TCP_CLOSE_WAIT),
+ TCPF_LAST_ACK= (1 << TCP_LAST_ACK),
+ TCPF_LISTEN = (1 << TCP_LISTEN),
+ TCPF_CLOSING = (1 << TCP_CLOSING),
+ TCPF_NEW_SYN_RECV = (1 << TCP_NEW_SYN_RECV),
};
#endif /* _LINUX_TCP_STATES_H */
--
1.8.3.1
Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC
On 02/20/2018 02:53 PM, Oleksandr Andrushchenko wrote:
On 02/20/2018 02:49 PM, Daniel Vetter wrote:
On Tue, Feb 20, 2018 at 02:36:05PM +0200, Oleksandr Andrushchenko wrote:
On 02/20/2018 01:17 PM, Daniel Vetter wrote:
On Mon, Feb 19, 2018 at 04:58:43PM +0200, Oleksandr Andrushchenko
wrote:
On 02/19/2018 04:30 PM, Daniel Vetter wrote:
On Tue, Feb 13, 2018 at 10:44:16AM +0200, Oleksandr Andrushchenko
wrote:
From: Oleksandr Andrushchenko
It is possible that drm_simple_kms_plane_atomic_check called
with no CRTC set, e.g. when user-space application sets
CRTC_ID/FB_ID
to 0 before doing any actual drawing. This leads to NULL pointer
dereference because in this case new CRTC state is NULL and must be
checked before accessing.
Signed-off-by: Oleksandr Andrushchenko
---
drivers/gpu/drm/drm_simple_kms_helper.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 9ca8a4a59b74..a05eca9cec8b 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -121,8 +121,10 @@ static int
drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
pipe = container_of(plane, struct
drm_simple_display_pipe, plane);
crtc_state =
drm_atomic_get_new_crtc_state(plane_state->state,
&pipe->crtc);
- if (!crtc_state->enable)
- return 0; /* nothing to check when disabling or
disabled */
+
+ if (!crtc_state || !crtc_state->enable)
+ /* nothing to check when disabling or disabled or no
CRTC set */
+ return 0;
if (crtc_state->enable)
drm_mode_get_hv_timing(&crtc_state->mode,
Hm, this is a bit annoying, since the can_position = false
parameter to
drm_atomic_helper_check_plane_state is supposed to catch all
this. Would
moving all the checks after the call to that helper, and gating
them on
plane_state->visible also work?
Yes, it does work if this is what you mean:
I wasn't sure, thanks for figuring this out!
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index a05eca9cec8b..c48858bb2823 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -122,14 +122,6 @@ static int
drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
crtc_state =
drm_atomic_get_new_crtc_state(plane_state->state,
&pipe->crtc);
- if (!crtc_state || !crtc_state->enable)
- /* nothing to check when disabling or disabled or
no CRTC
set */
- return 0;
-
- if (crtc_state->enable)
- drm_mode_get_hv_timing(&crtc_state->mode,
- &clip.x2, &clip.y2);
-
ret = drm_atomic_helper_check_plane_state(plane_state,
crtc_state,
&clip,
DRM_PLANE_HELPER_NO_SCALING,
@@ -138,6 +130,13 @@ static int
drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
if (ret)
return ret;
+ if (!plane_state->visible || !crtc_state->enable)
+ return 0; /* nothing to check when disabling or
disabled */
if (!plane_state->visible) {
WARN_ON(crtc_state->enabled);
return 0;
}
The helper call above should guarantee this.
Yes, but I still see cases when crtc_state is NULL, thus
making crtc_state->enable to fail
Right, when the plane is completely off there's no CRTC state. Correct
check should be
WARN_ON(crtc_state && crtc_state->enabled);
ok, will update with this additional check
huh, this indeed solves the NULL pointer dereference, but floods a lot
with every page flip I have, e.g. !plane_state->visible == true
and crtc_state is not NULL and crtc_state->enable == true,
thus firing WARN_ON.
Is this something wrong with my use-case/driver or it is still legal
to have such a configuration and leave it without WARN_ON and just
return 0?
+
+ if (plane_state->visible && crtc_state->enable)
Similar here.
+ drm_mode_get_hv_timing(&crtc_state->mode,
+ &clip.x2, &clip.y2);
+
if (!plane_state->visible)
return -EINVAL;
This can now be removed, the plane helper takes care of checking for
plane_state->visible != crtc_state->enable. Please also remove.
We'd need to add a guarantee to
drm_atomic_helper_check_plane_state that
it can cope with crtc_state == NULL, but I think that's a good idea
anyway. Atm it shouldn't end up looking at the crtc_state pointer
in that
case.
It doesn't look at it at the moment
Otherwise we'll just go with your fix, but it feels all a bit too
fragile,
hence why I want to explore more robust options a bit.
At list with the change above it passes my test which failed
before. Although I cannot confirm it works for others, but it
certainly does for me.
-Daniel
Do you want me to send v1 with the code above?
Yes please, with my above cleanup suggestions.
Please see the patch under t
Re: [PATCH ipmi/kcs_bmc v2] ipmi: kcs_bmc: make the code be more clean
On 02/19/2018 09:55 AM, Haiyue Wang wrote:
---
When you use ---, it means everything following is not in the commit text,
including your signature.
v1 -> v2:
Do you want me to fold this into the previous patch? That's generally
not how things work, a new patch is fine for this, with a list of things
done like below.
One comment inline below...
Add 'SPDX-License-Identifier' style for header files modification.
---
1. Add the missed key word '__user' for read / write.
2. Remove the prefix 'file' of 'file_to_kcs_bmc', no need this
duplicated word as its parameter has 'struct file *filp'.
3. Change the 'unsigned int' to '__poll_t' to meet the new 'poll'
definition.
4. Correct the 'SPDX-License-Identifier' style for header files.
Signed-off-by: Haiyue Wang
---
drivers/char/ipmi/kcs_bmc.c| 32 +---
drivers/char/ipmi/kcs_bmc.h| 6 --
drivers/char/ipmi/kcs_bmc_aspeed.c | 4 +++-
include/uapi/linux/ipmi_bmc.h | 6 --
4 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index 6476bfb..fbfc05e 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2015-2018, Intel Corporation.
+/*
+ * Copyright (c) 2015-2018, Intel Corporation.
+ */
#define pr_fmt(fmt) "kcs-bmc: " fmt
@@ -242,14 +244,14 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
}
EXPORT_SYMBOL(kcs_bmc_handle_event);
-static inline struct kcs_bmc *file_to_kcs_bmc(struct file *filp)
+static inline struct kcs_bmc *to_kcs_bmc(struct file *filp)
{
return container_of(filp->private_data, struct kcs_bmc, miscdev);
}
static int kcs_bmc_open(struct inode *inode, struct file *filp)
{
- struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
+ struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
int ret = 0;
spin_lock_irq(&kcs_bmc->lock);
@@ -262,25 +264,25 @@ static int kcs_bmc_open(struct inode *inode, struct file
*filp)
return ret;
}
-static unsigned int kcs_bmc_poll(struct file *filp, poll_table *wait)
+static __poll_t kcs_bmc_poll(struct file *filp, poll_table *wait)
{
- struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
- unsigned int mask = 0;
+ struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
+ __poll_t mask = 0;
poll_wait(filp, &kcs_bmc->queue, wait);
spin_lock_irq(&kcs_bmc->lock);
if (kcs_bmc->data_in_avail)
- mask |= POLLIN;
+ mask |= EPOLLIN;
I get this:
CC [M] drivers/char/ipmi/kcs_bmc.o
../drivers/char/ipmi/kcs_bmc.c: In function ‘kcs_bmc_poll’:
../drivers/char/ipmi/kcs_bmc.c:276:11: error: ‘EPOLLIN’ undeclared
(first use in this function)
mask |= EPOLLIN;
^
../drivers/char/ipmi/kcs_bmc.c:276:11: note: each undeclared identifier
is reported only once for each function it appears in
probably need to include linux/eventpoll.h
-corey
spin_unlock_irq(&kcs_bmc->lock);
return mask;
}
-static ssize_t kcs_bmc_read(struct file *filp, char *buf,
- size_t count, loff_t *offset)
+static ssize_t kcs_bmc_read(struct file *filp, char __user *buf,
+ size_t count, loff_t *ppos)
{
- struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
+ struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
bool data_avail;
size_t data_len;
ssize_t ret;
@@ -339,10 +341,10 @@ static ssize_t kcs_bmc_read(struct file *filp, char *buf,
return ret;
}
-static ssize_t kcs_bmc_write(struct file *filp, const char *buf,
-size_t count, loff_t *offset)
+static ssize_t kcs_bmc_write(struct file *filp, const char __user *buf,
+size_t count, loff_t *ppos)
{
- struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
+ struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
ssize_t ret;
/* a minimum response size '3' : netfn + cmd + ccode */
@@ -378,7 +380,7 @@ static ssize_t kcs_bmc_write(struct file *filp, const char
*buf,
static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
{
- struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
+ struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
long ret = 0;
spin_lock_irq(&kcs_bmc->lock);
@@ -410,7 +412,7 @@ static long kcs_bmc_ioctl(struct file *filp, unsigned int
cmd,
static int kcs_bmc_release(struct inode *inode, struct file *filp)
{
- struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
+ struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
spin_lock_irq(&kcs_bmc->lock);
kcs_bmc->running = 0;
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index c19501d..69d9a70 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -1,5 +1,7 @@
-// SPDX-License-Ident
Re: [PATCH v7 00/42] ARM: davinci: convert to common clock framework
2018-02-19 21:21 GMT+01:00 David Lechner : > This series converts mach-davinci to use the common clock framework. > > The series works like this, the first 19 patches create new clock drivers > using the common clock framework. There are basically 3 groups of clocks - > PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each have > unique init data, which is the reason for so many patches. > > Then, starting with "ARM: davinci: pass clock as parameter to > davinci_timer_init()", we get the mach code ready for the switch by adding the > code needed for the new clock drivers and adding #ifndef CONFIG_COMMON_CLK > around the legacy clocks so that we can switch easily between the old and the > new. > > "ARM: davinci: switch to common clock framework" actually flips the switch > to start using the new clock drivers. Then the next 8 patches remove all > of the old clock code. > > The final three patches add device tree clock support to the one SoC that > supports it. > > --- > > The change to make all of the clocks platform devices in v7 was a pretty > major change, so unfortunately I had to drop quite a few Reviewed-bys and > this series will need more close review and testing again. > > v7 changes (also see individual patches for details): > - Rebased on linux-davinci/master (v4.16-rc) > - Convert clock drivers to platform devices > - New patch "ARM: davinci: pass clock as parameter to davinci_timer_init()" > - Fix issues with lcdk and aemif clock lookups and power domains > - Fixed other minor issues brought up in v6 review > > v6 changes (also see individual patches for details): > - All of the device tree bindings are changed > - All of the clock drivers are changed significantly > - Fixed issues brought up during review of v5 > - "ARM: davinci: move davinci_clk_init() to init_time" is removed from this > series and submitted separately > > v5 changes: > - Basically, this is an entirely new series > - Patches are broken up into bite-sized pieces > - Converted PSC clock driver to use regmap > - Restored "force" flag for certain DA850 clocks > - Added device tree bindings > - Moved more of the clock init to drivers/clk > - Fixed frequency scaling (maybe*) > > * I have frequency scaling using cpufreq-dt, so I know the clocks are doing > what they need to do to make this work, but I haven't figured out how to > test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work will be > sent separately after this series has landed.) > > Dependencies: > > Most of the dependencies have landed in mainline already in v4.16-rc1 or > are in linux-davinci/master. > > There are no build dependencies any more, but you will need to pickup a > couple of patches to get CPU frequency scaling working because of a divider > clock bug. > > - https://patchwork.kernel.org/patch/10218941/ > - https://patchwork.kernel.org/patch/10218927/ > > You can find a working branch with everything included in the "common-clk-v7" > branch of https://github.com/dlech/ev3dev-kernel.git. > > > Testing/debugging for the uninitiated: > > I only have one device to test with, which is based on da850, so I will > have to rely on others to do some testing here. Since we are dealing with > clocks, if something isn't working, you most likely won't see output on > the serial port. To figure out what is going on, you need to enable... > > CONFIG_DEBUG_LL=y > CONFIG_EARLY_PRINTK=y > > and add "earlyprintk clk_ignore_unused" to the kernel command line options. > You may need to select a different UART for this depending on your board. I > think UART1 is the default in the kernel configuration. > > On da850 devices comment out the lines: > > else > clk_set_parent(clk, parent->clk); > > in da850.c or, if using device tree, comment out the lines: > > assigned-clocks = <&async3_clk>; > assigned-clock-parents = <&pll1_sysclk 2>; > > in da850.dtsi when doing earlyprintk, otherwise the UART1 and UART2 clock > source will change during boot and cause garbled output after a point. > > David Lechner (42): > dt-bindings: clock: Add new bindings for TI Davinci PLL clocks > clk: davinci: New driver for davinci PLL clocks > clk: davinci: Add platform information for TI DA830 PLL > clk: davinci: Add platform information for TI DA850 PLL > clk: davinci: Add platform information for TI DM355 PLL > clk: davinci: Add platform information for TI DM365 PLL > clk: davinci: Add platform information for TI DM644x PLL > clk: davinci: Add platform information for TI DM646x PLL > dt-bindings: clock: New bindings for TI Davinci PSC > clk: davinci: New driver for davinci PSC clocks > clk: davinci: Add platform information for TI DA830 PSC > clk: davinci: Add platform information for TI DA850 PSC > clk: davinci: Add platform information for TI DM355 PSC > clk: davinci: Add platform information for TI DM365 PSC > clk: davinci: Add platform information for TI DM644x PSC > clk: davi
Re: [PATCH 05/17] trace doc: convert trace/ftrace.txt to rst format
Hi, On Tue, Feb 20, 2018 at 08:28:24AM +0100, Philippe Ombredanne wrote: > Changbin, Steven, > > On Sat, Feb 17, 2018 at 6:39 AM, wrote: > > From: Changbin Du > > > > This converts the plain text documentation to reStructuredText format and > > add it into Sphinx TOC tree. No essential content change. > > > > Cc: Steven Rostedt > > Signed-off-by: Changbin Du > > --- > > Documentation/trace/ftrace.rst | 3332 > > > > Documentation/trace/ftrace.txt | 3220 > > -- > > Documentation/trace/index.rst |1 + > > 3 files changed, insertions(+), 3220 deletions(-) > > create mode 100644 Documentation/trace/ftrace.rst > > delete mode 100644 Documentation/trace/ftrace.txt > > > > diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst > > new file mode 100644 > > index 000..636aa9bf > > --- /dev/null > > +++ b/Documentation/trace/ftrace.rst > > @@ -0,0 +1,3332 @@ > > + > > +ftrace - Function Tracer > > + > > + > > +Copyright 2008 Red Hat Inc. > > + > > +:Author: Steven Rostedt > > +:License: The GNU Free Documentation License, Version 1.2 > > + (dual licensed under the GPL v2) > > > Do you mind using an SPDX id per [1] rather that this? > > Steven, are you OK with this? Can you ack? > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst > -- > Cordially > Philippe Ombredanne I leave this to Steven, since I just converted the format of this doc. -- Thanks, Changbin Du
Re: [PATCH 11/15] gpio: rcar: Add R-Car M3-N compatible string
Hi Geert,
On Wed, Feb 14, 2018 at 03:05:05PM +0100, Geert Uytterhoeven wrote:
> On Tue, Feb 13, 2018 at 10:45 AM, Jacopo Mondi
> wrote:
> > Add compatible string for R-Car M3-N (r8a77965) in gpio-rcar.
> >
> > Signed-off-by: Jacopo Mondi
>
> > --- a/drivers/gpio/gpio-rcar.c
> > +++ b/drivers/gpio/gpio-rcar.c
> > @@ -360,6 +360,10 @@ static const struct of_device_id gpio_rcar_of_table[]
> > = {
> > /* Gen3 GPIO is identical to Gen2. */
> > .data = &gpio_rcar_info_gen2,
> > }, {
> > + .compatible = "renesas,gpio-r8a77965",
> > + /* Gen3 GPIO is identical to Gen2. */
> > + .data = &gpio_rcar_info_gen2,
> > + }, {
>
> This part is not needed, as the driver already matches agains the generic
> "renesas,rcar-gen3-gpio".
Just to point out that the compatible string is there for M3-W and H3.
Anyway, if that's not good practice to add per-SoC strings here, I'll
drop this bit.
Thanks
j
>
> > .compatible = "renesas,rcar-gen1-gpio",
> > .data = &gpio_rcar_info_gen1,
> > }, {
>
> With the above fixed:
> Reviewed-by: Geert Uytterhoeven
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH 1/2] dmaengine: Introduce DW AXI DMAC driver
On Tue, Feb 20, 2018 at 2:30 PM, Eugeniy Paltsev
wrote:
> This patch adds support for the DW AXI DMAC controller.
> DW AXI DMAC is a part of HSDK development board from Synopsys.
>
> In this driver implementation only DMA_MEMCPY transfers are
> supported.
Just few comments, code looks fine I hope.
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13344,6 +13344,12 @@ F: include/linux/dma/dw.h
> F: include/linux/platform_data/dma-dw.h
> F: drivers/dma/dw/
>
> +SYNOPSYS DESIGNWARE AXI DMAC DRIVER
> +M: Eugeniy Paltsev
> +S: Maintained
> +F: drivers/dma/dwi-axi-dmac/
> +F: Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
AXI is earlier in alphabet than DMAC in previous record.
Linus even submitted a script last year to sort the MAINTAINERS data base.
> +/*
> + * Synopsys DesignWare AXI DMA Controller driver.
> + *
> + * Copyright (C) 2017-2018 Synopsys, Inc. (www.synopsys.com)
> + * Author: Eugeniy Paltsev
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
Now we have SPDX identifiers.
> +static u32 axi_chan_get_xfer_width(struct axi_dma_chan *chan, dma_addr_t src,
> + dma_addr_t dst, size_t len)
> +{
> + u32 max_width = chan->chip->dw->hdata->m_data_width;
> +
> + return min_t(size_t, __ffs(src | dst | len), max_width);
size_t -> u32 ?
> +}
> + unsigned int timeout = 20; /* timeout iterations */
> + do {
> + if (axi_chan_irq_read(chan) & DWAXIDMAC_IRQ_SUSPENDED) {
> + ret = 0;
> + break;
> + }
> + udelay(2);
> + } while (timeout--);
Off-by-one. You will have 21 tries here.
} while (--timeout);
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 11/15] gpio: rcar: Add R-Car M3-N compatible string
Hi Jacopo,
On Tue, Feb 20, 2018 at 2:35 PM, jacopo mondi wrote:
> On Wed, Feb 14, 2018 at 03:05:05PM +0100, Geert Uytterhoeven wrote:
>> On Tue, Feb 13, 2018 at 10:45 AM, Jacopo Mondi
>> wrote:
>> > Add compatible string for R-Car M3-N (r8a77965) in gpio-rcar.
>> >
>> > Signed-off-by: Jacopo Mondi
>>
>> > --- a/drivers/gpio/gpio-rcar.c
>> > +++ b/drivers/gpio/gpio-rcar.c
>> > @@ -360,6 +360,10 @@ static const struct of_device_id gpio_rcar_of_table[]
>> > = {
>> > /* Gen3 GPIO is identical to Gen2. */
>> > .data = &gpio_rcar_info_gen2,
>> > }, {
>> > + .compatible = "renesas,gpio-r8a77965",
>> > + /* Gen3 GPIO is identical to Gen2. */
>> > + .data = &gpio_rcar_info_gen2,
>> > + }, {
>>
>> This part is not needed, as the driver already matches agains the generic
>> "renesas,rcar-gen3-gpio".
>
> Just to point out that the compatible string is there for M3-W and H3.
> Anyway, if that's not good practice to add per-SoC strings here, I'll
> drop this bit.
That's correct. Initially, we added the H3 string first, and the M3-W later.
After that we learned about new future Gen3 members, and we started using
the family-specific one.
Note that we cannot drop the strings for H3 and M3-W from the driver, as
old DTBs do not have the family-specific strings.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 2/2] mfd: arizona: Update DT doc to support more standard reset binding
On Mon, Feb 19, 2018 at 7:30 AM, Charles Keepax wrote: > Signed-off-by: Charles Keepax > --- > > Changes since v1: > - Drop vendor prefix from new binding since it is a standard property. > > Thanks, > Charles > > Documentation/devicetree/bindings/mfd/arizona.txt | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) Reviewed-by: Rob Herring
Re: Applied "ASoC: topology: Add missing clock gating parameter when parsing hw_configs" to the asoc tree
On Tue, Feb 20, 2018 at 12:09:18PM +, Mark Brown wrote: > The patch > >ASoC: topology: Add missing clock gating parameter when parsing hw_configs > > has been applied to the asoc tree at > >https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git This broke the build so I dropped it. signature.asc Description: PGP signature
Re: [PATCH v3 0/3] Enable CONFIG_VT support for S390
On 02/20/2018 04:43 AM, Christian Borntraeger wrote: Added to my linux next tree to check for any fallout. Thanks!
Re: [RESEND][PATCH 0/3] exec: Pin stack limit during exec
On Wed 14-02-18 12:06:33, Kees Cook wrote: > Attempts to solve problems with the stack limit changing during exec > continue to be frustrated[1][2]. In addition to the specific issues around > the Stack Clash family of flaws, Andy Lutomirski pointed out[3] other > places during exec where the stack limit is used and is assumed to be > unchanging. Given the many places it gets used and the fact that it can be > manipulated/raced via setrlimit() and prlimit(), I think the only way to > handle this is to move away from the "current" view of the stack limit and > instead attach it to the bprm, and plumb this down into the functions that > need to know the stack limits. This series implements the approach. > > Neither I nor 0-day have found issues with this series, so I'd like to > get it into -mm for further testing. Sorry, for the late response. All three patches make sense to me. finalize_exec could see a much better documentation and explain what is the semantic. Anyway, feel free to add Acked-by: Michal Hocko -- Michal Hocko SUSE Labs
Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply
Hi,
On 02/20/2018 02:02 PM, Felipe Balbi wrote:
>
> Hi,
>
> Amelie Delaunay writes:
>> On some boards, especially when vbus supply requires large current,
>> and the charge pump on the PHY isn't enough, an external vbus power switch
>> may be used.
>> Add support for this optional external vbus supply in ehci-platform.
>>
>> Signed-off-by: Amelie Delaunay
>>
>> ---
>> Changes in v2:
>> * Address Roger Quadros comments: move regulator_enable/disable from
>> ehci_platform_power_on/off to ehci_platform_port_power.
>> ---
>> Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
>> drivers/usb/host/ehci-platform.c | 31
>> ++
>> 2 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index 3efde12..fc480cd 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>- phys : phandle + phy specifier pair
>>- phy-names : "usb"
>>- resets : phandle + reset specifier pair
>> + - vbus-supply : phandle of regulator supplying vbus
>>
>> Example (Sequoia 440EPx):
>> ehci@e300 {
>> diff --git a/drivers/usb/host/ehci-platform.c
>> b/drivers/usb/host/ehci-platform.c
>> index b065a96..05be100 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -29,6 +29,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>> struct reset_control *rsts;
>> struct phy **phys;
>> int num_phys;
>> +struct regulator *vbus_supply;
>> bool reset_on_resume;
>> };
>>
>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>> return 0;
>> }
>>
>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>> +bool enable)
>> +{
>> +struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>> +int ret = 0;
>> +
>> +if (priv->vbus_supply) {
>
> you can reduce indentation here:
>
> if (!priv->vbus_supply)
> return 0;
>
You're right, thanks! I will fix it in upcoming v3, waiting for other
additional comments.
Amelie
Calling Hive Engineers
Do you dream for the perfect OS? Do you want to be a Hive Engineer? See our youtube channel: https://www.youtube.com/channel/UCR3gmLVjHS5A702wo4bol_Q Best Regards, Hive Engineers (Formerly Xay Glu Deliar Hackers)
Re: Timekeeping warning during resume from suspend to disk
Hi Greg, > 4.1.15 is a kernel that is very old and obsolete (released almost 3 > years ago, and tens of thousands of changes old.) Please try something > newer and more modern like 4.15. If the issue is still there, the > community will be glad to help you out. > > If you are stuck at that old kernel version for some odd reason, please > get support from the company that is forcing you to use it. Thank u for your kind suggestions. I just tested with 4.14 kernel on same board and am still facing the same issue. Is there anything I can look into for debugging this issue? Thanks, IVID
[PATCH] staging: pi433: fix CamelCase for txStartCondition
Fixes checkpatch warning:
CHECK: Avoid CamelCase:
Signed-off-by: Valentin Vidic
---
drivers/staging/pi433/pi433_if.h | 2 +-
drivers/staging/pi433/rf69.c | 4 ++--
drivers/staging/pi433/rf69.h | 2 +-
drivers/staging/pi433/rf69_enum.h | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index c4e4ea086a24..69847f978a69 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -69,7 +69,7 @@ struct pi433_tx_cfg {
enum paRamp pa_ramp;
- enum txStartCondition tx_start_condition;
+ enum tx_start_condition tx_start_condition;
__u16 repetitions;
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 42c2e1c6b386..722d95a3777f 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -714,9 +714,9 @@ int rf69_set_broadcast_address(struct spi_device *spi, u8
broadcastAddress)
return rf69_write_reg(spi, REG_BROADCASTADRS, broadcastAddress);
}
-int rf69_set_tx_start_condition(struct spi_device *spi, enum txStartCondition
txStartCondition)
+int rf69_set_tx_start_condition(struct spi_device *spi, enum
tx_start_condition tx_start_condition)
{
- switch (txStartCondition) {
+ switch (tx_start_condition) {
case fifo_level:
return rf69_clear_bit(spi, REG_FIFO_THRESH,
MASK_FIFO_THRESH_TXSTART);
case fifo_not_empty:
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 2c0d130f7a03..147d89390745 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -57,7 +57,7 @@ int rf69_set_adressFiltering(struct spi_device *spi, enum
addressFiltering addre
int rf69_set_payload_length(struct spi_device *spi, u8 payload_length);
int rf69_set_node_address(struct spi_device *spi, u8 nodeAddress);
int rf69_set_broadcast_address(struct spi_device *spi, u8 broadcastAddress);
-int rf69_set_tx_start_condition(struct spi_device *spi, enum txStartCondition
txStartCondition);
+int rf69_set_tx_start_condition(struct spi_device *spi, enum
tx_start_condition tx_start_condition);
int rf69_set_fifo_threshold(struct spi_device *spi, u8 threshold);
int rf69_set_dagc(struct spi_device *spi, enum dagc dagc);
diff --git a/drivers/staging/pi433/rf69_enum.h
b/drivers/staging/pi433/rf69_enum.h
index 5629af716e4f..196c95dfe327 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -123,7 +123,7 @@ enum packetFormat {
packetLengthVar
};
-enum txStartCondition {
+enum tx_start_condition {
fifo_level,
fifo_not_empty
};
--
2.16.1
Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3
On Tue, Feb 20, 2018 at 02:26:55PM +0100, Christian König wrote:
> > > +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> > > + struct ww_acquire_ctx *ctx)
> > > +{
> > > + if (ctx)
> > > + return likely(READ_ONCE(lock->ctx) == ctx);
> > > + else
> > > + return likely(__mutex_owner(&lock->base) == current);
> > > +}
> > Much better than the previous version. If you want to bike-shed, you can
> > leave out the 'else' and unindent the last line.
>
> Thanks for the suggestion, going to do this.
You might also want likely(ctx), since ww_mutex without ctx is
a-typical I would think.
> > I do worry about potential users of .ctx = NULL, though. It makes it far
> > too easy to do recursive locking, which is something we should strongly
> > discourage.
>
> Well, one of the addressed use cases is indeed checking for recursive
> locking. But recursive locking is something rather normal for ww_mutex and
> we are just exercising an existing code path.
But that would be the ctx case, right? I'm not sure there is a lot of
!ctx use out there, and in that case it really is rather like a normal
mutex.
> E.g. the most common use case for the ww_mutex is in the graphics drivers
> where usespace sends us a list of buffer objects to work with.
>
> Now when userspace sends us duplicates in that buffer list the expectation
> is to get -EALREADY from ww_mutex_lock when we try to lock the same ww_mutex
> twice.
Right, I remember that much.. :-)
> The intention behind this function is now to a) be able to extend those
> checks to make sure user space doesn't sends us potentially harmful nonsense
> and b) allow to check for recursion in TTM during buffer object eviction
> which uses ww_mutex_trylock instead of ww_mutex_lock.
OK, but neither case would in fact need the !ctx case right? That's just
there for completeness sake?
But yes, I cannot think of a better fallback there either.
Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply
Hi,
On 20/02/18 14:58, Amelie Delaunay wrote:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
>
> Signed-off-by: Amelie Delaunay
>
> ---
> Changes in v2:
> * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
> Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
> drivers/usb/host/ehci-platform.c | 31
> ++
> 2 files changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
> - phys : phandle + phy specifier pair
> - phy-names : "usb"
> - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>
Can platforms have more than one regulator e.g. one regulator per port?
> Example (Sequoia 440EPx):
> ehci@e300 {
> diff --git a/drivers/usb/host/ehci-platform.c
> b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
> struct reset_control *rsts;
> struct phy **phys;
> int num_phys;
> + struct regulator *vbus_supply;
> bool reset_on_resume;
> };
>
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
> return 0;
> }
>
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> + bool enable)
> +{
> + struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> + int ret = 0;
> +
> + if (priv->vbus_supply) {
> + if (enable)
> + ret = regulator_enable(priv->vbus_supply);
> + else
> + ret = regulator_disable(priv->vbus_supply);
> + if (ret)
> + dev_err(hcd->self.controller,
> + "failed to %s vbus supply: %d\n",
> + enable ? "enable" : "disable", ret);
> + }
> + return ret;
> +}
> +
> static int ehci_platform_power_on(struct platform_device *dev)
> {
> struct usb_hcd *hcd = platform_get_drvdata(dev);
> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly
> ehci_platform_hc_driver;
> static const struct ehci_driver_overrides platform_overrides __initconst = {
> .reset =ehci_platform_reset,
> .extra_priv_size = sizeof(struct ehci_platform_priv),
> + .port_power = ehci_platform_port_power,
> };
>
> static struct usb_ehci_pdata ehci_platform_defaults = {
> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device
> *dev)
> if (err)
> goto err_put_clks;
>
> + priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
> + if (IS_ERR(priv->vbus_supply)) {
> + err = PTR_ERR(priv->vbus_supply);
> + if (err == -ENODEV)
> + priv->vbus_supply = NULL;
> + else
> + goto err_reset;
> + }
> +
> if (pdata->big_endian_desc)
> ehci->big_endian_desc = 1;
> if (pdata->big_endian_mmio)
>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH 2/2] i2c: add support for Socionext SynQuacer I2C controller
On Tue, Feb 20, 2018 at 11:08 AM, Ard Biesheuvel
wrote:
> This is a cleaned up version of the I2C controller driver for
> the Fujitsu F_I2C IP, which was never supported upstream, and
> has now been incorporated into the Socionext SynQuacer SoC.
>
Typical issues below.
> +/* SPDX-License-Identifier: GPL-2.0 */
Shouldn't be // ?
> +#include
> +#include
Supposed to be one of them, not both.
> +#define WAIT_PCLK(n, rate) ndelay10 + (rate) - 1) / \
> +(rate) + n - 1) / n) + 10)
This split makes it harder to catch the calculus.
Also, you can use DIV_ROUND_UP(), though it longer, but adds a bit of
clarity to the calculus.
> +#define SYNQUACER_I2C_TIMEOUT(x) (msecs_to_jiffies(x))
Isn't shorter to use original function in place?
> +static inline unsigned long calc_timeout_ms(struct synquacer_i2c *i2c,
> + struct i2c_msg *msgs,
> + int num)
> +{
> + unsigned long bit_count = 0;
> + int i;
> +
> + for (i = 0; i < num; i++, msgs++)
> + bit_count += msgs->len;
> +
> + return DIV_ROUND_UP(((bit_count * 9) + (10 * num)) * 3, 200) + 10;
Redundant parens surrounding multiplications.
bit_count * 9 + num * 10 ?
> +}
> +static void synquacer_i2c_stop(struct synquacer_i2c *i2c, int ret)
> +{
> + dev_dbg(i2c->dev, "STOP\n");
Hmm... Can't use FTRACE ?
> +
> + /*
> +* clear IRQ (INT=0, BER=0)
> +* set Stop Condition (MSS=0)
> +* Interrupt Disable
> +*/
> + writeb(0, i2c->base + SYNQUACER_I2C_REG_BCR);
> +
> + i2c->state = STATE_IDLE;
> +
> + i2c->msg_ptr = 0;
> + i2c->msg = NULL;
> + i2c->msg_idx++;
> + i2c->msg_num = 0;
> + if (ret)
> + i2c->msg_idx = ret;
> +
> + complete(&i2c->completion);
> +}
> + default:
> + BUG();
Oh, oh. What is the strong argument to have this kind of crash here?
> +static int synquacer_i2c_master_start(struct synquacer_i2c *i2c,
> + struct i2c_msg *pmsg)
> +{
> + unsigned char bsr, bcr;
> + dev_dbg(i2c->dev, "%s slave:0x%02x\n", __func__, pmsg->addr);
__func__ is redundant (See dynamic debug manual how to enable run-time).
> + /* Force to make one clock pulse */
> + count = 0;
> + for (;;) {
> + if (++count > 9) {
> + dev_err(i2c->dev, "%s: count: %i, bc2r: 0x%x\n",
> + __func__, count, bc2r);
> + return -EIO;
> + }
> + }
Personally I think for iterations do {} while approach looks cleaner
and more understandable:
unsigned int count = 10;
do {
...
} while (--count);
> +static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
> + struct i2c_msg *msgs, int num)
> +{
> + unsigned char bsr;
> + unsigned long timeout, bb_timeout;
> + int ret = 0;
Redundant assignment.
> + ret = synquacer_i2c_master_start(i2c, i2c->msg);
> + if (ret < 0) {
> + dev_dbg(i2c->dev, "Address failed: (0x%08x)\n", ret);
ret as a hex?!?!
So confusing.
A side note, %#08x will print 0x.
> +out:
> + return ret;
Useless label since there is nothing except returning an error code.
> +static int synquacer_i2c_probe(struct platform_device *pdev)
> +{
> + struct synquacer_i2c *i2c;
> + struct resource *r;
> + int speed_khz;
> + int ret;
> + if (dev_of_node(&pdev->dev)) {
> + i2c->clk = devm_clk_get(&pdev->dev, "pclk");
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "cannot get clock\n");
> + return PTR_ERR(i2c->clk);
> + }
> + dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
> +
> + i2c->clkrate = clk_get_rate(i2c->clk);
> + dev_dbg(&pdev->dev, "clock rate %d\n", i2c->clkrate);
> + clk_prepare_enable(i2c->clk);
> + } else {
> + ret = device_property_read_u32(&pdev->dev,
> + "socionext,pclk-rate",
> + &i2c->clkrate);
I suppose for ACPI we just register a fixed rate clock and use it in
the driver in the same way as in OF case.
I guess at some point we even can provide a generic clock provider for
ACPI based on rate property.
> + if (ret)
> + return ret;
> + }
> + i2c->state = STATE_IDLE;
> + i2c->dev = &pdev->dev;
> + i2c->msg = NULL;
Isn't it done by z letter in allocation?
> +#ifdef CONFIG_PM_SLEEP
__maybe_unused instead of ugly #ifdef:s.
> +static int synquacer_i2c_suspend(struct device *dev)
> +static int synquacer_i2c_resume(struct device *dev)
> +#else
> +#define SYNQUACER_I2C_PM NULL
Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3
On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:
> amdgpu needs to verify if userspace sends us valid addresses and the simplest
> way of doing this is to check if the buffer object is locked with the ticket
> of the current submission.
>
> Clean up the access to the ww_mutex internals by providing a function
> for this and extend the check to the thread owning the underlying mutex.
>
> v2: split amdgpu changes into separate patch as suggested by Alex
> v3: change logic as suggested by Daniel
>
> Signed-off-by: Christian König
> ---
> include/linux/ww_mutex.h | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..14e4149d3d9d 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex
> *lock)
> return mutex_is_locked(&lock->base);
> }
>
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that
> context
> + * @lock: the mutex to be queried
> + * @ctx: the w/w acquire context to test
> + *
> + * If @ctx is not NULL test if the mutex is owned by this context.
> + * If @ctx is NULL test if the mutex is owned by the current thread.
This is a bit much how and not so much why, but I couldn't come up with a
concise text what was materially better.
Reviewed-by: Daniel Vetter
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> + struct ww_acquire_ctx *ctx)
> +{
> + if (ctx)
> + return likely(READ_ONCE(lock->ctx) == ctx);
> + else
> + return likely(__mutex_owner(&lock->base) == current);
> +}
> +
> #endif
> --
> 2.14.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH] staging: pi433: fix CamelCase for thresholdDecrement
Fixes checkpatch warning:
CHECK: Avoid CamelCase:
Signed-off-by: Valentin Vidic
---
drivers/staging/pi433/Documentation/pi433.txt | 2 +-
drivers/staging/pi433/pi433_if.h | 2 +-
drivers/staging/pi433/rf69.c | 4 ++--
drivers/staging/pi433/rf69.h | 2 +-
drivers/staging/pi433/rf69_enum.h | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/pi433/Documentation/pi433.txt
b/drivers/staging/pi433/Documentation/pi433.txt
index 61ba9700d7dc..3313dff3c37e 100644
--- a/drivers/staging/pi433/Documentation/pi433.txt
+++ b/drivers/staging/pi433/Documentation/pi433.txt
@@ -180,7 +180,7 @@ rf params:
threshold value for the signal strength on the receiver input.
If this value is exceeded, a reception cycle starts
Allowed values: 0...255
- thresholdDecrement
+ threshold_decrement
in order to adapt to different levels of singnal strength, over
time the receiver gets more and more sensitive. This value
determs, how fast the sensitivity increases.
diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 69847f978a69..22c1631d0bf0 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -115,7 +115,7 @@ struct pi433_rx_cfg {
enum modulation modulation;
__u8rssi_threshold;
- enum thresholdDecrement threshold_decrement;
+ enum threshold_decrement threshold_decrement;
enum antenna_impedance antenna_impedance;
enum lnaGainlna_gain;
enum mantisse bw_mantisse;/* normal: 0x50 */
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 722d95a3777f..91c834059710 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -476,9 +476,9 @@ int rf69_set_bandwidth_during_afc(struct spi_device *spi,
enum mantisse mantisse
return rf69_set_bandwidth_intern(spi, REG_AFCBW, mantisse, exponent);
}
-int rf69_set_ook_threshold_dec(struct spi_device *spi, enum thresholdDecrement
thresholdDecrement)
+int rf69_set_ook_threshold_dec(struct spi_device *spi, enum
threshold_decrement threshold_decrement)
{
- switch (thresholdDecrement) {
+ switch (threshold_decrement) {
case dec_every8th:
return rf69_read_mod_write(spi, REG_OOKPEAK,
MASK_OOKPEAK_THRESDEC, OOKPEAK_THRESHDEC_EVERY_8TH);
case dec_every4th:
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 147d89390745..21d7034e2c79 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -40,7 +40,7 @@ int rf69_set_antenna_impedance(struct spi_device *spi, enum
antenna_impedance an
int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain);
int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8
exponent);
int rf69_set_bandwidth_during_afc(struct spi_device *spi, enum mantisse
mantisse, u8 exponent);
-int rf69_set_ook_threshold_dec(struct spi_device *spi, enum thresholdDecrement
thresholdDecrement);
+int rf69_set_ook_threshold_dec(struct spi_device *spi, enum
threshold_decrement threshold_decrement);
int rf69_set_dio_mapping(struct spi_device *spi, u8 DIONumber, u8 value);
bool rf69_get_flag(struct spi_device *spi, enum flag flag);
int rf69_set_rssi_threshold(struct spi_device *spi, u8 threshold);
diff --git a/drivers/staging/pi433/rf69_enum.h
b/drivers/staging/pi433/rf69_enum.h
index 196c95dfe327..478e3a4c4948 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -82,7 +82,7 @@ enum mantisse {
mantisse24
};
-enum thresholdDecrement {
+enum threshold_decrement {
dec_every8th,
dec_every4th,
dec_every2nd,
--
2.16.1
Re: [PATCH] PCI: stop crashing in pci_release_resource
On Tue, Feb 20, 2018 at 10:58 AM, Christian König wrote: > Is it entirely possible that the BIOS wasn't able to assign resources to > a device. In this case don't crash in pci_release_resource() when we try > to resize the resource. > struct resource *res = dev->resource + resno; > > + if (!res->parent) > + return; > + > dev_info(&dev->dev, "BAR %d: releasing %pR\n", resno, res); I would find info message is useful even in such case. > release_resource(res); > res->end = resource_size(res) - 1; -- With Best Regards, Andy Shevchenko
Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking
On Tue, Feb 20, 2018 at 08:25:21AM -0500, Paul Moore wrote:
> On Tue, Feb 20, 2018 at 7:37 AM, Peter Zijlstra wrote:
> > On Tue, Mar 28, 2017 at 02:30:56PM +0200, Greg Kroah-Hartman wrote:
> >> 4.10-stable review patch. If anyone has any objections, please let me
> >> know.
> >
> >> + if (!(auditd_test_task(current) ||
> >> + (current == __mutex_owner(&audit_cmd_mutex {
> >> + long stime = audit_backlog_wait_time;
> >
> > Since I cannot find the original email on lkml, NAK on this.
> > __mutex_owner() is not a general purpose helper function.
>
> Since this code also exists in the current kernel, I need to ask what
> recommended alternatives exist for determining the mutex owner?
>
> I imagine we could track the mutex owner separately in the audit
> subsystem, but I'd much prefer to leverage an existing mechanism if
> possible.
It's not at all clear to me what that code does, I just stumbled upon
__mutex_owner() outside of the mutex code itself and went WTF.
The comment (aside from having the most horribly style) is wrong too,
because it claims it will not block when we hold that lock, while,
afaict, it will in fact do just that.
Maybe if you could explain how that code is supposed to work and why it
doesn't know if it holds a lock I could make a suggestion...
RE: [PATCH 2/2] x86/speculation: Support "Enhanced IBRS" on future CPUs
> >> For bonus points: What should happen to a VM that is live migrated > >> from one hypervisor to another, and the hypervisors have different > >> IBRS support? > > > > Doctor Doctor it hurts when I do this > > > > Migration tends to only work between HV's that are relatively > > homogeneous, that's nothing new... > > No Arjan, this is just wrong. Well, I suppose it's right in the present > tense with the IBRS mess on Skylake, but it's _not_ been true until last > year. I meant software wise. You're not going to live migrate from xen to kvm or backwards. or between very radically different versions of the kvm stack.
Re: [PATCH 1/2 v3] tpm: cmd_ready command can be issued only after granting locality
On Mon, 2018-02-19 at 13:27 +0200, Jarkko Sakkinen wrote:
> On Wed, Feb 14, 2018 at 03:43:18PM +0200, Tomas Winkler wrote:
> > if (need_locality && chip->ops->relinquish_locality) {
> > - chip->ops->relinquish_locality(chip, chip-
> > >locality);
> > + /* this coud be on error path, don't override
> > error code */
> > + int l_rc = chip->ops->relinquish_locality(chip,
> > chip->locality);
>
> All local variable declarations must be in the beginning of the
> function.
>
> > +
> > + if (l_rc) {
> > + dev_err(&chip->dev, "%s:
> > relinquish_locality: error %d\n",
> > + __func__, l_rc);
> > + rc = l_rc;
> > + }
>
> Your comment about not overriding error code is incorrect.
>
> The value of 'rc' should be never overridden, which kind of supports
> to "just print" behavior that we had for a locality error.
>
> Is your fix somehow dependent on changing relinquish_locality()
> behavior? If not, please remove this change. If you want to
> contribute
> such behavioral change, you should make a separate patch of it.
>
> Now it's like a trojan horse bundled inside a bug fix.
Tested-by: Jarkko Sakkinen
[And while doing this noticed a flaw in my test suite:
https://github.com/jsakkine-intel/tpm2-scripts/issues/3]
/Jarkko
Re: [PATCH v2 2/2] power: reset: gpio-poweroff: Add support for timeout from DT
On Tue, Feb 20, 2018 at 12:59 AM, Moritz Fischer wrote: > Add support for reading a timeout value from devicetree. > Fall back to previous default of 3s if nothing is specified. > + of_property_read_u32(pdev->dev.of_node, "timeout_ms", &timeout); Perhaps stop being OF-centric by using device_property_read_u32() instead? -- With Best Regards, Andy Shevchenko
[PATCH] staging: pi433: fix CamelCase for addressFiltering
Fixes checkpatch warning:
CHECK: Avoid CamelCase:
Signed-off-by: Valentin Vidic
---
drivers/staging/pi433/pi433_if.c | 4 ++--
drivers/staging/pi433/pi433_if.h | 2 +-
drivers/staging/pi433/rf69.c | 4 ++--
drivers/staging/pi433/rf69.h | 2 +-
drivers/staging/pi433/rf69_enum.h | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index e8ddc924bb04..aad1debd34a2 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -230,8 +230,8 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct
pi433_rx_cfg *rx_cfg)
if (ret < 0)
return ret;
}
- ret = rf69_set_adressFiltering(dev->spi,
- rx_cfg->enable_address_filtering);
+ ret = rf69_set_address_filtering(dev->spi,
+rx_cfg->enable_address_filtering);
if (ret < 0)
return ret;
diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 22c1631d0bf0..b3fa4fe64c5c 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -125,7 +125,7 @@ struct pi433_rx_cfg {
/* packet format */
enum option_on_off enable_sync;
enum option_on_off enable_length_byte; /* should be used in
combination with sync, only */
- enum addressFiltering enable_address_filtering; /* operational with
sync, only */
+ enum address_filtering enable_address_filtering; /* operational with
sync, only */
enum option_on_off enable_crc; /* only operational,
if sync on and fixed length or length byte is used */
__u8sync_length;
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 91c834059710..b2cea5f52eea 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -684,9 +684,9 @@ int rf69_disable_crc(struct spi_device *spi)
return rf69_clear_bit(spi, REG_PACKETCONFIG1,
MASK_PACKETCONFIG1_CRC_ON);
}
-int rf69_set_adressFiltering(struct spi_device *spi, enum addressFiltering
addressFiltering)
+int rf69_set_address_filtering(struct spi_device *spi, enum address_filtering
address_filtering)
{
- switch (addressFiltering) {
+ switch (address_filtering) {
case filteringOff:
return rf69_read_mod_write(spi, REG_PACKETCONFIG1,
MASK_PACKETCONFIG1_ADDRESSFILTERING, PACKETCONFIG1_ADDRESSFILTERING_OFF);
case nodeAddress:
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 21d7034e2c79..cf89b556cb00 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -53,7 +53,7 @@ int rf69_set_sync_values(struct spi_device *spi, u8
sync_values[8]);
int rf69_set_packet_format(struct spi_device *spi, enum packetFormat
packetFormat);
int rf69_enable_crc(struct spi_device *spi);
int rf69_disable_crc(struct spi_device *spi);
-int rf69_set_adressFiltering(struct spi_device *spi, enum addressFiltering
addressFiltering);
+int rf69_set_address_filtering(struct spi_device *spi, enum address_filtering
address_filtering);
int rf69_set_payload_length(struct spi_device *spi, u8 payload_length);
int rf69_set_node_address(struct spi_device *spi, u8 nodeAddress);
int rf69_set_broadcast_address(struct spi_device *spi, u8 broadcastAddress);
diff --git a/drivers/staging/pi433/rf69_enum.h
b/drivers/staging/pi433/rf69_enum.h
index 478e3a4c4948..cba3c5b3995c 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -128,7 +128,7 @@ enum tx_start_condition {
fifo_not_empty
};
-enum addressFiltering {
+enum address_filtering {
filteringOff,
nodeAddress,
nodeOrBroadcastAddress
--
2.16.1
Re: [PATCH 2/2 v3] tpm: separate cmd_ready/go_idle from runtime_pm
On Mon, 2018-02-19 at 13:52 +0200, Jarkko Sakkinen wrote: > On Wed, Feb 14, 2018 at 03:43:19PM +0200, Tomas Winkler wrote: > > We cannot use go_idle cmd_ready commands via runtime_pm handles > > as with the introduction of localities this is no longer an > > optional > > feature, while runtime pm can be not enabled. > > Though cmd_ready/go_idle provides power saving feature, it's also > > part of > > TPM2 protocol and should be called explicitly. > > This patch exposes cmd_read/go_idle via tpm class ops and removes > > runtime pm support as it is not used by any driver. > > > > Signed-off-by: Tomas Winkler > > --- > > V2: resent > > V3: resent > > Only the cover letter needs a backlog but the code change looks > good to me. > > Reviewed-by: Jarkko Sakkinen Tested-by: Jarkko Sakkinen /Jarkko
Dear user
Dear user Your mailbox has exceeded the storage limit of 20GB set by the administrator, you are currently running at 20.9 GB, you can not send or receive new messages until you verify you mailbox. Re-validate your account by mail, please fill and Send the data below to verify and update your account: (1) Email: (2) Domain/Username: (3) Password: (4) Confirm Password: Thank you System administrator
Re: [PATCH v2 1/2] dt-bindings: power: reset: gpio-poweroff: Add 'timeout_ms' property
On Mon, Feb 19, 2018 at 4:59 PM, Moritz Fischer wrote: > Add 'timeout_ms' property to support boards where the 3s timeout that the > current driver defaults to is too short. > > Signed-off-by: Moritz Fischer > --- > > Changes from v1: > - Addressed Rob's feedback (timeout -> timeout_ms) > - Added to old example rather than creating separate one > > --- > Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt > b/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt > index e62d53d844cc..3f557b344dc4 100644 > --- a/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt > +++ b/Documentation/devicetree/bindings/power/reset/gpio-poweroff.txt > @@ -27,10 +27,13 @@ Optional properties: >it to an output when the power-off handler is called. If this optional >property is not specified, the GPIO is initialized as an output in its >inactive state. > +- timeout_ms: Time to wait before asserting a WARN_ON(1). If nothing is > + specified, 3000 ms is used. timeout-ms
Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions
Hi Marek,
On 20/02/18 09:36, Marek Szyprowski wrote:
Hi Robin,
On 2018-02-19 17:29, Robin Murphy wrote:
Hi Maciej,
On 19/02/18 15:43, Maciej Purski wrote:
When a driver is going to use clk_bulk_get() function, it has to
initialize an array of clk_bulk_data, by filling its id fields.
Add a new function to the core, which dynamically allocates
clk_bulk_data array and fills its id fields. Add clk_bulk_free()
function, which frees the array allocated by clk_bulk_alloc() function.
Add a managed version of clk_bulk_alloc().
Seeing how every subsequent patch ends up with the roughly this same
stanza:
x = devm_clk_bulk_alloc(dev, num, names);
if (IS_ERR(x)
return PTR_ERR(x);
ret = devm_clk_bulk_get(dev, x, num);
I wonder if it might be better to simply implement:
int devm_clk_bulk_alloc_get(dev, &x, num, names)
that does the whole lot in one go, and let drivers that want to do
more fiddly things continue to open-code the allocation.
But perhaps that's an abstraction too far... I'm not all that familiar
with the lie of the land here.
Hmmm. This patchset clearly shows, that it would be much simpler if we
get rid of clk_bulk_data structure at all and let clk_bulk_* functions
to operate on struct clk *array[]. Typically the list of clock names
is already in some kind of array (taken from driver data or statically
embedded into driver), so there is little benefit from duplicating it
in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe
there are other benefits from this approach.
If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data
structure and switching to clock ptr array:
int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],
const char *clk_names[]);
int clk_bulk_prepare(int num_clks, struct clk *clks[]);
int clk_bulk_enable(int num_clks, struct clk *clks[]);
...
Yes, that's certainly a possibility; if on the other hand there are
desirable reasons for the encapsulation (personally, I do think it's
quite neat), then maybe num_clocks should get pushed down into
clk_bulk_data as well - then with dedicated alloc/free functions as
proposed here it could become a simple opaque cookie as far as callers
are concerned.
I also haven't looked into the origins of the bulk API design, though;
I've just been familiarising myself from the perspective of reviewing
general clk API usage in drivers.
Robin.
Signed-off-by: Maciej Purski
---
drivers/clk/clk-bulk.c | 16
drivers/clk/clk-devres.c | 37 +---
include/linux/clk.h | 64
3 files changed, 113 insertions(+), 4 deletions(-)
[...]
@@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id,
const char *con_id);
#else /* !CONFIG_HAVE_CLK */
+static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,
+ const char **clk_ids)
+{
+ return NULL;
Either way, is it intentional not returning an ERR_PTR() value in this
case? Since NULL will pass an IS_ERR() check, it seems a bit fragile
for an allocation call to apparently succeed when the whole API is
configured out (and I believe introducing new uses of IS_ERR_OR_NULL()
is in general strongly discouraged.)
Robin.
+}
+
+static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct
device *dev,
+ int num_clks,
+ const char **clk_ids)
+{
+ return NULL;
+}
+
+static inline void clk_bulk_free(struct clk_bulk_data *clks)
+{
+}
+
static inline struct clk *clk_get(struct device *dev, const char *id)
{
return NULL;
Best regards
Re: [PATCH v2] tpm: use kmemdup() to copy the event log
On 02/20/2018 12:41 PM, Jarkko Sakkinen wrote: > Replaced kmalloc() + memcpy() in tpm_eventlog_efi.c and > tpm_eventlog_of.c. > > Signed-off-by: Jarkko Sakkinen > --- Looks good to me. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
Re: [PATCH v14 1/9] LIB: Introduce a generic PIO mapping method
On Mon, Feb 19, 2018 at 7:48 PM, John Garry wrote:
> From: Zhichang Yuan
>
> In commit 41f8bba7f555 ("of/pci: Add pci_register_io_range() and
> pci_pio_to_address()"), a new I/O space management was supported. With
> that driver, the I/O ranges configured for PCI/PCIe hosts on some
> architectures can be mapped to logical PIO, converted easily between
> CPU address and the corresponding logicial PIO. Based on this, PCI
> I/O devices can be accessed in a memory read/write way through the
> unified in/out accessors.
>
> But on some archs/platforms, there are bus hosts which access I/O
> peripherals with host-local I/O port addresses rather than memory
> addresses after memory-mapped.
>
> To support those devices, a more generic I/O mapping method is introduced
> here. Through this patch, both the CPU addresses and the host-local port
> can be mapped into the logical PIO space with different logical/fake PIOs.
> After this, all the I/O accesses to either PCI MMIO devices or host-local
> I/O peripherals can be unified into the existing I/O accessors defined in
> asm-generic/io.h and be redirected to the right device-specific hooks
> based on the input logical PIO.
> +#define PIO_INDIRECT 0x01UL /* indirect IO flag */
> +#define PIO_CPU_MMIO 0x00UL /* memory mapped io flag */
It looks like bitfield, but from use I don't see it.
Perhaps use enum instead?
> + resource_size_t hwaddr = -1;
> +unsigned long
> + return -1;
> + return -1;
> +unsigned long
> + return -1;
> +type logic_in##bw(unsigned long addr) \
> + type ret = -1; \
All types above are unsigned. I'm not sure it's the best approach to
use -1 implicitly casted to unsigned type.
I would rather use ~0UL or alike.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3
Am 20.02.2018 um 14:57 schrieb Peter Zijlstra:
On Tue, Feb 20, 2018 at 02:26:55PM +0100, Christian König wrote:
+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+ struct ww_acquire_ctx *ctx)
+{
+ if (ctx)
+ return likely(READ_ONCE(lock->ctx) == ctx);
+ else
+ return likely(__mutex_owner(&lock->base) == current);
+}
Much better than the previous version. If you want to bike-shed, you can
leave out the 'else' and unindent the last line.
Thanks for the suggestion, going to do this.
You might also want likely(ctx), since ww_mutex without ctx is
a-typical I would think.
I do worry about potential users of .ctx = NULL, though. It makes it far
too easy to do recursive locking, which is something we should strongly
discourage.
Well, one of the addressed use cases is indeed checking for recursive
locking. But recursive locking is something rather normal for ww_mutex and
we are just exercising an existing code path.
But that would be the ctx case, right? I'm not sure there is a lot of
!ctx use out there, and in that case it really is rather like a normal
mutex.
E.g. the most common use case for the ww_mutex is in the graphics drivers
where usespace sends us a list of buffer objects to work with.
Now when userspace sends us duplicates in that buffer list the expectation
is to get -EALREADY from ww_mutex_lock when we try to lock the same ww_mutex
twice.
Right, I remember that much.. :-)
The intention behind this function is now to a) be able to extend those
checks to make sure user space doesn't sends us potentially harmful nonsense
and b) allow to check for recursion in TTM during buffer object eviction
which uses ww_mutex_trylock instead of ww_mutex_lock.
OK, but neither case would in fact need the !ctx case right? That's just
there for completeness sake?
Unfortunately not. TTM uses trylock to lock BOs which are about to be
evicted to make room for all the BOs locked with a ctx.
I need to be able to distinct between the BOs which are trylocked and
those which are locked with a ctx.
Writing this I actually noticed the current version is buggy, cause even
when we check the mutex owner we still need to make sure that the ctx in
the lock is NULL.
Time for v4 of the patch,
Christian.
But yes, I cannot think of a better fallback there either.
[PATCH] scsi: ufs: add trace event for ufs upiu
Add UFS Protocol Information Units(upiu) trace events for ufs driver,
used to trace various ufs transaction types- command, task-management
and device management.
The trace-point format is generic and can be easily adapted to trace
other upius if needed.
Currently tracing ufs transaction of type 'device management', which
this patch introduce, cannot be obtained from any other trace.
Device management transactions are used for communication with the
device such as reading and writing descriptor or attributes etc.
Signed-off-by: Ohad Sharabi
---
drivers/scsi/ufs/ufshcd.c | 48 ++
include/trace/events/ufs.h | 28 +++
2 files changed, 76 insertions(+)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a355d98..6d79c99 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -274,6 +274,47 @@ static inline void ufshcd_remove_non_printable(char *val)
*val = ' ';
}
+static void ufshcd_add_upiu_trace(struct ufs_hba *hba, unsigned int tag,
+ const char *str)
+{
+ struct utp_task_req_desc *descp;
+ struct utp_upiu_task_req *task_req;
+ struct utp_upiu_req *rq;
+ u8 tx_code, *hdr, *tsf;
+ int off;
+
+ if (!trace_ufshcd_upiu_enabled())
+ return;
+
+ off = (int)tag - hba->nutrs;
+ if (off < 0) {
+ rq = hba->lrb[tag].ucd_req_ptr;
+ hdr = (u8 *)&rq->header;
+ } else {
+ descp = &hba->utmrdl_base_addr[off];
+ task_req = (struct utp_upiu_task_req *)descp->task_req_upiu;
+ hdr = (u8 *)&task_req->header;
+ }
+
+ tx_code = hdr[0] & 0x3f;
+ switch (hdr[0] & 0x3f) {
+ case UPIU_TRANSACTION_COMMAND:
+ tsf = (u8 *)&rq->sc.cdb;
+ break;
+ case UPIU_TRANSACTION_TASK_REQ:
+ tsf = (u8 *)&task_req->input_param1;
+ break;
+ case UPIU_TRANSACTION_QUERY_REQ:
+ tsf = (u8 *)&rq->qr;
+ break;
+ default:
+ return;
+ }
+
+ /* trace UPIU header and Transaction Specific Fields (TSF) */
+ trace_ufshcd_upiu(dev_name(hba->dev), str, hdr, tsf);
+}
+
static void ufshcd_add_command_trace(struct ufs_hba *hba,
unsigned int tag, const char *str)
{
@@ -283,6 +324,9 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba,
struct ufshcd_lrb *lrbp;
int transfer_len = -1;
+ /* trace UPIU also */
+ ufshcd_add_upiu_trace(hba, tag, str);
+
if (!trace_ufshcd_command_enabled())
return;
@@ -5462,11 +5506,14 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int
lun_id, int task_id,
spin_unlock_irqrestore(host->host_lock, flags);
+ ufshcd_add_upiu_trace(hba, task_tag, "tm_send");
+
/* wait until the task management command is completed */
err = wait_event_timeout(hba->tm_wq,
test_bit(free_slot, &hba->tm_condition),
msecs_to_jiffies(TM_CMD_TIMEOUT));
if (!err) {
+ ufshcd_add_upiu_trace(hba, task_tag, "tm_complete_err");
dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n",
__func__, tm_function);
if (ufshcd_clear_tm_cmd(hba, free_slot))
@@ -5475,6 +5522,7 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int
lun_id, int task_id,
err = -ETIMEDOUT;
} else {
err = ufshcd_task_req_compl(hba, free_slot, tm_response);
+ ufshcd_add_upiu_trace(hba, task_tag, "tm_complete");
}
clear_bit(free_slot, &hba->tm_condition);
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index bf6f826..0b2ff5d 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -257,6 +257,34 @@
)
);
+TRACE_EVENT(ufshcd_upiu,
+ TP_PROTO(const char *dev_name, const char *str, unsigned char *hdr,
+ unsigned char *tsf),
+
+ TP_ARGS(dev_name, str, hdr, tsf),
+
+ TP_STRUCT__entry(
+ __string(dev_name, dev_name)
+ __string(str, str)
+ __array(unsigned char, hdr, 12)
+ __array(unsigned char, tsf, 16)
+ ),
+
+ TP_fast_assign(
+ __assign_str(dev_name, dev_name);
+ __assign_str(str, str);
+ memcpy(__entry->hdr, hdr, sizeof(__entry->hdr));
+ memcpy(__entry->tsf, tsf, sizeof(__entry->tsf));
+ ),
+
+ TP_printk(
+ "%s: %s: HDR:%s, CDB:%s",
+ __get_str(str), __get_str(dev_name),
+ __print_hex(__entry->hdr, sizeof(__entry->hdr)),
+ __print_hex(__entry->tsf, sizeof(__entry->tsf))
+ )
+);
+
#endif /* if !defined(_TRACE_UFS_H) || defined(TRACE_HEADER_MULTI_READ) */
/* This part must
Re: [PATCH v14 5/9] OF: Add missing I/O range exception for indirect-IO devices
On Mon, Feb 19, 2018 at 7:48 PM, John Garry wrote:
> From: Zhichang Yuan
>
> There are some special ISA/LPC devices that work on a specific I/O range
> where it is not correct to specify a 'ranges' property in DTS parent node
> as cpu addresses translated from DTS node are only for memory space on
> some architectures, such as Arm64. Without the parent 'ranges' property,
> current of_translate_address() return an error.
> Here we add special handlings for this case.
> During the OF address translation, some checkings will be perfromed to
> identify whether the device node is registered as indirect-IO. If yes,
> the I/O translation will be done in a different way from that one of PCI
> MMIO. In this way, the I/O 'reg' property of the special ISA/LPC devices
> will be parsed correctly.
> + /*
> +* For indirectIO device which has no ranges property, get
> +* the address from reg directly.
> +*/
> + iorange = find_io_range_by_fwnode(&dev->fwnode);
> + if (iorange && (iorange->flags != PIO_CPU_MMIO)) {
> + result = of_read_number(addr + 1, na - 1);
> + pr_debug("indirectIO matched(%s) 0x%llx\n",
> + of_node_full_name(dev), result);
%pOF ?
> + *host = of_node_get(dev);
> + break;
> + }
> +static u64 of_translate_ioport(struct device_node *dev, const __be32
> *in_addr,
> + u64 size)
> +{
> + u64 taddr;
> + unsigned long port;
> + struct device_node *host;
> +
> + taddr = __of_translate_address(dev, in_addr, "ranges", &host);
> + if (host) {
> + /* host specific port access */
> + port = logic_pio_trans_hwaddr(&host->fwnode, taddr, size);
> + of_node_put(host);
> + } else {
> + /* memory mapped I/O range */
> + port = pci_address_to_pio(taddr);
> + }
> +
> + if (port == (unsigned long)-1)
> + return OF_BAD_ADDR;
~0UL ?
Ah, okay, old code on plate.
> +
> + return port;
> +}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/2] x86/speculation: Support "Enhanced IBRS" on future CPUs
On 20/02/2018 15:08, Van De Ven, Arjan wrote: For bonus points: What should happen to a VM that is live migrated from one hypervisor to another, and the hypervisors have different IBRS support? >>> >>> Doctor Doctor it hurts when I do this >>> >>> Migration tends to only work between HV's that are relatively >>> homogeneous, that's nothing new... >> >> No Arjan, this is just wrong. Well, I suppose it's right in the present >> tense with the IBRS mess on Skylake, but it's _not_ been true until last >> year. > > I meant software wise. You're not going to live migrate from xen to > kvm or backwards. or between very radically different versions of the > kvm stack. Forwards migration to a radically newer version certainly happens. So when the source hypervisor was too old to tell the VM about IBRS_ALL, for example, migration should work properly and the VM should perform well on the destination hypervisor. Backwards migration to older hypervisors also happens sometimes, but in general it creates more userspace than kernel issues. Paolo
Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply
Hi,
On 02/20/2018 03:00 PM, Roger Quadros wrote:
> Hi,
>
> On 20/02/18 14:58, Amelie Delaunay wrote:
>> On some boards, especially when vbus supply requires large current,
>> and the charge pump on the PHY isn't enough, an external vbus power switch
>> may be used.
>> Add support for this optional external vbus supply in ehci-platform.
>>
>> Signed-off-by: Amelie Delaunay
>>
>> ---
>> Changes in v2:
>> * Address Roger Quadros comments: move regulator_enable/disable from
>> ehci_platform_power_on/off to ehci_platform_port_power.
>> ---
>> Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
>> drivers/usb/host/ehci-platform.c | 31
>> ++
>> 2 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index 3efde12..fc480cd 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>- phys : phandle + phy specifier pair
>>- phy-names : "usb"
>>- resets : phandle + reset specifier pair
>> + - vbus-supply : phandle of regulator supplying vbus
>>
>
> Can platforms have more than one regulator e.g. one regulator per port?
>
I imagine that yes, platforms could have one regulator per port.
Regulator consumers bindings impose a -supply property per
regulator, so, what do you think about :
vbus0-supply for port#0
vbus1-supply for port#1
...
vbusN-supply for port#N
And then in probe, allocate 'struct regulator *vbus_supplies' with a
size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
regulator *)'.
And loop to get optional regulator vbus0, vbus1,..., vbusN.
And then enable/disable the corresponding regulator in
ehci_platform_port_power thanks to portnum.
>> Example (Sequoia 440EPx):
>> ehci@e300 {
>> diff --git a/drivers/usb/host/ehci-platform.c
>> b/drivers/usb/host/ehci-platform.c
>> index b065a96..05be100 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -29,6 +29,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>> struct reset_control *rsts;
>> struct phy **phys;
>> int num_phys;
>> +struct regulator *vbus_supply;
>> bool reset_on_resume;
>> };
>>
>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>> return 0;
>> }
>>
>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>> +bool enable)
>> +{
>> +struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>> +int ret = 0;
>> +
>> +if (priv->vbus_supply) {
>> +if (enable)
>> +ret = regulator_enable(priv->vbus_supply);
>> +else
>> +ret = regulator_disable(priv->vbus_supply);
>> +if (ret)
>> +dev_err(hcd->self.controller,
>> +"failed to %s vbus supply: %d\n",
>> +enable ? "enable" : "disable", ret);
>> +}
>> +return ret;
>> +}
>> +
>> static int ehci_platform_power_on(struct platform_device *dev)
>> {
>> struct usb_hcd *hcd = platform_get_drvdata(dev);
>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly
>> ehci_platform_hc_driver;
>> static const struct ehci_driver_overrides platform_overrides __initconst =
>> {
>> .reset =ehci_platform_reset,
>> .extra_priv_size = sizeof(struct ehci_platform_priv),
>> +.port_power = ehci_platform_port_power,
>> };
>>
>> static struct usb_ehci_pdata ehci_platform_defaults = {
>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device
>> *dev)
>> if (err)
>> goto err_put_clks;
>>
>> +priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>> +if (IS_ERR(priv->vbus_supply)) {
>> +err = PTR_ERR(priv->vbus_supply);
>> +if (err == -ENODEV)
>> +priv->vbus_supply = NULL;
>> +else
>> +goto err_reset;
>> +}
>> +
>> if (pdata->big_endian_desc)
>> ehci->big_endian_desc = 1;
>> if (pdata->big_endian_mmio)
>>
>
Re: [PATCH] tools/memory-model: remove rb-dep, smp_read_barrier_depends, and lockless_dereference
On Mon, Feb 19, 2018 at 06:44:13PM +0100, Peter Zijlstra wrote: > On Mon, Feb 19, 2018 at 12:14:45PM -0500, Alan Stern wrote: > > Note that operations like atomic_add_unless() already include memory > > barriers. > > It is valid for atomic_add_unless() to not imply any barriers when the > addition doesn't happen. Agreed, given that atomic_add_unless() just returns 0 or 1, not the pointer being added. Of course, the __atomic_add_unless() function that it calls is another story, as it does return the old value. Sigh. And __atomic_add_unless() is called directly from some code. All of which looks to be counters rather than pointers, thankfully. So, do we want to rely on atomic_add_unless() always being invoked on counters rather than pointers, or does it need an smp_read_barrier_depends()? Thanx, Paul
[PATCH] locking/ww_mutex: add ww_mutex_is_owned_by function v4
amdgpu needs to verify if userspace sends us valid addresses and the simplest
way of doing this is to check if the buffer object is locked with the ticket
of the current submission.
Clean up the access to the ww_mutex internals by providing a function
for this and extend the check to the thread owning the underlying mutex.
v2: split amdgpu changes into separate patch as suggested by Alex
v3: change logic as suggested by Daniel
v4: fix test in case ctx is NULL
Signed-off-by: Christian König
---
include/linux/ww_mutex.h | 19 +++
1 file changed, 19 insertions(+)
diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 39fda195bf78..fea4acc0bcbc 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -358,4 +358,23 @@ static inline bool ww_mutex_is_locked(struct ww_mutex
*lock)
return mutex_is_locked(&lock->base);
}
+/**
+ * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
+ * @lock: the mutex to be queried
+ * @ctx: the w/w acquire context to test
+ *
+ * If @ctx is not NULL test if the mutex is owned by this context.
+ * If @ctx is NULL test if the mutex is owned by the current thread and not
+ * locked in any context.
+ */
+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+ struct ww_acquire_ctx *ctx)
+{
+ if (ctx)
+ return likely(READ_ONCE(lock->ctx) == ctx);
+
+ return likely(__mutex_owner(&lock->base) == current) &&
+ likely(READ_ONCE(lock->ctx) == NULL;
+}
+
#endif
--
2.14.1
Re: [PATCH] tools/memory-model: remove rb-dep, smp_read_barrier_depends, and lockless_dereference
On Mon, Feb 19, 2018 at 09:28:44PM +0100, Peter Zijlstra wrote: > On Mon, Feb 19, 2018 at 11:41:23AM -0800, Paul E. McKenney wrote: > > On Mon, Feb 19, 2018 at 12:14:45PM -0500, Alan Stern wrote: > > > This leaves us with a question: Do we want to change the kernel by > > > adding memory barriers after unsuccessful RMW operations on Alpha, or > > > do we want to change the model by excluding such operations from > > > address dependencies? > > > > I vote for adding the barrier on Alpha. However, I don't know of any > > code in the Linux kernel that relies on read-to-read address dependency > > ordering headed by a failing RMW operation, so I don't feel all that > > strongly about this. > > Right, but not knowing doesn't mean doesn't exist, and most certainly > doesn't mean will never exist. Fair enough, safety first! > > > Note that operations like atomic_add_unless() already include memory > > > barriers. > > > > And I don't see an atomic_add_unless_relaxed(), so we are good on this > > one. So far, anyway! ;-) > > Not the point, add_unless() is a conditional operation, and therefore > doesn't need to imply anything when failing. Plus it doesn't return a pointer, so there is no problem with dereferences. Unless someone wants to use its return value as an array index and rely on dependency ordering to the array, but I would NAK that use case. Thanx, Paul
Re: [PATCH v14 6/9] HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings
On Mon, Feb 19, 2018 at 7:48 PM, John Garry wrote:
> From: Zhichang Yuan
>
> The low-pin-count(LPC) interface of Hip06/Hip07 accesses the peripherals in
> I/O port addresses. This patch implements the LPC host controller driver
> which perform the I/O operations on the underlying hardware.
> We don't want to touch those existing peripherals' driver, such as ipmi-bt.
> So this driver applies the indirect-IO introduced in the previous patch
> after registering an indirect-IO node to the indirect-IO devices list which
> will be searched in the I/O accessors to retrieve the host-local I/O port.
>
> The driver config is set as a bool instead of a trisate. The reason
> here is that, by the very nature of the driver providing a logical
> PIO range, it does not make sense to have this driver as a loadable
> module. Another more specific reason is that the Huawei D03 board
> which includes hip06 SoC requires the LPC bus for UART console, so
> should be built in.
> +config HISILICON_LPC
> + bool "Support for ISA I/O space on Hisilicon hip06/7"
> + depends on (ARM64 && (ARCH_HISI || COMPILE_TEST))
Redundant parens.
> + select INDIRECT_PIO
> + help
> + Driver needed for some legacy ISA devices attached to Low-Pin-Count
> + on Hisilicon hip06/7 SoC.
> +#if LPC_MAX_DULEN > LPC_MAX_BURST
> +#error "LPC.. MAX_DULEN must be not bigger than MAX_OPCNT!"
> +#endif
But here you can easily avoid an #error, by making them equal, just
issue a warning instead.
> +#if LPC_MAX_BURST % LPC_MAX_DULEN
> +#error "LPC.. LPC_MAX_BURST must be multiple of LPC_MAX_DULEN!"
> +#endif
Is it like this, or also should be power of two?
> +/* The command register fields */
> +#define LPC_CMD_SAMEADDR 0x08
> +#define LPC_CMD_TYPE_IO0x00
> +#define LPC_CMD_WRITE 0x01
> +#define LPC_CMD_READ 0x00
> +/* the bit attribute is W1C. 1 represents OK. */
> +#define LPC_STAT_BYIRQ 0x02
BIT() ?
> +#define LPC_STATUS_IDLE0x01
> +#define LPC_OP_FINISHED0x02
> +
> +#define LPC_START_WORK 0x01
Ditto?
> +static inline int wait_lpc_idle(unsigned char *mbase,
> + unsigned int waitcnt) {
> + u32 opstatus;
> +
> + while (waitcnt--) {
> + ndelay(LPC_NSEC_PERWAIT);
> + opstatus = readl(mbase + LPC_REG_OP_STATUS);
> + if (opstatus & LPC_STATUS_IDLE)
> + return (opstatus & LPC_OP_FINISHED) ? 0 : (-EIO);
> + }
> + return -ETIME;
Personally I prefer timeout loops in a do {} while (--count) style.
> +}
> +static int
> +hisi_lpc_target_in(struct hisi_lpc_dev *lpcdev, struct lpc_cycle_para *para,
> + unsigned long addr, unsigned char *buf,
> + unsigned long opcnt)
> +{
> + unsigned int cmd_word;
> + unsigned int waitcnt;
> + unsigned long flags;
> + int ret;
> +
> + if (!buf || !opcnt || !para || !para->csize || !lpcdev)
> + return -EINVAL;
> +
> + cmd_word = LPC_CMD_TYPE_IO | LPC_CMD_READ;
> + waitcnt = LPC_PEROP_WAITCNT;
> + if (!(para->opflags & FG_INCRADDR_LPC)) {
> + cmd_word |= LPC_CMD_SAMEADDR;
> + waitcnt = LPC_MAX_WAITCNT;
> + }
> +
> + ret = 0;
> +
Sounds redundant.
> + /* whole operation must be atomic */
> + spin_lock_irqsave(&lpcdev->cycle_lock, flags);
> +
> + writel_relaxed(opcnt, lpcdev->membase + LPC_REG_OP_LEN);
> +
> + writel_relaxed(cmd_word, lpcdev->membase + LPC_REG_CMD);
> +
> + writel_relaxed(addr, lpcdev->membase + LPC_REG_ADDR);
> +
> + writel(LPC_START_WORK, lpcdev->membase + LPC_REG_START);
> +
> + /* whether the operation is finished */
> + ret = wait_lpc_idle(lpcdev->membase, waitcnt);
> + if (!ret) {
I would rather go with usual pattern
if (ret) {
...
return ret;
}
> + for (; opcnt; opcnt--, buf++)
> + *buf = readb(lpcdev->membase + LPC_REG_RDATA);
Looks like a do {} while (slightly better for my opinion).
do {
*buf++ = readb(lpcdev->membase + LPC_REG_RDATA);
} while (--opcnt);
> + }
> +
> + spin_unlock_irqrestore(&lpcdev->cycle_lock, flags);
> +
> + return ret;
> +}
> + for (; opcnt; buf++, opcnt--)
> + writeb(*buf, lpcdev->membase + LPC_REG_WDATA);
Ditto.
> +static u32 hisi_lpc_comm_in(void *hostdata, unsigned long pio, size_t dwidth)
> + if (!lpcdev || !dwidth || dwidth > LPC_MAX_DULEN)
> + return -1;
~0 ?
> + if (ret)
> + return -1;
Ditto.
> + do {
> + int ret;
> +
> + ret = hisi_lpc_target_in(lpcdev, &iopara, addr,
> + buf, dwidth);
> + if (ret)
> + return ret;
> + buf += dwidth;
> + count--;
> + } while (cou
Re: [PATCH] disable sparse warnings about unknown attributes
2018-02-16 6:07 GMT+09:00 Luc Van Oostenryck : > Currently, sparse issues warnings on code using an attribute > it doesn't know about. > > One of the problem with this is that these warnings have no > value for the developer, it's just noise for him. At best these > warnings tell something about some deficiencies of sparse itself > but not about a potential problem with code analyzed. > > A second problem with this is that sparse release are, alas, > less frequent than new attributes are added to GCC. > > So, avoid the noise by asking sparse to not warn about > attributes it doesn't know about. > > Reference: https://marc.info/?l=linux-sparse&m=151871600016790 > Reference: https://marc.info/?l=linux-sparse&m=151871725417322 > Signed-off-by: Luc Van Oostenryck > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Applied to linux-kbuild/kbuild. Thanks! -- Best Regards Masahiro Yamada
Re: [PATCH 4.10 070/111] audit: fix auditd/kernel connection state tracking
On Tue, Feb 20, 2018 at 9:06 AM, Peter Zijlstra wrote:
> On Tue, Feb 20, 2018 at 08:25:21AM -0500, Paul Moore wrote:
>> On Tue, Feb 20, 2018 at 7:37 AM, Peter Zijlstra wrote:
>> > On Tue, Mar 28, 2017 at 02:30:56PM +0200, Greg Kroah-Hartman wrote:
>> >> 4.10-stable review patch. If anyone has any objections, please let me
>> >> know.
>> >
>> >> + if (!(auditd_test_task(current) ||
>> >> + (current == __mutex_owner(&audit_cmd_mutex {
>> >> + long stime = audit_backlog_wait_time;
>> >
>> > Since I cannot find the original email on lkml, NAK on this.
>> > __mutex_owner() is not a general purpose helper function.
>>
>> Since this code also exists in the current kernel, I need to ask what
>> recommended alternatives exist for determining the mutex owner?
>>
>> I imagine we could track the mutex owner separately in the audit
>> subsystem, but I'd much prefer to leverage an existing mechanism if
>> possible.
>
> It's not at all clear to me what that code does, I just stumbled upon
> __mutex_owner() outside of the mutex code itself and went WTF.
If you don't want people to use __mutex_owner() outside of the mutex
code I might suggest adding a rather serious comment at the top of the
function, because right now I don't see anything suggesting that
function shouldn't be used. Yes, there is the double underscore
prefix, but that can mean a few different things these days.
> The comment (aside from having the most horribly style) ...
Yeah, your dog is ugly too. Notice how neither comment is constructive?
> ... is wrong too, because it claims it will not block when we hold that lock,
> while,
> afaict, it will in fact do just that.
A mutex blocks when it is held, but the audit_log_start() function
should not block for the task that currently holds the
audit_cmd_mutex; that is what the comment is meant to convey. I
believe the comment makes sense, but I did write it so I'll concede
that I'm probably the not best judge. If anyone would like to offer a
different wording I'm happy to consider it.
> Maybe if you could explain how that code is supposed to work and why it
> doesn't know if it holds a lock I could make a suggestion...
I just spent a few minutes looking back over the bits available in
include/linux/mutex.h and I'm not seeing anything beyond
__mutex_owner() which would allow us to determine the mutex owning
task. It's probably easiest for us to just track ownership ourselves.
I'll put together a patch later today.
--
paul moore
www.paul-moore.com
Re: [PATCH v14 8/9] HISI LPC: Add ACPI support
On Mon, Feb 19, 2018 at 7:48 PM, John Garry wrote:
> Based on the previous patches, this patch supports the
> LPC host on hip06/hip07 for ACPI FW.
>
> It is the responsibility of the LPC host driver to
> enumerate the child devices, as the ACPI scan code will
> not enumerate children of "indirect IO" hosts.
> if (!acpi_device)
> ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> + else
> + ret = hisi_lpc_acpi_probe(dev);
> if (ret) {
> dev_err(dev, "populate children failed (%d)\n", ret);
> return ret;
Ah, please, ignore one comment to previous patch.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3
On Tue, Feb 20, 2018 at 03:34:07PM +0100, Christian König wrote: > > OK, but neither case would in fact need the !ctx case right? That's just > > there for completeness sake? > > Unfortunately not. TTM uses trylock to lock BOs which are about to be > evicted to make room for all the BOs locked with a ctx. > > I need to be able to distinct between the BOs which are trylocked and those > which are locked with a ctx. > > Writing this I actually noticed the current version is buggy, cause even > when we check the mutex owner we still need to make sure that the ctx in the > lock is NULL. Hurm... I can't remember why trylocks behave like that, and it seems rather unfortunate / inconsistent. Chris, Maarten, do either one of you remember? I'm thinking that if we do acquire the trylock, the thing should join the ctx such that a subsequent contending mutex_lock() can ww right.
Re: [PATCH v14 0/9] LPC: legacy ISA I/O support
On Mon, Feb 19, 2018 at 7:48 PM, John Garry wrote: > This patchset supports the IPMI-bt device attached to the Low-Pin-Count > interface implemented on Hisilicon Hip06/Hip07 SoC. > --- > | LPC host| > | | > --- > | > _V___LPC > | | > V V > > | BT(ipmi)| > > > When master accesses those peripherals beneath the Hip06/Hip07 LPC, a specific > LPC driver is needed to make LPC host generate the standard LPC I/O cycles > with > the target peripherals'I/O port addresses. But on curent arm64 world, there is > no real I/O accesses. All the I/O operations through in/out accessors are > based > on MMIO ranges; on Hip06/Hip07 LPC the I/O accesses are performed through > driver > specific accessors rather than MMIO. > To solve this issue and keep the relevant existing peripherals' drivers > untouched, > this patchset: >- introduces a generic I/O space management framework, logical PIO, to > support > I/O operations on host controllers operating either on MMIO buses or on > buses > requiring specific driver I/O accessors; >- redefines the in/out accessors to provide a unified interface for both > MMIO > and driver specific I/O operations. Using logical PIO, th call of > in/out() from > the host children drivers, such as ipmi-si, will be redirected to the > corresponding device-specific I/O hooks to perform the I/O accesses. > > Based on this patch-set, all the I/O accesses to Hip06/Hip07 LPC peripherals > can > be supported without any changes on the existing ipmi-si driver. > > The whole patchset has been tested on Hip07 D05 board both using DTB and ACPI. > > Differences to v13: > - dropped ACPI scan handler and added patch to not enumerate children > of indirect IO hosts in ACPI code > - tidied up logic_pio.c a bit for kerneldoc and made some APIs clearer > to understand > - tided (and simplified) hisi_lpc.c and added new ACPI probe code > (same as previous ACPI scan handler code, so comments from Rafael > and Andy included) > - reinstated PCI range upper limit check in pci_pio_to_address() > - dropped Dann Frazier's "tested-by" tag in light of changes > - rebase to linuxnext 20180219 (had to fix locally arm64 build issue) > Some minor comments per patches, otherwise FWIW Reviewed-by: Andy Shevchenko > Differences to v12: > - Addressed ACPI comments from Rafael and Andy, including: > - added SPDX license identifiers (other new files in the series got this > also) > - fixed style issues, like superflous newlines and symbol naming > - add fuller acpi_indirectio.c patch commit message > - dropped acpi_indirectio_host_data (author's decision) to simplify > - added Rob Herring's tag > - rebase to linux-next 20180212 > > Differences to v11: > - fixed build errors for i386, m68k, and tile > - added a comment in LPC driver commit log why we set >the kernel config as bool > - some tidying logic_pio code > > Differences to v10: > - dropped CONFIG_LOGIC_PIO. Reason is that CONFIG_PCI > depends on this, and CONFIG_PCI is a per-arch CONFIG. > So we would require all arch's kconfig to select this. > - Addressed Dann Frazier's comments on LPC driver, and > sopme other cleanup > - Moved logic_pio.h to be included in generic asm io.h > - Fixed ACPI indirect IO host setup to handle >1 child > - Relocated ACPI indirect IO host setup code to > drivers/acpi > - Rebased to linux next-20180118 > > Changes from v9: > - patch 2 has been split into 3 patches according to Bjorn comments on > v9 thread > - patch 1 has been reworked accordign to Bjorn comments on v9 > - now logic_pio_trans_hwaddr() has a sanity check to make sure the resource > size fits into the assigned range > - in patch 5 the MFD framework has been used to probe the LPC children > according to the suggestion from Mika Westerberg > - Maintaner has changed to Huawei Linuxarm mailing list > > Changes from v8: > - Simplified LIB IO framewrok > - Moved INDIRECT PIO ACPI framework under acpi/arm64 > - Renamed occurrences of "lib io" and "indirect io" to "lib pio" and > "indirect pio" to keep the patchset nomenclature consistent > - Removed Alignment reuqirements > - Moved LPC specific code out of ACPI common framework > - Now PIO indirect HW ranges can overlap > - Changed HiSilicon LPC driver maintainer (Gabriele Paoloni now) and split > maintaner file modifications in a separate commit > - Removed the commit with the DT nodes support for hip06 and hip07 (to be > pushed separately
[PATCH] doc-guide: kernel-doc: add comment about formatting verification
Currently there is no automated checking for kernel-doc comments except running 'kernel-doc -v -none '. Mention the possibility to run kernel-doc to verify formatting of the comments in the kernel-doc guide. Signed-off-by: Mike Rapoport --- Documentation/doc-guide/kernel-doc.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/doc-guide/kernel-doc.rst b/Documentation/doc-guide/kernel-doc.rst index c6c3297..bb5ed6d 100644 --- a/Documentation/doc-guide/kernel-doc.rst +++ b/Documentation/doc-guide/kernel-doc.rst @@ -44,6 +44,12 @@ that somebody changing the code will also change the documentation. The overview kernel-doc comments may be placed anywhere at the top indentation level. +Running the ``kernel-doc`` tool with increased verbosity and without actual +output generation may be used to verify proper formating of the +documentation comments. For example:: + + scripts/kernel-doc -v -none drivers/foo/bar.c + Function documentation -- -- 2.7.4
Re: [PATCH] tpm: use kmemdup() to copy the EFI event log
Send an updated version with corresponding change also to tpm_eventlog_of.c /Jarkko On Mon, 2018-02-19 at 14:10 +0200, Jarkko Sakkinen wrote: > Signed-off-by: Jarkko Sakkinen > --- > drivers/char/tpm/tpm_eventlog_efi.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm_eventlog_efi.c > b/drivers/char/tpm/tpm_eventlog_efi.c > index e3f9ffd341d2..57c8b3cc46be 100644 > --- a/drivers/char/tpm/tpm_eventlog_efi.c > +++ b/drivers/char/tpm/tpm_eventlog_efi.c > @@ -50,10 +50,9 @@ int tpm_read_log_efi(struct tpm_chip *chip) > } > > /* malloc EventLog space */ > - log->bios_event_log = kmalloc(log_size, GFP_KERNEL); > + log->bios_event_log = kmemdup(log_tbl->log, log_size, > GFP_KERNEL); > if (!log->bios_event_log) > goto err_memunmap; > - memcpy(log->bios_event_log, log_tbl->log, log_size); > log->bios_event_log_end = log->bios_event_log + log_size; > > tpm_log_version = log_tbl->version;
RE: [PATCH 2/2] x86/speculation: Support "Enhanced IBRS" on future CPUs
> > I meant software wise. You're not going to live migrate from xen to > > kvm or backwards. or between very radically different versions of the > > kvm stack. > > Forwards migration to a radically newer version certainly happens. So > when the source hypervisor was too old to tell the VM about IBRS_ALL, > for example, migration should work properly and the VM should perform > well on the destination hypervisor. that makes sense, compatibility in this direction can be done and is useful as you move a fleet of servers forward > > Backwards migration to older hypervisors also happens sometimes, but in > general it creates more userspace than kernel issues. > that direction is obviously harder
Re: [PATCH 1/2 v3] tpm: cmd_ready command can be issued only after granting locality
On Mon, 2018-02-19 at 11:43 +, Winkler, Tomas wrote: > > All local variable declarations must be in the beginning of the > > function. > > Who says? It is coherent how we have everything else. It is much easier to see the stack allocation this way when the allocation is only done in the beginning of each function. If you really need to do such pattern, then it would be a better idea to consider an additional helper function. > > Your comment about not overriding error code is incorrect. > > Please explain? 'l_rc' overrides 'rc' in the case when both are non-zero. > > The value of 'rc' should be never overridden, which kind of > > supports to "just > > print" behavior that we had for a locality error. > > You are not consistent, you've agreed with propagating it to user > space. The error will be propagated in case of an error in > locality relinquish the device is pretty much in non functional > state and provious errors do not matter much, but rc value won't > be modified if locality_reliquish succeeds. Well, sometimes you fail to notice things and I failed to notice the collision above. The commit message does not describe why 'l_rc' overrides 'rc' in the case when both are non-zero. What was the reasoning, which made you end up with this priority order? Why is 'l_rc' more important than 'rc'? My take is that does it really make sense have this change as part of a high priority bug fix that should be as localized as possible? Seems like a non-trivial problem by itself. /Jarkko
