* double proc entries
@ 2006-08-21 2:49 danny
2006-08-21 8:49 ` Yu Luming
0 siblings, 1 reply; 6+ messages in thread
From: danny @ 2006-08-21 2:49 UTC (permalink / raw)
To: linux-acpi
[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]
Hi,
Recently I noticed that unloading video.ko causes a warning from remove_proc_entry because the subdir is not empty. This is related to the fact that I have 2 VID
entries in /proc/acpi/video, and this messes up things a bit. The cause of this is that the VID entry appears both on the PCI and on the AGP bus, sysfs handles this
nicely:
./firmware/acpi/namespace/ACPI/_SB/PCI0/AGP/VID
./firmware/acpi/namespace/ACPI/_SB/PCI0/AGP/VID/DVI0
./firmware/acpi/namespace/ACPI/_SB/PCI0/AGP/VID/CRT0
./firmware/acpi/namespace/ACPI/_SB/PCI0/AGP/VID/LCD0
./firmware/acpi/namespace/ACPI/_SB/PCI0/VID
./firmware/acpi/namespace/ACPI/_SB/PCI0/VID/DVI0
./firmware/acpi/namespace/ACPI/_SB/PCI0/VID/CRT0
./firmware/acpi/namespace/ACPI/_SB/PCI0/VID/LCD0
More information can be found here:
http://qa.mandriva.com/show_bug.cgi?id=22249
proc/acpi/video/ does not know about AGP or PCI. Attached patch fixes the problem but is not so beautiful.
Maybe it's better to make another subdir in video for the parent of the device, but I thought this would be more likely to break userland apps.
Perhaps someone can do better.
Regards,
Danny
(Please CC on reply).
[-- Attachment #2: acpi_fix-double-proc-entries.patch --]
[-- Type: text/plain, Size: 2290 bytes --]
--- linux/drivers/acpi/video.c.orig 2006-06-20 18:31:55.000000000 +0900
+++ linux/drivers/acpi/video.c 2006-08-21 11:23:29.000000000 +0900
@@ -1183,13 +1183,18 @@
{
struct proc_dir_entry *entry = NULL;
struct acpi_video_bus *video;
+ char proc_dir_name[32];
ACPI_FUNCTION_TRACE("acpi_video_bus_add_fs");
video = (struct acpi_video_bus *)acpi_driver_data(device);
-
+
if (!acpi_device_dir(device)) {
- acpi_device_dir(device) = proc_mkdir(acpi_device_bid(device),
+ strcpy(proc_dir_name, acpi_device_bid(device));
+ strcat(proc_dir_name, "_");
+ strcat(proc_dir_name, acpi_device_bid(device->parent));
+
+ acpi_device_dir(device) = proc_mkdir(proc_dir_name,
acpi_video_dir);
if (!acpi_device_dir(device))
return_VALUE(-ENODEV);
@@ -1265,6 +1270,7 @@
static int acpi_video_bus_remove_fs(struct acpi_device *device)
{
struct acpi_video_bus *video;
+ char proc_dir_name[32];
ACPI_FUNCTION_TRACE("acpi_video_bus_remove_fs");
@@ -1276,7 +1282,11 @@
remove_proc_entry("POST_info", acpi_device_dir(device));
remove_proc_entry("POST", acpi_device_dir(device));
remove_proc_entry("DOS", acpi_device_dir(device));
- remove_proc_entry(acpi_device_bid(device), acpi_video_dir);
+
+ strcpy(proc_dir_name, acpi_device_bid(device));
+ strcat(proc_dir_name, "_");
+ strcat(proc_dir_name, acpi_device_bid(device->parent));
+ remove_proc_entry(proc_dir_name, acpi_video_dir);
acpi_device_dir(device) = NULL;
}
@@ -1748,12 +1758,13 @@
int result = 0;
acpi_status status = 0;
struct acpi_video_bus *video = NULL;
-
+ char proc_dir_name[32];
+
ACPI_FUNCTION_TRACE("acpi_video_bus_add");
if (!device)
return_VALUE(-EINVAL);
-
+
video = kmalloc(sizeof(struct acpi_video_bus), GFP_KERNEL);
if (!video)
return_VALUE(-ENOMEM);
@@ -1789,8 +1800,12 @@
goto end;
}
+ strcpy(proc_dir_name, acpi_device_bid(device));
+ strcat(proc_dir_name, "_");
+ strcat(proc_dir_name, acpi_device_bid(device->parent));
+
printk(KERN_INFO PREFIX "%s [%s] (multi-head: %s rom: %s post: %s)\n",
- ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
+ ACPI_VIDEO_DEVICE_NAME, proc_dir_name,
video->flags.multihead ? "yes" : "no",
video->flags.rom ? "yes" : "no",
video->flags.post ? "yes" : "no");
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: double proc entries
2006-08-21 2:49 double proc entries danny
@ 2006-08-21 8:49 ` Yu Luming
2006-08-21 8:50 ` danny
0 siblings, 1 reply; 6+ messages in thread
From: Yu Luming @ 2006-08-21 8:49 UTC (permalink / raw)
To: danny; +Cc: linux-acpi, Brown, Len
On Monday 21 August 2006 10:49, danny@mailmij.org wrote:
> Hi,
> Recently I noticed that unloading video.ko causes a warning from
> remove_proc_entry because the subdir is not empty. This is related to the
> fact that I have 2 VID entries in /proc/acpi/video, and this messes up
> things a bit. The cause of this is that the VID entry appears both on the
> PCI and on the AGP bus, sysfs handles this nicely:
> ./firmware/acpi/namespace/ACPI/_SB/PCI0/AGP/VID
>...
Yes, This is a problem.
> More information can be found here:
> http://qa.mandriva.com/show_bug.cgi?id=22249
>
> proc/acpi/video/ does not know about AGP or PCI. Attached patch fixes the
> problem but is not so beautiful. Maybe it's better to make another subdir
> in video for the parent of the device, but I thought this would be more
> likely to break userland apps. Perhaps someone can do better.
>
The beautiful way is to completely delete /proc/acpi, and turn to sysfs. :-)
But we still have to live with /proc/acpi for some times.
So, I think your patch is right thing.
>+ strcpy(proc_dir_name, acpi_device_bid(device));
>+ strcat(proc_dir_name, "_");
>+ strcat(proc_dir_name, acpi_device_bid(device->parent));
but, when you are using acpi_device_bid(device->parent), you
should have checked device->parent is NOT NULL.
--
Thanks,
Luming
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: double proc entries
2006-08-21 8:49 ` Yu Luming
@ 2006-08-21 8:50 ` danny
2006-08-21 22:02 ` Len Brown
0 siblings, 1 reply; 6+ messages in thread
From: danny @ 2006-08-21 8:50 UTC (permalink / raw)
To: Yu Luming; +Cc: danny, linux-acpi, Brown, Len
On Mon, Aug 21, 2006 at 04:49:12PM +0800, Yu Luming wrote:
> On Monday 21 August 2006 10:49, danny@mailmij.org wrote:
> > Hi,
> > Recently I noticed that unloading video.ko causes a warning from
> > remove_proc_entry because the subdir is not empty. This is related to the
> > fact that I have 2 VID entries in /proc/acpi/video, and this messes up
> > things a bit. The cause of this is that the VID entry appears both on the
> > PCI and on the AGP bus, sysfs handles this nicely:
> > ./firmware/acpi/namespace/ACPI/_SB/PCI0/AGP/VID
> >...
>
> Yes, This is a problem.
>
> > More information can be found here:
> > http://qa.mandriva.com/show_bug.cgi?id=22249
> >
> > proc/acpi/video/ does not know about AGP or PCI. Attached patch fixes the
> > problem but is not so beautiful. Maybe it's better to make another subdir
> > in video for the parent of the device, but I thought this would be more
> > likely to break userland apps. Perhaps someone can do better.
> >
>
> The beautiful way is to completely delete /proc/acpi, and turn to sysfs. :-)
> But we still have to live with /proc/acpi for some times.
> So, I think your patch is right thing.
>
> >+ strcpy(proc_dir_name, acpi_device_bid(device));
> >+ strcat(proc_dir_name, "_");
> >+ strcat(proc_dir_name, acpi_device_bid(device->parent));
>
> but, when you are using acpi_device_bid(device->parent), you
> should have checked device->parent is NOT NULL.
>
Yes I considered this, but then I thought, if there is actually a VID bus, it always has a parent. You cannot have a bus that correctly passes the checks for being
the acpi video device and not have a parent bus. Or can you?
Second question is the size of proc_dir_name: is 32 big enough? Should I have used strncat. I really have no idea at the maximum possible size of the names we get
from the dsdt.
Danny
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: double proc entries
2006-08-21 8:50 ` danny
@ 2006-08-21 22:02 ` Len Brown
2006-08-24 1:46 ` danny
0 siblings, 1 reply; 6+ messages in thread
From: Len Brown @ 2006-08-21 22:02 UTC (permalink / raw)
To: danny; +Cc: Yu Luming, linux-acpi
On Monday 21 August 2006 04:50, danny@mailmij.org wrote:
> On Mon, Aug 21, 2006 at 04:49:12PM +0800, Yu Luming wrote:
> > On Monday 21 August 2006 10:49, danny@mailmij.org wrote:
> > > Hi,
> > > Recently I noticed that unloading video.ko causes a warning from
> > > remove_proc_entry because the subdir is not empty. This is related to the
> > > fact that I have 2 VID entries in /proc/acpi/video, and this messes up
> > > things a bit. The cause of this is that the VID entry appears both on the
> > > PCI and on the AGP bus, sysfs handles this nicely:
> > > ./firmware/acpi/namespace/ACPI/_SB/PCI0/AGP/VID
> > >...
> >
> > Yes, This is a problem.
> >
> > > More information can be found here:
> > > http://qa.mandriva.com/show_bug.cgi?id=22249
> > >
> > > proc/acpi/video/ does not know about AGP or PCI. Attached patch fixes the
> > > problem but is not so beautiful. Maybe it's better to make another subdir
> > > in video for the parent of the device, but I thought this would be more
> > > likely to break userland apps. Perhaps someone can do better.
> > >
> >
> > The beautiful way is to completely delete /proc/acpi, and turn to sysfs. :-)
> > But we still have to live with /proc/acpi for some times.
> > So, I think your patch is right thing.
> >
> > >+ strcpy(proc_dir_name, acpi_device_bid(device));
> > >+ strcat(proc_dir_name, "_");
> > >+ strcat(proc_dir_name, acpi_device_bid(device->parent));
> >
> > but, when you are using acpi_device_bid(device->parent), you
> > should have checked device->parent is NOT NULL.
> >
> Yes I considered this, but then I thought, if there is actually a VID bus, it always has a parent. You cannot have a bus that correctly passes the checks for being
> the acpi video device and not have a parent bus. Or can you?
>
> Second question is the size of proc_dir_name: is 32 big enough? Should I have used strncat. I really have no idea at the maximum possible size of the names we get
> from the dsdt.
Ugh, I'd rather be deleting /proc/acpi than fixing bugs in it...
but I think you're looking for this:
#define ACPI_NAME_SIZE 4
or, more specifically,
typedef char acpi_bus_id[5];
is the type returned from acpi-device_bid()
-Len
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: double proc entries
2006-08-21 22:02 ` Len Brown
@ 2006-08-24 1:46 ` danny
2006-09-06 7:27 ` danny
0 siblings, 1 reply; 6+ messages in thread
From: danny @ 2006-08-24 1:46 UTC (permalink / raw)
To: Len Brown; +Cc: danny, Yu Luming, linux-acpi
Ok. I made an updated version. I leave it to you to decide to remove the proc entries or to push
something like this upstream for the time being. On my laptop this patch seems to resolve the problems.
One remaining question: why is
typedef char acpi_bus_id[5];
and not:
typedef char acpi_bus_id[ACPI_NAME_SIZE+1];
If you went through the trouble of defining the size, than it is better to use it everywhere?
Regards,
Danny
Acpi bus ids are not necessarily unique. For example, the VID entry can appear on both
the PCI and on the AGP bus. The proc entries for the acpi video devices were not aware
of this. Therefore, a double VID entry could occur in /proc/acpi/video/. This also lead
to a warning on remove_proc_entry because the wrong directory was removed first.
This patch renames the VID entries to include their parent bus (ie VID_PCI0 and VID_AGP).
Signed-off-by: Danny Tholen <obiwan@mailmij.org>
--- linux/drivers/acpi/video.c.orig 2006-06-20 18:31:55.000000000 +0900
+++ linux/drivers/acpi/video.c 2006-08-21 11:23:29.000000000 +0900
@@ -1183,13 +1183,18 @@
{
struct proc_dir_entry *entry = NULL;
struct acpi_video_bus *video;
+ char proc_dir_name[ACPI_NAME_SIZE*2+2];
ACPI_FUNCTION_TRACE("acpi_video_bus_add_fs");
video = (struct acpi_video_bus *)acpi_driver_data(device);
if (!acpi_device_dir(device)) {
- acpi_device_dir(device) = proc_mkdir(acpi_device_bid(device),
+ strncpy(proc_dir_name, acpi_device_bid(device), ACPI_NAME_SIZE);
+ strcat(proc_dir_name, "_");
+ strncat(proc_dir_name, acpi_device_bid(device->parent), ACPI_NAME_SIZE);
+
+ acpi_device_dir(device) = proc_mkdir(proc_dir_name,
acpi_video_dir);
if (!acpi_device_dir(device))
return_VALUE(-ENODEV);
@@ -1265,6 +1270,7 @@
static int acpi_video_bus_remove_fs(struct acpi_device *device)
{
struct acpi_video_bus *video;
+ char proc_dir_name[ACPI_NAME_SIZE*2+2];
ACPI_FUNCTION_TRACE("acpi_video_bus_remove_fs");
@@ -1276,7 +1282,11 @@
remove_proc_entry("POST_info", acpi_device_dir(device));
remove_proc_entry("POST", acpi_device_dir(device));
remove_proc_entry("DOS", acpi_device_dir(device));
- remove_proc_entry(acpi_device_bid(device), acpi_video_dir);
+
+ strncpy(proc_dir_name, acpi_device_bid(device), ACPI_NAME_SIZE);
+ strcat(proc_dir_name, "_");
+ strncat(proc_dir_name, acpi_device_bid(device->parent), ACPI_NAME_SIZE);
+ remove_proc_entry(proc_dir_name, acpi_video_dir);
acpi_device_dir(device) = NULL;
}
@@ -1748,6 +1758,7 @@
int result = 0;
acpi_status status = 0;
struct acpi_video_bus *video = NULL;
+ char proc_dir_name[ACPI_NAME_SIZE*2+2];
ACPI_FUNCTION_TRACE("acpi_video_bus_add");
@@ -1789,8 +1800,9 @@
goto end;
}
- printk(KERN_INFO PREFIX "%s [%s] (multi-head: %s rom: %s post: %s)\n",
+ printk(KERN_INFO PREFIX "%s [%s_%s] (multi-head: %s rom: %s post: %s)\n",
ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
+ acpi_device_bid(device->parent),
video->flags.multihead ? "yes" : "no",
video->flags.rom ? "yes" : "no",
video->flags.post ? "yes" : "no");
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: double proc entries
2006-08-24 1:46 ` danny
@ 2006-09-06 7:27 ` danny
0 siblings, 0 replies; 6+ messages in thread
From: danny @ 2006-09-06 7:27 UTC (permalink / raw)
To: danny; +Cc: Len Brown, Yu Luming, linux-acpi
On Thu, Aug 24, 2006 at 03:46:21AM +0200, danny@mailmij.org wrote:
> Ok. I made an updated version. I leave it to you to decide to remove the proc entries or to push
> something like this upstream for the time being. On my laptop this patch seems to resolve the problems.
>
<snip>
Please ignore that version. It was severly broken (oops) the moment the name of video bus was not 3 but
4 bytes long (the string would not be nulterminated by strcpy, leading to overflows if we use strcat
afterthat). Thanks to Arnaud Patard for pointing this out to me, and also for the fix: just initialize
all elements to 0.
Updated version follows.
Acpi bus ids are not necessarily unique. For example, the VID entry can appear on both
the PCI and on the AGP bus. The proc entries for the acpi video devices were not aware
of this. Therefore, a double VID entry could occur in /proc/acpi/video/. This also lead
to a warning on remove_proc_entry because the wrong directory was removed first.
This patch renames the VID entries to include their parent bus (ie VID_PCI0 and VID_AGP).
Signed-off-by: Danny Tholen <obiwan@mailmij.org>
--- linux-2.6.17.1/drivers/acpi/video.c.orig 2006-09-06 08:27:13.000000000 +0200
+++ linux-2.6.17.1/drivers/acpi/video.c 2006-09-06 09:17:14.000000000 +0200
@@ -1183,13 +1183,19 @@
{
struct proc_dir_entry *entry = NULL;
struct acpi_video_bus *video;
+ char proc_dir_name[ACPI_NAME_SIZE*2+2];
ACPI_FUNCTION_TRACE("acpi_video_bus_add_fs");
video = (struct acpi_video_bus *)acpi_driver_data(device);
if (!acpi_device_dir(device)) {
- acpi_device_dir(device) = proc_mkdir(acpi_device_bid(device),
+ memset(proc_dir_name, 0x00, ACPI_NAME_SIZE*2+2);
+ strncpy(proc_dir_name, acpi_device_bid(device), ACPI_NAME_SIZE);
+ strcat(proc_dir_name, "_");
+ strncat(proc_dir_name, acpi_device_bid(device->parent), ACPI_NAME_SIZE);
+
+ acpi_device_dir(device) = proc_mkdir(proc_dir_name,
acpi_video_dir);
if (!acpi_device_dir(device))
return_VALUE(-ENODEV);
@@ -1265,6 +1271,7 @@
static int acpi_video_bus_remove_fs(struct acpi_device *device)
{
struct acpi_video_bus *video;
+ char proc_dir_name[ACPI_NAME_SIZE*2+2];
ACPI_FUNCTION_TRACE("acpi_video_bus_remove_fs");
@@ -1276,7 +1283,12 @@
remove_proc_entry("POST_info", acpi_device_dir(device));
remove_proc_entry("POST", acpi_device_dir(device));
remove_proc_entry("DOS", acpi_device_dir(device));
- remove_proc_entry(acpi_device_bid(device), acpi_video_dir);
+
+ memset(proc_dir_name, 0x00, ACPI_NAME_SIZE*2+2);
+ strncpy(proc_dir_name, acpi_device_bid(device), ACPI_NAME_SIZE);
+ strcat(proc_dir_name, "_");
+ strncat(proc_dir_name, acpi_device_bid(device->parent), ACPI_NAME_SIZE);
+ remove_proc_entry(proc_dir_name, acpi_video_dir);
acpi_device_dir(device) = NULL;
}
@@ -1796,8 +1808,9 @@
goto end;
}
- printk(KERN_INFO PREFIX "%s [%s] (multi-head: %s rom: %s post: %s)\n",
+ printk(KERN_INFO PREFIX "%s [%s_%s] (multi-head: %s rom: %s post: %s)\n",
ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
+ acpi_device_bid(device->parent),
video->flags.multihead ? "yes" : "no",
video->flags.rom ? "yes" : "no",
video->flags.post ? "yes" : "no");
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-09-06 7:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-08-21 2:49 double proc entries danny
2006-08-21 8:49 ` Yu Luming
2006-08-21 8:50 ` danny
2006-08-21 22:02 ` Len Brown
2006-08-24 1:46 ` danny
2006-09-06 7:27 ` danny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox