Re: [REGRESSION v4.7] i915 / drm crash when undocking from DP monitors

2016-10-11 Thread Vadim Lobanov
Certainly, done and done. The bug is filed at:
https://bugs.freedesktop.org/show_bug.cgi?id=98211

Thanks!

On Tue, Oct 11, 2016 at 6:55 AM, Jani Nikula
 wrote:
> On Sat, 08 Oct 2016, Vadim Lobanov  wrote:
>> I'm seeing a repeatable crash on my HP EliteBook 840 G2/2216 when
>> booting it while in a docking station connected to two external
>> DisplayPort monitors, undocking, and then either logging out or
>> shutting down -- regardless of whether I've redocked it beforehand or
>> not. Both logout and shutdown work great if I do not undock the laptop
>> at all, so the badness correlates with the DP monitors going away.
>
> Please file a bug at [1] with the info in this mail, add drm.debug=14
> module parameter, and attach dmesg from boot to the problem into the
> bug.
>
> BR,
> Jani.
>
> [1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel
>
> --
> Jani Nikula, Intel Open Source Technology Center


[REGRESSION v4.7] i915 / drm crash when undocking from DP monitors

2016-10-07 Thread Vadim Lobanov
Hi folks,

I'm seeing a repeatable crash on my HP EliteBook 840 G2/2216 when
booting it while in a docking station connected to two external
DisplayPort monitors, undocking, and then either logging out or
shutting down -- regardless of whether I've redocked it beforehand or
not. Both logout and shutdown work great if I do not undock the laptop
at all, so the badness correlates with the DP monitors going away.

This is a regression introduced somewhere in the v4.6 -> v4.7
development timeframe: 4.6.0 works, 4.7.0 fails as described, and
4.8.0 crashes earlier still when undocking.

The graphics hardware involved is:

00:02.0 VGA compatible controller: Intel Corporation HD Graphics 5500
(rev 09) (prog-if 00 [VGA controller])
Subsystem: Hewlett-Packard Company ZBook 15u G2 Mobile Workstation
Flags: bus master, fast devsel, latency 0, IRQ 49
Memory at c000 (64-bit, non-prefetchable) [size=16M]
Memory at b000 (64-bit, prefetchable) [size=256M]
I/O ports at 5000 [size=64]
[virtual] Expansion ROM at 000c [disabled] [size=128K]
Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit-
Capabilities: [d0] Power Management version 2
Capabilities: [a4] PCI Advanced Features
Kernel driver in use: i915
Kernel modules: i915

And the crash that I see is similar to this:

Oct 07 17:47:16 localhost.localdomain kernel: BUG: unable to handle
kernel paging request at 00018c70
Oct 07 17:47:16 localhost.localdomain kernel: IP: []
queued_spin_lock_slowpath+0x108/0x190
Oct 07 17:47:16 localhost.localdomain kernel: PGD 0
Oct 07 17:47:16 localhost.localdomain kernel: Oops: 0002 [#1] SMP
Oct 07 17:47:16 localhost.localdomain kernel: Modules linked in:
rfcomm ccm xt_CHECKSUM tun ipt_MASQUERADE nf_nat_masquerade_ipv4
xt_addrtype nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_REJECT
nf_reject_
Oct 07 17:47:16 localhost.localdomain kernel:  sparse_keymap ppdev
irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel iwlwifi
intel_cstate intel_uncore intel_rapl_perf cfg80211 joydev uvcvideo
lpc_ich r
Oct 07 17:47:16 localhost.localdomain kernel: CPU: 2 PID: 855 Comm:
systemd-logind Not tainted 4.7.5-200.fc24.x86_64 #1
Oct 07 17:47:16 localhost.localdomain kernel: Hardware name:
Hewlett-Packard HP EliteBook 840 G2/2216, BIOS M71 Ver. 01.04
02/24/2015
Oct 07 17:47:16 localhost.localdomain kernel: task: 88043a12
ti: 880035d5 task.ti: 880035d5
Oct 07 17:47:16 localhost.localdomain kernel: RIP:
0010:[]  []
queued_spin_lock_slowpath+0x108/0x190
Oct 07 17:47:16 localhost.localdomain kernel: RSP:
0018:880035d53908  EFLAGS: 00010202
Oct 07 17:47:16 localhost.localdomain kernel: RAX: 00018c70
RBX: 880438716a50 RCX: 88044f498c40
Oct 07 17:47:16 localhost.localdomain kernel: RDX: 1b9a
RSI: 6e6f746f RDI: 880438716a54
Oct 07 17:47:16 localhost.localdomain kernel: RBP: 880035d53908
R08: 000c R09: 
Oct 07 17:47:16 localhost.localdomain kernel: R10: 880096e4e780
R11: 0898 R12: 88043ab3ec40
Oct 07 17:47:16 localhost.localdomain kernel: R13: 880438716a58
R14: 880427ebd800 R15: 8804396bd000
Oct 07 17:47:16 localhost.localdomain kernel: FS:
7f22e2cb5900() GS:88044f48()
knlGS:
Oct 07 17:47:16 localhost.localdomain kernel: CS:  0010 DS:  ES:
 CR0: 80050033
Oct 07 17:47:16 localhost.localdomain kernel: CR2: 00018c70
CR3: 00043a095000 CR4: 003406e0
Oct 07 17:47:16 localhost.localdomain kernel: DR0: 
DR1:  DR2: 
Oct 07 17:47:16 localhost.localdomain kernel: DR3: 
DR6: fffe0ff0 DR7: 0400
Oct 07 17:47:16 localhost.localdomain kernel: Stack:
Oct 07 17:47:16 localhost.localdomain kernel:  880035d53918
967ec350 880035d53940 967e9f2f
Oct 07 17:47:16 localhost.localdomain kernel:  88043ab3ec40
880438716a50 880438714800 880035d53970
Oct 07 17:47:16 localhost.localdomain kernel:  c00a155e
880427e49800 880438716800 880427ebd800
Oct 07 17:47:16 localhost.localdomain kernel: Call Trace:
Oct 07 17:47:16 localhost.localdomain kernel:  []
_raw_spin_lock+0x20/0x30
Oct 07 17:47:16 localhost.localdomain kernel:  []
__ww_mutex_lock+0x6f/0xa0
Oct 07 17:47:16 localhost.localdomain kernel:  []
drm_modeset_lock+0x4e/0xd0 [drm]
Oct 07 17:47:16 localhost.localdomain kernel:  []
drm_atomic_get_connector_state+0x34/0x1c0 [drm]
Oct 07 17:47:16 localhost.localdomain kernel:  []
__drm_atomic_helper_set_config+0x2a0/0x360 [drm_kms_helper]
Oct 07 17:47:16 localhost.localdomain kernel:  []
restore_fbdev_mode+0x22a/0x260 [drm_kms_helper]
Oct 07 17:47:16 localhost.localdomain kernel:  []
drm_fb_helper_restore_fbdev_mode_unlocked+0x34/0x80 [drm_kms_helper]
Oct 07 17:47:16 localhost.localdomain kernel:  []
drm_fb_helper_set_par+0x2d/0x50 [drm_kms_helper]
Oct 07 17:47:16 localhost.localdomain kernel:  []
intel_fbdev_set_par+0x1a/0x6

Re: Why is the kfree() argument const?

2008-01-18 Thread Vadim Lobanov
On Friday 18 January 2008 11:31:05 am Zan Lynx wrote:
> On Fri, 2008-01-18 at 11:14 -0800, Vadim Lobanov wrote:
> > Second, even if const did have stronger semantics that forbade the value
> > of x from being modified during execution of foo, the compiler still
> > could not reorder the two function calls, before it cannot assume that
> > the two functions (in their internal implementations) do not touch some
> > other, unknown to this code, global variable.
>
> This is why GCC has the pure and const function attributes.  These
> attributes are more powerful than the "const" keyword, and do allow
> optimizations with assumptions about global state.

Oh, absolutely. The problem, however, is that very very few people actually 
use these function attributes in their code. Heck, we don't even use the 
standard restrict keyword, much less gcc-specific function annotations.

I think that one part of the problem is because gcc seems to have had 
attribute diarrhea -- I know of noone who can recite more than 10% of the 
available attributes without glancing at the documentation. The other part of 
the problem is that gcc does no sanity checks on the provided attributes. For 
example, the code below compiles perfectly fine without a peep, even 
with -Wall and -Wextra.

int global;

void foobar(const int *x) __attribute__((const));

void foobar(const int *x)
{
if (*x)
*(int *)x = 7;
global = 11;
}

*grumble, grumble* :-)

-- Vadim Lobanov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Why is the kfree() argument const?

2008-01-18 Thread Vadim Lobanov
On Friday 18 January 2008 05:54:12 am Andy Lutomirski wrote:
> It almost sounds like the compiler is free to change:
>
> void foo(const int *x);
> foo(x);
> printf("%d", x);
>
> to:
>
> void foo(const int *x);
> printf("%d", x);
> foo(x);
>
> especially if it can prove that the pointer to x doesn't otherwise
> escape or that foo doesn't call anything that could see the pointer (and
> given that gcc has special magical markings for malloc, one way this
> could be "proven" is to have x be some freshly malloced object.

That's absolutely not true.

Let's unravel the code, by fixing usage of 'x' (which seems to vary at will 
between value and pointer in the above example), and by replacing printf with 
another opaque function. Our decls:
void foo(const int *ptr);
void bar(int val);
You're saying that this:
foo(&x);
bar(x);
can be reordered into this:
bar(x);
foo(&x);

