[PATCH 00/14] pinctrl: rockchip: fix editing const struct + constify rockchip_pin_ctrl

[Ilias Apalodimas]

Hi Jonas,

On Thu, 30 Jan 2025 at 23:20, Jonas Karlman <jonas at kwiboo.se> wrote:
>
> Hi Quentin,
>
> On 2025-01-30 15:16, Quentin Schulz wrote:
> > Hi Jonas,
> >
> > On 1/29/25 3:20 PM, Jonas Karlman wrote:
> >> Hi Quentin,
> >>
> >> On 2025-01-29 13:42, Quentin Schulz wrote:
> >>> While testing some WIP work done by Ilias Apalodimas on guaranteeing
> >>> read-only memory areas truly are handled as read-only[1], my RK3588
> >>> Tiger couldn't reach U-Boot CLI anymore because of the pinctrl driver
> >>> modifying a const struct rockchip_pin_ctrl, triggering a CPU abort.
> >>>
> >>> Instead of going the lazy way and unconstify it, let's fix the actual
> >>> issue in play.
> >>>
> >>> The member modified in the const is only ever used for setting a member
> >>> from another struct (not const that one). However this other member is
> >>> never read! Therefore we can simply afford to remove it which means the
> >>> sole reader of the member in the const is now gone, thus making it
> >>> possible to remove the member from the const struct as well.
> >>>
> >>> This also means we should be able to constify the private data of the
> >>> controller device for all Rockchip devices, instead of having those only
> >>> for RK356x, RK3588 and RV1126. With the constify done on top of Ilias
> >>> branch[1], my PX30 Ringneck, RK3399 Puma and RK3588 Tiger all reach
> >>> U-Boot CLI. No further test (like booting into Linux userspace) was
> >>> done.
> >>>
> >>> [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/tree/fix_memory_permissions
> >>>
> >>> Signed-off-by: Quentin Schulz <quentin.schulz at cherry.de>
> >>> ---
> >>> Quentin Schulz (14):
> >>>        pinctrl: rockchip: remove unused base_pin bank member
> >>>        pinctrl: rockchip: remove unused nr_pins controller member
> >>
> >> pin_base and nr_pins is used after my "rockchip: pinctrl: Add support
> >> for pinmux status cmd" series [1].
> >>
> >> The pin_base should probably be moved to udevice priv data or similar.
> >> nr_pins can probably also be moved to udevice priv data or constify in
> >> driver data.
> >>
> >> Do you want me to re-work/re-store these fields in a different way once
> >> I finally send a v3 of that series?
> >>
> >
> > What I can suggest is to remove the setting of ctrl->nr_pins as I
> > believe we shouldn't be modifying it and we don't need to store it, even
> > in your patch series as we discussed on IRC, and keep pin_base from the
> > bank.
> >
> > What I can offer instead is to use a local variable to replace the one
> > from the controller struct.
> >
> > """
> > diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> > b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> > index d449d07d32e..14aba6f370b 100644
> > --- a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> > +++ b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> > @@ -531,20 +531,22 @@ static struct rockchip_pin_ctrl
> > *rockchip_pinctrl_get_soc_data(struct udevice *d
> >       struct rockchip_pin_ctrl *ctrl =
> >                       (struct rockchip_pin_ctrl *)dev_get_driver_data(dev);
> >       struct rockchip_pin_bank *bank;
> > -     int grf_offs, pmu_offs, drv_grf_offs, drv_pmu_offs, i, j;
> > +     int grf_offs, pmu_offs, drv_grf_offs, drv_pmu_offs, ctrl_nr_pins, i, j;
> >
> >       grf_offs = ctrl->grf_mux_offset;
> >       pmu_offs = ctrl->pmu_mux_offset;
> >       drv_pmu_offs = ctrl->pmu_drv_offset;
> >       drv_grf_offs = ctrl->grf_drv_offset;
> >       bank = ctrl->pin_banks;
> > +     ctrl_nr_pins = 0;
> > +
> >
> >       for (i = 0; i < ctrl->nr_banks; ++i, ++bank) {
> >               int bank_pins = 0;
> >
> >               bank->priv = priv;
> > -             bank->pin_base = ctrl->nr_pins;
> > -             ctrl->nr_pins += bank->nr_pins;
> > +             bank->pin_base = ctrl_nr_pins;
> > +             ctrl_nr_pins += bank->nr_pins;
> >
> >               /* calculate iomux and drv offsets */
> >               for (j = 0; j < 4; j++) {
> > """
> >
> > Something like that to replace patches 1 and 2. This should allow you to
> > keep working on your other patch series without me making it too much
> > more difficult for you to rebase?
>
> Thanks for taking a look, and I agree this should help when I pick up
> and re-spin my series.
>
> >
> > We need this patch series in before
> > https://lore.kernel.org/u-boot/20250130072100.27297-1-ilias.apalodimas@linaro.org/T/#t
> > gets is otherwise we don't be able to boot anymore on RK356x, RK3588 and
> > RV1126. I don't think there's going to be much bikeshedding or
> > controversy about my series here (in v2 if you agree with the above
> > diff), the pinctrl series from last August seems like it could be
> > opening many can of worms so better not rebase this series on top of
> > that series. Do you agree with that?
>
> I fully agree, better to get this in first, I will try to re-spin my
> series on top of this sometime later once this has landed.

Ok, I'll post this after the weekend in case I get more review
comments. Keep in mind that i'll hide it behind a Kconfig option for
now, since we seem to have enough cases of code writing const
variables

Cheers
/Ilias
>
> Regards,
> Jonas
>
> >
> > Cheers,
> > Quentin
>