linux-kernel - Re: [PATCH] iio: tsl2772: Use device-managed API

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: <20190728083141.GA14194@onstation.org>
Date:   Sun, 28 Jul 2019 04:31:41 -0400
From:   Brian Masney <masneyb@...tation.org>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Chuhong Yuan <hslester96@...il.com>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: tsl2772: Use device-managed API

On Sat, Jul 27, 2019 at 12:57:49PM +0100, Jonathan Cameron wrote:
> On Fri, 26 Jul 2019 20:30:58 +0800
> Chuhong Yuan <hslester96@...il.com> wrote:
> 
> > Use devm_iio_device_register to simplify
> > the code.
> > 
> > Signed-off-by: Chuhong Yuan <hslester96@...il.com>
> 
> Please try to pick up on likely reviewers in your cc list.  In this case
> Brian did a lot of work cleaning these drivers up so I've added him.
> Not everyone keeps up with the linux-iio list for some reason ;)
> 
> Immediate thought was that you had broken the ordering but turns out
> it is already buggy.
> 
> As things stand, we remove the userspace interfaces (iio_device_unregister)
> after turning off the power.   This is obviously a bad idea and also
> form a purely "obviously correct" stand point, we aren't doing the reverse
> of probe.
> 
> So, I 'think' the right option is to reorder remove so that the power left
> on until after the iio_device_unregister call. At that point, we can't
> use devm_iio_device_register as we'll have the same incorrect ordering
> we currently have.
> 
> Brian, you looked at this driver most recently.  Thoughts?

devm_add_action() could be used in the probe function to schedule the call
to tsl2772_chip_off(). That would eliminate the need for
tsl2772_remove(). See tsl2772_disable_regulators_action() for an example in
that driver.

Chuhong: Another potential cleanup to shrink the size of this driver is
to move it over to the regulator_bulk_() API. I didn't realize that API
existed at the time I introduced the regulator support. If you're
interested in taking on that cleanup as well, I can test those changes
for you if you don't have the hardware.

Brian


> > ---
> >  drivers/iio/light/tsl2772.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
> > index 83cece921843..aa5891d105e8 100644
> > --- a/drivers/iio/light/tsl2772.c
> > +++ b/drivers/iio/light/tsl2772.c
> > @@ -1877,7 +1877,7 @@ static int tsl2772_probe(struct i2c_client *clientp,
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	ret = iio_device_register(indio_dev);
> > +	ret = devm_iio_device_register(&clientp->dev, indio_dev);
> >  	if (ret) {
> >  		tsl2772_chip_off(indio_dev);
> >  		dev_err(&clientp->dev,
> > @@ -1928,8 +1928,6 @@ static int tsl2772_remove(struct i2c_client *client)
> >  
> >  	tsl2772_chip_off(indio_dev);
> >  
> > -	iio_device_unregister(indio_dev);
> > -
> >  	return 0;
> >  }
> >