No way. First, the way that const is currently defined, the compiler cannot 
assume that the value of x did not change while foo was executing. So, it 
will not only be forced to leave the two functions in that order, it will 
even reload the value of x before passing it into bar. Go figure.

Second, even if const did have stronger semantics that forbade the value of x 
from being modified during execution of foo, the compiler still could not 
reorder the two function calls, before it cannot assume that the two 
functions (in their internal implementations) do not touch some other, 
unknown to this code, global variable.

-- Vadim Lobanov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Why is the kfree() argument const?

2008-01-18 Thread Vadim Lobanov
On Friday 18 January 2008 03:47:01 am Giacomo A. Catenazzi wrote:
> Changing the name of variables in your example:
>
> extern print_int(const int *);
>
> int main(int argc, char **argv)
> {
>extern int errno;
>
>errno = 0;
>print_int(&i);
>return errno;
> }

Except that changing int to extern int makes all the difference in the world: 
the variable went from being local to being global. The way const is 
currently defined, however, the compiler cannot take advantage of the fact 
that the variable was local in the former case.

> Ok, I changed int to extern int, but you see the point?
> Do you want complex rules about const, depending on
> context (extern, volatile,...) ?

Sometimes complexity is worth it.

-- Vadim Lobanov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Why is the kfree() argument const?

