[REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Kim Phillips]

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 <stuart.yoder@freescale.com>
Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
---
rebased onto 3.13-rc2, and reposted from first submission which
recieved no comments:

https://lkml.org/lkml/2013/10/11/53

 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 0605176..b83b16d 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 952b010..ed441d1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -200,6 +200,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,
@@ -233,6 +234,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.5

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Jan Kiszka]

On 2013-12-03 13:34, Kim Phillips wrote:
> 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 <stuart.yoder@freescale.com>
> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> ---
> rebased onto 3.13-rc2, and reposted from first submission which
> recieved no comments:
> 
> https://lkml.org/lkml/2013/10/11/53
> 
>  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 0605176..b83b16d 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 952b010..ed441d1 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -200,6 +200,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,
> @@ -233,6 +234,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;
> 

I think I only discussed this with Stuart in person at the KVM Forum:
Why not deriving the property "sysfs bind only" from the fact that a
device does wild-card binding? Are there use cases that benefit from
decoupling both features?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Kim Phillips]

On Tue, 03 Dec 2013 16:34:33 +0100
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2013-12-03 13:34, Kim Phillips wrote:
> > 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 <stuart.yoder@freescale.com>
> > Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> > ---
> > rebased onto 3.13-rc2, and reposted from first submission which
> > recieved no comments:
> > 
> > https://lkml.org/lkml/2013/10/11/53
> > 
> >  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 0605176..b83b16d 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 952b010..ed441d1 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -200,6 +200,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,
> > @@ -233,6 +234,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;
> 
> I think I only discussed this with Stuart in person at the KVM Forum:
> Why not deriving the property "sysfs bind only" from the fact that a
> device does wild-card binding? Are there use cases that benefit from
> decoupling both features?

you mean merge the two new flags sysfs_bind_only and platform driver's
match_any_dev into one new single driver flag, right?  good question.

I can't think of any, given none of the built-in PCI drivers set ANY_ID
across the {sub}vendor/{sub}device/class{_mask} board for boot-time (or
hotplug) binding.  Technically it's possible with the pci_stub driver
by setting its 'ids' module parameter in the kernel command line, but
looking at the commit that adds 'ids', it was only meant to be used to
"prevent built-in drivers from attaching to specific devices," so it's
unlikely anyone would have it set to ANY_ID.

I'll look at what changes are necessary, barring anyone else coming up
with a valid use-case.

Kim

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Scott Wood]

On Thu, 2013-12-05 at 17:45 +0000, Kim Phillips wrote:
> On Tue, 03 Dec 2013 16:34:33 +0100
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
> > On 2013-12-03 13:34, Kim Phillips wrote:
> > > 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 <stuart.yoder@freescale.com>
> > > Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> > > ---
> > > rebased onto 3.13-rc2, and reposted from first submission which
> > > recieved no comments:
> > > 
> > > https://lkml.org/lkml/2013/10/11/53
> > > 
> > >  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 0605176..b83b16d 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 952b010..ed441d1 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -200,6 +200,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,
> > > @@ -233,6 +234,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;
> > 
> > I think I only discussed this with Stuart in person at the KVM Forum:
> > Why not deriving the property "sysfs bind only" from the fact that a
> > device does wild-card binding? Are there use cases that benefit from
> > decoupling both features?
> 
> you mean merge the two new flags sysfs_bind_only and platform driver's
> match_any_dev into one new single driver flag, right?  good question.

What would combining them solve, other than making it more likely that
Greg complains about the wildcard because it would no longer be handled
at the bus level where all the other matching goes on?

They are logically separate things.  That doesn't change just because we
currently plan to use them together.

-Scott

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Kim Phillips]

On Thu, 5 Dec 2013 16:38:15 -0600
Scott Wood <scottwood@freescale.com> wrote:

> On Thu, 2013-12-05 at 17:45 +0000, Kim Phillips wrote:
> > On Tue, 03 Dec 2013 16:34:33 +0100
> > Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > 
> > > On 2013-12-03 13:34, Kim Phillips wrote:
> > > > 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 <stuart.yoder@freescale.com>
> > > > Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> > > > ---
> > > > rebased onto 3.13-rc2, and reposted from first submission which
> > > > recieved no comments:
> > > > 
> > > > https://lkml.org/lkml/2013/10/11/53
> > > > 
> > > >  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 0605176..b83b16d 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 952b010..ed441d1 100644
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -200,6 +200,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,
> > > > @@ -233,6 +234,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;
> > > 
> > > I think I only discussed this with Stuart in person at the KVM Forum:
> > > Why not deriving the property "sysfs bind only" from the fact that a
> > > device does wild-card binding? Are there use cases that benefit from
> > > decoupling both features?
> > 
> > you mean merge the two new flags sysfs_bind_only and platform driver's
> > match_any_dev into one new single driver flag, right?  good question.
> 
> What would combining them solve, other than making it more likely that
> Greg complains about the wildcard because it would no longer be handled
> at the bus level where all the other matching goes on?
> 
> They are logically separate things.  That doesn't change just because we
> currently plan to use them together.

Jan?  Given the above, what would be the advantage of merging
sysfs_bind_only and (PCI drivers' PCI_ANY_ID and platform drivers'
match_any_dev)?

Kim

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Jan Kiszka]

On 2013-12-09 19:58, Kim Phillips wrote:
> On Thu, 5 Dec 2013 16:38:15 -0600
> Scott Wood <scottwood@freescale.com> wrote:
> 
>> On Thu, 2013-12-05 at 17:45 +0000, Kim Phillips wrote:
>>> On Tue, 03 Dec 2013 16:34:33 +0100
>>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>
>>>> On 2013-12-03 13:34, Kim Phillips wrote:
>>>>> 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 <stuart.yoder@freescale.com>
>>>>> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
>>>>> ---
>>>>> rebased onto 3.13-rc2, and reposted from first submission which
>>>>> recieved no comments:
>>>>>
>>>>> https://lkml.org/lkml/2013/10/11/53
>>>>>
>>>>>  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 0605176..b83b16d 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 952b010..ed441d1 100644
>>>>> --- a/include/linux/device.h
>>>>> +++ b/include/linux/device.h
>>>>> @@ -200,6 +200,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,
>>>>> @@ -233,6 +234,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;
>>>>
>>>> I think I only discussed this with Stuart in person at the KVM Forum:
>>>> Why not deriving the property "sysfs bind only" from the fact that a
>>>> device does wild-card binding? Are there use cases that benefit from
>>>> decoupling both features?
>>>
>>> you mean merge the two new flags sysfs_bind_only and platform driver's
>>> match_any_dev into one new single driver flag, right?  good question.
>>
>> What would combining them solve, other than making it more likely that
>> Greg complains about the wildcard because it would no longer be handled
>> at the bus level where all the other matching goes on?
>>
>> They are logically separate things.  That doesn't change just because we
>> currently plan to use them together.
> 
> Jan?  Given the above, what would be the advantage of merging
> sysfs_bind_only and (PCI drivers' PCI_ANY_ID and platform drivers'
> match_any_dev)?

That you cannot configure (likely) meaningless or even harmful (bind-any
+ auto-bind) configurations.

I didn't follow if Greg expressed his opinion on this or a similar
scenario before. If he prefers separate knobs for a certain reason, he
likely wins.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Scott Wood]

On Mon, 2013-12-09 at 20:12 +0100, Jan Kiszka wrote:
> On 2013-12-09 19:58, Kim Phillips wrote:
> > On Thu, 5 Dec 2013 16:38:15 -0600
> > Scott Wood <scottwood@freescale.com> wrote:
> > 
> >> What would combining them solve, other than making it more likely that
> >> Greg complains about the wildcard because it would no longer be handled
> >> at the bus level where all the other matching goes on?
> >>
> >> They are logically separate things.  That doesn't change just because we
> >> currently plan to use them together.
> > 
> > Jan?  Given the above, what would be the advantage of merging
> > sysfs_bind_only and (PCI drivers' PCI_ANY_ID and platform drivers'
> > match_any_dev)?
> 
> That you cannot configure (likely) meaningless or even harmful (bind-any
> + auto-bind) configurations.

