diff mbox series

[v2] HID: logitech-dj: issue udev change event on device connection

Message ID 20200318192721.390630-1-lains@archlinux.org (mailing list archive)
State New
Delegated to: Jiri Kosina
Headers show
Series [v2] HID: logitech-dj: issue udev change event on device connection | expand

Commit Message

Filipe Laíns March 18, 2020, 7:27 p.m. UTC
As discussed in the mailing list:

> Right now the hid-logitech-dj driver will export one node for each
> connected device, even when the device is not connected. That causes
> some trouble because in userspace we don't have have any way to know if
> the device is connected or not, so when we try to communicate, if the
> device is disconnected it will fail.

The solution reached to solve this issue is to trigger an udev change
event when the device connects, this way userspace can just wait on
those connections instead of trying to ping the device.

Signed-off-by: Filipe Laíns <lains@archlinux.org>

---

v2:
  - Issue udev change event on the connected hid device, not on the
  receiver

---
 drivers/hid/hid-logitech-dj.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Bastien Nocera March 24, 2020, 10:20 a.m. UTC | #1
On Wed, 2020-03-18 at 19:27 +0000, Filipe Laíns wrote:
> As discussed in the mailing list:
> 
> > Right now the hid-logitech-dj driver will export one node for each
> > connected device, even when the device is not connected. That
> > causes
> > some trouble because in userspace we don't have have any way to
> > know if
> > the device is connected or not, so when we try to communicate, if
> > the
> > device is disconnected it will fail.

Why is it a problem that user-space communication fails? Note that
sending a signal without any way to fetch the state means that it's
always going to be racy.