2008-01-18 Thread Vadim Lobanov
On Thursday 17 January 2008 11:51:49 pm Giacomo Catenazzi wrote:
> Linus Torvalds wrote:
> > No, I'm saying that "const" has absolutely *zero* meaning on writes to an
> > object through _other_ pointers (or direct access) to the object.
>
> Hints: "restrict" is the C99 keyword for such requirement (or better
> "const restrict")

The restrict keyword controls aliasing, to be exact. And I'm skeptical that 
inserting const there would do anything at all.

> BTW I think C use non const free as a BIG warning about not to be
> to "smart" on optimization.

I must ask what relationship you think the const keyword has to compiler 
optimizations. I know of none, and I've yet to see that keyword cause any 
difference in the resulting assembly. It forces you to make your code clean 
and well-structured, but that's about it.

Of course, it would be an interesting experiment to potentially redefine the 
const keyword to have stronger semantics, such as having the compiler assume 
that a function taking a const pointer argument will not modify the memory 
the pointer points to, and thus saving itself a memory load in the caller 
after the function executes, as long as the data is not global. I imagine 
that this would lead to some simple and measurable optimizations, all the 
while (this is where I get into hand-waving territory) breaking a minimum 
amount of code in current existence.

But that is emphatically not how C is currently defined, and you're basically 
inventing an entirely new language... C2009 perhaps? :-)

-- Vadim Lobanov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

2008-01-09 Thread Vadim Lobanov
On Wednesday 09 January 2008 04:00:43 pm Alasdair G Kergon wrote:
> On Wed, Jan 09, 2008 at 03:31:00PM -0800, Vadim Lobanov wrote:
> > From 2.6.23's fs/ioctl.c - do_ioctl():
>
> Ah - you're talking about struct file_operations of course;
> I was talking about struct block_device_operations.
>
> Alasdair

Good point. :-)

-- Vadim Lobanov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

2008-01-09 Thread Vadim Lobanov
On Wednesday 09 January 2008 03:05:45 pm Alasdair G Kergon wrote:
> On Wed, Jan 09, 2008 at 04:58:46PM -0600, Chris Friesen wrote:
> > Alasdair G Kergon wrote:
> > >On Wed, Jan 09, 2008 at 11:46:03PM +0100, Andi Kleen wrote:
> > >>struct inode *inode = file->f_dentry->d_inode;
> > >
> > >And oops if that's not defined?
> >
> > Isn't this basically identical to what was being passed in to .ioctl()?
>
> Not in every case, unless someone's been through and created fake
> structures in all the remaining places that pass in a NULL 'file' because
> there isn't one available.

>From 2.6.23's fs/ioctl.c - do_ioctl():

if (filp->f_op->unlocked_ioctl) {
error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
if (error == -ENOIOCTLCMD)
error = -EINVAL;
goto out;
} else if (filp->f_op->ioctl) {
lock_kernel();
error = filp->f_op->ioctl(filp->f_path.dentry->d_inode,
  filp, cmd, arg);
unlock_kernel();
}

As a sidenote, please don't use f_dentry and f_vfsmnt, since they are just 
#defines for the correct fields. They were meant to be temporary transition 
helpers, but (alas) have refused to die thus far. If noone beats me to it, 
I'll take a look-see at deprecating them all the way.

-- Vadim Lobanov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [RFC] Documentation about unaligned memory access

2007-11-23 Thread Vadim Lobanov
On Thursday 22 November 2007 04:15:53 pm Daniel Drake wrote:
> Fortunately things are not too complex, as in most cases, the compiler
> ensures that things will work for you. For example, take the following
> structure:
>
>   struct foo {
>   u16 field1;
>   u32 field2;
>   u8 field3;
>   };
>
> Fortunately, the compiler understands the alignment constraints, so in the
> above case it would insert 2 bytes of padding inbetween field1 and field2.
> Therefore, for standard structure types you can always rely on the compiler
> to pad structures so that accesses to fields are suitably aligned (assuming
> you do not cast the field to a type of different length).

It would also insert 3 bytes of padding after field3, in order to satisfy 
alignment constraints for arrays of these structures.

> Sidenote: in the above example, you may wish to reorder the fields in the
> above structure so that the overall structure uses less memory. For
> example, moving field3 to sit inbetween field1 and field2 (where the
> padding is inserted) would shrink the overall structure by 1 byte:
>
>   struct foo {
>   u16 field1;
>   u8 field3;
>   u32 field2;
>   };

It will actually shrink it by 4 bytes, for the very same reason.

-- Vadim Lobanov


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: trouble loading self compiled vanilla kernel