If you want to put in a check that warns on bind-any plus auto-bind,
fine -- but that doesn't mean they should share a mechanism.  It's valid
to have no-auto-bind without a wildcard match.  And FWIW, PCI already
has wildcard matches without any no-auto-bind mechanism (it's presumably
not intended to specify PCI_ANY_ID for all fields, but it is allowed
AFAICT).

-Scott

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Greg Kroah-Hartman]

On Tue, Dec 03, 2013 at 04:34:33PM +0100, Jan Kiszka wrote:
> On 2013-12-03 13:34, Kim Phillips wrote:
> > 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 <stuart.yoder@freescale.com>
> > Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> > ---
> > rebased onto 3.13-rc2, and reposted from first submission which
> > recieved no comments:
> > 
> > https://lkml.org/lkml/2013/10/11/53
> > 
> >  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 0605176..b83b16d 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 952b010..ed441d1 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -200,6 +200,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,
> > @@ -233,6 +234,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;
> > 
> 
> I think I only discussed this with Stuart in person at the KVM Forum:
> Why not deriving the property "sysfs bind only" from the fact that a
> device does wild-card binding? Are there use cases that benefit from
> decoupling both features?

The driver core does not know if a bus, or a device, handles "wild card"
binding, so you can't do it at this level, sorry.

greg k-h

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Greg Kroah-Hartman]

On Tue, Dec 03, 2013 at 12:34:46PM +0000, Kim Phillips wrote:
> 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.

Why deal with this at all and not just deal with the "bind" sysfs file
instead?  That way no driver core logic needs to be changed at all, and
your userspace tools know _exactly_ which device is being bound to the
new device.

Don't mess with the "new_id" file for stuff like this, as you point out,
it's "tricky"...

thanks,

greg k-h

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Scott Wood]

On Wed, 2013-12-18 at 17:07 -0800, Greg Kroah-Hartman wrote:
> On Tue, Dec 03, 2013 at 12:34:46PM +0000, Kim Phillips wrote:
> > 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.
> 
> Why deal with this at all and not just deal with the "bind" sysfs file
> instead?  That way no driver core logic needs to be changed at all, and
> your userspace tools know _exactly_ which device is being bound to the
> new device.
> 
> Don't mess with the "new_id" file for stuff like this, as you point out,
> it's "tricky"...

As discussed before, "bind" does not bypass the ID checks, and thus it
does not work without either "new_id" or a wildcard match.

Or are you proposing changing "bind" so that it does bypass the ID
checks?  Or perhaps a new "force_bind" file that does?

-Scott

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Greg Kroah-Hartman]

On Thu, Dec 19, 2013 at 02:22:11PM -0600, Scott Wood wrote:
> On Wed, 2013-12-18 at 17:07 -0800, Greg Kroah-Hartman wrote:
> > On Tue, Dec 03, 2013 at 12:34:46PM +0000, Kim Phillips wrote:
> > > 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.
> > 
> > Why deal with this at all and not just deal with the "bind" sysfs file
> > instead?  That way no driver core logic needs to be changed at all, and
> > your userspace tools know _exactly_ which device is being bound to the
> > new device.
> > 
> > Don't mess with the "new_id" file for stuff like this, as you point out,
> > it's "tricky"...
> 
> As discussed before, "bind" does not bypass the ID checks, and thus it
> does not work without either "new_id" or a wildcard match.

Ah, forgot about that.

> Or are you proposing changing "bind" so that it does bypass the ID
> checks?  Or perhaps a new "force_bind" file that does?

No.  But you can use bind/unbind along with the existing new_id file to
get what you want today.  If you just happen to bind a device to a wrong
driver for a while, that's not really a problem, right?

I don't like this patch as we are adding lots of special and odd logic
to the core, for use by almost no one, which ensures that it will never
get tested, and will probably get broken in some subtle way in the
future.

Heck, I can't even ensure that you got it right now, with this tiny
patch, how do you know it works given that there are no users of this
flag anywhere (hint, you never showed me any...)

greg k-h

