Re: [PATCH v3 14/15] fs/files: export close_fd() symbol

[Al Viro]

On Thu, Sep 07, 2017 at 06:22:45PM -0300, Gustavo Padovan wrote:

> Sorry for my lack of knowledge here and thank you for the explanation,
> things are a lot clear to me. For some reasons I were trying to delay
> the sharing of the fd to a event later. I can delay the install of it
> but that my require __fd_install() to be available and exportedi as it
> may happen in a thread, but I believe you wouldn't be okay with that either,
> is that so?

Only if it has been given a reference to descriptor table to start with.
Which reference should've been acquired by the target process itself.

Why bother, anyway?  You need to handle the case when the stream has
ended just after you'd copied the value to userland; at that point you
obviously can't go hunting for all references to struct file in question,
so you have to guaratee that methods will start giving an error from
that point on.  What's the problem with just leaving it installed?

Both userland and kernel must cope with that sort of thing anyway, so
what does removing it from descriptor table and not reporting it buy
you?  AFAICS, it's an extra layer of complexity for no good reason -
you are not getting it offset by simplifications anywhere else...

Re: [PATCH v3 14/15] fs/files: export close_fd() symbol

[Al Viro]

On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Rename __close_fd() to close_fd() and export it to be able close files
> in modules using file descriptors.
> 
> The usecase that motivates this change happens in V4L2 where we send
> events to userspace with a fd that has file installed in it. But if for
> some reason we have to cancel the video stream we need to close the files
> that haven't been shared with userspace yet. Thus the export of
> close_fd() becomes necessary.
> 
> fd_install() happens when we call an ioctl to queue a buffer, but we only
> share the fd with userspace later, and that may happen in a kernel thread
> instead.

NAK.  As soon as the reference is in descriptor table, you *can't* do anything
to it.  This "sharing" part is complete BS - being _told_ that descriptor is
there does not matter at all.  That descriptor might be hit with dup2() as
soon as fd_install() has happened.  Or be closed, or any number of other things.

You can not take it back.  Once fd_install() is done, it's fucking done, period.
If V4L2 requires removing it from descriptor table, it's a shitty API and needs
to be fixed.

Again, NAK.

Re: [RFC 0/6] Module for tracking/accounting shared memory buffers

[Al Viro]

On Tue, Oct 11, 2016 at 04:50:04PM -0700, Ruchi Kandoi wrote:

> memtrack maintains a per-process list of shared buffer references, which is
> exported to userspace as /proc/[pid]/memtrack.  Buffers can be optionally
> "tagged" with a short string: for example, Android userspace would use this
> tag to identify whether buffers were allocated on behalf of the camera stack,
> GL, etc.  memtrack also exports the VMAs associated with these buffers so
> that pages already included in the process's mm counters aren't 
> double-counted.
> 
> Shared-buffer allocators can hook into memtrack by embedding
> struct memtrack_buffer in their buffer metadata, calling
> memtrack_buffer_{init,remove} at buffer allocation and free time, and
> memtrack_buffer_{install,uninstall} when a userspace process takes or
> drops a reference to the buffer.  For fd-backed buffers like dma-bufs, hooks 
> in
> fdtable.c and fork.c automatically notify memtrack when references are added 
> or
> removed from a process's fd table.
> 
> This patchstack adds memtrack hooks into dma-buf and ion.  If there's upstream
> interest in memtrack, it can be extended to other memory allocators as well,
> such as GEM implementations.

No, with a side of Hell, No.  Not to mention anything else,
* descriptor tables do not belong to any specific task_struct and
actions done by one show up in all who share that thing.
* shared descriptor table does not imply belonging to the same
group.
* shared descriptor table can become unshared at any point, invisibly
for that Fine Piece Of Software.
* while we are at it, blocking allocation under several spinlocks
(and with interrupts disabled, for good measure) is generally considered
a bloody bad idea.

That - just from the quick look through that patchset.  Bringing task_struct
into the API is already sufficient for a NAK.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] remove lots of IS_ERR_VALUE abuses

[Al Viro]

On Fri, May 27, 2016 at 11:23:25PM +0200, Arnd Bergmann wrote:

> @@ -837,7 +837,7 @@ static int load_flat_shared_library(int id, struct 
> lib_info *libs)
>  
>   res = prepare_binprm();
>  
> - if (!IS_ERR_VALUE(res))
> + if (res >= 0)

