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

[Zijun Hu]

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.


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
>