Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Arjan van de Ven]

This patch adds an "enable" sysfs attribute to each PCI device. When read it
shows the "enabled-ness" of the device, but you can write a "0" into it to
disable a device, and a "1" to enable it.

This later is needed for X and other cases where userspace wants to enable
the BARs on a device (typical example: to run the video bios on a secundary
head). Right now X does all this "by hand" via bitbanging, that's just evil.
This allows X to no longer do that but to just let the kernel do this.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
CC: Peter Jones <pjones@redhat.com>
CC: Dave Airlie <airlied@linux.ie>
---
 drivers/pci/pci-sysfs.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Index: linux-2.6.17-rc2-enable/drivers/pci/pci-sysfs.c
===================================================================
--- linux-2.6.17-rc2-enable.orig/drivers/pci/pci-sysfs.c
+++ linux-2.6.17-rc2-enable/drivers/pci/pci-sysfs.c
@@ -43,6 +43,7 @@ pci_config_attr(subsystem_vendor, "0x%04
 pci_config_attr(subsystem_device, "0x%04x\n");
 pci_config_attr(class, "0x%06x\n");
 pci_config_attr(irq, "%u\n");
+pci_config_attr(is_enabled, "%u\n");
 
 static ssize_t local_cpus_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
@@ -90,6 +91,28 @@ static ssize_t modalias_show(struct devi
 		       (u8)(pci_dev->class >> 16), (u8)(pci_dev->class >> 8),
 		       (u8)(pci_dev->class));
 }
+static ssize_t
+is_enabled_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+        struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (!pdev)
+		return 1;
+
+	/* this can crash the machine when done on the "wrong" device */
+	if (!capable(CAP_SYS_ADMIN))
+		return 1;
+
+	if (*buf == '0')
+		pci_disable_device(pdev);
+
+	if (*buf == '1')
+		pci_enable_device(pdev);
+
+	return 1;
+}
+
 
 struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(resource),
@@ -101,6 +124,7 @@ struct device_attribute pci_dev_attrs[] 
 	__ATTR_RO(irq),
 	__ATTR_RO(local_cpus),
 	__ATTR_RO(modalias),
+	__ATTR(enable, 0600, is_enabled_show, is_enabled_store),
 	__ATTR_NULL,
 };
 

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Andrew Morton]

Arjan van de Ven <arjan@linux.intel.com> wrote:
>
> +static ssize_t
> +is_enabled_store(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t count)
> +{
> +        struct pci_dev *pdev = to_pci_dev(dev);

whitespace broke

> +	if (!pdev)
> +		return 1;

Can this happen?

> +	/* this can crash the machine when done on the "wrong" device */
> +	if (!capable(CAP_SYS_ADMIN))
> +		return 1;

Don't the file's permissions suffice?

> +	if (*buf == '0')
> +		pci_disable_device(pdev);
> +
> +	if (*buf == '1')
> +		pci_enable_device(pdev);
> +
> +	return 1;

	return count;

> +}
> +
>  
>  struct device_attribute pci_dev_attrs[] = {
>  	__ATTR_RO(resource),
> @@ -101,6 +124,7 @@ struct device_attribute pci_dev_attrs[] 
>  	__ATTR_RO(irq),
>  	__ATTR_RO(local_cpus),
>  	__ATTR_RO(modalias),
> +	__ATTR(enable, 0600, is_enabled_show, is_enabled_store),
>  	__ATTR_NULL,
>  };
>  

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Arjan van de Ven]

> > +	if (!pdev)
> > +		return 1;
> 
> Can this happen?

eh I suppose not; the other code doesn't check it either; fixed

> 
> > +	/* this can crash the machine when done on the "wrong" device */
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return 1;
> 
> Don't the file's permissions suffice?

that's a more philosophical question; you can ask that question about
the entire capability system ;) Other code in the same file uses this
same capability for a same level of access though.

> 	return count;
> 
fixed


ok new patch below

This patch adds an "enable" sysfs attribute to each PCI device. When read it
shows the "enabled-ness" of the device, but you can write a "0" into it to
disable a device, and a "1" to enable it.

This later is needed for X and other cases where userspace wants to enable
the BARs on a device (typical example: to run the video bios on a secundary
head). Right now X does all this "by hand" via bitbanging, that's just evil.
This allows X to no longer do that but to just let the kernel do this.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
CC: Peter Jones <pjones@redhat.com>
CC: Dave Airlie <airlied@linux.ie>
---
 drivers/pci/pci-sysfs.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Index: linux-2.6.17-rc2-enable/drivers/pci/pci-sysfs.c
===================================================================
--- linux-2.6.17-rc2-enable.orig/drivers/pci/pci-sysfs.c
+++ linux-2.6.17-rc2-enable/drivers/pci/pci-sysfs.c
@@ -43,6 +43,7 @@ pci_config_attr(subsystem_vendor, "0x%04
 pci_config_attr(subsystem_device, "0x%04x\n");
 pci_config_attr(class, "0x%06x\n");
 pci_config_attr(irq, "%u\n");
+pci_config_attr(is_enabled, "%u\n");
 
 static ssize_t local_cpus_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
@@ -90,6 +91,25 @@ static ssize_t modalias_show(struct devi
 		       (u8)(pci_dev->class >> 16), (u8)(pci_dev->class >> 8),
 		       (u8)(pci_dev->class));
 }
+static ssize_t
+is_enabled_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	/* this can crash the machine when done on the "wrong" device */
+	if (!capable(CAP_SYS_ADMIN))
+		return count;
+
+	if (*buf == '0')
+		pci_disable_device(pdev);
+
+	if (*buf == '1')
+		pci_enable_device(pdev);
+
+	return count;
+}
+
 
 struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(resource),
@@ -101,6 +121,7 @@ struct device_attribute pci_dev_attrs[] 
 	__ATTR_RO(irq),
 	__ATTR_RO(local_cpus),
 	__ATTR_RO(modalias),
+	__ATTR(enable, 0600, is_enabled_show, is_enabled_store),
 	__ATTR_NULL,
 };
 

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Dave Airlie]

> This patch adds an "enable" sysfs attribute to each PCI device. When read it
> shows the "enabled-ness" of the device, but you can write a "0" into it to
> disable a device, and a "1" to enable it.
>
> This later is needed for X and other cases where userspace wants to enable
> the BARs on a device (typical example: to run the video bios on a secundary
> head). Right now X does all this "by hand" via bitbanging, that's just evil.
> This allows X to no longer do that but to just let the kernel do this.
>
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> CC: Peter Jones <pjones@redhat.com>
> CC: Dave Airlie <airlied@linux.ie>

ACK

This would allow me to remove the issue in X where loading the DRM at X 
startup acts differently than loading the DRM before X runs, due to Xs PCI 
probe running in-between... with this I can just enable all VGA devices 
and no worry whether they have a DRM or not..

Dave.

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
Linux kernel - DRI, VAX / pam_smb / ILUG

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Bjorn Helgaas]

On Saturday 29 April 2006 03:04, Dave Airlie wrote:
> 
> > This patch adds an "enable" sysfs attribute to each PCI device. When read it
> > shows the "enabled-ness" of the device, but you can write a "0" into it to
> > disable a device, and a "1" to enable it.
> >
> > This later is needed for X and other cases where userspace wants to enable
> > the BARs on a device (typical example: to run the video bios on a secundary
> > head). Right now X does all this "by hand" via bitbanging, that's just evil.
> > This allows X to no longer do that but to just let the kernel do this.
> >
> > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> > CC: Peter Jones <pjones@redhat.com>
> > CC: Dave Airlie <airlied@linux.ie>
> 
> ACK
> 
> This would allow me to remove the issue in X where loading the DRM at X 
> startup acts differently than loading the DRM before X runs, due to Xs PCI 
> probe running in-between... with this I can just enable all VGA devices 
> and no worry whether they have a DRM or not..

This sysfs "enable" patch seems like goodness.

But I hope that when X uses this, it only enables & disables VGA
devices it's actually using.  In the past, it seems like X has
blindly disabled *all* VGA devices in the system, even though
they might be in use by another X server.  I'm sure that's all
well-understood and cleaned up now; just wanted to make sure
this nightmare didn't recur.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Greg KH]

On Tue, May 02, 2006 at 10:14:45AM -0600, Bjorn Helgaas wrote:
> On Saturday 29 April 2006 03:04, Dave Airlie wrote:
> > 
> > > This patch adds an "enable" sysfs attribute to each PCI device. When read it
> > > shows the "enabled-ness" of the device, but you can write a "0" into it to
> > > disable a device, and a "1" to enable it.
> > >
> > > This later is needed for X and other cases where userspace wants to enable
> > > the BARs on a device (typical example: to run the video bios on a secundary
> > > head). Right now X does all this "by hand" via bitbanging, that's just evil.
> > > This allows X to no longer do that but to just let the kernel do this.
> > >
> > > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> > > CC: Peter Jones <pjones@redhat.com>
> > > CC: Dave Airlie <airlied@linux.ie>
> > 
> > ACK
> > 
> > This would allow me to remove the issue in X where loading the DRM at X 
> > startup acts differently than loading the DRM before X runs, due to Xs PCI 
> > probe running in-between... with this I can just enable all VGA devices 
> > and no worry whether they have a DRM or not..
> 
> This sysfs "enable" patch seems like goodness.
> 
> But I hope that when X uses this, it only enables & disables VGA
> devices it's actually using.  In the past, it seems like X has
> blindly disabled *all* VGA devices in the system, even though
> they might be in use by another X server.  I'm sure that's all
> well-understood and cleaned up now; just wanted to make sure
> this nightmare didn't recur.

Hopefully with the recent PCI changes to X, this will not happen.  If it
does, that's a big bug in X :)

thanks,

greg k-h

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jesse Barnes]

> > But I hope that when X uses this, it only enables & disables VGA
> > devices it's actually using.  In the past, it seems like X has
> > blindly disabled *all* VGA devices in the system, even though
> > they might be in use by another X server.  I'm sure that's all
> > well-understood and cleaned up now; just wanted to make sure
> > this nightmare didn't recur.
>
> Hopefully with the recent PCI changes to X, this will not happen.  If
> it does, that's a big bug in X :)

On some machines it still has no alternative, since the kernel doesn't 
have a VGA arbiter of any kind.  Yes this sucks.

Jesse

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Bjorn Helgaas]

On Saturday 29 April 2006 03:04, Dave Airlie wrote:
> > This patch adds an "enable" sysfs attribute to each PCI device. When read it
> > shows the "enabled-ness" of the device, but you can write a "0" into it to
> > disable a device, and a "1" to enable it.
> >
> > This later is needed for X and other cases where userspace wants to enable
> > the BARs on a device (typical example: to run the video bios on a secundary
> > head). Right now X does all this "by hand" via bitbanging, that's just evil.
> > This allows X to no longer do that but to just let the kernel do this.

I'm all in favor of cleaning up X.  But making the X code prettier without
changing the underlying issues of claiming and sharing resources doesn't
help much.  In fact, I suspect the ultimate plan for X does not involve
an "enable" attribute in sysfs, so this may just introduce ABI cruft that
will be difficult to remove later.