RE: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Stuart Yoder]

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, December 19, 2013 2:34 PM
> To: Wood Scott-B07421
> Cc: Kim Phillips; linux-kernel@vger.kernel.org; kvm@vger.kernel.org;
> Bhushan Bharat-R65777; Yoder Stuart-B08248; christoffer.dall@linaro.org;
> alex.williamson@redhat.com; a.motakis@virtualopensystems.com;
> agraf@suse.de; Sethi Varun-B16395
> Subject: Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag
> to allow binding via sysfs only
> 
> On Thu, Dec 19, 2013 at 02:22:11PM -0600, Scott Wood wrote:
> > On Wed, 2013-12-18 at 17:07 -0800, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 03, 2013 at 12:34:46PM +0000, Kim Phillips wrote:
> > > > 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.
> > >
> > > Why deal with this at all and not just deal with the "bind" sysfs
> file
> > > instead?  That way no driver core logic needs to be changed at all,
> and
> > > your userspace tools know _exactly_ which device is being bound to
> the
> > > new device.
> > >
> > > Don't mess with the "new_id" file for stuff like this, as you point
> out,
> > > it's "tricky"...
> >
> > As discussed before, "bind" does not bypass the ID checks, and thus it
> > does not work without either "new_id" or a wildcard match.
> 
> Ah, forgot about that.
> 
> > Or are you proposing changing "bind" so that it does bypass the ID
> > checks?  Or perhaps a new "force_bind" file that does?
> 
> No.  But you can use bind/unbind along with the existing new_id file to
> get what you want today.

Yes, but that only works for PCI.  There is no such concept for platform
drivers.

> If you just happen to bind a device to a wrong
> driver for a while, that's not really a problem, right?

It's annoying but not the end of the world.

> I don't like this patch as we are adding lots of special and odd logic
> to the core, for use by almost no one, which ensures that it will never
> get tested, and will probably get broken in some subtle way in the
> future.

It certainly will be used by users of vfio-platform.

Here is the problem-- the new platform device "match_any_dev" mechanism
in patch 2 of this series is not going to work without "sysfs_bind_only".
A platform driver that just sets "match_any_dev" will grab any or all 
platform devices during normal bus probing.

> Heck, I can't even ensure that you got it right now, with this tiny
> patch, how do you know it works given that there are no users of this
> flag anywhere (hint, you never showed me any...)

It has been tested on both vfio-pci and vfio-platform in some limited
experiments as far as I know, but that code is not ready for upstream
yet.

Thanks,
Stuart

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Greg Kroah-Hartman]

On Thu, Dec 19, 2013 at 09:06:21PM +0000, Stuart Yoder wrote:
> 
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> > Sent: Thursday, December 19, 2013 2:34 PM
> > To: Wood Scott-B07421
> > Cc: Kim Phillips; linux-kernel@vger.kernel.org; kvm@vger.kernel.org;
> > Bhushan Bharat-R65777; Yoder Stuart-B08248; christoffer.dall@linaro.org;
> > alex.williamson@redhat.com; a.motakis@virtualopensystems.com;
> > agraf@suse.de; Sethi Varun-B16395
> > Subject: Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag
> > to allow binding via sysfs only
> > 
> > On Thu, Dec 19, 2013 at 02:22:11PM -0600, Scott Wood wrote:
> > > On Wed, 2013-12-18 at 17:07 -0800, Greg Kroah-Hartman wrote:
> > > > On Tue, Dec 03, 2013 at 12:34:46PM +0000, Kim Phillips wrote:
> > > > > 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.
> > > >
> > > > Why deal with this at all and not just deal with the "bind" sysfs
> > file
> > > > instead?  That way no driver core logic needs to be changed at all,
> > and
> > > > your userspace tools know _exactly_ which device is being bound to
> > the
> > > > new device.
> > > >
> > > > Don't mess with the "new_id" file for stuff like this, as you point
> > out,
> > > > it's "tricky"...
> > >
> > > As discussed before, "bind" does not bypass the ID checks, and thus it
> > > does not work without either "new_id" or a wildcard match.
> > 
> > Ah, forgot about that.
> > 
> > > Or are you proposing changing "bind" so that it does bypass the ID
> > > checks?  Or perhaps a new "force_bind" file that does?
> > 
> > No.  But you can use bind/unbind along with the existing new_id file to
> > get what you want today.
> 
> Yes, but that only works for PCI.

