Most likely due to copy-paste mistake the device managed version of the
denoted reset control getter has been implemented with invalid semantic,
which can be immediately spotted by having "WARN_ON(shared && acquired)"
warning in the system log as soon as the method is called. Anyway let's
fix it by altering the boolean arguments passed to the
__devm_reset_control_bulk_get() method from
- shared = true, optional = false, acquired = true
to
+ shared = false, optional = true, acquired = true
That's what they were supposed to be in the first place (see the non-devm
version of the same method: reset_control_bulk_get_optional_exclusive()).
Fixes: 48d71395896d ("reset: Add reset_control_bulk API")
Signed-off-by: Serge Semin <[email protected]>
---
Changelog v4:
- This is a new patch added on v4 lap of the series.
---
include/linux/reset.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 8a21b5756c3e..514ddf003efc 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -731,7 +731,7 @@ static inline int __must_check
devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs,
struct reset_control_bulk_data *rstcs)
{
- return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true);
+ return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true);
}
/**
--
2.35.1
On 6/24/22 17:18, Serge Semin wrote:
> Most likely due to copy-paste mistake the device managed version of the
> denoted reset control getter has been implemented with invalid semantic,
> which can be immediately spotted by having "WARN_ON(shared && acquired)"
> warning in the system log as soon as the method is called. Anyway let's
> fix it by altering the boolean arguments passed to the
> __devm_reset_control_bulk_get() method from
> - shared = true, optional = false, acquired = true
> to
> + shared = false, optional = true, acquired = true
> That's what they were supposed to be in the first place (see the non-devm
> version of the same method: reset_control_bulk_get_optional_exclusive()).
>
> Fixes: 48d71395896d ("reset: Add reset_control_bulk API")
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Changelog v4:
> - This is a new patch added on v4 lap of the series.
> ---
> include/linux/reset.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index 8a21b5756c3e..514ddf003efc 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -731,7 +731,7 @@ static inline int __must_check
> devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs,
> struct reset_control_bulk_data *rstcs)
> {
> - return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true);
> + return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true);
> }
>
> /**
Good catch,
Reviewed-by: Dmitry Osipenko <[email protected]>
Although, this patch should be sent as standalone since it's unrelated
to the rest of the clk patches.
--
Best regards,
Dmitry
On Mon, Jun 27, 2022 at 01:35:28AM +0300, Dmitry Osipenko wrote:
> On 6/24/22 17:18, Serge Semin wrote:
> > Most likely due to copy-paste mistake the device managed version of the
> > denoted reset control getter has been implemented with invalid semantic,
> > which can be immediately spotted by having "WARN_ON(shared && acquired)"
> > warning in the system log as soon as the method is called. Anyway let's
> > fix it by altering the boolean arguments passed to the
> > __devm_reset_control_bulk_get() method from
> > - shared = true, optional = false, acquired = true
> > to
> > + shared = false, optional = true, acquired = true
> > That's what they were supposed to be in the first place (see the non-devm
> > version of the same method: reset_control_bulk_get_optional_exclusive()).
> >
> > Fixes: 48d71395896d ("reset: Add reset_control_bulk API")
> > Signed-off-by: Serge Semin <[email protected]>
> >
> > ---
> >
> > Changelog v4:
> > - This is a new patch added on v4 lap of the series.
> > ---
> > include/linux/reset.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/reset.h b/include/linux/reset.h
> > index 8a21b5756c3e..514ddf003efc 100644
> > --- a/include/linux/reset.h
> > +++ b/include/linux/reset.h
> > @@ -731,7 +731,7 @@ static inline int __must_check
> > devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs,
> > struct reset_control_bulk_data *rstcs)
> > {
> > - return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true);
> > + return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true);
> > }
> >
> > /**
>
> Good catch,
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
Thanks.
>
> Although, this patch should be sent as standalone since it's unrelated
> to the rest of the clk patches.
Mainly you are right, but I've discovered the problem in the framework
of this patchset development. So it was more suitable for me to submit
the patch in the framework of it.
There is an implicit context too. Due to the patchset containing the
reset-control-related patches I has been trying to attract the
Philipp' attention for about four months now, but with no luck.
Having the kernel API fix-patch in the series was supposed to
implicitly raise the patchset valuability for him. I don't know what
else to do, but Stephen said he wanted to see what Philipp thought of
the reset-part of the series.
-Sergey
>
> --
> Best regards,
> Dmitry
Hi Serge,
On Fr, 2022-06-24 at 17:18 +0300, Serge Semin wrote:
> Most likely due to copy-paste mistake the device managed version of the
> denoted reset control getter has been implemented with invalid semantic,
> which can be immediately spotted by having "WARN_ON(shared && acquired)"
> warning in the system log as soon as the method is called. Anyway let's
> fix it by altering the boolean arguments passed to the
> __devm_reset_control_bulk_get() method from
> - shared = true, optional = false, acquired = true
> to
> + shared = false, optional = true, acquired = true
> That's what they were supposed to be in the first place (see the non-devm
> version of the same method: reset_control_bulk_get_optional_exclusive()).
>
> Fixes: 48d71395896d ("reset: Add reset_control_bulk API")
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Changelog v4:
> - This is a new patch added on v4 lap of the series.
> ---
> include/linux/reset.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index 8a21b5756c3e..514ddf003efc 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -731,7 +731,7 @@ static inline int __must_check
> devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs,
> struct reset_control_bulk_data *rstcs)
> {
> - return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true);
> + return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true);
> }
You are right, thank you. I'll take this as a sign that the internal
functions should probably switch from collecting ever more boolean
arguments to a flags parameter, similar to enum gpiod_flags.
I'll pick this up separately into the reset/fixes branch.
regards
Philipp
On Wed, Jun 29, 2022 at 04:02:43PM +0200, Philipp Zabel wrote:
> Hi Serge,
>
> On Fr, 2022-06-24 at 17:18 +0300, Serge Semin wrote:
> > Most likely due to copy-paste mistake the device managed version of the
> > denoted reset control getter has been implemented with invalid semantic,
> > which can be immediately spotted by having "WARN_ON(shared && acquired)"
> > warning in the system log as soon as the method is called. Anyway let's
> > fix it by altering the boolean arguments passed to the
> > __devm_reset_control_bulk_get() method from
> > - shared = true, optional = false, acquired = true
> > to
> > + shared = false, optional = true, acquired = true
> > That's what they were supposed to be in the first place (see the non-devm
> > version of the same method: reset_control_bulk_get_optional_exclusive()).
> >
> > Fixes: 48d71395896d ("reset: Add reset_control_bulk API")
> > Signed-off-by: Serge Semin <[email protected]>
> >
> > ---
> >
> > Changelog v4:
> > - This is a new patch added on v4 lap of the series.
> > ---
> > ?include/linux/reset.h | 2 +-
> > ?1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/reset.h b/include/linux/reset.h
> > index 8a21b5756c3e..514ddf003efc 100644
> > --- a/include/linux/reset.h
> > +++ b/include/linux/reset.h
> > @@ -731,7 +731,7 @@ static inline int __must_check
> > ?devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs,
> > ? struct reset_control_bulk_data *rstcs)
> > ?{
> > - return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true);
> > + return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true);
> > ?}
>
> You are right, thank you. I'll take this as a sign that the internal
> functions should probably switch from collecting ever more boolean
> arguments to a flags parameter, similar to enum gpiod_flags.
>
> I'll pick this up separately into the reset/fixes branch.
Ok. Thanks. Meanwhile could you please take a look at the rest of the
reset-related patches of the series? They concern the reset-control
functionality updates of the Baikal-T1 CCU common-clock driver.
[PATCH RESEND v5 6/8] clk: baikal-t1: Move reset-controls code into a dedicated module
Link: https://lore.kernel.org/linux-clk/[email protected]/
[PATCH RESEND v5 7/8] clk: baikal-t1: Add DDR/PCIe directly controlled resets support
Link: https://lore.kernel.org/linux-clk/[email protected]/
The functionality has been placed in the clocks subsystem because the
reset-control is tightly coupled with the clock-control part of CCU.
It's a normal approach for the SoC CCU/RCU devices, which normally
have both clocks and reset controls intermixed. Anyway the main goal
of the series was to add the directly controlled resets of the
DDR and PCIe SoC sub-domains provided in the framework of the patch
7/8. Please see the patches log for details.
-Sergey
>
> regards
> Philipp