> This would allow me to remove the issue in X where loading the DRM at X 
> startup acts differently than loading the DRM before X runs, due to Xs PCI 
> probe running in-between... with this I can just enable all VGA devices 
> and no worry whether they have a DRM or not..

This seems to be the main justification for the patch.  But I don't know
enough about X and DRM to understand it, or why this patch is the best
way to solve it.

I think Jon has a pretty convincing argument, which I *do* understand.
Can you expand on this justification?  Do you envision long-term usage
of the sysfs "enable" attribute?

Bjorn

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Arjan van de Ven]

Bjorn Helgaas wrote:
> On Saturday 29 April 2006 03:04, Dave Airlie wrote:
>>> This patch adds an "enable" sysfs attribute to each PCI device. When read it
>>> shows the "enabled-ness" of the device, but you can write a "0" into it to
>>> disable a device, and a "1" to enable it.
>>>
>>> This later is needed for X and other cases where userspace wants to enable
>>> the BARs on a device (typical example: to run the video bios on a secundary
>>> head). Right now X does all this "by hand" via bitbanging, that's just evil.
>>> This allows X to no longer do that but to just let the kernel do this.
> 
> I'm all in favor of cleaning up X.  But making the X code prettier without
> changing the underlying issues of claiming and sharing resources doesn't
> help much.  In fact, I suspect the ultimate plan for X does not involve
> an "enable" attribute in sysfs, so this may just introduce ABI cruft that
> will be difficult to remove later.

it goes well beyond X. Things like vbetool need this too to get to the content
of the rom for example. There are several other such cases...

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Bjorn Helgaas]

On Thursday 04 May 2006 13:11, Arjan van de Ven wrote:
> Bjorn Helgaas wrote:
> > On Saturday 29 April 2006 03:04, Dave Airlie wrote:
> >>> This patch adds an "enable" sysfs attribute to each PCI device. When read it
> >>> shows the "enabled-ness" of the device, but you can write a "0" into it to
> >>> disable a device, and a "1" to enable it.
> >>>
> >>> This later is needed for X and other cases where userspace wants to enable
> >>> the BARs on a device (typical example: to run the video bios on a secundary
> >>> head). Right now X does all this "by hand" via bitbanging, that's just evil.
> >>> This allows X to no longer do that but to just let the kernel do this.
> > 
> > I'm all in favor of cleaning up X.  But making the X code prettier without
> > changing the underlying issues of claiming and sharing resources doesn't
> > help much.  In fact, I suspect the ultimate plan for X does not involve
> > an "enable" attribute in sysfs, so this may just introduce ABI cruft that
> > will be difficult to remove later.
> 
> it goes well beyond X. Things like vbetool need this too to get to the content
> of the rom for example. There are several other such cases...

There's already a "rom" file in sysfs.  Could vbetool and friends
use that?

How do vbetool and X coordinate their usage of "enable"?  What if we
throw an in-kernel VGA driver into the mix?  But I guess Jon has asked
all these questions before; I just didn't get warm fuzzies that there
were safe, maintainable answers.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Matthew Garrett]

Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:

> There's already a "rom" file in sysfs.  Could vbetool and friends
> use that?

Not if you have multiple graphics cards.

> How do vbetool and X coordinate their usage of "enable"?

vbetool won't run at anything other than a text console, and X won't 
mess with the graphics card if it's not on the current VT. You can mess 
this up if you try hard enough (multihead, for instance) but by and 
large it's a situation that you can avoid.

> What if we throw an in-kernel VGA driver into the mix?  But I guess
> Jon has asked all these questions before; I just didn't get warm
> fuzzies that there were safe, maintainable answers.

This probably isn't the right long-term answer, but the right long-term 
answer is going to be a very long time away. It's an improvement over 
what we have now. I certainly don't intend to leave vbetool relying on 
it - of course, the "right" answer is for graphics drivers to know how 
to program cards from scratch so we can get rid of vbetool altogether, 
but I'll probably be more concerned about getting my flying car to meet 
new emission standards than I will be by graphics cards at that stage.

-- 
Matthew Garrett | mjg59-chiark.mail.linux-rutgers.kernel@srcf.ucam.org

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/4/06, Matthew Garrett <mgarrett@chiark.greenend.org.uk> wrote:
> Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
>
> > There's already a "rom" file in sysfs.  Could vbetool and friends
> > use that?
>
> Not if you have multiple graphics cards.

Not true, the rom attribute maps the ROM into PCI space where ever the
kernel tells it to and reads it from there. It is the PCI VGA
emulation feature that forces the ROM to appear at C000:0. You can
have the ROM mapped and VGA emulation turned off.

This brings up another major point. X changes the PCI VGA emulation
routing from user space, another thing that it should not be doing. I
have posted patches before providing a sysfs VGA attribute on class
VGA devices. By setting the attribute to 1 you can control the active
VGA emulation device.

This is yet another way that user space can mess up the kernel. If VGA
routing is changes under fbdev (my attribute notifies fbdev, the fbdev
code for processing the notification did get checked in) then the
console will screw up. The usual screw up is that the console goes
blank because hardware fonts are not setup correctly on the new
console.

I also remember posting a patch for initializing all class VGA devices
at boot. This is a complication process since running the ROM will
leave that card as the active VGA device.  The initialization code
made sure to set the console back to the original VGA card after the
secondary one was initialized.  The initialization process needs to
run a user space app which provides real mode access or an x86
emulator. BenH has the x86 emulator code (people sticking PC hardware
into PowerPCs need the emulator). This code for running the ROM should
go into the kernel tree and work with klibc.

I would really like to see a well designed, comprehensive plan for
handling these issues instead of just providing convenience APIs (that
may be hard to get rid of) for the old X code. I'm willing to help if
people really are serious about fixing the problems.

I wrote down a lot of my thoughts on this area in the State of Linux
Graphics article last summer. There is a large section about kernel
issues.
http://people.freedesktop.org/~jonsmirl/graphics.html

>
> > How do vbetool and X coordinate their usage of "enable"?
>
> vbetool won't run at anything other than a text console, and X won't
> mess with the graphics card if it's not on the current VT. You can mess
> this up if you try hard enough (multihead, for instance) but by and
> large it's a situation that you can avoid.
>
> > What if we throw an in-kernel VGA driver into the mix?  But I guess
> > Jon has asked all these questions before; I just didn't get warm
> > fuzzies that there were safe, maintainable answers.
>
> This probably isn't the right long-term answer, but the right long-term
> answer is going to be a very long time away. It's an improvement over
> what we have now. I certainly don't intend to leave vbetool relying on
> it - of course, the "right" answer is for graphics drivers to know how
> to program cards from scratch so we can get rid of vbetool altogether,
> but I'll probably be more concerned about getting my flying car to meet
> new emission standards than I will be by graphics cards at that stage.
>
> --
> Matthew Garrett | mjg59-chiark.mail.linux-rutgers.kernel@srcf.ucam.org
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Peter Jones]

On Thu, 2006-05-04 at 16:40 -0400, Jon Smirl wrote:
> On 5/4/06, Matthew Garrett <mgarrett@chiark.greenend.org.uk> wrote:
> > Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> >
> > > There's already a "rom" file in sysfs.  Could vbetool and friends
> > > use that?
> >
> > Not if you have multiple graphics cards.
> 
> Not true, the rom attribute maps the ROM into PCI space where ever the
> kernel tells it to and reads it from there. It is the PCI VGA
> emulation feature that forces the ROM to appear at C000:0. You can
> have the ROM mapped and VGA emulation turned off.

It doesn't matter -- you can accomplish the same thing with e.g.
libx86emu and simply mapping the option rom to 0xc0000.  But you want to
do that in userland, not in the kernel.

> This brings up another major point. X changes the PCI VGA emulation
> routing from user space, another thing that it should not be doing. I
> have posted patches before providing a sysfs VGA attribute on class
> VGA devices. By setting the attribute to 1 you can control the active
> VGA emulation device.
> 
> This is yet another way that user space can mess up the kernel. If VGA
> routing is changes under fbdev (my attribute notifies fbdev, the fbdev
> code for processing the notification did get checked in) then the
> console will screw up.

And this change allows userland to avoid doing that.

> The usual screw up is that the console goes
> blank because hardware fonts are not setup correctly on the new
> console.

And that's completely unrelated to this problem.

-- 
  Peter

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Martin Mares]

Hello!

> > This is yet another way that user space can mess up the kernel. If VGA
> > routing is changes under fbdev (my attribute notifies fbdev, the fbdev
> > code for processing the notification did get checked in) then the
> > console will screw up.
> 
> And this change allows userland to avoid doing that.

Could you explain how?  I don't see how adding a "enable" attribute
to sysfs can solve these problems.

I agree with Arjan that the attribute could be useful for some special
tools, but using it in X is likely to be utterly wrong.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
A mathematician is a machine for converting coffee into theorems.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Peter Jones]

On Thu, 2006-05-04 at 23:17 +0200, Martin Mares wrote:
> Hello!
> 
> > > This is yet another way that user space can mess up the kernel. If VGA
> > > routing is changes under fbdev (my attribute notifies fbdev, the fbdev
> > > code for processing the notification did get checked in) then the
> > > console will screw up.
> > 
> > And this change allows userland to avoid doing that.
> 
> Could you explain how?  I don't see how adding a "enable" attribute
> to sysfs can solve these problems.

By allowing tools to read the rom from the pci device itself, instead of
whichever device's rom happens to be at 0xC0000.  libx86emul allows you
to define routines that it uses for memory access, so you can copy the
ROM out to normal memory, and map that memory to the appropriate address
that way.  X and vbetool both already have to do this, but without
copying the firmware image, to do DDC probing on x86_64.

> I agree with Arjan that the attribute could be useful for some special
> tools, but using it in X is likely to be utterly wrong.

I'm actually talking about vbetool here, though X could use these
methods.  Indeed, libx86emul comes from X itself.

-- 
  Peter

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Martin Mares]

Hello!

> By allowing tools to read the rom from the pci device itself, instead of
> whichever device's rom happens to be at 0xC0000.

The ROMs are already accessible through sysfs, aren't they?

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
Anyone who says sunshine brings happiness has never danced in the rain.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/4/06, Peter Jones <pjones@redhat.com> wrote:
> On Thu, 2006-05-04 at 23:17 +0200, Martin Mares wrote:
> > Hello!
> >
> > > > This is yet another way that user space can mess up the kernel. If VGA
> > > > routing is changes under fbdev (my attribute notifies fbdev, the fbdev
> > > > code for processing the notification did get checked in) then the
> > > > console will screw up.
> > >
> > > And this change allows userland to avoid doing that.
> >
> > Could you explain how?  I don't see how adding a "enable" attribute
> > to sysfs can solve these problems.
>
> By allowing tools to read the rom from the pci device itself, instead of
> whichever device's rom happens to be at 0xC0000.  libx86emul allows you
> to define routines that it uses for memory access, so you can copy the
> ROM out to normal memory, and map that memory to the appropriate address
> that way.  X and vbetool both already have to do this, but without
> copying the firmware image, to do DDC probing on x86_64.