2007-01-08 Thread Vadim Lobanov
On Mon, 2007-01-08 at 16:52 +0100, Jonas Svensson wrote:
> On Mon, 8 Jan 2007, Tilman Schmidt wrote:
> 
> > Jonas Svensson schrieb:
> > > I beleive make install does that in CentOS. There were a new initrd
> > > installed and it was not identical to the one supplied by CentOS.
> >
> > That's surprising. On SuSE I always have to build it separately
> > with mkinitrd, and the kernel makefiles are the same, after all.
> > Anyway, your symptoms definitely look like a bad initrd, so you
> > may want to have a closer look in that area. Perhaps build a
> > kernel you can boot without initrd for testing, ie. with the
> > drivers for the root disk and filesystem built in.
> 
> I will try it again, maybe I missed something. I will also try to read
> more on initrd and see if I am wrong to assume that make install does it
> for me.

In my experience on openSUSE, the following sequence of commands
installs both the kernel and the initrd:
make *config*
make
make modules_install
make install
However, if the order of the last two make invocations is switched, then
the initrd does not get generated (correctly or at all). Although
unlikely to be the problem, it's a simple thing to eliminate from the
list of possible borkages.

-- Vadim Lobanov


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] include/linux/slab.h: new KFREE() macro.

2007-01-08 Thread Vadim Lobanov
On Sun, 2007-01-07 at 23:29 -0800, Amit Choudhary wrote:
> I do not want to write this but I think that you are arguing just for the 
> heck of it. Please be
> sane.

No, I'm merely trying to demonstrate, on a logical basis, why the
proposed patch does not (in my opinion) belong within the kernel. The
fact that I'm not alone in voicing such disagreement should mean
something.

-- Vadim Lobanov


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] include/linux/slab.h: new KFREE() macro.

2007-01-07 Thread Vadim Lobanov
On Sun, 2007-01-07 at 20:09 -0800, Amit Choudhary wrote:
> I have already explained it earlier. I will try again. You will not need 
> free_2: and free_1: with
> KFREE(). You will only need one free: with KFREE.

So, to rephrase, your stated goal is to get rid of any non-singular goto
labels in function error handling paths? Aside from sounding trippy in a
non-conformist kind of way, what benefits will this give to the kernel?

I ask this because there's already one easy-to-spot downside: you'll end
up calling kfree(NULL) a bunch of times that can be, and should be,
avoided. Whereas turning my computer into a better space-heater using
noops (like repeated kfree(NULL) calls) may be a noble goal, I'd much
rather not waste this planet's limited resources unnecessarily.

> Also, let's say that count is different for each array? Then how do you 
> propose that memory be
> allocated in one pass?

The parameters to a '+' operator need not be equivalent.

> I have scanned the whole kernel to check whether people are checking for 
> return values of kmalloc,
> I found that at many places they don't and have sent patches for them. Now, 
> this too is brain
> damaged code. And during the scan I saw examples of what I described earlier.

These cases should be fixed independently of any particular KFREE()
agenda.

> KFREE() can fix some of those cases.

I am curious as to how a KFREE() macro can fix cases where people don't
check the kmalloc() return value.

> Below are some examples where people are doing KFREE() kind of stuff:

I glanced at one instance, and...

> arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c:  
> kfree(acpi_perf_data[j]);
> arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c-  
> acpi_perf_data[j] = NULL;

'acpi_perf_data' is a global and persistent data structure, where a NULL
value actually has a specific and distinct meaning (as in
acpi_cpufreq_cpu_init()). How you think this helps your argument with
setting a local pointer to NULL after free is beyond me.

-- Vadim Lobanov


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] include/linux/slab.h: new KFREE() macro.

2007-01-07 Thread Vadim Lobanov
On Sun, 2007-01-07 at 16:02 -0800, Amit Choudhary wrote:
> That's where KFREE(ptr) comes in so that the code doesn't look ugly and still 
> the purpose is
> achieved.

Shoving it into a macro makes it no better.

> > Reading code like that makes me say "wtf?", simply because 'ptr' is not
> > used thereafter,
> 
> Really? Then why do we have all the debugging options to catch re-use of the 
> memory that has been
> freed. So many debugging options has been implemented, so much effort has 
> gone into them, partly
> because programmers sometimes miss correct programming.

Which is exactly why we should continue to let programmers focus on
trying to get their code correct and letting the existing safety tools
catch thinkos, instead of distracting them with confusing and completely
pointless variable assignments.

> I do not know what you are talking about here. You are saying that a function 
> does not need three
> different arrays with different names. How can you say that? How do you know 
> what is the
> requirement?

What I said was that your example proves something entirely different
than what you think: rather than proving the need for your KFREE()
macro, it instead proves the need to design the code correctly from the
start, so as to avoid even thinking about this crud.

If you insist, here's your example function, trivially rewritten without
any NULL assignments. (I used two arrays, not three, to save space. The
basic idea works by-design for any random number of arrays, each of any
arbitrary size.)

struct type1 {
/* something */
};

struct type2 {
/* something */
};

#define COUNT 10

void function1(struct type1 **a1, struct type2 **a2, unsigned int sz);

