Re: [PATCH 3/6] scripts/coccinelle: New script to add ResetType to hold and exit phases

[Luc Michel]

On 17:08 Fri 12 Apr     , Peter Maydell wrote:
> We pass a ResetType argument to the Resettable class enter phase
> method, but we don't pass it to hold and exit, even though the
> callsites have it readily available.  This means that if a device
> cared about the ResetType it would need to record it in the enter
> phase method to use later on.  We should pass the type to all three
> of the phase methods to avoid having to do that.
> 
> This coccinelle script adds the ResetType argument to the hold and
> exit phases of the Resettable interface.
> 
> The first part of the script (rules holdfn_assigned, holdfn_defined,
> exitfn_assigned, exitfn_defined) update implementations of the
> interface within device models, both to change the signature of their
> method implementations and to pass on the reset type when they invoke
> reset on some other device.
> 
> The second part of the script is various special cases:
>  * method callsites in resettable_phase_hold(), resettable_phase_exit()
>    and device_phases_reset()
>  * updating the typedefs for the methods
>  * isl_pmbus_vr.c has some code where one device's reset method directly
>    calls the implementation of a different device's method
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc.michel@amd.com>

(I'm not a coccinelle expert but LGTM)

> ---
> The structure here is a bit of an experiment: usually I would make
> the coccinelle script cover the main mechanical change and do the
> special cases by hand-editing. But I thought it might be clearer to
> have the entire next commit be made by coccinelle, so reviewers don't
> have to go hunting through a 99% automated commit for the 1% hand
> written part. Let me know whether you like this or not...
> ---
>  scripts/coccinelle/reset-type.cocci | 133 ++++++++++++++++++++++++++++
>  1 file changed, 133 insertions(+)
>  create mode 100644 scripts/coccinelle/reset-type.cocci
> 
> diff --git a/scripts/coccinelle/reset-type.cocci 
> b/scripts/coccinelle/reset-type.cocci
> new file mode 100644
> index 00000000000..14abdd7bd0c
> --- /dev/null
> +++ b/scripts/coccinelle/reset-type.cocci
> @@ -0,0 +1,133 @@
> +// Convert device code using three-phase reset to add a ResetType
> +// argument to implementations of ResettableHoldPhase and
> +// ResettableEnterPhase methods.
> +//
> +// Copyright Linaro Ltd 2024
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +//
> +// for dir in include hw target; do \
> +// spatch --macro-file scripts/cocci-macro-file.h \
> +//        --sp-file scripts/coccinelle/reset-type.cocci \
> +//        --keep-comments --smpl-spacing --in-place --include-headers \
> +//        --dir $dir; done
> +//
> +// This coccinelle script aims to produce a complete change that needs
> +// no human interaction, so as well as the generic "update device
> +// implementations of the hold and exit phase methods" it includes
> +// the special-case transformations needed for the core code and for
> +// one device model that does something a bit nonstandard. Those
> +// special cases are at the end of the file.
> +
> +// Look for where we use a function as a ResettableHoldPhase method,
> +// either by directly assigning it to phases.hold or by calling
> +// resettable_class_set_parent_phases, and remember the function name.
> +@ holdfn_assigned @
> +identifier enterfn, holdfn, exitfn;
> +identifier rc;
> +expression e;
> +@@
> +ResettableClass *rc;
> +...
> +(
> + rc->phases.hold = holdfn;
> +|
> + resettable_class_set_parent_phases(rc, enterfn, holdfn, exitfn, e);
> +)
> +
> +// Look for the definition of the function we found in holdfn_assigned,
> +// and add the new argument. If the function calls a hold function
> +// itself (probably chaining to the parent class reset) then add the
> +// new argument there too.
> +@ holdfn_defined @
> +identifier holdfn_assigned.holdfn;
> +typedef Object;
> +identifier obj;
> +expression parent;
> +@@
> +-holdfn(Object *obj)
> ++holdfn(Object *obj, ResetType type)
> +{
> +    <...
> +-    parent.hold(obj)
> ++    parent.hold(obj, type)
> +    ...>
> +}
> +
> +// Similarly for ResettableExitPhase.
> +@ exitfn_assigned @
> +identifier enterfn, holdfn, exitfn;
> +identifier rc;
> +expression e;
> +@@
> +ResettableClass *rc;
> +...
> +(
> + rc->phases.exit = exitfn;
> +|
> + resettable_class_set_parent_phases(rc, enterfn, holdfn, exitfn, e);
> +)
> +@ exitfn_defined @
> +identifier exitfn_assigned.exitfn;
> +typedef Object;
> +identifier obj;
> +expression parent;
> +@@
> +-exitfn(Object *obj)
> ++exitfn(Object *obj, ResetType type)
> +{
> +    <...
> +-    parent.exit(obj)
> ++    parent.exit(obj, type)
> +    ...>
> +}
> +
> +// SPECIAL CASES ONLY BELOW HERE
> +// We use a python scripted constraint on the position of the match
> +// to ensure that they only match in a particular function. See
> +// https://public-inbox.org/git/alpine.DEB.2.21.1808240652370.2344@hadrien/
> +// which recommends this as the way to do "match only in this function".
> +
> +// Special case: isl_pmbus_vr.c has some reset methods calling others 
> directly
> +@ isl_pmbus_vr @
> +identifier obj;
> +@@
> +- isl_pmbus_vr_exit_reset(obj);
> ++ isl_pmbus_vr_exit_reset(obj, type);
> +
> +// Special case: device_phases_reset() needs to pass RESET_TYPE_COLD
> +@ device_phases_reset_hold @
> +expression obj;
> +identifier rc;
> +identifier phase;
> +position p : script:python() { p[0].current_element == "device_phases_reset" 
> };
> +@@
> +- rc->phases.phase(obj)@p
> ++ rc->phases.phase(obj, RESET_TYPE_COLD)
> +
> +// Special case: in resettable_phase_hold() and resettable_phase_exit()
> +// we need to pass through the ResetType argument to the method being called
> +@ resettable_phase_hold @
> +expression obj;
> +identifier rc;
> +position p : script:python() { p[0].current_element == 
> "resettable_phase_hold" };
> +@@
> +- rc->phases.hold(obj)@p
> ++ rc->phases.hold(obj, type)
> +@ resettable_phase_exit @
> +expression obj;
> +identifier rc;
> +position p : script:python() { p[0].current_element == 
> "resettable_phase_exit" };
> +@@
> +- rc->phases.exit(obj)@p
> ++ rc->phases.exit(obj, type)
> +// Special case: the typedefs for the methods need to declare the new 
> argument
> +@ phase_typedef_hold @
> +identifier obj;
> +@@
> +- typedef void (*ResettableHoldPhase)(Object *obj);
> ++ typedef void (*ResettableHoldPhase)(Object *obj, ResetType type);
> +@ phase_typedef_exit @
> +identifier obj;
> +@@
> +- typedef void (*ResettableExitPhase)(Object *obj);
> ++ typedef void (*ResettableExitPhase)(Object *obj, ResetType type);
> --
> 2.34.1
> 
> 

--