| Message ID | 20210628135132.73682-1-aardelean@deviqon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: light: adjd_s311: convert to device-managed functions | expand |
On Mon, 28 Jun 2021 16:51:32 +0300 Alexandru Ardelean <aardelean@deviqon.com> wrote: > This one is a little easier to convert to device-managed, now with the > devm_krealloc() function. > > The other iio_triggered_buffer_setup() and iio_device_register() can be > converted to their devm_ variants. And devm_krealloc() can be used to > (re)alloc the buffer. When the driver unloads, this will also be free'd. > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> > --- > drivers/iio/light/adjd_s311.c | 34 +++++----------------------------- > 1 file changed, 5 insertions(+), 29 deletions(-) > > diff --git a/drivers/iio/light/adjd_s311.c b/drivers/iio/light/adjd_s311.c > index 17dac8d0e11d..19d60d6986a1 100644 > --- a/drivers/iio/light/adjd_s311.c > +++ b/drivers/iio/light/adjd_s311.c > @@ -230,8 +230,8 @@ static int adjd_s311_update_scan_mode(struct iio_dev *indio_dev, > { > struct adjd_s311_data *data = iio_priv(indio_dev); > > - kfree(data->buffer); > - data->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); > + data->buffer = devm_krealloc(indio_dev->dev.parent, data->buffer, > + indio_dev->scan_bytes, GFP_KERNEL); I got some complaints about exactly this trick in a review recently so I'll pass them on. Whilst devm_krealloc() usage like this won't lose the original reference, its not what people expect from a realloc() case, so to not confuse people it is better to do a dance where you use a local variable, then only set data->buffer to it once we know the realloc succeeded. That avoids this looking like the anti-pattern it would be if that were a normal realloc in which case you would just have leaked the original allocation. More interestingly, why are we bothering with resizing the buffer dependent on what is enabled? Can't we just allocate a 128 byte buffer and not bother changing it as we really aren't wasting that much space? Just embed it in the adjd_s311_data structure directly and don't worry about the allocations. Will need to be aligned(8) though to avoid the push_to_buffer_with_timestamp() issue. Using something like struct { s16 chans[4]; s64 ts __aligned(8); /* I hate x86 32 bit */ } scan; Inside the priv structure should work nicely. > if (data->buffer == NULL) > return -ENOMEM; > > @@ -256,7 +256,6 @@ static int adjd_s311_probe(struct i2c_client *client, > return -ENOMEM; > > data = iio_priv(indio_dev); > - i2c_set_clientdata(client, indio_dev); > data->client = client; > > indio_dev->info = &adjd_s311_info; > @@ -265,34 +264,12 @@ static int adjd_s311_probe(struct i2c_client *client, > indio_dev->num_channels = ARRAY_SIZE(adjd_s311_channels); > indio_dev->modes = INDIO_DIRECT_MODE; > > - err = iio_triggered_buffer_setup(indio_dev, NULL, > - adjd_s311_trigger_handler, NULL); > + err = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, > + adjd_s311_trigger_handler, NULL); > if (err < 0) > return err; > > - err = iio_device_register(indio_dev); > - if (err) > - goto exit_unreg_buffer; > - > - dev_info(&client->dev, "ADJD-S311 color sensor registered\n"); > - > - return 0; > - > -exit_unreg_buffer: > - iio_triggered_buffer_cleanup(indio_dev); > - return err; > -} > - > -static int adjd_s311_remove(struct i2c_client *client) > -{ > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > - struct adjd_s311_data *data = iio_priv(indio_dev); > - > - iio_device_unregister(indio_dev); > - iio_triggered_buffer_cleanup(indio_dev); > - kfree(data->buffer); > - > - return 0; > + return devm_iio_device_register(&client->dev, indio_dev); > } > > static const struct i2c_device_id adjd_s311_id[] = { > @@ -306,7 +283,6 @@ static struct i2c_driver adjd_s311_driver = { > .name = ADJD_S311_DRV_NAME, > }, > .probe = adjd_s311_probe, > - .remove = adjd_s311_remove, > .id_table = adjd_s311_id, > }; > module_i2c_driver(adjd_s311_driver);
On Sat, 3 Jul 2021 at 20:47, Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 28 Jun 2021 16:51:32 +0300 > Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > This one is a little easier to convert to device-managed, now with the > > devm_krealloc() function. > > > > The other iio_triggered_buffer_setup() and iio_device_register() can be > > converted to their devm_ variants. And devm_krealloc() can be used to > > (re)alloc the buffer. When the driver unloads, this will also be free'd. > > > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> > > --- > > drivers/iio/light/adjd_s311.c | 34 +++++----------------------------- > > 1 file changed, 5 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/iio/light/adjd_s311.c b/drivers/iio/light/adjd_s311.c > > index 17dac8d0e11d..19d60d6986a1 100644 > > --- a/drivers/iio/light/adjd_s311.c > > +++ b/drivers/iio/light/adjd_s311.c > > @@ -230,8 +230,8 @@ static int adjd_s311_update_scan_mode(struct iio_dev *indio_dev, > > { > > struct adjd_s311_data *data = iio_priv(indio_dev); > > > > - kfree(data->buffer); > > - data->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); > > + data->buffer = devm_krealloc(indio_dev->dev.parent, data->buffer, > > + indio_dev->scan_bytes, GFP_KERNEL); > I got some complaints about exactly this trick in a review recently so I'll > pass them on. > > Whilst devm_krealloc() usage like this won't lose the original reference, its > not what people expect from a realloc() case, so to not confuse people it is > better to do a dance where you use a local variable, then only set data->buffer > to it once we know the realloc succeeded. > > That avoids this looking like the anti-pattern it would be if that were a normal > realloc in which case you would just have leaked the original allocation. > > More interestingly, why are we bothering with resizing the buffer dependent on what > is enabled? Can't we just allocate a 128 byte buffer and not bother changing it > as we really aren't wasting that much space? Just embed it in the adjd_s311_data > structure directly and don't worry about the allocations. Will need to be > aligned(8) though to avoid the push_to_buffer_with_timestamp() issue. > Using something like > > struct { > s16 chans[4]; > s64 ts __aligned(8); /* I hate x86 32 bit */ do you want to me t also add this comment? :p [just kidding] > } scan; > > Inside the priv structure should work nicely. i agree; will do it like this; i hesitated a bit due to the inertia of converting things to devm_ > > > > if (data->buffer == NULL) > > return -ENOMEM; > > > > @@ -256,7 +256,6 @@ static int adjd_s311_probe(struct i2c_client *client, > > return -ENOMEM; > > > > data = iio_priv(indio_dev); > > - i2c_set_clientdata(client, indio_dev); > > data->client = client; > > > > indio_dev->info = &adjd_s311_info; > > @@ -265,34 +264,12 @@ static int adjd_s311_probe(struct i2c_client *client, > > indio_dev->num_channels = ARRAY_SIZE(adjd_s311_channels); > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > - err = iio_triggered_buffer_setup(indio_dev, NULL, > > - adjd_s311_trigger_handler, NULL); > > + err = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, > > + adjd_s311_trigger_handler, NULL); > > if (err < 0) > > return err; > > > > - err = iio_device_register(indio_dev); > > - if (err) > > - goto exit_unreg_buffer; > > - > > - dev_info(&client->dev, "ADJD-S311 color sensor registered\n"); > > - > > - return 0; > > - > > -exit_unreg_buffer: > > - iio_triggered_buffer_cleanup(indio_dev); > > - return err; > > -} > > - > > -static int adjd_s311_remove(struct i2c_client *client) > > -{ > > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > > - struct adjd_s311_data *data = iio_priv(indio_dev); > > - > > - iio_device_unregister(indio_dev); > > - iio_triggered_buffer_cleanup(indio_dev); > > - kfree(data->buffer); > > - > > - return 0; > > + return devm_iio_device_register(&client->dev, indio_dev); > > } > > > > static const struct i2c_device_id adjd_s311_id[] = { > > @@ -306,7 +283,6 @@ static struct i2c_driver adjd_s311_driver = { > > .name = ADJD_S311_DRV_NAME, > > }, > > .probe = adjd_s311_probe, > > - .remove = adjd_s311_remove, > > .id_table = adjd_s311_id, > > }; > > module_i2c_driver(adjd_s311_driver); >
On Mon, 5 Jul 2021 09:38:21 +0300 Alexandru Ardelean <aardelean@deviqon.com> wrote: > On Sat, 3 Jul 2021 at 20:47, Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Mon, 28 Jun 2021 16:51:32 +0300 > > Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > > > This one is a little easier to convert to device-managed, now with the > > > devm_krealloc() function. > > > > > > The other iio_triggered_buffer_setup() and iio_device_register() can be > > > converted to their devm_ variants. And devm_krealloc() can be used to > > > (re)alloc the buffer. When the driver unloads, this will also be free'd. > > > > > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> > > > --- > > > drivers/iio/light/adjd_s311.c | 34 +++++----------------------------- > > > 1 file changed, 5 insertions(+), 29 deletions(-) > > > > > > diff --git a/drivers/iio/light/adjd_s311.c b/drivers/iio/light/adjd_s311.c > > > index 17dac8d0e11d..19d60d6986a1 100644 > > > --- a/drivers/iio/light/adjd_s311.c > > > +++ b/drivers/iio/light/adjd_s311.c > > > @@ -230,8 +230,8 @@ static int adjd_s311_update_scan_mode(struct iio_dev *indio_dev, > > > { > > > struct adjd_s311_data *data = iio_priv(indio_dev); > > > > > > - kfree(data->buffer); > > > - data->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); > > > + data->buffer = devm_krealloc(indio_dev->dev.parent, data->buffer, > > > + indio_dev->scan_bytes, GFP_KERNEL); > > I got some complaints about exactly this trick in a review recently so I'll > > pass them on. > > > > Whilst devm_krealloc() usage like this won't lose the original reference, its > > not what people expect from a realloc() case, so to not confuse people it is > > better to do a dance where you use a local variable, then only set data->buffer > > to it once we know the realloc succeeded. > > > > That avoids this looking like the anti-pattern it would be if that were a normal > > realloc in which case you would just have leaked the original allocation. > > > > More interestingly, why are we bothering with resizing the buffer dependent on what > > is enabled? Can't we just allocate a 128 byte buffer and not bother changing it > > as we really aren't wasting that much space? Just embed it in the adjd_s311_data > > structure directly and don't worry about the allocations. Will need to be > > aligned(8) though to avoid the push_to_buffer_with_timestamp() issue. > > Using something like > > > > struct { > > s16 chans[4]; > > s64 ts __aligned(8); /* I hate x86 32 bit */ > > do you want to me t also add this comment? :p > [just kidding] > > > } scan; > > > > Inside the priv structure should work nicely. > > i agree; will do it like this; > i hesitated a bit due to the inertia of converting things to devm_ A long discussion on rust usage in linux diverted into the issues around devm. I 'believe' that we are fine in IIO after some work Lars did a long time back to make us resilient to unbinds whilst the chardev was open, but probably worth keeping an eye on that discussion. https://lore.kernel.org/ksummit/CANiq72nkNrekzbxMci6vW02w=Q2L-SVTk_U4KN_LT8u_b=YPgw@mail.gmail.com/T/#m6db86a574237c22a32ecf49b596b3c2917967c5e I'm a tiny bit nervous that there might be races where we are doing the devm_realloc. I 'think' we are fine, but the 'think' and 'believe' in these statements expresses a slight lack of certainty! Jonathan > > > > > > > > if (data->buffer == NULL) > > > return -ENOMEM; > > > > > > @@ -256,7 +256,6 @@ static int adjd_s311_probe(struct i2c_client *client, > > > return -ENOMEM; > > > > > > data = iio_priv(indio_dev); > > > - i2c_set_clientdata(client, indio_dev); > > > data->client = client; > > > > > > indio_dev->info = &adjd_s311_info; > > > @@ -265,34 +264,12 @@ static int adjd_s311_probe(struct i2c_client *client, > > > indio_dev->num_channels = ARRAY_SIZE(adjd_s311_channels); > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > > > - err = iio_triggered_buffer_setup(indio_dev, NULL, > > > - adjd_s311_trigger_handler, NULL); > > > + err = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, > > > + adjd_s311_trigger_handler, NULL); > > > if (err < 0) > > > return err; > > > > > > - err = iio_device_register(indio_dev); > > > - if (err) > > > - goto exit_unreg_buffer; > > > - > > > - dev_info(&client->dev, "ADJD-S311 color sensor registered\n"); > > > - > > > - return 0; > > > - > > > -exit_unreg_buffer: > > > - iio_triggered_buffer_cleanup(indio_dev); > > > - return err; > > > -} > > > - > > > -static int adjd_s311_remove(struct i2c_client *client) > > > -{ > > > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > > > - struct adjd_s311_data *data = iio_priv(indio_dev); > > > - > > > - iio_device_unregister(indio_dev); > > > - iio_triggered_buffer_cleanup(indio_dev); > > > - kfree(data->buffer); > > > - > > > - return 0; > > > + return devm_iio_device_register(&client->dev, indio_dev); > > > } > > > > > > static const struct i2c_device_id adjd_s311_id[] = { > > > @@ -306,7 +283,6 @@ static struct i2c_driver adjd_s311_driver = { > > > .name = ADJD_S311_DRV_NAME, > > > }, > > > .probe = adjd_s311_probe, > > > - .remove = adjd_s311_remove, > > > .id_table = adjd_s311_id, > > > }; > > > module_i2c_driver(adjd_s311_driver); > >
> From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, July 11, 2021 12:26 PM > To: Alexandru Ardelean <aardelean@deviqon.com> > Cc: linux-iio <linux-iio@vger.kernel.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; pmeerw@pmeerw.net > Subject: Re: [PATCH] iio: light: adjd_s311: convert to device-managed > functions > > On Mon, 5 Jul 2021 09:38:21 +0300 > Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > On Sat, 3 Jul 2021 at 20:47, Jonathan Cameron <jic23@kernel.org> > wrote: > > > > > > On Mon, 28 Jun 2021 16:51:32 +0300 > > > Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > > > > > This one is a little easier to convert to device-managed, now with > the > > > > devm_krealloc() function. > > > > > > > > The other iio_triggered_buffer_setup() and iio_device_register() > can be > > > > converted to their devm_ variants. And devm_krealloc() can be > used to > > > > (re)alloc the buffer. When the driver unloads, this will also be > free'd. > > > > > > > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> > > > > --- > > > > drivers/iio/light/adjd_s311.c | 34 +++++----------------------------- > > > > 1 file changed, 5 insertions(+), 29 deletions(-) > > > > > > > > diff --git a/drivers/iio/light/adjd_s311.c > b/drivers/iio/light/adjd_s311.c > > > > index 17dac8d0e11d..19d60d6986a1 100644 > > > > --- a/drivers/iio/light/adjd_s311.c > > > > +++ b/drivers/iio/light/adjd_s311.c > > > > @@ -230,8 +230,8 @@ static int > adjd_s311_update_scan_mode(struct iio_dev *indio_dev, > > > > { > > > > struct adjd_s311_data *data = iio_priv(indio_dev); > > > > > > > > - kfree(data->buffer); > > > > - data->buffer = kmalloc(indio_dev->scan_bytes, > GFP_KERNEL); > > > > + data->buffer = devm_krealloc(indio_dev->dev.parent, data- > >buffer, > > > > + indio_dev->scan_bytes, GFP_KERNEL); > > > I got some complaints about exactly this trick in a review recently > so I'll > > > pass them on. > > > > > > Whilst devm_krealloc() usage like this won't lose the original > reference, its > > > not what people expect from a realloc() case, so to not confuse > people it is > > > better to do a dance where you use a local variable, then only set > data->buffer > > > to it once we know the realloc succeeded. > > > > > > That avoids this looking like the anti-pattern it would be if that > were a normal > > > realloc in which case you would just have leaked the original > allocation. > > > > > > More interestingly, why are we bothering with resizing the buffer > dependent on what > > > is enabled? Can't we just allocate a 128 byte buffer and not bother > changing it > > > as we really aren't wasting that much space? Just embed it in the > adjd_s311_data > > > structure directly and don't worry about the allocations. Will need > to be > > > aligned(8) though to avoid the push_to_buffer_with_timestamp() > issue. > > > Using something like > > > > > > struct { > > > s16 chans[4]; > > > s64 ts __aligned(8); /* I hate x86 32 bit */ > > > > do you want to me t also add this comment? :p > > [just kidding] > > > > > } scan; > > > > > > Inside the priv structure should work nicely. > > > > i agree; will do it like this; > > i hesitated a bit due to the inertia of converting things to devm_ > > A long discussion on rust usage in linux diverted into the issues around > devm. > I 'believe' that we are fine in IIO after some work Lars did a long time > back > to make us resilient to unbinds whilst the chardev was open, but > probably > worth keeping an eye on that discussion. > > https://urldefense.com/v3/__https://lore.kernel.org/ksummit/CANiq > 72nkNrekzbxMci6vW02w=Q2L- > SVTk_U4KN_LT8u_b=YPgw@mail.gmail.com/T/*m6db86a574237c22a3 > 2ecf49b596b3c2917967c5e__;Iw!!A3Ni8CS0y2Y!oeM8GJzKVXb8mYa1m > VJNw5fI2adsFk3FKkFzbnqyuDkUMKVTKQ3OoT0cnXP5rA$ > > I'm a tiny bit nervous that there might be races where we are doing > the devm_realloc. > I 'think' we are fine, but the 'think' and 'believe' in these statements > expresses > a slight lack of certainty! > > Jonathan > Hi, It's the second thread where I see you mentioning this, so this I will take the opportunity to also give a bit on though about this. I actually have in mind a RFC (hopefully sending it out this week) for this as I think we might still have some issues with open chardevs and device unbinding. What we have in [1] is not enough to make sure the whole thing is synchronized with device unbinding... We still have the door open to races where we call 'iio_buffer_ready()' or even 'rb->access->read()' after the device gets unbinded. Maybe we are lucky and nothing bad really happens and we just error out in the next time 'read()' is done on our fd. However, during the possible race, I think it's very likely that we end up touching the same data structures concurrently. On some devices, we surely (in theory and if all the stars align) have a path where 'iio_buffer_flush_hwfifo()' might be called with 'indio_dev->info' already set to NULL... IMO, the only way to have this fully in sync is to use the 'info_exist_lock' as it's done in [2]. I think [2] was actually "fixed" when Alex sent his patches for multi buffer support... Naturally, for the read case, we need to make sure we are not going to sleep with the mutex held so we might need an unlock -> lock dance which is not that nice. But I'm not really seeing another way. We also need to look at other file operations and also for the events case to see if this is also a thing. Naturally, I might be missing some subtlety and that's why I had this planned as RFC. But since is mentioned here, I thought I could bring this up as in the end I might not even need to send the patches :) [1]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-buffer.c#L117 [2]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-core.c#L1763 - Nuno Sá
On Mon, 12 Jul 2021 07:45:42 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Sunday, July 11, 2021 12:26 PM > > To: Alexandru Ardelean <aardelean@deviqon.com> > > Cc: linux-iio <linux-iio@vger.kernel.org>; Linux Kernel Mailing List > > <linux-kernel@vger.kernel.org>; pmeerw@pmeerw.net > > Subject: Re: [PATCH] iio: light: adjd_s311: convert to device-managed > > functions > > > > On Mon, 5 Jul 2021 09:38:21 +0300 > > Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > > > On Sat, 3 Jul 2021 at 20:47, Jonathan Cameron <jic23@kernel.org> > > wrote: > > > > > > > > On Mon, 28 Jun 2021 16:51:32 +0300 > > > > Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > > > > > > > This one is a little easier to convert to device-managed, now with > > the > > > > > devm_krealloc() function. > > > > > > > > > > The other iio_triggered_buffer_setup() and iio_device_register() > > can be > > > > > converted to their devm_ variants. And devm_krealloc() can be > > used to > > > > > (re)alloc the buffer. When the driver unloads, this will also be > > free'd. > > > > > > > > > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> > > > > > --- > > > > > drivers/iio/light/adjd_s311.c | 34 +++++----------------------------- > > > > > 1 file changed, 5 insertions(+), 29 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/light/adjd_s311.c > > b/drivers/iio/light/adjd_s311.c > > > > > index 17dac8d0e11d..19d60d6986a1 100644 > > > > > --- a/drivers/iio/light/adjd_s311.c > > > > > +++ b/drivers/iio/light/adjd_s311.c > > > > > @@ -230,8 +230,8 @@ static int > > adjd_s311_update_scan_mode(struct iio_dev *indio_dev, > > > > > { > > > > > struct adjd_s311_data *data = iio_priv(indio_dev); > > > > > > > > > > - kfree(data->buffer); > > > > > - data->buffer = kmalloc(indio_dev->scan_bytes, > > GFP_KERNEL); > > > > > + data->buffer = devm_krealloc(indio_dev->dev.parent, data- > > >buffer, > > > > > + indio_dev->scan_bytes, GFP_KERNEL); > > > > I got some complaints about exactly this trick in a review recently > > so I'll > > > > pass them on. > > > > > > > > Whilst devm_krealloc() usage like this won't lose the original > > reference, its > > > > not what people expect from a realloc() case, so to not confuse > > people it is > > > > better to do a dance where you use a local variable, then only set > > data->buffer > > > > to it once we know the realloc succeeded. > > > > > > > > That avoids this looking like the anti-pattern it would be if that > > were a normal > > > > realloc in which case you would just have leaked the original > > allocation. > > > > > > > > More interestingly, why are we bothering with resizing the buffer > > dependent on what > > > > is enabled? Can't we just allocate a 128 byte buffer and not bother > > changing it > > > > as we really aren't wasting that much space? Just embed it in the > > adjd_s311_data > > > > structure directly and don't worry about the allocations. Will need > > to be > > > > aligned(8) though to avoid the push_to_buffer_with_timestamp() > > issue. > > > > Using something like > > > > > > > > struct { > > > > s16 chans[4]; > > > > s64 ts __aligned(8); /* I hate x86 32 bit */ > > > > > > do you want to me t also add this comment? :p > > > [just kidding] > > > > > > > } scan; > > > > > > > > Inside the priv structure should work nicely. > > > > > > i agree; will do it like this; > > > i hesitated a bit due to the inertia of converting things to devm_ > > > > A long discussion on rust usage in linux diverted into the issues around > > devm. > > I 'believe' that we are fine in IIO after some work Lars did a long time > > back > > to make us resilient to unbinds whilst the chardev was open, but > > probably > > worth keeping an eye on that discussion. > > > > https://urldefense.com/v3/__https://lore.kernel.org/ksummit/CANiq > > 72nkNrekzbxMci6vW02w=Q2L- > > SVTk_U4KN_LT8u_b=YPgw@mail.gmail.com/T/*m6db86a574237c22a3 > > 2ecf49b596b3c2917967c5e__;Iw!!A3Ni8CS0y2Y!oeM8GJzKVXb8mYa1m > > VJNw5fI2adsFk3FKkFzbnqyuDkUMKVTKQ3OoT0cnXP5rA$ > > > > I'm a tiny bit nervous that there might be races where we are doing > > the devm_realloc. > > I 'think' we are fine, but the 'think' and 'believe' in these statements > > expresses > > a slight lack of certainty! > > > > Jonathan > > > > Hi, +CC Lars who might recall how this all works! > > It's the second thread where I see you mentioning this, so this I will take the > opportunity to also give a bit on though about this. I actually have in mind a RFC > (hopefully sending it out this week) for this as I think we might still have some > issues with open chardevs and device unbinding. > > What we have in [1] is not enough to make sure the whole thing is synchronized with > device unbinding... We still have the door open to races where we call 'iio_buffer_ready()' > or even 'rb->access->read()' after the device gets unbinded. Maybe we are lucky and > nothing bad really happens and we just error out in the next time 'read()' is done on > our fd. My understanding of that test is it was only intended to ensure a smooth exit 'after' the buffer pull down has occurred. From vague memory rather than careful analysis, the reason it is needed is we only send the break out signal once for a given buffer, so we need to be sure that userspace doesn't call read() then ignore the error returned due to the buffer going away mid read and call read() again. There may be races in the first time path though. In particularly I'm not sure the reference count on the buffer is raised during the read and it perhaps should be. > However, during the possible race, I think it's very likely that we end up touching > the same data structures concurrently. On some devices, we surely > (in theory and if all the stars align) have a path where 'iio_buffer_flush_hwfifo()' might > be called with 'indio_dev->info' already set to NULL... Yeah, the hwfifo stuff is more recent, it's definitely possible there is a race around that. > > IMO, the only way to have this fully in sync is to use the 'info_exist_lock' as it's done > in [2]. I think [2] was actually "fixed" when Alex sent his patches for multi buffer support... It's rather painful to take that lock. If we can make things safe with appropriate reference counting that's definitely preferable. For ioctl's they are always slow path so the exist_lock route is fine. > Naturally, for the read case, we need to make sure we are not going to sleep with the > mutex held so we might need an unlock -> lock dance which is not that nice. But I'm > not really seeing another way. We also need to look at other file operations and also > for the events case to see if this is also a thing. > > Naturally, I might be missing some subtlety and that's why I had this planned as RFC. > But since is mentioned here, I thought I could bring this up as in the end I might not > even need to send the patches :) Wise move :) I'd suggest that any fix in this space would ideally be accompanied by a confirmed race. Heavy use of sleeps can usually open one up enough to actually hit them in a few tries. Jonathan > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-buffer.c#L117 > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-core.c#L1763 > > - Nuno Sá
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Sent: Monday, July 12, 2021 12:37 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: Jonathan Cameron <jic23@kernel.org>; Alexandru Ardelean > <aardelean@deviqon.com>; linux-iio <linux-iio@vger.kernel.org>; > Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; > pmeerw@pmeerw.net; Lars-Peter Clausen <lars@metafoo.de> > Subject: Re: [PATCH] iio: light: adjd_s311: convert to device-managed > functions > > [External] > > On Mon, 12 Jul 2021 07:45:42 +0000 > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > From: Jonathan Cameron <jic23@kernel.org> > > > Sent: Sunday, July 11, 2021 12:26 PM > > > To: Alexandru Ardelean <aardelean@deviqon.com> > > > Cc: linux-iio <linux-iio@vger.kernel.org>; Linux Kernel Mailing List > > > <linux-kernel@vger.kernel.org>; pmeerw@pmeerw.net > > > Subject: Re: [PATCH] iio: light: adjd_s311: convert to device- > managed > > > functions > > > > > > On Mon, 5 Jul 2021 09:38:21 +0300 > > > Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > > > > > On Sat, 3 Jul 2021 at 20:47, Jonathan Cameron <jic23@kernel.org> > > > wrote: > > > > > > > > > > On Mon, 28 Jun 2021 16:51:32 +0300 > > > > > Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > > > > > > > > > This one is a little easier to convert to device-managed, now > with > > > the > > > > > > devm_krealloc() function. > > > > > > > > > > > > The other iio_triggered_buffer_setup() and > iio_device_register() > > > can be > > > > > > converted to their devm_ variants. And devm_krealloc() can > be > > > used to > > > > > > (re)alloc the buffer. When the driver unloads, this will also be > > > free'd. > > > > > > > > > > > > Signed-off-by: Alexandru Ardelean > <aardelean@deviqon.com> > > > > > > --- > > > > > > drivers/iio/light/adjd_s311.c | 34 +++++-------------------------- > --- > > > > > > 1 file changed, 5 insertions(+), 29 deletions(-) > > > > > > > > > > > > diff --git a/drivers/iio/light/adjd_s311.c > > > b/drivers/iio/light/adjd_s311.c > > > > > > index 17dac8d0e11d..19d60d6986a1 100644 > > > > > > --- a/drivers/iio/light/adjd_s311.c > > > > > > +++ b/drivers/iio/light/adjd_s311.c > > > > > > @@ -230,8 +230,8 @@ static int > > > adjd_s311_update_scan_mode(struct iio_dev *indio_dev, > > > > > > { > > > > > > struct adjd_s311_data *data = iio_priv(indio_dev); > > > > > > > > > > > > - kfree(data->buffer); > > > > > > - data->buffer = kmalloc(indio_dev->scan_bytes, > > > GFP_KERNEL); > > > > > > + data->buffer = devm_krealloc(indio_dev->dev.parent, > data- > > > >buffer, > > > > > > + indio_dev->scan_bytes, GFP_KERNEL); > > > > > I got some complaints about exactly this trick in a review > recently > > > so I'll > > > > > pass them on. > > > > > > > > > > Whilst devm_krealloc() usage like this won't lose the original > > > reference, its > > > > > not what people expect from a realloc() case, so to not confuse > > > people it is > > > > > better to do a dance where you use a local variable, then only > set > > > data->buffer > > > > > to it once we know the realloc succeeded. > > > > > > > > > > That avoids this looking like the anti-pattern it would be if that > > > were a normal > > > > > realloc in which case you would just have leaked the original > > > allocation. > > > > > > > > > > More interestingly, why are we bothering with resizing the > buffer > > > dependent on what > > > > > is enabled? Can't we just allocate a 128 byte buffer and not > bother > > > changing it > > > > > as we really aren't wasting that much space? Just embed it in > the > > > adjd_s311_data > > > > > structure directly and don't worry about the allocations. Will > need > > > to be > > > > > aligned(8) though to avoid the > push_to_buffer_with_timestamp() > > > issue. > > > > > Using something like > > > > > > > > > > struct { > > > > > s16 chans[4]; > > > > > s64 ts __aligned(8); /* I hate x86 32 bit */ > > > > > > > > do you want to me t also add this comment? :p > > > > [just kidding] > > > > > > > > > } scan; > > > > > > > > > > Inside the priv structure should work nicely. > > > > > > > > i agree; will do it like this; > > > > i hesitated a bit due to the inertia of converting things to devm_ > > > > > > A long discussion on rust usage in linux diverted into the issues > around > > > devm. > > > I 'believe' that we are fine in IIO after some work Lars did a long > time > > > back > > > to make us resilient to unbinds whilst the chardev was open, but > > > probably > > > worth keeping an eye on that discussion. > > > > > > > https://urldefense.com/v3/__https://lore.kernel.org/ksummit/CANiq > > > 72nkNrekzbxMci6vW02w=Q2L- > > > > SVTk_U4KN_LT8u_b=YPgw@mail.gmail.com/T/*m6db86a574237c22a3 > > > > 2ecf49b596b3c2917967c5e__;Iw!!A3Ni8CS0y2Y!oeM8GJzKVXb8mYa1m > > > VJNw5fI2adsFk3FKkFzbnqyuDkUMKVTKQ3OoT0cnXP5rA$ > > > > > > I'm a tiny bit nervous that there might be races where we are doing > > > the devm_realloc. > > > I 'think' we are fine, but the 'think' and 'believe' in these > statements > > > expresses > > > a slight lack of certainty! > > > > > > Jonathan > > > > > > > Hi, > > +CC Lars who might recall how this all works! > > > > > It's the second thread where I see you mentioning this, so this I will > take the > > opportunity to also give a bit on though about this. I actually have in > mind a RFC > > (hopefully sending it out this week) for this as I think we might still > have some > > issues with open chardevs and device unbinding. > > > > What we have in [1] is not enough to make sure the whole thing is > synchronized with > > device unbinding... We still have the door open to races where we > call 'iio_buffer_ready()' > > or even 'rb->access->read()' after the device gets unbinded. Maybe > we are lucky and > > nothing bad really happens and we just error out in the next time > 'read()' is done on > > our fd. > > My understanding of that test is it was only intended to ensure a > smooth exit 'after' the > buffer pull down has occurred. From vague memory rather than > careful analysis, the > reason it is needed is we only send the break out signal once for a > given buffer, > so we need to be sure that userspace doesn't call read() then ignore > the error returned > due to the buffer going away mid read and call read() again. There > may be races in the > first time path though. In particularly I'm not sure the reference count > on the buffer Yes, that is my concern. In theory , with the current code, there's no guarantee that some user might already be in a 'read()'-> passed the 'if (iio_dev->info)' check and then get's preempted for example. In that time, the device might very well be unbonded so that when the user is scheduled again, we will be left thinking the device is there but it won't. Or maybe even worst, we get re-scheduled in the exact time the device is unbonded so there's plenty of room for data races... As you stated, this is only a thing in the first path. And probably one of those concurrent things that might have a 1 in trillion chances to happen. But when they do, they are hell to debug :). > is raised during the read and it perhaps should be. > > > > However, during the possible race, I think it's very likely that we end > up touching > > the same data structures concurrently. On some devices, we surely > > (in theory and if all the stars align) have a path where > 'iio_buffer_flush_hwfifo()' might > > be called with 'indio_dev->info' already set to NULL... > > Yeah, the hwfifo stuff is more recent, it's definitely possible there is a > race around that. > > > > > IMO, the only way to have this fully in sync is to use the > 'info_exist_lock' as it's done > > in [2]. I think [2] was actually "fixed" when Alex sent his patches for > multi buffer support... > It's rather painful to take that lock. If we can make things safe with > appropriate reference > counting that's definitely preferable. > > For ioctl's they are always slow path so the exist_lock route is fine. > > > Naturally, for the read case, we need to make sure we are not going > to sleep with the > > mutex held so we might need an unlock -> lock dance which is not > that nice. But I'm > > not really seeing another way. We also need to look at other file > operations and also > > for the events case to see if this is also a thing. > > > > Naturally, I might be missing some subtlety and that's why I had this > planned as RFC. > > But since is mentioned here, I thought I could bring this up as in the > end I might not > > even need to send the patches :) > Wise move :) > > I'd suggest that any fix in this space would ideally be accompanied by a > confirmed race. Yeah, it makes sense but this is one of those cases where this is very hard to hit... - Nuno Sá
diff --git a/drivers/iio/light/adjd_s311.c b/drivers/iio/light/adjd_s311.c index 17dac8d0e11d..19d60d6986a1 100644 --- a/drivers/iio/light/adjd_s311.c +++ b/drivers/iio/light/adjd_s311.c @@ -230,8 +230,8 @@ static int adjd_s311_update_scan_mode(struct iio_dev *indio_dev, { struct adjd_s311_data *data = iio_priv(indio_dev); - kfree(data->buffer); - data->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); + data->buffer = devm_krealloc(indio_dev->dev.parent, data->buffer, + indio_dev->scan_bytes, GFP_KERNEL); if (data->buffer == NULL) return -ENOMEM; @@ -256,7 +256,6 @@ static int adjd_s311_probe(struct i2c_client *client, return -ENOMEM; data = iio_priv(indio_dev); - i2c_set_clientdata(client, indio_dev); data->client = client; indio_dev->info = &adjd_s311_info; @@ -265,34 +264,12 @@ static int adjd_s311_probe(struct i2c_client *client, indio_dev->num_channels = ARRAY_SIZE(adjd_s311_channels); indio_dev->modes = INDIO_DIRECT_MODE; - err = iio_triggered_buffer_setup(indio_dev, NULL, - adjd_s311_trigger_handler, NULL); + err = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, + adjd_s311_trigger_handler, NULL); if (err < 0) return err; - err = iio_device_register(indio_dev); - if (err) - goto exit_unreg_buffer; - - dev_info(&client->dev, "ADJD-S311 color sensor registered\n"); - - return 0; - -exit_unreg_buffer: - iio_triggered_buffer_cleanup(indio_dev); - return err; -} - -static int adjd_s311_remove(struct i2c_client *client) -{ - struct iio_dev *indio_dev = i2c_get_clientdata(client); - struct adjd_s311_data *data = iio_priv(indio_dev); - - iio_device_unregister(indio_dev); - iio_triggered_buffer_cleanup(indio_dev); - kfree(data->buffer); - - return 0; + return devm_iio_device_register(&client->dev, indio_dev); } static const struct i2c_device_id adjd_s311_id[] = { @@ -306,7 +283,6 @@ static struct i2c_driver adjd_s311_driver = { .name = ADJD_S311_DRV_NAME, }, .probe = adjd_s311_probe, - .remove = adjd_s311_remove, .id_table = adjd_s311_id, }; module_i2c_driver(adjd_s311_driver);
This one is a little easier to convert to device-managed, now with the devm_krealloc() function. The other iio_triggered_buffer_setup() and iio_device_register() can be converted to their devm_ variants. And devm_krealloc() can be used to (re)alloc the buffer. When the driver unloads, this will also be free'd. Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> --- drivers/iio/light/adjd_s311.c | 34 +++++----------------------------- 1 file changed, 5 insertions(+), 29 deletions(-)