void function2(void)
{
struct type1 *arr1[COUNT];
struct type2 *arr2[COUNT];
int i;

/* init */
for (i = 0; i < COUNT; i++) {
arr1[i] = kmalloc(sizeof(struct type1), ...);
if (!arr1[i])
goto free_1;
}
for (i = 0; i < COUNT; i++) {
arr2[i] = kmalloc(sizeof(struct type2), ...);
if (!arr2[i])
goto free_2;
}

/* work */
function1(arr1, arr2, COUNT);

/* fini */
i = COUNT;
free_2:
for (i--; i >= 0; i--) {
kfree(arr2[i]);
}
i = COUNT;
free_1:
for (i--; i >= 0; i--) {
kfree(arr1[i]);
}
}

In most cases, though, the above code design would be brain-damaged from
the start: unless the sizes involved are prohibitively large, the
function should be allocating all the memory in a single pass.

So, where's the demonstrated need for KFREE()?

-- Vadim Lobanov


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] include/linux/slab.h: new KFREE() macro.

2007-01-07 Thread Vadim Lobanov
On Sun, 2007-01-07 at 14:43 -0800, Amit Choudhary wrote:
> Any strong reason why not? x has some value that does not make sense and can 
> create only problems.
> And as I explained, it can result in longer code too. So, why keep this value 
> around. Why not
> re-initialize it to NULL.

Because it looks really STRANGE(tm). Consider the following function,
which is essentially what you're proposing in macro-ized form:
void foobar(void)
{
void *ptr;

ptr = kmalloc(...);
// actual work here
kfree(ptr);
ptr = NULL;
}
Reading code like that makes me say "wtf?", simply because 'ptr' is not
used thereafter, so setting it to NULL is both pointless and confusing
(it looks out-of-place, and therefore makes me wonder if there's
something stupidly tricky going on).

Also, arguably, your demonstration of why the lack of the proposed
KFREE() macro results in longer code is invalid. Whereas you wrote:
pointer *arr_x[size_x];
pointer *arr_y[size_y];
pointer *arr_z[size_z];
That really should have been:
pointer *arr[size_x + size_y + size_z];
or:
pointer **arr[3] = { arr_x, arr_y, arr_z };
In which case, the you only need one path in the function to handle
allocation failures, rather than the three that you were arguing for.

> If x should not be re-initialized to NULL, then by the same logic, we should 
> not even initialize
> local variables. And all of us know that local variables should be 
> initialized.

That's some strange and confused logic then. Here's my stab at the same
logical premise: "Using uninitialized values is bad." Notice how that,
in and of itself, makes no statements regarding freed pointers, since
the intent is not to use them after they've been freed anyway.

-- Vadim Lobanov


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [DISCUSS] Making system calls more portable.

2007-01-07 Thread Vadim Lobanov
On Sun, 2007-01-07 at 00:15 -0800, Amit Choudhary wrote:
> 1. Invoke a system call using its name. Pass its name to the kernel as an 
> argument of syscall() or
> some other function. Probably may make the invocation of the system call 
> slower. If the name
> doesn't match in the kernel then an error can be returned.
> 
> 2. Create a /proc entry that will return the number of the system call given 
> its name. This number
> can then be used to invoke the system call.

Your argument has a built-in assumption that, whereas syscall numbers do
collide, syscall names will not. This assumption is not true; people
will quite often pick the same name for something independent of each
other.

Additionally, the proposed solutions will require a dramatic increase in
memory, to store a static string name for each syscall, and a marked
increase in CPU usage, to do string hashing and matching for each
syscall invocation (and these can occur very often). This overhead is
hard to justify just to support custom vendor kernels, as Rene pointed
out in a separate reply.

> These approaches will also remove the dependency from user space header file 
> that contains the
> mapping from the system call name to its number. I hope that I made some 
> sense.

I thought that this file was "shipped upwards" by the kernel already, as
a sanitized header?

-- Vadim Lobanov


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Feature request: exec self for NOMMU.

2006-12-26 Thread Vadim Lobanov
On Wed, 2006-12-27 at 00:51 -0500, Rob Landley wrote:
> On Wednesday 27 December 2006 12:13 am, Ray Lee wrote:
> > How about openning an fd to yourself at the beginning of execution, then
> > calling fexecve later?
> 
> I haven't got a man page for fexecve.  Does libc have it?

It's implemented inside glibc, and uses /proc to execve() the file that
the fd points to. Here's the code from
sysdeps/unix/sysv/linux/fexecve.c:

fexecve (fd, argv, envp)
 int fd;
 char *const argv[];
 char *const envp[];
{
  ...
  /* We use the /proc filesystem to get the information.  If it is not
 mounted we fail.  */
  char buf[sizeof "/proc/self/fd/" + sizeof (int) * 3];
  __snprintf (buf, sizeof (buf), "/proc/self/fd/%d", fd);

  /* We do not need the return value.  */
  __execve (buf, argv, envp);
  ...
}


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] fdtable: Provide free_fdtable() wrapper.

2006-12-19 Thread Vadim Lobanov
Hi,

Christoph Hellwig has expressed concerns that the recent fdtable changes
expose the details of the RCU methodology used to release no-longer-used
fdtable structures to the rest of the kernel. The trivial patch below
addresses these concerns by introducing the appropriate free_fdtable() calls,
which simply wrap the release RCU usage. Since free_fdtable() is a one-liner,
it makes sense to promote it to an inline helper.

Please apply.

