diff -urN linux-2.5.27/include/linux/devfs_fs_kernel.h linux/include/linux/devfs_fs_kernel.h
--- linux-2.5.27/include/linux/devfs_fs_kernel.h 2002-07-20 21:11:30.000000000 +0200
+++ linux/include/linux/devfs_fs_kernel.h 2002-07-21 22:14:16.000000000 +0200
@@ -286,16 +286,6 @@
return;
}
-static inline kdev_t devfs_alloc_devnum (char type)
-{
- return NODEV;
-}
-
-static inline void devfs_dealloc_devnum (char type, kdev_t devnum)
-{
- return;
-}
-
static inline int devfs_alloc_unique_number (struct unique_numspace *space)
{
return -1;
Marcin Dalecki writes:
> Kill two inlines which are notwhere used and which don't make sense
> in the case someone is not compiling devfs at all.
Rejected. Linus, please don't apply this bogus patch. External patches
and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS
isn't needed.
Martin, why are you bothering with this kind of false cleanup? These
inline stubs don't take up any space in the object files, so why
bother? Also, given that the stubs were carefully added in the first
place, it suggests that there is a good reason for their presence.
Why didn't you stop and think it through before firing off a patch, or
at least ask me if you couldn't see why? This "patch first, think/ask
questions later" approach is disturbing.
Regards,
Richard....
Permanent: [email protected]
Current: [email protected]
Richard Gooch wrote:
> Marcin Dalecki writes:
>
>>Kill two inlines which are notwhere used and which don't make sense
>>in the case someone is not compiling devfs at all.
>
>
> Rejected. Linus, please don't apply this bogus patch. External patches
> and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS
> isn't needed.
Dare to actually *name* one of them?
> Martin, why are you bothering with this kind of false cleanup? These
> inline stubs don't take up any space in the object files, so why
> bother? Also, given that the stubs were carefully added in the first
> place, it suggests that there is a good reason for their presence.
They where not "carefully added".
The interface you are exposing is bogous.
Look in md.c for one example why.
Last time I counted you provide at least three different ways of object
allocations which play nasty games with major minor numbers in repeating
code in drivers all scattered over the kernel.
cd-roms are treated special md.c is doing. And you are doing the
whole object management in a side step instead of embarcing the
normal structures holding already device information so you get
of course memmory management problems...
> Why didn't you stop and think it through before firing off a patch, or
> at least ask me if you couldn't see why? This "patch first, think/ask
> questions later" approach is disturbing.
You didn't think doing devfs_fs_kernel.h. One simple sample from there:
devfs_get_maj_min(devfs_get_handle_from_inode((inode))
If I look at md.c which is using it... well better don't tell.
And the above of of course inside ({ })...
Everybody would expect the following to be only a single function:
extern devfs_handle_t devfs_get_handle (devfs_handle_t dir, const char
extern devfs_handle_t devfs_find_handle (devfs_handle_t dir, const char
And it was of course too hard to unify ops and handle:
extern void *devfs_get_ops (devfs_handle_t de);
extern void devfs_put_ops (devfs_handle_t de);
You couldn't resist adding the redundant devfs_ prefix overall in the
kernel:
extern devfs_register_chrdev (unsigned int major, const char *name,
struct file_operations *fops);
extern int devfs_register_blkdev (unsigned int major, const char *name,
struct block_device_operations *bdops);
extern int devfs_unregister_chrdev (unsigned int major, const char *name);
extern int devfs_unregister_blkdev (unsigned int major, const char *name);
Three different allocators and deallocators for one single subsystem,
preserving the illusion that there is in linux a real difference between
major and minor numbers...
extern int devfs_alloc_major (char type);
extern void devfs_dealloc_major (char type, int major);
extern kdev_t devfs_alloc_devnum (char type);
extern void devfs_dealloc_devnum (char type, kdev_t devnum);
extern int devfs_alloc_unique_number (struct unique_numspace *space);
extern void devfs_dealloc_unique_number (struct unique_numspace *space,
int number);
If flags are invalid -> add an invalid flag! instead of value return
through pointer.
static inline int devfs_get_flags (devfs_handle_t de, unsigned int *flags)
{
return 0;
}
And so on and so on.... Viro is simple right.
On Mon, 22 Jul 2002, Marcin Dalecki wrote:
> Richard Gooch wrote:
> > Marcin Dalecki writes:
> >
> >>Kill two inlines which are notwhere used and which don't make sense
> >>in the case someone is not compiling devfs at all.
> >
> >
> > Rejected. Linus, please don't apply this bogus patch. External patches
> > and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS
> > isn't needed.
>
> Dare to actually *name* one of them?
[snip]
OK, that's enough. Martin, kindly stay the fsck away from that pile of
garbage for a couple of weeks.
_All_ partition-related code is getting rewritten and the last thing
we need right now is additional clutter in the neighborhood. And
devfs_fs_kernel.h, shite as it is, qualifies.
Alexander Viro wrote:
>
> On Mon, 22 Jul 2002, Marcin Dalecki wrote:
>
>
>>Richard Gooch wrote:
>>
>>>Marcin Dalecki writes:
>>>
>>>
>>>>Kill two inlines which are notwhere used and which don't make sense
>>>>in the case someone is not compiling devfs at all.
>>>
>>>
>>>Rejected. Linus, please don't apply this bogus patch. External patches
>>>and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS
>>>isn't needed.
>>
>>Dare to actually *name* one of them?
>
>
> [snip]
>
> OK, that's enough. Martin, kindly stay the fsck away from that pile of
> garbage for a couple of weeks.
>
> _All_ partition-related code is getting rewritten and the last thing
> we need right now is additional clutter in the neighborhood. And
> devfs_fs_kernel.h, shite as it is, qualifies.
No problem as long as long somebody cares.
That part stuff needs treatment as well is obvious if one looks at the
extensive allocation fallback chains I have in my small sand pille...
Would you dare to keep the following botch in mind as well please:
/*
* Returns the (struct ata_device *) for a given device number. Return
* NULL if the given device number does not match any present drives.
*/
struct ata_device *get_info_ptr(kdev_t i_rdev)
{
unsigned int major = major(i_rdev);
int h;
for (h = 0; h < MAX_HWIFS; ++h) {
struct ata_channel *ch = &ide_hwifs[h];
if (ch->present && major == ch->major) {
int unit = DEVICE_NR(i_rdev);
if (unit < MAX_DRIVES) {
struct ata_device *drive = &ch->drives[unit];
if (drive->present)
return drive;
}
break;
}
}
return NULL;
}
This get's feed to the revalidate method.
struct block_device_operations ide_fops[] = {{
.owner = THIS_MODULE,
.open = ide_open,
.release = ide_release,
.ioctl = ata_ioctl,
.check_media_change = ide_check_media_change,
.revalidate = ata_revalidate
}};
and the following ide_xlate_1024(kdev_t i_rdev botch.
I would love to go the bdev way there too :-).
But then please keep in mind that the georgeous random number
device is using the major number of a device all over the kernel...
and it's feeding the following ugly global array:
void add_blkdev_randomness(int major)
{
if (major >= MAX_BLKDEV)
return;
if (blkdev_timer_state[major] == 0) {
rand_initialize_blkdev(major, GFP_ATOMIC);
if (blkdev_timer_state[major] == 0)
return;
}
add_timer_randomness(blkdev_timer_state[major], 0x200+major);
}
Which should of course look more like:
void add_blkdev_randomness(struct block_device *ptr)
{
add_timer_randomness(((unsigned long) ptr) %
SOME_REASONABLE_VALUE, 0x200);
}
Simple couldn't resist:
1. Enabled devfs... system printed far too long
incomprehensive device names and didn't reboot.
2. I disabled automatic devfs mount... system didn't find root part either.
The only single expirence in my life, where I thought that naming disks
C: D: E: Z: isn't the worst thing that can happen.
I went curious and looked there... and *cry*.
Marcin Dalecki writes:
> Richard Gooch wrote:
> > Marcin Dalecki writes:
> >
> >>Kill two inlines which are notwhere used and which don't make sense
> >>in the case someone is not compiling devfs at all.
> >
> >
> > Rejected. Linus, please don't apply this bogus patch. External patches
> > and drivers rely on the inline stubs so that #ifdef CONFIG_DEVFS_FS
> > isn't needed.
>
> Dare to actually *name* one of them?
Apart from my own sdmany patch, other people have contacted me about
this feature and said they were making use of it. I don't bother
tracking everyone who uses my code. I've got better things to do.
In any case, I don't *need* to justify it to you, particularly not
when the compiler completely optimises the inline stubs away. There is
*zero* benefit to removing them, and doing so only breaks external
code.
> You didn't think doing devfs_fs_kernel.h. One simple sample from there:
>
> devfs_get_maj_min(devfs_get_handle_from_inode((inode))
You've managed to pick a function that I'm not thrilled about
either. But it has been necessary.
> Everybody would expect the following to be only a single function:
>
> extern devfs_handle_t devfs_get_handle (devfs_handle_t dir, const char
> extern devfs_handle_t devfs_find_handle (devfs_handle_t dir, const char
devfs_get_handle() is the preferred interface. For compatibility
reasons, devfs_find_handle() remains. However, if you look at the
implementation for devfs_find_handle(), you'll notice that it's marked
for removal. But I believe in stable interfaces, so I want to give
people time to transition.
> And it was of course too hard to unify ops and handle:
>
> extern void *devfs_get_ops (devfs_handle_t de);
> extern void devfs_put_ops (devfs_handle_t de);
Huh? They are different animals.
> You couldn't resist adding the redundant devfs_ prefix overall in the
> kernel:
>
> extern devfs_register_chrdev (unsigned int major, const char *name,
> struct file_operations *fops);
> extern int devfs_register_blkdev (unsigned int major, const char *name,
> struct block_device_operations *bdops);
> extern int devfs_unregister_chrdev (unsigned int major, const char *name);
> extern int devfs_unregister_blkdev (unsigned int major, const char *name);
These do subtly different things than the non "devfs_" versions.
> Three different allocators and deallocators for one single subsystem,
> preserving the illusion that there is in linux a real difference between
> major and minor numbers...
>
> extern int devfs_alloc_major (char type);
> extern void devfs_dealloc_major (char type, int major);
> extern kdev_t devfs_alloc_devnum (char type);
> extern void devfs_dealloc_devnum (char type, kdev_t devnum);
Well, there *is* a difference between major and minor numbers. The two
different interfaces service different driver requirements.
Fortunately, one interface is built on top of the other.
> extern int devfs_alloc_unique_number (struct unique_numspace *space);
> extern void devfs_dealloc_unique_number (struct unique_numspace *space,
> int number);
These are completely unrelated to device numbers, so there's no point
even comparing them.
> If flags are invalid -> add an invalid flag! instead of value return
> through pointer.
>
> static inline int devfs_get_flags (devfs_handle_t de, unsigned int *flags)
> {
> return 0;
> }
That's a spurious objection. The return value indicates whether the
entry was valid or not. That is quite separate from flag values.
Mixing data types by overloading the return value is not a sensible
approach.
Regards,
Richard....
Permanent: [email protected]
Current: [email protected]