# cd /sys/bus/pci/devices/0000:01:00.0
# echo 1 >rom
# hexdump -C rom

As far as I know this works on every platform, not just the PC one.

Don't mess around with the hardware trying to get to the ROM. Use the
API provided by the kernel. Messing with the hardware will get it into
a state that the kernel doesn't know about and can ultimately crash
your system.

>
> > I agree with Arjan that the attribute could be useful for some special
> > tools, but using it in X is likely to be utterly wrong.
>
> I'm actually talking about vbetool here, though X could use these
> methods.  Indeed, libx86emul comes from X itself.
>
> --
>   Peter
>
>


--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Peter Jones]

On Thu, 2006-05-04 at 17:38 -0400, Jon Smirl wrote:

> # cd /sys/bus/pci/devices/0000:01:00.0
> # echo 1 >rom
> # hexdump -C rom
> 
> As far as I know this works on every platform, not just the PC one.

Yep, you're right, this works.  So we don't necessarily need it for the
vbetool case.  X still could use it though, instead of their scary
poke-at-memory way.

> Don't mess around with the hardware trying to get to the ROM. Use the
> API provided by the kernel. Messing with the hardware will get it into
> a state that the kernel doesn't know about and can ultimately crash
> your system.

Exactly who do you see here messing with the hardware directly instead
of using kernel APIs?

-- 
  Peter

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Peter Jones wrote:
> On Thu, 2006-05-04 at 17:38 -0400, Jon Smirl wrote:
> 
>># cd /sys/bus/pci/devices/0000:01:00.0
>># echo 1 >rom
>># hexdump -C rom
>>
>>As far as I know this works on every platform, not just the PC one.
> 
> Yep, you're right, this works.  So we don't necessarily need it for the
> vbetool case.  X still could use it though, instead of their scary
> poke-at-memory way.

Dave Airlie recently changed X to use sysfs for reading ROMs.  I'm also
working on some changes to eliminate nearly all of the PCI bus poking
that X does.  Search for "PCI rework" or "libpciaccess" in the xorg list
archives.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)

iD8DBQFEW6WDX1gOwKyEAw8RAt/aAJ0VDgqBNLbvyKExD0oS3nM5UsAFugCfaJpf
MLSBYFA6e96gyiAr4/j4T14=
=pJQD
-----END PGP SIGNATURE-----

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/5/06, Ian Romanick <idr@us.ibm.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Peter Jones wrote:
> > On Thu, 2006-05-04 at 17:38 -0400, Jon Smirl wrote:
> >
> >># cd /sys/bus/pci/devices/0000:01:00.0
> >># echo 1 >rom
> >># hexdump -C rom
> >>
> >>As far as I know this works on every platform, not just the PC one.
> >
> > Yep, you're right, this works.  So we don't necessarily need it for the
> > vbetool case.  X still could use it though, instead of their scary
> > poke-at-memory way.
>
> Dave Airlie recently changed X to use sysfs for reading ROMs.  I'm also
> working on some changes to eliminate nearly all of the PCI bus poking
> that X does.  Search for "PCI rework" or "libpciaccess" in the xorg list
> archives.

What's the story with PCI VGA routing?

DaveA I were chatting about an alternative to 'enable' attribute. You
would make a VGA driver that isn't bound to any PCI IDs. After the
kernel boots a user space program uses sysfs/vga/new_id to load it
with PCI IDs for all PCI class = VGA hardware. Anything that doesn't
have a driver loaded will then get bound to this VGA driver. In
conjunction with the driver udev would make a device node appear.
Opening the new device node would enable the device and control
ownership. If you want to load a device specific driver later, set
sysfs/vga/unbind=1, load the new device specific driver, set
sysfs/vga/bind=1.

This scheme all works within existing kernel mechanisms and does not
require adding an 'enable' attribute. It also addresses the problem of
who owns the state in the hardware; the state is owned by whoever has
the device node open. If another app wants to mess with the hardware
it won't be able to open the device node.

I would like to see other design alternatives considered on this
issue. The 'enable' attribute has a clear problem in that you can't
tell which user space program is trying to control the device.
Multiple programs accessing the video hardware with poor coordination
is already the source of many problems.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Greg KH]

On Fri, May 05, 2006 at 04:14:00PM -0400, Jon Smirl wrote:
> I would like to see other design alternatives considered on this
> issue. The 'enable' attribute has a clear problem in that you can't
> tell which user space program is trying to control the device.
> Multiple programs accessing the video hardware with poor coordination
> is already the source of many problems.

Who cares who "enabled" the device.  Remember, the majority of PCI
devices in the system are not video ones.  Lots of other types of
devices want this ability to enable PCI devices from userspace.  I've
been talking with some people about how to properly write PCI drivers in
userspace, and this attribute is a needed part of it.

And if X gets enabling the device wrong, again, who cares, it's not a
kernel issue. :)

thanks,

greg k-h

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/5/06, Greg KH <greg@kroah.com> wrote:
> On Fri, May 05, 2006 at 04:14:00PM -0400, Jon Smirl wrote:
> > I would like to see other design alternatives considered on this
> > issue. The 'enable' attribute has a clear problem in that you can't
> > tell which user space program is trying to control the device.
> > Multiple programs accessing the video hardware with poor coordination
> > is already the source of many problems.
>
> Who cares who "enabled" the device.  Remember, the majority of PCI
> devices in the system are not video ones.  Lots of other types of
> devices want this ability to enable PCI devices from userspace.  I've
> been talking with some people about how to properly write PCI drivers in
> userspace, and this attribute is a needed part of it.

User space program enables the device.
Next I load a device driver
next I rmmod the device driver and it disables the device
user space program trys to use the device
No coordination and user space program faults

Don't say this can't happen, it is a current source of conflict
between X and fbdev.

Should we just remove the ability to disable hardware?
How would that interact with hotplug?

> And if X gets enabling the device wrong, again, who cares, it's not a
> kernel issue. :)
>
> thanks,
>
> greg k-h
>


--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

> On 5/5/06, Greg KH <greg@kroah.com> wrote:
> > Who cares who "enabled" the device.  Remember, the majority of PCI
> > devices in the system are not video ones.  Lots of other types of
> > devices want this ability to enable PCI devices from userspace.  I've
> > been talking with some people about how to properly write PCI drivers in
> > userspace, and this attribute is a needed part of it.

The problem is not who 'enabled' the device. The problem is who owns
the state that has been loaded into the device. Without a mechanism
like open there is no way to serialize the programs trying to set
state into the device.

fbdev and X have this fight currently. On every VT swap they each
reload their state into the video hardware. There is no coordination
so both systems have to assume worst case and rebuild everything. This
is not a good environment to program in. Every time one of the systems
starts using some new feature of hardware (like acceleration
functions) new state recovery code needs to be written.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Greg KH]

On Fri, May 05, 2006 at 04:43:15PM -0400, Jon Smirl wrote:
> >On 5/5/06, Greg KH <greg@kroah.com> wrote:
> >> Who cares who "enabled" the device.  Remember, the majority of PCI
> >> devices in the system are not video ones.  Lots of other types of
> >> devices want this ability to enable PCI devices from userspace.  I've
> >> been talking with some people about how to properly write PCI drivers in
> >> userspace, and this attribute is a needed part of it.
> 
> The problem is not who 'enabled' the device. The problem is who owns
> the state that has been loaded into the device. Without a mechanism
> like open there is no way to serialize the programs trying to set
> state into the device.
> 
> fbdev and X have this fight currently. On every VT swap they each
> reload their state into the video hardware. There is no coordination
> so both systems have to assume worst case and rebuild everything. This
> is not a good environment to program in. Every time one of the systems
> starts using some new feature of hardware (like acceleration
> functions) new state recovery code needs to be written.

Yes, I agree that this is a big issue and one that needs to be worked
on.  However, this simple "enable" file has nothing to do with that
issue at all...

thanks,

greg k-h

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Greg KH]

On Fri, May 05, 2006 at 04:35:17PM -0400, Jon Smirl wrote:
> On 5/5/06, Greg KH <greg@kroah.com> wrote:
> >On Fri, May 05, 2006 at 04:14:00PM -0400, Jon Smirl wrote:
> >> I would like to see other design alternatives considered on this
> >> issue. The 'enable' attribute has a clear problem in that you can't
> >> tell which user space program is trying to control the device.
> >> Multiple programs accessing the video hardware with poor coordination
> >> is already the source of many problems.
> >
> >Who cares who "enabled" the device.  Remember, the majority of PCI
> >devices in the system are not video ones.  Lots of other types of
> >devices want this ability to enable PCI devices from userspace.  I've
> >been talking with some people about how to properly write PCI drivers in
> >userspace, and this attribute is a needed part of it.
> 
> User space program enables the device.
> Next I load a device driver
> next I rmmod the device driver and it disables the device
> user space program trys to use the device
> No coordination and user space program faults

Gun.  Foot.  Shoot.

> Don't say this can't happen, it is a current source of conflict
> between X and fbdev.

Not a PCI kernel issue :)

> Should we just remove the ability to disable hardware?

Huh?  Why?

> How would that interact with hotplug?

What does hotplug have to do at all with this?

thanks,

greg "not all the world is video cards" k-h

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/5/06, Greg KH <greg@kroah.com> wrote:
> On Fri, May 05, 2006 at 04:35:17PM -0400, Jon Smirl wrote:
> > On 5/5/06, Greg KH <greg@kroah.com> wrote:
> > >On Fri, May 05, 2006 at 04:14:00PM -0400, Jon Smirl wrote:
> > >> I would like to see other design alternatives considered on this
> > >> issue. The 'enable' attribute has a clear problem in that you can't
> > >> tell which user space program is trying to control the device.
> > >> Multiple programs accessing the video hardware with poor coordination
> > >> is already the source of many problems.
> > >
> > >Who cares who "enabled" the device.  Remember, the majority of PCI
> > >devices in the system are not video ones.  Lots of other types of
> > >devices want this ability to enable PCI devices from userspace.  I've
> > >been talking with some people about how to properly write PCI drivers in
> > >userspace, and this attribute is a needed part of it.
> >
> > User space program enables the device.
> > Next I load a device driver
> > next I rmmod the device driver and it disables the device
> > user space program trys to use the device
> > No coordination and user space program faults
>
> Gun.  Foot.  Shoot.

Why do we want to create problem like this when there is a simple
solution to preventing them. All it takes is a couple of rules:

1) To use a device it must have a device driver. It may be as simple
as a couple of lines of code. This driver will cause a device node to
be created.

2) If a user app want to use the device it opens the device node.

This builds a system where everybody knows what is going on. The
driver knows that user space is using the device. Multiple user space
users are blocked from conflicting because of the open. There is no
way to shoot yourself in the foot.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Greg KH]

