linux-kernel - Re: [PATCH v1 0/2] iio: core: remove iio_validate_own

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

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

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

Message-ID: <20240928155519.1112f995@jic23-huawei>
Date: Sat, 28 Sep 2024 15:55:19 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: Matti Vaittinen <mazziesaccount@...il.com>, Lars-Peter Clausen
 <lars@...afoo.de>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 0/2] iio: core: remove iio_validate_own_trigger()
 function

On Sun, 22 Sep 2024 13:07:21 +0200
Vasileios Amoiridis <vassilisamir@...il.com> wrote:

> On Sun, Sep 22, 2024 at 12:44:15PM +0300, Matti Vaittinen wrote:
> > On 9/21/24 23:07, Vasileios Amoiridis wrote:  
> > > On Sat, Sep 21, 2024 at 12:23:39PM -0700, Lars-Peter Clausen wrote:  
> > > > On 9/21/24 11:19, Vasileios Amoiridis wrote:  
> > > > > The iio_validate_own_trigger() function was added in this commit [1] but it is
> > > > > the same with the below function called iio_trigger_validate_own_device(). The
> > > > > bodies of the functions can be found in [2], [3].
> > > > > 
> > > > > [1]: https://lore.kernel.org/all/51cd3e3e74a6addf8d333f4a109fb9c5a11086ee.1683541225.git.mazziesaccount@gmail.com/
> > > > > [2]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L732
> > > > > [3]: https://elixir.bootlin.com/linux/v6.11/source/drivers/iio/industrialio-trigger.c#L752  
> > > > 
> > > > The signature of the two functions are different, the order of the
> > > > parameters is switched. So you can't just swap them out for the
> > > > `validate_trigger` callback since the signature is not compatible. But maybe
> > > > you can update the implementation of one of the functions to calling the
> > > > other function.
> > > >   
> > > 
> > > Hi Lars,
> > > 
> > > Hmm, I see what you mean. Still though, do you think that we could do some
> > > cleaning here? I can see 3 approaches:
> > > 
> > > 1) One of the 2 functions calls the other internally and nothing else has
> > > to change.  
> > 
> > I would go with this. Changing the signatures to be the same would be (in
> > my, not always humble enough, opinion) wrong. The different order of
> > parameters reflects the different idea. One checks if device for trigger is
> > the right one, the other checks if the trigger for the device is the right
> > one. Thus, the order of parameters should be different.
> > 
> > Calling the same implementation internally is fine with me. Maybe Jonathan
> > will share his opinion when recovers from all the plumbing in Vienna ;)
> > 
> > Yours,
> > 	-- Matti
> > 
> > -- 
> > Matti Vaittinen
> > Linux kernel developer at ROHM Semiconductors
> > Oulu Finland
> >   
> 
> Hi Matti!
> 
> Thanks for your comment! Well, I still think in my eyes is better to
> have one function do one thing instead of multiple. Also, I didn't
> think of this argument with the order of arguments, it makes sense.
> My experience is quite limited to how things should be in such a
> large project so I trust your opinion. I would still like to see
> what Jonathan has to say on this though, maybe he had some
> reasoning behind!!!
> 
No to changing the signatures. It removes the difference
in meaning of the callbacks even though they happen to have
the same implementation in this very simple (and common case).

In the trigger first one, that is the subject.  We are asking the
question 'is this trigger ok being used for this device'.
In the other the device is the subject and we asking the
question 'is this device ok to use this trigger'

When we are checking the combination you have here, sure they
become the same thing but there are devices where it
matters that the trigger is not used to drive other devices
(typically because it's a hardware line that goes nowhere
else, so no interrupts etc) but other triggers can be used
to drive this device (often by software triggering the scan).
We have the opposite case as well but that's often
a shortcut when it just happens to be really complex to get
the trigger to reset (often requires reading all the data
or similar) - that condition can almost always be relaxed
but sometimes it's a lot of code for a niche case.

So fine to change the implementation of one of these
checks on tightly coupled device and trigger to call the other
but don't touch the callback signatures as to that breaks the
logical parameter ordering.

Jonathan

> Have a nice day!
> 
> Cheers,
> Vasilis