Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'

[Antonios Motakis]

On Fri, Sep 26, 2014 at 5:37 PM, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> 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 <a.motakis@xxxxxxxxxxxxxxxxxxxxxx>
>
> 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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/