On Tue, Jun 14, 2022 at 11:11 AM Andrea Merello <[email protected]> wrote:
...
> >> + /* G: 2, 4, 8, 16 */
> >
> >Indentation of this comment is a bit off.
> >
> >> +static int bno055_acc_range_vals[] = {1962, 3924, 7848, 15696};
> >
> >Perhaps split this to 4 lines and put the comment on top of the third line?
>
> Not sure what you mean here, sorry. May you elaborate or provide an example, please?
static int ... [] = {
/* Comment goes here */
value1, value2, ..., valueN,
};
...
> >> +static void bno055_debugfs_init(struct iio_dev *iio_dev)
> >> +{
> >> + struct bno055_priv *priv = iio_priv(iio_dev);
> >> +
> >> + priv->debugfs = debugfs_create_file("firmware_version", 0400,
> >> + iio_get_debugfs_dentry(iio_dev),
> >> + priv, &bno055_fw_version_ops);
> >
> >> + devm_add_action_or_reset(priv->dev, bno055_debugfs_remove, priv->debugfs);
> >
> >Shouldn't we report the potential error here? It's not directly
> >related to debugfs, but something which is not directly related.
>
> The error eventually comes out from something that has nothing to do with debugs per se (i.e. the devm stuff), but it will only affect debugfs indeed.
>
> Assuming that we don't want to make the whole driver fail in case debugfs stuff fails (see last part of the comment above debugfs_create_file() implementation), and given that the devm_add_action_or_reset(), should indeed "reset" in case of failure (i.e. we should be in a clean situation anyway), I would say it should be OK not to propagate the error and let things go on.
As I said, it's not directly related to debugfs. Here is the resource
leak possible or bad things happen if you probe the driver, that fails
to add this call for removal, remove it, and try to insert again, in
such case the debugfs will be stale.
> However we can add a dev_warn() to report what happened.
Not sure if it would suffice, I leave it to Jonathan.
--
With Best Regards,
Andy Shevchenko
>
> ...
>
>> >> +static void bno055_debugfs_init(struct iio_dev *iio_dev)
>> >> +{
>> >> + struct bno055_priv *priv = iio_priv(iio_dev);
>> >> +
>> >> + priv->debugfs = debugfs_create_file("firmware_version", 0400,
>> >> + iio_get_debugfs_dentry(iio_dev),
>> >> + priv, &bno055_fw_version_ops);
>> >
>> >> + devm_add_action_or_reset(priv->dev, bno055_debugfs_remove, priv->debugfs);
>> >
>> >Shouldn't we report the potential error here? It's not directly
>> >related to debugfs, but something which is not directly related.
>>
>> The error eventually comes out from something that has nothing to do with debugs per se (i.e. the devm stuff), but it will only affect debugfs indeed.
>>
>> Assuming that we don't want to make the whole driver fail in case debugfs stuff fails (see last part of the comment above debugfs_create_file() implementation), and given that the devm_add_action_or_reset(), should indeed "reset" in case of failure (i.e. we should be in a clean situation anyway), I would say it should be OK not to propagate the error and let things go on.
>
>As I said, it's not directly related to debugfs. Here is the resource
>leak possible or bad things happen if you probe the driver, that fails
>to add this call for removal, remove it, and try to insert again, in
>such case the debugfs will be stale.
Hum, I would say this shouldn't ever happen: AFAICS devm_add_action_or_reset() is a wrapper around devm_add_action() and it's purpose is exactly to add a check for failure; devm_add_action_or_reset() immediately invokes the action handler in case devm_add_action() fails. IOW in case of failure to add the devm stuff, the debugfs file is removed immediately and it shouldn't cause any mess with next times probe()s; just the driver will go on without the debugfs file being here.
I think this is the point of using devm_add_action_or_reset() instead of dev_add_action() indeed, or am I missing something?
>> However we can add a dev_warn() to report what happened.
>
>Not sure if it would suffice, I leave it to Jonathan.
>
>--
>With Best Regards,
>Andy Shevchenko
On Tue, Jun 14, 2022 at 2:15 PM Andrea Merello <[email protected]> wrote:
...
> >> >> + devm_add_action_or_reset(priv->dev, bno055_debugfs_remove, priv->debugfs);
> >> >
> >> >Shouldn't we report the potential error here? It's not directly
> >> >related to debugfs, but something which is not directly related.
> >>
> >> The error eventually comes out from something that has nothing to do with debugs per se (i.e. the devm stuff), but it will only affect debugfs indeed.
> >>
> >> Assuming that we don't want to make the whole driver fail in case debugfs stuff fails (see last part of the comment above debugfs_create_file() implementation), and given that the devm_add_action_or_reset(), should indeed "reset" in case of failure (i.e. we should be in a clean situation anyway), I would say it should be OK not to propagate the error and let things go on.
> >
> >As I said, it's not directly related to debugfs. Here is the resource
> >leak possible or bad things happen if you probe the driver, that fails
> >to add this call for removal, remove it, and try to insert again, in
> >such case the debugfs will be stale.
>
> Hum, I would say this shouldn't ever happen: AFAICS devm_add_action_or_reset() is a wrapper around devm_add_action() and it's purpose is exactly to add a check for failure; devm_add_action_or_reset() immediately invokes the action handler in case devm_add_action() fails. IOW in case of failure to add the devm stuff, the debugfs file is removed immediately and it shouldn't cause any mess with next times probe()s; just the driver will go on without the debugfs file being here.
>
> I think this is the point of using devm_add_action_or_reset() instead of dev_add_action() indeed, or am I missing something?
Reading that code again and I think you are right, so dev_warn() will
be sufficient to show that we fail. OTOH, what is the point of adding
a resource for the failed debugfs call?
--
With Best Regards,
Andy Shevchenko
Il giorno mar 14 giu 2022 alle ore 17:11 Andy Shevchenko
<[email protected]> ha scritto:
>
> On Tue, Jun 14, 2022 at 2:15 PM Andrea Merello <[email protected]> wrote:
>
> ...
>
> > >> >> + devm_add_action_or_reset(priv->dev, bno055_debugfs_remove, priv->debugfs);
> > >> >
> > >> >Shouldn't we report the potential error here? It's not directly
> > >> >related to debugfs, but something which is not directly related.
> > >>
> > >> The error eventually comes out from something that has nothing to do with debugs per se (i.e. the devm stuff), but it will only affect debugfs indeed.
> > >>
> > >> Assuming that we don't want to make the whole driver fail in case debugfs stuff fails (see last part of the comment above debugfs_create_file() implementation), and given that the devm_add_action_or_reset(), should indeed "reset" in case of failure (i.e. we should be in a clean situation anyway), I would say it should be OK not to propagate the error and let things go on.
> > >
> > >As I said, it's not directly related to debugfs. Here is the resource
> > >leak possible or bad things happen if you probe the driver, that fails
> > >to add this call for removal, remove it, and try to insert again, in
> > >such case the debugfs will be stale.
> >
> > Hum, I would say this shouldn't ever happen: AFAICS devm_add_action_or_reset() is a wrapper around devm_add_action() and it's purpose is exactly to add a check for failure; devm_add_action_or_reset() immediately invokes the action handler in case devm_add_action() fails. IOW in case of failure to add the devm stuff, the debugfs file is removed immediately and it shouldn't cause any mess with next times probe()s; just the driver will go on without the debugfs file being here.
> >
> > I think this is the point of using devm_add_action_or_reset() instead of dev_add_action() indeed, or am I missing something?
>
> Reading that code again and I think you are right, so dev_warn() will
> be sufficient to show that we fail. OTOH, what is the point of adding
> a resource for the failed debugfs call?
Ah, you are right here: I'll make the call to
devm_add_action_or_reset() conditional to success of
debugfs_create_file(). In case any of the two fails we can also warn
the user.
> --
> With Best Regards,
> Andy Shevchenko