[PATCH -next] staging: gasket: interrupt: remove unused including
From: Yue Haibing Remove including that don't need it. Signed-off-by: Yue Haibing --- drivers/staging/gasket/gasket_interrupt.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c index 1cfbc12..176ac2a 100644 --- a/drivers/staging/gasket/gasket_interrupt.c +++ b/drivers/staging/gasket/gasket_interrupt.c @@ -9,7 +9,6 @@ #include #include #include -#include #ifdef GASKET_KERNEL_TRACE_SUPPORT #define CREATE_TRACE_POINTS #include ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: > > And who is going to decide which ones to pass? And who documents > > which ones are safe? > > > > I'd much rather have explicit, well documented dma-buf flags that > > might get translated to the DMA API flags, which are not error checked, > > not very well documented and way to easy to get wrong. > > > > I'm not sure having flags in dma-buf really solves anything > given drivers can use the attributes directly with dma_map > anyway, which is what we're looking to do. The intention > is for the driver creating the dma_buf attachment to have > the knowledge of which flags to use. Well, there are very few flags that you can simply use for all calls of dma_map*. And given how badly these flags are defined I just don't want people to add more places where they indirectly use these flags, as it will be more than enough work to clean up the current mess. What flag(s) do you want to pass this way, btw? Maybe that is where the problem is. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] vmbus: Switch to use new generic UUID API
Looks good, Reviewed-by: Christoph Hellwig ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 0/7] binderfs: debug galore
Hey everyone, Al gave me a really helpful review of binderfs and pointed out a range of bugs. The most obvious and serious ones have fortunately already been taken care of by patches sitting in Greg's char-misc-linus tree. The others are hopefully all covered in this patchset. /* Changelog */ Nothing major apart from working in a bunch of good comments and suggestions from Al. The most interesting one is probably the switch from d_alloc_name() + d_lookup() to lookup_one_len() when detecting name clashes between binder devices. This also forces us to switch from d_add() to d_instantiate() since lookup_one_len() adds new dentries to the hashqueue. If we were to use d_add() after this we'd end up with the same dentry over the same inode twice. I moved the switch from d_add() to d_instantiate() into a separate commit. The rest should hopefully be pretty mundane. Thanks! Christian Christian Brauner (7): binderfs: remove outdated comment binderfs: prevent renaming the control dentry binderfs: rework binderfs_fill_super() binderfs: rework binderfs_binder_device_create() binderfs: kill_litter_super() before cleanup binderfs: drop lock in binderfs_binder_ctl_create binderfs: switch from d_add() to d_instantiate() drivers/android/binderfs.c | 121 + 1 file changed, 43 insertions(+), 78 deletions(-) -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 1/7] binderfs: remove outdated comment
The comment stems from an early version of that patchset and is just
confusing now.
Cc: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- patch unchanged
---
drivers/android/binderfs.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e4ff4c3fa371..898d847f8505 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -373,10 +373,6 @@ static int binderfs_rename(struct inode *old_dir, struct
dentry *old_dentry,
static int binderfs_unlink(struct inode *dir, struct dentry *dentry)
{
- /*
-* The control dentry is only ever touched during mount so checking it
-* here should not require us to take lock.
-*/
if (BINDERFS_I(dir)->control_dentry == dentry)
return -EPERM;
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 3/7] binderfs: rework binderfs_fill_super()
Al pointed out that on binderfs_fill_super() error
deactivate_locked_super() will call binderfs_kill_super() so all of the
freeing and putting we currently do in binderfs_fill_super() is unnecessary
and buggy. Let's simply return errors and let binderfs_fill_super() take
care of cleaning up on error.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- correctly grab and stash a reference to task's ipc_ns to prevent leaks
- replace d_alloc_name() + d_lookup() combination with lookup_one_len()
- replace kmalloc() + strscpy() with kmemdup()
---
drivers/android/binderfs.c | 41 ++
1 file changed, 11 insertions(+), 30 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e73f9dbee099..89a2ee1a02f6 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -461,12 +461,9 @@ static const struct inode_operations
binderfs_dir_inode_operations = {
static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
{
+ int ret;
struct binderfs_info *info;
- int ret = -ENOMEM;
struct inode *inode = NULL;
- struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
-
- get_ipc_ns(ipc_ns);
sb->s_blocksize = PAGE_SIZE;
sb->s_blocksize_bits = PAGE_SHIFT;
@@ -488,15 +485,17 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
sb->s_op = &binderfs_super_ops;
sb->s_time_gran = 1;
- info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
- if (!info)
- goto err_without_dentry;
+ sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
+ if (!sb->s_fs_info)
+ return -ENOMEM;
+ info = sb->s_fs_info;
+
+ info->ipc_ns = get_ipc_ns(current->nsproxy->ipc_ns);
ret = binderfs_parse_mount_opts(data, &info->mount_opts);
if (ret)
- goto err_without_dentry;
+ return ret;
- info->ipc_ns = ipc_ns;
info->root_gid = make_kgid(sb->s_user_ns, 0);
if (!gid_valid(info->root_gid))
info->root_gid = GLOBAL_ROOT_GID;
@@ -504,12 +503,9 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
if (!uid_valid(info->root_uid))
info->root_uid = GLOBAL_ROOT_UID;
- sb->s_fs_info = info;
-
- ret = -ENOMEM;
inode = new_inode(sb);
if (!inode)
- goto err_without_dentry;
+ return -ENOMEM;
inode->i_ino = FIRST_INODE;
inode->i_fop = &simple_dir_operations;
@@ -520,24 +516,9 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
sb->s_root = d_make_root(inode);
if (!sb->s_root)
- goto err_without_dentry;
-
- ret = binderfs_binder_ctl_create(sb);
- if (ret)
- goto err_with_dentry;
-
- return 0;
-
-err_with_dentry:
- dput(sb->s_root);
- sb->s_root = NULL;
-
-err_without_dentry:
- put_ipc_ns(ipc_ns);
- iput(inode);
- kfree(info);
+ return -ENOMEM;
- return ret;
+ return binderfs_binder_ctl_create(sb);
}
static struct dentry *binderfs_mount(struct file_system_type *fs_type,
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 7/7] binderfs: switch from d_add() to d_instantiate()
In a previous commit we switched from a d_alloc_name() + d_lookup() combination to setup a new dentry and find potential duplicates to the more idiomatic lookup_one_len(). As far as I understand, this also means we need to switch from d_add() to d_instantiate() since lookup_one_len() will create a new dentry when it doesn't find an existing one and add the new dentry to the hash queues. So we only need to call d_instantiate() to connect the dentry to the inode and turn it into a positive dentry. If we were to use d_add() we sure see stack traces like the following indicating that adding the same dentry twice over the same inode: [ 744.441889] CPU: 4 PID: 2849 Comm: landscape-sysin Not tainted 5.0.0-rc1-brauner-binderfs #243 [ 744.441889] Hardware name: Dell DCS XS24-SC2 /XS24-SC2 , BIOS S59_3C20 04/07/2011 [ 744.441889] RIP: 0010:__d_lookup_rcu+0x76/0x190 [ 744.441889] Code: 89 75 c0 49 c1 e9 20 49 89 fd 45 89 ce 41 83 e6 07 42 8d 04 f5 00 00 00 00 89 45 c8 eb 0c 48 8b 1b 48 85 db 0f 84 81 00 00 00 <44> 8b 63 fc 4c 3b 6b 10 75 ea 48 83 7b 08 00 74 e3 41 83 e4 fe 41 [ 744.441889] RSP: 0018:b8c984e27ad0 EFLAGS: 0282 ORIG_RAX: ff13 [ 744.441889] RAX: 0038 RBX: 9407ef770c08 RCX: b8c980011000 [ 744.441889] RDX: b8c984e27b54 RSI: b8c984e27ce0 RDI: 9407e6689600 [ 744.441889] RBP: b8c984e27b28 R08: b8c984e27ba4 R09: 0007 [ 744.441889] R10: 9407e5c4f05c R11: 973f3eb9d84a94e5 R12: 0002 [ 744.441889] R13: 9407e6689600 R14: 0007 R15: 0007bfef7a13 [ 744.441889] FS: 7f0db13bb740() GS:9407f3b0() knlGS: [ 744.441889] CS: 0010 DS: ES: CR0: 80050033 [ 744.441889] CR2: 7f0dacc51024 CR3: 00032961a000 CR4: 06e0 [ 744.441889] Call Trace: [ 744.441889] lookup_fast+0x53/0x300 [ 744.441889] walk_component+0x49/0x350 [ 744.441889] ? inode_permission+0x63/0x1a0 [ 744.441889] link_path_walk.part.33+0x1bc/0x5a0 [ 744.441889] ? path_init+0x190/0x310 [ 744.441889] path_lookupat+0x95/0x210 [ 744.441889] filename_lookup+0xb6/0x190 [ 744.441889] ? __check_object_size+0xb8/0x1b0 [ 744.441889] ? strncpy_from_user+0x50/0x1a0 [ 744.441889] user_path_at_empty+0x36/0x40 [ 744.441889] ? user_path_at_empty+0x36/0x40 [ 744.441889] vfs_statx+0x76/0xe0 [ 744.441889] __do_sys_newstat+0x3d/0x70 [ 744.441889] __x64_sys_newstat+0x16/0x20 [ 744.441889] do_syscall_64+0x5a/0x120 [ 744.441889] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 744.441889] RIP: 0033:0x7f0db0ec2775 [ 744.441889] Code: 00 00 00 75 05 48 83 c4 18 c3 e8 26 55 02 00 66 0f 1f 44 00 00 83 ff 01 48 89 f0 77 30 48 89 c7 48 89 d6 b8 04 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 03 f3 c3 90 48 8b 15 e1 b6 2d 00 f7 d8 64 89 [ 744.441889] RSP: 002b:7ffc36bc9388 EFLAGS: 0246 ORIG_RAX: 0004 [ 744.441889] RAX: ffda RBX: 7ffc36bc9300 RCX: 7f0db0ec2775 [ 744.441889] RDX: 7ffc36bc9400 RSI: 7ffc36bc9400 RDI: 7f0dad26f050 [ 744.441889] RBP: 00c0bc60 R08: R09: 0001 [ 744.441889] R10: R11: 0246 R12: 7ffc36bc9400 [ 744.441889] R13: 0001 R14: ff9c R15: 00c0bc60 Cc: Al Viro Signed-off-by: Christian Brauner --- /* Changelog */ v1: - patch introduced --- drivers/android/binderfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index d537dcdb5d65..6a2185eb66c5 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -212,7 +212,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode, } inode->i_private = device; - d_add(dentry, inode); + d_instantiate(dentry, inode); fsnotify_create(root->d_inode, dentry); inode_unlock(d_inode(root)); -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 2/7] binderfs: prevent renaming the control dentry
- make binderfs control dentry immutable:
We don't allow to unlink it since it is crucial for binderfs to be
useable but if we allow to rename it we make the unlink trivial to
bypass. So prevent renaming too and simply treat the control dentry as
immutable.
- add is_binderfs_control_device() helper:
Take the opportunity and turn the check for the control dentry into a
separate helper is_binderfs_control_device() since it's now used in two
places.
- simplify binderfs_rename():
Instead of hand-rolling our custom version of simple_rename() just dumb
the whole function down to first check whether we're trying to rename the
control dentry. If we do EPERM the caller and if not call simple_rename().
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- simplify is_binderfs_control_device() to only take a dentry argument
instead of taking an unnecessary detour through the inode.
---
drivers/android/binderfs.c | 28 ++--
1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 898d847f8505..e73f9dbee099 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -346,34 +346,26 @@ static const struct super_operations binderfs_super_ops =
{
.statfs = simple_statfs,
};
+static inline bool is_binderfs_control_device(const struct dentry *dentry)
+{
+ struct binderfs_info *info = dentry->d_sb->s_fs_info;
+ return info->control_dentry == dentry;
+}
+
static int binderfs_rename(struct inode *old_dir, struct dentry *old_dentry,
struct inode *new_dir, struct dentry *new_dentry,
unsigned int flags)
{
- struct inode *inode = d_inode(old_dentry);
-
- /* binderfs doesn't support directories. */
- if (d_is_dir(old_dentry))
+ if (is_binderfs_control_device(old_dentry) ||
+ is_binderfs_control_device(new_dentry))
return -EPERM;
- if (flags & ~RENAME_NOREPLACE)
- return -EINVAL;
-
- if (!simple_empty(new_dentry))
- return -ENOTEMPTY;
-
- if (d_really_is_positive(new_dentry))
- simple_unlink(new_dir, new_dentry);
-
- old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime =
- new_dir->i_mtime = inode->i_ctime = current_time(old_dir);
-
- return 0;
+ return simple_rename(old_dir, old_dentry, new_dir, new_dentry, flags);
}
static int binderfs_unlink(struct inode *dir, struct dentry *dentry)
{
- if (BINDERFS_I(dir)->control_dentry == dentry)
+ if (is_binderfs_control_device(dentry))
return -EPERM;
return simple_unlink(dir, dentry);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 6/7] binderfs: drop lock in binderfs_binder_ctl_create
The binderfs_binder_ctl_create() call is a no-op on subsequent calls and
the first call is done before we unlock the suberblock. Hence, there is no
need to take inode_lock() in there. Let's remove it.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
Note, that fs/devptfs/inode.c:mknod_ptmx() is currently holding
inode_lock() too under the exact same circumstances. Seems that we can drop
it from there too.
/* Changelog */
v1:
- patch unchanged
---
drivers/android/binderfs.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index ba88be172aee..d537dcdb5d65 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -400,8 +400,6 @@ static int binderfs_binder_ctl_create(struct super_block
*sb)
if (!device)
return -ENOMEM;
- inode_lock(d_inode(root));
-
/* If we have already created a binder-control node, return. */
if (info->control_dentry) {
ret = 0;
@@ -440,12 +438,10 @@ static int binderfs_binder_ctl_create(struct super_block
*sb)
inode->i_private = device;
info->control_dentry = dentry;
d_add(dentry, inode);
- inode_unlock(d_inode(root));
return 0;
out:
- inode_unlock(d_inode(root));
kfree(device);
iput(inode);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 5/7] binderfs: kill_litter_super() before cleanup
Al pointed out that first calling kill_litter_super() before cleaning up
info is more correct since destroying info doesn't depend on the state of
the dentries and inodes. That the opposite remains true is not guaranteed.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- patch unchanged
---
drivers/android/binderfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 1e077498a507..ba88be172aee 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -531,11 +531,12 @@ static void binderfs_kill_super(struct super_block *sb)
{
struct binderfs_info *info = sb->s_fs_info;
+ kill_litter_super(sb);
+
if (info && info->ipc_ns)
put_ipc_ns(info->ipc_ns);
kfree(info);
- kill_litter_super(sb);
}
static struct file_system_type binder_fs_type = {
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 4/7] binderfs: rework binderfs_binder_device_create()
- switch from d_alloc_name() + d_lookup() to lookup_one_len():
Instead of using d_alloc_name() and then doing a d_lookup() with the
allocated dentry to find whether a device with the name we're trying to
create already exists switch to using lookup_one_len(). The latter will
either return the existing dentry or a new one.
- switch from kmalloc() + strscpy() to kmemdup():
Use a more idiomatic way to copy the name for the new dentry that
userspace gave us.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- patch introduced
---
drivers/android/binderfs.c | 39 +++---
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 89a2ee1a02f6..1e077498a507 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -106,7 +107,7 @@ bool is_binderfs_device(const struct inode *inode)
* @userp: buffer to copy information about new device for userspace to
* @req: struct binderfs_device as copied from userspace
*
- * This function allocated a new binder_device and reserves a new minor
+ * This function allocates a new binder_device and reserves a new minor
* number for it.
* Minor numbers are limited and tracked globally in binderfs_minors. The
* function will stash a struct binder_device for the specific binder
@@ -122,10 +123,10 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
struct binderfs_device *req)
{
int minor, ret;
- struct dentry *dentry, *dup, *root;
+ struct dentry *dentry, *root;
struct binder_device *device;
- size_t name_len = BINDERFS_MAX_NAME + 1;
char *name = NULL;
+ size_t name_len;
struct inode *inode = NULL;
struct super_block *sb = ref_inode->i_sb;
struct binderfs_info *info = sb->s_fs_info;
@@ -168,12 +169,13 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
inode->i_uid = info->root_uid;
inode->i_gid = info->root_gid;
- name = kmalloc(name_len, GFP_KERNEL);
+ req->name[BINDERFS_MAX_NAME] = '\0'; /* NUL-terminate */
+ name_len = strlen(req->name);
+ /* Make sure to include terminating NUL byte */
+ name = kmemdup(req->name, name_len + 1, GFP_KERNEL);
if (!name)
goto err;
- strscpy(name, req->name, name_len);
-
device->binderfs_inode = inode;
device->context.binder_context_mgr_uid = INVALID_UID;
device->context.name = name;
@@ -192,24 +194,21 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
root = sb->s_root;
inode_lock(d_inode(root));
- dentry = d_alloc_name(root, name);
- if (!dentry) {
+
+ /* look it up */
+ dentry = lookup_one_len(name, root, name_len);
+ if (IS_ERR(dentry)) {
inode_unlock(d_inode(root));
- ret = -ENOMEM;
+ ret = PTR_ERR(dentry);
goto err;
}
- /* Verify that the name userspace gave us is not already in use. */
- dup = d_lookup(root, &dentry->d_name);
- if (dup) {
- if (d_really_is_positive(dup)) {
- dput(dup);
- dput(dentry);
- inode_unlock(d_inode(root));
- ret = -EEXIST;
- goto err;
- }
- dput(dup);
+ if (d_really_is_positive(dentry)) {
+ /* already exists */
+ dput(dentry);
+ inode_unlock(d_inode(root));
+ ret = -EEXIST;
+ goto err;
}
inode->i_private = device;
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] binderfs: use __u32 for device numbers
We allow more then 255 binderfs binder devices to be created since there
are workloads that require more than that. If we use __u8 we'll overflow
after 255. So let's use a __u32.
Note that there's no released kernel with binderfs out there so this is
not a regression.
Signed-off-by: Christian Brauner
---
include/uapi/linux/android/binderfs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/android/binderfs.h
b/include/uapi/linux/android/binderfs.h
index b41628b77120..87410477aea9 100644
--- a/include/uapi/linux/android/binderfs.h
+++ b/include/uapi/linux/android/binderfs.h
@@ -22,8 +22,8 @@
*/
struct binderfs_device {
char name[BINDERFS_MAX_NAME + 1];
- __u8 major;
- __u8 minor;
+ __u32 major;
+ __u32 minor;
};
/**
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] binderfs: use correct include guards in header
When we switched over from binder_ctl.h to binderfs.h we forgot to change
the include guards. It's minor but it's obviously correct.
Signed-off-by: Christian Brauner
---
include/uapi/linux/android/binderfs.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/android/binderfs.h
b/include/uapi/linux/android/binderfs.h
index 65b2efd1a0a5..b41628b77120 100644
--- a/include/uapi/linux/android/binderfs.h
+++ b/include/uapi/linux/android/binderfs.h
@@ -4,8 +4,8 @@
*
*/
-#ifndef _UAPI_LINUX_BINDER_CTL_H
-#define _UAPI_LINUX_BINDER_CTL_H
+#ifndef _UAPI_LINUX_BINDERFS_H
+#define _UAPI_LINUX_BINDERFS_H
#include
#include
@@ -31,5 +31,5 @@ struct binderfs_device {
*/
#define BINDER_CTL_ADD _IOWR('b', 1, struct binderfs_device)
-#endif /* _UAPI_LINUX_BINDER_CTL_H */
+#endif /* _UAPI_LINUX_BINDERFS_H */
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
Hi,
On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
> Disable the SMFC before disabling the IDMA channel, instead of after,
> in csi_idmac_unsetup().
>
> This fixes a complete system hard lockup on the SabreAuto when streaming
> from the ADV7180, by repeatedly sending a stream off immediately followed
> by stream on:
>
> while true; do v4l2-ctl -d4 --stream-mmap --stream-count=3; done
>
> Eventually this either causes the system lockup or EOF timeouts at all
> subsequent stream on, until a system reset.
>
> The lockup occurs when disabling the IDMA channel at stream off. Stopping
> the video data stream entering the IDMA channel before disabling the
> channel itself appears to be a reliable fix for the hard lockup. That can
> be done either by disabling the SMFC or the CSI before disabling the
> channel. Disabling the SMFC before the channel is the easiest solution,
> so do that.
>
> Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
>
> Suggested-by: Peter Seiderer
> Reported-by: Gaël PORTAY
> Signed-off-by: Steve Longerbeam
Gaël, could we get a Tested-by: for this as well?
> Cc: sta...@vger.kernel.org
> ---
> Changes in v3:
> - switch from disabling the CSI before the channel to disabling the
> SMFC before the channel.
> Changes in v2:
> - restore an empty line
> - Add Fixes: and Cc: stable
> ---
> drivers/staging/media/imx/imx-media-csi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index e18f58f56dfb..8610027eac97 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -560,8 +560,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
> static void csi_idmac_unsetup(struct csi_priv *priv,
> enum vb2_buffer_state state)
> {
> - ipu_idmac_disable_channel(priv->idmac_ch);
> ipu_smfc_disable(priv->smfc);
> + ipu_idmac_disable_channel(priv->idmac_ch);
Steve, do you have any theory why this helps? It's a bit weird to
disable the SMFC module while the DMA channel is still enabled. I think
this warrants a big comment, given that enable order is SMFC_EN before
IDMAC channel enable.
Also ipu_smfc_disable is refcounted, so if the other CSI is capturing
simultaneously, this change has no effect.
FWIW, I don't see any regressions though.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap
Hi,
Sorry for being a bit sporadic on this. I was out travelling last week
with little time for email.
On Fri, Jan 18, 2019 at 11:16:31AM -0600, Andrew F. Davis wrote:
> On 1/17/19 7:11 PM, Liam Mark wrote:
> > On Thu, 17 Jan 2019, Andrew F. Davis wrote:
> >
> >> On 1/16/19 4:54 PM, Liam Mark wrote:
> >>> On Wed, 16 Jan 2019, Andrew F. Davis wrote:
> >>>
> On 1/16/19 9:19 AM, Brian Starkey wrote:
> > Hi :-)
> >
> > On Tue, Jan 15, 2019 at 12:40:16PM -0600, Andrew F. Davis wrote:
> >> On 1/15/19 12:38 PM, Andrew F. Davis wrote:
> >>> On 1/15/19 11:45 AM, Liam Mark wrote:
> On Tue, 15 Jan 2019, Andrew F. Davis wrote:
>
> > On 1/14/19 11:13 AM, Liam Mark wrote:
> >> On Fri, 11 Jan 2019, Andrew F. Davis wrote:
> >>
> >>> Buffers may not be mapped from the CPU so skip cache maintenance
> >>> here.
> >>> Accesses from the CPU to a cached heap should be bracketed with
> >>> {begin,end}_cpu_access calls so maintenance should not be needed
> >>> anyway.
> >>>
> >>> Signed-off-by: Andrew F. Davis
> >>> ---
> >>> drivers/staging/android/ion/ion.c | 7 ---
> >>> 1 file changed, 4 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/android/ion/ion.c
> >>> b/drivers/staging/android/ion/ion.c
> >>> index 14e48f6eb734..09cb5a8e2b09 100644
> >>> --- a/drivers/staging/android/ion/ion.c
> >>> +++ b/drivers/staging/android/ion/ion.c
> >>> @@ -261,8 +261,8 @@ static struct sg_table
> >>> *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> >>>
> >>> table = a->table;
> >>>
> >>> - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> >>> - direction))
> >>> + if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> >>> + direction, DMA_ATTR_SKIP_CPU_SYNC))
> >>
> >> Unfortunately I don't think you can do this for a couple reasons.
> >> You can't rely on {begin,end}_cpu_access calls to do cache
> >> maintenance.
> >> If the calls to {begin,end}_cpu_access were made before the call
> >> to
> >> dma_buf_attach then there won't have been a device attached so the
> >> calls
> >> to {begin,end}_cpu_access won't have done any cache maintenance.
> >>
> >
> > That should be okay though, if you have no attachments (or all
> > attachments are IO-coherent) then there is no need for cache
> > maintenance. Unless you mean a sequence where a non-io-coherent
> > device
> > is attached later after data has already been written. Does that
> > sequence need supporting?
>
> Yes, but also I think there are cases where CPU access can happen
> before
> in Android, but I will focus on later for now.
>
> > DMA-BUF doesn't have to allocate the backing
> > memory until map_dma_buf() time, and that should only happen after
> > all
> > the devices have attached so it can know where to put the buffer.
> > So we
> > shouldn't expect any CPU access to buffers before all the devices
> > are
> > attached and mapped, right?
> >
>
> Here is an example where CPU access can happen later in Android.
>
> Camera device records video -> software post processing -> video
> device
> (who does compression of raw data) and writes to a file
>
> In this example assume the buffer is cached and the devices are not
> IO-coherent (quite common).
>
> >>>
> >>> This is the start of the problem, having cached mappings of memory
> >>> that
> >>> is also being accessed non-coherently is going to cause issues one way
> >>> or another. On top of the speculative cache fills that have to be
> >>> constantly fought back against with CMOs like below; some coherent
> >>> interconnects behave badly when you mix coherent and non-coherent
> >>> access
> >>> (snoop filters get messed up).
> >>>
> >>> The solution is to either always have the addresses marked
> >>> non-coherent
> >>> (like device memory, no-map carveouts), or if you really want to use
> >>> regular system memory allocated at runtime, then all cached mappings
> >>> of
> >>> it need to be dropped, even the kernel logical address (area as
> >>> painful
> >>> as that would be).
> >
> > Ouch :-( I wasn't aware about these potential interconnect issues. How
> > "real" is that? It seems that we aren't really hitting that today on
> > real devices.
> >
>
> Sadly there is at least one real device like this now (TI AM654).
Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout
On Friday, January 18, 2019 5:04:50 PM CET Peter Zijlstra wrote: > On Fri, Jan 18, 2019 at 10:19:41AM -0500, Joel Fernandes wrote: > > You should document the variable names in the header comments. > > > > Also, this new API appears to conflict with definition of 'freezable' in > > wait_event_freezable I think, > > > > wait_event_freezable - sleep or freeze until condition is true. > > > > wait_event_freezable_hrtimeout - sleep but make sure freezer is not blocked. > > (your API) > > > > It seems to me these are 2 different definitions of 'freezing' (or I could > > be > > completely confused). But in the first case it calls try_to_freeze after > > schedule(). In the second case (your new API), it calls > > freezable_schedule(). > > > > So I am wondering why is there this difference in the 'meaning of > > freezable'. > > In the very least, any such subtle differences should be documented in the > > header comments IMO. > > Also; someone was recently hating on the whole freezing thing (again, > and rightfully). I just cannot remember who; Rafael you remember? Nope. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: android: ion: Support for mapping with dma mapping attributes
Hi Liam,
On Fri, Jan 18, 2019 at 10:37:47AM -0800, Liam Mark wrote:
> Add support for configuring dma mapping attributes when mapping
> and unmapping memory through dma_buf_map_attachment and
> dma_buf_unmap_attachment.
>
> For example this will allow ION clients to skip cache maintenance, by
> using DMA_ATTR_SKIP_CPU_SYNC, for buffers which are clean and haven't been
> accessed by the CPU.
How can a client know that the buffer won't be accessed by the CPU in
the future though?
I don't think we can push this decision to clients, because they are
lacking information about what else is going on with the buffer. It
needs to be done by the exporter, IMO.
Thanks,
-Brian
>
> Signed-off-by: Liam Mark
> ---
> drivers/staging/android/ion/ion.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c
> index 1fe633a7fdba..0aae845b20ba 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -268,8 +268,8 @@ static struct sg_table *ion_map_dma_buf(struct
> dma_buf_attachment *attachment,
> table = a->table;
>
> mutex_lock(&buffer->lock);
> - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> - direction)) {
> + if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> + direction, attachment->dma_map_attrs)) {
> mutex_unlock(&buffer->lock);
> return ERR_PTR(-ENOMEM);
> }
> @@ -287,7 +287,8 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment
> *attachment,
> struct ion_buffer *buffer = attachment->dmabuf->priv;
>
> mutex_lock(&buffer->lock);
> - dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
> + dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents, direction,
> +attachment->dma_map_attrs);
> a->dma_mapped = false;
> mutex_unlock(&buffer->lock);
> }
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/13] staging: wilc1000: make use of get_unaligned_le16/le32 to pack data
On Thu, Jan 17, 2019 at 01:21:10PM +, ajay.kat...@microchip.com wrote: > From: Ajay Singh > > Make use of get_unaligned_le16/le32 framework api's to pack data. > > Signed-off-by: Ajay Singh > --- > drivers/staging/wilc1000/host_interface.c | 15 +++ > drivers/staging/wilc1000/wilc_wlan_cfg.c | 27 +-- > 2 files changed, 16 insertions(+), 26 deletions(-) > > diff --git a/drivers/staging/wilc1000/host_interface.c > b/drivers/staging/wilc1000/host_interface.c > index c05c120..a718842 100644 > --- a/drivers/staging/wilc1000/host_interface.c > +++ b/drivers/staging/wilc1000/host_interface.c > @@ -2154,10 +2154,7 @@ void wilc_network_info_received(struct wilc *wilc, u8 > *buffer, u32 length) > struct host_if_drv *hif_drv; > struct wilc_vif *vif; > > - id = buffer[length - 4]; Not related to this patch, but so far as I can see, length can't be zero but there is nothing preventing if from being less than four so this could be reading from buffer[-3]. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] PCI: hv: Allocate physically contiguous hypercall params buffer
On Fri, Jan 18, 2019 at 02:17:16AM +0530, Ajay Kaher wrote: > hv_do_hypercall() assumes that we pass a segment from a physically > contiguous buffer. A buffer allocated on the stack may not work if > CONFIG_VMAP_STACK=y is set. > > Use kmalloc() to allocate this buffer. > Since you're going to need to resend this anyway, can you add in the commit message what this looks like from a user perspective? Presumably it's an occasional crash? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] PCI: hv: Allocate physically contiguous hypercall params buffer
On Mon, Jan 21, 2019 at 04:19:42PM +0300, Dan Carpenter wrote: > On Fri, Jan 18, 2019 at 02:17:16AM +0530, Ajay Kaher wrote: > > hv_do_hypercall() assumes that we pass a segment from a physically > > contiguous buffer. A buffer allocated on the stack may not work if > > CONFIG_VMAP_STACK=y is set. > > > > Use kmalloc() to allocate this buffer. > > > > Since you're going to need to resend this anyway, can you add in the > commit message what this looks like from a user perspective? Presumably > it's an occasional crash? > Never mind. I didn't realize this was a two year old patch. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/14] staging: android: ion: Allow heap name to be null
On 1/18/19 1:53 PM, Laura Abbott wrote: > On 1/16/19 9:12 AM, Andrew F. Davis wrote: >> On 1/16/19 9:28 AM, Brian Starkey wrote: >>> Hi Andrew, >>> >>> On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote: The heap name can be used for debugging but otherwise does not seem to be required and no other part of the code will fail if left NULL except here. We can make it required and check for it at some point, for now lets just prevent this from causing a NULL pointer exception. >>> >>> I'm not so keen on this one. In the "new" API with heap querying, the >>> name string is the only way to identify the heap. I think Laura >>> mentioned at XDC2017 that it was expected that userspace should use >>> the strings to find the heap they want. >>> >> >> Right now the names are only for debug. I accidentally left the name >> null once and got a kernel crash. This is the only spot where it is >> needed so I fixed it up. The other option is to make the name mandatory >> and properly error out, I don't want to do that right now until the >> below discussion is had to see if names really do matter or not. >> > > Yes, the heap names are part of the query API and are the expected > way to identify individual heaps for the API at the moment so having > a null heap name is incorrect. The heap name seemed like the best way > to identify heaps to userspace but if you have an alternative proposal > I'd be interested. > Not sure I have a better proposal right now, I'll re-work this patch to force heap names to be populated before ion_device_add_heap() instead. (do you think that function name is now is a misnomer? how do you feel about renaming that to just ion_add_heap()?) Andrew > Thanks, > Laura > >> > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[driver-core:debugfs_cleanup 115/119] net//ceph/debugfs.c:459:9: warning: 'return' with a value, in function returning void
tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
debugfs_cleanup
head: 94081c6f77f3899ac96ae7baa8c7bf13ccc1dfd2
commit: 633d249a0993958bc7f19599639256e46410b305 [115/119] ceph: fix changelog
config: x86_64-randconfig-a0-01211725 (attached as .config)
compiler: gcc-8 (Debian 8.2.0-14) 8.2.0
reproduce:
git checkout 633d249a0993958bc7f19599639256e46410b305
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
net//ceph/debugfs.c: In function 'ceph_debugfs_init':
>> net//ceph/debugfs.c:459:9: warning: 'return' with a value, in function
>> returning void
return 0;
^
net//ceph/debugfs.c:457:13: note: declared here
void __init ceph_debugfs_init(void)
^
vim +/return +459 net//ceph/debugfs.c
3d14c5d2 Yehuda Sadeh 2010-04-06 456
633d249a Greg Kroah-Hartman 2019-01-04 457 void __init ceph_debugfs_init(void)
3d14c5d2 Yehuda Sadeh 2010-04-06 458 {
3d14c5d2 Yehuda Sadeh 2010-04-06 @459 return 0;
3d14c5d2 Yehuda Sadeh 2010-04-06 460 }
3d14c5d2 Yehuda Sadeh 2010-04-06 461
:: The code at line 459 was first introduced by commit
:: 3d14c5d2b6e15c21d8e5467dc62d33127c23a644 ceph: factor out libceph from
Ceph file system
:: TO: Yehuda Sadeh
:: CC: Sage Weil
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/14] Misc ION cleanups and adding unmapped heap
On 1/18/19 2:19 PM, Laura Abbott wrote: > On 1/16/19 8:05 AM, Andrew F. Davis wrote: >> On 1/15/19 12:58 PM, Laura Abbott wrote: >>> On 1/15/19 9:47 AM, Andrew F. Davis wrote: On 1/14/19 8:39 PM, Laura Abbott wrote: > On 1/11/19 10:05 AM, Andrew F. Davis wrote: >> Hello all, >> >> This is a set of (hopefully) non-controversial cleanups for the ION >> framework and current set of heaps. These were found as I start to >> familiarize myself with the framework to help in whatever way I >> can in getting all this up to the standards needed for de-staging. >> >> I would like to get some ideas of what is left to work on to get ION >> out of staging. Has there been some kind of agreement on what ION >> should >> eventually end up being? To me it looks like it is being whittled >> away at >> to it's most core functions. To me that is looking like being a >> DMA-BUF >> user-space front end, simply advertising available memory backings >> in a >> system and providing allocations as DMA-BUF handles. If this is the >> case >> then it looks close to being ready to me at least, but I would >> love to >> hear any other opinions and concerns. >> > > Yes, at this point the only functionality that people are really > depending on is the ability to allocate a dma_buf easily from > userspace. > >> Back to this patchset, the last patch may be a bit different than the >> others, it adds an unmapped heaps type and creation helper. I >> wanted to >> get this in to show off another heap type and maybe some issues we >> may >> have with the current ION framework. The unmapped heap is used >> when the >> backing memory should not (or cannot) be touched. Currently this kind >> of heap is used for firewalled secure memory that can be allocated >> like >> normal heap memory but only used by secure devices (OP-TEE, crypto >> HW, >> etc). It is basically just copied from the "carveout" heap type with >> the >> only difference being it is not mappable to userspace and we do not >> clear >> the memory (as we should not map it either). So should this really >> be a >> new heap type? Or maybe advertised as a carveout heap but with an >> additional allocation flag? Perhaps we do away with "types" >> altogether >> and just have flags, coherent/non-coherent, mapped/unmapped, etc. >> >> Maybe more thinking will be needed afterall.. >> > > So the cleanup looks okay (I need to finish reviewing) but I'm not a > fan of adding another heaptype without solving the problem of adding > some sort of devicetree binding or other method of allocating and > placing Ion heaps. That plus uncached buffers are one of the big > open problems that need to be solved for destaging Ion. See > https://lore.kernel.org/lkml/20181120164636.jcw7li2uaa3cmwc3@DESKTOP-E1NTVVP.localdomain/ > > > > for some background on that problem. > I'm under the impression that adding heaps like carveouts/chunk will be rather system specific and so do not lend themselves well to a universal DT style exporter. For instance a carveout memory space can be reported by a device at runtime, then the driver managing that device should go and use the carveout heap helpers to export that heap. If this is the case then I'm not sure it is a problem for the ION core framework to solve, but rather the users of it to figure out how best to create the various heaps. All Ion needs to do is allow exporting and advertising them IMHO. >>> >>> I think it is a problem for the Ion core framework to take care of. >>> Ion is useless if you don't actually have the heaps. Nobody has >>> actually gotten a full Ion solution end-to-end with a carveout heap >>> working in mainline because any proposals have been rejected. I think >>> we need at least one example in mainline of how creating a carveout >>> heap would work. >> >> In our evil vendor trees we have several examples. The issue being that >> Ion is still staging and attempts for generic DT heap definitions >> haven't seemed to go so well. So for now we just keep it specific to our >> platforms until upstream makes a direction decision. >> > > Yeah, it's been a bit of a chicken and egg in that this has been > blocking Ion getting out of staging but we don't actually have > in-tree users because it's still in staging. > I could post some of our Ion exporter patches anyway, might not have a chance at getting in while it's still in staging but could show there are users trying. Kinda the reason I posted the unmapped heap. The OP-TEE folks have been using it out-of-tree waiting for Ion destaging, but without more users/examples upstream it seems to hold back destaging work in the first place.. >>> Thanks for the background thread link,
Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap
On 1/18/19 3:43 PM, Liam Mark wrote:
> On Fri, 18 Jan 2019, Andrew F. Davis wrote:
>
>> On 1/17/19 7:04 PM, Liam Mark wrote:
>>> On Thu, 17 Jan 2019, Andrew F. Davis wrote:
>>>
On 1/16/19 4:48 PM, Liam Mark wrote:
> On Wed, 16 Jan 2019, Andrew F. Davis wrote:
>
>> On 1/15/19 1:05 PM, Laura Abbott wrote:
>>> On 1/15/19 10:38 AM, Andrew F. Davis wrote:
On 1/15/19 11:45 AM, Liam Mark wrote:
> On Tue, 15 Jan 2019, Andrew F. Davis wrote:
>
>> On 1/14/19 11:13 AM, Liam Mark wrote:
>>> On Fri, 11 Jan 2019, Andrew F. Davis wrote:
>>>
Buffers may not be mapped from the CPU so skip cache maintenance
here.
Accesses from the CPU to a cached heap should be bracketed with
{begin,end}_cpu_access calls so maintenance should not be needed
anyway.
Signed-off-by: Andrew F. Davis
---
drivers/staging/android/ion/ion.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 14e48f6eb734..09cb5a8e2b09 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -261,8 +261,8 @@ static struct sg_table *ion_map_dma_buf(struct
dma_buf_attachment *attachment,
table = a->table;
- if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
- direction))
+ if (!dma_map_sg_attrs(attachment->dev, table->sgl,
table->nents,
+ direction, DMA_ATTR_SKIP_CPU_SYNC))
>>>
>>> Unfortunately I don't think you can do this for a couple reasons.
>>> You can't rely on {begin,end}_cpu_access calls to do cache
>>> maintenance.
>>> If the calls to {begin,end}_cpu_access were made before the call to
>>> dma_buf_attach then there won't have been a device attached so the
>>> calls
>>> to {begin,end}_cpu_access won't have done any cache maintenance.
>>>
>>
>> That should be okay though, if you have no attachments (or all
>> attachments are IO-coherent) then there is no need for cache
>> maintenance. Unless you mean a sequence where a non-io-coherent
>> device
>> is attached later after data has already been written. Does that
>> sequence need supporting?
>
> Yes, but also I think there are cases where CPU access can happen
> before
> in Android, but I will focus on later for now.
>
>> DMA-BUF doesn't have to allocate the backing
>> memory until map_dma_buf() time, and that should only happen after
>> all
>> the devices have attached so it can know where to put the buffer. So
>> we
>> shouldn't expect any CPU access to buffers before all the devices are
>> attached and mapped, right?
>>
>
> Here is an example where CPU access can happen later in Android.
>
> Camera device records video -> software post processing -> video
> device
> (who does compression of raw data) and writes to a file
>
> In this example assume the buffer is cached and the devices are not
> IO-coherent (quite common).
>
This is the start of the problem, having cached mappings of memory that
is also being accessed non-coherently is going to cause issues one way
or another. On top of the speculative cache fills that have to be
constantly fought back against with CMOs like below; some coherent
interconnects behave badly when you mix coherent and non-coherent
access
(snoop filters get messed up).
The solution is to either always have the addresses marked non-coherent
(like device memory, no-map carveouts), or if you really want to use
regular system memory allocated at runtime, then all cached mappings of
it need to be dropped, even the kernel logical address (area as painful
as that would be).
>>>
>>> I agree it's broken, hence my desire to remove it :)
>>>
>>> The other problem is that uncached buffers are being used for
>>> performance reason so anything that would involve getting
>>> rid of the logical address would probably negate any performance
>>> benefit.
>>>
>>
>> I wouldn't go as far as to remove them just yet.. Liam seems pretty
>> adamant that they have valid uses. I'm just not sure performance is one
>> of them, maybe in the case of software locks between devices or
>> something where there needs to be a lot of back and forth interleaved
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
Philipp,
On Mon, Jan 21, 2019 at 12:49:10PM +0100, Philipp Zabel wrote:
> Hi,
>
> On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
> > Disable the SMFC before disabling the IDMA channel, instead of after,
> > in csi_idmac_unsetup().
> >
> > This fixes a complete system hard lockup on the SabreAuto when streaming
> > from the ADV7180, by repeatedly sending a stream off immediately followed
> > by stream on:
> >
> > while true; do v4l2-ctl -d4 --stream-mmap --stream-count=3; done
> >
> > Eventually this either causes the system lockup or EOF timeouts at all
> > subsequent stream on, until a system reset.
> >
> > The lockup occurs when disabling the IDMA channel at stream off. Stopping
> > the video data stream entering the IDMA channel before disabling the
> > channel itself appears to be a reliable fix for the hard lockup. That can
> > be done either by disabling the SMFC or the CSI before disabling the
> > channel. Disabling the SMFC before the channel is the easiest solution,
> > so do that.
> >
> > Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
> >
> > Suggested-by: Peter Seiderer
> > Reported-by: Gaël PORTAY
> > Signed-off-by: Steve Longerbeam
>
> Gaël, could we get a Tested-by: for this as well?
>
I have not tested the v3 yet. I have planned to do it later this day for
a all night testing and report the result then.
Gael
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] Drivers: hv: vmbus: Expose counters for interrupts and full conditions
On Thu, Jan 17, 2019 at 09:11:03AM -0800, Stephen Hemminger wrote:
>
>
> > +static ssize_t channel_intr_in_full_show(const struct vmbus_channel
> > *channel,
> > +char *buf)
> > +{
> > + return sprintf(buf, "%llu\n", channel->intr_in_full);
> > +}
>
>
> intr_in_full is u64, which is not the same as unsigned long long.
> to be correct you need a cast here.
>
Thanks for the feedback. I'll fix this issue in all four of the "_show"
functions that are added in this patch.
> > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > > index dcb6977afce9..7e5239123276 100644
> > > --- a/include/linux/hyperv.h
> > > +++ b/include/linux/hyperv.h
> > > @@ -751,6 +751,27 @@ struct vmbus_channel {
> > > u64 interrupts; /* Host to Guest interrupts */
> > > u64 sig_events; /* Guest to Host events */
> > >
> > > + /* Interrupt counts for 2 types of Guest to Host interrupts */
> > > + u64 intr_in_full; /* in ring buffer, full to not full */
> > > + u64 intr_out_empty; /* out ring buffer, empty to not empty */
> > > +
> > > + /*
> > > + * The total number of write operations that encountered a full
> > > + * outbound ring buffer.
> > > + */
> > > + u64 out_full_total;
> > > + /*
> > > + * The number of write operations that were the first to encounter a
> > > + * full outbound ring buffer.
> > > + */
> > > + u64 out_full_first;
>
> Adding more fields changes cache layout which can cause
> additional cache miss in the hot path.
>
Good point. I think that the "intr_out_empty" field is in a good
location, but the "intr_in_full", "out_full_first", and "out_full_total"
fields could be moved to the end of the struct. These variables are used
only when ring buffer full conditions occur. Ring buffer full conditions
shouldn't be encountered often, and, if they are, they're a signal that
changes should probably be made to prevent them.
If you have any other suggestions for this, please let me know.
> > > + /*
> > > + * Indicates that a full outbound ring buffer was encountered. The flag
> > > + * is set to true when a full outbound ring buffer is encountered and
> > > + * set to false when a write to the outbound ring buffer is completed.
> > > + */
> > > + bool out_full_flag;
>
> Discussion on kernel mailing list. Recommends against putting bool
> in structures since that pads to full sizeof(int). Could this be
> part of a bitfield?
>
There are currently 4 other bool variables in this struct. Maybe some or
all of the bool variables could be placed adjacent to each other and
changed into bitfields. I'll need to look into this.
> > > /* Channel callback's invoked in softirq context */
> > > struct tasklet_struct callback_event;
> > > void (*onchannel_callback)(void *context);
> > > @@ -936,6 +957,23 @@ static inline void *get_per_channel_state(struct
> > > vmbus_channel *c)
> > > static inline void set_channel_pending_send_size(struct vmbus_channel *c,
> > >u32 size)
> > > {
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&c->outbound.ring_lock, flags);
> > > +
> > > + if (size) {
> > > + ++c->out_full_total;
> > > +
> > > + if (!c->out_full_flag) {
> > > + ++c->out_full_first;
> > > + c->out_full_flag = true;
> > > + }
> > > + } else {
> > > + c->out_full_flag = false;
> > > + }
> > > +
> > > + spin_unlock_irqrestore(&c->outbound.ring_lock, flags);
>
> If this is called often, the additional locking will impact performance.
>
In hv_sock, each call of "hvs_stream_has_space()" results in a call to
"channel_set_pending_send_size()", so this could be a concern. I'll work
on addressing this issue.
> > > c->outbound.ring_buffer->pending_send_sz = size;
> > > }
> > >
>
> Could I propose another alternative.
>
> It might be more useful to count the guest to host interaction events
> rather than the ring buffer.
>
> For example the number of calls to:
> vmbus_set_event which means host exit call
> vmbus_setevent fastpath using sync_set_bit
> calls to rinbuffer_write that returned -EAGAIN
>
> These would require less locking, reuse existing code paths
> and not require additional state.
>
I'm not sure that this approach would provide the data that we're
looking for. For example, we're interested in evaluating how often ring
buffer write operations encounter full conditions. For this, we need to
know how many interaction events were caused by the ring buffer being
full. Counting the number of calls to "vmbus_set_event()" and
"vmbus_setevent()" wouldn't allow us to determine what caused the events.
For counting the full conditions, the number of calls to
"ring_buffer_write()" that returned -EAGAIN isn't sufficient because
hv_sock doesn't use the -EAGAIN path to determine that the ring buffer is
full. Therefore, we need to count the number of full conditions in both
"ring_buffe
Re: [PATCH 1/4] media: imx: csi: Allow unknown nearest upstream entities
On Sat, 2019-01-19 at 13:45 -0800, Steve Longerbeam wrote:
> On i.MX6, the nearest upstream entity to the CSI can only be the
> CSI video muxes or the Synopsys DW MIPI CSI-2 receiver.
>
> However the i.MX53 has no CSI video muxes or a MIPI CSI-2 receiver.
> So allow for the nearest upstream entity to the CSI to be something
> other than those.
>
> Fixes: bf3cfaa712e5c ("media: staging/imx: get CSI bus type from nearest
> upstream entity")
>
> Signed-off-by: Steve Longerbeam
> Cc: sta...@vger.kernel.org
> ---
> drivers/staging/media/imx/imx-media-csi.c | 18 ++
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 555aa45e02e3..b9af7d3d4974 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -154,9 +154,10 @@ static inline bool requires_passthrough(struct
> v4l2_fwnode_endpoint *ep,
> /*
> * Parses the fwnode endpoint from the source pad of the entity
> * connected to this CSI. This will either be the entity directly
> - * upstream from the CSI-2 receiver, or directly upstream from the
> - * video mux. The endpoint is needed to determine the bus type and
> - * bus config coming into the CSI.
> + * upstream from the CSI-2 receiver, directly upstream from the
> + * video mux, or directly upstream from the CSI itself. The endpoint
> + * is needed to determine the bus type and bus config coming into
> + * the CSI.
> */
> static int csi_get_upstream_endpoint(struct csi_priv *priv,
>struct v4l2_fwnode_endpoint *ep)
> @@ -172,7 +173,8 @@ static int csi_get_upstream_endpoint(struct csi_priv
> *priv,
> if (!priv->src_sd)
> return -EPIPE;
>
> - src = &priv->src_sd->entity;
> + sd = priv->src_sd;
> + src = &sd->entity;
>
> if (src->function == MEDIA_ENT_F_VID_MUX) {
> /*
> @@ -186,6 +188,14 @@ static int csi_get_upstream_endpoint(struct csi_priv
> *priv,
> src = &sd->entity;
> }
>
> + /*
> + * If the source is neither the video mux nor the CSI-2 receiver,
> + * get the source pad directly upstream from CSI itself.
> + */
> + if (src->function != MEDIA_ENT_F_VID_MUX &&
Will it work correctly if there's an external MUX connected to the CSI?
> + sd->grp_id != IMX_MEDIA_GRP_ID_CSI2)
> + src = &priv->sd.entity;
> +
> /* get source pad of entity directly upstream from src */
> pad = imx_media_find_upstream_pad(priv->md, src, 0);
> if (IS_ERR(pad))
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] media: imx: Rename functions that add IPU-internal subdevs/links
On Sat, 2019-01-19 at 13:45 -0800, Steve Longerbeam wrote: > For the functions that add and remove the internal IPU subdevice > descriptors and links between them, rename them to make clear they > are the subdevs and links internal to the IPU. Also rename the > platform data structure for the internal IPU subdevices. > No functional changes. > > Signed-off-by: Steve Longerbeam Acked-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] media: imx: Don't register IPU subdevs/links if CSI port missing
On Sat, 2019-01-19 at 13:46 -0800, Steve Longerbeam wrote:
> The second IPU internal sub-devices were being registered and links
> to them created even when the second IPU is not present. This is wrong
> for i.MX6 S/DL and i.MX53 which have only a single IPU.
>
> Fixes: e130291212df5 ("[media] media: Add i.MX media core driver")
>
> Signed-off-by: Steve Longerbeam
Reviewed-by: Philipp Zabel
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] staging: rtl8712: drop pointless static qualifier in r8712_efuse_pg_packet_write()
On 1/21/19 1:53 AM, YueHaibing wrote: There is no need to have the 'intrepeat_times' variable static since new value always be assigned before use it. Signed-off-by: YueHaibing --- drivers/staging/rtl8712/rtl8712_efuse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8712/rtl8712_efuse.c b/drivers/staging/rtl8712/rtl8712_efuse.c index 8bc45ff..39eb743 100644 --- a/drivers/staging/rtl8712/rtl8712_efuse.c +++ b/drivers/staging/rtl8712/rtl8712_efuse.c @@ -358,7 +358,7 @@ u8 r8712_efuse_pg_packet_write(struct _adapter *padapter, const u8 offset, u8 pg_header = 0; u16 efuse_addr = 0, curr_size = 0; u8 efuse_data, target_word_cnts = 0; - static int repeat_times; + int repeat_times; int sub_repeat; u8 bResult = true; Clearly, the value of this variable is not intended to be carried over between calls to this routine. Your fix is correct. Acked-by: Larry Finger Thanks, Larry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
On 1/21/19 3:49 AM, Philipp Zabel wrote:
Hi,
On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
Disable the SMFC before disabling the IDMA channel, instead of after,
in csi_idmac_unsetup().
This fixes a complete system hard lockup on the SabreAuto when streaming
from the ADV7180, by repeatedly sending a stream off immediately followed
by stream on:
while true; do v4l2-ctl -d4 --stream-mmap --stream-count=3; done
Eventually this either causes the system lockup or EOF timeouts at all
subsequent stream on, until a system reset.
The lockup occurs when disabling the IDMA channel at stream off. Stopping
the video data stream entering the IDMA channel before disabling the
channel itself appears to be a reliable fix for the hard lockup. That can
be done either by disabling the SMFC or the CSI before disabling the
channel. Disabling the SMFC before the channel is the easiest solution,
so do that.
Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
Suggested-by: Peter Seiderer
Reported-by: Gaël PORTAY
Signed-off-by: Steve Longerbeam
Gaël, could we get a Tested-by: for this as well?
Cc: sta...@vger.kernel.org
---
Changes in v3:
- switch from disabling the CSI before the channel to disabling the
SMFC before the channel.
Changes in v2:
- restore an empty line
- Add Fixes: and Cc: stable
---
drivers/staging/media/imx/imx-media-csi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index e18f58f56dfb..8610027eac97 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -560,8 +560,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
static void csi_idmac_unsetup(struct csi_priv *priv,
enum vb2_buffer_state state)
{
- ipu_idmac_disable_channel(priv->idmac_ch);
ipu_smfc_disable(priv->smfc);
+ ipu_idmac_disable_channel(priv->idmac_ch);
Steve, do you have any theory why this helps? It's a bit weird to
disable the SMFC module while the DMA channel is still enabled.
It does fix the hang, but I only have a vague theory as to why. That by
disabling the DMA channel while its internal FIFO is getting filled is
causing the hang, maybe due to a simultaneous update of the channel's
internal FIFO write pointer and the channel disable bit. By disabling
the SMFC (or the CSI), writes to the channel's internal FIFO stop.
I think
this warrants a big comment, given that enable order is SMFC_EN before
IDMAC channel enable.
Also ipu_smfc_disable is refcounted, so if the other CSI is capturing
simultaneously, this change has no effect.
Sigh, you're right. Let me go back to disabling the CSI before the
channel, the CSI enable/disable is not refcounted (it doesn't need to be
since it is single use) so it doesn't have this problem.
Should we drop this patch or keep it (with a big comment)? By only
changing the disable order to "CSI then channel", the hang is reliably
fixed from my and Gael's testing, but my concern is that by not
disabling the SMFC before the channel, the SMFC could still empty its
FIFO to the channel's internal FIFO and still create a hang.
Steve
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
On 1/21/19 10:43 AM, Steve Longerbeam wrote: On 1/21/19 3:49 AM, Philipp Zabel wrote: Also ipu_smfc_disable is refcounted, so if the other CSI is capturing simultaneously, this change has no effect. Sigh, you're right. Let me go back to disabling the CSI before the channel, the CSI enable/disable is not refcounted (it doesn't need to be since it is single use) so it doesn't have this problem. Should we drop this patch or keep it (with a big comment)? By only changing the disable order to "CSI then channel", the hang is reliably fixed from my and Gael's testing, but my concern is that by not disabling the SMFC before the channel, the SMFC could still empty its FIFO to the channel's internal FIFO and still create a hang. Well, as you said it will have no effect if both CSI's are streaming with the SMFC, in which case the danger would still exist. Perhaps it would be best to just drop this patch. Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Investment Proposal of € 7.5M
Hello I want to seek for your assistance and partnership to invest in your country. I have € 7.5M (Seven million five hundred thousand Euros) that I want to invest in your country and I am willing to offer you 20% of the money for your assistance in this project. If you are interested, kindly reply me as soon as you read my mail for more details. I am waiting for your response Sincerely Lisa Herrera ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Investment Proposal of € 7.5M
Hello I want to seek for your assistance and partnership to invest in your country. I have € 7.5M (Seven million five hundred thousand Euros) that I want to invest in your country and I am willing to offer you 20% of the money for your assistance in this project. If you are interested, kindly reply me as soon as you read my mail for more details. I am waiting for your response Sincerely Lisa Herrera ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Investment Proposal of € 7.5M
Hello I want to seek for your assistance and partnership to invest in your country. I have € 7.5M (Seven million five hundred thousand Euros) that I want to invest in your country and I am willing to offer you 20% of the money for your assistance in this project. If you are interested, kindly reply me as soon as you read my mail for more details. I am waiting for your response Sincerely Lisa Herrera ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Mon, 21 Jan 2019, Christoph Hellwig wrote: > On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: > > > And who is going to decide which ones to pass? And who documents > > > which ones are safe? > > > > > > I'd much rather have explicit, well documented dma-buf flags that > > > might get translated to the DMA API flags, which are not error checked, > > > not very well documented and way to easy to get wrong. > > > > > > > I'm not sure having flags in dma-buf really solves anything > > given drivers can use the attributes directly with dma_map > > anyway, which is what we're looking to do. The intention > > is for the driver creating the dma_buf attachment to have > > the knowledge of which flags to use. > > Well, there are very few flags that you can simply use for all calls of > dma_map*. And given how badly these flags are defined I just don't want > people to add more places where they indirectly use these flags, as > it will be more than enough work to clean up the current mess. > > What flag(s) do you want to pass this way, btw? Maybe that is where > the problem is. > The main use case is for allowing clients to pass in DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In ION the buffers aren't usually accessed from the CPU so this allows clients to often avoid doing unnecessary cache maintenance. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On 1/21/19 1:44 PM, Liam Mark wrote: > On Mon, 21 Jan 2019, Christoph Hellwig wrote: > >> On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: And who is going to decide which ones to pass? And who documents which ones are safe? I'd much rather have explicit, well documented dma-buf flags that might get translated to the DMA API flags, which are not error checked, not very well documented and way to easy to get wrong. >>> >>> I'm not sure having flags in dma-buf really solves anything >>> given drivers can use the attributes directly with dma_map >>> anyway, which is what we're looking to do. The intention >>> is for the driver creating the dma_buf attachment to have >>> the knowledge of which flags to use. >> >> Well, there are very few flags that you can simply use for all calls of >> dma_map*. And given how badly these flags are defined I just don't want >> people to add more places where they indirectly use these flags, as >> it will be more than enough work to clean up the current mess. >> >> What flag(s) do you want to pass this way, btw? Maybe that is where >> the problem is. >> > > The main use case is for allowing clients to pass in > DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > ION the buffers aren't usually accessed from the CPU so this allows > clients to often avoid doing unnecessary cache maintenance. > How can a client know that no CPU access has occurred that needs to be flushed out? > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap
On Mon, 21 Jan 2019, Andrew F. Davis wrote:
> On 1/18/19 3:43 PM, Liam Mark wrote:
> > On Fri, 18 Jan 2019, Andrew F. Davis wrote:
> >
> >> On 1/17/19 7:04 PM, Liam Mark wrote:
> >>> On Thu, 17 Jan 2019, Andrew F. Davis wrote:
> >>>
> On 1/16/19 4:48 PM, Liam Mark wrote:
> > On Wed, 16 Jan 2019, Andrew F. Davis wrote:
> >
> >> On 1/15/19 1:05 PM, Laura Abbott wrote:
> >>> On 1/15/19 10:38 AM, Andrew F. Davis wrote:
> On 1/15/19 11:45 AM, Liam Mark wrote:
> > On Tue, 15 Jan 2019, Andrew F. Davis wrote:
> >
> >> On 1/14/19 11:13 AM, Liam Mark wrote:
> >>> On Fri, 11 Jan 2019, Andrew F. Davis wrote:
> >>>
> Buffers may not be mapped from the CPU so skip cache maintenance
> here.
> Accesses from the CPU to a cached heap should be bracketed with
> {begin,end}_cpu_access calls so maintenance should not be needed
> anyway.
>
> Signed-off-by: Andrew F. Davis
> ---
> drivers/staging/android/ion/ion.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c
> index 14e48f6eb734..09cb5a8e2b09 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -261,8 +261,8 @@ static struct sg_table
> *ion_map_dma_buf(struct
> dma_buf_attachment *attachment,
> table = a->table;
> - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> - direction))
> + if (!dma_map_sg_attrs(attachment->dev, table->sgl,
> table->nents,
> + direction, DMA_ATTR_SKIP_CPU_SYNC))
> >>>
> >>> Unfortunately I don't think you can do this for a couple reasons.
> >>> You can't rely on {begin,end}_cpu_access calls to do cache
> >>> maintenance.
> >>> If the calls to {begin,end}_cpu_access were made before the call
> >>> to
> >>> dma_buf_attach then there won't have been a device attached so the
> >>> calls
> >>> to {begin,end}_cpu_access won't have done any cache maintenance.
> >>>
> >>
> >> That should be okay though, if you have no attachments (or all
> >> attachments are IO-coherent) then there is no need for cache
> >> maintenance. Unless you mean a sequence where a non-io-coherent
> >> device
> >> is attached later after data has already been written. Does that
> >> sequence need supporting?
> >
> > Yes, but also I think there are cases where CPU access can happen
> > before
> > in Android, but I will focus on later for now.
> >
> >> DMA-BUF doesn't have to allocate the backing
> >> memory until map_dma_buf() time, and that should only happen after
> >> all
> >> the devices have attached so it can know where to put the buffer.
> >> So we
> >> shouldn't expect any CPU access to buffers before all the devices
> >> are
> >> attached and mapped, right?
> >>
> >
> > Here is an example where CPU access can happen later in Android.
> >
> > Camera device records video -> software post processing -> video
> > device
> > (who does compression of raw data) and writes to a file
> >
> > In this example assume the buffer is cached and the devices are not
> > IO-coherent (quite common).
> >
>
> This is the start of the problem, having cached mappings of memory
> that
> is also being accessed non-coherently is going to cause issues one
> way
> or another. On top of the speculative cache fills that have to be
> constantly fought back against with CMOs like below; some coherent
> interconnects behave badly when you mix coherent and non-coherent
> access
> (snoop filters get messed up).
>
> The solution is to either always have the addresses marked
> non-coherent
> (like device memory, no-map carveouts), or if you really want to use
> regular system memory allocated at runtime, then all cached mappings
> of
> it need to be dropped, even the kernel logical address (area as
> painful
> as that would be).
>
> >>>
> >>> I agree it's broken, hence my desire to remove it :)
> >>>
> >>> The other problem is that uncached buffers are being used for
> >>> performance reason so anything that would involve getting
> >>> rid of the logical address would probably negate an
Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap
On Fri, 18 Jan 2019, Andrew F. Davis wrote:
> On 1/17/19 7:11 PM, Liam Mark wrote:
> > On Thu, 17 Jan 2019, Andrew F. Davis wrote:
> >
> >> On 1/16/19 4:54 PM, Liam Mark wrote:
> >>> On Wed, 16 Jan 2019, Andrew F. Davis wrote:
> >>>
> On 1/16/19 9:19 AM, Brian Starkey wrote:
> > Hi :-)
> >
> > On Tue, Jan 15, 2019 at 12:40:16PM -0600, Andrew F. Davis wrote:
> >> On 1/15/19 12:38 PM, Andrew F. Davis wrote:
> >>> On 1/15/19 11:45 AM, Liam Mark wrote:
> On Tue, 15 Jan 2019, Andrew F. Davis wrote:
>
> > On 1/14/19 11:13 AM, Liam Mark wrote:
> >> On Fri, 11 Jan 2019, Andrew F. Davis wrote:
> >>
> >>> Buffers may not be mapped from the CPU so skip cache maintenance
> >>> here.
> >>> Accesses from the CPU to a cached heap should be bracketed with
> >>> {begin,end}_cpu_access calls so maintenance should not be needed
> >>> anyway.
> >>>
> >>> Signed-off-by: Andrew F. Davis
> >>> ---
> >>> drivers/staging/android/ion/ion.c | 7 ---
> >>> 1 file changed, 4 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/android/ion/ion.c
> >>> b/drivers/staging/android/ion/ion.c
> >>> index 14e48f6eb734..09cb5a8e2b09 100644
> >>> --- a/drivers/staging/android/ion/ion.c
> >>> +++ b/drivers/staging/android/ion/ion.c
> >>> @@ -261,8 +261,8 @@ static struct sg_table
> >>> *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> >>>
> >>> table = a->table;
> >>>
> >>> - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> >>> - direction))
> >>> + if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> >>> + direction, DMA_ATTR_SKIP_CPU_SYNC))
> >>
> >> Unfortunately I don't think you can do this for a couple reasons.
> >> You can't rely on {begin,end}_cpu_access calls to do cache
> >> maintenance.
> >> If the calls to {begin,end}_cpu_access were made before the call
> >> to
> >> dma_buf_attach then there won't have been a device attached so the
> >> calls
> >> to {begin,end}_cpu_access won't have done any cache maintenance.
> >>
> >
> > That should be okay though, if you have no attachments (or all
> > attachments are IO-coherent) then there is no need for cache
> > maintenance. Unless you mean a sequence where a non-io-coherent
> > device
> > is attached later after data has already been written. Does that
> > sequence need supporting?
>
> Yes, but also I think there are cases where CPU access can happen
> before
> in Android, but I will focus on later for now.
>
> > DMA-BUF doesn't have to allocate the backing
> > memory until map_dma_buf() time, and that should only happen after
> > all
> > the devices have attached so it can know where to put the buffer.
> > So we
> > shouldn't expect any CPU access to buffers before all the devices
> > are
> > attached and mapped, right?
> >
>
> Here is an example where CPU access can happen later in Android.
>
> Camera device records video -> software post processing -> video
> device
> (who does compression of raw data) and writes to a file
>
> In this example assume the buffer is cached and the devices are not
> IO-coherent (quite common).
>
> >>>
> >>> This is the start of the problem, having cached mappings of memory
> >>> that
> >>> is also being accessed non-coherently is going to cause issues one way
> >>> or another. On top of the speculative cache fills that have to be
> >>> constantly fought back against with CMOs like below; some coherent
> >>> interconnects behave badly when you mix coherent and non-coherent
> >>> access
> >>> (snoop filters get messed up).
> >>>
> >>> The solution is to either always have the addresses marked
> >>> non-coherent
> >>> (like device memory, no-map carveouts), or if you really want to use
> >>> regular system memory allocated at runtime, then all cached mappings
> >>> of
> >>> it need to be dropped, even the kernel logical address (area as
> >>> painful
> >>> as that would be).
> >
> > Ouch :-( I wasn't aware about these potential interconnect issues. How
> > "real" is that? It seems that we aren't really hitting that today on
> > real devices.
> >
>
> Sadly there is at least one real device like this now (TI AM654). We
> spent some time working with the ARM interconnect spec designers to see
> if this was allowed behavior, final
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Mon, 21 Jan 2019, Andrew F. Davis wrote: > On 1/21/19 1:44 PM, Liam Mark wrote: > > On Mon, 21 Jan 2019, Christoph Hellwig wrote: > > > >> On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: > And who is going to decide which ones to pass? And who documents > which ones are safe? > > I'd much rather have explicit, well documented dma-buf flags that > might get translated to the DMA API flags, which are not error checked, > not very well documented and way to easy to get wrong. > > >>> > >>> I'm not sure having flags in dma-buf really solves anything > >>> given drivers can use the attributes directly with dma_map > >>> anyway, which is what we're looking to do. The intention > >>> is for the driver creating the dma_buf attachment to have > >>> the knowledge of which flags to use. > >> > >> Well, there are very few flags that you can simply use for all calls of > >> dma_map*. And given how badly these flags are defined I just don't want > >> people to add more places where they indirectly use these flags, as > >> it will be more than enough work to clean up the current mess. > >> > >> What flag(s) do you want to pass this way, btw? Maybe that is where > >> the problem is. > >> > > > > The main use case is for allowing clients to pass in > > DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > > which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > > ION the buffers aren't usually accessed from the CPU so this allows > > clients to often avoid doing unnecessary cache maintenance. > > > > How can a client know that no CPU access has occurred that needs to be > flushed out? > I have left this to clients, but if they own the buffer they can have the knowledge as to whether CPU access is needed in that use case (example for post-processing). For example with the previous version of ION we left all decisions of whether cache maintenance was required up to the client, they would use the ION cache maintenance IOCTL to force cache maintenance only when it was required. In these cases almost all of the access was being done by the device and in the rare cases CPU access was required clients would initiate the required cache maintenance before and after the CPU access. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On 1/21/19 2:20 PM, Liam Mark wrote: > On Mon, 21 Jan 2019, Andrew F. Davis wrote: > >> On 1/21/19 1:44 PM, Liam Mark wrote: >>> On Mon, 21 Jan 2019, Christoph Hellwig wrote: >>> On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: >> And who is going to decide which ones to pass? And who documents >> which ones are safe? >> >> I'd much rather have explicit, well documented dma-buf flags that >> might get translated to the DMA API flags, which are not error checked, >> not very well documented and way to easy to get wrong. >> > > I'm not sure having flags in dma-buf really solves anything > given drivers can use the attributes directly with dma_map > anyway, which is what we're looking to do. The intention > is for the driver creating the dma_buf attachment to have > the knowledge of which flags to use. Well, there are very few flags that you can simply use for all calls of dma_map*. And given how badly these flags are defined I just don't want people to add more places where they indirectly use these flags, as it will be more than enough work to clean up the current mess. What flag(s) do you want to pass this way, btw? Maybe that is where the problem is. >>> >>> The main use case is for allowing clients to pass in >>> DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance >>> which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In >>> ION the buffers aren't usually accessed from the CPU so this allows >>> clients to often avoid doing unnecessary cache maintenance. >>> >> >> How can a client know that no CPU access has occurred that needs to be >> flushed out? >> > > I have left this to clients, but if they own the buffer they can have the > knowledge as to whether CPU access is needed in that use case (example for > post-processing). > > For example with the previous version of ION we left all decisions of > whether cache maintenance was required up to the client, they would use > the ION cache maintenance IOCTL to force cache maintenance only when it > was required. > In these cases almost all of the access was being done by the device and > in the rare cases CPU access was required clients would initiate the > required cache maintenance before and after the CPU access. > I think we have different definitions of "client", I'm talking about the DMA-BUF client (the importer), that is who can set this flag. It seems you mean the userspace application, which has no control over this flag. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap
On 1/21/19 5:22 AM, Brian Starkey wrote:
> Hi,
>
> Sorry for being a bit sporadic on this. I was out travelling last week
> with little time for email.
>
> On Fri, Jan 18, 2019 at 11:16:31AM -0600, Andrew F. Davis wrote:
>> On 1/17/19 7:11 PM, Liam Mark wrote:
>>> On Thu, 17 Jan 2019, Andrew F. Davis wrote:
>>>
On 1/16/19 4:54 PM, Liam Mark wrote:
> On Wed, 16 Jan 2019, Andrew F. Davis wrote:
>
>> On 1/16/19 9:19 AM, Brian Starkey wrote:
>>> Hi :-)
>>>
>>> On Tue, Jan 15, 2019 at 12:40:16PM -0600, Andrew F. Davis wrote:
On 1/15/19 12:38 PM, Andrew F. Davis wrote:
> On 1/15/19 11:45 AM, Liam Mark wrote:
>> On Tue, 15 Jan 2019, Andrew F. Davis wrote:
>>
>>> On 1/14/19 11:13 AM, Liam Mark wrote:
On Fri, 11 Jan 2019, Andrew F. Davis wrote:
> Buffers may not be mapped from the CPU so skip cache maintenance
> here.
> Accesses from the CPU to a cached heap should be bracketed with
> {begin,end}_cpu_access calls so maintenance should not be needed
> anyway.
>
> Signed-off-by: Andrew F. Davis
> ---
> drivers/staging/android/ion/ion.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c
> index 14e48f6eb734..09cb5a8e2b09 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -261,8 +261,8 @@ static struct sg_table
> *ion_map_dma_buf(struct dma_buf_attachment *attachment,
>
> table = a->table;
>
> - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> - direction))
> + if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> + direction, DMA_ATTR_SKIP_CPU_SYNC))
Unfortunately I don't think you can do this for a couple reasons.
You can't rely on {begin,end}_cpu_access calls to do cache
maintenance.
If the calls to {begin,end}_cpu_access were made before the call
to
dma_buf_attach then there won't have been a device attached so the
calls
to {begin,end}_cpu_access won't have done any cache maintenance.
>>>
>>> That should be okay though, if you have no attachments (or all
>>> attachments are IO-coherent) then there is no need for cache
>>> maintenance. Unless you mean a sequence where a non-io-coherent
>>> device
>>> is attached later after data has already been written. Does that
>>> sequence need supporting?
>>
>> Yes, but also I think there are cases where CPU access can happen
>> before
>> in Android, but I will focus on later for now.
>>
>>> DMA-BUF doesn't have to allocate the backing
>>> memory until map_dma_buf() time, and that should only happen after
>>> all
>>> the devices have attached so it can know where to put the buffer.
>>> So we
>>> shouldn't expect any CPU access to buffers before all the devices
>>> are
>>> attached and mapped, right?
>>>
>>
>> Here is an example where CPU access can happen later in Android.
>>
>> Camera device records video -> software post processing -> video
>> device
>> (who does compression of raw data) and writes to a file
>>
>> In this example assume the buffer is cached and the devices are not
>> IO-coherent (quite common).
>>
>
> This is the start of the problem, having cached mappings of memory
> that
> is also being accessed non-coherently is going to cause issues one way
> or another. On top of the speculative cache fills that have to be
> constantly fought back against with CMOs like below; some coherent
> interconnects behave badly when you mix coherent and non-coherent
> access
> (snoop filters get messed up).
>
> The solution is to either always have the addresses marked
> non-coherent
> (like device memory, no-map carveouts), or if you really want to use
> regular system memory allocated at runtime, then all cached mappings
> of
> it need to be dropped, even the kernel logical address (area as
> painful
> as that would be).
>>>
>>> Ouch :-( I wasn't aware about these potential interconnect issues. How
>>> "real" is that? It seems that we aren't really hitting that today on
>>> real devices.
>>>
>>
>> Sadly there i
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Mon, Jan 21, 2019 at 11:44:10AM -0800, Liam Mark wrote: > The main use case is for allowing clients to pass in > DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > ION the buffers aren't usually accessed from the CPU so this allows > clients to often avoid doing unnecessary cache maintenance. This can't work. The cpu can still easily speculate into this area. Moreover in general these operations should be cheap if the addresses aren't cached. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Mon, Jan 21, 2019 at 12:20:42PM -0800, Liam Mark wrote: > I have left this to clients, but if they own the buffer they can have the > knowledge as to whether CPU access is needed in that use case (example for > post-processing). That is an API design which the user is more likely to get wrong than right and thus does not pass the smell test. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Mon, 21 Jan 2019, Christoph Hellwig wrote: > On Mon, Jan 21, 2019 at 11:44:10AM -0800, Liam Mark wrote: > > The main use case is for allowing clients to pass in > > DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > > which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > > ION the buffers aren't usually accessed from the CPU so this allows > > clients to often avoid doing unnecessary cache maintenance. > > This can't work. The cpu can still easily speculate into this area. Can you provide more detail on your concern here. The use case I am thinking about here is a cached buffer which is accessed by a non IO-coherent device (quite a common use case for ION). Guessing on your concern: The speculative access can be an issue if you are going to access the buffer from the CPU after the device has written to it, however if you know you aren't going to do any CPU access before the buffer is again returned to the device then I don't think the speculative access is a concern. > Moreover in general these operations should be cheap if the addresses > aren't cached. > I am thinking of use cases with cached buffers here, so CMO isn't cheap. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Mon, 21 Jan 2019, Christoph Hellwig wrote: > On Mon, Jan 21, 2019 at 12:20:42PM -0800, Liam Mark wrote: > > I have left this to clients, but if they own the buffer they can have the > > knowledge as to whether CPU access is needed in that use case (example for > > post-processing). > > That is an API design which the user is more likely to get wrong than > right and thus does not pass the smell test. > With the previous version of ION Android ION clients were successfully managing all their cache maintenance. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Mon, 21 Jan 2019, Andrew F. Davis wrote: > On 1/21/19 2:20 PM, Liam Mark wrote: > > On Mon, 21 Jan 2019, Andrew F. Davis wrote: > > > >> On 1/21/19 1:44 PM, Liam Mark wrote: > >>> On Mon, 21 Jan 2019, Christoph Hellwig wrote: > >>> > On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: > >> And who is going to decide which ones to pass? And who documents > >> which ones are safe? > >> > >> I'd much rather have explicit, well documented dma-buf flags that > >> might get translated to the DMA API flags, which are not error checked, > >> not very well documented and way to easy to get wrong. > >> > > > > I'm not sure having flags in dma-buf really solves anything > > given drivers can use the attributes directly with dma_map > > anyway, which is what we're looking to do. The intention > > is for the driver creating the dma_buf attachment to have > > the knowledge of which flags to use. > > Well, there are very few flags that you can simply use for all calls of > dma_map*. And given how badly these flags are defined I just don't want > people to add more places where they indirectly use these flags, as > it will be more than enough work to clean up the current mess. > > What flag(s) do you want to pass this way, btw? Maybe that is where > the problem is. > > >>> > >>> The main use case is for allowing clients to pass in > >>> DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > >>> which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > >>> ION the buffers aren't usually accessed from the CPU so this allows > >>> clients to often avoid doing unnecessary cache maintenance. > >>> > >> > >> How can a client know that no CPU access has occurred that needs to be > >> flushed out? > >> > > > > I have left this to clients, but if they own the buffer they can have the > > knowledge as to whether CPU access is needed in that use case (example for > > post-processing). > > > > For example with the previous version of ION we left all decisions of > > whether cache maintenance was required up to the client, they would use > > the ION cache maintenance IOCTL to force cache maintenance only when it > > was required. > > In these cases almost all of the access was being done by the device and > > in the rare cases CPU access was required clients would initiate the > > required cache maintenance before and after the CPU access. > > > > I think we have different definitions of "client", I'm talking about the > DMA-BUF client (the importer), that is who can set this flag. It seems > you mean the userspace application, which has no control over this flag. > I am also talking about dma-buf clients, I am referring to both the userspace and kernel component of the client. For example our Camera ION client has both a usersapce and kernel component and they have ION buffers, which they control the access to, which may or may not be accessed by the CPU in certain uses cases. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 2/3] media: imx: csi: Stop upstream before disabling IDMA channel
Move upstream stream off to just after receiving the last EOF completion
and disabling the CSI (and thus before disabling the IDMA channel) in
csi_stop(). For symmetry also move upstream stream on to beginning of
csi_start().
Doing this makes csi_s_stream() more symmetric with prp_s_stream() which
will require the same change to fix a hard lockup.
Signed-off-by: Steve Longerbeam
Cc: sta...@vger.kernel.org
---
drivers/staging/media/imx/imx-media-csi.c | 25 ---
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 920e38885292..d851ca2497b4 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -753,10 +753,16 @@ static int csi_start(struct csi_priv *priv)
output_fi = &priv->frame_interval[priv->active_output_pad];
+ /* start upstream */
+ ret = v4l2_subdev_call(priv->src_sd, video, s_stream, 1);
+ ret = (ret && ret != -ENOIOCTLCMD) ? ret : 0;
+ if (ret)
+ return ret;
+
if (priv->dest == IPU_CSI_DEST_IDMAC) {
ret = csi_idmac_start(priv);
if (ret)
- return ret;
+ goto stop_upstream;
}
ret = csi_setup(priv);
@@ -784,6 +790,8 @@ static int csi_start(struct csi_priv *priv)
idmac_stop:
if (priv->dest == IPU_CSI_DEST_IDMAC)
csi_idmac_stop(priv);
+stop_upstream:
+ v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
return ret;
}
@@ -799,6 +807,9 @@ static void csi_stop(struct csi_priv *priv)
*/
ipu_csi_disable(priv->csi);
+ /* stop upstream */
+ v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
+
if (priv->dest == IPU_CSI_DEST_IDMAC) {
csi_idmac_stop(priv);
@@ -966,23 +977,13 @@ static int csi_s_stream(struct v4l2_subdev *sd, int
enable)
goto update_count;
if (enable) {
- /* upstream must be started first, before starting CSI */
- ret = v4l2_subdev_call(priv->src_sd, video, s_stream, 1);
- ret = (ret && ret != -ENOIOCTLCMD) ? ret : 0;
- if (ret)
- goto out;
-
dev_dbg(priv->dev, "stream ON\n");
ret = csi_start(priv);
- if (ret) {
- v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
+ if (ret)
goto out;
- }
} else {
dev_dbg(priv->dev, "stream OFF\n");
- /* CSI must be stopped first, then stop upstream */
csi_stop(priv);
- v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
}
update_count:
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 1/3] media: imx: csi: Disable CSI immediately after last EOF
Disable the CSI immediately after receiving the last EOF before stream
off (and thus before disabling the IDMA channel). Do this by moving the
wait for EOF completion into a new function csi_idmac_wait_last_eof().
This fixes a complete system hard lockup on the SabreAuto when streaming
from the ADV7180, by repeatedly sending a stream off immediately followed
by stream on:
while true; do v4l2-ctl -d4 --stream-mmap --stream-count=3; done
Eventually this either causes the system lockup or EOF timeouts at all
subsequent stream on, until a system reset.
The lockup occurs when disabling the IDMA channel at stream off. Disabling
the CSI before disabling the IDMA channel appears to be a reliable fix for
the hard lockup.
Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
Reported-by: Gaël PORTAY
Signed-off-by: Steve Longerbeam
Cc: sta...@vger.kernel.org
---
Changes in v4:
- Disabling SMFC will have no effect if both CSI's are streaming. So
go back to disabling CSI before channel as in v2, but split up
csi_idmac_stop such that ipu_csi_disable can still be called within
csi_stop.
Changes in v3:
- switch from disabling the CSI before the channel to disabling the
SMFC before the channel.
Changes in v2:
- restore an empty line
- Add Fixes: and Cc: stable
---
drivers/staging/media/imx/imx-media-csi.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 7abfe0aa1418..920e38885292 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -662,7 +662,7 @@ static int csi_idmac_start(struct csi_priv *priv)
return ret;
}
-static void csi_idmac_stop(struct csi_priv *priv)
+static void csi_idmac_wait_last_eof(struct csi_priv *priv)
{
unsigned long flags;
int ret;
@@ -679,7 +679,10 @@ static void csi_idmac_stop(struct csi_priv *priv)
&priv->last_eof_comp, msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
if (ret == 0)
v4l2_warn(&priv->sd, "wait last EOF timeout\n");
+}
+static void csi_idmac_stop(struct csi_priv *priv)
+{
devm_free_irq(priv->dev, priv->eof_irq, priv);
devm_free_irq(priv->dev, priv->nfb4eof_irq, priv);
@@ -786,6 +789,16 @@ static int csi_start(struct csi_priv *priv)
static void csi_stop(struct csi_priv *priv)
{
+ if (priv->dest == IPU_CSI_DEST_IDMAC)
+ csi_idmac_wait_last_eof(priv);
+
+ /*
+* Disable the CSI asap, after syncing with the last EOF.
+* Doing so after the IDMA channel is disabled has shown to
+* create hard system-wide hangs.
+*/
+ ipu_csi_disable(priv->csi);
+
if (priv->dest == IPU_CSI_DEST_IDMAC) {
csi_idmac_stop(priv);
@@ -793,8 +806,6 @@ static void csi_stop(struct csi_priv *priv)
if (priv->fim)
imx_media_fim_set_stream(priv->fim, NULL, false);
}
-
- ipu_csi_disable(priv->csi);
}
static const struct csi_skip_desc csi_skip[12] = {
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 3/3] media: imx: prpencvf: Stop upstream before disabling IDMA channel
Upstream must be stopped immediately after receiving the last EOF and
before disabling the IDMA channel. This can be accomplished by moving
upstream stream off to just after receiving the last EOF completion in
prp_stop(). For symmetry also move upstream stream on to end of
prp_start().
This fixes a complete system hard lockup on the SabreAuto when streaming
from the ADV7180, by repeatedly sending a stream off immediately followed
by stream on:
while true; do v4l2-ctl -d1 --stream-mmap --stream-count=3; done
Eventually this either causes the system lockup or EOF timeouts at all
subsequent stream on, until a system reset.
The lockup occurs when disabling the IDMA channel at stream off. Stopping
the video data stream entering the IDMA channel before disabling the
channel itself appears to be a reliable fix for the hard lockup.
Fixes: f0d9c8924e2c3 ("[media] media: imx: Add IC subdev drivers")
Reported-by: Gaël PORTAY
Tested-by: Gaël PORTAY
Signed-off-by: Steve Longerbeam
Cc: sta...@vger.kernel.org
---
Changes in v4:
- none.
Changes in v3:
- Reword the commit subject and message. No functional changes.
Changes in v2:
- Add Fixes: and Cc: stable
---
drivers/staging/media/imx/imx-ic-prpencvf.c | 26 ++---
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c
b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 053a911d477a..3637693c2bc8 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -706,12 +706,23 @@ static int prp_start(struct prp_priv *priv)
goto out_free_nfb4eof_irq;
}
+ /* start upstream */
+ ret = v4l2_subdev_call(priv->src_sd, video, s_stream, 1);
+ ret = (ret && ret != -ENOIOCTLCMD) ? ret : 0;
+ if (ret) {
+ v4l2_err(&ic_priv->sd,
+"upstream stream on failed: %d\n", ret);
+ goto out_free_eof_irq;
+ }
+
/* start the EOF timeout timer */
mod_timer(&priv->eof_timeout_timer,
jiffies + msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
return 0;
+out_free_eof_irq:
+ devm_free_irq(ic_priv->dev, priv->eof_irq, priv);
out_free_nfb4eof_irq:
devm_free_irq(ic_priv->dev, priv->nfb4eof_irq, priv);
out_unsetup:
@@ -743,6 +754,12 @@ static void prp_stop(struct prp_priv *priv)
if (ret == 0)
v4l2_warn(&ic_priv->sd, "wait last EOF timeout\n");
+ /* stop upstream */
+ ret = v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
+ if (ret && ret != -ENOIOCTLCMD)
+ v4l2_warn(&ic_priv->sd,
+ "upstream stream off failed: %d\n", ret);
+
devm_free_irq(ic_priv->dev, priv->eof_irq, priv);
devm_free_irq(ic_priv->dev, priv->nfb4eof_irq, priv);
@@ -1173,15 +1190,6 @@ static int prp_s_stream(struct v4l2_subdev *sd, int
enable)
if (ret)
goto out;
- /* start/stop upstream */
- ret = v4l2_subdev_call(priv->src_sd, video, s_stream, enable);
- ret = (ret && ret != -ENOIOCTLCMD) ? ret : 0;
- if (ret) {
- if (enable)
- prp_stop(priv);
- goto out;
- }
-
update_count:
priv->stream_count += enable ? 1 : -1;
if (priv->stream_count < 0)
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/4] media: imx: csi: Allow unknown nearest upstream entities
On 1/21/19 8:50 AM, Philipp Zabel wrote:
On Sat, 2019-01-19 at 13:45 -0800, Steve Longerbeam wrote:
On i.MX6, the nearest upstream entity to the CSI can only be the
CSI video muxes or the Synopsys DW MIPI CSI-2 receiver.
However the i.MX53 has no CSI video muxes or a MIPI CSI-2 receiver.
So allow for the nearest upstream entity to the CSI to be something
other than those.
Fixes: bf3cfaa712e5c ("media: staging/imx: get CSI bus type from nearest
upstream entity")
Signed-off-by: Steve Longerbeam
Cc: sta...@vger.kernel.org
---
drivers/staging/media/imx/imx-media-csi.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 555aa45e02e3..b9af7d3d4974 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -154,9 +154,10 @@ static inline bool requires_passthrough(struct
v4l2_fwnode_endpoint *ep,
/*
* Parses the fwnode endpoint from the source pad of the entity
* connected to this CSI. This will either be the entity directly
- * upstream from the CSI-2 receiver, or directly upstream from the
- * video mux. The endpoint is needed to determine the bus type and
- * bus config coming into the CSI.
+ * upstream from the CSI-2 receiver, directly upstream from the
+ * video mux, or directly upstream from the CSI itself. The endpoint
+ * is needed to determine the bus type and bus config coming into
+ * the CSI.
*/
static int csi_get_upstream_endpoint(struct csi_priv *priv,
struct v4l2_fwnode_endpoint *ep)
@@ -172,7 +173,8 @@ static int csi_get_upstream_endpoint(struct csi_priv *priv,
if (!priv->src_sd)
return -EPIPE;
- src = &priv->src_sd->entity;
+ sd = priv->src_sd;
+ src = &sd->entity;
if (src->function == MEDIA_ENT_F_VID_MUX) {
/*
@@ -186,6 +188,14 @@ static int csi_get_upstream_endpoint(struct csi_priv *priv,
src = &sd->entity;
}
+ /*
+* If the source is neither the video mux nor the CSI-2 receiver,
+* get the source pad directly upstream from CSI itself.
+*/
+ if (src->function != MEDIA_ENT_F_VID_MUX &&
Will it work correctly if there's an external MUX connected to the CSI?
By external MUX are you referring to some MUX that's external to the SoC
(e.g. not the CSI muxes which are external to the IPU but internal to
the SoC)? If so then yes it will still work (and of course it works if
the MUX in question is the CSI muxes).
The function csi_get_upstream_endpoint() is only looking for, and stops
at, the first sub-device that is external to the SoC, in order to
determine the bus type coming into the SoC of the currently enabled
pipeline. And that sub-device could be anything, including another MUX.
Steve
+ sd->grp_id != IMX_MEDIA_GRP_ID_CSI2)
+ src = &priv->sd.entity;
+
/* get source pad of entity directly upstream from src */
pad = imx_media_find_upstream_pad(priv->md, src, 0);
if (IS_ERR(pad))
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Drivers: hv: vmbus: Add mutex lock to channel show functions
The channel level "_show" functions are vulnerable to race conditions.
Add a mutex lock and unlock around the call to the channel level "_show"
functions in vmbus_chan_attr_show().
This problem was discussed here: https://lkml.org/lkml/2018/10/18/830
Signed-off-by: Kimberly Brown
---
drivers/hv/vmbus_drv.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 403fee01572c..e8189bc168da 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1414,6 +1414,7 @@ static ssize_t vmbus_chan_attr_show(struct kobject *kobj,
= container_of(attr, struct vmbus_chan_attribute, attr);
const struct vmbus_channel *chan
= container_of(kobj, struct vmbus_channel, kobj);
+ ssize_t ret;
if (!attribute->show)
return -EIO;
@@ -1421,7 +1422,10 @@ static ssize_t vmbus_chan_attr_show(struct kobject *kobj,
if (chan->state != CHANNEL_OPENED_STATE)
return -EINVAL;
- return attribute->show(chan, buf);
+ mutex_lock(&vmbus_connection.channel_mutex);
+ ret = attribute->show(chan, buf);
+ mutex_unlock(&vmbus_connection.channel_mutex);
+ return ret;
}
static const struct sysfs_ops vmbus_chan_sysfs_ops = {
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show functions
> From: Kimberly Brown
> Sent: Monday, January 21, 2019 6:08 PM
> Subject: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show functions
>
> The channel level "_show" functions are vulnerable to race conditions.
> Add a mutex lock and unlock around the call to the channel level "_show"
> functions in vmbus_chan_attr_show().
>
> This problem was discussed here:
> https://lkml.org/lkml/2018/10/18/830
>
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1414,6 +1414,7 @@ static ssize_t vmbus_chan_attr_show(struct kobject
> *kobj,
> = container_of(attr, struct vmbus_chan_attribute, attr);
> const struct vmbus_channel *chan
> = container_of(kobj, struct vmbus_channel, kobj);
> + ssize_t ret;
>
> if (!attribute->show)
> return -EIO;
> @@ -1421,7 +1422,10 @@ static ssize_t vmbus_chan_attr_show(struct
> kobject *kobj,
> if (chan->state != CHANNEL_OPENED_STATE)
> return -EINVAL;
>
> - return attribute->show(chan, buf);
> + mutex_lock(&vmbus_connection.channel_mutex);
> + ret = attribute->show(chan, buf);
> + mutex_unlock(&vmbus_connection.channel_mutex);
> + return ret;
> }
It looks this patch is already able to fix the original issue:
6712cc9c2211 ("vmbus: don't return values for uninitalized channels"),
as chan->state can't be CHANNEL_OPENED_STATE when
channel->ringbuffer_page is not set up yet, or has been freed.
Thanks,
-- Dexuan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show functions
On Tue, Jan 22, 2019 at 03:46:48AM +, Dexuan Cui wrote:
> > From: Kimberly Brown
> > Sent: Monday, January 21, 2019 6:08 PM
> > Subject: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show
> > functions
> >
> > The channel level "_show" functions are vulnerable to race conditions.
> > Add a mutex lock and unlock around the call to the channel level "_show"
> > functions in vmbus_chan_attr_show().
> >
> > This problem was discussed here:
> > https://lkml.org/lkml/2018/10/18/830
> >
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1414,6 +1414,7 @@ static ssize_t vmbus_chan_attr_show(struct kobject
> > *kobj,
> > = container_of(attr, struct vmbus_chan_attribute, attr);
> > const struct vmbus_channel *chan
> > = container_of(kobj, struct vmbus_channel, kobj);
> > + ssize_t ret;
> >
> > if (!attribute->show)
> > return -EIO;
> > @@ -1421,7 +1422,10 @@ static ssize_t vmbus_chan_attr_show(struct
> > kobject *kobj,
> > if (chan->state != CHANNEL_OPENED_STATE)
> > return -EINVAL;
> >
> > - return attribute->show(chan, buf);
> > + mutex_lock(&vmbus_connection.channel_mutex);
> > + ret = attribute->show(chan, buf);
> > + mutex_unlock(&vmbus_connection.channel_mutex);
> > + return ret;
> > }
>
> It looks this patch is already able to fix the original issue:
> 6712cc9c2211 ("vmbus: don't return values for uninitalized channels"),
> as chan->state can't be CHANNEL_OPENED_STATE when
> channel->ringbuffer_page is not set up yet, or has been freed.
>
> Thanks,
> -- Dexuan
I think that patch 6712cc9c2211 fixes the problem when the channel is
not set up yet, but it doesn't fix the problem when the channel is being
closed. The channel could be freed after the check that "chan->state" is
CHANNEL_OPENED_STATE, while the "attribute->show()" function is running.
Actually, there should be checks that "chan" is not null and that
"chan->state" is CHANNEL_OPENED_STATE within the locked section. I'll
need to fix that.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