if (res == 0), please - prepare_binprm() returns 0 or -E...

> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -521,7 +521,7 @@ static int p9_check_errors(struct p9_client *c, struct 
> p9_req_t *req)
>   if (p9_is_proto_dotu(c))
>   err = -ecode;
>  
> - if (!err || !IS_ERR_VALUE(err)) {
> + if (!err || !IS_ERR_VALUE((unsigned long)err)) {

Not really - it's actually
if (p9_is_proto_dotu(c) && ecode < 512)
err = -ecode;

if (!err) {
...
> @@ -608,7 +608,7 @@ static int p9_check_zc_errors(struct p9_client *c, struct 
> p9_req_t *req,
>   if (p9_is_proto_dotu(c))
>   err = -ecode;
>  
> - if (!err || !IS_ERR_VALUE(err)) {
> + if (!err || !IS_ERR_VALUE((unsigned long)err)) {

Ditto.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][davinci] ccdc_update_raw_params() frees the wrong thing

[Al Viro]

On Tue, Jan 05, 2016 at 05:37:06PM +, Lad, Prabhakar wrote:
> On Sun, Dec 13, 2015 at 12:32 AM, Al Viro <v...@zeniv.linux.org.uk> wrote:
> > Passing a physical address to free_pages() is a bad idea.
> > config_params->fault_pxl.fpc_table_addr is set to virt_to_phys()
> > of __get_free_pages() return value; what we should pass to free_pages()
> > is its phys_to_virt().  ccdc_close() does that properly, but
> > ccdc_update_raw_params() doesn't.
> >
> > Signed-off-by: Al Viro <v...@zeniv.linux.org.uk>
> >
> Acked-by: Lad, Prabhakar <prabhakar.cse...@gmail.com>
> 
> Regards,
> --Prabhakar Lad

Which tree should it go through?  I can certainly put that into
vfs.git#work.misc, but it looks like a better fit for linux-media tree, or
the davinci-specific one...
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH][davinci] ccdc_update_raw_params() frees the wrong thing

[Al Viro]

Passing a physical address to free_pages() is a bad idea.
config_params->fault_pxl.fpc_table_addr is set to virt_to_phys()
of __get_free_pages() return value; what we should pass to free_pages()
is its phys_to_virt().  ccdc_close() does that properly, but
ccdc_update_raw_params() doesn't.

Signed-off-by: Al Viro <v...@zeniv.linux.org.uk>

diff --git a/drivers/media/platform/davinci/dm644x_ccdc.c 
b/drivers/media/platform/davinci/dm644x_ccdc.c
index ffbefdf..6fba32b 100644
--- a/drivers/media/platform/davinci/dm644x_ccdc.c
+++ b/drivers/media/platform/davinci/dm644x_ccdc.c
@@ -261,7 +261,7 @@ static int ccdc_update_raw_params(struct 
ccdc_config_params_raw *raw_params)
 */
if (raw_params->fault_pxl.fp_num != config_params->fault_pxl.fp_num) {
if (fpc_physaddr != NULL) {
-   free_pages((unsigned long)fpc_physaddr,
+   free_pages((unsigned long)fpc_virtaddr,
   get_order
   (config_params->fault_pxl.fp_num *
   FP_NUM_BYTES));
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ->poll() instances shouldn't be indefinitely blocking

[Al Viro]

ase types)
sound/core/pcm_native.c:3152:24:expected restricted __poll_t
sound/core/pcm_native.c:3152:24:got int
sound/core/pcm_native.c:3152:24: warning: incorrect type in return expression 
(different base types)
sound/core/pcm_native.c:3191:24:expected restricted __poll_t
sound/core/pcm_native.c:3191:24:got int
sound/core/pcm_native.c:3191:24: warning: incorrect type in return expression 
(different base types)
--- not sure, hadn't looked in detail.
sound/core/seq/oss/seq_oss.c:203:24:expected restricted __poll_t
sound/core/seq/oss/seq_oss.c:203:24:got int
sound/core/seq/oss/seq_oss.c:203:24: warning: incorrect type in return 
expression (different base types)
--- impossible to hit without struct file being corrupted
sound/core/seq/seq_clientmgr.c:1102:24:expected restricted __poll_t
sound/core/seq/seq_clientmgr.c:1102:24:got int
sound/core/seq/seq_clientmgr.c:1102:24: warning: incorrect type in return 
expression (different base types)
--- ditto
drivers/gpu/vga/vgaarb.c:1169:24: warning: incorrect type in return expression 
(different base types)
drivers/gpu/vga/vgaarb.c:1169:24:expected restricted __poll_t
drivers/gpu/vga/vgaarb.c:1169:24:got int
--- ditto

That's on amd64/allmodconfig build with the annotations I'd done for
->poll().  They also have caught a couple of dubious places in net/9p,
still looking in there.  FWIW, the typical impact of annotations on a
driver is something like
@@ -2155,11 +2155,11 @@ pmu_write(struct file *file, const char __user *buf,
return 0;
 }
 
-static unsigned int
+static __poll_t
 pmu_fpoll(struct file *filp, poll_table *wait)
 {
struct pmu_private *pp = filp->private_data;
-   unsigned int mask = 0;
+   __poll_t mask = 0;
unsigned long flags;

if (pp == 0)

IOW, not really intrusive.  make C=2 CF="-D__CHECK_ENDIAN -D__CHECK_POLL__"
right now, might as well make poll annotations trigger on __CHECK_ENDIAN__
alone - there's not much noise left.  Again, I have no problem with
adding "if the MSB is set, it's probably a broken driver returning -E...
instead of the valid bitmap, let's warn about it", but IMO it's better
dealt with by sparse at build time; bogus values are not returned on
common paths, so the runtime test coverage won't be good.

See vfs.git#work.poll-annotations for the current state of that queue.  The
last commit needs to be split and folded back into the relevant commits
before - the queue was originally done back in March and last week I'd
ported it to current mainline, the last commit being the annotations
in the code that had been added since then.

The current summary follows:
Shortlog:
Al Viro (20):
  switch poll.h to generic-y where possible
  annotate POLL... constants and ->poll() return value
  annotate poll_table_struct->_key and poll_table_entry->key
  annotate wake_..._poll() last argument
  fs: annotate ->poll() instances
  annotate proto_ops ->poll()
  annotate proto_ops ->poll() instances
  net: annotate file_operations ->poll() instances
  kernel: annotate ->poll() instances
  annotate posix clock
  annotate security/
  annotate mm/
  annotate ipc/
  annotate drivers/media and its users
  annotate sound/
  annotate tty
  annotate assorted drivers
  VFS annotations
  annotations around wakeup callbacks
  missing ->poll() annotations [splitme]

Diffstat:
 arch/alpha/include/asm/Kbuild  |  1 +
 arch/alpha/include/uapi/asm/poll.h |  1 -
 arch/blackfin/include/uapi/asm/poll.h  |  4 +--
 arch/cris/arch-v10/drivers/gpio.c  |  6 ++--
 arch/cris/arch-v10/drivers/sync_serial.c   |  8 ++---
 arch/cris/arch-v32/drivers/sync_serial.c   |  8 ++---
 arch/frv/include/uapi/asm/poll.h   |  2 +-
 arch/ia64/include/asm/Kbuild   |  1 +
 arch/ia64/include/uapi/asm/poll.h  |  1 -
 arch/ia64/kernel/perfmon.c |  4 +--
 arch/m32r/include/asm/Kbuild   |  1 +
 arch/m32r/include/uapi/asm/poll.h  |  1 -
 arch/m68k/include/uapi/asm/poll.h  |  2 +-
 arch/microblaze/include/asm/Kbuild |  1 +
 arch/microblaze/include/uapi/asm/poll.h|  1 -
 arch/mips/include/uapi/asm/poll.h  |  2 +-
 arch/mips/kernel/rtlx.c|  4 +--
 arch/mn10300/include/asm/Kbuild|  1 +
 arch/mn10300/include/uapi/asm/poll.h   |  1 -
 arch/powerpc/include/asm/Kbuild|  1 +
 arch/powerpc/include/uapi/asm/poll.h   |  1 -
 arch/powerpc/kernel/rtasd.c|  2 +-
 arch/powerpc/platforms/cell/spufs/file.c   | 16 +-
 arch/powerpc/platforms/powernv/opal-prd.c  |  2 +-
 arch/s390/include/asm/Kbuild   |

videobuf_vm_{open,close} race fixes

[Al Viro]

just use videobuf_queue_lock(map-q) to protect map-count; vm_area_operations
-open() and -close() are called just under vma-vm_mm-mmap_sem, which
doesn't help the drivers at all, since clonal VMAs are normally in different
address spaces...

Signed-off-by: Al Viro v...@zeniv.linux.org.uk
---
WARNING: it's only build-tested

diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c 
b/drivers/media/v4l2-core/videobuf-dma-contig.c
index 67f572c3..8204c88 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
@@ -66,11 +66,14 @@ static void __videobuf_dc_free(struct device *dev,
 static void videobuf_vm_open(struct vm_area_struct *vma)
 {
struct videobuf_mapping *map = vma-vm_private_data;
+   struct videobuf_queue *q = map-q;
 
-   dev_dbg(map-q-dev, vm_open %p [count=%u,vma=%08lx-%08lx]\n,
+   dev_dbg(q-dev, vm_open %p [count=%u,vma=%08lx-%08lx]\n,
map, map-count, vma-vm_start, vma-vm_end);
 
+   videobuf_queue_lock(q);
map-count++;
+   videobuf_queue_unlock(q);
 }
 
 static void videobuf_vm_close(struct vm_area_struct *vma)
@@ -82,12 +85,11 @@ static void videobuf_vm_close(struct vm_area_struct *vma)
dev_dbg(q-dev, vm_close %p [count=%u,vma=%08lx-%08lx]\n,
map, map-count, vma-vm_start, vma-vm_end);
 
-   map-count--;
-   if (0 == map-count) {
+   videobuf_queue_lock(q);
+   if (!--map-count) {
struct videobuf_dma_contig_memory *mem;
 
dev_dbg(q-dev, munmap %p q=%p\n, map, q);
-   videobuf_queue_lock(q);
 
/* We need first to cancel streams, before unmapping */
if (q-streaming)
@@ -126,8 +128,8 @@ static void videobuf_vm_close(struct vm_area_struct *vma)
 
kfree(map);
 
-   videobuf_queue_unlock(q);
}
+   videobuf_queue_unlock(q);
 }
 
 static const struct vm_operations_struct videobuf_vm_ops = {
diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c 
b/drivers/media/v4l2-core/videobuf-dma-sg.c
index 828e7c1..9db674c 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -338,11 +338,14 @@ EXPORT_SYMBOL_GPL(videobuf_dma_free);
 static void videobuf_vm_open(struct vm_area_struct *vma)
 {
struct videobuf_mapping *map = vma-vm_private_data;
+   struct videobuf_queue *q = map-q;
 
dprintk(2, vm_open %p [count=%d,vma=%08lx-%08lx]\n, map,
map-count, vma-vm_start, vma-vm_end);
 
+   videobuf_queue_lock(q);
map-count++;
+   videobuf_queue_unlock(q);
 }
 
 static void videobuf_vm_close(struct vm_area_struct *vma)
@@ -355,10 +358,9 @@ static void videobuf_vm_close(struct vm_area_struct *vma)
dprintk(2, vm_close %p [count=%d,vma=%08lx-%08lx]\n, map,
map-count, vma-vm_start, vma-vm_end);
 
-   map-count--;
-   if (0 == map-count) {
+   videobuf_queue_lock(q);
+   if (!--map-count) {
dprintk(1, munmap %p q=%p\n, map, q);
-   videobuf_queue_lock(q);
for (i = 0; i  VIDEO_MAX_FRAME; i++) {
if (NULL == q-bufs[i])
continue;
@@ -374,9 +376,9 @@ static void videobuf_vm_close(struct vm_area_struct *vma)
q-bufs[i]-baddr = 0;
q-ops-buf_release(q, q-bufs[i]);
}
-   videobuf_queue_unlock(q);
kfree(map);
}
+   videobuf_queue_unlock(q);
return;
 }
 
diff --git a/drivers/media/v4l2-core/videobuf-vmalloc.c 
b/drivers/media/v4l2-core/videobuf-vmalloc.c
index 2ff7fcc..1365c65 100644
--- a/drivers/media/v4l2-core/videobuf-vmalloc.c
+++ b/drivers/media/v4l2-core/videobuf-vmalloc.c
@@ -54,11 +54,14 @@ MODULE_LICENSE(GPL);
 static void videobuf_vm_open(struct vm_area_struct *vma)
 {
struct videobuf_mapping *map = vma-vm_private_data;
+   struct videobuf_queue *q = map-q;
 
dprintk(2, vm_open %p [count=%u,vma=%08lx-%08lx]\n, map,
map-count, vma-vm_start, vma-vm_end);
 
+   videobuf_queue_lock(q);
map-count++;
+   videobuf_queue_unlock(q);
 }
 
 static void videobuf_vm_close(struct vm_area_struct *vma)
@@ -70,12 +73,11 @@ static void videobuf_vm_close(struct vm_area_struct *vma)
dprintk(2, vm_close %p [count=%u,vma=%08lx-%08lx]\n, map,
map-count, vma-vm_start, vma-vm_end);
 
-   map-count--;
-   if (0 == map-count) {
+   videobuf_queue_lock(q);
+   if (!--map-count) {
struct videobuf_vmalloc_memory *mem;
 
dprintk(1, munmap %p q=%p\n, map, q);
-   videobuf_queue_lock(q);
 
/* We need first to cancel streams, before unmapping */
if (q-streaming)
@@ -114,8 +116,8 @@ static void videobuf_vm_close(struct vm_area_struct *vma)
 
kfree(map

Re: [PATCH] omap_vout: find_vma() needs -mmap_sem held

[Al Viro]

On Sun, Dec 16, 2012 at 09:01:10PM +0100, Paul Bolle wrote:
  +   vma = find_vma(mm, virtp);
  } else if (vma  (vma-vm_flags  VM_IO)  vma-vm_pgoff) {
 
 Shouldn't that line become 
 if (vma  (vma-vm_flags  VM_IO)  vma-vm_pgoff) {
 
 so that this actually compiles?

*Do'h*

Yes, it should.  Mea culpa...

Signed-off-by: Al Viro v...@zeniv.linux.org.uk
---
diff --git a/drivers/media/platform/omap/omap_vout.c 
b/drivers/media/platform/omap/omap_vout.c
index 9935040..cb564d0 100644
--- a/drivers/media/platform/omap/omap_vout.c
+++ b/drivers/media/platform/omap/omap_vout.c
@@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
struct vm_area_struct *vma;
struct mm_struct *mm = current-mm;
 
-   vma = find_vma(mm, virtp);
/* For kernel direct-mapped memory, take the easy way */
-   if (virtp = PAGE_OFFSET) {
-   physp = virt_to_phys((void *) virtp);
-   } else if (vma  (vma-vm_flags  VM_IO)  vma-vm_pgoff) {
+   if (virtp = PAGE_OFFSET)
+   return virt_to_phys((void *) virtp);
+
+   down_read(current-mm-mmap_sem);
+   vma = find_vma(mm, virtp);
+   if (vma  (vma-vm_flags  VM_IO)  vma-vm_pgoff) {
/* this will catch, kernel-allocated, mmaped-to-usermode
   addresses */
physp = (vma-vm_pgoff  PAGE_SHIFT) + (virtp - vma-vm_start);
+   up_read(current-mm-mmap_sem);
} else {
/* otherwise, use get_user_pages() for general userland pages */
int res, nr_pages = 1;
struct page *pages;
-   down_read(current-mm-mmap_sem);
 
res = get_user_pages(current, current-mm, virtp, nr_pages, 1,
0, pages, NULL);
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] omap_vout: find_vma() needs -mmap_sem held

[Al Viro]

Walking rbtree while it's modified is a Bad Idea(tm); besides,
the result of find_vma() can be freed just as it's getting returned
to caller.  Fortunately, it's easy to fix - just take -mmap_sem a bit
earlier (and don't bother with find_vma() at all if virtp = PAGE_OFFSET -
in that case we don't even look at its result).

Cc: sta...@vger.kernel.org [2.6.35]
Signed-off-by: Al Viro v...@zeniv.linux.org.uk
---
diff --git a/drivers/media/platform/omap/omap_vout.c 
b/drivers/media/platform/omap/omap_vout.c
index 9935040..984512f 100644
--- a/drivers/media/platform/omap/omap_vout.c
+++ b/drivers/media/platform/omap/omap_vout.c
@@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
struct vm_area_struct *vma;
struct mm_struct *mm = current-mm;
 
-   vma = find_vma(mm, virtp);
/* For kernel direct-mapped memory, take the easy way */
-   if (virtp = PAGE_OFFSET) {
-   physp = virt_to_phys((void *) virtp);
+   if (virtp = PAGE_OFFSET)
+   return virt_to_phys((void *) virtp);
+
+   down_read(current-mm-mmap_sem);
+   vma = find_vma(mm, virtp);
} else if (vma  (vma-vm_flags  VM_IO)  vma-vm_pgoff) {
/* this will catch, kernel-allocated, mmaped-to-usermode
   addresses */
physp = (vma-vm_pgoff  PAGE_SHIFT) + (virtp - vma-vm_start);
+   up_read(current-mm-mmap_sem);
} else {
/* otherwise, use get_user_pages() for general userland pages */
int res, nr_pages = 1;
struct page *pages;
-   down_read(current-mm-mmap_sem);
 
res = get_user_pages(current, current-mm, virtp, nr_pages, 1,
0, pages, NULL);
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap_vout: find_vma() needs -mmap_sem held

[Al Viro]

On Sat, Dec 15, 2012 at 08:12:37PM +, Al Viro wrote:
   Walking rbtree while it's modified is a Bad Idea(tm); besides,
 the result of find_vma() can be freed just as it's getting returned
 to caller.  Fortunately, it's easy to fix - just take -mmap_sem a bit
 earlier (and don't bother with find_vma() at all if virtp = PAGE_OFFSET -
 in that case we don't even look at its result).

While we are at it, what prevents VIDIOC_PREPARE_BUF calling
v4l_prepare_buf() - (e.g) vb2_ioctl_prepare_buf() - vb2_prepare_buf() -
__buf_prepare() - __qbuf_userptr() - vb2_vmalloc_get_userptr() - find_vma(),
AFAICS without having taken -mmap_sem anywhere in process?  The code flow
is bloody convoluted and depends on a bunch of things done by initialization,
so I certainly might've missed something...
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Al Viro]

On Wed, Oct 03, 2012 at 09:38:52AM -0700, Linus Torvalds wrote:
 Yeah, that bugzilla shows the problem with Kay as a maintainer too,
 not willing to own up to problems he caused.
 
 Can you actually see the problem? I did add the attached patch as an
 attachment to the bugzilla, so the reporter there may be able to test
 it, but it's been open for a long while..
 
 Anyway. Attached is a really stupid patch that tries to do the direct
 firmware load as suggested by Ivan. It has not been tested very
 extensively at all (but I did test that it loaded the brcmsmac
 firmware images on my laptop so it has the *potential* to work).

+   if (!S_ISREG(inode-i_mode))
+   return false;
+   size = i_size_read(inode);

Probably better to do vfs_getattr() and check mode and size in kstat; if
it's sufficiently hot for that to hurt, we are fucked anyway.

+   file = filp_open(path, O_RDONLY, 0);
+   if (IS_ERR(file))
+   continue;
+printk(from file '%s' , path);
+   success = fw_read_file_contents(file, fw);
+   filp_close(file, NULL);

fput(file), please.  We have enough misuses of filp_close() as it is...

 We are apparently better off trying to avoid udev like the plague.
 Doing something very similar to this for module loading is probably a
 good idea too.

That, or just adding usr/udev in the kernel git tree and telling the
vertical integrators to go kiss a lamprey...
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Al Viro]

On Wed, Oct 03, 2012 at 10:32:08AM -0700, Linus Torvalds wrote:
 On Wed, Oct 3, 2012 at 10:09 AM, Al Viro v...@zeniv.linux.org.uk wrote:
 
  +   if (!S_ISREG(inode-i_mode))
  +   return false;
  +   size = i_size_read(inode);
 
  Probably better to do vfs_getattr() and check mode and size in kstat; if
  it's sufficiently hot for that to hurt, we are fucked anyway.
 
  +   file = filp_open(path, O_RDONLY, 0);
  +   if (IS_ERR(file))
  +   continue;
  +printk(from file '%s' , path);
  +   success = fw_read_file_contents(file, fw);
  +   filp_close(file, NULL);
 
  fput(file), please.  We have enough misuses of filp_close() as it is...
 
 Ok, like this?

Looks sane.  TBH, I'd still prefer to see udev forcibly taken over and put into
usr/udev in kernel tree - I don't trust that crowd at all and the fewer
critical userland bits they can play leverage games with, the safer we are.  

Al, that -- close to volunteering for maintaining that FPOS kernel-side...
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Can you review or ack this patch?

[Al Viro]

On Tue, Aug 23, 2011 at 08:33:25AM +0200, Hans Verkuil wrote:
 (and resent again, this time with the correct linux-fsdevel mail address)
 (Resent as requested by Andrew Morton since this is still stuck)
 
 Hi Al, Andrew,
 
 Can you take a look at this patch and send an Ack or review comments?

Not at the moment - I'm half-asleep and tired as hell.  Will do in the
morning...
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html