On Fri, May 05, 2006 at 05:15:02PM -0400, Jon Smirl wrote:
> On 5/5/06, Greg KH <greg@kroah.com> wrote:
> >On Fri, May 05, 2006 at 04:35:17PM -0400, Jon Smirl wrote:
> >> On 5/5/06, Greg KH <greg@kroah.com> wrote:
> >> >On Fri, May 05, 2006 at 04:14:00PM -0400, Jon Smirl wrote:
> >> >> I would like to see other design alternatives considered on this
> >> >> issue. The 'enable' attribute has a clear problem in that you can't
> >> >> tell which user space program is trying to control the device.
> >> >> Multiple programs accessing the video hardware with poor coordination
> >> >> is already the source of many problems.
> >> >
> >> >Who cares who "enabled" the device.  Remember, the majority of PCI
> >> >devices in the system are not video ones.  Lots of other types of
> >> >devices want this ability to enable PCI devices from userspace.  I've
> >> >been talking with some people about how to properly write PCI drivers in
> >> >userspace, and this attribute is a needed part of it.
> >>
> >> User space program enables the device.
> >> Next I load a device driver
> >> next I rmmod the device driver and it disables the device
> >> user space program trys to use the device
> >> No coordination and user space program faults
> >
> >Gun.  Foot.  Shoot.
> 
> Why do we want to create problem like this when there is a simple
> solution to preventing them. All it takes is a couple of rules:
> 
> 1) To use a device it must have a device driver. It may be as simple
> as a couple of lines of code. This driver will cause a device node to
> be created.
> 
> 2) If a user app want to use the device it opens the device node.
> 
> This builds a system where everybody knows what is going on. The
> driver knows that user space is using the device. Multiple user space
> users are blocked from conflicting because of the open. There is no
> way to shoot yourself in the foot.

That sounds like a nice way to mediate userspace usages of PCI devices,
yes.  Much like a dot lockfile is done for tty devices today, all in
userspace (which hints that this too can be done in userspace with no
kernel involvement...)

But it still has nothing to do with this enable sysfs file :)

thanks,

greg k-h

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/5/06, Greg KH <greg@kroah.com> wrote:
> On Fri, May 05, 2006 at 05:15:02PM -0400, Jon Smirl wrote:
> > On 5/5/06, Greg KH <greg@kroah.com> wrote:
> > >On Fri, May 05, 2006 at 04:35:17PM -0400, Jon Smirl wrote:
> > >> On 5/5/06, Greg KH <greg@kroah.com> wrote:
> > >> >On Fri, May 05, 2006 at 04:14:00PM -0400, Jon Smirl wrote:
> > >> >> I would like to see other design alternatives considered on this
> > >> >> issue. The 'enable' attribute has a clear problem in that you can't
> > >> >> tell which user space program is trying to control the device.
> > >> >> Multiple programs accessing the video hardware with poor coordination
> > >> >> is already the source of many problems.
> > >> >
> > >> >Who cares who "enabled" the device.  Remember, the majority of PCI
> > >> >devices in the system are not video ones.  Lots of other types of
> > >> >devices want this ability to enable PCI devices from userspace.  I've
> > >> >been talking with some people about how to properly write PCI drivers in
> > >> >userspace, and this attribute is a needed part of it.
> > >>
> > >> User space program enables the device.
> > >> Next I load a device driver
> > >> next I rmmod the device driver and it disables the device
> > >> user space program trys to use the device
> > >> No coordination and user space program faults
> > >
> > >Gun.  Foot.  Shoot.
> >
> > Why do we want to create problem like this when there is a simple
> > solution to preventing them. All it takes is a couple of rules:
> >
> > 1) To use a device it must have a device driver. It may be as simple
> > as a couple of lines of code. This driver will cause a device node to
> > be created.
> >
> > 2) If a user app want to use the device it opens the device node.
> >
> > This builds a system where everybody knows what is going on. The
> > driver knows that user space is using the device. Multiple user space
> > users are blocked from conflicting because of the open. There is no
> > way to shoot yourself in the foot.
>
> That sounds like a nice way to mediate userspace usages of PCI devices,
> yes.  Much like a dot lockfile is done for tty devices today, all in
> userspace (which hints that this too can be done in userspace with no
> kernel involvement...)
>
> But it still has nothing to do with this enable sysfs file :)

It has everything to do with the 'enable' file. The 'enable' file lets
you change the state of the hardware without an ownership mechanism. 
Other device users will not be notified of the state change. Since the
other users can't be sure of the state of the hardware when they are
activated, they will have to reload their state into the hardware on
every activation.

So as a result of this every interrupt service routine should now
include pci_enable(). If you don't include this and someone from user
space disables the hardware you're going to GPF. fbdev is already
forced to take defensive measures like this since X will randomly
disable it's hardware while it has an ISR active.

On the other hand, if you say you're only going to enable hardware and
not disable it, then we should simply get rid of the PCI
enable/disable functions and permanently enable everything at boot.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Dave Airlie]

> It has everything to do with the 'enable' file. The 'enable' file lets
> you change the state of the hardware without an ownership mechanism.
> Other device users will not be notified of the state change. Since the
> other users can't be sure of the state of the hardware when they are
> activated, they will have to reload their state into the hardware on
> every activation.

Jon

you seem to miss the fact that this can be done now without the enable
flag, setpci can be used to disable the BARs, again the enable flag
doesn't change that....

Dave.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/5/06, Dave Airlie <airlied@gmail.com> wrote:
> > It has everything to do with the 'enable' file. The 'enable' file lets
> > you change the state of the hardware without an ownership mechanism.
> > Other device users will not be notified of the state change. Since the
> > other users can't be sure of the state of the hardware when they are
> > activated, they will have to reload their state into the hardware on
> > every activation.
>
> Jon
>
> you seem to miss the fact that this can be done now without the enable
> flag, setpci can be used to disable the BARs, again the enable flag
> doesn't change that....

Existence of a problem API doesn't justify creating more of them.

The minimal vga driver combined with new_id scheme is very simple, it
works on older kernels, it does not create a new API and it tracks
ownership of the hardware state. But instead everyone seems to want
the 'enable' file despite it's obvious problems.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Krzysztof Halasa]

"Jon Smirl" <jonsmirl@gmail.com> writes:

> The minimal vga driver combined with new_id scheme is very simple, it
> works on older kernels, it does not create a new API and it tracks
> ownership of the hardware state.

But it works only with VGAs (I, for example, use setpci-alike things
with non-VGA cards but it's dangerous - who knows if the BARs are set
correctly if the device wasn't enabled by the kernel).
-- 
Krzysztof Halasa

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/6/06, Krzysztof Halasa <khc@pm.waw.pl> wrote:
> "Jon Smirl" <jonsmirl@gmail.com> writes:
>
> > The minimal vga driver combined with new_id scheme is very simple, it
> > works on older kernels, it does not create a new API and it tracks
> > ownership of the hardware state.
>
> But it works only with VGAs (I, for example, use setpci-alike things
> with non-VGA cards but it's dangerous - who knows if the BARs are set
> correctly if the device wasn't enabled by the kernel).

Substitute vga with the name of whatever class of device you are
working on and build it a minimal driver for it. The technique is
generic.

> --
> Krzysztof Halasa
>


--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Krzysztof Halasa]

"Jon Smirl" <jonsmirl@gmail.com> writes:

> Substitute vga with the name of whatever class of device you are
> working on and build it a minimal driver for it. The technique is
> generic.

The card in question _has_ a driver. I, for example, just need a way
to write EEPROM data to it (vendor/device ID etc). The card has to be
selected by PCI bus and slot (device) number, not by device class
and/or IDs, because it can contain garbage and/or some generic IDs
with generic device class.

I'm not against the additional driver but it has to be able to work
with any specified card (as setpci does). But if it's that simple
then why not do that in the PCI code instead (holding some device
file open isn't a problem)?
-- 
Krzysztof Halasa

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/6/06, Krzysztof Halasa <khc@pm.waw.pl> wrote:
> "Jon Smirl" <jonsmirl@gmail.com> writes:
>
> > Substitute vga with the name of whatever class of device you are
> > working on and build it a minimal driver for it. The technique is
> > generic.
>
> The card in question _has_ a driver. I, for example, just need a way
> to write EEPROM data to it (vendor/device ID etc). The card has to be
> selected by PCI bus and slot (device) number, not by device class
> and/or IDs, because it can contain garbage and/or some generic IDs
> with generic device class.

Hardware like you describe violates the PCI spec and it should not be
expected that Linux will support it in the general case. It sounds
like this is some kind of prototype hardware.

I would probably handle this by writing an unbound device driver that
exposes a sysfs file for bus:slot. When you write the bus:slot to the
file it would then bind to the appropriate hardware and enable it. The
newly bound driver would support the driver firmware loader interface
as a means of getting your data in.

>
> I'm not against the additional driver but it has to be able to work
> with any specified card (as setpci does). But if it's that simple
> then why not do that in the PCI code instead (holding some device
> file open isn't a problem)?
> --
> Krzysztof Halasa
>


--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Krzysztof Halasa]

"Jon Smirl" <jonsmirl@gmail.com> writes:

>> The card in question _has_ a driver. I, for example, just need a way
>> to write EEPROM data to it (vendor/device ID etc). The card has to be
>> selected by PCI bus and slot (device) number, not by device class
>> and/or IDs, because it can contain garbage and/or some generic IDs
>> with generic device class.
>
> Hardware like you describe violates the PCI spec

What exactly does it violate?

> and it should not be
> expected that Linux will support it in the general case. It sounds
> like this is some kind of prototype hardware.

No. Just one of the final steps of production, or in-system update.

> I would probably handle this by writing an unbound device driver that
> exposes a sysfs file for bus:slot. When you write the bus:slot to the
> file it would then bind to the appropriate hardware and enable it. The
> newly bound driver would support the driver firmware loader interface
> as a means of getting your data in.

What is also needed is that end users perform this task from time to
time. They generally don't want to have to compile the kernel :-)

You know, even now it can be done (entirely in userspace), and only
enabling the device seems a bit dangerous.
-- 
Krzysztof Halasa

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Kyle Moffett]

On May 6, 2006, at 19:16:25, Krzysztof Halasa wrote:
> "Jon Smirl" <jonsmirl@gmail.com> writes:
>>> The card in question _has_ a driver. I, for example, just need a  
>>> way to write EEPROM data to it (vendor/device ID etc). The card  
>>> has to be selected by PCI bus and slot (device) number, not by  
>>> device class and/or IDs, because it can contain garbage and/or  
>>> some generic IDs with generic device class.
>>
>> Hardware like you describe violates the PCI spec
>
> What exactly does it violate?

"device class and/or IDs ... can contain garbage".  That seems to  
violate PCI spec to me.

>> I would probably handle this by writing an unbound device driver  
>> that exposes a sysfs file for bus:slot. When you write the  
>> bus:slot to the file it would then bind to the appropriate  
>> hardware and enable it. The newly bound driver would support the  
>> driver firmware loader interface as a means of getting your data in.
>
> What is also needed is that end users perform this task from time  
> to time. They generally don't want to have to compile the kernel :-)
>
> You know, even now it can be done (entirely in userspace), and only  
> enabling the device seems a bit dangerous.

Jon Smirl gave a great description of exactly how to write such a  
"driver".  I seem to recall that we already have the ability to  
trigger manual PCI binding by bus:slot number; in combination with an  
appropriate "write EEPROM with firmware file" driver that has no  
default list of PCI devices; you could easily manually trigger a bind  
of the device.  On the other hand I would personally never put such a  
device in my system, what if the garbage device IDs happened to match  
a device with poorly-written PCI IDE drivers that panic when they  
bind to a device which does not match their expectations?  In any  
case what you really need for all those cases is a simplistic stub  
driver that provides some sort of in-kernel mutual exclusion.

Cheers,
Kyle Moffett

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Krzysztof Halasa]