> The solution reached to solve this issue is to trigger an udev change
> event when the device connects, this way userspace can just wait on
> those connections instead of trying to ping the device.
> 
> Signed-off-by: Filipe Laíns <lains@archlinux.org>
> 
> ---
> 
> v2:
>   - Issue udev change event on the connected hid device, not on the
>   receiver
> 
> ---
>  drivers/hid/hid-logitech-dj.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> logitech-dj.c
> index 48dff5d6b605..282e57dd467d 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct hid_device
> *hdev,
>  {
>  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
>  	struct dj_report *dj_report = (struct dj_report *) data;
> +	struct dj_device *dj_dev;
>  	unsigned long flags;
>  
>  	/*
> @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct hid_device
> *hdev,
>  
>  	spin_lock_irqsave(&djrcv_dev->lock, flags);
>  
> -	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> +	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> +
> +	if (!dj_dev) {
>  		/* received an event for an unknown device, bail out */
>  		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
>  		goto out;
> @@ -1464,6 +1467,8 @@ static int logi_dj_dj_event(struct hid_device
> *hdev,
>  		if (dj_report-
> >report_params[CONNECTION_STATUS_PARAM_STATUS] ==
>  		    STATUS_LINKLOSS) {
>  			logi_dj_recv_forward_null_report(djrcv_dev,
> dj_report);
> +		} else {
> +			kobject_uevent(&dj_dev->hdev->dev.kobj,
> KOBJ_CHANGE);
>  		}
>  		break;
>  	default:
Filipe Laíns March 24, 2020, 1:46 p.m. UTC | #2
On Tue, 2020-03-24 at 11:20 +0100, Bastien Nocera wrote:
> On Wed, 2020-03-18 at 19:27 +0000, Filipe Laíns wrote:
> > As discussed in the mailing list:
> > 
> > > Right now the hid-logitech-dj driver will export one node for each
> > > connected device, even when the device is not connected. That
> > > causes
> > > some trouble because in userspace we don't have have any way to
> > > know if
> > > the device is connected or not, so when we try to communicate, if
> > > the
> > > device is disconnected it will fail.
> 
> Why is it a problem that user-space communication fails? Note that
> sending a signal without any way to fetch the state means that it's
> always going to be racy.

It failing is not the problem. The problem is knowing when the device
is available again. Right now the only way to do that is to listen for
events or periodically ping it.

We want to only export the HID++ hidraw node when the device is
available but that will take a while. We will have to test and sync up
userspace. I also want to write tests for the driver before, to make
sure there are no regressions. We had a thread discussing this, IIRC
you were in CC.

> > The solution reached to solve this issue is to trigger an udev change
> > event when the device connects, this way userspace can just wait on
> > those connections instead of trying to ping the device.
> > 
> > Signed-off-by: Filipe Laíns <lains@archlinux.org>
> > 
> > ---
> > 
> > v2:
> >   - Issue udev change event on the connected hid device, not on the
> >   receiver
> > 
> > ---
> >  drivers/hid/hid-logitech-dj.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> > logitech-dj.c
> > index 48dff5d6b605..282e57dd467d 100644
> > --- a/drivers/hid/hid-logitech-dj.c
> > +++ b/drivers/hid/hid-logitech-dj.c
> > @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct hid_device
> > *hdev,
> >  {
> >  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
> >  	struct dj_report *dj_report = (struct dj_report *) data;
> > +	struct dj_device *dj_dev;
> >  	unsigned long flags;
> >  
> >  	/*
> > @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct hid_device
> > *hdev,
> >  
> >  	spin_lock_irqsave(&djrcv_dev->lock, flags);
> >  
> > -	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> > +	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> > +
> > +	if (!dj_dev) {
> >  		/* received an event for an unknown device, bail out */
> >  		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> >  		goto out;
> > @@ -1464,6 +1467,8 @@ static int logi_dj_dj_event(struct hid_device
> > *hdev,
> >  		if (dj_report-
> > > report_params[CONNECTION_STATUS_PARAM_STATUS] ==
> >  		    STATUS_LINKLOSS) {
> >  			logi_dj_recv_forward_null_report(djrcv_dev,
> > dj_report);
> > +		} else {
> > +			kobject_uevent(&dj_dev->hdev->dev.kobj,
> > KOBJ_CHANGE);
> >  		}
> >  		break;
> >  	default:
Bastien Nocera March 24, 2020, 2:03 p.m. UTC | #3
On Tue, 2020-03-24 at 13:46 +0000, Filipe Laíns wrote:
> On Tue, 2020-03-24 at 11:20 +0100, Bastien Nocera wrote:
> > On Wed, 2020-03-18 at 19:27 +0000, Filipe Laíns wrote:
> > > As discussed in the mailing list:
> > > 
> > > > Right now the hid-logitech-dj driver will export one node for
> > > > each
> > > > connected device, even when the device is not connected. That
> > > > causes
> > > > some trouble because in userspace we don't have have any way to
> > > > know if
> > > > the device is connected or not, so when we try to communicate,
> > > > if
> > > > the
> > > > device is disconnected it will fail.
> > 
> > Why is it a problem that user-space communication fails? Note that
> > sending a signal without any way to fetch the state means that it's
> > always going to be racy.
> 
> It failing is not the problem. The problem is knowing when the device
> is available again. Right now the only way to do that is to listen
> for
> events or periodically ping it.
> 
> We want to only export the HID++ hidraw node when the device is
> available but that will take a while. We will have to test and sync
> up
> userspace. I also want to write tests for the driver before, to make
> sure there are no regressions. We had a thread discussing this, IIRC
> you were in CC.

If I need to remember some old thread to know what we're talking about,
then that means that the commit message is probably not good enough...

Please add some links to the relevant discussion on bug forums if
there's interesting data there too.


> 
> > > The solution reached to solve this issue is to trigger an udev
> > > change
> > > event when the device connects, this way userspace can just wait
> > > on
> > > those connections instead of trying to ping the device.
> > > 
> > > Signed-off-by: Filipe Laíns <lains@archlinux.org>
> > > 
> > > ---
> > > 
> > > v2:
> > >   - Issue udev change event on the connected hid device, not on
> > > the
> > >   receiver
> > > 
> > > ---
> > >  drivers/hid/hid-logitech-dj.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> > > logitech-dj.c
> > > index 48dff5d6b605..282e57dd467d 100644
> > > --- a/drivers/hid/hid-logitech-dj.c
> > > +++ b/drivers/hid/hid-logitech-dj.c
> > > @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct
> > > hid_device
> > > *hdev,
> > >  {
> > >  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
> > >  	struct dj_report *dj_report = (struct dj_report *) data;
> > > +	struct dj_device *dj_dev;
> > >  	unsigned long flags;
> > >  
> > >  	/*
> > > @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct
> > > hid_device
> > > *hdev,
> > >  
> > >  	spin_lock_irqsave(&djrcv_dev->lock, flags);
> > >  
> > > -	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> > > +	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> > > +
> > > +	if (!dj_dev) {
> > >  		/* received an event for an unknown device, bail out */
> > >  		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> > >  		goto out;
> > > @@ -1464,6 +1467,8 @@ static int logi_dj_dj_event(struct
> > > hid_device
> > > *hdev,
> > >  		if (dj_report-
> > > > report_params[CONNECTION_STATUS_PARAM_STATUS] ==
> > >  		    STATUS_LINKLOSS) {
> > >  			logi_dj_recv_forward_null_report(djrcv_dev,
> > > dj_report);
> > > +		} else {
> > > +			kobject_uevent(&dj_dev->hdev->dev.kobj,
> > > KOBJ_CHANGE);
> > >  		}
> > >  		break;
> > >  	default:
Filipe Laíns March 24, 2020, 2:10 p.m. UTC | #4
On Tue, 2020-03-24 at 15:03 +0100, Bastien Nocera wrote:
> On Tue, 2020-03-24 at 13:46 +0000, Filipe Laíns wrote:
> > On Tue, 2020-03-24 at 11:20 +0100, Bastien Nocera wrote:
> > > On Wed, 2020-03-18 at 19:27 +0000, Filipe Laíns wrote:
> > > > As discussed in the mailing list:
> > > > 
> > > > > Right now the hid-logitech-dj driver will export one node for
> > > > > each
> > > > > connected device, even when the device is not connected. That
> > > > > causes
> > > > > some trouble because in userspace we don't have have any way to
> > > > > know if
> > > > > the device is connected or not, so when we try to communicate,
> > > > > if
> > > > > the
> > > > > device is disconnected it will fail.
> > > 
> > > Why is it a problem that user-space communication fails? Note that
> > > sending a signal without any way to fetch the state means that it's
> > > always going to be racy.
> > 
> > It failing is not the problem. The problem is knowing when the device
> > is available again. Right now the only way to do that is to listen
> > for
> > events or periodically ping it.
> > 
> > We want to only export the HID++ hidraw node when the device is
> > available but that will take a while. We will have to test and sync
> > up
> > userspace. I also want to write tests for the driver before, to make
> > sure there are no regressions. We had a thread discussing this, IIRC
> > you were in CC.
> 
> If I need to remember some old thread to know what we're talking about,
> then that means that the commit message is probably not good enough...

Should I put in the commit message what is planned next?

> Please add some links to the relevant discussion on bug forums if
> there's interesting data there too.

You can find the discussion in [1][2].

[1] https://www.spinics.net/lists/linux-input/thrd2.html#65615
[2] https://www.spinics.net/lists/linux-input/msg65615.html

> > > > The solution reached to solve this issue is to trigger an udev
> > > > change
> > > > event when the device connects, this way userspace can just wait
> > > > on
> > > > those connections instead of trying to ping the device.
> > > > 
> > > > Signed-off-by: Filipe Laíns <lains@archlinux.org>
> > > > 
> > > > ---
> > > > 
> > > > v2:
> > > >   - Issue udev change event on the connected hid device, not on
> > > > the
> > > >   receiver
> > > > 
> > > > ---
> > > >  drivers/hid/hid-logitech-dj.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> > > > logitech-dj.c
> > > > index 48dff5d6b605..282e57dd467d 100644
> > > > --- a/drivers/hid/hid-logitech-dj.c
> > > > +++ b/drivers/hid/hid-logitech-dj.c
> > > > @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct
> > > > hid_device
> > > > *hdev,
> > > >  {
> > > >  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
> > > >  	struct dj_report *dj_report = (struct dj_report *) data;
> > > > +	struct dj_device *dj_dev;
> > > >  	unsigned long flags;
> > > >  
> > > >  	/*
> > > > @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct
> > > > hid_device
> > > > *hdev,
> > > >  
> > > >  	spin_lock_irqsave(&djrcv_dev->lock, flags);
> > > >  
> > > > -	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> > > > +	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
> > > > +
> > > > +	if (!dj_dev) {
> > > >  		/* received an event for an unknown device, bail out */
> > > >  		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> > > >  		goto out;
> > > > @@ -1464,6 +1467,8 @@ static int logi_dj_dj_event(struct
> > > > hid_device
> > > > *hdev,
> > > >  		if (dj_report-
> > > > > report_params[CONNECTION_STATUS_PARAM_STATUS] ==
> > > >  		    STATUS_LINKLOSS) {
> > > >  			logi_dj_recv_forward_null_report(djrcv_dev,
> > > > dj_report);
> > > > +		} else {
> > > > +			kobject_uevent(&dj_dev->hdev->dev.kobj,
> > > > KOBJ_CHANGE);
> > > >  		}
> > > >  		break;
> > > >  	default:
Bastien Nocera March 24, 2020, 2:24 p.m. UTC | #5
On Tue, 2020-03-24 at 14:10 +0000, Filipe Laíns wrote:
> On Tue, 2020-03-24 at 15:03 +0100, Bastien Nocera wrote:
> > On Tue, 2020-03-24 at 13:46 +0000, Filipe Laíns wrote:
> > > On Tue, 2020-03-24 at 11:20 +0100, Bastien Nocera wrote:
> > > > On Wed, 2020-03-18 at 19:27 +0000, Filipe Laíns wrote:
> > > > > As discussed in the mailing list:
> > > > > 
> > > > > > Right now the hid-logitech-dj driver will export one node
> > > > > > for
> > > > > > each
> > > > > > connected device, even when the device is not connected.
> > > > > > That
> > > > > > causes
> > > > > > some trouble because in userspace we don't have have any
> > > > > > way to
> > > > > > know if
> > > > > > the device is connected or not, so when we try to
> > > > > > communicate,
> > > > > > if
> > > > > > the
> > > > > > device is disconnected it will fail.
> > > > 
> > > > Why is it a problem that user-space communication fails? Note
> > > > that
> > > > sending a signal without any way to fetch the state means that
> > > > it's
> > > > always going to be racy.
> > > 
> > > It failing is not the problem. The problem is knowing when the
> > > device
> > > is available again. Right now the only way to do that is to
> > > listen
> > > for
> > > events or periodically ping it.
> > > 
> > > We want to only export the HID++ hidraw node when the device is
> > > available but that will take a while. We will have to test and
> > > sync
> > > up
> > > userspace. I also want to write tests for the driver before, to
> > > make
> > > sure there are no regressions. We had a thread discussing this,
> > > IIRC
> > > you were in CC.
> > 
> > If I need to remember some old thread to know what we're talking
> > about,
> > then that means that the commit message is probably not good
> > enough...
> 
> Should I put in the commit message what is planned next?

You should put in everything that explains why this change is needed,
and how user-space is supposed to use that information. Nobody should
have to refer back to the mailing-list thread to find this out.

> > Please add some links to the relevant discussion on bug forums if
> > there's interesting data there too.
> 
> You can find the discussion in [1][2].
> 
> [1] https://www.spinics.net/lists/linux-input/thrd2.html#65615
> [2] https://www.spinics.net/lists/linux-input/msg65615.html

I was in CC: because "might be maintaining one of the affected
userspace stacks", but I don't know what that stack might be for me. Is
it for upower (which I only work on seldom and definitely don't
maintain), or something else?

> > > > > The solution reached to solve this issue is to trigger an
> > > > > udev
> > > > > change
> > > > > event when the device connects, this way userspace can just
> > > > > wait
> > > > > on
> > > > > those connections instead of trying to ping the device.
> > > > > 
> > > > > Signed-off-by: Filipe Laíns <lains@archlinux.org>
> > > > > 
> > > > > ---
> > > > > 
> > > > > v2:
> > > > >   - Issue udev change event on the connected hid device, not
> > > > > on
> > > > > the
> > > > >   receiver
> > > > > 
> > > > > ---
> > > > >  drivers/hid/hid-logitech-dj.c | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-
> > > > > logitech-dj.c
> > > > > index 48dff5d6b605..282e57dd467d 100644
> > > > > --- a/drivers/hid/hid-logitech-dj.c
> > > > > +++ b/drivers/hid/hid-logitech-dj.c
> > > > > @@ -1412,6 +1412,7 @@ static int logi_dj_dj_event(struct
> > > > > hid_device
> > > > > *hdev,
> > > > >  {
> > > > >  	struct dj_receiver_dev *djrcv_dev =
> > > > > hid_get_drvdata(hdev);
> > > > >  	struct dj_report *dj_report = (struct dj_report *)
> > > > > data;
> > > > > +	struct dj_device *dj_dev;
> > > > >  	unsigned long flags;
> > > > >  
> > > > >  	/*
> > > > > @@ -1447,7 +1448,9 @@ static int logi_dj_dj_event(struct
> > > > > hid_device
> > > > > *hdev,
> > > > >  
> > > > >  	spin_lock_irqsave(&djrcv_dev->lock, flags);
> > > > >  
> > > > > -	if (!djrcv_dev->paired_dj_devices[dj_report-
> > > > > >device_index]) {
> > > > > +	dj_dev = djrcv_dev->paired_dj_devices[dj_report-
> > > > > >device_index];
> > > > > +
> > > > > +	if (!dj_dev) {
> > > > >  		/* received an event for an unknown device,
> > > > > bail out */
> > > > >  		logi_dj_recv_queue_notification(djrcv_dev,
> > > > > dj_report);
> > > > >  		goto out;
> > > > > @@ -1464,6 +1467,8 @@ static int logi_dj_dj_event(struct
> > > > > hid_device
> > > > > *hdev,
> > > > >  		if (dj_report-
> > > > > > report_params[CONNECTION_STATUS_PARAM_STATUS] ==
> > > > >  		    STATUS_LINKLOSS) {
> > > > >  			logi_dj_recv_forward_null_report(djrcv_
> > > > > dev,
> > > > > dj_report);
> > > > > +		} else {
> > > > > +			kobject_uevent(&dj_dev->hdev->dev.kobj,
> > > > > KOBJ_CHANGE);
> > > > >  		}
> > > > >  		break;
> > > > >  	default:
diff mbox series

Patch

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 48dff5d6b605..282e57dd467d 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1412,6 +1412,7 @@  static int logi_dj_dj_event(struct hid_device *hdev,
 {
 	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
 	struct dj_report *dj_report = (struct dj_report *) data;
+	struct dj_device *dj_dev;
 	unsigned long flags;
 
 	/*
@@ -1447,7 +1448,9 @@  static int logi_dj_dj_event(struct hid_device *hdev,
 
 	spin_lock_irqsave(&djrcv_dev->lock, flags);
 
-	if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
+	dj_dev = djrcv_dev->paired_dj_devices[dj_report->device_index];
+
+	if (!dj_dev) {
 		/* received an event for an unknown device, bail out */
 		logi_dj_recv_queue_notification(djrcv_dev, dj_report);
 		goto out;
@@ -1464,6 +1467,8 @@  static int logi_dj_dj_event(struct hid_device *hdev,
 		if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] ==
 		    STATUS_LINKLOSS) {
 			logi_dj_recv_forward_null_report(djrcv_dev, dj_report);
+		} else {
+			kobject_uevent(&dj_dev->hdev->dev.kobj, KOBJ_CHANGE);
 		}
 		break;
 	default: