linux-kernel - Re: [PATCH 7/8] reset: amlogic: add auxiliary reset driver support

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: <1jv81cgv4z.fsf@starbuckisacylon.baylibre.com>
Date: Thu, 11 Jul 2024 11:01:16 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>,  Philipp Zabel
 <p.zabel@...gutronix.de>,  Jan Dakinevich
 <jan.dakinevich@...utedevices.com>,  linux-kernel@...r.kernel.org,
  linux-amlogic@...ts.infradead.org,  linux-clk@...r.kernel.org
Subject: Re: [PATCH 7/8] reset: amlogic: add auxiliary reset driver support

On Wed 10 Jul 2024 at 15:49, Stephen Boyd <sboyd@...nel.org> wrote:

> Quoting Jerome Brunet (2024-07-10 09:25:16)
>> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
>> index e34a10b15593..5cc767d50e8f 100644
>> --- a/drivers/reset/reset-meson.c
>> +++ b/drivers/reset/reset-meson.c
> [...]
>> +
>> +int devm_meson_rst_aux_register(struct device *dev,
>> +                               struct regmap *map,
>> +                               const char *adev_name)
>> +{
>> +       struct meson_reset_adev *raux;
>> +       struct auxiliary_device *adev;
>> +       int ret;
>> +
>> +       raux = kzalloc(sizeof(*raux), GFP_KERNEL);
>> +       if (!raux)
>> +               return -ENOMEM;
>> +
>> +       ret = ida_alloc(&meson_rst_aux_ida, GFP_KERNEL);
>
> Do we expect more than one device with the same name? I wonder if the
> IDA can be skipped.

I've wondered about that too.

I don't think it is the case right now but I'm not 100% sure.
Since I spent time thinking about it, I thought it would just be safer (and
relatively cheap) to put in and enough annoying debugging the
expectation does not hold true.

I don't have a strong opinion on this. What do you prefer ?

>
>> +       if (ret < 0)
>> +               goto raux_free;
>> +
>> +       raux->map = map;
>> +
>> +       adev = &raux->adev;
>> +       adev->id = ret;
>> +       adev->name = adev_name;
>> +       adev->dev.parent = dev;
>> +       adev->dev.release = meson_rst_aux_release;
>> +       device_set_of_node_from_dev(&adev->dev, dev);
>> +
>> +       ret = auxiliary_device_init(adev);
>> +       if (ret)
>> +               goto ida_free;
>> +
>> +       ret = __auxiliary_device_add(adev, dev->driver->name);
>> +       if (ret) {
>> +               auxiliary_device_uninit(adev);
>> +               return ret;
>> +       }
>> +
>> +       return devm_add_action_or_reset(dev, meson_rst_aux_unregister_adev,
>> +                                       adev);
>> +
>> +ida_free:
>> +       ida_free(&meson_rst_aux_ida, adev->id);
>> +raux_free:
>> +       kfree(raux);
>> +       return ret;
>> +
>
> Nitpick: Drop extra newline?
>
>> +}
>> +EXPORT_SYMBOL_GPL(devm_meson_rst_aux_register);
>> +
>> +MODULE_DESCRIPTION("Amlogic Meson Reset driver");
>>  MODULE_AUTHOR("Neil Armstrong <narmstrong@...libre.com>");
>> +MODULE_AUTHOR("Jerome Brunet <jbrunet@...libre.com>");
>>  MODULE_LICENSE("Dual BSD/GPL");
>> diff --git a/include/soc/amlogic/meson-auxiliary-reset.h b/include/soc/amlogic/meson-auxiliary-reset.h
>> new file mode 100644
>> index 000000000000..8fdb02b18d8c
>> --- /dev/null
>> +++ b/include/soc/amlogic/meson-auxiliary-reset.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __SOC_AMLOGIC_MESON_AUX_RESET_H
>> +#define __SOC_AMLOGIC_MESON_AUX_RESET_H
>> +
>> +#include <linux/err.h>
>> +
>> +struct device;
>> +struct regmap;
>> +
>> +#ifdef CONFIG_RESET_MESON
>> +int devm_meson_rst_aux_register(struct device *dev,
>> +                               struct regmap *map,
>> +                               const char *adev_name);
>> +#else
>> +static inline int devm_meson_rst_aux_register(struct device *dev,
>> +                                             struct regmap *map,
>> +                                             const char *adev_name)
>> +{
>> +       return -EOPNOTSUPP;
>
> Shouldn't this be 'return 0' so that the clk driver doesn't have to care
> about the config?

I don't think the system (in general) would be able function without the reset
driver, so the question is rather phylosophical.

Let's say it could, if this returns 0, consumers of the reset controller
will defer indefinitely ... which is always a bit more difficult to sort
out.

If it returns an error, the problem is pretty obvious, helping people
solve it quickly.

Also the actual device we trying to register provides clocks and reset.
It is not like the reset is an optional part we don't care about.

On this instance it starts from clock, but it could have been the other
way around. Both are equally important.

I'd prefer if it returns an error when the registration can't even start.

-- 
Jerome