No, not only PCI.

> There is no such concept for platform drivers.

Then fix that.

Or make your device not be a platform device, odds are that's the better
solution in the end, right?

> > If you just happen to bind a device to a wrong
> > driver for a while, that's not really a problem, right?
> 
> It's annoying but not the end of the world.
> 
> > I don't like this patch as we are adding lots of special and odd logic
> > to the core, for use by almost no one, which ensures that it will never
> > get tested, and will probably get broken in some subtle way in the
> > future.
> 
> It certainly will be used by users of vfio-platform.
> 
> Here is the problem-- the new platform device "match_any_dev" mechanism
> in patch 2 of this series is not going to work without "sysfs_bind_only".
> A platform driver that just sets "match_any_dev" will grab any or all 
> platform devices during normal bus probing.

No it will not, it will fail in the probe function as it knows to not
grab the device, just like any driver for other busses that say it can
"handle all Intel PCI devices" and the like.

> > Heck, I can't even ensure that you got it right now, with this tiny
> > patch, how do you know it works given that there are no users of this
> > flag anywhere (hint, you never showed me any...)
> 
> It has been tested on both vfio-pci and vfio-platform in some limited
> experiments as far as I know, but that code is not ready for upstream
> yet.

Then I'll wait until you get something that actually is worth
upstreaming before worrying about this again.

greg k-h

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Scott Wood]

On Thu, 2013-12-19 at 13:43 -0800, Greg Kroah-Hartman wrote:
> On Thu, Dec 19, 2013 at 09:06:21PM +0000, Stuart Yoder wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> > > Sent: Thursday, December 19, 2013 2:34 PM
> > > To: Wood Scott-B07421
> > > Cc: Kim Phillips; linux-kernel@vger.kernel.org; kvm@vger.kernel.org;
> > > Bhushan Bharat-R65777; Yoder Stuart-B08248; christoffer.dall@linaro.org;
> > > alex.williamson@redhat.com; a.motakis@virtualopensystems.com;
> > > agraf@suse.de; Sethi Varun-B16395
> > > Subject: Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag
> > > to allow binding via sysfs only
> > > 
> > > No.  But you can use bind/unbind along with the existing new_id file to
> > > get what you want today.
> > 
> > Yes, but that only works for PCI.
> 
> No, not only PCI.
> 
> > There is no such concept for platform drivers.
> 
> Then fix that.

We've already explained why that would be bad.

> Or make your device not be a platform device, odds are that's the better
> solution in the end, right?

How would that solve anything?  We'd just be talking about there not
being such a mechanism for the device tree "bus" instead.

> > > If you just happen to bind a device to a wrong
> > > driver for a while, that's not really a problem, right?
> > 
> > It's annoying but not the end of the world.

Lots of bugs are "not the end of the world" but that doesn't mean we
don't fix them.  It certainly doesn't mean we duplicate the source of
the bug in new subsystems.

> > > I don't like this patch as we are adding lots of special and odd logic
> > > to the core, for use by almost no one, which ensures that it will never
> > > get tested, and will probably get broken in some subtle way in the
> > > future.
> > 
> > It certainly will be used by users of vfio-platform.
> > 
> > Here is the problem-- the new platform device "match_any_dev" mechanism
> > in patch 2 of this series is not going to work without "sysfs_bind_only".
> > A platform driver that just sets "match_any_dev" will grab any or all 
> > platform devices during normal bus probing.
> 
> No it will not, it will fail in the probe function as it knows to not
> grab the device, just like any driver for other busses that say it can
> "handle all Intel PCI devices" and the like.

How will it "know not to grab the device"?  The knowledge of whether the
binding was explicitly requested or not does not get passed through to
the probe function.

It would be a shame to have to implement code in VFIO, and a userspace
interface to go along with it, to keep track of which devices have been
authorized to be bound when we already have sysfs bind -- we just need a
way to differentiate that from automatic binding.

-Scott

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Greg Kroah-Hartman]

