Hi,
Santosh and I are having a problem figuring out how to enable binding
(and re-binding) platform devices to a platform VFIO driver (see
Antonis' WIP: [1]) in an upstream-acceptable manner.
Binding platform drivers currently depends on a string match in the
device node's compatible entry. On an arndale, one can currently
rebind the same device to the same driver like so:
echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
And one can bind it to the vfio-dt driver, as Antonis instructs, by
appending a 'vfio-dt' string to the device tree compatible entry for
the device. Then this would work:
echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
Consequently, the hack patch below [2] allows any platform device to be
bound to the vfio-dt driver, without making changes to the device
tree. It's a hack because I don't see having any driver name specific
code in drivers/base/bus.c being upstream acceptable.
Alternately, device tree compatible entries may be made writeable after
boot, e.g.:
echo vfio-platform > /proc/device-tree/i2c\@12CE0000/compatible
[note s/vfio-dt/vfio-platform/]
but that would require the vfio-platform module be reloaded, thereby
unbinding it from any existing devices it was bound to: we're
seeking a more dynamic solution.
Alex Graf (cc'd) proposed an alternate approach: re-write the driver
name in the device's sysfs entry:
echo "vfio-platform" > /sys/bus/platform/devices/101e0000.rtc/driver/driver_name
The advantage of this approach is that we can achieve the re-bind
(unbind + bind) as an atomic operation, which alleviates userspace from
having to coordinate with other device operations (I think VM migration
is an example case here).
Note that driver_name currently doesn't exist in sysfs, so it would
either have to be added, or another means developed to rename the
driver symlink itself:
cd /sys/bus/platform/devices/12ce0000.i2c
ln -s ../../bus/platform/drivers/s5p-ehci /tmp/tmp-link
mv -Tf /tmp/tmp-link driver
So I guess the question is: Is our understanding corret - are we on
the right track at all here? Is the hack below definitely not
acceptable? Is it correct to assume upstream maintainers are against
writing compatible entries to the device tree sysfs at runtime? Would
a driver_name be acceptable to add to sysfs, or should we investigate
something like the atomic mv command above further?
Thanks,
Kim
[1] Note that in this RFC, 'vfio-dt' is the name for the driver (-dt for
device tree) which has already been pointed out as a misnomer and
should probably be rewritten as 'vfio-platform':
http://lists.linux-foundation.org/pipermail/iommu/2013-August/006284.html
[2]
>From 6fa383d3f7bb53eb5efbb324c07484191b29543d Mon Sep 17 00:00:00 2001
From: Kim Phillips <[email protected]>
Date: Fri, 27 Sep 2013 14:36:04 -0500
Subject: [PATCH] hack/rfc: allow vfio-dt to bind to any platform device
---
drivers/base/bus.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 4c289ab..1cf08d6 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -201,7 +201,8 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
int err = -ENODEV;
dev = bus_find_device_by_name(bus, NULL, buf);
- if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
+ if (dev && dev->driver == NULL && (driver_match_device(drv, dev) ||
+ !strcmp(drv->name, "vfio-dt"))) {
if (dev->parent) /* Needed for USB */
device_lock(dev->parent);
device_lock(dev);
--
1.8.4
On Tue, 2013-10-01 at 13:38 -0500, Kim Phillips wrote:
> Hi,
>
> Santosh and I are having a problem figuring out how to enable binding
> (and re-binding) platform devices to a platform VFIO driver (see
> Antonis' WIP: [1]) in an upstream-acceptable manner.
>
> Binding platform drivers currently depends on a string match in the
> device node's compatible entry. On an arndale, one can currently
> rebind the same device to the same driver like so:
>
> echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
>
> And one can bind it to the vfio-dt driver, as Antonis instructs, by
> appending a 'vfio-dt' string to the device tree compatible entry for
> the device. Then this would work:
>
> echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
>
> Consequently, the hack patch below [2] allows any platform device to be
> bound to the vfio-dt driver, without making changes to the device
> tree. It's a hack because I don't see having any driver name specific
> code in drivers/base/bus.c being upstream acceptable.
Modifying the device tree is the worse part of this.
Is this part of your later suggestion to make compatible writeable after
boot, or are you talking about messing with the device tree before boot
(putting software config in the device tree, among other ickiness)?
> Alternately, device tree compatible entries may be made writeable after
> boot, e.g.:
>
> echo vfio-platform > /proc/device-tree/i2c\@12CE0000/compatible
>
> [note s/vfio-dt/vfio-platform/]
>
> but that would require the vfio-platform module be reloaded, thereby
> unbinding it from any existing devices it was bound to: we're
> seeking a more dynamic solution.
Eww.
Not to mention that the VFIO user might want to know what the compatible
was, or that we might later want to unbind from VFIO and rebind to the
kernel...
> Alex Graf (cc'd) proposed an alternate approach: re-write the driver
> name in the device's sysfs entry:
>
> echo "vfio-platform" > /sys/bus/platform/devices/101e0000.rtc/driver/driver_name
>
> The advantage of this approach is that we can achieve the re-bind
> (unbind + bind) as an atomic operation, which alleviates userspace from
> having to coordinate with other device operations (I think VM migration
> is an example case here).
>
> Note that driver_name currently doesn't exist in sysfs, so it would
> either have to be added, or another means developed to rename the
> driver symlink itself:
I think the ideal interface would be if you could write the sysfs device
name into the vfio bind file (or some new file in the same directory),
and have it claim that device (preferably with an atomic unbind from the
previous driver). We shouldn't be messing around with compatible
(either modifying it or telling VFIO which compatibles to look for) when
we know the specific devices (not just type of devices) we want to bind.
-Scott
On Tue, 2013-10-01 at 14:15 -0500, Scott Wood wrote:
> On Tue, 2013-10-01 at 13:38 -0500, Kim Phillips wrote:
> > Hi,
> >
> > Santosh and I are having a problem figuring out how to enable binding
> > (and re-binding) platform devices to a platform VFIO driver (see
> > Antonis' WIP: [1]) in an upstream-acceptable manner.
> >
> > Binding platform drivers currently depends on a string match in the
> > device node's compatible entry. On an arndale, one can currently
> > rebind the same device to the same driver like so:
> >
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
> >
> > And one can bind it to the vfio-dt driver, as Antonis instructs, by
> > appending a 'vfio-dt' string to the device tree compatible entry for
> > the device. Then this would work:
> >
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
> >
> > Consequently, the hack patch below [2] allows any platform device to be
> > bound to the vfio-dt driver, without making changes to the device
> > tree. It's a hack because I don't see having any driver name specific
> > code in drivers/base/bus.c being upstream acceptable.
>
> Modifying the device tree is the worse part of this.
I think I missed something. How do you reconcile "without making
changes to the device tree" with "appending a 'vfio-dt' string to the
device tree compatible entry"?
-Scott
On Tue, Oct 01, 2013 at 01:38:31PM -0500, Kim Phillips wrote:
> Hi,
>
> Santosh and I are having a problem figuring out how to enable binding
> (and re-binding) platform devices to a platform VFIO driver (see
> Antonis' WIP: [1]) in an upstream-acceptable manner.
>
> Binding platform drivers currently depends on a string match in the
> device node's compatible entry. On an arndale, one can currently
> rebind the same device to the same driver like so:
>
> echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
>
> And one can bind it to the vfio-dt driver, as Antonis instructs, by
> appending a 'vfio-dt' string to the device tree compatible entry for
> the device. Then this would work:
>
> echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
>
> Consequently, the hack patch below [2] allows any platform device to be
> bound to the vfio-dt driver, without making changes to the device
> tree. It's a hack because I don't see having any driver name specific
> code in drivers/base/bus.c being upstream acceptable.
You are correct.
What is wrong with just doing the above unbind/bind things through
sysfs, that is what it is there for, right?
greg k-h
On Tue, 1 Oct 2013 14:15:38 -0500
Scott Wood <[email protected]> wrote:
> On Tue, 2013-10-01 at 13:38 -0500, Kim Phillips wrote:
> > Hi,
> >
> > Santosh and I are having a problem figuring out how to enable binding
> > (and re-binding) platform devices to a platform VFIO driver (see
> > Antonis' WIP: [1]) in an upstream-acceptable manner.
> >
> > Binding platform drivers currently depends on a string match in the
> > device node's compatible entry. On an arndale, one can currently
> > rebind the same device to the same driver like so:
> >
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
> >
> > And one can bind it to the vfio-dt driver, as Antonis instructs, by
> > appending a 'vfio-dt' string to the device tree compatible entry for
> > the device. Then this would work:
> >
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
> >
> > Consequently, the hack patch below [2] allows any platform device to be
> > bound to the vfio-dt driver, without making changes to the device
> > tree. It's a hack because I don't see having any driver name specific
> > code in drivers/base/bus.c being upstream acceptable.
>
> Modifying the device tree is the worse part of this.
>
> Is this part of your later suggestion to make compatible writeable after
> boot, or are you talking about messing with the device tree before boot
> (putting software config in the device tree, among other ickiness)?
writeable after boot
> > Alternately, device tree compatible entries may be made writeable after
> > boot, e.g.:
> >
> > echo vfio-platform > /proc/device-tree/i2c\@12CE0000/compatible
> >
> > [note s/vfio-dt/vfio-platform/]
> >
> > but that would require the vfio-platform module be reloaded, thereby
> > unbinding it from any existing devices it was bound to: we're
> > seeking a more dynamic solution.
>
> Eww.
>
> Not to mention that the VFIO user might want to know what the compatible
> was,
well, technically the user would be able to get that info by reading
compatible before writing it, and ideally write the original value back
in addition to the new value.
> or that we might later want to unbind from VFIO and rebind to the
> kernel...
I believe that's independent: it would depend on which driver's (VFIO,
kernel, or other) sysfs file the device address gets written into.
> > Alex Graf (cc'd) proposed an alternate approach: re-write the driver
> > name in the device's sysfs entry:
> >
> > echo "vfio-platform" > /sys/bus/platform/devices/101e0000.rtc/driver/driver_name
> >
> > The advantage of this approach is that we can achieve the re-bind
> > (unbind + bind) as an atomic operation, which alleviates userspace from
> > having to coordinate with other device operations (I think VM migration
> > is an example case here).
> >
> > Note that driver_name currently doesn't exist in sysfs, so it would
> > either have to be added, or another means developed to rename the
> > driver symlink itself:
>
> I think the ideal interface would be if you could write the sysfs device
> name into the vfio bind file (or some new file in the same directory),
> and have it claim that device (preferably with an atomic unbind from the
> previous driver).
ok.
> We shouldn't be messing around with compatible
> (either modifying it or telling VFIO which compatibles to look for) when
> we know the specific devices (not just type of devices) we want to bind.
ok, but I still don't see how to get past driver_match_device()'s
refusal to allow bind a non-compatible driver (or one who's name isn't
in the compatible list).
Kim
On Tue, 1 Oct 2013 14:17:16 -0500
Scott Wood <[email protected]> wrote:
> On Tue, 2013-10-01 at 14:15 -0500, Scott Wood wrote:
> > On Tue, 2013-10-01 at 13:38 -0500, Kim Phillips wrote:
> > > Hi,
> > >
> > > Santosh and I are having a problem figuring out how to enable binding
> > > (and re-binding) platform devices to a platform VFIO driver (see
> > > Antonis' WIP: [1]) in an upstream-acceptable manner.
> > >
> > > Binding platform drivers currently depends on a string match in the
> > > device node's compatible entry. On an arndale, one can currently
> > > rebind the same device to the same driver like so:
> > >
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
> > >
> > > And one can bind it to the vfio-dt driver, as Antonis instructs, by
> > > appending a 'vfio-dt' string to the device tree compatible entry for
> > > the device. Then this would work:
> > >
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
> > >
> > > Consequently, the hack patch below [2] allows any platform device to be
> > > bound to the vfio-dt driver, without making changes to the device
> > > tree. It's a hack because I don't see having any driver name specific
> > > code in drivers/base/bus.c being upstream acceptable.
> >
> > Modifying the device tree is the worse part of this.
>
> I think I missed something. How do you reconcile "without making
> changes to the device tree" with "appending a 'vfio-dt' string to the
> device tree compatible entry"?
one doesn't need to append 'vfio-dt' to the device tree compatibles if
one uses the hack that makes bind_store ignore trying to
driver_match_device() if the driver is 'vfio-dt'.
Kim
On Tue, 1 Oct 2013 13:00:54 -0700
Greg Kroah-Hartman <[email protected]> wrote:
> On Tue, Oct 01, 2013 at 01:38:31PM -0500, Kim Phillips wrote:
> > Hi,
> >
> > Santosh and I are having a problem figuring out how to enable binding
> > (and re-binding) platform devices to a platform VFIO driver (see
> > Antonis' WIP: [1]) in an upstream-acceptable manner.
> >
> > Binding platform drivers currently depends on a string match in the
> > device node's compatible entry. On an arndale, one can currently
> > rebind the same device to the same driver like so:
> >
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
> >
> > And one can bind it to the vfio-dt driver, as Antonis instructs, by
> > appending a 'vfio-dt' string to the device tree compatible entry for
> > the device. Then this would work:
> >
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
> >
> > Consequently, the hack patch below [2] allows any platform device to be
> > bound to the vfio-dt driver, without making changes to the device
> > tree. It's a hack because I don't see having any driver name specific
> > code in drivers/base/bus.c being upstream acceptable.
>
> You are correct.
>
> What is wrong with just doing the above unbind/bind things through
> sysfs, that is what it is there for, right?
The bind fails because the compatible string in the device tree doesn't
match that of the VFIO platform driver, so driver_match_device always
returns false.
Kim
On Tue, 2013-10-01 at 16:59 -0500, Kim Phillips wrote:
> On Tue, 1 Oct 2013 14:15:38 -0500
> Scott Wood <[email protected]> wrote:
>
> > I think the ideal interface would be if you could write the sysfs device
> > name into the vfio bind file (or some new file in the same directory),
> > and have it claim that device (preferably with an atomic unbind from the
> > previous driver).
>
> ok.
...which apparently is what you are already doing (except for the atomic
part). My recollection of how this works on PCI (via new_id) apparently
kept me from reading it properly. :-P
> > We shouldn't be messing around with compatible
> > (either modifying it or telling VFIO which compatibles to look for) when
> > we know the specific devices (not just type of devices) we want to bind.
>
> ok, but I still don't see how to get past driver_match_device()'s
> refusal to allow bind a non-compatible driver (or one who's name isn't
> in the compatible list).
Probably something similar to your hack, except use a flag or some other
neutral mechanism rather than a driver name.
The flag could be something like "I'll try to bind to any device on this
bus, but only if explicitly requested".
-Scott
On Tue, Oct 01, 2013 at 05:02:44PM -0500, Kim Phillips wrote:
> On Tue, 1 Oct 2013 13:00:54 -0700
> Greg Kroah-Hartman <[email protected]> wrote:
>
> > On Tue, Oct 01, 2013 at 01:38:31PM -0500, Kim Phillips wrote:
> > > Hi,
> > >
> > > Santosh and I are having a problem figuring out how to enable binding
> > > (and re-binding) platform devices to a platform VFIO driver (see
> > > Antonis' WIP: [1]) in an upstream-acceptable manner.
> > >
> > > Binding platform drivers currently depends on a string match in the
> > > device node's compatible entry. On an arndale, one can currently
> > > rebind the same device to the same driver like so:
> > >
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
> > >
> > > And one can bind it to the vfio-dt driver, as Antonis instructs, by
> > > appending a 'vfio-dt' string to the device tree compatible entry for
> > > the device. Then this would work:
> > >
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
> > >
> > > Consequently, the hack patch below [2] allows any platform device to be
> > > bound to the vfio-dt driver, without making changes to the device
> > > tree. It's a hack because I don't see having any driver name specific
> > > code in drivers/base/bus.c being upstream acceptable.
> >
> > You are correct.
> >
> > What is wrong with just doing the above unbind/bind things through
> > sysfs, that is what it is there for, right?
>
> The bind fails because the compatible string in the device tree doesn't
> match that of the VFIO platform driver, so driver_match_device always
> returns false.
>
It sounds like this is not going to be pretty almost no matter what
we'll end up doing: Inherently VFIO is going to bind to a device without
the device tree entry for that device ever saying anything about VFIO.
How is this solved for PCI? Can we use some analogy from that work to
construct the missing piece?
-Christoffer
On Wed, 2013-10-02 at 02:53 +0100, Christoffer Dall wrote:
> On Tue, Oct 01, 2013 at 05:02:44PM -0500, Kim Phillips wrote:
> > On Tue, 1 Oct 2013 13:00:54 -0700
> > Greg Kroah-Hartman <[email protected]> wrote:
> >
> > > On Tue, Oct 01, 2013 at 01:38:31PM -0500, Kim Phillips wrote:
> > > > Hi,
> > > >
> > > > Santosh and I are having a problem figuring out how to enable binding
> > > > (and re-binding) platform devices to a platform VFIO driver (see
> > > > Antonis' WIP: [1]) in an upstream-acceptable manner.
> > > >
> > > > Binding platform drivers currently depends on a string match in the
> > > > device node's compatible entry. On an arndale, one can currently
> > > > rebind the same device to the same driver like so:
> > > >
> > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
> > > >
> > > > And one can bind it to the vfio-dt driver, as Antonis instructs, by
> > > > appending a 'vfio-dt' string to the device tree compatible entry for
> > > > the device. Then this would work:
> > > >
> > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
> > > >
> > > > Consequently, the hack patch below [2] allows any platform device to be
> > > > bound to the vfio-dt driver, without making changes to the device
> > > > tree. It's a hack because I don't see having any driver name specific
> > > > code in drivers/base/bus.c being upstream acceptable.
> > >
> > > You are correct.
> > >
> > > What is wrong with just doing the above unbind/bind things through
> > > sysfs, that is what it is there for, right?
> >
> > The bind fails because the compatible string in the device tree doesn't
> > match that of the VFIO platform driver, so driver_match_device always
> > returns false.
> >
> It sounds like this is not going to be pretty almost no matter what
> we'll end up doing: Inherently VFIO is going to bind to a device without
> the device tree entry for that device ever saying anything about VFIO.
>
> How is this solved for PCI? Can we use some analogy from that work to
> construct the missing piece?
PCI supports a dynamic ID table for driver/device matching, see
pci_add_dynid(). The problem is that this gets a little sloppy for the
period where you have multiple drivers that can claim the same device,
especially in the presence of hotplug. Thus the desire to improve the
situation with some kind of direct binding interface. Thanks,
Alex
On Tue, Oct 01, 2013 at 08:35:56PM -0600, Alex Williamson wrote:
> On Wed, 2013-10-02 at 02:53 +0100, Christoffer Dall wrote:
> > On Tue, Oct 01, 2013 at 05:02:44PM -0500, Kim Phillips wrote:
> > > On Tue, 1 Oct 2013 13:00:54 -0700
> > > Greg Kroah-Hartman <[email protected]> wrote:
> > >
> > > > On Tue, Oct 01, 2013 at 01:38:31PM -0500, Kim Phillips wrote:
> > > > > Hi,
> > > > >
> > > > > Santosh and I are having a problem figuring out how to enable binding
> > > > > (and re-binding) platform devices to a platform VFIO driver (see
> > > > > Antonis' WIP: [1]) in an upstream-acceptable manner.
> > > > >
> > > > > Binding platform drivers currently depends on a string match in the
> > > > > device node's compatible entry. On an arndale, one can currently
> > > > > rebind the same device to the same driver like so:
> > > > >
> > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
> > > > >
> > > > > And one can bind it to the vfio-dt driver, as Antonis instructs, by
> > > > > appending a 'vfio-dt' string to the device tree compatible entry for
> > > > > the device. Then this would work:
> > > > >
> > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
> > > > >
> > > > > Consequently, the hack patch below [2] allows any platform device to be
> > > > > bound to the vfio-dt driver, without making changes to the device
> > > > > tree. It's a hack because I don't see having any driver name specific
> > > > > code in drivers/base/bus.c being upstream acceptable.
> > > >
> > > > You are correct.
> > > >
> > > > What is wrong with just doing the above unbind/bind things through
> > > > sysfs, that is what it is there for, right?
> > >
> > > The bind fails because the compatible string in the device tree doesn't
> > > match that of the VFIO platform driver, so driver_match_device always
> > > returns false.
> > >
> > It sounds like this is not going to be pretty almost no matter what
> > we'll end up doing: Inherently VFIO is going to bind to a device without
> > the device tree entry for that device ever saying anything about VFIO.
> >
> > How is this solved for PCI? Can we use some analogy from that work to
> > construct the missing piece?
>
> PCI supports a dynamic ID table for driver/device matching, see
> pci_add_dynid(). The problem is that this gets a little sloppy for the
> period where you have multiple drivers that can claim the same device,
> especially in the presence of hotplug. Thus the desire to improve the
> situation with some kind of direct binding interface. Thanks,
>
So that's called on the vfio pci driver?
Wouldn't a sysfs file to add compatibility strings to the vfio-platform
driver make driver_match_device return true and make everyone happy?
There would be an issue of binding priority to solve, I guess similar to
the PCI problem, but then at least the two device types would share a
common orthogonal challenge.
-Christoffer
On Wed, 2013-10-02 at 16:14 +0100, Christoffer Dall wrote:
> On Tue, Oct 01, 2013 at 08:35:56PM -0600, Alex Williamson wrote:
> > On Wed, 2013-10-02 at 02:53 +0100, Christoffer Dall wrote:
> > > On Tue, Oct 01, 2013 at 05:02:44PM -0500, Kim Phillips wrote:
> > > > On Tue, 1 Oct 2013 13:00:54 -0700
> > > > Greg Kroah-Hartman <[email protected]> wrote:
> > > >
> > > > > On Tue, Oct 01, 2013 at 01:38:31PM -0500, Kim Phillips wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Santosh and I are having a problem figuring out how to enable binding
> > > > > > (and re-binding) platform devices to a platform VFIO driver (see
> > > > > > Antonis' WIP: [1]) in an upstream-acceptable manner.
> > > > > >
> > > > > > Binding platform drivers currently depends on a string match in the
> > > > > > device node's compatible entry. On an arndale, one can currently
> > > > > > rebind the same device to the same driver like so:
> > > > > >
> > > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
> > > > > >
> > > > > > And one can bind it to the vfio-dt driver, as Antonis instructs, by
> > > > > > appending a 'vfio-dt' string to the device tree compatible entry for
> > > > > > the device. Then this would work:
> > > > > >
> > > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/12ce0000.i2c/driver/unbind
> > > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
> > > > > >
> > > > > > Consequently, the hack patch below [2] allows any platform device to be
> > > > > > bound to the vfio-dt driver, without making changes to the device
> > > > > > tree. It's a hack because I don't see having any driver name specific
> > > > > > code in drivers/base/bus.c being upstream acceptable.
> > > > >
> > > > > You are correct.
> > > > >
> > > > > What is wrong with just doing the above unbind/bind things through
> > > > > sysfs, that is what it is there for, right?
> > > >
> > > > The bind fails because the compatible string in the device tree doesn't
> > > > match that of the VFIO platform driver, so driver_match_device always
> > > > returns false.
> > > >
> > > It sounds like this is not going to be pretty almost no matter what
> > > we'll end up doing: Inherently VFIO is going to bind to a device without
> > > the device tree entry for that device ever saying anything about VFIO.
> > >
> > > How is this solved for PCI? Can we use some analogy from that work to
> > > construct the missing piece?
> >
> > PCI supports a dynamic ID table for driver/device matching, see
> > pci_add_dynid(). The problem is that this gets a little sloppy for the
> > period where you have multiple drivers that can claim the same device,
> > especially in the presence of hotplug. Thus the desire to improve the
> > situation with some kind of direct binding interface. Thanks,
> >
> So that's called on the vfio pci driver?
This happens at the PCI bus driver level, all PCI drivers support
dynamic IDs with a sysfs entry for adding and removing them. vfio-pci
starts with an empty ID table and all it sees are .probe() callbacks
when the dynamic table is updated and a match is made.
> Wouldn't a sysfs file to add compatibility strings to the vfio-platform
> driver make driver_match_device return true and make everyone happy?
Seems like it. Since you don't have a bus driver providing that
infrastructure for you the driver would need to do it by itself.
> There would be an issue of binding priority to solve, I guess similar to
> the PCI problem, but then at least the two device types would share a
> common orthogonal challenge.
Perhaps some sort of "force_bind" sysfs entry created by the driver that
can unbind the existing driver and skip the match code. Thanks,
Alex
> -----Original Message-----
> From: Christoffer Dall [mailto:[email protected]]
> Sent: Wednesday, October 02, 2013 10:14 AM
> To: Alex Williamson
> Cc: Kim Phillips; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> Yoder Stuart-B08248; Wood Scott-B07421; Sethi Varun-B16395; Bhushan
> Bharat-R65777; [email protected]; [email protected];
> [email protected]
> Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> device
>
> On Tue, Oct 01, 2013 at 08:35:56PM -0600, Alex Williamson wrote:
> > On Wed, 2013-10-02 at 02:53 +0100, Christoffer Dall wrote:
> > > On Tue, Oct 01, 2013 at 05:02:44PM -0500, Kim Phillips wrote:
> > > > On Tue, 1 Oct 2013 13:00:54 -0700
> > > > Greg Kroah-Hartman <[email protected]> wrote:
> > > >
> > > > > On Tue, Oct 01, 2013 at 01:38:31PM -0500, Kim Phillips wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Santosh and I are having a problem figuring out how to enable
> binding
> > > > > > (and re-binding) platform devices to a platform VFIO driver
> (see
> > > > > > Antonis' WIP: [1]) in an upstream-acceptable manner.
> > > > > >
> > > > > > Binding platform drivers currently depends on a string match in
> the
> > > > > > device node's compatible entry. On an arndale, one can
> currently
> > > > > > rebind the same device to the same driver like so:
> > > > > >
> > > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-
> i2c/12ce0000.i2c/driver/unbind
> > > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
> > > > > >
> > > > > > And one can bind it to the vfio-dt driver, as Antonis
> instructs, by
> > > > > > appending a 'vfio-dt' string to the device tree compatible
> entry for
> > > > > > the device. Then this would work:
> > > > > >
> > > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-
> i2c/12ce0000.i2c/driver/unbind
> > > > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-dt/bind
> > > > > >
> > > > > > Consequently, the hack patch below [2] allows any platform
> device to be
> > > > > > bound to the vfio-dt driver, without making changes to the
> device
> > > > > > tree. It's a hack because I don't see having any driver name
> specific
> > > > > > code in drivers/base/bus.c being upstream acceptable.
> > > > >
> > > > > You are correct.
> > > > >
> > > > > What is wrong with just doing the above unbind/bind things
> through
> > > > > sysfs, that is what it is there for, right?
> > > >
> > > > The bind fails because the compatible string in the device tree
> doesn't
> > > > match that of the VFIO platform driver, so driver_match_device
> always
> > > > returns false.
> > > >
> > > It sounds like this is not going to be pretty almost no matter what
> > > we'll end up doing: Inherently VFIO is going to bind to a device
> without
> > > the device tree entry for that device ever saying anything about
> VFIO.
> > >
> > > How is this solved for PCI? Can we use some analogy from that work
> to
> > > construct the missing piece?
> >
> > PCI supports a dynamic ID table for driver/device matching, see
> > pci_add_dynid(). The problem is that this gets a little sloppy for the
> > period where you have multiple drivers that can claim the same device,
> > especially in the presence of hotplug. Thus the desire to improve the
> > situation with some kind of direct binding interface. Thanks,
> >
> So that's called on the vfio pci driver?
>
> Wouldn't a sysfs file to add compatibility strings to the vfio-platform
> driver make driver_match_device return true and make everyone happy?
I had a similar thought. Why can't we do something like:
echo "fsl,i2c" > /sys/bus/platform/drivers/vfio-platform/new_compatible
echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
The first steps tell vfio-platform to register itself to handle
"fsl,i2c" compatible devices. The second step does the bind.
Stuart
On Wed, 2013-10-02 at 13:25 -0500, Yoder Stuart-B08248 wrote:
>
> > -----Original Message-----
> > From: Christoffer Dall [mailto:[email protected]]
> > Sent: Wednesday, October 02, 2013 10:14 AM
> > To: Alex Williamson
> > Cc: Kim Phillips; [email protected]; linux-
> > [email protected]; [email protected]; [email protected];
> > Yoder Stuart-B08248; Wood Scott-B07421; Sethi Varun-B16395; Bhushan
> > Bharat-R65777; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> > device
> >
> > Wouldn't a sysfs file to add compatibility strings to the vfio-platform
> > driver make driver_match_device return true and make everyone happy?
>
> I had a similar thought. Why can't we do something like:
>
> echo "fsl,i2c" > /sys/bus/platform/drivers/vfio-platform/new_compatible
> echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
>
> The first steps tell vfio-platform to register itself to handle
> "fsl,i2c" compatible devices. The second step does the bind.
Needing to specify the compatible is hacky (we already know what device
we want to bind; why do we need to scrounge up more information than
that, and add a new sysfs interface for extending compatible matches,
and a more flexible data structure to back that up?), and is racy on
buses that can hotplug (which driver gets the new device?).
What's wrong with a non-vfio-specific flag that a driver can set, that
indicates that the driver is willing to try to bind to any device on the
bus if explicitly requested via the existing sysfs bind mechanism?
-Scott
On Wed, Oct 02, 2013 at 01:32:38PM -0500, Scott Wood wrote:
> On Wed, 2013-10-02 at 13:25 -0500, Yoder Stuart-B08248 wrote:
> >
> > > -----Original Message-----
> > > From: Christoffer Dall [mailto:[email protected]]
> > > Sent: Wednesday, October 02, 2013 10:14 AM
> > > To: Alex Williamson
> > > Cc: Kim Phillips; [email protected]; linux-
> > > [email protected]; [email protected]; [email protected];
> > > Yoder Stuart-B08248; Wood Scott-B07421; Sethi Varun-B16395; Bhushan
> > > Bharat-R65777; [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> > > device
> > >
> > > Wouldn't a sysfs file to add compatibility strings to the vfio-platform
> > > driver make driver_match_device return true and make everyone happy?
> >
> > I had a similar thought. Why can't we do something like:
> >
> > echo "fsl,i2c" > /sys/bus/platform/drivers/vfio-platform/new_compatible
> > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> >
> > The first steps tell vfio-platform to register itself to handle
> > "fsl,i2c" compatible devices. The second step does the bind.
>
> Needing to specify the compatible is hacky (we already know what device
> we want to bind; why do we need to scrounge up more information than
> that, and add a new sysfs interface for extending compatible matches,
> and a more flexible data structure to back that up?), and is racy on
> buses that can hotplug (which driver gets the new device?).
Why hacky? It seems quite reasonable to me that the user has to tell a
subsystem that from a certain point it should be capable of handling
some device.
As for the data structure, isn't this a simple linked list?
The problem with the race seems to be a common problem that hasn't even
been solved for PCI yet, so I'm wondering if this is not an orthogonal
issue with a separate solution, such as a priority or something like
that.
Yes, once you've added the new_compatible to the vfio-platform driver,
it's up for grabs from both the new and the old driver, but that could
be solved by always making sure that the vfio-platform driver is checked
first.
(I'm not familiar with these data structures, but I would imagine
something like re-inserting the vfio-platform driver in the
list/tree/... whenever adding a new_compatible value might possibly be
one solution).
>
> What's wrong with a non-vfio-specific flag that a driver can set, that
> indicates that the driver is willing to try to bind to any device on the
> bus if explicitly requested via the existing sysfs bind mechanism?
>
It sounds more hackish to me to invent some 'generic' flag to solve a
very specific case. What you're suggesting would let users specify that
a serial driver should handle a NIC hardware, no? That sounds much much
worse to me.
-Christoffer
On Wed, 2 Oct 2013 11:43:30 -0700
Christoffer Dall <[email protected]> wrote:
> On Wed, Oct 02, 2013 at 01:32:38PM -0500, Scott Wood wrote:
> > On Wed, 2013-10-02 at 13:25 -0500, Yoder Stuart-B08248 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Christoffer Dall [mailto:[email protected]]
> > > > Sent: Wednesday, October 02, 2013 10:14 AM
> > > > To: Alex Williamson
> > > > Cc: Kim Phillips; [email protected]; linux-
> > > > [email protected]; [email protected]; [email protected];
> > > > Yoder Stuart-B08248; Wood Scott-B07421; Sethi Varun-B16395; Bhushan
> > > > Bharat-R65777; [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> > > > device
> > > >
> > > > Wouldn't a sysfs file to add compatibility strings to the vfio-platform
> > > > driver make driver_match_device return true and make everyone happy?
> > >
> > > I had a similar thought. Why can't we do something like:
> > >
> > > echo "fsl,i2c" > /sys/bus/platform/drivers/vfio-platform/new_compatible
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> > >
> > > The first steps tell vfio-platform to register itself to handle
> > > "fsl,i2c" compatible devices. The second step does the bind.
> >
> > Needing to specify the compatible is hacky (we already know what device
> > we want to bind; why do we need to scrounge up more information than
> > that, and add a new sysfs interface for extending compatible matches,
> > and a more flexible data structure to back that up?), and is racy on
> > buses that can hotplug (which driver gets the new device?).
>
> Why hacky? It seems quite reasonable to me that the user has to tell a
> subsystem that from a certain point it should be capable of handling
> some device.
I think what Scott is saying is that the first echo above is somewhat
redundant with the second: they're both talking to the vfio-platform
driver about an i2c device.
> As for the data structure, isn't this a simple linked list?
>
> The problem with the race seems to be a common problem that hasn't even
> been solved for PCI yet, so I'm wondering if this is not an orthogonal
> issue with a separate solution, such as a priority or something like
> that.
I agree; adding 'direct'/'atomic' functionality to the existing bind
sysfs file, i.e., bind_store() logic to perform device_release_driver()
logic if dev->driver != NULL, all under the same device_lock() is an
independent problem from binding the VFIO platform driver to a platform
device.
> Yes, once you've added the new_compatible to the vfio-platform driver,
> it's up for grabs from both the new and the old driver, but that could
> be solved by always making sure that the vfio-platform driver is checked
> first.
not sure I understand the priority problem here - haven't looked at PCI
yet - but, given the above 'atomic bind' functionality described above,
the new and old driver wouldn't be requesting to bind to the same
device simultaneously, no?
> (I'm not familiar with these data structures, but I would imagine
> something like re-inserting the vfio-platform driver in the
> list/tree/... whenever adding a new_compatible value might possibly be
> one solution).
>
> > What's wrong with a non-vfio-specific flag that a driver can set, that
> > indicates that the driver is willing to try to bind to any device on the
> > bus if explicitly requested via the existing sysfs bind mechanism?
> >
> It sounds more hackish to me to invent some 'generic' flag to solve a
> very specific case. What you're suggesting would let users specify that
> a serial driver should handle a NIC hardware, no? That sounds much much
> worse to me.
I thought that was the nature of VFIO drivers...it's a 'meta-' driver,
used for enabling userspace drivers at large.
Kim
On Wed, Oct 02, 2013 at 03:04:15PM -0500, Kim Phillips wrote:
> On Wed, 2 Oct 2013 11:43:30 -0700
> Christoffer Dall <[email protected]> wrote:
>
> > On Wed, Oct 02, 2013 at 01:32:38PM -0500, Scott Wood wrote:
> > > On Wed, 2013-10-02 at 13:25 -0500, Yoder Stuart-B08248 wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Christoffer Dall [mailto:[email protected]]
> > > > > Sent: Wednesday, October 02, 2013 10:14 AM
> > > > > To: Alex Williamson
> > > > > Cc: Kim Phillips; [email protected]; linux-
> > > > > [email protected]; [email protected]; [email protected];
> > > > > Yoder Stuart-B08248; Wood Scott-B07421; Sethi Varun-B16395; Bhushan
> > > > > Bharat-R65777; [email protected]; [email protected];
> > > > > [email protected]
> > > > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> > > > > device
> > > > >
> > > > > Wouldn't a sysfs file to add compatibility strings to the vfio-platform
> > > > > driver make driver_match_device return true and make everyone happy?
> > > >
> > > > I had a similar thought. Why can't we do something like:
> > > >
> > > > echo "fsl,i2c" > /sys/bus/platform/drivers/vfio-platform/new_compatible
> > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> > > >
> > > > The first steps tell vfio-platform to register itself to handle
> > > > "fsl,i2c" compatible devices. The second step does the bind.
> > >
> > > Needing to specify the compatible is hacky (we already know what device
> > > we want to bind; why do we need to scrounge up more information than
> > > that, and add a new sysfs interface for extending compatible matches,
> > > and a more flexible data structure to back that up?), and is racy on
> > > buses that can hotplug (which driver gets the new device?).
> >
> > Why hacky? It seems quite reasonable to me that the user has to tell a
> > subsystem that from a certain point it should be capable of handling
> > some device.
>
> I think what Scott is saying is that the first echo above is somewhat
> redundant with the second: they're both talking to the vfio-platform
> driver about an i2c device.
>
ok, fair enough, I didn't commit particularly to that interface, but
having a special hook for checking a flag kind of like the strcmp hack
you posted, just seems weird to me; it would be better if the vfio
driver can add the bind string to the list of compatible devices it can
bind to, and then use the generic bind mechanism in the kernel. But I'm
honestly not familiar enough with the implementaiton specific bits of
the syfs interface to argue how the changes are for one method vs. the
other.
> > As for the data structure, isn't this a simple linked list?
> >
> > The problem with the race seems to be a common problem that hasn't even
> > been solved for PCI yet, so I'm wondering if this is not an orthogonal
> > issue with a separate solution, such as a priority or something like
> > that.
>
> I agree; adding 'direct'/'atomic' functionality to the existing bind
> sysfs file, i.e., bind_store() logic to perform device_release_driver()
> logic if dev->driver != NULL, all under the same device_lock() is an
> independent problem from binding the VFIO platform driver to a platform
> device.
>
> > Yes, once you've added the new_compatible to the vfio-platform driver,
> > it's up for grabs from both the new and the old driver, but that could
> > be solved by always making sure that the vfio-platform driver is checked
> > first.
>
> not sure I understand the priority problem here - haven't looked at PCI
> yet - but, given the above 'atomic bind' functionality described above,
> the new and old driver wouldn't be requesting to bind to the same
> device simultaneously, no?
>
AFAIU, the problem occurs with hotplug. If you add the compatibility
string to a new driver and then hotplug a device with the string, then
which driver gets the device?
> > (I'm not familiar with these data structures, but I would imagine
> > something like re-inserting the vfio-platform driver in the
> > list/tree/... whenever adding a new_compatible value might possibly be
> > one solution).
> >
> > > What's wrong with a non-vfio-specific flag that a driver can set, that
> > > indicates that the driver is willing to try to bind to any device on the
> > > bus if explicitly requested via the existing sysfs bind mechanism?
> > >
> > It sounds more hackish to me to invent some 'generic' flag to solve a
> > very specific case. What you're suggesting would let users specify that
> > a serial driver should handle a NIC hardware, no? That sounds much much
> > worse to me.
>
> I thought that was the nature of VFIO drivers...it's a 'meta-' driver,
> used for enabling userspace drivers at large.
>
Yes, vfio is a meta driver, therefore it needs to be able to do
something special, but the generic driver/device/bus matching framework
doesn't need an extra generic feature allowing you to bind driver X to
device Y for all combinations of X and Y depending on some flag...
Someone please correct me if there are more use cases for this and this
is in fact worth a generic solution.
-Christoffer
On Wed, 2013-10-02 at 11:43 -0700, Christoffer Dall wrote:
> On Wed, Oct 02, 2013 at 01:32:38PM -0500, Scott Wood wrote:
> > On Wed, 2013-10-02 at 13:25 -0500, Yoder Stuart-B08248 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Christoffer Dall [mailto:[email protected]]
> > > > Sent: Wednesday, October 02, 2013 10:14 AM
> > > > To: Alex Williamson
> > > > Cc: Kim Phillips; [email protected]; linux-
> > > > [email protected]; [email protected]; [email protected];
> > > > Yoder Stuart-B08248; Wood Scott-B07421; Sethi Varun-B16395; Bhushan
> > > > Bharat-R65777; [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> > > > device
> > > >
> > > > Wouldn't a sysfs file to add compatibility strings to the vfio-platform
> > > > driver make driver_match_device return true and make everyone happy?
> > >
> > > I had a similar thought. Why can't we do something like:
> > >
> > > echo "fsl,i2c" > /sys/bus/platform/drivers/vfio-platform/new_compatible
> > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> > >
> > > The first steps tell vfio-platform to register itself to handle
> > > "fsl,i2c" compatible devices. The second step does the bind.
> >
> > Needing to specify the compatible is hacky (we already know what device
> > we want to bind; why do we need to scrounge up more information than
> > that, and add a new sysfs interface for extending compatible matches,
> > and a more flexible data structure to back that up?), and is racy on
> > buses that can hotplug (which driver gets the new device?).
>
> Why hacky? It seems quite reasonable to me that the user has to tell a
> subsystem that from a certain point it should be capable of handling
> some device.
But the reason that driver can handle the device has nothing to do with
the compatible -- it can handle any device on the bus (except for
limitations checked for in the probe function), but it's a policy
decision whether we want it to handle any particular device (as opposed
to a particular type of device).
Now, if we end up requiring a device-aware kernel stub for VFIO platform
devices (as was discussed for handling reset and such), we won't want a
wildcard match, but we'd still want to say that devices only get bound
to vfio upon explicit request. We also would not want userspace adding
to vfio's compatible list in that case. So perhaps a flag for "only
bind on explicit request" should be separate from the ability to supply
a wildcard match. VFIO PCI could still use the wildcard match.
> As for the data structure, isn't this a simple linked list?
It could be, but currently it's an array, which is what all the drivers
are expecting to provide. Adding a second parallel mechanism (like PCI
dynid) seems excessive compared to adding a wildcard match (on PCI such
a mechanism happened to already exist, which made it easy to use for
this, even if it wasn't necessarily the best approach). And then what
happens on non-device-tree platform devices?
> The problem with the race seems to be a common problem that hasn't even
> been solved for PCI yet, so I'm wondering if this is not an orthogonal
> issue with a separate solution, such as a priority or something like
> that.
>
> Yes, once you've added the new_compatible to the vfio-platform driver,
> it's up for grabs from both the new and the old driver, but that could
> be solved by always making sure that the vfio-platform driver is checked
> first.
...which is the opposite of what you want if a different device of the
same type is plugged in after you add the device type ID to vfio. A
"bind only by request" flag would avoid that problem.
As for the possibility that the normal driver would claim it (maybe due
to the bus being rescanned after a hotplug event?), that could be
addressed with a mechanism to atomically unbind-and-rebind, or (perhaps
easier) by making it so that once a device has been explicitly unbound,
it can only be bound again by explicit request (which would also allow a
user to say "I don't want to use this device at all" without it getting
rebound later).
Better still would be an independent "don't bind by default" flag that
the user can set in sysfs (this is different from the driver's "don't
bind by default" flag that is set by the driver), so that the action is
reversible, and so a user can set this policy regardless of whether a
driver has been loaded yet.
> > What's wrong with a non-vfio-specific flag that a driver can set, that
> > indicates that the driver is willing to try to bind to any device on the
> > bus if explicitly requested via the existing sysfs bind mechanism?
> >
> It sounds more hackish to me to invent some 'generic' flag to solve a
> very specific case.
new_compatible would be to solve an even more specific case, but both
new_compatible and a don't-bind-by-default flag could have applications
elsewhere.
> What you're suggesting would let users specify that
> a serial driver should handle a NIC hardware, no? That sounds much much
> worse to me.
The flag (and wildcard match, if applicable) would be set by the driver,
not by the user. Whereas PCI's "new_id" and the "new_compatible" being
suggested in this thread would allow exactly that, assuming the driver
doesn't reject the device in the probe function.
-Scott
On Wed, 2013-10-02 at 21:13 +0100, Christoffer Dall wrote:
> On Wed, Oct 02, 2013 at 03:04:15PM -0500, Kim Phillips wrote:
> > On Wed, 2 Oct 2013 11:43:30 -0700
> > Christoffer Dall <[email protected]> wrote:
> >
> > > On Wed, Oct 02, 2013 at 01:32:38PM -0500, Scott Wood wrote:
> > > > What's wrong with a non-vfio-specific flag that a driver can set, that
> > > > indicates that the driver is willing to try to bind to any device on the
> > > > bus if explicitly requested via the existing sysfs bind mechanism?
> > > >
> > > It sounds more hackish to me to invent some 'generic' flag to solve a
> > > very specific case. What you're suggesting would let users specify that
> > > a serial driver should handle a NIC hardware, no? That sounds much much
> > > worse to me.
> >
> > I thought that was the nature of VFIO drivers...it's a 'meta-' driver,
> > used for enabling userspace drivers at large.
> >
> Yes, vfio is a meta driver, therefore it needs to be able to do
> something special, but the generic driver/device/bus matching framework
> doesn't need an extra generic feature allowing you to bind driver X to
> device Y for all combinations of X and Y depending on some flag...
Not all combinations of X and Y. Only instances of X that advertise
that this is OK.
> Someone please correct me if there are more use cases for this and this
> is in fact worth a generic solution.
Note that the wildcard match that I suggested in the e-mail I just sent
would likely be implemented by the bus match code -- not by generic
driver model code. It would still be less intrusive than implementing a
dynamic match mechanism for each bus type (and for device tree, ACPI,
etc in the case of platform bus).
-Scott
On Wed, Oct 02, 2013 at 03:14:02PM -0500, Scott Wood wrote:
> On Wed, 2013-10-02 at 11:43 -0700, Christoffer Dall wrote:
> > On Wed, Oct 02, 2013 at 01:32:38PM -0500, Scott Wood wrote:
> > > On Wed, 2013-10-02 at 13:25 -0500, Yoder Stuart-B08248 wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Christoffer Dall [mailto:[email protected]]
> > > > > Sent: Wednesday, October 02, 2013 10:14 AM
> > > > > To: Alex Williamson
> > > > > Cc: Kim Phillips; [email protected]; linux-
> > > > > [email protected]; [email protected]; [email protected];
> > > > > Yoder Stuart-B08248; Wood Scott-B07421; Sethi Varun-B16395; Bhushan
> > > > > Bharat-R65777; [email protected]; [email protected];
> > > > > [email protected]
> > > > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> > > > > device
> > > > >
> > > > > Wouldn't a sysfs file to add compatibility strings to the vfio-platform
> > > > > driver make driver_match_device return true and make everyone happy?
> > > >
> > > > I had a similar thought. Why can't we do something like:
> > > >
> > > > echo "fsl,i2c" > /sys/bus/platform/drivers/vfio-platform/new_compatible
> > > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> > > >
> > > > The first steps tell vfio-platform to register itself to handle
> > > > "fsl,i2c" compatible devices. The second step does the bind.
> > >
> > > Needing to specify the compatible is hacky (we already know what device
> > > we want to bind; why do we need to scrounge up more information than
> > > that, and add a new sysfs interface for extending compatible matches,
> > > and a more flexible data structure to back that up?), and is racy on
> > > buses that can hotplug (which driver gets the new device?).
> >
> > Why hacky? It seems quite reasonable to me that the user has to tell a
> > subsystem that from a certain point it should be capable of handling
> > some device.
>
> But the reason that driver can handle the device has nothing to do with
> the compatible -- it can handle any device on the bus (except for
> limitations checked for in the probe function), but it's a policy
> decision whether we want it to handle any particular device (as opposed
> to a particular type of device).
>
> Now, if we end up requiring a device-aware kernel stub for VFIO platform
> devices (as was discussed for handling reset and such), we won't want a
> wildcard match, but we'd still want to say that devices only get bound
> to vfio upon explicit request. We also would not want userspace adding
> to vfio's compatible list in that case. So perhaps a flag for "only
> bind on explicit request" should be separate from the ability to supply
> a wildcard match. VFIO PCI could still use the wildcard match.
>
I don't disagree on your observations here, the question is only how it
fits with the existing driver/device/bus code. I just don't think
having a series of flag and having to test all sorts of combination of
those flags to see if any driver can bind to some device sounds very
nice, if we always have the case that, either:
(1) There's one and only one driver for a device
(2) There's the driver itself, and now the user asked for VFIO to bind
to a device as well.
If we need something more flexible overall for the binding, then yes,
some set of appropriate flags is probably a good idea, but if we're only
trying to solve (2), it seems better to me to keep changes as isolated
to VFIO as possible.
> > As for the data structure, isn't this a simple linked list?
>
> It could be, but currently it's an array, which is what all the drivers
> are expecting to provide. Adding a second parallel mechanism (like PCI
> dynid) seems excessive compared to adding a wildcard match (on PCI such
> a mechanism happened to already exist, which made it easy to use for
> this, even if it wasn't necessarily the best approach). And then what
> happens on non-device-tree platform devices?
>
> > The problem with the race seems to be a common problem that hasn't even
> > been solved for PCI yet, so I'm wondering if this is not an orthogonal
> > issue with a separate solution, such as a priority or something like
> > that.
> >
> > Yes, once you've added the new_compatible to the vfio-platform driver,
> > it's up for grabs from both the new and the old driver, but that could
> > be solved by always making sure that the vfio-platform driver is checked
> > first.
>
> ...which is the opposite of what you want if a different device of the
> same type is plugged in after you add the device type ID to vfio. A
> "bind only by request" flag would avoid that problem.
ok, then reverse the priority.
>
> As for the possibility that the normal driver would claim it (maybe due
> to the bus being rescanned after a hotplug event?), that could be
> addressed with a mechanism to atomically unbind-and-rebind, or (perhaps
> easier) by making it so that once a device has been explicitly unbound,
> it can only be bound again by explicit request (which would also allow a
> user to say "I don't want to use this device at all" without it getting
> rebound later).
>
> Better still would be an independent "don't bind by default" flag that
> the user can set in sysfs (this is different from the driver's "don't
> bind by default" flag that is set by the driver), so that the action is
> reversible, and so a user can set this policy regardless of whether a
> driver has been loaded yet.
>
this does sound reasonable...
> > > What's wrong with a non-vfio-specific flag that a driver can set, that
> > > indicates that the driver is willing to try to bind to any device on the
> > > bus if explicitly requested via the existing sysfs bind mechanism?
> > >
> > It sounds more hackish to me to invent some 'generic' flag to solve a
> > very specific case.
>
> new_compatible would be to solve an even more specific case, but both
> new_compatible and a don't-bind-by-default flag could have applications
> elsewhere.
>
I'm not a fan of doing something because something "_could_ have
applications elsewhere". I think we need concrete cases to back up the
choices for doing something in a specific way. I think that VFIO is one
very specific deviant from how all other driver/device binding works,
and therefore doing something very specific to VFIO, as isolated as
possible to VFIO, is the right thing.
> > What you're suggesting would let users specify that
> > a serial driver should handle a NIC hardware, no? That sounds much much
> > worse to me.
>
> The flag (and wildcard match, if applicable) would be set by the driver,
> not by the user. Whereas PCI's "new_id" and the "new_compatible" being
> suggested in this thread would allow exactly that, assuming the driver
> doesn't reject the device in the probe function.
>
ok, fair enough, but still, I don't see the _generic_ need for having a
kernel feature that allows some random driver to bind to a device, but
then again, I'm not an authority in this area.
-Christoffer
On Wed, Oct 02, 2013 at 11:43:30AM -0700, Christoffer Dall wrote:
> > What's wrong with a non-vfio-specific flag that a driver can set, that
> > indicates that the driver is willing to try to bind to any device on the
> > bus if explicitly requested via the existing sysfs bind mechanism?
> >
> It sounds more hackish to me to invent some 'generic' flag to solve a
> very specific case. What you're suggesting would let users specify that
> a serial driver should handle a NIC hardware, no? That sounds much much
> worse to me.
You can do that today, with any PCI driver (or USB driver as well), just
use the bind/unbind files in sysfs and you had better "know" what you
are doing...
On Wed, Oct 02, 2013 at 09:27:38PM +0100, Christoffer Dall wrote:
> > > What you're suggesting would let users specify that
> > > a serial driver should handle a NIC hardware, no? That sounds much much
> > > worse to me.
> >
> > The flag (and wildcard match, if applicable) would be set by the driver,
> > not by the user. Whereas PCI's "new_id" and the "new_compatible" being
> > suggested in this thread would allow exactly that, assuming the driver
> > doesn't reject the device in the probe function.
> >
> ok, fair enough, but still, I don't see the _generic_ need for having a
> kernel feature that allows some random driver to bind to a device, but
> then again, I'm not an authority in this area.
Again, this is already there, and has been for years with no problems,
so I really don't understand the issue you have with it.
greg k-h
On Wed, Oct 02, 2013 at 01:37:35PM -0700, [email protected] wrote:
> On Wed, Oct 02, 2013 at 11:43:30AM -0700, Christoffer Dall wrote:
> > > What's wrong with a non-vfio-specific flag that a driver can set, that
> > > indicates that the driver is willing to try to bind to any device on the
> > > bus if explicitly requested via the existing sysfs bind mechanism?
> > >
> > It sounds more hackish to me to invent some 'generic' flag to solve a
> > very specific case. What you're suggesting would let users specify that
> > a serial driver should handle a NIC hardware, no? That sounds much much
> > worse to me.
>
> You can do that today, with any PCI driver (or USB driver as well), just
> use the bind/unbind files in sysfs and you had better "know" what you
> are doing...
>
OK, yikes, ignore my comment then.
On Wed, Oct 02, 2013 at 01:39:43PM -0700, [email protected] wrote:
> On Wed, Oct 02, 2013 at 09:27:38PM +0100, Christoffer Dall wrote:
> > > > What you're suggesting would let users specify that
> > > > a serial driver should handle a NIC hardware, no? That sounds much much
> > > > worse to me.
> > >
> > > The flag (and wildcard match, if applicable) would be set by the driver,
> > > not by the user. Whereas PCI's "new_id" and the "new_compatible" being
> > > suggested in this thread would allow exactly that, assuming the driver
> > > doesn't reject the device in the probe function.
> > >
> > ok, fair enough, but still, I don't see the _generic_ need for having a
> > kernel feature that allows some random driver to bind to a device, but
> > then again, I'm not an authority in this area.
>
> Again, this is already there, and has been for years with no problems,
> so I really don't understand the issue you have with it.
>
As I said, I'm not an authority, just sounded to me like we were trying
to build something very generic to solve a very specific case, but I
certainly don't want to invent problems that don't exist.
-Christoffer
On Wed, 2013-10-02 at 13:37 -0700, [email protected] wrote:
> On Wed, Oct 02, 2013 at 11:43:30AM -0700, Christoffer Dall wrote:
> > > What's wrong with a non-vfio-specific flag that a driver can set, that
> > > indicates that the driver is willing to try to bind to any device on the
> > > bus if explicitly requested via the existing sysfs bind mechanism?
> > >
> > It sounds more hackish to me to invent some 'generic' flag to solve a
> > very specific case. What you're suggesting would let users specify that
> > a serial driver should handle a NIC hardware, no? That sounds much much
> > worse to me.
>
> You can do that today, with any PCI driver (or USB driver as well), just
> use the bind/unbind files in sysfs and you had better "know" what you
> are doing...
sysfs bind won't work if it driver_match_device() fails. PCI has
PCI_ANY_ID, so the missing piece for PCI is a way to say that the driver
should not bind to a device except when explicitly requested via sysfs
bind.
I don't see any equivalent functionality to PCI_ANY_ID for platform
devices.
-Scott
On Wed, Oct 02, 2013 at 04:08:41PM -0500, Scott Wood wrote:
> On Wed, 2013-10-02 at 13:37 -0700, [email protected] wrote:
> > On Wed, Oct 02, 2013 at 11:43:30AM -0700, Christoffer Dall wrote:
> > > > What's wrong with a non-vfio-specific flag that a driver can set, that
> > > > indicates that the driver is willing to try to bind to any device on the
> > > > bus if explicitly requested via the existing sysfs bind mechanism?
> > > >
> > > It sounds more hackish to me to invent some 'generic' flag to solve a
> > > very specific case. What you're suggesting would let users specify that
> > > a serial driver should handle a NIC hardware, no? That sounds much much
> > > worse to me.
> >
> > You can do that today, with any PCI driver (or USB driver as well), just
> > use the bind/unbind files in sysfs and you had better "know" what you
> > are doing...
>
> sysfs bind won't work if it driver_match_device() fails. PCI has
> PCI_ANY_ID, so the missing piece for PCI is a way to say that the driver
> should not bind to a device except when explicitly requested via sysfs
> bind.
>
> I don't see any equivalent functionality to PCI_ANY_ID for platform
> devices.
Nor should it. If you are wanting to bind platform devices to different
things based on "ids" or "strings" or something else, then you had
better not be using a platform device because that is not what you have
anymore.
Yes, I know the OF stuff uses platform devices, and again, it's one
reason why I don't like it at all. So fix OF devices "properly",
creating your own bus and device type, and then you will not have these
issues.
thanks,
greg "I should never have let platform devices be created" k-h
On Wed, 2013-10-02 at 14:16 -0700, [email protected] wrote:
> On Wed, Oct 02, 2013 at 04:08:41PM -0500, Scott Wood wrote:
> > On Wed, 2013-10-02 at 13:37 -0700, [email protected] wrote:
> > > On Wed, Oct 02, 2013 at 11:43:30AM -0700, Christoffer Dall wrote:
> > > > > What's wrong with a non-vfio-specific flag that a driver can set, that
> > > > > indicates that the driver is willing to try to bind to any device on the
> > > > > bus if explicitly requested via the existing sysfs bind mechanism?
> > > > >
> > > > It sounds more hackish to me to invent some 'generic' flag to solve a
> > > > very specific case. What you're suggesting would let users specify that
> > > > a serial driver should handle a NIC hardware, no? That sounds much much
> > > > worse to me.
> > >
> > > You can do that today, with any PCI driver (or USB driver as well), just
> > > use the bind/unbind files in sysfs and you had better "know" what you
> > > are doing...
> >
> > sysfs bind won't work if it driver_match_device() fails. PCI has
> > PCI_ANY_ID, so the missing piece for PCI is a way to say that the driver
> > should not bind to a device except when explicitly requested via sysfs
> > bind.
> >
> > I don't see any equivalent functionality to PCI_ANY_ID for platform
> > devices.
>
> Nor should it. If you are wanting to bind platform devices to different
> things based on "ids" or "strings" or something else, then you had
> better not be using a platform device because that is not what you have
> anymore.
I don't see how anything could be considered a platform device under
your definition. Even before all the device tree stuff came along,
platform devices were still bound based on strings.
> Yes, I know the OF stuff uses platform devices, and again, it's one
> reason why I don't like it at all. So fix OF devices "properly",
> creating your own bus and device type, and then you will not have these
> issues.
That's what we used to have... It was merged with platform bus because
so many devices may be probed multiple different ways (device tree,
platform data, ACPI, etc). OF is not a bus. A platform device
discovered from OF is still a platform device, just as an i2c device
discovered from OF is still an i2c device.
If you don't like devices that don't sit on some formalized bus and can
be described in more than one way, fine, but that won't make them go
away.
And even if we did still have a separate OF platform bus, my point about
there not being a wildcard match applies to of_device_id as well. It
certainly is not the case that "this is already there, and has been for
years with no problems".
> greg "I should never have let platform devices be created" k-h
The alternative is what? A bunch of duplicated code, with a different
bus type for every SoC family, just so you can put a name on it?
-Scott
On Wed, Oct 02, 2013 at 04:35:15PM -0500, Scott Wood wrote:
> On Wed, 2013-10-02 at 14:16 -0700, [email protected] wrote:
> > On Wed, Oct 02, 2013 at 04:08:41PM -0500, Scott Wood wrote:
> > > On Wed, 2013-10-02 at 13:37 -0700, [email protected] wrote:
> > > > On Wed, Oct 02, 2013 at 11:43:30AM -0700, Christoffer Dall wrote:
> > > > > > What's wrong with a non-vfio-specific flag that a driver can set, that
> > > > > > indicates that the driver is willing to try to bind to any device on the
> > > > > > bus if explicitly requested via the existing sysfs bind mechanism?
> > > > > >
> > > > > It sounds more hackish to me to invent some 'generic' flag to solve a
> > > > > very specific case. What you're suggesting would let users specify that
> > > > > a serial driver should handle a NIC hardware, no? That sounds much much
> > > > > worse to me.
> > > >
> > > > You can do that today, with any PCI driver (or USB driver as well), just
> > > > use the bind/unbind files in sysfs and you had better "know" what you
> > > > are doing...
> > >
> > > sysfs bind won't work if it driver_match_device() fails. PCI has
> > > PCI_ANY_ID, so the missing piece for PCI is a way to say that the driver
> > > should not bind to a device except when explicitly requested via sysfs
> > > bind.
> > >
> > > I don't see any equivalent functionality to PCI_ANY_ID for platform
> > > devices.
> >
> > Nor should it. If you are wanting to bind platform devices to different
> > things based on "ids" or "strings" or something else, then you had
> > better not be using a platform device because that is not what you have
> > anymore.
>
> I don't see how anything could be considered a platform device under
> your definition.
Devices that you just "know" are at a specific memory location ahead of
time.
> Even before all the device tree stuff came along,
> platform devices were still bound based on strings.
Not all of them, there are lots that are not, look at ISA devices on a
PC platform for one example (your pc speaker, keyboard controller,
etc.)
> > Yes, I know the OF stuff uses platform devices, and again, it's one
> > reason why I don't like it at all. So fix OF devices "properly",
> > creating your own bus and device type, and then you will not have these
> > issues.
>
> That's what we used to have... It was merged with platform bus because
> so many devices may be probed multiple different ways (device tree,
> platform data, ACPI, etc).
And I still say that was a mistake I should have never let happen. I
think you should handle binding devices to multiple busses in the driver
code for the different drivers, like we already do today for lots of
different devices (USB host controllers being one example.)
> OF is not a bus.
It's a way to describe the device tree to the kernel, and as such, it's
a "bus" as far as the driver model is concerned. Lots of things are
"busses" for the driver core that you wouldn't think of as a "bus".
A better way to think of busses in the driver core is as a "subsystem".
In fact, we want to change busses to "subsystem" one of these days, udev
has supported that for over 5 years now for when we eventually get
around to it...
> A platform device discovered from OF is still a platform device, just
> as an i2c device discovered from OF is still an i2c device.
Devices don't change what they are just because of what "subsystem" they
are created from. Again, look at USB host controllers as an example of
that.
> If you don't like devices that don't sit on some formalized bus and can
> be described in more than one way, fine, but that won't make them go
> away.
Think of "subsystem" instead, that should make more sense.
> And even if we did still have a separate OF platform bus, my point about
> there not being a wildcard match applies to of_device_id as well. It
> certainly is not the case that "this is already there, and has been for
> years with no problems".
That's an OF problem then, feel free to fix it there, but not in the
driver core with a crazy "ignore this bus type string" hack :)
> > greg "I should never have let platform devices be created" k-h
>
> The alternative is what? A bunch of duplicated code, with a different
> bus type for every SoC family, just so you can put a name on it?
The amount of duplicated code should be really small. If it's too
large, let me know and I'll make driver core helpers for it.
thanks,
greg k-h
On Wed, 2013-10-02 at 16:40 -0700, [email protected] wrote:
> On Wed, Oct 02, 2013 at 04:35:15PM -0500, Scott Wood wrote:
> > On Wed, 2013-10-02 at 14:16 -0700, [email protected] wrote:
> > > On Wed, Oct 02, 2013 at 04:08:41PM -0500, Scott Wood wrote:
> > > > I don't see any equivalent functionality to PCI_ANY_ID for platform
> > > > devices.
> > >
> > > Nor should it. If you are wanting to bind platform devices to different
> > > things based on "ids" or "strings" or something else, then you had
> > > better not be using a platform device because that is not what you have
> > > anymore.
> >
> > I don't see how anything could be considered a platform device under
> > your definition.
>
> Devices that you just "know" are at a specific memory location ahead of
> time.
>
> > Even before all the device tree stuff came along,
> > platform devices were still bound based on strings.
>
> Not all of them, there are lots that are not, look at ISA devices on a
> PC platform for one example (your pc speaker, keyboard controller,
> etc.)
Using platform devices to let board files provide this information to
driver files was a big improvement over hacking up the drivers directly
to know about all sorts of different boards/SoCs. Once you split that
knowledge into a different place you need a way of matching the two.
BTW, this is done with the pc speaker as far as I can tell --
arch/x86/kernel/pcspeaker.c registers a device using the string
"pcspkr" (as do some non-PC platforms).
> > And even if we did still have a separate OF platform bus, my point about
> > there not being a wildcard match applies to of_device_id as well. It
> > certainly is not the case that "this is already there, and has been for
> > years with no problems".
>
> That's an OF problem then, feel free to fix it there, but not in the
> driver core with a crazy "ignore this bus type string" hack :)
Even if we did this for OF and ACPI, you'd still have a problem if you
want to use VFIO with a platform device that only has plain old
platform data; thus, the platform bus (not driver core) seems to be the
appropriate place for a wildcard match if we end up needing it. It may
be moot though, since for platform devices we may want explicit kernel
support for a device even with vfio, in order to reset/quiesce it
between users.
What it looks like we do still want from the driver core is the ability
for a driver to say that it should not be bound to a device except via
explicit sysfs bind, and the ability for a user to say that a device
should not be bound to a driver except via explicit sysfs bind. This is
a separate issue from making driver_match_device() happy (in some
earlier e-mails in the thread these two issues were not properly
separated).
-Scott
On Thu, Oct 03, 2013 at 01:33:27PM -0500, Scott Wood wrote:
> What it looks like we do still want from the driver core is the ability
> for a driver to say that it should not be bound to a device except via
> explicit sysfs bind,
You can do that today by not providing any device ids in your driver
structure, relying on the dynamic ids the driver core creates.
> and the ability for a user to say that a device should not be bound to
> a driver except via explicit sysfs bind.
That's not going to happen, as how can the kernel know a specific device
is going to want this, before it asks the drivers about it?
Or, just don't ever create a driver that matches that device, then rely
on userspace to do the binding explicitly.
Either way, no driver core changes are needed from what I can tell.
greg k-h
On Thu, 2013-10-03 at 11:54 -0700, [email protected] wrote:
> On Thu, Oct 03, 2013 at 01:33:27PM -0500, Scott Wood wrote:
> > What it looks like we do still want from the driver core is the ability
> > for a driver to say that it should not be bound to a device except via
> > explicit sysfs bind,
>
> You can do that today by not providing any device ids in your driver
> structure, relying on the dynamic ids the driver core creates.
I'm not sure what you mean by dynamic ids, but if the driver doesn't
provide any match data, then driver_match_device() will return 0 and
bind_store() will fail.
> > and the ability for a user to say that a device should not be bound to
> > a driver except via explicit sysfs bind.
>
> That's not going to happen, as how can the kernel know a specific device
> is going to want this, before it asks the drivers about it?
This would normally be set by the user prior to unbinding from a
different driver, though it would also be nice to be able to set it at
boot time (e.g. via the kernel command line).
> Or, just don't ever create a driver that matches that device, then rely
> on userspace to do the binding explicitly.
That breaks the normal use case where you want the device to be bound to
the normal driver without doing anything special. And again,
driver_match_device() will cause the bind to fail.
-Scott
On Thu, Oct 03, 2013 at 02:11:34PM -0500, Scott Wood wrote:
> On Thu, 2013-10-03 at 11:54 -0700, [email protected] wrote:
> > On Thu, Oct 03, 2013 at 01:33:27PM -0500, Scott Wood wrote:
> > > What it looks like we do still want from the driver core is the ability
> > > for a driver to say that it should not be bound to a device except via
> > > explicit sysfs bind,
> >
> > You can do that today by not providing any device ids in your driver
> > structure, relying on the dynamic ids the driver core creates.
>
> I'm not sure what you mean by dynamic ids,
The "new_id" file in sysfs for a driver.
> but if the driver doesn't provide any match data, then
> driver_match_device() will return 0 and bind_store() will fail.
bind/unbind in sysfs can override this, I think, maybe it needs to be
combined with the new_id file to work properly, it's been a long time
since I last looked at that code path.
> > > and the ability for a user to say that a device should not be bound to
> > > a driver except via explicit sysfs bind.
> >
> > That's not going to happen, as how can the kernel know a specific device
> > is going to want this, before it asks the drivers about it?
>
> This would normally be set by the user prior to unbinding from a
> different driver, though it would also be nice to be able to set it at
> boot time (e.g. via the kernel command line).
Do that for your driver then, if you really want this, but it's not
going into the driver core, sorry. It should never be parsing kernel
command lines, nor should you.
> > Or, just don't ever create a driver that matches that device, then rely
> > on userspace to do the binding explicitly.
>
> That breaks the normal use case where you want the device to be bound to
> the normal driver without doing anything special. And again,
> driver_match_device() will cause the bind to fail.
So, you are asking for something that really is impossible from what I
can tell. Please figure out _exactly_ the semantics of what you want,
as I'm totally confused and am giving up on this thread without seeing a
patch anywhere.
greg "back to patch review" k-h
Have been thinking about this issue some more. As Scott mentioned,
'wildcard' matching for a driver can be fairly done in the platform
bus driver. We could add a new flag to the platform driver struct:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4f8bef3..4d6cf14 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -727,6 +727,10 @@ static int platform_match(struct device *dev, struct device_driver *drv)
struct platform_device *pdev = to_platform_device(dev);
struct platform_driver *pdrv = to_platform_driver(drv);
+ /* the driver matches any device */
+ if (pdrv->match_any)
+ return 1;
+
/* Attempt an OF style match first */
if (of_driver_match_device(dev, drv))
return 1;
However, the more problematic issue is that a bus driver has no way to
differentiate from an explicit bind request via sysfs and a bind that
happened through bus probing.
I think something like the new flag in the snippet below would enable the platform
bus to support platform drivers that only bind on explicit request:
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 4c289ab..daf6d24 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -201,7 +201,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
int err = -ENODEV;
dev = bus_find_device_by_name(bus, NULL, buf);
- if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
+ if (dev && dev->driver == NULL && driver_match_device(drv, dev, 1)) {
if (dev->parent) /* Needed for USB */
device_lock(dev->parent);
device_lock(dev);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 35fa368..bb53785 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -389,7 +389,7 @@ static int __device_attach(struct device_driver *drv, void *data)
{
struct device *dev = data;
- if (!driver_match_device(drv, dev))
+ if (!driver_match_device(drv, dev, 0))
return 0;
return driver_probe_device(drv, dev);
@@ -450,7 +450,7 @@ static int __driver_attach(struct device *dev, void *data)
* is an error.
*/
- if (!driver_match_device(drv, dev))
+ if (!driver_match_device(drv, dev, 0))
return 0;
if (dev->parent) /* Needed for USB */
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 2cbc677..7a15ef3 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -114,9 +114,9 @@ extern void driver_detach(struct device_driver *drv);
extern int driver_probe_device(struct device_driver *drv, struct device *dev);
extern void driver_deferred_probe_del(struct device *dev);
static inline int driver_match_device(struct device_driver *drv,
- struct device *dev)
+ struct device *dev, int explicit_bind)
{
- return drv->bus->match ? drv->bus->match(dev, drv) : 1;
+ return drv->bus->match ? drv->bus->match(dev, drv, explicit_bind) : 1;
}
Of, course the above change would need to be propagated to the different
bus drivers that implement the 'match' function.
Regards,
Stuart
On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> Have been thinking about this issue some more. As Scott mentioned,
> 'wildcard' matching for a driver can be fairly done in the platform
> bus driver. We could add a new flag to the platform driver struct:
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4f8bef3..4d6cf14 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -727,6 +727,10 @@ static int platform_match(struct device *dev, struct device_driver *drv)
> struct platform_device *pdev = to_platform_device(dev);
> struct platform_driver *pdrv = to_platform_driver(drv);
>
> + /* the driver matches any device */
> + if (pdrv->match_any)
> + return 1;
> +
> /* Attempt an OF style match first */
> if (of_driver_match_device(dev, drv))
> return 1;
>
> However, the more problematic issue is that a bus driver has no way to
> differentiate from an explicit bind request via sysfs and a bind that
> happened through bus probing.
Again, I think the wildcard match should be orthogonal to "don't bind by
default" as far as the mechanism goes.
There's already a "bool suppress_bind_attrs" to prevent sysfs
bind/unbind. I suggested a similar flag to mean the oppsosite -- bind
*only* through sysfs. Greg KH was skeptical and wanted to see a patch
before any further discussion.
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 2cbc677..7a15ef3 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -114,9 +114,9 @@ extern void driver_detach(struct device_driver *drv);
> extern int driver_probe_device(struct device_driver *drv, struct device *dev);
> extern void driver_deferred_probe_del(struct device *dev);
> static inline int driver_match_device(struct device_driver *drv,
> - struct device *dev)
> + struct device *dev, int explicit_bind)
> {
> - return drv->bus->match ? drv->bus->match(dev, drv) : 1;
> + return drv->bus->match ? drv->bus->match(dev, drv, explicit_bind) : 1;
> }
>
> Of, course the above change would need to be propagated to the different
> bus drivers that implement the 'match' function.
...which would not be a problem with my approach, because you could
handle it in the callers of driver_match_device().
-Scott
On Wed, Oct 09, 2013 at 07:02:25PM +0000, Yoder Stuart-B08248 wrote:
> Have been thinking about this issue some more. As Scott mentioned,
> 'wildcard' matching for a driver can be fairly done in the platform
> bus driver. We could add a new flag to the platform driver struct:
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4f8bef3..4d6cf14 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -727,6 +727,10 @@ static int platform_match(struct device *dev, struct device_driver *drv)
> struct platform_device *pdev = to_platform_device(dev);
> struct platform_driver *pdrv = to_platform_driver(drv);
>
> + /* the driver matches any device */
> + if (pdrv->match_any)
> + return 1;
> +
> /* Attempt an OF style match first */
> if (of_driver_match_device(dev, drv))
> return 1;
>
> However, the more problematic issue is that a bus driver has no way to
> differentiate from an explicit bind request via sysfs and a bind that
> happened through bus probing.
That was by design, nice to see I implemented it properly :)
> I think something like the new flag in the snippet below would enable the platform
> bus to support platform drivers that only bind on explicit request:
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 4c289ab..daf6d24 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -201,7 +201,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> int err = -ENODEV;
>
> dev = bus_find_device_by_name(bus, NULL, buf);
> - if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
> + if (dev && dev->driver == NULL && driver_match_device(drv, dev, 1)) {
Magic flags are the spawn of your favorite anti-$DIETY. I'm never going
to accept that, sorry.
If you really want to do something "special" for the platform bus, then
do it only for the platform bus. But even then, you'll find me arguing
that you really don't want to do it at all, sorry.
I'm still yet to be convinced this is even an issue at all, but maybe
that's just the jetlag talking...
greg k-h
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, October 09, 2013 2:22 PM
> To: Yoder Stuart-B08248
> Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex Williamson;
> [email protected]; [email protected];
> [email protected]; Sethi Varun-B16395; Bhushan Bharat-R65777;
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> device
>
> On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> > Have been thinking about this issue some more. As Scott mentioned,
> > 'wildcard' matching for a driver can be fairly done in the platform
> > bus driver. We could add a new flag to the platform driver struct:
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 4f8bef3..4d6cf14 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -727,6 +727,10 @@ static int platform_match(struct device *dev,
> struct device_driver *drv)
> > struct platform_device *pdev = to_platform_device(dev);
> > struct platform_driver *pdrv = to_platform_driver(drv);
> >
> > + /* the driver matches any device */
> > + if (pdrv->match_any)
> > + return 1;
> > +
> > /* Attempt an OF style match first */
> > if (of_driver_match_device(dev, drv))
> > return 1;
> >
> > However, the more problematic issue is that a bus driver has no way to
> > differentiate from an explicit bind request via sysfs and a bind that
> > happened through bus probing.
>
> Again, I think the wildcard match should be orthogonal to "don't bind by
> default" as far as the mechanism goes.
>
> There's already a "bool suppress_bind_attrs" to prevent sysfs
> bind/unbind. I suggested a similar flag to mean the oppsosite -- bind
> *only* through sysfs. Greg KH was skeptical and wanted to see a patch
> before any further discussion.
Ah, think I understand now...yes that works as well, and would be
less intrustive. So are you writing a patch? :)
It would be something like this, right?
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 35fa368..c9a61ea 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -389,7 +389,7 @@ static int __device_attach(struct device_driver *drv, void *data)
{
struct device *dev = data;
- if (!driver_match_device(drv, dev))
+ if (!drv->explicit_bind_only && !driver_match_device(drv, dev))
return 0;
return driver_probe_device(drv, dev);
@@ -450,7 +450,7 @@ static int __driver_attach(struct device *dev, void *data)
* is an error.
*/
- if (!driver_match_device(drv, dev))
+ if (!drv->explicit_bind_only && !driver_match_device(drv, dev))
return 0;
if (dev->parent) /* Needed for USB */
diff --git a/include/linux/device.h b/include/linux/device.h
index 2a9d6ed..f2be980 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -236,6 +236,7 @@ struct device_driver {
const char *mod_name; /* used for built-in modules */
bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
+ bool explicit_bind_only; /* enables bind/unbind via sysfs only */
const struct of_device_id *of_match_table;
const struct acpi_device_id *acpi_match_table;
Stuart
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, 2013-10-09 at 12:16 -0700, [email protected] wrote:
> On Wed, Oct 09, 2013 at 07:02:25PM +0000, Yoder Stuart-B08248 wrote:
> > Have been thinking about this issue some more. As Scott mentioned,
> > 'wildcard' matching for a driver can be fairly done in the platform
> > bus driver. We could add a new flag to the platform driver struct:
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 4f8bef3..4d6cf14 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -727,6 +727,10 @@ static int platform_match(struct device *dev, struct device_driver *drv)
> > struct platform_device *pdev = to_platform_device(dev);
> > struct platform_driver *pdrv = to_platform_driver(drv);
> >
> > + /* the driver matches any device */
> > + if (pdrv->match_any)
> > + return 1;
> > +
> > /* Attempt an OF style match first */
> > if (of_driver_match_device(dev, drv))
> > return 1;
> >
> > However, the more problematic issue is that a bus driver has no way to
> > differentiate from an explicit bind request via sysfs and a bind that
> > happened through bus probing.
>
> That was by design, nice to see I implemented it properly :)
>
> > I think something like the new flag in the snippet below would enable the platform
> > bus to support platform drivers that only bind on explicit request:
> >
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 4c289ab..daf6d24 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -201,7 +201,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > int err = -ENODEV;
> >
> > dev = bus_find_device_by_name(bus, NULL, buf);
> > - if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
> > + if (dev && dev->driver == NULL && driver_match_device(drv, dev, 1)) {
>
> Magic flags are the spawn of your favorite anti-$DIETY. I'm never going
> to accept that, sorry.
>
> If you really want to do something "special" for the platform bus, then
> do it only for the platform bus. But even then, you'll find me arguing
> that you really don't want to do it at all, sorry.
It's not (or shouldn't be) special for the platform bus. The "don't
bind by default" flag (note that I am *not* referring to the above code
snippet) would be useful for VFIO PCI as well, as it would replace the
hacky and racy usage of new_id.
> I'm still yet to be convinced this is even an issue at all, but maybe
> that's just the jetlag talking...
If this is because you think we should use new_id, we just had a
discussion in this thread about why that's no good.
-Scott
On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, October 09, 2013 2:22 PM
> > To: Yoder Stuart-B08248
> > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex Williamson;
> > [email protected]; [email protected];
> > [email protected]; Sethi Varun-B16395; Bhushan Bharat-R65777;
> > [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> > device
> >
> > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> > > Have been thinking about this issue some more. As Scott mentioned,
> > > 'wildcard' matching for a driver can be fairly done in the platform
> > > bus driver. We could add a new flag to the platform driver struct:
> > >
> > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > index 4f8bef3..4d6cf14 100644
> > > --- a/drivers/base/platform.c
> > > +++ b/drivers/base/platform.c
> > > @@ -727,6 +727,10 @@ static int platform_match(struct device *dev,
> > struct device_driver *drv)
> > > struct platform_device *pdev = to_platform_device(dev);
> > > struct platform_driver *pdrv = to_platform_driver(drv);
> > >
> > > + /* the driver matches any device */
> > > + if (pdrv->match_any)
> > > + return 1;
> > > +
> > > /* Attempt an OF style match first */
> > > if (of_driver_match_device(dev, drv))
> > > return 1;
> > >
> > > However, the more problematic issue is that a bus driver has no way to
> > > differentiate from an explicit bind request via sysfs and a bind that
> > > happened through bus probing.
> >
> > Again, I think the wildcard match should be orthogonal to "don't bind by
> > default" as far as the mechanism goes.
> >
> > There's already a "bool suppress_bind_attrs" to prevent sysfs
> > bind/unbind. I suggested a similar flag to mean the oppsosite -- bind
> > *only* through sysfs. Greg KH was skeptical and wanted to see a patch
> > before any further discussion.
>
> Ah, think I understand now...yes that works as well, and would be
> less intrustive. So are you writing a patch? :)
I've been meaning to since the previous round of discussion, but I've
been busy. Would someone else be able to test it in the context of
using it for VFIO?
> It would be something like this, right?
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 35fa368..c9a61ea 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -389,7 +389,7 @@ static int __device_attach(struct device_driver *drv, void *data)
> {
> struct device *dev = data;
>
> - if (!driver_match_device(drv, dev))
> + if (!drv->explicit_bind_only && !driver_match_device(drv, dev))
> return 0;
if (drv->explicit_bind_only || !driver_match_device(drv, dev))
return 0;
> return driver_probe_device(drv, dev);
> @@ -450,7 +450,7 @@ static int __driver_attach(struct device *dev, void *data)
> * is an error.
> */
>
> - if (!driver_match_device(drv, dev))
> + if (!drv->explicit_bind_only && !driver_match_device(drv, dev))
> return 0;
Likewise -- or error out earlier in driver_attach().
Otherwise, that looks about right, for the driver side (though
driver_attach could error out earlier rather than testing it inside the
loop).
The other half of fixing the raciness is to ensure that the device
doesn't get bound back to a non-VFIO driver (e.g. due to a module load
or new_id). The solution I proposed for that was a similar
explicit-bind-only flag for a device, that the user sets through sysfs
prior to unbinding. This would also be useful in non-VFIO contexts to
simply say "I don't want to use this device at all".
-Scott
On Wed, 9 Oct 2013 15:03:19 -0500
Scott Wood <[email protected]> wrote:
> On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, October 09, 2013 2:22 PM
> > >
> > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> > > > Have been thinking about this issue some more. As Scott mentioned,
thanks for bringing this up again.
> > > There's already a "bool suppress_bind_attrs" to prevent sysfs
> > > bind/unbind. I suggested a similar flag to mean the oppsosite -- bind
> > > *only* through sysfs. Greg KH was skeptical and wanted to see a patch
> > > before any further discussion.
> >
> > Ah, think I understand now...yes that works as well, and would be
> > less intrustive. So are you writing a patch? :)
>
> I've been meaning to since the previous round of discussion, but I've
> been busy. Would someone else be able to test it in the context of
> using it for VFIO?
yes - see below.
> Otherwise, that looks about right, for the driver side (though
> driver_attach could error out earlier rather than testing it inside the
> loop).
I've made the changes you suggested and tested the resulting diff below
on an arndale board. I successfully performed the following sequence of
commands after first changing the i2c@12C80000 node in the device tree
to be exclusively compatible with "vfio":
===
# ls -l /sys/bus/platform/drivers/vfio-platform/
total 0
--w------- 1 root root 4096 Sep 24 19:17 bind
--w------- 1 root root 4096 Sep 24 19:13 uevent
--w------- 1 root root 4096 Sep 24 19:18 unbind
# ls -l /sys/bus/platform/drivers/s3c-i2c
total 0
lrwxrwxrwx 1 root root 0 Sep 24 19:11 12c60000.i2c -> ../../../../devices/12c60000.i2c
lrwxrwxrwx 1 root root 0 Sep 24 19:11 12c90000.i2c -> ../../../../devices/12c90000.i2c
lrwxrwxrwx 1 root root 0 Sep 24 19:20 12ce0000.i2c -> ../../../../devices/12ce0000.i2c
--w------- 1 root root 4096 Sep 24 19:18 bind
--w------- 1 root root 4096 Sep 24 19:11 uevent
--w------- 1 root root 4096 Sep 24 19:17 unbind
# ls -l /sys/devices/12c80000.i2c/driver # this is the one with the 'vfio' compatible
ls: cannot access /sys/devices/12c80000.i2c/driver: No such file or directory
# ls -l /sys/devices/12ce0000.i2c/driver
lrwxrwxrwx 1 root root 0 Sep 24 19:18 /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/s3c-i2c
# echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/unbind
# ls -l /sys/devices/12ce0000.i2c/driver
ls: cannot access /sys/devices/12ce0000.i2c/driver: No such file or directory
# echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
# ls -l /sys/devices/12ce0000.i2c/driver
lrwxrwxrwx 1 root root 0 Sep 24 19:21 /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/vfio-platform
# echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/unbind
# ls -l /sys/devices/12ce0000.i2c/driver
# echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/bind
[ 722.137524] s3c-i2c 12ce0000.i2c: slave address 0x38
[ 722.141037] s3c-i2c 12ce0000.i2c: bus frequency set to 65 KHz
[ 722.150605] s3c-i2c 12ce0000.i2c: i2c-8: S3C I2C adapter
# ls -l /sys/devices/12ce0000.i2c/driver
lrwxrwxrwx 1 root root 0 Sep 24 19:21 /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/s3c-i2c
#
====
so it's correctly not allowing 'vfio' driver to bind to a device tree
compatible it's declared, and it then can bind the i2c @ 12ce0000 device
to the vfio-platform driver, and unbind and bind it back to the i2c
driver.
For clarity's sake, before this diff, the command:
echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
would error with:
echo: write error: No such device
> The other half of fixing the raciness is to ensure that the device
> doesn't get bound back to a non-VFIO driver (e.g. due to a module load
> or new_id). The solution I proposed for that was a similar
> explicit-bind-only flag for a device, that the user sets through sysfs
> prior to unbinding. This would also be useful in non-VFIO contexts to
> simply say "I don't want to use this device at all".
I can take a look at doing this if you're still busy.
Thanks,
Kim
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 73f6c29..da81442 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -201,7 +201,8 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
int err = -ENODEV;
dev = bus_find_device_by_name(bus, NULL, buf);
- if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
+ if (dev && dev->driver == NULL && (drv->sysfs_bind_only ||
+ driver_match_device(drv, dev))) {
if (dev->parent) /* Needed for USB */
device_lock(dev->parent);
device_lock(dev);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 35fa368..6f85279 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -389,7 +389,7 @@ static int __device_attach(struct device_driver *drv, void *data)
{
struct device *dev = data;
- if (!driver_match_device(drv, dev))
+ if (drv->sysfs_bind_only || !driver_match_device(drv, dev))
return 0;
return driver_probe_device(drv, dev);
@@ -476,6 +476,9 @@ static int __driver_attach(struct device *dev, void *data)
*/
int driver_attach(struct device_driver *drv)
{
+ if (drv->sysfs_bind_only)
+ return 0;
+
return bus_for_each_dev(drv->bus, NULL, drv, __driver_attach);
}
EXPORT_SYMBOL_GPL(driver_attach);
diff --git a/drivers/vfio/vfio_platform.c b/drivers/vfio/vfio_platform.c
index b92d7bb..ba578b2 100644
--- a/drivers/vfio/vfio_platform.c
+++ b/drivers/vfio/vfio_platform.c
@@ -297,6 +297,7 @@ static struct platform_driver vfio_platform_driver = {
.name = "vfio-platform",
.owner = THIS_MODULE,
.of_match_table = vfio_platform_match,
+ .sysfs_bind_only = true,
},
};
diff --git a/include/linux/device.h b/include/linux/device.h
index 94638ef..a3ae81e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -199,6 +199,7 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus);
* @owner: The module owner.
* @mod_name: Used for built-in modules.
* @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @sysfs_bind_only: Only allow bind/unbind via sysfs.
* @of_match_table: The open firmware table.
* @acpi_match_table: The ACPI match table.
* @probe: Called to query the existence of a specific device,
@@ -232,6 +233,7 @@ struct device_driver {
const char *mod_name; /* used for built-in modules */
bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
+ bool sysfs_bind_only; /* only allow bind/unbind via sysfs */
const struct of_device_id *of_match_table;
const struct acpi_device_id *acpi_match_table;
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, October 10, 2013 1:33 AM
> To: Yoder Stuart-B08248
> Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex Williamson; linux-
> [email protected]; [email protected]; [email protected]; Sethi
> Varun-B16395; Bhushan Bharat-R65777; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device
>
> On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, October 09, 2013 2:22 PM
> > > To: Yoder Stuart-B08248
> > > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex
> > > Williamson; [email protected];
> > > [email protected]; [email protected]; Sethi Varun-B16395;
> > > Bhushan Bharat-R65777; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a
> > > platform device
> > >
> > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> > > > Have been thinking about this issue some more. As Scott
> > > > mentioned, 'wildcard' matching for a driver can be fairly done in
> > > > the platform bus driver. We could add a new flag to the platform driver
> struct:
> > > >
> > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > > index 4f8bef3..4d6cf14 100644
> > > > --- a/drivers/base/platform.c
> > > > +++ b/drivers/base/platform.c
> > > > @@ -727,6 +727,10 @@ static int platform_match(struct device *dev,
> > > struct device_driver *drv)
> > > > struct platform_device *pdev = to_platform_device(dev);
> > > > struct platform_driver *pdrv = to_platform_driver(drv);
> > > >
> > > > + /* the driver matches any device */
> > > > + if (pdrv->match_any)
> > > > + return 1;
> > > > +
> > > > /* Attempt an OF style match first */
> > > > if (of_driver_match_device(dev, drv))
> > > > return 1;
> > > >
> > > > However, the more problematic issue is that a bus driver has no
> > > > way to differentiate from an explicit bind request via sysfs and a
> > > > bind that happened through bus probing.
> > >
> > > Again, I think the wildcard match should be orthogonal to "don't
> > > bind by default" as far as the mechanism goes.
> > >
> > > There's already a "bool suppress_bind_attrs" to prevent sysfs
> > > bind/unbind. I suggested a similar flag to mean the oppsosite --
> > > bind
> > > *only* through sysfs. Greg KH was skeptical and wanted to see a
> > > patch before any further discussion.
> >
> > Ah, think I understand now...yes that works as well, and would be
> > less intrustive. So are you writing a patch? :)
>
> I've been meaning to since the previous round of discussion, but I've been busy.
> Would someone else be able to test it in the context of using it for VFIO?
I wish I could have but I do not have vfio-platform stuff.
>
> > It would be something like this, right?
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index
> > 35fa368..c9a61ea 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -389,7 +389,7 @@ static int __device_attach(struct device_driver
> > *drv, void *data) {
> > struct device *dev = data;
> >
> > - if (!driver_match_device(drv, dev))
> > + if (!drv->explicit_bind_only && !driver_match_device(drv,
> > + dev))
> > return 0;
>
> if (drv->explicit_bind_only || !driver_match_device(drv, dev))
> return 0;
Scott,
I am trying to understand what you are proposing here (example "DEVICE" can be handled by "DRIVER1" and "VFIO-PLATFORM-DRIVER"):
- By default drv->explicit_bind_only will be clear in all drivers.
- By default device->explicit_bind_only will also be clear for all devices.
- On boot, matching devices will bound to the respective driver (DEVICE >==> DRIVER1).
This will never bound with VFIO-PLATFORM-DRIVER. So far same as before.
- Via Sysfs interface set drv->explicit_bind_only for VFIO-PLATFORM-DRIVER.
- Then for the devices user want, set device->explicit_bind_only.
- unbind DEVICE from DRIVER1
- bind DEVICE with VFIO-PLATFORM-DRIVER. This time it will be successful because (device->explicit_bind_only && drv->explicit_bind_only) is set.
- Now when done, unbind the DEVICE from VFIO-PLATFORM-DRIVER.
- Now user can re-bind the device with either DRIVER1 or VFIO-PLATFORM-DRIVER.
- Now once drv->explicit_bind_only is set in VFIO-PLATFORM-DRIVER, and a new device comes (device - hotplug) then can gets bound to matching drive and not with VFIO-PLATFORM-DRIVER.
This looks ok to me :)
Thanks
-Bharat
>
> > return driver_probe_device(drv, dev); @@ -450,7 +450,7 @@
> > static int __driver_attach(struct device *dev, void *data)
> > * is an error.
> > */
> >
> > - if (!driver_match_device(drv, dev))
> > + if (!drv->explicit_bind_only && !driver_match_device(drv,
> > + dev))
> > return 0;
>
> Likewise -- or error out earlier in driver_attach().
>
> Otherwise, that looks about right, for the driver side (though driver_attach
> could error out earlier rather than testing it inside the loop).
>
> The other half of fixing the raciness is to ensure that the device doesn't get
> bound back to a non-VFIO driver (e.g. due to a module load or new_id). The
> solution I proposed for that was a similar explicit-bind-only flag for a device,
> that the user sets through sysfs prior to unbinding. This would also be useful
> in non-VFIO contexts to simply say "I don't want to use this device at all".
>
> -Scott
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of
> Kim Phillips
> Sent: Thursday, October 10, 2013 8:36 AM
> To: Wood Scott-B07421
> Cc: Yoder Stuart-B08248; Wood Scott-B07421; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Sethi Varun-B16395; Bhushan
> Bharat-R65777; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device
>
> On Wed, 9 Oct 2013 15:03:19 -0500
> Scott Wood <[email protected]> wrote:
>
> > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, October 09, 2013 2:22 PM
> > > >
> > > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> > > > > Have been thinking about this issue some more. As Scott
> > > > > mentioned,
>
> thanks for bringing this up again.
>
> > > > There's already a "bool suppress_bind_attrs" to prevent sysfs
> > > > bind/unbind. I suggested a similar flag to mean the oppsosite --
> > > > bind
> > > > *only* through sysfs. Greg KH was skeptical and wanted to see a
> > > > patch before any further discussion.
> > >
> > > Ah, think I understand now...yes that works as well, and would be
> > > less intrustive. So are you writing a patch? :)
> >
> > I've been meaning to since the previous round of discussion, but I've
> > been busy. Would someone else be able to test it in the context of
> > using it for VFIO?
>
> yes - see below.
>
> > Otherwise, that looks about right, for the driver side (though
> > driver_attach could error out earlier rather than testing it inside
> > the loop).
>
> I've made the changes you suggested and tested the resulting diff below on an
> arndale board. I successfully performed the following sequence of commands
> after first changing the i2c@12C80000 node in the device tree to be exclusively
> compatible with "vfio":
>
> ===
> # ls -l /sys/bus/platform/drivers/vfio-platform/
> total 0
> --w------- 1 root root 4096 Sep 24 19:17 bind
> --w------- 1 root root 4096 Sep 24 19:13 uevent
> --w------- 1 root root 4096 Sep 24 19:18 unbind # ls -l
> /sys/bus/platform/drivers/s3c-i2c total 0
> lrwxrwxrwx 1 root root 0 Sep 24 19:11 12c60000.i2c ->
> ../../../../devices/12c60000.i2c
> lrwxrwxrwx 1 root root 0 Sep 24 19:11 12c90000.i2c ->
> ../../../../devices/12c90000.i2c
> lrwxrwxrwx 1 root root 0 Sep 24 19:20 12ce0000.i2c ->
> ../../../../devices/12ce0000.i2c
> --w------- 1 root root 4096 Sep 24 19:18 bind
> --w------- 1 root root 4096 Sep 24 19:11 uevent
> --w------- 1 root root 4096 Sep 24 19:17 unbind # ls -l
> /sys/devices/12c80000.i2c/driver # this is the one with the 'vfio' compatible
> ls: cannot access /sys/devices/12c80000.i2c/driver: No such file or directory #
> ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:18
> /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/s3c-i2c
> # echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/unbind
> # ls -l /sys/devices/12ce0000.i2c/driver
> ls: cannot access /sys/devices/12ce0000.i2c/driver: No such file or directory #
> echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> # ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:21
> /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/vfio-platform
> # echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/unbind
> # ls -l /sys/devices/12ce0000.i2c/driver # echo 12ce0000.i2c >
> /sys/bus/platform/drivers/s3c-i2c/bind
> [ 722.137524] s3c-i2c 12ce0000.i2c: slave address 0x38 [ 722.141037] s3c-i2c
> 12ce0000.i2c: bus frequency set to 65 KHz [ 722.150605] s3c-i2c 12ce0000.i2c:
> i2c-8: S3C I2C adapter # ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1
> root root 0 Sep 24 19:21 /sys/devices/12ce0000.i2c/driver ->
> ../../bus/platform/drivers/s3c-i2c
> #
> ====
>
> so it's correctly not allowing 'vfio' driver to bind to a device tree compatible
> it's declared, and it then can bind the i2c @ 12ce0000 device to the vfio-
> platform driver, and unbind and bind it back to the i2c driver.
>
> For clarity's sake, before this diff, the command:
>
> echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
>
> would error with:
>
> echo: write error: No such device
>
> > The other half of fixing the raciness is to ensure that the device
> > doesn't get bound back to a non-VFIO driver (e.g. due to a module load
> > or new_id). The solution I proposed for that was a similar
> > explicit-bind-only flag for a device, that the user sets through sysfs
> > prior to unbinding. This would also be useful in non-VFIO contexts to
> > simply say "I don't want to use this device at all".
>
> I can take a look at doing this if you're still busy.
>
> Thanks,
>
> Kim
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 73f6c29..da81442
> 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -201,7 +201,8 @@ static ssize_t bind_store(struct device_driver *drv, const
> char *buf,
> int err = -ENODEV;
>
> dev = bus_find_device_by_name(bus, NULL, buf);
> - if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
> + if (dev && dev->driver == NULL && (drv->sysfs_bind_only ||
> + driver_match_device(drv, dev))) {
Should not we check
if (dev && dev->driver == NULL &&
(device->explicit_bind_only && drv->explicit_bind_only) ||
driver_match_device(drv, dev)))
> if (dev->parent) /* Needed for USB */
> device_lock(dev->parent);
> device_lock(dev);
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 35fa368..6f85279 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -389,7 +389,7 @@ static int __device_attach(struct device_driver *drv, void
> *data) {
> struct device *dev = data;
>
> - if (!driver_match_device(drv, dev))
> + if (drv->sysfs_bind_only || !driver_match_device(drv, dev))
Likewise ..
Thanks
-Bharat
> return 0;
>
> return driver_probe_device(drv, dev);
> @@ -476,6 +476,9 @@ static int __driver_attach(struct device *dev, void *data)
> */
> int driver_attach(struct device_driver *drv) {
> + if (drv->sysfs_bind_only)
> + return 0;
> +
> return bus_for_each_dev(drv->bus, NULL, drv, __driver_attach); }
> EXPORT_SYMBOL_GPL(driver_attach); diff --git a/drivers/vfio/vfio_platform.c
> b/drivers/vfio/vfio_platform.c index b92d7bb..ba578b2 100644
> --- a/drivers/vfio/vfio_platform.c
> +++ b/drivers/vfio/vfio_platform.c
> @@ -297,6 +297,7 @@ static struct platform_driver vfio_platform_driver = {
> .name = "vfio-platform",
> .owner = THIS_MODULE,
> .of_match_table = vfio_platform_match,
> + .sysfs_bind_only = true,
> },
> };
>
> diff --git a/include/linux/device.h b/include/linux/device.h index
> 94638ef..a3ae81e 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -199,6 +199,7 @@ extern struct klist *bus_get_device_klist(struct bus_type
> *bus);
> * @owner: The module owner.
> * @mod_name: Used for built-in modules.
> * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> + * @sysfs_bind_only: Only allow bind/unbind via sysfs.
> * @of_match_table: The open firmware table.
> * @acpi_match_table: The ACPI match table.
> * @probe: Called to query the existence of a specific device,
> @@ -232,6 +233,7 @@ struct device_driver {
> const char *mod_name; /* used for built-in modules */
>
> bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
> + bool sysfs_bind_only; /* only allow bind/unbind via sysfs */
>
> const struct of_device_id *of_match_table;
> const struct acpi_device_id *acpi_match_table;
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a
> message to [email protected] More majordomo info at
> http://vger.kernel.org/majordomo-info.html
> I am trying to understand what you are proposing here (example "DEVICE"
> can be handled by "DRIVER1" and "VFIO-PLATFORM-DRIVER"):
> - By default drv->explicit_bind_only will be clear in all drivers.
> - By default device->explicit_bind_only will also be clear for all
> devices.
> - On boot, matching devices will bound to the respective driver (DEVICE
> >==> DRIVER1).
> This will never bound with VFIO-PLATFORM-DRIVER. So far same as
> before.
> - Via Sysfs interface set drv->explicit_bind_only for VFIO-PLATFORM-
> DRIVER.
No. VFIO-PLATFORM-DRIVER is _always_ explicit_bind_only and thus will be
statically set in the driver. See Kim's patch.
> - Then for the devices user want, set device->explicit_bind_only.
> - unbind DEVICE from DRIVER1
> - bind DEVICE with VFIO-PLATFORM-DRIVER. This time it will be successful
> because (device->explicit_bind_only && drv->explicit_bind_only) is set.
> - Now when done, unbind the DEVICE from VFIO-PLATFORM-DRIVER.
> - Now user can re-bind the device with either DRIVER1 or VFIO-PLATFORM-
> DRIVER.
> - Now once drv->explicit_bind_only is set in VFIO-PLATFORM-DRIVER, and a
> new device comes (device - hotplug) then can gets bound to matching drive
> and not with VFIO-PLATFORM-DRIVER.
Otherwise, it looks correct to me.
Stuart
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, 2013-10-10 at 02:45 -0500, Bhushan Bharat-R65777 wrote:
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, October 10, 2013 1:33 AM
> > To: Yoder Stuart-B08248
> > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex Williamson; linux-
> > [email protected]; [email protected]; [email protected]; Sethi
> > Varun-B16395; Bhushan Bharat-R65777; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device
> >
> > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
> > > Ah, think I understand now...yes that works as well, and would be
> > > less intrustive. So are you writing a patch? :)
> >
> > I've been meaning to since the previous round of discussion, but I've been busy.
> > Would someone else be able to test it in the context of using it for VFIO?
>
> I wish I could have but I do not have vfio-platform stuff.
VFIO PCI without new_id would also be a useful test.
-Scott
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, October 10, 2013 8:53 PM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Yoder Stuart-B08248; Kim Phillips; Christoffer Dall; Alex
> Williamson; [email protected]; [email protected];
> [email protected]; Sethi Varun-B16395; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device
>
> On Thu, 2013-10-10 at 02:45 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Thursday, October 10, 2013 1:33 AM
> > > To: Yoder Stuart-B08248
> > > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex
> > > Williamson; linux- [email protected];
> > > [email protected]; [email protected]; Sethi Varun-B16395;
> > > Bhushan Bharat-R65777; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a
> > > platform device
> > >
> > > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
> > > > Ah, think I understand now...yes that works as well, and would be
> > > > less intrustive. So are you writing a patch? :)
> > >
> > > I've been meaning to since the previous round of discussion, but I've been
> busy.
> > > Would someone else be able to test it in the context of using it for VFIO?
> >
> > I wish I could have but I do not have vfio-platform stuff.
>
> VFIO PCI without new_id would also be a useful test.
I will do that :)
-Bharat
>
> -Scott
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, 2013-10-10 at 03:01 -0500, Bhushan Bharat-R65777 wrote:
>
> > -----Original Message-----
> > From: [email protected] [mailto:[email protected]] On Behalf Of
> > Kim Phillips
> > Sent: Thursday, October 10, 2013 8:36 AM
> > To: Wood Scott-B07421
> > Cc: Yoder Stuart-B08248; Wood Scott-B07421; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Sethi Varun-B16395; Bhushan
> > Bharat-R65777; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device
> >
> > On Wed, 9 Oct 2013 15:03:19 -0500
> > Scott Wood <[email protected]> wrote:
> >
> > > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
> > > > > From: Wood Scott-B07421
> > > > > Sent: Wednesday, October 09, 2013 2:22 PM
> > > > >
> > > > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> > > > > > Have been thinking about this issue some more. As Scott
> > > > > > mentioned,
> >
> > thanks for bringing this up again.
> >
> > > > > There's already a "bool suppress_bind_attrs" to prevent sysfs
> > > > > bind/unbind. I suggested a similar flag to mean the oppsosite --
> > > > > bind
> > > > > *only* through sysfs. Greg KH was skeptical and wanted to see a
> > > > > patch before any further discussion.
> > > >
> > > > Ah, think I understand now...yes that works as well, and would be
> > > > less intrustive. So are you writing a patch? :)
> > >
> > > I've been meaning to since the previous round of discussion, but I've
> > > been busy. Would someone else be able to test it in the context of
> > > using it for VFIO?
> >
> > yes - see below.
> >
> > > Otherwise, that looks about right, for the driver side (though
> > > driver_attach could error out earlier rather than testing it inside
> > > the loop).
> >
> > I've made the changes you suggested and tested the resulting diff below on an
> > arndale board. I successfully performed the following sequence of commands
> > after first changing the i2c@12C80000 node in the device tree to be exclusively
> > compatible with "vfio":
This is not the intended usage. Leave the device tree alone, add a
wildcard option to platform_match() and use it in VFIO, and set
drv->sysfs_bind_only in VFIO.
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 73f6c29..da81442
> > 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -201,7 +201,8 @@ static ssize_t bind_store(struct device_driver *drv, const
> > char *buf,
> > int err = -ENODEV;
> >
> > dev = bus_find_device_by_name(bus, NULL, buf);
> > - if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
> > + if (dev && dev->driver == NULL && (drv->sysfs_bind_only ||
> > + driver_match_device(drv, dev))) {
>
> Should not we check
> if (dev && dev->driver == NULL &&
> (device->explicit_bind_only && drv->explicit_bind_only) ||
> driver_match_device(drv, dev)))
device->sysfs_bind_only would be a separate patch.
As for drv->sysfs_bind_only, that does not override
driver_match_device(). Wildcard matches are separate and are handled by
individual bus match functions. This function does not need to be
changed at all for drv->sysfs_bind_only.
-Scott
Platform drivers such as the vfio-platform "meta-" driver [1]
should be allowed to specify that they can bind to any device,
much like PCI drivers can with PCI_ANY_ID.
Currently, binding platform drivers to devices depends on:
- a string match in the device node's compatible entry (OF)
- a string match in the ACPI id list (ACPI)
- a string match in the id_table (platform data)
- a string match on the driver name (fall-back)
none of which allow for the notion of "match any."
This patch adds the notion by adding a "match any device" boolean to
struct platform_driver, for drivers to be able to set and thus not cause
platform_match() to fail when a bind is requested.
[1] http://www.spinics.net/lists/kvm/msg96701.html
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/base/platform.c | 4 ++++
include/linux/platform_device.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4f8bef3..0ca20d4 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -727,6 +727,10 @@ static int platform_match(struct device *dev, struct device_driver *drv)
struct platform_device *pdev = to_platform_device(dev);
struct platform_driver *pdrv = to_platform_driver(drv);
+ /* the driver matches any device */
+ if (pdrv->match_any_dev)
+ return 1;
+
/* Attempt an OF style match first */
if (of_driver_match_device(dev, drv))
return 1;
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index ce8e4ff..2d25d50 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -178,6 +178,7 @@ struct platform_driver {
int (*resume)(struct platform_device *);
struct device_driver driver;
const struct platform_device_id *id_table;
+ bool match_any_dev;
};
#define to_platform_driver(drv) (container_of((drv), struct platform_driver, \
--
1.8.4
VFIO supports pass-through of devices to user space - for sake
of illustration, say a PCI e1000 device:
- the e1000 is first unbound from the PCI e1000 driver via sysfs
- the vfio-pci driver is told via new_id that it now handles e1000 devices
- the e1000 is explicitly bound to vfio-pci through sysfs
However, now we have two drivers in the system that both handle e1000
devices. A hotplug event could then occur and it is ambiguous as to which
driver will claim the device. The desired semantics is that vfio-pci is
only bound to devices by explicit request in sysfs. This patch makes this
possible by introducing a sysfs_bind_only flag in struct device_driver.
Signed-off-by: Stuart Yoder <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
this patchset is available on top of the WIP exynos-iommu v10 [1] and
vfio-platform v2 [2] patchsets, here:
git://git.linaro.org/people/kimphill/linux.git binding-dev
[1] http://www.spinics.net/lists/linux-samsung-soc/msg23301.html
[2] http://www.spinics.net/lists/kvm/msg96701.html
drivers/base/dd.c | 5 ++++-
include/linux/device.h | 2 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 35fa368..6f85279 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -389,7 +389,7 @@ static int __device_attach(struct device_driver *drv, void *data)
{
struct device *dev = data;
- if (!driver_match_device(drv, dev))
+ if (drv->sysfs_bind_only || !driver_match_device(drv, dev))
return 0;
return driver_probe_device(drv, dev);
@@ -476,6 +476,9 @@ static int __driver_attach(struct device *dev, void *data)
*/
int driver_attach(struct device_driver *drv)
{
+ if (drv->sysfs_bind_only)
+ return 0;
+
return bus_for_each_dev(drv->bus, NULL, drv, __driver_attach);
}
EXPORT_SYMBOL_GPL(driver_attach);
diff --git a/include/linux/device.h b/include/linux/device.h
index 2a9d6ed..e63c3fe 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -203,6 +203,7 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus);
* @owner: The module owner.
* @mod_name: Used for built-in modules.
* @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @sysfs_bind_only: Only allow bind/unbind via sysfs.
* @of_match_table: The open firmware table.
* @acpi_match_table: The ACPI match table.
* @probe: Called to query the existence of a specific device,
@@ -236,6 +237,7 @@ struct device_driver {
const char *mod_name; /* used for built-in modules */
bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
+ bool sysfs_bind_only; /* only allow bind/unbind via sysfs */
const struct of_device_id *of_match_table;
const struct acpi_device_id *acpi_match_table;
--
1.8.4
Set match_any_dev and sysfs_bind_only such that echoing a platform
device ID to the driver sysfs bind file will successfully match and bind
the device to the vfio-platform meta-driver in accordance to the
desired semantics for vfio drivers.
Signed-off-by: Kim Phillips <[email protected]>
---
this patch depends on the vfio-platform WIP:
http://www.spinics.net/lists/kvm/msg96701.html
drivers/vfio/vfio_platform.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/vfio/vfio_platform.c b/drivers/vfio/vfio_platform.c
index b92d7bb..c42d50d 100644
--- a/drivers/vfio/vfio_platform.c
+++ b/drivers/vfio/vfio_platform.c
@@ -297,7 +297,9 @@ static struct platform_driver vfio_platform_driver = {
.name = "vfio-platform",
.owner = THIS_MODULE,
.of_match_table = vfio_platform_match,
+ .sysfs_bind_only = true,
},
+ .match_any_dev = true,
};
module_platform_driver(vfio_platform_driver);
--
1.8.4
Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
conflics with other drivers in the event of a hotplug.
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/vfio/pci/vfio_pci.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 6ab71b9..bdd7833 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -901,6 +901,9 @@ static struct pci_driver vfio_pci_driver = {
.probe = vfio_pci_probe,
.remove = vfio_pci_remove,
.err_handler = &vfio_err_handlers,
+ .driver = {
+ .sysfs_bind_only = true,
+ },
};
static void __exit vfio_pci_cleanup(void)
--
1.8.4
On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> conflics with other drivers in the event of a hotplug.
>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 6ab71b9..bdd7833 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -901,6 +901,9 @@ static struct pci_driver vfio_pci_driver = {
> .probe = vfio_pci_probe,
> .remove = vfio_pci_remove,
> .err_handler = &vfio_err_handlers,
> + .driver = {
> + .sysfs_bind_only = true,
> + },
> };
>
> static void __exit vfio_pci_cleanup(void)
You also need to add a PCI_ANY_ID match in order to be able to get rid
of the new_id usage.
-Scott
On Fri, 11 Oct 2013 15:43:40 -0500
Scott Wood <[email protected]> wrote:
> On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> > conflics with other drivers in the event of a hotplug.
> >
> > Signed-off-by: Kim Phillips <[email protected]>
> > ---
> > drivers/vfio/pci/vfio_pci.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index 6ab71b9..bdd7833 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -901,6 +901,9 @@ static struct pci_driver vfio_pci_driver = {
> > .probe = vfio_pci_probe,
> > .remove = vfio_pci_remove,
> > .err_handler = &vfio_err_handlers,
> > + .driver = {
> > + .sysfs_bind_only = true,
> > + },
> > };
> >
> > static void __exit vfio_pci_cleanup(void)
>
> You also need to add a PCI_ANY_ID match in order to be able to get rid
> of the new_id usage.
thanks - see below.
Can someone with a PCI bus test this? Bharat?
Kim
>From a8d6c12f2ec763c2ac7fd384a3397c370cc1b932 Mon Sep 17 00:00:00 2001
From: Kim Phillips <[email protected]>
Date: Thu, 10 Oct 2013 22:16:34 -0500
Subject: [PATCH 3/4 v2] VFIO: pci: amend vfio-pci for explicit binding via sysfs
only
Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
conflics with other drivers in the event of a hotplug. Also replace
the only dynamic ids assignment with a table with a single PCI_ANY_ID
entry since writing the sysfs bind file without having to specify ids
via the new_id file first should no longer be necessary.
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/vfio/pci/vfio_pci.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 6ab71b9..c5b434f 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -895,12 +895,22 @@ static struct pci_error_handlers vfio_err_handlers = {
.error_detected = vfio_pci_aer_err_detected,
};
+static DEFINE_PCI_DEVICE_TABLE(vfio_pci_id_table) = {
+ { PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID) },
+ { 0 }
+};
+
+MODULE_DEVICE_TABLE(pci, vfio_pci_id_table);
+
static struct pci_driver vfio_pci_driver = {
.name = "vfio-pci",
- .id_table = NULL, /* only dynamic ids */
+ .id_table = vfio_pci_id_table, /* no dynamic ids */
.probe = vfio_pci_probe,
.remove = vfio_pci_remove,
.err_handler = &vfio_err_handlers,
+ .driver = {
+ .sysfs_bind_only = true, /* bind only via sysfs */
+ },
};
static void __exit vfio_pci_cleanup(void)
--
1.8.4
> -----Original Message-----
> From: Kim Phillips [mailto:[email protected]]
> Sent: Friday, October 11, 2013 6:17 PM
> To: Wood Scott-B07421
> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; Yoder Stuart-B08248;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> Sethi Varun-B16395; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding
> via sysfs only
>
> On Fri, 11 Oct 2013 15:43:40 -0500
> Scott Wood <[email protected]> wrote:
>
> > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > Force the vfio-pci driver to only be bound explicitly via sysfs to
> avoid
> > > conflics with other drivers in the event of a hotplug.
> > >
> > > Signed-off-by: Kim Phillips <[email protected]>
> > > ---
> > > drivers/vfio/pci/vfio_pci.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/vfio/pci/vfio_pci.c
> b/drivers/vfio/pci/vfio_pci.c
> > > index 6ab71b9..bdd7833 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -901,6 +901,9 @@ static struct pci_driver vfio_pci_driver = {
> > > .probe = vfio_pci_probe,
> > > .remove = vfio_pci_remove,
> > > .err_handler = &vfio_err_handlers,
> > > + .driver = {
> > > + .sysfs_bind_only = true,
> > > + },
> > > };
> > >
> > > static void __exit vfio_pci_cleanup(void)
> >
> > You also need to add a PCI_ANY_ID match in order to be able to get rid
> > of the new_id usage.
>
> thanks - see below.
>
> Can someone with a PCI bus test this? Bharat?
>
> Kim
>
> From a8d6c12f2ec763c2ac7fd384a3397c370cc1b932 Mon Sep 17 00:00:00 2001
> From: Kim Phillips <[email protected]>
> Date: Thu, 10 Oct 2013 22:16:34 -0500
> Subject: [PATCH 3/4 v2] VFIO: pci: amend vfio-pci for explicit binding
> via sysfs
> only
>
> Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> conflics with other drivers in the event of a hotplug. Also replace
> the only dynamic ids assignment with a table with a single PCI_ANY_ID
> entry since writing the sysfs bind file without having to specify ids
> via the new_id file first should no longer be necessary.
>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 6ab71b9..c5b434f 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -895,12 +895,22 @@ static struct pci_error_handlers vfio_err_handlers
> = {
> .error_detected = vfio_pci_aer_err_detected,
> };
>
> +static DEFINE_PCI_DEVICE_TABLE(vfio_pci_id_table) = {
> + { PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID) },
> + { 0 }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, vfio_pci_id_table);
> +
> static struct pci_driver vfio_pci_driver = {
> .name = "vfio-pci",
> - .id_table = NULL, /* only dynamic ids */
> + .id_table = vfio_pci_id_table, /* no dynamic ids */
> .probe = vfio_pci_probe,
> .remove = vfio_pci_remove,
> .err_handler = &vfio_err_handlers,
> + .driver = {
> + .sysfs_bind_only = true, /* bind only via sysfs */
> + },
> };
>
> static void __exit vfio_pci_cleanup(void)
sysfs bind only seems orthogonal to PCI_ANY_ID...I would have split
those into 2 different patches.
Stuart
On Mon, 2013-10-14 at 08:01 -0500, Yoder Stuart-B08248 wrote:
>
> > -----Original Message-----
> > From: Kim Phillips [mailto:[email protected]]
> > Sent: Friday, October 11, 2013 6:17 PM
> > To: Wood Scott-B07421
> > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; Yoder Stuart-B08248;
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; [email protected];
> > Sethi Varun-B16395; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding
> > via sysfs only
> >
> > On Fri, 11 Oct 2013 15:43:40 -0500
> > Scott Wood <[email protected]> wrote:
> >
> > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > Force the vfio-pci driver to only be bound explicitly via sysfs to
> > avoid
> > > > conflics with other drivers in the event of a hotplug.
> > > >
> > > > Signed-off-by: Kim Phillips <[email protected]>
> > > > ---
> > > > drivers/vfio/pci/vfio_pci.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/vfio/pci/vfio_pci.c
> > b/drivers/vfio/pci/vfio_pci.c
> > > > index 6ab71b9..bdd7833 100644
> > > > --- a/drivers/vfio/pci/vfio_pci.c
> > > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > > @@ -901,6 +901,9 @@ static struct pci_driver vfio_pci_driver = {
> > > > .probe = vfio_pci_probe,
> > > > .remove = vfio_pci_remove,
> > > > .err_handler = &vfio_err_handlers,
> > > > + .driver = {
> > > > + .sysfs_bind_only = true,
> > > > + },
> > > > };
> > > >
> > > > static void __exit vfio_pci_cleanup(void)
> > >
> > > You also need to add a PCI_ANY_ID match in order to be able to get rid
> > > of the new_id usage.
> >
> > thanks - see below.
> >
> > Can someone with a PCI bus test this? Bharat?
> >
> > Kim
> >
> > From a8d6c12f2ec763c2ac7fd384a3397c370cc1b932 Mon Sep 17 00:00:00 2001
> > From: Kim Phillips <[email protected]>
> > Date: Thu, 10 Oct 2013 22:16:34 -0500
> > Subject: [PATCH 3/4 v2] VFIO: pci: amend vfio-pci for explicit binding
> > via sysfs
> > only
> >
> > Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> > conflics with other drivers in the event of a hotplug. Also replace
> > the only dynamic ids assignment with a table with a single PCI_ANY_ID
> > entry since writing the sysfs bind file without having to specify ids
> > via the new_id file first should no longer be necessary.
> >
> > Signed-off-by: Kim Phillips <[email protected]>
> > ---
> > drivers/vfio/pci/vfio_pci.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index 6ab71b9..c5b434f 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -895,12 +895,22 @@ static struct pci_error_handlers vfio_err_handlers
> > = {
> > .error_detected = vfio_pci_aer_err_detected,
> > };
> >
> > +static DEFINE_PCI_DEVICE_TABLE(vfio_pci_id_table) = {
> > + { PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID) },
> > + { 0 }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, vfio_pci_id_table);
> > +
> > static struct pci_driver vfio_pci_driver = {
> > .name = "vfio-pci",
> > - .id_table = NULL, /* only dynamic ids */
> > + .id_table = vfio_pci_id_table, /* no dynamic ids */
> > .probe = vfio_pci_probe,
> > .remove = vfio_pci_remove,
> > .err_handler = &vfio_err_handlers,
> > + .driver = {
> > + .sysfs_bind_only = true, /* bind only via sysfs */
> > + },
> > };
> >
> > static void __exit vfio_pci_cleanup(void)
>
> sysfs bind only seems orthogonal to PCI_ANY_ID...I would have split
> those into 2 different patches.
The mechanisms are orthogonal, but the change in use case is not.
sysfs_bind_only is required for PCI_ANY_ID to be safely used here. I
don't see a huge need to split them, but if they are split then
sysfs_bind_only must be first.
-Scott
> -----Original Message-----
> From: Kim Phillips [mailto:[email protected]]
> Sent: Saturday, October 12, 2013 4:47 AM
> To: Wood Scott-B07421
> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; Yoder Stuart-B08248;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected]; Sethi
> Varun-B16395; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Fri, 11 Oct 2013 15:43:40 -0500
> Scott Wood <[email protected]> wrote:
>
> > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> > > conflics with other drivers in the event of a hotplug.
> > >
> > > Signed-off-by: Kim Phillips <[email protected]>
> > > ---
> > > drivers/vfio/pci/vfio_pci.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index 6ab71b9..bdd7833 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -901,6 +901,9 @@ static struct pci_driver vfio_pci_driver = {
> > > .probe = vfio_pci_probe,
> > > .remove = vfio_pci_remove,
> > > .err_handler = &vfio_err_handlers,
> > > + .driver = {
> > > + .sysfs_bind_only = true,
> > > + },
> > > };
> > >
> > > static void __exit vfio_pci_cleanup(void)
> >
> > You also need to add a PCI_ANY_ID match in order to be able to get rid
> > of the new_id usage.
>
> thanks - see below.
>
> Can someone with a PCI bus test this? Bharat?
Hello Kim,
I can test that we can get rid of new_id and use "bind" to bind the device to vfio_pci.
Other thing is generating hotplug, or reorder the driver registration by tweaking Makefile to test sysfs_bind_only way to bind is not yet tested.
Thanks
-Bharat
>
> Kim
>
> From a8d6c12f2ec763c2ac7fd384a3397c370cc1b932 Mon Sep 17 00:00:00 2001
> From: Kim Phillips <[email protected]>
> Date: Thu, 10 Oct 2013 22:16:34 -0500
> Subject: [PATCH 3/4 v2] VFIO: pci: amend vfio-pci for explicit binding via sysfs
> only
>
> Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> conflics with other drivers in the event of a hotplug. Also replace
> the only dynamic ids assignment with a table with a single PCI_ANY_ID
> entry since writing the sysfs bind file without having to specify ids
> via the new_id file first should no longer be necessary.
>
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 6ab71b9..c5b434f 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -895,12 +895,22 @@ static struct pci_error_handlers vfio_err_handlers = {
> .error_detected = vfio_pci_aer_err_detected,
> };
>
> +static DEFINE_PCI_DEVICE_TABLE(vfio_pci_id_table) = {
> + { PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID) },
> + { 0 }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, vfio_pci_id_table);
> +
> static struct pci_driver vfio_pci_driver = {
> .name = "vfio-pci",
> - .id_table = NULL, /* only dynamic ids */
> + .id_table = vfio_pci_id_table, /* no dynamic ids */
> .probe = vfio_pci_probe,
> .remove = vfio_pci_remove,
> .err_handler = &vfio_err_handlers,
> + .driver = {
> + .sysfs_bind_only = true, /* bind only via sysfs */
> + },
> };
>
> static void __exit vfio_pci_cleanup(void)
> --
> 1.8.4
On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> conflics with other drivers in the event of a hotplug.
We can't break userspace, so we can't disable the current method of
binding devices to vfio-pci. We can add a new method and perhaps
deprecate the existing mechanism to be removed at some point in the
future. Thanks,
Alex
> Signed-off-by: Kim Phillips <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 6ab71b9..bdd7833 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -901,6 +901,9 @@ static struct pci_driver vfio_pci_driver = {
> .probe = vfio_pci_probe,
> .remove = vfio_pci_remove,
> .err_handler = &vfio_err_handlers,
> + .driver = {
> + .sysfs_bind_only = true,
> + },
> };
>
> static void __exit vfio_pci_cleanup(void)
On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> > conflics with other drivers in the event of a hotplug.
>
> We can't break userspace, so we can't disable the current method of
> binding devices to vfio-pci. We can add a new method and perhaps
> deprecate the existing mechanism to be removed at some point in the
> future. Thanks,
I thought the existing method involved using sysfs bind, and this was
just eliminating a race. How does the bind get triggered currently?
-Scott
On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > Force the vfio-pci driver to only be bound explicitly via sysfs to avoid
> > > conflics with other drivers in the event of a hotplug.
> >
> > We can't break userspace, so we can't disable the current method of
> > binding devices to vfio-pci. We can add a new method and perhaps
> > deprecate the existing mechanism to be removed at some point in the
> > future. Thanks,
>
> I thought the existing method involved using sysfs bind, and this was
> just eliminating a race. How does the bind get triggered currently?
OK, so it seems it's relying on the write to new_id calling
driver_attach(). Sigh. I guess we could make driver-sysfs-bind-only be
settable via sysfs, and have new-userspace set both that and PCI_ANY_ID
(or the specific ID if userspace prefers) via new_id. The platform bus
patches could continue as is, since there's no existing mechanism to
break.
-Scott
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Monday, October 28, 2013 11:40 PM
> To: Alex Williamson
> Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421; Yoder Stuart-B08248;
> [email protected]; [email protected];
> [email protected]; [email protected]; Sethi Varun-B16395;
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > Force the vfio-pci driver to only be bound explicitly via sysfs to
> > > > avoid conflics with other drivers in the event of a hotplug.
> > >
> > > We can't break userspace, so we can't disable the current method of
> > > binding devices to vfio-pci. We can add a new method and perhaps
> > > deprecate the existing mechanism to be removed at some point in the
> > > future. Thanks,
> >
> > I thought the existing method involved using sysfs bind, and this was
> > just eliminating a race. How does the bind get triggered currently?
>
> OK, so it seems it's relying on the write to new_id calling driver_attach().
> Sigh. I guess we could make driver-sysfs-bind-only be settable via sysfs, and
> have new-userspace set both that and PCI_ANY_ID (or the specific ID if userspace
> prefers) via new_id. The platform bus patches could continue as is, since
> there's no existing mechanism to break.
What about changing the store_new_id() to bypass exact ids check if driver have PCI_ANY_ID?
-Bharat
>
> -Scott
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, 2013-10-28 at 22:38 -0500, Bhushan Bharat-R65777 wrote:
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Monday, October 28, 2013 11:40 PM
> > To: Alex Williamson
> > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421; Yoder Stuart-B08248;
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Sethi Varun-B16395;
> > [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> > sysfs only
> >
> > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > > Force the vfio-pci driver to only be bound explicitly via sysfs to
> > > > > avoid conflics with other drivers in the event of a hotplug.
> > > >
> > > > We can't break userspace, so we can't disable the current method of
> > > > binding devices to vfio-pci. We can add a new method and perhaps
> > > > deprecate the existing mechanism to be removed at some point in the
> > > > future. Thanks,
> > >
> > > I thought the existing method involved using sysfs bind, and this was
> > > just eliminating a race. How does the bind get triggered currently?
> >
> > OK, so it seems it's relying on the write to new_id calling driver_attach().
> > Sigh. I guess we could make driver-sysfs-bind-only be settable via sysfs, and
> > have new-userspace set both that and PCI_ANY_ID (or the specific ID if userspace
> > prefers) via new_id. The platform bus patches could continue as is, since
> > there's no existing mechanism to break.
>
> What about changing the store_new_id() to bypass exact ids check if driver have PCI_ANY_ID?
I don't follow.
-Scott
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, October 29, 2013 9:11 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> [email protected]; [email protected];
> [email protected]; [email protected]; Sethi Varun-B16395;
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Mon, 2013-10-28 at 22:38 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Monday, October 28, 2013 11:40 PM
> > > To: Alex Williamson
> > > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421; Yoder
> > > Stuart-B08248; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; Sethi Varun-B16395; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > binding via sysfs only
> > >
> > > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> > > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > > > Force the vfio-pci driver to only be bound explicitly via
> > > > > > sysfs to avoid conflics with other drivers in the event of a hotplug.
> > > > >
> > > > > We can't break userspace, so we can't disable the current method
> > > > > of binding devices to vfio-pci. We can add a new method and
> > > > > perhaps deprecate the existing mechanism to be removed at some
> > > > > point in the future. Thanks,
> > > >
> > > > I thought the existing method involved using sysfs bind, and this
> > > > was just eliminating a race. How does the bind get triggered currently?
> > >
> > > OK, so it seems it's relying on the write to new_id calling driver_attach().
> > > Sigh. I guess we could make driver-sysfs-bind-only be settable via
> > > sysfs, and have new-userspace set both that and PCI_ANY_ID (or the
> > > specific ID if userspace
> > > prefers) via new_id. The platform bus patches could continue as is,
> > > since there's no existing mechanism to break.
> >
> > What about changing the store_new_id() to bypass exact ids check if driver
> have PCI_ANY_ID?
>
> I don't follow.
store_new_id() function id defined as:
static ssize_t store_new_id(struct device_driver *driver, const char *buf, size_t count)
{
struct pci_driver *pdrv = to_pci_driver(driver);
const struct pci_device_id *ids = pdrv->id_table;
<snip>
/* Only accept driver_data values that match an existing id_table
entry */
if (ids) {
retval = -EINVAL;
while (ids->vendor || ids->subvendor || ids->class_mask) {
if (driver_data == ids->driver_data) {
retval = 0;
break;
}
ids++;
}
if (retval) /* No match */
return retval;
}
retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
class, class_mask, driver_data);
<snip>
So when ids == NULL it does not check of vendor etc and calls pci_add_dynid() which in turn calls driver_attach().
If we change the above loop to break if ids->vendor == PCI_ANY_ID && ids->subvendor == PCI_ANY_ID then also we will call pci_add_dyids().
-Bharat
>
> -Scott
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, 2013-10-28 at 22:52 -0500, Bhushan Bharat-R65777 wrote:
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, October 29, 2013 9:11 AM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Sethi Varun-B16395;
> > [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> > sysfs only
> >
> > On Mon, 2013-10-28 at 22:38 -0500, Bhushan Bharat-R65777 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Monday, October 28, 2013 11:40 PM
> > > > To: Alex Williamson
> > > > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421; Yoder
> > > > Stuart-B08248; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; Sethi Varun-B16395; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > > binding via sysfs only
> > > >
> > > > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> > > > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > > > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > > > > Force the vfio-pci driver to only be bound explicitly via
> > > > > > > sysfs to avoid conflics with other drivers in the event of a hotplug.
> > > > > >
> > > > > > We can't break userspace, so we can't disable the current method
> > > > > > of binding devices to vfio-pci. We can add a new method and
> > > > > > perhaps deprecate the existing mechanism to be removed at some
> > > > > > point in the future. Thanks,
> > > > >
> > > > > I thought the existing method involved using sysfs bind, and this
> > > > > was just eliminating a race. How does the bind get triggered currently?
> > > >
> > > > OK, so it seems it's relying on the write to new_id calling driver_attach().
> > > > Sigh. I guess we could make driver-sysfs-bind-only be settable via
> > > > sysfs, and have new-userspace set both that and PCI_ANY_ID (or the
> > > > specific ID if userspace
> > > > prefers) via new_id. The platform bus patches could continue as is,
> > > > since there's no existing mechanism to break.
> > >
> > > What about changing the store_new_id() to bypass exact ids check if driver
> > have PCI_ANY_ID?
> >
> > I don't follow.
>
> store_new_id() function id defined as:
>
> static ssize_t store_new_id(struct device_driver *driver, const char *buf, size_t count)
> {
> struct pci_driver *pdrv = to_pci_driver(driver);
> const struct pci_device_id *ids = pdrv->id_table;
>
> <snip>
> /* Only accept driver_data values that match an existing id_table
> entry */
> if (ids) {
> retval = -EINVAL;
> while (ids->vendor || ids->subvendor || ids->class_mask) {
> if (driver_data == ids->driver_data) {
> retval = 0;
> break;
> }
> ids++;
> }
> if (retval) /* No match */
> return retval;
> }
>
> retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
> class, class_mask, driver_data);
> <snip>
>
>
> So when ids == NULL it does not check of vendor etc and calls pci_add_dynid() which in turn calls driver_attach().
>
> If we change the above loop to break if ids->vendor == PCI_ANY_ID && ids->subvendor == PCI_ANY_ID then also we will call pci_add_dyids().
What problem are you trying to solve?
-Scott
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, October 29, 2013 10:00 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> [email protected]; [email protected];
> [email protected]; [email protected]; Sethi Varun-B16395;
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Mon, 2013-10-28 at 22:52 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, October 29, 2013 9:11 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > Stuart-B08248; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; Sethi Varun-B16395; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > binding via sysfs only
> > >
> > > On Mon, 2013-10-28 at 22:38 -0500, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Monday, October 28, 2013 11:40 PM
> > > > > To: Alex Williamson
> > > > > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421;
> > > > > Yoder Stuart-B08248; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]; Sethi Varun-B16395; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]
> > > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > > > binding via sysfs only
> > > > >
> > > > > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> > > > > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > > > > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > > > > > Force the vfio-pci driver to only be bound explicitly via
> > > > > > > > sysfs to avoid conflics with other drivers in the event of a
> hotplug.
> > > > > > >
> > > > > > > We can't break userspace, so we can't disable the current
> > > > > > > method of binding devices to vfio-pci. We can add a new
> > > > > > > method and perhaps deprecate the existing mechanism to be
> > > > > > > removed at some point in the future. Thanks,
> > > > > >
> > > > > > I thought the existing method involved using sysfs bind, and
> > > > > > this was just eliminating a race. How does the bind get triggered
> currently?
> > > > >
> > > > > OK, so it seems it's relying on the write to new_id calling
> driver_attach().
> > > > > Sigh. I guess we could make driver-sysfs-bind-only be settable
> > > > > via sysfs, and have new-userspace set both that and PCI_ANY_ID
> > > > > (or the specific ID if userspace
> > > > > prefers) via new_id. The platform bus patches could continue as
> > > > > is, since there's no existing mechanism to break.
> > > >
> > > > What about changing the store_new_id() to bypass exact ids check
> > > > if driver
> > > have PCI_ANY_ID?
> > >
> > > I don't follow.
> >
> > store_new_id() function id defined as:
> >
> > static ssize_t store_new_id(struct device_driver *driver, const char
> > *buf, size_t count) {
> > struct pci_driver *pdrv = to_pci_driver(driver);
> > const struct pci_device_id *ids = pdrv->id_table;
> >
> > <snip>
> > /* Only accept driver_data values that match an existing id_table
> > entry */
> > if (ids) {
> > retval = -EINVAL;
> > while (ids->vendor || ids->subvendor || ids->class_mask) {
> > if (driver_data == ids->driver_data) {
> > retval = 0;
> > break;
> > }
> > ids++;
> > }
> > if (retval) /* No match */
> > return retval;
> > }
> >
> > retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
> > class, class_mask, driver_data); <snip>
> >
> >
> > So when ids == NULL it does not check of vendor etc and calls pci_add_dynid()
> which in turn calls driver_attach().
> >
> > If we change the above loop to break if ids->vendor == PCI_ANY_ID && ids-
> >subvendor == PCI_ANY_ID then also we will call pci_add_dyids().
>
> What problem are you trying to solve?
new_id interface to continue working as before.
-Bharat
>
> -Scott
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, 2013-10-28 at 23:31 -0500, Bhushan Bharat-R65777 wrote:
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, October 29, 2013 10:00 AM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Sethi Varun-B16395;
> > [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> > sysfs only
> >
> > On Mon, 2013-10-28 at 22:52 -0500, Bhushan Bharat-R65777 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Tuesday, October 29, 2013 9:11 AM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > > Stuart-B08248; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; Sethi Varun-B16395; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > > binding via sysfs only
> > > >
> > > > On Mon, 2013-10-28 at 22:38 -0500, Bhushan Bharat-R65777 wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Monday, October 28, 2013 11:40 PM
> > > > > > To: Alex Williamson
> > > > > > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421;
> > > > > > Yoder Stuart-B08248; [email protected];
> > > > > > [email protected]; [email protected];
> > > > > > [email protected]; Sethi Varun-B16395; [email protected];
> > > > > > [email protected]; [email protected];
> > > > > > [email protected]
> > > > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > > > > binding via sysfs only
> > > > > >
> > > > > > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> > > > > > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > > > > > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > > > > > > Force the vfio-pci driver to only be bound explicitly via
> > > > > > > > > sysfs to avoid conflics with other drivers in the event of a
> > hotplug.
> > > > > > > >
> > > > > > > > We can't break userspace, so we can't disable the current
> > > > > > > > method of binding devices to vfio-pci. We can add a new
> > > > > > > > method and perhaps deprecate the existing mechanism to be
> > > > > > > > removed at some point in the future. Thanks,
> > > > > > >
> > > > > > > I thought the existing method involved using sysfs bind, and
> > > > > > > this was just eliminating a race. How does the bind get triggered
> > currently?
> > > > > >
> > > > > > OK, so it seems it's relying on the write to new_id calling
> > driver_attach().
> > > > > > Sigh. I guess we could make driver-sysfs-bind-only be settable
> > > > > > via sysfs, and have new-userspace set both that and PCI_ANY_ID
> > > > > > (or the specific ID if userspace
> > > > > > prefers) via new_id. The platform bus patches could continue as
> > > > > > is, since there's no existing mechanism to break.
> > > > >
> > > > > What about changing the store_new_id() to bypass exact ids check
> > > > > if driver
> > > > have PCI_ANY_ID?
> > > >
> > > > I don't follow.
> > >
> > > store_new_id() function id defined as:
> > >
> > > static ssize_t store_new_id(struct device_driver *driver, const char
> > > *buf, size_t count) {
> > > struct pci_driver *pdrv = to_pci_driver(driver);
> > > const struct pci_device_id *ids = pdrv->id_table;
> > >
> > > <snip>
> > > /* Only accept driver_data values that match an existing id_table
> > > entry */
> > > if (ids) {
> > > retval = -EINVAL;
> > > while (ids->vendor || ids->subvendor || ids->class_mask) {
> > > if (driver_data == ids->driver_data) {
> > > retval = 0;
> > > break;
> > > }
> > > ids++;
> > > }
> > > if (retval) /* No match */
> > > return retval;
> > > }
> > >
> > > retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
> > > class, class_mask, driver_data); <snip>
> > >
> > >
> > > So when ids == NULL it does not check of vendor etc and calls pci_add_dynid()
> > which in turn calls driver_attach().
> > >
> > > If we change the above loop to break if ids->vendor == PCI_ANY_ID && ids-
> > >subvendor == PCI_ANY_ID then also we will call pci_add_dyids().
> >
> > What problem are you trying to solve?
>
> new_id interface to continue working as before.
In what specific way does this allow new_id to continue working as
before? Be verbose.
-Scott
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, October 29, 2013 10:05 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> [email protected]; [email protected];
> [email protected]; [email protected]; Sethi Varun-B16395;
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Mon, 2013-10-28 at 23:31 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, October 29, 2013 10:00 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > Stuart-B08248; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; Sethi Varun-B16395; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > binding via sysfs only
> > >
> > > On Mon, 2013-10-28 at 22:52 -0500, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Tuesday, October 29, 2013 9:11 AM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > > > Stuart-B08248; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]; Sethi Varun-B16395; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]
> > > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > > > binding via sysfs only
> > > > >
> > > > > On Mon, 2013-10-28 at 22:38 -0500, Bhushan Bharat-R65777 wrote:
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Wood Scott-B07421
> > > > > > > Sent: Monday, October 28, 2013 11:40 PM
> > > > > > > To: Alex Williamson
> > > > > > > Cc: Kim Phillips; Bhushan Bharat-R65777; Wood Scott-B07421;
> > > > > > > Yoder Stuart-B08248; [email protected];
> > > > > > > [email protected];
> > > > > > > [email protected]; [email protected]; Sethi
> > > > > > > Varun-B16395; [email protected];
> > > > > > > [email protected]; [email protected];
> > > > > > > [email protected]
> > > > > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for
> > > > > > > explicit binding via sysfs only
> > > > > > >
> > > > > > > On Mon, 2013-10-28 at 13:00 -0500, Scott Wood wrote:
> > > > > > > > On Mon, 2013-10-28 at 11:47 -0600, Alex Williamson wrote:
> > > > > > > > > On Fri, 2013-10-11 at 01:27 -0500, Kim Phillips wrote:
> > > > > > > > > > Force the vfio-pci driver to only be bound explicitly
> > > > > > > > > > via sysfs to avoid conflics with other drivers in the
> > > > > > > > > > event of a
> > > hotplug.
> > > > > > > > >
> > > > > > > > > We can't break userspace, so we can't disable the
> > > > > > > > > current method of binding devices to vfio-pci. We can
> > > > > > > > > add a new method and perhaps deprecate the existing
> > > > > > > > > mechanism to be removed at some point in the future.
> > > > > > > > > Thanks,
> > > > > > > >
> > > > > > > > I thought the existing method involved using sysfs bind,
> > > > > > > > and this was just eliminating a race. How does the bind
> > > > > > > > get triggered
> > > currently?
> > > > > > >
> > > > > > > OK, so it seems it's relying on the write to new_id calling
> > > driver_attach().
> > > > > > > Sigh. I guess we could make driver-sysfs-bind-only be
> > > > > > > settable via sysfs, and have new-userspace set both that and
> > > > > > > PCI_ANY_ID (or the specific ID if userspace
> > > > > > > prefers) via new_id. The platform bus patches could
> > > > > > > continue as is, since there's no existing mechanism to break.
> > > > > >
> > > > > > What about changing the store_new_id() to bypass exact ids
> > > > > > check if driver
> > > > > have PCI_ANY_ID?
> > > > >
> > > > > I don't follow.
> > > >
> > > > store_new_id() function id defined as:
> > > >
> > > > static ssize_t store_new_id(struct device_driver *driver, const
> > > > char *buf, size_t count) {
> > > > struct pci_driver *pdrv = to_pci_driver(driver);
> > > > const struct pci_device_id *ids = pdrv->id_table;
> > > >
> > > > <snip>
> > > > /* Only accept driver_data values that match an existing id_table
> > > > entry */
> > > > if (ids) {
> > > > retval = -EINVAL;
> > > > while (ids->vendor || ids->subvendor || ids->class_mask) {
> > > > if (driver_data == ids->driver_data) {
> > > > retval = 0;
> > > > break;
> > > > }
> > > > ids++;
> > > > }
> > > > if (retval) /* No match */
> > > > return retval;
> > > > }
> > > >
> > > > retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
> > > > class, class_mask, driver_data);
> > > > <snip>
> > > >
> > > >
> > > > So when ids == NULL it does not check of vendor etc and calls
> > > > pci_add_dynid()
> > > which in turn calls driver_attach().
> > > >
> > > > If we change the above loop to break if ids->vendor == PCI_ANY_ID
> > > >&& ids- subvendor == PCI_ANY_ID then also we will call pci_add_dyids().
> > >
> > > What problem are you trying to solve?
> >
> > new_id interface to continue working as before.
>
> In what specific way does this allow new_id to continue working as before? Be
> verbose.
What I observed that this patch (kim's patch) new_id interface stops working. This is found to be because
store_new_id() checks for pdrv->id_table which is no more NULL, so the below check fails
if (ids) {
^^
This is no more NULL, so enter inside the loop
retval = -EINVAL;
while (ids->vendor || ids->subvendor || ids->class_mask) {
if (driver_data == ids->driver_data) {
retval = 0;
break;
}
ids++;
}
if (retval) /* No match */
return retval;
^^^^^
This is where it returns as -EINVAL
}
I tried a quick test of what I am saying but it does not work directly (although passes from the above mentioned check), may be some more changes required :(
-Bharat
>
> -Scott
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, 2013-10-28 at 23:45 -0500, Bhushan Bharat-R65777 wrote:
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, October 29, 2013 10:05 AM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Sethi Varun-B16395;
> > [email protected]; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> > sysfs only
> >
> > On Mon, 2013-10-28 at 23:31 -0500, Bhushan Bharat-R65777 wrote:
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Tuesday, October 29, 2013 10:00 AM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > > Stuart-B08248; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; Sethi Varun-B16395; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > > binding via sysfs only
> > > >
> > > > On Mon, 2013-10-28 at 22:52 -0500, Bhushan Bharat-R65777 wrote:
> > > > > So when ids == NULL it does not check of vendor etc and calls
> > > > > pci_add_dynid()
> > > > which in turn calls driver_attach().
> > > > >
> > > > > If we change the above loop to break if ids->vendor == PCI_ANY_ID
> > > > >&& ids- subvendor == PCI_ANY_ID then also we will call pci_add_dyids().
> > > >
> > > > What problem are you trying to solve?
> > >
> > > new_id interface to continue working as before.
> >
> > In what specific way does this allow new_id to continue working as before? Be
> > verbose.
>
>
> What I observed that this patch (kim's patch) new_id interface stops working.
Yes.
> This is found to be because store_new_id() checks for pdrv->id_table which is no more NULL, so the below check fails
I do not think that is the reason. The reason is because
sysfs_bind_only is set, and this is not a direct sysfs bind.
> if (ids) {
> ^^
> This is no more NULL, so enter inside the loop
>
> retval = -EINVAL;
> while (ids->vendor || ids->subvendor || ids->class_mask) {
> if (driver_data == ids->driver_data) {
> retval = 0;
> break;
> }
> ids++;
> }
> if (retval) /* No match */
> return retval;
> ^^^^^
> This is where it returns as -EINVAL
Why wouldn't it have broken out of the loop earlier, since driver_data
and ids->driver_data should both be zero? I assume this is with a patch
to do PCI_ANY_ID in vfio-pci.
-Scott
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, October 29, 2013 10:25 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder Stuart-B08248;
> [email protected]; [email protected];
> [email protected]; [email protected]; Sethi Varun-B16395;
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit binding via
> sysfs only
>
> On Mon, 2013-10-28 at 23:45 -0500, Bhushan Bharat-R65777 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, October 29, 2013 10:05 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > Stuart-B08248; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; Sethi Varun-B16395; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > binding via sysfs only
> > >
> > > On Mon, 2013-10-28 at 23:31 -0500, Bhushan Bharat-R65777 wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Tuesday, October 29, 2013 10:00 AM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: Wood Scott-B07421; Alex Williamson; Kim Phillips; Yoder
> > > > > Stuart-B08248; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]; Sethi Varun-B16395; [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]
> > > > > Subject: Re: [PATCH 3/4] VFIO: pci: amend vfio-pci for explicit
> > > > > binding via sysfs only
> > > > >
> > > > > On Mon, 2013-10-28 at 22:52 -0500, Bhushan Bharat-R65777 wrote:
> > > > > > So when ids == NULL it does not check of vendor etc and calls
> > > > > > pci_add_dynid()
> > > > > which in turn calls driver_attach().
> > > > > >
> > > > > > If we change the above loop to break if ids->vendor ==
> > > > > >PCI_ANY_ID && ids- subvendor == PCI_ANY_ID then also we will call
> pci_add_dyids().
> > > > >
> > > > > What problem are you trying to solve?
> > > >
> > > > new_id interface to continue working as before.
> > >
> > > In what specific way does this allow new_id to continue working as
> > > before? Be verbose.
> >
> >
> > What I observed that this patch (kim's patch) new_id interface stops working.
>
> Yes.
>
> > This is found to be because store_new_id() checks for pdrv->id_table
> > which is no more NULL, so the below check fails
>
> I do not think that is the reason. The reason is because sysfs_bind_only is
> set, and this is not a direct sysfs bind.
>
> > if (ids) {
> > ^^
> > This is no more NULL, so enter inside the loop
> >
> > retval = -EINVAL;
> > while (ids->vendor || ids->subvendor || ids->class_mask) {
> > if (driver_data == ids->driver_data) {
> > retval = 0;
> > break;
> > }
> > ids++;
> > }
> > if (retval) /* No match */
> > return retval; ^^^^^ This is where it returns
> > as -EINVAL
>
> Why wouldn't it have broken out of the loop earlier, since driver_data and ids-
> >driver_data should both be zero? I assume this is with a patch to do
> PCI_ANY_ID in vfio-pci.
hmmm, I am pretty sure I have seen that issue a few time (below is command line output) but now I am not getting any error reported. Although device is not binding to driver because of sysfs_bind_only as you mentioned (I thought of this as a second issue). If I will be able to reproduce the first issue then I will let you guys know otherwise there was no first issue :(
root@p5040ds:/sys/bus/pci# echo 0000:01:00.0 > devices/0000\:01\:00.0/driver/unbind
e1000e 0000:01:00.0 eth0: removed PHC
root@p5040ds:/sys/bus/pci# echo 8086 10d3 > drivers/vfio-pci/new_id
-sh: echo: write error: Invalid argument
root@p5040ds:/sys/bus/pci# echo 0000:01:00.0 > drivers/vfio-pci/bind
-Bharat
>
> -Scott
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?