Re: [PATCH] avctp: Set more descriptive name for uinput device

[Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>]

On Wed, Nov 27, 2019 at 8:07 AM Pali Rohár <pali.rohar@xxxxxxxxx> wrote:
>
> Hi!
>
> On Wednesday 27 November 2019 17:50:40 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> >
> > On Tue, Nov 26, 2019 at 11:31 PM Pali Rohár <pali.rohar@xxxxxxxxx> wrote:
> > >
> > > ---
> > >  profiles/audio/avctp.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
> > > index 70a3e40b2..ae53587ad 100644
> > > --- a/profiles/audio/avctp.c
> > > +++ b/profiles/audio/avctp.c
> > > @@ -1181,8 +1181,7 @@ static int uinput_create(char *name)
> > >         }
> > >
> > >         memset(&dev, 0, sizeof(dev));
> > > -       if (name)
> > > -               strncpy(dev.name, name, UINPUT_MAX_NAME_SIZE - 1);
> > > +       snprintf(dev.name, sizeof(dev.name), "Bluetooth Audio/Video Remote Control%s%s", name ? " " : "", name ? name : "");
> > >
> > >         dev.id.bustype = BUS_BLUETOOTH;
> > >         dev.id.vendor  = 0x0000;
> > > --
> > > 2.11.0
> >
> > It is already setting a bustype to BUS_BLUETOOTH why do you need to
> > make it more specific,
>
> Because more tools shows only device name. Also in kernel dmesg is only
> device name. And MAC address is really something which does not say
> anything about device...
>
> For this reason also older input devices (implemented in kernel) exports
> more descriptive names, e.g. "AT Translated Set 2 keyboard" or "ETPS/2
> Elantech TrackPoint" or "Integrated IR Camera".
>
> > btw it needs to be limited to UINPUT_MAX_NAME_SIZE.
>
> Is not this implicitly defined by sizeof()?
>
> > Id say if we want to make it declare the connection type
>
> Yes, connection type would be really useful.
>
> Now I'm playing with exporting "button press even" from HSP profile via
> uinput device too and therefore connection type should be there.
>
> > that probably something that needs to be encoded in
> > the bus itself, like BUS_BLUETOOTH_AVCTP, BUS_BLUETOOTH_HOG, etc.
>
> But bus type is kernel API and there is no such AVCTP or HOG. So we need
> to stick with BUS_BLUETOOH.
>
> Other kernel devices also put protocol information into name (e.g.
> touchpad / trackpoints), so it is ideal place also for bluetooth
> devices.
>
> And probably most userspace devices are mapped to BUS_VIRTUAL. There
> are no subtypes.
>
> --
> Pali Rohár
> pali.rohar@xxxxxxxxx

Wouldn't it be better to use the device name for uinput rather than the address?

In `init_uinput` (avctp.c), uinput_create is called with
device_get_address instead of device_get_name. It would probably be
better to use the device name for the uinput and set the device
address as the `uniq` attribute (as it is done for /dev/uhid for HID
devices).

So in the following, replace ATTR{name} with something like "Some
Company Headphones", ATTR{uniq} = "33:33:33:ff:ff:ff" and ATTR{phys}
with the controller's address.

$ udevadm info -a -p /sys/devices/virtual/input/input21/
...
  looking at device '/devices/virtual/input/input21':
    KERNEL=="input21"
    SUBSYSTEM=="input"
    DRIVER==""
    ATTR{inhibited}=="0"
    ATTR{name}=="33:33:33:FF:FF:FF"
    ATTR{phys}==""
    ATTR{properties}=="0"
    ATTR{uniq}==""

This is what uhid looks like in comparison:

$ udevadm info -a -p /sys/class/misc/uhid/0005\:046D\:B33B.0002/input/input18/

  looking at device
'/devices/virtual/misc/uhid/0005:046D:B33B.0002/input/input18':
    KERNEL=="input17"
    SUBSYSTEM=="input"
    DRIVER==""
    ATTR{inhibited}=="0"
    ATTR{name}=="Keyboard K780 Keyboard"
    ATTR{phys}=="ab:cd:ef:12:34:56"
    ATTR{properties}=="0"
    ATTR{uniq}=="33:33:33:33:ff:ff"


Abhishek