linux-kernel - Re: [PATCH] iio: health: afe4403: retrieve a valid iio

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:   Mon, 16 Jan 2017 10:36:42 -0800
From:   Alison Schofield <amsfield22@...il.com>
To:     "Andrew F. Davis" <afd@...com>
Cc:     jic23@...nel.org, knaack.h@....de, lars@...afoo.de,
        pmeerw@...erw.net, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: health: afe4403: retrieve a valid iio_dev in
 suspend/resume

On Mon, Jan 16, 2017 at 12:22:03PM -0600, Andrew F. Davis wrote:
> On 01/16/2017 12:10 PM, Alison Schofield wrote:
> > On Mon, Jan 16, 2017 at 10:38:27AM -0600, Andrew F. Davis wrote:
> >> On 01/14/2017 09:51 PM, Alison Schofield wrote:
> >>> The suspend/resume functions were using dev_to_iio_dev() to get
> >>> the iio_dev. That only works on IIO dev's.  Replace it with spi
> >>> functions to get the correct iio_dev.
> >>>
> >>> Signed-off-by: Alison Schofield <amsfield22@...il.com>
> >>
> >> Was this found with an automated tool? If not, it might be nice to have
> >> a Coccinelle style check for this. Anyway for this and the afe4404
> >> version patch:
> >>
> >> Acked-by: Andrew F. Davis <afd@...com>
> >>
> > Hi Andrew,
> > 
> > Just caught my eye while looking at these drivers for another reason:
> > they popped up in a cocci scan looking for drivers with regmap and dev
> > struct in global data. The dev struct may be redundant since it
> > can be retrieved from regmap.  I'm going to look at that a bit further
> > and send a patch if appropriate.
> > 
> 
> I have seen these patches before, and I always NACK them when I can. My
> stance is that just because we can find creative ways to recover
> information doesn't mean we should. It doesn't and anything and it is
> much more clear to me to just keep a copy of *dev to reference when
> needed, rather than some round-about method of fetching it through other
> entities.

Thanks for the heads up...removed from queue.
alisons

> 
> > Back to dev_to_iio_dev...it's a case of history repeating itself, but
> > not so often anymore.  I grep'd one more that I'll patch.  So, although
> > it appears close to extinction, a cocci check would be great.  It would
> > be even more valuble if worked with a more general check of the IIO
> > conversion/naming functions that are ripe for misuse.
> > 
> 
> I'm not a huge fan of all these macro tricks to fetch types for this
> reason here, we lose visibility into the containing types. Like here we
> have a function (dev_to_iio_dev) that goes from dev to iio_dev with no
> real compile time way to know if it is the *right* dev type...
> 
> > I'll propose this as an intern task.
> > 
> > If anyone has read this far and wants to chime in with their favorite
> > misused funcs to add to the todo list...please do.
> > 
> > thanks,
> > alisons
> > 
> >>> ---
> >>>  drivers/iio/health/afe4403.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/health/afe4403.c b/drivers/iio/health/afe4403.c
> >>> index 9a08146..6bb23a4 100644
> >>> --- a/drivers/iio/health/afe4403.c
> >>> +++ b/drivers/iio/health/afe4403.c
> >>> @@ -422,7 +422,7 @@ MODULE_DEVICE_TABLE(of, afe4403_of_match);
> >>>  
> >>>  static int __maybe_unused afe4403_suspend(struct device *dev)
> >>>  {
> >>> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >>> +	struct iio_dev *indio_dev = spi_get_drvdata(to_spi_device(dev));
> >>>  	struct afe4403_data *afe = iio_priv(indio_dev);
> >>>  	int ret;
> >>>  
> >>> @@ -443,7 +443,7 @@ static int __maybe_unused afe4403_suspend(struct device *dev)
> >>>  
> >>>  static int __maybe_unused afe4403_resume(struct device *dev)
> >>>  {
> >>> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >>> +	struct iio_dev *indio_dev = spi_get_drvdata(to_spi_device(dev));
> >>>  	struct afe4403_data *afe = iio_priv(indio_dev);
> >>>  	int ret;
> >>>  
> >>>