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

[Saravana Kannan]

On Wed, Sep 11, 2024 at 5:51 AM Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
>
> On 2024/9/11 00:27, Saravana Kannan wrote:
> > 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:
> >>>>
> >>> 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, ...
> >>
> .....
>
> >> 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.
> >
>
> sorry, not agree with you.
>
> IMO, it is easy to make amba_match() return bool type as shown below:
>
> make amba_match() always match with AMBA device with INvalid periphid
> and move reading id operation into amba_dma_configure().
>
> Above solution can have the same logical as current one but it looks ugly.
>
> so i make below optimizations to get this patch series:
>
> 1) only make AMBA device with INvalid periphid match with existing empty
>    amba_proxy_drv to reduce unnecessary reading id operation.

No it doesn't. Once match() returns -EPROBE_DEFER we don't try
matching with other drivers. So it doesn't cause more reads.

> 2) moving reading id operation to amba_probe() looks more graceful.

To do a driver/device match, you need to periphid. It doesn't make
sense to push that into some stub probe function instead of doing it
where it's needed. In the match function.

> Look at below 3 consecutive history commits:
>
> git log --pretty='%h (\"%s\")' 656b8035b0ee -3
> Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
> bus_type.match()")
> Commit: 17f29d36e473 ("ARM: 8523/1: sa1111: ensure no negative value
> gets returned on positive match")
> Commit: 82ec2ba2b181 ("ARM: 8522/1: drivers: nvdimm: ensure no negative
> value gets returned on positive match")

Those are commits from 2016! Way before any of the cleanup was done.

> the first AMBA related commit breaks that a bus_type's match() have bool
> type return value.

Have you actually looked at the definition of match and it's doc? It's
return type is int and not bool. And the doc says it should return
-EPROBE_DEFER.

> the remaining two commits at the same time really do not like negative
> return value for a bus_type's match().

This whole series is fixing a non-issue because you have a subjective
opinion that the reading of periphid should happen outside of the
match() function where it's actually needed.

And you even have a comment saying it's adding a race.

Russell,

Definite huge NACK from me. Please don't merge this series. I don't
see it fixing anything and it's moving around code for
pointless/questionable reasons. If it is fixing any real bug, I've yet
to hear it explained properly.

If I don't reply further, it means my NACK stands. If the replies
somehow convince me to remove my NACK, I'll do so.

-Saravana