Signed-off-by: Vadim Lobanov <[EMAIL PROTECTED]>

diff -pru old/fs/file.c new/fs/file.c
--- old/fs/file.c   2006-12-19 19:54:23.0 -0800
+++ new/fs/file.c   2006-12-19 20:04:02.0 -0800
@@ -206,7 +206,7 @@ static int expand_fdtable(struct files_s
copy_fdtable(new_fdt, cur_fdt);
rcu_assign_pointer(files->fdt, new_fdt);
if (cur_fdt->max_fds > NR_OPEN_DEFAULT)
-   call_rcu(&cur_fdt->rcu, free_fdtable_rcu);
+   free_fdtable(cur_fdt);
} else {
/* Somebody else expanded, so undo our attempt */
free_fdarr(new_fdt);
diff -pru old/include/linux/file.h new/include/linux/file.h
--- old/include/linux/file.h2006-12-19 19:54:25.0 -0800
+++ new/include/linux/file.h2006-12-19 20:03:19.0 -0800
@@ -80,6 +80,11 @@ extern int expand_files(struct files_str
 extern void free_fdtable_rcu(struct rcu_head *rcu);
 extern void __init files_defer_init(void);
 
+static inline void free_fdtable(struct fdtable *fdt)
+{
+   call_rcu(&fdt->rcu, free_fdtable_rcu);
+}
+
 static inline struct file * fcheck_files(struct files_struct *files, unsigned 
int fd)
 {
struct file * file = NULL;
diff -pru old/kernel/exit.c new/kernel/exit.c
--- old/kernel/exit.c   2006-12-19 19:54:52.0 -0800
+++ new/kernel/exit.c   2006-12-19 20:04:20.0 -0800
@@ -466,7 +466,7 @@ void fastcall put_files_struct(struct fi
fdt = files_fdtable(files);
if (fdt != &files->fdtab)
kmem_cache_free(files_cachep, files);
-   call_rcu(&fdt->rcu, free_fdtable_rcu);
+   free_fdtable(fdt);
}
 }
 


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Patch to fixe Data Acess error in dup_fd

2006-11-17 Thread Vadim Lobanov
On Fri, 2006-11-17 at 18:38 +0530, Sharyathi Nagesh wrote:
>   I looked into a few memtests that were run in similar machine. There
> are a few slab corruption issues but not while running memtest and no
> other issues.
>   Seems difficult to replicate.

Bummer.

Bisection with vanilla (non-distro) kernels may be your best bet, then.
I have tried to replicate this issue in my spare time, but so far have
been unsuccessful.

-- Vadim Lobanov

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [OT] volatile keyword

2005-08-25 Thread Vadim Lobanov
On Thu, 25 Aug 2005, Christopher Friesen wrote:

> Vadim Lobanov wrote:
>
> > I'm positive I'm doing something wrong here. In fact, I bet it's the
> > volatile cast within the loop that's wrong; but I'm not sure how to do
> > it correctly. Any help / pointers / discussion would be appreciated.
>
> You need to cast is as dereferencing a volatile pointer.
>
> Chris
>

I figured it was something along these lines. In that case, is the
following code (from kernel/posix-timers.c) really doing the right
thing?

do
expires = timr->it_timer.expires;
while ((volatile long) (timr->it_timer.expires) != expires);

Seems it's casting the value, not the pointer.

-VadimL
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[OT] volatile keyword

2005-08-25 Thread Vadim Lobanov
Hi,

The recent discussion on the list concerning memory barriers and write
ordering took a side-trip to the volatile keyword, especially its
correct / incorrect usage. Someone posted a link to the LKML archives,
in which the argument is made that it is best to keep 'volatile' _out_
of variable and structure definitions, and _into_ the code that uses
them. I was curious, so I decided to try this out (looking at
kernel/posix-timers.c for inspiration)...

Here's sample userland program number one, written one way:
===
#include 
#include 
#include 

struct sync {
volatile unsigned long lock;
volatile unsigned long value;
};

struct sync data;

void * thread (void * arg) {
sleep(5);
data.value = 0;
data.lock = 0;

return NULL;
}

int main (void) {
pthread_t other;

data.lock = 1;
data.value = 1;
pthread_create(&other, NULL, thread, NULL);
while (data.lock);
printf("Value is %lu.\n", data.value);
pthread_join(other, NULL);

return 0;
}
===

And here's what should be the same program, written the "suggested" way:
===
#include 
#include 
#include 

struct sync {
unsigned long lock;
unsigned long value;
};

struct sync data;

void * thread (void * arg) {
sleep(5);
data.value = 0;
data.lock = 0;

return NULL;
}

int main (void) {
pthread_t other;

data.lock = 1;
data.value = 1;
pthread_create(&other, NULL, thread, NULL);
while ((volatile unsigned long)(data.lock));
printf("Value is %lu.\n", data.value);
pthread_join(other, NULL);

return 0;
}
===

The first program works exactly as expected. The second program,
however, never syncs with the sleeping thread. In fact, for the second
program, gcc compiles the while loop down to an infinite loop.

I'm positive I'm doing something wrong here. In fact, I bet it's the
volatile cast within the loop that's wrong; but I'm not sure how to do
it correctly. Any help / pointers / discussion would be appreciated.

Thanks. :-)
-VadimL
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Why system call need to copy the date from the userspace before using it

2005-04-16 Thread Vadim Lobanov
On Sat, 16 Apr 2005, Hacksaw wrote:

> >if you want actual concrete examples, let me know.
> I'd love a few, but maybe privately?
>
>
> I can certainly see where always copying is simpler; I certainly consider this
> to be an optimization, which must be looked at carefully, lest you end up with
> that which speed things up a little, but adds a big maintenance headache.
>
> But this strikes me as a potentially big speed up for movement through
> devices. (Or is there already a mechanism for that?)
>
> >It checks if the LAST page belongs to userland, and fails if not;
> I can't claim to know how memory assignment goes. I suppose that this
> statement means that the address space the userland program sees is 
> continuous?
>
> If not I could see a scenario where that would allow someone to get at data
> that isn't theirs, by allocating around until they got an chunk far up in
> memory, then just specified a start address way lower with the right size to
> end up on their chunk.
>
> I'm assuming this isn't a workable scenario, right?

For the copy_from_user() operation, we're still talking about virtual
memory. In virtual memory terms, each userspace program resides in the
lower addresses, while the kernel takes up the higher addresses. The
user program can pass the kernel any virtual memory pointer it feels
like.

The kernel first checks that it won't try to read from itself, simply by
checking that the last page, belonging to the highest virtual address of
the supposed buffer, does not belong to kernel space. So far so good.

Now the only thing that can go wrong is that the user program told the
kernel that the buffer exists, but some or all of the pages are not
mapped in virtual memory. This is taken care of "transparently" during
the copy -- if we try to copy from a page that isn't mapped, the kernel
will catch the exception, realize that the buffer was bogus, and return
an error.

All of this works because virtual memory is much more restrictive than
physical memory in terms of what data resides where.

> --
> You are in a maze of twisty passages, all alike. Again.
> http://www.hacksaw.org -- http://www.privatecircus.com -- KB1FVD
>
>

-Vadim Lobanov
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Why system call need to copy the date from the userspace before using it

2005-04-15 Thread Vadim Lobanov
On Sat, 16 Apr 2005, Hacksaw wrote:

> Sorry if this bugs anyone, but I'm learning things here.
>
> What I would expect the kernel to do is this:
>
> system_call_data_prep (userdata, size){
>
>if !4G/4G {
>   for each page from userdata to userdata+size
>   {
>   if the page is swapped out, swap it in
>   if the page is not owned by the user process, return -ENOWAYMAN
>   otherwise, lock the page
>   }
>   return userdata;
> }
> else { //kernel land and userland are mutually exclusive
>  copy the data into kernel land
>  return kernelland_copy_of_userdata;
>   }
> }
>
> (And then the syscall would need to run the opposite function
> sys_call_data_unprep to unlock pages.)
>
> Hmm, maybe that interface sucks.

That's one approach. Unfortunately, it's not what the kernel currently
does. The root of the problem is -- it needs to copy the data, even if
the kernel can access userspace data. There are many reasons for why
this is a simpler way to program the interface; if you want actual
concrete examples, let me know.

In order to accomplish the copy_from_user() procedure, from the i386
perspective, the kernel first figures out where userspace is telling it
to look for the data buffer. It checks if the LAST page belongs to
userland, and fails if not; this works because the kernel sits in higher
memory. Then it simply does the direct copy. If during the copy it hits
an invalid page, the exception handler code will run, realize that the
exception occurred because of the copy, and return an error code right
then and there.

Lots of details left out, but this is the 10,000 foot view, I think.

-Vadim Lobanov

> Is it anything close to that?
>
> --
> The best is the enemy of the good  -- Voltaire
> The Good Enough is the enemy of the Great -- Me
> http://www.hacksaw.org -- http://www.privatecircus.com -- KB1FVD
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Further copy_from_user() discussion.

2005-04-15 Thread Vadim Lobanov
On Fri, 15 Apr 2005, Catalin Marinas wrote:

> Vadim Lobanov <[EMAIL PROTECTED]> wrote:
> > I think I misspoke a bit in my email above. The intent was not to
> > eliminate all might_sleep() calls from the copy_from_user() code path;
> > but rather juggle the source around a bit so there is only one
> > might_sleep() call per each code path. Currently, in the default case,
> > it calls it twice.
> >
> > By the way, is the following still true about might_sleep()?
> > http://kerneltrap.org/node/3440/10103
>
> With Ingo's realtime-preempt patch, might_sleep() expands to
> might_resched(). The latter expands to cond_resched() only if
> CONFIG_PREEMPT_VOLUNTARY is enabled (for CONFIG_PREEMPT_RT this is not
> needed since the kernel is involuntarily preemptible). In this case it
> might be useful to have might_sleep() only called before memset().
>
> --
> Catalin
>

I agree that might_sleep() needs to be placed in the code judiciously...
just probably not so close together as it is now. :-) I can work this
out in a patch, _if_ people want me to roll a patch in the first place.

-Vadim Lobanov
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Further copy_from_user() discussion.

2005-04-14 Thread Vadim Lobanov
On Thu, 14 Apr 2005, Catalin Marinas wrote:

> Vadim Lobanov <[EMAIL PROTECTED]> wrote:
> > 2. Would it be possible to eliminate the might_sleep() call in
> > copy_from_user()? It seems that, very soon after, the __copy_from_user()
> > macro does another might_sleep(), with very few instructions in between.
> > But there might be some trick here that I'm missing.
>
> might_sleep() is used for debugging the possible sleep while in an
> atomic operation. I think it is safe to check this for all the calls
> to copy_from_user(), no matter if the access is OK or not (memset
> being used in the latter case). The same is for
> __copy_from_user(). Anyway, if you don't enable
> CONFIG_DEBUG_SPINLOCK_SLEEP, the might_sleep() macro is empty.
>
> --
> Catalin
>

Thanks for the response.

I think I misspoke a bit in my email above. The intent was not to
eliminate all might_sleep() calls from the copy_from_user() code path;
but rather juggle the source around a bit so there is only one
might_sleep() call per each code path. Currently, in the default case,
it calls it twice.

By the way, is the following still true about might_sleep()?
http://kerneltrap.org/node/3440/10103

-Vadim Lobanov
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Further copy_from_user() discussion.

