| 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
| ||
|
Message-ID: <CANhBUQ0NW18uFfoPkmecRwZ=EjWedB-ao8nyxfW1NFWg+DpTow@mail.gmail.com>
Date: Sun, 28 Jul 2019 16:52:15 +0800
From: Chuhong Yuan <hslester96@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Kevin Tsai <ktsai@...ellamicro.com>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: light: Use device-managed APIs
Jonathan Cameron <jic23@...nel.org> 于2019年7月28日周日 上午1:08写道：
>
> On Fri, 26 Jul 2019 17:08:29 +0800
> Chuhong Yuan <hslester96@...il.com> wrote:
>
> > Use device-managed APIs to simplify the code.
> > The remove functions are redundant now and can
> > be deleted.
> >
> > Signed-off-by: Chuhong Yuan <hslester96@...il.com>
> Hi,
>
> This should have been split along the lines of the drivers as these
> have different authors and hence could be merged at different times.
> Not to mention the nature of the changes isn't exactly the same
> between the two.
>
> I might have taken the cm3323 one with a small tweak as below if they
> had been separate.
>
> Thanks,
>
> Jonathan
I will separate them and resend later.
>
> > ---
> > drivers/iio/light/cm3323.c | 31 ++++++++++-----------------
> > drivers/iio/light/si1145.c | 44 ++++++++++++++------------------------
> > 2 files changed, 27 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
> > index 50f3438c2b49..fd352c0a4507 100644
> > --- a/drivers/iio/light/cm3323.c
> > +++ b/drivers/iio/light/cm3323.c
> > @@ -101,15 +101,16 @@ static int cm3323_init(struct iio_dev *indio_dev)
> > return 0;
> > }
> >
> > -static void cm3323_disable(struct iio_dev *indio_dev)
> > +static void cm3323_disable(void *data)
> > {
> > int ret;
> > - struct cm3323_data *data = iio_priv(indio_dev);
> > + struct iio_dev *indio_dev = data;
> > + struct cm3323_data *cm_data = iio_priv(indio_dev);
> >
> > - ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF,
> > + ret = i2c_smbus_write_word_data(cm_data->client, CM3323_CMD_CONF,
> > CM3323_CONF_SD_BIT);
> > if (ret < 0)
> > - dev_err(&data->client->dev, "Error writing reg_conf\n");
> > + dev_err(&cm_data->client->dev, "Error writing reg_conf\n");
> > }
> >
> > static int cm3323_set_it_bits(struct cm3323_data *data, int val, int val2)
> > @@ -243,25 +244,16 @@ static int cm3323_probe(struct i2c_client *client,
> > return ret;
> > }
> >
> > - ret = iio_device_register(indio_dev);
> > + ret = devm_add_action_or_reset(&client->dev, cm3323_disable, indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = devm_iio_device_register(&client->dev, indio_dev);
> > if (ret < 0) {
> > dev_err(&client->dev, "failed to register iio dev\n");
> > - goto err_init;
> > + return ret;
>
> I'd drop the failed to register message and just
> return devm_iio_device_register(...);
>
> Most of the likely paths that can cause that to fail report errors anyway
> in the core code.
>
> > }
> >
> > - return 0;
> > -err_init:
> > - cm3323_disable(indio_dev);
> > - return ret;
> > -}
> > -
> > -static int cm3323_remove(struct i2c_client *client)
> > -{
> > - struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > -
> > - iio_device_unregister(indio_dev);
> > - cm3323_disable(indio_dev);
> > -
> > return 0;
> > }
> >
> > @@ -276,7 +268,6 @@ static struct i2c_driver cm3323_driver = {
> > .name = CM3323_DRV_NAME,
> > },
> > .probe = cm3323_probe,
> > - .remove = cm3323_remove,
> > .id_table = cm3323_id,
> > };
> >
> > diff --git a/drivers/iio/light/si1145.c b/drivers/iio/light/si1145.c
> > index 6579d2418814..61867552b27c 100644
> > --- a/drivers/iio/light/si1145.c
> > +++ b/drivers/iio/light/si1145.c
> > @@ -1271,13 +1271,14 @@ static int si1145_probe_trigger(struct iio_dev *indio_dev)
> > return 0;
> > }
> >
> > -static void si1145_remove_trigger(struct iio_dev *indio_dev)
> > +static void si1145_remove_trigger(void *data)
> > {
> > - struct si1145_data *data = iio_priv(indio_dev);
> > + struct iio_dev *indio_dev = data;
> > + struct si1145_data *si_data = iio_priv(indio_dev);
> >
> > - if (data->trig) {
> > - iio_trigger_unregister(data->trig);
> > - data->trig = NULL;
> > + if (si_data->trig) {
> > + iio_trigger_unregister(si_data->trig);
>
> This doesn't look right. Why not use the devm trigger
> registration directly?
>
> > + si_data->trig = NULL;
>
> I find it unlikely that we actually need to set this to
> NULL, but please check closely in the driver.
> The obvious path would be that the interrupt fired
> after the trigger was unregistered (shouldn't be possible
> unless something odd is going on), however there is no
> obviously signs of this being checked there.
>
Thanks for your advice!
I will check whether data->trig is not needed to be set to NULL
and can devm_iio_trigger_register be used.
If so, I will revise them in version 2.
Regards,
Chuhong
>
> > }
> > }
> >
> > @@ -1332,7 +1333,8 @@ static int si1145_probe(struct i2c_client *client,
> > if (ret < 0)
> > return ret;
> >
> > - ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > + ret = devm_iio_triggered_buffer_setup(&client->dev,
> > + indio_dev, NULL,
> > si1145_trigger_handler, &si1145_buffer_setup_ops);
> > if (ret < 0)
> > return ret;
> > @@ -1340,23 +1342,21 @@ static int si1145_probe(struct i2c_client *client,
> > if (client->irq) {
> > ret = si1145_probe_trigger(indio_dev);
> > if (ret < 0)
> > - goto error_free_buffer;
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&client->dev,
> > + si1145_remove_trigger, indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > } else {
> > dev_info(&client->dev, "no irq, using polling\n");
> > }
> >
> > - ret = iio_device_register(indio_dev);
> > + ret = devm_iio_device_register(&client->dev, indio_dev);
> > if (ret < 0)
> > - goto error_free_trigger;
> > + return ret;
> >
> > return 0;
> > -
> > -error_free_trigger:
> > - si1145_remove_trigger(indio_dev);
> > -error_free_buffer:
> > - iio_triggered_buffer_cleanup(indio_dev);
> > -
> > - return ret;
> > }
> >
> > static const struct i2c_device_id si1145_ids[] = {
> > @@ -1371,23 +1371,11 @@ static const struct i2c_device_id si1145_ids[] = {
> > };
> > MODULE_DEVICE_TABLE(i2c, si1145_ids);
> >
> > -static int si1145_remove(struct i2c_client *client)
> > -{
> > - struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > -
> > - iio_device_unregister(indio_dev);
> > - si1145_remove_trigger(indio_dev);
> > - iio_triggered_buffer_cleanup(indio_dev);
> > -
> > - return 0;
> > -}
> > -
> > static struct i2c_driver si1145_driver = {
> > .driver = {
> > .name = "si1145",
> > },
> > .probe = si1145_probe,
> > - .remove = si1145_remove,
> > .id_table = si1145_ids,
> > };
> >
>
Powered by blists - more mailing lists