Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'
On Fri, Sep 26, 2014 at 5:37 PM, Russell King - ARM Linux
wrote:
>
> On Tue, Sep 23, 2014 at 04:46:06PM +0200, Antonios Motakis wrote:
> > As already demonstrated with PCI [1] and the platform bus [2], a
> > driver_override property in sysfs can be used to bypass the id matching
> > of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
> > device requested by the user.
> >
> > [1]
> > http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
> > [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
> >
> > Signed-off-by: Antonios Motakis
>
> I have to ask why this is even needed in the first place. To take the
> example in [2], what's wrong with:
>
> echo fff51000.ethernet >
> /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
> echo fff51000.ethernet > /sys/bus/platform/drivers/vfio-platform/bind
>
> and similar for AMBA.
>
> All we would need to do is to introduce a way of having a driver accept
> explicit bind requests.
>
> In any case:
>
> > +static ssize_t driver_override_store(struct device *_dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct amba_device *dev = to_amba_device(_dev);
> > + char *driver_override, *old = dev->driver_override, *cp;
> > +
> > + if (count > PATH_MAX)
> > + return -EINVAL;
> > +
> > + driver_override = kstrndup(buf, count, GFP_KERNEL);
> > + if (!driver_override)
> > + return -ENOMEM;
> > +
> > + cp = strchr(driver_override, '\n');
> > + if (cp)
> > + *cp = '\0';
>
> I hope that is not replicated everywhere. This allows up to a page to be
> allocated, even when the first byte may be a newline. This is wasteful.
>
> How about:
>
> if (count > PATH_MAX)
> return -EINVAL;
>
> cp = strnchr(buf, count, '\n');
> if (cp)
> count = cp - buf - 1;
>
> if (count) {
> driver_override = kstrndup(buf, count, GFP_KERNEL);
> if (!driver_override)
> return -ENOMEM;
> } else {
> driver_override = NULL;
> }
>
> kfree(dev->driver_override);
> dev->driver_override = driver_override;
I implemented something along this lines and tested it a bit. The
kernel already splits the input by newlines, and the function is being
called for each one separately; so this change doesn't save any
memory.
>
> Also:
>
> > +static ssize_t driver_override_show(struct device *_dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct amba_device *dev = to_amba_device(_dev);
> > +
> > + return sprintf(buf, "%s\n", dev->driver_override);
> > +}
>
> Do we really want to do a NULL pointer dereference here?
>
> --
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.
--
Antonios Motakis
Virtual Open Systems
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'
On Fri, Sep 26, 2014 at 5:37 PM, Russell King - ARM Linux
wrote:
> On Tue, Sep 23, 2014 at 04:46:06PM +0200, Antonios Motakis wrote:
>> As already demonstrated with PCI [1] and the platform bus [2], a
>> driver_override property in sysfs can be used to bypass the id matching
>> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
>> device requested by the user.
>>
>> [1]
>> http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
>> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
>>
>> Signed-off-by: Antonios Motakis
>
> I have to ask why this is even needed in the first place. To take the
> example in [2], what's wrong with:
>
> echo fff51000.ethernet >
> /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
> echo fff51000.ethernet > /sys/bus/platform/drivers/vfio-platform/bind
>
> and similar for AMBA.
>
> All we would need to do is to introduce a way of having a driver accept
> explicit bind requests.
I don't have strong feelings on whether it should be done one way or
the other, however the approach proposed here is identical to the one
already accepted in mainline for PCI and platform devices. Should we
do something different for AMBA?
>
> In any case:
>
>> +static ssize_t driver_override_store(struct device *_dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct amba_device *dev = to_amba_device(_dev);
>> + char *driver_override, *old = dev->driver_override, *cp;
>> +
>> + if (count > PATH_MAX)
>> + return -EINVAL;
>> +
>> + driver_override = kstrndup(buf, count, GFP_KERNEL);
>> + if (!driver_override)
>> + return -ENOMEM;
>> +
>> + cp = strchr(driver_override, '\n');
>> + if (cp)
>> + *cp = '\0';
>
> I hope that is not replicated everywhere. This allows up to a page to be
> allocated, even when the first byte may be a newline. This is wasteful.
>
> How about:
>
> if (count > PATH_MAX)
> return -EINVAL;
>
> cp = strnchr(buf, count, '\n');
> if (cp)
> count = cp - buf - 1;
>
> if (count) {
> driver_override = kstrndup(buf, count, GFP_KERNEL);
> if (!driver_override)
> return -ENOMEM;
> } else {
> driver_override = NULL;
> }
>
> kfree(dev->driver_override);
> dev->driver_override = driver_override;
Ack.
>
> Also:
>
>> +static ssize_t driver_override_show(struct device *_dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct amba_device *dev = to_amba_device(_dev);
>> +
>> + return sprintf(buf, "%s\n", dev->driver_override);
>> +}
>
> Do we really want to do a NULL pointer dereference here?
I'll add a check here.
Thanks
--
Antonios Motakis
Virtual Open Systems
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'
On Tue, Sep 23, 2014 at 04:46:06PM +0200, Antonios Motakis wrote:
> As already demonstrated with PCI [1] and the platform bus [2], a
> driver_override property in sysfs can be used to bypass the id matching
> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
> device requested by the user.
>
> [1]
> http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
>
> Signed-off-by: Antonios Motakis
I have to ask why this is even needed in the first place. To take the
example in [2], what's wrong with:
echo fff51000.ethernet >
/sys/bus/platform/devices/fff51000.ethernet/driver/unbind
echo fff51000.ethernet > /sys/bus/platform/drivers/vfio-platform/bind
and similar for AMBA.
All we would need to do is to introduce a way of having a driver accept
explicit bind requests.
In any case:
> +static ssize_t driver_override_store(struct device *_dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct amba_device *dev = to_amba_device(_dev);
> + char *driver_override, *old = dev->driver_override, *cp;
> +
> + if (count > PATH_MAX)
> + return -EINVAL;
> +
> + driver_override = kstrndup(buf, count, GFP_KERNEL);
> + if (!driver_override)
> + return -ENOMEM;
> +
> + cp = strchr(driver_override, '\n');
> + if (cp)
> + *cp = '\0';
I hope that is not replicated everywhere. This allows up to a page to be
allocated, even when the first byte may be a newline. This is wasteful.
How about:
if (count > PATH_MAX)
return -EINVAL;
cp = strnchr(buf, count, '\n');
if (cp)
count = cp - buf - 1;
if (count) {
driver_override = kstrndup(buf, count, GFP_KERNEL);
if (!driver_override)
return -ENOMEM;
} else {
driver_override = NULL;
}
kfree(dev->driver_override);
dev->driver_override = driver_override;
Also:
> +static ssize_t driver_override_show(struct device *_dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct amba_device *dev = to_amba_device(_dev);
> +
> + return sprintf(buf, "%s\n", dev->driver_override);
> +}
Do we really want to do a NULL pointer dereference here?
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'
Ok, they will be posted separately.
Thanks
On Wed, Sep 24, 2014 at 12:45 AM, Alex Williamson
wrote:
> On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
>> As already demonstrated with PCI [1] and the platform bus [2], a
>> driver_override property in sysfs can be used to bypass the id matching
>> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
>> device requested by the user.
>>
>> [1]
>> http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
>> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
>>
>> Signed-off-by: Antonios Motakis
>> ---
>> drivers/amba/bus.c | 44
>> include/linux/amba/bus.h | 1 +
>> 2 files changed, 45 insertions(+)
>
> Why are 7 & 8 being dragged into this series? Combine them into a
> single patch and post it separately.
>
>>
>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>> index 3cf61a1..473177c 100644
>> --- a/drivers/amba/bus.c
>> +++ b/drivers/amba/bus.c
>> @@ -17,6 +17,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>>
>> @@ -42,6 +43,10 @@ static int amba_match(struct device *dev, struct
>> device_driver *drv)
>> struct amba_device *pcdev = to_amba_device(dev);
>> struct amba_driver *pcdrv = to_amba_driver(drv);
>>
>> + /* When driver_override is set, only bind to the matching driver */
>> + if (pcdev->driver_override)
>> + return !strcmp(pcdev->driver_override, drv->name);
>> +
>> return amba_lookup(pcdrv->id_table, pcdev) != NULL;
>> }
>>
>> @@ -58,6 +63,44 @@ static int amba_uevent(struct device *dev, struct
>> kobj_uevent_env *env)
>> return retval;
>> }
>>
>> +static ssize_t driver_override_show(struct device *_dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct amba_device *dev = to_amba_device(_dev);
>> +
>> + return sprintf(buf, "%s\n", dev->driver_override);
>> +}
>> +
>> +static ssize_t driver_override_store(struct device *_dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct amba_device *dev = to_amba_device(_dev);
>> + char *driver_override, *old = dev->driver_override, *cp;
>> +
>> + if (count > PATH_MAX)
>> + return -EINVAL;
>> +
>> + driver_override = kstrndup(buf, count, GFP_KERNEL);
>> + if (!driver_override)
>> + return -ENOMEM;
>> +
>> + cp = strchr(driver_override, '\n');
>> + if (cp)
>> + *cp = '\0';
>> +
>> + if (strlen(driver_override)) {
>> + dev->driver_override = driver_override;
>> + } else {
>> +kfree(driver_override);
>> +dev->driver_override = NULL;
>> + }
>> +
>> + kfree(old);
>> +
>> + return count;
>> +}
>> +
>> #define amba_attr_func(name,fmt,arg...)
>> \
>> static ssize_t name##_show(struct device *_dev,
>> \
>> struct device_attribute *attr, char *buf)\
>> @@ -80,6 +123,7 @@ amba_attr_func(resource, "\t%016llx\t%016llx\t%016lx\n",
>> static struct device_attribute amba_dev_attrs[] = {
>> __ATTR_RO(id),
>> __ATTR_RO(resource),
>> + __ATTR_RW(driver_override),
>> __ATTR_NULL,
>> };
>>
>> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
>> index fdd7e1b..7c011e7 100644
>> --- a/include/linux/amba/bus.h
>> +++ b/include/linux/amba/bus.h
>> @@ -32,6 +32,7 @@ struct amba_device {
>> struct clk *pclk;
>> unsigned intperiphid;
>> unsigned intirq[AMBA_NR_IRQS];
>> + char*driver_override;
>> };
>>
>> struct amba_driver {
>
>
>
--
Antonios Motakis
Virtual Open Systems
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'
On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> As already demonstrated with PCI [1] and the platform bus [2], a
> driver_override property in sysfs can be used to bypass the id matching
> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
> device requested by the user.
>
> [1]
> http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
>
> Signed-off-by: Antonios Motakis
> ---
> drivers/amba/bus.c | 44
> include/linux/amba/bus.h | 1 +
> 2 files changed, 45 insertions(+)
Why are 7 & 8 being dragged into this series? Combine them into a
single patch and post it separately.
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 3cf61a1..473177c 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -17,6 +17,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -42,6 +43,10 @@ static int amba_match(struct device *dev, struct
> device_driver *drv)
> struct amba_device *pcdev = to_amba_device(dev);
> struct amba_driver *pcdrv = to_amba_driver(drv);
>
> + /* When driver_override is set, only bind to the matching driver */
> + if (pcdev->driver_override)
> + return !strcmp(pcdev->driver_override, drv->name);
> +
> return amba_lookup(pcdrv->id_table, pcdev) != NULL;
> }
>
> @@ -58,6 +63,44 @@ static int amba_uevent(struct device *dev, struct
> kobj_uevent_env *env)
> return retval;
> }
>
> +static ssize_t driver_override_show(struct device *_dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct amba_device *dev = to_amba_device(_dev);
> +
> + return sprintf(buf, "%s\n", dev->driver_override);
> +}
> +
> +static ssize_t driver_override_store(struct device *_dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct amba_device *dev = to_amba_device(_dev);
> + char *driver_override, *old = dev->driver_override, *cp;
> +
> + if (count > PATH_MAX)
> + return -EINVAL;
> +
> + driver_override = kstrndup(buf, count, GFP_KERNEL);
> + if (!driver_override)
> + return -ENOMEM;
> +
> + cp = strchr(driver_override, '\n');
> + if (cp)
> + *cp = '\0';
> +
> + if (strlen(driver_override)) {
> + dev->driver_override = driver_override;
> + } else {
> +kfree(driver_override);
> +dev->driver_override = NULL;
> + }
> +
> + kfree(old);
> +
> + return count;
> +}
> +
> #define amba_attr_func(name,fmt,arg...)
> \
> static ssize_t name##_show(struct device *_dev,
> \
> struct device_attribute *attr, char *buf)\
> @@ -80,6 +123,7 @@ amba_attr_func(resource, "\t%016llx\t%016llx\t%016lx\n",
> static struct device_attribute amba_dev_attrs[] = {
> __ATTR_RO(id),
> __ATTR_RO(resource),
> + __ATTR_RW(driver_override),
> __ATTR_NULL,
> };
>
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index fdd7e1b..7c011e7 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -32,6 +32,7 @@ struct amba_device {
> struct clk *pclk;
> unsigned intperiphid;
> unsigned intirq[AMBA_NR_IRQS];
> + char*driver_override;
> };
>
> struct amba_driver {
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'
As already demonstrated with PCI [1] and the platform bus [2], a
driver_override property in sysfs can be used to bypass the id matching
of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
device requested by the user.
[1]
http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
[2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
Signed-off-by: Antonios Motakis
---
drivers/amba/bus.c | 44
include/linux/amba/bus.h | 1 +
2 files changed, 45 insertions(+)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 3cf61a1..473177c 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
@@ -42,6 +43,10 @@ static int amba_match(struct device *dev, struct
device_driver *drv)
struct amba_device *pcdev = to_amba_device(dev);
struct amba_driver *pcdrv = to_amba_driver(drv);
+ /* When driver_override is set, only bind to the matching driver */
+ if (pcdev->driver_override)
+ return !strcmp(pcdev->driver_override, drv->name);
+
return amba_lookup(pcdrv->id_table, pcdev) != NULL;
}
@@ -58,6 +63,44 @@ static int amba_uevent(struct device *dev, struct
kobj_uevent_env *env)
return retval;
}
+static ssize_t driver_override_show(struct device *_dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct amba_device *dev = to_amba_device(_dev);
+
+ return sprintf(buf, "%s\n", dev->driver_override);
+}
+
+static ssize_t driver_override_store(struct device *_dev,
+struct device_attribute *attr,
+const char *buf, size_t count)
+{
+ struct amba_device *dev = to_amba_device(_dev);
+ char *driver_override, *old = dev->driver_override, *cp;
+
+ if (count > PATH_MAX)
+ return -EINVAL;
+
+ driver_override = kstrndup(buf, count, GFP_KERNEL);
+ if (!driver_override)
+ return -ENOMEM;
+
+ cp = strchr(driver_override, '\n');
+ if (cp)
+ *cp = '\0';
+
+ if (strlen(driver_override)) {
+ dev->driver_override = driver_override;
+ } else {
+ kfree(driver_override);
+ dev->driver_override = NULL;
+ }
+
+ kfree(old);
+
+ return count;
+}
+
#define amba_attr_func(name,fmt,arg...)
\
static ssize_t name##_show(struct device *_dev,
\
struct device_attribute *attr, char *buf)\
@@ -80,6 +123,7 @@ amba_attr_func(resource, "\t%016llx\t%016llx\t%016lx\n",
static struct device_attribute amba_dev_attrs[] = {
__ATTR_RO(id),
__ATTR_RO(resource),
+ __ATTR_RW(driver_override),
__ATTR_NULL,
};
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index fdd7e1b..7c011e7 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -32,6 +32,7 @@ struct amba_device {
struct clk *pclk;
unsigned intperiphid;
unsigned intirq[AMBA_NR_IRQS];
+ char*driver_override;
};
struct amba_driver {
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