Kyle Moffett <mrmacman_g4@mac.com> writes:

> "device class and/or IDs ... can contain garbage".  That seems to
> violate PCI spec to me.

Well, maybe not exactly garbage but just no useful IDs.

> Jon Smirl gave a great description of exactly how to write such a
> "driver".  I seem to recall that we already have the ability to
> trigger manual PCI binding by bus:slot number; in combination with an
> appropriate "write EEPROM with firmware file" driver that has no
> default list of PCI devices; you could easily manually trigger a bind
> of the device.

Writing EEPROM is not a problem and can be done safely from user space
(mmap /dev/mem). Doing it in the kernel seems like an overkill,
especially if you do the operation once in few years it's much easier
to just download a (statically linked?) binary than messing with
the kernel.

It doesn't even interfere with the "main" driver and can be done while
the device is operating (given that previous EEPROM made sense,
otherwise the driver would not load).

> On the other hand I would personally never put such a
> device in my system, what if the garbage device IDs happened to match
> a device with poorly-written PCI IDE drivers that panic when they
> bind to a device which does not match their expectations?

That isn't possible in this case.

The EEPROM has to be written somehow anyway.

>  In any
> case what you really need for all those cases is a simplistic stub
> driver that provides some sort of in-kernel mutual exclusion.

Right. I.e., "enable" interface with, possibly, locking mechanism,
instead of some per-class "driver".
-- 
Krzysztof Halasa

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Kyle Moffett]

On May 7, 2006, at 08:05:59, Krzysztof Halasa wrote:
> Kyle Moffett <mrmacman_g4@mac.com> writes:
>> Jon Smirl gave a great description of exactly how to write such a  
>> "driver".  I seem to recall that we already have the ability to  
>> trigger manual PCI binding by bus:slot number; in combination with  
>> an appropriate "write EEPROM with firmware file" driver that has  
>> no default list of PCI devices; you could easily manually trigger  
>> a bind of the device.
>
> Writing EEPROM is not a problem and can be done safely from user  
> space (mmap /dev/mem).

This is *exactly* what we don't want to do!  The whole point of this  
thread is to prevent the need to use /dev/mem and /dev/kmem for  
anything except debugging.

> Doing it in the kernel seems like an overkill, especially if you do  
> the operation once in few years it's much easier to just download a  
> (statically linked?) binary than messing with the kernel.

Ewww, I certainly wouldn't trust a binary statically-linked binary  
program that mmaps /dev/mem or /dev/kmem, and I certainly bet that  
90% of the people on this list would feel likewise.  We'd much prefer  
a program which does this:

   #! /bin/sh
   cp firmware.bin /lib/firmware/some_firmware_file.bin
   echo -n eeprom_load_driver >/sys/device/$PCI_ID/bind
   echo -n 1 >/sys/device/$PCI_ID/unbind

Simple, obviously correct, and uses a nice reuseable driver too!

> It doesn't even interfere with the "main" driver and can be done  
> while the device is operating (given that previous EEPROM made  
> sense, otherwise the driver would not load).

No!  That would be even worse!  You're then having userspace poke at  
the driver while a kernel driver is loaded, which is *exactly* what X  
is getting into trouble for doing.  If you want to add firmware  
update capability, add it to the preexisting primary driver.

>> In any case what you really need for all those cases is a  
>> simplistic stub driver that provides some sort of in-kernel mutual  
>> exclusion.
>
> Right. I.e., "enable" interface with, possibly, locking mechanism,  
> instead of some per-class "driver".

No, not an "enable" interface.  In this case the kernel should do  
basically all of the poking at PCI resources for you.  If you  
_really_ want to do that kind of update in userspace, write a stub  
driver which just enables the device on bind, disables it on unbind,  
and mmap and write to the sysfs "rom" file.

Cheers,
Kyle Moffett

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Krzysztof Halasa]

Kyle Moffett <mrmacman_g4@mac.com> writes:

> This is *exactly* what we don't want to do!  The whole point of this
> thread is to prevent the need to use /dev/mem and /dev/kmem for
> anything except debugging.

Look, it's me who's using that and I tell you I want just that :-)

> Ewww, I certainly wouldn't trust a binary statically-linked binary
> program that mmaps /dev/mem or /dev/kmem

And would you trust a binary which doesn't have "/dev/mem" string
in it?

Anyway you can compile it yourself if you want. It's not about trust,
it's about simplicity and robustness.

>    #! /bin/sh
>    cp firmware.bin /lib/firmware/some_firmware_file.bin
>    echo -n eeprom_load_driver >/sys/device/$PCI_ID/bind
>    echo -n 1 >/sys/device/$PCI_ID/unbind
>
> Simple, obviously correct, and uses a nice reuseable driver too!

Sure. If the driver is loaded/available. What if, say, the
distribution you use doesn't have it?

> No!  That would be even worse!  You're then having userspace poke at
> the driver while a kernel driver is loaded, which is *exactly* what X
> is getting into trouble for doing.

So what? The driver and EEPROM updater don't conflict.

>  If you want to add firmware
> update capability, add it to the preexisting primary driver.

It will not load with blank or invalid EEPROM :-)

> No, not an "enable" interface.  In this case the kernel should do
> basically all of the poking at PCI resources for you.

Because?

>  If you
> _really_ want to do that kind of update in userspace, write a stub
> driver which just enables the device on bind, disables it on unbind,
> and mmap and write to the sysfs "rom" file.

It has nothing to do with any "ROM".
-- 
Krzysztof Halasa

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Pavel Machek]

Hi!

> >It has everything to do with the 'enable' file. The 
> >'enable' file lets
> >you change the state of the hardware without an 
> >ownership mechanism.
> >Other device users will not be notified of the state 
> >change. Since the
> >other users can't be sure of the state of the hardware 
> >when they are
> >activated, they will have to reload their state into 
> >the hardware on
> >every activation.
> 
> you seem to miss the fact that this can be done now 
> without the enable
> flag, setpci can be used to disable the BARs, again the 
> enable flag
> doesn't change that....

...well, when you launch setpci, you are firmly in 'unsupported' land.
While 'enable' sounds like something where users expect it to be
supported.



-- 
Thanks for all the (sleeping) penguins.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Kyle Moffett]

On May 7, 2006, at 09:12:01, Pavel Machek wrote:
>>> It has everything to do with the 'enable' file. The 'enable' file  
>>> lets you change the state of the hardware without an ownership  
>>> mechanism. Other device users will not be notified of the state  
>>> change. Since the other users can't be sure of the state of the  
>>> hardware when they are activated, they will have to reload their  
>>> state into the hardware on every activation.
>>
>> you seem to miss the fact that this can be done now without the  
>> enable flag, setpci can be used to disable the BARs, again the  
>> enable flag doesn't change that....
>
> ...well, when you launch setpci, you are firmly in 'unsupported'  
> land.  While 'enable' sounds like something where users expect it  
> to be supported.

*Especially* since there are a number of users (including myself) who  
have tendencies to go wandering around sysfs tinkering with the  
available values.  Not having seen this thread, I would have had no  
problem doing "echo -n 0 >enable" on some device thinking that it was  
a fairly standard way to turn off the power to my soundcard when I'm  
not using it, and likely result in a kernel panic because I suddenly  
disabled the BARs on the device out from under the driver.

Cheers,
Kyle Moffett

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Arjan van de Ven]

> *Especially* since there are a number of users (including myself) who  
> have tendencies to go wandering around sysfs tinkering with the  
> available values.  Not having seen this thread, I would have had no  
> problem doing "echo -n 0 >enable" on some device thinking that it was  
> a fairly standard way to turn off the power to my soundcard 

it is

> when I'm  
> not using it, and likely result in a kernel panic because I suddenly  
> disabled the BARs on the device out from under the driver.

well it is in the pci dir, not in the sound dir. You know you're not
using the driver layer at that point, but are directly poking the
hardware. Just like the resources (eg the contents of the bars) are in
that same directory... you're not echo;ing random crap into those as
well, are you?

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Dave Airlie]

>
> So as a result of this every interrupt service routine should now
> include pci_enable(). If you don't include this and someone from user
> space disables the hardware you're going to GPF. fbdev is already
> forced to take defensive measures like this since X will randomly
> disable it's hardware while it has an ISR active.

Jon please stop spouting crap at least, every ISR doesn't need pci_enable, 
as it doesn't need it now, you are NOT listening, adding the enable bit 
DOES NOT change things, if someone runs setpci from userspace now they can 
do this, ROOT CAN CRASH YOUR MACHINE, FILM AT 11.

If you enable the rom at the moment you can crash things, some devices 
don't have enough address decoders to decode a bar and the ROM, so me 
enabling ROM decoding as your original patch did, can cause system 
lockups,

why is this different, why didn't you write the ROM code patch properly 
then and we woulnd't have to to hear about it now...

Dave.

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
Linux kernel - DRI, VAX / pam_smb / ILUG

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/8/06, Dave Airlie <airlied@linux.ie> wrote:
> >
> > So as a result of this every interrupt service routine should now
> > include pci_enable(). If you don't include this and someone from user
> > space disables the hardware you're going to GPF. fbdev is already
> > forced to take defensive measures like this since X will randomly
> > disable it's hardware while it has an ISR active.
>
> Jon please stop spouting crap at least, every ISR doesn't need pci_enable,
> as it doesn't need it now, you are NOT listening, adding the enable bit
> DOES NOT change things, if someone runs setpci from userspace now they can
> do this, ROOT CAN CRASH YOUR MACHINE, FILM AT 11.

Do what you want, this has gone on too long. There are simple schemes
that can fix these holes but nobody seems to want to use them.  Just
because something was broken in the past doesn't justify a new API to
continue supporting the breakage in the future. I am amazed at the
amount energy being expended to support this flawed API when
alternatives are available.

> If you enable the rom at the moment you can crash things, some devices
> don't have enough address decoders to decode a bar and the ROM, so me
> enabling ROM decoding as your original patch did, can cause system
> lockups,
>
> why is this different, why didn't you write the ROM code patch properly
> then and we woulnd't have to to hear about it now...

