lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20141217042345.GC7837@mail.corp.redhat.com>
Date:	Tue, 16 Dec 2014 23:23:45 -0500
From:	Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:	Peter Wu <peter@...ensteyn.nl>
Cc:	Jiri Kosina <jkosina@...e.cz>,
	Nestor Lopez Casado <nlopezcasad@...itech.com>,
	Peter Hutterer <peter.hutterer@...-t.net>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] HID: logitech-hidpp: bail out if wtp_connect fails

On Dec 17 2014 or thereabouts, Peter Wu wrote:
> Sorry for the rapid mail, I forgot to mention something.
> 
> wtp_connect won't work on non-HID++ devices. What about moving it down,
> between the generic routines (reading protocol and name) and
> hidpp_allocate_input? Then the connected parameter can also be dropped.

No, this will not work. wtp_connect sets the device in the raw report
mode. If we call it after hidpp_allocate_input, this will work on the
first connect. Then, if you switch off/on the device, the connect_event
will be called, but the device will not be set in the raw mode.

We really need to unconditionally call wtp_connect at each
connect_event.

> 
> Kind regards,
> Peter
> 
> On Wednesday 17 December 2014 00:33:55 Peter Wu wrote:
> > On Tuesday 16 December 2014 17:06:01 Benjamin Tissoires wrote:
> > > If wtp_connect() fails, that means most of the time that the device has
> > > been disconnected. Subsequent attempts to contact the device will fail
> > > too, so it's simpler to bail out earlier.
> > > 
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c | 15 +++++++++------
> > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index d008d71..c0fb5fe 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -914,24 +914,24 @@ static int wtp_allocate(struct hid_device *hdev, const struct hid_device_id *id)
> > >  	return 0;
> > >  };
> > >  
> > > -static void wtp_connect(struct hid_device *hdev, bool connected)
> > > +static int wtp_connect(struct hid_device *hdev, bool connected)
> > >  {
> > >  	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > >  	struct wtp_data *wd = hidpp->private_data;
> > >  	int ret;
> > >  
> > >  	if (!connected)
> > > -		return;
> > > +		return 0;
> > 
> > "0" is success, what about -1 or -ENODEV here to signal an error? The
> > following line (in hidpp_connect_event) returns on !connected, but that
> > is no reason to return 0 here.

0 is fine here. Maybe I over thought the API, but the connect_event is
sent whenever the device is connected or disconnected.
This allows a subdriver to do things on connect and on disconnect. For
instance, you could delete the input node on disconnect. This is not
something we want though, so the disconnect event is just discarded.

But this disconnect event is not an error, it is just a discarded event,
so returning success is fine.

> > 
> > ("No connection" sounds like an error condition to me as "[wtp_]connect"
> > cannot do something meaningful.)
> > 
> > Whether or not you change the return value is up to you. This patch is
> > Reviewed-by: Peter Wu <peter@...ensteyn.nl>

Thanks for the review!

Cheers,
Benjamin

> > 
> > Kind regards,
> > Peter
> > 
> > >  	if (!wd->x_size) {
> > >  		ret = wtp_get_config(hidpp);
> > >  		if (ret) {
> > >  			hid_err(hdev, "Can not get wtp config: %d\n", ret);
> > > -			return;
> > > +			return ret;
> > >  		}
> > >  	}
> > >  
> > > -	hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index,
> > > +	return hidpp_touchpad_set_raw_report_state(hidpp, wd->mt_feature_index,
> > >  			true, true);
> > >  }
> > >  
> > > @@ -1115,8 +1115,11 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> > >  	struct input_dev *input;
> > >  	char *name, *devm_name;
> > >  
> > > -	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> > > -		wtp_connect(hdev, connected);
> > > +	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> > > +		ret = wtp_connect(hdev, connected);
> > > +		if (ret)
> > > +			return;
> > > +	}
> > >  
> > >  	if (!connected || hidpp->delayed_input)
> > >  		return;
> > > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