Adding Jason and LKML. to cc's...
-------- Forwarded Message --------
From: Roland Vossen <[email protected]>
To: [email protected]
Cc: [email protected] <[email protected]>,
[email protected] <[email protected]>
Subject: Fwd: Re: Why is dynamic debug disabled for staging drivers ?
Date: Wed, 6 Apr 2011 17:09:04 +0200
Hello Rusty,
I would like to make a change to module.c and noticed with 'git blame'
that you were the last person who touched these specific lines in module.c:
http://lxr.linux.no/#linux+v2.6.38/kernel/module.c#L2792
===== Corresponding commit for your reference:
commit 811d66a0e1e99902d365497eec7884113a2665bd
Author: Rusty Russell <[email protected]>
Date: Thu Aug 5 12:59:12 2010 -0600
module: group post-relocation functions into post_relocation()
This simply hoists more code out of load_module; we also put the
identification of the extable and dynamic debug table in with the
others in find_module_sections().
We move the taint check to the actual add/remove of the dynamic debug
info: this is certain (find_module_sections is too early).
Signed-off-by: Rusty Russell <[email protected]>
Cc: Yehuda Sadeh <[email protected]>
==============
Can you tell me if the following proposed code change in module.c is ok
with you:
/* This has to be done once we're sure module name is unique. */
if (!mod->taints || mod->taints == (1U << TAINT_CRAP))
dynamic_debug_setup(info.debug, info.num_debug);
Thanks, Roland.
-------- Original Message --------
Subject: Re: Why is dynamic debug disabled for staging drivers ?
Date: Wed, 6 Apr 2011 07:13:29 -0700
From: Greg KH <[email protected]>
To: Roland Vossen <[email protected]>
CC: [email protected] <[email protected]>
On Wed, Apr 06, 2011 at 03:54:37PM +0200, Roland Vossen wrote:
> Hi,
>
> I want to replace the proprietary logging mechanism in brcm80211
> with a Linux mechanism. 'Dynamic debug' seemed to be a good fit.
> But, to my disappointment, I discovered that dynamic debugging is
> not supported for drivers from the staging dir:
>
> - staging drivers are marked tainted (ref:
> http://lxr.linux.no/#linux+v2.6.38/kernel/module.c#L2417)
>
> - subsequently tainted drivers don't get dynamic debug goodies (ref:
> http://lxr.linux.no/#linux+v2.6.38/kernel/module.c#L2792)
>
> I wonder if this is intentional behavior. Does anybody know ?
Ah, no it isn't. I didn't realize that tainted modules didn't get
dynamic debug stuff, that's not nice. I'd be glad to take a fix that
allows drivers tainted with the TAINT_CRAP flag to be able to use
dynamic debug.
In looking at that check, it probably can be fixed to just make sure
that the module name is unique (i.e. only a specific taint flag check)
which is the real goal of that check in module.c.
thanks,
greg k-h
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
On Wed, Apr 06, 2011 at 08:34:35AM -0700, Joe Perches wrote:
> Adding Jason and LKML. to cc's...
>
> -------- Forwarded Message --------
> From: Roland Vossen <[email protected]>
> To: [email protected]
> Cc: [email protected] <[email protected]>,
> [email protected] <[email protected]>
> Subject: Fwd: Re: Why is dynamic debug disabled for staging drivers ?
> Date: Wed, 6 Apr 2011 17:09:04 +0200
>
> Hello Rusty,
>
> I would like to make a change to module.c and noticed with 'git blame'
> that you were the last person who touched these specific lines in module.c:
>
> http://lxr.linux.no/#linux+v2.6.38/kernel/module.c#L2792
>
> ===== Corresponding commit for your reference:
>
> commit 811d66a0e1e99902d365497eec7884113a2665bd
> Author: Rusty Russell <[email protected]>
> Date: Thu Aug 5 12:59:12 2010 -0600
>
> module: group post-relocation functions into post_relocation()
>
> This simply hoists more code out of load_module; we also put the
> identification of the extable and dynamic debug table in with the
> others in find_module_sections().
>
> We move the taint check to the actual add/remove of the dynamic debug
> info: this is certain (find_module_sections is too early).
>
> Signed-off-by: Rusty Russell <[email protected]>
> Cc: Yehuda Sadeh <[email protected]>
>
> ==============
>
>
> Can you tell me if the following proposed code change in module.c is ok
> with you:
>
> /* This has to be done once we're sure module name is unique. */
> if (!mod->taints || mod->taints == (1U << TAINT_CRAP))
> dynamic_debug_setup(info.debug, info.num_debug);
>
> Thanks, Roland.
>
>
> -------- Original Message --------
> Subject: Re: Why is dynamic debug disabled for staging drivers ?
> Date: Wed, 6 Apr 2011 07:13:29 -0700
> From: Greg KH <[email protected]>
> To: Roland Vossen <[email protected]>
> CC: [email protected] <[email protected]>
>
> On Wed, Apr 06, 2011 at 03:54:37PM +0200, Roland Vossen wrote:
> > Hi,
> >
> > I want to replace the proprietary logging mechanism in brcm80211
> > with a Linux mechanism. 'Dynamic debug' seemed to be a good fit.
> > But, to my disappointment, I discovered that dynamic debugging is
> > not supported for drivers from the staging dir:
> >
> > - staging drivers are marked tainted (ref:
> > http://lxr.linux.no/#linux+v2.6.38/kernel/module.c#L2417)
> >
> > - subsequently tainted drivers don't get dynamic debug goodies (ref:
> > http://lxr.linux.no/#linux+v2.6.38/kernel/module.c#L2792)
> >
> > I wonder if this is intentional behavior. Does anybody know ?
>
> Ah, no it isn't. I didn't realize that tainted modules didn't get
> dynamic debug stuff, that's not nice. I'd be glad to take a fix that
> allows drivers tainted with the TAINT_CRAP flag to be able to use
> dynamic debug.
>
Indeed, orginally dynamic debug didn't have a taint restriction...I'm not
sure when it got picked up...
> In looking at that check, it probably can be fixed to just make sure
> that the module name is unique (i.e. only a specific taint flag check)
> which is the real goal of that check in module.c.
>
The check for name uniqueness is performed right before the dynamic
debug setup:
...
mutex_lock(&module_mutex);
if (find_module(mod->name)) {
err = -EEXIST;
goto unlock;
}
/* This has to be done once we're sure module name is unique. */
if (!mod->taints)
dynamic_debug_setup(info.debug, info.num_debug);
The concern I would have in allowing tainted modules is that we are
relying on a specific format for the dynamic debug section in the
compiled module. For example, if a module was built with a format with an old
section, we could potentially get confused. This can be solved with
versioning, but that adds extra complexity. We can of course also make
sure we don't change the format...
Notice that tracepoints, which also rely on a specific module format,
also employ a taint flag check.
thanks,
-Jason
On Wed, Apr 06, 2011 at 02:41:59PM -0400, Jason Baron wrote:
> The concern I would have in allowing tainted modules is that we are
> relying on a specific format for the dynamic debug section in the
> compiled module. For example, if a module was built with a format with an old
> section, we could potentially get confused. This can be solved with
> versioning, but that adds extra complexity. We can of course also make
> sure we don't change the format...
I wouldn't worry about that at all. If you try to load a kernel module
that was not built against the kernel you are running, all bets are off
and lots of bad things can happen in other areas.
> Notice that tracepoints, which also rely on a specific module format,
> also employ a taint flag check.
Ok, but TAINT_CRAP shouldn't be part of that check, right?
thanks,
greg k-h
On Wed, Apr 06, 2011 at 11:55:25AM -0700, Greg KH wrote:
> On Wed, Apr 06, 2011 at 02:41:59PM -0400, Jason Baron wrote:
> > The concern I would have in allowing tainted modules is that we are
> > relying on a specific format for the dynamic debug section in the
> > compiled module. For example, if a module was built with a format with an old
> > section, we could potentially get confused. This can be solved with
> > versioning, but that adds extra complexity. We can of course also make
> > sure we don't change the format...
>
> I wouldn't worry about that at all. If you try to load a kernel module
> that was not built against the kernel you are running, all bets are off
> and lots of bad things can happen in other areas.
>
> > Notice that tracepoints, which also rely on a specific module format,
> > also employ a taint flag check.
>
> Ok, but TAINT_CRAP shouldn't be part of that check, right?
>
Right, allowing -staging drivers (which are part of the kernel tree) to
make use of dynamic debug facility seems reasonable.
thanks,
-Jason