This is a known part of the PCI spec, it is not a bug. There is a ROM
API that the driver for the offending hardware should call to disable
(or copy) the ROM attribute.  If the driver is missing the ROM disable
call that is a bug. I don't know what hardware has the address decoder
problem so I can't fix the drivers. This is documented in the header
file and has been posted to LKML. This hardware is uncommon, but If
you do identify hardware that is missing the address decoder please
inform me and the driver maintainer and we will add a call to disable
the ROM attribute.

As far as I know only a single person has hit this and they didn't
tell me the PCI ID of the problem hardware. I suspect the problem
hardware is an older Adaptec RAID controller.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Adam Belay]

On Fri, May 05, 2006 at 01:26:03PM -0700, Greg KH wrote:
> On Fri, May 05, 2006 at 04:14:00PM -0400, Jon Smirl wrote:
> > I would like to see other design alternatives considered on this
> > issue. The 'enable' attribute has a clear problem in that you can't
> > tell which user space program is trying to control the device.
> > Multiple programs accessing the video hardware with poor coordination
> > is already the source of many problems.
> 
> Who cares who "enabled" the device.  Remember, the majority of PCI
> devices in the system are not video ones.  Lots of other types of
> devices want this ability to enable PCI devices from userspace.  I've
> been talking with some people about how to properly write PCI drivers in
> userspace, and this attribute is a needed part of it.
> 
> And if X gets enabling the device wrong, again, who cares, it's not a
> kernel issue. :)
> 
> thanks,
> 
> greg k-h
> -

The main issue I see with the "enable" sysfs attribute is that it doesn't
mediate who has ownership of the device's state and context, as Jon has
mentioned.  As a result, userspace might, not knowing any better, disable
the device while its kernel driver is trying to access it, or a number of
other fatal senarios.

However, I understand that video is not the only issue at hand, and there is
some interest in userspace PCI drivers.  At the very least, we should prevent
any writing to the "enable" attribute if a kernelspace driver is bound to
the device.  This patch completely ignores such issues.

We might want to take this a step further and create a generic PCI userspace
kernel driver that exports knobs such as "enable" in sysfs and also handles
some baseline suspend/resume behavior and whatever else is needed for
userspace drivers.  This will ensure that the kernel is aware that the
hardware is owned by a userspace driver, and it will make it clear to a user
app that it is allowed to control the device.

Thanks,
Adam

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Benjamin Herrenschmidt]

> By allowing tools to read the rom from the pci device itself, instead of
> whichever device's rom happens to be at 0xC0000.  libx86emul allows you
> to define routines that it uses for memory access, so you can copy the
> ROM out to normal memory, and map that memory to the appropriate address
> that way.  X and vbetool both already have to do this, but without
> copying the firmware image, to do DDC probing on x86_64.
> 
> > I agree with Arjan that the attribute could be useful for some special
> > tools, but using it in X is likely to be utterly wrong.
> 
> I'm actually talking about vbetool here, though X could use these
> methods.  Indeed, libx86emul comes from X itself.

There are reasons why you may have to read the image at c0000... There's
a bunch of laptops where it's in fact the only way to get to the video
BIOS as it doesn't have a ROM attached to the video chip (it's burried
in the main BIOS which thankfully copied it to c0000 when running it).
In some cases, the BISO ROM self-modifies it's c0000 and it's that
modified copy that the X (or fbdev) driver should get. Remeber that
drivers needs access to the ROM for more than just POSTing the chip...

In addition, we need to centralize VGA routing mecanism in the kernel
for other reasons. For example, because it might imply disabling MMIO or
IO access to other cards (currently, we have to disable _all_ other
cards on the same bus segment that claim to be VGA since we have no way
to know which ones might be able to simply be configured not to decode
legacy VGA accesses, though an appropriate arbiter ABI would allow to
handle that too). Thus am fbdev or X server currently running on another
card would explode in flames while something like vbetool is re-routing
VGA access to a card for POST'ing or other things vbetool can do.

It's a shitty problem, it's complicated, but there is no easy way out
thanks to PC manufacturers and Intel keeping absolutely disgusting VGA
"legacy" crap part of the PCI standard for so long.

Note that I posted various prototypes of what a VGA abiter could look
like a while ago. I have no time to resume work on that and my
prototypes weren't perfect, but that's something one interested party
could probably use as a starting point to a more complete solution.
 
Ben.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/13/06, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> There are reasons why you may have to read the image at c0000... There's
> a bunch of laptops where it's in fact the only way to get to the video
> BIOS as it doesn't have a ROM attached to the video chip (it's burried
> in the main BIOS which thankfully copied it to c0000 when running it).
> In some cases, the BISO ROM self-modifies it's c0000 and it's that
> modified copy that the X (or fbdev) driver should get. Remeber that
> drivers needs access to the ROM for more than just POSTing the chip...

Whenever klibc gets merged it would probably be good to add a
libemu86. Did you get one put together that you're happy with?

Between the ROM attribute, klibc and libemu86 there will then be
enough support to write a tiny POST program that POSTs secondary and
non-x86 primary cards at boot. It will still need a little support in
sysfs for PCI bus VGA routing but we're almost there.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Benjamin Herrenschmidt]

On Sat, 2006-05-13 at 20:56 -0400, Jon Smirl wrote:
> On 5/13/06, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > There are reasons why you may have to read the image at c0000... There's
> > a bunch of laptops where it's in fact the only way to get to the video
> > BIOS as it doesn't have a ROM attached to the video chip (it's burried
> > in the main BIOS which thankfully copied it to c0000 when running it).
> > In some cases, the BISO ROM self-modifies it's c0000 and it's that
> > modified copy that the X (or fbdev) driver should get. Remeber that
> > drivers needs access to the ROM for more than just POSTing the chip...
> 
> Whenever klibc gets merged it would probably be good to add a
> libemu86. Did you get one put together that you're happy with?

No, not so far yet. Haven't had much time though. Did some expriments
based on what you sent me back then, got it to work with loads of hacks,
but I never properly cleaned it up.

> Between the ROM attribute, klibc and libemu86 there will then be
> enough support to write a tiny POST program that POSTs secondary and
> non-x86 primary cards at boot. It will still need a little support in
> sysfs for PCI bus VGA routing but we're almost there.

And we need to "capture" the resulting BIOS image after POST and have a
away to give that to the drivers as it will contain useful tables that
the driver will need as well...

Ben.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/14/06, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Sat, 2006-05-13 at 20:56 -0400, Jon Smirl wrote:
> > On 5/13/06, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > > There are reasons why you may have to read the image at c0000... There's
> > > a bunch of laptops where it's in fact the only way to get to the video
> > > BIOS as it doesn't have a ROM attached to the video chip (it's burried
> > > in the main BIOS which thankfully copied it to c0000 when running it).
> > > In some cases, the BISO ROM self-modifies it's c0000 and it's that
> > > modified copy that the X (or fbdev) driver should get. Remeber that
> > > drivers needs access to the ROM for more than just POSTing the chip...
> >
> > Whenever klibc gets merged it would probably be good to add a
> > libemu86. Did you get one put together that you're happy with?
>
> No, not so far yet. Haven't had much time though. Did some expriments
> based on what you sent me back then, got it to work with loads of hacks,
> but I never properly cleaned it up.
>
> > Between the ROM attribute, klibc and libemu86 there will then be
> > enough support to write a tiny POST program that POSTs secondary and
> > non-x86 primary cards at boot. It will still need a little support in
> > sysfs for PCI bus VGA routing but we're almost there.
>
> And we need to "capture" the resulting BIOS image after POST and have a
> away to give that to the drivers as it will contain useful tables that
> the driver will need as well...

The ROM API already has support it in for storing a RAM based copy. I
believe it works but no one is using it so it is not well tested.
Something will need to be hooked up to get the copy from user space
back into the kernel. At one point I was working on generalizing
request_firmware to make it handle more that firmware downloads.

>
> Ben.
>
>
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Patrick McFarland]

On Saturday 13 May 2006 20:29, Benjamin Herrenschmidt wrote:
> a long post.

So, why do we insist on keeping legacy hardware around? I mean, serial and 
parallel ports are basically dead, as are ps/2 ports (USB killed them all). 
VGA basically died out when DVI came around. Traditional IA32 is now dying 
out thanks to x86-64. The basic internals have been surplanted by APIC. We 
have a power management API, ACPI, which was unheard of on x86 15 years ago.

IDE is dying out, being replaced with SATA. Old SCSI methods are being 
replaced with Serial SCSI. I mean, I could basically go on for hours saying 
that pretty much all legacy hardware and methods are being replaced or have 
already been replaced.

Even the traditional IBM PC BIOS is being replaced, by various new firmware 
logic methods (such as openfirmware on x86, or intel's one who's name I 
forget.)

So, why do we continue keeping VGA around? Why isn't there a better method? 
Benjamin's post basically describes a technology that has been hacked to hell 
and back and has been around entirely too long, so why hasn't someone created 
something new?

-- 
Patrick McFarland || www.AdAstraPerAspera.com
"Computer games don't affect kids; I mean if Pac-Man affected us as kids,
we'd all be running around in darkened rooms, munching magic pills and
listening to repetitive electronic music." -- Kristian Wilson, Nintendo,
Inc, 1989

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/13/06, Patrick McFarland <diablod3@gmail.com> wrote:
> On Saturday 13 May 2006 20:29, Benjamin Herrenschmidt wrote:
> > a long post.
>
> So, why do we insist on keeping legacy hardware around? I mean, serial and
> parallel ports are basically dead, as are ps/2 ports (USB killed them all).
> VGA basically died out when DVI came around. Traditional IA32 is now dying
> out thanks to x86-64. The basic internals have been surplanted by APIC. We
> have a power management API, ACPI, which was unheard of on x86 15 years ago.

Because it is the only video interface we have documentation for.
Almost all of the video hardware can run in non-VGA mode but we don't
have the docs to do this on NVidia/ATI.

It is also a universal interface supported by all video cards. You can
get things like GRUB and the BIOS up on it with minimal code that will
work on all video cards.

To get rid of it the video hardware manufacturers would have to come
together and define a new, open standard. That doesn't look likely to
happen so we are stuck with it. I do agree that it is extremely messy
to work with.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/4/06, Peter Jones <pjones@redhat.com> wrote:
> On Thu, 2006-05-04 at 16:40 -0400, Jon Smirl wrote:
> > On 5/4/06, Matthew Garrett <mgarrett@chiark.greenend.org.uk> wrote:
> > > Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > >
> > > > There's already a "rom" file in sysfs.  Could vbetool and friends
> > > > use that?
> > >
> > > Not if you have multiple graphics cards.
> >
> > Not true, the rom attribute maps the ROM into PCI space where ever the
> > kernel tells it to and reads it from there. It is the PCI VGA
> > emulation feature that forces the ROM to appear at C000:0. You can
> > have the ROM mapped and VGA emulation turned off.
>
> It doesn't matter -- you can accomplish the same thing with e.g.
> libx86emu and simply mapping the option rom to 0xc0000.  But you want to
> do that in userland, not in the kernel.

It is much more complicated than than you describe. Go look at the ROM
code already checked in. Laptop video ROMs are not simple PCI devices
that can be mapped around. They are stored in compressed form inside
the system ROM and expanded at boot. If you lose the shadow copy in
RAM there is no API for getting it back. These compressed ROMs are the
source of a lot of laptop user's problems with suspend/resume on
Linux.

VGA support for multiple cards is a very complicated problem. It
really needs to be comprehenisvely designed (but not necessarily
implemented all at once). What we have now is twenty years worth of
hacks that work 95% of the time. But there are many, many holes in the
current scheme.

>
> > This brings up another major point. X changes the PCI VGA emulation
> > routing from user space, another thing that it should not be doing. I
> > have posted patches before providing a sysfs VGA attribute on class
> > VGA devices. By setting the attribute to 1 you can control the active
> > VGA emulation device.
> >
> > This is yet another way that user space can mess up the kernel. If VGA
> > routing is changes under fbdev (my attribute notifies fbdev, the fbdev
> > code for processing the notification did get checked in) then the
> > console will screw up.
>
> And this change allows userland to avoid doing that.
>
> > The usual screw up is that the console goes
> > blank because hardware fonts are not setup correctly on the new
> > console.
>
> And that's completely unrelated to this problem.
>
> --
>   Peter
>
>


--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Peter Jones]

On Thu, 2006-05-04 at 17:18 -0400, Jon Smirl wrote: 
> On 5/4/06, Peter Jones <pjones@redhat.com> wrote:

> > It doesn't matter -- you can accomplish the same thing with e.g.
> > libx86emu and simply mapping the option rom to 0xc0000.  But you want to
> > do that in userland, not in the kernel.
> 
> It is much more complicated than than you describe.

I didn't really feel like explaining the parts we both already know.
I'll try to remember to do so in the future.

> Go look at the ROM code already checked in. Laptop video ROMs are not
> simple PCI devices that can be mapped around. They are stored in
> compressed form inside the system ROM and expanded at boot.

Yes, and this format is documented, too.  But right now there's no way
to get access to it with tools to actually do anything.

> If you lose the shadow copy in RAM there is no API for getting it back.

Except to enable the BAR and read it from the assigned address...

> These compressed ROMs are the source of a lot of laptop user's
> problems with suspend/resume on Linux.

Absolutely.  That's why I want a method to access them, which this
"enable" file provides.

> VGA support for multiple cards is a very complicated problem.

Please quit jumping up and down in the bicycle path telling everybody
how hard it is to ride a bike.

-- 
  Peter

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/4/06, Peter Jones <pjones@redhat.com> wrote:
> On Thu, 2006-05-04 at 17:18 -0400, Jon Smirl wrote:
> > On 5/4/06, Peter Jones <pjones@redhat.com> wrote:
>
> > > It doesn't matter -- you can accomplish the same thing with e.g.
> > > libx86emu and simply mapping the option rom to 0xc0000.  But you want to
> > > do that in userland, not in the kernel.
> >
> > It is much more complicated than than you describe.
>
> I didn't really feel like explaining the parts we both already know.
> I'll try to remember to do so in the future.
>
> > Go look at the ROM code already checked in. Laptop video ROMs are not
> > simple PCI devices that can be mapped around. They are stored in
> > compressed form inside the system ROM and expanded at boot.
>
> Yes, and this format is documented, too.  But right now there's no way
> to get access to it with tools to actually do anything.
>
> > If you lose the shadow copy in RAM there is no API for getting it back.
>
> Except to enable the BAR and read it from the assigned address...

Let me be clear here. A lot of laptop video hardware does not have a
video ROM. Therefore you can not enable the BAR and read it from an
assigned address since there is no ROM to be read.  Instead the video
ROM images are compressed and stored inside the system BIOS ROM. The
location of the image is not a public thing.  At boot time the
compressed video ROM is expanded out of the system ROM into shadow RAM
at C000:0. There is no real ROM there, it is only a copy in RAM. If
you lose that copy the only way to get it back is to reset the
machine. The ROM code already in the kernel knows about these shadow
copies and won't mess them up.


> > These compressed ROMs are the source of a lot of laptop user's
> > problems with suspend/resume on Linux.
>
> Absolutely.  That's why I want a method to access them, which this
> "enable" file provides.
>
> > VGA support for multiple cards is a very complicated problem.
>
> Please quit jumping up and down in the bicycle path telling everybody
> how hard it is to ride a bike.
>
> --
>   Peter
>
>


--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Peter Jones]

On Thu, 2006-05-04 at 17:48 -0400, Jon Smirl wrote:

> Let me be clear here. A lot of laptop video hardware does not have a
> video ROM.

Why do you assume we're only talking about laptops?

-- 
  Peter

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/4/06, Peter Jones <pjones@redhat.com> wrote:
> On Thu, 2006-05-04 at 17:48 -0400, Jon Smirl wrote:
>
> > Let me be clear here. A lot of laptop video hardware does not have a
> > video ROM.
>
> Why do you assume we're only talking about laptops?

Because when you make statements like...
> Except to enable the BAR and read it from the assigned address...

That statement doesn't hold true for laptops. It indicates that you
may not be aware of all of the issues around VGA bus routing and video
ROMs. For example there are a lot of special issues with VGA support
in large large machines like the SGI Altix that have multiple
independent PCI domains. Owners of all of these other architectures
(PPC especially) having been beating me up for two years making me
aware of all of their weird cases.


>
> --
>   Peter
>
>


--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Arjan van de Ven]

Bjorn Helgaas wrote:
> On Thursday 04 May 2006 13:11, Arjan van de Ven wrote:
>> Bjorn Helgaas wrote:
>>> On Saturday 29 April 2006 03:04, Dave Airlie wrote:
>>>>> This patch adds an "enable" sysfs attribute to each PCI device. When read it
>>>>> shows the "enabled-ness" of the device, but you can write a "0" into it to
>>>>> disable a device, and a "1" to enable it.
>>>>>
>>>>> This later is needed for X and other cases where userspace wants to enable
>>>>> the BARs on a device (typical example: to run the video bios on a secundary
>>>>> head). Right now X does all this "by hand" via bitbanging, that's just evil.
>>>>> This allows X to no longer do that but to just let the kernel do this.
>>> I'm all in favor of cleaning up X.  But making the X code prettier without
>>> changing the underlying issues of claiming and sharing resources doesn't
>>> help much.  In fact, I suspect the ultimate plan for X does not involve
>>> an "enable" attribute in sysfs, so this may just introduce ABI cruft that
>>> will be difficult to remove later.
>> it goes well beyond X. Things like vbetool need this too to get to the content
>> of the rom for example. There are several other such cases...
> 
> There's already a "rom" file in sysfs.  Could vbetool and friends
> use that?

not unless the device is enabled.


> How do vbetool and X coordinate their usage of "enable"?

Just never disable :)

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Eric W. Biederman]

Arjan van de Ven <arjan@linux.intel.com> writes:

>> > +	if (!pdev)
>> > +		return 1;
>> 
>> Can this happen?
>
> eh I suppose not; the other code doesn't check it either; fixed
>
>> 
>> > +	/* this can crash the machine when done on the "wrong" device */
>> > +	if (!capable(CAP_SYS_ADMIN))
>> > +		return 1;
>> 
>> Don't the file's permissions suffice?
>
> that's a more philosophical question; you can ask that question about
> the entire capability system ;) Other code in the same file uses this
> same capability for a same level of access though.


A minor nit.  This level of access should really be CAP_SYS_RAWIO.

Eric

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 4/29/06, Arjan van de Ven <arjan@linux.intel.com> wrote:
> This patch adds an "enable" sysfs attribute to each PCI device. When read it
> shows the "enabled-ness" of the device, but you can write a "0" into it to
> disable a device, and a "1" to enable it.

What is the rationale for this? Doing this encourages people to write
device drivers in user space that probably should be a kernel driver.
What are you going to do if two competing apps want to set it to two
different states?

An alternate way to fix this problem is to write a device driver that
attaches to hardware with PCI class VGA. The driver could then provide
a device for each card found. By opening the device you can control
who owns and enables it.

We already have a lot of problems with multiple drivers trying to
control a single piece of hardware. Adding an "enable" attribute makes
it even easier to build conflicting drivers.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Arjan van de Ven]

Jon Smirl wrote:
> On 4/29/06, Arjan van de Ven <arjan@linux.intel.com> wrote:
>> This patch adds an "enable" sysfs attribute to each PCI device. When 
>> read it
>> shows the "enabled-ness" of the device, but you can write a "0" into 
>> it to
>> disable a device, and a "1" to enable it.
> 
> What is the rationale for this?

you snipped that out of the email ;)

> Doing this encourages people to write
> device drivers in user space that probably should be a kernel driver.

not really, there's not a lot you can do. What you CAN do is read roms and stuff like that;
the vbetool and X need that for example

> What are you going to do if two competing apps want to set it to two
> different states?

then you're root and you just shot yourself in the proverbial foot.


> An alternate way to fix this problem is to write a device driver that
> attaches to hardware with PCI class VGA.

and then that sucks too because in linux only 1 driver can bind to a device,
AND you're limited to only vga devices.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/2/06, Arjan van de Ven <arjan@linux.intel.com> wrote:
> > An alternate way to fix this problem is to write a device driver that
> > attaches to hardware with PCI class VGA.
>
> and then that sucks too because in linux only 1 driver can bind to a device,
> AND you're limited to only vga devices.
>

There are many other ways to solve this problem. There are two rules
that need to be followed.
1) One driver per device
2) Stop mucking with hardware without having a driver loaded.

There are many other solutions to this problem that follow the Linux
driver model. For example build skeleton fbdev drivers for all of the
VGA device PCI IDs (they are enumerated in the X code). These drivers
can just be empty fbdev drivers, the fbdev core will supply the
open/enable logic. DRM already knows how to coordinate with fbdev so
that there aren't multiple drivers binding to the hardware.

You could do the generic VGA class binding after all of the other
drivers are loaded. And only bind to the free devices.

We could modify the driver system to allow a PCI class binding and a
device specific binding. The device specific binding would override
the class binding.

All of these have been proposed before. In my opinion an 'enable'
attribute is the worst solution in the bunch.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Arjan van de Ven]

Jon Smirl wrote:
> All of these have been proposed before.

I think you forgot to attach the patch

> In my opinion an 'enable'
> attribute is the worst solution in the bunch.

you again limit yourself to VGA. I don't.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/2/06, Arjan van de Ven <arjan@linux.intel.com> wrote:
> Jon Smirl wrote:
> > All of these have been proposed before.
>
> I think you forgot to attach the patch
>
> > In my opinion an 'enable'
> > attribute is the worst solution in the bunch.
>
> you again limit yourself to VGA. I don't.

Haven't we learned that mucking with hardware from user space without
having a device driver loaded is a very bad idea.  What is the
motivation for providing an API whose only purpose is to enable this
bad behavior?


--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Arjan van de Ven]

On Tue, 2006-05-02 at 13:13 -0400, Jon Smirl wrote:
> On 5/2/06, Arjan van de Ven <arjan@linux.intel.com> wrote:
> > Jon Smirl wrote:
> > > All of these have been proposed before.
> >
> > I think you forgot to attach the patch
> >
> > > In my opinion an 'enable'
> > > attribute is the worst solution in the bunch.
> >
> > you again limit yourself to VGA. I don't.
> 
> Haven't we learned that mucking with hardware from user space without
> having a device driver loaded is a very bad idea.

Not quite. It needs a reasonable sane userspace sure, but it's not
unreasonable for several cases.

>   What is the
> motivation for providing an API whose only purpose is to enable this
> bad behavior?

you're very selective in what you read and only think about X.
There's many other reasons to enable/disable devices post boot. 
Having a driver just to call pci_enable_device() is silly; and
there are various scenarios you may want. Userland suspend/resume is
only one of those, posting video cards is one, chip health monitoring is
one, etc etc etc. Don't let your lack of imagination ruin things.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/2/06, Arjan van de Ven <arjan@linux.intel.com> wrote:
> you're very selective in what you read and only think about X.
> There's many other reasons to enable/disable devices post boot.
> Having a driver just to call pci_enable_device() is silly; and
> there are various scenarios you may want. Userland suspend/resume is
> only one of those, posting video cards is one, chip health monitoring is
> one, etc etc etc. Don't let your lack of imagination ruin things.

Allowing user space control of a device without a mechanism to assign
ownership of the device is a very bad idea. There is no way for one
user space program to tell if another is messing with the device.
There has to be a mechanism like opening the device to indicate which
process owns the device and is allowed to set their state into it (for
states that can conflict, enabling the device is definitely a state
that can conflict).

This attribute will also cause problem with existing drivers. Almost
every driver in the kernel will GPF if you disable the hardware on it
while the driver is loaded and active.  This problem has been the
result of numerous OOPses when X decides to disable hardware that
fbdev is using. I also can not see how user space suspend/resume can
disable PCI hardware without coordinating with an active device
driver.

The rule needs to be that if you want to use a device it has to have a
driver. Anything else results in chaos. It doesn't matter if these
drivers have a tiny API, their purpose is to control ownership of the
hardware.

You may call this silly but it is a real pain to spend hours debugging
code only to discover that it failed because some other app unknown to
you altered the state of the hardware while you were using it.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Peter Jones]

On Tue, 2006-05-02 at 15:00 -0400, Jon Smirl wrote:
> I also can not see how user space suspend/resume can
> disable PCI hardware without coordinating with an active device
> driver.

And that's not a reasonable thing for userspace to do, if an active
device driver is using that.  But sometimes it might want to turn a
device on, perform some action, and then disable it again.

[...]
> You may call this silly but it is a real pain to spend hours debugging
> code only to discover that it failed because some other app unknown to
> you altered the state of the hardware while you were using it.

"Doctor, it hurts when I stab myself in the eye..."

-- 
  Peter

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Dave Airlie]

>
> Allowing user space control of a device without a mechanism to assign
> ownership of the device is a very bad idea. There is no way for one
> user space program to tell if another is messing with the device.
> There has to be a mechanism like opening the device to indicate which
> process owns the device and is allowed to set their state into it (for
> states that can conflict, enabling the device is definitely a state
> that can conflict).

Jon stop being so dramatic, this is just like letting userspace map
the BARs, without ownership through sysfs, which is a good thing, you
can still map /dev/mem, look we have lots of ways to shoot ourselves
in the foot, if we *want* to.

> The rule needs to be that if you want to use a device it has to have a
> driver. Anything else results in chaos. It doesn't matter if these
> drivers have a tiny API, their purpose is to control ownership of the
> hardware.

Again we can already bypass device drivers.... so don't worry so much....

This attribute allows us to enable devices that X otherwise would hand
enable, that's all, it doesn't allow a user with vi to do it ....

> You may call this silly but it is a real pain to spend hours debugging
> code only to discover that it failed because some other app unknown to
> you altered the state of the hardware while you were using it.
>

Again we have lots of ways for this to happen...

Now X is stupid don't get me wrong and sorry Bjorn it might still
enable/disable devices it doesn't use (unless configured with some
arcania in xorg.conf), but this at least is step 1, this should allow
me to remove all the PCI BAR modfiying code from the Linux code paths,
and to be honest that is a very good start, we still need some sort of
VGA arbitration (which is why X actually messes with cards it doesn't
know about, as it has to make sure everyone isn't decoding the VGA
IOs...)

Dave.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/2/06, Dave Airlie <airlied@gmail.com> wrote:
> Jon stop being so dramatic, this is just like letting userspace map
> the BARs, without ownership through sysfs, which is a good thing, you
> can still map /dev/mem, look we have lots of ways to shoot ourselves
> in the foot, if we *want* to.

So why don't we just build a VGA class driver or make null fbdev
drivers? That solution works and stays in the model of one driver per
device and user space using a device driver to control the hardware.
This is a known problem and we have a valid solution, why build a new
API perpetuating the old model? If there wasn't a workable alternative
available I wouldn't be complaining.

Have you seen this method of getting root from X?
http://www.cansecwest.com/slides06/csw06-duflot.ppt
It is referenced from Theo de Raadt interview on kerneltrap
http://kerneltrap.org/node/6550

I am happy to see progress being made at getting X out of the PCI bus business.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Dave Airlie]

>
> So why don't we just build a VGA class driver or make null fbdev
> drivers? That solution works and stays in the model of one driver per
> device and user space using a device driver to control the hardware.
> This is a known problem and we have a valid solution, why build a new
> API perpetuating the old model? If there wasn't a workable alternative
> available I wouldn't be complaining.

We never have had one device driver for graphics devices, it would be
nice yes, it won't be happening today or tomorrow though, I'm trying
for now to remove as much of the evil crap from X as I can *now* so it
works on certain systems.... I am also however trying to not break
current deployments of systems already out there, the enable thing
allows this, without adding the vga class stuff, I know I'm going to
have to sort this merging of DRM and fb out at some stage, but so far
I've falied 3 times and I hate going back to it,

I'm going to have to force Greg to sit down and do some hacking at OLS
this year, no getting away without fully understanding what we need
(guess what the class changes didn't really help :-)

>
> Have you seen this method of getting root from X?
> http://www.cansecwest.com/slides06/csw06-duflot.ppt
> It is referenced from Theo de Raadt interview on kerneltrap
> http://kerneltrap.org/node/6550
>
> I am happy to see progress being made at getting X out of the PCI bus business.

Yes, and it really isn't a surprise, X is evil, but to be honest
fixing X doesn't fix that problem, it does allow you to fix the OS
more than likely though...

Dave.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Matthew Wilcox]

On Tue, May 02, 2006 at 05:52:09PM -0400, Jon Smirl wrote:
> Have you seen this method of getting root from X?
> http://www.cansecwest.com/slides06/csw06-duflot.ppt
> It is referenced from Theo de Raadt interview on kerneltrap
> http://kerneltrap.org/node/6550

That's a great indication of why securelevels aren't.
It pretty much fits the Linux model of "once you're root, you can do
anything".  The POSIX Capabilities really don't help either.
I suppose SELinux might be able to help, but I don't care to get into
that discussion here ;-)

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

On Tue, 02 May 2006 18:19:14 MDT, Matthew Wilcox said:

> I suppose SELinux might be able to help, but I don't care to get into
> that discussion here ;-)

The RedHat patch splatting most of /dev/mem would help more than SELinux would.
(Of course, that assumes that the offending address space is someplace that
X doesn't actually need itself, and that the patch blocks access to....)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/2/06, Matthew Wilcox <matthew@wil.cx> wrote:
> On Tue, May 02, 2006 at 05:52:09PM -0400, Jon Smirl wrote:
> > Have you seen this method of getting root from X?
> > http://www.cansecwest.com/slides06/csw06-duflot.ppt
> > It is referenced from Theo de Raadt interview on kerneltrap
> > http://kerneltrap.org/node/6550
>
> That's a great indication of why securelevels aren't.
> It pretty much fits the Linux model of "once you're root, you can do
> anything".  The POSIX Capabilities really don't help either.
> I suppose SELinux might be able to help, but I don't care to get into
> that discussion here ;-)

And there is the root exploit found by Coverity this week too:
http://news.yahoo.com/s/zd/20060502/tc_zd/177195

X is multiple megabytes of code needlessly running as root. If we
could convince X to use device drivers to talk to the hardware it
wouldn't need to run as root.  This is part of why I am against this
patch, it is another crutch to let X keep on running as root instead
of fixing the underlying problem.

--
Jon Smirl
jonsmirl@gmail.com

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Dave Airlie]

> >
> > That's a great indication of why securelevels aren't.
> > It pretty much fits the Linux model of "once you're root, you can do
> > anything".  The POSIX Capabilities really don't help either.
> > I suppose SELinux might be able to help, but I don't care to get into
> > that discussion here ;-)
>
> And there is the root exploit found by Coverity this week too:
> http://news.yahoo.com/s/zd/20060502/tc_zd/177195
>
> X is multiple megabytes of code needlessly running as root. If we
> could convince X to use device drivers to talk to the hardware it
> wouldn't need to run as root.  This is part of why I am against this
> patch, it is another crutch to let X keep on running as root instead
> of fixing the underlying problem.
>

That root hole is weeks old by now, just got to yahoo today, what you
say is true, however we can't just turn all the things in Linux off
that make X run as root, and then say go fix X we don't care. There
are steps to be taken, unfortunately they are neither pretty or can be
done really quickly....

Dave/

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Arjan van de Ven]

Jon Smirl wrote:
> On 5/2/06, Dave Airlie <airlied@gmail.com> wrote:
>> Jon stop being so dramatic, this is just like letting userspace map
>> the BARs, without ownership through sysfs, which is a good thing, you
>> can still map /dev/mem, look we have lots of ways to shoot ourselves
>> in the foot, if we *want* to.
> 
> So why don't we just build a VGA class driver or make null fbdev

I think your mail client is defective, you somehow managed to not attach the patch *again*.

Re: Add a "enable" sysfs attribute to the pci devices to allow userspace (Xorg) to enable devices without doing foul direct access

[Jon Smirl]

On 5/3/06, Arjan van de Ven <arjan@linux.intel.com> wrote:
> Jon Smirl wrote:
> > On 5/2/06, Dave Airlie <airlied@gmail.com> wrote:
> >> Jon stop being so dramatic, this is just like letting userspace map
> >> the BARs, without ownership through sysfs, which is a good thing, you
> >> can still map /dev/mem, look we have lots of ways to shoot ourselves
> >> in the foot, if we *want* to.
> >
> > So why don't we just build a VGA class driver or make null fbdev
>
> I think your mail client is defective, you somehow managed to not attach the patch *again*.
>

Lack of a patch for a correct solution is no reason to put a bad
solution into the kernel.  GregKH has a lot to say about PCI class
drivers and he has commented on this thread yet. DaveA and I have both
attempt to do the correct fix both those attempts have fallen apart
main for political reasons.


--
Jon Smirl
jonsmirl@gmail.com