linux-kernel - Re: [PATCH 0/2] media protect enable and disable source handler paths

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 29 Nov 2016 07:15:26 -0200
From:   Mauro Carvalho Chehab <mchehab@...pensource.com>
To:     Shuah Khan <shuahkh@....samsung.com>
Cc:     mchehab@...nel.org, mkrufky@...uxtv.org, klock.android@...il.com,
        elfring@...rs.sourceforge.net, max@...mpel.org,
        hans.verkuil@...co.com, javier@....samsung.com,
        chehabrafael@...il.com, sakari.ailus@...ux.intel.com,
        laurent.pinchart+renesas@...asonboard.com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] media protect enable and disable source handler
 paths

Em Mon, 28 Nov 2016 19:15:12 -0700
Shuah Khan <shuahkh@....samsung.com> escreveu:

> These two patches fix enable and disable source handler paths. These
> aren't dependent patches, grouped because they fix similar problems.

Those two patches should be fold, as applying just the first patch
would cause au0828 to try to double lock.

> 
> This work is triggered by a review comment from Mauro Chehab on a
> snd_usb_audio patch about protecting the enable and disabel handler
> path in it.
> 
> Ran tests to make sure enable and disable handler paths work. When
> digital stream is active, analog app finds the tuner busy and vice
> versa. Also ran the Sakari's unbind while video stream is active test.

Sorry, but your patches descriptions don't make things clear:

- It doesn't present any OOPS or logs that would help to
  understand what you're trying to fix;

- From what I understood, you're moving the lock out of
  enable/disable handlers, and letting their callers to do
  the locks themselves. Why? Are there any condition where it
  won't need to be locked?

- It is not touching documentation. If now the callbacks should
  not implement locks, this should be explicitly described.

Btw, I think it is a bad idea to let the callers to handle
the locks. The best would be, instead, to change the code in
some other way to avoid it, if possible. If not possible at all,
clearly describe why it is not possible and insert some comments
inside the code, to avoid some cleanup patch to mess up with this.

Regards

Thanks,
Mauro