On Thu, Dec 19, 2013 at 04:15:03PM -0600, Scott Wood wrote:
> On Thu, 2013-12-19 at 13:43 -0800, Greg Kroah-Hartman wrote:
> > On Thu, Dec 19, 2013 at 09:06:21PM +0000, Stuart Yoder wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> > > > Sent: Thursday, December 19, 2013 2:34 PM
> > > > To: Wood Scott-B07421
> > > > Cc: Kim Phillips; linux-kernel@vger.kernel.org; kvm@vger.kernel.org;
> > > > Bhushan Bharat-R65777; Yoder Stuart-B08248; christoffer.dall@linaro.org;
> > > > alex.williamson@redhat.com; a.motakis@virtualopensystems.com;
> > > > agraf@suse.de; Sethi Varun-B16395
> > > > Subject: Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag
> > > > to allow binding via sysfs only
> > > > 
> > > > No.  But you can use bind/unbind along with the existing new_id file to
> > > > get what you want today.
> > > 
> > > Yes, but that only works for PCI.
> > 
> > No, not only PCI.
> > 
> > > There is no such concept for platform drivers.
> > 
> > Then fix that.
> 
> We've already explained why that would be bad.

No you haven't, or if you have, my squirrel-brain doesn't remember it...

> > Or make your device not be a platform device, odds are that's the better
> > solution in the end, right?
> 
> How would that solve anything?  We'd just be talking about there not
> being such a mechanism for the device tree "bus" instead.

Nope, you could add it there, like PCI and other busses have.

> > > > I don't like this patch as we are adding lots of special and odd logic
> > > > to the core, for use by almost no one, which ensures that it will never
> > > > get tested, and will probably get broken in some subtle way in the
> > > > future.
> > > 
> > > It certainly will be used by users of vfio-platform.
> > > 
> > > Here is the problem-- the new platform device "match_any_dev" mechanism
> > > in patch 2 of this series is not going to work without "sysfs_bind_only".
> > > A platform driver that just sets "match_any_dev" will grab any or all 
> > > platform devices during normal bus probing.
> > 
> > No it will not, it will fail in the probe function as it knows to not
> > grab the device, just like any driver for other busses that say it can
> > "handle all Intel PCI devices" and the like.
> 
> How will it "know not to grab the device"?  The knowledge of whether the
> binding was explicitly requested or not does not get passed through to
> the probe function.

Nor should it, as a driver should not know, nor care about this.

It's up to the BUS to handle this if it really wants to, and I'm afraid
that I really am not convinced that the driver core needs to handle it
either.

But again, as you don't have anything that could actually use this code
that is mergable, it's a totally moot point, sorry.

greg k-h

