Bug 8798

Summary: Duplicate VID directory in /proc/acpi/video
Product: ACPI Reporter: Chris Rankin (rankincj)
Component: Power-VideoAssignee: Zhang Rui (rui.zhang)
Status: CLOSED CODE_FIX    
Severity: normal CC: acpi-bugzilla
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.22 Subsystem:
Regression: --- Bisected commit-id:
Attachments: dmesg output
file names must be different in the same directory.
use device bus_id in procfs
patch vs 2.6.23-rc1 from comment #5
patch: use unique ACPI device names in procfs
patch-use-unique-device-name-in-procfs
patch-video-driver-use-unique-device-name-in-procfs
patch: hack the duplicate AML name "VID" problem on T61

Description Chris Rankin 2007-07-23 15:30:17 UTC
Most recent kernel where this bug did not occur:
Distribution:
Hardware Environment:
Software Environment:
Problem Description:


Steps to reproduce:
Comment 1 Chris Rankin 2007-07-23 15:32:23 UTC
Oops.

Anyway, the problem is:

# ls -alis /proc/acpi/video
total 0
4026532491 0 dr-xr-xr-x  4 root root 0 2007-07-23 23:20 .
4026531915 0 dr-xr-xr-x 12 root root 0 2007-07-23 23:13 ..
4026532513 0 dr-xr-xr-x  5 root root 0 2007-07-23 23:25 VID
4026532513 0 dr-xr-xr-x  5 root root 0 2007-07-23 23:25 VID

The VID/ directory is appearing in the directory listing more than once.
Comment 2 Chris Rankin 2007-07-23 15:34:40 UTC
Created attachment 12109 [details]
dmesg output
Comment 3 Zhang Rui 2007-07-24 18:07:06 UTC
Hmm,
First, the device name we export in /proc is from BIOS. We can't promise that the name is unique. So we should use something else rather than the string from ACPI namespace.
Second, proc_mkdir() doesn't inspect for duplicated names. Anyway, this is another bug and I'll send out a patch.
Comment 4 Zhang Rui 2007-07-24 23:44:54 UTC
Created attachment 12131 [details]
file names must be different in the same directory.
Comment 5 Zhang Rui 2007-07-24 23:50:58 UTC
Created attachment 12132 [details]
use device bus_id in procfs

This will change the /proc/acpi a lot.
But we need something else as the device name rather than the arbitrary stings from BIOS.
IMO, this is the most convient and reasonable way to fix it.
And I don't see there are any user space tools relying on the old device names.
Comment 6 Len Brown 2007-07-25 19:03:29 UTC
Chris,
Does patch in comment #5 work for you?
Comment 7 Len Brown 2007-07-25 23:29:20 UTC
Created attachment 12149 [details]
patch vs 2.6.23-rc1 from comment #5

applied to acpi-test
Comment 8 Chris Rankin 2007-07-26 14:43:27 UTC
Yes, that patch has had the desired effect (I think):

# ls -alsi /proc/acpi/video/
total 0
4026532136 0 dr-xr-xr-x  4 root root 0 2007-07-26 22:32 .
4026531915 0 dr-xr-xr-x 12 root root 0 2007-07-26 22:10 ..
4026532137 0 dr-xr-xr-x  5 root root 0 2007-07-26 22:35 video:00
4026532158 0 dr-xr-xr-x  5 root root 0 2007-07-26 22:35 video:01

# cat /sys/bus/acpi/devices/video\:0?/device\:0?/path
\_SB_.PCI0.VID_.LCD0
\_SB_.PCI0.VID_.CRT0
\_SB_.PCI0.VID_.DVI0
\_SB_.PCI0.AGP_.VID_.LCD0
\_SB_.PCI0.AGP_.VID_.CRT0
\_SB_.PCI0.AGP_.VID_.DVI0

Why is there an AGP video device? I thought this laptop used PCI-Express.
Comment 9 Zhang Rui 2007-07-28 01:01:12 UTC
Len,
please drop the patch in comment #5 from acpi-test.
It breaks the sbs driver.
Comment 10 Zhang Rui 2007-07-31 04:12:43 UTC
Created attachment 12212 [details]
patch: use unique ACPI device names in procfs
Comment 11 Len Brown 2007-08-02 08:18:34 UTC
re: patch in comment #4 to error-out in generic procfs code
on registration of files with duplicate names.
Maybe this is correct, but as we are abandoning procfs
and as it doesn't really fix the issue at hand where we presumably
need two different files, so lets not apply this one.

re: patch in comment #5 to change the names in all of /proc/acpi/*/*
Again, probably technically correct, and this one actually should
fix the problem at hand, but as we want /proc/acpi/
to become legacy, lets not stir the pot by changing the strings
on everybody.  It will probably just upset somebody
who will have to make a userspace change.  Instead, lets save
that pain for their migration to sysfs.  Also, per comment #9,
this patch causes sbs to fail because it was counting on the
fixed size of these strings:-(


Re: comment #8 why AGP_
Chris, these strings are completely arbitrary, it could just
as well be called ABCD -- it is up to the BIOS developer to
choose (or in this case, probably inherit) whatever strings
they want.  Part of the migration from procfs to sysfs is to
stop exposing (and confusing) user-space with these internal
AML variable names -- they make for a really poor user/kernel API.

re: patch in comment #10 to change all /proc/acpi/*/*
Again, too disruptive for a legacy interface.

re: what to do
The problem at hand is two /proc/acpi/video/VID files
We should make the late-comer /proc/acpi/video/VID:1 or something
to make this "instance" of it unique.  It would be ideal if the
solution applies to all of /proc/acpi/*/* -- but I'd settle
for a modification to the video.c file that solves just
this instance of perhaps a more general problem -- since we've
not seen the problem elsewhere, and we want to leave procfs
behind with as few changes to it as possible.
Comment 12 Zhang Rui 2007-08-06 22:36:36 UTC
Created attachment 12285 [details]
patch-use-unique-device-name-in-procfs
Comment 13 Zhang Rui 2007-08-06 22:54:51 UTC
Created attachment 12286 [details]
patch-video-driver-use-unique-device-name-in-procfs

Patches in comment#4 #12 are needed as well.

Len,
This can be a generic solution for all ACPI devices,
which need only few change in the ACPI driver code.

BTW: IMO, allowing duplicate name in procfs is a bug,
the patch in comment#4 fix this bug in procfs generic code,
and I think it should be sent to linux-fsdevel, do you think so?
Comment 14 Len Brown 2007-08-15 20:32:07 UTC
yes, please send the patch in comment #4 directly upstream.

Re: patch in comment #12 -- the code to
generate the new name should be in a single macro or routine,
not duplicated in multiple places.  Also, it strikes me as a lot
of code to handle a corner-case in a feature we are trying to delete.

I think it would be better to ship patch in comment #4 
and fail to create the duplicate, and focus on getting
this driver properly supported in sysfs.  Either that,
or some really small hack that fixes the problem at hand
(say, only for the video driver), but is ifdef'd to be
deleted with the proc code.
Comment 15 Zhang Rui 2007-08-21 02:06:57 UTC
Created attachment 12467 [details]
patch: hack the duplicate AML name "VID" problem on T61
Comment 16 Len Brown 2007-08-25 21:07:26 UTC
patch in comment #15 shipped in 2.6.23-rc3-git9,
closed.