> Hi Andi,
> I grepped and tried to do what you suggested.
> The first file that git grep reported was:
> arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
>
> So I cooked up the following patch (probably mangled, just to give you
> a rough idea of what I did):
> diff --g
On Thursday 10 January 2008 03:39, Alasdair G Kergon wrote:
> On Thu, Jan 10, 2008 at 01:49:15AM -0800, Daniel Phillips wrote:
> > So what stops you from changing to unlocked_ioctl for the main
> > device mapper ctl_ioctl?
>
> Nothing - patches to do this are queued for 2.6.25:
Nice. This removes
On Thu, Jan 10, 2008 at 01:49:15AM -0800, Daniel Phillips wrote:
> So what stops you from changing to unlocked_ioctl for the main device
> mapper ctl_ioctl?
Nothing - patches to do this are queued for 2.6.25:
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-ioctl-rem
> So if I write my own driver and have never heard of ioctls running on BKL
> before I can rather be sure that I just can change the interface of the ioctl
> function, put it in unlocked_ioctl and are fine? Cool.
If you know the BKL is not needed in your code you should use unlocked_ioctl
correc
Andi Kleen wrote:
> > Can you explain the rationale behind that running on the BKL? What type
> > of
>
> It used to always run with the BKL because everything used to
> and originally nobody wanted to review all ioctl handlers in tree to see if
> they can run with more fine grained locking. A lot p
Hi Alasdair,
On Wednesday 09 January 2008 14:40, Alasdair G Kergon wrote:
> Device-mapper for example in dm_blk_ioctl(), has no need for BKL so
> drops it immediately, but it does need the inode parameter, so it is
> unable to switch as things stand.
So what stops you from changing to unlocked_io
> Can you explain the rationale behind that running on the BKL? What type of
It used to always run with the BKL because everything used to
and originally nobody wanted to review all ioctl handlers in tree to see if
they can run with more fine grained locking. A lot probably can though.
> thing
Andi Kleen wrote:
> Here's a proposal for some useful code transformations the kernel janitors
> could do as opposed to running checkpatch.pl.
>
> Most ioctl handlers still running implicitely under the big kernel
> lock (BKL). But long term Linux is trying to get away from that. There is a
> new -
On Wed, Jan 09, 2008 at 10:45:13PM +, 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?
For file_operations which we talk about here it always is defined.
Block_device
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.
>
> Ala
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
--
[EMAIL PROTECTED]
--
To unsubscribe from this list: send the line "
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 i
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
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()?
Chris
--
To unsubscribe from this list: send the lin
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?
Alasdair
--
[EMAIL PROTECTED]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Mor
On Wed, Jan 09, 2008 at 10:40:26PM +, Alasdair G Kergon wrote:
> On Wed, Jan 09, 2008 at 02:12:40PM -0600, Matt Mackall wrote:
> > You'll need to change the prototype, the unlocked version doesn't take
> > an inode. And you'll need to make sure that nothing in the function uses
> > the inode, w
On Wed, Jan 09, 2008 at 02:12:40PM -0600, Matt Mackall wrote:
> You'll need to change the prototype, the unlocked version doesn't take
> an inode. And you'll need to make sure that nothing in the function uses
> the inode, which I think Andi forgot to mention.
This old chestnut again. Perhaps we
On Tue, 2008-01-08 at 20:58 +0100, Paolo Ciarrocchi wrote:
> On Jan 8, 2008 5:40 PM, Andi Kleen <[EMAIL PROTECTED]> wrote:
> So I cooked up the following patch (probably mangled, just to give you
> a rough idea of what I did):
> diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
>
> Yes, very good point. Does checkpatch.pl enforce diff -p already?
> If not, should it?
I don't think it should. That would reject a lot of perfectly good
patches.
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More
On 14:17, Richard Knutsson wrote:
> Would had preferred:
>
> if (x) {
>result = -E;
>goto out;
> }
>
> then:
>
> result = -E;
> if (x)
>goto out;
>
AFAIK, the second form is preferred in Linux because it is better
readable and it generates slightly better code.
Thanks for
Andre Noll wrote:
On 17:40, Andi Kleen wrote:
Here's a proposal for some useful code transformations the kernel janitors
could do as opposed to running checkpatch.pl.
Here's my take on drivers/scsi/sg.c. It's only compile-tested on x86-64.
... and x86 with all(yes|mod)config. :)
On 17:40, Andi Kleen wrote:
> Here's a proposal for some useful code transformations the kernel janitors
> could do as opposed to running checkpatch.pl.
Here's my take on drivers/scsi/sg.c. It's only compile-tested on x86-64.
Please review.
Andre
---
Convert sg.c to the new unlocked_ioctl entr
Andi Kleen <[EMAIL PROTECTED]> writes:
> I imagined it would check for
>
> +struct file_operations ... = {
> + ...
> + .ioctl = ...
>
> That wouldn't catch the case of someone adding only .ioctl to an
> already existing file_operations which is not visible in the patch context,
> but
On Wed, Jan 09, 2008 at 02:41:52AM +0100, Andi Kleen wrote:
> There are a few like scsi_host_template that don't have a unlocked_ioctl yet,
> but that is just something that needs to be fixed.
There's few enough scsi LLDDs with an ioctl method that ->ioctl should
be switched over in a single patch
On Tue, Jan 08, 2008 at 09:31:24PM -0400, Kevin Winchester wrote:
> Arnd Bergmann wrote:
> > On Wednesday 09 January 2008, Andi Kleen wrote:
> >> I imagined it would check for
> >>
> >> +struct file_operations ... = {
> >> + ...
> >> + .ioctl = ...
> >>
> >> That wouldn't catch the cas
Arnd Bergmann wrote:
> On Wednesday 09 January 2008, Andi Kleen wrote:
>> I imagined it would check for
>>
>> +struct file_operations ... = {
>> + ...
>> + .ioctl = ...
>>
>> That wouldn't catch the case of someone adding only .ioctl to an
>> already existing file_operations which is
On Wednesday 09 January 2008, Andi Kleen wrote:
> I imagined it would check for
>
> +struct file_operations ... = {
> + ...
> + .ioctl = ...
>
> That wouldn't catch the case of someone adding only .ioctl to an
> already existing file_operations which is not visible in the patch cont
On Wed, Jan 09, 2008 at 01:40:58AM +0100, Arnd Bergmann wrote:
> On Tuesday 08 January 2008, Andi Kleen wrote:
> > > Thanks, Andi! I think it'd very useful change.
> >
> > Reminds me this is something that should be actually flagged
> > in checkpatch.pl too
> >
> > Andy, it would be good if check
On Tuesday 08 January 2008, Andi Kleen wrote:
> > Thanks, Andi! I think it'd very useful change.
>
> Reminds me this is something that should be actually flagged
> in checkpatch.pl too
>
> Andy, it would be good if checkpatch.pl complained about .ioctl =
> as opposed to .unlocked_ioctl = ...
Th
> Sorry about the noise here - I now notice that not all .ioctl function
> pointers have the option of changing to .unlocked_ioctl. In this case,
> the ioctl is in the struct scsi_host_template, rather than struct
> file_operations.
>
> I'll try to be a little more careful about the git grepping
Andi Kleen wrote:
> On Tue, Jan 08, 2008 at 07:50:47PM -0400, Kevin Winchester wrote:
>> Andi Kleen wrote:
>>> Here's a proposal for some useful code transformations the kernel janitors
>>> could do as opposed to running checkpatch.pl.
>>>
>>
>>
>> I notice that every driver in drivers/ata uses a
On Tue, Jan 08, 2008 at 07:50:47PM -0400, Kevin Winchester wrote:
> Andi Kleen wrote:
> > Here's a proposal for some useful code transformations the kernel janitors
> > could do as opposed to running checkpatch.pl.
> >
>
>
> I notice that every driver in drivers/ata uses a .ioctl that points to
> [EMAIL PROTECTED]:~/linux-2.6/mm$ grep "struct file_operations" *
> shmem.c:static const struct file_operations shmem_file_operations;
> shmem.c:static const struct file_operations shmem_file_operations = {
> swapfile.c:static const struct file_operations proc_swaps_operations = {
>
> Am I right
Matthew Wilcox пишет:
> On Tue, Jan 08, 2008 at 01:16:06PM -0700, Matthew Wilcox wrote:
>> On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
>>> Yes of course, I've been silly in didn't verify whether the file compile
>>> but I would appreciate to know whether I'm on the right track
Andi Kleen wrote:
> Here's a proposal for some useful code transformations the kernel janitors
> could do as opposed to running checkpatch.pl.
>
I notice that every driver in drivers/ata uses a .ioctl that points to
ata_scsi_ioctl(). I could add the BKL to that function, and then change
all of
On Jan 9, 2008 12:06 AM, Andi Kleen <[EMAIL PROTECTED]> wrote:
> > I would suggest to only work on files that compile. e.g. do a
> >
> > make allyesconfig
> > make -j$[$(grep -c processor /proc/cpuinfo)*2] &1 |tee LOG
> > (will probably take a long time)
> >
> > first and then only mod
> I would suggest to only work on files that compile. e.g. do a
>
> make allyesconfig
> make -j$[$(grep -c processor /proc/cpuinfo)*2] &1 |tee LOG (will
> probably take a long time)
>
> first and then only modify files when are mentioned in "LOG"
Actually since this will probably t
On Jan 8, 2008 9:42 PM, Andi Kleen <[EMAIL PROTECTED]> wrote:
> > I grepped and tried to do what you suggested.
>
> First a quick question: how would you rate your C knowledge? Did you
> ever write a program yourself?
Yes I did but I probably beeing inactive for too much time,
I need to back studi
> I grepped and tried to do what you suggested.
First a quick question: how would you rate your C knowledge? Did you
ever write a program yourself?
My proposal assumes that you have at least basic C knowledge.
> The first file that git grep reported was:
> arch/arm/common/rtctime.c:static const
On Jan 8, 2008 9:21 PM, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> On Tue, Jan 08, 2008 at 01:16:06PM -0700, Matthew Wilcox wrote:
> > On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
> > > Yes of course, I've been silly in didn't verify whether the file compile
> > > but I would
On Tue, 8 Jan 2008 20:58:04 +0100
"Paolo Ciarrocchi" <[EMAIL PROTECTED]> wrote:
> -static const struct file_operations rtc_fops = {
> +static long rtc_fioctl(struct file_operations rtc_fops)
> +{
> + lock_kernel();
> .owner = THIS_MODULE,
> .llseek = no_llseek,
>
On Tue, Jan 08, 2008 at 01:16:06PM -0700, Matthew Wilcox wrote:
> On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
> > Yes of course, I've been silly in didn't verify whether the file compile
> > but I would appreciate to know whether I'm on the right track or not.
>
> Well ... yo
On Tue, Jan 08, 2008 at 09:03:13PM +0100, Paolo Ciarrocchi wrote:
> On Jan 8, 2008 9:00 PM, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> > On Tue, Jan 08, 2008 at 08:58:04PM +0100, Paolo Ciarrocchi wrote:
> > > -static const struct file_operations rtc_fops = {
> > > +static long rtc_fioctl(struct fi
On Jan 8, 2008 9:00 PM, Matthew Wilcox <[EMAIL PROTECTED]> wrote:
> On Tue, Jan 08, 2008 at 08:58:04PM +0100, Paolo Ciarrocchi wrote:
> > -static const struct file_operations rtc_fops = {
> > +static long rtc_fioctl(struct file_operations rtc_fops)
> > +{
> > + lock_kernel();
> > .owner
On Tue, Jan 08, 2008 at 08:58:04PM +0100, Paolo Ciarrocchi wrote:
> -static const struct file_operations rtc_fops = {
> +static long rtc_fioctl(struct file_operations rtc_fops)
> +{
> + lock_kernel();
> .owner = THIS_MODULE,
You should probably restrict yourself to files that yo
On Jan 8, 2008 5:40 PM, Andi Kleen <[EMAIL PROTECTED]> wrote:
>
> Here's a proposal for some useful code transformations the kernel janitors
> could do as opposed to running checkpatch.pl.
>
> Most ioctl handlers still running implicitely under the big kernel
> lock (BKL). But long term Linux is tr
> Thanks, Andi! I think it'd very useful change.
Reminds me this is something that should be actually flagged
in checkpatch.pl too
Andy, it would be good if checkpatch.pl complained about .ioctl =
as opposed to .unlocked_ioctl = ...
Also perhaps if a whole new file_operations with a ioctl is ad
On Tue, Jan 08, 2008 at 05:40:15PM +0100, Andi Kleen wrote:
> Here's a proposal for some useful code transformations the kernel janitors
> could do as opposed to running checkpatch.pl.
>
> Most ioctl handlers still running implicitely under the big kernel
> lock (BKL). But long term Linux is tryi
[Andi Kleen - Tue, Jan 08, 2008 at 05:40:15PM +0100]
|
| Here's a proposal for some useful code transformations the kernel janitors
| could do as opposed to running checkpatch.pl.
|
[...snip...]
| -Andi
|
|
got it, thanks
- Cyrill -
--
To unsubscribe from this list: send the l
Here's a proposal for some useful code transformations the kernel janitors
could do as opposed to running checkpatch.pl.
Most ioctl handlers still running implicitely under the big kernel
lock (BKL). But long term Linux is trying to get away from that. There is a new
->unlocked_ioctl entry point
50 matches
Mail list logo