RE: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Stuart Yoder]

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, December 19, 2013 4:32 PM
> To: Wood Scott-B07421
> Cc: Yoder Stuart-B08248; Kim Phillips; linux-kernel@vger.kernel.org;
> kvm@vger.kernel.org; Bhushan Bharat-R65777; christoffer.dall@linaro.org;
> alex.williamson@redhat.com; a.motakis@virtualopensystems.com;
> agraf@suse.de; Sethi Varun-B16395
> Subject: Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag
> to allow binding via sysfs only
> 
> On Thu, Dec 19, 2013 at 04:15:03PM -0600, Scott Wood wrote:
> > On Thu, 2013-12-19 at 13:43 -0800, Greg Kroah-Hartman wrote:
> > > On Thu, Dec 19, 2013 at 09:06:21PM +0000, Stuart Yoder wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> > > > > Sent: Thursday, December 19, 2013 2:34 PM
> > > > > To: Wood Scott-B07421
> > > > > Cc: Kim Phillips; linux-kernel@vger.kernel.org;
> kvm@vger.kernel.org;
> > > > > Bhushan Bharat-R65777; Yoder Stuart-B08248;
> christoffer.dall@linaro.org;
> > > > > alex.williamson@redhat.com; a.motakis@virtualopensystems.com;
> > > > > agraf@suse.de; Sethi Varun-B16395
> > > > > Subject: Re: [REPOST][PATCH 1/2] driver core: Add new
> device_driver flag
> > > > > to allow binding via sysfs only
> > > > >
> > > > > No.  But you can use bind/unbind along with the existing new_id
> file to
> > > > > get what you want today.
> > > >
> > > > Yes, but that only works for PCI.
> > >
> > > No, not only PCI.
> > >
> > > > There is no such concept for platform drivers.
> > >
> > > Then fix that.
> >
> > We've already explained why that would be bad.
> 
> No you haven't, or if you have, my squirrel-brain doesn't remember it...
> 
> > > Or make your device not be a platform device, odds are that's the
> better
> > > solution in the end, right?
> >
> > How would that solve anything?  We'd just be talking about there not
> > being such a mechanism for the device tree "bus" instead.
> 
> Nope, you could add it there, like PCI and other busses have.
> 
> > > > > I don't like this patch as we are adding lots of special and odd
> logic
> > > > > to the core, for use by almost no one, which ensures that it will
> never
> > > > > get tested, and will probably get broken in some subtle way in
> the
> > > > > future.
> > > >
> > > > It certainly will be used by users of vfio-platform.
> > > >
> > > > Here is the problem-- the new platform device "match_any_dev"
> mechanism
> > > > in patch 2 of this series is not going to work without
> "sysfs_bind_only".
> > > > A platform driver that just sets "match_any_dev" will grab any or
> all
> > > > platform devices during normal bus probing.
> > >
> > > No it will not, it will fail in the probe function as it knows to not
> > > grab the device, just like any driver for other busses that say it
> can
> > > "handle all Intel PCI devices" and the like.
> >
> > How will it "know not to grab the device"?  The knowledge of whether
> the
> > binding was explicitly requested or not does not get passed through to
> > the probe function.
> 
> Nor should it, as a driver should not know, nor care about this.
> 
> It's up to the BUS to handle this if it really wants to, and I'm afraid
> that I really am not convinced that the driver core needs to handle it
> either.
> 
> But again, as you don't have anything that could actually use this code
> that is mergable, it's a totally moot point, sorry.

Understand, but what assumption do we develop vfio-plaform with?  That
a driver core 'sysfs_bind_only' flag is not an option, period?  If that
is the case we need to go back to square one and invent some new mechanism
to bind devices to the vfio-platform driver.

I guess it would need to be the platform bus equivalent of new_id.

But, then we're left with the potential racy situation where multiple drivers
can potentially grab a device and it's ambiguous and non-deterministic at to which
driver binds to it.

Stuart

Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag to allow binding via sysfs only

[Greg Kroah-Hartman]

On Thu, Dec 19, 2013 at 11:08:55PM +0000, Stuart Yoder wrote:
> > > How will it "know not to grab the device"?  The knowledge of whether
> > the
> > > binding was explicitly requested or not does not get passed through to
> > > the probe function.
> > 
> > Nor should it, as a driver should not know, nor care about this.
> > 
> > It's up to the BUS to handle this if it really wants to, and I'm afraid
> > that I really am not convinced that the driver core needs to handle it
> > either.
> > 
> > But again, as you don't have anything that could actually use this code
> > that is mergable, it's a totally moot point, sorry.
> 
> Understand, but what assumption do we develop vfio-plaform with?  That
> a driver core 'sysfs_bind_only' flag is not an option, period?  If that
> is the case we need to go back to square one and invent some new mechanism
> to bind devices to the vfio-platform driver.

Ok, why the total confusion between PCI and platform devices here?  Why
keep mentioning both of them in a semi-interchangeable way?

> I guess it would need to be the platform bus equivalent of new_id.

Then add it, there's nothing stopping the platform bus from supporting
this.

> But, then we're left with the potential racy situation where multiple drivers
> can potentially grab a device and it's ambiguous and non-deterministic at to which
> driver binds to it.

Welcome to life in hotpluggable systems, this is nothing new :)

Seriously, userspace has the ability to sort this all out after devices
show up, use it.  Don't try to create convoluted schemes in the driver
core that are confusing and don't really seem to work.

People have asked for a "priority" of drivers binding to devices for
over a decade now, as "other" operating systems have it.  Turns out, no
one has ever needed it badly enough to actually implement it...

good luck,

greg k-h