2005-04-13 Thread Vadim Lobanov
Hi,

Interested by the recent discussions concerning the copy_from_user()
function, I browsed the 2.6.11.7 kernel source, and came up with a few
questions.

1. Is there any particular reason why __copy_from_user_ll() is currently
EXPORT_SYMBOL()ed for i386? At least none of the in-tree modules
currently seem to use it, and __copy_from_user() seems like what most
would want anyway. If __copy_from_user_ll() is unexported, it looks like
we can eliminate the BUG_ON() statement within it.

2. Would it be possible to eliminate the might_sleep() call in
copy_from_user()? It seems that, very soon after, the __copy_from_user()
macro does another might_sleep(), with very few instructions in between.
But there might be some trick here that I'm missing.

Please enlighten. :-)

-Vadim Lobanov
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: Why system call need to copy the date from the userspace before using it

2005-04-12 Thread Vadim Lobanov
On Wed, 13 Apr 2005, Eshwar wrote:

>
> >The quick and simple answer to this question is: data integrity.
>
> >The main thing to understand is that, from the perspective of the
> >kernel, any user input provided in the form of system calls must have
> >immutable data. Only if the data is immutable can the kernel code parse
> >it and decide what to do, without getting into really hairy race
> >conditions. And, for that matter, it's much simpler and less error-prone
> >to program code where you don't have to worry about the inputs changing
> >around you all the time.
>
> Does this approach lead to major performance bottleneck??
>

It should not be so much of a performance bottleneck -- this kind of
operation lends itself naturally to parallelization, since it has few
(if any) dependencies. The only race I can think of off-hand is the
exit() syscall, but I'm sure that's already handled elsewhere (just not
sure of the details) In the end, however, if you believe my previous
email, then you should believe that the copy has to happen in any case.

I don't have any actual data points on-hand. Perhaps someone else does?

-Vadim Lobanov
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Why system call need to copy the date from the userspace before using it

2005-04-12 Thread Vadim Lobanov
On Wed, 13 Apr 2005, Tomko wrote:

> Hi all,
>
> I am new to linux , hope someone can help me.
> While i am reading the source code of the linux system call , i find
> that the system call need to call copy_from_user() to copy the data from
> user space to kernel space before using it . Why not use it directly as
> the system call has got the address ?  Furthermore , how to distinguish
> between user space and kernel space ?
>
> Thx a lot,
>
> TOM
> -

The quick and simple answer to this question is: data integrity.

The main thing to understand is that, from the perspective of the
kernel, any user input provided in the form of system calls must have
immutable data. Only if the data is immutable can the kernel code parse
it and decide what to do, without getting into really hairy race
conditions. And, for that matter, it's much simpler and less error-prone
to program code where you don't have to worry about the inputs changing
around you all the time.

So, you might say, what's wrong with the user code giving the kernel a
pointer to a userland buffer? After all, the calling task will be
blocked while the system call is being executed on its behalf. The
biggest problem is that the buffer can still be modified, while the
system call is executing, by another userland thread running in the same
virtual memory context. Or, for that matter, by another process that has
this chunk of memory shared with the original task. There are
innumerable ways for the data to potentially change in the middle of the
system call, and the simplest solution ends up being to copy the data to
kernelspace before working with it. That way, no userland tasks can
change it on you.

I'm sure there are other reasons for doing the copy, that someone will
be able to chime in with. Other input is always welcome. :-)

-Vadim Lobanov
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: easy softball jiffies quest(ion)

2005-04-08 Thread Vadim Lobanov
.  i would like to write a mouse manager where i could assign
> different actions for each button, something very similar to the kensington
> interface that's available for, um, windows.  i found some xwindow functions
> for button pressing events, but i don't know how to get into the mouse driver
> or button events in xwindows or gnome, etc.

Depends upon what functionality you want to have those buttons to have.
If it's nothing super fancy, it might very well be doable in user-space
without having to muck around inside the kernel. A combination of
xmodmap and configuration files, maybe? (Not an expert on this, I'm
afraid to say.)

> if somebody has a direction to go for that, that would be a big help.
>
> thanks, or tgfl(inux),
> philip dahlquist

Vadim Lobanov.

> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/