Re: [PATCH RFC 0/3] amba: bus: Move reading periphid operation from amba_match() to amba_probe()

[Saravana Kannan]

On Tue, Sep 10, 2024 at 5:17 AM Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
>
> On 2024/9/9 15:24, Saravana Kannan wrote:
> > On Sun, Sep 8, 2024 at 4:38 PM Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
> >>
> >> This patch series is to make amba_match(), as bus_type @amba_bustype's
> >> match(), also follow below ideal rule:
> >>
> >> bus_type's match() should only return bool type compatible integer 0 or
> >> 1 ideally since its main operations are lookup and comparison normally.
> >>
> >> Which has been followed by match() of all other bus_types in current
> >> kernel tree.
> >
> > The intent or need for this patch series is completely unclear. The
> > code you are moving around was also pretty delicate and hard to get
> > right.
> >
> > Without a much better description for why we need this, I'd like to
> > give this a NACK.
> >
> > Also, patch 3/3 is not at all easy to understand and seems to be doing
> > way more than what the commit message is trying to do.
> >
>
> thanks for your code review.
>
> let me explain the issue here firstly to go on with discussion, will
> correct it by next revision.
>
> amba_match(), as bus_type @amba_bustype's match(), operate hardware to
> read id, may return -EPROBE_DEFER consequently.
>
> this design is not very good and has several disadvantages shown below:
>
> 1) it is not good time to operate hardware in a bus_type's match().
>    hardware is not ready to operate normally in a bus_type's match()
>    as driver_probe_device() shown, there are still many preparations
>    to make hardware to operate after a bus_type's match(), for example,
>    resuming device and its ancestors, ensuring all its suppliers have
>    drivers bound, activating its power domain, ...
>
> 2) it should not operate hardware in a bus_type's match().
>    a bus_type's match() will obviously be triggered frequently, and
> hardware operation is slow normally, it will reduce efficiency for
> device attaching driver if operate hardware in a bus_type's match().
>
>    a bus_type's match() will become not reentry for a device and driver
>    if operating hardware is failed but can't recover initial hardware state.
>
> 3) for driver_attach(), a bus_type's match() are called without
>    device_lock(dev) firstly, it often causes concurrent issue when
> operate hardware within a bus_type's match(), look at below AMBA related
> fix:
>    Commit: 25af7406df59 ("ARM: 9229/1: amba: Fix use-after-free in
> amba_read_periphid()")
>    which introduce an extra @periphid_lock to fix this issue.
>
> 4) it may not be proper for a bus_type's match() to return -EPROBE_DEFER
> which will stop driver API bus_rescan_devices() from scanning other
> remaining devices, that is not expected as discussed by below thread:
>
> https://lore.kernel.org/all/20240904-bus_match_unlikely-v1-3-122318285261@xxxxxxxxxxx/
>
> 5) amba_match() is the only bus_type's match which breaks below ideal
> rule in current kernel tree:
>    bus_type's match() should only return bool type compatible integer 0
> or 1 ideally since its main operations are lookup and comparison normally.

All of this used to happen even if the bus match wasn't doing what
it's doing today. You don't seem to have full context on how amba
devices are added and probed. What you see now is a clean
up/simplification of how things used to work.

Please go read this patch history and git log history for these files
to get more context.

Nack for the entire series. It'll never go in.

-Saravana

>
>
> Our purpose is to solve this issue then enforce the ideal rule mentioned
> in 5).
>
> so we send this patch series to start a topic about how to solve this
> issue (^^).
>
> > -Saravana
> >
>
>