from:"Mazin Rezk"

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

2020-07-27 Thread Mazin Rezk

On Monday, July 27, 2020 7:42 PM, Mazin Rezk  wrote:

> On Monday, July 27, 2020 5:32 PM, Daniel Vetter  wrote:
>
> > On Mon, Jul 27, 2020 at 11:11 PM Mazin Rezk  wrote:
> > >
> > > On Monday, July 27, 2020 4:29 PM, Daniel Vetter  wrote:
> > >
> > > > On Mon, Jul 27, 2020 at 9:28 PM Christian König
> > > >  wrote:
> > > > >
> > > > > Am 27.07.20 um 16:05 schrieb Kazlauskas, Nicholas:
> > > > > > On 2020-07-27 9:39 a.m., Christian König wrote:
> > > > > >> Am 27.07.20 um 07:40 schrieb Mazin Rezk:
> > > > > >>> This patch fixes a race condition that causes a use-after-free 
> > > > > >>> during
> > > > > >>> amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking
> > > > > >>> commits
> > > > > >>> are requested and the second one finishes before the first.
> > > > > >>> Essentially,
> > > > > >>> this bug occurs when the following sequence of events happens:
> > > > > >>>
> > > > > >>> 1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
> > > > > >>> deferred to the workqueue.
> > > > > >>>
> > > > > >>> 2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
> > > > > >>> deferred to the workqueue.
> > > > > >>>
> > > > > >>> 3. Commit #2 starts before commit #1, dm_state #1 is used in the
> > > > > >>> commit_tail and commit #2 completes, freeing dm_state #1.
> > > > > >>>
> > > > > >>> 4. Commit #1 starts after commit #2 completes, uses the freed 
> > > > > >>> dm_state
> > > > > >>> 1 and dereferences a freelist pointer while setting the context.
> > > > > >>
> > > > > >> Well I only have a one mile high view on this, but why don't you 
> > > > > >> let
> > > > > >> the work items execute in order?
> > > > > >>
> > > > > >> That would be better anyway cause this way we don't trigger a cache
> > > > > >> line ping pong between CPUs.
> > > > > >>
> > > > > >> Christian.
> > > > > >
> > > > > > We use the DRM helpers for managing drm_atomic_commit_state and 
> > > > > > those
> > > > > > helpers internally push non-blocking commit work into the system
> > > > > > unbound work queue.
> > > > >
> > > > > Mhm, well if you send those helper atomic commits in the order A,B and
> > > > > they execute it in the order B,A I would call that a bug :)
> > > >
> > > > The way it works is it pushes all commits into unbound work queue, but
> > > > then forces serialization as needed. We do _not_ want e.g. updates on
> > > > different CRTC to be serialized, that would result in lots of judder.
> > > > And hw is funny enough that there's all kinds of dependencies.
> > > >
> > > > The way you force synchronization is by adding other CRTC state
> > > > objects. So if DC is busted and can only handle a single update per
> > > > work item, then I guess you always need all CRTC states and everything
> > > > will be run in order. But that also totally kills modern multi-screen
> > > > compositors. Xorg isn't modern, just in case that's not clear :-)
> > > >
> > > > Lucking at the code it seems like you indeed have only a single dm
> > > > state, so yeah global sync is what you'll need as immediate fix, and
> > > > then maybe fix up DM to not be quite so silly ... or at least only do
> > > > the dm state stuff when really needed.
> > > >
> > > > We could also sprinkle the drm_crtc_commit structure around a bit
> > > > (it's the glue that provides the synchronization across commits), but
> > > > since your dm state is global just grabbing all crtc states
> > > > unconditionally as part of that is probably best.
> > > >
> > > > > > While we could duplicate a copy of that code with nothing but the
> > > > > > workqueue changed that isn't something I'd really like to maintain
> > > > > > going forward.
> > > > >
> > > > > I'm not t

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

2020-07-27 Thread Mazin Rezk

On Monday, July 27, 2020 5:32 PM, Daniel Vetter  wrote:

> On Mon, Jul 27, 2020 at 11:11 PM Mazin Rezk  wrote:
> >
> > On Monday, July 27, 2020 4:29 PM, Daniel Vetter  wrote:
> >
> > > On Mon, Jul 27, 2020 at 9:28 PM Christian König
> > >  wrote:
> > > >
> > > > Am 27.07.20 um 16:05 schrieb Kazlauskas, Nicholas:
> > > > > On 2020-07-27 9:39 a.m., Christian König wrote:
> > > > >> Am 27.07.20 um 07:40 schrieb Mazin Rezk:
> > > > >>> This patch fixes a race condition that causes a use-after-free 
> > > > >>> during
> > > > >>> amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking
> > > > >>> commits
> > > > >>> are requested and the second one finishes before the first.
> > > > >>> Essentially,
> > > > >>> this bug occurs when the following sequence of events happens:
> > > > >>>
> > > > >>> 1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
> > > > >>> deferred to the workqueue.
> > > > >>>
> > > > >>> 2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
> > > > >>> deferred to the workqueue.
> > > > >>>
> > > > >>> 3. Commit #2 starts before commit #1, dm_state #1 is used in the
> > > > >>> commit_tail and commit #2 completes, freeing dm_state #1.
> > > > >>>
> > > > >>> 4. Commit #1 starts after commit #2 completes, uses the freed 
> > > > >>> dm_state
> > > > >>> 1 and dereferences a freelist pointer while setting the context.
> > > > >>
> > > > >> Well I only have a one mile high view on this, but why don't you let
> > > > >> the work items execute in order?
> > > > >>
> > > > >> That would be better anyway cause this way we don't trigger a cache
> > > > >> line ping pong between CPUs.
> > > > >>
> > > > >> Christian.
> > > > >
> > > > > We use the DRM helpers for managing drm_atomic_commit_state and those
> > > > > helpers internally push non-blocking commit work into the system
> > > > > unbound work queue.
> > > >
> > > > Mhm, well if you send those helper atomic commits in the order A,B and
> > > > they execute it in the order B,A I would call that a bug :)
> > >
> > > The way it works is it pushes all commits into unbound work queue, but
> > > then forces serialization as needed. We do _not_ want e.g. updates on
> > > different CRTC to be serialized, that would result in lots of judder.
> > > And hw is funny enough that there's all kinds of dependencies.
> > >
> > > The way you force synchronization is by adding other CRTC state
> > > objects. So if DC is busted and can only handle a single update per
> > > work item, then I guess you always need all CRTC states and everything
> > > will be run in order. But that also totally kills modern multi-screen
> > > compositors. Xorg isn't modern, just in case that's not clear :-)
> > >
> > > Lucking at the code it seems like you indeed have only a single dm
> > > state, so yeah global sync is what you'll need as immediate fix, and
> > > then maybe fix up DM to not be quite so silly ... or at least only do
> > > the dm state stuff when really needed.
> > >
> > > We could also sprinkle the drm_crtc_commit structure around a bit
> > > (it's the glue that provides the synchronization across commits), but
> > > since your dm state is global just grabbing all crtc states
> > > unconditionally as part of that is probably best.
> > >
> > > > > While we could duplicate a copy of that code with nothing but the
> > > > > workqueue changed that isn't something I'd really like to maintain
> > > > > going forward.
> > > >
> > > > I'm not talking about duplicating the code, I'm talking about fixing the
> > > > helpers. I don't know that code well, but from the outside it sounds
> > > > like a bug there.
> > > >
> > > > And executing work items in the order they are submitted is trivial.
> > > >
> > > > Had anybody pinged Daniel or other people familiar with the helper code
> > > > about it?
> > >
> > > Yeah something is

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

2020-07-27 Thread Mazin Rezk

On Monday, July 27, 2020 4:29 PM, Daniel Vetter  wrote:

> On Mon, Jul 27, 2020 at 9:28 PM Christian König
>  wrote:
> >
> > Am 27.07.20 um 16:05 schrieb Kazlauskas, Nicholas:
> > > On 2020-07-27 9:39 a.m., Christian König wrote:
> > >> Am 27.07.20 um 07:40 schrieb Mazin Rezk:
> > >>> This patch fixes a race condition that causes a use-after-free during
> > >>> amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking
> > >>> commits
> > >>> are requested and the second one finishes before the first.
> > >>> Essentially,
> > >>> this bug occurs when the following sequence of events happens:
> > >>>
> > >>> 1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
> > >>> deferred to the workqueue.
> > >>>
> > >>> 2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
> > >>> deferred to the workqueue.
> > >>>
> > >>> 3. Commit #2 starts before commit #1, dm_state #1 is used in the
> > >>> commit_tail and commit #2 completes, freeing dm_state #1.
> > >>>
> > >>> 4. Commit #1 starts after commit #2 completes, uses the freed dm_state
> > >>> 1 and dereferences a freelist pointer while setting the context.
> > >>
> > >> Well I only have a one mile high view on this, but why don't you let
> > >> the work items execute in order?
> > >>
> > >> That would be better anyway cause this way we don't trigger a cache
> > >> line ping pong between CPUs.
> > >>
> > >> Christian.
> > >
> > > We use the DRM helpers for managing drm_atomic_commit_state and those
> > > helpers internally push non-blocking commit work into the system
> > > unbound work queue.
> >
> > Mhm, well if you send those helper atomic commits in the order A,B and
> > they execute it in the order B,A I would call that a bug :)
>
> The way it works is it pushes all commits into unbound work queue, but
> then forces serialization as needed. We do _not_ want e.g. updates on
> different CRTC to be serialized, that would result in lots of judder.
> And hw is funny enough that there's all kinds of dependencies.
>
> The way you force synchronization is by adding other CRTC state
> objects. So if DC is busted and can only handle a single update per
> work item, then I guess you always need all CRTC states and everything
> will be run in order. But that also totally kills modern multi-screen
> compositors. Xorg isn't modern, just in case that's not clear :-)
>
> Lucking at the code it seems like you indeed have only a single dm
> state, so yeah global sync is what you'll need as immediate fix, and
> then maybe fix up DM to not be quite so silly ... or at least only do
> the dm state stuff when really needed.
>
> We could also sprinkle the drm_crtc_commit structure around a bit
> (it's the glue that provides the synchronization across commits), but
> since your dm state is global just grabbing all crtc states
> unconditionally as part of that is probably best.
>
> > > While we could duplicate a copy of that code with nothing but the
> > > workqueue changed that isn't something I'd really like to maintain
> > > going forward.
> >
> > I'm not talking about duplicating the code, I'm talking about fixing the
> > helpers. I don't know that code well, but from the outside it sounds
> > like a bug there.
> >
> > And executing work items in the order they are submitted is trivial.
> >
> > Had anybody pinged Daniel or other people familiar with the helper code
> > about it?
>
> Yeah something is wrong here, and the fix looks horrible :-)
>
> Aside, I've also seen some recent discussion flare up about
> drm_atomic_state_get/put used to paper over some other use-after-free,
> but this time related to interrupt handlers. Maybe a few rules about
> that:
> - dont
> - especially not when it's interrupt handlers, because you can't call
> drm_atomic_state_put from interrupt handlers.
>
> Instead have an spin_lock_irq to protect the shared date with your
> interrupt handler, and _copy_ the date over. This is e.g. what
> drm_crtc_arm_vblank_event does.

Nicholas wrote a patch that attempted to resolve the issue by adding every
CRTC into the commit to use use the stall checks. [1] While this forces
synchronisation on commits, it's kind of a hacky method that may take a
toll on performance.

Is it possible to have a DRM helper that forces synchronisation on some
commits without having to add every CRTC into the co

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

2020-07-27 Thread Mazin Rezk

On Monday, July 27, 2020 9:26 AM, Kazlauskas, Nicholas 
 wrote:

> On 2020-07-27 1:40 a.m., Mazin Rezk wrote:
> > This patch fixes a race condition that causes a use-after-free during
> > amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking commits
> > are requested and the second one finishes before the first. Essentially,
> > this bug occurs when the following sequence of events happens:
> >
> > 1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
> > deferred to the workqueue.
> >
> > 2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
> > deferred to the workqueue.
> >
> > 3. Commit #2 starts before commit #1, dm_state #1 is used in the
> > commit_tail and commit #2 completes, freeing dm_state #1.
> >
> > 4. Commit #1 starts after commit #2 completes, uses the freed dm_state
> > 1 and dereferences a freelist pointer while setting the context.
> >
> > Since this bug has only been spotted with fast commits, this patch fixes
> > the bug by clearing the dm_state instead of using the old dc_state for
> > fast updates. In addition, since dm_state is only used for its dc_state
> > and amdgpu_dm_atomic_commit_tail will retain the dc_state if none is found,
> > removing the dm_state should not have any consequences in fast updates.
> >
> > This use-after-free bug has existed for a while now, but only caused a
> > noticeable issue starting from 5.7-rc1 due to 3202fa62f ("slub: relocate
> > freelist pointer to middle of object") moving the freelist pointer from
> > dm_state->base (which was unused) to dm_state->context (which is
> > dereferenced).
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207383
> > Fixes: bd200d190f45 ("drm/amd/display: Don't replace the dc_state for fast 
> > updates")
> > Reported-by: Duncan <1i5t5.dun...@cox.net>
> > Signed-off-by: Mazin Rezk 
> > ---
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 ++-
> >   1 file changed, 27 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 86ffa0c2880f..710edc70e37e 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -8717,20 +8717,38 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> > *dev,
> >  * the same resource. If we have a new DC context as part of
> >  * the DM atomic state from validation we need to free it and
> >  * retain the existing one instead.
> > +*
> > +* Furthermore, since the DM atomic state only contains the DC
> > +* context and can safely be annulled, we can free the state
> > +* and clear the associated private object now to free
> > +* some memory and avoid a possible use-after-free later.
> >  */
> > -   struct dm_atomic_state *new_dm_state, *old_dm_state;
> >
> > -   new_dm_state = dm_atomic_get_new_state(state);
> > -   old_dm_state = dm_atomic_get_old_state(state);
> > +   for (i = 0; i < state->num_private_objs; i++) {
> > +   struct drm_private_obj *obj = 
> > state->private_objs[i].ptr;
> >
> > -   if (new_dm_state && old_dm_state) {
> > -   if (new_dm_state->context)
> > -   dc_release_state(new_dm_state->context);
> > +   if (obj->funcs == adev->dm.atomic_obj.funcs) {
> > +   int j = state->num_private_objs-1;
> >
> > -   new_dm_state->context = old_dm_state->context;
> > +   dm_atomic_destroy_state(obj,
> > +   state->private_objs[i].state);
> > +
> > +   /* If i is not at the end of the array then the
> > +* last element needs to be moved to where i was
> > +* before the array can safely be truncated.
> > +*/
> > +   if (i != j)
> > +   state->private_objs[i] =
> > +   state->private_objs[j];
> >
> > -   if (old_dm_state->context)
> > -   dc_retain_state(old_dm_state->context);
> > +

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

2020-07-26 Thread Mazin Rezk

On Monday, July 27, 2020 1:40 AM, Mazin Rezk  wrote:
> This patch fixes a race condition that causes a use-after-free during
> amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking commits
> are requested and the second one finishes before the first. Essentially,
> this bug occurs when the following sequence of events happens:
>
> 1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
> deferred to the workqueue.
>
> 2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
> deferred to the workqueue.
>
> 3. Commit #2 starts before commit #1, dm_state #1 is used in the
> commit_tail and commit #2 completes, freeing dm_state #1.
>
> 4. Commit #1 starts after commit #2 completes, uses the freed dm_state
> 1 and dereferences a freelist pointer while setting the context.
>
> Since this bug has only been spotted with fast commits, this patch fixes
> the bug by clearing the dm_state instead of using the old dc_state for
> fast updates. In addition, since dm_state is only used for its dc_state
> and amdgpu_dm_atomic_commit_tail will retain the dc_state if none is found,
> removing the dm_state should not have any consequences in fast updates.
>
> This use-after-free bug has existed for a while now, but only caused a
> noticeable issue starting from 5.7-rc1 due to 3202fa62f ("slub: relocate
> freelist pointer to middle of object") moving the freelist pointer from
> dm_state->base (which was unused) to dm_state->context (which is
> dereferenced).
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207383
> Fixes: bd200d190f45 ("drm/amd/display: Don't replace the dc_state for fast 
> updates")
> Reported-by: Duncan <1i5t5.dun...@cox.net>
> Signed-off-by: Mazin Rezk 
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 ++-
>  1 file changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 86ffa0c2880f..710edc70e37e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8717,20 +8717,38 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>* the same resource. If we have a new DC context as part of
>* the DM atomic state from validation we need to free it and
>* retain the existing one instead.
> +  *
> +  * Furthermore, since the DM atomic state only contains the DC
> +  * context and can safely be annulled, we can free the state
> +  * and clear the associated private object now to free
> +  * some memory and avoid a possible use-after-free later.
>*/
> - struct dm_atomic_state *new_dm_state, *old_dm_state;
>
> - new_dm_state = dm_atomic_get_new_state(state);
> - old_dm_state = dm_atomic_get_old_state(state);
> + for (i = 0; i < state->num_private_objs; i++) {
> + struct drm_private_obj *obj = 
> state->private_objs[i].ptr;
>
> - if (new_dm_state && old_dm_state) {
> - if (new_dm_state->context)
> - dc_release_state(new_dm_state->context);
> + if (obj->funcs == adev->dm.atomic_obj.funcs) {
> + int j = state->num_private_objs-1;
>
> - new_dm_state->context = old_dm_state->context;
> + dm_atomic_destroy_state(obj,
> + state->private_objs[i].state);
> +
> + /* If i is not at the end of the array then the
> +  * last element needs to be moved to where i was
> +  * before the array can safely be truncated.
> +  */
> + if (i != j)
> + state->private_objs[i] =
> + state->private_objs[j];
>
> - if (old_dm_state->context)
> - dc_retain_state(old_dm_state->context);
> + state->private_objs[j].ptr = NULL;
> + state->private_objs[j].state = NULL;
> + state->private_objs[j].old_state = NULL;
> + state->private_objs[j].new_state = NULL;
> +
> + state->num_private_objs = j;
> + break;
> +

[PATCH] drm/amd/display: Clear dm_state for fast updates

2020-07-26 Thread Mazin Rezk

This patch fixes a race condition that causes a use-after-free during
amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking commits
are requested and the second one finishes before the first. Essentially,
this bug occurs when the following sequence of events happens:

1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is
deferred to the workqueue.

2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is
deferred to the workqueue.

3. Commit #2 starts before commit #1, dm_state #1 is used in the
commit_tail and commit #2 completes, freeing dm_state #1.

4. Commit #1 starts after commit #2 completes, uses the freed dm_state
1 and dereferences a freelist pointer while setting the context.

Since this bug has only been spotted with fast commits, this patch fixes
the bug by clearing the dm_state instead of using the old dc_state for
fast updates. In addition, since dm_state is only used for its dc_state
and amdgpu_dm_atomic_commit_tail will retain the dc_state if none is found,
removing the dm_state should not have any consequences in fast updates.

This use-after-free bug has existed for a while now, but only caused a
noticeable issue starting from 5.7-rc1 due to 3202fa62f ("slub: relocate
freelist pointer to middle of object") moving the freelist pointer from
dm_state->base (which was unused) to dm_state->context (which is
dereferenced).

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207383
Fixes: bd200d190f45 ("drm/amd/display: Don't replace the dc_state for fast 
updates")
Reported-by: Duncan <1i5t5.dun...@cox.net>
Signed-off-by: Mazin Rezk 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 ++-
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 86ffa0c2880f..710edc70e37e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8717,20 +8717,38 @@ static int amdgpu_dm_atomic_check(struct drm_device 
*dev,
 * the same resource. If we have a new DC context as part of
 * the DM atomic state from validation we need to free it and
 * retain the existing one instead.
+*
+* Furthermore, since the DM atomic state only contains the DC
+* context and can safely be annulled, we can free the state
+* and clear the associated private object now to free
+* some memory and avoid a possible use-after-free later.
 */
-   struct dm_atomic_state *new_dm_state, *old_dm_state;

-   new_dm_state = dm_atomic_get_new_state(state);
-   old_dm_state = dm_atomic_get_old_state(state);
+   for (i = 0; i < state->num_private_objs; i++) {
+   struct drm_private_obj *obj = 
state->private_objs[i].ptr;

-   if (new_dm_state && old_dm_state) {
-   if (new_dm_state->context)
-   dc_release_state(new_dm_state->context);
+   if (obj->funcs == adev->dm.atomic_obj.funcs) {
+   int j = state->num_private_objs-1;

-   new_dm_state->context = old_dm_state->context;
+   dm_atomic_destroy_state(obj,
+   state->private_objs[i].state);
+
+   /* If i is not at the end of the array then the
+* last element needs to be moved to where i was
+* before the array can safely be truncated.
+*/
+   if (i != j)
+   state->private_objs[i] =
+   state->private_objs[j];

-   if (old_dm_state->context)
-   dc_retain_state(old_dm_state->context);
+   state->private_objs[j].ptr = NULL;
+   state->private_objs[j].state = NULL;
+   state->private_objs[j].old_state = NULL;
+   state->private_objs[j].new_state = NULL;
+
+   state->num_private_objs = j;
+   break;
+   }
}
}

--
2.27.0

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

2020-07-24 Thread Mazin Rezk

On Saturday, July 25, 2020 12:59 AM, Duncan <1i5t5.dun...@cox.net> wrote:

> On Sat, 25 Jul 2020 03:03:52 +0000
> Mazin Rezk mn...@protonmail.com wrote:
>
> > > Am 24.07.20 um 19:33 schrieb Kees Cook:
> > >
> > > > There was a fix to disable the async path for this driver that
> > > > worked around the bug too, yes? That seems like a safer and more
> > > > focused change that doesn't revert the SLUB defense for all
> > > > users, and would actually provide a complete, I think, workaround
> >
> > That said, I haven't seen the async disabling patch. If you could
> > link to it, I'd be glad to test it out and perhaps we can use that
> > instead.
>
> I'm confused. Not to put words in Kees' mouth; /I/ am confused (which
> admittedly could well be just because I make no claims to be a
> coder and am simply reading the bug and thread, but I'd appreciate some
> "unconfusing" anyway).
>
> My interpretation of the "async disabling" reference was that it was to
> comment #30 on the bug:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=207383#c30
>
> ... which (if I'm not confused on this point too) appears to be yours.
> There it was stated...
>
> > > > >
>
> I've also found that this bug exclusively occurs when commit_work is on
> the workqueue. After forcing drm_atomic_helper_commit to run all of the
> commits without adding to the workqueue and running the OS, the issue
> seems to have disappeared.
> <<<<
>
> Would not forcing all commits to run directly, without placing them on
> the workqueue, be "async disabling"? That's what I /thought/ he was
> referencing.

Oh, I thought he was referring to a different patch. Kees, could I get
your confirmation on this?

The change I made actually affected all of the DRM code, although this could
easily be changed to be specific to amdgpu. (By forcing blocking on
amdgpu_dm's non-blocking commit code)

That said, I'd still need to test further because I only did test it for a
couple of hours then. Although it should work in theory.

>
> OTOH your base/context swap idea sounds like a possibly "less
> disturbance" workaround, if it works, and given the point in the
> commit cycle... (But if it's out Sunday it's likely too late to test
> and get it in now anyway; if it's another week, tho...)

The base/context swap idea should make the use-after-free behave how it
did in 5.6. Since the bug doesn't cause an issue in 5.6, it's less of a
"less disturbance" workaround and more of a "no disturbance" workaround.

Thanks,
Mazin Rezk

>
> 
>
> Duncan - No HTML messages please; they are filtered as spam.
> "Every nonfree program has a lord, a master --
> and if you use the program, he is your master." Richard Stallman

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

2020-07-24 Thread Mazin Rezk

On Friday, July 24, 2020 5:19 PM, Paul Menzel  wrote:

> Dear Kees,
>
> Am 24.07.20 um 19:33 schrieb Kees Cook:
>
> > On Fri, Jul 24, 2020 at 09:45:18AM +0200, Paul Menzel wrote:
> >
> > > Am 24.07.20 um 00:32 schrieb Kees Cook:
> > >
> > > > On Thu, Jul 23, 2020 at 09:10:15PM +, Mazin Rezk wrote:
> > > > As Linux 5.8-rc7 is going to be released this Sunday, I wonder, if 
> > > > commit
> > > > 3202fa62f ("slub: relocate freelist pointer to middle of object") 
> > > > should be
> > > > reverted for now to fix the regression for the users according to 
> > > > Linux’ no
> > > > regression policy. Once the AMDGPU/DRM driver issue is fixed, it can be
> > > > reapplied. I know it’s not optimal, but as some testing is going to be
> > > > involved for the fix, I’d argue it’s the best option for the users.
> >
> > Well, the SLUB defense was already released in v5.7, so I'm not sure it
> > really helps for amdgpu_dm users seeing it there too.
>
> In my opinion, it would help, as the stable release could pick up the
> revert, ones it’s in Linus’ master branch.
>
> > There was a fix to disable the async path for this driver that worked
> > around the bug too, yes? That seems like a safer and more focused
> > change that doesn't revert the SLUB defense for all users, and would
> > actually provide a complete, I think, workaround whereas reverting
> > the SLUB change means the race still exists. For example, it would be
> > hit with slab poisoning, etc.
>
> I do not know. If there is such a fix, that would be great. But if you
> do not know, how should a normal user? ;-)
>
> Kind regards,
>
> Paul
>
> Kind regards,
>
> Paul

If we're talking about workarounds now, I suggest simply swapping the base
and context variables in struct dm_atomic_state. By that way, we won't need
to change non-amdgpu parts of the code (e.g. by reverting the SLUB patch).

Prior to 3202fa62f, the freelist pointer was stored in dm_state->base which
was never dereferenced and therefore caused no noticeable issue. After
3202fa62f, the freelist pointer is stored in the middle of the struct (i.e.
dm_state->context).

Swapping the position of the base and context variables in dm_atomic_state
should, in theory, revert this code back to it's pre-5.7 state since the
code would be back to overwriting base instead.

If we decide to use this workaround, I can write the patch and do more
extended tests to confirm it works around the issues.

That said, I haven't seen the async disabling patch. If you could link to
it, I'd be glad to test it out and perhaps we can use that instead.

Thanks,
Mazin Rezk

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

2020-07-24 Thread Mazin Rezk

On Thursday, July 23, 2020 6:57 PM, Mazin Rezk  wrote:

> It seems that I spoke too soon. I ran the system for another hour after
> submitting the patch and the bug just occurred. :/
>
> Sadly, that means the bug isn't really fixed and that I have to go
> investigate further.
>
> At the very least, this patch seems to delay the occurrence of the bug
> significantly which may help in further discovering the cause.
>
> On Thursday, July 23, 2020 6:16 PM, Kazlauskas, Nicholas 
> nicholas.kazlaus...@amd.com wrote:
>
> > On 2020-07-23 5:10 p.m., Mazin Rezk wrote:
> >
> > > When amdgpu_dm_atomic_commit_tail is running in the workqueue,
> > > drm_atomic_state_put will get called while amdgpu_dm_atomic_commit_tail is
> > > running, causing a race condition where state (and then dm_state) is
> > > sometimes freed while amdgpu_dm_atomic_commit_tail is running. This bug 
> > > has
> > > occurred since 5.7-rc1 and is well documented among polaris11 users [1].
> > > Prior to 5.7, this was not a noticeable issue since the freelist pointer
> > > was stored at the beginning of dm_state (base), which was unused. After
> > > changing the freelist pointer to be stored in the middle of the struct, 
> > > the
> > > freelist pointer overwrote the context, causing dc_state to become garbage
> > > data and made the call to dm_enable_per_frame_crtc_master_sync dereference
> > > a freelist pointer.
> > > This patch fixes the aforementioned issue by calling drm_atomic_state_get
> > > in amdgpu_dm_atomic_commit before drm_atomic_helper_commit is called and
> > > drm_atomic_state_put after amdgpu_dm_atomic_commit_tail is complete.
> > > According to my testing on 5.8.0-rc6, this should fix bug 207383 on
> > > Bugzilla [1].
> > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=207383
> > > Fixes: 3202fa62f ("slub: relocate freelist pointer to middle of object")
> > > Reported-by: Duncan 1i5t5.dun...@cox.net
> > > Signed-off-by: Mazin Rezk mn...@protonmail.com
> >
> > Thanks for the investigation and your patch. I appreciate the help in
> > trying to narrow down the root cause as this issue has been difficult to
> > reproduce on my setups.
> > Though I'm not sure this really resolves the issue - we make use of the
> > drm_atomic_helper_commit helper function from DRM which internally does
> > what you're doing with this patch:
> > drm_atomic_state_get(state);
> > if (nonblock)
> > queue_work(system_unbound_wq, >commit_work);
> >
> > else
> > commit_tail(state);
> >
> >
> > So even when it gets queued off to the unbound workqueue we still have a
> > reference on the state.
> > That reference gets dropped as part of commit tail helper in DRM as well:
> > if (funcs && funcs->atomic_commit_tail)
> >
> > funcs->atomic_commit_tail(old_state);
> >
> > else
> > drm_atomic_helper_commit_tail(old_state);
> >
> >
> > commit_time_ms = ktime_ms_delta(ktime_get(), start);
> > if (commit_time_ms > 0)
> >
> > drm_self_refresh_helper_update_avg_times(old_state,
> >  (unsigned long)commit_time_ms,
> >  new_self_refresh_mask);
> >
> >
> > drm_atomic_helper_commit_cleanup_done(old_state);
> > drm_atomic_state_put(old_state);
>
> I initially noticed that right after I wrote this patch so I was expecting
> the patch to fail. However, after several hours of testing, the crash just
> didn't occur so I believed the bug was fixed.
>
> > So instead of a use after free happening when we access the state we get
> > a double-free happening later at the end of commit tail in DRM.
> > What I think would be the right next step here is to actually determine
> > what sequence of IOCTLs and atomic commits are happening under your
> > setup with a very verbose dmesg log. You can set a debug level for DRM
> > in your kernel parameters with something like:
> > drm.debug=0x54
> > I don't see anything in amdgpu_dm.c that looks like it would be freeing
> > the state so I suspect something in the core is this doing this.
>
> Going through the KASAN use-after-free bug report in the Bugzilla
> attachments, it appears that the state is being freed at the end of
> commit_tail. Perhaps amdgpu_dm_atomic_commit_tail is being called on the
> the same old state twice? I can't quite think of any other possible
> explanation as to why that happens.

I think I've more or less con

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

2020-07-23 Thread Mazin Rezk

It seems that I spoke too soon. I ran the system for another hour after
submitting the patch and the bug just occurred. :/

Sadly, that means the bug isn't really fixed and that I have to go
investigate further.

At the very least, this patch seems to delay the occurrence of the bug
significantly which may help in further discovering the cause.

On Thursday, July 23, 2020 6:16 PM, Kazlauskas, Nicholas 
 wrote:

> On 2020-07-23 5:10 p.m., Mazin Rezk wrote:
>
> > When amdgpu_dm_atomic_commit_tail is running in the workqueue,
> > drm_atomic_state_put will get called while amdgpu_dm_atomic_commit_tail is
> > running, causing a race condition where state (and then dm_state) is
> > sometimes freed while amdgpu_dm_atomic_commit_tail is running. This bug has
> > occurred since 5.7-rc1 and is well documented among polaris11 users [1].
> > Prior to 5.7, this was not a noticeable issue since the freelist pointer
> > was stored at the beginning of dm_state (base), which was unused. After
> > changing the freelist pointer to be stored in the middle of the struct, the
> > freelist pointer overwrote the context, causing dc_state to become garbage
> > data and made the call to dm_enable_per_frame_crtc_master_sync dereference
> > a freelist pointer.
> > This patch fixes the aforementioned issue by calling drm_atomic_state_get
> > in amdgpu_dm_atomic_commit before drm_atomic_helper_commit is called and
> > drm_atomic_state_put after amdgpu_dm_atomic_commit_tail is complete.
> > According to my testing on 5.8.0-rc6, this should fix bug 207383 on
> > Bugzilla [1].
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=207383
> > Fixes: 3202fa62f ("slub: relocate freelist pointer to middle of object")
> > Reported-by: Duncan 1i5t5.dun...@cox.net
> > Signed-off-by: Mazin Rezk mn...@protonmail.com
>
> Thanks for the investigation and your patch. I appreciate the help in
> trying to narrow down the root cause as this issue has been difficult to
> reproduce on my setups.
>
> Though I'm not sure this really resolves the issue - we make use of the
> drm_atomic_helper_commit helper function from DRM which internally does
> what you're doing with this patch:
>
> drm_atomic_state_get(state);
> if (nonblock)
> queue_work(system_unbound_wq, >commit_work);
>
> else
>   commit_tail(state);
>
>
> So even when it gets queued off to the unbound workqueue we still have a
> reference on the state.
>
> That reference gets dropped as part of commit tail helper in DRM as well:
>
> if (funcs && funcs->atomic_commit_tail)
>
>   funcs->atomic_commit_tail(old_state);
>
> else
>   drm_atomic_helper_commit_tail(old_state);
>
>
> commit_time_ms = ktime_ms_delta(ktime_get(), start);
> if (commit_time_ms > 0)
>
>   drm_self_refresh_helper_update_avg_times(old_state,
>(unsigned long)commit_time_ms,
>new_self_refresh_mask);
>
>
> drm_atomic_helper_commit_cleanup_done(old_state);
>
> drm_atomic_state_put(old_state);
>

I initially noticed that right after I wrote this patch so I was expecting
the patch to fail. However, after several hours of testing, the crash just
didn't occur so I believed the bug was fixed.

> So instead of a use after free happening when we access the state we get
> a double-free happening later at the end of commit tail in DRM.
>
> What I think would be the right next step here is to actually determine
> what sequence of IOCTLs and atomic commits are happening under your
> setup with a very verbose dmesg log. You can set a debug level for DRM
> in your kernel parameters with something like:
>
> drm.debug=0x54
>
> I don't see anything in amdgpu_dm.c that looks like it would be freeing
> the state so I suspect something in the core is this doing this.

Going through the KASAN use-after-free bug report in the Bugzilla
attachments, it appears that the state is being freed at the end of
commit_tail. Perhaps amdgpu_dm_atomic_commit_tail is being called on the
the same old state twice? I can't quite think of any other possible
explanation as to why that happens.

>
> > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++
> > 1 file changed, 3 insertions(+)
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 86ffa0c2880f..86d6652872f2 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -7303,6 +7303,7 @@ static int amdgpu_dm_atomic_commit(struct drm_device 
> > *dev,
> > * unset legacy_cursor_update

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

2020-07-23 Thread Mazin Rezk

On Thursday, July 23, 2020 6:32 PM, Kees Cook  wrote:

> On Thu, Jul 23, 2020 at 09:10:15PM +0000, Mazin Rezk wrote:
>
> > When amdgpu_dm_atomic_commit_tail is running in the workqueue,
> > drm_atomic_state_put will get called while amdgpu_dm_atomic_commit_tail is
> > running, causing a race condition where state (and then dm_state) is
> > sometimes freed while amdgpu_dm_atomic_commit_tail is running. This bug has
> > occurred since 5.7-rc1 and is well documented among polaris11 users [1].
> > Prior to 5.7, this was not a noticeable issue since the freelist pointer
> > was stored at the beginning of dm_state (base), which was unused. After
> > changing the freelist pointer to be stored in the middle of the struct, the
> > freelist pointer overwrote the context, causing dc_state to become garbage
> > data and made the call to dm_enable_per_frame_crtc_master_sync dereference
> > a freelist pointer.
> > This patch fixes the aforementioned issue by calling drm_atomic_state_get
> > in amdgpu_dm_atomic_commit before drm_atomic_helper_commit is called and
> > drm_atomic_state_put after amdgpu_dm_atomic_commit_tail is complete.
> > According to my testing on 5.8.0-rc6, this should fix bug 207383 on
> > Bugzilla [1].
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=207383
>
> Nice work tracking this down!
>
> > Fixes: 3202fa62f ("slub: relocate freelist pointer to middle of object")
>
> I do, however, object to this Fixes tag. :) The flaw appears to have
> been with amdgpu_dm's reference tracking of "state" in the nonblocking
> case. (How this reference counting is supposed to work correctly, though,
> I'm not sure.) If I look at where the drm helper was split from being
> the default callback, it looks like this was what introduced the bug:
>
> da5c47f682ab ("drm/amd/display: Remove acrtc->stream")
>
> ? 3202fa62f certainly exposed it much more quickly, but there was a race
> even without 3202fa62f where something could have realloced the memory
> and written over it.
>
> ---
>
> Kees Cook


Thanks, I'll be sure to avoid using 3202fa62f as the cause next time.
I just thought to do that because it was what made the use-after-free cause
a noticeable bug.

Also, by the way, I just realised the patch didn't completely solve the bug.
Sorry about that, making an LKML thread on this was hasty on my part. Should
I get further confirmation from the Bugzilla thread before submitting a patch
for this bug in the future?

Thanks,
Mazin Rezk

[PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

2020-07-23 Thread Mazin Rezk

When amdgpu_dm_atomic_commit_tail is running in the workqueue,
drm_atomic_state_put will get called while amdgpu_dm_atomic_commit_tail is
running, causing a race condition where state (and then dm_state) is
sometimes freed while amdgpu_dm_atomic_commit_tail is running. This bug has
occurred since 5.7-rc1 and is well documented among polaris11 users [1].

Prior to 5.7, this was not a noticeable issue since the freelist pointer
was stored at the beginning of dm_state (base), which was unused. After
changing the freelist pointer to be stored in the middle of the struct, the
freelist pointer overwrote the context, causing dc_state to become garbage
data and made the call to dm_enable_per_frame_crtc_master_sync dereference
a freelist pointer.

This patch fixes the aforementioned issue by calling drm_atomic_state_get
in amdgpu_dm_atomic_commit before drm_atomic_helper_commit is called and
drm_atomic_state_put after amdgpu_dm_atomic_commit_tail is complete.

According to my testing on 5.8.0-rc6, this should fix bug 207383 on
Bugzilla [1].

[1] https://bugzilla.kernel.org/show_bug.cgi?id=207383

Fixes: 3202fa62f ("slub: relocate freelist pointer to middle of object")
Reported-by: Duncan <1i5t5.dun...@cox.net>
Signed-off-by: Mazin Rezk 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 86ffa0c2880f..86d6652872f2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7303,6 +7303,7 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev,
 * unset legacy_cursor_update
 */

+   drm_atomic_state_get(state);
return drm_atomic_helper_commit(dev, state, nonblock);

/*TODO Handle EINTR, reenable IRQ*/
@@ -7628,6 +7629,8 @@ static void amdgpu_dm_atomic_commit_tail(struct 
drm_atomic_state *state)

if (dc_state_temp)
dc_release_state(dc_state_temp);
+
+   drm_atomic_state_put(state);
 }


--
2.27.0

[PATCH] HID: logitech: Use HIDPP_RECEIVER_INDEX instead of 0xff

2020-07-04 Thread Mazin Rezk

Some parts of hid-logitech-dj explicitly referred to 0xff for the
receiver index. This patch changes those references to the
HIDPP_RECEIVER_INDEX definition.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-dj.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 48dff5d6b605..a78c13cc9f47 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1153,7 +1153,7 @@ static int logi_dj_recv_query_paired_devices(struct 
dj_receiver_dev *djrcv_dev)
if (!dj_report)
return -ENOMEM;
dj_report->report_id = REPORT_ID_DJ_SHORT;
-   dj_report->device_index = 0xFF;
+   dj_report->device_index = HIDPP_RECEIVER_INDEX;
dj_report->report_type = REPORT_TYPE_CMD_GET_PAIRED_DEVICES;
retval = logi_dj_recv_send_report(djrcv_dev, dj_report);
kfree(dj_report);
@@ -1175,7 +1175,7 @@ static int logi_dj_recv_switch_to_dj_mode(struct 
dj_receiver_dev *djrcv_dev,

if (djrcv_dev->type == recvr_type_dj) {
dj_report->report_id = REPORT_ID_DJ_SHORT;
-   dj_report->device_index = 0xFF;
+   dj_report->device_index = HIDPP_RECEIVER_INDEX;
dj_report->report_type = REPORT_TYPE_CMD_SWITCH;
dj_report->report_params[CMD_SWITCH_PARAM_DEVBITFIELD] = 0x3F;
dj_report->report_params[CMD_SWITCH_PARAM_TIMEOUT_SECONDS] =
@@ -1204,7 +1204,7 @@ static int logi_dj_recv_switch_to_dj_mode(struct 
dj_receiver_dev *djrcv_dev,
memset(buf, 0, HIDPP_REPORT_SHORT_LENGTH);

buf[0] = REPORT_ID_HIDPP_SHORT;
-   buf[1] = 0xFF;
+   buf[1] = HIDPP_RECEIVER_INDEX;
buf[2] = 0x80;
buf[3] = 0x00;
buf[4] = 0x00;
--
2.27.0

Re: [PATCH v7 1/3] HID: logitech-hidpp: Support translations from short to long reports

2019-10-22 Thread Mazin Rezk

On Tuesday, October 22, 2019 6:15 AM, Benjamin Tissoires 
 wrote:

> Hi Mazin,
>
> On Sun, Oct 20, 2019 at 6:41 AM Mazin Rezk mn...@protonmail.com wrote:
>
> > This patch allows short reports to be translated into long reports.
> > hidpp_validate_device now returns a u8 instead of a bool which represents
> > the supported reports. The corresponding bits (i.e.
> > HIDPP_REPORT_*_SUPPORTED) are set if an HID++ report is supported.
> > If a short report is being sent and the device does not support it, it is
> > instead sent as a long report.
> > Thanks,
> > Mazin
> >
> > Signed-off-by: Mazin Rezk mn...@protonmail.com
> >
> > ---
>
> Yep, I like this patch much better.
>
> I also tested the 0xb012 MX Master, and it now works like a charm :)
>
> nitpick: can you squash the next patch into this one? Because to be
> useful, this patch really need one or 2 supported devices.

I'll do that in v8 but I'm just going to wait until this patch is tested
more so I can see what other changes I need to make before the patch is
finalised.

Thanks,
Mazin

>
> Cheers,
> Benjamin
>
> > drivers/hid/hid-logitech-hidpp.c | 25 +++--
> > 1 file changed, 19 insertions(+), 6 deletions(-)
> > diff --git a/drivers/hid/hid-logitech-hidpp.c 
> > b/drivers/hid/hid-logitech-hidpp.c
> > index e9bba282f9c1..ee604b17514f 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -49,6 +49,10 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > #define HIDPP_REPORT_LONG_LENGTH 20
> > #define HIDPP_REPORT_VERY_LONG_MAX_LENGTH 64
> > +#define HIDPP_REPORT_SHORT_SUPPORTED BIT(0)
> > +#define HIDPP_REPORT_LONG_SUPPORTED BIT(1)
> > +#define HIDPP_REPORT_VERY_LONG_SUPPORTED BIT(2)
> > +
> > #define HIDPP_SUB_ID_CONSUMER_VENDOR_KEYS 0x03
> > #define HIDPP_SUB_ID_ROLLER 0x05
> > #define HIDPP_SUB_ID_MOUSE_EXTRA_BTNS 0x06
> > @@ -183,6 +187,7 @@ struct hidpp_device {
> >
> > unsigned long quirks;
> > unsigned long capabilities;
> >
> >
> > - u8 supported_reports;
> >
> >   struct hidpp_battery battery;
> >   struct hidpp_scroll_counter vertical_wheel_counter;
> >
> >
> >
> > @@ -340,6 +345,11 @@ static int hidpp_send_rap_command_sync(struct 
> > hidpp_device *hidpp_dev,
> > struct hidpp_report *message;
> > int ret, max_count;
> >
> > - /* Send as long report if short reports are not supported. */
> >
> >
> > - if (report_id == REPORT_ID_HIDPP_SHORT &&
> >
> >
> > - !(hidpp_dev->supported_reports & 
> > HIDPP_REPORT_SHORT_SUPPORTED))
> >
> >
> > - report_id = REPORT_ID_HIDPP_LONG;
> >
> >
> > - switch (report_id) {
> >   case REPORT_ID_HIDPP_SHORT:
> >   max_count = HIDPP_REPORT_SHORT_LENGTH - 4;
> >
> >
> >
> > @@ -3458,10 +3468,11 @@ static int hidpp_get_report_length(struct 
> > hid_device *hdev, int id)
> > return report->field[0]->report_count + 1;
> > }
> > -static bool hidpp_validate_device(struct hid_device *hdev)
> > +static u8 hidpp_validate_device(struct hid_device *hdev)
> > {
> > struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> >
> > - int id, report_length, supported_reports = 0;
> >
> >
> >
> > - int id, report_length;
> >
> >
> > - u8 supported_reports = 0;
> >
> >   id = REPORT_ID_HIDPP_SHORT;
> >   report_length = hidpp_get_report_length(hdev, id);
> >
> >
> >
> > @@ -3469,7 +3480,7 @@ static bool hidpp_validate_device(struct hid_device 
> > *hdev)
> > if (report_length < HIDPP_REPORT_SHORT_LENGTH)
> > goto bad_device;
> >
> > - supported_reports++;
> >
> >
> >
> > - supported_reports |= HIDPP_REPORT_SHORT_SUPPORTED;
> >   }
> >
> >   id = REPORT_ID_HIDPP_LONG;
> >
> >
> >
> > @@ -3478,7 +3489,7 @@ static bool hidpp_validate_device(struct hid_device 
> > *hdev)
> > if (report_length < HIDPP_REPORT_LONG_LENGTH)
> > goto bad_device;
> >
> > - supported_reports++;
> >
> >
> >
> > - supported_reports |= HIDPP_REPORT_LONG_SUPPORTED;
> >   }
> >
> >   id = REPORT_ID_HIDPP_VERY_LONG;
> >
> >

[PATCH v7 3/3] HID: logitech-hidpp: Support WirelessDeviceStatus connect events

2019-10-19 Thread Mazin Rezk

This patch allows hidpp_report_is_connect_event to support
WirelessDeviceStatus connect events.

The WirelessDeviceStatus feature index is stored in hidpp_device when
probed. The connect event's fap feature_index is compared against it if the
device supports it.

Thanks,
Mazin

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 39 
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 19b315e4e91b..c8b23568d0b1 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -191,6 +191,8 @@ struct hidpp_device {

struct hidpp_battery battery;
struct hidpp_scroll_counter vertical_wheel_counter;
+
+   u8 wireless_feature_index;
 };

 /* HID++ 1.0 error codes */
@@ -403,10 +405,13 @@ static inline bool hidpp_match_error(struct hidpp_report 
*question,
(answer->fap.params[0] == question->fap.funcindex_clientid);
 }

-static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
+static inline bool hidpp_report_is_connect_event(struct hidpp_device *hidpp,
+   struct hidpp_report *report)
 {
-   return (report->report_id == REPORT_ID_HIDPP_SHORT) &&
-   (report->rap.sub_id == 0x41);
+   return (hidpp->wireless_feature_index &&
+   (report->fap.feature_index == hidpp->wireless_feature_index)) ||
+   ((report->report_id == REPORT_ID_HIDPP_SHORT) &&
+   (report->rap.sub_id == 0x41));
 }

 /**
@@ -1283,6 +1288,24 @@ static int hidpp_battery_get_property(struct 
power_supply *psy,
return ret;
 }

+/* -- 
*/
+/* 0x1d4b: Wireless device status 
*/
+/* -- 
*/
+#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS  0x1d4b
+
+static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
+{
+   u8 feature_type;
+   int ret;
+
+   ret = hidpp_root_get_feature(hidpp,
+HIDPP_PAGE_WIRELESS_DEVICE_STATUS,
+>wireless_feature_index,
+_type);
+
+   return ret;
+}
+
 /* -- 
*/
 /* 0x2120: Hi-resolution scrolling
*/
 /* -- 
*/
@@ -3078,7 +3101,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device 
*hidpp, u8 *data,
}
}

-   if (unlikely(hidpp_report_is_connect_event(report))) {
+   if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
atomic_set(>connected,
!(report->rap.params[0] & (1 << 6)));
if (schedule_work(>work) == 0)
@@ -3628,6 +3651,14 @@ static int hidpp_probe(struct hid_device *hdev, const 
struct hid_device_id *id)
hidpp_overwrite_name(hdev);
}

+   if (connected && hidpp->protocol_major >= 2) {
+   ret = hidpp_set_wireless_feature_index(hidpp);
+   if (ret == -ENOENT)
+   hidpp->wireless_feature_index = 0;
+   else if (ret)
+   goto hid_hw_init_fail;
+   }
+
if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
ret = wtp_get_config(hidpp);
if (ret)
--
2.23.0

[PATCH v7 2/3] HID: logitech-hidpp: Support MX Master (b012, b01e) over Bluetooth

2019-10-19 Thread Mazin Rezk

This patch adds support for the MX Master over Bluetooth (b012, b01e).

I have only added the devices that have been tested, so the 0xb017 variant
of the MX Master needs to be added once it is tested.

Since this patch series adds support for Bluetooth LE devices in general,
other Bluetooth LE devices should also be added later

Thanks,
Mazin

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ee604b17514f..19b315e4e91b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3792,6 +3792,11 @@ static const struct hid_device_id hidpp_devices[] = {
{ /* MX5500 keyboard over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b),
  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
+   { /* MX Master mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
{}
 };

--
2.23.0

[PATCH v7 1/3] HID: logitech-hidpp: Support translations from short to long reports

2019-10-19 Thread Mazin Rezk

This patch allows short reports to be translated into long reports.

hidpp_validate_device now returns a u8 instead of a bool which represents
the supported reports. The corresponding bits (i.e.
HIDPP_REPORT_*_SUPPORTED) are set if an HID++ report is supported.

If a short report is being sent and the device does not support it, it is
instead sent as a long report.

Thanks,
Mazin

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index e9bba282f9c1..ee604b17514f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -49,6 +49,10 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_REPORT_LONG_LENGTH   20
 #define HIDPP_REPORT_VERY_LONG_MAX_LENGTH  64

+#define HIDPP_REPORT_SHORT_SUPPORTED   BIT(0)
+#define HIDPP_REPORT_LONG_SUPPORTEDBIT(1)
+#define HIDPP_REPORT_VERY_LONG_SUPPORTED   BIT(2)
+
 #define HIDPP_SUB_ID_CONSUMER_VENDOR_KEYS  0x03
 #define HIDPP_SUB_ID_ROLLER0x05
 #define HIDPP_SUB_ID_MOUSE_EXTRA_BTNS  0x06
@@ -183,6 +187,7 @@ struct hidpp_device {

unsigned long quirks;
unsigned long capabilities;
+   u8 supported_reports;

struct hidpp_battery battery;
struct hidpp_scroll_counter vertical_wheel_counter;
@@ -340,6 +345,11 @@ static int hidpp_send_rap_command_sync(struct hidpp_device 
*hidpp_dev,
struct hidpp_report *message;
int ret, max_count;

+   /* Send as long report if short reports are not supported. */
+   if (report_id == REPORT_ID_HIDPP_SHORT &&
+   !(hidpp_dev->supported_reports & HIDPP_REPORT_SHORT_SUPPORTED))
+   report_id = REPORT_ID_HIDPP_LONG;
+
switch (report_id) {
case REPORT_ID_HIDPP_SHORT:
max_count = HIDPP_REPORT_SHORT_LENGTH - 4;
@@ -3458,10 +3468,11 @@ static int hidpp_get_report_length(struct hid_device 
*hdev, int id)
return report->field[0]->report_count + 1;
 }

-static bool hidpp_validate_device(struct hid_device *hdev)
+static u8 hidpp_validate_device(struct hid_device *hdev)
 {
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
-   int id, report_length, supported_reports = 0;
+   int id, report_length;
+   u8 supported_reports = 0;

id = REPORT_ID_HIDPP_SHORT;
report_length = hidpp_get_report_length(hdev, id);
@@ -3469,7 +3480,7 @@ static bool hidpp_validate_device(struct hid_device *hdev)
if (report_length < HIDPP_REPORT_SHORT_LENGTH)
goto bad_device;

-   supported_reports++;
+   supported_reports |= HIDPP_REPORT_SHORT_SUPPORTED;
}

id = REPORT_ID_HIDPP_LONG;
@@ -3478,7 +3489,7 @@ static bool hidpp_validate_device(struct hid_device *hdev)
if (report_length < HIDPP_REPORT_LONG_LENGTH)
goto bad_device;

-   supported_reports++;
+   supported_reports |= HIDPP_REPORT_LONG_SUPPORTED;
}

id = REPORT_ID_HIDPP_VERY_LONG;
@@ -3488,7 +3499,7 @@ static bool hidpp_validate_device(struct hid_device *hdev)
report_length > HIDPP_REPORT_VERY_LONG_MAX_LENGTH)
goto bad_device;

-   supported_reports++;
+   supported_reports |= HIDPP_REPORT_VERY_LONG_SUPPORTED;
hidpp->very_long_report_length = report_length;
}

@@ -3536,7 +3547,9 @@ static int hidpp_probe(struct hid_device *hdev, const 
struct hid_device_id *id)
/*
 * Make sure the device is HID++ capable, otherwise treat as generic HID
 */
-   if (!hidpp_validate_device(hdev)) {
+   hidpp->supported_reports = hidpp_validate_device(hdev);
+
+   if (!hidpp->supported_reports) {
hid_set_drvdata(hdev, NULL);
devm_kfree(>dev, hidpp);
return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
--
2.23.0

[PATCH v7 0/3] Logitech HID++ Bluetooth LE support

2019-10-19 Thread Mazin Rezk

This series allows hid-logitech-hidpp to support Bluetooth LE HID++
devices. Only the MX Master is added right now but more HID++ Bluetooth LE
devices can be added once they are tested.

Thanks,
Mazin

Changes since [v6]:

- Based patch on "HID: logitech-hidpp: rework device validation"

- Removed the need for additional quirks

Changes since [v5]:

- Fixed bug where added quirks would overflow an unsigned long

- Changed the reserved quirk class bits from 0..20 to 0..15

Changes since [v4]:

- Omitted "HID: logitech: Add feature 0x0001: FeatureSet"

- Stored WirelessDeviceStatus feature index in hidpp_device

- Made Bluetooth quirk class alias quirks instead of vice versa

- Omitted non-tested devices

Changes since [v3]:

- Renamed hidpp20_featureset_get_feature to
  hidpp20_featureset_get_feature_id.

- Re-ordered hidpp20_featureset_get_count and
  hidpp20_featureset_get_feature_id based on their command IDs.

- Made feature_count initialize to 0 before running
  hidpp20_get_features.

Changes since [v2]:

- Split up the single patch into a series

Changes since [v1]:

- Added WirelessDeviceStatus support

[v6] 
https://lore.kernel.org/lkml/ggKipcQplIlTFmoP3hPnrQ-7_5-C0PKGd5feFymts3uenIBA8zOwz47YmKheD34H1rpkguDAGdx5YbS9UqpwfjT5Ir0Lji941liLVp--QtQ=@protonmail.com
[v5] 
https://lore.kernel.org/lkml/Mbf4goGxXZTfWwWtQQUke_rNf8kezpNOS9DVEVHf6RnnmjS1oRtMOJf4r14WfCC6GRYVs7gi0uZcIJ18Va2OJowzSbyMUGwLrl6I5fjW48o=@protonmail.com
[v4] 
https://lore.kernel.org/lkml/uBbIS3nFJ1jdYNLHcqjW5wxQAwmZv0kmYEoeoPrxNhfzi6cHwmCOY-ewdqe7S1hNEj-p4Hd9D0_Y3PymUTdh_6WFXuMmIYUkV2xaKCPMYz0=@protonmail.com
[v3] 
https://lore.kernel.org/lkml/l7xYjnA9EGfZe03FsrFhnH2aMq8qS8plWhHVvOtY_l4ShZ1NV6HA6hn9aI-jAzbLYUGFCIQCIKkx9z42Uoj4-AZDwBfRcAecYIn-0ZA5upE=@protonmail.com
[v2] https://www.spinics.net/lists/linux-input/msg63467.html
[v1] https://www.spinics.net/lists/linux-input/msg63317.html

Mazin Rezk (3):
  HID: logitech-hidpp: Support translations from short to long reports
  HID: logitech-hidpp: Support MX Master (b012, b01e) over Bluetooth
  HID: logitech-hidpp: Support WirelessDeviceStatus connect events

 drivers/hid/hid-logitech-hidpp.c | 69 +++-
 1 file changed, 59 insertions(+), 10 deletions(-)

--
2.23.0

Re: [PATCH v6 2/2] HID: logitech: Support WirelessDeviceStatus connect events

2019-10-18 Thread Mazin Rezk

On Friday, October 18, 2019 11:38 AM, Benjamin Tissoires 
 wrote:

> On Mon, Oct 14, 2019 at 8:36 PM Mazin Rezk mn...@protonmail.com wrote:
>
> > This patch allows WirelessDeviceStatus (0x1d4b) events to be detected as
> > connection events in the hid-logitech-hidpp module.
> > Devices with HIDPP_QUIRK_WIRELESS_DEVICE_STATUS use WirelessDeviceStatus
> > instead of traditional connect events. Since all Bluetooth LE devices seem
> > to act this way, HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.
> >
> > Signed-off-by: Mazin Rezk mn...@protonmail.com
> >
> > ---
> >
> > drivers/hid/hid-logitech-hidpp.c | 42 
> > 1 file changed, 37 insertions(+), 5 deletions(-)
> > diff --git a/drivers/hid/hid-logitech-hidpp.c 
> > b/drivers/hid/hid-logitech-hidpp.c
> > index 997b1056850a..9b3df57ca857 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -60,6 +60,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > #define HIDPP_QUIRK_CLASS_K750 BIT(4)
> > /* bits 2..15 are reserved for classes /
> > +#define HIDPP_QUIRK_WIRELESS_DEVICE_STATUS BIT(19)
> > #define HIDPP_QUIRK_MISSING_SHORT_REPORTS BIT(20)
> > / #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */#define 
> > HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS BIT(22)
> > @@ -82,7 +83,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
> > HIDPP_QUIRK_HI_RES_SCROLL_X2121)
> > -#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
> > +#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE (HIDPP_QUIRK_MISSING_SHORT_REPORTS 
> > | \
> >
> > -  
> > HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)
> >
> >
> >
> > #define HIDPP_QUIRK_DELAYED_INIT HIDPP_QUIRK_NO_HIDINPUT
> > @@ -189,6 +191,8 @@ struct hidpp_device {
> >
> > struct hidpp_battery battery;
> > struct hidpp_scroll_counter vertical_wheel_counter;
> >
> >
> > -
> > - u8 wireless_feature_index;
> >
> >
> >
> > };
> > /* HID++ 1.0 error codes */
> > @@ -402,10 +406,14 @@ static inline bool hidpp_match_error(struct 
> > hidpp_report *question,
> > (answer->fap.params[0] == question->fap.funcindex_clientid);
> > }
> > -static inline bool hidpp_report_is_connect_event(struct hidpp_report 
> > *report)
> > +static inline bool hidpp_report_is_connect_event(struct hidpp_device 
> > *hidpp,
> >
> > -  struct hidpp_report 
> > *report)
> >
> >
> >
> > {
> >
> > - return (report->report_id == REPORT_ID_HIDPP_SHORT) &&
> >
> >
> > - (report->rap.sub_id == 0x41);
> >
> >
> >
> > - return ((hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS) &&
> >
> >
> > - (report->fap.feature_index == 
> > hidpp->wireless_feature_index)) ||
> >
> >
> > -   (((report->report_id == REPORT_ID_HIDPP_SHORT) ||
> >
> >
> > - (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)) &&
> >
> >
> > - (report->rap.sub_id == 0x41));
> >
> >
>
> I wonder if we need a quirk after all: if
> hidpp->wireless_feature_index is non null, that means that we have the
> quirk.
> Unless the feature is present for non BLE devices, in which case, we
> might want to enable them one by one, for now.
>
> Cheers,
> Benjamin

Come to think of it, it does seem redundant. I'll likely extend the
WirelessDeviceStatus check to all HID++ 2.0 devices and just forgo the
added quirks entirely.

Thanks,
Mazin

>
> > }
> > /**
> > @@ -1282,6 +1290,24 @@ static int hidpp_battery_get_property(struct 
> > power_supply *psy,
> > return ret;
> > }
> > +/* 
> > -- /
> > +/ 0x1d4b: Wireless device status /
> > +/ 
> > -- 
> > */+#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS 0x1d4b
> > +
> > +static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
> > +{
> >
> > - u8 feature_type;
> >
> >
> > - int ret;
> >
> >
> > -
> > - ret = hidpp_root_get_feature(hidpp,
> >
> >
> > -

Re: [PATCH v6 1/2] HID: logitech: Add MX Master over Bluetooth

2019-10-18 Thread Mazin Rezk

On Friday, October 18, 2019 11:36 AM, Benjamin Tissoires 
 wrote:

> Hi Mazin,
>
> On Mon, Oct 14, 2019 at 8:36 PM Mazin Rezk mn...@protonmail.com wrote:
>
> > This patch adds support for the MX Master (b01e and b012) and also adds
> > foundational code for other Bluetooth LE HID++ devices to be added.
> > Some devices do not support short reports and thus have a quirk
> > (HIDPP_QUIRK_MISSING_SHORT_REPORTS) that forces short reports to be sent as
> > long reports. Since all Bluetooth LE HID++ devices seem to act this way,
> > HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.
> > To allow for some space for future quirks, I changed the comment that
> > defines the bits reserved for classes from 2...20 to 2..15.
> >
> > Signed-off-by: Mazin Rezk mn...@protonmail.com
> >
> > ---
> >
> > drivers/hid/hid-logitech-hidpp.c | 24 +++-
> > 1 file changed, 23 insertions(+), 1 deletion(-)
> > diff --git a/drivers/hid/hid-logitech-hidpp.c 
> > b/drivers/hid/hid-logitech-hidpp.c
> > index 0179f7ed77e5..997b1056850a 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -59,7 +59,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > #define HIDPP_QUIRK_CLASS_G920 BIT(3)
> > #define HIDPP_QUIRK_CLASS_K750 BIT(4)
> > -/* bits 2..20 are reserved for classes /
> > +/ bits 2..15 are reserved for classes /
> > +#define HIDPP_QUIRK_MISSING_SHORT_REPORTS BIT(20)
> > / #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */#define 
> > HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS BIT(22)
> > #define HIDPP_QUIRK_NO_HIDINPUT BIT(23)
> > @@ -81,6 +82,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
> > HIDPP_QUIRK_HI_RES_SCROLL_X2121)
> > +#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
> > +
> > #define HIDPP_QUIRK_DELAYED_INIT HIDPP_QUIRK_NO_HIDINPUT
> > #define HIDPP_CAPABILITY_HIDPP10_BATTERY BIT(0)
> > @@ -340,6 +343,12 @@ static int hidpp_send_rap_command_sync(struct 
> > hidpp_device *hidpp_dev,
> > struct hidpp_report *message;
> > int ret, max_count;
> >
> > - /* Force long reports on devices that do not support short 
> > reports */
> >
> >
> > - if (hidpp_dev->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS &&
> >
> >
> > - report_id == REPORT_ID_HIDPP_SHORT)
> >
> >
> > - report_id = REPORT_ID_HIDPP_LONG;
> >
> >
>
> Wouldn't it be faster to just store which report needs to be put here
> in struct hidpp_device?
>
> > -
> > - switch (report_id) {
> >   case REPORT_ID_HIDPP_SHORT:
> >   max_count = HIDPP_REPORT_SHORT_LENGTH - 4;
> >
> >
> >
> > @@ -3482,6 +3491,12 @@ static bool hidpp_validate_report(struct hid_device 
> > *hdev, int id,
> > static bool hidpp_validate_device(struct hid_device *hdev)
> > {
> >
> > - struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> >
> >
> > - /* Skip the short report check if the device does not support it 
> > */
> >
> >
> > - if (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)
> >
> >
> > - return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> >
> >
> > -  HIDPP_REPORT_LONG_LENGTH, 
> > false);
> >
> >
> > -
>
> I just merged Andrey's report detection, which means you will need to
> update this hunk:
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-5.4/upstream-fixes=905d754c53a522aacf806ea1d3e7c929148c1910
>
> The good thing, is that now you can simply auto-detect if the short
> report is missing. If the returned report_length is null, you know
> that the report is missing (and thus you can remember to set the
> quirk/which report id is needed).
>
> Cheers,
> Benjamin

Thank you, I'll try to rework this patch based on the changes introduced in
that commit.

Thanks,
Mazin

>
> > return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
> >  HIDPP_REPORT_SHORT_LENGTH, false) &&
> >hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> >
> >
> > @@ -3773,6 +3788,13 @@ static const struct hid_device_id hidpp_devices[] = {
> > { /* MX5500 keyboard over Bluetooth */
> > HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b),
> > .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
> >
> > - { /* MX Master mouse over Bluetooth */
> >
> >
> > -   HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
> >
> >
> > -   .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
> >
> >
> > -  HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
> >
> >
> > - { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
> >
> >
> > -   .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
> >
> >
> > -  HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
> >   {}
> >
> >
> >
> > };
> > --
> > 2.23.0

[PATCH v6 2/2] HID: logitech: Support WirelessDeviceStatus connect events

2019-10-14 Thread Mazin Rezk

This patch allows WirelessDeviceStatus (0x1d4b) events to be detected as
connection events in the hid-logitech-hidpp module.

Devices with HIDPP_QUIRK_WIRELESS_DEVICE_STATUS use WirelessDeviceStatus
instead of traditional connect events. Since all Bluetooth LE devices seem
to act this way, HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 42 
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 997b1056850a..9b3df57ca857 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -60,6 +60,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_CLASS_K750 BIT(4)

 /* bits 2..15 are reserved for classes */
+#define HIDPP_QUIRK_WIRELESS_DEVICE_STATUS BIT(19)
 #define HIDPP_QUIRK_MISSING_SHORT_REPORTS  BIT(20)
 /* #define HIDPP_QUIRK_CONNECT_EVENTS  BIT(21) disabled */
 #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS   BIT(22)
@@ -82,7 +83,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
 HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
 HIDPP_QUIRK_HI_RES_SCROLL_X2121)

-#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
+#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE (HIDPP_QUIRK_MISSING_SHORT_REPORTS | \
+HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)

 #define HIDPP_QUIRK_DELAYED_INIT   HIDPP_QUIRK_NO_HIDINPUT

@@ -189,6 +191,8 @@ struct hidpp_device {

struct hidpp_battery battery;
struct hidpp_scroll_counter vertical_wheel_counter;
+
+   u8 wireless_feature_index;
 };

 /* HID++ 1.0 error codes */
@@ -402,10 +406,14 @@ static inline bool hidpp_match_error(struct hidpp_report 
*question,
(answer->fap.params[0] == question->fap.funcindex_clientid);
 }

-static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
+static inline bool hidpp_report_is_connect_event(struct hidpp_device *hidpp,
+struct hidpp_report *report)
 {
-   return (report->report_id == REPORT_ID_HIDPP_SHORT) &&
-   (report->rap.sub_id == 0x41);
+   return ((hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS) &&
+   (report->fap.feature_index == hidpp->wireless_feature_index)) ||
+ (((report->report_id == REPORT_ID_HIDPP_SHORT) ||
+   (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)) &&
+   (report->rap.sub_id == 0x41));
 }

 /**
@@ -1282,6 +1290,24 @@ static int hidpp_battery_get_property(struct 
power_supply *psy,
return ret;
 }

+/* -- 
*/
+/* 0x1d4b: Wireless device status 
*/
+/* -- 
*/
+#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS  0x1d4b
+
+static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
+{
+   u8 feature_type;
+   int ret;
+
+   ret = hidpp_root_get_feature(hidpp,
+HIDPP_PAGE_WIRELESS_DEVICE_STATUS,
+>wireless_feature_index,
+_type);
+
+   return ret;
+}
+
 /* -- 
*/
 /* 0x2120: Hi-resolution scrolling
*/
 /* -- 
*/
@@ -3077,7 +3103,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device 
*hidpp, u8 *data,
}
}

-   if (unlikely(hidpp_report_is_connect_event(report))) {
+   if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
atomic_set(>connected,
!(report->rap.params[0] & (1 << 6)));
if (schedule_work(>work) == 0)
@@ -3624,6 +3650,12 @@ static int hidpp_probe(struct hid_device *hdev, const 
struct hid_device_id *id)
hidpp_overwrite_name(hdev);
}

+   if (connected && (hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)) {
+   ret = hidpp_set_wireless_feature_index(hidpp);
+   if (ret)
+   goto hid_hw_init_fail;
+   }
+
if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
ret = wtp_get_config(hidpp);
if (ret)
--
2.23.0

[PATCH v6 1/2] HID: logitech: Add MX Master over Bluetooth

2019-10-14 Thread Mazin Rezk

This patch adds support for the MX Master (b01e and b012) and also adds
foundational code for other Bluetooth LE HID++ devices to be added.

Some devices do not support short reports and thus have a quirk
(HIDPP_QUIRK_MISSING_SHORT_REPORTS) that forces short reports to be sent as
long reports. Since all Bluetooth LE HID++ devices seem to act this way,
HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.

To allow for some space for future quirks, I changed the comment that
defines the bits reserved for classes from 2...20 to 2..15.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..997b1056850a 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -59,7 +59,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_CLASS_G920 BIT(3)
 #define HIDPP_QUIRK_CLASS_K750 BIT(4)

-/* bits 2..20 are reserved for classes */
+/* bits 2..15 are reserved for classes */
+#define HIDPP_QUIRK_MISSING_SHORT_REPORTS  BIT(20)
 /* #define HIDPP_QUIRK_CONNECT_EVENTS  BIT(21) disabled */
 #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS   BIT(22)
 #define HIDPP_QUIRK_NO_HIDINPUTBIT(23)
@@ -81,6 +82,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
 HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
 HIDPP_QUIRK_HI_RES_SCROLL_X2121)

+#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
+
 #define HIDPP_QUIRK_DELAYED_INIT   HIDPP_QUIRK_NO_HIDINPUT

 #define HIDPP_CAPABILITY_HIDPP10_BATTERY   BIT(0)
@@ -340,6 +343,12 @@ static int hidpp_send_rap_command_sync(struct hidpp_device 
*hidpp_dev,
struct hidpp_report *message;
int ret, max_count;

+   /* Force long reports on devices that do not support short reports */
+   if (hidpp_dev->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS &&
+   report_id == REPORT_ID_HIDPP_SHORT)
+   report_id = REPORT_ID_HIDPP_LONG;
+
+
switch (report_id) {
case REPORT_ID_HIDPP_SHORT:
max_count = HIDPP_REPORT_SHORT_LENGTH - 4;
@@ -3482,6 +3491,12 @@ static bool hidpp_validate_report(struct hid_device 
*hdev, int id,

 static bool hidpp_validate_device(struct hid_device *hdev)
 {
+   struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+   /* Skip the short report check if the device does not support it */
+   if (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)
+   return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
+HIDPP_REPORT_LONG_LENGTH, false);
+
return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
 HIDPP_REPORT_SHORT_LENGTH, false) &&
   hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
@@ -3773,6 +3788,13 @@ static const struct hid_device_id hidpp_devices[] = {
{ /* MX5500 keyboard over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b),
  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
+   { /* MX Master mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
+   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
{}
 };

--
2.23.0

Re: [PATCH v5 1/2] HID: logitech: Add MX Master over Bluetooth

2019-10-13 Thread Mazin Rezk

On Sunday, October 13, 2019 9:28 PM, kbuild test robot  wrote:

> Hi Mazin,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [cannot apply to v5.4-rc2 next-20191010]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see 
> https://stackoverflow.com/a/37406982]
>
> url: 
> https://github.com/0day-ci/linux/commits/Mazin-Rezk/HID-logitech-Add-MX-Master-over-Bluetooth/20191014-071534
> config: mips-allmodconfig (attached as .config)
> compiler: mips-linux-gcc (GCC) 7.4.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
> -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.4.0 make.cross ARCH=mips
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot l...@intel.com
>
> All warnings (new ones prefixed by >>):
>
> In file included from include/linux/ioport.h:15:0,
> from include/linux/device.h:15,
> from drivers/hid/hid-logitech-hidpp.c:13:
> drivers/hid/hid-logitech-hidpp.c: In function 'hidpp_send_rap_command_sync':
>
> > > include/linux/bits.h:8:26: warning: left shift count >= width of type 
> > > [-Wshift-count-overflow]
>
> #define BIT(nr)   (UL(1) << (nr))
>  ^
>
>
> > > drivers/hid/hid-logitech-hidpp.c:74:43: note: in expansion of macro 'BIT'
>
> #define HIDPP_QUIRK_MISSING_SHORT_REPORTS BIT(32)
>   ^~~
>
>
> > > drivers/hid/hid-logitech-hidpp.c:347:26: note: in expansion of macro 
> > > 'HIDPP_QUIRK_MISSING_SHORT_REPORTS'
>
>  if (hidpp_dev->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS &&
>
>  ^
>
>
> drivers/hid/hid-logitech-hidpp.c: In function 'hidpp_validate_device':
>
> > > include/linux/bits.h:8:26: warning: left shift count >= width of type 
> > > [-Wshift-count-overflow]
>
> #define BIT(nr)   (UL(1) << (nr))
>  ^
>
>
> > > drivers/hid/hid-logitech-hidpp.c:74:43: note: in expansion of macro 'BIT'
>
> #define HIDPP_QUIRK_MISSING_SHORT_REPORTS BIT(32)
>   ^~~
>
>
> drivers/hid/hid-logitech-hidpp.c:3496:22: note: in expansion of macro 
> 'HIDPP_QUIRK_MISSING_SHORT_REPORTS'
> if (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)
>
>  ^
>
>
> drivers/hid/hid-logitech-hidpp.c: At top level:
>
> > > include/linux/bits.h:8:26: warning: left shift count >= width of type 
> > > [-Wshift-count-overflow]
>
> #define BIT(nr)   (UL(1) << (nr))
>  ^
>
>
> > > drivers/hid/hid-logitech-hidpp.c:74:43: note: in expansion of macro 'BIT'
>
> #define HIDPP_QUIRK_MISSING_SHORT_REPORTS BIT(32)
>   ^~~
>
>
> drivers/hid/hid-logitech-hidpp.c:85:40: note: in expansion of macro 
> 'HIDPP_QUIRK_MISSING_SHORT_REPORTS'
> #define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
> ^
>
> > > drivers/hid/hid-logitech-hidpp.c:3794:5: note: in expansion of macro 
> > > 'HIDPP_QUIRK_CLASS_BLUETOOTH_LE'
>
> HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
> ^~
>
>
> > > include/linux/bits.h:8:26: warning: left shift count >= width of type 
> > > [-Wshift-count-overflow]
>
> #define BIT(nr)   (UL(1) << (nr))
>  ^
>
>
> > > drivers/hid/hid-logitech-hidpp.c:74:43: note: in expansion of macro 'BIT'
>
> #define HIDPP_QUIRK_MISSING_SHORT_REPORTS BIT(32)
>   ^~~
>
>
> drivers/hid/hid-logitech-hidpp.c:85:40: note: in expansion of macro 
> 'HIDPP_QUIRK_MISSING_SHORT_REPORTS'
> #define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
> ^
> drivers/hid/hid-logitech-hidpp.c:3797:5: note: in expansion of macro 
> 'HIDPP_QUIRK_CLASS_BLUETOOTH_LE'
> HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
> ^~
>
> --

[PATCH v5 2/2] HID: logitech: Support WirelessDeviceStatus connect events

2019-10-11 Thread Mazin Rezk

This patch allows WirelessDeviceStatus (0x1d4b) events to be detected as
connection events in the hid-logitech-hidpp module.

Devices with HIDPP_QUIRK_WIRELESS_DEVICE_STATUS use WirelessDeviceStatus
instead of traditional connect events. Since all Bluetooth LE devices seem
to act this way, HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 42 
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3692fb883602..0dfa9b22b536 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -72,6 +72,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS BIT(30)
 #define HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS BIT(31)
 #define HIDPP_QUIRK_MISSING_SHORT_REPORTS  BIT(32)
+#define HIDPP_QUIRK_WIRELESS_DEVICE_STATUS BIT(33)

 /* These are just aliases for now */
 #define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS
@@ -82,7 +83,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
 HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
 HIDPP_QUIRK_HI_RES_SCROLL_X2121)

-#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
+#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE (HIDPP_QUIRK_MISSING_SHORT_REPORTS | \
+HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)

 #define HIDPP_QUIRK_DELAYED_INIT   HIDPP_QUIRK_NO_HIDINPUT

@@ -189,6 +191,8 @@ struct hidpp_device {

struct hidpp_battery battery;
struct hidpp_scroll_counter vertical_wheel_counter;
+
+   u8 wireless_feature_index;
 };

 /* HID++ 1.0 error codes */
@@ -402,10 +406,14 @@ static inline bool hidpp_match_error(struct hidpp_report 
*question,
(answer->fap.params[0] == question->fap.funcindex_clientid);
 }

-static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
+static inline bool hidpp_report_is_connect_event(struct hidpp_device *hidpp,
+struct hidpp_report *report)
 {
-   return (report->report_id == REPORT_ID_HIDPP_SHORT) &&
-   (report->rap.sub_id == 0x41);
+   return ((hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS) &&
+   (report->fap.feature_index == hidpp->wireless_feature_index)) ||
+ (((report->report_id == REPORT_ID_HIDPP_SHORT) ||
+   (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)) &&
+   (report->rap.sub_id == 0x41));
 }

 /**
@@ -1282,6 +1290,24 @@ static int hidpp_battery_get_property(struct 
power_supply *psy,
return ret;
 }

+/* -- 
*/
+/* 0x1d4b: Wireless device status 
*/
+/* -- 
*/
+#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS  0x1d4b
+
+static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
+{
+   u8 feature_type;
+   int ret;
+
+   ret = hidpp_root_get_feature(hidpp,
+HIDPP_PAGE_WIRELESS_DEVICE_STATUS,
+>wireless_feature_index,
+_type);
+
+   return ret;
+}
+
 /* -- 
*/
 /* 0x2120: Hi-resolution scrolling
*/
 /* -- 
*/
@@ -3077,7 +3103,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device 
*hidpp, u8 *data,
}
}

-   if (unlikely(hidpp_report_is_connect_event(report))) {
+   if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
atomic_set(>connected,
!(report->rap.params[0] & (1 << 6)));
if (schedule_work(>work) == 0)
@@ -3624,6 +3650,12 @@ static int hidpp_probe(struct hid_device *hdev, const 
struct hid_device_id *id)
hidpp_overwrite_name(hdev);
}

+   if (connected && (hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)) {
+   ret = hidpp_set_wireless_feature_index(hidpp);
+   if (ret)
+   goto hid_hw_init_fail;
+   }
+
if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
ret = wtp_get_config(hidpp);
if (ret)
--
2.23.0

[PATCH v5 1/2] HID: logitech: Add MX Master over Bluetooth

2019-10-11 Thread Mazin Rezk

This patch adds support for the MX Master (b01e and b012) and also adds
foundational code for other Bluetooth LE HID++ devices to be added.

Some devices do not support short reports and thus have a quirk
(HIDPP_QUIRK_MISSING_SHORT_REPORTS) that forces short reports to be sent as
long reports. Since all Bluetooth LE HID++ devices seem to act this way,
HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..3692fb883602 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -71,6 +71,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_HIDPP_WHEELS   BIT(29)
 #define HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS BIT(30)
 #define HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS BIT(31)
+#define HIDPP_QUIRK_MISSING_SHORT_REPORTS  BIT(32)

 /* These are just aliases for now */
 #define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS
@@ -81,6 +82,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
 HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
 HIDPP_QUIRK_HI_RES_SCROLL_X2121)

+#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
+
 #define HIDPP_QUIRK_DELAYED_INIT   HIDPP_QUIRK_NO_HIDINPUT

 #define HIDPP_CAPABILITY_HIDPP10_BATTERY   BIT(0)
@@ -340,6 +343,12 @@ static int hidpp_send_rap_command_sync(struct hidpp_device 
*hidpp_dev,
struct hidpp_report *message;
int ret, max_count;

+   /* Force long reports on devices that do not support short reports */
+   if (hidpp_dev->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS &&
+   report_id == REPORT_ID_HIDPP_SHORT)
+   report_id = REPORT_ID_HIDPP_LONG;
+
+
switch (report_id) {
case REPORT_ID_HIDPP_SHORT:
max_count = HIDPP_REPORT_SHORT_LENGTH - 4;
@@ -3482,6 +3491,12 @@ static bool hidpp_validate_report(struct hid_device 
*hdev, int id,

 static bool hidpp_validate_device(struct hid_device *hdev)
 {
+   struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+   /* Skip the short report check if the device does not support it */
+   if (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)
+   return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
+HIDPP_REPORT_LONG_LENGTH, false);
+
return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
 HIDPP_REPORT_SHORT_LENGTH, false) &&
   hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
@@ -3773,6 +3788,13 @@ static const struct hid_device_id hidpp_devices[] = {
{ /* MX5500 keyboard over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b),
  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
+   { /* MX Master mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
+   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
{}
 };

--
2.23.0

[PATCH v4 3/4] HID: logitech: Add feature 0x0001: FeatureSet

2019-10-10 Thread Mazin Rezk

On Saturday, October 5, 2019 9:04 PM, Mazin Rezk  wrote:

> This patch adds support for the 0x0001 (FeatureSet) feature. This feature
> is used to look up the feature ID of a feature index on a device and list
> the total count of features on the device.
>
> I also added the hidpp20_get_features function which iterates through all
> feature indexes on the device and stores a map of them in features an
> hidpp_device struct. This function runs when an HID++ 2.0 device is probed.

Changes in the version:
 - Renamed hidpp20_featureset_get_feature to
   hidpp20_featureset_get_feature_id.

 - Re-ordered hidpp20_featureset_get_count and
   hidpp20_featureset_get_feature_id based on their command IDs.

 - Made feature_count initialize to 0 before running hidpp20_get_features.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 90 
 1 file changed, 90 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index a0efa8a43213..2062be922c08 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -190,6 +190,9 @@ struct hidpp_device {

struct hidpp_battery battery;
struct hidpp_scroll_counter vertical_wheel_counter;
+
+   u16 *features;
+   u8 feature_count;
 };

 /* HID++ 1.0 error codes */
@@ -911,6 +914,82 @@ static int hidpp_root_get_protocol_version(struct 
hidpp_device *hidpp)
return 0;
 }

+/* -- 
*/
+/* 0x0001: FeatureSet 
*/
+/* -- 
*/
+
+#define HIDPP_PAGE_FEATURESET  0x0001
+
+#define CMD_FEATURESET_GET_COUNT   0x00
+#define CMD_FEATURESET_GET_FEATURE 0x11
+
+static int hidpp20_featureset_get_count(struct hidpp_device *hidpp,
+   u8 feature_index, u8 *count)
+{
+   struct hidpp_report response;
+   int ret;
+
+   ret = hidpp_send_fap_command_sync(hidpp, feature_index,
+   CMD_FEATURESET_GET_COUNT, NULL, 0, );
+
+   if (ret)
+   return ret;
+
+   *count = response.fap.params[0];
+
+   return ret;
+}
+
+static int hidpp20_featureset_get_feature_id(struct hidpp_device *hidpp,
+   u8 featureset_index, u8 feature_index, u16 *feature_id)
+{
+   struct hidpp_report response;
+   int ret;
+
+   ret = hidpp_send_fap_command_sync(hidpp, featureset_index,
+   CMD_FEATURESET_GET_FEATURE, _index, 1, );
+
+   if (ret)
+   return ret;
+
+   *feature_id = (response.fap.params[0] << 8) | response.fap.params[1];
+
+   return ret;
+}
+
+static int hidpp20_get_features(struct hidpp_device *hidpp)
+{
+   int ret;
+   u8 featureset_index, featureset_type;
+   u8 i;
+
+   ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_FEATURESET,
+_index, _type);
+
+   if (ret == -ENOENT) {
+   hid_warn(hidpp->hid_dev, "Unable to retrieve feature set.");
+   return 0;
+   }
+
+   if (ret)
+   return ret;
+
+   ret = hidpp20_featureset_get_count(hidpp, featureset_index,
+   >feature_count);
+
+   if (ret)
+   return ret;
+
+   hidpp->features = devm_kzalloc(>hid_dev->dev,
+   hidpp->feature_count * sizeof(u16), GFP_KERNEL);
+
+   for (i = 0; i < hidpp->feature_count && !ret; i++)
+   ret = hidpp20_featureset_get_feature_id(hidpp, featureset_index,
+   i, &(hidpp->features[i]));
+
+   return ret;
+}
+
 /* -- 
*/
 /* 0x0005: GetDeviceNameType  
*/
 /* -- 
*/
@@ -3625,6 +3704,17 @@ static int hidpp_probe(struct hid_device *hdev, const 
struct hid_device_id *id)
hidpp_overwrite_name(hdev);
}

+   /* Cache feature indexes and IDs to check reports faster */
+   hidpp->feature_count = 0;
+
+   if (hidpp->protocol_major >= 2) {
+   if (hidpp20_get_features(hidpp)) {
+   hid_err(hdev, "%s:hidpp20_get_features returned 
error\n",
+   __func__);
+   goto hid_hw_init_fail;
+   }
+   }
+
if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
ret = wtp_get_config(hidpp);
if (ret)
--
2.23.0

[PATCH v4 4/4] HID: logitech: Support WirelessDeviceStatus connect events

2019-10-10 Thread Mazin Rezk

On Saturday, October 5, 2019 9:05 PM, Mazin Rezk  wrote:

> This patch makes WirelessDeviceStatus (0x1d4b) events get detected as
> connection events on devices with HIDPP_QUIRK_WIRELESS_DEVICE_STATUS.
>
> This quirk is currently an alias for HIDPP_QUIRK_CLASS_BLUETOOTH since
> the added Bluetooth devices do not support regular connect events.

No changes have been made to this patch in v4.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2062be922c08..b2b5fe2c74db 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -84,6 +84,7 @@ MODULE_PARM_DESC(disable_tap_to_click,

 /* Just an alias for now, may possibly be a catch-all in the future */
 #define HIDPP_QUIRK_MISSING_SHORT_REPORTS  HIDPP_QUIRK_CLASS_BLUETOOTH
+#define HIDPP_QUIRK_WIRELESS_DEVICE_STATUS HIDPP_QUIRK_CLASS_BLUETOOTH

 #define HIDPP_QUIRK_DELAYED_INIT   HIDPP_QUIRK_NO_HIDINPUT

@@ -406,9 +407,22 @@ static inline bool hidpp_match_error(struct hidpp_report 
*question,
(answer->fap.params[0] == question->fap.funcindex_clientid);
 }

-static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
+#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS  0x1d4b
+
+static inline bool hidpp_report_is_connect_event(struct hidpp_device *hidpp,
+struct hidpp_report *report)
 {
-   return (report->report_id == REPORT_ID_HIDPP_SHORT) &&
+   if (hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS) {
+   /* If feature is invalid, skip array check */
+   if (report->fap.feature_index > hidpp->feature_count)
+   return false;
+
+   return (hidpp->features[report->fap.feature_index] ==
+HIDPP_PAGE_WIRELESS_DEVICE_STATUS);
+   }
+
+   return ((report->report_id == REPORT_ID_HIDPP_SHORT) ||
+   (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)) &&
(report->rap.sub_id == 0x41);
 }

@@ -3157,7 +3171,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device 
*hidpp, u8 *data,
}
}

-   if (unlikely(hidpp_report_is_connect_event(report))) {
+   if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
atomic_set(>connected,
!(report->rap.params[0] & (1 << 6)));
if (schedule_work(>work) == 0)
--
2.23.0

[PATCH v4 2/4] HID: logitech: Support HID++ devices without short reports

2019-10-10 Thread Mazin Rezk

On Saturday, October 5, 2019 9:04 PM, Mazin Rezk  wrote:

> This patch allows the hid-logitech-hidpp module to support devices that do
> not have support for Short HID++ reports. So far, it seems that Bluetooth
> HID++ 2.0 devices are missing short reports.

> This has been tested and confirmed with the MX Master and MX Master 2S and
> is therefore likely the case with the other Bluetooth devices.

No changes have been made to this patch in v4.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 37 ++--
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 85fd0c17cc2f..a0efa8a43213 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -71,6 +71,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_HIDPP_WHEELS   BIT(29)
 #define HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS BIT(30)
 #define HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS BIT(31)
+#define HIDPP_QUIRK_CLASS_BLUETOOTHBIT(5)

 /* These are just aliases for now */
 #define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS
@@ -81,6 +82,9 @@ MODULE_PARM_DESC(disable_tap_to_click,
 HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
 HIDPP_QUIRK_HI_RES_SCROLL_X2121)

+/* Just an alias for now, may possibly be a catch-all in the future */
+#define HIDPP_QUIRK_MISSING_SHORT_REPORTS  HIDPP_QUIRK_CLASS_BLUETOOTH
+
 #define HIDPP_QUIRK_DELAYED_INIT   HIDPP_QUIRK_NO_HIDINPUT

 #define HIDPP_CAPABILITY_HIDPP10_BATTERY   BIT(0)
@@ -340,6 +344,12 @@ static int hidpp_send_rap_command_sync(struct hidpp_device 
*hidpp_dev,
struct hidpp_report *message;
int ret, max_count;

+   /* Force long reports on devices that do not support short reports */
+   if (hidpp_dev->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS &&
+   report_id == REPORT_ID_HIDPP_SHORT)
+   report_id = REPORT_ID_HIDPP_LONG;
+
+
switch (report_id) {
case REPORT_ID_HIDPP_SHORT:
max_count = HIDPP_REPORT_SHORT_LENGTH - 4;
@@ -3482,6 +3492,12 @@ static bool hidpp_validate_report(struct hid_device 
*hdev, int id,

 static bool hidpp_validate_device(struct hid_device *hdev)
 {
+   struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+   /* Skip the short report check if the device does not support it */
+   if (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)
+   return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
+HIDPP_REPORT_LONG_LENGTH, false);
+
return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
 HIDPP_REPORT_SHORT_LENGTH, false) &&
   hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
@@ -3775,22 +3791,29 @@ static const struct hid_device_id hidpp_devices[] = {
  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
{ /* MX Anywhere 2 mouse over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ /* MX Anywhere 2S mouse over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ /* MX Master mouse over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ /* MX Master 2S mouse over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{}
 };

--
2.23.0

[PATCH v4 1/4] HID: logitech: Add MX Mice over Bluetooth

2019-10-10 Thread Mazin Rezk

On Saturday, October 5, 2019 9:04 PM, Mazin Rezk  wrote:

> This patch adds support for several MX mice over Bluetooth. The device IDs
> have been copied from the libratbag device database and their features
> have been based on their DJ device counterparts.

No changes have been made to this patch in v4. However, it should be noted
that the only device that has been thoroughly tested in this patch is the
MX Master (b01e). Further testing for the other devices may be required.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..85fd0c17cc2f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3773,6 +3773,24 @@ static const struct hid_device_id hidpp_devices[] = {
{ /* MX5500 keyboard over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b),
  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
+   { /* MX Anywhere 2 mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { /* MX Anywhere 2S mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { /* MX Master mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { /* MX Master 2S mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
{}
 };

--
2.23.0

Re: [PATCH v3 3/4] HID: logitech: Add feature 0x0001: FeatureSet

2019-10-06 Thread Mazin Rezk

On Sunday, October 6, 2019 11:25 AM, Filipe Laíns  wrote:

> On Sun, 2019-10-06 at 01:04 +0000, Mazin Rezk wrote:
> > This patch adds support for the 0x0001 (FeatureSet) feature. This feature
> > is used to look up the feature ID of a feature index on a device and list
> > the total count of features on the device.
> >
> > I also added the hidpp20_get_features function which iterates through all
> > feature indexes on the device and stores a map of them in features an
> > hidpp_device struct. This function runs when an HID++ 2.0 device is probed.
> >
> > Signed-off-by: Mazin Rezk 
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 92 
> >  1 file changed, 92 insertions(+)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c 
> > b/drivers/hid/hid-logitech-hidpp.c
> > index a0efa8a43213..64ac94c581aa 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -190,6 +190,9 @@ struct hidpp_device {
> >
> > struct hidpp_battery battery;
> > struct hidpp_scroll_counter vertical_wheel_counter;
> > +
> > +   u16 *features;
> > +   u8 feature_count;
> >  };
> >
> >  /* HID++ 1.0 error codes */
> > @@ -911,6 +914,84 @@ static int hidpp_root_get_protocol_version(struct 
> > hidpp_device *hidpp)
> > return 0;
> >  }
> >
> > +/* 
> > -- 
> > */
> > +/* 0x0001: FeatureSet  
> >*/
> > +/* 
> > -- 
> > */
> > +
> > +#define HIDPP_PAGE_FEATURESET  0x0001
> > +
> > +#define CMD_FEATURESET_GET_COUNT   0x00
> > +#define CMD_FEATURESET_GET_FEATURE 0x11
> > +
> > +static int hidpp20_featureset_get_feature(struct hidpp_device *hidpp,
>
> Can you change this to `hidpp20_featureset_get_feature_id` please? So
> that we keep in sync with the documentation, and to avoid minor
> confusion as IRoot has a `get_feature` function.

I will change this in v4, thanks.

>
> > +   u8 featureset_index, u8 feature_index, u16 *feature_id)
> > +{
> > +   struct hidpp_report response;
> > +   int ret;
> > +
> > +   ret = hidpp_send_fap_command_sync(hidpp, featureset_index,
> > +   CMD_FEATURESET_GET_FEATURE, _index, 1, );
> > +
> > +   if (ret)
> > +   return ret;
> > +
> > +   *feature_id = (response.fap.params[0] << 8) | response.fap.params[1];
> > +
> > +   return ret;
> > +}
> > +
> > +static int hidpp20_featureset_get_count(struct hidpp_device *hidpp,
> > +   u8 feature_index, u8 *count)
> > +{
> > +   struct hidpp_report response;
> > +   int ret;
> > +
> > +   ret = hidpp_send_fap_command_sync(hidpp, feature_index,
> > +   CMD_FEATURESET_GET_COUNT, NULL, 0, );
> > +
> > +   if (ret)
> > +   return ret;
> > +
> > +   *count = response.fap.params[0];
> > +
> > +   return ret;
> > +}
>
> Just a nitpick but can we put this before
> `hidpp20_featureset_get_feature`? This way we keep the ID order.

That makes sense. I will change this in v4, thanks.

>
> > +
> > +static int hidpp20_get_features(struct hidpp_device *hidpp)
> > +{
> > +   int ret;
> > +   u8 featureset_index, featureset_type;
> > +   u8 i;
> > +
> > +   hidpp->feature_count = 0;
> > +
> > +   ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_FEATURESET,
> > +_index, _type);
> > +
> > +   if (ret == -ENOENT) {
> > +   hid_warn(hidpp->hid_dev, "Unable to retrieve feature set.");
> > +   return 0;
> > +   }
> > +
> > +   if (ret)
> > +   return ret;
> > +
> > +   ret = hidpp20_featureset_get_count(hidpp, featureset_index,
> > +   >feature_count);
> > +
> > +   if (ret)
> > +   return ret;
> > +
> > +   hidpp->features = devm_kzalloc(>hid_dev->dev,
> > +   hidpp->feature_count * sizeof(u16), GFP_KERNEL);
> > +
> > +   for (i = 0; i < hidpp->feature_count && !ret; i++)
> > +   ret = hidpp20_featureset_get_feature(hidpp, featureset_index,
> > +   i, &(hidpp->features[i]));
> > +
> > +   return ret;
> > +}
> > +
> >  /* 
> >

Re: [PATCH v3 1/4] HID: logitech: Add MX Mice over Bluetooth

2019-10-06 Thread Mazin Rezk

On Sunday, October 6, 2019 11:07 AM, Filipe Laíns  wrote:

> On Sun, 2019-10-06 at 01:04 +0000, Mazin Rezk wrote:
>
> > This patch adds support for several MX mice over Bluetooth. The device IDs
> > have been copied from the libratbag device database and their features
> > have been based on their DJ device counterparts.
> >
> > Signed-off-by: Mazin Rezk mn...@protonmail.com
> >
> > ---
> >
> > drivers/hid/hid-logitech-hidpp.c | 18 ++
> > 1 file changed, 18 insertions(+)
> > diff --git a/drivers/hid/hid-logitech-hidpp.c 
> > b/drivers/hid/hid-logitech-hidpp.c
> > index 0179f7ed77e5..85fd0c17cc2f 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -3773,6 +3773,24 @@ static const struct hid_device_id hidpp_devices[] = {
> > { /* MX5500 keyboard over Bluetooth */
> > HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b),
> > .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
> >
> > -   { /* MX Anywhere 2 mouse over Bluetooth */
> > -   HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013),
> > -   .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> > -   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018),
> > -   .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> > -   { /* MX Anywhere 2S mouse over Bluetooth */
> > -   HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a),
> > -   .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> > -   { /* MX Master mouse over Bluetooth */
> > -   HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
> > -   .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> > -   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017),
> > -   .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> > -   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
> > -   .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> > -   { /* MX Master 2S mouse over Bluetooth */
> > -   HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019),
> > -   .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
> > {}
> > };
> >
> >
> > --
> > 2.23.0
>
> I think you should only add the mice you tested. We are not sure if
> this devices actually do work properly with the current stack. I will
> try to test some devices after Tuesday.
>
> Filipe Laíns

I have only really been able to test this patch on the MX Master (0xb01e).
However, I suspect that many of the added devices work in a similar way.
I could completely remove the devices I have not tested but I feel like it
would be better if we somehow kept track of what devices have been tested.

[PATCH v3 4/4] HID: logitech: Support WirelessDeviceStatus connect events

2019-10-05 Thread Mazin Rezk

This patch makes WirelessDeviceStatus (0x1d4b) events get detected as
connection events on devices with HIDPP_QUIRK_WIRELESS_DEVICE_STATUS.

This quirk is currently an alias for HIDPP_QUIRK_CLASS_BLUETOOTH since
the added Bluetooth devices do not support regular connect events.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 64ac94c581aa..4a6e41c2c9fc 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -84,6 +84,7 @@ MODULE_PARM_DESC(disable_tap_to_click,

 /* Just an alias for now, may possibly be a catch-all in the future */
 #define HIDPP_QUIRK_MISSING_SHORT_REPORTS  HIDPP_QUIRK_CLASS_BLUETOOTH
+#define HIDPP_QUIRK_WIRELESS_DEVICE_STATUS HIDPP_QUIRK_CLASS_BLUETOOTH

 #define HIDPP_QUIRK_DELAYED_INIT   HIDPP_QUIRK_NO_HIDINPUT

@@ -406,9 +407,22 @@ static inline bool hidpp_match_error(struct hidpp_report 
*question,
(answer->fap.params[0] == question->fap.funcindex_clientid);
 }

-static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
+#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS  0x1d4b
+
+static inline bool hidpp_report_is_connect_event(struct hidpp_device *hidpp,
+struct hidpp_report *report)
 {
-   return (report->report_id == REPORT_ID_HIDPP_SHORT) &&
+   if (hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS) {
+   /* If feature is invalid, skip array check */
+   if (report->fap.feature_index > hidpp->feature_count)
+   return false;
+
+   return (hidpp->features[report->fap.feature_index] ==
+HIDPP_PAGE_WIRELESS_DEVICE_STATUS);
+   }
+
+   return ((report->report_id == REPORT_ID_HIDPP_SHORT) ||
+   (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)) &&
(report->rap.sub_id == 0x41);
 }

@@ -3159,7 +3173,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device 
*hidpp, u8 *data,
}
}

-   if (unlikely(hidpp_report_is_connect_event(report))) {
+   if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
atomic_set(>connected,
!(report->rap.params[0] & (1 << 6)));
if (schedule_work(>work) == 0)
--
2.23.0

[PATCH v3 3/4] HID: logitech: Add feature 0x0001: FeatureSet

2019-10-05 Thread Mazin Rezk

This patch adds support for the 0x0001 (FeatureSet) feature. This feature
is used to look up the feature ID of a feature index on a device and list
the total count of features on the device.

I also added the hidpp20_get_features function which iterates through all
feature indexes on the device and stores a map of them in features an
hidpp_device struct. This function runs when an HID++ 2.0 device is probed.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 92 
 1 file changed, 92 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index a0efa8a43213..64ac94c581aa 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -190,6 +190,9 @@ struct hidpp_device {

struct hidpp_battery battery;
struct hidpp_scroll_counter vertical_wheel_counter;
+
+   u16 *features;
+   u8 feature_count;
 };

 /* HID++ 1.0 error codes */
@@ -911,6 +914,84 @@ static int hidpp_root_get_protocol_version(struct 
hidpp_device *hidpp)
return 0;
 }

+/* -- 
*/
+/* 0x0001: FeatureSet 
*/
+/* -- 
*/
+
+#define HIDPP_PAGE_FEATURESET  0x0001
+
+#define CMD_FEATURESET_GET_COUNT   0x00
+#define CMD_FEATURESET_GET_FEATURE 0x11
+
+static int hidpp20_featureset_get_feature(struct hidpp_device *hidpp,
+   u8 featureset_index, u8 feature_index, u16 *feature_id)
+{
+   struct hidpp_report response;
+   int ret;
+
+   ret = hidpp_send_fap_command_sync(hidpp, featureset_index,
+   CMD_FEATURESET_GET_FEATURE, _index, 1, );
+
+   if (ret)
+   return ret;
+
+   *feature_id = (response.fap.params[0] << 8) | response.fap.params[1];
+
+   return ret;
+}
+
+static int hidpp20_featureset_get_count(struct hidpp_device *hidpp,
+   u8 feature_index, u8 *count)
+{
+   struct hidpp_report response;
+   int ret;
+
+   ret = hidpp_send_fap_command_sync(hidpp, feature_index,
+   CMD_FEATURESET_GET_COUNT, NULL, 0, );
+
+   if (ret)
+   return ret;
+
+   *count = response.fap.params[0];
+
+   return ret;
+}
+
+static int hidpp20_get_features(struct hidpp_device *hidpp)
+{
+   int ret;
+   u8 featureset_index, featureset_type;
+   u8 i;
+
+   hidpp->feature_count = 0;
+
+   ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_FEATURESET,
+_index, _type);
+
+   if (ret == -ENOENT) {
+   hid_warn(hidpp->hid_dev, "Unable to retrieve feature set.");
+   return 0;
+   }
+
+   if (ret)
+   return ret;
+
+   ret = hidpp20_featureset_get_count(hidpp, featureset_index,
+   >feature_count);
+
+   if (ret)
+   return ret;
+
+   hidpp->features = devm_kzalloc(>hid_dev->dev,
+   hidpp->feature_count * sizeof(u16), GFP_KERNEL);
+
+   for (i = 0; i < hidpp->feature_count && !ret; i++)
+   ret = hidpp20_featureset_get_feature(hidpp, featureset_index,
+   i, &(hidpp->features[i]));
+
+   return ret;
+}
+
 /* -- 
*/
 /* 0x0005: GetDeviceNameType  
*/
 /* -- 
*/
@@ -3625,6 +3706,17 @@ static int hidpp_probe(struct hid_device *hdev, const 
struct hid_device_id *id)
hidpp_overwrite_name(hdev);
}

+   /* Cache feature indexes and IDs to check reports faster */
+   if (hidpp->protocol_major >= 2) {
+   if (hidpp20_get_features(hidpp)) {
+   hid_err(hdev, "%s:hidpp20_get_features returned 
error\n",
+   __func__);
+   goto hid_hw_init_fail;
+   }
+   } else {
+   hidpp->feature_count = 0;
+   }
+
if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
ret = wtp_get_config(hidpp);
if (ret)
--
2.23.0

[PATCH v3 2/4] HID: logitech: Support HID++ devices without short reports

2019-10-05 Thread Mazin Rezk

This patch allows the hid-logitech-hidpp module to support devices that do
not have support for Short HID++ reports. So far, it seems that Bluetooth
HID++ 2.0 devices are missing short reports.

This has been tested and confirmed with the MX Master and MX Master 2S and
is therefore likely the case with the other Bluetooth devices.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 37 ++--
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 85fd0c17cc2f..a0efa8a43213 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -71,6 +71,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_HIDPP_WHEELS   BIT(29)
 #define HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS BIT(30)
 #define HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS BIT(31)
+#define HIDPP_QUIRK_CLASS_BLUETOOTHBIT(5)

 /* These are just aliases for now */
 #define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS
@@ -81,6 +82,9 @@ MODULE_PARM_DESC(disable_tap_to_click,
 HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
 HIDPP_QUIRK_HI_RES_SCROLL_X2121)

+/* Just an alias for now, may possibly be a catch-all in the future */
+#define HIDPP_QUIRK_MISSING_SHORT_REPORTS  HIDPP_QUIRK_CLASS_BLUETOOTH
+
 #define HIDPP_QUIRK_DELAYED_INIT   HIDPP_QUIRK_NO_HIDINPUT

 #define HIDPP_CAPABILITY_HIDPP10_BATTERY   BIT(0)
@@ -340,6 +344,12 @@ static int hidpp_send_rap_command_sync(struct hidpp_device 
*hidpp_dev,
struct hidpp_report *message;
int ret, max_count;

+   /* Force long reports on devices that do not support short reports */
+   if (hidpp_dev->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS &&
+   report_id == REPORT_ID_HIDPP_SHORT)
+   report_id = REPORT_ID_HIDPP_LONG;
+
+
switch (report_id) {
case REPORT_ID_HIDPP_SHORT:
max_count = HIDPP_REPORT_SHORT_LENGTH - 4;
@@ -3482,6 +3492,12 @@ static bool hidpp_validate_report(struct hid_device 
*hdev, int id,

 static bool hidpp_validate_device(struct hid_device *hdev)
 {
+   struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+   /* Skip the short report check if the device does not support it */
+   if (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)
+   return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
+HIDPP_REPORT_LONG_LENGTH, false);
+
return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
 HIDPP_REPORT_SHORT_LENGTH, false) &&
   hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
@@ -3775,22 +3791,29 @@ static const struct hid_device_id hidpp_devices[] = {
  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
{ /* MX Anywhere 2 mouse over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ /* MX Anywhere 2S mouse over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ /* MX Master mouse over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{ /* MX Master 2S mouse over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019),
- .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |
+HIDPP_QUIRK_CLASS_BLUETOOTH },
{}
 };

--
2.23.0

[PATCH v3 1/4] HID: logitech: Add MX Mice over Bluetooth

2019-10-05 Thread Mazin Rezk

This patch adds support for several MX mice over Bluetooth. The device IDs
have been copied from the libratbag device database and their features
have been based on their DJ device counterparts.

Signed-off-by: Mazin Rezk 
---
 drivers/hid/hid-logitech-hidpp.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..85fd0c17cc2f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3773,6 +3773,24 @@ static const struct hid_device_id hidpp_devices[] = {
{ /* MX5500 keyboard over Bluetooth */
  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b),
  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
+   { /* MX Anywhere 2 mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { /* MX Anywhere 2S mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { /* MX Master mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
+   { /* MX Master 2S mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019),
+ .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 },
{}
 };

--
2.23.0

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

Re: [PATCH] drm/amd/display: Clear dm_state for fast updates

[PATCH] drm/amd/display: Clear dm_state for fast updates

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

[PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free

[PATCH] HID: logitech: Use HIDPP_RECEIVER_INDEX instead of 0xff

Re: [PATCH v7 1/3] HID: logitech-hidpp: Support translations from short to long reports

[PATCH v7 3/3] HID: logitech-hidpp: Support WirelessDeviceStatus connect events

[PATCH v7 2/3] HID: logitech-hidpp: Support MX Master (b012, b01e) over Bluetooth

[PATCH v7 1/3] HID: logitech-hidpp: Support translations from short to long reports

[PATCH v7 0/3] Logitech HID++ Bluetooth LE support

Re: [PATCH v6 2/2] HID: logitech: Support WirelessDeviceStatus connect events

Re: [PATCH v6 1/2] HID: logitech: Add MX Master over Bluetooth

[PATCH v6 2/2] HID: logitech: Support WirelessDeviceStatus connect events

[PATCH v6 1/2] HID: logitech: Add MX Master over Bluetooth

Re: [PATCH v5 1/2] HID: logitech: Add MX Master over Bluetooth

[PATCH v5 2/2] HID: logitech: Support WirelessDeviceStatus connect events

[PATCH v5 1/2] HID: logitech: Add MX Master over Bluetooth

[PATCH v4 3/4] HID: logitech: Add feature 0x0001: FeatureSet

[PATCH v4 4/4] HID: logitech: Support WirelessDeviceStatus connect events

[PATCH v4 2/4] HID: logitech: Support HID++ devices without short reports

[PATCH v4 1/4] HID: logitech: Add MX Mice over Bluetooth

Re: [PATCH v3 3/4] HID: logitech: Add feature 0x0001: FeatureSet

Re: [PATCH v3 1/4] HID: logitech: Add MX Mice over Bluetooth

[PATCH v3 4/4] HID: logitech: Support WirelessDeviceStatus connect events

[PATCH v3 3/4] HID: logitech: Add feature 0x0001: FeatureSet

[PATCH v3 2/4] HID: logitech: Support HID++ devices without short reports

[PATCH v3 1/4] HID: logitech: Add MX Mice over Bluetooth

35 matches

Site Navigation

Mail list logo

Footer information